Hackerrank Cut the sticks Solution I have started learning Java recently and I solved the following problem on Hackerrank.

The challenge is to :

  You are given NN sticks, where the length of each stick is a positive integer. A cut operation is performed on the sticks such that all of them are reduced by the length of the smallest stick.

  Suppose we have six sticks of the following lengths:
  5 4 4 2 2 8

  Then, in one cut operation we make a cut of length 2 from each of the six >sticks. For the next cut operation four sticks are left (of non-zero length), >whose lengths are the following:
  3 2 2 6

  The above step is repeated until no sticks are left.

  Given the length of NN sticks, print the number of sticks that are left before each subsequent cut operations.

  Note: For each cut operation, you have to recalcuate the length of smallest sticks (excluding zero-length sticks).

  Input Format
  The first line contains a single integer NN.
  The next line contains NN integers: a0, a1,...aN-1 separated by space, where ai represents the length of ith stick.

  Output Format
  For each operation, print the number of sticks that are cut, on separate lines.

Sample Input :

6
5 4 4 2 2 8

Sample Output:

6
4
2
1

I am trying to get better at writing good code for my solutions. Please give me your suggestions.

 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Scanner;

 public class CutSticks {
 public static void main(String[] args) {

    Scanner keyboard = new Scanner(System.in);

    int size = keyboard.nextInt(); // get total number of sticks

    ArrayList<Integer> list = new ArrayList<Integer>(size);
    for(int i=0;i<size;i++)
    {
        int values = keyboard.nextInt();
        list.add(values);
    }
     list.removeAll(Collections.singleton(0));// remove all zeroes if present
     Collections.sort(list); // sort in ascending order
        while(list.size()>0)
        {
            int smallest = list.get(0);  // get the smallest element
            for(int i =0 ;i < list.size();i++)
            {

                list.set(i, list.get(i) - smallest);
            }
            //System.out.println(list);
            System.out.println(list.size());
            list.removeAll(Collections.singleton(0)); // remove the elements which are zero after cutting the sticks
            //System.out.println(list);
        }

   }

  }

Improving the algorithm

Realize that you don't actually need to remove elements from a collection. You can just count how many sticks would disappear in each step.

Take for example this sorted list of sticks:

1 1 1 2 2 3 3 3 3 3 4 5 5 5

At each cutting step, the shortest sticks disappear:

1 1 1 2 2 3 3 3 3 3 4 5 5 5
^^^^^
3 will disappear

      2 2 3 3 3 3 3 4 5 5 5
      ^^^
      2 will disappear

          3 3 3 3 3 4 5 5 5
          ^^^^^^^^^
          5 will disappear

And so on. You can just count the number of sticks that will disappear, and figure out from that how many will remain.

Use interface types in declarations

Instead of this:

ArrayList<Integer> list = new ArrayList<Integer>(size);

It's recommended to declare variables with their interface types:

List<Integer> list = new ArrayList<Integer>(size);

Style

This is way too compact writing style:

for(int i=0;i<size;i++)

It's much more readable this way, by putting spaces around operators:

for (int i = 0; i < size; i++)

Checking an empty list

Instead of this:

    while(list.size()>0)

A more idiomatic way to write:

    while(!list.isEmpty())

Suggested implementation

Putting the above tips together:

public static void main(String[] args) {
    List<Integer> sticks = readSticksFromStdin();
    Collections.sort(sticks);

    int pos = 0;
    int remaining = sticks.size();
    while (0 < remaining) {
        System.out.println(remaining);
        int count = countEqualFrom(sticks, pos);
        pos += count;
        remaining -= count;
    }
}

private static int countEqualFrom(List<Integer> sticks, int from) {
    int value = sticks.get(from);
    for (int i = 1; from + i < sticks.size(); ++i) {
        if (value != sticks.get(from + i)) {
            return i;
        }
    }
    return sticks.size() - from;
}

private static List<Integer> readSticksFromStdin() {
    Scanner scanner = new Scanner(System.in);
    int num = scanner.nextInt();
    List<Integer> sticks = new ArrayList<>(num);
    for (int i = 0; i < num; ++i) {
        sticks.add(scanner.nextInt());
    }
    return sticks;
}


Calculating user birth information Can you please check if I've written the code correctly?

The task was:

Calculate the user's month of birth as a a number, where January = 0 through to December = 11.
Take the string entered
Get the substring, being the first three characters
Convert to uppercase
Find the starting location of the three-letter abbreviation in the month abbreviations string, and divide this by 3 (this is not the only way to find the month number, but it allows us to practice searching in a string)

var year = prompt('Enter year of birth as a 4 digit integer');

var month = prompt('Enter the name of the month of birth');

// Chop everything after the first 3 characters and make it uppercase
month = month.substring(0,3).toUpperCase();

// Store your array in months, differently named than the month input
var months = ["JAN", "FEB", "MAR", "APR", "MAY", "JUN", "JUL", "AUG", "SEP", "OCT",
"NOV", "DEC"];

// We then use array.indexOf() to locate it in the array
var pos = months.indexOf(month);
if (pos >= 0) {
// valid month, number is pos
}

var date = prompt('Enter day of birth as an integer');

Everything looks good.

That said, array.indexOf wasn't supported in Internet Explorer until version 9.

If you need that support, the Mozilla Developer Network has a handy function you can include in your code to provide support for those browsers:

if (!Array.prototype.indexOf) {
    Array.prototype.indexOf = function (searchElement /*, fromIndex */ ) {
        "use strict";
        if (this == null) {
            throw new TypeError();
        }
        var t = Object(this);
        var len = t.length >>> 0;
        if (len === 0) {
            return -1;
        }
        var n = 0;
        if (arguments.length > 0) {
            n = Number(arguments[1]);
            if (n != n) { // shortcut for verifying if it's NaN
                n = 0;
            } else if (n != 0 && n != Infinity && n != -Infinity) {
                n = (n > 0 || -1) * Math.floor(Math.abs(n));
            }
        }
        if (n >= len) {
            return -1;
        }
        var k = n >= 0 ? n : Math.max(len - Math.abs(n), 0);
        for (; k < len; k++) {
            if (k in t && t[k] === searchElement) {
                return k;
            }
        }
        return -1;
    }
}


Take a desired string, iterate through objects to see if it exists in a given field and append a number until a unique string is found I wanted to in the most generic way possible write a method in C# to achieve the following.

Take in a string, a set of objects, and a function to access the field of a given object
Look at all of the strings from the fields of these objects to match against provided string
If provided string is not unique in a case insensitive manner, append _x to the end, where x is an incrementing integer until a unique string is found

With that in mind, I created this.

private string GetUniqueStringForModel<T>(string originalString, IEnumerable<T> enumerableObjects, Func<T, string> stringFieldFunction) where T : class
{
    var uniqueString = originalString;
    var duplicateCount = 1;

    while (enumerableObjects.Select(stringFieldFunction).ToList().Any(currentString => string.Equals(currentString, uniqueString, StringComparison.InvariantCultureIgnoreCase)))
    {
        uniqueString = originalString + "_" + duplicateCount++;
    }

    return uniqueString;
}

I would rather have not put the ToList() in but when working with Entity Framework it was complaining about doing the string comparison in LINQ (presumably because it couldn't compile it SQL).

Any thoughts or ideas for improvement?

There's certainly room for improvement.

Move that that Select(stringFieldFunction).ToList() out of the while loop. Iterating an IEnumerable might be quite expensive, and there's no need to repeat that work when you find a duplicate.
Select only those strings that have originalString as a prefix. That will reduce the amount of items you need to check against when adjusting the suffix.
Instead of making a list, turn those results into a (hash)set, for faster lookup.

Regarding EF complaining: apparently it doesn't support those StringComparsion overloads, so it'll have to fetch all rows from the database and run your Any predicate in memory, which may be slower as expected. Maybe an alternative approach like using ToUpperInvariant can be translated to SQL?

Applying the above changes will give you the following:

private string GetUniqueValue<T>(string value, IEnumerable<T> items, Func<T, string> getValue)
{
    var possibleDuplicates = items
        .Select(getValue)
        .Where(val => val.StartsWith(value, StringComparison.InvariantCultureIgnoreCase))
        .ToHashSet();

    var result = value;
    var suffix = 1;
    while (possibleDuplicates.Contains(result))
    {
        result = value + "_" + suffix;
        suffix += 1;
    }
    return result;
}

Addendum

As t3chb0t and ErikE already pointed out, if you want the benefits of Linq-to-SQL, then you need to use IQueryable<T>. Not only that, you also need to make sure that you're using the Queryable Linq extension methods, not the Enumerable variants.

The difference is that Enumerable methods have Func<> parameters, while the Queryable variants have Expression<Func<>> parameters. An Expression is a data structure that represents a piece of code, which makes translation to SQL possible. Because stringFieldFunction is a Func, not an Expression<Func>, the Enumerable.Select variant is used. That cannot be translated to SQL, so all data has to be loaded from the database before the Select (and any subsequent operation) can be performed on it.

To recap: use IQuerable<T>, make sure you're using Queryable Linq methods, and only use supported methods within your expressions.


Code Vita: Form the maximum possible time in the HH:MM:SS format using any six of nine given single digits Problem Description

  The objective is to form the maximum possible time in the HH:MM:SS
  format using any six of nine given single digits (not necessarily
  distinct)

  Given a set of nine single (not necessarily distinct)
  digits, say 0, 0, 1, 3, 4, 6, 7, 8, 9, it is possible to form many
  distinct times in a 24 hour time format HH:MM:SS, such as 17:36:40 or
  10:30:41 by using each of the digits only once. The objective is to
  find the maximum possible valid time (00:00:01 to 24:00:00) that can
  be formed using some six of the nine digits exactly once. In this
  case, it is 19:48:37.

  Input Format

  A line consisting of a sequence of 9 (not necessarily distinct) single
  digits (any of 0-9) separated by commas. The sequence will be
  non-decreasing

  Output

  The maximum possible time in a 24 hour clock (00:00:01 to 24:00:00) in
  a HH:MM:SS form that can be formed by using some six of the nine given
  digits (in any order) precisely once each. If no combination of any
  six digits will form a valid time, the output should be the word
  Impossible

  Explanation

  Example 1

  Input

  0,0,1,1,3,5,6,7,7

  Output 17:57:36

  Explanation: The maximum valid
  time in a 24 hour clock that can be formed using some six of the 9
  digits precisely once is 17:57:36

  Example 2

  Input

  3,3,3,3,3,3,3,3,3

  Output

  Impossible

  Explanation: No set of six digits from the input may be used to
  form a valid time.

The Code:

def bubbleSort(arr):
    n = len(arr)
    for i in range(n):
        for j in range(0, n-i-1):
            if arr[j] < arr[j+1] :
                arr[j], arr[j+1] = arr[j+1], arr[j]

seq_a=list(map(int,input().split(',')))
# print(seq_a)
bubbleSort(seq_a)
# print(seq_a)

hrs=[None,None]
mins=[None,None]
secs=[None,None]

#hrs
for x in seq_a:
    if x <= 2:
        hrs[0]=x
        seq_a.remove(x)
        break
# print(seq_a)
# print(hrs,mins,secs)

if hrs[0]:
    if hrs[0]==2:
        for x in seq_a:
            if x <= 4:
                hrs[1]=x
                seq_a.remove(x)
                break
    else:
        for x in seq_a:
            if x <= 9:
                hrs[1]=x
                seq_a.remove(x)
                break
# print(seq_a)
# print(hrs,mins,secs)

#mins
if hrs[1]:
    for x in seq_a:
        if x < 6:
            mins[0]=x
            seq_a.remove(x)
            break
# print(seq_a)
# print(hrs,mins,secs)

if mins[0]:
    for x in seq_a:
        if x <= 9:
            mins[1]=x
            seq_a.remove(x)
            break
# print(seq_a)
# print(hrs,mins,secs)

#secs
if mins[1]:
    for x in seq_a:
        if x < 6:
            secs[0]=x
            seq_a.remove(x)
            break
# print(seq_a)
# print(hrs,mins,secs)

if secs[0]:
    for x in seq_a:
        if x <= 9:
            secs[1]=x
            seq_a.remove(x)
            break
# print(seq_a)
# print(hrs,mins,secs)

if secs[1]:
    print(str(hrs[0])+str(hrs[1])+':'+str(mins[0])+str(mins[1])+':'+str(secs[0])+str(secs[1]))
else:
    print('Impossible')

Bugs

Your program produces these erroneous results:

0,5,5,5,5,5,5,5,9 → Impossible (should be 09:55:55)
0,2,5,5,5,5,5,5,5 → Impossible (should be 20:55:55)

Due to tests like if hrs[0], if hrs[1], if mins[1], and if secs[1], hours, minutes, or seconds is a multiple of 10, or if the hour starts with 0.
2,2,4,4,4,4,4,4,7 → 24:47:44 (should be 22:47:44)

The challenge states that 24:00:00 is the upper bound.  The correct answer, 22:47:44, demonstrates that a naïve greedy algorithm is not sufficient to solve this problem.  You might optimistically select 24 for the hour (which would work for input such as 0,0,0,0,2,4,9,9,9).  However, for the input 2,2,4,4,4,4,4,4,7, you should then find that there is no valid minute if you pick 24 as the hour, and you would need to backtrack and try 22 for the hour.

Algorithm

There is no need to implement bubbleSort.  Just call sorted(seq_a, reverse=True).  (By PEP 8 naming conventions, the function should be named bubble_sort.)

Your code is repetitive: you write a similar for loop to handle each of the six digits.  You should generalize them to be handled by one loop, which accepts different upper limits for each digit.  Note that you use <= tests for most of those loops, but < 6 for some loops — the inconsistency is confusing.

Suggested solution

I would define a placewise_max function to generalize all of your loops, then call it with three limiting template strings.  Instead of loops with break statements, though, I would use next() with a generator expression.

Note that there is no need to parse each character as a numeric digit: ASCII comparison will work just as well.

def placewise_max(max_template, pool):
    """
    Try to form the lexicographically greatest string from the characters in
    the pool, where the character in each position of the output does not
    exceed the character at the corresponding position in the template.
    Return the empty string if any position cannot be filled.

    >>> placewise_max('91210', list('02301'))
    '31200'
    >>> placewise_max('elmo', 'abcdefghijklm')
    'elmk'
    >>> placewise_max('elmo', 'limo')
    ''
    """
    pool = sorted(pool, reverse=True)
    output = []
    try:
        for t in max_template:
            char = next(c for c in iter(pool) if c <= t)
            pool.remove(char)
            output.append(char)
        return ''.join(output)
    except StopIteration:   # next() failed to pick from the pool
        return ''

def max_time(digits):
    best = max(
        placewise_max('240000', digits),
        placewise_max('235959', digits),
        placewise_max('195959', digits)
    )
    if best and (best != '000000'):
        return '{0}{1}:{2}{3}:{4}{5}'.format(*best)

if __name__ == '__main__':
    print(max_time(input().split(',')) or 'Impossible')

Educational tips

The solution above uses some rather advanced techniques, and may be overwhelming for a beginner.  However, I would point out an essential next step for improving your coding skills.

It is critical to learn to write functions, each with a clearly defined purpose, that accept parameters, return results, and use no global variables.  Document the purpose, parameters, and outputs in a docstring, as I've done.  That will force you to package your code into small, reusable chunks.  For example, can you write a function that accepts a string (or list) of characters, and that returns a tuple with two items — the best possible hour that can be formed from those characters, and all the leftover characters that were not used to form the hour?

Defining a function is necessary to solve this problem correctly, because, as I've pointed out above, a correct solution requires some trial and error.  If you are unable to reuse the code that performs the trials, then you will end up writing if-else statements to handle all of the failure cases, and it's going to be a huge mess!


Run external command from Python I have to run an external program from Python.  The external program makes many temporary files (which conflict with other instance of same program).

My strategy is:

get current directory (native) path
change to temp directory
run external command
copy output to native directory
change back to native directory

Here is my code:

import os, tempfile
current_dir = os.getcwd()
tmp_dir = tempfile.mkdtemp()
input_file, outfile = 'input_file', 'result'
os.system('cp %s %s' %(input_file, tmp_dir))
os.chdir(tmp_dir)
cmd = 'pwd> tmp_01.txt;ls -ltr >>tmp_01.txt' ##system command goes here
os.system(cmd)
os.system('cp tmp_01.txt %s/%s' %(current_dir, outfile))
os.chdir(current_dir)

My questions are:

Is it right or is there a simpler way to do it?
Do I need to remove the tmp directory?
Is it safe to use this code as Python module/class in a web application? Does it make shell injection easy, or can it possible reveal more information than needed?

Give a try to shutil and subprocess.

To copy all the files:
http://docs.python.org/2/library/shutil.html#shutil.copyfile

import shutil
shutil.copy(src_file, destination_file)

To make the system calls:
http://docs.python.org/2/library/subprocess.html#subprocess.check_call

import subprocess
subprocess.check_call('ls -al', shell=True)


Asking the user to input a proper email address The purpose of this code is to ask the user to input an email address but it must contain "@" and ".co". The code works but it is too long so I would like to know if there is any way to shorten it using functions.

email_address = input("What is your email address? ")
while "@" not in email_address:
    email_address = input("Your email address must have '@' in it\nPlease write your email address again: ")
    if len(email_address) <= 6 :
        email_address = input("Your email address is too short\nPlease write your email address again: ")
    if "." not in email_address:
        email_address = input("Your email address must have '.' in it\nPlease write your email address again: ")
while "." not in email_address:
    email_address = input("Your email address must have '.' in it\nPlease write your email address again: ")
    if len(email_address) <= 6 :
        email_address = input("Your email address is too short\nPlease write your email address again: ")
    if "@" not in email_address:
        email_address = input("Your email address must have '@' in it\nPlease write your email address again: ")

Okay so as a general comment, validation like this is fine (well except
it's discarding potentially valid addresses with the check for . and
the minimum length, but I guess you know that), but in a real
application you most likely want to just send an email and check if it
was received, that also deals with a whole lot of other problems.

That said, of course this snippet can be shortened.  I don't see why the
two permutations of the loop are necessary in the first place.

Everything duplicated has the potential to be eliminated, but the
result might be more or less readable and possibly longer (because there
are only two cases here for example, so making the code more generic
would only pay off with more cases).

What's the function going to be here?  Checking if all the characters
are in the email string, e.g. check_email_contains.  What varies?
Only the specific characters really.  So pass that in and return the
final address (without using globals) to be future proof.

Looks like this:

email_address = check_email_contains(input("What is your email address? "), "@.")

Lastly, the check function is very simple too:

def check_email_contains(email_address, characters, min_length=6):
    while True:
        for character in characters:
            if character not in email_address:
                email_address = input("Your email address must have '{}' in it\nPlease write your email address again: ".format(character))
                continue
        if len(email_address) <= min_length:
            email_address = input("Your email address is too short\nPlease write your email address again: ")
            continue
        return email_address

The loops in there are doing a bit too much, I'll leave that as an
exercise for the reader to optimise it.


String reversal in Python This code will take as output any string (long, empty, with spaces...) and will return its reverse. Can I improve this algorithm so that it can be faster?

Right now its complexity is \$O(n)\$.

def reverse(stri):
    output = ''
    length = len(stri)
    while length > 0:
        output += stri[-1]
        stri, length = (stri[0:length - 1], length - 1)
    return output

Yes, this can be faster. Adding strings using + is usually a bad idea in Python, since strings are immutable. This means that whenever you add two strings, a new string needs to be allocated with the size of the resulting strings and then both string contents need to be copied there. Even worse is doing so in a loop, because this has to happen ever time. Instead you usually want to build a list of strings and ''.join them at the end (where you pay this cost only once).

But here you can just use the fact that strings can be sliced and you can specify a negative step:

def reverse_g(s):
    return s[::-1]

Here is a timing comparison for random strings of length from one up to 1M characters, where reverse is your function and reverse_g is this one using slicing. Note the double-log scale, for the largest string your function is almost a hundred thousand times slower.

The reverse_s function uses the reversed built-in, as suggested in the (now deleted, so 10k+ reputation) answer by @sleblanc and assumes you actually need the reversed string and not just an iterator over it:

def reverse_s(s):
    return ''.join(reversed(s))

The reverse_b function uses the C implementation, compiled with -O3, provided in the answer by @Broman, with a wrapper to create the string buffers and extract the output:

from ctypes import *

revlib = cdll.LoadLibrary("rev.so")
_reverse_b = revlib.reverse
_reverse_b.argtypes = [c_char_p, c_char_p, c_size_t]

def reverse_b(s):
    stri = create_string_buffer(s.encode('utf-8'))
    stro = create_string_buffer(b'\000' * (len(s)+1))
    _reverse_b(stro, stri, len(s) - 1)
    return stro.value.decode()

In the no interface version, just the call to _reverse_b is timed.


Beginner's Sudoku solver in C I'm beginner programmer and I've challenged myself to write sudoku solver in C. I have finished it in few days and now I want to make it faster.

With this sample input execution time is 1.781 seconds (which includes loading of program, I'm using code:blocks in which time is automatically measured)

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

Here's the code:

 #include<stdio.h>

int sudoku[9][9]=
{
    0,0,0,0,0,4,0,0,0,
    0,6,8,0,0,0,5,0,0,
    0,2,0,0,0,0,0,7,6,
    6,0,0,0,0,0,8,9,0,
    0,0,5,2,6,0,0,0,0,
    0,0,0,9,0,0,1,0,0,
    0,0,0,0,0,7,0,5,0,
    0,4,0,0,0,0,0,0,1,
    0,0,0,0,5,1,4,0,0
};
int a[9][9]=
{
    0,0,0,0,0,4,0,0,0,
    0,6,8,0,0,0,5,0,0,
    0,2,0,0,0,0,0,7,6,
    6,0,0,0,0,0,8,9,0,
    0,0,5,2,6,0,0,0,0,
    0,0,0,9,0,0,1,0,0,
    0,0,0,0,0,7,0,5,0,
    0,4,0,0,0,0,0,0,1,
    0,0,0,0,5,1,4,0,0
};
//PRINT SUDOKU
void printSudoku()
{
    int i,j;
    for(i = 0; i < 9; i++) { if(i==3 || i==6) printf("\n");
        for(j = 0; j < 9; j++)    {
            printf("%d",sudoku[i][j]);
            printf(" ");
            if(j>7) printf("\n");
            if(j==2 || j==5) printf("  ");

        }
    }
    printf("\n");
}
//CHECK ROW
int checkRow(int num, int i)    {
    int j,x=1;
    for(j = 0; j < 9; j++)
        if(sudoku[i][j] == num)    return 0;

    return x;
}
//CHECK COLUMN
int checkCol(int num, int j) {
    int i,x=1;
    for(i = 0; i < 9; i++)
        if(sudoku[i][j]==num)    return 0;

    return x;
}

//CHECK SQUARE
int checkSquare(int num, int xj, int yi)
{
    int x=1;
    int i,j;
    for(i = yi; i < yi+3; i++)
        for(j = xj; j < xj+3; j++)
            if(sudoku[i][j]==num)    return 0;

    return x;
}
//MAIN PROGRAM
main()
{
    int i,j,num,row,col;
    int pi,pj,xj,yi,square;

    printSudoku();

    for(i = 0; i < 9; i++)
        for(j = 0; j < 9; j++) {
            if(sudoku[i][j]==0)    {
                num=1;

                do    {
                    if(num>9)    {
                        pi=i;
                        if(j == 0)  {
                            if(i!=0)    {
                                pj=8;
                                pi=i-1;
                            }
                        }
                        else    {
                            pj=j-1;
                        }
                        for(i = pi; i >= 0; i--)
                            for(j = pj; j >= 0; j--)    {
                                pj=8;

                                if(a[i][j]==0 && sudoku[i][j]==9)    {
                                    sudoku[i][j]=0;
                                }
                                else if(a[i][j]==0 && sudoku[i][j]<9)    {

                                    num=sudoku[i][j]+1;
                                    sudoku[i][j]=0;
                                    goto check;
                                }
                            }
                    }
                check:row=checkRow(num,i);
                    col=checkCol(num,j);
                    if(j<3)    xj=0;
                    if(j>2 && j<6) xj=3;
                    if(j>5) xj=6;
                    if(i<3)    yi=0;
                    if(i>2 && i<6) yi=3;
                    if(i>5) yi=6;
                    square=checkSquare(num,xj,yi);
                    if(row==1 && col==1 && square==1)
                        sudoku[i][j]=num;
                    else
                        num++;

                }
                while(row==0 || col==0 || square==0);
            }
        }

    printf("\n");
    printSudoku();

}

As I've said I'm a beginner programmer so sorry that my code is long and messy.

Does anyone know how can I make this faster?
Would it be faster if I used recursion?
And also how can I measure real execution time?

Advice 1

Instead of having checkRow, checkCol and checkSquare, I suggest you allocate a simple char array for each row, column and minisquare:

char visited[10];

You initialize each visited[0], ..., visited[9] to zero. Next, when you put a number in an empty cell, you mark it in three such "sets": current column, current row and current minisquare. This arrangement will bring the complexity of checking each row, column and minisquare from \$\Theta(d)\$ to \$\Theta(1)\$, where \$d\$ is the width/height of the input sudoku board.

Advice 2

What comes to the actual algorithm, I suggest you use recursion/backtracking. That will clean your code quite a bit, and will allow you better adapting it to, say, \$4 \times 4\$ or \$16 \times 16\$-sudokus.

The idea is as follows. You march through the board rows, each row from left to write. You leave the cells that have a predefined value as is, but you put 1 to the first empty cell, and recur to the next cell. If 1 does not belong, you try 2, 3 and so on. Once you have found a valid value, move to the next empty cell. At some point it might happen than none of the numbers fit. In such a case you go one step backwards (backtracking) and try increment the previous value. If you can translate from Java, see this, starting from line 163.


Run length encoding of an input string (Coderbyte 'Run Length' challenge) I'd like feedback on my solution to the outlined (medium level) programming challenge. What might be a more efficient or Pythonic solution?

The challenge as outlined by Coderbyte:

  [Run Length] (https://www.coderbyte.com/solution/Run%20Length?tblang=german)

  Have the function RunLength(str) take the str parameter being passed and
  return a compressed version of the string using the Run-length encoding
  algorithm. This algorithm works by taking the occurrence of each repeating
  character and outputting that number along with a single character of the
  repeating sequence. For example: "wwwggopp" would return 3w2g1o2p. The
  string will not contain any numbers, punctuation, or symbols.

import doctest
import logging
import timeit

logging.basicConfig(
    level=logging.DEBUG,
    format="%(asctime)s - %(levelname)s - %(message)s")
# logging.disable(logging.CRITICAL)

def compress_string(s: str) -> str:
    """
    Indicate the freq of each char in s, removing repeated, consecutive chars

    E.g. "aaabbc" outputs '3a2b1c'

    :param s: the string to compress
    :type s: str
    :return: s as a compressed string
    :rtype: str

    >>> compress_string(s="wwwggopp")
    '3w2g1o2p'
    >>> compress_string(s="aaa")
    '3a'
    >>> compress_string(s="")
    ''
    >>> compress_string(s="b")
    '1b'
    >>> compress_string(s="abcd")
    '1a1b1c1d'
    """
    if s == "":
        return ""

    # indexes of change in characters
    indexes = [i+1 for i in range(len(s) - 1) if s[i+1] != s[i]]

    # add start and end index for slicing of s
    indexes.insert(0, 0)
    indexes.append(len(s))

    # slice string
    slices = [s[indexes[i]:indexes[i+1]] for i in range(len(indexes)-1)]

    compressed_str = "".join(f"{len(s)}{s[0]}" for s in slices)

    return compressed_str

if __name__ == "__main__":
    print(timeit.timeit("compress_string(s='wwwggoppg')",
                        setup="from __main__ import compress_string",
                        number=100000))
    doctest.testmod()

You could just make a new list rather than use list.insert and list.append.

indexes.insert(0, 0)
indexes.append(len(s))

indexes = [0] + indexes + [len(s)]

For your "slice string" part, I'd make a function called pairwise. As this is a rather well know recipe in Python.

def pairwise(iterable):
    return (
        (iterable[i], iterable[i+1])
        for i in range(len(iterable) - 1)
    )

You could combine slices and compressed_str into one comprehension. If you do you don't need to use len as indexes[i+1] - indexes[i] is the length of the string.

return "".join(
    f"{b - a}{s[a]}"
    for a, b in pairwise(indexes)
)

def pairwise(iterable):
    return (
        (iterable[i], iterable[i+1])
        for i in range(len(iterable) - 1)
    )

def compress_string(s: str) -> str:
    if s == "":
        return ""

    indexes = [
        i+1
        for i in range(len(s) - 1)
        if s[i+1] != s[i]
    ]
    indexes = [0] + indexes + [len(s)]
    return "".join(
        f"{b - a}{s[a]}"
        for a, b in pairwise(indexes)
    )

Itertools

The pairwise function could be better described using the pairwise recipe.
If you utilize itertools.groupby the challenge is super easy, as it makes slices for you.

def compress_string(s: str) -> str:
    if s == "":
        return ""
    return "".join(
        f"{sum(1 for _ in g)}{k}"
        for k, g in itertools.groupby(s)
    )


Hibernate Native Query This is my code to populate a Class and later convert it to JSON object.

public List<CSATGraphJSON> convertListToCSATGraphJSON(List<Object[]> list) throws ParseException{
        List<CSATGraphJSON> customers=new ArrayList<CSATGraphJSON>();
        //going through each list of object
        for (int i = 0; i < list.size(); i++) {
            CSATGraphJSON graphJSON=new CSATGraphJSON();
            Object eachObject=list.get(i);
            int index=0;
            //iterating the each object array
            for(Object obj: (Object[]) eachObject){
                System.out.println(obj.toString());
                System.out.println(obj.getClass());
                switch(index){
                    case 0:{
                        if(obj!=null){
                            // if not null
                            /*DateFormat df = new SimpleDateFormat("yyyy-MM-dd",Locale.US);
                            Date parsedDate = null;
                            parsedDate = df.parse(obj.toString());
                            // gives you java.util.Date
                            customerList.setResponsedatestr(parsedDate);*/
                            graphJSON.setX(obj.toString());
                        }
                        break;
                    }
                    case 1:{
                        if(obj!=null){
                            graphJSON.setVery_poor(Integer.parseInt(obj.toString()));
                        }
                        break;
                    }
                    case 2:{
                        if(obj!=null){
                            graphJSON.setPoor(Integer.parseInt(obj.toString()));
                        }
                        break;
                    }
                    case 3:{
                        if(obj!=null){
                            graphJSON.setAverage(Integer.parseInt(obj.toString()));
                        }
                        break;
                    }
                    case 4:{
                        if(obj!=null){
                            graphJSON.setGood(Integer.parseInt(obj.toString()));
                        }
                        break;
                    }
                    case 5:{
                        if(obj!=null){
                            graphJSON.setVery_good(Integer.parseInt(obj.toString()));
                        }
                        break;
                    }
                    case 6:{
                        if(obj!=null){
                            graphJSON.setTotal(Integer.parseInt(obj.toString()));
                        }
                        break;
                    }
                    default:{
                        break;
                    }
                }
                index++;

            }
            customers.add(graphJSON);
        }

        return customers;
    }

Here List list the result of createSQLQuery() in hibernate as it returns List of Object ,so it is converted to List of CSATGraphJSON object.

Please suggest any alternative method for the switch statement as it is making the code lengthy.

Naming

Java method names should not contain underscore characters. It's also preferable to use CamelCase even for abbreviations for readability (e.g. graphJson instead of graphJSON. convertListToCSATGraphJSON is pretty clumsy and could stand to be renamed for simplicity and clarity.

Implementation

I'm going to assume there's a good reason you're hand-rolling this. If not, hibernate has tools to help do this for you.

You have both for loops backwards. The first one should be an enhanced for-loop, and the second should be old-school.

There is absolutely no reason for the second for loop or the switch statement to exist. Just directly set the values by accessing the index number in the Object[] directly.

You may run into trouble later using a List<Object[]>. Generics and arrays do not play nicely together. Changing that is outside the scope of this review.

Small static helper methods can greatly clean up the variable assignments.

If you were to make all these changes, your code might look more like:

public List<CSATGraphJSON> convertListToCSATGraphJSON(final List<Object[]> list) throws ParseException {
    final List<CSATGraphJSON> customers = new ArrayList<CSATGraphJSON>();
    for (final Object[] objects : list) {
        final CSATGraphJSON graphJson = new CSATGraphJSON();
        graphJson.setX(parseString(objects[0]));
        graphJson.setVeryPoor(parseInt(objects[1]));
        graphJson.setPoor(parseInt(objects[2]));
        graphJson.setAverage(parseInt(objects[3]));
        graphJson.setGood(parseInt(objects[4]));
        graphJson.setVeryGood(parseInt(objects[5]));
        graphJson.setTotal(parseInt(objects[6]));
        customers.add(graphJson);
    }

    return customers;
}

private static String parseString(final Object o) {
    if (o == null) {
        return null;
    }
    return o.toString();
}

private static int parseInt(final Object o) {
    if (o == null) {
        return 0;
    }
    return Integer.parseInt(o.toString());
}


Simple clock view model I have been trying to wrap my head around MVVM for the last week or more and still struggling a bit. I have watched Jason Dolingers MVVM video and gone through Reed Copsey lessons and still find myself wondering if i am doing this right...   I found both sources very interesting yet a bit different on the approach.  If anyone has any other links, I would be interested as I would really like to learn this.

What is the best practice for the model to alert the viewmodel the something has happened?  As you will see in the code below, I created a very simple clock application.  I am using an event in my model but am not sure if this is the best way to handle this.  The output of the program is as expected, however I'm more interested in if I'm actually using the pattern correctly.  Any thoughts comments etc would be appreciated.

My model

using System;
using System.Threading;

namespace Clock
{
    public class ClockModel
    {
        private const int TIMER_INTERVAL = 50;

        private DateTime _time;

        public event Action<DateTime> TimeArrived;

        public ClockModel()
        {
            Thread thread = new Thread(new ThreadStart(GenerateTimes));
            thread.IsBackground = true;
            thread.Priority = ThreadPriority.Normal;
            thread.Start();
        }

        public DateTime DateTime
        {
            get
            {
                return _time;
            }
            set
            {
                this._time = value;
                if (TimeArrived != null)
                {
                    TimeArrived(DateTime);
                }
            }
        }

        private void GenerateTimes()
        {
            while (true)
            {
                DateTime = DateTime.Now;
                Thread.Sleep(TIMER_INTERVAL);
            }
        }
    }
}

My View

<Window x:Class="Clock.MainWindow"
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
        xmlns:ViewModels="clr-namespace:Clock"
        Title="MainWindow" Height="75" Width="375">
    <Window.DataContext>
        <ViewModels:ClockViewModel />
    </Window.DataContext>
    <StackPanel Background="Black">
            <TextBlock Text="{Binding Path=DateTime}" Foreground="White" Background="Black" FontSize="30" TextAlignment="Center" />
    </StackPanel>
</Window>

My View Model

using System;
using System.ComponentModel;

namespace Clock
{
    public class ClockViewModel : INotifyPropertyChanged
    {
        private DateTime _time;
        private ClockModel clock;

        public ClockViewModel()
        {
            clock = new ClockModel();
            clock.TimeArrived += new Action<DateTime>(clock_TimeArrived);
        }

        private void clock_TimeArrived(DateTime time)
        {
            DateTime = time;
        }

        public DateTime DateTime
        {
            get
            {
                return _time;
            }

            set
            {
                _time = value;
                this.RaisePropertyChanged("DateTime");
            }
        }

        /// <summary>
        /// Occurs when a property value changes.
        /// </summary>
        public event PropertyChangedEventHandler PropertyChanged;

        /// <summary>
        /// Raises the property changed event.
        /// </summary>
        /// <param name="propertyName">Name of the property.</param>
        private void RaisePropertyChanged(string property)
        {
            if (PropertyChanged != null)
            {
                PropertyChanged(this, new PropertyChangedEventArgs(property));
            }
        }
    }
}

It seems your implementation is correct. You also touch a common discussion of MVVM.

In MVVM should the ViewModel or Model implement INotifyPropertyChanged?

One could argue you could let the model implement INotifyPropertyChanged. I'm not experienced enough with MVVM to answer this with a pro/contra argumentation.

The main intent however is implemented and the separation is there either way.


Update the frequency of Sidekiq job I'm working on a feature which user can set frequency of a job. I am using Sidekiq.

My solution is when someone updates the frequency from view, I will delete all old scheduled jobs and create a new job in the action of controller.

I'm looking for a cleaner and more succinct way to do this.

Right now, I have a frequency column on the settings table. There is an update_frequency method in DashboardController to delete all old scheduled jobs and create a new scheduled job.

app/controllers/dashboard_controller.rb

class DashboardController < ApplicationController
  def update_frequency
    setting = Setting.find_by(id: params[:id])
    if setting.blank?
      render json: {status: 404, msg: 'Not found.'}
    else
      if setting.update_attributes(frequency: params[:frequency])
        # find and delete all scheduled jobs
        scheduler = Sidekiq::ScheduledSet.new
        old_jobs = scheduler.select {|work| work.klass == 'HardWorker'}
        old_jobs.each(&:delete) if old_jobs.present?

        # add new schedule job
        HardWorker.perform_in(setting.frequency.minutes)

        render json: {status: 200, msg: 'update frequency successful.'}
      else
        render json: {status: 500, msg: 'update frequency failed.'}
      end
    end
end

A HardWorker to do something and create a new scheduled job:

app/workers/hard_worker.rb

class HardWorker
  include Sidekiq::Worker

  def perform
    # Do something
    setting = Setting.first
    HardWorker.perform_in(setting.frequency.minutes)
  end
end

I don't think there is a more succinct way to do that with Sidekiq, you will probably need to delete and recreate jobs.

Some comments

You probably don't need to check if old_jobs.present? when deleting. I am not familiar with ScheduledSet#select but I would imagine that it returns an empty array when no job is selected.

You don't need to update your jobs when the frequency is the same as the one already set. Even if you check it in the frontend it is a good idea to check it here to in case the view and the database are out of sync (read: in case someone changed the setting while your view was open).

Instead of using nested ifs, I like to use guards.
I also like to extract local variables into methods (see def setting).

Here is an example of how you could refactor it.

It may seem longer, but it is easier to understand when looking at the update_frequency method. If you need more precise understanding on what is considered as same_frequency, or reschedule_jobs, then you can dive into these methods. Otherwise looking at update_frequency probably gives you everything you need to understand when browsing the code (in 6 months when you will have forgotten of this part)

class DashboardController < ApplicationController
  def update_frequency
    return render_not_found unless setting
    return render_same_frequency if same_frequency?
    return render_update_failed unless update_model

    reschedule_jobs

    render json: { status: 200, msg: 'update frequency successful.' }
  end

  def render_not_found
    render json: { status: 404, msg: 'Not found.' }
  end

  def render_update_failed
    render json: { status: 500, msg: 'update frequency failed.' }
  end

  def render_same_frequency
    render json: { status: 200, msg: 'Frequency updated to the same value' }
  end

  def update_model
    setting.update_attributes(frequency: new_frequency)
  end

  def same_frequency?
    setting.frequency == params[:frequency].to_i
  end

  def setting
    @setting ||= Setting.find_by(id: params[:id])
  end

  def reschedule_jobs
    # find and delete all scheduled jobs
    Sidekiq::ScheduledSet.new
                         .select { |work| work.klass == 'HardWorker' }
                         .each(&:delete)

    # add new schedule job
    HardWorker.perform_in(setting.frequency.minutes)
  end
end


Slow data-processing and inefficient memory usage in .NET containers I am writing a text classifier, and in order to do so, I need TF/IDF values per every word of my single text.

Then I need to use the cosine similarity:

$$similarity = cos(\theta) = \dfrac{A \cdot B}{\lVert A \lVert \lVert B \lVert} = \dfrac{\overset{n}{\underset{i=1}{\LARGE\Sigma}}A_i \times B_i}{\sqrt{ \overset{n}{\underset{i=1}{\LARGE\Sigma}}(A_i)^2} \times \sqrt{\overset{n}{\underset{i=1}{\LARGE\Sigma}}(B_i)^2}}$$

This requires processing of a big data storage (all of the texts that already exists in my database). The problem is that my code is doing his job for about 2 hours (quite too long) and breaks giving me message that I have run out of memory. I think that the main method might be very, very unoptimised.

public static void CreateCategoryClasses()
{
    deserializeClasses = Deserialize();
    howManyClasses = deserializeClasses.Count;
    ewhClass = new EventWaitHandle[howManyClasses];

    for (var i = 0; i < 5; ++i)
    {
        categoryClasses.Enqueue(new ConcurrentDictionary<string, double>());
        result.Enqueue(new ConcurrentDictionary<string, double>());
    }
    WaitCallback threadMethod = ParseCategories;
    ThreadPool.SetMaxThreads(howManyStudents, howManyClasses);

    for (var i = 0; i < howManyClasses; ++i)
    {
        ewhClass[i] = new EventWaitHandle(false, EventResetMode.AutoReset);
        ThreadPool.QueueUserWorkItem(threadMethod, i);
    }
    for (var i = 0; i < howManyClasses; ++i)
    {
        ewhClass[i].WaitOne();
    }
    threadMethod = AddIDF;
    ThreadPool.SetMaxThreads(howManyStudents, 5);

    for (var i = 0; i < 5; ++i)
    {
        ewhClass[i] = new EventWaitHandle(false, EventResetMode.AutoReset);
        ThreadPool.QueueUserWorkItem(threadMethod, i);
    }
    for (var i = 0; i < 5; ++i)
    {
        ewhClass[i].WaitOne();
    }

    List<List<SingleWords>> efekt = new List<List<SingleWords>>(5);

    for (int i = 0; i < 5; ++i)
    {
        foreach (var secondWord in categoryClasses.ToList()[i])
        {
            result[i].Add(new SingleWords(secondWord.Key, secondWord.Value));
        }
    }
    var xmls = new XmlSerializer(typeof(List<List<SingleWords>>));
    using (var sw = new StreamWriter(@"categoryClasses.xml"))
    {
        xmls.Serialize(sw, efekt);
    }
}

private static void AddIDF(object index)
{
    Console.WriteLine("Thread started with id:" + index + " and number: " + Thread.CurrentThread.ManagedThreadId);
    var i = index as int?;
    double sum;
    foreach (var word in categoryClasses.ElementAt(i.Value))
    {
        sum =
            deserializeClasses.Count(
                clas =>
                    clas.Bag.Where(x => clas.Category == ((Categories)i.Value).ToString())
                        .Contains(new Words(word.Key, 0, 0)));
        var temp = Convert.ToDouble(sum) /
                   Convert.ToDouble(
                       deserializeClasses.Count(x => x.Category == ((Categories)i.Value).ToString()));
        result.ElementAt(i.Value).AddOrUpdate(word.Key, temp, (s, d) => temp);
    }
    ewhClass[(i).Value].Set();
    Console.WriteLine("Thread ended with id:" + index + " and number: " + Thread.CurrentThread.ManagedThreadId);
}

Edit in response to OP comment above:

CreateCategoryClasses may appear to be slow because of the WaitOne calls.  You're essentially waiting waiting on either ParseCategories or AddIDF to complete before moving on. It would help if you could also post the ParseCategories method as well.  Otherwise, you're just adding stuff to collections, which generally shouldn't perform too bad, except for some of my comments below.

Original answer:

Some of this is performance suggestions, some is just general advice...

If you have one, run this through a performance profiler. This can point you directly to the lines that are using the most CPU time very quickly. Personally, I use ANTS from Red Gate.  Another good one is DotTrace from JetBrains.
Why is categoryClasses a ConcurrentQueue?  As a general rule, a Queue is used for removing the items as you process them. If you just need to loop over a collection (possibly multiple times), a List<T> would be a better choice. This would also avoid the call to ToList() each time through your secondWord loop.  ToList() will iterate through the entire collection every time you call it (granted it's only 5 element, but still...). This may also apply to result as well, depending on where else it might be used.
You call ElementAt() every time through your foreach loop in AddIDF, even though i isn't changing.  If you must use a ConcurrentQueue for categoryClasses, then ElementAt() is \$O(i)\$.  If you can use a List, then it will be \$O(1)\$.  You could save the current ConcurrentDictionary before the loop to avoid the repetitive indexing.
String comparisons are slow compared to integers and enums (your Where() calls in AddIDF). If you changed Category to an enum or integer (or possibly add a second property for comparison purposes), that would improve the count performance.
You pass an int into your AddIDF method, not an int?, so you can just do a direct cast to an int.  This avoids the .Value property everywhere you access the index in AddIDF.
sum is already a double, so there's no need to call Convert.ToDouble. Because sum is a double, temp will be a double, even if the result of Count is an int.
What is this 5 I keep seeing?  Typically it's good to assign this to a const value for clarity.


Listing all numbers that are more composite than every number less than it This code is my first naive attempt at writing a program that lists all numbers less than a given value that are more composite (have more divisors) than any number less than that number (for example, 12 is more composite than any number less than it).

def composite_count(limit):
    answer = []
    most_divisors = 0
    for i in range(limit):
        tally = 0
        for j in range(i):
            if not (i + 1) % (j + 1):
                tally += 1
        if tally > most_divisors:
            most_divisors = tally
            answer.append((i + 1))
    return answer

After running the program a few times, I started to see some patterns like the fact that all of the listed answers were even, that all answers greater than 12 are divisible by 12, or that all answers greater than 60 are divisible by 60. I also ran some tests involving prime factorization and found that although the more composite numbers do not always have more prime factors, the sum of their prime factors are always larger. Despite all this information I was not able to figure out any changes to the algorithm that would create significant time saves.

Can someone please help me make this code meaningfully faster?

The biggest thing that will gain you performance is breaking this up into two functions, one that gets the prime factor list of a number, and another that computes the total number of factors from a prime factorization (using This method). Using this, we get the following

def generate_primes(end):
    primes = []
    for i in range(2, end):
        for j in range(2, i):
            if i % j == 0:
                break
        else:
            primes.append(i)
    return primes

def prime_power_generator(x, primes):
    # Most of these steps will not be run.
    # The largest prime iterated to will be the largest prime factor of x
    # (not including x)
    for p in primes:
        if x % p == 0:
            times = 0
            while x % p == 0:
                times += 1
                x //= p
            yield times
            if x == 1:
                return

def number_of_factors_with_help(x, primes):
    answer = 1
    for power in prime_power_generator(x, primes):
        answer *= power+1
    return answer

def better_composite_count(limit):
    answer = [1]
    primes = generate_primes(int(limit**.5)+1)
    most_divisors = 0
    for x in range(2,limit+1,2):
        num_divisors = number_of_factors_with_help(x, primes)
        if num_divisors > most_divisors:
            most_divisors = num_divisors
            answer.append(x)
    return answer

By limit=10**5, this is clearly superior, taking .02 seconds, instead of 50 for the OP. However, there is a better way. These numbers have several properties that allow much more a much faster search. For any highly compoite n, the prime factorization of n:

contains consecutive primes starting with 2
with decreasing frequency
the largest prime factor has frequency 1 (except for 4 and 36)

There are very few numbers that satisfy these constraints, and computing them by their prime factorization means we don't have to factor them later. These insights leads to the following code:

def gen_primes():
    D = {}
    q = 2  # first integer to test for primality.

    while True:
        if q not in D:
              # not marked composite, must be prime
              yield q

              #first multiple of q not already marked
              D[q * q] = [q]
        else:
              for p in D[q]:
                    D.setdefault(p + q, []).append(p)
              # no longer need D[q], free memory
              del D[q]
        q += 1

def num_factors(prime_factorization):
    ans = 1
    for power in prime_factorization:
        ans *= power+1
    return ans

def composite_candidates(primes, limit, factors=(1,), num=2, candidates=None, idx=0):
    if candidates == None:
        candidates={1:1, 4:3, 36:9}
    if num < limit:
        if factors[-1] == 1:
            candidates[num] = num_factors(factors)
        # current exponent must be equal or less than previous one
        if len(factors) == 1 or factors[-2] > factors[-1]:
            # we either stay with current prime and increase exponent
            composite_candidates(primes,
                                 limit,
                                 factors[:-1] + (factors[-1] + 1,),
                                 num * primes[idx],
                                 candidates,
                                 idx)
        # or move to next prime
        idx += 1
        composite_candidates(primes,
                             limit,
                             factors + (1,),
                             num * primes[idx],
                             candidates,
                             idx)
    return candidates

def best_composite_count(limit):
    # primes up to the largest possible in a highly composite number
    primes = []
    prime_product = 1
    for prime in gen_primes():
        primes.append(prime)
        prime_product *= prime
        if prime_product > limit:
            break

    # generate numbers, and number of factors for numbers that might work
    candidates = composite_candidates(primes, limit)
    composites = []
    most_divisors = 0
    # its important that they are sorted to know current highest number of divisors
    for num in sorted(candidates.keys()):
        num_divisors = candidates[num]
        if num_divisors > most_divisors:
            composites.append(num)
            most_divisors = num_divisors
    return composites

This yields another similarly massive increase over the previous, and can sieve up to 10^30 in less than 5 seconds.


Inserting e-mail subscribers into MySQL I'm new to Python and I am learning database connections from here.

This is the code I am using to insert e-mail subscribers into a table:

conn = mysql.connector.connect(user="user", password="password", host="127.0.0.1", database="db")
cursor = conn.cursor()
query = ("INSERT INTO test(name,email) VALUES(%s,%s)")
data = ("cool", "cool")
cursor.execute(query, data)
conn.commit()
cursor.close()
conn.close()

Is this type of data insertion safe and can stop SQL injections?

You've got the right idea! By passing the values in along with the query you're doing what other languages call a "prepare" on the final query to make sure that the values being passed are quoted/escaped properly to avoid issues with injection.


Letter-scoring Scrabble class I wrote Ruby code (copied below) but was told that the code does not compile (it does on my computer) and that it does not answer the question:

  Create a class called Scrabble that implements a method called score that accepts one word as a parameter and returns the scrabble score associated with that word

The code compiles on my computer and provides the response that I believe was required.  Can anyone point out issues with the code quality? I realize I did not write any unit tests for this code.

I could add more inline comments in the code (I added comments at the top of the class which I removed from the code I copied here).

class Scrabble

  attr_reader :word

  def initialize(word)
    @word = word
  end

  def score
    letters = word.upcase.split('')

    total = 0
    letters.each do |letter|
      total += letter_scores[letter]
    end

    total
  end

# scores for each letter
  def letter_scores
    {  "A"=>1, "B"=>3, "C"=>3, "D"=>2,
      "E"=>1, "F"=>4, "G"=>2, "H"=>4,
      "I"=>1, "J"=>8, "K"=>5, "L"=>1,
      "M"=>3, "N"=>1, "O"=>1, "P"=>3,
      "Q"=>10, "R"=>1, "S"=>1, "T"=>1,
      "U"=>1, "V"=>4, "W"=>4, "X"=>8,
      "Y"=>4, "Z"=>10
    }
  end
end

puts "Type in one word for scrabble scorer"
word = gets.chomp
puts "Scrabble score for " + word + " is " + Scrabble.new(word).score.to_s

Some comments:

score method: With functional - instead of imperative - style, you'll get write more compact code.
You may use the abstraction Hash#values_at.
letter_scores is constant. It can be a class constant, then.
String interpolation: Use "#{var} = #{value}" style.
[edit] As you say, word should be an argument of score, not the class initializer.

I'd write:

class Scrabble
  LETTER_SCORES = {...}

  def score(word)
    scores = LETTER_SCORES.values_at(*word.upcase.chars)
    scores.compact.reduce(0, :+)
  end
end

scrabble = Scrabble.new
puts("Type in one word for scrabble scorer:")
word = gets.strip
puts("Scrabble score for #{word} is #{scrabble.score(word)}")


Vehicle-crossing simulation It seems like this code should be shorter, but with all the error checking it is long and hard to follow. This is for a simulation of vehicles crossing a bridge, and this part is dealing with mutexes. I tried extracting a function that is passed the mutex and locks/unlocks it and prints any errors, but when I did that it broke the code (it's been my experience on this site to not include any broken code so I won't elaborate on what I tried).

/*global variables*/
pthread_mutex_t Bridge = PTHREAD_MUTEX_INITIALIZER;
pthread_mutex_t okToOutput = PTHREAD_MUTEX_INITIALIZER;
/*...*/
/*this is the function that gets passed to pthread_create()*/
void* startSim(void *arg)
{

    Vehichle* inData = (Vehichle*)arg;
    if(usleep(inData->sleepT1))
        cerr << "Vehichle " << inData->licencePlate << " failed to sleep" << endl;
    if(pthread_mutex_lock(&okToOutput))
        cerr << "failed to lock output" << endl;
    cout << "Vehichle " << inData->licencePlate << " is ready to go " << dirToStr(inData->direction) << endl;
    if(pthread_mutex_unlock(&okToOutput))
        cerr << "failed to unlock output" << endl;

    if(pthread_mutex_lock(&Bridge))
        cerr << "failed to lock bridge" << endl;
    if(pthread_mutex_lock(&okToOutput))
        cerr << "failed to lock output" << endl;
    cout << "Vehichle " << inData->licencePlate << " is ON the main bridge going " << dirToStr(inData->direction) << endl;
    if(pthread_mutex_unlock(&okToOutput))
        cerr << "failed to unlock output" << endl;
    if(usleep(inData->sleepT2))
        cerr << "Vehichle " << inData->licencePlate << " failed to sleep" << endl;
    if(pthread_mutex_lock(&okToOutput))
        cerr << "failed to lock output" << endl;
    cout << "Vehichle " << inData->licencePlate << " is OFF the main bridge after going " << dirToStr(inData->direction) << endl;
    if(pthread_mutex_unlock(&okToOutput))
        cerr << "failed to unlock output" << endl;
    if(pthread_mutex_unlock(&Bridge))
        cerr << "failed to lock bridge" << endl;

    return NULL;
}

My compiler doesn't properly support C++11 so no smart pointers etc.

As mentioned in the comments, there's not much to review.

Don't abuse using namespace std

Putting using namespace std within your program is generally a bad habit that you'd do well to avoid.

Factor out common code

In this case, the code gets a lock, does something, and releases the lock.  That could easily be isolated to a common function:

class Locker
{
     pthread_mutex_t&   mutex;
     bool               ok;
   public:
     operator bool() const {return ok;}

     Locker(pthread_mutex_t& mutex)
         : mutex(mutex)
         , ok(true)
     {
         if(pthread_mutex_lock(&mutex) != 0) {
             cerr << "failed to lock output" << endl;
             ok = false;
         }
     }
     ~Locker()
     {
         if (ok) {
             if(pthread_mutex_unlock(&mutex) != 0) {
                 cerr << "failed to unlock output" << endl;
             }
         }
     }
};
void report(Vehicle *v, const char *msg)
{
    Locker    lock(okToOutput);
    if (lock) {
        cout << "Vehicle " << v->licencePlate << msg << dirToStr(v->direction) << endl;
    }
}

To do this correctly, a failure to lock should exit the routine and not attempt to send output data.

Avoid C-style casts

Instead of (Vehicle *)arg use static_cast<Vehicle *>(arg).  It makes much more clear to humans reading your code what you're doing.


Custom jQuery validation error messages We have a rule that all of our validation messages must be in a summary, and thus the default "this field is required" doesn't cut it because the messages lose their context in a summary and therefore need specific field indicators.

I have a solution that I like rather well, but it soon became clear that there was a need for messages outside of just the required field (email, URL, custom methods like phoneUS, etc), so I made some additions to my function.

I've been using jQuery for a while, but I'm not an expert in the optimization area, so I wanted to get some expert help on whether the function below could be optimized...my question is, "is there actually a better way to handle custom error messages in a summary?"

$('.required, .email').each(function(index) {
  var $this = $(this);

  var label = (
    $this.is(':radio')
    ? $("label[data-name='"+$this.attr('name')+"']")
    : label = $("label[for='"+$this.attr('id')+"']")
  );

  var customMessages = [{}];

  if($this.hasClass('required')){
    customMessages.required = "'" + label.text() + "' is required.";
  }

  if($this.hasClass('email')){
    customMessages.email = "'" + label.text() + "' has an invalid email address.";
  }

  $this.rules("add", {
    messages: customMessages
  });
});

Here is the jsFiddle:
http://jsfiddle.net/GD5nw/1/

This looks fine to me.

I only have 2 minor points:

You could assign the .text() once in the var statements so that your string concats look like customMessages.email = "'" + label + "' has an invalid email address."; This way you are not repeating text() all over the place.
var customMessages = [{}]; looks odd, because you are afterwards assigning straight to the array instead of the empty object. Simply replacing that with var customMessages = {};  works in the fiddle.


Minimalist HyperScript implementation I've tried to roll out my own HyperScript implementation. I would be interested in any suggestions to improve it or point me to any other better implementation which is still maintained. I checked the HyperScript project, but it doesn't seem to have any recent updates, so I tried this:

var DOM = {
    h: function(tagName, props, children) {
        var element = document.createElement(tagName);
        if (props) {
            for (var item in props) {
                element[item] = props[item];
            }
        }
        if (children && Array.isArray(children) && children.length > 0) {
            children.forEach(function(child) {
                element.appendChild(child);
            });
        }
        return element;
    },
    div: function(props, children) {
        return this.h('div', props, children);
    },
    label: function(props, children) {
        return this.h('label', props, children);
    },
    input: function(props, children) {
        return this.h('input', props, children);
    },
    button: function(props, children) {
        return this.h('button', props, children);
    },
    li: function(props, children) {
        return this.h('li', props, children);
    }
};

Usage:

DOM.button({ className: "destroy" });

The fun thing about these kinds of libraries is that they're fun to implement. The sad thing about them is that they aren't really fun to work with. You will beg for markup-like syntax in the long run.

But anyways,

div: function(props, children) {
    return this.h('div', props, children);
},
label: function(props, children) {
    return this.h('label', props, children);
},
input: function(props, children) {
    return this.h('input', props, children);
},
button: function(props, children) {
    return this.h('button', props, children);
},
li: function(props, children) {
    return this.h('li', props, children);
}

This can be simplified into a list of element names. You can run through it and assign a function to DOM for each name.

['div', 'li', ...].forEach(elementName => {
  DOM[elementName] = function(props, children){
    return this.h(elementName, props, children);
  };
});

As for your h function, you should guard for-in with hasOwnProperty to avoid iterating over prototype elements. Alternatively, you can use Object.keys and forEach. Also, silencing errors is terrible when debugging. If the user passed in something not an array, best inform them immediately. In the following code, it only catches on falsy values. Anything else, like a non-array or something will throw because forEach expects an array.

h: function(tagName, props, children) {
    var element = document.createElement(tagName);

    Object.keys(props || []).forEach( prop => {
      element[prop] = props[prop];
    });

    (children || []).forEach(child => {
      element.appendChild(child);
    });

    return element;
},


Return the correct object type in Javascript I am currently wondering if the following piece of Javascript code could be simplified? I would like to return the correct {},[],"" without having to pass a type as argument. Is it possible?

function getObjectProperty(object,field,type){
    switch (type){
      case "Array":
        return object && object.hasOwnProperty(field) ? object[field] : [];
        break;
      case "String":
        return object && object.hasOwnProperty(field) ? object[field] : "";
        break;
      case "Object":
        return object && object.hasOwnProperty(field) ? object[field] : {};
        break;
      case "Boolean":
        return object && object.hasOwnProperty(field) ? object[field] : false;
        break;
    }
  }

1)   I am currently wondering if the following piece of Javascript code could be simplified?

yes, something like this, would be more accurate and simpler to read

var defaultValues = {
  "Array": [],
  "Boolean": true,
  "Object": {},
  "String": ""
}

function getObjectProperty(object,field,type){
  if (object.hasOwnProperty(field)) return object[field]
  return defaultValues[type]
}

  2) I would like to return the correct {},[],"" without having to pass a type as argument. Is it possible?

This is only possible, if the field exist.
Otherwise whats the type of foo in {bar: "bar"} ?

But what are you actually doing:

check if the field is in the object
If true, return the value, otherwise a defaultValue

So you could simplify it to this:

function getValue(obj, field, defaultValue) {
   return obj.hasOwnProperty(field) ? obj[field] : defaultValue
}


Move object by one up or down algorithm in a custom order Basically, I did an object (using hibernate) with a field called sorting_order. This field needs to be unique and I wish to swap two object by one. So one element has to be after or before the current element to be able to move. My object can only move by one up or down. I came up with this code, but I was wondering if there was a better way to do this.

Thank you!

/**
     * Move two personal task macro
     *
     * @param userProfileModel - The {@link UserProfileModel}.
     * @param move - true = up or false = down.
     */
    private void movePersonnalTaskMacro(UserProfileModel userProfileModel, boolean move) {
        Assert.notNull(userProfileModel, "userProfileModel required.");

        logger.info("Preparing to move a personnal task macro.");

        PersonnalTaskMacro selectedPersonnalTaskMacro = userProfileModel.getSelectedPersonnalTaskMacro();

        /* Initialize sorting order (increment/decrement) */
        int selectedPersonnalTaskMacroSortOrder = selectedPersonnalTaskMacro.getSortingOrder();
        int nextPersonnalTaskMacroSortOrder = selectedPersonnalTaskMacroSortOrder + 1;
        int previousPersonnalTaskMacroSortOrder = selectedPersonnalTaskMacroSortOrder - 1;

        /* Condition validation */
        int personnalTaskMacroListSize = userProfileModel.getPersonnalTaskMacroList().size();
        int selectedPersonnalTaskMacroIndex = userProfileModel.getPersonnalTaskMacroList().indexOf(
                selectedPersonnalTaskMacro);

        if (personnalTaskMacroListSize > 1) {
            if (move == false && selectedPersonnalTaskMacroIndex >= 1) {
                int previousPersonnalTaskMacroIndex = selectedPersonnalTaskMacroIndex - 1;
                PersonnalTaskMacro previousPersonnalTaskMacro = userProfileModel.getPersonnalTaskMacroList().get(
                        previousPersonnalTaskMacroIndex);

                previousPersonnalTaskMacro.setSortingOrder(selectedPersonnalTaskMacroSortOrder);
                selectedPersonnalTaskMacro.setSortingOrder(previousPersonnalTaskMacroSortOrder);

                logger.info("Moved the selected personnal task macro down by one");
            }

            if (move == true && nextPersonnalTaskMacroSortOrder <= personnalTaskMacroListSize) {
                int nextPersonnalTaskMacroIndex = selectedPersonnalTaskMacroIndex + 1;
                PersonnalTaskMacro nextPersonnalTaskMacro = userProfileModel.getPersonnalTaskMacroList().get(
                        nextPersonnalTaskMacroIndex);

                nextPersonnalTaskMacro.setSortingOrder(selectedPersonnalTaskMacroSortOrder);
                selectedPersonnalTaskMacro.setSortingOrder(nextPersonnalTaskMacroSortOrder);

                logger.info("Moved the selected personnal task macro up by one");
            }

            Collections.sort(userProfileModel.getPersonnalTaskMacroList());
        }
    }

private void movePersonnalTaskMacro (UserProfileModel userProfileModel, boolean move) {

This is a private method but it does not access any members of its class. It is a smell. This probably means its home is one of the parameters, (usually first). UserProfileModel seems like a case of smurf typing. boolean move we will probably need a Replace Parameter with Method

PersonnalTaskMacro selectedPersonnalTaskMacro = userProfileModel.getSelectedPersonnalTaskMacro();

/* Initialize sorting order (increment/decrement) */
int selectedPersonnalTaskMacroSortOrder = selectedPersonnalTaskMacro.getSortingOrder();

A case of Inappropriate Intimacy. Call chains such as userProfileModel.getSelectedPersonnalTaskMacro().getSortingOrder() usually are.

int personnalTaskMacroListSize = userProfileModel.getPersonnalTaskMacroList().size();
int selectedPersonnalTaskMacroIndex = userProfileModel.getPersonnalTaskMacroList().indexOf(
        selectedPersonnalTaskMacro);

More cases of the same. Also we see down .getPersonnalTaskMacroList() returns the actual field (Because you can sort it outside of the owner class). The collection should be encapsulated.

Since you plan to do nothing if personnalTaskMacroListSize is not greater than 1. say explicitly so. It makes it more readable, and saves you also from Arrow Code.

if (personnalTaskMacroListSize > 1) {

to

if (personnalTaskMacroListSize <= 1) return ;

Dispatch on parameter, is a bad idea:

  if (move == false

  if (move == true

Moreover it is not needed in the first place. Many times user clicks Up or Down, and instead of

  onUpClicked() {
    movePersonnalTaskMacroUp();
  }

  onDownClicked() {
    movePersonnalTaskMacroDown();
  }

We get something like:

  onUpClicked() {
    movePersonnalTaskMacro(true);
  }

  onDownClicked() {
    movePersonnalTaskMacro(false);
  }

Less clear while reading the call site. Less clear while reading the implementation. Hides (some times loses) the users intent in the meantime.

personnalTaskMacro.getSortingOrder
PersonnalTaskMacro.setSortingOrder Why does a Personal Task Macro needs to know in what order it participates in some collection of User Profile. If we decide to have a dozen more different lists of such macros, will we add a dozen more fields to Personal Task Macros. I am guessing this was because of some ORM issue. You had a sort order column in personalTaskMacro table so you had a property in that class. If that is the case you should fix your mapping. If not, why was it?

Collections.sort(userProfileModel.getPersonnalTaskMacroList());

Why do you need to sort if you only swapped two elements of the list. Also as mentioned above collections should be encapsulated. (If you need to have a property so your ORM can access it, make those getter and setter private.)

Here is my suggestion. Main refactorings are Move Method and Replace Parameter with Explicit Methods and Replace Nested Conditional with Guard Clauses. Also DRY the element swapping as suggested by @tomdemuyt.

class UserProfile {
    List<PersonalTask> personalTaskList;
    int selected;

    public void moveSelectedUp() {
        if (personalTaskList.size() <= 1) return;

        if (selected <= 0) return;

        swap(personalTaskList, selected, selected - 1);
        selected--;
    }

    public void moveSelectedDown() {
        if (personalTaskList.size() <= 1) return;

        if (selected >= personalTaskList.size() - 1)  return;

        swap(personalTaskList, selected, selected + 1);
        selected++;
    }

    static <T> void swap(List<T> list, int a, int b) {
        T tmp = list.get(a);
        list.set(a, list.get(b));
        list.set(b, tmp);
    }
}


Privacy approach in JavaScript I have come up with an idea for adding privacy support in JavaScript. I haven't found something similar in the net, so I'm thinking it's probably because the idea is bad, but yet, I want to see some response, just to be sure.

var util = {
    s4: function() {
        return Math.floor((1 + Math.random()) * 0x10000)
                    .toString(16)
                    .substring(1);
    },

    guid: function() {
        return util.s4() + util.s4() + '-' + util.s4() + '-' + util.s4() + '-' +
                util.s4() + '-' + util.s4() + util.s4() + util.s4();
    }
};

// template
var Class = (function() {
    var privates = {};
    var getPrivates = function(obj) {
        return privates[ obj.getGUID() ];
    };

    function Class() {
        var guid = util.guid();
        privates[guid] = {};
        this.getGUID = function() {
            return guid;
        };
    };

    return Class;
})();

var MyClass = (function() {
    var privates = {};
    var getPrivates = function(obj) {
        return privates[ obj.getGUID() ];
    };

    function MyClass(a, b) {
        var guid = util.guid();
        privates[guid] = {};
        this.getGUID = function() {
            return guid;
        };

        var private = getPrivates(this);
        private.a = a;
        this.b = b;
    };

    return MyClass;
})();

var obj = new MyClass("private", "public");
console.log(obj.a, obj.b);

Interesting question,

as mentioned in the comments, privacy support can be (better) achieved with modules, or with IFFE's.

On the whole your design is not sound, your GUID creation code is so weak in generating unique id's that you will have overlapping GUID's and (hard to reproduce) bugs.

Furthermore, the variables are not exactly private if you can iterate over privates and see all the values stored in there..

Finally, I think you might have a pretty good case for a double assignment here, this

var MyClass = (function() {
    var privates = {};
    var getPrivates = function(obj) {
        return privates[ obj.getGUID() ];
    };

    function MyClass(a, b) {
        var guid = util.guid();
        privates[guid] = {};
        this.getGUID = function() {
            return guid;
        };

        var private = getPrivates(this);
        private.a = a;
        this.b = b;
    };

    return MyClass;
})();

could be

var MyClass = (function() {
    var privates = {};

    function MyClass(a, b) {
        var private = privates[util.guid()] = {};
        private.a = a;
        this.b = b;
    };

    return MyClass;
})();

From a lint/hint perspective you have some semicolon trouble, but nothing too bad.


Point distance program written without a framework I have been developing a cross-platform game based on open C libraries (mainly glfw) with a direct focus on Windows development. The beginning of my programming career taught me familiarity in Objective-C, however I've been using pure C for the past few years in embedded applications.

Now that I'm trying to return to proper desktop program development, I'm wondering if it's a good idea to transfer my Objective-C skills to Windows in order to tidy up my C code. My current way of programming is creating a struct for data that I need and then creating functions around those structs: struct Point {int x, int y}; and then float Point_calculateDistance(struct Point *obj1, struct Point *obj2). This approach to me seemed very redundant and too object-oriented to write all in C. It would make more sense to me to write an Objective-C class to do this where everything can be more encapsulated.

I prefer not to use any framework for my code to increase the portability. I'm currently using VS Code and MSYS2 (mingw64) with GCC 8.3.0. I threw together a working example that demonstrates how I attempt to build a simple class. Does my code look reasonable? Am I performing any bad habits (within reason considering that I'm not using a framework)? Does this currently leak memory? Let me know what you think.

#include <objc/objc.h>
#include <objc/runtime.h>
#include <objc/Object.h>
#include <stdio.h>
#include <stdlib.h>
#include <math.h>

@interface Point : Object {
    float x;
    float y;
}

@property float x;
@property float y;

-(id)initWithX: (float)xval AndY: (float)yval;
-(float)calculateDistance: (Point *)other;

@end

@implementation Point

@synthesize x;
@synthesize y;

+(id)alloc {
    id obj = malloc(class_getInstanceSize(self));
    object_setClass(obj, self);
    return obj;
}

-(void)dealloc {
    free(self);
}

-(id)init {
    x = 0.0;
    y = 0.0;
    return self;
}

-(id)initWithX: (float)xval AndY: (float)yval {
    x = xval;
    y = yval;
    return self;
}

-(float)calculateDistance: (Point *)other {
    return sqrt((x - other.x) * (x - other.x) + (y - other.y) * (y - other.y));
}

@end

int main(int argc, char **argv) {
    Point *point1 = [[Point alloc] init];
    Point *point2 = [[Point alloc] initWithX: 1.5 AndY: 1.5];

    printf("Distance is %f.\n", [point1 calculateDistance: point2]);

    [point1 dealloc];
    [point2 dealloc];
    return EXIT_SUCCESS;
}

It's an interesting idea to write Windows code in Objective-C. If you were using NeXTStep frameworks (or GNUStep) it would make a little more sense. I don't understand your desire to avoid frameworks,  which tend to be force multipliers saving you time and effort. (And of course, you are using OpenGL and GLFW, so this seems like an arbitrary choice.) Regardless, given those constraints, here are my thoughts.
Language
In my opinion Objective-C is the wrong choice for this particular project. I say that as someone who makes a living programming largely in Objective-C. The problem, as I see it, is that you lose 2 important things that you would get from other languages:

The ability to use objects or structs for doing your work and passing them directly to OpenGL/GLFW.
The ability to use operator overloading.

OpenGL expects to receive geometry as an array of vertex attributes (or several different arrays – one for each attribute). That's going to be impossible using the class as you've written it. Each Objective-C object is its own entity on the heap. If  you have an array of them, it's really just an array of pointers to the objects, which may or may not be contiguous in memory. You won't be able to just call glVertexAttribPointer() or other similar functions and pass a pointer to the array since the array will just be an array of other pointers. In C you'd have an array of Point structs either on the stack or the heap and could just pass the address of the first element to the above function. Likewise with something like a C++ std::vector<Point> you'd be able to pass the address of the first element (assuming Point had no v-table). In addition to  making it harder to send  the data to the GPU, it also makes processing the data on the CPU slower because you lose cache coherency when the data isn't contiguous.
You'll also want to do math on your Point objects. While you can write methods on your Objective-C class to add, subtract, etc., it's not as natural as using a language that allows you to overload operators. In particular, C++ and Swift both allow this and it makes writing graphics code feel much more natural.
Class
This class is rather thin. It doesn't do very much that's useful. What it does do, it appears to do correctly. It might be useful to swap the sqrt() call with a call to hypot(x,y) instead.
At a minimum, if you're doing 2D graphics, you'll probably want to add the following methods:
- (void)add:(Point*)p;
- (void)subtract:(Point*)p;
- (float)dotProduct:(Point*)p;
- (void)normalize;
- (void)multiplyScalar:(float)s;
- (void)multiplyVector:(Point*)p;
- (void)divideScalar:(float)s;
- (void)divideVector:(Point*)p;

And eventually, you'll probably want a Matrix class for things like scaling and rotation operations.


Rails controller method to create a customer with credit card info I'm working on a Rails application that has a lot of logic in the controller that should be in the model. In the app/controllers/customers_controller.rb#create there is logic to create stripe cc info for the customer. I think this should be in the model.

How can I move this logic into the model and still handle any exceptions in the controller?

app/controllers/customers_controller.rb

def create
  ...
  card = {
        'object' => 'card', 'number' => card_number, 'exp_month' => card_expire_month, 'exp_year' => card_expire_year,
        'cvc' => card_cvc, 'address_zip' => parms['zipcode'], 'name' => "#{parms['first_name']} #{parms['last_name']}",
        'address_line1' => parms['street_address_1'], 'address_state' => parms['state'],
        'address_line2' => parms['street_address_2'], 'address_city' => parms['city']
      }

      begin
        resp = Stripe::Customer.create(
          description: 'Customer for DoctorNow',
          metadata: { customer_id: @customer.id },
          email: @customer.email,
          source: card)
        @customer.assign_attributes(
          stripe_customer_id: resp['id'],
          stripe_card_id: resp['sources']['data'][0]['id'],
          stripe_card_last4: resp['sources']['data'][0]['last4'],
          stripe_card_type: resp['sources']['data'][0]['brand']
        )

        event('card_preauth', 'customer_id', @customer.id)
      rescue Stripe::CardError => e
        @err = stripe_error(e, 'creating_card_card_error', 0)
        render :err_show, status: :payment_required, json: @err
        event('card_preauth_failed', 'customer_id', @customer.id)
        return
      rescue Stripe::InvalidRequestError => e
        @err = stripe_error(e, 'create_card_invalid', 0)
        event('card_preauth_failed', 'customer_id', @customer.id)
        render :err_show, status: :payment_required, json: @err
        return
      rescue Stripe::AuthenticationError => e
        @err = stripe_error(e, 'create_card_authentication', 0)
        event('card_preauth_failed', 'customer_id', @customer.id)
        render :err_show, status: :payment_required, json: @err
        return
      rescue Stripe::APIConnectionError => e
        @err = stripe_error(e, 'create_card_connect', 0)
        event('card_preauth_failed', 'customer_id', @customer.id)
        render :err_show, status: :payment_required, json: @err
        return
      rescue Stripe::StripeError => e
        @err = stripe_error(e, 'create_card_stripe', 0)
        event('card_preauth_failed', 'customer_id', @customer.id)
        render :err_show, status: :payment_required, json: @err
        return
      rescue => e
        @err = stripe_error(e, 'create_card_generic', 0)
        render :err_show, status: :payment_required, json: @err
        event('card_preauth_failed', 'customer_id', @customer.id)
        return
      end
  ...
end

Personally, I wouldn't want to see this in a model either (at least not an ActiveRecord model).

I'd probably go with something more like this:

app/controllers/customers_controller.rb

def create
  create_customer = CreateCustomer.new(params)
  create_customer.perform
  if create_customer.success?
    redirect_to some_path, notice: 'success'
  else
    render :err_show, status: :payment_required, json: create_customer.err
  end
end

app/interactors/create_customer.rb

class CreateCustomer

  attr_reader :customer, :card, :err

  def initialize(customer, parms)
    @customer = customer
    @card = {
    'object' => 'card', 'number' => card_number, 'exp_month' => card_expire_month, 'exp_year' => card_expire_year,
    'cvc' => card_cvc, 'address_zip' => parms['zipcode'], 'name' => "#{parms['first_name']} #{parms['last_name']}",
    'address_line1' => parms['street_address_1'], 'address_state' => parms['state'],
    'address_line2' => parms['street_address_2'], 'address_city' => parms['city']
    }
    @err = nil
  end

  def perform
    resp = Stripe::Customer.create(
      description: 'Customer for DoctorNow',
      metadata: { customer_id: @customer.id },
      email: @customer.email,
      source: card)
    @customer.assign_attributes(
      stripe_customer_id: resp['id'],
      stripe_card_id: resp['sources']['data'][0]['id'],
      stripe_card_last4: resp['sources']['data'][0]['last4'],
      stripe_card_type: resp['sources']['data'][0]['brand']
    )
    event('card_preauth', 'customer_id', @customer.id)
  rescue Stripe::CardError => e
    log_error('creating_card_card_error')
  rescue Stripe::InvalidRequestError => e
    log_error('create_card_invalid')
  rescue Stripe::AuthenticationError => e
    log_error('create_card_authentication')
  rescue Stripe::APIConnectionError => e
    log_error('create_card_connect')
  rescue Stripe::StripeError => e
    log_error('create_card_stripe')
  rescue => e
    log_error('create_card_generic')
  end

  def success?
    @err.nil? && @customer.stripe_customer_id.present?
  end

  private

  def log_error(error)
    @err = stripe_error(e, error, 0)
    event('card_preauth_failed', 'customer_id', @customer.id)
  end

end


Gathering IDs to find values from objects My HTML has three divs, each id tagged with the logic behind my object (ma/shadow/traps) which contain 10 div each (every icons), also ID-tagged according to the object. On click, I'm gathering the ID and the parent ID to traverse the object and find the value.

It's only possible to raise the value if the pre-requirement is not 0, or if the pre-requirement is false (no pre-req). My problem comes when there are 2 pre-requirements which need to be checked. I check the first one, and if the first meets the pre-req, I check if a second one exists, and check it.

There has to be a better way of checking on variables than this nightmare of ifs. It works, but I don't think it's good code at ALL. I'm not looking for a full copy/paste code here, but more for a more efficient logic on how to do what I'm already doing.

$(function () {
var basemin = 0;
var basemax = 20;
var skill = {
    ma:{
    //sample object
    },
    shadow:{
        clawMastery:{
            base:0,
            preReq:false
        }, psychicHammer:{
            base:0,
            preReq:false
        }, burstOfSpeed:{
            base:0,
            preReq:'clawMastery'
        }, weaponBlock:{
            base:0,
            preReq:'clawMastery'
        }, cloakOfShadows:{
            base:0,
            preReq:'psychicHammer'
        }, fade:{
            base:0,
            preReq:'burstOfSpeed'
        }, shadowWarrior:{
            base:0,
            preReq:'cloakOfShadows',
            preReq2:'weaponBlock'
        }, mindBlast:{
            base:0,
            preReq:'cloakOfShadows'
        }, venom:{
            base:0,
            preReq:'fade'
        }, shadowMaster:{
            base:0,
            preReq:'shadowWarrior'
        }
    },
    traps:{
    //sample object
    }
};
$('#tree div').bind("contextmenu", function (e) {
    e.preventDefault();
});
$('#tree .tab div').mousedown(function (e) {
    var $this = $(this);
    $this.attr('unselectable', 'on').css('UserSelect', 'none').css('MozUserSelect', 'none');
    var $tab = $this.parent().attr("id");
    var $skill = $this.attr("id");
    function checkPreReq(e) {
        var $preReq = skill[$tab][e]['preReq'];
        var $preReq2 = skill[$tab][e]['preReq2'];
        alert($preReq);
        if ($preReq === false) {
            alert('no pre-req');
            return true;
        } else if ($preReq !== false) {
            alert('pre-req1 exists');
            if(skill[$tab][$preReq]['base'] > 0) {
                alert('pre-req1 higher than 0');
                if ($preReq2 === undefined) {
                    alert('no second pre-req, go on');
                    return true;
                } else {
                    alert('check second pre-req value');
                    if (skill[$tab][$preReq2]['base'] > 0) {
                        alert('pre-req2 higher than 0');
                        return true;
                    } else {
                        alert('pre-req2 not met (0)');
                        return false;
                    }
                }
            } else {
                alert('pre-req1 not met (0)');
                return false;
            }
        } else {
            alert('else');
            return false
        }
    }
    if (e.which == 1) {
        //leftclick
        if (skill[$tab][$skill]['base'] < basemax && checkPreReq($skill)) {
            skill[$tab][$skill]['base'] += 1;
            //$rem_skills -= 1;
        }
    } else if (e.which == 3) {
        //rightclick
        if (skill[$tab][$skill]['base'] > basemin) {
            skill[$tab][$skill]['base'] -= 1;
            //$rem_skills += 1;
        }
    }
    $this.find(".lvl").text(skill[$tab][$skill]['base']);
    //$("#output").find(".rem_skills").text($rem_skills);
});
});

If there are zero, one, two or possible more prerequirements, generalize their storage. Do not use multiple properties and nested if-else-statements, but an array and a loop.

The function should then look like this:

function checkPreReq(e) {
    var preReqs = skill[tab][e].preReqs;
    if (!preReqs)
        return true;
    for (var i=0; i<preReqs.length; i++) {
        var preReq = preReqs[i];
        if (skill[tab][preReq].base <= 0)
            // failed at least this preReq
            return false;
    }
    // passed all requirements
    return true;
}


C++11 factory pattern with lambdas Anonymous functions are one of the best features of C++11. They make everything so beautiful!

However, one can get carried away and start overusing them.

This code calls a function that reads through a file and invokes a callback everytime that something matches a regex. Since I read two different files but I want callbacks only to differ for a constant, I chose this "lambda-returning-lambda" factory pattern.

auto callback_factory =
    [&] (IPMarkerType start, IPMarkerType stop) -> std::function<void(IPNode<T>&)>
{
    return [&,start,stop](IPNode<T> & node){
        markers.push_back(IPMarker<T>(node.ip, start));
        markers.push_back(IPMarker<T>(node.ip.network_ones(node.prefix), stop));
    };
};

read_regexp<T>("C:\\path\\to\\file1.txt", regex, callback_factory(ipm_a_open,ipm_a_close));
read_regexp<T>("C:\\path\\to\\file2.txt", regex, callback_factory(ipm_b_open,ipm_b_close));

Callbacks are really local just to the calling function, so I think that external functor would not be justified. On the other hand, this code smells a lot to me, so I would like to hear your comments.

First of all, I will assume there is something like template <typename T> somewhere in your code. Otherwise, I doubt it would compile.

There is something you could do to improve your program: use emplace_back instead of push_back with an object construction. That way, objects will be created right in the collection (list, vector, etc...). By the way, I'll also assume that markers is a standard collection.

auto callback_factory =
    [&] (IPMarkerType start, IPMarkerType stop) -> std::function<void(IPNode<T>&)>
{
    return [&,start,stop](IPNode<T> & node){
        markers.emplace_back(node.ip, start);
        markers.emplace_back(node.ip.network_ones(node.prefix), stop);
    };
};

And, yeah, something still smells: I really wonder how and/or where you declared markers. If it's a global variable, then I think the template parameter may cause some problems.


City and District class examples for teaching OOP in PHP I wrote these two classes for teaching OOP in PHP. Do you think it's breaking some good practices or SOLID principles?

<?php

class City
{
    private $id;
    private $name;
    private $districts = [];

    public function __construct($id, string $name)
    {
        $this->id = $id;
        $this->name = $name;
    }

    public function id()
    {
        return $this->id;
    }

    public function name():string
    {
        return $this->name;
    }

    public function addDistrict(District $district)
    {
        if (!isset($this->districts[$district->id()])) {
            $this->districts[$district->id()] = new District($district->id(), $this, $district->name());
        }
    }

    public function getArray():array
    {
        $districts = [];
        foreach ($this->districts as $district) {
            array_push($districts, $district->getArray());
        }
        return ['id' => $this->id(), 'name' => $this->name(), 'districts' => $districts];
    }

    public function districts():array
    {
        return $this->districts;
    }
}

class District
{
    private $id;
    private $cityId;
    private $name;

    public function __construct($id, City $city, string $name)
    {
        $this->id = $id;
        $this->name = $name;
        $this->cityId = $city->id();
    }

    public function getArray():array
    {
        return ['id' => $this->id, 'name' => $this->name];
    }

    public function id()
    {
        return $this->id;
    }

    public function name():string
    {
        return $this->name;
    }
}

Here are some questions I'm asking myself related to your chosen design:

Why not return type on the id() functions – You have return types on most of the other functions, but why not on either of the id functions?
Why the getArray()? – This seems like something used by functions like var_dump(), but then it would be better to implement __debug_info, which var_dump() could take usage of directly.
Possibly rename the private variables – I'm not sure if php has naming standards, but in my view having both a private variable name and a function name() seems kind of redundant, and possibly unfortunate. I would possibly rename the variable into _name or similar.

Unfortunate connection between District and City

In a City you might add Districts, and each district has links to the id of the cities, making a complete circle. This could be unfortunate, and possibly dangerous. Still you don't have a circular reference as you store only the city id, and not a complete reference to the City object.

But I fail to see the good usage of these classes, as they seem to miss any useful methods. Like you can't look up a district and find the corresponding city. Neither can you do anything besides creating or deleting the cities and districts. A good object design would make the object have all needed variables and methods for handling related functions.

Lastly regarding the addDistrict() function, I think I would find it natural to allow the City class to fully create districts. That is to have function addDistrict($id, $name) within City class, and then let it create the District object.