Finding a Cartesian product of multiple lists I came up with an extension method to find a Cartesian product of multiple IEnumerable sets. I was able to achieve lazy enumeration via yield return, but I didn't think of a way to do it non-recursively. The result ended up being a recursive lazy enumeration iterator method, the first of its kind! At least as far as I've ever written.

The idea of the problem came from a Stack Overflow question where a guy had many sets of characters, and wanted to generate a combination of all of them. Now I'm just interested because it's a fun problem!

I'd appreciate any kind of review, although here's two specific things I'm most interested in:

Can this algorithm can be de-recursed? If so - should it be, and how?
Is there some way it could be generalized even more, beyond what I've done?

public static class MultiCartesianExtension
{
    public static IEnumerable<TInput[]> MultiCartesian<TInput>(this IEnumerable<IEnumerable<TInput>> input)
    {
        return input.MultiCartesian(x => x);
    }

    public static IEnumerable<TOutput> MultiCartesian<TInput, TOutput>(this IEnumerable<IEnumerable<TInput>> input, Func<TInput[], TOutput> selector)
    {
        // Materializing here to avoid multiple enumerations.
        var inputList = input.ToList();
        var buffer = new TInput[inputList.Count];
        var results = MultiCartesianInner(inputList, buffer, 0);
        var transformed = results.Select(selector);
        return transformed;
    }

    private static IEnumerable<TInput[]> MultiCartesianInner<TInput>(IList<IEnumerable<TInput>> input, TInput[] buffer, int depth)
    {
        foreach (var current in input[depth])
        {
            buffer[depth] = current;
            if (depth == buffer.Length - 1)
            {
                // This is to ensure usage safety - the original buffer
                // needs to remain unmodified to ensure a correct sequence.
                var bufferCopy = (TInput[])buffer.Clone();
                yield return bufferCopy;
            }
            else
            {
                // Funky recursion here
                foreach (var a in MultiCartesianInner(input, buffer, depth + 1))
                {
                    yield return a;
                }
            }
        }
    }
}

Usage:

var input = new string[]
{
    "AB",
    "123",
    "@#",
};

foreach (var result in input.MultiCartesian(x => new string(x)))
{
    Console.WriteLine(result);
}

// Results:
// A1@
// A1#
// A2@
// A2#
// A3@
// A3#
// B1@
// B1#
// B2@
// B2#
// B3@
// B3#

You can indeed create a cartesian product without recursion. What you need is to use the enumerator.

I used a non-generic enumerator to work with any types.

public static IEnumerable Cartesian(this IEnumerable<IEnumerable> items)
{
    var slots = items
       // initialize enumerators
       .Select(x => x.GetEnumerator())
       // get only those that could start in case there is an empty collection
       .Where(x => x.MoveNext())
       .ToArray();

    while (true)
    {
        // yield current values
        yield return slots.Select(x => x.Current);

        // increase enumerators
        foreach (var slot in slots)
        {
            // reset the slot if it couldn't move next
            if (!slot.MoveNext())
            {
                // stop when the last enumerator resets
                if (slot == slots.Last()) { yield break; }
                slot.Reset();
                slot.MoveNext();
                // move to the next enumerator if this reseted
                continue;
            }
            // we could increase the current enumerator without reset so stop here
            break;
        }
    }
}

Example:

var letters = new string[] { "A", "B" };
var numbers = new[] { 1, 2, 3 };
var symbols = new[] { "@", "#" };
var empty = new string[] { };

var collections = new IEnumerable[] { letters, numbers, symbols, empty };
collections.Cartesian()
    // this is just for show
   .Select(x => string.Join("", x.Cast<object>()))
   .OrderBy(x => x)
   .Dump(); // linqpad

result:

A1#
A1@
A2#
A2@
A3#
A3@
B1#
B1@
B2#
B2@
B3#
B3@


Simple generic double buffer pattern I wrote a small generic implementation of a simple generic double buffer pattern, and I was wondering if it's actually thread safe or can be improved in any way.

Note: The specific part that I'm worrying about thread safety is the Swap() function.

/// <summary>
///     Simple generic double buffer implementation class
/// </summary>
public class DoubleBuffer<T> where T : class
{
    private T _current;

    public DoubleBuffer(T current, T next)
    {
        _current = current;
        Next = next;
    }

    /// <summary>
    ///     Next buffer waiting in line (no active usage should be here)
    /// </summary>
    public T Next { get; private set; }

    /// <summary>
    ///     Currently active buffer (active usage should be here)
    /// </summary>
    public T Current
    {
        get { return _current; }
    }

    /// <summary>
    ///     Swaps between the buffers
    /// </summary>
    /// <returns>Returns the buffer previously used before the swap</returns>
    public T Swap()
    {
        var swappedBuffer = _current;
        Interlocked.Exchange(ref _current, Next);
        Next = swappedBuffer;
        return swappedBuffer;
    }
}

Please correct me if I am wrong, but isn't such scenario possible with accepted answer?

public T Swap()
{
    var swappedBuffer = Interlocked.Exchange(ref _current, Next); // 1
    Next = swappedBuffer; // 2
    return swappedBuffer; // 3
}

And it goes like that: (A and B denotes some object of type T):

_current = A
Next = B

Thread 1 executes Line 1:
swappedBuffer = A
_current = B
Next = B

Thread 2 executes Line 1:
swappedBuffer = B
_current = B
Next = B

Thread 1 executes Line 2:
Next = A

Thread 2 executes Line 2:
Next = B

Now both _current and Next are references to B.


Python shuffle the middle letters in all the words of a sentence The purpose of the program is to shuffle only the middle letters of each word in a sentence, leaving it still readable like this:

  Aoccdrnig to a rscheearch at Cmabrigde Uinervtisy, it deosn't mttaer in waht oredr the ltteers in a wrod are, the olny iprmoetnt tihng is taht the frist and lsat ltteer be at the rghit pclae.

def garble(sentence):

    return ' '.join([word[0] + ''.join(random.sample([char for char in word[1:-1]],len(word[1:-1]))) + word[-1] if len(word) > 1 else word for word in  sentence.split()])

I definitely think I have gone overboard putting it all in one line but I'm not sure where to separate it

The fact that Python allows fitting things on a single line and pack in comprehensions, does not mean you need to try to squash the code to be shorter hurting readability. I would probably at least expand the code into multiple lines with descriptive variable names.

Here is, though, an alternative solution based on regular expression replacement function:

from random import shuffle
import re

RE_GARBLE = re.compile(r"\b(\w)(\w+)(\w)\b")

def garble_word(match):
    first, middle, last = match.groups()

    middle = list(middle)
    shuffle(middle)

    return first + ''.join(middle) + last

def garble(sentence):
    return RE_GARBLE.sub(garble_word, sentence)


Shooter game simulation The program is a little code about guns with user input. The program is a perfect representation of the beauty of functions. What do you think of it?

import sys
import time

#determines reload time
def reloading(reload_time):
    print('\nReloading', end='')

    for dot in range(reload_time):
        time.sleep(1)
        print('.', end='')

#parameters including the numerator/denominator, fire rate, and reload speed
def gun(ROUNDS, left, fire_rate, the_reload):
    print('\nshoot!')
    #hold enter
    rounds = ROUNDS
    print('\n{}/{}'.format(rounds, left))

    while rounds > 0 or left > 0:
        shoot = input()
        rounds -= 1
        #decrement each time input is given
        print('{}/{}'.format(rounds, left))

        if rounds == 0 and left == 0:
            print('OUT OF AMMO')
            break
        elif shoot.upper() == 'DONE':
            sure = input('Are you sure?\n> ')
            if sure.lower() == 'y':
                start()
                #allocates back to gun selection
        elif rounds == 0:
            if left <= ROUNDS:
                reloading(the_reload)
                rounds += left
                left -= left
                print('\n{}/{}'.format(rounds, left))
                #if bullets left is less than the magazine
            else:
                reloading(the_reload)
                rounds += ROUNDS
                left -= ROUNDS
                print('\n{}/{}'.format(rounds, left))

        time.sleep(fire_rate)
        #fire rate

def start():
    firearm = input('CHOOSE YOUR GUN (ENTER to exit)\n> ')

    if firearm.upper() == 'ASSAULT' or firearm.upper() == 'ASSUALT':
        gun(40, 130, 0.1, 7)
    elif firearm.upper() == 'SHOTGUN':
        gun(6, 30, 2, 12)
    elif firearm.upper() == 'SMG':
        gun(40, 200, 0, 5)
    elif firearm.upper() == 'SNIPER':
        gun(1, 15, 0, 5)
    elif firearm.upper() == 'MARKSMAN':
        gun(20, 100, 0.8, 9)
    elif firearm.upper() == 'PISTOL':
        gun(10, 100, 0.6, 3)
    else:
        sys.exit()
start()

Apart from that I don't quite get the point of your game, there are a few things that caught my eye:

1. User input
Your user input system is not particularly intuitive to use. Without the source code the user has virtually no way to see what inputs are acceptable at the moment or what your game does.

2. Documentation
Your functions are documented like this at the moment:

#determines reload time
def reloading(reload_time):
    ...

Enter the official Style Guide for Python Code (aka PEP8)! There, the official recommendation on how to document function is to use """documentation strings""" like so:

def reloading(reload_time):
    """determines reload time"""
    ...

Documentation written in that style helps Python's built-in help(...) function as well as basically all Python IDEs to pick it up. Future-you, and depending on the path you choose also others, will thank you a lot if you learn this early and stick to it.

3. Start
The general notes on user input apply here. But apart from that, there is also a lot of repeated code here. You call firearm.upper() on every branch of the if statement, when a simple firearm = input('CHOOSE YOUR GUN (ENTER to exit)\n> ').upper() could do that in one place.

It's also common practice to wrap the code that is supposed to be run in a scripty-manner in if __name__ == "__main__":. This basically tells the Python interpreter (and also people looking at your code) that this part is meant to be run as script and is not part of the code that's eventually going to  be imported into some other script. See also the official documentation or this Stack Overflow post for further details.

if __name__ == "__main__":
    # This makes sure you game will not start if you ever import the file
    start()

sys.exit() is also a bit uncommon here but we will touch on that in a moment.

4. Reloading
Slow reloading is quite a killer (pun intended). Especially if it looks like nothing is happening. When I did a test run with your game on my Windows laptop, nothing happened after Reloading appeared on the console until the "loading" actually finished. This was what I expected, since at least on Windows print does not automatically flush the buffer so you will see nothing in the reload cycle. Using print('.', end='', flush=True) fixes that.

5. Gun play
gun(...) is were the "fun" starts. This is actually your main gameplay, so gun is likely not a really good name. Often methods have some kind of action description in their name like fire_gun(...) or something similar. Nouns like gun are usually only used for class names.

Since we are alread nit-picking about names, let's look at the paramters:

ROUNDS: all uppercase names are by convention usually reserved for global, odule, or class level constant values. Here, the capitalization seems to be used to disambiguate the value from the internal variable rounds. But why not choose a name that seems more appropriate for what the value actually represents like magazine_capacity?
left is very generic here and leaves room for interpretation. total_rounds or reserve_capacity sound like more appropriate names in this context here.
fire_rate is in its own right actually quite okay, although the variable name contradicts the way you use it. A rate is usually defined as something per amount of time, like shots per seconds. In your case it's actually used the other way round as time between two shots. So consider changing  either of them to fit the other.
the_reload: Why this name? Why not time_to_reload?

Now that we have sorted this out, let's look at the control flow: On the top-most level you repeat the fire-till-empty-then-reload cycly until you are completely out of ammo. That's fine. There is a little bit of repeated code in rounds == 0 where print('\n{}/{}'.format(rounds, total_rounds)) could be used just once outside the inner if statement

elif rounds == 0:
    if total_rounds <= magazine_capacity:
        # if bullets left is less than the magazine
        ...
    else:
        ...
    print('\n{}/{}'.format(rounds, total_rounds))

The first case with if rounds == 0 and total_rounds == 0: is also fine. With elif shoot.upper() == 'DONE': things start to get a little bit troubling. The code in this block is as follows:

elif shoot.upper() == 'DONE':
    sure = input('Are you sure?\n> ')
    if sure.lower() == 'y':
        start()

Since this basically has nothing to do with the gameplay, the code breaks the single responsibility principle which is often recommended when thinking about/designing functions. This a higher level gameplay element and should better be placed in start(). Doing this will also help you to get rid of the yet to surface error caused by exceeding the maximum recursion depth. You can get at that point if you get into that branch of the if statement over and over again, calling start(), which then in turn calls gun(...), which then calls start() again, and so on and so forth, until the program crashes.

So how to do this in a better way?

def fire_gun(magazine_capacity, reserve_capacity, fire_rate, time_to_reload):
    """parameters including the numerator/denominator, fire rate, and reload speed"""

    ...

    while rounds > 0 or reserve_capacity > 0:
        shoot = input()

        rounds -= 1
        print('{}/{}'.format(rounds, reserve_capacity))

        if rounds == 0 and reserve_capacity == 0:
            print('OUT OF AMMO')
            break
        elif shoot.upper() == 'DONE':
            sure = input('Are you sure?\n> ')
            if sure.lower() == 'y':
                break
        elif rounds == 0:
            ...

        time.sleep(fire_rate)   # Faster!!

def start():
    while True:
        firearm = input('CHOOSE YOUR GUN (ENTER to exit)\n> ').upper()

        if firearm == 'ASSAULT':
            fire_gun(40, 130, 0.1, 7)
        ...
        else:
            break

In this version there is no recursion and you come back to the "weapons menu" whenever you are out of ammo of the user chooses to quit before. No recursion, no recursion limit, no crash! Cool.

6. Comments
You are using them wrong! Comments in source code are usually best used to explain why you are doing something, and not what you are doing, especially if it's quite obvious, like e.g.

rounds -= 1
# decrement each time input is given

This is clear from the code. No need to waste a comment on this.

Sometimes you have to do a little bit more to make a comment unnecessary, e.g. there at

if left <= ROUNDS:
    ...
    #if bullets left is less than the magazine

If you stick to the naming recommendations from above this can become

if reserve_capacity <= magazine_capacity:
    ...

See? No need for that comment any more! The code is now self-explaining.

On a not-strictly Python-related note: you are the first person I get to know who somehow likes to stick their comments below the line they are commenting on. Most people I know (including me) would expect the comment to live on the line above or on the same line as the code that comments belongs to.

Advanced ideas

You find that long if statement in start() cumbersome to read and hard to extend? You would like to extend your weapons locker with ease? You are willing to look into Python's magic box? Then read on.

Often this kind of long if statement can be transformed into something that's easier to maintain and extend using a Python dictionary. It a so called mapping, which maps a key to a value.

In your concrete example one could use something like this:

def start():
    guns = {
        "ASSAULT": lambda: fire_gun(40, 130, 0.1, 7),
        "SHOTGUN": lambda: fire_gun(6, 30, 2, 12),
        ...   # much more guns here
    }

    while True:
        firearm = input('CHOOSE YOUR GUN (ENTER to exit)\n> ').upper()

        try:
            guns[firearm]()
        except KeyError:
            break

The gun names are used as keys of the dictionary, this is quite obvious. Depending on your Python knowledge, you might also know something about lambda expressions as well. If not, think of them as functions or whatevere you like that can be "called", but without a explicit name. That's how guns[firearm]() works: For a given key, say "ASSAULT", the dictionary returns a lambda expression likelambda: fire_gun(40, 130, 0.1, 7)in our example. You could also assign this to a variable likeparameterized_version_of_fire_gun = guns[firearm]. You can think ofparameterized_version_of_fire_gunas a function at that moment that you would call like you callreloading`, for example. If it helps you can also think of it as a "normal" Python function:

def parameterized_version_of_fire_gun():
    fire_gun(40, 130, 0.1, 7)

So the next logical step is now to call parameterized_version_of_fire_gun() (← Note the opening and closing parenthesis).

As a bonus, this version would allow you to print guns.keys() to easily show the user all the possibilites to choose from.


Check if the array contains duplicate values The problem is old and simple but I wanted to try it in a different way. With some extra memory I have created an OOP-based solution. Hopefully it doesn't hurt in the interview.

import java.util.*;
import java.lang.*;
import java.io.*;

class Main {
  static class Solution {
    private int[] items;
    private List<Integer> duplicates;

    Solution(int[] items) {
      this.items = items;
      this.duplicates = new ArrayList<>();
      if (items.length > 1) {
        solve();
      }
    }

    private void solve() {
      Set<Integer> appeared = new HashSet<>();
      for (int item : items) {
        if (!appeared.add(item)) {
          duplicates.add(item);
        }
      }
    }

    public boolean hasDuplicate() {
      return duplicates.size() > 0 ? true : false;
    }
  }

  private static int[] readInput() {
    Scanner s = new Scanner(System.in);
    int N = s.nextInt();
    int[] arr = new int[N];

    for (int i = 0; i < N; i++) {
      arr[i] = s.nextInt();
    }
    return arr;
  }

  private static void tests() {
    // Falsy
    System.out.println(new Solution(new int[]{}).hasDuplicate());
    System.out.println(new Solution(new int[]{1}).hasDuplicate());
    System.out.println(new Solution(new int[]{1, 2}).hasDuplicate());
    System.out.println(new Solution(new int[]{Integer.MIN_VALUE, Integer.MAX_VALUE}).hasDuplicate());

    // Truthy
    System.out.println(new Solution(new int[]{1, 1}).hasDuplicate());
    System.out.println(new Solution(new int[]{Integer.MAX_VALUE, Integer.MAX_VALUE}).hasDuplicate());
    System.out.println(new Solution(new int[]{Integer.MIN_VALUE, Integer.MIN_VALUE}).hasDuplicate());
  }

  public static void main(String[] args) {
    //System.out.println(new Solution(readInput()).hasDuplicate());
    tests();
  }
}

Note: I cannot use JUnit or any testing framework during the interview.

Wasted memory

There is nothing in the description and the posted code to justify storing the input array and duplicates inside a class. This is a waste of memory.

If you just want to check if an array has duplicate values,
you should do just that. This means you could return early as soon as you found the answer:

public boolean hasDuplicate(int[] items) {
  Set<Integer> appeared = new HashSet<>();
  for (int item : items) {
    if (!appeared.add(item)) {
      return true;
    }
  }
  return false;
}

It's worth noting that fully reading the items array is also a wasted memory. You might even get a follow-up question "what if there are more numbers to check than fit in memory?". Instead of fully reading this array, consider streaming options, for example passing a Scanner, or Iterator<Integer> to hasDuplicate, which will read one number at a time, avoiding unnecessary I/O and memory usage. Just don't forget to close the input resource in the caller method after the computation is finished.

Idiomatic expressions

This is both tedious and not idiomatic:

return duplicates.size() > 0 ? true : false;

Use boolean expressions directly, and use the isEmpty method of collections (and strings) to check for emptiness:

return !duplicates.isEmpty();

A word about testing

  Note: I cannot use JUnit or any testing framework during the interview.

You cannot run a main method with tests either.
You can write down proper unit tests during an interview just as you can write the main method you did with tests.


Efficiently Generate Subset of all Permutations or Combinations with and without Repetition (C++) Suppose I want to write some functions which return the vector containing a vector representing all permutations or combinations with or without repetition (dependent on the function) of a subset of a given size of a given input vector.  Right now, my code is looking like this:

#include <vector>
#include <algorithm>
#include <iostream>

template <typename T>
struct permcomb
{
  std::vector<std::vector<T>> end_set;
  std::vector<T>* data;
  permcomb(std::vector<T>& param) : data(&param) {}

  void permutation_help(std::vector<T> seen, int recurse_num, bool repetition, bool comb)
  {
    if(recurse_num == 0)
    {
      if(end_set.size() > 0 && comb)
      {
        bool f = false;
        for(int i = 0; i < end_set.size(); i++)
        {
          if(std::is_permutation(end_set[i].begin(), end_set[i].end(), seen.begin()))
          {
            f = true;
          }
        }
        if(!f)
        {
          end_set.push_back(seen);
        }
      }
      else // permutations always choose this one
      {
        end_set.push_back(seen);
      }
    }
    else
    {
      for(int i = 0; i < (*data).size(); i++)
      {
        if(repetition || find(seen.begin(), seen.end(), (*data)[i]) == seen.end()) // if not found
        {
          seen.push_back((*data)[i]);
          permutation_help(seen, recurse_num - 1, repetition, comb);
          seen.pop_back();
        }
      }
    }
  }
};

// return all permutations no repetition
template <typename T>
std::vector<std::vector<T>> rapnr(std::vector<T>& data, int subset_size)
{
  permcomb<T> helpstruct(data);
  std::vector<T> empty {};
  helpstruct.permutation_help(empty, subset_size, false, false);
  return helpstruct.end_set;
}

// return all permutations with repetition
template <typename T>
std::vector<std::vector<T>> rapwr(std::vector<T>& data, int subset_size)
{
  permcomb<T> helpstruct(data);
  std::vector<T> empty {};
  helpstruct.permutation_help(empty, subset_size, true, false);
  return helpstruct.end_set;
}

// return all combinations no repitition
template <typename T>
std::vector<std::vector<T>> racnr(std::vector<T>& data, int subset_size)
{
  permcomb<T> helpstruct(data);
  std::vector<T> empty {};
  helpstruct.permutation_help(empty, subset_size, false, true);
  return helpstruct.end_set;
}

// return all combinations with repetition
template <typename T>
std::vector<std::vector<T>> racwr(std::vector<T>& data, int subset_size)
{
  permcomb<T> helpstruct(data);
  std::vector<T> empty {};
  helpstruct.permutation_help(empty, subset_size, true, true);
  return helpstruct.end_set;
}

This seems to be giving me the right answers so far... but I'm not really happy with the way I went about generating the combinations.  Literally the only thing I could think of doing was generating all of the permutations and then eliminating all but one of those which were permutations of each other.  This clearly doesn't seem very efficient, but I was at a loss for else I could do.  Similarly, I don't feel as I generated the permutations that well either since I always have to go back and search for items that I've already inserted to make sure that I don't insert them again (in the case that repetition isn't allowed).  So what steps can I take to optimize what I have so far?

I'm going to focus on the algorithmic aspects of your code rather than the style, as I don't know C++. You want to generate permutations and combinations with unique elements, so all you need to do is find the sublist of unique elements, and find the permutations and combinations of that. To uniquify, you should use a structure with a fast contains method (tree or hash set), and add each element is the vector to it if it isn't already. This will use O(n) memory, but your output is much later, so that overhead is justified given that it will save a lot of time later.


Nesting versus GOTO: which is better to avoid? In Java they're not really known as GOTO statements and are rather referred to as Branching Statements, but I find that the former term is a bit more indicative of what they actually do.

Regardless, I've been designing some code recently which has me in a bind.  On the one hand, I've been taught to avoid avid use of such statements because they make code both non-linear and more difficult to follow.  They of course have excellent uses, and I'm not one of those puritans who says that they should be discarded at any cost.

On the other hand, there's also a common tenet that says I ought to avoid deep nesting of conditions and loops if it's possible and reasonable to do so.

So here's my conundrum.  I'm hoping someone can guide me and give me some solid reasoning and best practices advice, because apparently my brain can't handle the conflicting signals.

Several times through my code, I have a for() loop which is something like the following (where lines is a List<String> and values is a Map<String, String>):

for(String line : lines) {

    if(line.charAt(0) == '#') {
        LOGGER.debug("Skipping commented line in file.");
        continue;
    }

    line = line.trim().toLowerCase();
    String[] pair = line.split("=");

    if(pair.length != 2) {
        LOGGER.error("Skipping malformed line in file: " + line);
        continue;
    }

    pair[0] = pair[0].trim();
    pair[1] = pair[1].trim();

    if(values.containsKey(pair[0])) {
        LOGGER.debug("Value is already assigned.");
        continue;
    }

    values.put(pair[0], pair[1]);

    //...and so on with still more processing
}

The reason I gave a little bit more in this code snippet than is strictly required to understand the use case is because I think the fact that the for loop isn't really "tight" and that it performs a lot of processing is relevant to the style choice.  To me, it makes the code feel somewhat sloppy and messy to have multiple areas scattered throughout it which bump the reader back to the beginning of the loop.  In other words, it feels a lot like the good old GOTO that everyone loves to hate on.

So my other design option is to write it with the following style:

for(String line : lines) {
    if(!line.charAt(0) == '#') {
        line = line.trim().toLowerCase();
        String[] pair = line.split("=");
        if(pair.length == 2) {
            pair[0] = pair[0].trim();
            pair[1] = pair[1].trim();
            if(!values.containsKey(pair[0])) {
                values.put(pair[0], pair[1]);
                //...and so on with still more processing
            }
            else {
                LOGGER.debug("Value is already assigned.");
            }
        }
        else {
            LOGGER.error("Skipping malformed line in file: " + line);
        }
    }
    else {
        LOGGER.debug("Skipping commented line in file.");
    }
}

Both of these techniques/styles makes my eye twitch spastically for various reasons, and I keep rewriting my code to fit whichever rationalization wins out in the current moment.  I also think that if you definitively choose one as "The Style" to always go with, it's trivial to extend the above code snippets to the point where it makes more sense to go with the other.

Does anyone have a more definitive statement of when to use one style over the other?  Or even just some good reasoning to apply to a situation to make sure you are following the best rule and coding practice?  Any general tips, tricks, and advice for dealing with these sorts of situations?

A different way is to let these pairs be represented by a class, which itself has a static factory method that will return null on failure.

public final class Pair {
    private String key;
    private String value;

    public Pair(key, value) {
        // TODO: Add proper argument checking/throw exceptions.

        this.key = key;
        this.value = value;
    }

    public String getKey() {
        return key;
    }

    public String getValue() [
        return value;
    }

    /**
     * Returns the string representation which looks like @{code key=value}.
     */
    @Override
    public String toString() {
        return key + "=" + value;
    }
}

And the static factory method:

/**
 * Constructs a Pair from the given string,
 * returns null if the string is misformed or
 * the string was null.
 *
 * A well formed input looks like {@code key=value} and
 * does not start with a {@code #} as that indicates comments.
 */
public static Pair fromString(String str) {
    // We do not accept nulls and empty strings.
    if (str == null || str.empty()) {
        return null;
    }

    // TODO: Figure out if trimming of str would be a good idea.

    // Skip comments.
    if (str.startsWith("#")) {
        return null;
    }

    String[] splitted = str.split("=");

    // We also do not accept anything else.
    if (splitted.length != 2) {
        return null;
    }

    // TODO: Are you accepting zero-length keys/values?

    return new Pair(splitted[0], splitted[1]);
}

It basically looks the same as your loop, with two important differences:

It's a method of its own, easily reusable, easily testable.
It has documentation what the input should look like.

Now to your loop:

for (String line : lines) {
    Pair pair = Pair.fromString(line);
    if (pair != null && !values.containsKey(pair.getKey())) {
        // TODO
    }
}

That does not only shorten the code inside the loop, but will in the end make it more readable because you're now using pair.getValue() instead of pair[1].

Based on the context of this I would suggest other names, though, like StringPair, Setting, SettingsPair or ConfigEntry.

Here is your loop with the log statements (assuming that the LOGGER has an overload that works similar to Logger.log(...)):

for (String line : lines) {
    Pair pair = Pair.fromString(line);
    if (pair != null) {
        if (!values.containsKey(pair.getKey()) {
            // TODO
        } else {
            LOGGER.debug("Pair is already assigned: {0}", pair);
        }
    } else {
        LOGGER.error("Skipping line: {0}", line);
    }
}

There are no fine-grained error messages there because it should be obvious from the input why the fromString function could not create the Pair.

Though we're back at two if-statemens with else branches, readability is a lot better.

This does violate Item 43 from Effective Java (2nd Edition) which goes like this:

  Return empty arrays or collections, not nulls

But why did I than suggest this approach? Effective Java states that it is better to throw fine grained exceptions rather then return null to ease dealing with the method. This is true for any sort of method, but it is not what this approach is supposed to do. We don't care what kind of problem there is, we just want an object that is usable for our program logic or move on, discarding fine grained errors in the process.

If you use this approach you need to keep some things in mind:

Don't use a constructor for this purpose but a static factory method.
Document the behavior and purpose of the static factory method thoroughly.
It should be obvious when seeing the input what the function returns.
Know when to use this and when to use exception handling (be aware why this is not a generic all-in-one solution to everything).
If this is inside a library/public API, always provide a way that does throw fine grained exceptions.

In case you're parsing ini or settings file with this which do not require "special" treatment, the answer from AJMansfield is correct.

  Do not roll your own version if java.util.Properties works for it.


Bouncing square in a box I wrote my first animation in Java FX and would like a code review.

I created StackPane with Rectangle. Rectangle starts to move inside StackPane to right-bottom direction and changes direction after hitting the  StackPane bound.

Please review the code that moves Rectangle. Is it a correct implementation of a Java FX animation?

//...
new RectangleMover(pane, shape);
// ...

RectangleMover.java

public class RectangleMover extends Timer
{
   private final StackPane pane;
   private final Rectangle shape;
   private boolean moveRight = true;
   private boolean moveBottom = true;

   public RectangleMover(StackPane pane, Rectangle shape)
   {
      super(true);
      this.pane = pane;
      this.shape = shape;
      this.scheduleAtFixedRate(new Task(), 0, 10);
   }

   private static int boolToInt(boolean bool)
   {
      return bool ? 1 : -1;
   }

   private final class Task extends TimerTask
   {
      @Override
      public void run()
      {
         shape.setLayoutX(shape.getLayoutX() + boolToInt(moveRight));
         shape.setLayoutY(shape.getLayoutY() + boolToInt(moveBottom));

         if (shape.getLayoutX() >= pane.getWidth() - shape.getWidth())
         {
            moveRight = false;
         }
         if (shape.getLayoutY() >= pane.getHeight() - shape.getHeight())
         {
            moveBottom = false;
         }
         if (shape.getLayoutX() <= 0)
         {
            moveRight = true;
         }
         if (shape.getLayoutY() <= 0)
         {
            moveBottom = true;
         }
      }
   }
}

JFXTester.java (main class)

import javafx.application.Application;
import javafx.event.ActionEvent;
import javafx.event.EventHandler;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.layout.StackPane;
import javafx.scene.paint.Color;
import javafx.scene.shape.Rectangle;
import javafx.stage.Stage;

public class JFXTester extends Application
{

   StackPane root;

   @Override
   public void start(Stage primaryStage)
   {

      root = new StackPane();
      root.getChildren().add(createStartButton());

      Scene scene = new Scene(root, 300, 250);

      primaryStage.setTitle("Hello World!");
      primaryStage.setScene(scene);
      primaryStage.show();
   }

   private Rectangle createRectangle()
   {
      Rectangle r = new Rectangle();
      r.setX(0);
      r.setY(0);
      r.setWidth(30);
      r.setHeight(30);
      r.setFill(new Color(0.8, 0.7, 0.6, 0.5));

      return r;
   }

   private Button createStartButton()
   {
      final Button btn = new Button();
      btn.setText("START!");
      btn.setOnAction(new EventHandler<ActionEvent>()
      {
         @Override
         public void handle(ActionEvent event)
         {
            btn.setVisible(false);
            Rectangle r = createRectangle();
            root.getChildren().add(r);
            new RectangleMover(root, r);
         }
      });
      return btn;
   }

   public static void main(String[] args)
   {
      launch(args);
   }
}

How to run application (java 1.7.0_51-b13):

D:\.temporary\java>dir /a-d /b /s
D:\.temporary\java\jfxtester\JFXTester.java
D:\.temporary\java\jfxtester\RectangleMover.java

D:\.temporary\java>%JAVA_HOME%\bin\javac -cp .;%JRE_HOME_7%\lib\jfxrt.jar ./jfxtester/JFXTester.java

D:\.temporary\java>%JRE_HOME_7%\bin\java -cp .;%JRE_HOME_7%\lib\jfxrt.jar jfxtester.JFXTester

No, this is not the correct way to do it. I would recommend that you read JavaFX Transitions documentation.

I personally could not get your code to work properly (Using Java 8, but that should not matter as Java is always backwards compatible). The rectangle showed on the screen, but it did not move at all.

I got this exception:

Exception in thread "Timer-0" java.lang.IllegalStateException: Not on FX application thread; currentThread = Timer-0
at com.sun.javafx.tk.Toolkit.checkFxUserThread(Unknown Source)
at com.sun.javafx.tk.quantum.QuantumToolkit.checkFxUserThread(Unknown Source)
at javafx.scene.Scene.addToDirtyList(Unknown Source)
at javafx.scene.Node.addToSceneDirtyList(Unknown Source)
at javafx.scene.Node.impl_markDirty(Unknown Source)
at javafx.scene.shape.Shape.impl_markDirty(Unknown Source)
at javafx.scene.Node.impl_transformsChanged(Unknown Source)
at javafx.scene.Node$13.invalidated(Unknown Source)
at javafx.beans.property.DoublePropertyBase.markInvalid(Unknown Source)
at javafx.beans.property.DoublePropertyBase.set(Unknown Source)
at javafx.scene.Node.setLayoutX(Unknown Source)

One I fixed that problem to make it run on the JavaFX thread, the rectangle still did not move an inch pixel.

About the cleaniness of your code:

public class RectangleMover extends Timer

extends Timer is a very bad smell here. There's absolutely no reason to extend Timer. Use a timer as a field inside your class instead. Prefer composition over inheritance.

Speaking of Timer, you could say that it is more or less deprecated, it is often better to use a ScheduledExecutorService. However, if you do the animation properly, you will not need a Timer or ExecutorService at all.

I do like how you're using your boolToInt method (just a shame that I can't the results of it does not seem to be working). I also like how you have separated your methods overall.

I just don't like that your code does not seem to be working for me...


Chain of responsibility and handlers dependent on each other I'm importing data from file to many tables in database. It may look like this:

First Name, Last Name, Meeting
John, Doe, 2020-04-24 08:00:00

Some fields like "First Name" and "Last Name" are being saved in one table and other fields like "Meeting" are being saved in different tables. There are a lot of more fields and a lot of more rows in my case.

I create handler for each table and put it in a handler chain to reduce amount of code. Each handler knows which fields it can handle. My base handler class looks like this:

abstract class TableHandler
{
    protected $nextHandler;

    abstract public function isFieldSupported(string $field): bool;
    abstract public function handle(string $fieldName, string $fieldValue);

    public function next(TableHandler $nextHandler): TableHandler
    {
        $this->nextHandler = $nextHandler;
    }

    public function handleField(string $fieldName, string $fieldValue)
    {
        $this->handle($fieldName, $fieldValue);

        if ($this->nextHandler()) {
            $this->nextHandler->handleField($fieldName, $fieldValue);
        }
    }

    public function reset(): void
    {
        $this->resetHandler();

        if ($this->nextHandler()) {
            $this->nextHandler->reset();
        }
    }

    private function nextHandler(): bool
    {
        return $this->nextHandler !== null;
    }
}

I use my handler chain this way:

$table1->next($table2)
    ->next($table3);

foreach ($data as $row) {
    foreach ($headers as $index => $header) {
        $table1->handleField($header, $row[$index]);
    }

    $id = $table1->save();
    $table2->save($id);
    $table3->save($id);

    $table1->reset();
}

As you can see I was able to handle fields and reset all handlers by using only first handler. Chain does the rest of the job. The problem is that I don't know how can I improve saving data. When I insert record in first table I get its ID. I need to pass this ID to further handlers because rows that I insert in those tables are related to row from first table so ID is needed. Bacause of it I need to save data separately. I would like to avoid it. Is it possible?

I'm not to sure about your base handler. It has methods like handle() and handleField() that look very similar. One will probably do. I also noted that the save() method is completely missing from your code. Why? You use it, so it should be there. Or at least in an usage example.

As to your actual problem with saving, you can hand over the id's as an argument to the next save() in the clain. Something like:

public function save(array $insertIds = [])
{
    // ..... put needed insert ids into data here ....
    // then perform the insert and get the new insert id
    $newInsertId = $this->insertRow($this->table,$this->data);
    // add that to the array of insert ids
    $insertIds[$this->table] = $newInsertId;
    // progress down the chain, if possible
    if ($this->nextHandler()) {
        $this->nextHandler->save($insertIds);
    }
}

Note that in the $insertIds array the keys are table names, and the values are the actual insert ids. This way all insert ids are available to save() methods down the chain.

It could be useful to return the $insertIds.

Note that the order of chaining is important. You have to think about that when you create the chain. Perhaps it would be a good idea to allow for multiple next handlers.


Taking change - convergent iteration through iterate For the Code Review community challenge "Implement StackSTV", the algorithm requires a "convergent iterative scheme" to calculate the keep ratios of candidates that have been elected.

To simplify the calculation of those keep ratios I have written following function to truncate an infinite list that calculates the keep ratio scheme's next step.

I wonder whether I missed some basic library function and whether the function is idiomatic haskell:

{-
    Take items from a list, as long as the last item taken is not the same as the next item in the list.
    Passing an empty list returns an empty list.
-}
takeModified :: Eq a => [a] -> [a]
takeModified (x:xs) = [x] ++ go x xs
    where
    go :: Eq a => a -> [a] -> [a]
    go elem (x':xs')
       | elem == x' = []
       | otherwise  = [x'] ++ go x' xs'
    go elem [] = []
takeModified [] = []

Typically, where clauses don't include full type annotations, though this is mostly a matter of personal preference.
For prepending items to a list, prefer x:xs over [x] ++ xs.
In your "base case" of the recursive go function: go elem [] = [], elem is not used. Prefer _ in this case.
Your comment exceeds 80 characters in width. Again, a matter of personal preference. The note about passing an empty list returns an empty list is probably not necessary as this is very clearly the case from the code itself.

Here is how I would write this function:

takeWhileNotDup :: Eq a => [a] -> [a]
takeWhileNotDup [] = []
takeWhileNotDup (x:xs) = x:go x xs
  where
    go _ [] = []
    go previous (x:xs)
      | previous /= x = x:go x xs
      | otherwise = []


Making the save and cancel buttons editable The following code makes a div tag editable and handles the save and cancel buttons. But there is a lot of duplication. How could I remove the duplication on this?

var originalTexts = {};
$("div.edit-panel").find("span").click(function() {
    var panel = $(this).closest('div.simple-panel');
    var text  = panel.find(".info");
    var id = $(this).parent().attr("id");
    panel.find(".edit-panel").addClass('editable');
    text.addClass("content-editable").prop('contenteditable', 'true');
    originalTexts[id] = text.text();
})

$("div.edit-panel").find(".cancel-editable").click(function() {
    var panel = $(this).closest('div.simple-panel');
    var text  = panel.find(".info");
    var id    = $(this).parent().attr("id");
    text.text(originalTexts[id]);
    panel.find(".edit-panel").removeClass('editable');
    text.removeClass("content-editable").prop('contenteditable', 'false');
})

$("div.edit-panel").find(".save-editable").click(function() {
    var panel = $(this).closest('div.simple-panel');
    var text  = panel.find(".info");
    var currentText = text.text();
    text.text(currentText);
    panel.find(".edit-panel").removeClass('editable');
    text.removeClass("content-editable").prop('contenteditable', 'false');
})

How about using just one declaration and an if block? You remove the top three lines by this for each function -

assuming buttons are <button>

var originalTexts = {};
$("div.edit-panel").find("button,span").click(function(){
    var panel = $(this).closest('div.simple-panel');
    var text  = panel.find(".info");
    var id = $(this).parent().attr("id");
    var currentText = text.text();

    if ($(this).is("span")){
        panel.find(".edit-panel").addClass('editable');
        text.addClass("content-editable").prop('contenteditable', 'true');
        originalTexts[id] = text.text();
    }else if ($(this).hasClass("cancel-editable")){
        text.text(originalTexts[id]);
        panel.find(".edit-panel").removeClass('editable');
        text.removeClass("content-editable").prop('contenteditable', 'false');
    }else if ($(this).hasClass("save-editable")){
        text.text(currentText);
        panel.find(".edit-panel").removeClass('editable');
        text.removeClass("content-editable").prop('contenteditable', 'false');
    }
});

You can reduce more codes if you can store the id and other information inside the data attributes of each button. But I don't know the html, so could not suggest on that.


Password Manager UI: Populating DataGridView is Painfully Slow I'm designing a small account/password manager using C# and SQL Server CE. LINQ to SQL is my ORM of choice.

Here's the data model representing how the SQL Server CE database is set up:

And here's my GUI with a cheesy color-coding system to show how the data is mapped to the GUI:

Since the user is able to dynamically add whatever fields and columns they want, I can't simply bind the DataGridView to a table or collection. Instead, the DataGridView's columns, rows, and cells each have to be populated separately and manually.

Below is my implementation. It gets the job done, but it's TERRIBLY SLOW! We're talking 5-6 seconds to populate the DataGridView with only 10 accounts and 10 fields. The implementation just feels clunky. I've considered creating a DataTable and binding that to the DataGridView, but I would still have to manually populate the columns and rows, and it would be harder without the Tag property to identify columns, rows, and cells. Any suggestions to improve anything ranging from my database design to my GUI are welcome!

Business Logic Methods

    public static List<Account> GetAccountsByCategory(int categoryID)
    {
        using (PassShedEntities context = new PassShedEntities())
        {
            return context.Account.Where(a => a.Category_id == categoryID).OrderBy(a => a.Id).ToList();
        }
    }

    public static Credential GetCredential(int accountID, int fieldID)
    {
        using (PassShedEntities context = new PassShedEntities())
        {
            return (from c in context.Credential
                    join f in context.Field on c.Field_id equals f.Id
                    join a in context.Account on c.Account_id equals a.Id
                    where c.Account_id == accountID
                    where f.Id == fieldID
                    select c).SingleOrDefault();
        }
    }

    public static List<Field> GetFieldsByCategoryID(int categoryID)
    {
        using (PassShedEntities context = new PassShedEntities())
        {
            return context.Field.Where(f => f.Category_id == categoryID).OrderBy(f => f.Display_index).ToList();
        }
    }

Populate Fields (DataGridView Columns)

    private void PopulateFields(int categoryID)
    {
        dgvShed.Columns.Clear();

        foreach (Field field in FieldLogic.GetFieldsByCategoryID(categoryID))
        {
            DataGridViewColumn fieldColumn = new DataGridViewTextBoxColumn();
            fieldColumn.Name = field.Label;
            fieldColumn.HeaderText = field.Label;
            fieldColumn.Tag = field.Id;

            if (field.Display_index != null)
            {
                fieldColumn.DisplayIndex = (int)field.Display_index;
            }

            dgvShed.Columns.Add(fieldColumn);
        }
    }

Populate Accounts (DataGridView Rows)

    private void PopulateAccounts(int categoryID)
    {
        dgvShed.Rows.Clear();

        foreach (Account account in AccountLogic.GetAccountsByCategory(categoryID))
        {
            DataGridViewRow accountRow = new DataGridViewRow();
            accountRow.Tag = account.Id;

            foreach (DataGridViewColumn column in dgvShed.Columns)
            {
                Credential credential = CredentialLogic.GetCredential(account.Id, (int)column.Tag);

                DataGridViewTextBoxCell credCell = new DataGridViewTextBoxCell();
                credCell.Tag = credential.Id;
                credCell.Value = credential.Value;

                accountRow.Cells.Add(credCell);
            }

            dgvShed.Rows.Add(accountRow);
        }
    }

What you call business logic methods is actually your data access logic. And you've put that in a bunch of static methods on what I'm assuming is a static class, which means the GUI code / "populate fields/accounts" is tightly coupled with that specific data access logic implementation (AccountLogic); I'd make them instance methods and inject an instance of AccountLogic and CredentialLogic as a dependency, into the constructor of whoever needs it.

Your data model seems to have navigation properties. Your code isn't using them:

        return (from c in context.Credential
                join f in context.Field on c.Field_id equals f.Id
                join a in context.Account on c.Account_id equals a.Id
                where c.Account_id == accountID
                where f.Id == fieldID
                select c).SingleOrDefault();

Could be:

return context.Credential
              .Where(c => c.Account_id == accountID && c.Field.ID == fieldID)
              .SingleOrDefault();

Not sure whether/how it would affect performance though, but definitely clearer.

foreach (Account account in AccountLogic.GetAccountsByCategory(categoryID))
{
    ...
}

Might have zero performance impact, but I find this cleaner:

var accounts = AccountLogic.GetAccountsByCategory(categoryID)
foreach (var account in accounts)
{
    ...
}

As for performance, I'd be tempted to blame WinForms and constant redrawing taking place - try to disable the refreshing of the DGV's as you're manipulating them; I'm pretty sure WinForms is redrawing the grid every time a column is added, and that requires quite a lot of computing that's not needed until you're done adding all the columns.


Excel Duplicate File name with different Revisions I have an Excel where i have file names in one column with the revision numbers in another column. I need to remove the duplicates(file with higher version should be retained and lowerversion deleted).Can you guys help me with excel VBA for this.

Eg Raw Data

Desired output from macro

What i have tried( i am new to VBA , tried the following with help of google)

with Sheets("Sheet1")
    LRow1 = .Cells(Rows.Count, 1).End(xlUp).row
    LRow3 = "A1:" & getColumnAlpha & LRow1

     For d = 2 To LRow1
    Cells(d, 4) = Cells(d, 1) & "_" & Cells(d, 3)
    Next d
    Range("A2:F" & LRow1).Sort Key1:=Range("D2:D" & LRow1), order1:=xlAscending, Header:=xlNo
    'Sheets("Sheet1").Columns(4).Copy Destination:=Sheets("Test").Columns(2)
    Dim test As String
    test = LRow1
    finishedRow = LRow1
    'finishedRow() = test.ToStringArray
    'ReDim finishedRow(LRow1) As String
    row = 1
    resultRow = 1
    index = 1

    Do While .Range("D" & row) <> ""

      If UBound(Filter(finishedRow, row)) < 0 Then
    finishedRow = row
    index = index + 1
    innerRow = 1

    Do While .Range("D" & innerRow) <> ""

        If UBound(Filter(finishedRow, innerRow)) < 0 Then

            If .Range("D" & row) = .Range("D" & innerRow) Then

                If .Range("B" & row) < .Range("B" & innerRow) Then
                'Update time in result record
                'Sheets("Sheet1").Range("B" & resultRow) = "Delete"
                 Rows(resultRow).EntireRow.Delete

                End If

                            'Add row to record array
                            finishedRow = innerRow
                            index = index + 1

                        End If

                    End If

                    'Increase inner row
                    innerRow = innerRow + 1

                Loop

                'Increase result row
                resultRow = resultRow + 1

            End If

            'Increase row
            row = row + 1
    Loop

End With

This code removes duplicates but requires the macro to be run multiple times

Although this question is slightly off-topic I will give you a run down of your code. I will start with some bugs in your code, then make some general remarks about the coding style, explain what is probably causing your code to miss duplicates and, finally, propose an approach that removes all duplicates in one go.

Bugs

The bugs I referred to above are that you frequently refer to cells on the ActiveSheet instead to those on Sheet1 because you have omitted the dot in front.

Style

Now, let me say something about the coding style.

You should indent your code properly. In your code the start and end of if blocks and loops do not match up, which makes it very hard to read.
One of the best practices to improve readability of your code is to use meaningful names. E.g. LRow3 does not tell you at all what it is. (It containing 'row' I would expect it to be a Long, but actually it is a String. (I hope you declared it above the code you posted.)) As far as I can see it might better be called sometime like dataRangeString. If anybody else ever reads this again, or you in a few months, he will be grateful for the added readability.
This is not a major concern here, but try to follow the single responsibility principle, which basically tells you that a unit of code should do one and only one thing. E.g. the first for loop could be its own sub called SaveConcatenatedKeyInKeyColumn.
In the With block variable you make an implicit reference to the ActiveWorkbook. It is good practice to make that explicit. Generally, you should avoid implicit references and default members, because they hinder readability and hide subtle bugs.

For more help with the coding style you might want to have a look at Rubberduck. (Full disclosure, I am a contributer.)

Potential Cause of Problems

Now to what I think might be the problem with your code. When you are deleting rows, all rows below get shifted up. This makes you skip consecutive rows that should have been deleted.
To solve this, always remove rows bottom to top.

Alternative Approaches

The approach I want to propose is to sort first by filename (Key1) and then by version (Key2). All you have to do then is to go from bottom to top and remove every row where the row below it has the same filename in the filename column. Because you sorted by version as the second key, only the highest version survives.

Alternatively, you could use a Dictionary, which requires a reference to VBScript, to store the highest version seen so far in a first pass over your data. (Use Exists to see whether you have seen the filename before. If not, save the revision with the filename as key. If yes, compare the current revision for the filename to that in the dictionary and save it if it is higher.) In a second pass, from bottom to top, remove every row where the version is not the one in the dictionary.

The second approach is faster for large data sets since you do not have to sort. However, if you want to sort the filenames anyway, as your example suggests, there is no real performance advantage.


Updating / validating mass update rails association My recipes model is as follows

has_many :quantities
has_many :ingredients, :through => :quantities, :uniq => true
has_many :sizes, :through => :quantities, :uniq => true

And the controller action im trying to refactor

class Admin::QuantitiesController < Admin::AdminController
  layout 'admin'

  authorize_resource :class => :controller

  def create
    recipe        = Recipe.find(params[:recipe_id])
    values      = params[:quantity][:ingredient]
    @quantities = Quantity.find(values.keys)

    @quantities_errors = {}
    @quantities.each do |quantity|
        if quantity.value != values[quantity.id.to_s]
            quantity.value = values[quantity.id.to_s]
            if quantity.valid?
                quantity.save
            else
                @quantities_errors[quantity.id] = quantity.errors.messages
            end
        end
    end

    unless @quantities_errors
        redirect_to edit_admin_recipe_path(recipe), notice: 'Ingredients was successfully added.'
    else
        redirect_to edit_admin_recipe_path(recipe), alert: @quantities_errors
    end

  end

end

Some notes:

This is a edit_admin_recipe form, so you should POST to Admin::RecipesController#update.
Use Rails infrastructure for the models (accepts_nested_attributes_for) to save nested data. Controllers must be kept simple and clean.


Creating a linked list of n length I wrote this working code to create a linked list of a given length and print each node's values.

#include <stdlib.h> //NULL
#include <stdio.h>  //printf

typedef struct Node {
    int val;
    struct Node* next;
} node;

node* make_ll(int len){
    node* head = malloc(sizeof(node));
    node* cur = head;
    for (int i = 0; i < len; i++) {
        cur->val = i;
        cur->next = (i < (len - 1)) ? malloc(sizeof(node)) : NULL;
        cur = cur->next;
    }
    return head;
}

void print_ll(node* head){
    node* cur = head;
    while (cur != NULL) {
        printf("Node: %d @ %p\n", cur->val, cur);
        cur = cur->next;
    }
}

int main(){
    node* ll = make_ll(4);
    print_ll(ll);
    free(ll);
}

Output:

Node: 0 @ 0x81b010
Node: 1 @ 0x81b030
Node: 2 @ 0x81b050
Node: 3 @ 0x81b070

I'm wondering if there's a practical way to write make_ll() so that, rather than checking if we've reached the last desired node on every pass of the for loop and setting cur->next accordingly, as done here with line 14:

cur->next = (i < (len - 1)) ? malloc(sizeof(node)) : NULL;

we can simply malloc() for each "next" node until the last, setting it equal to NULL.

I'm new to C, mostly influenced by Python, and the things I tried to no avail were the result of my ignorance of the languages' difference in scope rules. It would probably be noisy to try to reproduce them here.

Any pointers in the right direction?

naked malloc calls

Note that malloc() simply allocates memory - you have no guarantees about what the newly allocated memory contains. For that reason it is better to write a function which both allocates and initializes an new object, i.e.:

node* newNode(int v) {
  node* p = (node*) malloc( sizeof(node) );
  // assume malloc never fails
  p->val = v;
  p->next = NULL;
  return p;
}

Now use newNode() where you are using malloc() directly.

make_ll

First of all, does your make_ll work for len = 0?

For comparison, here is a recursive solution for make_ll:

node* make_ll(int len) {
  if (len <= 0) {
    return NULL;        // a list of length 0
  } else {
    node *tail = make_ll(len-1);
    node *head = newNode(len);
    head->next = tail;
    return head;
  }
}

And here is a way I would transform this into a loop which avoids double checking the exit condition:

node* make_ll(int len) {
  if (len <= 0) {
    return NULL;
  }
  node* head = newNode(len);
  node* last = head;
  for (; len > 0; --len) {
    node* n = newNode(len);
    last->next = n;
    last = n;
  }
  return head;
}

The idea is to keep track of the last node in the chain so you can update its next field when you create a new node.


Pairwise Hamming Distance This is a program to return the sum of pairwise Hamming Distance of the binary string representation of a vector of integers. Please let me know how I can make this better?

int Solution::hammingDistance(const vector<int> &A) {
    vector<string> binrep;
    int N=A.size();
    int i,j, rem, n;
    for(i=0;i<N;i++){
        n=A[i];
        string bin;
        if (n == 0){
            bin.push_back((char)('0'));
            binrep.push_back(bin);
            continue;
        }
        while (n > 0) {
        rem = n % 2;
        bin.push_back((char)('0' + rem));
        n /= 2;
        }
        binrep.push_back(bin);
    }
    int ans=0;
    for(i=0;i<N;i++){
        for(j=i+1;j<N;j++){
            int k = 0, count = 0;
            while (binrep[i][k] != '\0' && binrep[j][k] !='\0')
            {
                if (binrep[i][k] != binrep[j][k])
                    count++;
                k++;
            }
            if(binrep[i][k] == '\0' && binrep[j][k]!='\0'){
                while (binrep[j][k] !='\0'){
                    if(binrep[j][k]=='1')
                        count++;
                    k++;
                }
            }
            else if (binrep[i][k] != '\0' && binrep[j][k]=='\0'){
                while (binrep[i][k] !='\0'){
                    if(binrep[i][k]=='1')
                        count++;
                    k++;
                }
            }
            ans+=count*2;
            ans=ans%1000000007;
        }
    }
    return(ans);
}

Code Review

Never use C cast

The C cast is dangerously hard to spot. In C++ we have replaced this with four distinct cast operators that all do slightly different things. They are very easy to spot (as you should not really be doing cast anyway and when you do we want to be able to spot it easily for code review).

            bin.push_back((char)('0'));

So if this was C++ that would use the static cast.

            bin.push_back(static_cast<char>('0'));

But not really. Anything that is in single quotes '0' already has a type of char so you don't actually need to cast it.

            bin.push_back('0');

Also in this situation. you only push back one character. So why not just use assignment in the first place.

            bin = "0"; // much more clear.

Actually there is no need to even use the variable bin. In the next line you can simply push back a literal string.

            binrep.push_back(bin);

            // why not just
            binrep.push_back("0");

Use C++ classes where you can.

        while (n > 0) {
            rem = n % 2;
            bin.push_back((char)('0' + rem));
            n /= 2;
        }

So you are building a binary version of the number in a string. Easier way to do that using std::bitset.

        std::bitset<8> value(n);
        bin.push_back(value.to_string()); // You may need to reverse
                                          // or something I did not pay
                                          // full attention to what you
                                          // were doing.

When looping over containers use the range based for.

    for(i=0;i<N;i++){

    // can be replaced by this.
    for(auto const& item: binrep) {

Look for standard algorithms where you have a loop:

This looks like you are counting.

                while (binrep[j][k] !='\0'){
                    if(binrep[j][k]=='1')
                        count++;
                    k++;
                }

                // I think it may have been easier to write:
                count += std::count(std::begin(binrep[j]), std::end(binrep[j]), '1');

This is also self documenting as you can see what you are doing. The algorithm is appropriately named to make your actions obvious.

###BUG

            binrep[i][k] != '\0'

You can not look for a terminating \0 on a C++ std::string. Accessing beyond the end of the string is undefined behavior. If you want a C-String version to loop over you need to call c_str() on the object (but better to use a range based for.

Style

A lot of this (style) is preference and dependent on your team. I think it is important for the community to be consistent and have describes some common practices below. Others will inevitably disagree as this is opinion, refer to your teams style guide.

But your horizontal spacing (it does not exist) makes your code hard to read it is all packed together horizontally. It just makes the code horrible (and thus harder to read). Technically nothing wrong with that, but the whole point of coding is to make it easy to read for the next human that has to decipher your code.

for(i=0;i<N;i++){

// I would write it like this:

for(int i = 0; i < N; ++i) {

Your usage of braces ( {} ) is inconsistent. Neither version is wrong. But using two different styles is jarring (and again makes it hard to read).

        while (n > 0) {
            // STUFF
        }

   // and

        while (binrep[i][k] != '\0' && binrep[j][k] !='\0')
        {
        }

Always indent sub-blocks and always use braces {} around sub blocks.

        // Not indenting the sub block makes it hard to read.
        while (n > 0) {
        rem = n % 2;
        bin.push_back((char)('0' + rem));
        n /= 2;
        }

        // Not using braces opens you up to maintenance problems.
        // larger than needed modifications when a bug is fixed.
        // thus reduced readability in the change log.
        //
        // There are also corner cases that are going where not
        // using the braces are going to be an error so prefer
        // to use them.
        if (binrep[i][k] != binrep[j][k])
            count++;

Declare variables one per line and at the point where they are used. This makes it easier to check types of local objects, and becomes much more important when your types have constructors and  destructors.

    // Also the inconsistent use of space is jarring.
    int i,j, rem, n;

Variable names. Please use them. Modern code is supposed to be self documenting. So reading the variable name and how it is used is supposed to help me to understand the context of the code.

    // I have no idea what these do (rem: short for remainder?)
    int i,j, rem, n;

    for(int loop = 0; loop < size; ++loop) // Its a loop.

With naming. It's traditional to use an initial upper case letter for user defined types while an initial lower case letter is used for objects (variables/functions). This is important in C++ because types are so much more important than in C. We want to understand when we are referring to a user type and when we are referring to an object (the distinction is important and the use of visual cues can help make the code self documenting).

NOTE: The use of identifiers that are all uppercase is still (even in C++) traditionally reserved for macros. You should avoid their use for other things. N is an all uppercase identifier.

Pick a style and be consistent. Consistency is key in making code that is easy to maintain.


Removing duplicates from an unsorted Array I have written this program to remove the duplicates from an unsorted array. Please review and give me your input on how I can improve it a little more.

public static void main(String[] args) {

    int[] arr = {4, 3, 4, 2, 6, 1, 1, 7, 6, 8, 9, 9, 1, 1};

    // perform quick sort for o(n log n) time.
    quickSort(arr, 0, arr.length - 1);

    System.out.println(Arrays.toString(arr));
    // now remove the duplicate elements.
    arr = removeDuplicates(arr);
    System.out.println("Unique elements in Array :" + Arrays.toString(arr));
}
private static int[] removeDuplicates(int[] arr) {
    // if array length is one, return the array
    if (arr.length <= 1) {
        return arr;
    }

    // new array keep the length equal to current array.
    int[] uniqueArray = new int[arr.length];
    // keep the lastfound number to be 0th index.
    int lastFound = arr[0];
    // copy the 0th index value to new arrays 0th.
    uniqueArray[0] = arr[0];

    // keep track of number of duplicates found.
    int totalDuplicates = 0;

    // current index position to put the unique number
    int currPos = 1;
    for (int i = 0; i < arr.length; i++) {

        if (lastFound == arr[i]) {
            totalDuplicates++;
        } else {
            lastFound = arr[i];
            uniqueArray[currPos] = arr[i];
            currPos++;
        }
    }

    // at this point array wil have unique numbers along with empty
    // slots at the end in array, lets remove those.

    int newLength = arr.length - totalDuplicates;

    uniqueArray = Arrays.copyOf(uniqueArray, newLength + 1);
    return uniqueArray;
}

private static void quickSort(int[] arr, int s, int e) {

    if (s < e) {
        int pivot = findPivot(arr, s, e);
        System.out.println(pivot);
        // sort left
        quickSort(arr, s, pivot - 1);
        // sort right
        quickSort(arr, pivot + 1, e);
    }

}

private static int findPivot(int[] arr, int s, int e) {
    int p = arr[e];
    int i = s;
    for (int j = s; j < e; j++) {
        if (arr[j] <= p) {
            swap(arr, i, j);
            i = i + 1;
        }
    }
    swap(arr, e, i);
    return i;
}

private static void swap(int[] arr, int i, int j) {
    int t = arr[i];
    arr[i] = arr[j];
    arr[j] = t;
}

Improving removeDuplicates

It's recommended to use an enhanced for-each loop instead of a counting loop when possible:

for (int num : arr) {
    if (lastFound == num) {
        totalDuplicates++;
    } else {
        lastFound = num;
        uniqueArray[currPos] = num;
        currPos++;
    }
}

And you can get rid of totalDuplicates, as currPos already contains that same information:

int currPos = 1;
for (int num : arr) {
    if (lastFound != num) {
        lastFound = num;
        uniqueArray[currPos] = num;
        currPos++;
    }
}

return Arrays.copyOf(uniqueArray, currPos);

I noticed later that your original code does an unnecessary comparison for the first element or arr: it would be enough to iterate from the 2nd element. Unfortunately, for that we need to bring back the counting loop:

int currPos = 1;
for (int i = 1; i < arr.length; ++i) {
    int num = arr[i];
    if (lastFound != num) {
        lastFound = num;
        uniqueArray[currPos] = num;
        currPos++;
    }
}

return Arrays.copyOf(uniqueArray, currPos);

Reduce allocations

Note that removeDuplicates allocates an array to collect the unique elements (uniqueArray),
and finally returns a clone of that array, with the appropriate size,
thus allocating for one more array.
If you don't mind modifying the input array, then you can avoid the allocation of uniqueArray by overwriting the content of the input array:

private static int[] removeDuplicates(int[] arr) {
    if (arr.length <= 1) {
        return arr;
    }

    int lastFound = arr[0];

    int currPos = 1;
    for (int i = 1; i < arr.length; ++i) {
        int num = arr[i];
        if (lastFound != num) {
            lastFound = num;
            arr[currPos++] = num;
        }
    }

    return Arrays.copyOf(arr, currPos);
}

Unit testing

To verify the implementation works,
and for safe refactoring,
it's good to have some unit tests around, for example:

@Test
public void test_1_2_3() {
    int[] orig = {1, 2, 3};
    assertArrayEquals(orig, removeDuplicates(orig.clone()));
}

@Test
public void test_empty() {
    int[] orig = {};
    assertArrayEquals(orig, removeDuplicates(orig.clone()));
}

@Test
public void test_single() {
    int[] orig = {3};
    assertArrayEquals(orig, removeDuplicates(orig.clone()));
}

@Test
public void test_1_1_1() {
    int[] orig = {1, 1, 1};
    assertArrayEquals(new int[]{1}, removeDuplicates(orig.clone()));
}

@Test
public void test_1_1_1_2_2_3_3_3() {
    int[] orig = {1, 1, 1, 2, 2, 3, 3, 3};
    assertArrayEquals(new int[]{1, 2, 3}, removeDuplicates(orig.clone()));
}

@Test
public void test_1_2_3_3_3() {
    int[] orig = {1, 2, 3, 3, 3};
    assertArrayEquals(new int[]{1, 2, 3}, removeDuplicates(orig.clone()));
}

@Test
public void test_1_2_2_2_3() {
    int[] orig = {1, 2, 2, 2, 3};
    assertArrayEquals(new int[]{1, 2, 3}, removeDuplicates(orig.clone()));
}


Automating a WordPress install I do not have a whole lot to do over winter break, so I wrote this little script to automate a Wordpress install (currently can only install once instance) on a fresh Debian server (tested, working with Wheezy). It may be pretty sloppy because it's the first thing I've actually tried, but it's a start I guess. I was not too worried about security with this script, but I tried to handle the passwords as best as possible, and they are not printed out at any time (except in .my.cnf, which gets deleted).

I heard somewhere that it is better to print variables like ${DOCUMENT_ROOT} instead of just $DOCUMENT_ROOT. Are there any other recommended tips like this to make scripts perform better / easier to maintain?

#!/bin/bash
#auto wordpress installer

DOCUMENT_ROOT="/var/www/wordpress"
MYSQL_ROOT_PASS="$(cat /dev/urandom | tr -dc 'a-zA-Z0-9' | fold -w 16 | head -n 1)"

## uses this server email to set up apache's config file
echo "Enter in the email for the server administrator:"
read SERVER_ADMIN_EMAIL

apt-get update
apt-get upgrade

## Set up passwords so mysql-server install doesn't have password prompt
debconf-set-selections <<< "mysql-server mysql-server/root_password password $MYSQL_ROOT_PASS"
debconf-set-selections <<< "mysql-server mysql-server/root_password_again password $MYSQL_ROOT_PASS"

## install the required packages to run
apt-get -y install apache2 install libapache2-mod-php5 install libapache2-mod-auth-mysql install php5-mysql
apt-get -y install mysql-server

## download and extract wordpress
wget http://wordpress.org/latest.tar.gz
tar -xzvf latest.tar.gz

## sets up variables for wordpress installation
MYSQL_DB=wordpress$(echo "$RANDOM")
MYSQL_USER=wordpress$(echo "$RANDOM")
MYSQL_USER_PASS="$(cat /dev/urandom | tr -dc 'a-zA-Z0-9' | fold -w 16 | head -n 1)"

## creates a .my.cnf so you can run mysql from the command line without password prompt
printf "[mysql]\nuser=root\npassword=\""$MYSQL_ROOT_PASS"\"\n" > ~/.my.cnf

## adds a wordpress user with own password and creates database for wordpress
mysql --defaults-file=~/.my.cnf -e "create database $MYSQL_DB; create user "$MYSQL_USER"@localhost; set password for "$MYSQL_USER"@localhost = PASSWORD(\""$MYSQL_USER_PASS"\"); GRANT ALL PRIVILEGES ON "$MYSQL_DB".* TO "$MYSQL_USER"@localhost IDENTIFIED BY '"$MYSQL_USER_PASS"'; flush privileges;"

## removes the .my.cnf file which contains mysql's root password
rm -r ~/.my.cnf

## sets up wordpress to use the newly created user and password
cp ~/wordpress/wp-config-sample.php ~/wordpress/wp-config.php
sed -i s/database_name_here/$MYSQL_DB/ ~/wordpress/wp-config.php
sed -i s/username_here/$MYSQL_USER/ ~/wordpress/wp-config.php
sed -i s/password_here/$MYSQL_USER_PASS/ ~/wordpress/wp-config.php

## puts wordpress in the appropriate place and changes permissions
mv wordpress /var/www/
sudo chown www-data:www-data /var/www/wordpress -R

## configures apache to serve wordpress as the site root
cp /etc/apache2/sites-available/default ./default.bak
sed -i s/webmaster@localhost/$SERVER_ADMIN_EMAIL/ /etc/apache2/sites-available/default
sed -i s@/\var\/www@${DOCUMENT_ROOT}@ /etc/apache2/sites-available/default
service apache2 reload

## removes the password used to do an unattended install of mysql-server
echo PURGE | debconf-communicate mysql-server

## browse to this URL to configure the wordpress install
echo "browse to the url /wp-admin/install.php to configure wordpress"

This line is long and hard to read with many commands jammed inside:

mysql --defaults-file=~/.my.cnf -e "create database $MYSQL_DB; create user "$MYSQL_USER"@localhost; set password for "$MYSQL_USER"@localhost = PASSWORD(\""$MYSQL_USER_PASS"\"); GRANT ALL PRIVILEGES ON "$MYSQL_DB".* TO "$MYSQL_USER"@localhost IDENTIFIED BY '"$MYSQL_USER_PASS"'; flush privileges;"

A more readable way to write this:

cat << EOF | mysql --defaults-file=~/.my.cnf
create database $MYSQL_DB;
create user "$MYSQL_USER"@localhost;
set password for "$MYSQL_USER"@localhost = PASSWORD(\""$MYSQL_USER_PASS"\");
GRANT ALL PRIVILEGES ON "$MYSQL_DB".* TO "$MYSQL_USER"@localhost IDENTIFIED BY '"$MYSQL_USER_PASS"';
flush privileges;
EOF

Drop the -r here, as that's useful for recursively removing directories, but you have a simple file here:

rm -r ~/.my.cnf

It seems the script is designed to setup a single WordPress site per system. It would be useful to extract the logic of conducting a WordPress site, so that you could setup multiple sites per system easily if needed.


Showing a welcome screen once per day I have this method, which first basically shows a Welcome screen to user only once per day. The code I have so far is this

private func showGreetingScreen (tabbar: UITabBarController) {
    let defaults = UserDefaults.standard
    if defaults.object(forKey: "greetingDate") == nil {
        defaults.set(Date(), forKey: "greetingDate")
        presentWelcomeScreen(tabbar: tabbar)
    } else {
        if let greetingDate = defaults.object(forKey: "greetingDate") as? Date {
            let order = Calendar.current.compare(Date(), to: greetingDate, toGranularity: .day)
            if order == .orderedDescending {
                defaults.set(Date(), forKey: "greetingDate")
                presentWelcomeScreen(tabbar: tabbar)
            }
        }
    }
}

Now as you can see first I am checking whether Welcome Screen was shown in past or not and then I am saving current date. Then next time app will load, it will compare current date and the date Welcome Screen was shown and based on that It will show Welcome screen to User.
I have feeling that this code can be refactored more.

There is some code duplication for the cases “greeting date has never been set” and “last greeting was yesterday or earlier.” This can be avoided if you use the nil-coalescing operator ?? to set the last greeting date to a default value in the past:

let defaults = UserDefaults.standard
let lastGreeting = defaults.object(forKey: "greetingDate") as? Date ?? .distantPast
let now = Date()
if Calendar.current.compare(now, to: lastGreeting, toGranularity: .day) == .orderedDescending {
    defaults.set(now, forKey: "greetingDate")
    presentWelcomeScreen(tabbar: tabbar)
}


Javascript inheritance: is my solution correct? I have been developing Javascript for few months, and, as a former Java developer, needed a simple way to perform class inheritance.

My needs are:

having private members not accessible (encapsulation),
to make inheritance simple,
to avoid 'new' keywords bugs,
to let the superclass methods accessible using the this.super property.

The purpose of this request is to indicate me if the solution I developped should be applied into my project or if there are some misbehaviour risks by using it, and if you have remarks on how to enhance it.

My code is below, thank you for your reviews.

/**
* Allows to retrieve the global object (used to test if the 'new' keyword is needed)
*/
function getGlobal(){
    return (function(){
            return this;
    }).call(null);
}

if (typeof Object.extend !== 'function') {
    Object.extend = function (object, superClassConstructor, superConstructorArgs) {
        //Inherits from the super object
        superClassConstructor.apply(object, superConstructorArgs);

        //make this.super allow to access to inherited object original functions
        object.super = {};
        for (var property in object){
            object.super[property] = object[property];
        }
    };
}
//The superclass
function SuperClass(options){
    //Make sure we will not alter global object
    if (this === getGlobal()){
        throw new Error("Constructor cannot be called as a function (new keyword should not be omitted)");
    }
    //Use of jshashtable-2.1:
    var attributes = new Hashtable();

    for (var key in options){
        attributes.put(key, options[key]);
    }

    this.setValue = function(key, value){
        if(value != null){
            attributes.put(key, value);
        }
        else{
            attributes.remove(key);
        }
    };

    this.getValue = function(key){
        return attributes.get(key);
    };
}

/**
* Inheriting class
*/
function ProjectModel(projectId) {
    //Make sure we will not alter global object
    if (this === getGlobal()){
        throw new Error("Constructor cannot be called as a function (new keyword should not be omitted)");
    }
    //extends SuperClass
    Object.extend(this, SuperClass, [{
                                    id:projectId,
                                    'one':1,
                                    'two':"2"
                                    }]);

    this.myType = "ProjectNumber";
    //Overriding a function
    this.getValue = function(key){
        return this.myType + ":" + this.super.getValue(key);
    };
}

var project = new ProjectModel(123);
alert(project.getValue('one')); //this displays 'ProjectNumber:123'

It's generally a bad idea to try writing code in one language as if it were a different language.

Don't fight your tool; use it as it was designed. JavaScript has prototype-based OO rather than class-based OO, so use it like that.

Read "Javascript - the Good Parts" and learn to write actually good JavaScript rather than "Java in JavaScript" (for which other developers working with your code will hate and/or mock you).


Simple random falling object animation in Java I am new to programming but I tried to make my code as readable as possible! Hopefully you can see what it does before you run the program. I did not implement an ups or fps controller, the animation may be fast for some computers and slower for others.

import javax.swing.*;
import java.awt.*;
import java.util.Random;

public class App extends JPanel {

    //changing these values will change the size of the game, while still remaining functional
    //within the size limit specified.
    static int windowWidth = 1300;
    static int windowHeight = 800;

    int randNumb = 0;
    int squareWidth = 25;
    int squareHeight = 25;
    int squareYLocation = -squareHeight;
    boolean numberCreated = false;
    static boolean gameRunning = false;

    //generates a random Y value inside the window for the square to spawn at
    public void generateRandomNumber() {
        Random rand = new Random();
        randNumb = rand.nextInt(windowWidth - squareWidth);
        numberCreated = true;
    }

    //paints a black screen, then paints a rectangle on top of the black screen
    public void paint(Graphics g) {
        g.setColor(Color.black);
        g.fillRect(0, 0, windowWidth, windowHeight);
        g.setColor(Color.BLUE);
        g.fillRect(randNumb, squareYLocation, squareWidth, squareHeight);
    }

    public void update() {

        //calls the generateRandomNumber() method which gives the square a random x value inside the screen
        if (!numberCreated) {
            generateRandomNumber();
        }
        //moves the squares y coordinate towards the bottom of the screen and stops once it hits the bottom
        if (squareYLocation <= windowHeight) {
            squareYLocation++;

            //resets the x and y location to a new position
        } else {
            numberCreated = false;
            squareYLocation = -squareHeight;
        }
    }

    //sets the while loop to true to start the game
    public void start() {
        gameRunning = true;
    }

    public static void main(String[] args) throws InterruptedException {

        App game = new App();
        JFrame frame = new JFrame();
        frame.add(game);
        frame.setVisible(true);
        frame.setSize(windowWidth, windowHeight);
        frame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
        frame.setTitle("Raining Squares");
        frame.setResizable(false);
        frame.setLocationRelativeTo(null);

        game.start();

        //updates square position, repaints square, and slows down update and paint speed.
        while (gameRunning) {
            game.update();
            game.repaint();
            Thread.sleep(1);
        }
    }
}

Here is my list of possible improvements:

Constants should be made final and if possible also private

private static final int WINDOW_WIDTH = 1300;
private static final int WINDOW_HEIGHT = 800;
private static final int SQUARE_WIDTH = 25;
private static final int SQUARE_HEIGHT = 25;

Class members should remain private, if accessible from other classes use setters/getters

private int randNumb = 0;
private int squareYLocation = -SQUARE_HEIGHT;
private boolean numberCreated;
private boolean gameRunning;

'

Limit class methods visibility, if no other class uses them

private void generateRandomNumber() {
    Random rand = new Random();
    randNumb = rand.nextInt(windowWidth - squareWidth);
    numberCreated = true;
}

private void paint(Graphics g) {
    g.setColor(Color.black);
    g.fillRect(0, 0, windowWidth, windowHeight);
    g.setColor(Color.BLUE);
    g.fillRect(randNumb, squareYLocation, squareWidth, squareHeight);
}

Don't use one letter names for object instances, use the full name or give an appropriate name that is easy to understand or reflects something relevant.

private void paint(Graphics graphics) {
    graphics.setColor(Color.black);
    graphics.fillRect(0, 0, windowWidth, windowHeight);
    graphics.setColor(Color.BLUE);
    graphics.fillRect(randNumb, squareYLocation, squareWidth, squareHeight);
}

Consistent use of Whitespace, it ok to add extras white lines but be consistent about it. Eg. update/main have a whiteline, other methods don't.
The naming of numberCreated was a bit confusing, I first thought it was only generated once but after looking through the code I saw that the flag could be set to false again. I would suggest changing to a different name eg invalidateNumber or something else that makes it more clear.
App extends JFrame is not a good name for the class, GameFrame extends JFrame would be better. And even better would be the use of the composition pattern to have an App class. (You can separate your game logic from its View (MVC)).
Commenting style is not really conform the guidelines. Your style for inline comments is ok. But normally method comments are something like this:

 /**
  * Your comment goes here, also use
  * @param for your params
  */
 public void paint(Graphics g) {
     g.setColor(Color.black);
     g.fillRect(0, 0, windowWidth, windowHeight);
     g.setColor(Color.BLUE);
     g.fillRect(randNumb, squareYLocation, squareWidth, squareHeight);
 }


A class for remember me Looking for feedback on correctness and code structure.

Follow up to:

SStorage (remember) - v0

Note that b.c. the user can only change the storage type when logging in, there is no need to copy back and forth between storage types.

/***************************************************************************************************
**SStorage - handles remember option of logging in and namespaces the domain as well
*/
    var SStorage = $A.Class.create({
        Name: 'SStorage',
        A: {

            // storage type
            storage:    null,

            // set the namespace here
            ns:         'arcmarks_',

            // storage key which keeps track of storage type
            indicator:  'arcmarks_h_token'
        },

        // type of storage is set when the user logs in
        // and called below
        setType: function (remember) {
            this.A.storage = remember ? $A.localStorage : $A.sessionStorage;
        },

        // determine type of storage at load MUserAny
        load: function () {
            this.setType($A.localStorage[this.A.indicator]);
        },

        // basic setters and getters with namespacing
        set: function (key, value) {
            this.A.storage[this.A.ns + key] = value;
        },
        get: function (key) {
            return this.A.storage[this.A.ns + key];
        },
        setObj: function (o) {
            $A.someKey(o, function (val, key) {
                this.A.storage[this.A.ns + key] = val;
            }, this);
        },
        getObj: function () {
            var o = {},
                real_key,
                namespace;
            $A.someKey(this.A.storage, function (val, key) {

                // retrieves after the name-space
                real_key = key.slice(this.A.ns.length);

                // retrieves the name-space
                namespace = key.slice(0, this.A.ns.length);
                if (namespace === this.A.ns) {
                    o[real_key] = val;
                }
            }, this);
            return o;
        },

        // clear only namespaced items
        clear: function () {
            var key;
            for (key in this.A.storage) {
                if (key.slice(0, this.A.len) === this.A.ns) {
                    this.A.storage.removeItem(key);
                }
            }
        }
    }, true);

From a once over:

You are a bit overdoing the blank lines in your definition of A
I like the namespace idea, given how you implement getObj and setObj, it would be nice if the caller could set the namespace.
setType -> I get that remember probably means you want to remember info across sessions, hence you choose localStorage and not sessionStorage, still something like persist or persistAcrossSessions could be more informative.
setObj -> does totally not what I would expect it to do, I would expect your code to JSON.stringify() and assign that to a key, however you store each property individually which can make for a very messy retrieval
getObj -> does exactly what I thought i would, give all properties in 1 object, this is very limited functionality. You should consider really consider JSON in your class.
clear -> I would make var key part of the for loop : for (var key in this.A.storage) {
Commenting is uneven, the 2 functions which are most surprising to me have no comments.


Hardware Clock Divider I have created a clock divider that works as follows:

If the division factor is 0, pass the clock through unchanged.
Otherwise, flip the output clock signal after the specified number of input clock cycles has passed.

I am relatively new to hardware design, so I would like advice on general best practices that I may have violated, as well as whether there would be any robustness issues. (I know that the output clock will be delayed since it passes through gates, but I don't think this is a problem as long as downstream logic does not attempt to use both clocks simultaneously.)

library IEEE;
use IEEE.STD_LOGIC_1164.ALL;
use IEEE.NUMERIC_STD.ALL;

entity config_clk_div is
    Port ( clk : in STD_LOGIC;
           divfactor : in STD_LOGIC_VECTOR (15 downto 0);
           clkout : out STD_LOGIC);
end config_clk_div;

architecture Behavioral of config_clk_div is
    signal clk_counter : UNSIGNED(15 downto 0);
    signal current_clk : STD_LOGIC;
begin
    process (clk, divfactor) is
    begin
        if divfactor = x"0000" then
            clkout <= clk;
            current_clk <= clk;
        else
            if rising_edge(clk) then
                clk_counter <= clk_counter + 1;
                if clk_counter >= unsigned(divfactor) then
                    clk_counter <= to_unsigned(1,16);
                    current_clk <= not current_clk;
                    -- Not here as well to avoid lag
                    clkout <= not current_clk;
                else
                    clkout <= current_clk;
                end if;
            end if;
        end if;
    end process;

end Behavioral;

Yes, there are some issues.

With process (clk, divfactor) you're making it difficult for the synthesis software to generate a nice clocked process. So you should separate the clocked process from the clock multiplexer.

A bigger problem is the clock multiplexer. divfactor is — even if it is synchronous — a 16-bit value, for which all bits have to be checked for 0. This introduces quite some logic, which will always be glitchy... and you're putting it directly into your clock output. danger! danger! A glitchy clock results in undefined behavior... it can even lock your FSMs to an unresolvable state.

Instead check the condition in a clocked process and set a single synchronous output based on its value.

This is how I would write something like this:

library ieee;
use ieee.std_logic_1164.all;

entity config_clk_div is
    port (
        clk : in std_logic;
        divfactor : in std_logic_vector (15 downto 0); -- Note this is actually not a factor!
        clkout : out std_logic);
end entity;

architecture rtl of config_clk_div is
    use ieee.numeric_std.all;
    signal clk_counter : unsigned(15 downto 0) := (others => '0');
    signal div_clk : std_logic := '1';

    signal divider_disabled : std_logic := '0';
begin
    process (clk)
    begin
        if rising_edge(clk) then
            if divfactor = x"0000" then
                divider_disabled <= '1';
            else
                divider_disabled <= '0';
                if clk_counter = 0 then
                    clk_counter <= unsigned(divfactor) - 1;
                    div_clk <= not div_clk;
                else
                    clk_counter <= clk_counter - 1;
                end if;
            end if;
        end if;
    end process;

    clkout <= clk when divider_disabled = '1' else div_clk;
end architecture;

Note: counting down on a counter makes the compare (to 0) a bit easier. Although this makes more of a difference when designing ASICs.


Print odd and even numbers using two threads in Java I was asked below question in an interview where I need to print out even and odd numbers using two threads so I came up with below code one without synchronization and other with synchronization.

/**
 * Without synchronization
 *
 */
class Print {
  private static final int MAX = 10;
  private static int count = 1;
  private boolean isOdd = true;

  public void printEven() {
    while (true) {
      if (count > MAX)
        break;
      if (!isOdd) {
        System.err.println(Thread.currentThread().getName() + " : " + count++);
        isOdd = true;
      }
    }
  }

  public void printOdd() {
    while (true) {
      if (count > MAX)
        break;
      if (isOdd) {
        System.err.println(Thread.currentThread().getName() + " : " + count++);
        isOdd = false;
      }
    }
  }
}

/**
 * Using synchronization
 *
 */
class Print2 {
  private static final int MAX = 10;
  private static int count = 1;
  private Object obj = new Object();

  public void printEven() {
    while (true) {
      if (count > MAX)
        break;
      synchronized (obj) {
        System.err.println(Thread.currentThread().getName() + " : " + count++);
        obj.notify();
        try {
          obj.wait();
        } catch (InterruptedException e) {
        }
      }
    }
  }

  public void printOdd() {
    while (true) {
      if (count > MAX)
        break;
      synchronized (obj) {
        System.err.println(Thread.currentThread().getName() + " : " + count++);
        obj.notify();
        try {
          obj.wait();
        } catch (InterruptedException e) {
        }
      }
    }
  }
}

public class PrintEvenOddTester {
  public static void main(String[] args) {
    Print p = new Print();
    Thread t1 = new Thread(() -> p.printEven());
    t1.setName("EVEN");
    Thread t2 = new Thread(() -> p.printOdd());
    t2.setName("ODD");

    t1.start();
    t2.start();
  }
}

I wanted to check whether there is any better or efficient way to do these kind of tasks? I ran both the code and it works fine so wanted to check if everything looks good from thread safety and synchronization perspective.

Since in my above code, I am using while(true) loop which will take some CPU's so I am not sure whether this is the best way to do it?

In Print both variables count and isOdd should be marked as volatile then it would be thread safe, but not very efficient.

In Print2 you assume that no one else could wake up your thread which is incorrect as thread could be awaked by many things - in such light you need to have additional if check there to check if it really should print the variable - like this isOdd in the Print. Having this wait/notify with a volatile isOdd check would be a solution that is both tread safe and efficient by not spinning the loops unnecessarily, count should also be volatile as it is shared beetwen threads.

If you want to learn java concurrency well you can read a book "Java Concurrency in Practice" - it will give you a good understanding of this subject and will allow you to pass inteview questions like this one.

You can ask here or on SO for specific things about java concurrency, but I think that it would be hard and inefficient for you to learn it this way. Much better (faster, less painfull)  approach would be to read this book first and then ask questions if something will stay unclear for you. Good wishes!