Simple LRU cache with an expiration time How can I improve the performance of the code below?

package utils.session;

import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.TimeUnit;

/**
 * <p>
 * A very simple LRU cache implementation, used for our server side session
 * management.
 * </p>
 *
 * @author royguo@abcdefg.com
 */
public class SessionCache {
  private static ConcurrentHashMap<String, Entry> cache = new ConcurrentHashMap<>();

  static class Entry {
    private final Object value;
    private final Long expiration;
    private Long lastUseTime;

    public Entry(Object value, Long expiration) {
      this.value = value;
      this.expiration = expiration;
      this.lastUseTime = System.currentTimeMillis();
    }

    public boolean isExpired() {
      return (lastUseTime + expiration) < System.currentTimeMillis();
    }

    public Object getValue() {
      return this.value;
    }

    public void updateUseTime() {
      this.lastUseTime = System.currentTimeMillis();
    }
  }

  static {
    /**
     * Scan our LRU cache every 30 minutes, remove expired keys.
     */
    new Thread(new Runnable() {
      @Override
      public void run() {
        try {
          while (true) {
            for (String key : cache.keySet()) {
              if (cache.get(key).isExpired())
                cache.remove(key);
            }
            Thread.sleep(TimeUnit.MINUTES.toMillis(30));
          }
        } catch (Exception e) {
          e.printStackTrace();
        }
      }
    }).start();
  }

  /**
   * Put a new object into cache, and speicify its expiration time. Everytime we
   * get the object from cache, its expiration time would be updated.
   */
  public static void put(String key, Object value, Long expiration) {
    cache.put(key, new Entry(value, expiration));
  }

  /**
   * Get instance from cache, no need to be thread-safe.
   */
  public static Object get(String key) {
    Entry entry = cache.get(key);
    if (!entry.isExpired()) {
      entry.updateUseTime();
      return entry.getValue();
    } else {
      cache.remove(key);
      return null;
    }
  }
}

I still don't see how it is LRU, but that's beside the point.

One obvious problem is a race between testing for cache.get(key).isExpired() and cache.remove(key) on the next line. It is very well possible for another thread to preempt right in between just to get() that object.

A thread performing get() would believe that it updated useTime, yet the entry would have been removed.

Depending on the use case, the consequences could be catastrophic, or harmless.


Creating User objects I'm creating a PHP object with an array of data:

$row = array(
  'id' => 123,
  'username' => 'Testing',
  'status' => 1,
  'created_on' => '2014-09-20',
  'created_by' => 1,
  'updated_on' => null,
  'updated_by' => null,
);

and I create a corresponding object using the following code:

$model = new User();
$model->id = (int)$row['id'];
$model->username = $row['username'];
$model->status = (int)$row['status'];
if($row['created_on'] != null) {
  $model->created_on = new DateTime($row['created_on']);
}
$model->created_by = (int)$row['created_by'];
if($row['updated_on'] != null) {
  $model->updated_on = new DateTime($row['updated_on']);
}
if($row['updated_by'] != null) {
  $model->updated_by = (int)$row['updated_by'];
}

Is there any way to simplify it?

P.S. I'm not using any PHP framework and I'm not going to use one.

Note: I'm using PHP 5.5

UPDATE: My current simplified version:

$model = new User();
foreach($row as $k => $v) {
  if(isset($model->$k)) {
    if(is_int($v)) {
      $model->$k = (int)$v;
    } elseif(is_string($v)) {
      if(($k == 'created_on' || $k == 'updated_on') && $v != null) {
        $model->$k = new DateTime($v);
      } else {
        $model->$k = $v;
      }
    }
  }
}

Am I making the codes more complicated?

The reason I made the 2nd version is that I worried the 1st version expands if the $row expands.

I don't think there is much to simplify with your code.

Used short array syntax, since you're running PHP 5.5.
Renamed $model to $user: It increases readability + it's one less character.
If-statement syntactical sugar brackets removed.

Code:

$row = [
  'id'         => 123,
  'username'   => 'Testing',
  'status'     => 1,
  'created_on' => '2014-09-20',
  'created_by' => 1,
  'updated_on' => null,
  'updated_by' => null
];

$user             = new User();
$user->id         = (int) $row['id'];
$user->username   = $row['username'];
$user->status     = (int) $row['status'];
$user->created_by = (int) $row['created_by'];

if ($row['created_on']) $user->created_on = new DateTime($row['created_on']);
if ($row['updated_on']) $user->updated_on = new DateTime($row['updated_on']);
if ($row['updated_by']) $user->updated_by = (int) $row['updated_by'];

I have the impression that your User class is just a container and everybody can edit its properties directly. You might wanna enforce some encapsulation here by using setters and getters. This way you can hide its internal properties and have room to validate any values being set and make sure a User object can never reach a state it wasn't meant to be in.

About your update:

  Am I making the codes more complicated?

Yes! Do you really want to sacrifice all this readability just to get a more "simplified" solution? Really, it ends up over-complicating the solution instead.

Remember that if your code is made out of a bunch of complex and over-complicated logic  because you wanted to "simplify" things, only a computer would understand that with no problem. In a week when you come back to look at your code, you will not be able to understand what's happening at a glance. It will take you a minute, and more often then not it makes you forget how the piece of code you're trying to understand fits in the bigger picture.

Martin Fowler said: "Any fool can write code that a computer can understand. Good programmers write code that humans can understand". And that goes especially if you work in a team. You should value readability over simplified code that over-complicates the solution. It will save you (and your team) a lot of time now and in the future.

  The reason I made the 2nd version is that I worried the 1st version expands if the $row expands.

It's completely acceptable that if your user table gets a new usable column that you would also need to add an extra setter in your code for it. Generally it's something that would rarely happen often.

The solution in your post isn't even complete in regards to your concern. For example, what if you decided to update your table with a column modified_on? You still would have to append || $k == 'modified_on' to the if-statement expression and add the class property for this new column.


Measuring the time for the bubble sort algorithm I created a program that measures the time for the bubble sort algorithm to sort a list of numbers:

#include <iostream>
#include <fstream>
#include <vector>
#include <time.h>

std::vector<int> FiletoVector()
{
    std::ifstream SortFile("Number_Sort.input");

    std::vector<int> vector;
    int read;

    while(SortFile >> read)
    {
        vector.push_back(read);
    }

    return vector;
}

void PrintTime(std::string sorttype, std::vector<int> funcinput(std::vector<int>), std::vector<int> vecinput)
{
    //Start Time
    float time = clock();

    funcinput(vecinput);

    //End Time
    time = (clock() - time) / CLOCKS_PER_SEC;

    std::cout << sorttype << " took " << time << " seconds!" << std::endl;
}

std::vector<int> BubbleSort(std::vector<int> vecinput)
{
    for(int i = 0; i < vecinput.size(); i++)
    {
        for(int n = 0; n < vecinput.size(); n++)
        {
            //std::cout << vecinput[n] << " " << vecinput[n + 1] << std::endl;
            if(vecinput[n] > vecinput[n + 1])
            {
                int a = vecinput[n];
                int b = vecinput[n + 1];

                vecinput[n] = b;
                vecinput[n + 1] = a;
            }
        }
    }

    return vecinput;
}

int main()
{
    std::vector<int> nums = FiletoVector();

    //REMEMBER FUNCTION IN PARAMETER HAS NO ()
    PrintTime("Bubble Sort", BubbleSort, nums);

    /*
    //Print Ordered List and Time (Bubble)
    for(int i = 0; i < vecBubble.size(); i++)
    {
        std::cout << vecBubble[i] << std::endl;
    }
    std::cout << time << " seconds!" << std::endl;
    */
}

As you can see the first function is to take the file (Number_Sort.input) and make it into a vector (since it can change size in elements). Then my second function is prints out the time it takes for the sorting algorithm to sort all of its data. Finally my last function is the bubble sort algorithm.

The reason I put my code in this fashion is that so I can later on learn other algorithms and put them in here, and then compare the speed that they execute them with different data sets.

But my code feels a bit messy, I feel like I can further make it more readable, specifically the PrintTime function and all of those parameters. I am not even sure if I did the funcinput(std::vector<int>) in a proper way. I had to make the input of the bubble sort function as a separator parameter (I could not do PrintTime("Bubble Sort", BubbleSort(nums)). And all of that std::vector<int> is gonna kill me! Is there any way to clean that mess too?

Some tips:

std::vector<int> FiletoVector()
{
    std::ifstream SortFile("Number_Sort.input");

    std::vector<int> vector;
    int read;

    while(SortFile >> read)
    {
        vector.push_back(read);
    }

    return vector;
}

Perfectly good readable code. But you can simplify it a bit using iterators.

The standard contains an input iterator that reads values from a stream.

`std::istream_iterator<T>`  // can be used to iterate through a stream.

Since most C++ loops are based on iterators lets update your code to use an iterator based approach.

std::vector<int> FiletoVector()
{
    std::ifstream SortFile("Number_Sort.input");

    std::vector<int> vector;

    std::istream_iterator<int>  fileIter(SortFile)
    for(; fileIter != std::istream_iterator<int>(); ++fileIter)
    {
        vector.push_back(*fileIter);
    }

    return vector;
}

But if you look at the constructor of vector you will see that you can constructor the vector using two iterators. So we can simplify the above code too:

std::vector<int> FiletoVector()
{
    std::ifstream SortFile("Number_Sort.input");

    std::vector<int> vector(std::istream_iterator<int>(SortFile),
                            std::istream_iterator<int>());

    return vector;
}

We can take this one step further as we don't need to have a local member. Just return the value:

std::vector<int> FiletoVector()
{
    std::ifstream SortFile("Number_Sort.input");

    return std::vector<int>(std::istream_iterator<int>(SortFile),
                            std::istream_iterator<int>());
}

Lets look at the timming function next:

void PrintTime(std::string sorttype, std::vector<int> funcinput(std::vector<int>), std::vector<int> vecinput)
{
    // Note `sortType` is passed by value (so you get a copy)
    //      funcinput  has to be a function that takes a vector of int
    //                 that seems overly limiting.
    //      vecinput   Again you pass by value (so you get a copy)
    //                 This is probably on purpose so this is fine.
    //                 But be careful.
    //

    //Start Time
    float time = clock();

    funcinput(vecinput);

    //End Time
    time = (clock() - time) / CLOCKS_PER_SEC;

    std::cout << sorttype << " took " << time << " seconds!" << std::endl;
}

That all seems perfectly reasonable. But you are limiting yourself to sorting functions. I don't see the need for that. You should be able to time any function.

So just let your users pass in any function. Then you can specialize the function using a lambda later.

template<typename Action>
void PrintTime(std::string const& message, Action action)
{
    //Start Time
    float time = clock();

    action();

    //End Time
    time = (clock() - time) / CLOCKS_PER_SEC;

    std::cout << message << " took " << time << " seconds!\n";
}

Now you can call the function like this:

 PrintTime("Bubble Sort", [num](){BubbleSort(nums);});

So lets look at your sorting.

The first thing I notice is that you return a vector. This implies you are copying the vector at some point. Usually you sort in place for optimum performance.

You also pass by value. Which gets you that copy.

std::vector<int> BubbleSort(std::vector<int> vecinput)

Personally I would pass by reference to avoid a copy and then sort in place which means returning nothing.

void BubbleSort(std::vector<int>& vecinput)
^^^^                            ^

Secondly by specifying std::vector<int> you are limiting yourself to only sorting vectors of int. That are lots of things than can be sorted. So you should generalize this to sort any type.

template<typename T>
void BubbleSort(std::vector<T>& vecinput)

You can take this a step further by abstract the container type by allowing the user to pass in iterators.

template<typename I>
void BubbleSort(I begin, I end)

This is how the standard works. You have a bunch of containers and a bunch of algorithms. That glue that holds it all together are iterators. So you can perform nearly any algorithm on any container by passing iterators into the algorithm.

This allows you to sort vectors:

std::vector<int>   x = Load();
BubbleSort(std::begin(x), std::end(x));

But now you can also sort C-Arrays.

int  y[] = { 6,7,10,7,2,3,10};
BubbleSort(std::begin(y), std::end(y));

Three notes on your implementation of bubble sort

The best case version of bubble sort is O(n). You only need to do a single pass over the container if it is already sorted. So you should track if there was a swap on a given pass and exit early if no changes were made.
Secondly both loops go from [0..size). The inner loop does not need to cover the whole container. Each pass you have moved at least one item all the way to the correct location. So the next pass you don't need to check that item.
You do some manual swapping. There is a built in std::swap() in the standard that allows you to swap to object optimally.

This gives you something like this:

   bool noswap = false;
   for(int i = 0; noswap && i < vecinput.size(); i++)
   {
        noswap = true;
        for(int n = 0; n < vecinput.size() - i - 1; n++)
        {
            if(vecinput[n] > vecinput[n + 1])
            {
                std::swap(vecinput[n], vecinput[n + 1])
                noswap = false;
            }
        }
   }


Help tidying up a Draw call Simple static class that will write out a message that is passed in.  The SpriteSheet is 256x256 and the A-Z starts at line 240 and the 0-9 starts at 248.  Each character is 8x8.  I hate the if (ix >= 32) and wondered if there is a tidier way of doing this?  I'm pleased it's only one Draw call, but it looks ugly.

public static class Font
{
    private static String chars = "" + //
        "ABCDEFGHIJKLMNOPQRSTUVWXYZ      " + //
        "0123456789.,!?'\"-+=/\\%()<>:;     " + //
        "";

    public static void Draw(string msg, SpriteSheet sprites, SpriteBatch spriteBatch, int x, int y, Color color)
    {
        msg = msg.ToUpper();
        for (int i = 0; i < msg.Length; i++)
        {
            int ix = chars.IndexOf(msg[i]);
            int w = 0, h = 240;
            if (ix >= 0)
            {
                if (ix >= 32)
                {
                    w = 32;
                    h = 248;
                }
                spriteBatch.Draw(sprites.Sheet, new Rectangle(x + i * 8, y, 8, 8), new Rectangle((ix - w) * 8, h, 8, 8), color);
            }
        }
    }
}

It is hard to suggest improvements because the variable names are not very meaningful and, thus, it is hard to understand what they are for and which is the intention behind the code.
Lesson to learn: using meaningful variable names is important.

Here is an attempt.
"Opening brackets not taking a new line" is a matter of style, disregard it.

Could you address the code comments I left?

public static class Font
{
    private const string chars =
        "ABCDEFGHIJKLMNOPQRSTUVWXYZ      " + // I assume the spaces are padding to 32 chars.
        "0123456789.,!?'\"-+=/\\%()<>:;     " // 35chars?!? What is the use of the 5 spaces?
        ;
    // Since the sprites never change, you can have them as a static property.
    private static SpriteSheet _Sprites = ...;

    public static void Draw(string msg, SpriteBatch spriteBatch, int xOffset, int yOffset, Color color) {
        msg = msg.ToUpper();
        for (int x = 0; x < msg.Length; x++) {
            int spriteX = chars.IndexOf(msg[x]);
            if (spriteX < 0) {
                continue; // Skipping char.
            }
            int spriteY = 240;
            if (spriteX >= 32) {
                // I really do not understand the justification of this...
                // Drawing a 1 is supposed to get a B, right? Strange.
                spriteX -= 32;
                spriteY = 248;
            }
            Rectangle charMask = new Rectangle(xOffset + x * 8, yOffset, 8, 8);
            Rectangle spriteMask = new Rectangle(spriteX * 8, spriteY, 8, 8);
            // XNA method: http://msdn.microsoft.com/en-us/library/ff433987.aspx
            spriteBatch.Draw(Sprites.Sheet, charMask, spriteMask, color);
        }
    }
}


Tower of Hanoi Recursive Implementation using Python with OOP I have just finished a small program for Tower of Hanoi using recursion. I did it to practice OOP which I recently learnt. It took me about 2 hours to research about the recursive algorithm and write out the code. It works, after much debugging... However, after comparing to other implementations, I realised my code is rather long, plus unconventional when it comes to how I approach and execute as well as the naming conventions.

Could I have some feedback about the program? Thank you!

class rod:
    move_count = 0

    def __init__(self,disks=[],position=""):
        self.diskslist=disks
        self.rod_position=position
    # Remove the top disk of a rod
    def remove_top(self):
        print(f"Move the disk of size {self.diskslist[0].size} from {self.rod_position} Rod ", end="")
        rod.move_count += 1
        return self.diskslist.pop(0)
    # Add a disk to the top of a rod. Only allows in two conditions a. No disks currently on that rod b. Disk smaller than the disk below it
    def add_to_top(self,current_disk,announce=True):
        if len(self.diskslist) == 0:
            if announce==True:
                print(f"on to the top of the {self.rod_position} Rod", end="\n")
            self.diskslist.insert(0,current_disk)
            return True
        else:
            if current_disk.size < self.diskslist[-1].size:
                if announce == True:
                    print(f"on to the top of the {self.rod_position} Rod", end="\n")
                self.diskslist.insert(0,current_disk)
                return True
            else:
                return False

class disk:
    num=0
    def __init__(self,size):
        self.size=size
        disk.num += 1

#Generating 8 disks of increasing size
disk_num=8
disks = []
for i in range(disk_num):
    disks.append(disk(i))

#Generating 3 rods
rods = []
rods.append(rod([],"Left"))
rods.append(rod([],"Middle"))
rods.append(rod([],"Right"))

# Attaching all disks to the left rod
for i in range(len(disks)):
    rods[0].add_to_top(disks[len(disks)-i-1],False)

def recursive_solve(current_rod, lower_range, target_rod):
    # If only moving the top disk, execute it
    if lower_range == 0:
        rods[target_rod].add_to_top(rods[current_rod].remove_top())
    # If not, keeping simplifying the recursion.
    else:
        alt_rod = 3 - current_rod - target_rod
        recursive_solve(current_rod, lower_range - 1, alt_rod)
        recursive_solve(current_rod, 0, target_rod)
        recursive_solve(alt_rod, lower_range - 1, target_rod)

print(f"The steps needed to move {disk_num} pieces of disks are:")
recursive_solve(0,len(disks)-1,2)

for i in range(len(rods)):
    print(f'\n For rod number {i+1}:',end=" ")
    for j in range(len(rods[i].diskslist)):
        print(rods[i].diskslist[j].size,end=" ")

print(f'\n The number of moves taken in total is {rod.move_count}')

Your code seems to be working and trying to use OOP is a nice touch.
Let's see what can be improved.

Style

There is an official standard Python style guide called PEP 8. This is a highly recommended reading. It gives guidelines to help writing code that is both readable and consistent. The Python community tries to follow these guidelines, more or less strictly (a key aspect of PEP 8 is that it provides guidelines and not strict rules to follow blindly).

It deals with various aspects of the code style: naming conventions, indentation convention, etc.

You'll find various tools to try to check whether your code is PEP 8 compliant and if is it not, to try and fix this:

pycodestyle package (formerly known as pep8) to check you code
pep8online to check your code with an online tool
autopep8 package to fix your code automatically
Also, this is also checked by various linters: pylint, pyflakes, flake8, etc.
There also a code formatter getting very popular in the Python communinity called Black.

In your case, various things can be modified: class names should start with an uppercase letter, whitespace can be improved, etc.

Docstring

There is also a document describing Docstring conventions called PEP 257.

The comments you've written on top of the functions definition could be used as the function docstring.

The Disk class

It seems like num is not needed. Thus, the whole class could be written:

class Disk:
    def __init__(self, size):
        self.size = size

Ultimately, we could try to see if such a class is needed at all.

Building disks

disk_num could be written in uppercase as it is a constant.

disks could be initialised using list comprehension:

# Generating disks of increasing size
DISK_NUM = 8
disks = [Disk(i) for i in range(DISK_NUM)]

Building rods

You could use keyword arguments to provide only the position value without the need to provide the disks value (and thus, rely on the default behavior). Another option could be to change the argument order to have the position first - by the way, maybe "name" would be better than "position" as it conveys better the fact that it is not an integer used for computation but just a string just for logging.

rods.append(Rod(position="Left"))
rods.append(Rod(position="Middle"))
rods.append(Rod(position="Right"))

We could once again use list comprehension to define the rods here:

# Generating rods
rods = [Rod(position=p) for p in ("Left", "Middle", "Right")]

Comparison to boolean

You don't need to write: if announce==True, you can simply write: if announce.

Rewrite logic in add_to_top

You could use elif to make the various cases easier to identify and remove a level of indentation in the second part of the function.

def add_to_top(self,current_disk,announce=True):
    """Add a disk to the top of a rod. Only allows in two conditions a. No disks currently on that rod b. Disk smaller than the disk below it."""
    if len(self.diskslist) == 0:
        if announce:
            print(f"on to the top of the {self.rod_position} Rod", end="\n")
        self.diskslist.insert(0,current_disk)
        return True
    elif current_disk.size < self.diskslist[-1].size:
        if announce:
            print(f"on to the top of the {self.rod_position} Rod", end="\n")
        self.diskslist.insert(0,current_disk)
        return True
    else:
        return False

It is now clearer that the 2 first cases are identical and can be merged:

def add_to_top(self,current_disk,announce=True):
    """Add a disk to the top of a rod. Only allows in two conditions a. No disks currently on that rod b. Disk smaller than the disk below it."""
    if (len(self.diskslist) == 0) or (current_disk.size < self.diskslist[-1].size):
        if announce:
            print(f"on to the top of the {self.rod_position} Rod", end="\n")
        self.diskslist.insert(0,current_disk)
        return True
    else:
        return False

Also, maybe you could throw an exception when the disk size is invalid instead of just returning False:

def add_to_top(self,current_disk,announce=True):
    """Add a disk to the top of a rod. Only allows in two conditions a. No disks currently on that rod b. Disk smaller than the disk below it."""
    if self.diskslist and current_disk.size >= self.diskslist[-1].size:
        raise Exception("Invalid disk size")

    if announce:
        print(f"on to the top of the {self.rod_position} Rod", end="\n")
    self.diskslist.insert(0,current_disk)

Reconsider the Rod initialisation

A disk list can be provided to the Rod initialisation.

Two things are a bit awkward here:

you are using Mutable Default Arguments which is a common Gotcha in Python
the provided list may be unsorted: if we really want to provide the list, maybe we should pass it through the add_to_top logic element by element or just check that it is sorted.

It may be easier to weaken the API by getting rid of it and default to [].

def __init__(self, name=""):
    self.diskslist = []
    self.rod_name = name

At this stage, the code looks like:

class Rod:
    move_count = 0

    def __init__(self, name=""):
        self.diskslist = []
        self.rod_name = name

    def remove_top(self):
        """Remove the top disk of a rod."""
        print(f"Move the disk of size {self.diskslist[0].size} from {self.rod_name} Rod ", end="")
        Rod.move_count += 1
        return self.diskslist.pop(0)

    def add_to_top(self,current_disk,announce=True):
        """Add a disk to the top of a rod. Only allows in two conditions a. No disks currently on that rod b. Disk smaller than the disk below it."""
        if self.diskslist and current_disk.size >= self.diskslist[-1].size:
            raise Exception("Invalid disk size")

        if announce:
            print(f"on to the top of the {self.rod_name} Rod", end="\n")
        self.diskslist.insert(0,current_disk)

class Disk:
    def __init__(self, size):
        self.size = size

def recursive_solve(current_rod, lower_range, target_rod):
    # If only moving the top disk, execute it
    if lower_range == 0:
        rods[target_rod].add_to_top(rods[current_rod].remove_top())
    # If not, keeping simplifying the recursion.
    else:
        alt_rod = 3 - current_rod - target_rod
        recursive_solve(current_rod, lower_range - 1, alt_rod)
        recursive_solve(current_rod, 0, target_rod)
        recursive_solve(alt_rod, lower_range - 1, target_rod)

# Generating disks of increasing size
DISK_NUM = 8
disks = [Disk(i) for i in range(DISK_NUM)]

# Generating rods
rods = [Rod(name=p) for p in ("Left", "Middle", "Right")]

# Attaching all disks to the left rod
for i in range(len(disks)):
    rods[0].add_to_top(disks[len(disks)-i-1],False)

print(f"The steps needed to move {DISK_NUM} pieces of disks are:")
recursive_solve(0,len(disks)-1,2)

for i in range(len(rods)):
    print(f'\n For rod number {i+1}:',end=" ")
    for j in range(len(rods[i].diskslist)):
        print(rods[i].diskslist[j].size,end=" ")

print(f'\n The number of moves taken in total is {Rod.move_count}')

Loop with the proper tools

I highly recommend Ned Batchelder's talk "Loop like a native". Basically, anytime you use something like for xxx in range(len(yyy)), there is a better way to do it. In your case, enumerate can help you as well to write:

# Attaching all disks to the left rod
for d in reversed(disks):
    rods[0].add_to_top(d, False)

print(f"The steps needed to move {DISK_NUM} pieces of disks are:")
recursive_solve(0,len(disks)-1,2)

for i, rod in enumerate(rods):
    print(f'\n For rod number {i+1}:',end=" ")
    for d in rod.diskslist:
        print(d.size,end=" ")

Also, we could consider that we can refer to rod by their name instead of their positions and thus get rid of positions altogether:

for rod in rods:
    print(f'\n For rod {rod.rod_name}:',end=" ")
    for d in rod.diskslist:
        print(d.size,end=" ")

The recursive_solve function

The fact that you access rods from the recursive_solve function seems a bit weird to me. It may be easier to provide the 3 rods to the function.

Also, providing the length of disks should not be necessary: in the best possible world, we could just rely on the number of disks on the starting rod.

I've performed a few changes to handle this, using an intermediate function:

def recursive_solve(start_rod, end_rod, third_rod, nb_disk_to_move):
    if nb_disk_to_move == 1:
        end_rod.add_to_top(start_rod.remove_top())
    # If not, keeping simplifying the recursion.
    else:
        recursive_solve(start_rod, third_rod, end_rod, nb_disk_to_move - 1)
        recursive_solve(start_rod, end_rod, third_rod, 1)
        recursive_solve(third_rod, end_rod, start_rod, nb_disk_to_move - 1)

def move(start_rod, end_rod, third_rod):
    nb_disk = len(start_rod.diskslist)
    print(f"The steps needed to move {nb_disk} pieces of disks are:")
    recursive_solve(start_rod, end_rod, third_rod, nb_disk)

...
move(rods[0], rods[2], rods[1])

Also, that removes the need for the disks variable:

# Attaching all disks to the left rod
for i in reversed(range(DISK_NUM)):
    rods[0].add_to_top(Disk(i), False)

move(rods[0], rods[2], rods[1])

Finally, the function can be slightly simplified:

def recursive_solve(start_rod, end_rod, third_rod, nb_disk_to_move):
    if nb_disk_to_move:
        recursive_solve(start_rod, third_rod, end_rod, nb_disk_to_move - 1)
        end_rod.add_to_top(start_rod.remove_top())
        recursive_solve(third_rod, end_rod, start_rod, nb_disk_to_move - 1)

More OOP

One of the principle of OOP is the encapsulation. In our case, disklist could be considered as an implementation detail which is best not to rely on. In order to print the content on a rod, we could write a __str__ magic method. Then, the calling code becomes much simpler:

    def __str__(self):
        return self.rod_name + ": " + " ".join(str(d.size) for d in self.diskslist)

for rod in rods:
    print(f' For rod {rod}')

At this stage, the code looks like:

class Rod:
    move_count = 0

    def __init__(self, name):
        self.diskslist = []
        self.rod_name = name

    def remove_top(self):
        """Remove the top disk of a rod."""
        print(f"Move the disk of size {self.diskslist[0].size} from {self.rod_name} Rod ", end="")
        Rod.move_count += 1
        return self.diskslist.pop(0)

    def add_to_top(self,current_disk,announce=True):
        """Add a disk to the top of a rod. Only allows in two conditions a. No disks currently on that rod b. Disk smaller than the disk below it."""
        if self.diskslist and current_disk.size >= self.diskslist[-1].size:
            raise Exception("Invalid disk size")

        if announce:
            print(f"on to the top of the {self.rod_name} Rod", end="\n")
        self.diskslist.insert(0,current_disk)

    def __str__(self):
        return self.rod_name + ": " + " ".join(str(d.size) for d in self.diskslist)

class Disk:
    def __init__(self, size):
        self.size = size

def recursive_solve(start_rod, end_rod, third_rod, nb_disk_to_move):
    if nb_disk_to_move:
        recursive_solve(start_rod, third_rod, end_rod, nb_disk_to_move - 1)
        end_rod.add_to_top(start_rod.remove_top())
        recursive_solve(third_rod, end_rod, start_rod, nb_disk_to_move - 1)

def move(start_rod, end_rod, third_rod):
    nb_disk = len(start_rod.diskslist)
    print(f"The steps needed to move {nb_disk} pieces of disks are:")
    recursive_solve(start_rod, end_rod, third_rod, nb_disk)

# Generating rods
rods = [Rod(name=p) for p in ("Left", "Middle", "Right")]

# Attaching all disks to the left rod
DISK_NUM = 8
for i in reversed(range(DISK_NUM)):
    rods[0].add_to_top(Disk(i), False)

move(rods[0], rods[2], rods[1])

for rod in rods:
    print(f' For rod {rod}')

print(f'\n The number of moves taken in total is {Rod.move_count}')

I have to stop here, I'll try to continue.


Finding unique values in an array with possible nested arrays I'm writing a function that receives an array and returns an object with the count of unique values, pretty much like other questions here or in StackExchange. Except that in my case, this array may contain a nested array, like:

var array = [1, 2, 'a', 'b', [3, 'c', 1], 'd', 2, 'b'];

Here is my code:

function findUniques(array, occurrences) {
    if (!occurrences) {
        occurrences = {};
    }

    //checking if parameter passed is an array
    if (Array.isArray(array)) {
        for (let i = 0; i < array.length; i++) {
            //if item is a nested array, call findUniques again
            if (Array.isArray(array[i])) {
                occurrences = findUniques(array[i], occurrences);
            } else {
                //add to/update count in occurrences object
                occurrences[array[i]] = occurrences[array[i]] + 1 || 1;
            }
        }
    }

    return occurrences;
}

It's returning the following object, as it was supposed to:

{
    '1': 2,
    '2': 2,
    '3': 1,
    a: 1,
    b: 2,
    c: 1,
    d: 1
}

My worry is that I'm using recursion.

Is it possible to optimize this function?

(Bear in mind that I must only use vanilla JavaScript)

When we use recursion, we are building an algorithm based on an implicit1 stack, the function call stack. We are pushing and popping elements, essentially every time we recurse down and back up. If we make the stack explicit in our code, we can make the method iterative instead, and skip the recursive calls.

A pseudocode version of the algorithm would look like:

function findOccurences(array):
    ensure array is actually an array

    set occurrences to empty object

    create a stack
    push all elements in array onto the stack
    while the stack is not empty:
        if pop returns an array:
            push all of the elements in the popped array onto the stack
            continue while

        process the popped element

    return occurrences

A nice feature in Javascript arrays is that they have stack semantics2, so we can just use a built in array as our stack. We can actually get a little more clever than that, though. Since we are putting every element from one array into our stack, which is also an array, we could just as effectively clone the original array and use that as our stack3. We can then just translate the pseudocode into Javascript.

function findOccurrences(array) {
    if(!Array.isArray(array)) {
        return {};
    }

    var occurrences = {};

    // JS arrays don't have a native clone method so
    // Array#slice with a start point of 0 creates
    // a copy of our array. If handling in the same
    // order is important, tack a .reverse() at the
    // end.
    var stack = array.slice(0);

    while(stack.length !== 0) {
        var nextElement = stack.pop();
        if(Array.isArray(nextElement)) {
            // We use some fanciness here so we don't have
            // to write out an explicit loop
            [].push.apply(stack, nextElement);
            continue;
        }

        occurrences[nextElement] = occurrences[nextElement] + 1 || 1;
    }

    return occurrences;
}

1 Implicit to our algorithm, explicit to our runtime.

2 Array#push, Array#pop

3 We could just use the original array, but since it is passed by reference, we are going to corrupt the array in the process and that could lead to unwanted side effects.


Guessing words from scrambled letters How could I possibly shorten / clean this up?  I suppose mainly clean up the loop at the start asking whether they would like to scramble the code.

def userAnswer(letters):
    print("Can you make a word from these letters? "+str(letters)+" :")
    x = input("Would you like to scrample the letters? Enter 1 to scramble or enter to guess :")
    while x == '1':
        print(''.join(random.sample(letters,len(letters))))
        x = input("Would you like to scrample the letters? Enter 1 to scramble or enter to guess :")
    word = input("What is your guess? :")
    word = word.lower()
    if checkSubset(word, letters) == True and checkWord(word) == True:
        print("Yes! You can make a word from those letters!")
    else:
        print("Sorry, you cannot make that word from those letters")

userAnswer("agrsuteod")

Before presenting my version, I would like to make a couple of comments:

Use descriptive names. The name userAnswer gives me an impression of just getting the user's answer, nothing else. I like to suggest using names such as startScrambleGame, runScrambleGame or the like.
Avoid 1-letter names such as x -- it does not tell you much.
I don't know which version of Python you are using, but mine is 2.7 and input() gave me troubles: it thinks my answer is the name of a Python variable or command. I suggest using raw_input() instead.
Your code calls input() 3 times. I suggest calling raw_input() only once in a loop. See code below.
The if checkSubset()... logic should be the same, even if you drop == True.

Here is my version of userAnswer, which I call startScrambleGame:

def startScrambleGame(letters):
    print("Can you make a word from these letters: {}?".format(letters))
    while True:
        answer = raw_input('Enter 1 to scramble, or type a word to guess: ')
        if answer != '1':
            break
        print(''.join(random.sample(letters, len(letters))))

    word = answer.lower()
    if checkSubset(word, letters) and checkWord(word):
        print("Yes! You can make a word from those letters!")
    else:
        print("Sorry, you cannot make that word from those letters")

startScrambleGame("agrsuteod")


Business logic in service method I'm currently writing a small asset tracking system.

The implementation should meet the following requirements:

  The system; scans the transfers every 15 minutes. If the transfer is
  accepted by the receiver by the specified transfer time, realizes the
  transfer. When a transfer is realized, assets are assigned to the
  receiver as of transfer time; if the transfer is permanent, actual
  owner is changed also.

  The system; If the transfer operation is not accepted by the receiver
  by the specified transfer time; cancels the transfer.

Above requirement is dependent on this straight-forward requirement (UI related parts omitted):

  Transferring user; When a new transfer is started,
  [...]
  and specifies

  the time when the transfer will be realized
  whether the transfer is temporary or permanent
  assets to be transferred
  the receiver
  [... and saves ....]

The implementation:

import java.util.Collections;
import java.util.HashSet;
import java.util.Set;

import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

@Transactional
@Service
public class TransferServiceImpl implements TransferService {

    private Repository<Transfer, Long> repository;
    private Repository<Asset, Long> assetRepository;

    @Override
    public void informTransferTimeIsUp(long transferId) {
        Transfer transfer = repository.get(transferId);
        transfer.informTransferTimeIsUp();

        if (transfer.isRealized()) {
            Set<Asset> transferredAssets = assetRepository.getAll(transfer.getTransferredAssets());
            for (Asset asset : transferredAssets) {
                if (TransferType.TEMPORARY.equals(transfer.getTransferType())) {
                    asset.transferTemporarily(transfer.getReceiver());
                } else {
                    asset.transferPermanently(transfer.getReceiver());
                }
                assetRepository.update(asset);
            }
        }

        repository.update(transfer);
    }

}

class Transfer {
    long receiver;
    Date transferTime;
    TransferType transferType;
    Set<Long> transferredAssets = new HashSet<Long>();

    boolean accepted;
    boolean timeIsUp;
    boolean realized;
    boolean cancelled;

    public void informTransferTimeIsUp() {
        this.timeIsUp = true;

        if (accepted) {
            this.realized = true;
        } else {
            this.cancelled = true;
        }
    }

    public long getReceiver() {
        return this.receiver;
    }

    public Set<Long> getTransferredAssets() {
        return Collections.unmodifiableSet(transferredAssets);
    }

    public TransferType getTransferType() {
        return transferType;
    }

    public boolean isRealized() {
        return realized;
    }

}

class Asset {
    Long actualOwnerId;
    Long currentOwnerId;

    boolean blocked;

    public void transferTemporarily(long receiverId) {
        this.currentOwnerId = receiverId;
        this.unblockIfBlocked();
    }

    public void transferPermanently(long receiverId) {
        this.actualOwnerId = receiverId;
        this.currentOwnerId = receiverId;
        this.unblockIfBlocked();
    }

    private void unblockIfBlocked() {
        this.blocked = false;
    }

}

Dependencies to get the above code to compile:

interface TransferService {
    void informTransferTimeIsUp(long transferId);
}

interface Repository<TEntity, TKey> {

    TEntity get(TKey key);

    Set<TEntity> getAll(Set<TKey> keys);

    void update(TEntity asset);

}

enum TransferType {TEMPORARY, PERMANENT}

Is there too much business logic in the service layer?

Set<Asset> transferredAssets = assetRepository.getAll(transfer.getTransferredAssets());
for (Asset asset : transferredAssets) { ... }

Since the transfer isn't done yet, and also to prevent ambiguity post-transfer, may I suggest renaming the method to getAssets()? You can also inline the call:

for (Asset asset : assetRepository.getAll(transfer.getTransferredAssets())) { ... }

Two minor improvements I can suggest for the snippet below:

It's safe to compare enums by ==.
Have a transfer(TransferType, long) method in Asset so that the toggling is done within Asset: callers only need to supply the TransferType value.

if (TransferType.TEMPORARY.equals(transfer.getTransferType())) {
    asset.transferTemporarily(transfer.getReceiver());
} else {
    asset.transferPermanently(transfer.getReceiver());
}

Since the use of realized and cancelled are just opposites of each other, you may want to consider having just a single boolean field, to keep the implementation simpler.

if (accepted) {
    this.realized = true;
} else {
    this.cancelled = true;
}

In Asset:

public void transferTemporarily(long receiverId) {
    this.currentOwnerId = receiverId;
    this.unblockIfBlocked();
}

public void transferPermanently(long receiverId) {
    this.actualOwnerId = receiverId;
    this.currentOwnerId = receiverId;
    this.unblockIfBlocked();
}

This can be slightly simplified as:

public void transferTemporarily(long receiverId) {
    this.currentOwnerId = receiverId;
    this.unblockIfBlocked();
}

public void transferPermanently(long receiverId) {
    transferTemporarily(receiverId);
    this.actualOwnerId = receiverId;
}

Alternatively, if reading temporarily inside a permanent method is slightly confusing, have a new method setCurrentOwnerAndUnblock() to do that instead...


Return true if a portion of one string can be rearranged to create the other string function scramble(str1,str2) {
  let result;
  for(let i = 0; i < str2.length; i++){

    let found = str1.match(new RegExp(str2.charAt(i)));

    if(found === null){
      result = false;
      break;
    } else {
      str1 = str1.replace(found, "");
      result = true;
    }
  }

  return result;
}

The purpose of this function is to verify if the letters of the second string are contained in the first string and can be rescrambled to create the the word:

scramble('rkqodlw', 'world'); //true
scramble("scriptjavx", "javascript"); //false
scramble('katas', 'steak'); //false

The above is working but I am looking to improve the performance time to less than 12000ms for for any given string.

12 seconds
is forever for a CPU. However your solutions is so complex, 12 seconds is likely it timing out on a string that is only a few K long.
These code sites also like to throw curve balls, passing arguments that have very quick solutions if you check for them, but if you don't will take so long it will time you out.
Also know that these sites run distributed virtual machines on which they test code, some (all that I tested) time how long it takes to get a result not how long your code takes to run. If its busy that time will include how long it takes to locate CPU/Memory resource (VM), setup context (Javascript runtime), and then execute your code. So times vary and should never be used as an indication of performance.
Performant and Complexity
The first step to fast solutions is always write performant code,
Basic performant optimisations

Locate opportunities to exit early. In this case, is the first string is shorter than the second then you know the result must be false.

Locate lines of code that are repeated in iteration that can be done once. In this case you set result to true each time you find a match. You need only do it once by assigning true to result when you declare it.

Avoid redundancy. Redundant lines of code take time to execute. In this case result is not needed as you can return the result directly from within the statement found===null, removing the need for break, and defining result.

Develop a good Understanding of available language features and stay up to date. MDN has a good Javascript reference. Knowing this reference in your head will makes your code faster, to know it in your head means you must continuously be studying the reference as JavaScript is always changing.
String.match is not the only way to test for a character. Also it can return an array which is less performant than a bool such as RegExp.test, and String.includes or a Number such as String.indexOf.
Once a year read the Javascript Spec (current ECMAScript2019) Its long and dry, not an easy read for beginners (or 40+ year vets) , so read it again, and again. This will not only reenforce your internal mind javascript reference but give you an understanding of how javascript does what it does. (Help you understand JS complexity. See below)
Test your understanding. Is str1 === str2 faster than str1.length === str2.length ? If you dont know write some code to find out.

A more performant solution
From that you can get a good improvement in overall performance. An example follows
function scramble(str1,str2) {
    var i;
    if (str1.length >= str2.length) {
        if (str1 === str2) { return true }
        for (i = 0; i < str2.length; i++) {
            const str = str1.replace(str2[i]);
            if (str.length === str1.length) { return false }
            str1 = str;
        }
        return true;
    }
    return false;
}

That may pass the time limit but as the size of the strings grows the improvement in performance reduces.
For 2 char strings this a twice as fast. When str1 is shorter than str2 this is a above umteenQuadBillion times faster. If str1 is longer or equal len to str2 then for a 4K string it will likely timeout, taking only a few ms shorter to solve.

Complexity
In computer science there is a difference between performance and complexity. The above code has better performance and hence will always be a little quicker, however it has the same complexity and thus not going to help you.
The metric n
We measure an algorithm's complexity in terms of some metric. In this case the metric can be the mean size of both strings. We call it n and use that to create a formula that counts the number operations per n needed to complete the task.
For example count A's in a string
// Note len is string len
count = 0
for(i = 0; i < len; i++){
    if(str[i] === "A") {
        count ++
    }

}

2 operations to set count and start the loop.

1 op for the loop.

1 op for the test.

if there is an "A" 1 op to count.
We can say that the complexity has a worst (all A's) case of 2 + 3 * n complexity and a best case (No A's) of 2 + 2 * n

The difference between best a worst case can be huge, generally we define what the complexity is for (worst, mean, or best case). Without that we assume the complexity is the worst case
Big O notation.
Formalized we call the expression of complexity big O notation and you will see it as a math function, sometimes O(2 + 3 * n) but most often just the n or a number O(n), O(n2) or O(1).
Complexity grows quickly
Lets up the complexity and count the number of chars in str1 that exist in str2
count = 0
for(i = 0; i < len1; i++){
    for(j = 0; j < len2; j++){
        if(str1[i] === str2[j]) {
            count ++
        }
    }
}

Now n is the mean string len and there are

2 ops to get into the first loop,
for each n there is

1 op to loop and another to enter the inner loop. thus 2 * n.
then for each inner loop

1 to loop, and 2 to count.

The complexity is O(2 + 2 * n + 3 * n * n)
Note that the  there is a n * n or we can express it as n^2 or n2 thus O(2 + 2n + 3n2)
The highest power
The function inside the () is called a polynomial.
There is a property of polynomials that allow for simplification.
As the value of n grows the size of the term that has the highest power n2 gets so large that the other terms (2 + 2n + 3 * ) become insignificant.
With this in mind Big O ignore all but the highest power. So for the above the complexity is not O(2 + 2n + 3n2) but  O(n2)
Javascript complexity.
As part of understanding javascript you need to know the complexity of the many javascript functions and operators.
Some examples
if(a === b)  // if numbers O(1)
if(a === b)  // if strings O(n) where n is string len (it may be a little lower (O(nlogn)
if(a.includes(b)) // if a is array of numbers b is a number O(n) n is size of array
if(a.includes(b)) // if a is array of string b is a string the worst case O(n**2)

So a single JS line can not just be counted as a single operations, each line can have vastly different complexity
So looking at your code the outer loop is O(n) but inside we have str1.match and str1.replace both of which are O(n). So overall its O(n2). The same for the more performant version I wrote. The str1.replace is O(n) thus the solution is O(n2)
Optimise complexity target the highest big O power
So if we can reduce the size of the highest power in a solution, even if that means more code we can get huge improvements in complexity.
One solution is to go over the first string and count the number of times each letter occurs, storing the count in an array, one item for each possible character. Then go over the second array and for each subtract 1 from the counts. If any of the counts are negative we know that the first string is missing a value that is in the second.
function scramble(str1,str2) {
    const buf = new Uint8Array(256);
    var i = 0;
    if (str1.length >= str2.length) {
        if (str1 === str2) { return true }
        while(i < str1.length) {
            buf[str1.charCodeAt(i++)] ++;
        }
        i = 0;
        while(i < str2.length) {
            const idx = str2.charCodeAt(i++);
            if(buf[idx] === 0) { return false }
            buf[idx] --;
        }
        return true;
    }
    return false;
}

If we write out the full complexity its 256 + 3 + 2n + 1 + 4n + 1 or 261 + 6n the 256 is to create and fill the array with zeros. If the strings are unicode it would be 65536 + 5 + 5n
But only the highest power matters so the above function is O(n)
Now if the strings are the same size and the mean length is 4000 this solution should be around 4000 times faster.
Caveats
There are however some caveats for the above solution
The array holding the counts must be able to count all the characters.
To ensure the function will work use an array of 32bit unsigned ints which will handle strings upto 4Gb is size. If the function errors the strings may be unicode and thus you will need to use a larger array 65536 items long.
The O(n) solution.
function scramble(str1, str2) {
    const buf = new Uint32Array(256);  // For unicode array size is 65536
    var i = 0;
    if (str1.length >= str2.length) {
        if (str1 === str2) { return true }
        while (i < str1.length) {
            buf[str1.charCodeAt(i++)] ++;
        }
        i = 0;
        while (i < str2.length) {
            const idx = str2.charCodeAt(i++);
            if (buf[idx] === 0) { return false }
            buf[idx] --;
        }
        return true;
    }
    return false;
}


Product of all but one number in a sequence - Follow Up (2) This code is a improved version of implementation which asked for a review: Product of all but one number in a sequence.
#include <stdio.h>
#include <stdlib.h>

//without division, with O(n) time, but extra space complexity as suggested
//return new array on the heap
int *derive_products(const int *nums, int count)
{
    int *retval = malloc(count * sizeof *retval);
    if (!retval && count)
        return 0;

    int mult=1; //product of prefix or suffix elements

    //swipe up
    for(int i=0; i<count; i++) {
        retval[i] = mult;
        mult *= nums[i];
    }

    //swipe down
    for(int j=count-1, mult=1; j>=0; j--) {
        retval[j] *= mult;
        mult *= nums[j];
    }

    return retval;
}

int main()
{
    /*Given an array of integers, return a new array such that each element at index i of the
    new array is the product of all the numbers in the original array except the one at i.
    For example, if our input was [1, 2, 3, 4, 5], the expected output would be
    [120, 60, 40, 30, 24] */
    int nums[] = {1, 2, 2, 4, 6};
    int size = sizeof(nums)/sizeof(nums[0]);

    int *products = derive_products(nums, size); //get a new array

    for (int i = 0; i < size; i++)
        printf("%d ", products[i] );

    free(products); //release heap memory

}

Proper scoping
int mult=1; //product of prefix or suffix elements

mult above is only used in the first loop. The one used in the second loop is defined there, and limited to that loop. This one should be the same.

Proper naming

j in the second loop is an unexpected name. As it is an outer loop, you should use i (again).

int size = sizeof(nums)/sizeof(nums[0]);

While size isn't really wrong, I would expect a different unit of measurement (namely bytes, as for sizeof, instead of elements) considering the name. I suggest going for count.

Check for failure
int *products = derive_products(nums, size); //get a new array

The above can fail, in which case it returns null. Don't just assume success.

Comments
//without division, with O(n) time, but extra space complexity as suggested
//return new array on the heap

Comments should be to the point, omit the obvious, and describe the motivation.
The above could be:
// without division in O(n) time
// free(result)

Yes, writing down the task as a comment is a good idea. But if you have test-data, just write a test.

Your comment after a line of code are pretty superfluous. Are you forced to sprinkle more comments?
See Guessing a number, but comments concerning

Whitespace

Your use of whitespace is inconsistent. Do you surround binary operators with single whitespace?
Sometimes you do, sometimes you don't. Imho, doing so looks better, so standardise on that. There are plenty automatic code formatters, so you don't even have to do the hard work yourself.


Parsing string into hash-map with 2D coordinates as keys I had never programmed in a functional programming language before. I think that this function has large nesting. Is there way to rewrite it better? I am most worried about the use of "flatten". Is there way to get rid of it? Also, is the indentation correct and does the function name correspond to Clojure naming style?

(defrecord Point [x y])
(def delimiter #" ")

(defn str->cells
  "Converts string with cells separated with
  delimiter and new lines to Points hash map"
  [^String s]
  {:pre [(re-matches #"^((?:^|\s)(?:.|\s)(?=\s|$))+$" s)]}
  (apply
    hash-map
    (flatten
      (map-indexed
        (fn [y line]
          (map-indexed
            (fn [x value] [(Point. x y) value])
            (str/split line delimiter)))
        (str/split-lines s)))))

;; Example of use:
(str->cells "1 2\n3")  ;; -> {#Point{:x 1, :y 0} "2", #Point{:x 0, :y 0} #Point{:x 0, :y 1} "3"}

Since there is no answers, I'll post my new version of code:

(defn line->cells
  "Converts string with cells to sequence of maps of Points
  It is str->cells helper, so it requires y argument."
  [str->value ^Long y ^String line]
  {:pre [(re-matches #"^((?:^| )(?:.+?| )(?= |$))+$" line)]}
  (map-indexed
    (fn [x value] {(Point. x y) (str->value value)})
    (str/split line #" ")))

(defn str->cells
  "Converts string with cells separated with
  delimiter and new lines to Points hash map"
  [^String s str->value]
  {:pre [(re-matches #"^((?:^|\s)(?:.+?| )(?=\s|$))+$" s)]}
  (->> s
       (str/split-lines)
       (map-indexed (partial line->cells str->value))
       (flatten)
       (into {})))


Php how to properly connect functions with pagination Am I doing this right? I do not know how to properly connect functions with pagination. To make everything work, I have to duplicate this variables. When I add variables to the function and to the index, then everything works.

$rowperpage = 10;
$page = $_GET['page'] ?? 1;
$page = $page - 1;
$p = $page * $rowperpage;

Function

function find_all_products_by_cat_id2($cat_id, $options=[]) {
    global $db;

    $visible = $options['visible'] ?? false;

    $rowperpage = 10;
    $page = $_GET['page'] ?? 1;
    $page = $page - 1;
    $p = $page * $rowperpage;

    $sql = "SELECT * FROM products ";
    $sql .= "WHERE cat_id='" . db_escape($db, $cat_id) . "' ";
    if($visible) {
      $sql .= "AND visible = true ";
    }
    $sql .= "ORDER BY prod_name ASC ";
    $sql .= "LIMIT ".$p.", ".$rowperpage." ";
    $result = mysqli_query($db, $sql);
    confirm_result_set($result);
    return $result;

index.php

<?php

        $rowperpage = 10;
        $page = $_GET['page'] ?? 1;
        $page = $page - 1;
        $p = $page * $rowperpage;

        $category_id = $_GET['id'] ?? 1;

        $products_count = count_products_by_cat_id($category_id, ['visible' => true]);

        if(isset($_GET['id'])) {
        $category_id = $_GET['id'];

        $product_set = find_all_products_by_cat_id2($category_id, ['visible' => true]);

        if(!$product_set) {
            redirect_to(url_for('/index.php'));
        }

        $product_count = mysqli_num_rows($product_set);

        if($product_count == 0) {
              echo "
No more products
";
              }

 ?>

I think I found a solution.

Update 1
Function

  function find_by_sql($sql) {
    global $db;

    $result = mysqli_query($db, $sql);
    confirm_result_set($result);
    return $result;
  }

index.php

$visible = $options['visible'] ?? false;

$sql = "SELECT * FROM products ";
$sql .= "WHERE cat_id='" . db_escape($db, $category_id) . "' ";
if($visible) {
  $sql .= "AND visible = true ";
}
$sql .= "ORDER BY prod_name ASC ";
$sql .= "LIMIT ".$p.", ".$rowperpage." ";

$product_set = find_by_sql($sql, ['visible' => true]);

Assuming you have a BOOL/BOOLEAN column called visible, you might consider using a more recognized syntax like visible <> 0 or visible = 0 as advised here: MySQL: “= true” vs “is true” on BOOLEAN. When is it advisable to use which one? And Which one is vendor independent? After all, BOOLEAN columns are really just TINYINT columns.

I have to assume that confirm_result_set($result); is fetching the rows and preparing them in some manner.  Perhaps you'd like to adjust the variable name to be more intuitive/descriptive.

Some developers, myself included, do not recommend the use of global as a means of transferring variables into a function's scope.  IMO, it is cleaner to pass the connection variable as a parameter in the function call (like you do with $sql).

I don't know what db_escape() is doing, but the surest advice is to urge you to use prepared statements with placeholders.  As a less classy alternative, so long as $category_id is an integer, you can cast it as an integer ((int)$category_id) before using it in your query. Joomla, for instance, doesn't offer prepared statements yet (available in the next major version) so they use integer/float casting for security on numeric values.  Also, numeric values do not need to be quoted in queries.

ASC is not necessary in your query, it is the default sorting direction and can be omitted.

The LIMIT clause string can be written without concatenation: $sql .= "LIMIT $p, $rowperpage";


Reusable webdriver framework using SOLID design principles I am trying to design some classes which I intent to use as a framework. I would like them to be compliant to SOLID design principles.

I had a basic structure and then I implemented Strategy design pattern and tried adding dependency injection as well.

Would like this to be reviewed so that I know if my understanding is correct or can still be made better.

public interface IBrowser
{
    IWebDriver GetWebDriver(string webDriverFolderPath);
}

public class EdgeBrowser : IBrowser
{
    public IWebDriver GetWebDriver(string webDriverFolderPath)
    {
        var options = new EdgeOptions()
        {
            PageLoadStrategy = PageLoadStrategy.Eager,
            UseInPrivateBrowsing = true
        };

        return new EdgeDriver(webDriverFolderPath, options, TimeSpan.FromSeconds(60));
    }
}

public class IEBrowser : IBrowser
{
    public IWebDriver GetWebDriver(string webDriverFolderPath)
    {
        var options = new InternetExplorerOptions()
        {
            IntroduceInstabilityByIgnoringProtectedModeSettings = true
        };

        return new InternetExplorerDriver(webDriverFolderPath, options, TimeSpan.FromSeconds(60));
    }
}

public interface IConfiguration
{
    string GetConfiguration(string key);
}

public class AppSettingsConfiguration : IConfiguration
{
    public string GetConfiguration(string key)
    {
        return ConfigurationManager.AppSettings[key].ToString();
    }
}

public class WebDriver
{
    private static Dictionary<Enums.Browsers, IBrowser> Browsers = new Dictionary<Enums.Browsers, IBrowser>();
    private static Dictionary<string, Enums.Browsers> BrowserMapper = new Dictionary<string, Enums.Browsers>();
    private static AppSettingsConfiguration _AppSettingsConfiguration;

    static WebDriver()
    {
        Browsers.Add(Enums.Browsers.Edge, new EdgeBrowser());
        Browsers.Add(Enums.Browsers.IE, new IEBrowser());
        Browsers.Add(Enums.Browsers.Chrome, new ChromeBrowser());
        Browsers.Add(Enums.Browsers.Firefox, new FirefoxBrowser());

        BrowserMapper.Add("Edge", Enums.Browsers.Edge);
        BrowserMapper.Add("IE", Enums.Browsers.IE);
        BrowserMapper.Add("Internet Explorer", Enums.Browsers.IE);
        BrowserMapper.Add("Chrome", Enums.Browsers.Chrome);
        BrowserMapper.Add("Firefox", Enums.Browsers.Firefox);
        BrowserMapper.Add("edge", Enums.Browsers.Edge);
        BrowserMapper.Add("ie", Enums.Browsers.IE);
        BrowserMapper.Add("internet explorer", Enums.Browsers.IE);
        BrowserMapper.Add("chrome", Enums.Browsers.Chrome);
        BrowserMapper.Add("firefox", Enums.Browsers.Firefox);
        BrowserMapper.Add("internetexplorer", Enums.Browsers.IE);
        BrowserMapper.Add("InternetExplorer", Enums.Browsers.IE);
    }

    public WebDriver(AppSettingsConfiguration appSettingsConfiguration)
    {
        _AppSettingsConfiguration = appSettingsConfiguration;
    }

    public IWebDriver GetWebDriver(string browser)
    {
        var browserName = BrowserMapper[browser];

        return Browsers[browserName].GetWebDriver(_AppSettingsConfiguration.GetConfiguration("WebDriverFolderPath"));
    }
}

public void Initialize()
{
    if (WebDriver == null)
    {
        WebDriver = new WebDriver(new AppSettingsConfiguration()).GetWebDriver(BaseData.Browser);
    }
}

I'm not really sure that there is any good argument having these static fields, especially when it comes to unit testing the WebDriver class.

public class WebDriver
{
    private static Dictionary... // this
    private static Dictionary... // this
    private static AppSetting... // this

    static WebDriver() // this
    {
        ...
    }

    ...
}

I would suggest you change those to private readonly fields instead.

and then we have this Initialize() method hanging out on it's own -- which means your code could not compile as is.

public class Foo // where is the class?
{
    public void Initialize()
    {
        if (WebDriver == null)
        {
            WebDriver = new WebDriver(new AppSettingsConfiguration())
                .GetWebDriver(BaseData.Browser);
        }
    }
}

I highly would not recommend passing around the AppSettingsConfiguration class, when it seems like the only time you use it is to pull a string value by calling _AppSettingsConfiguration.GetConfiguration("WebDriverFolderPath").

It would make more sense for you to handle reading your configuration somewhere else and not make the WebDriver responsible for handling that... (This actually breaks your "S" in the SOLID principle).

I would probably advise to consider refactoring it along these lines...

public enum Browsers { Unknown, Ie, Chrome, Edge, FireFox }

public class WebDriverFactory
{
    private readonly string _path;

    public WebDriverFactory(string path)
    {
        if (String.IsNullOrEmpty(path)) throw new ArgumentNullException(nameof(path));
        // also, consider to a check that the path exists

        _path = path;
    }

    public IWebDriver Construct(Browsers browser)
    {
        switch (browser)
        {
            case Browser.Edge:
            {
                var options = new EdgeOptions()
                {
                    PageLoadStrategy = PageLoadStrategy.Eager,
                    UseInPrivateBrowsing = true
                };
                return new EdgeDriver(_path, options, TimeSpan.FromSeconds(60));
            }
            case.Unknown:
                throw new Exception("not a valid browser");
            default:
                throw new NotImplementedException();
        }
    }
}


Automatically implement operator+ using CRTP I have multiple classes that should support addition. For each of them, the operator += is implemented as T& operator+=(const T& rhs). I also want to add the operator +, but this implementation will use += and will therefore be similiar in all classes. My idea was to implement a template class using the Curiously Recurring Template Pattern that provides an implementation. The result is code like this:

template<class T>
class Sumable {
private:
    Sumable() = default;

public:
    Sumable(const Sumable<T>&) = delete;
    Sumable(Sumable<T>&&) = delete;

    T operator+(const T&) const&;
    T operator+(const T&) && ;
    T operator+(T&&) const&;
    T operator+(T&&) && ;

    friend T;
};

template<class T>
inline T Sumable<T>::operator+(const T & rhs) const&
{
    T result(*static_cast<const T*>(this));
    return std::move(result += rhs);
}

template<class T>
inline T Sumable<T>::operator+(const T & rhs) &&
{
    return std::move<T&>(*static_cast<T*>(this) += rhs);
}

template<class T>
inline T Sumable<T>::operator+(T && rhs) const&
{
    return std::move(rhs += *this);
}

template<class T>
inline T Sumable<T>::operator+(T && rhs) &&
{
    return std::move(rhs += *this);
}

Classes that support addition would extend from Sumable<T> like for example class Vector : public Sumable<Vector>

The operator is overloaded four times to prevent the creation of unnecessary temporary objects. The idea is described here https://stackoverflow.com/questions/6006527/overloading-on-r-value-references-and-code-duplication, but I return T by value to prevent the problems described in this question.
I also follow this idea https://stackoverflow.com/questions/11224838/prevent-user-from-deriving-from-incorrect-crtp-base/11241079 to prevent incorrect use of the class.

In a simple test, this worked like expected. Are there any edge cases in which this design can cause problems? And is it a good idea to use this design in production code?

Edit
The code I used to test this, compiles under MSVS 2017:

#include "stdafx.h"
#include <iostream>
#include "Sumable.h"

using namespace std;

struct Test : public Sumable<Test>{
    int data;

    Test(int data) : data(data) {};
    Test(const Test& that) : data(that.data)
    {
        cout << "copy" << endl;
    };
    Test(Test&& that) : data(that.data)
    {
        cout << "move" << endl;
    };
    Test& operator+=(const Test& that) {
        this->data += that.data;
        return *this;
    }
};
int main()
{
    Test a{ 1 };
    Test b{ 2 };
    Test c{ 3 };
    cout << (a + b + c + a + b + c).data << endl;
    return 0;
}

Missing header
I had to #include <utility> to get a definition for std::move().
Don't delete your destructors
You didn't show us your tests, but I struggled to get my obvious code to compile.  Part of the reason was the deleted constructors; I changed the default constructor to protected and made the copy/move constructors to protected and = default, and then I was able to inherit without having to explicitly write copy/move construct/assign in my subclass:
struct Test : public Sumable<Test>
{
    int i;
    Test(int i) : i(i) {}
    Test& operator+=(const Test& t) { i += t.i; return *this; }
};

There's no need to move() when returning
I know the question is tagged C++11, but even there, any benefit is questionable (and particularly unlikely to exist when the function is inlined).  When you move the code to a more recent standard, it's actively harmful.
Pass by value instead of copying from a reference
If you can insist on the usual x + y == y + x (which seems to be assumed already, given the third and fourth methods, which compute rhs += *this), then you can pass rhs by value (since we need to copy one of the arguments), which will be move-constructed as and when appropriate:
template<class T>
inline T Sumable<T>::operator+(T rhs) const
{
    return rhs += *static_cast<const T*>(this);
}

That's now one method instead of four.
N.B. I had to introduce the static_cast there, as your code wouldn't compile with GCC 8.0 without it.
Consider free functions
Inheritance isn't necessarily the best solution to everything in C++.  You may be better writing free functions instead (in the same namespace as your classes, or in a specific includable namespace, along the lines of std::rel_ops).
Here's a start, including a basic test:
#include <type_traits>

namespace arithmetic_ops
{
    template<typename T, typename U>
    auto operator+(T t, U&& u)
        -> typename std::remove_reference<decltype(t+=u)>::type
    {
        return t += u;
    }

    template<typename T, typename U>
    auto operator+(T&& t, U u)
        -> typename std::enable_if<!std::is_assignable<T,U>::value,
                                   decltype(operator+(u,t))>::type
    {
        return u + t;
    }
}

namespace test {
    struct Test
    {
        int i;
        Test(int i) : i(i) {}
        Test& operator+=(const Test& t) { i += t.i; return *this; }
    };
}

int main()
{
    using namespace arithmetic_ops;

    test::Test a(4);
    test::Test b(-4);
    return (0 + a + b + 0).i;
}

I've used forwarding references above for the non-copied argument - that allows us to have only one method for the class-first case (the second version being an override for cases such as 0 + a where the first argument must be converted).
The enable_if avoids ambiguity when t + u and u + t are both possible.
The ::type and ::value members get shortened from C++14, with _t and _v suffixes to the names (e.g. std::remove_reference_t<decltype(t+=u)> and std::enable_if_t<!std::is_assignable_v<T,U>, decltype(operator+(u,t))>.
a and/or b can be made const in this test without breaking it.


Changing line colors and move text for menu navigation This code basically changes the color of the lines and move text for menu navigation.  Is there a simpler way of writing it?

$('.nav1 a').click(function () {
    $('.nav1').addClass("current_nav1");
    $('.sf_menu li').removeClass('current_nav2 current_nav3 current_nav4 current_nav5 current_nav6');
    $('#content, #home').slideDown(2000);
    $('#design, #seo, #galery, #about, #contact').slideUp(2000);
});

$('.nav2 a').click(function () {
    $('.nav2').addClass("current_nav2");
    $('.sf_menu li').removeClass('current_nav1  current_nav3 current_nav4 current_nav5 current_nav6 ');
    $('#content, #design').slideDown(2000);
    $('#home, #seo, #galery, #about, #contact').slideUp(2000);
});

$('.nav3 a').click(function () {
    $('.nav3').addClass("current_nav3");
    $('.sf_menu li').removeClass('current_nav1 current_nav2  current_nav4 current_nav5 current_nav6');
    $('#content, #seo').slideDown(2000);
    $('#home, #design, #galery, #about, #contact').slideUp(2000);
});

$('.nav4 a').click(function () {
    $('.nav4').addClass("current_nav4");
    $('.sf_menu li').removeClass('current_nav1 current_nav2 current_nav3  current_nav5 current_nav6');
    $('#content, #galery').slideDown(2000);
    $('#home, #design, #seo, #about, #contact').slideUp(2000);
});

$('.nav5 a').click(function () {
    $('.nav5').addClass("current_nav5");
    $('.sf_menu li').removeClass('current_nav1 current_nav2 current_nav3 current_nav4  current_nav6');
    $('#content, #about').slideDown(2000);
    $('#home, #design, #seo, #galery, #contact').slideUp(2000);
});

$('.nav6 a').click(function () {
    $('.nav6').addClass("current_nav6");
    $('.sf_menu li').removeClass('current_nav1 current_nav2 current_nav3 current_nav4 current_nav5 ');
    $('#content, #contact').slideDown(2000);
    $('#home, #design, #seo, #galery, #about').slideUp(2000);
});

Why do you need different navXXX and current_navXXX classes? Just have a single

var menus = {//you need some kind of mapping between elements clicked and menu items
  id1: '#home',
  id2: '#design',
  id3: '#seo',
  id4: '#galery',
  id5: '#about',
  id6: '#contact'
};

$('.nav a').click(function () {
    $('.sf_menu li').removeClass('current_nav');
    $(this).addClass("current_nav");

    var selected = menus[$(this).attr('id')];
    $('#content, ' + selected).slideDown(2000);

    for (var menu in menus) {
        if (menus[menu] != selected)
           $(menus[menu]).slideUp(2000);
    }
});


Guess the Number In C++ Mind that this is my first actual program in C++ and I tried to prepare for any possible input.  Also the reason I included static_cast<void>(generator(range)) is to throw away the first random value because I read on LearnCpp.com that throwing away the first is a common practice to produce more random results.  Anyways here is the code, all suggestions are welcome.
#include <iostream>
#include <limits>
#include <random>

bool validateInput(int input)
{
    using namespace std;

    if (1 <= input && input <= 100)
        return true;
    else if (!input)
        return false;
    else
        return false;
}

int generateRandomNumber()
{
    std::random_device dev;
    std::mt19937_64 range(dev());
    std::uniform_int_distribution<std::mt19937_64::result_type> generator(1, 100);
    static_cast<void>(generator(range));
    return static_cast<int>(generator(range));
}

bool placement(int input, int target)
{
    using namespace std;
    if (input > target)
    {
        cout << "Guess is too high.";
        return false;
    }

    else if (input < target)
    {
        cout << "Guess is too low.";
        return false;
    }
    else
    {
        cout << "Guess is correct!";
        return true;
    }
}

int main()
{
    using namespace std;
    cout << "You have 7 guesses to guess what number I am thinking of between 1 and 100.\n";
    int winning_num{ generateRandomNumber() };
    static int tries{ 1 };
    do
    {
        int num;
        cout << "\nGuess#" << tries << ": Enter a number -> ";
        cin >> num;
        if (!cin)
        {
            cin.clear();
            cin.ignore(numeric_limits<streamsize>::max(), '\n');
            continue;
        }

        cin.ignore(numeric_limits<streamsize>::max(), '\n');

        if (validateInput(num))
            ;
        else
        {
            continue;
        }
        if (!placement(num, winning_num))
        {
            cout << "\nIncorrect!\n";
            ++tries;
        }
        else
        {
            cout << "\nCorrect!\n";
            exit(0);
        }

    }
    while (tries <= 7);
    cout << "\nYou did not guess the number in the alloted amount of guesses :(\nTry Again!\n";

}

I like that your code (read functions) follows Single Responsibility Principle.
And also that you've put the number comparison code in a separate function unlike

User guesses the number generated by computer
Computer tries to guess your inputted number

And also that you use static_cast. All are good coding practices!

I'd prefer if you do the text printing in the placement function itself and call it something like compare_with_guess. That way, you don't have to return values and use another if-else block in the caller. Also, the output messages can also be controlled all at one place.
Another way is to use struct like:
struct InputNum{
private:
    const int input;
public:
    InputNum(const int num):input(num){};

    int get() const{
        return input;
    }
    bool greater_than(const int target) const {
        return input > target;
    }
    bool less_than(const int target) const {
        return input < target;
    }
    bool equals(const int target) const {
        return input == target;
    }
}

Use const whenever possible.
const int winning_num{ generateRandomNumber() };// +1 for {} initialiser syntax.

void compare_with_guess(const int input, const int guess){ // After making the changes I suggested above.

bool validateInput(const int input){

Avoid  using namespace std; completely, let alone using it multiple times in the code.
https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice


Determine closest ancestor that has a certain HTML class When writing JavaScript code for my own projects, this functionality is the singlemost used algorithm that does not already exist: Finding an ancestor element that has a certain HTML class.

Algorithm:

Does the current element have a parent element?

Yes: Continue with step 2
No: There are no solutions to the task, return a null value

Does the parent element of the current element have the required class?

Yes: Return the parent element
No: Set the current element to its parent element and repeat step 1

Code:

Element.prototype.getAncestorByClassName = function(className) {
    var currentParent = this.parentElement
    while (true) {
        // If the root of the DOM is reached, an ancestor cannot be found anymore.
        if (currentParent === null) {
            return null
        }

        if (currentParent.classList.contains(className)) {
            return currentParent
        }

        var currentParent = currentParent.parentElement
    }
}

Notes:

I want to be able to call element.getAncestorByClassName("class-name") hence the use of Element.prototype. Does this approach yield any problems? I am aware that I would override any getAncestorByClassName() functions that are introduced in future versions of ECMAScript
There are possibly multiple results for the task I specified. The algorithm will return the closest ancestor that has the given class.
If there are no results, null is returned as other functions like getElementById() do the same.
Browser support should cover all modern browsers. I do not consider IE8–IE10 modern browsers. The lack of support for classList on these is noted but not essential to me right now.

Extending native prototypes is a little iffy. Not that this should cause any problems, but in principle it's best to avoid. The reason is mostly that if everyone extend native prototypes, then you're more likely to run into issues (i.e. if you include a library that tramples on your extensions or vice-versa). Hence the general consensus is to try to leave them alone.

All that said, the main point approach it with open eyes, and proceed at your own risk.

In this case, I doubt you risk much, but it's always a good idea to consider something like:

if (!Element.prototype.getAncestorByClassName) {
  // extend the prototype
}

just to only do the extension if the function doesn't already exist. Of course, this is considerably more useful for polyfills, where a standardised function does exist and its signature is known. Here, if a getAncestorByClassName function already exists, it's hard to know if it works the same as yours since either one would be non-standard. Anyway, just something to consider.

As for the code:

The first step you take is to call currentNode.parentElement. This precludes you finding the element itself. Granted the function is called get ancestor, but still... say you call it on an element like body > p > span.highlight, and ask it to find the class highlight: You'd get null back. I'd include the initial element in the search.

Your also doing some unnecessary work in checking for === null inside the loop, since the while loop's condition could do that. Since you always end up exiting the loop somehow, it might as well be apparent in the loop's own condition, rather than have it be explicitly defined as an infinite loop.

And there's no reason to redeclare var currentNode inside the loop; it's already declared.

Also not a fan of the "minimal semicolons"-style, but that's just me. Works without them too.

Cleaned up a bit, it could look like:

Element.prototype.getAncestorByClassName = function(className) {
    var currentElement = this;

    while (currentElement) {
        if (currentElement.classList.contains(className)) {
            return currentElement;
        }
        currentElement = currentElement.parentElement;
    }

    return null;
}

Of course, since you're extending the Element prototype you could be clever and make it recursive. It looks nice, but I'd probably stick to the simpler loop myself:

Element.prototype.getAncestorByClassName = function(className) {
    if (this.classList.contains(className)) {
        return this;
    }

    if (!this.parentElement) {
        return null;
    }

    return this.parentElement.getAncestorByClassName(className);
}


Counting Sort Implementation in C# Here is my Implementation. It is I think almost like algorithm.

 public static uint[] CountingSort(uint[] A ,uint max)
        {
            uint[] B = new uint[A.Length];
            int[] c = new int[max +1 ];
            for (int j = 0; j < A.Length; j++)
                c[A[j]]++;
            for (int i = 1; i < max + 1; i++)
                c[i] += c[i - 1];
            for (int j = A.Length-1 ; j>=0; j--)
                B[--c[A[j]]] = A[j];

            return B;
        }

Var

Use implicit typing when the right-hand side of the declaration makes the type obvious.

uint[] B = new uint[A.Length];

should be

var B = new uint[A.Length];

Naming

Single character variable names are some of the least useful names you could give your variables. Your variables have a purpose. Name them appropriately using these purposes.

B means nothing.

Spacing

Give your code some room to breathe, the compiler will cut it out anyway when it gets to the optimization stage, and there's no sense making it harder to read for maintenance programmers. Also consider surrounding your loop bodies with braces. This way if somebody came in and added a line to the loop body, it wouldn't break.

Design

Use foreach when iterating over every item in a collection, it more accurately conveys your meaning.

Secondly, I cannot find a use for manually specifying max. Instead you should calculate this using:

var max = A.Max();

All things combined, I recommend:

public static uint[] CountingSort(uint[] unsorted)
    {
        var max = unsorted.Max();
        var sorted = new uint[A.Length];
        var count = new int[max +1];

        foreach(var unsortedValue in unsorted)
        {
            count[unsortedValue]++;
        }

        for (var i = 1; i < max + 1; i++)
        {
            count[i] += count[i - 1];
        }

        for (var j = unsorted.Length-1 ; j>=0; j--)
        {
            sorted[--count[unsorted[j]]] = unsorted[j];
        }

        return sorted ;
    }

Optionally

You could consider making this method an extension method of the uint[] type:

public static uint[] CountingSort(this uint[] unsorted)
    {
        var max = unsorted.Max();
        var sorted = new uint[unsorted.Length];
        var count = new int[max +1];

        foreach(var unsortedValue in unsorted)
        {
            count[unsortedValue]++;
        }

        for (var i = 1; i < max + 1; i++)
        {
            count[i] += count[i - 1];
        }

        for (var j = unsorted.Length-1 ; j>=0; j--)
        {
            sorted[--count[unsorted[j]]] = unsorted[j];
        }

        return sorted ;
    }

Then you can call the method like this:

var sorted = unsorted.CountingSort();

But this is optional and depends on your desired use cases.

Secondly, consider using an interface such as IList to allow callers of your method more freedom when specifying data types:

    public static IList<uint> CountingSort(this IList<uint> unsorted)
    {
        var max = unsorted.Max();
        var sorted = new uint[unsorted.Count];
        var count = new int[max +1];

        foreach(var unsortedValue in unsorted)
        {
            count[unsortedValue]++;
        }

        for (var i = 1; i < max + 1; i++)
        {
            count[i] += count[i - 1];
        }

        for (var j = unsorted.Count-1 ; j>=0; j--)
        {
            sorted[--count[unsorted[j]]] = unsorted[j];
        }

        return sorted;
    }

Now you can call:

new List<uint>().CountSort();
new uint[].CountSort();
new Collection<uint>().CountSort();

With way more flexibility.


Winterbash 2014 "Red Shirt" Estimation Some fellow code reviewers (hi @Janos!) have been inquiring about a SEDE query to allow to check progress of the Red Shirt "hat" progression.

Try it here!

Background

  Red Shirt

  cast 5 downvotes on posts that are later deleted or closed

Limitations

There are certain Stack Exchange limitations which make querying this information a bit tricky. Namely:

The data is only refreshed once a week, on Sundays. This makes it impossible to have "real time" results.
User voting activity is anonymous, in that a user can only see their own voting activity in their own profile. This disallows joining voting and user data on SEDE.

Assumptions

I have made certain assumptions, based on the trends I generally see on closed questions. They are:

A user who votes to close/delete a bad question will usually also downvote the question.
A user who downvotes a question will usually do so before voting to close/delete.

Usage

To get usefulness out of this (and as indicated in the SQL comments at the top of the query):

  The way that this report can be used is by comparing the results set
  side-by-side with your "votes" under your activity reports.
  Filter by down-votes and look to see if questions you down-voted are
  in the result set below.

For example:

Query

/*
Winter Bash 2014
"Red Shirt" hat estimation
"Cast 5 downvotes on posts that are later deleted or closed"

The way that this report can be used is by comparing the results set
side-by-side with your "votes" under your activity reports.
Filter by down-votes and look to see if questions you down-voted are
in the result set below.
*/

-- NumberWeeks: Number of weeks to go back

-- DATETIME VARIABLES

DECLARE @today DATETIME;
SET @today = CURRENT_TIMESTAMP;

DECLARE @weeks_ago INT;
SET @weeks_ago = ##NumberWeeks:int?4##;
-- Number of weeks must not go into the future, hence the following:
SET @weeks_ago = (CASE WHEN @weeks_ago >0 THEN -@weeks_ago ELSE @weeks_ago END);

DECLARE @target_week DATETIME;
SET @target_week = DATEADD(WEEK, @weeks_ago, CURRENT_TIMESTAMP);

-- POST-RELATED VARIABLES

DECLARE @downvote TINYINT;
SET @downvote = (SELECT Id FROM VoteTypes WHERE Name LIKE 'Down%'); --3

DECLARE @question_post TINYINT;
SET @question_post = (SELECT Id FROM PostTypes WHERE Name = 'Question'); --1

DECLARE @closed_post TINYINT;
SET @closed_post = (SELECT Id FROM PostHistoryTypes WHERE Name = 'Post Closed'); --10

DECLARE @deleted_post TINYINT;
SET @closed_post = (SELECT Id FROM PostHistoryTypes WHERE Name = 'Post Deleted'); --12

-- QUERY BEGINS

WITH cte_downvoted_posts AS
(
  SELECT Votes.PostId AS dvote
  FROM Votes
  INNER JOIN Posts ON Votes.PostId = Posts.Id
  WHERE VoteTypeId = @downvote
)

SELECT
    Posts.Id AS [Post Link]               -- magic column
  , Posts.OwnerUserId AS [User Link]      -- magic column
  , Posts.CreationDate AS [Creation Date]
  , Posts.ClosedDate AS [Closed Date]

FROM Posts
INNER JOIN cte_downvoted_posts
    ON Posts.Id = cte_downvoted_posts.dvote
INNER JOIN PostHistory
    ON Posts.Id = PostHistory.PostId

WHERE Posts.CreationDate <= @today
  AND Posts.CreationDate > @target_week
  AND Posts.PostTypeId = @question_post
  AND Posts.ClosedDate IS NOT NULL

GROUP BY
    Posts.Id
  , Posts.OwnerUserId
  , Posts.CreationDate
  , Posts.ClosedDate

ORDER BY
    Posts.CreationDate DESC

Concerns

Nitpicks are fine! Anything from naming to indentation to inconsistencies, please don't be shy to point out anything at all!
I noticed a lot of nested loops in the execution plan, when it's doing joins. Is there a better way to do this to avoid them?
Is there a way to make this query result set more useful, or user-friendly?
Are my comments appropriate/useful? Should I have fewer, or more of them?

I find this pretty nicely done and really hard to pick on.
But here's a little something.
Instead of writing this condition this way:

WHERE Posts.CreationDate <= @today
  AND Posts.CreationDate > @target_week

This is shorter and simpler:

WHERE Posts.CreationDate BETWEEN @target_week AND @today

Keep in mind though that's not exactly the same,
because if I remember correctly,
BETWEEN uses <= and >= conditions.
But even if so, it might not make a difference anyway in the case of this query.
(I confirmed with 1 week to go back, the results are identical.)

Btw, @Malachi pointed out an excellent article about range queries and their dangers.

A very minor nitpick, but in this column alias I would spell out the "dvote" nicely as "downvote":

  SELECT Votes.PostId AS dvote


Hackers Ranking String Similarity I am currently trying to teach myself some programming. I have started to work with Python by doing this challenge.

For each test case, I need to find the sum of the self-similarities of a string with each of its suffixes.  For example, given the string ababaa, the self-similarity scores are

6 (because ababaa = ababaa)
0 (because ababaa ≠ babaa)
3 (because ababaa and abaa share three initial characters)
0 (because ababaa ≠ baa)
1 (because ababaa and aa share one initial character)
1 (because ababaa and a share one initial character)

… for a total score of 11.

When I test run it works out fine, however when I run this code with longer strings it takes too long time to run, so the site shuts down the program.  Each input string consists of up to 100000 lowercase characters.

Is this because this code make unnecessary loops? Or what else might the problem be here?

# Solution.py for https://www.hackerrank.com/challenges/string-similarity
import sys
for line in sys.stdin:
  if line.islower():
    line = line[:-1] # line will be compared to suffix
    line_length = len(line)
    points = 0
    for s in xrange(0,line_length):
      suffix = line[s:]
      suffix_length = len(suffix)
      count = 1
      while count < suffix_length+1:
        if line.startswith(suffix[:1]): # if line and first character of suffix mach
          if line.startswith(suffix[:suffix_length]):
            points += len(suffix[:suffix_length])
            break
          else:
            if line.startswith(suffix[:count+1]):
              count += 1
            elif line.startswith(suffix[:count]):
              points += len(suffix[:count])
              break
        else:
          break
    print points

It is slow because you use slow algorithm ("too many loops"). This problem might be a bit hard for beginner. If you want to solve it anyway, be sure to search for tutorials on string algorithms (try to look at http://en.wikipedia.org/wiki/Aho-Corasick and change it a bit). Maybe dynamic programming will help you with this or other problems.

PS I hope you understand nobody here can not spoil the solution. Have fun with programming.

You could always try another problems:

Timus one of the best: http://acm.timus.ru/problemset.aspx

Project Euler if you like mathematics: http://projecteuler.net/

(Please feel free to edit and add some more.)


Star Elements implementation in Java Given an unsorted array. The task is to find all the star and super
  star elements in the array. Star are those elements which are strictly
  greater than all the elements on its right side. Super star are those
  elements which are strictly greater than all the elements on its left
  and right side.

  Note: Assume first element (A[0]) is greater than all the elements on
  its left side, And last element (A[n-1]) is greater than all the
  elements on its right side.

  Input: The first line of input contains an integer T denoting the
  number of test cases. Then T test cases follow. Each test case
  consists of two lines. First line of each test case contains an
  Integer N denoting size of array and the second line contains N space
  separated elements.

  Output: For each test case, print the space separated star elements
  and then in new line print super star elements. If no super star
  element present in array then print "-1".

  Constraints: 1<=T<=200 1<=N<=106 1<=A[i]<=106

  Example: Input:

2
6
4 2 5 7 2 1
3 8 6 5

  Output:

7 2 1
7
8 6 5
8

My approach:

/*package whatever //do not write package name here */

import java.util.Scanner;
import java.util.HashMap;
import java.util.Map;
import java.util.Collections;
import java.util.Set;
import java.util.*;

class GFG {

    private static void findElems (int [] arr)
        {
            int len = arr.length;

            HashMap<Integer,Integer> stars = new HashMap<>();

            stars.put(arr[len-1],len - 1);

            int maxRight = arr[len - 1];

            for (int i = len - 2; i >=0; i--)
                {
                    if (arr[i] > maxRight)
                    {
                         stars.put(arr[i],i);
                         maxRight = arr[i];
                    }
                }

           int maxStar = Collections.max(stars.keySet());

           int maxStarInd = (Integer)stars.get(maxStar);
           int flag = 0;

           for (int i = 0; i < len;i++)
            {
                if  (i != maxStarInd)
                    {
                        if (arr[i] == maxStar)
                            {
                                flag = 1;
                                break;
                            }
                    }
            }

            Set <Integer> starSet = stars.keySet();

            //Integer [] starKeys = starSet.toArray (new Integer[stars.size()]);
            LinkedList <Integer> revSet = new LinkedList<>(starSet);
            Collections.sort(revSet, Collections.reverseOrder());

            Iterator<Integer> iter = revSet.iterator();

            while (iter.hasNext())
                {
                    System.out.print(iter.next() + " ");
                }

            System.out.println();
         /*   for (int i = starKeys.length - 1; i >= 0; i--)
                {
                    System.out.println(starKeys[i]);
                }
        */
            if (flag == 0)
                {
                    System.out.println(maxStar);
                }
            else
                {
                   System.out.println(-1);
                }

        }

    public static void main (String[] args) {
        Scanner sc = new Scanner(System.in);

        int numTests = sc.nextInt();

        for (int i = 0; i < numTests; i++)
            {
                int size = sc.nextInt();
                int [] arr = new int[size];

                for (int j = 0; j < size; j++)
                    {
                        arr[j] = sc.nextInt();
                    }

                findElems(arr);
            }

    }
}

I have the following questions with regards to the above code:

How can I further improve my approach?
Is there a better way to solve this question?
Are there any grave code violations that I have committed?
Can space and time complexity be further improved?
Is my code very redundant?

Reference

How can I further improve my approach?

  Is there a better way to solve this question?

Let's start by making some observations about the problem description and the possible inputs:

The star elements will always be a strictly decreasing sequence
There will always be one star element: the rightmost one
The leftmost star may or may not be a super star

The posted code uses a map to find the star elements.
This is unnecessary, a list would be fine.
The loop you already have going from right to left could prepend the found star element to the list of stars.

After finding the stars, you need to check if the first star is a super star or not.
This is easy to verify, for example by a second scan of the array,
checking that precisely one element is equal,
and all other elements are strictly less than the leftmost star.

In this verification step,
you could avoid scanning the entire array if you know the index of the leftmost star, but such optimization is not really important,
because the order of time complexity of the solution will be the same.

This proposed solution uses fewer redundant data structures (a single list instead of a map and a set), and performs faster (no sorting).

  Are there any grave code violations that I have committed?

The findElems method does too many things.
It finds the stars and super stars and prints them.
It would be better to organize programs in a way that one method does one thing.
Following the suggested sketch above,
there could be one function to find the stars,
one to verify if an element is a super star,
and one to print.
When organized that way,
the logical steps will be easier to understand,
and the solution will be much easier to test.

  Can space and time complexity be further improved?

The suggested sketch above is better because it eliminates the sorting of the stars. That would be a significant improvement when the input set is a strictly decreasing sequence, where all elements are stars. In this extreme case the time complexity would be reduced from \$O(n \log n)\$ to \$O(n)\$.
However, since the problem description says there are maximum 106 elements, this improvement would be hardly measurable, insignificant.

  Is my code very redundant?

As mentioned earlier, the map and set can be replaced with a single list,
if you change the approach as I outlined.