Slot Machine in Python 3 I have made this program which simulates a slot machine.

import random, time, os

def input_verify(message):
    while True:
        user_input = input(message).lower()
        if user_input in ("yes", "no"):
            return user_input
        print("Invalid Input")

def win(number_of_matches, fruit):
    global balance
    print("Well done! You got", number_of_matches, "of the same fruit!")
    time.sleep(0.5)
    reward = fruits[fruit][1] * number_of_matches
    print("You won £{}!".format(reward))
    balance += reward

fruits = {"apple": [10000, 1], "banana": [7000, 1.5], "lemon": [6000, 1.75], "orange": [5000, 2], "kiwi": [4000, 4],
          "peach": [3000, 6], "avocado": [2000, 10], "grapes": [1000, 20], "mango": [500, 100], "melon": [1, 10000]}
weighted_fruits = [fruit for fruit in fruits for i in range(fruits[fruit][0])]

print("Welcome to my slot machine! Each turn costs £1.")
balance = 10
if os.path.exists("One Armed Bandit.txt"):
    with open("One Armed Bandit.txt", "r") as f:
        contents = f.readlines()
    continue_saved_game = input_verify("Do you want to continue your saved game?")
    if continue_saved_game == "yes":
        balance = float(contents[1].strip())

time.sleep(0.5)
print("\nYour current balance is £{}.\n".format(balance))
time.sleep(1)
while True:
    balance -= 1
    reward = 0
    a, b, c = [random.choice(weighted_fruits) for i in range(3)]
    print("Spinning...")
    for fruit in (a, b, c):
        time.sleep(0.5)
        print(fruit)
    time.sleep(0.5)
    if a == b == c:
        win(3, a)
    elif a == b or a == c or b == c:
        if b == c:
            win(2, c)
        else:
            win(2, a)
    else:
        print("Unlucky. None of your fruits matched each other.")
    time.sleep(0.5)
    print("\nYour current balance is £{}.\n".format(balance))
    time.sleep(0.5)
    replay = input_verify("Do you want to play again?")
    if replay == "no":
        save = input_verify("Do you want to save your game?")
        if save == "yes":
            name = input("What is you name?")
            with open("One Armed Bandit.txt", "w") as f:
                f.write("{}'s Balance:\n{}".format(name, balance))
        break

I don't like how there are so many time.sleep() calls scattered throughout my code. Is there a way to achieve the same thing but in a cleaner way. Moreover, could you suggest some ways to make it more efficient?

Thanks for sharing your code,

You should write code to adhere to the PEP8 standard, for the most part you've done that correctly, you have snake_case variable and function names, but you've put all your imports on one line.

import random, time, os

should become

import random
import time
import os

Give one line per import.

Your code lacks a main guard. If I was to import any functions or classes from your current code, your slot machine game would start! Generally you just want to wrap your main code in a conditional check that looks like

if __name__ == "__main__":
    main()

where main is a function that starts the execution of your program.

A pattern you use in your code is relying on a string value ("yes" or "no") to determine which function executes. This is standard enough and nothing really wrong with it, but I would recommend instead of checking for this string literals, to use boolean values instead which are taken from shorter helper functions. A quick example.

You offer the ability to save and load game state, this is a good thing. However, you save the state in a Human readable fashion - i.e. "Bob's Balance: 24". Unless you're relying on this format somewhere else or even just displaying it, you could just save a single number in a file. No need to record a name.

The main code does a lot, you could break this up into helper functions. Some possible function names that jump out are.

load_game, save_game, wants_to_save, wants_to_replay

A pattern you use in your code is relying on a string value ("yes" or "no") to determine which function executes. This is standard enough and nothing really wrong with it, but I would recommend instead of checking for this string literals, to use boolean values instead which are taken from shorter helper functions. A quick example.

Your current code for prompting for saving the game

    replay = input_verify("Do you want to play again?")
    if replay == "no":
        save = input_verify("Do you want to save your game?")
        if save == "yes":
            name = input("What is you name?")
            with open("One Armed Bandit.txt", "w") as f:
                f.write("{}'s Balance:\n{}".format(name, balance))
        break

I would create some helper functions that look like

def wants_to_replay():
    return input_verify("Do you want to play again?") == "yes"

def wants_to_save():
    return input_verify("Do you want to save your game?") == "yes"

def save():
    name = input("What is you name?")
    with open("One Armed Bandit.txt", "w") as f:
        f.write("{}'s Balance:\n{}".format(name, balance))

And then the code becomes

 if not wants_to_replay():
    if wants_to_save():
        name = input("What is you name?")
        with open("One Armed Bandit.txt", "w") as f:
           f.write("{}'s Balance:\n{}".format(name, balance))
    break

The presence of a global variable is a bit suspect. This usually indicates that maybe you should be using a class instead. Perhaps a SlotMachine object.

It might be nice to be able to instantiate a slot machine object and call a start method on it.

slot_machine = SlotMachine(saved_balance)
slot_machine.start()

this line,

a, b, c = [random.choice(weighted_fruits) for i in range(3)]

when there is an unused variable, you should indicate as such by naming it _
so this line becomes

a, b, c = [random.choice(weighted_fruits) for _ in range(3)]

But then a, b and c are not great variable names. They do have a small scope and are used immediately after, but there's no reason to not give them a better name.

Why not just call them all_fruit then in the next loop you can just use

for fruit in all_fruit:
    ...

In the next section you compare them to each other, you could simply then unpack these into variables like

fruit_a, fruit_b, fruit_c = all_fruit

I think single letter variable names (excluding _ and loop counters) should be avoided at all costs.

You currently have all the fruits values hard coded in, it might be worth while to extract that data into a file to make it easier to add/remove new ones. That's probably not a huge deal though and may have no real benefit.

I'm not sure if there's any cleaner way to achieve the sleeps without scattering the calls throughout the code. You could maybe use named constants for the duration, but there may not be much value in that either.

You have no docstrings in your code. The print statements act as a form of documentation however, and your variable names are (mostly) very good. But there may be some room for some useful comments.

Hopefully this review was useful for you!


Write a class with some properties and indexed Is there a better way to write it? I need the best way to write it. It is this principle? Please show me a better way

it breaks SRP?

 public class PlacementLocation
{
    public int Row { get; set; }
    public int Column { get; set; }
    public int Location { get; set; }
}

public class PlacementLocations : PlacementLocation
{
    private void SetLocationInList()
    {
        _placementLocations = new List<PlacementLocation>
            {
                new PlacementLocation {Column = 1, Row = 9, Location = 1},
                new PlacementLocation {Column = 2, Row = 9, Location = 2},
                new PlacementLocation {Column = 3, Row = 9, Location = 3},
                new PlacementLocation {Column = 4, Row = 9, Location = 4},
                new PlacementLocation {Column = 5, Row = 9, Location = 5},
            };
    }

    private List<PlacementLocation> _placementLocations;

    public PlacementLocations()
    {
        _placementLocations = new List<PlacementLocation>();
        SetLocationInList();
    }

    public Tuple<int, int> this[int location]
    {
        get { return new Tuple<int, int>(_placementLocations[location].Row, _placementLocations[location].Column); }
    }

    public int this[int row, int column]
    {
        get
        {
            PlacementLocation singleOrDefault =
                _placementLocations.SingleOrDefault(d => d.Column == column && d.Row == row);
            return singleOrDefault != null ? singleOrDefault.Location : 0;
        }
    }
}

Update :

  public class PlacementLocations
{

    #region Variables

    private readonly List<PlacementLocation> _placementLocations;

    #endregion

    #region Constructor

    public PlacementLocations(List<PlacementLocation> placementLocations)
    {
        _placementLocations = placementLocations;
    }

    #endregion

    #region Public Methods

    public Tuple<int, int> FindRowColumn(int location)
    {
        return GetRowColumn(location);
    }

    public int FindLocation(int row, int column)
    {
        return GetLocation(row, column);
    }

    #endregion

    #region Indexes

    public Tuple<int, int> this[int location]
    {
        get { return GetRowColumn(location); }
    }

    public int this[int row, int column]
    {
        get { return GetLocation(row, column); }
    }

    #endregion

    #region Private Methods

    private Tuple<int, int> GetRowColumn(int location)
    {
        return new Tuple<int, int>(_placementLocations[location].Row, _placementLocations[location].Column);
    }

    private int GetLocation(int row, int column)
    {
        PlacementLocation singleOrDefault =
            _placementLocations.SingleOrDefault(d => d.Column == column && d.Row == row);
        return singleOrDefault != null ? singleOrDefault.Location : 0;
    }

    #endregion

}

public class LocationsFactory
{
    public static PlacementLocations GetPredefinedLocations()
    {
        var placementLocationList = new List<PlacementLocation>
            {
                new PlacementLocation {Column = 1, Row = 9, Location = 1},
                new PlacementLocation {Column = 2, Row = 9, Location = 2},
                new PlacementLocation {Column = 3, Row = 9, Location = 3},
                new PlacementLocation {Column = 4, Row = 9, Location = 4},
                new PlacementLocation {Column = 5, Row = 9, Location = 5},
            };

        return new PlacementLocations(placementLocationList);
    }
}

The inheritance doesn't make sense. 'PlacementLocations' is the object representing a collection of 'PlacementLocation' objects.

public class PlacementLocations
{
    private List<PlacementLocation> _placementLocations;

    public PlacementLocations(List<PlacementLocation> locations)
    {
        _placementLocations = locations;
    }

    public Tuple<int, int> this[int location]
    {
        get { return new Tuple<int, int>(_placementLocations[location].Row, _placementLocations[location].Column); }
    }

    public int this[int row, int column]
    {
        get
        {
            PlacementLocation singleOrDefault =
                _placementLocations.SingleOrDefault(d => d.Column == column && d.Row == row);
            return singleOrDefault != null ? singleOrDefault.Location : 0;
        }
    }
}

Class should not contain particular initialization values: use constructor parameters instead and initialize externally:

...

var placementLocationList = new List<PlacementLocation>
{
    new PlacementLocation { Column = 1, Row = 9, Location = 1 },
    new PlacementLocation { Column = 2, Row = 9, Location = 2 },
    new PlacementLocation { Column = 3, Row = 9, Location = 3 },
    new PlacementLocation { Column = 4, Row = 9, Location = 4 },
    new PlacementLocation { Column = 5, Row = 9, Location = 5 },
};

var placementLocations = new PlacementLocations(placementLocationList);

...

Update:
if you need predefined locations list you can use some sort of factory...

public class LocationsFactory
{
    public static PlacementLocations GetPredefinedLocations()
    {
        var placementLocationList = new List<PlacementLocation>
        {
            new PlacementLocation { Column = 1, Row = 9, Location = 1 },
            new PlacementLocation { Column = 2, Row = 9, Location = 2 },
            new PlacementLocation { Column = 3, Row = 9, Location = 3 },
            new PlacementLocation { Column = 4, Row = 9, Location = 4 },
            new PlacementLocation { Column = 5, Row = 9, Location = 5 },
        };

        return new PlacementLocations(placementLocationList);
    }
}


Least significant digit (LSD) radix sort of strings I am learning string algorithms and wrote this C example based on a Java example I found online.  Any feedback relating to style, code clarity, whether it is generic enough to be re-used, interface, etc, etc would be very much appreciated.

/*
 Demonstration of least significant digit (LSD) radix sort of strings
 Specifically UK vehicle registration numbers
*/
#include <stdio.h>
#include <string.h>
#include <stdlib.h>

#define PLATELENGTH 7
#define ALPHABET 36

/* Characters must be uppercase or digits */
void lsd_string_sort(char* a[], const int length, int string_size) {
   int R = ALPHABET;
   int W = string_size;
   const int N = length;
   char id = 0;
   char** aux = (char**)malloc(length * string_size);

   for(int d = W-1; d >= 0; d--) {
      int* count = (int*)calloc(R+1, sizeof(int));
      /* whether character or digit */
      id = (*(a[0]+d) >= '0' && *(a[0]+d) <= '9') ? '0' : 'A';
      /* Compute frequency counts */
      for(int i = 0; i < N; i++)
         count[*(a[i]+d) - id + 1]++;
      /* transform counts to indices */
      for(int r = 0; r < R; r++)
         count[r+1] += count[r];
      /* distribute to temp array */
      for(int i = 0; i < N; i++)
          aux[count[(*(a[i]+d) - id)]++] = a[i];
      /* copy back to original array */
      for(int i = 0; i < N; i++)
         a[i] = aux[i];

      free(count);
   }
   free(aux);
}

int main() {

   /* in real application would use re-sizing array */
   char* plates[1000] = {0};

   int arr[10] = {0};
   char plate[16];  /* extra space just in case */
   int n = 0;
   printf("Please enter list of UK registration plates (skip spaces between registration numbers)\n");
   while(scanf("%s", &plate) == 1) {
      plates[n++] = strdup(plate);
   }

   printf("List before sorting\n");
   for(int i = 0; i < n; i++)
      printf("%s\n", plates[i]);

   lsd_string_sort(&plates[0], n, PLATELENGTH);

   printf("List after sorting\n");
   for(int i = 0; i < n; i++)
      printf("%s\n", plates[i]);

   while(n > 0)
      free(plates[--n]);

   return 0;
}

This macro name is misleading:

#define ALPHABET 36

The alphabet consists of 26 letters, not 36.  But in a CS context, this can be implied (in this case, it appears to include A-Z and 0-9).

However, this macro is storing the alphabet's size, not the alphabet itself.  The name should clearly specify that.  A more appropriate name could be ALPHABET_LENGTH.

On another note, the other macro should be renamed to PLATE_LENGTH.  Underscores are commonly used to separate each word in a macro.
Avoid single-character variable names:

int R = ALPHABET;
int W = string_size;

Based on the context (fortunately), I can tell that these hold sizes.  Other than that, they are very unclear and could lead to maintenance problems at some point.  Always give variables descriptive names, unless they're simple loop counters.
You have the list-displaying code in main() twice.  Why not make it a function?  This will reduce the amount of code in main() and maintain DRY, making it clearer to read and to maintain.

void display(char list[], int n)
{
    for (int i = 0; i < n; i++)
        printf("%s\n", plates[i]);
}

You could keep the label outputs in main(), right before the function calls.


Add dash between more than two consecutive numbers and comma between other numbers in array I have an ordered array of integer values: 10, 11, 12, 13, 15, 16, 19
and I need to write a dash between first and last consecutive values when there are more than two consecutive values (10-13). Between other values I need to write a comma and whitespace.
Output for this input must be 10-13, 15, 16, 19.
I wrote a method that implements this:

class Program
{
    private static string AddDashesBetweenConsecutiveNumbers(int[] orderedArray)
    {
        if (orderedArray.Length == 0 || orderedArray == null)
        {
            return string.Empty;
        }
        var groupedArray = orderedArray.Select((x, i) => new { Difference = i - x, Value = x })
                .GroupBy(x => x.Difference)
                .Select(group => group.Select(a => a.Value))
                .Select(arr => arr.Count() > 2
                            ? $"{arr.First()}-{arr.Last()}"
                            : string.Join(", ", arr));
        return string.Join(", ", groupedArray);
    }
    static void Main(string[] args)
    {
        int[] array = { 10, 11, 12, 13, 15, 16, 19 };

        Console.WriteLine(AddDashesBetweenConsecutiveNumbers(array)); // Output: "10-13, 15, 16, 19"
    }
}

What I wrote seems to work, but this code has some disadvantages, especially in  readability of the LINQ expression. So if you have any ideas how to improve this method or have better or alternative solution, that would be great to find out.

Sorry but no comments (yet) on the LINQ.

Your first conditional has a logic error.  You should be checking if orderedArray is null before you check if its length is 0.

The method has a leap of faith that someone is inputting an array sorted in ascending order.  There is nothing in your code to prevent otherwise.

What if someone wanted to pass in a List or IEnumerable?  Restricting input to an array is fairly limiting.

The variable groupedArray is not an array, so the name is misleading.  A quick reworking of all of the above is:

private static string AddDashesBetweenConsecutiveNumbers(IEnumerable<int> values)
{
    // Avoid Count() since it requires full list enumeration.
    // FirstOrDefault() would return default(int) or 0, which is not null.
    // Any() will find the first one without a full list enumeration.
    if (values == null || !values.Any())
    {
        return string.Empty;
    }

    // If the input values is already sorted, this will be fast.
    // If the input values is not already sorted, good thing we do it here.
    var orderedValues = values.OrderBy(x => x);

    var groupedValues = orderedValues.Select((x, i) => new { Difference = i - x, Value = x })
            .GroupBy(x => x.Difference)
            .Select(group => group.Select(a => a.Value))
            .Select(arr => arr.Count() > 2
                        ? $"{arr.First()}-{arr.Last()}"
                        : string.Join(", ", arr));

    return string.Join(", ", groupedValues);
}

I'm sure you are capable of writing a non-LINQ version if you desired.  I include a version below.

private static string AddDashesBetweenConsecutiveNumbers2(IEnumerable<int> values)
{
// Avoid Count() since it requires full list enumeration.
// FirstOrDefault() would return default(int) or 0, which is not null.
if (values == null || !values.Any())
{
    return string.Empty;
}

// If the input values is already sorted, this will be fast.
// If the input values is not already sorted, good thing we do it here.
var orderedValues = values.OrderBy(x => x);

return string.Join(", ", GetGroupedIntegers(orderedValues));
}

private static IEnumerable<string> GetGroupedIntegers(IOrderedEnumerable<int> orderedValues)
{
var sublist = new List<int>();

Func<string> getSubListOutput = () =>
{
    switch (sublist.Count)
    {
        case 1:
            return sublist.First().ToString();
        case 2:
            return sublist.First().ToString() + ", " + sublist.Last().ToString();
        default:
            return sublist.First().ToString() + "-" + sublist.Last().ToString();
    }
};

foreach (var value in orderedValues)
{
    if (sublist.Count == 0 || sublist.Last() == value - 1)
    {
        sublist.Add(value);
    }
    else
    {
        yield return getSubListOutput();
        sublist = new List<int>() { value };
    }
}

yield return getSubListOutput();
}

Got to run to a meeting, but another flaw in both your original and my answer is that there is no check for distinct integers, but that can easily be done.

UPDATE:
Corrected code as per comments.


Find number of whole weeks between months in the same year I wrote a class to return the number of whole weeks between months in the same year. I need suggestions on my code.

public int year;
public String m1;
public String m2;

public Solution(int Y, String A, String B) throws ParseException {
    this.year = Y;
    this.m1 = A;
    this.m2 = B;

    String dateOneInput = year + "-" + m1;
    String dateTwoInput = year + "-" + m2;

    SimpleDateFormat formatter = new SimpleDateFormat("yyyy-MMM");

    //DATE 1
    Calendar c1 = Calendar.getInstance();
    c1.setTime(formatter.parse(dateOneInput));
    //Get first monday of the month
    c1.set(Calendar.DAY_OF_WEEK, Calendar.MONDAY);
    c1.set(Calendar.DAY_OF_WEEK_IN_MONTH, 1);

    System.out.println("Date 1 : " + c1.getTime());

    //DATE 2
    Calendar c2 = Calendar.getInstance();
    c2.setTime(formatter.parse(dateTwoInput));
    //Get last sunday of the month
    c2.set(Calendar.DAY_OF_WEEK, Calendar.SUNDAY);
    c2.set(Calendar.DAY_OF_WEEK_IN_MONTH, -1);

    System.out.println("Date 2 : " + c2.getTime());

    //Difference in weeks
    int weeks = c2.get(Calendar.WEEK_OF_YEAR) - c1.get(Calendar.WEEK_OF_YEAR);
    System.out.println("Number of weeks :" + (1 + weeks));

}

/**
 * @param args the command line arguments
 * @throws java.text.ParseException
 */
public static void main(String[] args) throws ParseException {
    Solution s = new Solution(2012, "June", "July");
}

public int year;
public String m1;
public String m2;

public Solution(int Y, String A, String B) throws ParseException {
    this.year = Y;
    this.m1 = A;
    this.m2 = B;

These fields shouldn't be public. By making them public you've exposed them to the outside world. These are internal details that should be properly encapsulated. Truthfully though, you don't need them at all. I would just work directly with the arguments that you passed in. Rename them though. Your current names don't tell the client what kind of values to expect. What's an "A"? How would another dev know what to pass into this method without looking at the internals? They wouldn't.

public Solution(int year, String startMonth, String endMonth) throws ParseException


Input processing/state machine function I'm making a Mario clone in pygame and I have just wrote a function proccesses inputs and acts as a state machine for Mario. Everything is working just as I want it, but I was wondering if you can think of any more efficient way to write this code.

A note that these methods are all a part of a class called mario.

def __init__(self):
    #The inputs currently binded to each of marios actions.
    self.keybinds = {
        'right': K_RIGHT,
        'left' : K_LEFT,
        'jump' : K_z,
        'spd'  : K_LSHIFT
    }

    # The current state mario is in.
    # Valid states: 'stand', 'air', 'death'.
    self.state = 'stand'

    # The current action mario is doing.
    # Valid actions: 'none', 'walk', 'run', 'jump'.
    self.action = 'none'

    # The current direction mario is facing.
    # Valid values: 'r', 'l'.
    self.faceDir = 'r'

    # The direction that the user wants mario to be moving in
    self.des_dir = 'n'

    # The current speed mario is moving at.
    # Valid speeds: 'norm', 'fast'.
    self.spd = 'norm'

    # A boolean based on whether a character instance can jump.
    self.canJump = True

    self.crnt_mdx = 2.7 # The current max x velocity mario can move at.

    self.slow_dx = 2.7 # The max x velocity mario can move at when he is walking.
    self.fast_dx = 4.7 # The max x velocity mario can move at when he is running.

def processInput(self, keys, keyup):
    #keys = a list of all keybord keys currently being pressed.
    #I have imported a module that has a bunch of variables that represent each key.
    #keyup = the current key that was just released, if any.

    if keyup == self.keybinds['jump']:
        self.canJump = True

    if self.state == 'stand' or self.state == 'air':
        if keys[self.keybinds['right']]:
            self.moveHorz('r')
            self.accelerate('r')
            self.set_crnt_acc('r')
            self.faceDir = 'r'
            self.des_dir = 'r'
        elif keys[self.keybinds['left']]:
            self.moveHorz('l')
            self.accelerate('l')
            self.set_crnt_acc('l')
            self.faceDir = 'l'
            self.des_dir = 'l'
        else:
            if self.state == 'stand' and self.horzDir != 'n':
                self.slowDown()
            self.des_dir = 'n'

    if self.state == 'stand':
        if keys[self.keybinds['spd']]:
            self.crnt_mdx = self.fast_dx
        else:
            self.crnt_mdx = self.slow_dx

        if keys[K_z] and self.canJump:
            self.canJump = False
            self.action = 'jump'
    else:
        pass

Style

The style for naming in Python is as follows.

Variables should be in snake_case.
Constant variables should be in UPPER_SNAKE_CASE.
Functions should also be in snake_case.
Classes should be in PascalCase.

You should also replace comments in functions like this:

#keys = a list of all keybord keys currently being pressed.
#I have imported a module that has a bunch of variables that represent each key.
#keyup = the current key that was just released, if any.

To docstrings, like this:

"""Brief description of the function/class.

More detailed description of function/class.

Keyword Arguments:
argument -- Argument description.
...
"""

Design

There's not much about your design that I can nitpick, the only thing I'd really suggest is shortening these two blocks of code:

self.moveHorz('r')
self.accelerate('r')
self.set_crnt_acc('r')
self.faceDir = 'r'
self.des_dir = 'r'

And:

self.moveHorz('l')
self.accelerate('l')
self.set_crnt_acc('l')
self.faceDir = 'l'
self.des_dir = 'l'

To a shortened method, like this:

def change_direction(self, direction):
    if direction == "l" or direction == "r":
        self.moveHorz(direction)
        self.accelerate(direction)
        self.set_crnt_acc(direction)
        self.faceDir = direction
        self.des_dir = direction
    else:
        print("Direction must either be \"l\" or \"r\"")

This is especially useful if you need to repeat this code in many places.

On a side note, you should get rid of the magic values 'l' and 'r' and do something like this:

LEFT = 'l'
RIGHT = 'r'

You could also use integers to represent this as well:

LEFT = 0
RIGHT = 1

Or if you're feeling really adventurous, you can use an Enum, like this:

class Directions(Enum):
    LEFT = 0
    RIGHT = 1


Concurrent Linked Hash Set Basically I want to create a concurrent LinkedHashSet which returns proper size too.

I am mainly concerned about adding and removing iterations.  Suggestions relating to implementation of set modification during iteration are also welcomed.

import java.util.Collection;
import java.util.LinkedHashSet;

//TODO still not sure
public class ConcurrentLinkedHashSet<E> extends LinkedHashSet<E>
{
    private static final long serialVersionUID = 1L;
    private boolean isUpdating = false;

    @Override
    public int size()
    {
        while (isUpdating)
        {
            try
            {
                wait();
            } catch (InterruptedException e)
            {
                e.printStackTrace();
                return (Integer) null;
            }
        }
        return super.size();
    }

    @Override
    public synchronized boolean addAll(Collection<? extends E> arg0)
    {
        while (isUpdating)
        {
            try
            {
                wait();
            } catch (InterruptedException e)
            {
                e.printStackTrace();
                return false;
            }
        }
        isUpdating = true;
        boolean result = false;
        result = super.addAll(arg0);
        isUpdating = false;
        notify();
        return result;
    }

    @Override
    public synchronized boolean add(E arg0)
    {
        while (isUpdating)
        {
            try
            {
                wait();
            } catch (InterruptedException e)
            {
                e.printStackTrace();
                return false;
            }
        }
        isUpdating = true;
        boolean result = super.add(arg0);
        isUpdating = false;
        notify();
        return result;
    }

    @Override
    public boolean remove(Object arg0)
    {
        while (isUpdating)
        {
            try
            {
                wait();
            } catch (InterruptedException e)
            {
                e.printStackTrace();
                return false;
            }
        }
        isUpdating = true;
        boolean result = super.remove(arg0);
        isUpdating = false;
        notify();
        return result;
    }

    @Override
    public boolean removeAll(Collection<?> arg0)
    {
        while (isUpdating)
        {
            try
            {
                wait();
            } catch (InterruptedException e)
            {
                e.printStackTrace();
                return false;
            }
        }
        isUpdating = true;
        boolean result = super.removeAll(arg0);
        isUpdating = false;
        notify();
        return result;

    }
}

EDIT : Updated the code based on reviews and is available on github

There are several thread safety issues :

if size(), remove() and removeAll() need to wait(), they will throw an IllegalMonitorStateException since they do not hold the lock on this at the time they call wait();
for that same reason, they may see stale values for isUpdating
if size() would be interrupted while waiting (supposing that gets fixed) its return statement would result in a NullPointerException.
while many threads may be waiting, only one will be notified when isUpdating is set to false again, since you call notify() instead of notifyAll()
operations such as contains() and isEmpty() may see stale values since they are not properly synchronized.
when interrupted all overridden methods violate the Set contract.
the interrupted flag is cleared upon interruption since the InterruptedException is caught and the state is not set to interrupted again.
not strictly a problem, but you synchronize on this. This means third party code locking on your Set will influence its behavior. It is preferrable to lock on a private Object.

I'm not sure what you mean exactly by 'proper size'.

Also, I think that if you properly implement this, you'll basically end up with the same behavior as what you'd get from Collections.synchronizedSet(new LinkedHashSet())


Classes to serialize departments and their employees I have many small but similar classes like these:

class DepartmentSerializer < Serializer
  self.root = false
  attributes :id, :name
  has_many :employees, serializer: EmployeeSerializer
  has_one :location, serializer: LocationNameSerializer
end

class DepartmentWithManagersSerializer < Serializer
  self.root = false
  attributes :id, :name
  has_many :primary_managers, serializer: ManagerSerializer
  has_many :secondary_managers, serializer: ManagerSerializer
end

class DepartmentNameSerializer < Serializer
  self.root = false
  attributes :id, :name
end

class DepartmentWithEmployeesSerializer < Serializer
  self.root = false
  attributes :id, :name
  has_many :employees, serializer: EmployeeNameSerializer
end

As you can see they're all just variations on a theme. There is a potential here for a combinatorial explosion of classes if there are enough use cases. Should I do something like ActiveRecord's find methods which allow you to create methods like .find_by_name_and_manager_and_location?

Using inheritance, first you can clean up your structure as following:

class DepartmentNameSerializer < Serializer
  self.root = false
  attributes :id, :name
end

class DepartmentSerializer < DepartmentNameSerializer
  has_many :employees, serializer: EmployeeSerializer
  has_one :location, serializer: LocationNameSerializer
end

class DepartmentWithManagersSerializer < DepartmentNameSerializer
  has_many :primary_managers, serializer: ManagerSerializer
  has_many :secondary_managers, serializer: ManagerSerializer
end

class DepartmentWithEmployeesSerializer < DepartmentNameSerializer
  has_many :employees, serializer: EmployeeNameSerializer
end

There is little you can do to really enhance your code any further in terms of readability and reusability if you want to separate all serializer contexts into separate classes like this.

If you are using ActiveModel::Serializers which I think you do, since 0.8.0 you can use ActiveRecord-Serialization-like :except and :only to add some serialization context, so you could refactor your structure to work like the following:

class DepartmentSerializer < Serializer
  self.root = false
  attributes :id, :name, :employee_names, :location_names
  # it is afaik not possible to pass serialization context to the other serializers
  # via association
  # has_many :employee_names, serializer: EmployeeSerializer, only: :name
  # has_one :location_names, serializer: LocationSerializer, only: :name
  has_many :employees, serializer: EmployeeSerializer
  has_many :primary_managers, serializer: ManagerSerializer
  has_many :secondary_managers, serializer: ManagerSerializer

  def employee_names
    EmployeeSerializer.new(object.employees, only: :name) # + root: false if EmployeeSerializer uses a root
  end

  def location_names
    LocationSerializer.new(object.locations, only: :name) # + root: false if LocationSerializer uses a root
  end
end

And your controllers can call the DepartmentSerializer like this:

class DepartmentController < ApplicationController # or other

  def some_context_with_managers
    render json: @department, except: %i(employees employee_names location_names)
  end

  # ... and so on
end


Bisection to find roots in C++ I have written a short C/C++ code finding root by bisection. (This is a simple iterative numerical method allowing to find the root of an equation i.e. x such that f(x) = 0). Bisection Method

The header simply consists of guards and of the following lines:

#include <functional>
double bisection(double x1,
                 double x2,
                 double e,
                 std::function<double(double)>& f);

In order to get plain C code, the std::function<double(double)> in the signature of the function has to be turned into a double(*f)(double);

I have two questions.

The first is if this code can be improved (minor improvements).
The second is if recursion is a good choice.

namespace {
    double bisection(double x1,
                     double x2,
                     double e,
                     std::function<double(double)>& f,
                     double fx1,
                     double fx2){
        double mid = x1*0.5 + x2*0.5;
        if (x2 - x1 < e)
            return mid;
        double fmid = f(mid);
        if ((fmid>0.0) == (fx1 > 0.0))
            return bisection(mid, x2, e, f, fmid, fx2);
        return bisection(x1, mid, e, f, fx1, fmid);
    }
}

double bisection(double x1,
                 double x2,
                 double e,
                 std::function<double(double)>& f){
    double fx1 = f(x1);
    double fx2 = f(x2);
    return bisection(x1, x2, e, f, fx1, fx2);
}

An example of main using my code is:

#include "bisection.hpp"
int main(){
    bisection(0.0, 1.0, 1.0E-6, [](double x){return x*x-0.5;})

    return 0;
}

please notice that x1 and x2 are expected to be ordered: x1 < x2. Moreover the user should be aware of the fact that there will be a log_2((x_2-x_1)/tolerance) calls stack.

If any of fmid, fx1 or fx2 are zero (or close to zero), you should stop, since you already found an answer. That's also in Wikipedia's example algorithm. Even for the constant function f = [](double){return 0;}, your code will still continue and take a total of \$ \log_2 \left(\frac{x_2-x_1}{\varepsilon}\right)\$ calls.
Your recursion cannot get transformed into a loop by GCC. The recursive algorithm looks nice, but since we end up with \$ \log_2 \left(\frac{x_2-x_1}{\varepsilon}\right)\$ calls. An iterative variant is easy to write, so I'd use that one.
Not only must \$x_1 <x_2 \$, but also \$f(x_1) f(x_2) < 0\$, that is, the signs of \$f(x_1)\$ and \$f(x_2)\$ must differ. Otherwise your algorithm might not find the root. I only mention this because you wrote that \$x_1\$ and \$x_2\$ are expected to be ordered, but that's not enough.

Other than that, I strongly suggest you to use braces even for single statement ifs, as missing braces have been a source of errors too often.

Alternatively a little bit more whitespace in your function can make it a lot easier to read:

namespace {
double bisection(double x1,
                 double x2,
                 double e,
                 std::function<double(double)>& f,
                 double fx1,
                 double fx2){

    const double mid = x1*0.5 + x2*0.5;
    const double fmid = f(mid);

    if (x2 - x1 < e || fmid == 0.0) {
        return mid;
    }

    if ((fmid > 0.0) == (fx1 > 0.0)) {
        return bisection(mid, x2, e, f, fmid, fx2);
    } else {
        return bisection(x1, mid, e, f, fx1, fmid);
    }
}
}

I would use the ternary operator though instead of if/else, but that's only a style question. But then again, I would write this in an iterative way, since we don't need an additional helper.


Generating a sequential number for app-wide use I have to generate a sequential number for groovy-grails app wide use and came up with the following.  However, is there a better way to do this?

Domain classes:

class RoastIdCounter {
    int counter =0
    static constraints = {
    }
}

This one has no views or controller associated with it.  It's purely for db interaction in a service class.

class RoastId {
    Family family
    Integer nextId
    long timeCreated = new Date().time

    static constraints = {
        family()
        nextId()
    }
    static belongsTo = [ Family ]
}

class Family  extends {

    String farmId;
    String welcome;
    String familyName;
    String ourFarm;
    String howMuchDoWeGetPaid;

    SortedSet pictureAlbums;
    SortedSet pictures;

    Integer pictureAlbumsCount = 0;
    Integer picturesCount = 0;

    Date dateCreated = new Date()
    Date lastUpdated = new Date()

    static hasMany = [ roastIds : RoastId, pictureAlbums : PictureAlbum, pictures : Picture ]

    static constraints = {
        farmId()
        familyName( )
        welcome( widget:'textarea' )
        ourFarm( widget:'textarea' )
        howMuchDoWeGetPaid( widget:'textarea' )

        dateCreated()
        lastUpdated()
}

    String toString() { "$farmId - $familyName" }

} // Family

Service class for dealing with the number generation:

class RoastIdCounterService {

    static transactional = true

    def getNextRoastId() {

        def ric = RoastIdCounter.list()[-1]

        if( ric != null ){
            ric.lock()
            ric.counter = ric.counter + 1
            ric.save()
            return ric.counter
        } else {
            return -1
        }
    }
}

Controller:

Here I'm just sending the object all pre-populated:

def create = {
    def roastIdInstance = new RoastId()
    roastIdInstance.properties = params
    roastIdInstance.nextId = roastIdCounterService.getNextRoastId()
    return [roastIdInstance: roastIdInstance]
}

Environment: Grails 1.3.7, PostgreSql 8.4

Would there be a better way to do this? What potential problems am I overlooking?

Have you considered using an AtomicLong instead of the RoastIdCounterService? You can then call AtomicLong.incrementAndGet() to get the next value. The atomic long will take care of the locking required for safe concurrent access.


Rock Paper Scissor with a CPU (console) I tried my hands on Rock-Paper-Scissors. But the twist is that the game has a brain that aims to learns your behaviour.

The brain is a simple counter that updates the player's current choice based on players previous choices. The level determines the number of previous choices cpu will remember. In any event the player tracks its own inputs and makes calculated choices to defeat the CPU, I have added a timed input function i.e. if you don't provide input within 5 secs, you lose.

The game works perfectly, however I am unhappy with the main function. There are too many try-except blocks. My questions are

What are possible improvements ?
How do I improve the print_cpu_data function in the way it shows the information ?
If I wish to make a GUI for the game (e.g tkinter, Pygame, etc), what part of the code should be made into functions and how to do so efficiently ?

As I am not a beginner (somewhat less than intermediate) , I appreciate some advanced ideas and concepts.

"""
Rock-Paper-Scissor with CPU
"""
from itertools import product
import select
import sys

SCORE_TABLE = [[1, 0, 2],
               [2, 1, 0],
               [0, 2, 1]]
SCORE_STR = ['LOSE', 'DRAW', 'WIN']
ROCK = 0
PAPER = 1
SCISSOR = 2
CHOICE_STR = ['ROCK', 'PAPER', 'SCISSOR']
WIN_MOVE = [PAPER, SCISSOR, ROCK]

class GameCpu:
    """Class to create game CPU"""
    def __init__(self, level=1):
        self.level = level
        self.probs = {}
        for t in product([ROCK, PAPER, SCISSOR], repeat=level):
            self.probs[t] = [0, 0, 0]
        self.prev = [0]*level

    def get_cpu_choice(self):
        """Return CPU choice"""
        tmp = self.probs[tuple(self.prev)]
        pred = tmp.index(max(tmp))
        return WIN_MOVE[pred]

    def train_cpu(self, choice_player):
        """Train CPU with current player choice"""
        self.probs[tuple(self.prev)][choice_player] += 1
        self.prev.pop(0)
        self.prev.append(choice_player)

    def print_cpu_data(self):
        """Print CPU data"""
        print('\nPattern', '\t'*self.level, 'Choice count\n')
        for key, val in self.probs.items():
            print(*[f'{CHOICE_STR[k]}' for k in key], sep=', ', end='\t\t')
            print(*[f'{CHOICE_STR[i]}:{v}' for i, v in enumerate(val)])

def input_with_timeout(prompt, timeout):
    """Timed input"""
    sys.stdout.write(prompt)
    sys.stdout.flush()
    ready, _, _ = select.select([sys.stdin], [], [], timeout)
    if ready:
        return sys.stdin.readline().rstrip('\n')
    raise Exception

def main():
    """main function"""
    while True:
        try:
            level = int(input('Enter CPU level (Level 1,2,3): '))
            assert 1 <= level <= 3
            break
        except:
            print('Invalid.')

    print('\nPress Q to quit game\
        \nRemember you have got only 5secs to give input.\
        \nThe choices are ROCK:1  PAPER:2  SCISSOR:3\n')

    cpu = GameCpu(level)
    scores = [0, 0, 0]
    timeout = 5
    while True:
        choice_player = None
        try:
            choice_player = input_with_timeout('Enter choice: ', timeout)
        except:
            print('Times up')

        if not choice_player:
            break
        if choice_player.lower() == 'q':
            break
        try:
            choice_player = int(choice_player)-1
            assert 0 <= choice_player <= 2
        except:
            print('Invalid. ', end='\n')
            continue

        choice_cpu = cpu.get_cpu_choice()
        s = SCORE_TABLE[choice_player][choice_cpu]
        scores[s] += 1
        print(f'{SCORE_STR[s]}')

        cpu.train_cpu(choice_player)

    total = max(1, sum(scores))
    percent = [100*s/total for s in scores]

    print('\nSTATS: \n')
    print(f'Total games played: {total}')
    print(*[f'{SCORE_STR[i]}:{p:.2f}% ' for i, p in enumerate(percent)])
    cpu.print_cpu_data()

if __name__ == '__main__':
    main()

How do I improve the print_cpu_data function in the way it shows the information ?

Well, nowadays the most common architectural pattern (design scheme) for many projects is the MVC its principle is to separate the classes and code constructs in your project into 3 packages (model-view-controller)

In the model goes all the code which constitutes your application logic for example your class GameCpu
In the view goes the code which displays the information that the user is supposed to see in exmaple a class which uses tkinter, Pygame etc.
In the controller goes the code which allows communication between model and view.

You need to be aware that Desktop apps and Web apps have a different approach for the MVC due its nature.

If I wish to make a GUI for the game (e.g tkinter, Pygame, etc), what part of the code should be made into functions and how to do so efficiently ?

As I said, you could use an MVC pattern which is basically 3 packages named model, view, controller and import classes accordingly.

I believe your question is more likely to be answered with modularization and refactoring

Modularization is to split independent code blocs into functions, and refactoring is to simplify the code you have written.

Well, I hope it helped you, maybe you need a couple of youtube videos to understand how MVC works. I know, it seems more work, but when you are debugging medium to semi-large personal projects it is worthy, in great scale projects its indispensable. (not just MVC, to use an architectural pattern with an enterprise architecture [the names makes it sound complex but actually it isn't]).

Note: You may ask why did i not answered your first question, well, the code does not work when I execute it, so I can't make suggestions in the air, plus if I recommend a new form to solve this problem, many mods request answers to be bassed on the poster's code.


Is this the right way to write OOP PDO classes? I have just begun to work with OOP and I am converting my current project into OOP classes. I have a few questions about this User class.

class User {
    private $db;

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

    function set_user($input) {
        check if user exists and if yes set
        $this->user = $userid;
    }

    function info() {
        return user info
    }

    function awards() {
        return user awards
    }
}

I am wondering if this is the correct approach, and how should this be used. When a user is logged in I have a $logged_in = new User; variable set and then I go about setting the user and pulling required variables. When we get on certain pages there are specific sections that have foreach statements that display other users and their awards. Do I create a new instance of class User in each foreach or do I create one instance of the class and use that everywhere needed?

This project is a CMS and on nearly every page it displays multiple areas containing user info. I am wondering what the best way to do this is.

If a User object represents a single User, you would want one User instance per actual user you are working with in the application. For example, say your user might have friends:

class User {
    private $friends;

    public function addFriend(User $friend) {
        if (!in_array($friend, $this->friends)) {
            $this->friends[] = $friend;
        }
    }
}

Now, your User object can hold an array of other User objects, who he's friends with! You can do all sorts of cool stuff with that, like listing mutual friends (since those Users have their $friends array too!), displaying a list of friends on the page, or just figuring out who has more friends.

The User object is commonly known as a Domain object, it's an object which is part of the Domain Logic or Business Logic, i.e. the actual application, the actual purpose of the application, the part that does the actual work.

Review:

Most naming conventions in PHP dictate that $variableNames should be camelCased and not lowercase_underscores, however, stick with that works for you, and don't break existing convention if you're working in a collaborative project.
It's often preferable to separate the part where the User's details are kept and actual computation and processing takes place, and the part which is responsible for fetching and saving the User's details from permanent storage of some sort.

Consider this:

class User {
    private $id;
    private $name;

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

This User object is different, it doesn't know where the details come from, they are injected from that magical place outside of the object where it doesn't know anything about. This User could have come from a database, he could have come from session, a file, a REST api, or I could have just made it up for testing purposes.

So how do you get the details from the database? You use the Data Mapper pattern.

class UserMapper {

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

    /**
      * Fetch user details from the database based on ID.
      * If $user exists, fill it. If not, create a new one.
      */
    public function fetch($id, User $user = null) { ... }
    /**
      * Save $user back into the database
      */
    public function save(User $user) { ... }

    ....
}

You can have more methods like "fetch the N top user with most comments", or "Fetch all the friends of this $user"

The point is, the Mapper is responsible for getting/saving all necessary data from the database or whatever other permanent storage you might need, and fill the actual Domain Object, which is then used by the application to do processing.


How can I better the readability of inline test data? [TestCase(new[] { 1, 2 }, 1, Result = 2)]
[TestCase(new[] { 1, 2 }, 2, Result = 1)]
[TestCase(new[] { 1, 2, 3 }, 2, Result = 2)]
[TestCase(new[] { 1, 2, 3, 4 }, 2, Result = 2)]
[TestCase(new[] { 1, 2, 3, 4 }, 10, Result = 1)]
public int TotalPageCountIsAccurate(int[] testSequence, int pageSize)
{
    var pagedList = new PagedList<int>(testSequence, 0, pageSize);
    return pagedList.TotalPageCount;
}

I wrote this test about five minutes ago and I can barely understand it - I dread to think how hard it will be to read in a few weeks. How can I refactor the TestCase attribute to improve readability?

I thought about using named arguments or (maybe) introducing a some well-named fields, but neither of these ideas appear to be feasible.

If I'm right you could do at least two things:

Generate the array and pass only the number of items to the test method.
Rename testSequence to something more descriptive, like items, elements etc.

[TestCase(2, 1, Result = 2)]
[TestCase(2, 2, Result = 1)]
[TestCase(3, 2, Result = 2)]
[TestCase(4, 2, Result = 2)]
[TestCase(4, 10, Result = 1)]
public int TotalPageCountIsAccurate(int itemCount, int pageSize)
{
    // generates the item array with itemCount items
    var items = Enumerable.Range(0, itemCount).ToArray();

    var pagedList = new PagedList<int>(items, 0, pageSize);
    return pagedList.TotalPageCount;
}

(I can't check whether is it valid C# or not but I hope you get the idea.)

I would also create a test with an empty array.


Optimize Performance challenge 'Vinay Queried ' Input:

  The first line contains N denoting the length of Target String. This
  line is followed by the Target String. The Target String is followed
  by an integer Q denoting the number of queries asked. After this Q
  lines follow each line containing a Query String.

  Output:

  For each query output the total number of distinct Cruel Pairs that
  satisfies the given conditions.

  Constraints:

  \$1 \le N \le 5000\$

  \$1 \le Q \le 10^4\$

  Sum of Lengths of all Query Strings \$\le\$ 106

  All strings are composed only of '0' - '9'

  SAMPLE INPUT

5
75201
5
78945884875
22
00048
77
501

  SAMPLE OUTPUT

0
8
8
6
5

  Explanation

  Query String 1 : No such Pair exist.

  Query String 2 : Pairs are (1,2) , (1,3) , (1,4) , (1,5) , (2,3) ,
  (2,4) , (2,5) , (3,5)

  Query String 5 : Pairs are (1,3) , (1,4) , (1,5) , (2,4) , (2,5)

  Time Limit:1.0 sec(s) for each input file.

  Memory Limit:256 MB

  Source Limit:1024 KB

FUll question:
http://qa.geeksforgeeks.org/9744/vinay-queried-interesting-problem-optimization

Code:

def get_all_substrings(input_string, length):
    return [int(input_string[i:j + 1]) for i in xrange(length) for j in xrange(i, length)]

N = int(raw_input())

target = raw_input()

target_subsets = get_all_substrings(target, N)
target = int(target)

Q_N = int(raw_input())

queries_list=[]
output=[]

for x in xrange(Q_N):
    queries_list.append(int(raw_input()))

for x in queries_list:
    if x > target:
        print 0
        continue
    cruel_pairs = sum(i>x for i in target_subsets)
    print cruel_pairs

Target string could be 5000 digits strong. It means that the target_subsets can have up to 12500000 elements. It is huge (I am surprised that the memory limit was not exceeded). You scan all of them for each query. Time limit exceeded obviously.

One obvious improvement is to sort target_subsets, and look for a lower bound of a query value (in logarithmic time). The result is simply len(target_subsets) - lower_bound.

This is still suboptimal, because the way you compute target_subsets has \$O(N^3)\$ time complexity. It is another candidate for TLE. It is more or less trivial to reduce it to \$(N^2)\$, but I'd still be uncomfortable.

The solution is a simple linear scan, along the lines of

Let `d` be a first non-zero digit of a query string
For target digit 't' at index 'i'
    if t > d, there are `len(target_string) - len(query) - i` pairs (as long as it is positive);
    if t == d, there _may_ produce one extra pair;
    it 0 < t < d, there are `len(target_string) - len(query) - i - 1` pairs (again, as long as it is positive);
    if t == 0, there are as many pairs as for the next non-zero digit.


Beautifulsoup scrape posts that has the word python in it with no python tag This is my first time web scraping, and here is the code I whipped up:
from bs4 import BeautifulSoup
import requests
import time

keywords = ['python']

for n in range(1000):
    res = requests.get(f"https://stackoverflow.com/questions?tab=newest&page={n}")
    time.sleep(3) # Sleep to avoid getting rate limited again
    soup = BeautifulSoup(res.text, "html.parser")
    questions = soup.select(".question-summary") # List of all question summaries in the current page
    for que in questions:
        found = False
        tagged = False
        q = que.select_one('.question-hyperlink').getText() # Store the title of the question
        for a in que.find_all('a', href=True):
            u = a['href'] # Store the link
            if u.split('/')[1] == 'questions' and u.split('/')[2] != 'tagged': # If this link is a question and not a tag
                res2 = requests.get("https://stackoverflow.com" + u) # Send request for that question
                time.sleep(3) # Extra precaution to avoid getting rate limited again
                soup2 = BeautifulSoup(res2.text, "html.parser")
                body = str(soup2.select(".s-prose")) # This is the body of the question
                if any(key in body for key in keywords):
                    found = True
            if 'tagged/python' in u:
                tagged = True

        if found and not tagged:
            print(q)

My code basically scrapes Stack Overflow posts newest first, and prints out all the posts that has the keyword "python" in its body, but no python tag. I want to know, did I implement the algorithm optimally? Can you show me where to improve?

The most important change: Check the tags before even getting the question's page. If it's tagged with python, then you know the question doesn't fit your criteria, regardless of what's in the body. Considering how popular the python tag is, this should save a good amount of time and processing.
I introduced a requests.Session object, which could improved performance. I also tweaked the variable names, and replaced the CSS selectors with the find()/find_all() methods. It's mostly a matter of personal preference, though.
Keep in mind that this code contains no error handling.
import requests
from bs4 import BeautifulSoup

tag_keywords = ['python']
content_keywords = ['python'.casefold()]

res = []

with requests.Session() as sess:
    for page_num in range(1):
        new_questions_page_req = sess.get(f'https://stackoverflow.com/questions?tab=newest&page={page_num}')
        soup = BeautifulSoup(new_questions_page_req.content, 'lxml')
        questions_container = soup.find('div', attrs={'id': 'questions', 'class': 'flush-left'})
        questions_list = questions_container.find_all('div', attrs={'class': 'question-summary'}, recursive=False)

        for curr_question_cont in questions_list:
            question_id = curr_question_cont['id'][17:]
            summary_elem = curr_question_cont.find('div', attrs={'class': 'summary'})
            tags_container = summary_elem.find('div', attrs={'class': 'tags'})
            tag_names = [elem.get_text() for elem in tags_container.find_all('a', recursive=False)]

            if not any(curr_tag in tag_keywords for curr_tag in tag_names):
                question_rel_url = summary_elem.find('h3').find('a', attrs={'class': 'question-hyperlink'})['href']
                question_page_req = sess.get(f'https://stackoverflow.com/q/{question_id}')
                question_page_soup = BeautifulSoup(question_page_req.content, 'lxml')
                question_body = question_page_soup.find('div', attrs={'class': 's-prose'})
                question_body_text = question_body.get_text().casefold()

                if any(curr_keyword in question_body_text for curr_keyword in content_keywords):
                    res.append(question_id)

print(res)


Effectively convert little endian byte slice to int32 I have a data stream of bytes and I'd like to get a little endian encoded int32 from four bytes. Is there a better way than to do this like the following code?

package main

func read_int32(data []byte) int32 {
    return int32(uint32(data[0]) + uint32(data[1])<<8 + uint32(data[2])<<16 + uint32(data[3])<<24)
}

func main() {
    println(read_int32([]byte{0xFE,0xFF,0xFF,0xFF})) // -2
    println(read_int32([]byte{0xFF,0x00,0x00,0x00})) // 255
}

The code above seems to work fine, but perhaps there is a built-in function in Go that I've missed or there is a super cool hack that does that in one instruction?

encoding/binary package may have what you need. Check this: http://golang.org/pkg/encoding/binary/#example_Read

Your code with modified read_int32 function could be:

package main
import (
    "bytes"
    "encoding/binary"
    "fmt"
)

func read_int32(data []byte) (ret int32) {
    buf := bytes.NewBuffer(data)
    binary.Read(buf, binary.LittleEndian, &ret)
    return
}

func main() {
    fmt.Println(read_int32([]byte{0xFE, 0xFF, 0xFF, 0xFF})) // -2
    fmt.Println(read_int32([]byte{0xFF, 0x00, 0x00, 0x00})) // 255
}

Also you can interpret big endian by replacing binary.LittleEndian with binary.BigEndian


A set of queries in PHP to show occupancy of various cabins Okay so as I learn more about PHP and really strive to improve my code I have a few questions about a current setup of mine and how to approach it in the best way.

I think I have all the tools necessary to get this done the right way, but I am having trouble putting them together. I am also trying to separate my code better.

Scenario: the script in question is cabins.php which basically pulls information from the db to populate the page with cabin information based on the cabin id $_GET variable. So the user clicks on a cabin, then the page is populated with that cabins information. There are four sub menus to choose from for each cabin so I am accessing this information using a switch statement, but I am having trouble finding the best way to run my queries.

My data is structured that most of the cabin information is held in 'cabin_content' but I also have 'cabin_large_images' and 'cabin_small_images' which each hold images that are tied to the cabin_content table through cabin_id.

Each sub section requires different information from the cabin_content table, but each sub section ALWAYS requires all the images from cabin_large_images that match the cabin id.

So each section is like this:
-we must query cabin_content and get the specific information we need
-we must query cabin_large_images and get all the images with the cabin_id.

One section requires all of the above plus we have to query cabin_small_images and get all the images from that table with matching cabin id.

So right now I have it set up like this, which I know needs improvement...obviously that is why I am here...Pseudo code:

if(isset($_GET['cabin')){

 //run the query to get cabin_large_images because we know we need them regardless of the section

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

       switch ($type){

       case 'a':

//run the query on cabin_content for the specific info we need in this situation

       break;

       case 'b':

//run the query on cabin_content for the specific info we need in this situation

       break;

       case 'c':

//run the query on cabin_content for the specific info we need in this situation
//run an additional query on cabin_small_images

       break;

      }

 }

//populate content to view

Okay so obviously what I don't like about this is we are running as many as three queries per page load. I tried using inner joins, but I just could never access what I needed from the query properly:

SELECT cabin_name, peak_week, max_occupancy, image
FROM cabin_content
INNER JOIN cabin_images
  ON cabin_content.cabin_id = cabin_images.cabin_id
WHERE cabin_content.cabin_id = '$cabin'";

The issue here is that we can end up with multiple images from $row['images'] which we then need to iterate through in the view. Now, we complicate the situation even more when for case 'c' we also need to query cabin_small_images:

SELECT cabin_name, peak_week, max_occupancy, image, image_small
FROM cabin_content
INNER JOIN cabin_images
  ON cabin_content.cabin_id = cabin_images.cabin_id
INNER JOIN cabin_small_images
  ON cabin_images.cabin_id = cabin_small_images.cabin_id
WHERE cabin_content.cabin_id = '$cabin'";

So, from that I need all the regular content and I need to iterate through 'image' and 'small_images' and I just could never get it right.

Alright so in closing, if the way I have it structured is good, then fine, but I have a feeling the best way to do this is with the joins, in which case I need help accessing all the information from the query. Or I need your other conceptual suggestions on the best way to do this!

There is nothing wrong with using multiple queries. Three simple queries versus a single, convoluted monster query. Guess which option will cause you the least problems long term?

You get the extra overhead of parsing three queries, but, its about the same amount of work for the DBMS to get the data in both cases.

Also I think you should consider holding your images in separate files and just hold the file names in the database. You can then use HTML "<image>" tags to reference these. This will make the images "cache"able on the client side, and, will move the work of retrieving the images to the web server, which is much more efficient and will allow for a degree of parallelism when loading the users page.


Removing this odd while(true) for checking the queue I was reviewing some WPF code I've written in C# last year.

The scenario is the following:

I've got a usercontrol that show usermessages and it's docked in the bottom of the application's workspace
I've got a ConcurrentQueue that receives message via a IMessageMediator (from Catel, but's irrilevant how data are pushed)
I've a workerprocess that checks if there's some item in the queue and add them to a list that's bound to a grid

private readonly ConcurrentQueue<UserMessage> queue;
private readonly List<UserMessage> dataItems;

private void worker_DoWork(object sender, DoWorkEventArgs e)
{
    while (true)
    {
        if (!queue.IsEmpty)
        {
            UserMessage message;
            while (queue.TryDequeue(out message))
            {
                dataItems.Add(message);
            }
            RaisePropertyChanged(() => FilteredDataItems);
            RaisePropertyChanged(() => ErrorCountMessage);
            RaisePropertyChanged(() => WarningsCountMessage);

        }
        Thread.Sleep(500);
    }
}

//That's used to show only filtered items (Error/Warning....)

public IEnumerable<UserMessage> FilteredDataItems
{
    get
    {
        if (!enabledFilters.Any())
            return dataItems;
        return dataItems.Where(x => enabledFilters.Contains(x.LogMessageType));
    }
}

How can I remove this while(true) and the ugly/odd Thread.Sleep(500)?

It sounds like you are sleeping only to avoiding busy waiting on the queue too frequently.

How about following the suggestion in this SO answer:

https://stackoverflow.com/a/5014271/2268996

so that you use the blocking Take method in your worker with code like this:

while (true)
{
    UserMessage m = blockingQueue.Take();
    dataItems.add(m);
    RaisePropertyChanged(() => FilteredDataItems);
    ... (no need to sleep here)...
}

You will still have the while (true) but not the Thread.sleep(...) call. Will that work for your situation?


Print inverse right triangle using recursion I wrote a program to print the following pattern using recursion to hands on recursion and my output is perfect but I want to know another and the most optimized approach for this.

This is the sample output of my program if we insert n = 5:

54321
4321
321
21
1

public class JavaApplication2 {

    /**
     * @param args the command line arguments
     */
    public static void main(String[] args) {
        Scanner scan = new Scanner(System.in);
        int n = scan.nextInt();
        printNumber(n,n);
        scan.close();
    }
    static int printNumber(int n,int a) {
            if(a == 0)
                return 0;
            else
            {
               System.out.print(n);

               if(n==1)
               {
                   System.out.println();
                   --a;
                   n = a +1;
               }
               n--;
            }
            return printNumber(n, a);
    }
}

Your code is pretty well written, however as always there are things we can change, and other things that are good that I would like to point out.

Positives:

You used a method. Dividing and conquering(although it seems small) is very important for shortening and simplifying code. It also prevents bugs.
Your code is easy to read

Things to fix:

It isn't good to give variables meaningless names. give them names like line rather than a, or num rather than n.
You indented 8 spaces after your printNumber method, rather than the usual suggested(and advised) 4 spaces.
Instead of doing return printNumber(n, a), simply call the method again, using printNumber(n, a). This makes the program just loop within that method.

If I were just to clean up your code and shorten it when applicable it would look like this:

public class JavaApplication2 {
    public static void main(String[] args) {
        Scanner scan = new Scanner(System.in);
        int highestNum = scan.nextInt();
        countDown(highestNum);
        scan.close();
    }

    static int countDown(int highNum) {
        int length = highNum;
        if(length != 0) {
            for(int i = length; i > 0; i--) {
                System.out.print(i);
            }
            System.out.println();
            highNum--;
            countDown(highNum);
        }
        return 0;
    }
}

Main changes to your code:

Made your main part the if, rather than the else. This removed the need for an else completely. It keeps looping until line is 0.
Simplified and shortened(a lot).
Changed names of method and variables.

There are many other simple ways to get the same output. Here's a very simple one using a for loop:

public class JavaApplication2 {
    public static void main(String[] args) {
        Scanner scan = new Scanner(System.in);
        int num = scan.nextInt();
        countDown(num);
        scan.close();
    }
    static int countDown(int num) {
        for(int i = num; i > 0; i--){
            System.out.print(i);
        }
        num--;
        System.out.println();
        if(num != 0) {
            countDown(num);
        }
        return 0;
    }
}

This program prints the countdown until it is 0, then it starts a new line, decreases num by one, and repeats.

Here's another way just using a double for loop and no recursion.

public class JavaApplication2 {
    public static void main(String[] args) {
        Scanner scan = new Scanner(System.in);
        int num = scan.nextInt();
        countDown(num);
        scan.close();
    }

    static void countDown(int num) {
        for(int height = num; height > 0; height--){
            for(int index = height; index > 0; index--) {
                System.out.print(index);
            }
            System.out.println();
        }
    }
}

This one prints index until it is 0 num times, decreasing every time.

The only reason I posted the other possibilities here is really just to prove no. there is no "most optimized" way of achieving this output. That's the beauty of programming, really. You can do the same thing 100,000 different ways. You can make this program using recursion, for-loops, and many other ways too, but they will all give the same output. The goal is to shoot for the cleanest, shortest, and most readable code. If you have any questions, please comment below.


Handling library entity Below you'll find a Symfony2 service I wrote to assist in handling the Library entity. Each user in my app has a personal library where one can upload items to and a purchased library, that contains items one has purchased. Both libraries fall under a parent library.

At several points in my controller, I need to get the library for the user to save data. If the library doesn't exist yet, I want to create it.

The two switches in the getUserLibrary() function especially bother me.

<?php

namespace Smoovi\SmooviBundle\Entity\Manager;

use Doctrine\ORM\EntityManager;

use Smoovi\SmooviBundle\Entity\Library,
    Smoovi\UserBundle\Entity\User;

class LibraryManager
{
    const TYPE_PURCHASED = 1;
    const TYPE_PERSONAL  = 2;

    protected $em;

    public function __construct(EntityManager $em)
    {
        $this->em = $em;
    }

    /**
     * Get a user's personal library
     *
     * @param \Smoovi\UserBundle\Entity\User $user
     * @return Library
     */
    protected function getPersonalLibrary($user)
    {
        return $this->getUserLibrary($user, self::TYPE_PERSONAL);
    }

    /**
     * Get a user's library for purchased objects
     *
     * @param \Smoovi\UserBundle\Entity\User $user
     * @return Library
     */
    public function getPurchasedLibrary($user)
    {
        return $this->getUserLibrary($user, self::TYPE_PURCHASED);
    }

    /**
     * Get or create a library based on $type
     *
     * @param User $user
     * @param $type
     * @return Library
     * @throws \Exception
     */
    protected function getUserLibrary(User $user, $type)
    {
        $libRepo = $this->em->getRepository('SmooviSmooviBundle:Library');

        // we first try to find the user's parent library
        // if not, create it
        /** @var \Smoovi\SmooviBundle\Entity\Library $topLibrary */
        $topLibrary = $libRepo->findOneBy(array('user' => $user, 'parent' => null));
        if (is_null($topLibrary)) {
            $library = new Library();
            $library
                ->setUser($user)
                ->setName($user->getFirstname()."'s Library")
                ->setIndex(1)
            ;
            $this->em->persist($library);
        }

        // look for the sublibrary
        // create it if necessary
        $where = array('user' => $user, 'parent' => $topLibrary);
        switch($type) {
            case self::TYPE_PURCHASED:
                $where['purchased'] = true;
                $libraryName = 'Purchases';
            break;
            case self::TYPE_PERSONAL:
                $where['personal'] = true;
                $libraryName = 'Objects';
            break;
            default:
                throw new \Exception('Library subtype not implemented');
        }
        /** @var \Smoovi\SmooviBundle\Entity\Library $subLibrary */
        $subLibrary = $libRepo->findOneBy($where);
        if (is_null($subLibrary)) {
            $subLibrary = new Library();
            $subLibrary
                ->setParent($topLibrary)
                ->setUser($user)
                ->setName($libraryName)
                ->setIndex(1)
            ;
            switch($type) {
                case self::TYPE_PURCHASED:
                    $subLibrary->setPurchased(true);
                break;
                case self::TYPE_PERSONAL:
                    $subLibrary->setPersonal(true);
                break;
                default:
                    throw new \Exception('Library subtype not implemented');
            }
            $this->em->persist($subLibrary);
        }
        $this->em->flush();

        return $subLibrary;
    }
}

Switch statements can often be a sign that you need an extended class. Doctrine supports Inheritance Mapping. With some liberal re-factoring I created a PersonalLibrary and PurchasedLibrary, each which extend "Library". The various classes might look like this:

/**
 * @Entity(repositoryClass="LibraryRepository")
 * @InheritanceType("SINGLE_TABLE")
 * @DiscriminatorColumn(name="libraryType", type="string")
 * @DiscriminatorMap({1 = "PurchasedLibrary", 2 = "PersonalLibrary"})
 */
class Library
{
    //abbreviated class. add the properties and setters/getters as needed

    /**
     * @param \Smoovi\UserBundle\Entity\User $user
     * @param null|Library $parent
     */
    public function __construct(User $user, Library $parent = null)
    {
        $this->setUser($user);
        $this->setParent($parent);
        $this->setIndex(1);
    }

    public function setUser(User $user)
    {
        $this->user = $user;
        $this->setName($user->getFirstname()."'s Library");
    }
}

class PurchasedLibrary extends Library
{
    public function __construct(User $user, Library $parent = null)
    {
        parent::__construct($user, $parent);
        $this->setLibraryType(1);
    }

    public function setUser(User $user)
    {
        $this->setUser($user);
        $this->setName('Objects');
    }
}

class PersonalLibrary extends Library
{
    public function __construct(User $user, Library $parent = null)
    {
        parent::__construct($user, $parent);
        $this->setLibraryType(2);
    }

    public function setUser(User $user)
    {
        $this->setUser($user);
        $this->setName('Purchases');
    }
}

class LibraryRepository extends EntityRepository
{
    /**
     * Get a user's library. If unable to create, return null.
     *
     * @param \Smoovi\UserBundle\Entity\User $user
     * @param int $type
     * @return null\Library
     */
    public function getOrCreateLibrary(User $user, $type)
    {
        $parent = $this->getOrCreateParentLibrary($user);

        $sub = $this->findOneBy(array(
            'user' => $user,
            'parent' => $parent,
            'libraryType' => $type
        ));
        if (! $sub) {
            $sub = $this->createNewLibrary($user, $type, $parent);
            if (! $sub) {
                return null;
            }
        }

        $this->persist($sub);

        return $sub;
    }

    public function createNewLibrary(User $user, $type, $parent = null)
    {
        switch ($type) {
            case 1:
                return new PersonaLibrary($user, $parent);
                break;
            case 2:
                return new PurchasedLibrary($user, $parent);
                break;
        }

        return null;
    }

    private function getOrCreateParentLibrary(User $user)
    {
        $parent = $this->findOneBy(array(
            'user' => $user,
            'parent' => null,
            'libraryType' => null,
        ));
        if (! $parent) {
            $parent = new Library($user);
        }

        return $parent;
    }
}

Notes:

I'm using a Library Repository instead of a Library Manager. It's how I'd do it but, your LibraryManager may be just fine too.
I moved much of the behavior (deriving name, etc) into the setters of the Entities. This free's your Manager/Repository from this concern.
I added user and parent to the base Library Entity constructor. I'm assuming that in your domain, a Library must have a user.
It seems odd that "Index" is always 1
It seems odd that a "Top Level" table does not seem to be purchased or personal.
I would not flush the entity manager inside the those methods. Instead, I would let my calling code do it. The reason is that you may have other db operations to perform along with these. Postponing the flush operation allows you to do everything in 1 transaction.
I renamed your method names to indicate that there could be side effects as a result of calling the method (get OR create).
No need to persist parent. Persisting the sub (assuming associations are set up) should persist the parent.
I think you'll always need at least 1 switch in there somewhere that is used to construct the new entity based on it's type.


Simple utility/convenience module - am I doing it right? I have a large library (four packages) of data processing code that I have written over the last 12 months. I am quite new to python and would like to see if I am doing things correctly.

This utility/convenience code is currently kept in a module called utils that is imported in many places (for an example, see this question). I thought it would be a simple place to start as it has no dependency on my other code.

The purpose of the first two functions is to switch between cumulative energy meter readings and actual energy consumption values.

The second two functions allow me to move between datetime and timestamp values, usually because I store datetime information in numpy arrays of timestamps for some calculations but need them as datetimes for formatting.

The switching between gaps and flags is useful because data interpolation identifies missing data and records it as boolean numpy arrays (flags) but I often need to identify contiguous chunks of missing data.

import numpy as np, datetime, time, logging

def movement_from_integ(integ):
    return np.append(np.nan, np.diff(integ))

def integ_from_movement(movement):
    return np.append(0.0, np.cumsum(movement[1:]))

def timestamp_from_datetime(dt):
    return np.array([time.mktime(d.timetuple()) for d in dt])

def datetime_from_timestamp(ts):
    return [datetime.datetime.fromtimestamp(s) for s in ts]

def gaps_from_flags(flags, ts):
    logger = logging.getLogger('gaps_from_flags')
    """given missing flags and timestamps produces a list of contiguous gaps"""
    _gap = {'from': None, 'to': None}
    result = []
    for i in xrange(len(flags)):
        if not flags[i]:
            logger.debug(flags[i])
            if _gap['from'] != None:
                result.append(_gap)
                logger.debug('Gap saved [%s -> %s]' % (_gap['from'], _gap['to']))
                _gap = {'from': None, 'to': None}
        else:
            if _gap['from'] == None:
                logger.debug('%s: New gap initialised at %s' % (flags[i], ts[i]))
                _gap['from'] = ts[i]
            logger.debug('%s: Gap extended to %s' % (flags[i], ts[i]))
            _gap['to'] = ts[i]
    if _gap['from'] != None:
        result.append(_gap)
        logger.debug('Gap saved [%s -> %s]' % (_gap['from'], _gap['to']))
    return result

def flags_from_gaps(gaps, ts):
    """given gaps and timestamps produces an array of boolean values indicating whether data are missing at each timestamp"""
    flags = np.array([False]*len(ts), dtype=bool)
    for g in gaps:
        a = (ts >= g['from']) & (ts <=g['to'])
        flags[a] = True
    return flags

Is it OK to keep a set of functions like this in an otherwise object oriented project?

Am I doing the flags->gaps->flags processing in a sensible way?

I am aware there is such a thing as a masked array in numpy but I'm not sure how much work it would be to convert my entire code base over to using it.

It was really hard to understand what you're code is doing, but it does have some easy spottable "problems":

is None vs. == None

PEP8 clearly states:

  Comparisons to singletons like None should always be done with is or is not, never the equality operators.

There's some further explaination here.

So you should change lines like:

if _gap['from'] == None

to:

if _gap['from'] is None

zip()/izip()

Let's take a look about what zip() does:

>>> x = [1, 2, 3]
>>> y = ['a', 'b', 'c']
>>> for num,ch in zip(x,y):
...     print num,ch
...
1 a
2 b
3 c

itertools.izip() is just the generator version of zip in Python 2 (like xrange() for range()). So instead of:

for i in xrange(len(flags)):

You may want to do:

from itertools import izip

for flag,time_stamp in izip(flags,ts):
    # ...

Don't pile imports

Is usually a better practice to import each package on its own line (it really helps readability). So, in your case, this will be better:

import logging    # I usually import logging as first
import numpy as np
import datetime
import time

Python has a very flexible syntax (as you've surely noticed), use it smartly and do not abuse of it.

& vs and

You have this line:

a = (ts >= g['from']) & (ts <=g['to'])

Are you sure you weren't looking for:

a = (ts >= g['from']) and (ts <=g['to'])

Or better:

a = g['to'] <= ts <= g['from']

_gap: unPythonic name

Your gaps_from_flags is full of _gap, self._gap might be a "private" attribute of an object, but _gap is a very strange name, if it's a gap just call it gap!

numpy.array creation

I would suggest a numpy tutorial.

After reading it, you may want to change this line:

flags = np.array([False]*len(ts), dtype=bool)

with:

flags = np.zeros((1,len(ts)), dtype=bool)

Let's talk about Design

Now that we cleared some things a bit, let's talk about your design. As I stated at the beginning I don't fully understand what you're doing, so fell free to provide more information, correct me if get things wrong, etc... etc...

First: go read The Zen of Python if you haven't already and then come back. Have you read it? Well! Now keep it mind every time you write a new line of code :)
Let me state this clear: I don't like those first four functions.
There's really a point of having one-line utility function? Are they really that useful if you'll have to replace one line with another one line?
If you had only one of them, with a complex one line and it was a real pain to repeat it all the times I might understand, but there are four of them and they are doing a very simple task: convertion.
So this leads me to: Why are you converting so much?
Do you really need it? I guess a better design will get rid of that. One conversion at the beginning and one at the end I'd say that's what one usually need with the proper design.
It seems like you're logging too much, but that's just my opinion thrown there.
gaps_from_flags is doing some really strange things. It seems that it's using a temporary gap dict. It looks very unpythonic. Borrowing from the Zen of Python I'd say that is ugly, complex and readability counts.
If I get what you're trying to do, you could use itertools.groupby(), probably something like this:

res = []
for k,g in itertools.groupby(zip(flags,tp), lambda x: x[0]):
    time_stamps = list(g)
    res.append({'from': time_stamps[0], 'to': time_stamps[-1]})

There's still way room for improvement in the above example, but as I said it's just a wild guess and an example. (Probably it doesn't even perfectly fit your flagging system).
Once again this leads me to think that you could probably get rid entirely of your flagging system. It looks painful to maintain. Is it really necessary? Perhaps there's a better design for what you're trying to do.
It seems like you're using numpy arrays like simple lists. There's a particular reason for using numpy here? And remember that: "premature optimization is the root of all evil".
You asked if it's ok to keep a set of function in an OO project. Well, the answer for Python is surely: Yes!


Validator for a user registration form In a user registration form, the validator currently checks not only user input, but also whether or not the user exists. This seems like business logic, which in terms of design should be in the service layer in a "save user" type method. On the other hand, it is highly convenient to keep it in the validator.

Should there be a non-input related check in the validator?

public void validate(Object target, Errors errors) {
    User user = (User) target;
    ValidationUtils.rejectIfEmpty(errors, "name", "name.required", "Error msg...");
    ValidationUtils.rejectIfEmpty(errors, "username", "username.required", "Error msg...");
    ValidationUtils.rejectIfEmpty(errors, "password", "password.required", "Error msg...");

    // check if user already exists
    String username = user.getUsername();
    if (username != null && userService.userExists(username)) {
        errors.rejectValue("username", "username.exists", "Username is taken.");
    }
}

as it probably hits a database, i would call userExists from outside the validate function.
So, registration method first checks the input. Then calls userService.Register(User user, Errors errors). Register detects that username already exists and throws. And validation does not need to know about usernames and that they are unique or non-unique.


Using atom to reference state in Clojure I am trying to implement a 2D fighting game in Clojure both for fun and learning.

Since the characters in a fighter game are essentially state machines, I constructed the following code:

(defrecord Frames [frames id])

where "frames" refer to the actual datas (hitboxes texture-names and etc) and "id" will be an atom referencing the "frames" data representing the current frame the character is in.

so I will initialize a frame like this (used :a :b :c just for testing):

(def f (Frames. [:a :b :c :d :e :f :g] (atom 0)))

and will retrieve the current frame by this:

(defn get-current-frame [frames] (nth (:frames frames) @(:id frames)))

and update by:

(defn update-current-frame [frames active] (reset! (:id frames) active))

and it works fine:

(def f (Frames. [:a :b :c :d :e :f :g] (atom 0)))
(update-current-frame f 3)
(get-current-frame f) ;; => :d

Obviously it works, but I am extremely in doubt that this is a good practice.

Due to my poor knowledge with Clojure, I am here asking

Is using atom referencing a vector the best way for representing simple states?
Should I use this kind of data structure for implementing fighting game frames?
If ignoring data structures, am I defining the functions (get-current-frame) and (get-current-frame) in an idiomatic way?

No, you probably don't want to store an atom in the record.  You probably want:

(defn get-current-frame [frame]
  (nth (:frames frame) (:id frame)))

(defn update-current-frame [frame active]
  (assoc frame :id active))

(def f (Frames. [:a :b :c :d :e :f :g] 0))

Then

> (get-current-frame f)
:a
> (get-current-frame (update-current-frame f 4))
:e

If you feel the need to have an atom:

brain.core> (def f (atom (Frames. [:a :b :c :d :e :f :g] 0)))
#'brain.core/f
brain.core> (get-current-frame @f)
:a
brain.core> (swap! f update-current-frame 4)
#brain.core.Frames{:frames [:a :b :c :d :e :f :g], :id 4}
brain.core> (get-current-frame @f)
:e


Is this a good way to limit the occurrence of OutOfMemoryException? I have a class that uses the SqlBulkCopy to bulk insert data into a SQL Server database.  My original implementation just passed the m_buffer (which is a reference to a class that implements IDataReader) directly into the WriteToServer. Now a new requirement has been added that, upon failure, the process should retry the Bulk Insert command.  Why you ask? Because one of our clients get intermittent SQL exceptions because they have crappy hardware.

Okay, I digress, since IDataReader is read-once I needed to come up with something different.  I chose to load the IDataReader into a DataTable first and then pass the DataTable into the WriteToServer method.  Now my problem is that I will be using more memory and since I do not know anything about the data (the number of columns nor the amount of data in each row) I wanted to attempt to make the code safe from an OutOfMemoryException.

Unfortunately I could not find a good way to determine the amount of available memory, so instead I chose to dump the data every 50,000 rows or when the memory allocation reaches an arbitrary number (1 gig), whichever comes first.

Does this sound like a sound approach or is there a better way?

// m_buffer is a read-once cache (implements IDataReader) that pulls
// data from an external source as needed so it uses very little memory.
// My original implementation just used m_buffer as the parameter of
// WriteToServer but now I have to add retry logic into the process.
// The retry logic is in the PutDataIntoDatabase method.

const long MAX_MEMORY_TO_USE = 1073741824;  // 1 gig

DataTable dataTable = new DataTable(m_tableName);
foreach (DataField d in m_buffer.GetColumns())
    dataTable.Columns.Add(new DataColumn(d.FieldName, d.FieldType));

while (m_buffer.Read())
{
    DataRow row = dataTable.NewRow();
    for (int i = 0; i < m_buffer.FieldCount; i++)
        row[i] = m_buffer.GetValue(i);

    dataTable.Rows.Add(row);

    long totalMemory = GC.GetTotalMemory(false);
    if (rowCount++ > 50000 || totalMemory > MAX_MEMORY_TO_USE)
    {
        PutDataIntoDatabase(dataTable);
        dataTable.Clear();
        rowCount = 0;
    }
}

if (dataTable.Rows.Count > 0)
    PutDataIntoDatabase(dataTable);

Based on your comments it looks like your code is actually some sort of library used by different clients in different environments. If that's the case I would rather let clients define the batch size they want rather than writing tricks around memory management on your own.

If you do need to manage the memory - I would suggest to reduce the frequency of GC.GetTotalMemory calls, e.g. check it once per 1000 rows or so. Bear in mind that since your component is not the only one in the application it may turn out that application has already consumed 1GB of RAM thus causing you to reduce the batch size to minimum.


Coverage processing on multiple Java projects with gmake As part of my data collection, I have to run multiple kinds of coverage processing on multiple Java projects. Below is my main Makefile intented only for gmake. Portability is not a requirement for me (for this project), but DRY and following make best practices are. Please comment on my code, and if there is any way to make this better.

.PHONY: checkout all clobber clean $(checkout.all)
.SECONDARY: $(addprefix projects/,$(names))

root:=$(CURDIR)
names=$(shell cat etc/projects.txt)
checkout.all=$(addprefix checkout-,$(names))
clean.all=$(addprefix clean-,$(names))

testgen.types=original randoop tpalus

all:
  @echo use 'make <type>-<project>'
  @echo projects = $(names)
  @echo types = $(testgen.types)

checkout: $(checkout.all)
  @echo $@ done.

checkout-%: projects/%/pom.xml
  @echo $@ done.

projects build: ;  mkdir -p $@

projects/%/pom.xml: | projects build
  cd projects && $(root)/bin/checkout $*
  touch $@

clean: $(clean.all)
  @echo $@ done.

clean-%: | projects/%/pom.xml
  cd projects/$* && git clean -xfd && git reset --hard
  @echo $@ done.

clobber-%:
  cd projects && rm -rf $*

#-----------------------------------------------------------------
root:=$(CURDIR)

define testgen =
$1 : $(addprefix $(1)-,$(names))
  @echo $$(@) done.

$1-% : projects/%/.$(1).done
  @echo $$(@) done.

%/.$1.done : | %/pom.xml
  echo $$(MAKE) -C $$(*) root=$$(root) $(1)
endef

$(foreach var,$(testgen.types),$(eval $(call testgen,$(var))))

Some clarification about the intended purpose of this project. I am doing an analysis on a large number of  projects from github. My method of action is to clone each project, run multiple tests on them. Before running any project, I do a git clean to make sure that the artifacts of previous tests are not present in the project directories. The types of tests and projects names vary frequently, and hence I have kept them outside the main Makefile.

.PHONY: checkout all clobber clean $(checkout.all)

There's no clobber target, so I think that will be ignored.

The root variable is unnecessary (CURDIR should be well known by anyone using make), and is even defined twice.

names=$(shell cat etc/projects.txt)

In my experience it's more common to write out the list in the makefile. If this is so long as to clutter up the file, you could create a separate makefile for variables like this and include variables.mk or similar.

projects build: ;  mkdir -p $@

Why not split this into two lines? Also, the -p doesn't make any difference since it's only creating a single directory under the current one.

touch $@

This looks like a hack to get around checkout not updating the modified date properly, or to rebuild a project when make doesn't think it needs to be rebuilt. You might be able to just set the relevant target .PHONY or change it so that rebuilding is only done when necessary.

cd projects/$* && git clean -xfd && git reset --hard

This is rather brutal. I'd rather advise to use non-recursive make (based on  Recursive Make Considered Harmful) and enumerate the files to remove to make sure you never remove anything other than generated files. You could have a separate target with cd projects/$* && git clean -xnd to check whether there's anything you still need to clean.

clobber-%:
  cd projects && rm -rf $*

This also looks dangerous. You're effectively saying that make clobber-whatever will delete whatever no matter what it is. I can't recall where I heard this advice, but I do believe it's good practice to enumerate the files which makefiles can and should work with, rather than adding general-purpose tactical nuclear missiles.

define testgen =
...
$(foreach var,$(testgen.types),$(eval $(call testgen,$(var))))

There's a lot of indirection going on here, so it's difficult to understand what will actually be run in the end. Are you sure this couldn't be done simpler?

This is a personal preference, but I tend to avoid any echo statements because the make output should make it clear what is actually happening, and comments like this often tend to get out of sync with the rest of the code. On the other hand, you can include special targets to get the value of all/any variables.