Optimizing String combinations in Java I need to obtain all combinations of a String of length k from the given elements.

For example, for

char[] elements = {'a', 'b', 'c'}
k = 4

the output should be:

  aaaa, aaab, ..., cccc.

Now I have the following code which gives proper results BUT isn't fast enough. What can I improve in my code?

public static ArrayList<String> printAllKLength(char[] elements, int nrElements, int patternLength) {
    ArrayList<String> patternVariations = new ArrayList<String>();
    patternVariations = printAllKLengthRec(elements, "", nrElements, patternLength, patternVariations);

    return patternVariations;
}

public static ArrayList<String> printAllKLengthRec(char[] elements, String prefix, int nrElements, int patternLength, ArrayList<String> patternVariations) {

    if (patternLength == 0) {
        patternVariations.add(prefix);
        //System.out.println(prefix);
        return patternVariations;
    }

    for (int i = 0; i < nrElements; ++i) {
        String newPrefix = prefix + elements[i];
        printAllKLengthRec(elements, newPrefix, nrElements, patternLength - 1, patternVariations);
    }
    return patternVariations;
}

In the wild I don't find much recursion, it seems mostly used during exercises. I'm assuming this is one as well. Just hope you already handed it in.

one pointer for you, you are passing nrElements. Is this the same as elements.length? you might be able to remove it altogether from the interface of your methods.
Second pointer: your printAllKLengthRec returns an array , but it also updates the (same) array passed into it as an input argument. you can remove the return value completely. It is not needed. trust that the method will update the array you pass as input.

the way I would solve this myself would be something like:

public List<String> getStrings(char[] chars, int k) {
    int[] indexes = new int[k]; // prefills with 0
    List<String> result = new ArrayList<String>();
    boolean done = false;

    while (!done) { // stopcondition is inside the loop
        // construct 1 string with current indexes
        StringBuffer buffer = new StringBuffer();
        for (int i = 0; i < k; ++i) {
            buffer.append(chars[indexes[i]]);
        }
        result.add(buffer.toString());

        indexes[0]++;

        for (int i = 0; i < k; ++i) {
            if (indexes[i] == chars.length) { // overflow should go into i+1, set this index back to 0
                if (i + 1 == k) {
                    // loop done. would overflow.
                    done = true;
                    break;
                } else {
                    indexes[i] = 0;
                    indexes[i + 1]++;
                }
            }
        }
    }

    return result;
}

It is more lines, but it's nonrecursive which might make things a bit more manageable. For one you can split subparts into a separate method (for instance the indexes could become a class of it's own) and reuse it for other methods that need the same. I find recursion really only fixes it's one issue, but can't easily be reused for other things. Unless you write a good datastructure around it, like tree lookups and such.

just measured, my implementation was faster 3 times out of 4, but :) that's the problem with measuring using System.nanotime. All sorts of stuff can happen during execution to make the result not untrustworthy.


Calculating how many Pomodori fit into a period of time I've written some code to calculate how many Pomodori can fit into a period of time, including long and short breaks. I'm looking for alternative solutions because what I have at the moment doesn't feel right. I think I'm missing something.

A Pomodoro is an interval of time 25 minutes long and after each one there should be a short 5 minute break. But every 4x Pomodori should be followed by a long 15 minutes break. So we have the following:

╔═══════╦════════╦═════════════╗
║ Time  ║ Length ║ Activity    ║
╠═══════╬════════╬═════════════╣
║ 12:00 ║ 25:00  ║ Pomodoro    ║
║ 12:25 ║ 05:00  ║ Short Break ║
║ 12:30 ║ 25:00  ║ Pomodoro    ║
║ 12:55 ║ 05:00  ║ Short Break ║
║ 13:00 ║ 25:00  ║ Pomodoro    ║
║ 13:25 ║ 05:00  ║ Short Break ║
║ 13:30 ║ 25:00  ║ Pomodoro    ║
║ 13:55 ║ 15:00  ║ Long Break  ║
║ 14:10 ║ 25:00  ║ Pomodoro    ║
╚═══════╩════════╩═════════════╝

At the moment I use a loop to populate a list and then delete the last element if it's not a Pomodoro, as there's no point in ending the list of Pomodori with a break.

pomodori = []
iterations = 0
seconds = 3 * 60 * 60

while seconds > 0:
    # zero and even numbers are always Pomodori
    if iterations % 2 == 0 or iterations == 0:
        pomodori.append('pomodoro')
        seconds -= 25 * 60
    else:
        quotient, remainder = divmod(iterations+1, 4)

        # if the quotient is even and the remainder is zero, then we
        # are just after a 4x Pomodori and should add a long break
        if quotient % 2 == 0 and remainder == 0:
            pomodori.append('long-break')
            seconds -= 15 * 60
        # otherwise, we're at a short break
        else:
            pomodori.append('short-break')
            seconds -= 5 * 60

    iterations += 1

# remove breaks that are not followed by a Pomodoro
if pomodori[-1] != 'pomodoro':
    del pomodori[-1]

EDIT:

I've gone ahead with jonrsharpe's solution. The code is here for those interested.

I think it would be simpler just to apply some ingenuity and partially unroll the loop.  While it's possible to fit in a long cycle (pomodoro, short break, pomodoro, short break, pomodoro, short break, pomodoro, long break) with time left over, then do that.  While it's possible to fit in a short cycle (pomodoro, short break) with time left over, do that.

I'd also define some constants for readability.

pomodori is not quite an appropriate name for the result, since it includes pomodori and breaks.

It would be a good habit to encapsulate your code in a function.

SECOND = SECONDS = 1
MINUTE = MINUTES = 60 * SECONDS
HOUR = HOURS = 60 * MINUTES

POMODORO = 25 * MINUTES
SHORT_BREAK = 5 * MINUTES
LONG_BREAK = 15 * MINUTES

def manage(time):
    """ Devise a Pomodoro schedule for a time span, given in seconds. """
    schedule = []
    while time > 4 * POMODORO + 3 * SHORT_BREAK + LONG_BREAK:
        schedule.extend(3 * ['pomodoro', 'short-break'] + ['pomodoro', 'long-break'])
        time -= 4 * POMODORO + 3 * SHORT_BREAK + LONG_BREAK
    while time > POMODORO + SHORT_BREAK:
        schedule.extend(['pomodoro', 'short-break'])
        time -= POMODORO + SHORT_BREAK
    # 0 < time < POMODORO + SHORT_BREAK
    schedule.append('pomodoro')
    return schedule

print(manage(3 * HOURS))


Substring search in Java I am trying to make an algorithm that will find the index of a given substring (which I have labeled pattern) in a given String (which I have labeled text). This method can be compared to String.indexOf(String str). In addition to general feedback, I'm curious what the time complexity of my method is, and would appreciate if someone can help me figure it out.
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.NoSuchElementException;
import java.util.Set;

public class SubstringSearch {
    public static void main(String[] args) {
        String text = "This is a test String";
        String pattern = "is a";
        int index = search(pattern, text);
        System.out.println(index);
    }

    private static int search(String pattern, String text) {
        int patternLength = pattern.length();
        int textLength = text.length();

        Set<Integer> set = new HashSet<>(textLength);
        List<Integer> addList = new ArrayList<>(textLength);

        for (int i = 0; i < textLength; i++) {
            set.add(0);

            for (Integer index : set) {
                if (pattern.charAt(index) == text.charAt(i)) {
                    addList.add(index + 1);
                }
            }

            set.clear();
            set.addAll(addList);

            if (set.contains(patternLength)) {
                return i - patternLength + 1;
            }
        }

        throw new NoSuchElementException();
    }
}

Nice implementation, my suggestions:

Bug: text="aa aaa" and pattern="aaa" returns 1 instead of 3. Whether to handle this edge case  depends on your use case, but if text can be any string then it should be considered.
Input validation: if pattern is empty or null, a null pointer exception is thrown. You can fix it by adding a condition at the beginning of the method.
Naming: the method name search is too general. Consider a more specific one, like firstIndexOf or similar. The variable names set and addList also can be improved.
Arguments order: this is a personal opinion, but I would put the string being searched (text) before the string to search for (pattern).
Exception if not found: for such a function is common to return -1 instead of an exception when the pattern is not found. In doubt, document the method.
Testing: this method deserves more than one test, possibly using JUnit.
Complexity: the complexity seems to be \$O(textLenght*patternLength)\$. The outer for-loop iterates on text, while the inner for-loop won't do more than patternLength iterations otherwise pattern.charAt(index) will throw an exception. The time complexity of String#indexOf is \$O(n*m)\$ so  the main difference to your solution regarding complexity (worst case) is probably the space complexity. Anyway, it's better to focus on the correct functionality first and performance later.

As a reference this is the native implementation:
static int indexOf(char[] source, int sourceOffset, int sourceCount,
        char[] target, int targetOffset, int targetCount,
        int fromIndex) {
    if (fromIndex >= sourceCount) {
        return (targetCount == 0 ? sourceCount : -1);
    }
    if (fromIndex < 0) {
        fromIndex = 0;
    }
    if (targetCount == 0) {
        return fromIndex;
    }

    char first = target[targetOffset];
    int max = sourceOffset + (sourceCount - targetCount);

    for (int i = sourceOffset + fromIndex; i <= max; i++) {
        /* Look for first character. */
        if (source[i] != first) {
            while (++i <= max && source[i] != first);
        }

        /* Found first character, now look at the rest of v2 */
        if (i <= max) {
            int j = i + 1;
            int end = j + targetCount - 1;
            for (int k = targetOffset + 1; j < end && source[j]
                    == target[k]; j++, k++);

            if (j == end) {
                /* Found whole string. */
                return i - sourceOffset;
            }
        }
    }
    return -1;
}


Game Of Life OOP Python This is pretty much an OOP version of this question, with the improvements:
import copy
import shelve

class GameOfLife(object):
    def __init__(self, board, generations = 10):
        self.__board = board
        for i in range(generations):
            print(self)
            self.__nextGeneration()

    def __str__(self):
        string = ""
        for row in self.__board:
            for cell in row:
                if cell:
                    string += "#"
                else:
                    string += "."
            string += "\n"
        return string

    def __isInRange(self, row, cell):
        return 0 <= row < len(self.__board) and \
               0 <= cell < len(self.__board[0])

    def __countSurrounding(self, row, cell):
        SURROUNDING = ((row - 1, cell - 1),
                       (row - 1, cell    ),
                       (row - 1, cell + 1),
                       (row    , cell - 1),
                       (row    , cell + 1),
                       (row + 1, cell - 1),
                       (row + 1, cell    ),
                       (row + 1, cell + 1))
        count = 0
        for surrRow, surrCell in SURROUNDING:
            if self.__isInRange(surrRow, surrCell) and \
               self.__board[surrRow][surrCell]:
                count += 1
        return count

    def __nextGeneration(self):
        nextBoard = copy.deepcopy(self.__board)
        for row in range(len(self.__board)):
            for cell in range(len(self.__board[0])):
                if self.__board[row][cell] and \
                   self.__countSurrounding(row, cell) not in (2, 3):
                    nextBoard[row][cell] = 0
                elif not self.__board[row][cell] and \
                     self.__countSurrounding(row, cell) == 3:
                    nextBoard[row][cell] = 1
        self.__board = nextBoard

def main():
    boardFile = shelve.open("boardFile.dat")
    board = boardFile["board"]
    game = GameOfLife(board)

if __name__ == "__main__":
    main()

Where boardFile.dat is just a shelved file containing a boolean array.
Can anyone provide an honest code review?
Thanks.

Object-Orientedness

I find your object-oriented design deficient.  Basically, it's a bunch of procedural code that has been thrown into a class.  But what can you do with the GameOfLife object that you instantiate?  Nothing!

I would split the functionality into a Board class and a GameOfLife class.  The Board should be able to provide its own string representation, report the contents of a cell, set the contents of a cell, check if a coordinate exists, clone itself, and perhaps load from or save to a file.  The GameOfLife class would be a generator; each time you call life.next() it would produce a new board.

Your main() function should look like this:

board = Board.load('boardFile.dat')
game = GameOfLifeGenerator(board)
for _ in range(11):
    print(board)
    board = game.next()

Alternatively,

from itertools import islice

board = Board(15)
board.fill(row=7, col=7)
for state in islice(GameOfLifeGenerator(board), 11):
    print(state)

Naming

You shouldn't be using double-underscore variables.  If you want to suggest that an instance variable is private, use a name with a single underscore.

You use cell to mean column number, which is confusing terminology.  A "cell" should refer to one of the points on the grid; a cell has row and column coordinates.  (In contrast, I don't object as much to using row as shorthand for the coordinate of a row.  English is just weird that way.)

In __countSurrounding(), the SURROUNDING list shouldn't be all caps.  I'd consider that to be a variable rather than a constant.  (You could define it as SURROUNDING_OFFSETS = ((-1, -1), (-1, 0), ...) instead, which would be a constant.)  Also consider laying out the code this way for quick visualization:

surrounding = ((row - 1, col - 1), (row - 1, col), (row - 1, col + 1),
               (row    , col - 1),                 (row    , col + 1),
               (row + 1, col - 1), (row + 1, col), (row + 1, col + 1))

Miscellaneous

In __isInRange() and __nextGeneration(), you use len(__board[0]) as the width.  I would prefer that you use len(__board(row)) instead.

The __str__() function could be implemented more succinctly as

def __str__(self):
    return "\n".join(
        [''.join(
            ['#' if cell else '.' for cell in row]
         ) for row in self._board]
    ) + "\n"

or

def __str__(self):
    return "\n".join(
        map(lambda row: ''.join(
            map(lambda cell: '#' if cell else '.', row)
        ), self._board)
    ) + "\n"


Find the longest sequence Problem:
Find the longest sequence consisting only of 0's, and the longest sequence consisting only of 1's?
Solution:
import sys

longestSequenceForOne = None
startIndexForOne = None
longestSequenceForZero = None
startIndexForZero = None

def setLongestSequence(array, longestSequence, startIndex):
    global longestSequenceForOne
    global startIndexForOne
    global longestSequenceForZero
    global startIndexForZero

    if array[startIndex] == 1:
        if longestSequenceForOne == None or longestSequenceForOne < longestSequence:
            longestSequenceForOne = longestSequence
            startIndexForOne = startIndex
    elif array[startIndex] == 0:
        if longestSequenceForZero == None or longestSequenceForZero < longestSequence:
            longestSequenceForZero = longestSequence
            startIndexForZero = startIndex

def findLongestSequence(array, size):
    """
    Find the longest sequence of 0's and 1's
    """
    longestSequence = 1
    beginIndex = None
    index = 1
    startIndex = 0
    sameElementCount = 1

    while (index < size):
        if array[index] == array[index-1]:
            sameElementCount += 1
        elif sameElementCount > longestSequence:
            longestSequence = sameElementCount
            startIndex = index - sameElementCount
            sameElementCount = 1
            setLongestSequence(array, longestSequence, startIndex)
        else:
            sameElementCount = 1
        index += 1
    if sameElementCount > longestSequence:
        longestSequence = sameElementCount
        startIndex = index - sameElementCount
        setLongestSequence(array, longestSequence, startIndex)

if __name__ == '__main__':
    size = int(input())
    if size < 2 or size > 100:
        sys.exit()
    array = [int(x) for x in input().split() if x == '1' or '0']
    if len(array) != size:
        sys.exit()
    findLongestSequence(array, size)
    print(startIndexForZero, longestSequenceForZero)
    print(startIndexForOne, longestSequenceForOne)

Correctness
Input

12
0 1 0 1 1 1 0 1 0 0 0 0

Output

8 4
3 3

Input
17
1 1 0 1 0 1 0 0 1 0 0 0 0 1 0 1 0

Output

9 4
0 2

Questions:

Does the code require better error handling?

Can this code get more pythonic?

Correctness

Input
1 1 1 1 0 0 0

Output
None, None
0, 4

The reason is obvious: the longestSequence does not distinguish between longest sequence of zeroes and longest sequence of ones, so 0 0 0 is not considered long enough.
General

Avoid globals; prefer returning values.
There is no need to pass size; a list already knows its size. For the same reason, size = int(input()) is redundant and reduces usability.


Random number guessing game in python I just started learning Python this week and put this together for my first program (outside of a Hello World one). Does anyone have any feedback/suggestions for improvement for this? I've tried to forsee potential user input issues but I'd love to hear some thoughts from people who know what they are doing!

import random
guessesTaken = 0
# sets the guess counter
print ('Hello! What is your name?')
name = input()
# intro for user
print ('Hi, ' + name + ', I am thinking of a number between 1 and 100.' "\n" 'Do you want to try to guess my number? Please type yes or no.')
play = input()
# user inputs whether or not they want to play

while play != 'yes' and play != 'no':
    print ("I'm sorry, I didn't understand that. Please type yes or no.")
    play = input ()
# provision for invalid user input: if the input is not yes or no, they will be prompted to re-enter

    while play == 'yes':
    # the game continues
        number = random.randint (1,100)
    # the number will be between 1 and 100
        while guessesTaken < 20:
            print ('Take a guess!')
    # sets a max of 20 guesses
            try:
                guess = int(input())
            except:
                print ("I'm sorry, I didn't understand that. Please type a number between 1 and 100.")
                continue
    # provision for invalid input - if input is not an integer, user will be prompted to re-enter input
            guessesTaken += 1
    # adds to the guess counter
            if guess < number:
                print ('Your guess is too low!' "\n" 'Try again. You have ' + str(20-guessesTaken) + ' guesses left.')
            elif guess > number:
                print ('Your guess is too high!' "\n" 'Try again. You have ' + str(20-guessesTaken) + ' guesses left.')
            elif guess == number:
                break
    # if the guess is too high or too low, the loop continues. If the user guesses right, the loop ends
        if guess == number:
            print ('You got it right!' "\n" 'It only took ' + str(guessesTaken) + ' guesses!')
    # in case of correct guess within the allowed number of tries
        else:
            print ('Sorry, the number I am thinking of is ' + str(number) + '.')
    # if the user exceeds the number of guesses allowed
        guessesTaken = 0
    # resets guess counter
        print ('That was fun, ' + name + '! Want to play again?' "\n" 'Please type yes or no.')
        play = input ()

# asks if user wants to play again, with provision for invalid input
if play == 'no':
    print ("That's too bad, see you next time!")
# ends the game

Commenting

Almost universally, comments go before the code they're commenting, rather than after. Conforming to what people expect will make it much easier to read.

Comments should describe why, not what.

# the game continues
while play == 'yes':

# sets a max of 20 guesses
while guessesTaken < 20:
        print ('Take a guess!')

# adds to the guess counter
guessesTaken += 1

That's all examples of where you're describing something that's kind of obvious from looking at the code. This makes it harder to read, because they's more going on. Here's an example of a better comment:

try:
    # If the user enters a number out of range, still counts as a guess
    guess = int(input())
except:
    print ("I'm sorry...")
    continue

Code to show intentions

I'm guessing you haven't learned about functions, so I won't say too much here. They're a good tool that would help a lot to make things more clear. Instead, let's talk about your main loop.

while play != 'yes' and play != 'no':
    print ("I'm sorry, I didn't understand that. Please type yes or no.")
    play = input ()

This looks like a section to do input validation. This is why @Graipher asked if the indentation afterward was correct. To make it more clear, break up the continuation from the validation:

while play != "no":

    #Invalid input
    if play != "yes":
        print ("I'm sorry, I didn't understand that. Please type yes or no.")
        play = input ()
        continue

It's also common to do something like while True, then explicitly break out whenever you're done.

Now, guessesTaken is in a lot of places it doesn't really need to be, which makes it hard to keep track of. Instead of setting it at the beginning, just set it at the beginning of the loop.

while play == 'yes':
    number = random.randint (1,100)
    guessesTaken = 0

This way, your initialization happens in one place, when you use the variable you don't have to go back as far to see what it is, and you don't have to reset it at the end.

Everything is kind of running together. You can use blank lines (and functions!) to break it into logical sections. In the play loop, you can put a blank line in between when you get the guess, and when you check it. Indenting your comments to align with the rest of the code will also make it easier to see the blocks.

Finally, while play == 'yes': This should really be if instead of while. Right now, you have three different loops, which makes it hard to keep track of where the breaks and continues are sending you.


Bash Compress Managed Directory Ignoring .git, bower_components, node_modules, etc Fairly easy way to compress a wordpress plugin (or similar) as a zip file without including all the dependencies used to create and manage the development, but which don't belong in the final product.

#!/bin/bash
# This script zips a directory, excluding specified files, types and subdirectories.
#  while zipping the directory it excludes hidden directories and certain file types

[[ "`/usr/bin/tty`" == "not a tty" ]] && . ~/.bash_profile

DIRECTORY=$(cd `dirname $0` && pwd)

if [[ -z $1 ]]; then
  echo "Usage: managed_directory_compressor /your-directory/ zip-file-name"
else
  DIRECTORY_TO_COMPRESS=${1%/}
  ZIPPED_FILE="$2.zip"

  COMPRESS_IGNORE_FILE=("\.git" "*.zip" "*.csv" "*.json" "gulpfile.js" "*.rb" "*.bak" "*.swp" "*.back" "*.merge" "*.txt" "*.sh" "bower_components" "node_modules")
  COMPRESS_IGNORE_DIR=("bower_components" "node_modules")

  IGNORE_LIST=("*/\.*" "\.* "\/\.*"")
  if [[ -n $COMPRESS_IGNORE_FILE ]]; then
      for IGNORE_FILES in "${COMPRESS_IGNORE_FILE[@]}"; do
          IGNORE_LIST+=("$DIRECTORY_TO_COMPRESS/$IGNORE_FILES/*")
      done
      for IGNORE_DIR in "${COMPRESS_IGNORE_DIR[@]}"; do
          IGNORE_LIST+=("$DIRECTORY_TO_COMPRESS/$IGNORE_DIR/")
      done
  fi

  zip -r "$ZIPPED_FILE" "$DIRECTORY_TO_COMPRESS" -x "${IGNORE_LIST[@]}" # >/dev/null
  # echo zip -r "$ZIPPED_FILE" "$DIRECTORY_TO_COMPRESS" -x "${IGNORE_LIST[@]}" # >/dev/null
  echo $DIRECTORY_TO_COMPRESS "compressed as" $ZIPPED_FILE.
fi

I'm just going to poke at some of your unix conventions and validation.

Your general handling looks polished. I like the way that you handle whether this is an interactive terminal, but wonder whether it would simply be better to use the more standard check for $PS1 or the i flag ( reference here ). If you are going to use tty to tell, you should probably rely on the exit status rather than the text message.

The way you find the user directory is great, and is similar to what I normally do too. There are some variations on it, but it is more than good enough.

This next one is the one that concerns me most though:

if [[ -z $1 ]]; then
  echo "Usage: managed_directory_compressor /your-directory/ zip-file-name"
else
  .....

That code has three problems....

your script takes 2 parameters, but you only check for 1.
if the parameter is missing, you still exit with a success exit code (0). You should exit with something else .. (like 1).
there is no need for an else condition. Treat it like a guard-clause...

This is how I would do it (and I would use $# and not $2 because having too many arguments is as bad as not having enough....

if [[ $# -ne 2 ]]; then
  echo "Usage: managed_directory_compressor /your-directory/ zip-file-name"
  exit 1
fi

Then there is no need to indent, or so on, for the remaining code.

The remaining code looks relatively good. There's no error handling on the zip operation though.

One last thing, your first comments are duplicated:

# This script zips a directory, excluding specified files, types and subdirectories.
#  while zipping the directory it excludes hidden directories and certain file types

If you're going to comment things, make them good.


File locking between server threads I have written the below code where the server is getting requests from clients on a socket and creating a thread per client. Each client thread is then writing to a file which is common to all the threads. The file has been opened when main starts, so the same fd is being used by each thread. In such a scenario I have tried to achieve locking on file when one thread is writing to it. Since the threads are of same process so flock can't simply lock the file, hence mutex is used.

/*Logging function*/
void write_to_file(int op_fd,char *name)
{
   flock(op_fd,LOCK_EX);
   pthread_mutex_lock(&f_lck);
      write(op_fd,name,20);
   pthread_mutex_unlock(&f_lck);
   flock(op_fd,LOCK_UN);
}

/*Client thread function*/

void *clnt_thread(void * arg)
{
  int this_fd=*(int *)arg;
  /*str is a continuous memory array of the size of the dat_com struct.This will be filled up initially on receive as a normal array
   , but then it'll typecasted to point to struct dat_com object*/
  char str[sizeof(dat_com)],str_send[25];

  memset(&str,'\0',sizeof(str));
  dat_com *incm_dat;    // struct to contain the incoming data from client .

  while(1)
  {
  read(this_fd,str,sizeof(str));
  /*typecast to struct dat_com so that it is ready to be inserted into linked list or file.*/
  incm_dat=(dat_com *)&str;

   if(strncmp(str,"quit",4)==0)
    break;

  /*Call write to file function*/
  write_to_file(o_fd,incm_dat->name);
  fprintf(stderr,"Client said: %s",incm_dat->name);

  /*client wants to close connection?*/

  sprintf(str_send,"%s",incm_dat->name);
  send(this_fd,str_send,sizeof(incm_dat->name),MSG_NOSIGNAL);
  }
close(this_fd);
return 0;
}

This is working as expected at the moment. Is it good practice to lock this way, or is there any other best practice? If I have to put this code into production, what changes should I make that are not standard practice?

There is no reason to use flock and  a mutex, unless you are sharing
with another process (as opposed to thread).  I would just use the mutex and
remember to check the return values from the pthread_* and write calls.

Some other comments:

name in write_to_file should be const and the write call in that
function should use the length of the name, not an assumed 20.
you need to check the status of function calls (read, write, send
etc).
depending upon where the data being written is coming from and what it
contains, you may have byte-ordering problems that are not handled.
Since you are mixing string data and dat_com structures on the socket, you
risk getting them mixed up.  You'd be better sticking with one format, maybe
strings and have a way to check alignment and log/recover-from errors.
Your fprint calls are not synchronized across threads and may end up
interleaved.
When sending the name, there is no need to copy it into a string first.

sprintf(str_send,"%s",incm_dat->name);
send(this_fd,str_send,sizeof(incm_dat->name),MSG_NOSIGNAL);

just send it directly:

send(this_fd, incm_dat->name, strlen(incm_dat->name), MSG_NOSIGNAL);

Your variable and type names are poorly chosen.  dat_com means little,
this_fd should be just fd, incm_dat could be just data, o_fd as a
global should be longer and more descriptive of function.


Ackermann function in Python 2.7 I'm relatively new to Python. I just want some constructive feedback on how to improve my code efficiency, style, error handling, etc.

In this case, I've programmed the Ackermann function, but it won't evaluate well since it's built to handle the recursions relatively poorly - I'll have to fix that in the future. Any tips?

Also, if there are any areas of exploitation in the errors, please let me know too.

For reference, I'm programming this on Ubuntu 16.04 LTS and the language is Python 2.7.12

# Make sure the collected values are of the appropriate forms
def collection ():
    while True:
        # Collect raw_input for m_val and n_val
        m_val = raw_input ("Please enter a nonnegative integer (m) : ")
        n_val = raw_input ("Please enter another nonnegative integer (n): ")

        # Make sure input is acceptable
        try:
            m_val = float(m_val)
            n_val = float(n_val)

            # m_val needs to be a nonnegative integer
            # Check equivalence to 0 first
            if m_val == 0:
                pass
            # Then check if the value is nonintegral
            elif int(m_val) - m_val:
                print("\nYou entered a float for m!\n")
                continue
            # Then make sure the value is nonnegative
            elif m_val < 0:
                print("\nYou entered a negative value for m!\n")
                continue

            # n_val needs to be a nonnegative integer
            # Check equialence to 0 first
            if n_val == 0:
                pass
            # Then check if the value is nonintegral
            elif int (n_val) - n_val:
                print("\nYou enterd a float for n!\n")
                continue
            # Then make sure the value is nonnegative
            elif n_val < 0:
                print("\nYou entered a negative value for n!\n")
                continue

            # If the code makes it this far, it should be ready for return
            return int(m_val), int(n_val)

        except ValueError:
            print("\nPlease enter numerical values for m and n.\n")

def ackermann(m,n):
    if m == 0:
        return n + 1
    elif m > 0 and n == 0:
        return ackermann(m-1,1)
    elif m > 0 and n > 0:
        return ackermann (m-1, ackermann(m,n-1))
    else:
        print "The Value Doesn't Go into the Domain!"

(m,n) = collection()
print ( ackermann(m,n) )

def ackermann(m,n):
    ...
    else:
        print "The Value Doesn't Go into the Domain!"

Printing the error message to standard output looks like a bad design choice (if you code is run automatically and no one is checking the output, it would just silently return None. I don't think you want it). It would more reasonable to throw an exception in this case (I'd throw an instance of the ValueError) because a situation is exceptional (the input is invalid and there's no meaningful way for this function to handle it). Moreover, one function should be responsible for one thing. Your code computes the value of the ackermann's function and does the logging at the same time. It's another reason to handle invalid inputs using exceptions.

You can also simplify the code in the collection function by parsing the input as an integer. There's no point in parsing it as a float and then checking if it's an integer. You could rely on a standard int function to do this job for you. This way, it would be enough to check that both numbers are non-negative.

Now let's talk about variable naming:

collection is a strange name for a function. It's name doesn't give any clue about what it actually does.  Even something simple and generic like read_input would be better.
It's a bad practice to use one variable for two different things. n_val and m_val stand for the user's input as a string and then they turn into integers. I'd rather create two separate variables for each of them (one for the input and the other one for it's integral value).

You use of whitespace is inconsistent. For instance, there's a space after the name of the collection function, but there's none after the name of the ackermann function. Whatever style you choose, you should always be consistent with it (according to the PEP standard, there should be no space after the function name, but there should be one after the comma. I would stick to it unless I have compelling reasons not to).

It's also a good practice to write doc comments for all your functions and classes. The comments inside the code should be more about telling why the code does what it does or why a specific design decision was made. Redundant comments that just tell what the code does create noise and actually make it less readable. For instance, this comment is self-evident:

# Check equivalence to 0 first
if m_val == 0:
    pass

You can just remove it (and all other similar comments). Moreover, if you need to make a comment about what the piece of code does (like "# Then check if the value is nonintegral"), it's a hint that it should probably be implemented as another function with a descriptive name. You should strive for a self-documenting code.


How can this layered PHP architecture be improved? This is my first "large scale" system I've designed for myself. Since I'm more of a front end developer than backend developer, I'm not sure if my architecture is sound.

I've created a Factory class which I use to create all my classes and connect to the DB. I think I've tried to use used Singelton(?) pattern, but I'm not really sure.

It probably would be easier if you knew more about the system, but in short it's an online directory mixed with user interaction and social media.

It would be great if I could get some feedback on this.

My factory class

include_once('abc.class.php');

class Factory {
  // One function for each class I include
  // This is the only place I include class files
  function new_abc_obj($id = NULL) { return new Abc(Conn::get_conn(), $id); }
}

My DB connection class

class Conn {
  private static $conn = NULL;

  /* Establish PDO DB connection */
  private static function init() {
      $conf = self::config();
      try {
        self::$conn = new PDO($conf['dsn'], $conf['user'], $conf['pass'], array(PDO::MYSQL_ATTR_INIT_COMMAND => "SET NAMES utf8"));
      }
      catch (PDOException $e) {
        // We remove the username if we get [1045] Access denied
        if (preg_match("/\b1045\b/i", $e->getMessage()))
          echo "SQLSTATE[28000] [1045] Access denied for user 'name removed' @ 'localhost' (using password: YES)";
        else
          echo $e->getMessage();
      }
  }

  /* Get DB connection */
  public static function get_conn() {
    if (!self::$conn) { self::init(); }
    return self::$conn;
  }

  /* Get username, password and db data from config.php */
  private static function config() {
    $conf = array();

    $conf['user']    = DB_USER;
    $conf['pass']    = DB_PASSWORD;
    $conf['dsn']     = 'mysql:dbname='.DB_NAME.';host='.DB_HOST;

    return $conf;
  }
}

My class interface

interface iAbc {
  public function one();
  public function two();
}

My Business Logic Layer (BLL)

class Abc extends AbcDAO implements iAbc {
  private $data = array();
  protected $db;

  function __construct(&$db, $id) {
    $this->db = &$db;

    if($id) {
      $this->setData($id);
    }
  }

  // __set, __get, __isset is here

  public function one(){};
  public function two(){
    $result = parent::get_some_data();
    return $result;
  };
}

My Data Access Layer (DLL)

class AbcDAO {
  protected function get_some_data($name){
    $sql ="SELECT 123;";
    $stmt = $this->db->prepare($sql);
    $stmt->bindParam(':name', $name, PDO::PARAM_STR);
    $stmt->execute();
    $result = $stmt->fetchAll();
    return $result;
  }
}

And this is how I create the object:

$my_abc = Factory::new_abc_obj();
$test = $my_abc->two();

Factory

Three things about this:

Consider using an autoloader (see PRS-0). Then you don't have to bother about, where files get included.
A Factory is used to create a set of objects, which depends on some context. For example, you could have a Filter and a Renderer for XHTML or HTML5. In order not having to check which one is needed again and again, you have an XhmtlFactory and an Html5Factory with createFilter() and createRenderer() methods.
Factory methods usually are named create....

In your context, a Factory is not helping you.

DB Connection

Don't use static methods, if you can avoid it. Inject dependencies whereever possible. Do not (finally) handle errors within your classes. Leave that up to the caller. The method to get an instance is usually called getInstance() (and is one of the few cases, where static calls are reasonable). The initialization is then made in a private constructor.

class Conn
{
    private $conn = null;

    /**
     * Establish PDO DB connection
     */
    private function __construct(Config $conf)
    {
        try {
            self::$conn = new PDO($conf->dsn, $conf->user, $conf->pass, array(PDO::MYSQL_ATTR_INIT_COMMAND => "SET NAMES utf8"));
        } catch (PDOException $e) {
            $message = $e->getMessage();
            // We remove the username if we get [1045] Access denied
            if (preg_match('/\b1045\b/i', $message)) {
                $message = "SQLSTATE[28000] [1045] Access denied for user 'name removed' @ 'localhost' (using password: YES)";
            }
            throw new ErrorException($message);
        }
    }

    /**
     * Get DB connection
     */
    public static function getInstance(Config $conf)
    {
        if (is_null($this->$conn)) {
            $this->conn = new Conn($config);
        }
        return $this->conn;
    }
}

class Config
{
    public $dsn  = 'mysql:dbname=' . DB_NAME . ';host=' . DB_HOST;
    public $user = DB_USER;
    public $pass = DB_PASSWORD;
}

To make the singleton pattern in Conn complete, the magic method __clone() should made private, so the connection cannot be copied.

Business Logic Layer

Keep the business and data access layer separated. The business layer ideally does not know anything about storage. The data access layer is injected into the business layer.

class Abc implements iAbc
{
    private $data = array();
    protected $dal;

    public function __construct($dal, $id = null)
    {
        $this->dal = $dal;

        if (!is_null($id)) {
            $this->setData($id);
        }
    }

    // __set, __get, __isset is here

    public function one(){};

    public function two()
    {
        $name = 'foo';
        $result = $this->dal->get_some_data($name);
        return $result;
    };
}

Data Access Layer

class AbcDao
{
    private $db = null;

    public function __construct(Conn $db)
    {
        $this->db = $db;
    }

    public function get_some_data($name)
    {
        $sql ="SELECT 123;";
        $stmt = $this->db->prepare($sql);
        $stmt->bindParam(':name', $name, PDO::PARAM_STR);
        $stmt->execute();

        $result = $stmt->fetchAll();
        return $result;
    }
}

Controller

In your controller, you now really have control.

$config = new Config;        // You could use any other set of config data
$conn   = new Conn($config); // You could use any other storage method
$my_abc = new Abc($conn);
$test   = $my_abc->two();

or, shorter

$my_abc = new Abc(new Conn(new Config));
$test   = $my_abc->two();


Website with images that pan to different sides I've just started programming with JavaScript and I am currently working on this hobby website project of mine. The site is supposed to display pages filled with product images than can be "panned" to either the left or right. Each "page" containing about 24 medium sized pictures, one page almost completely fills out an entire screen. When the user chooses to look at the next page he needs to click'n'drag to the left (for example) to let a new page (dynamically loaded through an AJAX script) slides into the view.

This requires for my JavaScript to "slide" two of these mentioned pages synchronously by the width of a screen. This results in a really low framerate. Firefox and Opera lag a bit, Chrome has it especially bad: 1 frame of animation takes approx. 100 milliseconds, thus making the animation look very "laggy".

I do not use jQuery, nor do I want to use it or any other library to "do the work for me". At least not until I know for sure that what I am trying to do can not be done with a couple of lines of self-written code.

So far I have figured out that the specific way I manipulate the DOM is causing the performance-drop. The routine looks like this:

function slide(){
this.t=new Date().getTime()-this.msBase;

if(this.t>this.msDura)
{
this.callB.call(this.ref,this.nFrames);
return false;
}

//calculating the displacement of both elements
//
this.pxProg=this.tRatio*this.t;

this.eA.style.left=(this.pxBaseA+this.pxProg)+'px';
this.eB.style.left=(this.pxBaseB+this.pxProg)+'px';

if(bRequestAnimationStatus)requestAnimationFrame(slide.bind(this));
else window.setTimeout(slide.bind(this),16);

this.nFrames++;

};

...

//starting an animation
//
slide.call({ 'eA':theMiddlePage,
'eB':neighboorPage,
'callB':theCallback,
'msBase':new Date().getTime(),
'msDura':400,
'tRatio':((0-pxScreenWidth)/400),
'nFrames':0,
'ref':myObject,
'pxBaseA':theMiddlePage.offsetLeft,
'pxBaseB':neighboorPage.offsetLeft
        });

I've noticed that when I let the AJAX script load less images into each page, the animation becomes much faster. The separate images seem to create more overhead than I have expected. Is there another way to do this?

(Reposted from Stack Overflow here)

Well I can only guess without seeing the code where the DOM is actually accessed but one thing that looks big to me is that you appear to be manipulating each individual element in seperate frames. If you did the calcs in one function for all elements that would be (I believe) one set of reflow calcs set off in the browser rather than per elements-moved.

Basically any time there is a change to the DOM a series of reflow calculations is set off in the browser. I think when say three DOM changes are made in one function call, the browser is aware of all three before it starts doing anything. Whereas with seperate funcs it does that same work multiple times.

How much reflow calculation gets set off for an action on the DOM in any given browser isn't something I'm a real wizard on but IIRC tends to be a fairly crude operation that doesn't necessarily take "out of flow" elements like absolutely positioned containers into consideration. That said, I still tend to find the DOM seems to have an easier time of it when you try to optimize by taking movable elements out of flow first.

If you don't need to support older browsers, look into CSS3 animation approaches. These can drastically improve animation performance since browsers can optimize for actions before they happen.

Barring that, avoid touching the DOM as much as possible in loops. If you must access properties on a DOM element multiple times, don't keep reacquiring it. Assign it to a var, effectively caching that operation (note: might not matter in newer browsers). But actually making changes to the DOM is where you need to optimize the most. Sidestep with pre-set CSS3 animations or follow some of the older-school stuff I've outlined here and we can examine in more depth if needed.


Critique wanted on JS I'm trying to re-learn some JS fundamentals and wondered if anyone knowledgabe could tell me if I'm doing something very wrong in my approach to this simple word counting class.

I want to create a class that takes arbitrary text and returns information about its wordcount, frequency of words, etc. One of the methods I have so far returns a hash where each key coresponding to a word has an element called count which is its frequency in the text.

One usage is to produce an output that will be printed to a file with WORD COUNT on each line.

function WordCounter(){
   this.text = new String()
}

WordCounter.prototype.addWords = function(words){
    this.text += words
}

WordCounter.prototype.count = function(){
    return this.text.split(" ").length
}
WordCounter.prototype.getFrequenciesAsHash = function(){
    var words = this.text.split(" ")
    var frequencies = {}
    while(words.length > 0){
        var word = words.pop()
    if(typeof frequencies[word] == "undefined"){
        frequencies[word] = {}
        frequencies[word].count = 0
    }
    frequencies[word].count++
    }
    return frequencies
}

Conceptually, I feel like you are doing this backwards. You should be storing an array of strings instead of repeatedly splitting a string into an array.

Here's my take on it:

function WordCounter(){
   this.words = []; // You should just use an array. It makes more sense here.
}

WordCounter.prototype.addWords = function(){
    // this allows you to pass in words as multiple arguments, or as an array of words
    // i.e.:
    // myWordCounter.addWords(["one","two","three"]);
    // or
    // myWordCounter.addWords("one", "two", "three");
    this.words = this.words.concat([].slice.call(arguments));
}
WordCounter.prototype.count = function(){
    return this.words.length;
}
WordCounter.prototype.toString = function(){
    //this replaces the text property
    return this.words.join(" ");
}
WordCounter.prototype.getFrequenciesAsHash = function(){
    // reduce is much more elegant here.
    // if you have to support older browsers, just use a shim to fill in array method support
    return this.words.reduce(function(hash, word){
        // no need to do a typeof.
        if(hash[word] === undefined)
            hash[word] = 0; // no reason to create an object

        hash[word]++;

        return hash;
    }, {});
}

If you want to be able to add words from a multi-word string, I would suggest a separate method for clarity:

WordCounter.prototype.extractWords = function(string){
    this.addWords(string.match(/\w+/g));
}

The naming could be better.


Calculator and Formatter classes Calculator class:

class Calculator
{
    /**
     * @return array
     */
    function doPowTable ($n)
    {
        $table = array();
        for ($i = 1; $i <= $n; $i++)
        {
            $table[] = array(
                'i' => $i,
                'factor' => pow($i, 2)
            );
        }
        return $table;
    }
}

Formatter class:

class Formatter
{
    /**
     * @return array
     */
    function getAndFormat ($n)
    {
        $table = (new Calculator()).doPowTable ($n);
        return array_map (function($item) {
            $item['i_factor'] = $item['i'].'_'.$item['factor'];
            return $item;
        }, $table);
    }
}

They say it's bad because the helper hard-codes the Calculator. But I think it's how it is supposed to be. It would have been better if I didn't give the $n to it but an array created by the doPowTable() method. Why?

What if I want to put the Formatter class under test? How can I mock the Calculator dependency if it is hard-coded?

I suggest you put that into a separate public method so that it would be possible for other developers to create a partial mock and test it. It will also be clearer since you start reading the class what its dependencies are.

class Formatter
{
    /**
     * @var Calculator
     */
    private $calculator;

    /**
     * @param Calculator $calculator
     * @return $this
     */
    public function setCalculator(Calculator $calculator)
    {
        $this->calculator = $calculator;

        return $this;
    }

    /**
     * @return Calculator
     */
    public function getCalculator()
    {
        return $this->calculator;
    }

    /**
     * @return array
     */
    function getAndFormat ($n)
    {
        $table = $this->getCalculator()->doPowTable($n);

        return array_map (function($item) {
            $item['i_factor'] = $item['i'].'_'.$item['factor'];
            return $item;
        }, $table);
    }
}

This way you can either provide a different calculator or a mocked one for testing purposes.

Please consider the example below. It shows how to create a partial mock of the Formatter class to override the Calculator dependency.

$mockedCalculator = $this->getMock(Calculator::class);

// set your mocked calculator behaviour here

$formatter = $this->getMockBuilder(Formatter::class)
    ->disableOriginalConstructor()
    ->setMethods(['getCalculator'])
    ->getMock();

$formatter
    ->expects($this->once())
    ->method('getCalculator')
    ->willReturn($mockedCalculator);

$n = 123; // it's just an example
$this->assertEquals($expectedValue, $formatter->getAndFormat($n));


Dataset Copy, Change, then Write new Rows back I have this code, but I think it is too clumsy. It feels like a 12 point turn, is there a way to optimize it?

Please note that this generates new rows from existing rows.

//Create New Dataset
DataSet myDataSet = new DataSet();
DataTable myDataTable = myDataSet.Tables.Add("Temp");
myDataTable.Columns.Add("ID");
myDataTable.Columns.Add("JobNumber");
myDataTable.Columns.Add("DirectoryName");
myDataTable.Columns.Add("DrawingName");
myDataTable.Columns.Add("DateAdded");
myDataTable.Columns.Add("LastAccessedDate");
myDataTable.Columns.Add("LastAccessedUserName");
myDataTable.Columns.Add("ClientName");
myDataTable.Columns.Add("ContentType");
myDataTable.Columns.Add("Description");

//Read From Database
nexusDBDataSet.Drawing_Table.Clear();
drawing_TableTableAdapter.FillByJobNumber(nexusDBDataSet.Drawing_Table, OldJobcardNumber);
foreach (DataRow myRow in nexusDBDataSet.Drawing_Table.Rows)
{
    //Read from database
    string DirectoryName = myRow["DirectoryName"].ToString();
    string DrawingName = myRow["DrawingName"].ToString();
    string DateAdded = myRow["DateAdded"].ToString();
    string LastAccessedDate = myRow["LastAccessedDate"].ToString();
    string LastAccessedUserName = myRow["LastAccessedUserName"].ToString();
    string ClientName = myRow["ClientName"].ToString();
    string ContentType = myRow["ContentType"].ToString();
    string Description = myRow["Description"].ToString();

    //write to temp data table
    DataRow DrawingDataRow = myDataTable.NewRow();
    DrawingDataRow["JobNumber"] = NewJobcardNumber;
    DrawingDataRow["DirectoryName"] = DirectoryName.Replace("\\" + OldJobcardNumber + "\\", "\\" + NewJobcardNumber + "\\");
    DrawingDataRow["DrawingName"] = DrawingName;
    DrawingDataRow["DateAdded"] = DateAdded;
    DrawingDataRow["LastAccessedDate"] = LastAccessedDate;
    DrawingDataRow["LastAccessedUserName"] = LastAccessedUserName;
    DrawingDataRow["ClientName"] = ClientName;
    DrawingDataRow["ContentType"] = ContentType;
    DrawingDataRow["Description"] = Description;
    myDataTable.Rows.Add(DrawingDataRow);

}

//Refresh DataBase
nexusDBDataSet.Drawing_Table.Clear();
drawing_TableTableAdapter.Fill(nexusDBDataSet.Drawing_Table);

foreach (DataRow myRow in myDataTable.Rows)
{
    //read from temp data table
    string JobNumber = myRow["JobNumber"].ToString();
    string DirectoryName = myRow["DirectoryName"].ToString();
    string DrawingName = myRow["DrawingName"].ToString();
    string DateAdded = myRow["DateAdded"].ToString();
    string LastAccessedDate = myRow["LastAccessedDate"].ToString();
    string LastAccessedUserName = myRow["LastAccessedUserName"].ToString();
    string ClientName = myRow["ClientName"].ToString();
    string ContentType = myRow["ContentType"].ToString();
    string Description = myRow["Description"].ToString();

    //Write back to DataBase
    DataRow DrawingDataRow = nexusDBDataSet.Drawing_Table.NewRow();
    DrawingDataRow["JobNumber"] = JobNumber;
    DrawingDataRow["DirectoryName"] = DirectoryName;
    DrawingDataRow["DrawingName"] = DrawingName;
    DrawingDataRow["DateAdded"] = DateAdded;
    DrawingDataRow["LastAccessedDate"] = LastAccessedDate;
    DrawingDataRow["LastAccessedUserName"] = LastAccessedUserName;
    DrawingDataRow["ClientName"] = ClientName;
    DrawingDataRow["ContentType"] = ContentType;
    DrawingDataRow["Description"] = Description;

    nexusDBDataSet.Drawing_Table.Rows.Add(DrawingDataRow);
}

myDataSet.Dispose();
//Updating
this.Validate();
drawing_TableBindingSource.EndEdit();
tableAdapterManager.UpdateAll(nexusDBDataSet);
RefreshData();

So basically you make a copy of all rows for a specific job, replace the job number and append the changed row back to the database. Sounds like this should be doable in one loop with the help of ItemArray and no need for a temporary table:

nexusDBDataSet.Drawing_Table.Clear();
drawing_TableTableAdapter.Fill(nexusDBDataSet.Drawing_Table);
foreach (DataRow jobRow in nexusDBDataSet.Drawing_Table.Select(string.Format("JobNumber = '{0}'", OldJobcardNumber)))
{
    var newRow = nexusDBDataSet.Drawing_Table.NewRow();
    newRow.ItemArray = jobRow.ItemArray;

    string directoryName = jobRow["DirectoryName"].ToString();
    newRow["JobNumber"] = NewJobcardNumber;
    newRow["DirectoryName"] = DirectoryName.Replace("\\" + OldJobcardNumber + "\\", "\\" + NewJobcardNumber + "\\");

    nexusDBDataSet.Drawing_Table.Rows.Add(newRow);
}
// save table


Replacing text in an array I have this piece of code to replace current text inside an array of html divs with values coming from a JSON file:

    $.each($('.something'), function(){
        if($(this).text()=="1"){
            $(this).html(jsondata.CameraTypeLabel1);
        }else if($(this).text()=="2"){
            $(this).html(jsondata.CameraTypeLabel2);
        }
    });

I'd like to make it "dynamic": if I'd have 100 values, I don't want to copy/paste it 100 times.

Maybe it would be better creating an array "CameraTypeLabel" in the JSON string ?

EDIT: Fixed the selector, it was a typo.

Thanks!

jQuery's .html() method can take a callback function - it'll be called for every element in the collection - and will use the returned value to set the element's html.

You can use bracket notation to access the key in your jsondata, so that you can dynamically get to the key you want with the string in the brackets:

$('.something').html(function () {
    return jsondata[ 'CameraTypeLabel' + $.text(this) ];
});

Note: $.text(this) is the same as $(this).text(), just a bit faster; it doesn't create a new jQuery object at every step in the loop.

If all you want to do is update the text, you can pass a function to .text() itself:

$('.something').text(function (i, oldText) {
    return jsondata[ 'CameraTypeLabel' + oldText ];
});


Counting Facebook and Twitter Reactions I wrote / modified two scripts to count Facebook and Twitter reactions. Anything I can do to improve these?

Count Facebook Reactions

 class FacebookReactions
 {
    public $url;

    function __construct($url)
    {
        $query  = 'SELECT like_count, total_count FROM link_stat WHERE url="'.$url.'"';
        $apifql="https://api.facebook.com/method/fql.query?format=json&query=".urlencode($query);

        $json=file_get_contents($apifql);
        $fb_obj=json_decode($json);
        $fb_array=object_2_array($fb_obj);

        $this->total = $fb_array[0]['total_count'];
        $this->like = $fb_array[0]['like_count'];
    }
 }

Count Twitter Reactions

 class TwitterReactions
 {
    public $url = '';
    public $output = array();
    public $count = 0;
    public $total = 100;

    function __construct($url,$total)
    {
        $this->url = $url;

        $query = 'http://search.twitter.com/search.json?q='.$url.'&result_type=mixed&rpp='.$total;
        $reactions=file_get_contents($query);
        $reactions_array=json_decode($reactions, TRUE);

        foreach($reactions_array as $key => $results)
        {
            if(is_array($results)){
                foreach($results as $key => $result){
                    $this->output[$key]['user'] = $result['from_user'];
                    $this->output[$key]['image'] = $result['profile_image_url'];
                    $this->output[$key]['message'] = ereg_replace("[[:alpha:]]+://[^<>[:space:]]+[[:alnum:]/]","\\0", $result['text']);
                    $this->output[$key]['date'] =  date('m.d.y',strtotime($result['created_at']));
                }
            }
        }
        $this->count = count($this->output);
    }
 }

Both classes have undefined or unused class properties. The first class does not use the $url property and never defines the $total or $like properties. The second class just doesn't use the $total property. If these properties really aren't being used, then you can just remove them from the class. However, if you meant to use them, then you will have to do so using the following syntax: $this->url.

PHP will soon be deprecating default method types. In other words, methods must be explicitly defined as public, private, or protected. Right now they are all public by default, but there's no telling how long that will last. Plan for the future and start defining them now so you won't have to later.

I'm not sure where the object_2_array() function is coming from as this is not a function included in the default PHP library. If you are just trying to get the JSON object into an array, then you can do the same thing you did for your twitter JSON object and set the second parameter of json_decode() to TRUE.

Instead of explicitly defining the first array index, just shift the first element off the initial array. Makes it a little more pleasant to read and removes ambiguity as to where the "0" index came from. If you don't want to modify the existing array, you can just assign the value of the first array element to a different variable. However, as it stands this does not appear to matter as the initial array is not being used anyways. Both methods are shown below.

$results = $fb_array[0];
//OR
$results = array_shift( $fb_array );

$this->total = $results['total_count'];
$this->like = $results['like_count'];

When using foreach loops to iterate over an array, it is not always necessary to define the array key as a parameter, only if you are planning on using that value. So, on your first foreach loop in the twitter constructor you can remove the $key => bit as it is unnecessary. Also, keep in mind, it is usually a bad idea to overwrite a variable and should be avoided if not explicitly meant. Even though it is unused, you are overwriting your first $key variable with the second foreach loop. This is bad practice as it could lead to mistakes later. If you need another $key variable you can call it $key2 and the first $key1, or keep the first as it is and call the second key $index. Or anything else really, just so long as it makes sense.

With all that being said, I don't think it was necessary to create these as classes. These are simple enough to where you could have more easily created these procedurally. Additionally, please make sure code you submit for review is functional. As it stands, many of these errors looks like they could have been made from copy-pasting code together.


Random string of variable length generator Below is my code for generating a random string of random length. I am using it in integration tests. Currently I have almost 2000 tests running each time a commit is made to master and a few of those tests are using this class.

I would like to make sure the code is optimized so that the tests complete as quickly as possible.

public class Strings
{
    private static readonly Random Random = new Random();

    private static int RandomChar => Random.Next(char.MinValue, char.MaxValue);

    public static string GenerateRandomStringOfSetLength(int length)
    {
        return GenerateRandomString(length);
    }

    private static string GenerateRandomString(int length)
    {
        var stringBuilder = new StringBuilder();

        while (stringBuilder.Length - 1 <= length)
        {
            var character = Convert.ToChar(RandomChar);
            if (!char.IsControl(character))
            {
                stringBuilder.Append(character);
            }
        }

        return stringBuilder.ToString();
    }
}

Just two remarks:

RandomChar should be a method because it returns a different result each time - this is just a convention that we usually follow in C#

  In general, methods represent actions and properties represent data. Properties are meant to be used like fields, meaning that properties should not be computationally complex or produce side effects.

from Choosing Between Properties and Methods

so

  Do use a property, rather than a method, if the value of the property is stored in the process memory and the property would just provide access to the value.

but

  Do use a method, rather than a property, in the following situations.

amonong many others:

  The operation returns a different result each time it is called, even if the parameters do not change. For example, the NewGuid method returns a different value each time it is called.

You can cast the result to char and don't need the Convert.ToChar

_

private static char RandomChar() => (char)Random.Next(char.MinValue, char.MaxValue);

You can also improve the performance by precalculating the array with chars:

private static readonly char[] Chars =
    Enumerable
    .Range(char.MinValue, char.MaxValue)
    .Select(x => (char)x)
    .Where(c => !char.IsControl(c))
    .ToArray();

the RandomChar method would take the values from this array:

private static char RandomChar() => Chars[Random.Next(0, Chars.Length)];

so building the string can be a simple loop:

for (int i = 0; i < length; i++)
{
    stringBuilder.Append(RandomChar());
}

Without the StringBuilder this seems to be faster in tests by just ~8ms for 100.000 loops and a string lenght of 1.000

var chars = new char[length];
for (int i = 0; i < length; i++)
{
    chars[i] = RandomChar();
}
return new string(chars);

StringBuilder.ToString processes the data in order to build the string. It's quite fast but in this particular situation the minimal overhead is noticable. Generally the StringBuilder is the fastet way to build strings so using it is definitely the right choice.

Changing the Random.Next(0, max) to Random.Next(max) improves the performance by another 10ms for the same tests.


Printing a factorial from an integer I am very new to functional programming. I wrote this program where the user can enter an integer and it will print the factorial. If the user enters a negative integer or not an integer at all, it'll give the user a second chance.

This doesn't feel very functional to me. How can I make this code more functional (esp. the main function)? Or is this just fine?

module Main where
import Data.Char

factorial :: Integer -> Integer
factorial 0 = 1
factorial n = product [1..n]

isIntegral :: String -> Bool
isIntegral [] = False
isIntegral st = all isDigit st

main :: IO ()
main = do
  putStrLn "Enter a non-negative integer:"
  numberStr <- getLine
  if isIntegral numberStr
    then do
      let number = read numberStr :: Integer
      if number < 0
        then do
          main
        else do
          putStrLn $ show $ factorial number
    else do
      main

The line factorial 0 = 1 is redundant. The product of the empty list is already 1.

When writing a function that only works with strings (instead of arbitrary lists), I'd prefer to use "" instead of [] to denote the empty string. This way it's more obvious we're dealing with strings.

Since your isIntegral function will return false for negative numbers (because - is not a digit), you should name it something like isNaturalNumber instead.

Instead of applying read to the result of getLine, you can just use readLn. (Edit: since in your latest revision you do error checking on the string before using read, this no longer applies). Likewise you can use print instead of putStrLn and show.

Instead of having multiple $s on the same line, it is usually preferred to combine .s and $s. However using print you only need print $ factorial number anyway.

There is no point in prefixing a single expression with do, so you should remove the dos from your if and else.

Also since your isIntegral/isNaturalNumber function already returns false for negative numbers, there's no need to check whether the result is negative after calling read, so you can get rid of the if-statement and the duplication of calling main recursively from two different places.


Finding Second Largest Element in an Array Here's my implementation using divide and conquer in C++.

What do you think of this implementation regarding running time ?

#include <iostream>
#include <tuple>
using namespace std;

tuple <int,int> largest_two_numbers(int *arr,int size)
{
    //The algorithm is based on returning a tuple with the largest two numbers in the array

    if (size==1)
        return tuple<int,int>(arr[0],arr[0]);
    if (size==2)
        return tuple<int,int>(arr[0],arr[1]);

    //Split the arr into two arrays
    int arr_1[size/2],arr_2[size/2+(size&1)];
    for (int i = 0; i < size/2; ++i)
    {
        arr_1[i]=arr[i];
        arr_2[i]=arr[i+size/2];
    }
    if (size&1) arr_2[size/2]=arr[size-1];

    //Return two tuples one for the right half and the other for the left
    tuple <int,int> right= largest_two_numbers(arr_1,size/2);
    tuple <int,int> left = largest_two_numbers(arr_2,size/2+(size&1));

    //Declare an array maxis with the values inside the two tuples combined and find the largest two numbers
    int maxis[]={get<0>(right),get<1>(right),get<0>(left),get<1>(left)};
    int max1=maxis[0],max2=maxis[0];
    for (int i = 0; i < 4; ++i)
    {
        if (maxis[i]>max1) max1=maxis[i];
    }
    for (int i = 0; i < 4; ++i)
    {
        if (maxis[i]>max2 && maxis[i]<max1) max2=maxis[i];
    }

    //Return a tuple with the largest two numbers
    return tuple<int,int>(max1,max2);
}

int main(){
    int arr[]={-1,98,22,1,0,301,112},n=7;

    //Print the second largest element in the array which is the second element in the tupple
    cout << get<1>(largest_two_numbers(arr,n));
}

Inefficient algorithm

As stated in the comments, the algorithm is actually pretty convoluted. One cannot find the maximum without looking at all of the elements, if they don't have any idea about the pattern of the input.

On top of that, it allocates quite a lot of memory.

Non-standard C++

VLA (variable length array) is not part of C++. It might be an extension in your compiler

Using C++ as C

The code is extremely imperative. It has a lot of control that it doesn't use, and doesn't have any benefit over standard algorithms. It is also bound to int arrays, to pointers, in fact. May be as C code it would be ok (if algorithm problems are fixed), but this is not effective usage of C++ features.

Benchmarking

Usually when seriously talking about performance people also use benchmarks. Having some real statistics on multiple platforms will strengthen your argument.

Alternative approach

Now that we know that we need to pass through anyway, lets write simple loop over the whole range:

#include <functional>
#include <array>
#include <utility>

template <typename ForwardIterator, typename Compare = std::less<>>
std::array<ForwardIterator, 2> find_2greatest(ForwardIterator first,
                                              ForwardIterator last,
                                              Compare cmp = {})
{
    if (first == last)
    {
        return {first, first};
    }

    auto greatest = first;
    auto runner_up = greatest;

    for (++first; first != last; ++first)
    {
        if (cmp(*runner_up, *first))
        {
            runner_up = first;
            if (cmp(*greatest, *runner_up))
            {
                std::swap(greatest, runner_up);
            }
        }
    }

    return {greatest, runner_up};
}

#include <vector>
#include <iostream>

int main()
{
    std::vector<int> v{1, 1, 2, 3, 4, 5, 6};

    auto greatest2 = find_2greatest(std::begin(v), std::end(v));

    std::cout << "the greatest: " << *greatest2.front() << '\n'
              << "second greatest: " << *greatest2.back() << '\n';
}

The code doesn't return the objects themselves. They might be non-copyable, or user might want only the location of those elements. In general, C++ search algorithms usually return iterators. Also it is not bound to only vectors or ints, it can work on anything that allows multiple passed over the range. Your naming might be better than mine though.


Optimizing Python code for Project Euler #5 (LCM of all numbers from 1 to 20) This is my solution for the 5th problem of Project Euler, which asks for the least common multiples of the numbers from 1 to 20 (inclusive). Is there any way to improve the while loop conditional instead of using the sum?

table = range(1, 21)
result = 1
pf = 2
while sum(table) > len(table):
   flag = False
   for x, y in enumerate(table):
      if y % pf == 0:
         table[x] = y/pf
         flag = True
   if flag:
      result *= pf
   else:
      pf += 1
print result

Your intent is to terminate the loop when all entries in table become 1. This

while sum(table) > len(table):

is a rather obscure way of expressing that intent. I suggest instead

while max(table) > 1:

or the more explicit and short-circuiting

while any(x > 1 for x in table):


Finding longest word in a string I was working on a problem from Coderbyte. The challenge was to write a function that takes in a string and returns the longest word in the string. If two words are the same size then it asks to return the first one. The input will never be empty.

function longestword(str){
    var replaced = str.replace(/[^A-Za-z\s]/g,"");
    var final = replaced.split(" ").sort(function(a,b){return b.length - a.length})
    return final[0];
    }
longestword("This is a string theres two words that are the same size oh no!")
=>'string'

What do you think about my code?

longestword should be named using an interCaps naming convention.  Also, replaced and final aren't exactly helpful variable names.  However, if you were to have an intermediate result called words, it would be quite clear what that stands for.

It's good practice to terminate all statements with semicolons, even though JavaScript allows them to be omitted.

function longestWord(str){
    var words = str.replace(/[^A-Za-z\s]/g, "").split(" ");
    var wordsByDescendingLength = words.sort(function(a, b) {
        return b.length - a.length;
    });
    return wordsByDescendingLength[0];
}

Note that hyphenated words will have their hyphens dropped, both when comparing lengths and when returning the result.


Adding HTML DOM template I'm new to JS and jQuery. I wrote this function which just does what I want, but it seems very unreadable, and because of my experience, I don't know a better way to handle it.

Could you please give advise me how to refactor it?

function new_section(id, name, order) {

    return $('
')
        .attr('id', 'section_' + id)
        .attr('data-id', id)
        .attr('data-order', order)
        .append($('
')
            .addClass('head-row')
            .attr('id', 'section_' + id + '_head_row')
            .append($('')
                .attr('id', 'section_' + id + '_name_cell')
                .attr('colspan', window.row_length)
                .text(name)
                .addClass('bg-info')

                .append($('').addClass('edit-section').attr('href', '#')
                    .append($('').addClass('fa fa-pencil text-muted')))

                .append($('').addClass('pipe-edit-section text-muted').text('|'))

                .append($('').addClass('section-up').attr('href', '#')
                    .append($('').addClass('fa fa-chevron-up text-muted')))

                .append($('').addClass('section-down').attr('href', '#')
                    .append($('').addClass('fa fa-chevron-down text-muted')))

                .append($('').addClass('pipe-edit-section text-muted').text('|'))

                 .append($('').addClass('delete-section').attr('href', '#')
                    .append($('').addClass('fa fa-times-circle text-muted')))

        )
    );
}

UPD:

Found underscore templates. What do you think about this approach:

section = _.template([
            '<tbody id="section_<%= id %>" data-id="<%= id %>" data-order="<%= order %>">',
                '<tr id="section_<%= id %>_head_row" class="head-row">',
                    '<td id="section_<%= id %>_name_cell" class="bg-info" colspan="<%= row_length %>">',
                        '<%= name %>',
                        '',
                            '',
                        '',
                        '|',
                        '',
                            '',
                        '',
                        '',
                            '',
                        '',
                        '|',
                        '',
                            '',
                        '',
                    '	',
            '
'].join('\n'));

There isn't much to refactor here and you already identified the problem scope.

You are much better off with a JavaScript template engine.

I won't be too opinionated here, but popular options are:

http://handlebarsjs.com/
https://github.com/janl/mustache.js/
http://documentcloud.github.io/underscore/#template
http://ejohn.org/blog/javascript-micro-templating/


Insertion Sort Implementation Using For And While Loop Introduction

I'm fairly new to programming (and Java) and I've actually never implemented Insertion Sort.

I know that there's pseudo-code defined in the wikipedia article (linked above), but the goal was to implement the algorithm without looking at the pseudo-code and only using the wiki article's graphic for reference.

Concerns

After I completed my implementation, I looked at a couple implementations on CodeReview.

One uses double for-loops vs. a for-loop and a while-loop. Is the double for-loop more readable?
Method signature should be void and should simply return the modified original array. Again, is this behavior better / more-expected? My argument against this is that maybe it could lead to some unexpected behavior if the user does not expected the original array to be modified? Or unexpected behavior down the road if the user forgets that the returned array has been modified. That being said, with proper documentation, this could be avoided.

Implementation

public class InsertionSorter {
  public static int[] sort(int[] numbers) {
    int[] sorted = numbers.clone();
    for (int i = 1; i < sorted.length; i++) {
      int j = i - 1;
      int numberToInsert = sorted[i];
      while (j >= 0 && numberToInsert < sorted[j]) {
        sorted[j + 1] = sorted[j];
        sorted[j] = numberToInsert;
        j--;
      }
    }
    return sorted;
  }
}

Thoughts on best practices

  Method signature should be void and should simply return the modified original array. Again, is this behavior better / more-expected? My argument against this is that maybe it could lead to some unexpected behavior if the user does not expected the original array to be modified? Or unexpected behavior down the road if the user forgets that the returned array has been modified. That being said, with proper documentation, this could be avoided.

This is a variant of the Single Responsibility Principle.  Your method is supposed to sort something.  Should it also copy something?  Or only sort?  Note that there is no way to sort in place with your version (except to copy back over the original array).  But with a sort-in-place version, you can easily duplicate your method's functionality:

int[] sorted = unsorted.clone();
InPlaceSorter.sort(sorted);

Now we have unsorted, which is the original array, and sorted, which is a new sorted version of the original array.

Documentation and the fact that saying

int[] sorted = InPlaceSorter.sort(unsorted);

will produce a compiler error should be enough to make clear that the array is being modified.

By contrast, calling

InsertionSorter.sort(unsorted);

without assigning the result will silently not do anything useful.  So I'd find your version riskier, as it doesn't warn when someone tries to do the wrong thing.

If you do stick to your original version, consider changing the name to make it clearer.  E.g. generateSortedCopyOf.  If I read that, I expect it to return a copy without modifying the original.  And I wouldn't try calling it to do a sort-in-place.

  One uses double for-loops vs. a for-loop and a while-loop. Is the double for-loop more readable?

Personally, I don't have a strong feeling about this one way or the other.  All for loops can be written as while loops and sometimes I find it more convenient.

      int j = i - 1;
      int numberToInsert = sorted[i];
      while (j >= 0 && numberToInsert < sorted[j]) {
        sorted[j + 1] = sorted[j];
        sorted[j] = numberToInsert;
        j--;
      }

But this does translate nicely to a for loop.

      int numberToInsert = sorted[i];
      for (int j = i - 1; j >= 0 && numberToInsert < sorted[j]; j--) {
        sorted[j + 1] = sorted[j];
        sorted[j] = numberToInsert;
      }

No gymnastics required to fit it into the for pattern.  I think that I'd vote for this solution as simpler in this case.  But see ahead.

Optimizations

This solution does more assignments than needed.  Consider

      int j = i - 1;
      int numberToInsert = sorted[i];
      for (; j >= 0 && numberToInsert < sorted[j]; j--) {
        sorted[j + 1] = sorted[j];
      }
      j++;

      sorted[j] = numberToInsert;

Now it's not nearly as obvious that this should be a for loop.

      int j = i - 1;
      int numberToInsert = sorted[i];
      while (j >= 0 && numberToInsert < sorted[j]) {
        sorted[j + 1] = sorted[j];
        j--;
      }
      j++;

      sorted[j] = numberToInsert;

Either way though, this could save you a bunch of assignments at the expense of always doing an assignment (we can check that i != j if we want to avoid that useless assignment).  But looking at this, we have another option:

      int j = i - 1;
      int numberToInsert = sorted[i];
      while (j >= 0 && numberToInsert < sorted[j]) {
        j--;
      }
      j++;

      if (j != i) {
        System.arraycopy(sorted, j, sorted, j + 1, i - j);
        sorted[j] = numberToInsert;
      }

Now we allow the Java compiler to do any optimizations that System.arraycopy allows.  At worst this should be the same as the original.

And we might do even better if we take advantage of the sorted portion of the array.

      int numberToInsert = sorted[i];
      int j = Math.abs(1 + Arrays.binarySearch(sorted, 0, i - 1, numberToInsert));

      if (j != i) {
        System.arraycopy(sorted, j, sorted, j + 1, i - j);
        sorted[j] = numberToInsert;
      }

Of course, one of the benefits of an insertion sort is its simplicity.  This replaces two things we originally did in a single loop with two system calls.  That may not be faster in some cases.  In particular, consider the case of an already sorted list.  The original algorithm runs in \$O(n)\$ time in that case, as the inner loop simply doesn't iterate.  This version would run in \$O(n \log n)\$ because binarySearch is a logarithmic operation.

Note:  adding one to the binarySearch result and taking the absolute value always gives the insertion point.  This is because a positive result is the current location of the element, and we want to insert after the current location in case of duplicates (technically we want to insert after the last duplicate, but I didn't add that logic); and a negative result is one less than the insertion point.

Another issue is that the original version might parallelize better.  One core can do comparisons while the other core writes values.  Or maybe the compiler optimizes binarySearch and arraycopy better.  You'd have to test to see, and you might get different results on different platforms.


Minimum steps to reach target by a Knight In Scala Given a chessboard of N size (square matrix), the position of Knight and position of a target, find out minimum steps (both count and exact steps) from start to target for a Knight.

If it is not possible to reach to the given position, return -1 as step count.

Here is my implementation in Scala (assuming N = 4 or 4X4 chess board):

import scala.collection.mutable._

object KnightMoves extends App {
  case class Pos(row: Int, col: Int)
  val Size = 4

  def calculateMoves(from: Pos, target: Pos ): (Int, Seq[Pos])= {
    val pendingPos = collection.mutable.Queue[Pos](from)
    val positionVisited = collection.mutable.HashMap[Pos, (Int, Seq[Pos])](from -> (0, Seq()))
    var targetReached = false

    while(pendingPos.nonEmpty && !targetReached) {
      val p = pendingPos.dequeue()
      possibleMoves(p) foreach { position =>
        if ( position == target) {
          targetReached = true
        } else if (!(positionVisited contains position)) {
          pendingPos enqueue position
        }
        positionVisited += position -> ((positionVisited(p)._1 + 1,(positionVisited(p)._2 ++ Seq(p))))
      }
    }
    if (targetReached) positionVisited(target) else (-1, Seq())
  }

  def isValidPos(position: Pos): Boolean =
    ((0 until Size) contains position.row) && ((0 until Size) contains position.col)

  def possibleMoves(position: Pos): List[Pos] =
    List(Pos(position.row - 2, position.col + 1),
        Pos(position.row - 2, position.col - 1),
        Pos(position.row + 2, position.col + 1),
        Pos(position.row + 2, position.col - 1),
        Pos(position.row - 1  , position.col + 2),
        Pos(position.row - 1  , position.col - 2),
        Pos(position.row + 1  , position.col + 2),
        Pos(position.row + 1  , position.col - 2)
    ) filter( pos => isValidPos(pos))

  println(calculateMoves(Pos(0,1),Pos(0,0)))
  println(calculateMoves(Pos(0,1),Pos(0,2)))
}

This program generates the following output for two test statements at bottom.

(3,ArrayBuffer(Pos(0,1), Pos(2,0), Pos(1,2)))
(3,ArrayBuffer(Pos(0,1), Pos(2,2), Pos(1,0)))

At first glance, the isValidPos() method looks to be clear and succinct, requiring only two tests to see if the passed position is on the board.

def isValidPos(position: Pos): Boolean =
  ((0 until Size) contains position.row) && ((0 until Size) contains position.col)

But it turns out that the contains() method on a Range is a bit complicated and most valid positions will have to pass around 10 if checks before the method returns true, so there are probably more efficient ways to get the same result.

But a bigger question is, can this be done without any mutable collections or variables?

One approach is to create a Knight class that holds all current valid board positions and the history of how each position was arrived at and then, when taking the next step, instead of modifying the Knight instance you create a new immutable Knight instance with all the new information built from the current (old) data.

type Cell = (Int,Int)

class Knight(dim :Int, history :Map[Cell,Cell], val cells :Set[Cell]) {
  def this(dim :Int, cell :Cell) = this(dim, Map(cell->(-1,-1)), Set(cell))

  def backtrack(cell :Cell) :List[Cell] =
    if (history(cell) == (-1,-1)) cell :: Nil
    else                          cell :: backtrack(history(cell))

  def next :Knight = (new Knight(dim, _:Map[Cell,Cell], _:Set[Cell])).tupled{
    cells.foldLeft((history, Set.empty[Cell])){
      case ((hMap,cSet),(x,y)) =>
        val newSet = Set((x-2, y-1), (x-2, y+1), (x-1, y-2), (x-1, y+2)
                        ,(x+1, y-2), (x+1, y+2), (x+2, y-1), (x+2, y+1)
                        ).filter{ case (a,b) =>
              a >= 0 && a < dim && b >= 0 && b < dim && !hMap.keySet((a,b))
                                }
        (hMap ++ newSet.map(_ -> (x,y)).toMap, cSet ++ newSet)
    }
  }
}

def knightMoves(boardSize :Int, knight :Cell, target :Cell) :List[Cell] = {
  def loop(k1 :Knight, k2 :Knight) :List[Cell] = {
    val met :Set[Cell] = k1.cells.intersect(k2.cells)
    if (met.nonEmpty) k1.backtrack(met.head).reverse ++
                      k2.backtrack(met.head).tail
    else if (k1.cells.isEmpty || k2.cells.isEmpty) Nil
    else if (k1.cells.size > k2.cells.size) loop(k1, k2.next)
    else                                    loop(k1.next, k2)
  }
  loop(new Knight(boardSize, knight), new Knight(boardSize, target))
}

knightMoves(3,(0,1),(1,1))    //res0: List[Cell] = List()
knightMoves(14,(0,1),(10,12)) //res1: List[Cell] = List((0,1), (2,2), (4,3), (6,4), (7,6), (8,8), (9,10), (10,12))

Some efficiency is obtained here by making the target position a Knight instance as well. That way they can "move" toward each other and, as soon as they intersect, a path from one to the other is found.


HTML - single page layout - absolute positioning I am doing web apps, mostly single page apps. So i have to give absolute and fixed positioning a lot via css.

For Example, consider this page layout:

CSS

.app-header
{
   position: fixed;
   top: 0px;
   left: 0px;
   width: 100%;
}

.main-app-area
{
   width: 100%;
   height: 100%;
}

.app-page
{
  width: 100%;
  height: 100%;
  position: absolute;
  top: 0px;
  left: 0px;
  visibility: hidden;
}

I really don't know, Is this right way of doing single page apps? Will it cause any performance problems?

Have a look at this, maybe this helps a little. I made only little changes. first of all you have to know a child element is always positioned absolute or relative to his parent. so it is important to make child and parent elements, except you want a div as a placeholder to load the data in another way...

play arround with the code (like why is position:absolute 5px top and left only 5px from the orange...) it may helps to understand. also have a look at the class .app-page.active !

xxx

        yyy

zzz


Simple sorting algorithm speed I was working on an assignment in my Python class to sort 10 numbers but I wanted to be able to sort any set of numbers be it large or small. The assignment didn't really cover any sorting algorithms other than bubble sort so I started working and decided to just do something simple find the smallest number and remove it then put it in a temp list and then assign that list to the original to make it sorted. I was able to compress this down into the first sorting.

But I wanted to see if I could make it faster by also looking for the max and popping them both out in separate lists. But I was told python runs on a single thread and there were far more things to run than the first.

So why is the second sort slightly faster despite having more instructions than the first?

I ran this on python 3.8

import time
import random
rand_list = []
for i in range(0,10000):
    n = random.randint(1,100000)
    rand_list.append(n)
print('Random List Creation Complete')

list_to_sort1 = rand_list[:]
list_to_sort2 = rand_list[:]

''' Sort Smallest to Largest'''
start_time = time.time()

templist=[]
while 0 < len(list_to_sort1):
    templist.append(list_to_sort1.pop(list_to_sort1.index(min(list_to_sort1))))
list_to_sort1 = templist

print('\nSorting Complete: My sort took', time.time() - start_time, "to run")

''' Sort Smallest to Largest'''
start_time = time.time()

min_half = []
max_half = []
max_pos = 0
min_pos = 0
while 0 < len(list_to_sort2):
    max_pos = list_to_sort2.index(max(list_to_sort2))
    min_pos = list_to_sort2.index(min(list_to_sort2))

    if len(list_to_sort2) != 1:
        if max_pos > min_pos:
            max_half.append(list_to_sort2.pop(max_pos))
            min_half.append(list_to_sort2.pop(min_pos))
        else:
            min_half.append(list_to_sort2.pop(min_pos))
            max_half.append(list_to_sort2.pop(max_pos))
    else:
        min_half.append(list_to_sort2.pop(0))

max_half.reverse()
list_to_sort2 = min_half + max_half

print('\nSorting Complete: My sort 2 took', time.time() - start_time, "to run")

time.sleep(20)

So why is the second sort slightly faster [...]

On my machine, the first algorithm is "faster" (1.588s vs. 1.62s).
The number of samples you have taken (1) is too low to draw conclusions.
For significant statistics, you should run at least 20 times and then compare the average execution time.

  [...] slightly faster [...]

The difference between your two runs is 4 ms.
The precision of the default clock on Windows is ~16 ms., so 4 ms is even within measuring tolerance.

  [...] having more processes

You still only have 1 process. It's not that you find the minimum and maximum in parallel. It's done one after the another.

  other than bubble sort

Bubble sort is typically an in-place algorithm, which means that it operates on one list only. Your implementation works on two lists, templist and list_to_sort1. I would not call this a bubble sort implementation.

BTW:
Python's min() and index() functions seem to be pretty optimized.
A real bubble sort is slower by a factor of ~10 on my machine:

list_to_sort3 = rand_list[:]
start_time = time.time()
for i in range(len(list_to_sort3)):
    for j in range(i):
        if list_to_sort3[j] > list_to_sort3[i]:
            list_to_sort3[j], list_to_sort3[i] = list_to_sort3[i], list_to_sort3[j]
print('\nSorting Complete: Bubble sort took', time.time() - start_time, "to run")

  (comments) I actually meant instructions not processes in reference to the second sort there is a lot more going on yet it takes about the same time.

In version 1 you have:

while 0 < len(list_to_sort1):
    templist.append(list_to_sort1.pop(list_to_sort1.index(min(list_to_sort1))))

This loop runs 10000 times, because it pop()s one item per loop. Thus it also calls min(), index() and append() 10000 times.

In version 2 you have:

while 0 < len(list_to_sort2):
    max_pos = list_to_sort2.index(max(list_to_sort2))
    min_pos = list_to_sort2.index(min(list_to_sort2))

    if len(list_to_sort2) != 1:
        if max_pos > min_pos:
            max_half.append(list_to_sort2.pop(max_pos))
            min_half.append(list_to_sort2.pop(min_pos))
        else:
            min_half.append(list_to_sort2.pop(min_pos))
            max_half.append(list_to_sort2.pop(max_pos))
    else:
        min_half.append(list_to_sort2.pop(0))

This loop only runs 5000 times, because you pop() twice per loop (either in if or in else). Thus it calls min(), max(), index() (twice), append() (twice) also 5000 times.

As you can see, the amount of method calls is the same, although it's more lines of written code.


Regex password strength test I've been following a Python programming book and reached the Regex chapter where I encountered this challenge:

Write a password strength checker that checks if a password is:

At least 8 character long
Contains at least one uppercase and lowercase letter.
And contains at least one digit.

I managed to do so, but frankly my solution seems both long and ugly. I was hoping someone could shed some light as to how I could maybe condense my program and improve readability if possible.
import re
def is_password_strong():
    password = input('Enter a password to test: ')

    length_regex = re.compile(r'.{8,}')
    length = True if length_regex.search(password) != None else False

    uppercase_regex = re.compile(r'[A-Z]')
    lowercase_regex = re.compile(r'[a-z]')
    uppercase = True if uppercase_regex.search(password) != None else False
    lowercase = True if lowercase_regex.search(password) != None else False
    case = True if uppercase and lowercase == True else False

    digit_regex = re.compile(r'[0-9]')
    digit = True if digit_regex.search(password) != None else False

    return(length and case and digit == True)
print(is_password_strong())

And here is my non-regex solution if you can't read the above and want to see what I am trying to do with regex:
def is_password_strong():
    password = input('Enter a password to test: ')
    length = len(password) >= 8
    case = password != password.upper() and password != password.lower()
    digit = any(c.isdigit() for c in password)

    return(length and case and digit == True)

I know this isn't a good test of password strength, but it was the book's challenge. I'm not implementing it for any real use.

Here are the major comments:

Ideally functions should do one thing at once. Your function is doing two: (1) asking the user for a password and (2) evaluating the strength of that password.

You should break this code into two functions: (1) ask_user_for_password() and (2) is_password_strong(password).
Regexes are not always the right tool for the job. (I appreciate this is a chapter on regexes, but the point has to be made.)

Python has fairly powerful string  manipulation tools. You should reach for those before opening your regex toolbox – I think the Python constructs are usually much easier to read and debug. Brownie points for writing a version that doesn’t use regexes – I would almost always prefer this version in a “real” codebase.
Be lazy about evaluation. This function is fairly fast, because it doesn’t have many checks. Even so, you can save yourself a little work – as soon as a check fails, you can return False.

(It’s fine to bail out early for a password strength evaluation. Try to avoid doing this when doing real authentication – if your function takes noticeably different time depending on what’s wrong with the function, you’re vulnerable to a timing attack.)
Your function should have a docstring. Docstrings are the preferred way to document functions in Python. For this function, it should be fairly simple -- it evaluates a password, and returns True/False depending on whether it meets certain criteria -- but you should still write it.

And a few nitpicks:

Compare to None with “is”, not “==” and “!=”.

It’s always preferred to check using “is”, which checks identity, not equality – this makes sure the object you’re dealing with really is None, and not a class with a strange equality operator. It’s also faster.
You can tidy up your booleans.

Quite a few of your lines boil down to:

result = True if (check != None) else False

This line reduces to:

result = (check is not None)

That’s more readable and direct.

Then your final boolean can be reduced to:

return (length and case and digit)

which is equivalent.
Put your main code in an if __name__ == '__main__' block. This means that it only runs when the file is called directly – not, for example, when it’s imported as a module.

This makes your code easier to reuse.