Find the longest sequence of increasing numbers in an array I have the following task: to find the largest sequence of increasing numbers in an array.

I'll give you an example : 2 3 4 1 50 2 3 4 5 will return the sequence of 2 3 4 5 - that is the largest sequence of increasing numbers in the given array.
Another one is : 5 -1 10 20 3 4 which will return -1 10 20.

If there are two equal sequences -for example 4 5 1 2 -1 which has the sequences  4 5 and 1 2 the first sequence will be returned.

Below is my method

public static List<Integer> findLongestIncreasingSequence(int[] numbersToBeProcessed) {

        List<Integer> longestIncreasingSequence = new ArrayList<Integer>();
        List<Integer> currentNumbersSequence = new ArrayList<Integer>();

        for (int i = 0; i < numbersToBeProcessed.length; i++) {

            int currentNumber = numbersToBeProcessed[i];
            int nextNumber = 0;

            // checks if it is not the last number in the array
            if (i != numbersToBeProcessed.length - 1) {
                nextNumber = numbersToBeProcessed[i + 1];
            }
            currentNumbersSequence.add(currentNumber);
            //checks if the current sequence ends here(the next number is smaller or equal)
            //or if the array ends
            if (currentNumber >= nextNumber || i == numbersToBeProcessed.length - 1) {
                // checks if the current sequence is bigger
                if (currentNumbersSequence.size() > longestIncreasingSequence.size()) {
                    longestIncreasingSequence.clear();
                    longestIncreasingSequence.addAll(currentNumbersSequence);
                }
                //clear the current sequence so it can start all over again
                currentNumbersSequence.clear();
            }
        }

        return longestIncreasingSequence;
    }

Any feedback received on this method is more than welcome. The thing I dislike the most is the check I make for the last element in order to avoid ArrayIndexOutOfBounds exception.

The thing I dislike the most is the check I make for the last element in order to avoid ArrayIndexOutOfBounds exception.

So let's avoid that check. Let's structure our loop such that we're back-comparing. That is, instead of comparing index i to index i+1, we compare index i to index i-1. That way, if we start at 1, we'll always have a valid comparison - since otherwise our loop invariant would've triggered:

for (int i = 1; i < vals.length; ++i) {
    if (vals[i] > vals[i-1]) {
        // continuing this sequence
    }
    else {
        // start a new sequence, of length 1, from i
    }
}

No further checks against vals.length are necessary, once we handle the empty case. The rest of the algorithm involves just keeping track of a current running sequence and the best sequence so far, which you could do with a local ArrayList<Integer> but better to just do with a couple ints.


Cube root (Newton's method) Newton's method for finding cube roots states that for any given \$x\$ and a guess \$y\$, a better approximation is \$\dfrac{(\dfrac{x}{y^2} + 2y)}{3}\$.

What do you think of this code for finding a cube root in Scheme?

(define (improveguess y x)
  ; y is a guess for cuberoot(x)
  (/ (+ (/ x (expt y 2)) (* 2 y)) 3))

(define (cube x) (* x x x))

(define (goodenough? guess x)
  (< (/ (abs (- (cube guess) x)) guess) 0.0001))

(define (cuberoot x) (cuberoot-iter 1.0 x))

(define (cuberoot-iter guess x)
  (if (goodenough? guess x) guess
      (cuberoot-iter (improveguess guess x) x)))

If you look at your code for this exercise as well as the one about approximating the square root and the one about finding epsi, you'll notice a common pattern:

You have a function which performs a single step and a predicate which tells you when you're done. You then apply the stepping function until the predicate is met. When you encounter a common pattern like this, the best thing to do is usually to abstract it. So let's define an apply-until function which takes a stepping function, a predicate and an initial value and applies the function to the value until the predicate is met:

(define (apply-until step done? x)
  (if (done? x) x
      (apply-until (step x) step done?)))

You can now define your cuberoot function using apply-until instead of cuberoot-iter:

(define (cuberoot x)
  (apply-until (lambda (y) (improve-guess y x)) (lambda (guess) (goodenough? guess)) 1.0))

You can also move your helper functions as local functions into the cuberoot function. This way they don't need to take x as an argument (as they will close over it) and can thus be passed directly to apply-until without using lambda:

(define (cuberoot x)
  (define (improveguess y)
    ; y is a guess for cuberoot(x)
    (/ (+ (/ x (expt y 2)) (* 2 y)) 3))

  (define (goodenough? guess)
    (< (/ (abs (- (cube guess) x)) guess) 0.0001))

  (apply-until improveguess goodenough? 1.0))


Image hosting site image downloader using requests and BeautifulSoup I went about this the way I have because Selenium is slow and inconvenient (opens browser) and I was unable to find the href attribute in the link element using BeautifulSoup. Html included below. On the downside, this appears to only ever find 25 images.

#! python3

# Saves photos to file from flickr.com using specified search term

import bs4
import logging
import os
import re
import requests
import shutil
import sys
import time

def find_link(element):
    """Finds link using a regular expression"""

    link_regex = re.compile(r"//c\d+.staticflickr.com/\d+/\d+/\w+\.jpg")
    # dictionary of element attributes
    element_attr_dict = element.attrs

    # get list of element attribute values wherein image link lies
    attr_list = [element_attr_dict[key] for key in element_attr_dict.keys()]
    attr_string = ""

    # link exists within a string list element
    for element in attr_list:
        if type(element) == str:
            attr_string += element
    match = link_regex.search(attr_string)
    if match:
        link = "https:" + match.group()
    else:
        link = None
    return link

def main():
    """Downloads specified type/number of 'flickr' images to folder

    Takes three command line arguments: "filename, search_term, number_
    images". Saves images to folder. Number of images saved based upon
    number requested by user or number found during search. Whichever is
    lower.

    Arguments:
    search_term   -- search image site using this term
    number_images -- maximum number of images to save to folder
    """

    try:
        search_term, number_images = sys.argv[1:]
        number_images = int(number_images)
    except ValueError:
        print("Something went wrong. Command line input must be of \
format: 'filename searchterm numberimages'")
        return

    links = []
    # make folder to store photos and name using search term
    html_path = r"C:\Users\Dave\Desktop\2016Coding\AutomateBoring\11" + \
                r"-WebScraping\flickrhtml.txt"
    path = \
    r"C:\Users\Dave\Desktop\2016Coding\AutomateBoring\11-WebScraping" + \
    r"\gimages\requests"
    folder_path = os.path.join(path, search_term)
    if os.path.exists(folder_path):
        shutil.rmtree(folder_path)
    os.makedirs(folder_path)

    print("Finding photos...")

    # get links to photos
    res = requests.get("https://www.flickr.com/search/?text=" + search_term)
    res.raise_for_status()
    soup = bs4.BeautifulSoup(res.text, "html.parser")
    found_elems = soup.select(".photo-list-photo-view")

    # incase number found images < requested
    number_save_images = min(number_images, len(found_elems))

    print("Found {} images".format(number_save_images))

    for found_elem in found_elems[:number_save_images]:
        link = find_link(found_elem)
        links.append(link)

    # write images to file
    print("Writing images to folder...")
    for image_link in links:
        basename = os.path.basename(image_link)
        save_file_name = os.path.join(folder_path, basename)
        res = requests.get(image_link)
        res.raise_for_status()

        with open(save_file_name, "wb") as f:
            for chunk in res.iter_content(100000):
                f.write(chunk)

    print("Images saved at: {}".format(folder_path))
    print("*****Done*****")

if __name__ == "__main__":
    main()

According to PEP8 standard library import should be first.

Function find_link(element):

The docstring of the function find_link should probably say something about the argument type.
Why is the argument element (and not element_attributes) when it looks like all you use is element.attrs?
The list comprehension could be written as: attr_list = list(element_attr_dict.values())
But, you don't need it at all. You can simply write for element in attr_dict.values().

To summarize, you can delete most lines in the beginning. Instead start with

def find_link(element_attributes):
    link_regex = re.compile(r"//c\d+.staticflickr.com/\d+/\d+/\w+\.jpg")
    attr_string = ""
    for element in element_attributes.values():
        ...

As for the main function:

Investigate argparse.
Use logging instead of print.
Split the main function to sub-functions. At least saving an image from a link should probably be one.
The for loop where you append a link to a list can be expressed as a list comprehension.

I hope that helps!


Python password generator based on the current month I am relatively new to python and am trying to teach myself.

I have created a python program that will take the current month's first three letters, and add one letter to the first, two to the second and three to the third.

It does this by converting the month's characters to an ASCII number, then adding X to that.

Please see the below code, which is what I wrote, and if you can, please provide me with feedback, how you feel it can be shortened etc.

import datetime

myDate = datetime.datetime.now()
month = myDate.strftime("%b")
month_list = list(month)
#below line is for testing other months or words. To use, please comment out the above three lines (myDate, month and month_list)
#month_list = ["Z", "z", "Z", ""]

letter_one = month_list[0]
letter_one_ascii = ord(letter_one)
letter_one_ascii += 1

if letter_one_ascii == 91 or letter_one_ascii == 123:
    letter_one_ascii = 65
else:
    letter_one = chr(letter_one_ascii)

letter_two = month_list[1]
letter_two_ascii = ord(letter_two)
letter_two_ascii += 2

if letter_two_ascii == 91 or letter_two_ascii == 123:
    letter_two_ascii = 65
elif letter_two_ascii == 92 or letter_two_ascii == 124:
    letter_two_ascii = 66
else:
    letter_two = chr(letter_two_ascii)

letter_three = month_list[2]
letter_three_ascii = ord(letter_three)
letter_three_ascii += 3

if letter_three_ascii == 91 or letter_three_ascii == 123:
    letter_three_ascii = 65
elif letter_three_ascii == 92 or letter_three_ascii == 124:
    letter_three_ascii = 66
elif letter_three_ascii == 93 or letter_three_ascii == 125:
    letter_three_ascii = 67
else:
    letter_three == chr(letter_three_ascii)

letter_one = chr(letter_one_ascii)
letter_two = chr(letter_two_ascii)
letter_three = chr(letter_three_ascii)

print "Your Password is: " + letter_one.upper(), letter_two.upper(), letter_three.upper()

Independent of the actual programming language you break a lot of rules and recommendations.

First - you shall not repeat yourself. The snippet

if letter_one_ascii == 91 or letter_one_ascii == 123:
    letter_one_ascii = 65

is repeated with slightly changed names

if letter_three_ascii == 91 or letter_three_ascii == 123:
    letter_three_ascii = 65

or slightly changed values

elif letter_three_ascii == 92 or letter_three_ascii == 124:
    letter_three_ascii = 66

Most probably you did not type that but you did copy-paste and edited names and/or values. This is very error prone as you could easily forget to change a name or value. Such errors are very hard to find. What would you do if you had to extend this generator to handle 5 characters? or 6?
Typical solution: write a function that shifts by n letters and another one shifting by one letter

def shift_by_one(letter):
    # some magic here
    return new_letter

def shift_by_n(letter, n):
    # some magic here using previously defined function 'shift_by_one'
    return new_letter

you are not repeating yourself any more and extending comes easy. finally you should implement another function shifting a word according to your rules

def shift_word(word):
    # some magic here using previously defined function 'shift_by_n'
    return new_word

Second - you shall name your magic numbers, especially if they are appearing more than once. but even if appearing only once, what does 124 stand for? or 66? This is hard to maintain for you and even harder for a different person.
Instead of 65 you could have something like

first_letter_ascii = ord('A')

Third - boundary checking. you can do that after incrementing like you did, but you can also do it before. The latter is preferred if outside values do not exist or do not make sense.
Instead of

letter_one_ascii = ord(letter_one)
letter_one_ascii += 1

if letter_one_ascii == 91 or letter_one_ascii == 123:
    letter_one_ascii = 65
else:
    letter_one = chr(letter_one_ascii)

you could easily write

if letter_one == 'Z' or letter_one == 'z':
    letter_one = 'A'
else:
    letter_one_ascii = ord(letter_one)
    letter_one_ascii += 1
    letter_one = chr(letter_one_ascii)

That is much more readable and by the way this would make the body for shift_by_one.

Other options for boundary checking are modulo operations

arr = 'ABCD'
print(arr[4%len(arr)])

and deliberatly extending arrays outside

arr = 'ABCDA'
print(arr[4])

Fourth - you handle a lot of special cases as you process uppercase and lowercase. however in your final output you are interested in uppercase only. convert month to uppercase immediately and eliminate all 90+ numbers respectively letter 'z'.

month = myDate.strftime("%b").upper()

this also enables you to use a modulo boundary check and we can eliminate the loop in shift_by_n

def shift_by_n(letter, n):
    index = ord(letter) - ord('A')
    index = (index + n) % 26
    letter = chr(index + ord('A'))
    return letter

Finally some python stuff:
you do not have to convert a string into a list to access characters, string provides slicing and iterability.

word = myDate.strftime("%b").upper()[:3]

if you end up in a list of characers and want to have a string you can join all characters by

''.join(mylist)

another nice feature in python is list comprehension which we use in shift_word

letters = [shift_by_n(letter,n+1) for n, letter in enumerate(word)]

we end up in

import datetime

def shift_by_n(letter, n):
    index = ord(letter) - ord('A')
    index = (index + n) % 26
    letter = chr(index + ord('A'))
    return letter

def shift_word(word):
    letters = [shift_by_n(letter,n+1) for n, letter in enumerate(word)]
    return ''.join(letters)

myDate = datetime.datetime.now()
word = myDate.strftime("%b").upper()[:3]
print "Your Password is: " + shift_word(word)


Python Calculator Test Driven Development I am a beginner in TDD and I just coded a Calculator in Python with TDD approach. I would be more than happy to hear your opinions out. And since I am a beginner in TDD, I would love to hear some tips.

Tests :

import Calculator
import unittest

class CalculatorTests(unittest.TestCase):

    def setUp(self):
        self.calculator = Calculator.Calculator()

    def test_add_method(self):
        result = self.calculator.add(4, 2)
        self.assertEqual(6, result)

    def test_add_method_invalid_value(self):
        self.assertRaises(ValueError, self.calculator.add, "four", "five")

    def test_multiply_method(self):
        result = self.calculator.multiply(5, 3)
        self.assertEqual(15, result)

    def test_multiply_method_invalid_value(self):
        self.assertRaises(ValueError, self.calculator.multiply, "four", "five")

    def test_sub_method(self):
        result = self.calculator.sub(6, 4)
        self.assertEqual(2, result)

    def test_sub_method_invalid_value(self):
        self.assertRaises(ValueError, self.calculator.sub, "four", "five")

    def test_div_method(self):
        result = self.calculator.div(5, 1)
        self.assertEqual(5, result)

    def test_div_method_invalid_value(self):
        self.assertRaises(ValueError, self.calculator.div, "five", "four")

    def test_div_method_zero(self):
        self.assertRaises(ZeroDivisionError, self.calculator.div, 5, 0)

if __name__ == '__main__':
    unittest.main()

Calculator.py :

number_types = (int, float, complex)

class Calculator:

    @staticmethod
    def validate_args(x, y):
        if not isinstance(x, number_types) and not isinstance(y, number_types):
            raise ValueError

    def add(self, x, y):
        self.validate_args(x, y)
        return x + y

    def multiply(self, x, y):
        self.validate_args(x, y)
        return x*y

    def sub(self, x, y):
        self.validate_args(x, y)
        return x-y

    def div(self, x, y):
        self.validate_args(x, y)
        return x/y

TDD
It's quite hard to review TDD as it's more of a way to do work than an observable product. What I mean is that whether you did TDD or not, the unit tests would mostly be the same.
One minor thing I'd point out is that the c variable name isn't good. One strong point of TDD/Unit Testing IMO is that it can act as a documentation of your code, so it should be explicit. Name it calculator; you won't be losing much space doing so.
Calculator class
Regarding the class itself, there are two things I'd like to review.
Nesting makes the code hard to read. In your case it's not so bad because methods are very short, but it's a good habit you should take.
Instead of :
if isinstance(x, self.number_types) and isinstance(y, self.number_types):
    return x + y
else:
    raise ValueError

I would do
if not isinstance(x, self.number_types) and not isinstance(y, self.number_types):
    raise ValueError

return x + y

This way if you had to add code in your method, you wouldn't need to hang in a else statement for the whole flow.
Second, it's fine that you throw a ValueError, but I think you should add a message otherwise we can't know why the calculator crashed.
Exceptions
With regards to the comments, I'd like to talk a little bit more about your exception handling. Should it be raised in the UI or in the class itself?
The boring answer would be "It depends". In my opinion, there are advantages to both cases.

If the exception is raised in UI, it is raised as soon as possible, which means you don't have to work with potentially invalid data in your application flow.

If the exception is raised in the class itself, you are 100% sure that wherever/however you call your calculator, the input will be validated.

So, what is better? I'd say it's better if the exception is raised in the class, as you'll be sure you won't ever have to repeat your validation if you reuse your calculator elsewhere. But, if I think about the MVC.Net's way of working, for example, it validates the user input on client and server side. This way, there is a quick validation that is made to make sure everything makes sense, then in the server validation (in your case, you class), the input is thoroughly validated then used.
For the sake of "having the best code possible", I'd say do the validation in both and see for yourself what is your gain :)


Scroll with multiple, and going different directions I have a large image of an L-shaped painting on one page. I created divs over parts of the painting I want to explain. When the page loads, I want it to scroll down the length of the image, and then move to the left of the image, stopping at each div for a few seconds. I understand how to get the page to scroll down on load, but I do not understand how to continue scrolling once it has reached the first div. Should I target the data-box and not the class, so it will continue to move through each div box?

Ok, I got my code to work.

I kept my HTML the same but made a few changes to my jQuery.

    $(document).ready(function(){
$('html, body').animate({
    scrollTop: $(".ganesh").offset().top,
}, 5000).animate({
    scrollLeft: $(".bird1").offset().left,
}, 5000).animate({
    scrollLeft: $(".saar").offset().left,
}, 5000).animate({
     scrollLeft: $(".anna").offset().left,
}, 5000).animate({
     scrollLeft: $(".friends1").offset().left,
}, 5000).animate({
     scrollLeft: $(".quote").offset().left,
}, 5000);
});


Best way to access lists and nest them with if statements Trying to improve my Python skills by creating a small text adventure. I have used lists to access certain information, but I am not sure if I am using them correctly. For instance, I have used a list to store weapons but wasn't sure of the syntax for accessing them in an if statement. Also, if there is any advice on how to improve the code in general that would be appreciated. I am a beginner so would prefer a simple alternative.
weapon = ["sword", "stone", "flower"]

# Create user defined functions for the game first.

def title():
    print("In front of you there is a portal. You have no idea where it leads")
    print("portal #1")

# Call function

title()
portal = input("Type 1: ")

if portal == "1":
    print("There's a wizard conjuring a spell.")
    print("What do you do?")
    print("1. Talk to the wizard.")
    print("2. Attack the wizard.")
    print("3. Run away.")

    choice_wizard = input("Type 1,2,3: ")

    if choice_wizard == "1":
        print("The wizard turns you into a frog. Ribbit!")

    elif choice_wizard == "2":
        print("You see a sword laying on the ground")
        print("Would you like to take the sword? Press y or n to continue")

        weapon_input = input("Type y or n: ")
        if weapon_input == "y" or weapon_input == "Y":
            weapon = []
            weapon.append("sword")
            print("You are now carrying a " + weapon[0] + ", You kill the wizard")
        elif weapon_input == "n" or weapon_input == "N":
            weapon = []
            weapon.append("stone")
            print("You are now carrying a " + weapon[0] + ", You hit the wizard in the head with it")
            print("He falls over unconscious and dies")
    elif choice_wizard == "3":
        print("The wizard tries turning you into a frog but misses.")
        print("...You escape.")

Here are just a few comments to help improve your code:

When you have a collection, think about how you are going to access the collection. Most of the time, you are going to treat it as a group of items (example: "weapons") and so you should use a plural name. Rarely, you will treat a collection as a single item (example: "database"), and so you should use a singular name.

Always use functions. Yes, python supports having statements directly "up against the wall." But it is much, much easier to write test cases, to debug your code, and to change your mind about what order to do things if you have a single name (function) for a group of statements. Moving things around as blobs of text is miserable and prone to errors.

When you find yourself doing something twice, write a function! In your case, asking the user for some input is a good place to put a function:
 show_message("There's a wizard conjuring a spell.")
 choice = get_choice("What do you do?",
       "Talk to the wizard",
       "Attack the wizard",
       "Run away")

As far as your collection of weapons is concerned, think about what kind of game you are writing. If you are writing a "dungeon" style game, where there is going to be a lot of fighting monsters, then having real weapon-objects with stats will be important. In that case, you'll probably want either a set or a list, but you'll want a separate variable to track which of the possibly many weapons is currently being wielded.
On the other hand, if you are writing more of a "text adventure" game, where just having "the sword" is enough to beat a monster, then you might not want a collection at all. Instead, you might want one or more boolean variables, or perhaps a dictionary full of booleans.

      if user_has['the sword']:
          show_message("The troll runs away!")
      else:
          show_message("The troll eats you. You have died.")

To specifically answer your question about how to access members of lists: note that you are using the list.append() function. This takes an object and adds it to the end of the list. So if you have 11 items in the list, and you .append another item, it will be the 12th item (index 11, because list indexes are 0-based).
You can access that item in two ways. The "hard way" is to compute the length of the list and subtract one:
 item = "sword"
 weapons.append(item)
 sword_index = len(weapons) - 1
 print("You have a", weapons[sword_index])

An easier way is to reply on the fact that python supports negative indexes as being from the end of the list. That is, list[-1] is the last item, list[-2] is the next-to-last item, etc. This works with indexes and with slices, so you can also say list[1:-3] or list[-5:] for example. See note 3 in the sequence docs for details.


A program to tell you if there are 2 numbers in an array that add to a given sum there is a video on life at google about what an interview would be like and so i watched it and decided to go along and come up with a solution to the problem myself and i found it really easy this is the solution i came up with

the question is to make a program to find 2 numbers in an array that add up to a sum return true if two such numbers exists return false if they do not

var gotPair = function(arr, sum) {
  this.data = arr;
  this.sum = sum
  this.check = [];
  this.num = null;
  this.out = false;
  for (var i = 0; i < this.data.length; i++) {
    this.num = this.sum - this.data[i];
    if (check.includes(this.num)) {
      this.out = true;
    }
    this.check.push(data[i]);
  }
  return this.out;
}

did i do a good job?
i made sure it always worked so tell me if its good.
i currently want to work for google and i am 14 so anyone who can help me get there i would be very grateful

There are a few problems with the function right now.

gotPair leaks variables to the global scope. When you say this.data, this refers to the global scope so you create a global property called data. See How does the "this" keyword work? on SO.
Instead of storing an out variable, you can just return true when you find a match. This both reduces the complexity of the function and improves the speed. Imagine you have an array of 10 billion elements and you find the match in the first 2 elements. There's no need to go through the rest of the array.
Avoid creating variables you don't need. this.data is just arr, num isn't necessary as the subtraction can be performed in the call to includes, this.sum is the same as sum, and check is not needed as it is just a copy of arr from index 0 to i.

Here are a few alternative implementations.

Similar to your logic, but looping backwards so that we can take advantage of the fromIndex argument of the includes function. I've included type checks in this function to show how they might be done, but some people prefer to leave them out in Javascript and let the function throw "naturally".

function hasSumPair(arr, sum) {
    if (!Array.isArray(arr)) throw new Error('The first argument must be an array')
    if (Number.isNaN(sum)) throw new Error('The second argument must be a number')

    for (let i = arr.length - 2; i >= 0; i--) {
        if (arr.includes(sum - arr[i], i)) {
            return true
        }
    }
    return false
}

Explicitly looping through the array twice. This makes what the function is doing very clear but results in code that is somewhat uglier.

function hasSumPair(arr, sum) {
    for (let i = 0; i < arr.length; i++) {
        for (let j = i + 1; j < arr.length - 1; j++) {
            if (arr[i] + arr[j] === sum) {
                return true
            }
        }
    }
    return false
}

Here is a one liner that will have some problems with very large arrays since it will copy the array for each run.

function hasSumPair(arr, sum) {
    return arr.some((num1, index) => arr.slice(index + 1).some(num2 => num1 + num2 === sum))
}

This implementation is not the fastest as it has to sort the array first. If you can assume that the array is sorted, this would be the fastest solution (which is often very important at interviews for companies like Google)

function hasSumPair(arr, sum) {
    // .sort() mutates the array, so copy it using .slice() first.
    let sorted = arr.slice().sort()
    for (let i = 0; i < sorted.length - 1; i++) {
        for (let j = i + 1; j < sorted.length && sorted[j] + sorted[i] <= sum; j++) {
            if (sorted[j] + sorted[i] === sum) {
                return true
            }
        }
    }
    return false
}

The same function as above, but making use of some (Note that on the last iteration, arr[i + 1] will be undefined which will result in a NaN result and will work as expected). This is likely the implementation I would use if the array is sorted, and likely even if the array isn't sorted if the speed is not an issue.

function hasSumPair(arr, sum) {
    return arr.slice().sort()
        .some((a, i, sorted) => sorted.slice(i).some(b => a + b == sum))
}

Which of these functions is best is largely a matter of opinion and the case in which it will be used. Most of the time I recommend choosing readability over speed. Speed should only be considered if the time it takes to run the function is creating problems.


Linked List class based implementation in Python I have the following class based linked list created in Python. I need some help in the best way to actually call and test this (print the results and output to the screen).

For an accepted answer, could someone please
1. use my existing code, to test/output results showing the linked list implementation
2. Make any comments on my existing code and suggestions as to how it could be improved (if any)

I am not sure how to "call" the testing method to see the results, or whether it is better to take that out and test outside the class. If the latter is preferred, could someone please demonstrate this, as I am unclear/unsure of the creation of nodes and next pointer objects in order to test the class.

Code:

class Node:
        def __init__(self,contents=None, next=None):
                #Here we are defining the two attributes - contents of the list and the next pointer
                self.contents = contents
                self.next = next

        def getContents(self):
                return self.contents

        def __str__(self):
                return str(self.contents)

        def print_list(node):
                while node:
                        print(node.getContents())
                        node=node.next
                print()

        def testing():
                #Add content to each node
                node1=Node("A")
                node2=Node("C")
                node3=Node("D")
                node4=Node("B")
                #Define the pointers for each node (e.g. follow through A to B to C to D)
                node1.next=node4
                node4.next=node2
                node2.next=node3
                print(print_list(node1))

I am also unclear on the implementation, as when creating a queue you would have something like the below to add an element. Would this not be needed here for a linked list?

Method for adding an item to a queue

def add(self,item): #method to add something
        self.items.append(item) #to add something, use append

This would be added to the queue (and tested) like so:

    myQ.add("A")
    myQ.add("B")
    myQ.add("C")

I think some guidance on what exactly is happening (comments) would be helpful too.

First, there are some awkwardnesses in your code:

You should not use next as a variable because, it is a built-in function.
The getContents function is useless, because contents is a public attribute.
Your print_list method is ill-formed, you need to use self.

You can simplify like this:

class Node:
    def __init__(self,contents=None, next_node=None):
        self.contents = contents
        self.next_node = next_node

    def __str__(self):
        return str(self.contents)

    def print_list(self):
        node = self
        while node is not None:
            print(node.contents)
            node = node.next_node
        print()

To test your code, you can use the Unit Testing Framework, for instance:

import unittest

class TestNode(unittest.TestCase):

    def test_construcor(self):
        node1 = Node("A")
        node2 = Node("C")
        node3 = Node("D")
        node4 = Node("B")
        # Define the pointers for each node (e.g. follow through A to B to C to D)
        node1.next_node = node4
        node4.next_node = node2
        node2.next_node = node3
        node1.print_list()
        self.assertEqual(node1.next_node.contents, 'B')

if __name__ == '__main__':
    unittest.main()


Parsing configuration files I've written a function to parse configuration files. It doesn't handle file opening, but instead takes an array with the file contents and tries to find fields based on an array containing the fields' names. If found, it puts the content into its respective place.

The only rules it has so far are:

each field is delimited by a newline '\n'
case doesn't matter for the field's name
after a field's name has been found, leading spaces and tabs are irrelevant and discarded
lines containing unrecognized fields are ignored
the character separating the field's name and its respective content must be specified in the field's name

Example file:

FIELD 1: CONTENT 1
FIELD 2: CONTENT 2
FIELD 3: CONTENT 3
FIELD 4: CONTENT 4
FIELD 5: CONTENT 5

Here's an example call:

char *configuration = load_file(file_name);
char *field1;
char *field2;
char *field3;
char *field4;
char *field5;

static const char *fields[] = {
        "FIELD 1:",
        "FIELD 2:",
        "FIELD 3:",
        "FIELD 4:",
        "FIELD 5:",
        0
    };
    char **destinations[] = {
        &field1,
        &field2,
        &field3,
        &field4,
        &field5,
        0
    };

    //Parse and load fields
    parse(destinations, fields, configuration);

And the function:

//strings allocated must be freed by the caller
static void parse(char ***destinations, const char **fields, const char *source)
{
    char buffer[BUFFER_SIZE];
    int i, j, buffer_position;

    while(*source != '\0')
    {
        for(i = 0; fields[i] != 0; ++i)
        {
            //check if first character match, case insensitive
            if(*fields[i] == *source || inverse_case(*fields[i]) == *source)
            {
                //check if subsequent characters match
                for(j = 0; fields[i][j] != '\0'; ++j)
                {
                    if(fields[i][j] != source[j] && inverse_case(fields[i][j]) != source[j]) break;
                }

                //it's not the right field, look for another
                if(fields[i][j] != '\0') continue;

                //found, update pointer position
                source += j;

                //skip leading spaces and tabs
                while(*source == ' ' || *source == '\t') ++source;

                //collect content
                for(buffer_position = 0; *source != '\n' && buffer_position < BUFFER_SIZE - 1; ++buffer_position, ++source)
                {
                    buffer[buffer_position] = *source;
                }
                buffer[buffer_position] = '\0';

                //if it's an empty line, nothing to copy
                if(buffer_position > 0)
                {
                    //allocate memory for destination and copy
                    if((*destinations[i] = malloc(buffer_position + 1)) != 0)
                    {
                        COPY(*destinations[i], buffer);
                    }

                    //if there's an error on malloc(), we just skip the field and notify the user
                    else
                    {
                        printf("Error: malloc(%d)\n", buffer_position + 1);
                    }
                }

                //since we found a field that matches, no need to keep looking
                //break the for loop, skip the while and process next line
                goto next_iteration;
            }
        }
        //no matches for the current line just skip it entirely
        while(*++source && *source != '\n');

        next_iteration:
        ++source;
    }
}

My questions:

Would it be considered better if the line delimiter was defined by the caller?
By adding the options to not ignore spaces and tabs, and set case sensitivity on/off, would the parameter list be considered too big?

I'll only address C programming issues in this answer, not the algorithm itself.

Program design and programming practice

As a rule of thumb, there is never a reason to use more than two levels of indirection in C programming. More levels than that always originates from flawed program design. In your case, you are allocating an array of pointers to char. So it should be char* destinations[].
Because of the above reason, the function should be changed to

static void parse (char **destinations, const char **fields, const char *source)

or if you would prefer:

static void parse (char**       destinations,
                   const char** fields,
                   const char*  source)

It is bad practice to allocate memory in a different module than the one doing the allocation. If parse() does memory allocation, then a function located in the same module should handle the cleanup, not the caller. I would advise to create a function called parse_cleanup() or similar, which does this.
The function parse is performing a parsing algorithm. The function shouldn't concern itself with anything else but that. Dynamic memory allocation is completely unrelated to the task "parsing". Consider splitting the code in two sub-functions: one which handles the algorithm and one for the memory allocation.
Your loop control is made needlessly complex. You control the iteration from no less than 5 places: outer while loop, first for loop, continue statement, goto statement. This is spaghetti code and needs to be rewritten from scratch.

You don't need nested loops at all. Throughout the iteration, you will have to constantly check for \0, as any string can end anywhere. You can merge the while and for into a single while. That will also mean that you can remove the goto.
Instead of having one big super loop that does a lot of things, consider writing a simple loop that calls upon various functions. Try to avoid break if possible, and always avoid continue and goto, as the latter two are completely superfluous language features that many programmers consider hard to read.

Consider to add some error handling to the function. Parsers in particular usually have many things that can go wrong. Then it is nice for the caller to have detailed error information.
Investigate if you can't use the standard functions isblank(), isspace() and toupper() from ctype.h, rather than your own custom ones.

Coding style
(these remarks are more subjective and not everyone will agree)

Always write statements on a line of their own and always use braces. This makes the code easier to read and prevent bugs during code maintenance. For example:

while(*source == ' ' || *source == '\t')
{
  ++source;
}

The above is especially important in the case of while(*++source && *source != '\n');, because there you can't tell whether the programmer intentionally wrote an empty loop, or if they accidentally wrote a ; at the end of the line, which is a very common, classic bug. It is far better to write:

while(x)
{} // this means: "yes, I did intend this loop to be empty"

Never use the ++ operator in the same expression as other operators, it can easily lead to undefined behavior bugs such as the classic i = i + i++; bug. But it is also harder to read. Mixing ++ with other operators is pointless anyhow, since it doesn't affect performance or the final machine code in any way. One example of how to rewrite it:

do
{
  source++;
} while (*source != '\0' && *source != '\n');

(And of course the check for \0 should be explicit like in this example - that is good practice and you do it everywhere else in your code.)
Avoid assignment inside conditions. Just as with the ++ operator, it can lead to bugs, and there is never a reason to have assignment inside conditions, it is another 100% superfluous feature of the language.

Most compilers will warn against assignment inside conditions, so it is annoying to have it there. You should not disable such warnings, because they will find the bug x=y where you intended to have x==y.

Furthermore, in the case of if((*destinations[i] = malloc(buffer_position + 1)) != 0), did you actually intend to set the contents to NULL if allocation fails? It may or may not be better to leave the pointer alone in that case. Your function call documentation must mention which applies.
It is good practice to compare against NULL rather than 0 when dealing with pointers, to show the reader that you did intend to compare a pointer and not an integer value.
If you heed my advice to never use ++ together with other operators, then the need for either prefix ++ or postfix ++ is gone: they turn completely equivalent. Ask yourself if you are using prefix ++ for a particular reason. Because people who do think they have a reason, tend to either name something based on superstition or something about how compilers were designed in the 80s.


Read a palindrome of unknown length I have a task to write the function:
  int read_palindrome(); // input comes from stdin

which will read one line from standard input and returns 1 if the line is a palindrome and 0 otherwise. A line is terminated by the newline character (’\n’) and the does not include the newline.

There are requirements to be met:

There is no assumption about the length of the input.
You are also not allowed to read the input twice, e.g. read the input, forget you read the input but remember the length, read the input again. which results in the input being read twice.

You are also not allowed to create a very large buffer to store the input reasoning that the input line might be expected to be smaller than a very large buffer.
The reason for this restriction is that we will consider the memory usage of the program.

The task is to come out with a correct program with the best CPU time and memory usage.

I have attempted my code below. May I know if there is anything I could improve on here to optimise for correctness, CPU and memory?

#include <stdlib.h>
#include <string.h>
#include <stdio.h>

int read_palindrome();
int check_palindrome2(char *, int);

int main()
{
    if (read_palindrome()) printf("input is a palindrome");
    else printf("input is not a palindrome");
    return 0;
}

int read_palindrome() {
    unsigned int len_max = 128;
    unsigned int current_size = 0;
    char *pStr = malloc(len_max);
    current_size = len_max;
    int i;
    char c;
    if (pStr != NULL) {
        while (( c = getchar() ) != '\n') {
            pStr[i] = (char)c;
            i++;
            if(i == current_size) {
                current_size = i+len_max;
                pStr = realloc(pStr, current_size);
            }
        }

        pStr[i] = '\0';
        free(pStr);
    }
    return check_palindrome2(pStr,i);
}

int check_palindrome2(char *s, int length) {
    for (int i = 0; i < length; i++) {
        if (s[i]!= s[length-i-1]) {
            return 0;
        }
    }
    return 1;
}

free(pStr) is misplaced. Once it is freed, you may not touch it anymore. Calling check_palindrome2(pStr) after freeing causes an undefined behavior.

Consider

    int rc = check_palindrome2(pStr, i);
    free(pStr);
    return rc;

check_palindrome2 could be called with a null pointer, but doesn't test it.
current_size += len_max is more clear than current_size = i+len_max.
read_palindrome is a misnomer. It doesn't read a palindrome. It reads a string of an unknown length. Now, SRP mandates splitting it into independent read_line and is_palindrome functions.
You test malloc for a failure. Keep in mind that realloc may fail as well.

    char * tmp = realloc(....);
    if (tmp == NULL) {
        free(pStr);
        return error;
    }
    pStr = tmp;

NB: a simple pStr = realloc(....) is not good. If realloc fails, you'd have a memory leak.


CS50 Vigenere program I wrote a program for a problem in Harvard's CS50 course, Vigenere (week 2).

Here is a description of what the program needs to do:

  Design and implement a program that encrypts messages using Vigenère’s cipher.

  Implement your program in a file called vigenere.c in a directory
  called vigenere.
  Your program must accept a single command-line argument: a keyword, k, composed entirely of alphabetical characters.
  If your program is executed without any command-line arguments, with more than one command-line argument, or with one command-line argument that contains any non-alphabetical character, your program should print an error (of your choice) and exit immediately, with main returning 1 (thereby signifying an error).
  Otherwise, your program must proceed to prompt the user for a string of plaintext, p, (as by a prompt for plaintext:) which it must then encrypt according to Vigenère’s cipher with k, ultimately printing the result (prepended with ciphertext: and ending with a newline) and exiting, with main returning 0.
  With respect to the characters in k, you must treat A and a as 0, B and b as 1, …, and Z and z as 25.
  Your program must only apply Vigenère’s cipher to a character in p if that character is a letter. All other characters (numbers, symbols, spaces, punctuation marks, etc.) must be outputted unchanged. Moreover, if your code is about to apply the jth character of k to the ith character of p, but the latter proves to be a non-alphabetical character, you must wait to apply that jth character of k to the next alphabetical character in p; you must not yet advance to the next character in k.
  Your program must preserve the case of each letter in p.

I received full credit for my answer, but I still want to clean up the code if possible. I am new to programming, so I enjoy seeing what changes more experienced programmers would make.

#include <cs50.h>
#include <stdio.h>
#include <string.h>
#include <ctype.h>

int check(string argv);
int shift(char c);

int main(int argc, string argv[])
{
    //check for valid key and save
    int valid = 0;
    if (argc == 2)
    {
        valid = check(argv[1]);
    }
    else
    {
        printf("Usage: ./vigenere keyword\n");
        return 1;
    }

    //begin encryption
    int key;
    int index = -1;
    int new_code;
    if (valid == 1)
    {
        //get plaintext to encrypt
        string plaintext = get_string("plaintext: ");
        printf("ciphertext: ");

        //iterate through each char of plaintext
        for (int j = 0, l = strlen(plaintext); j < l; j++)
        {
            if (isalpha(plaintext[j]))
            {
                //iterate through the keyword only when there is an alpha in the plaintext
                index++;
                key = shift(tolower(argv[1][index % strlen(argv[1])]));
                new_code = (int) plaintext[j] + key;
                if ((int) tolower(plaintext[j]) + key > 122)
                {
                    new_code = new_code - 26;
                    printf("%c", (char) new_code);
                }
                else
                {
                    printf("%c", (char) new_code);
                }
            }
            else
            {
                printf("%c", (char) plaintext[j]);
            }
        }
        printf("\n");
    }
    else
    {
        printf("Usage: ./vigenere keyword\n");
        return 1;
    }
}

//create key
int shift(char c)
{
    int key = (int) c - 97;
    return key;
}

//decide if key is valid
int check (string argv)
{
    int val = 0;
    for (int i = 0, n = strlen(argv); i < n; i++)
    {
        if (isalpha(argv[i]))
        {
            val = 1;
        }
        else
        {
            val = 0;
            break;
        }
    }
    return val;
}

It's usually best to return as early as possible for invalid cases. This avoids unnecessary nesting and complexity.

e.g. the main() function has a lot of redundant code:

int main(int argc, string argv[])
{
    //check for valid key and save
    int valid = 0;
    if (argc == 2)
    {
        valid = check(argv[1]);
    }
    else
    {
        printf("Usage: ./vigenere keyword\n");
        return 1;
    }

    ...
    if (valid == 1)
    {
        ...
    }
    else
    {
        printf("Usage: ./vigenere keyword\n");
        return 1;
    }
}

and could be shortened to:

int main(int argc, string argv[])
{
    if (argc != 2 || check(argv[1]) != 0)
    {
        printf("Usage: ./vigenere keyword\n");
        return 1;
    }

    ...
}

The main loop of the program could be abstracted to a separate function (after checking for a valid key / printing usage on error).

Similarly check() could be written as:

int check (string argv)
{
    for (int i = 0, n = strlen(argv); i < n; i++)
    {
        if (!isalpha(argv[i]))
        {
            return 1;
        }
    }

    return 0;
}

check() is not an informative name for a function. It should at least be check_key, or is_all_alpha or something. shift() is a similarly cryptic name.

By using strlen, we are unnecessarily iterating the string twice (once to find the null at the end, then again to process it). We can simply run until we find a null character:

for (int j = 0; plaintext[j]; ++j)

Don't use magic numbers. What are 122, 26, or 97?

Variables should be declared as close to their point of usage as possible, and initialized with real values. (key, index, new_code).

It would be better to use another variable (e.g. keyword) as an alias, rather than using argv[1] throughout the code.

There is no need to strlen() the keyword for every character.

The main loop always ends up printing a char. The rest of the code just decides what the char will be. So perhaps it could be abstracted into a function. Also, since we are expected to treat both 'a' and 'A' the same, we can simplify things by only outputting letters in lowercase.

I'd be inclined to do something like this (not tested):

string key = argv[1];
int key_len = strlen(key);
int key_i = 0;
for (int j = 0; plaintext[j]; ++j)
{
    char p = plaintext[j];
    char c = !isalpha(p) ? p : encode_char(p, key[key_i++ % key_len]);
    printf("%c", c);
}

printf("\n");
...

char encode_char(char plaintext, char key)
{
    assert(isalpha(plaintext)); // #include <assert.h>

    int k = shift(tolower(key));
    int p = shift(tolower(plaintext));

    return unshift((p + k) % 26);
}


FIFO queue for audio data I have created a data structure to handle some audio data I am generating (simple sine wave right now). It had some errors in it due to some misunderstanding of my ancient C knowledge that has been fixed by my cousin. I am wondering if there may yet still be more issues.

I have considered turning this into a C++ class, but because I don't have enough experience with C++ I would likely turn it into a huge memory leak waiting to happen. This is really being used with Objective C on the iPad - but nothing prevents this from being used elsewhere.

All of the functions are publicly accessible, and should be fairly obvious how they should be used. But essentially you alloc the structure, push and pop (which make it sound like a stack but it very much is not), and finally release the structure. You will notice it is in a C Object Oriented Design - I did that intentionally to make things easier to remember.

I did not use a circular buffer because I hadn't reached the ballmer peak yet when I was writing this and I couldn't quite get the right form. The queue basically works on the principle that you append on the end, and take data off the top - and the stuff left is moved up to the top.

I was looking to use some vector acceleration functions from this at some point. It presently is being used in threaded code behind locks with NSCondition.

To see how this is being used now, the structure is used in this .m file and is part of this project.

// from the header file - just converted to a typedef, untested...
typedef struct {
    float *buffer;
    float *end;
    size_t max_length;
} FIFO;

#include "FIFO.h"

FIFO *FIFO_alloc(size_t length) {
    FIFO *restrict ptr = NULL;

    ptr = (FIFO*)calloc(1, sizeof(FIFO));
    if (ptr == NULL) {
        return NULL;
    }

    ptr->buffer = (float*)calloc(length, sizeof(float));
    if (ptr->buffer == NULL) {
        free(ptr);
        return NULL;
    }

    ptr->end = ptr->buffer;
    ptr->max_length = length;

    return ptr;
}

void FIFO_release(FIFO *fifo) {
    if (fifo) {
        free(fifo->buffer);
        free(fifo);
    }
}

bool FIFO_push(FIFO *restrict fifo, const float *restrict data, size_t count) {
    if (!fifo || !data || count == 0) return false;

    // test for the length available
    if (fifo->max_length - (int)(fifo->end - fifo->buffer) < count) {
        return false;
    }

    memcpy(fifo->end, data, count*sizeof(float));
    fifo->end += count;

    return true;
}

bool FIFO_pop(FIFO *restrict fifo, float *restrict data, size_t count) {
    if (!fifo || !data) return false;
    if (count == 0 || FIFO_size(fifo) < count) return false;

    // move count items from the buffer
    memcpy(data, fifo->buffer, count*sizeof(float));
    // move empty space back to the beginning
    memmove(fifo->buffer, fifo->buffer + count, (fifo->end - fifo->buffer - count) * sizeof(float));

    // reposition end pointer
    fifo->end -= count;

    return true;
}

size_t FIFO_size(const FIFO *fifo) {
    if (fifo) {
        return (size_t)(fifo->end - fifo->buffer);
    }

    return 0;
}

size_t FIFO_maxsize(const FIFO *fifo) {
    if (fifo) {
        return fifo->max_length;
    }

    return 0;
}

Here's a few suggestions in addition to what Keith mentions.

I would try to avoid the memory copy and move operations. You mention a circular buffer, which is one way of doing it, another would be to have the fifo hold discrete buffers and pop each buffer from the front in it's entirety. Avoiding the copy and move operations should speed up the code significantly, which I assume is of concern when processing audio data :)

Also, I would drop the typedef, just use struct fifo instead. This makes it easier to convert this into a full ADT (Abstract Data Type) later, and just forward declare the struct to users while keeping the actual struct private.

As a final note, are you absolutely sure that the restrict keyword is the proper thing to use here?

Edit:
Here's one naive way of getting rid of the memove operation:

typedef struct {
    float *buffer;
    float *start;
    float *end;
    size_t max_length;
} FIFO;

bool FIFO_pop(FIFO *restrict fifo, float *restrict data, size_t count) {
    if (!fifo || !data) return false;
    if (count == 0 || FIFO_available(fifo) < count) return false;

    // move count items from the buffer
    memcpy(data, fifo->start, count*sizeof(float));
    fifo->start += count;

    return true;
}

This will only work if you know the max sized buffer that you need, but it should give you an idea.


Filter leading zeroes from numbers using only character operations for input and output Problem:  to eliminate initial zeroes from numbers besides those followed by '.' or alone.  For example if we input a-000123+bc+0000.0008-0000+0001.07 , we will get a-123+bc+0.0008-0+1.07).

Using putchar and getchar only. No strings and pointers involved. I have working code, but I think it's confusing and overloaded. Can someone suggest how it might be simplified or an easier algorithm?

char curr_char = 0, prev_char = 0,flag=0;
while ((curr_char = getchar()) != '&')
{

    if (isdigit(curr_char) && !(isdigit(prev_char)))
        flag = 1;
    if (flag)
    {
        while (true)
        {
            if (curr_char=='0'); else break;
            curr_char = getchar();
        }

        if (curr_char == '.')putchar('0');

        while (true)
        {
            if (isdigit(curr_char))putchar(curr_char); else break;
            curr_char = getchar();
        }

        if (curr_char == '.')
        {
            putchar('.');
            while (isdigit(curr_char = getchar()))putchar(curr_char);
        }
        flag = 0;
    }
    else putchar(curr_char);
    prev_char = curr_char;
}

I see a number of things that could be improved here.

Include the appropriate headers

The code as posted is just a fragment, but only a few additional lines would make it into a complete program.  It's usually best to provide a complete program for review.  With that said, the missing headers are these:

#include <stdio.h>
#include <stdlib.h>
#include <ctype.h>
#include <stdbool.h>

Check for EOF

The code currently looks for the & character, but that's not in the specification.  It would probably make more sense to look for the end of file which is EOF.

Restructure your code

Any time you find yourself writing while (true) and then putting a conditional break within the loop, this should be a strong indication that something is not quite right. Also, lines like this:

if (curr_char=='0'); else break;

are really quite awful.  That could easily have been written instead as:

if (curr_char != '0')
    break;

which makes it much easier to understand.  Even better would have been to write the loop as:

while (cur_char != '0')
    curr_char = getchar();

Eliminate unneeded variables

The code defines curr_char, prev_char and flag but it's possible to rework the algorithm to eliminate at least one of these.  Also, curr_char and prev_char are good names, but flag is not.  It's entirely too generic and doesn't say what it's intended to flag.

Fix your algorithm

If we feed a03+0.0g to this code, it should print a3+0.0g but what it actually prints is a30.0 which is clearly not correct.  Part of the problem here is that the implementation of the algorithm is quite convoluted and difficult to follow.  For that reason, I would recommend a different approach.

Start with a state machine

Starting with a state machine is a very good way to work out what a program should do in advance of writing any code.  Having a design first and then writing code is essential to good programming practice.  For example, this state machine can be easily implemented in code.  I used an enum to represent the state and then a switch statement to drive the code.  In it, each transition is labelled as condition/action.

As an example of how to code this, here's the first part of the code including the code for the nonNum state:

int main()
{
    enum { nonNum, inInt, inDec, zeroSupp } state = nonNum;
    int ch;
    while ((ch = getchar()) != EOF) {
        switch (state) {
            case nonNum:
                if (ch == '0') {
                    state = zeroSupp;
                } else if (ch > '0' && ch <= '9') {
                    state = inInt;
                    putchar(ch);
                } else {
                    putchar(ch);
                }
                break;
      /* the rest of the state machine goes here */


Fetching data from nested JSON using jQuery and displaying in table I'm displaying JSON data in a tabular format on a html page using jQuery.

In the function I'm looping the resp.DATA once to get the key names so it can be displayed as the headings in the table, and a second time to get the values of the keys for all customers.

Can this be done in a way that I only have to loop the resp.DATA once?

JSON data:

    var resp= {
    "ERROR": [],
    "DATA": [{
        "CustomerID": "234",
        "BranchID": "1",
        "LocationID": "26",
        "FirstName": "Lakshmi "
    }, {
        "CustomerID": "235",
        "BranchID": "1",
        "LocationID": "6",
        "FirstName": "Arora "
    }, {
        "CustomerID": "236",
        "BranchID": "1",
        "LocationID": "68",
        "FirstName": "S.K.Raman "
    }, {
        "CustomerID": "237",
        "BranchID": "1",
        "LocationID": "38",
        "FirstName": "Vidya Rao "
    }, {
        "CustomerID": "238",
        "BranchID": "1",
        "LocationID": "18",
        "FirstName": "Raju "
    }, {
        "CustomerID": "239",
        "BranchID": "1",
        "LocationID": "49",
        "FirstName": "K.B.Hebbar "
    }],
    "META": {
        "currentPageNumber": "1",
        "totalPages": "11",
        "rowcountCurrentPage": "10",
        "rowcountTotal": "107"
    }
}

Code:

            var content = '
';
            var heading = "
";
            var data = "";
            $.each(resp.DATA[0], function(ke, va) {
                heading += '' + ke + '	';
            });
            $.each(resp.DATA, function(key, value) {

                data += "
";
                $.each(value, function(k, v) {
                   data += '' + v + '	';
                });
                data += "
";
            });
            heading += "
";
           content += heading + data + "
";
            $('#data').append(content);

HTML file:

<!DOCTYPE html>

    <title>COnsuming rest webservice</title>
    <link rel="stylesheet" href="http://maxcdn.bootstrapcdn.com/bootstrap/3.3.6/css/bootstrap.min.css">

Consuming REST api

    </script>

You can see a working example here.

You may take advantage of reduce() and map() functions to have no loop at all, like this:

$('#data').append(
  resp.DATA.reduce((table, obj, index) => (
    table + (
      !index ? (
        Object.keys(obj).reduce((header, key) => `${header}<th>${key}`, '
')
      ) : ''
    ) + $.map(obj, v => v).reduce((row, value) => `${row}<td>${value}`, '
')
  ), '
') + '
'
);

I also used some tagged strings, so more reducing the size of the code.
Additionally it works a bit faster than the original version, as you can observe with the following snippet.

var resp= {
  "ERROR": [],
  "DATA": [{
    "CustomerID": "234",
    "BranchID": "1",
    "LocationID": "26",
    "FirstName": "Lakshmi "
  }, {
    "CustomerID": "235",
    "BranchID": "1",
    "LocationID": "6",
    "FirstName": "Arora "
  }, {
    "CustomerID": "236",
    "BranchID": "1",
    "LocationID": "68",
    "FirstName": "S.K.Raman "
  }, {
    "CustomerID": "237",
    "BranchID": "1",
    "LocationID": "38",
    "FirstName": "Vidya Rao "
  }, {
    "CustomerID": "238",
    "BranchID": "1",
    "LocationID": "18",
    "FirstName": "Raju "
  }, {
    "CustomerID": "239",
    "BranchID": "1",
    "LocationID": "49",
    "FirstName": "K.B.Hebbar "
  }],
  "META": {
    "currentPageNumber": "1",
    "totalPages": "11",
    "rowcountCurrentPage": "10",
    "rowcountTotal": "107"
  }
}

var begTime = performance.now();var content = '
';
var heading = "
";
var data = "";
$.each(resp.DATA[0], function(ke, va) {
  heading += '' + ke + '	';
});
$.each(resp.DATA, function(key, value) {
  data += "
";
  $.each(value, function(k, v) {
    data += '' + v + '	';
  });
  data += "
";
});
heading += "
";
content += heading + data + "
";
$('#data').append(content);
$('#data').append(`<h3>OLD: ${performance.now() - begTime} ms</h3>`);

var begTime = performance.now();
$('#data').append(
  resp.DATA.reduce((table, obj, index) => (
    table + (
      !index ? (
        Object.keys(obj).reduce((header, key) => `${header}<th>${key}`, '
')
      ) : ''
    ) + $.map(obj, v => v).reduce((row, value) => `${row}<td>${value}`, '
')
  ), '
') + '
'
);

$('#data').append(`<h3>NEW: ${performance.now() - begTime} ms</h3>`);
<!DOCTYPE html>

    <title>COnsuming rest webservice</title>
    <link rel="stylesheet" href="http://maxcdn.bootstrapcdn.com/bootstrap/3.3.6/css/bootstrap.min.css">

Consuming REST api

    </script>

NOTE: in your version as well as mine, we never take care of the keys in nested objects: they work only if they appear always in the same order!


Simplify javascript redirecting functions logic I have javascript file which is ugly but I have not idea how to make it better.
There are many function for redirecting with different number of parameters.
Just few functions are used for other thinks. All this functions will be called from Jquery event.
Like this:

$("#btnSomeButtonID").click(function () { SelectPTO(); });

I would like to simplify logic.
Do you have any ideas for refactoring?
Thank you for your help.

// Require refactoring, all is  public. Need to investigate what is best way.

var pushupdatesList; //public
var pushupdatesListRun = true;//public
var loading = false;//public

// functions with 0 parameters
function StartLoading() {
    if (loading)
        return;

    loading = true;
    $("#loadingbar").show();
}

function StopLoading() {
    loading = false;
    $("#loadingbar").hide();
}

// redirect functions with 0 parameters
function OpenAddCompany() {
    location.href = "/Administration/AddCompany";
}

function OpenAdminTools() {
    StartLoading();
    location.href = "/Administration/GetCompanies";
}

function Logout() {
    StartLoading();
    AbortPushUpdates();
    loading = 0;
    location.href = "/Home/Logoff";
}

function SelectPTO() {
    StartLoading();
    AbortPushUpdates();
    loading = 0;
    location.href = "/Home/SelectEquipmentCollection";
}

function OpenChangePassword() {
    StartLoading();
    loading = 0;
    location.href = "/Home/ChangePassword";
}

// functions with 1 parameter
function OpenGetPTOs(ID) {
    StartLoading();
    loading = 0;
    location.href = "/Administration/GetEquipmentCollections/" + ID;
}

function OpenAddPTO(ID) {
    StartLoading();
    loading = 0;
    location.href = "/Administration/AddEquipmentCollection/" + ID;
}

function OpenGetAccountsForCompany(ID) {
    StartLoading();
    loading = 0;
    location.href = "/Administration/GetAccountsForCompany/" + ID;
}

function OpenAddAccount(ID) {
    StartLoading();
    loading = 0;
    location.href = "/Administration/AddAccount/" + ID;
}

// functions with 2 parameters
function OpenGetAccountsForPTO(ID, CompanyID) {
    StartLoading();
    loading = 0;
    location.href = "/Administration/GetAccountsForEquipmentCollection/" + ID + "?CompanyID=" + CompanyID;
}

function OpenAddAccountToPTO(ID, CompanyID) {
    StartLoading();
    loading = 0;
    location.href = "/Administration/AddAccountToEquipmentCollection/" + ID + "?CompanyID=" + CompanyID;
}

function OpenGetVehiclesForPTO(ID, CompanyID) {
    StartLoading();
    loading = 0;
    location.href = "/Administration/GetEquipmentForCollection/" + ID + "?CompanyID=" + CompanyID;
}

function OpenAddVehicleToPTO(ID, CompanyID) {
    StartLoading();
    loading = 0;
    location.href = "/Administration/AddEquipmentToCollection/" + ID + "?CompanyID=" + CompanyID;
}

The first thing I see which will cut the file at least in half is making a single redirectToPage function that redirects to a given page. You have many copies of the same basic function which currently looks like this

function functionName(params) {
    StartLoading();
    loading = 0;
    location.href = "someURL"
}

When you see a similar block of code like this being repeated multiple times, it's a good time to refactor, (it's debatable, but I would consider refactoring at 2 repetitions, and definitely refactor at 3). Your new function will be

function redirectToPage(url) {
    StartLoading();
    loading = false; // explained below
    location.href = url;
}

You can now erase most of that file, using this one more generic function and passing the urls as arguments.

You are also using loading as a boolean sometimes, and sometimes as an integer. Your code will be easier to reason about if you stick to one. It looks like you're only using loading as a boolean, so I changed loading = 0 to loading = false. You can make this change throughout the rest of the file as well.

This may be enough changes for you. If not, there are a few other functions that you have that redirect to a page but with slightly different functionality, such as calling AbortPushUpdates or not setting the loading variable. You can extend your new redirectToPage function to handle these cases like so

/**
 * Call this function giving the url to redirect to, and other options
 */
function redirectToPage(url, options) {
   if (!options || !options.hasOwnProperty('startLoading') || options.startLoading == true) {
     // default is to run StartLoading()
     StartLoading();
   }

   if (options && options.hasOwnProperty('abortPushUpdates') && options.abortPushUpdates) {
     // default is to NOT call AbortPushUpdates
     AbortPushUpdates();
   }

   if (!options || !options.hasOwnProperty('setLoading') || options.setLoading == true) {
     // default is to set the loading variable
     loading = false;
   }

   // Always redirect
   location.href = url;
 }

You can now use this function in any of the following ways

redirectToPage('/Administration/AddAccount/' + accountID);
redirectToPage('/Administration/AddCompany', {setLoading: false, startLoading: false})
redirectToPage('/Home/SelectEquipmentCollection', {abortPushUpdates: true})

Good luck with your refactor, and let me know if you have any follow up questions!


Function to convert ISO-8859-1 to UTF-8 I wrote this function last year to convert between the two encodings and just found it. It takes a text buffer and its size, then converts to UTF-8 if there's enough space.

What should be changed to improve quality?

int iso88951_to_utf8(unsigned char *content, size_t max_size)
{
    unsigned char *copy;
    size_t conversion_count; //number of chars to convert / bytes to add

    copy = content;
    conversion_count = 0;

    //first run to see if there's enough space for the new bytes
    while(*content)
    {
        if(*content >= 0x80)
        {
            ++conversion_count;
        }
        ++content;
    }
    if(content - copy + conversion_count >= max_size)
    {
        return ERROR;
    }

    while(content >= copy && conversion_count)
    {
        //repositioning current characters to make room for new bytes
        if(*content < 0x80)
        {
            *(content + conversion_count) = *content;
        }
        else
        {
            *(content + conversion_count)   = 0x80 | (*content & 0x3f); //last byte
            *(content + --conversion_count) = 0xc0 |  *content >> 6;    //first byte
        }
        --content;
    }
    return SUCCESS;

}

The character set is named ISO-8859-1, not ISO-8895-1.  Rename your function accordingly.

Change the return value to be more informative:

Return 0 on success.
If max_size is too small, return the minimum value of max_size that would be sufficient to accommodate the output (including the trailing \0).

I would also change the parameter to take a signed char * to be a bit more natural.

I think that the implementation could look tidier if you dealt with pointers instead of offsets.

It would be nice if you NUL-terminated the result, so that the caller does not have to zero out the entire buffer before calling this function.

size_t iso8859_1_to_utf8(char *content, size_t max_size)
{
    char *src, *dst;

    //first run to see if there's enough space for the new bytes
    for (src = dst = content; *src; src++, dst++)
    {
        if (*src & 0x80)
        {
            // If the high bit is set in the ISO-8859-1 representation, then
            // the UTF-8 representation requires two bytes (one more than usual).
            ++dst;
        }
    }

    if (dst - content + 1 > max_size)
    {
        // Inform caller of the space required
        return dst - content + 1;
    }

    *(dst + 1) = '\0';
    while (dst > src)
    {
        if (*src & 0x80)
        {
            *dst-- = 0x80 | (*src & 0x3f);                     // trailing byte
            *dst-- = 0xc0 | (*((unsigned char *)src--) >> 6);  // leading byte
        }
        else
        {
            *dst-- = *src--;
        }
    }
    return 0;  // SUCCESS
}


Swift HackerRank Missing Numbers Challenge I'm attempting to solve the "Missing Numbers" challenge on HackerRank where I'm tasked with finding missing elements comparing two arrays.

Foundation's NSCountedSet is a little pokey on large data sets, I rolled my counted set with a Swift Dictionary. I'm able to pass all the test cases with the exception of the last case which has ~10,000 elements in each array.

I looked through some of the comments for hints on how to complete the challenge, but they're all in other languages with uncommented code and non-descriptive variable naming.

// I put this in to save non-HackerRank users the hassle of getting inputs
let firstArrayCount = Int(readLine()!)! // unused
let listA = readLine()!.components(separatedBy: " ").map({Int($0)!})
let secondArrayCount = Int(readLine()!)! // unused
let listB = readLine()!.components(separatedBy: " ").map({Int($0)!})

// This is my method to create a countedSet
func countedSet(array: [Int]) -> Dictionary<Int, Int> {
    var dict = Dictionary<Int, Int>()

    for element in array {
        if dict[element] == nil {
            dict[element] = 1
        } else {
            dict[element] = dict[element]! + 1
        }
    }

    return dict
}

// Works fine on everything except large data sets
func missingNumbers(listA: [Int], listB: [Int]) -> String {
    var output: [Int] = []

    let comparisonSet = Set(listB) // list be is larger

    let ctdSetListA = countedSet(array: listA)
    let ctdSetListB = countedSet(array: listB)

    for element in comparisonSet {
        if ctdSetListA[element] != ctdSetListB[element] {
            output.append(element)
        }
    }

    return output.sorted().map({String($0)}).joined(separator: " ")
}

I welcome suggestions on how to speed up my code. Thank you for reading.

The "create or update" in the countedSet function
can be simplified using the nil-coalescing operator ??:

func countedSet(array: [Int]) -> Dictionary<Int, Int> {
    var dict = Dictionary<Int, Int>()
    for element in array {
        dict[element] = (dict[element] ?? 0) + 1
    }
    return dict
}

This also saves one dictionary lookup if the value is already present in the dictionary.

I would (but that may be a matter of personal preference) separate the string
conversion completely from the difference computation. Return an integer
array from missingNumbers:

func missingNumbers(listA: [Int], listB: [Int]) -> [Int]

and do the string conversion at the call site:

let missing = missingNumbers(listA: listA, listB: listB)
print(missing.map({String($0)}).joined(separator: " "))

or print the numbers directly, without string conversion:

let missing = missingNumbers(listA: listA, listB: listB)
for num in missing {
    print(num, terminator: " ")
}
print()

The comparisonSet in the missingNumbers function is not needed. Since B is
the "larger" set, it suffices to iterate over the counted set generated from B:

func missingNumbers(listA: [Int], listB: [Int]) -> [Int] {
    var output: [Int] = []

    let ctdSetListA = countedSet(array: listA)
    let ctdSetListB = countedSet(array: listB)

    for (element, count) in ctdSetListB {
        if ctdSetListA[element] != count {
            output.append(element)
        }
    }

    return output.sorted()
}

The next improvement is to use only one counted set: Add the elements from B
and subtract the elements from A. Then check which elements have a positive count
in the end:

func missingNumbers(listA: [Int], listB: [Int]) -> [Int] {

    var ctdSet = countedSet(array: listB)
    for element in listA {
        ctdSet[element]! -= 1 // We know that elements in A are present in B
    }

    let output = ctdSet.filter { $0.value > 0 }.map { $0.key }
    return output.sorted()
}

Generally, a "filter + map" operation can be optimized with flatMap:

    let output = ctdSet.flatMap { $0.value > 0 ? $0.key : nil }

The essential clue however is (I assume) the given constraint

  The difference between maximum and minimum number in B is less than or equal to 100.

which means that we can use a fixed sized array to keep track of the number of
occurrences of each number, and all
dictionary lookups now become "simple" array subscript operations.
This also makes sorting the missing numbers redundant:

func missingNumbers(listA: [Int], listB: [Int]) -> [Int] {

    guard let minValue = listB.min() else { return [] } // Empty input

    var counts = Array(repeating: 0, count: 101)
    for elem in listB {
        counts[elem - minValue] += 1
    }
    for elem in listA {
        counts[elem - minValue] -= 1
    }

    var output: [Int] = []
    for i in counts.indices {
        if counts[i] > 0 {
            output.append(i + minValue)
        }
    }
    return output
}

The final for loop creating the output array also can be written concisely in a functional
way as

    let output = counts.enumerated().flatMap { $0.element > 0 ? $0.offset + minValue : nil }

Another possible performance bottleneck is when the input data is read:

let listA = readLine()!.components(separatedBy: " ").map({Int($0)!})

The input lines can contain up to 1,000,010 integers, which means
that many temporary string are created. An alternative is to use
Scanner from the Foundation framework to read integers directly
from the string. And since the number of integers is known, we
can call reserveCapacity() to avoid reallocations while the
array grows:

func splitToIntegers(_ s: String, count: Int) -> [Int] {
    var result: [Int] = []
    result.reserveCapacity(count)
    var n = 0
    let scanner = Scanner(string: s)
    while scanner.scanInt(&n) {
        result.append(n)
    }
    return result
}

let firstArrayCount = Int(readLine()!)!
let listA = splitToIntegers(readLine()!, count: firstArrayCount)
let secondArrayCount = Int(readLine()!)!
let listB = splitToIntegers(readLine()!, count: secondArrayCount)

This turned out to be much faster in my test.


Is it acceptable to use a function property as its own private storage? I need to write a function that accepts a function as argument, if this function is new, adds it to an array otherwise do not add it.

I assigned a property a my function StoreFunctions and use it as the private storage space, is this an acceptable way to do it? I tested it with function c and d, I did get the correct results back.

function StoreFunctions(func) {
  if (StoreFunctions.funcArray.includes(func))
    return 'function already added';
  else {
    StoreFunctions.funcArray.push(func);
    return 'a new function is added';}
}

StoreFunctions.funcArray = [];

function c() {return 1};
function d() {return 2};
console.log(StoreFunctions(c));
console.log(StoreFunctions(d));
console.log(StoreFunctions(c));

It works, but it's weird, and the variable intended for use only inside the function isn't private, which is potentially confusing and potentially unsafe.

Functions, being objects, can have arbitrary key-value pairs assigned to them, so you can put properties on it like you're doing. It's not unheard of. Usually, this sort of thing is reserved for static properties and methods which are associated with the class/function, but don't make sense as instance methods, and are meant for use by outside consumers. For a trivial example, the below function exposes a canEat property:

function Person() {
}
Person.canEat = ['Apple', 'Banana', 'Carrot'];
Person.prototype.eat = function(food) {
  if (!Person.includes(food)) {
    throw new Error('Invalid food');
  }
  // do stuff depending on what argument is
}

Maybe it'll change over time, so users of Person can check Person.canEat before calling eat. But this is all intended for users of Person.

Another example of a useful static property on a function intended for outside use is Number.MAX_SAFE_INTEGER.

But if a property isn't meant to be used outside of the function, putting it on the function itself both sabotages the variable's privacy and is confusing (because a reader may well expect that a property accessible anywhere may be intended for use anywhere, which isn't what you mean).

If you want the function to be able to store data persistently and for itself only, you can use a closure instead. Here's one option:

const storeFunctions = (() => {
  const funcArray = [];
  return (func) => {
    if (funcArray.includes(func))
      return 'function already added';
    else {
      funcArray.push(func);
      return 'a new function is added';
    }
  }
})();

function c() { return 1 };
function d() { return 2 };
console.log(storeFunctions(c));
console.log(storeFunctions(d));
console.log(storeFunctions(c));

Professional Javascript often uses modules to organize code, in which case the IIFE is not needed - instead, have the module that defines the storeFunctions declare an array private to that module, and export the function:

const funcArray = [];
export const storeFunctions = (func) => {
  if (funcArray.includes(func))
    return 'function already added';
  else {
    funcArray.push(func);
    return 'a new function is added';
  }
};

import { storeFunctions } from './storeFunctions';
function c() {return 1};
function d() {return 2};
console.log(StoreFunctions(c));
console.log(StoreFunctions(d));
console.log(StoreFunctions(c));

The above is what I'd prefer. For any non-trivial script, modules make organizing code much easier.

Another note on your code - unless you need a collection with rearrangeable indicies, rather than using an array and iterating over it with .includes, it would probably be more appropriate to use a Set, which is a generic collection of values and whose lookup time with .has is an order of complexity faster than an array's .includes.

const storeFunctions = (() => {
  const funcSet = new Set();
  return (func) => {
    if (funcSet.has(func))
      return 'function already added';
    else {
      funcSet.add(func);
      return 'a new function is added';
    }
  }
})();

function c() { return 1 };
function d() { return 2 };
console.log(storeFunctions(c));
console.log(storeFunctions(d));
console.log(storeFunctions(c));


Filtering file for command line tool I've written a little command-line utility for filtering log files. It's like grep, except instead of operating on lines it operates on log4j-style messages, which may span multiple lines, with the first line always including the logging level (TRACE, DEBUG etc.).

Example usage on a file short.log with contents like this:

16:16:12 DEBUG - Something happened, here's a couple lines of info:
 debug line
 another debug line
16:16:14 - I'm being very verbose 'cause you've put me on TRACE
  trace info
16:16:15 TRACE - single line trace
16:16:16 DEBUG - single line debug

logrep -f short.log DEBUG produces:

16:16:12 DEBUG - Something happened, here's a couple lines of info:
 debug line
 another debug line
16:16:16 DEBUG - single line debug

I think the main loop of the program could probably be simplified with some sort of parse and filter.

  file = fileinput.input(options.file)
  try:
    line = file.next()
    while True:
      if any(s in line for s in loglevels):
        if filter in line:
          sys.stdout.write(line)
          line = file.next()
          while not any(s in line for s in loglevels):
            sys.stdout.write(line)
            line = file.next()
          continue
      line = file.next()
  except StopIteration:
    return

I see two problems with your loops.

In terms of style, calling file.next() and catching StopIteration is highly unconventional.  The normal way to iterate is:

for line in fileinput.input(options.file):
    …

In terms of functionality, I would personally consider the grepper to be buggy because it will fail to find a log message where the filter keyword that you are seeking appears on a continuation line.

To solve both problems, I would decompose the problem into two parts: reconstructing the logical messages (somewhat ugly) and searching (relatively straightforward).

import fileinput
import re

def log_messages(lines):
    """
    Given an iterator of log lines, generate pairs of
    (level, message), where message is a logical log message.
    possibly multi-line.
    """
    log_level_re = re.compile(r'\b(TRACE|DEBUG|WARN|ERROR|CRITICAL)\b')
    message = None
    for line in lines:
        match = log_level_re.search(line)
        if match:                               # First line
            if message is not None:
                yield level, message
            level, message = match.group(), line
        elif message is not None:               # Continuation line
            message += line
    if message is not None:                     # End of file
        yield level, message

for level, message in log_messages(fileinput.input(options.file)):
    if filter in message:
        sys.stdout.write(message)

Note that I've used a regular expression to look for TRACE, DEBUG, etc.  The \b anchors ensure that we don't mistake words like "INTRACELLULAR" for a TRACE message.


Mapping words to letters which contains Given sentence for example: ala ma kota, kot koduje w Javie kota I should to every letter occurring in the sentence I should map every string that this letter contains. So the output should be:

{a=[ala, javie, kota, ma], d=[koduje], e=[javie, koduje], i=[javie], j=[javie, koduje], k=[koduje, kot, kota], l=[ala], m=[ma], o=[koduje, kot, kota], t=[kot, kota], u=[koduje], v=[javie], w=[w]}

I have written algorithm for it, but I think that there is a better way do to this using streams (or somehow else).
Code;

String sentence = "ala ma kota, kot koduje w Javie kota .kota";

        String purifiedLowerCaseSentence = getPurifiedLowerCaseSentence(sentence);
        Set<Character> characters = getEveryCharacterOccurringInSentence(purifiedLowerCaseSentence);
        String[] splittedSentence = purifiedLowerCaseSentence.split("\\s");
        Map<Character, Set<String>> characterWithAccordingWord = new LinkedHashMap<>();

        for (char iteratedCharacter : characters) {
            for (String iteratedWord : splittedSentence) {
                if (iteratedWord.indexOf(iteratedCharacter) >= 0) {
                    characterWithAccordingWord.computeIfAbsent(iteratedCharacter, k -> new TreeSet<>());
                    characterWithAccordingWord.get(iteratedCharacter).add(iteratedWord);
                }
            }
        }

And these two methods which are placed in other class:

static String getPurifiedLowerCaseSentence(String sentence) {
        return sentence.replaceAll("\\p{P}", "").toLowerCase();
    }

    static Set<Character> getEveryCharacterOccurringInSentence(String sentence) {
        return sentence.chars()
                .mapToObj(e -> (char) e).collect(Collectors.toSet());
    }

Not sure about its efficiency or whether or not it improves your algorithm, but since you want to use streams, this would be an option:

public static void main(String[] args) {
        String sentence = "ala ma kota, kot koduje w Javie kota .kota";
        String sanitizedSentence = sentence.replaceAll("[^A-Za-z ]", "").toLowerCase();
        String[] words = sanitizedSentence.split(" ");
        Map<String,List<String>> result = sanitizedSentence
            .chars()
            .filter(Character::isAlphabetic)
            .distinct()
            .sorted()
            .mapToObj(c->(char)c)
            .map(Object::toString)
            .collect(Collectors.toMap(
                Function.identity(),
                c->Stream.of(words).distinct().filter(word -> word.contains(c)).sorted().collect(Collectors.toList()),
                (a,b)->a,
                LinkedHashMap::new // <- to ensure they are shown in order when printing
            ));
        System.out.println(result);
    }


Printing a diamond-shaped figure How can I optimize this code to have fewer loops and return values for unit testing?

 public class Diamond {

public void DiamondShape(int num) {

   for(int ucount=num;ucount>0;ucount--) {
//Loop to print blank space
    for(int count_sp=1;count_sp<ucount;count_sp++)
        System.out.printf(" ");
//Loop to print *
    for(int count_ast=num;count_ast>=ucount;count_ast--)
        System.out.printf("* ");
    System.out.println();
   }

//Loop for lower half
  for(int lcount=1;lcount<num;lcount++) {
    //Loop to print blank space
    for(int count_sp=0;count_sp<lcount;count_sp++)
            System.out.printf(" ");
    //Loop to print *
        for(int count_ast=num-1;count_ast>=lcount;count_ast--)
            System.out.printf("* ");
    System.out.println();
    }
  }
}

Output when num = 3:

   *
  * *
 * * *
  * *
   *

This is how the output should be. num indicates the star in center line.

First of all: Use a StringBuilder instead of System.out.println so you can easily compare the result of your method with an expected output.

Your test could look like: (I renamed you method to draw and moved the size to the instance.)

@Test
public void test4()
{
    StringBuilder expected = new StringBuilder();
    expected.append( "   * \n" );
    expected.append( "  * * \n" );
    expected.append( " * * * \n" );
    expected.append( "* * * * \n" );
    expected.append( " * * * \n" );
    expected.append( "  * * \n" );
    expected.append( "   * \n" );
    assertEquals( expected.toString(), new Diamond( 4 ).draw() );
}

After you have all your tests (I have just created some for 0-4) you can try to extract some methods with duplicate code.

public class Diamond
{
    private int size;

    public Diamond( int size )
    {
        this.size = size;
    }

    public String draw()
    {
        StringBuilder result = new StringBuilder();
        for( int ucount = size; ucount > 0; ucount-- )
        {
            appendSpaces( result, ucount - 1 );
            appendStars( result, size - ucount + 1 );
            newLine( result );
        }

        for( int lcount = 1; lcount < size; lcount++ )
        {
            appendSpaces( result, lcount );
            appendStars( result, size - lcount );
            newLine( result );
        }
        return result.toString();
    }

    private void newLine( StringBuilder result ) // just for better readability
    {
        result.append( "\n" );
    }

    private void appendStars( StringBuilder result, int count ) // just for better readability
    {
        repeat( result, "* ", count );
    }

    private void appendSpaces( StringBuilder result, int count ) // just for better readability
    {
        repeat( result, " ", count );
    }

    private void repeat( StringBuilder result, String string, int count )
    {
        for( int c = 0; c < count; c++ )
            result.append( string );
    }
}

If you want to get rid of one of the loops you could even merge it to:

public String draw()
{
    StringBuilder result = new StringBuilder();
    for( int ucount = size; ucount >= -size; ucount-- )
    {
        boolean isMiddleRows = ucount == 0 || ucount == -1;
        if( isMiddleRows ) continue;
        appendSpaces( result, Math.abs( ucount ) - 1 );
        appendStars( result, size - Math.abs( ucount ) + 1 );
        newLine( result );
    }
    return result.toString();
}

It's pretty cool to have enough tests do try such refactoring :)


Rolling Average class sanity check I just want to make sure that my Rolling (Moving) Average class is understandable and reasonably thread safe.
My main questions are:

Is having totalCounts as a long just for a potential edge case overkill?

Is checking to see if the number of observed counts is 1 (and thus, just return the observed total), also edge case overkill?

Is there any way to minimize the locking in Add Method? (It's not a bottle neck or anything, but I'm just wondering if I might be locking that part more than needed)

--
public sealed class RollingAverage
{
    public RollingAverage()
        : this(defaultMaxRememberedNumbers)
    {
        //
    }

    public RollingAverage(int maxRememberedNumbers)
    {
        if (maxRememberedNumbers <= 0)
        {
            throw new ArgumentException("maxRememberedNumbersmust be greater than 0.", "maxRememberedNumbers");
        }

        this.counts = new Queue<int>(maxRememberedNumbers);
        this.maxSize = maxRememberedNumbers;
        this.currentTotal = 0L;
        this.padLock = new Object();
    }

    private const int defaultMaxRememberedNumbers = 10;

    private readonly Queue<int> counts;
    private readonly int maxSize;

    private long currentTotal;

    private object padLock;

    public void Add(int value)
    {
        lock (this.padLock)
        {
            if (this.counts.Count == this.maxSize)
            {
                this.currentTotal -= (long)this.counts.Dequeue();
            }

            this.counts.Enqueue(value);

            this.currentTotal += (long)this.value;
        }
    }

    public int CurrentAverage
    {
        get
        {
            long lenCounts;
            long observedTotal;

            lock (this.padLock)
            {
                lenCounts = (long)this.counts.Count;
                observedTotal = this.currentTotal;
            }

            if (lenCounts == 0)
            {
                throw new InvalidOperationException("No counts to average.");
            }
            else if (lenCounts == 1)
            {
                return (int)observedTotal;
            }
            else
            {
                return (int)(observedTotal / lenCounts);
            }
        }
    }

    public void Clear()
    {
        lock (this.padLock)
        {
            this.currentTotal = 0L;
            this.counts.Clear();
        }
    }
}

-Edit-
Fixed inconsistent use of "this.".

To answer your questions:

It's hard to say.  If you expect a large number of values or moderate number of large values, you could potentially overflow an int.  Otherwise, you may be fine using int instead of long.
Yes, it's overkill, since dividing the total by the count would yield the correct answer anyways.
Unfortunately, no.  You need to lock anything which can be mutated by other threads, and you're doing precisely that.

I would do a few things:

Use a different naming format for fields than you do for local variables.  The most common format I have seen with .NET code is an underscore prefix, though the naming guidelines do not have any particular rule for private fields.  Once that is done, you can drop all the this references.
Constants should use different casing from fields.  SCREAMING CASE is the one I follow and see most often, but the framework isn't really consistent.  There appears to be a mix of PascalCase and SCREAMING_CASE, depending on which class you look at in ILDASM, even just within mscorlib.dll.
Avoid unnecessary casting.  .NET performs implicit casting in many cases, so there is no need to use most of the casts.  The only exception is casting the calculated average to int (from long).
Use proper exceptions.  The exception in the constructor should instead be ArgumentOutOfRangeException, as it more clearly states what the problem was.
Returning from within a lock works, so there is no need to define local variables for the total in the CurrentAverage property

public sealed class RollingAverage
{
    public RollingAverage()
        : this(DEFAULT_MAX_CAPACITY)
    {
    }

    public RollingAverage(int maxCapacity)
    {
        if (maxCapacity <= 0)
        {
            throw new ArgumentOutOfRangeException("maxRememberedNumbersmust be greater than 0.",
                                                    "maxRememberedNumbers");
        }

        _counts = new Queue<int>(maxCapacity);
        _maxSize = maxCapacity;
        _currentTotal = 0L;
        _padLock = new Object();
    }

    private const int DEFAULT_MAX_CAPACITY = 10;

    private readonly Queue<int> _counts;
    private readonly int _maxSize;

    private long _currentTotal;
    private object _padLock;

    public void Add(int value)
    {
        lock (_padLock)
        {
            if (_counts.Count == _maxSize)
            {
                _currentTotal -= _counts.Dequeue();
            }

            _counts.Enqueue(value);
            _currentTotal += value;
        }
    }

    public int CurrentAverage
    {
        get
        {
            lock (_padLock)
            {
                var lenCounts = _counts.Count;
                if (lenCounts == 0)
                {
                    throw new InvalidOperationException("No counts to average.");
                }

                return (int)(_currentTotal / lenCounts);
            }
        }
    }

    public void Clear()
    {
        lock (_padLock)
        {
            _currentTotal = 0L;
            _counts.Clear();
        }
    }
}

Other things to consider

Consider using a float or double type for the CurrentAverage property.  It is better to preserve the correct decimal answer in case you need a more exact average.  You can always truncate the answer back down to an int or float in the caller if you really want.
If the ratio of reads/writes heavily favors reads, consider using a ReaderWriterLockSlim.  This will allow multiple readers to retrieve the average without blocking one another.  Just remember to differentiate between obtaining the lock in reader mode versus writer or upgrade mode.