Dynamic Linq from string with EF I wrote an EF browser to dynamically navigate through entities. Beside basic properties of system types, each entity can have several singular or plural navigation properties. In order to support all of these using a single batch of code, I come up with the following solution considering the following requirement:

Each entity has an integer ID primary key.

Sample query strings (Person is an entity (ID = 10) with Job and Cars navigation property):

Person/10/Job

Person/10/Cars

Solution:

public List<dynamic> LoadEntitiesByQuery(String query) {

    String[] parts = query.Split("/".ToCharArray(), StringSplitOptions.RemoveEmptyEntries);
    String entityName = parts[0];
    int id = int.Parse(parts[1]);
    String subEntityName = parts[2];

    Type entityType = GetTypeFromEntityName(entityName);

    var tempObj = db.Set(entityType)
                    .AsQueryable()
                    .Where(String.Format("ID == {0}", id))
                    .FirstOrDefault();

    var propertyInfo = tempObj.GetType().GetProperty(subEntityName);

    List<dynamic> objects;

    if (propertyInfo.PropertyType.IsGenericType)
        objects = ((IEnumerable<dynamic>)propertyInfo.GetValue(tempObj, null)).ToList();
    else
    {
        var tr = propertyInfo.GetValue(tempObj, null);
        objects = new List<dynamic>();
        objects.Add(tr);
    }

    return objects;
}

internal static Type GetTypeFromEntityName(string entityName)
{
    Type entityType = null;
    switch (entityName)
    {
        case "Person":
            entityType = typeof(Person);
            break;
        case "Job":
            entityType = typeof(Job);
            break;

        ///  ....
    }
    return entityType;
}

I'm looking for faster ways of doing dynamic query instead of reflection.

I would write an extension method to do parsing of input string. so it should not be inside  your query method.
I have declared a class named SearchCriteria to encapsulate all data member you want t o query.

this is just a first set of refactoring which should be included.

public class SearchCriteria
{
    public Type EntityType { get; set; }

    public string SubEntityName { get; set; }

    public int EntityId { get; set; }

}

public static class ParseExtension
{
    public static SearchCriteria Parse(this string query)
    {
        string[] parts = query.Split("/".ToCharArray(), StringSplitOptions.RemoveEmptyEntries);
        var entityName = parts[0];
        var id = int.Parse(parts[1]);
        var subEntityName = parts[2];

        return new SearchCriteria
        {
            EntityType = GetTypeFromEntityName(entityName),
            EntityId = id,
            SubEntityName = subEntityName
        };
    }

    private static Type GetTypeFromEntityName(string entityName)
    {
        Type entityType = null;
        switch (entityName)
        {
            case "Person":
                entityType = typeof(Person);
                break;
            case "Job":
                entityType = typeof(Job);
                break;
        }
        return entityType;
    }
}

public List<dynamic> LoadEntitiesByQuery(SearchCriteria query)
{
    Type entityType = query.EntityType;

    var tempObj = db.Set(entityType)
                    .AsQueryable()
                    .Where(string.Format("ID == {0}", query.EntityId))
                    .FirstOrDefault();

    var propertyInfo = tempObj.GetType().GetProperty(query.SubEntityName);

    List<dynamic> objects;

    if (propertyInfo.PropertyType.IsGenericType)
        objects = ((IEnumerable<dynamic>)propertyInfo.GetValue(tempObj, null)).ToList();
    else
    {
        var tr = propertyInfo.GetValue(tempObj, null);
        objects = new List<dynamic>();
        objects.Add(tr);
    }

    return objects;
}


Plague Inc. in Python - Extremely Early Stage I've been working to create a game in Python similar to Plague Inc. and instead of just writing one big clump of code and then getting it reviewed, I wanted to do it stage-by-stage and make it a little easier. Also, reviews might possibly help me later on in the development process.

import datetime
import random

countries = [
   dict(Name = "China", Population = "1,363,560,000"),
   dict(Name = "India", Population = "1,242,070,000"),
   dict(Name = "United States", Population = "317,768,000"),
   dict(Name = "Indonesia", Population = "249,866,000"),
   dict(Name = "Brazil", Population = "201,032,714"),
   dict(Name = "Pakistan", Population = "186,015,000"),
   dict(Name = "Nigeria", Population = "173,615,000"),
   dict(Name = "Bangladesh", Population = "152,518,015"),
   dict(Name = "Russia", Population = "143,700,000"),
   dict(Name = "Japan", Population = "127,120,000"),
   dict(Name = "Mexico", Population = "119,713,203"),
   dict(Name = "Philippines", Population = "99,329,000"),
   dict(Name = "Vietnam", Population = "89,708,900"),
   dict(Name = "Egypt", Population = "86,188,600"),
   dict(Name = "Germany", Population = "80,716,000"),
   dict(Name = "Iran", Population = "77,315,000"),
   dict(Name = "Turkey", Population = "76,667,864"),
   dict(Name = "Thailand", Population = "65,926,261"),
   dict(Name = "France", Population = "65,844,000"),
   dict(Name = "United Kingdom", Population = "63,705,000"),
   dict(Name = "Italy", Population = "59,996,777"),
   dict(Name = "South Africa", Population = "52,981,991"),
   dict(Name = "South Korea", Population = "50,219,669"),
   dict(Name = "Colombia", Population = "47,522,000"),
   dict(Name = "Spain", Population = "46,609,700"),
   dict(Name = "Ukraine", Population = "45,410,071"),
   dict(Name = "Kenya", Population = "44,354,000"),
   dict(Name = "Argentina", Population = "40,117,096"),
   dict(Name = "Poland", Population = "38,502,396"),
   dict(Name = "Sudan", Population = "37,964,000"),
   dict(Name = "Uganda", Population = "35,357,000"),
   dict(Name = "Canada", Population = "35,344,962"),
   dict(Name = "Iraq", Population = "34,035,000"),
   ]

current_date = datetime.datetime.strftime(datetime.date.today(), "%m-%d-%Y")

print "Welcome to a Python version of Plague Inc."
user = raw_input('\nWhat is your name? ').title() # for later on in the game
print '\nCountries to choose from are\n'
print "Country".ljust(15),"Population\n"
for country in countries:
    print country["Name"].ljust(15), country["Population"]
startcountry = raw_input('\nWhat country would you like to start in? ').title()

if startcountry == 'Random': # random.sample can be used for more than one country later on
    startcountry = random.choice(countries)

diseasename = raw_input('\nWhat would you like to name your disease? ').title()

print '\nNews Bulletin'.ljust(45), current_date
print '-' * 55
print 'An unknown disease named', diseasename, 'has struck', startcountry, 'by storm.'
print 'Doctors and scientists will need to collaborate to find out more information about this disease.'

Some thoughts:

As the game goes on, you’ll need to record more information about the countries. Whether they’re doing medical research, border closures, infection/mortality rates, etc. Rather than storing all this information in a list of dictionaries, I’d suggest creating objects for each country. Here’s a simple example:

class Country(object):
    def __init__(self, name, population):
        self.name = name
        self.population = population

china = Country("China", 1363560000)

Also follow martijnn2008’s advice, and load the countries from an external file (probably a CSV). Keeping the data separate from the code is generally good practice. You’d probably be better off having a small dataset in the program to start with, as you build the basic logic, then start using the full data set later.

As unholysampler suggests, use an int for the population; this will make your life much easier later when chunks of the population start dying. You can always define a function that does nice comma-printing on your population data later.
Have different functions for parts of the game. For example, a start_game() function which just includes the code for setting up a new game, then another function news_bulletin() which prints a random news bulletin.

This will make it easier to reuse this code later – for example, if the user gets to the end of the game and you want to ask "Play again?", you can just redirect them to start_game() rather than jumping through hoops.
Rather than expecting the user to enter a name that’s formatted exactly as it’s stored in your data, you could:

Do something to clean up messy input. For example, right now, "china" is an invalid input, but any reasonable person can see what they meant. Perhaps do something like

if start_country.lower() in list_of_countries:
     # more code here

And weirdness happens if I choose a country name which isn't on your list. You could perhaps just ask again and again until you get an answer, or just pick one at random for me. I just want to get on with the game!
Just offer them a choice from a numbered list. Here’s a very crude mockup of how that might work:

Which country would you like to choose?
1 China
2 India
3 United States
Type the corresponding number: # wait for input
3
You have chosen "United States"

Magic numbers as arguments to ljust() throughout your code. Icky and fragile. Pull them out and make them variables, or better try to compute the necessary values on the fly.

ETA: The problem arises because you’re assuming that the window is 55 characters wide (for example, the print '-' * 55 line). You should pull this out into a window_width variable, so you don't have problems later on (and if necessary, can adapt it for wider or narrower windows).

What if, later, you want to add the Independent Former Republic of New South Nebelsbad? Stuff like that might cause you problems if you’re rigid about the width.
In your import statements, I would write

from datetime import datetime

Cuts out datetime.datetime statements, which just look weird. Also note that your MM-DD-YY may confuse players for whom that isn’t their default date system. How about

>>> current_date = datetime.datetime.strftime(datetime.date.today(), "%d %B, %Y")
>>> print current_date
29 April, 2014

That looks much more natural, and is unambiguous.


std::vector composition with self populating super-powers I have written a deliberately pretty simple class that wraps a std::vector to basically make it capable of self-populate itself with random generated numbers, simply to  be able to test the result of some sorting algorithms that I'm working on.

This is only a hobby project, it is not production code :-) so I'd like to know if there are any ways to improve this design or my approach.

Besides good practices, style and design, two of my biggest doubts are:

populate(): If I had used std::iota() followed by std::shuffle, as explained on the comments, I believe that the code would be much smaller, simpler and would kind-of have the same result (it wouldn't allow repeated numbers, as it does now).
std::mt19937 and std::uniform_int_distribution: mostly, if their implementation looks... OK. Looks like the need to explicitly seed the vector is inevitable, but I don't really like it. :-( this would obviously be avoided if I used my std::iota() implementation. I feel like this breaks the class' "super-powers".

Edit: Forgot to add that, regarding number 2, I'm also a bit curious about the way I'm seeding the engine. Does it looks correct?

Following, the header file:

#ifndef algorithmicVec_H
#define algorithmicVec_H

#include <vector>
#include <random>
#include <chrono>
#include <cstdint>

/**
*  std::vector composition with pseudo-random number self-populating capabilities
*/
class algorithmicVec {
private:
  std::vector<uint32_t> vec;
  static std::mt19937 gen;
  static std::uniform_int_distribution<uint32_t> uint_dist99;

  /**
  *  Returns a random uint32_t within the range of 0..99
  */
  uint32_t getRandomNumber();

public:
  /**
  *  Seeds static std::mt19937 with the current time in milliseconds since epoch
  */
  static void seed();

  /**
  *  Populates the vector with n elements using a std::uniform_int_distribution
  *  fed with std::mt19937.
  *
  *  Using std::iota() followed by std::shuffle was considered, but not implemented
  *  mostly because random generated numbers seemed more interesting at the time,
  *  even to observe the result of the tested algorithms with possible repeated numbers.
  */
  void populate (int sz);

  /**
  *  Returns item at position pos
  */
  uint32_t at(int pos);

  /**
  *  Returns an iterator to the first item of the vector
  */
  std::vector<uint32_t>::iterator begin();

  /**
  *  Returns an iterator past-to-the-end of the vector
  */
  std::vector<uint32_t>::iterator end();
};

#endif

The implementation file:

#include "algorithmicVec.hpp"

std::mt19937 algorithmicVec::gen;
std::uniform_int_distribution<uint32_t> algorithmicVec::uint_dist99(0, 99);

void algorithmicVec::seed() {
  unsigned long seed = std::chrono::system_clock::now().time_since_epoch() /
                       std::chrono::milliseconds(1);
  gen.seed(seed);
}

void algorithmicVec::populate (int sz) {
  for (int i = 0; i < sz; ++i) {
    vec.push_back(getRandomNumber());
  }
}

uint32_t algorithmicVec::getRandomNumber() {
  return uint_dist99(gen);
}

uint32_t algorithmicVec::at(int pos) {
  return vec.at(pos);
}

std::vector<uint32_t>::iterator algorithmicVec::begin() {
  return vec.begin();
}

std::vector<uint32_t>::iterator algorithmicVec::end() {
  return vec.end();
}

A minimal test case:

#include <iostream>
#include "algorithmicVec.cpp"

int main() {
  algorithmicVec::seed();
  algorithmicVec myVec;
  myVec.populate(10); // populates myVec with 10 random numbers
  for (auto a: myVec) {
    std::cout << a << std::endl;
  }
}

I think it is overkill to write an own class. This gives you many issues because you have to reimplement vector's interface.

In the current state I don't see any advantage over having just a function that returns a vector that is filled randomly:

#include <vector>
#include <random>
#include <chrono>
#include <algorithm>
#include <iterator>

std::vector<uint32_t> generateRandomVector(std::size_t size) {
    static std::mt19937 gen(
            std::chrono::system_clock::now().time_since_epoch()
                    / std::chrono::milliseconds(1));
    static std::uniform_int_distribution<uint32_t> uint_dist99(0, 99);
    std::vector<uint32_t> result;
    result.reserve(size);
    std::generate_n(std::back_inserter(result), size,
            [&] {return uint_dist99(gen);});
    return result;
}

Use it like this:

#include <iostream>

int main() {
    for (auto a : generateRandomVector(20)) {
        std::cout << a << std::endl;
    }
}

This approach has the advantage of cleanly separating the functionality of the vector (which is to store data) from the functionality of the random generation (which just manipulates a vector).

Of course this interface looses some flexibility but if you need it then you should write a class that allows to create random vectors and not a vector class that happens to fill itself with random numbers.

Your Approaches

Using iota and shuffle does not really shorten the code of populate and somewhat "obfuscates" the thing that is going on there (so you would have to add additional comments to state what you want to achieve and why you restrict yourself to permutations).
The seeding of the random number generator can be done during construction which means during static initialization in the implementation file:

std::size_t getSeedFromCurrentTime() {
    return std::chrono::system_clock::now().time_since_epoch() /
           std::chrono::milliseconds(1);
}

std::mt19937 algorithmicVec::gen(getSeedFromCurrentTime);

About obtaining the seeding from the time I have no experience and cannot help you there.


Determine if simple arithmetic expression is valid This looks very messy to me, and I'm fairly new to java, so I don't know what data structures could have helped me achieve the same result more efficiently. I just went ahead and brute-forced all possible failing scenarios:

Could this be further optimized to reduce the number of checks?

public static boolean isAnOperator(char c){
        return (c == '*' || c == '/' || c == '+' || c == '-' || c == '%');
    }

    private static boolean isValidExpression(String expression){
        //TEST 1: False if expression starts or ends with an operator
        if (isAnOperator(expression.charAt(0)) || isAnOperator(expression.charAt(expression.length()-1)))
            return false;

        //TEST 2: False if test has mismatching number of opening and closing parantheses
        int openParenthCount = 0;
        int closedParenthCount = 0;
        int consecutiveOpCount = 0;

        for (int i = 0; i < expression.length(); i++){
            if (expression.charAt(i) == '('){
                openParenthCount++;
                consecutiveOpCount = 0;
                //SUBTEST: False if expression ends with '('
                if (i == expression.length()-1) return false;
            }
            if (expression.charAt(i) == ')'){
                closedParenthCount++;
                consecutiveOpCount = 0;
                //SUBTEST: False if expression starts with ')'
                if (i == 1) return false;

            }
            if (isAnOperator(expression.charAt(i))){
                consecutiveOpCount++;
                //TEST 3: False if operator is preceded by opening paranthesis or followed by closing paranthesis
                if (expression.charAt(i-1) == '(' || expression.charAt(i+1) == ')')
                    return false;
            }
            //TEST 4: False if 2 operators found next to each other
            if (isAnOperator(expression.charAt(i)) || consecutiveOpCount > 0)
                return false;
        }
        if (openParenthCount != closedParenthCount)
            return false;

        //LAST TEST 5: All preceding checks ensure that
        // by this point, all exisiting parantheses open and close properly
        return true;
    }

It is not always an easy task to solve this sort of problem. For data structures, we traditionally use a stack for open and close parenthesis but this is only needed if the type of parenthesis can differ and need nesting correctly. For only "(" and ")" I simmply use an int.

I re-wrote the isAnOperator to a switch statement - its personal choice that one.

I added a isANumber function to aid some checking.

I unit tested the function with 9 tests and worked till it past them all. (JUnitParams, read it as the string is the expression and the boolean is what I expect your function to return)

Your original code fails 3 of these tests - but it may because of not handling spaces in the expressions but you didn't provide any example data

To handle parenthesis I used a counter - but decrement it if I find a close. If I get into a negative number then something went wrong and if it isn't 0 at the end it was wrong. You can work out from the final value if there were too many close or opens. Simple boolean values keep track of wether the last character was an operation or a open parenthesis.

I welcome feedback on this answer - I'm trying to keep it as close to the original code as possible

$(
            $("()", true),
            $("3", true),
            $("3 + 2", true),
            $("(3 + (2))", true),
            $(")", false),
            $("+3", false),
            $("3+-2", false),
            $("(3 + (3 + 2)", false),
            $("(x + 3)", false)
    );

I've re-written the function to be cleaner with less variables:

public static boolean isAnOperator(char c) {
    switch (c) {
        case '*':
        case '/':
        case '+':
        case '-':
        case '%':
            return true;
        default:
            return false;
    }
}
public static boolean isANumber(char c){
    return ((int)c) >= 48 && ((int)c) <= 57;
}

public static boolean isValidExpression2(String expression) {
    // TEST 1
    if (isAnOperator(expression.charAt(0)) || isAnOperator(expression.charAt(expression.length() - 1))) {
        return false;
    }

    int openParenthCount = 0;
    boolean lastWasOp = false;
    boolean lastWasOpen = false;

    for (char c : expression.toCharArray()) {
        if(c == ' ') continue;
        if (c == '(') {
            openParenthCount++;
            lastWasOpen = true;
            continue;
        } else if (c == ')') {
            if (openParenthCount <= 0 || lastWasOp) {
                return false;
            }
            openParenthCount--;
        }else if (isAnOperator(c)){
            if (lastWasOp || lastWasOpen) return false;
            lastWasOp = true;
            continue;
        }else if(!isANumber(c)){
            return false;
        }
        lastWasOp = false;
        lastWasOpen = false;
    }
    if(openParenthCount != 0) return false;
    if(lastWasOp || lastWasOpen) return false;
    return true;
}


Shell script to download and extract a tarball from GitHub I don't want anything to execute if any preceding step fails:

#!/bin/sh

file="v0.9"
renamed=$file".tar.gz"
dir="utils/external/firepython/"
location="https://github.com/darwin/firepython/tarball/$file"

wget --no-check-certificate $location --output-document=$renamed && \
mkdir -p $dir && \
gunzip $renamed && \
echo "extracting to $dir" && \
tar xf $file".tar" --directory $dir --strip-components 1 && \
echo "Cleaning up..." && \
rm -r $file".tar" && \
echo "Done"

You're looking for set -e. From POSIX:

  -e
  When this option is on, if a simple command fails for any of the reasons listed in Consequences of Shell Errors or returns an exit status value >0, and is not part of the compound list following a while, until, or if keyword, and is not a part of an AND or OR list, and is not a pipeline preceded by the ! reserved word, then the shell shall immediately exit.

In other words, plain commands cause the shell to exit if they fail. (You can use something like command || true to allow command to return nonzero.) If you need to perform some cleanup, you can set a trap for the EXIT pseudo-signal.

And better double-quote all your variable expansions. That way your script won't fail horribly if you ever point it at a directory or an URL containing ? or * or a space.

dry -r
wget --no-check-certificate "$location" --output-document="$renamed"
mkdir -p "$dir"
gunzip "$renamed"
echo "extracting to $dir"
tar xf "$file.tar" --directory "$dir" --strip-components 1
echo "Cleaning up..."
rm -r "$file.tar"
echo "Done"

Another useful shell idiom to pass optional arguments to a shell script without hassle is to set variables only if they're unset. That way you can pass arguments through the environment, e.g. file=v0.9.1 myscript.

: "${file=v0.9}"
: "${renamed=$file.tar.gz}"
: "${dir=utils/external/firepython/}"
: "${location=https://github.com/darwin/firepython/tarball/$file}"


Force-unwrapped property as a contract I have a class, which is meant to show a full screen modal controller with a set of actions (apart from actions it also has some static content, a lot of labels and images which are irrelevant, so I omitted approximately 500 lines of code to keep it simple). It looks as follows:

import UI_Framework

class DescriptionViewController: UIViewController {

    var actions: [(title: String, action: () -> Void)]! {
        didSet {
            setUpActions(actions)
        }
    }

    private var actionStackView: UIStackView!

    // MARK: Lifecycle

    init(actions: [(title: String, action: () -> Void)]? = nil) {
        super.init(nibName: nil, bundle: nil)
        if let actions = actions {
            self.actions = actions
        } else {
            self.actions = [(title: "Close", action: { [weak self] in
                 guard let strongSelf = self else {
                     return
                 }
                 strongSelf.dismiss(animated: true)
            }]
        }
    }

    func viewDidLoad() {
        super.viewDidLoad()
        actionStackView = UIStackView(axis: .horizontal, spacing: 20)
        view.addSubview(actionStackView)
        actionStackView.fillSuperView()
        setUpActions(actions)
    }

    // MARK: Private

    private func setUpActions(_ actions: [(title: String, action: () -> Void)]) {
        guard let actionStackView = actionStackView else {
            return
        }

        actionStackView.subviews.forEach { $0.removeFromSuperview() }
        actions.map { CustomButton(title: $0.title, onClick: $0.action) }.forEach { actionStackView.addArrangedSubview($0) }
    }

}

Important note - I cannot define default actions during first phase of initialization because these actions are actually referencing to the class instace itself.

The idea behind having actions property force unwrapped instead of optional is because it's actually not supposed to have a nil value, as it may get somewhat confusing: what is meant by nil? Empty array of actions or default actions? It's especially crucial in the constructor, as it can take any of the values and should treat them differently. So the force-unwrapped property explains it by itself - do not make me nil, set empty array if you want empty array, keep it explicit and clear. Even if anyone sets it to nil by mistake it should get easily discovered - app will just crash.

However there is an opinion that force-unwrapping should be avoided at any cost to not introduce an error-prone code which might crash the app.

What is the better approach here?

A couple of observations:

You said:

  ... there is an opinion that force-unwrapping should be avoided at any cost to not introduce an error-prone code which might crash the app.

Obviously, “at any cost” is too strong. If it was to be avoided at all costs, it wouldn’t be part of the language. But avoid it if you reasonably can. But we use it all the time (e.g. @IBOutlet references). So use it where appropriate, but avoid it if you can (without contorting yourself too much).
You go on to say:

  The idea behind having actions property force unwrapped instead of optional is because it’s actually not supposed to have a nil value, as it may get somewhat confusing: what is meant by nil?

I’d buy that argument if you didn’t then have a constructor that accepted nil and then supplied that default “close” action.

Personally, I’d embrace that it is optional and then have setUpActions handle the nil and empty scenarios.

Thus, perhaps:

public struct Action {
    let title: String
    let action: (() -> Void)?

    init(title: String, action: (() -> Void)? = nil) {
        self.title = title
        self.action = action
    }
}

class DescriptionViewController: UIViewController {
    private var actions: [Action]? = nil

    // MARK: Lifecycle

    init(actions: [Action]? = nil) {
        self.actions = actions

        super.init(nibName: nil, bundle: nil)
    }

    required init?(coder aDecoder: NSCoder) {
        fatalError("init(coder:) has not been implemented")
    }

    override func viewDidLoad() {
        super.viewDidLoad()

        let actionStackView = UIStackView(axis: .horizontal, spacing: 20)
        view.addSubview(actionStackView)
        actionStackView.fillSuperView()
        setUpActions(for: actionStackView)
    }

    func addAction(_ action: Action) {
        if actions == nil { actions = [] }
        actions!.append(action)
    }
}

// MARK: - Private methods

private extension DescriptionViewController {
    func setUpActions(for stackView: UIStackView) {
        if actions?.isEmpty ?? true {
            actions = [Action(title: "Close")]
        }

        actions?.map { action in
            CustomButton(title: action.title) { [weak self] in
                    action.action?()
                    self?.dismiss(animated: true)
                }
            }
            .forEach { stackView.addArrangedSubview($0) }
    }
}

A few observations in the above:

Note, I’ve replaced the named tuple scenario with a custom type, Action. That seems better than repeating the tuple definition repeatedly.
I’ve also changed it to add that default “close” action if nil or isEmpty, though if you want it to only do that in the nil scenario, that’s a simple change, too.
You don’t need that strongSelf pattern in the “close” closure. self?.dismiss(...) achieves the same thing. Besides, if you did need that strongSelf pattern, since Swift 4.2, you’d just do:

guard let self = self else { ... }

See SE-0079.
I’ve deferred the setUpActions until viewDidLoad, permitting a flow where you initialize it without any actions and then subsequently set the actions (but presumably still before you present it).

I’ve also simplified this, eliminating this concept of adding actions after the view controller has already been presented. Go ahead and keep that stuff if you need it, but it has a hint of code smell.

For example, in what situation will you have this presented and then subsequently add new actions. That might not be a great UX (where the user may be in the processing of trying to tap the screen and then everything shifts around as new buttons are added, possibly resulting on a tap on something other than what they originally intended).
As part of this refactoring, I’ve also removed the observer on actions. In general, one should hesitate to add observer to a collection. E.g. what if the app developer is adding one button at a time to the array. By the time they add the fifth button, the setUpActions actions will have been called five times. Sure, you probably won’t do that, but it’s the sort of thing that bites you later on, when you forget what’s going on. Admittedly, there are likely so few items that it matters, but as a general rule, be careful with observers on collections. Besides, it’s better to defer the building of UIKit controls until viewDidLoad.
I’ve tweaked this so that the setUpActions routine adds the self?.dismiss(animated: true) itself. That way, when the app developer adds their own actions, they don’t have to worry about dismissing the DescriptionViewController itself, but rather every button will, after running the supplied closure (if any) will automatically, dismiss the DescriptionViewController. Then, you can do things like:

let actionSheet = DescriptionViewController()
actionSheet.addAction(Action(title: "Save") {
    // put save logic here
})

actionSheet.addAction(Action(title: "Cancel")) // no closure needed as we don't need to do anything

We can avoid the cruft of dismissing the DescriptionViewController from every added action.
By the way, you’ve said that you’ve omitted some 500 lines of code. Hopefully that’s an exaggeration or much of that code isn’t actually in the view controller (e.g. the UIStackView extension, the CustomButton, the UIView extension, etc.). But if you are saying that there are 500 more lines in the view controller, you might want to think creatively about refactoring that. (E.g. in the spirit of Dave DeLong’s A Better MVC.)


Yet another OpenGL Shader: the Builder pattern and lifetime management I've noticed that whenever a shader program class is implemented (from some questions on this site, as well as more reputable implementations), that:

The class tries to do too much. Binding to the current context, setting attribute variable values, and much more.
Use the JavaBeans pattern in one form or another, allowing the shader program to be in an invalid or incomplete state. For example, in between addShaderFromSource calls, the program is only partially built, and not usable.

My ideal shader program class would do much less; its purpose would just be to manage the lifetime of a shader program object, while maintaining a valid state throughout the object's lifetime (either a zero program or a valid, linked program), and the solution I came up with to achieve this uses the builder pattern.

void deleteAttachedShaders(GLuint programId) {
    constexpr GLsizei MAX_SHADER_COUNT = 5;
    GLuint shaderIds[MAX_SHADER_COUNT];
    GLsizei shaderActualCount;
    glGetAttachedShaders(programId, MAX_SHADER_COUNT, &shaderActualCount, shaderIds);

    for(int i = 0; i < shaderActualCount; ++i) {
        glDetachShader(programId, shaderIds[i]);
        glDeleteShader(shaderIds[i]);
    }
}

class GLShaderProgram
{
public:

    class Builder {
    public:
        Builder() : linked_(false) {
            programId_ = glCreateProgram();
        }

        Builder &addShaderFromSource(std::string const &shaderSource, GLenum shaderType) {
            char const *shaderCStr = shaderSource.c_str();
            GLuint shaderId = glCreateShader(shaderType);
            glShaderSource(shaderId, 1, &shaderCStr, nullptr);
            glCompileShader(shaderId);
            glAttachShader(programId_, shaderId);
            return *this;
        }

        Builder &addShaderFromFile(std::string const &shaderFile, GLenum shaderType) {
            std::ifstream fileName(shaderFile);
            std::istreambuf_iterator<char> fileBegin(fileName), fileEnd;
            std::string fileContents(fileBegin, fileEnd);
            return addShaderFromSource(fileContents, shaderType);
        }

        GLShaderProgram linkProgram() {
            glLinkProgram(programId_);
            GLShaderProgram theProgram(programId_);
            deleteAttachedShaders(programId_);
            linked_ = true;
            return theProgram;
        }

        ~Builder() {
             if(!linked_) {
                 glDeleteProgram(programId_);
             }
         }
    private:
        GLuint programId_;
        bool linked_;
    };

    GLShaderProgram() : programId_(0) { }

    GLShaderProgram(GLShaderProgram &&other) {
        *this = std::move(other);
    }

    GLShaderProgram &operator=(GLShaderProgram &&other)
    {
        programId_ = other.programId_;
        other.programId_ = 0;

        if (other.programId_ != 0) {
            glDeleteProgram(other.programId_);
        }

        return *this;
    }

    ~GLShaderProgram() {
        glDeleteShader(programId_);
    }

    GLuint programId() const { return programId_; }

    GLShaderProgram(GLShaderProgram const &other) = delete;
    GLShaderProgram &operator=(GLShaderProgram const &other) = delete;
private:
    GLShaderProgram(GLuint programId) : programId_(programId) { }
    GLuint programId_;
};

Example usage:

auto shaderProgram = GLShaderProgram::Builder()
    .addShaderFromFile("vertex.glsl", GL_VERTEX_SHADER)
    .addShaderFromFile("fragment.glsl", GL_FRAGMENT_SHADER)
    .linkProgram();

glUseProgram(shaderProgram.programId());

Any comments on the code's safety/quality would be appreciated.

Your implementation is very OOPy and I'd call it a good rendering of the builder pattern. Your usage example makes for a nice "fluent" API, BUT, I would personally not buy it.

The reason why 99% of the implementations out there are just reinventing OpenGL in a OOP way is because everyone takes the sorter path of a bottom up approach. They look at the API and see: Humm, so it looks like we have this glShaderSource thingy, so my class will need an addShaderSource method...

If they'd take a top down approach instead, they would first consider if exposing an addShaderSource method of sorts is even necessary.

First basic constraint of a GL shader program is that it requires at least two stages: Vertex and Fragment. So it doesn't even make sense for a shader program to exist without these two stages.
Linking the program is also a low-level implementation detail that I'd avoid exposing to the interface whenever possible. Again, a shader that fails to link is useless and an unlinked object should not be allowed to propagate.

Take all that into account and what should come to you is the good ole factory function. It receives all the required inputs and return a valid object or fails in your favourite way (return null, exception, error code, etc) without leaking intermediate incomplete objects.

//
// Creates and links the shader or returns a null to indicate failure.
// You could also do a lot more for error handling, like throwing and exception
// with the compiler info log or returning the info log as an optional output parameter.
// I usually integrate this kind of stuff with a log system, so that's where my shader info log goes.
//
std::unique_ptr<GLShaderProgram> createGLShaderProgram(const std::string & vertexFilename, const std::string & fragmentFilename);

Now suppose you'd like to support more shader stages rather then the minimal Vertex+Fragment pair, like Geometry shader or Compute shaders. There are many ways to do it, but sticking to our previous example, I'd personally introduce a helper structure in this case:

struct ProgramStage
{
    enum Type
    {
        Vertex,
        Fragment,
        Geometry,
        // ... what have you ...
    };

    Type type;
    std::string filename; // This could also be a source code string instead if it suits you.
};

std::unique_ptr<GLShaderProgram> createGLShaderProgram(const std::vector<ProgramStage> & stages);

I hope the above snippets made the idea clear to you. The point is not to just try to wrap OpenGL in a class, but take a step back and actually consider what is really needed, which is avoiding the propagation of incomplete objects (I'd also add to that a simple and clean interface!). Your builder pattern doesn't do that. If the user forgets to add a mandatory stage, BAM!, incomplete state.

Now lets look at the rest of the code.

Move operator is broken:

GLShaderProgram &operator=(GLShaderProgram &&other)
{
    programId_ = other.programId_;
    other.programId_ = 0;

    if (other.programId_ != 0) {
        glDeleteProgram(other.programId_);
    }

    return *this;
}

Look closely to it in isolation for a few seconds. Do you realize the bug? other.programId_ is set to zero, then you test if it is non-zero in the next line to delete it. That if check is always false. But there is another less obvious error there. programId_ from this object gets overwritten, so you are officially leaking that handle if it contained a valid object.

You'd better just scrap that operator and rewrite a new one using the copy and swap idiom.

glDeleteShader != glDeleteProgram:

~GLShaderProgram() {
    glDeleteShader(programId_);
}

I'm fairly confident that the member programId_ is a shader program, not a shader object/stage, so it is being deleted by the wrong function. This is undefined behavior at best. You should be using glDeleteProgram, that was probably a typo, since in other places you use the correct function. Blame OpenGL for using goddamned integers for everything!

Make deleteAttachedShaders a member of something?

If that function is not useful elsewhere, then it should be made a static member of one of your classes.


Macro that generates a defn I need to save nested data to disk for a project. Right now, my state is a nested record, and has proven to be a pain to serialize. I tried compressing it to a map, but after testing it, I realized my code failed in a unfortunate corner case that proved it to impractical. I decided to take the suggestion to drop using records altogether since I don't need the speed, but found it annoying to write ->-ish pseudo-constructors for everything I needed to represent. I decided to write a macro that takes a vector of keyword fields, and emits a defn that creates an map with the field names mapped to the arguments of the function:

(defmap Hello [:one :two :three])
(->Hello 1 2 3)

{:one 1, :two 2, :three 3}

I decided for the macro to take keywords as arguments so they're introduced to IntelliJ. defrecord doesn't give any hints, so the first time you use a record's fields, they aren't suggested by IntelliJ.

Unfortunately, IntelliJ complains about Hello not existing, and doesn't recognize ->Hello as a declared function, so this isn't very nice to use, but it was a fun little sidetrack. I don't know if I'll actually use it, but it's the most complicated macro I think I've ever written, and I'd like it reviewed.

I'd like to know if I'm doing anything roundabout or overly complicated. I'd also like to know if there's a way to hint to IntelliJ that Hello doesn't need to be defined, and that ->Hello does.

(defmacro defmap
  "Generates a defn that creates a function with the name (str \"->\" map-name) that
    returns a map. The map will have keys of fields, with the values of the arguments passed to the function."
  [map-name fields]
  (let [adjusted-name# (str "->" map-name)
        args# (mapv (comp symbol name)  fields)
        body-map# (into {} (map #(do [% (symbol %2)]) fields args#))]

    `(defn ~(symbol adjusted-name#) ~args# ~body-map#)))

The code is nice, there are only some minor improvements here.

I love that you have written a docstring.
You do not need the # in the variable names. These names are used for generating symbols but you do not use this capability in the code.
You could make adjusted-make a symbol when defined in the let block. This way the constructed form would be simpler and more readable.
I prefer doing assert calls for error handling in macros. You could check that the first parameter is a symbol? and the second is a vector? of keys.
Instead of (into {} (map ...)) you can also use zipmap to construct the map.

The code can be rewritten as the following:

  (defmacro defmap
"Generates a defn that creates a function with the name (str \"->\" map-name) that
returns a map. The map will have keys of fields, with the values of the arguments passed to the function."
[map-name fields]
(assert (symbol? map-name))
(assert (vector? fields))
(let [adjusted-name (symbol (str "->" map-name))
      args          (mapv (comp symbol name) fields)
      body-map      (zipmap fields args)]
  `(defn ~adjusted-name ~args ~body-map)))


Providing one-time calculations to frequently used methods I'm new to JS/jQuery and wrote some JavaScript/jQuery lines to create a "pulsating" object.

I get the "pulse" effect by increasing/resizing the object and then bringing it back to its original state periodically.  I was uncomfortable with calculating the necessary values every time they are needed, so I used the first thing that came to my mind - global variables and only modify their values when the page is fully loaded.  But I also don't like that approach.

What is best practice in JavaScript/jQuery for this kind of problem, and how can I refactor my code accordingly?

Here is the code (JavaScript only, please see my link for HTML&CSS):

var elementOrigHeight;
var elementOrigWidth;
var elementIncreasedHeight;
var elementIncreasedWidth;
var elementNewTop;
var elementNewLeft;

$(document).ready(function () {
    var element = $('#circle');
    var growInPercent = 100;
    setGlobalVariables(element, growInPercent);
    var durationGetBigger = 500;
    var durationGetSmaller = 100;
    var frequency = 3;
    var pause = 3000;
    pulsate(element, durationGetBigger, durationGetSmaller,
    frequency, pause);
});

function pulsate(element, durationBigger, durationSmaller, frequency, pause) {
    for (var i = 0; i < frequency; i++) {
        getBigger(element, durationBigger);
        getSmaller(element, durationSmaller);
    }
    setTimeout(function () {
        pulsate(element, durationBigger, durationSmaller, frequency, pause)
    }, pause);
}

function getBigger(element, duration) {
    element.animate({
        width: elementIncreasedWidth,
        height: elementIncreasedHeight,
        top: elementNewTop,
        left: elementNewLeft,
        opacity: 1
    }, duration, 'linear');
}

function getSmaller(element, duration) {
    element.animate({
        width: elementOrigWidth,
        height: elementOrigHeight,
        top: 0,
        left: 0,
        opacity: 0.5
    }, duration, 'linear');
}

function setGlobalVariables(element, percent) {
    elementOrigHeight = element.height();
    elementIncreasedHeight = Math.round(elementOrigHeight*(1+percent/100));
    if (elementOrigHeight % 2 > 0) { // odd
        // make sure increased size is odd too
        elementIncreasedHeight = Math.floor(elementIncreasedHeight/2)*2+1;
    }
    else {
        elementIncreasedHeight = Math.floor(elementIncreasedHeight/2)*2
    }
    elementOrigWidth = element.width();
    elementIncreasedWidth = Math.round(elementOrigWidth*(1+percent/100));
    if (elementOrigWidth % 2 > 0) { // odd
        elementIncreasedWidth = Math.floor(elementIncreasedWidth/2)*2+1;
    }
    else {
        elementIncreasedWidth = Math.floor(elementIncreasedWidth/2)*2;
    }
    elementNewLeft = -Math.round((elementIncreasedWidth - elementOrigWidth)/2);
    elementNewTop = -Math.round((elementIncreasedHeight - elementOrigHeight)/2);
}

Ok, so after some (more!) search on the net, I will propose one thing that I was already pondering about (having a little experience mostly with OOP languages Java and C#).
creating a class/datastructure to hold the values
I was inspired by these sources:

answer to this question
answer nr 2 to this question
and this webmonkey explanation

still not sure if this is a "better" approach in JavaScript, but at least for my rather inexperienced eyes it looks more structured
the refactored version of my pulsating object is here
the slightly changed javascript (find the class ElementWrapper at the end):

$(document).ready(function () {
    var element = $('#circle');
    var growInPercent = 100;
    var elementWrapper = new ElementWrapper(element, growInPercent);

    var durationGetBigger = 500;
    var durationGetSmaller = 100;
    var frequency = 3;
    var pause = 3000;
    pulsate(elementWrapper, durationGetBigger, durationGetSmaller,
    frequency, pause);
});

function pulsate(elementWrapper, durationBigger, durationSmaller, frequency, pause) {
    for (var i = 0; i < frequency; i++) {
        getBigger(elementWrapper, durationBigger);
        getSmaller(elementWrapper, durationSmaller);
    }
    setTimeout(function () {
        pulsate(elementWrapper, durationBigger, durationSmaller, frequency, pause)
    }, pause);
}

function getBigger(elementWrapper, duration) {
    elementWrapper.element.animate({
        width: elementWrapper.increasedWidth,
        height: elementWrapper.increasedHeight,
        top: elementWrapper.newTop,
        left: elementWrapper.newLeft,
        opacity: 1
    }, duration, 'linear');
}

function getSmaller(elementWrapper, duration) {
    elementWrapper.element.animate({
        width: elementWrapper.originalWidth,
        height: elementWrapper.originalHeight,
        top: 0,
        left: 0,
        opacity: 0.5
    }, duration, 'linear');
}

function ElementWrapper(element, percent) {
    this.element = element;
    this.originalHeight = element.height();
    this.originalWidth = element.width();
    this.increasedHeight = getIncreasedValue(this.originalHeight, percent);
    this.increasedWidth = getIncreasedValue(this.originalWidth, percent);
    this.newLeft = -Math.round((this.increasedWidth - this.originalWidth) / 2);
    this.newTop = -Math.round((this.increasedHeight - this.originalHeight) / 2);

    function getIncreasedValue(originalValue, percent) {
        var increase = Math.round(originalValue * (1 + percent / 100));
        if (originalValue % 2 > 0) { // odd
            // make sure increased size is odd too
            return Math.floor(increase / 2) * 2 + 1;
        }
        else {
            return Math.floor(increase / 2) * 2;
        }
    }
}


Wildcard pattern-matching algorithm I was trying to write code which does a validation for the below wildcards:

  '?'  ------> The question mark indicates there is zero or one of the preceding
  element. For example, colou?r matches both "color" and "colour".
  '*'  ------>   The asterisk indicates there is zero or more of the preceding element. For example, ab*c matches "ac", "abc", "abbc", "abbbc", and
  so on.
  '+'  ------>  The plus sign indicates there is one or more of the preceding element. For example, ab+c matches "abc", "abbc", "abbbc", and so on,
  but not "ac".

I know that Java does provide pattern matching with wild cards out of box, but this is more of an exercise to understand the crux by not using libraries.

static boolean matches(String pattern, String text) {
    if (text == null && pattern == null) {
        return true;
    }
    if (text == null || pattern == null) {
        return false;
    }
    if (text.equals(EMPTY_STRING) && pattern.equals(EMPTY_STRING)) {
        return true;
    }
    if (text.equals(EMPTY_STRING) || pattern.equals(EMPTY_STRING)) {
        return false;
    }
    char[] p = pattern.toCharArray();
    char[] t = text.toCharArray();
    int indexP1 = 0, indexP2 = 1, indexT = 0;
    while (true) {
        if (indexP1 == p.length && indexT == t.length){
            return true;
        } else if (indexP1 == p.length || indexT == t.length){
            return false;
        } else {
            if (indexP2 < p.length && p[indexP2] == '*'){//case: a*
                while (t[indexT] == p[indexP1]) {
                    indexT++;
                    if(indexT==t.length)break;
                }
                indexP1 = indexP1 + 2;
                indexP2 = indexP2 + 2;
            }
            else if (indexP2 < p.length && p[indexP2] == '+') {//case: a+
                if(t[indexT] != p[indexP1]){
                    return false;
                }
                while (t[indexT] == p[indexP1]) {
                    indexT++;
                    if(indexT==t.length)break;
                }
                indexP1 = indexP1 + 2;
                indexP2 = indexP2 + 2;
            }
            else if (indexP2 < p.length && p[indexP2] == '?') {//case: a?
                if (t[indexT] == p[indexP1]) {
                    indexT++;
                }
                indexP1 += 2;
                indexP2 += 2;
            } else {//case: a
                if (t[indexT] != p[indexP1]) {
                    return false;
                }
                indexP1++;
                indexP2++;
                indexT++;
            }
        }
    }
}

Right away, I see two bugs:

First, it is not valid to say

if (text.equals(EMPTY_STRING) || pattern.equals(EMPTY_STRING)) {
    return false;
}

… because a non-empty pattern can certainly match an empty string.

Second, it takes a state machine to do regular expression matching properly; all I saw in your code were a few pointers.  (What are indexP1, indexP2, and indexT supposed to stand for?  Is there some invariant stateabout them that you could write out in a comment?)  It seems that you are matching * and + greedily.  That can't be right; sometimes you might need to backtrack and consider another possibility in order to get a match.

I didn't analyze the code too deeply, but those two serious concerns suggest that you will need to work at this code a lot more anyway.


string-length, structs, pointers in c So since I'm just new with learning C (this is the second day now), I'd be very happy if someone could review my code:

what I tried to do is:

create a structure containing a string- and a length-property
I want to create a function, that checks for the length of the string and sets the length property to the result
check, whether the result is correct with comparing to strlen of the original string
print the length of the string

This all works so far. Do you have any hints, what I could do better (or is ugly), what is wrong or where are any risks in my code?

this is my "helloworld.h" header file:

#include <stdio.h>
#include <stdlib.h>

struct SomeStringStruct
{
    char* actual_string;
    int length;
};
void calculate_length(struct SomeStringStruct*);

int get_string_length(char*);

and this is my "helloworld.c" source file:

#include <stdio.h>
#include <stdlib.h>
#include "helloworld.h"

int main(int argc, char* argv[])
{
    char string[] = "This is a test";
    char* ptrString = string;

    printf("strlen ptrString: %lu\n", strlen(ptrString));
    printf("get_string_length ptrString: %lu\n", get_string_length(ptrString));

    return 0;
}

void calculate_length(struct SomeStringStruct* ptrAnyStringStruct)
{
    ptrAnyStringStruct->length = 0;

    while(ptrAnyStringStruct->actual_string[ptrAnyStringStruct->length] != '\0')
        ptrAnyStringStruct->length++;
}

int get_string_length(char* string)
{
    struct SomeStringStruct myString;
    myString.actual_string = string;

    calculate_length(&myString);

    return myString.length;
};

Don't use Hungarian notation

The compiler doesn't need it and doesn't check it, and for a human it is all too easy to accidentily add the wrong prefix and get confused later. So no ptr in front of pointer variables.

Array variables can be used as pointers.

In the following code:

char string[] = "This is a test";
char* ptrString = string;

You don't need the second variable at all. Just use string directly.

Include the right header files

The function strlen() is officially defined in <string.h>, so make sure to #include that at the top of your program. Sometimes your program works, maybe even without compiler warnings, but you cannot assume that it will keep working in the future or that you can port it to other systems if you don't.

Don't include headers you don't use

You include headers at the top of helloworld.h that you are not using in that file. You should remove them.

Protect your header files from multiple inclusion

When you have a larger project, it can happen that your own header files need to #include some of your other header files. It can then become unavoidable that a header files inadvertantly gets included twice. To prevent any errors from happening, use the following trick to allow a header file to be included multiple times:

#ifndef __HELLOWORLD_H__
#define __HELLOWORLD_H__
...the real contents of helloworld.h here...
#endif

Use better names

Variables and structs should have names that are clear and to the point. Naming something SomeStringStruct is not to the point, it sounds vague and handwavy. Choose something better. The same goes for ptrAnyStringStruct.

Use size_t for lengths and sizes

The proper type to store the length of a buffer or a string is size_t. If you read the manual page of strlen(), you will also notice that it returns a value of type size_t.

Use the correct format specifier for size_t

Use %zu instead of %lu when you are printing a variable of type size_t.
If %zu is not supported by your compiler, then cast the variable to the right type when printing it; for example with %lu, cast it to long unsigned int:

printf("string length: %lu\n", (long unsigned int)strlen(string));

Be aware though that on some systems (notably 64-bits Windows), long int can be smaller than size_t, and if you would ever have a string longer than can be represented in a long int, the output will be incorrect.


Logfile parser for Team Fortress Classic video game I'm currently working on a logfile parser for a pretty old videogame called Team Fortress Classic, which can be compared to Counter-Strike 1.6 and is available on Valve's Steam platform.

I started out trying to make a collection of nicknames for each player. Every player joining the server has a unique ID, the STEAM_ID.
Once a player joins the server I can parse the ID off the logfile:

L 07/08/2013 - 19:25:40: "Marcus<47><STEAM_0:1:111><>" entered the game

So the STEAM_ID for the joining player would be STEAM_0:1:111, for the database I strip off the static prefix STEAM_ and only add 0:1:111 to the steam_id field in the database to keep the size smaller.

Now a player can change his in-game nickname from let's say Marcus to sucraM. I thought about making a one-to-many relationship between the tables players and aliases, adding the nickname the player was first seen with to the players table as a primary nickname to easier keep track of the player (later that would be useful i.e. when a known player joins the server with a new nickname I could print a line "new_nickname is also known as primary_nickname" to tell everyone who they are actually dealing with) and using a foreign key field player_id_fk in the aliases-table to be able to add several aliases for one players.id.

The database structure I created looks like the following;

players table:

CREATE TABLE players (
    id SMALLINT UNSIGNED AUTO_INCREMENT PRIMARY KEY,
    steam_id VARCHAR(15) UNIQUE,
    name VARCHAR(15)
) ENGINE = 'InnoDB';

aliases table:

CREATE TABLE aliases (
    id INT UNSIGNED AUTO_INCREMENT PRIMARY KEY,
    player_id_fk SMALLINT UNSIGNED,
    name VARCHAR(31),
    FOREIGN KEY (player_id_fk) REFERENCES players(id) ON UPDATE CASCADE ON DELETE CASCADE
) ENGINE = 'InnoDB';

Populating the tables with dummy data:

INSERT INTO players (id, steam_id, name) VALUES
    (NULL, '0:1:111', 'Marcus'),
    (NULL, '0:1:222', 'Benjamin');
INSERT INTO aliases (id, player_id_fk, name) VALUES
    (NULL, (SELECT id FROM players WHERE steam_id = '0:1:111'), 'Marcus The Great'),
    (NULL, (SELECT id FROM players WHERE steam_id = '0:1:111'), 'sucraM'),
    (NULL, (SELECT id FROM players WHERE steam_id = '0:1:222'), 'Benjamin The Weirdo');

Finding a player by steam_id and return his primary nick and known aliases:

SELECT players.steam_id, players.name, group_concat(aliases.name separator ', ') AS aliases
FROM players
INNER JOIN aliases
ON players.id = aliases.player_id_fk
WHERE players.steam_id = '0:1:111'

Hence I'm a novice to relational databases I have a few questions:

Am I using one-to-many the right way?
Can the layout be improved in any form?
If I'm going to print all known player names and their aliases to a website I might have to split aliases (group_concat() part in SELECT query) by separator ", " - now if a player is using the alias/name "Hello, its me!" this will be a problem as the alias/primary nickname will be split into "Hello" and "its me!".

Of course I could use a separator that is very unlikely to used by any player in their nicknames but I consider that method pretty unclean. I'm sure there has to be a query which would print steam_id, primary nick (players.name) and all the players' aliases so I can just iterate through them, I just can't figure out how to achieve this.

Any suggestions?

Your 3rd question is off-topic for the site, but I do believe that you are using the one-to-many relationships correctly.

When you are finding a player by steam_id you don't actually need to select the steam_id unless you are going to use it for something in an application.  This will speed up the query (probably not enough to be visible though).

Another thing, and this is just a thought because I am not positive on how this works:

In your players table you could change the primary key to be the Steam ID since that is always going to be unique and you are always going to give that as a value on the insertion of a record.

On the same note, you can change the foreign key of the aliases table to be the Steam ID as well.

Still add the Constraint of NOT NULL, Primary Key And Foreign Key as appropriate.


Finding mongodb records in batches using mongoid I want to get records in batches. But mongoid doc says to avoid using skip due as it can be expensive. I wrote this method to iterate through a large number records efficiently.

module Mongoid
  module Batches
    def find_each(batch_size = 1000)
      return to_enum(:find_each, batch_size) unless block_given?
      find_in_batches(batch_size) do |documents|
       documents.each { |document| yield document }
     end
   end
   def find_in_batches(batch_size = 1000)
     return to_enum(:find_in_batches, batch_size) unless block_given?
     documents = self.asc(:created_at).limit(batch_size).asc(:id).to_a
     while documents.any?
       documents_size = documents.size
       primary_key_offset = documents.last.id

       yield documents
       break if documents_size < batch_size
       documents = self.where(:id.gt => primary_key_offset).asc(:created_at).limit(batch_size).asc(:id).to_a
     end
   end
 end
end
Mongoid::Criteria.include Mongoid::Batches

Actually there is no need to take this way since mongodb cursor by defaults returns batchwise


Rock, Paper, Scissors, Lizard and Spock - Relational Paradigm I found a few question about the game and IMHO it's a relational problem and better addressed by set logic. Thus I created this solution in SQL to get away from ugly JavaScript, elegant java and practical C#

--drop table [Hand]
--drop table [Action]

create table [Hand]
(
     Id int primary key not null
    ,Name varchar(20) not null
)

create table [Action]
(
     WinsId int not null
    ,LosesId int not null
    ,ActionText varchar(100) not null
)
GO

alter table [Action] with nocheck add constraint fk_ActionhandWins foreign key(WinsId) references [Hand] (Id)
alter table [Action] with nocheck add constraint fk_ActionhandLoses foreign key(LosesId) references [Hand] (Id)
GO

insert into [Hand] (Id, Name)
values
 (1, 'Rock')
,(2, 'Paper')
,(3, 'Scissors')
,(4, 'Lizard')
,(5, 'Spock')

insert into [Action] (winsId, ActionText, LosesId)
values
  (1, 'Crushes', 3)
 ,(1, 'Crushes', 4)
 ,(2, 'Covers', 1)
 ,(2, 'Disproves', 5)
 ,(3, 'Cuts', 2)
 ,(3, 'Beheads', 4)
 ,(4, 'Eats', 2)
 ,(4, 'Poisons', 5)
 ,(5, 'Vaporizes', 1)
 ,(5, 'Distrois', 3)

 GO

 -- All possiblities
 select w.Name + ' ' + a.ActionText + ' ' + l.Name
 from [Action] a
 join [Hand] w on w.Id = a.WinsId
 join [Hand] l on l.Id = a.LosesId

GO
--drop function dbo.RockPaperScissorsLizardSpock
create function dbo.RockPaperScissorsLizardSpock(@Player1 int, @Player2 int)
returns @Result table(Result varchar(200), [Text] varchar(200))
as begin

    insert into @Result
             select 'Player 1 Wins!' as Result, w.Name + ' ' + a.ActionText + ' ' + l.Name as [Text]
             from [Action] a
             join [Hand] w on w.Id = a.WinsId
             join [Hand] l on l.Id = a.LosesId
             where w.Id = @Player1
             and l.Id = @Player2
             UNION
             select 'Player 2 Wins!' as Result, w.Name + ' ' + a.ActionText + ' ' + l.Name as [Text]
             from [Action] a
             join [Hand] w on w.Id = a.WinsId
             join [Hand] l on l.Id = a.LosesId
             where w.Id = @Player2
             and l.Id = @Player1

    if not exists(select * from @Result)
    insert into @Result values ('Tie!','')

    return

end;
GO
-- Testing cases
select * from dbo.RockPaperScissorsLizardSpock(1,2)
select * from dbo.RockPaperScissorsLizardSpock(2,1)
select * from dbo.RockPaperScissorsLizardSpock(1,1)
GO

Advantages of this approach are it doesn't require any implementation to add more "hands" just keep populating the two tables.

Are there any disadvantages of this approach compared to using OOP?

Validate actions

The Action table could use some validation logic:
WinsId and LosesId should be unique together.
You could enforce this with a unique constraint.

But there's more for the game to make sense:

WinsId should not be equal to LosesId
Each WinsId and LosesId should have the same count.
That is, each action should beat and be beaten by the same number of other actions.

Although these conditions are not possible to enforce with constraints,
it would be good to add a helper function that can be used to perform a sanity check easily.

Handling ties

Instead of this:

INSERT INTO @Result
SELECT 'Player 1 Wins!', ...
UNION
SELECT 'Player 2 Wins!', ...

IF NOT EXISTS(SELECT * FROM @Result)
INSERT INTO @Result VALUES ('Tie!','')

I don't have an SQL Server with me to test,
but I think this simpler alternative or similar should work:

INSERT INTO @Result
SELECT 'Player 1 Wins!', ...
UNION
SELECT 'Player 2 Wins!', ...
UNION
SELECT 'Tie', '' WHERE @Player1 = @Player2

Highlight SQL keywords

The common practice is to capitalize SQL keywords, like this:

CREATE TABLE [Hand]
(
     Id INT PRIMARY KEY NOT NULL
    ,Name VARCHAR(20) NOT NULL
)

Use indentation effectively

Good indentation goes a long way to helping readers understand the code.
A block of code like this, overindented and practically flat at the same,
is not really helpful:

insert into @Result
         select 'Player 1 Wins!' as Result, w.Name + ' ' + a.ActionText + ' ' + l.Name as [Text]
         from [Action] a
         join [Hand] w on w.Id = a.WinsId
         join [Hand] l on l.Id = a.LosesId
         where w.Id = @Player1
         and l.Id = @Player2
         UNION
         select 'Player 2 Wins!' as Result, w.Name + ' ' + a.ActionText + ' ' + l.Name as [Text]
         from [Action] a
         join [Hand] w on w.Id = a.WinsId
         join [Hand] l on l.Id = a.LosesId
         where w.Id = @Player2
         and l.Id = @Player1

It would be better this way:

INSERT INTO @Result
SELECT 'Player 1 Wins!' AS Result, w.Name + ' ' + a.ActionText + ' ' + l.Name AS [Text]
    FROM [Action] a
        JOIN [Hand] w ON w.Id = a.WinsId
        JOIN [Hand] l ON l.Id = a.LosesId
    WHERE w.Id = @Player1 AND l.Id = @Player2
UNION
SELECT 'Player 2 Wins!' AS Result, w.Name + ' ' + a.ActionText + ' ' + l.Name AS [Text]
    FROM [Action] a
        JOIN [Hand] w ON w.Id = a.WinsId
        JOIN [Hand] l ON l.Id = a.LosesId
    WHERE w.Id = @Player2 AND l.Id = @Player1

Naming

Single letter table aliases like w, l, a are not very helpful.
It would help understanding this code if you spelled out winner, loser, action.


Showing different forms for user type with Django/Python in views.py I have 4 user account types, realtor, insurance broker, auto insurance agent, and mortgage broker. Each one of the users will have a different form for each to submit. I have the following in views.py to render the form:

def _get_form(profile, postdata=None):
    # @TODO - come up with a better solution to render forms on user types than this.
    form = None

    if profile.affiliate_type == UserProfile.INSURANCE_BROKER:
        if postdata:
            form = InsuranceBrokerForm(postdata, instance=profile)
        else:
            form = InsuranceBrokerForm(instance=profile)
    elif profile.affiliate_type == UserProfile.AUTO_DEALERSHIP:
        if postdata:
            form = AutoDealershipForm(postdata, instance=profile)
        else:
            form = AutoDealershipForm(instance=profile)
        form = AutoDealershipForm
    elif profile.affiliate_type == UserProfile.MORTGAGE_BROKER:
        if postdata:
            form = MortgageBrokerForm(postdata, instance=profile)
        else:
            form = MortgageBrokerForm(instance=profile)
    elif profile.affiliate_type == UserProfile.REALTOR:
        if postdata:
            form = RealtorForm(postdata, instance=profile)
        else:
            form = RealtorForm(instance=profile)
    return form

and it's being used like so:

@login_required
def affiliate_form(request, template_name="affiliates/affiliate_form.html"):
    user = request.user
    profile = get_object_or_404(UserProfile, user=user)
    form = _get_form(profile)
    if profile.user != user:
        return HttpResponseForbidden("Forbidden. Please try again.")
    if request.method == 'POST':
        postdata = request.POST.copy()
        form = _get_form(postdata)
        if form.is_valid():
            form.save()
            messages.success(request, 'Your lead has been created.')
            url = urlresolvers.reverse('affiliates')
            return redirect(url)
    else:
        form = _get_form(profile)

    return render(request, template_name, {
        'form': form
    })

As you can see, I am calling _get_form(profile) but this seems dirty. Is there a cleaner way to go about this?

Dicts are good for lookups

When encountering stacked ifs that are all alike, I tend to pickup a dict and try to put it to good use.  One way to structure the variable parts of your routine is with something like:

user_form_types = {
    UserProfile.INSURANCE_BROKER: InsuranceBrokerForm,
    UserProfile.AUTO_DEALERSHIP: AutoDealershipForm,
    UserProfile.MORTGAGE_BROKER: MortgageBrokerForm,
    UserProfile.REALTOR: RealtorForm,
}

This one structure allows the variability in the routine to be described in a manner that makes it easy to read and understand that variability, and easy to use the resulting data.

Using data as code

Since Python makes it easy to reference a class and then instantiate it, the dict can be used, and the routine can be boiled down to simply:

user_form_type = user_form_types.get(profile.affiliate_type)
if not user_form_type:
    form = None
else:
    if postdata:
        form = user_form_type(postdata, instance=profile)
    else:
        form = user_form_type(instance=profile)
return form

Entire Recast Routine

def _get_form(profile, postdata=None):

    user_types = {
        UserProfile.INSURANCE_BROKER: InsuranceBrokerForm,
        UserProfile.AUTO_DEALERSHIP: AutoDealershipForm,
        UserProfile.MORTGAGE_BROKER: MortgageBrokerForm,
        UserProfile.REALTOR: RealtorForm,
    }

    user_form_type = user_types.get(profile.affiliate_type)
    if not user_form_type:
        form = None
    else:
        if postdata:
            form = user_form_type(postdata, instance=profile)
        else:
            form = user_form_type(instance=profile)
    return form


Extracting coordinates file that are inside a bounding box I'm scanning a CSV file to see which values fit inside of a given box. The CSV file has X and Y coordinates, and the arguments to the function are the perimeters of a box (i.e, top, bottom, left side, right side). I don't want to use any external libraries; part of the purpose of this is to learn how to speed up code manually.

I'm fairly new to Java, so any input would be helpful. I'm trying to get this down to 100ms if possible, but right now it's taking about 1000ms.

Scanner dataStream = new Scanner(fileMain);
while (dataStream.hasNext()){
    String data = dataStream.next();
    String[] values = data.split(",", 3);
    if(Double.parseDouble(values[0]) > lowX && Double.parseDouble(values[0]) < highX){
            if(Double.parseDouble(values[1]) < highY && Double.parseDouble(values[1]) > lowY){
                System.out.println(data);

    }

dataStream.close();

Data is formatted like so:

90.92, 102.3
40.28, 82.32
13.02, 80
72, 104.82

First of all, note that you can always use Point2D.Double for representing points in a plane.

Next, I would split your program into two methods: the first one for pruning points that are not within a specified bounding box, and the second one for reading the points from the standard input, and putting them into the first one.

What comes to coding conventions, I would have a blank line before the if statement. Also, you should surround the condition of an if statement with a single space. So, instead of

String[] values = data.split(",", 3);
if(Double.parseDouble(values[0])){ ...

you should write

String[] values = data.split(",", 3);

if (Double.parseDouble(values[0])) { ...

All in all, I had this in mind:

import java.awt.geom.Point2D;
import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
import java.util.Scanner;

public class CoordinateUtilities {

    public static List<Point2D.Double>
        getPointsWithinBoundingBox(final List<Point2D.Double> pointList,
                                   final double lowX,
                                   final double highX,
                                   final double lowY,
                                   final double highY) {
        Objects.requireNonNull(pointList, "The list of points is null.");
        checkXBounds(lowX, highX);
        checkYBounds(lowY, highY);
        final List<Point2D.Double> ret = new ArrayList<>(pointList.size());

        for (final Point2D.Double point : pointList) {
            final double x = point.x;
            final double y = point.y;

            if (lowX <= x && x <= highX && lowY <= y && y <= highY) {
                ret.add(point);
            }
        }

        return ret;
    }

    public static void main(final String... args) {
        final Scanner scanner = new Scanner(System.in);
        final double lowX  = 50.0;
        final double highX = 100.0;
        final double lowY  = 30.0;
        final double highY = 60.0;

        final List<Point2D.Double> allPoints = new ArrayList<>();

        while (scanner.hasNextLine()) {
            final String line = scanner.nextLine();

            if (line.trim().equals("quit")) {
                break;
            }

            final Point2D.Double point = parsePoint(line);

            if (point != null) {
                allPoints.add(point);
            }
        }

        final List<Point2D.Double> boundedPoints =
                getPointsWithinBoundingBox(allPoints, lowX, highX, lowY, highY);

        boundedPoints.forEach(System.out::println);
    }

    private static Point2D.Double parsePoint(final String line) {
        final String[] tokens = line.trim().split("\\s*,\\s*");

        if (tokens.length < 2) {
            return null;
        }

        try {
            final double x = Double.parseDouble(tokens[0]);
            final double y = Double.parseDouble(tokens[1]);
            return new Point2D.Double(x, y);
        } catch (final NumberFormatException ex) {
            return null;
        }
    }

    private static void checkXBounds(final double lowX, final double highX) {
        if (Double.isNaN(lowX)) {
            throw new IllegalArgumentException(
                    "The lower bound of the x-coordinate is NaN.");
        }

        if (Double.isNaN(highX)) {
            throw new IllegalArgumentException(
                    "The upper bound of the x-coordinate is NaN.");
        }

        if (lowX > highX) {
            throw new IllegalArgumentException(
                    "The lower bound of the x-coordinate (" + lowX + "is " +
                    "larger than the upper bound of the x-coordinate (" +
                    highX + ").");
        }
    }

    private static void checkYBounds(final double lowY, final double highY) {
        if (Double.isNaN(lowY)) {
            throw new IllegalArgumentException(
                    "The lower bound of the y-coordinate is NaN.");
        }

        if (Double.isNaN(highY)) {
            throw new IllegalArgumentException(
                    "The upper bound of the y-coordinate is NaN.");
        }

        if (lowY > highY) {
            throw new IllegalArgumentException(
                    "The lower bound of the y-coordinate (" + lowY + "is " +
                    "larger than the upper bound of the y-coordinate (" +
                    highY + ").");
        }
    }
}

Hope that helps.


Detecting connected acyclic through Tarjan strongly connected components variant I want to verify that a given directed graph is connected and acyclic (DAG). I have implemented a modification of the Tarjan's strongly connected components algorithm in imperative style (as the original).

Every suggestion to improve simplicity, readability and suitable use of F# constructs will be wellcome.

Note: the current algorithm returns the value for acyclic property for the first cluster in disconnected graphs. I'm ok with that. Once I know it is not DAG I no need to analyze it further. But with this in mind I suppose the algorithm could be improved.

open System.Collections.Generic

type GraphKind = {
        acyclic : bool;
        connected : bool
    }

 let kindOfGraph getChildren v numNodes =

    let stack = Stack<_>()
    let visited = Dictionary<_,int * int>(HashIdentity.Structural)
    let discovered = HashSet<_>(HashIdentity.Structural)
    let sccs = ref 0
    let order = ref 0

    let rec scc getChildren v =

        stack.Push (v)
        visited.Add(v, (!order, !order))
        incr order

        let vindex, vlink = visited.[v]

        getChildren v
        |> List.map (fun w ->
            if not <| visited.ContainsKey w then
                scc getChildren w
                visited.[v] <- (vindex, min vlink (snd visited.[w]))
            else if not <| discovered.Contains w then
                visited.[v] <- (vindex,min vlink (fst visited.[w]))
        ) |> ignore

        let idx, lnk = visited.[v]

        if idx = lnk then

            let rec getscc (s : Stack<_>) root =
                let w = s.Pop()
                discovered.Add w |> ignore
                match w with
                | x when x = root -> ()
                | _ -> getscc s root

            getscc stack v
            incr sccs

    scc getChildren v
    {
        acyclic = (!sccs = !order);
        connected = (!order = numNodes)
    }

And some tests using FsUnit and NUnit

open NUnit.Framework
open FsUnit

[<TestFixture>]
type ``kind of graph`` () =

let aTree = function
| 1 -> [2; 3]
| 2 -> [4; 5]
| _ -> []

let aTriangle = function
| 1 -> [2]
| 2 -> [3]
| 3 -> [1]

let aConnectedAcyclic = function
| 1 -> [2; 3]
| 2 -> [4; 5; 6]
| 4 -> [7]
| 6 -> [8; 9]
| 3 -> [10; 11; 13]
| 10 -> [9; 12]
| 11 -> [13]
| _ -> []

let aDisconnectedAcyclic = function
| 1 -> [2; 3]
| 4 -> [5]
| _ -> []

let aDisconnectedCyclic = function
| 1 -> [2]
| 2 -> [3]
| 3 -> [1]
| 4 -> [5]
| _ -> []

let aSingleNode = function | _ -> []

[<Test>] member x.
    ``given a tree`` () =
        kindOfGraph aTree 1 5 |> should equal { acyclic = true; connected = true}

[<Test>] member x.
    ``given a connected triangle`` () =
        kindOfGraph aTriangle 1 3 |> should equal { acyclic = false; connected = true}

[<Test>] member x.
    ``given a connected acyclic graph`` () =
        kindOfGraph aConnectedAcyclic 1 13 |> should equal { acyclic = true; connected = true}

[<Test>] member x.
    ``given a single node`` () =
        kindOfGraph aSingleNode 1 1 |> should equal { acyclic = true; connected = true}

[<Test>] member x.
    ``given a disconnected acyclic graph`` () =
        kindOfGraph aDisconnectedAcyclic 1 5 |> should equal { acyclic = true; connected = false}

 [<Test>] member x.
    ``given a disconnected cyclic graph`` () =
        kindOfGraph aDisconnectedCyclic 1 5 |> should equal { acyclic = false; connected = false}

I don't think graph algorithms work well with functional programming: you have to track which nodes were already visited and that pretty much requires global mutable state. You can simulate mutable state in a purely functional way by adding a parameter and a return value to the graph-walking function, but that quickly becomes tedious.

Because of that, I think that using imperative F#, or maybe even better, C# is a good choice here.

Your code does not tell you whether the graph is weakly connected, it tells you whether all nodes are reachable from the first node. (I'm assuming you're not interested in strongly connected graphs, because that implies cycles.) For example, given:

let aLine = function
| 0 -> [1]
| 1 -> [2]
| 2 -> [3]
| 3 -> []

Then (kindOfGraph aLine 0 4).connected returns true, while (kindOfGraph aLine 1 4).connected returns false. I don't know whether this is a bug in the code or whether you're misusing the word "connected" to mean something different, I could imagine both being the case.

I don't like that you're representing the graph as a function (plus the first node and size). I think that graph should be a self contained data structure, maybe something like Map<int, int list>.

I don't understand why are you using Tarjan's algorithm here.

To find out whether a graph contains cycles, DFS is enough.

To find whether all nodes are reachable from a specific node, simplified DFS (that doesn't try other starting nodes) will work.

To find whether a graph is weakly connected is more complicated. I think the simplest way would be to convert it to an undirected graph (by adding the reverse of each edge) and then performing DFS on that.

Since you're not actually interested in the connected and acyclic properties separately and since you're aware that your code is not always accurate when it comes to being acyclic, I would return only a single boolean value, not a record with misleading field names.


Filling A Linked List With Data From File And Handling User Status Messages I have completed an university assignment on C. While the code is fully functional based on the specifications of the exercise, I like high-quality code and would like to ask for opinions on how can certain parts of it be improved. Be advised that I cannot change any function signature.

The first part is the below load() function whose job is to create and fill a linked list based on entries of a file. If the specified file doesn't exist then a new file must be created. Unfortunately, due to the structure of the signature, I am forced to fclose() the file (opening / closing files are expensive operations) as I don't have any way to store the file pointer.

I don't quite like the double fopen() call there. Also, while I've found a way to detect when a file is blank or when it has wrong format, I still have the feeling that there are cases where undefined behavior can occur.

studentList* load(char *filename)
{
    studentList *list = newList();

    FILE *f = fopen(filename, "r");
    if (f == NULL)
    {
        f = fopen(filename, "w");
        fclose(f);
        printf("File %s not found. A new blank file has been created.\n",filename);
        return list;
    }

    fseek(f, 0, SEEK_END);
    if (ftell(f) != 0)
    {
        rewind(f);
        while (!feof(f))
        {
            student st;
            int res = fscanf(f, "%d%s", &st.id, st.name);
            if (res == 2)
                add(list, st);
        }
        fflush(f);
        fclose(f);
    }
    printf("File %s loaded successfully.\n",filename);
    return list;
}

The second part is more generic and has to do with the way status messages are printed to the console for the user. Excluding load() where I don't have any other option due to the nature of the signature , the other functions that provide basic functionalities of a linked list like add() , delete() , find() etc. don't have any printf() or puts() as I consider that method a dirty code style and besides that, I want to have full control of when a status message is printed.

Instead, these functions return an status code int where then an other function takes that status message and prints the corresponding status message to the console. I have not found a better method to achieve this result. Any ideas in this part would be highly appreciated.

Example of my code:

int res = add(list, st);
printAddStudentResult(res);

The first thing that strikes me is that you're writing error and status messages to the standard output stream, when usually these go to stderr, the standard error stream.

I'm not sure it makes much sense to attempt to create an empty file if the file opening fails - why not leave this until it's time to save?  What if the reason it failed is because the file doesn't have read permission for the process?  And how do you know whether creating a new file succeeded or not?

I'd argue that it's better to return a null pointer if the file opening fails, so that client code can distinguish this from a successful load of an empty list:

if (f == NULL)
{
    /* NULL return signifies nothing could be read */
    return NULL;
}

studentList *list = newList();

I'm assuming newList() is an allocation function that returns a null pointer if the allocation fails; that means we shouldn't attempt to use it until we've checked it's a valid pointer.

while (!eof) is a common anti-pattern; what we want to do instead is more like while (fscanf(...)==2) - and that removes the need to measure the file size before reading the contents.

This line is clearly wrong

    fflush(f);

f is an input stream, so fflush(f) is Undefined Behaviour.  Just remove this call.


Father-son project: Drawing on a grid I made this bit of code that uses jQuery in a simple HTML file to see if my son would think it is fun/interesting and to help me with my jQuery skills. The thing that was hard was the selectors towards the bottom of the script block.  I am wondering if there is a better way to do this.  Warning: it auto-refreshes to 're-pixel' the screen (for lack of a better word).

Also I am building tables but I am sure they are not right, it was my intention to grab the innerwidth and innerheight and create a table that is divided into fractional squares that perfectly fill the view-able browser screen.  I had trouble getting the logic right.

Also, I had to place a span tag in side the  blocks so the wouldn't collapse on me.  I wanted to do this in divs but I wasn't sure how to get the same effect.

$(document).ready(function () {
// find random colors
function colorMe() {
    var newColor = '#' + (0x1000000 + (Math.random()) * 0xffffff).toString(16).substr(1, 6);
    return newColor;
}
// not sure about this but i got it to work in some wierd way.
var wb = Math.floor(window.innerWidth / 200);
var hb = Math.floor(window.innerHeight / 200);

// table buildout append cells and rows to the table.
// Note the yucky span tag to prevent td collapse
var table = $('

');
for (i = 0; i < 100; i++) {
    row = $('

');
    for (j = 0; j < 200; j++) {
        var row1 = $('	').html('

');
        table.append(row);
        row.append(row1);
    }
}
// table to body append
$('body').append(table);

// mouse event to emulate a pen commenting out the last two give it a fun calligraphy look
$("td").hover(function () {
    var whoIsMe = $(this).index();

    $(this).stop()
        .animate({
        "opacity": "0"
    }, "fast");

    $(this).next("td")
        .stop()
        .animate({
        "opacity": "0"
    }, "fast");

    $(this).prev("td")
        .stop()
        .animate({
        "opacity": "0"
    }, "fast");

    $(this).parent()
        .next()
        .children("td")
        .eq(whoIsMe)
        .stop()
        .animate({
        "opacity": "0"
    }, "fast");

    $(this).parent()
        .prev()
        .children("td")
        .eq(whoIsMe)
        .stop()
        .animate({
        "opacity": "0"
    }, "fast");
});
// eventually reload for a fresh palette of colored squares
setTimeout(function () {
    window.location.reload(1);
}, 50000);
});

Here is a link to a jsfiddle.

To make them square:

var size = Math.max(window.innerWidth,window.innerHeight)/200;
var xbound = Math.floor(window.innerWidth/size);
var ybound = Math.floor(window.innerHeight/size);

Simply use xbound and ybound as the upper limits in the for loops.

Try to use CSS where you can.

Additionally, your script loads very slowly because of the excessive DOM manipulation. Rather than adding each element to the page one by one, create a single object and then paste that in one go. Just add up the html code with the += operator before inserting it into the page.


Is this correct way to setup DB Infrastructure with repository pattern and Dapper So, I am using Repository pattern with Dapper and ADO.NET. I have my based DB infrastructure setup as follows:
public class ConnectionFactory: IConnectionFactory
{
    private readonly string dbConnectionString = Convert.ToString(ConfigurationManager.ConnectionStrings["MyDBConn"].ConnectionString);
    private MySqlConnection _dbConnection;
    public IDbConnection GetDBConnection
    {
        get
        {
            if(string.IsNullOrEmpty(dbConnectionString))
            {
                throw new ArgumentNullException("DB Connection string received a null argument!");
            }

            if (_dbConnection == null)
            {
                _dbConnection = new MySqlConnection(dbConnectionString);
            }

            if (_dbConnection.State != ConnectionState.Open)
            {
                _dbConnection.Open();
            }

            return _dbConnection;
        }
    }

    public void CloseDBConnection()
    {
        if (_dbConnection != null && _dbConnection.State == ConnectionState.Open)
        {
            _dbConnection.Close();
        }
    }
}

And I have a Repository class that uses this infrastructure:
public async Task<IEnumerable<UserInformation>> GetUserInformation()
{
    IEnumerable<UserInformation> list;
    string querystring = "SELECT * FROM users WHERE valid=1;";

    try
    {
        list = await SqlMapper.QueryAsync<UserInformation>(_connectionFactory.GetDBConnection, querystring, param, commandType: CommandType.Text);
    }
    finally
    {
        _connectionFactory.CloseDBConnection();
    }

    return list;
}

For reading my IReader and converting to a dataset, I do the following:
public async Task<DataSet> GetOrderDetails(int oid)
{
    DataSet dataset = new DataSet();
    string querystring = "select * from orders WHERE OrderId=@oid";
    DynamicParameters param = new DynamicParameters();
    param.Add("@oid", oid);

    using (var list = await SqlMapper.ExecuteReaderAsync(_connectionFactory.GetDBConnection, querystring, param, commandType: CommandType.Text))
    {
        dataset = helper.ConvertDataReaderToDataSet(list);
    }

    return dataset;
}

Here ConvertDataReaderToDataSet is a method that only converts the IReader to a Dataset.
The problem that I having is that I am getting the following error after a while (from logs):
2/10/2021 12:37:44 AM
Type: Exception
Error Message: error connecting: Timeout expired.  The timeout period elapsed prior to obtaining a connection from the pool.  This may have occurred because all pooled connections were in use and max pool size was reached.
Method Name: GetOrderDetails

I have researched this error and it suggests a memory leak. Can you guys help me identify what is the problem and review my DB infrastructure setup?
Edit: After a discussion, I have changed the implementation to this:
public async Task<IEnumerable<UserInformation>> GetUserInformation()
{
    IEnumerable<UserInformation> list;
    string querystring = "SELECT * FROM users WHERE valid=1;";

    using (var conn = new MySqlConnection(dbConnectionString))
    {
        list = await conn.QueryAsync<UserInformation>(querystring, commandType: CommandType.Text);
    }

    return list;
}

Quick remarks:

Why do you convert a string to a string: Convert.ToString(ConfigurationManager.ConnectionStrings["MyDBConn"].ConnectionString);? It's even called a ConnectionString.

Why do you use Dapper and then don't use it properly? Why return a DataSet? Why use SqlMapper and DynamicParameters? Why make life so hard for yourself when Dapper is making things so easy? Look at the homepage of Dapper Tutorial and Documentation:
 using (var conn = new SqlConnection(connectionString))
 {
     var invoices = conn.Query<Invoice>(sql);
 }

That is all you need. No endless checking of the state of your connection, no opening and closing it yourself, no try...finally blocks which completely ignore any exception that might occur. Write the correct query and let Dapper map the results to your class.


Random walk generator - looking for optimizations (Python) I just started learning Python and this is the first code I wrote. It's a Random Walk generator. I think I got it to work pretty well but I'm also sure there are more effective and less clunky ways to do this. Could you give me some tips on what to change to make the code shorter/more optimized and just objectively more "pythonian" and better?
# Random Walk in two dimensions

import random

steps = "a"
maximal = "a"
minimal = "a"
Xstep = 0
Ystep = 0
StepSize = 0
PosNeg = 0

def new_line():
    print(" ")

def invalid_enter_number():
    print("Invalid input! Enter a number.")
    new_line()

def invalid_enter_positive():
    print("Invalid input! Enter a positive number.")
    new_line()

new_line()
print("Random Walk in two dimensions")
new_line()

while type(steps) != int:
    try:
        steps = int(input("How many steps should be done? "))
        while steps <= 0:
            invalid_enter_positive()
            steps = int(input("How many steps should be done? "))
    except:
        steps = ValueError
        invalid_enter_number()
        continue

while type(maximal) != int:
    try:
        maximal = int(input("How big is the biggest possible step? "))
        while maximal <= 0:
            invalid_enter_positive()
            maximal = int(input("How big is the biggest possible step? "))
    except:
        maximal = ValueError
        invalid_enter_number()
        continue

if maximal != 1:
    while type(minimal) != int:
        try:
            minimal = int(input("How big is the smallest possible step? "))
            while minimal <= 0 or minimal >= maximal:
                if minimal <= 0:
                    invalid_enter_positive()
                    minimal = int(input("How big is the smallest possible step? "))
                else:
                    print("Number must be smaller than the biggest possible step!")
                    new_line()
                    minimal = int(input("How big is the smallest possible step? "))
        except:
            minimal = ValueError
            invalid_enter_number()
            continue
else:
    minimal = 1

new_line()
print("Number of steps:", steps)

if maximal != 1:
    print("Step size range:", minimal, "-", maximal)
else:
    print("Step size: 1")

new_line()
print("Steps:")

while steps > 0:
    StepSize = random.randint(minimal, maximal)
    PosNeg = random.randint(0, 1)
    chance = random.randint(0, 1)
    if chance == 0 and PosNeg == 0:
        Xstep += StepSize
    elif chance == 0 and PosNeg == 1:
        Xstep -= StepSize
    elif chance == 1 and PosNeg == 0:
        Ystep += StepSize
    else:
        Ystep -= StepSize
    print("X:", Xstep, " Y:", Ystep)
    steps -= 1

Here's one suggestion; other readers will probably offer more.
Several times, you need to get an integer from the user. You
could create a function to handle such details. The code
below shows what the function might look light and how you
would use it.
def int_from_user(prompt):
    while True:
        try:
            n = int(input(prompt + ' '))
            if n > 0:
                return n
            else:
                raise ValueError()
        except ValueError:
            print('Invalid input! Enter a positive number.')

steps = int_from_user('How many steps should be done?')
print(steps)