Win7 atomic access class I have a simple atomic access class designed to work with Windows threads:

template <class aa_type>
class AtomicAccess{
private:
    CRITICAL_SECTION lock;
    aa_type value;
public:
    AtomicAccess(){
        InitializeCriticalSection(&lock);
    }

    ~AtomicAccess(){
        DeleteCriticalSection(&lock);
    }

    AtomicAccess &operator=(const aa_type &val){
        EnterCriticalSection(&lock);
        value = val;
        LeaveCriticalSection(&lock);
        return *this;
    }

    AtomicAccess &operator++(int){
        EnterCriticalSection(&lock);
        value++;
        LeaveCriticalSection(&lock);
        return *this;
    }

    AtomicAccess &operator--(int){
        EnterCriticalSection(&lock);
        value--;
        LeaveCriticalSection(&lock);
        return *this;
    }

    bool operator==(const aa_type &val){
        aa_type current;
        EnterCriticalSection(&lock);
        current = value;
        LeaveCriticalSection(&lock);
        return current == val;
    }

    bool operator!=(const aa_type &val){
        aa_type current;
        EnterCriticalSection(&lock);
        current = value;
        LeaveCriticalSection(&lock);
        return current != val;
    }
};

typedef AtomicAccess<int> int_aa;

Right now I am using it to only atomic-access integers, and the only operators I need are = (for assignment), ++, --, == and !=. If I decide I need more operators, I will add them.

Testing race conditions is very tricky, so I would like to ask here if my approach will work or if I am missing something.

Konrad's answer has a big advantage: it is safe!
In your code sample, if an operator raise an exception before you leave the critical section, this one will never be released. In Konrad's answer, not matter what, the critical section will be unlocked when you exit the scope, even if you don't catch an exception.


Python small brute-forcer I have 20+ key set. Keyset is like:

Key[0] = { "PossibleKey0", "PossibleKey0Another", "AnotherPossibleKey0" }
Key[1] = { ....

There is an encryption which needs to be broken, it will receive 20+ keys, one after another. So it applies decryption one-by-one.

So key[0] has 3 different possible keys, same for key[1], key[2], etc.

I should try each key with its each possible keys. I decided to write it like:

for i in range(0, len(Key[0])):
    for j in range(0, len(Key[1])):
        for k in range(0, len(Key[2])):
            for l in range(0, len(Key[2])):
                Decrypt(Key[0][i], Key[1][j], Key[2][k], Key[3][l])

But it looks OK for small key sets, what's better approach when there will be 20+ keys?

In general, you can use itertools.product to factor out nested loops, and tuple unpacking with * to neatly deal with multiple arguments:

from itertools import product

for keys in product(*Key):
    Decrypt(*keys)

A simple example:

>>> from itertools import product
>>> keys = ["ab", "cd", "ef", "gh"]
>>> for k in product(*keys):
    print k

('a', 'c', 'e', 'g')
('a', 'c', 'e', 'h')
('a', 'c', 'f', 'g')
('a', 'c', 'f', 'h')
('a', 'd', 'e', 'g')
('a', 'd', 'e', 'h')
('a', 'd', 'f', 'g')
('a', 'd', 'f', 'h')
('b', 'c', 'e', 'g')
('b', 'c', 'e', 'h')
('b', 'c', 'f', 'g')
('b', 'c', 'f', 'h')
('b', 'd', 'e', 'g')
('b', 'd', 'e', 'h')
('b', 'd', 'f', 'g')
('b', 'd', 'f', 'h')

Note that for four sets of 20 keys, there would be 160,000 combinations (20 ** 4 == 160000).


Simple carbon emissions from fuel and fire calculations I would like to get the simple carbon emissions calculations below written in the most Pythonic / straightforward way.

FUEL_FACTORS = {'diesel': {'energy_factor': 38.6,
                           'co2_emission_factor': 69.9,
                           'ch4_emission_factor': 0.1,
                           'n2o_emission_factor': 0.5
                          },
                'petrol': {'energy_factor': 34.2,
                           'co2_emission_factor': 67.4,
                           'ch4_emission_factor': 0.5,
                           'n2o_emission_factor': 1.8
                           },
                'avgas':  {'energy_factor': 33.1,
                           'co2_emission_factor': 67.0,
                           'ch4_emission_factor': 0.05,
                           'n2o_emission_factor': 0.7}
                }

FIRE_FACTORS = {'gwp_co2': 1,
                'gwp_ch4': 25,
                'gwp_n2o': 298,
                'ch4_co2_mass_ratio': 7.182e-3,
                'n2o_co2_mass_ratio': 1.329e-5,
                }

def get_fuel_emission_factor(fuel):
    """Get fuel total CO2 emission factor."""
    factors = FUEL_FACTORS[fuel]
    energy_f = factors['energy_factor']
    fuel_emission_factor = 0
    for i in ['co2', 'ch4', 'n2o']:
        gas_emission_factor = factors[i + '_emission_factor']
        fuel_emission_factor += energy_f * gas_emission_factor * 1e-3
    return fuel_emission_factor

def fuel_to_co2(fuel, quantity):
    """Get total tCO2 emission by kL of fuel."""
    fuel_emission_factor = get_fuel_emission_factor(fuel)
    return fuel_emission_factor * quantity

def burned_c_to_co2(tC):
    """Get CH4 and N2O emissions in tCO2 from tonnes of C burned"""
    ch4_emissions = tC * FIRE_FACTORS['gwp_ch4'] * FIRE_FACTORS['ch4_co2_mass_ratio']
    n2o_emissions = tC * FIRE_FACTORS['gwp_n2o'] * FIRE_FACTORS['n2o_co2_mass_ratio']
    return ch4_emissions + n2o_emissions

fuel_emissions = fuel_to_co2('petrol', 2.7395)
fire_emissions = burned_c_to_co2(5.74)

Define a constant for greenhouse gases GHG to avoid hard-coding them in separate functions:

GHG = ('co2', 'ch4', 'n2o')  # greenhouse gases

get_fuel_emission_factor function.
The for loop with accumulation is concisely replaced with builtin sum function on generator expression:

def get_fuel_emission_factor(fuel):
    """Get fuel total CO2 emission factor."""
    factors = FUEL_FACTORS[fuel]
    return sum(factors['energy_factor'] * factors[f'{gas}_emission_factor'] / 1000
               for gas in GHG)

fuel_to_co2 function.
Inline variable technique can be applied:

def fuel_to_co2(fuel, quantity):
    """Get total tCO2 emission by kL of fuel."""
    return get_fuel_emission_factor(fuel) * quantity


A simple PHP login system I'm super new to PHP and I'm trying to get a functional login system together. This is the code I have:

class/user.php

<?php
 class Users {
     public $username = null;
     public $password = null;
     public $salt = "Zo4rU5Z1YyKJAASY0PT6EUg7BBYdlEhPaNLuxAwU8lqu1ElzHv0Ri7EM6irpx5w";

     public function __construct( $data = array() ) {
         if( isset( $data['username'] ) ) $this->username = stripslashes( strip_tags( $data['username'] ) );
         if( isset( $data['password'] ) ) $this->password = stripslashes( strip_tags( $data['password'] ) );
     }

     public function storeFormValues( $params ) {
        //store the parameters
        $this->__construct( $params );
     }

     public function userLogin() {
         $success = false;
         try{
            $con = new PDO( DB_DSN, DB_USERNAME, DB_PASSWORD );
            $con->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );
            $sql = "SELECT * FROM users WHERE username = :username AND password = :password LIMIT 1";

            $stmt = $con->prepare( $sql );
            $stmt->bindValue( "username", $this->username, PDO::PARAM_STR );
            $stmt->bindValue( "password", hash("sha256", $this->password . $this->salt), PDO::PARAM_STR );
            $stmt->execute();

            $valid = $stmt->fetchColumn();

            if( $valid ) {
                $success = true;
            }

            $con = null;
            return $success;
         }catch (PDOException $e) {
             echo $e->getMessage();
             return $success;
         }
     }

     public function register() {
        $correct = false;
            try {
                $con = new PDO( DB_DSN, DB_USERNAME, DB_PASSWORD );
                $con->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );
                $sql = "INSERT INTO users(username, password) VALUES(:username, :password)";

                $stmt = $con->prepare( $sql );
                $stmt->bindValue( "username", $this->username, PDO::PARAM_STR );
                $stmt->bindValue( "password", hash("sha256", $this->password . $this->salt), PDO::PARAM_STR );
                $stmt->execute();
                return "Registration Successful
 Login Now";
            }catch( PDOException $e ) {
                return $e->getMessage();
            }
     }

 }

?>

login.php

<?php
    include_once("config.php");
?>
<?php if( !(isset( $_POST['login'] ) ) ) { ?>
    <form method="post" action="">
        <input type="text" maxlength="30" required autofocus name="username" />
        <input type="password" maxlength="30" required name="password" />
        <input type="submit" name="login" value="Log me in" />
    </form>
<?php
} else {
    $usr = new Users;
    $usr->storeFormValues( $_POST );

    if( $usr->userLogin() ) {
        echo "Welcome";
    } else {
        echo "Incorrect Username/Password";
    }
}
?>

register.php

<?php
    include_once("config.php");
?>

<?php if( !(isset( $_POST['register'] ) ) ) { ?>
    <form method="post">
        <input type="text" id="usn" maxlength="30" required autofocus name="username" />
        <input type="password" id="passwd" maxlength="30" required name="password" />
        <input type="password" id="conpasswd" maxlength="30" required name="conpassword" />
        <input type="submit" name="register" value="Register" />
    </form>

<?php
} else {
    $usr = new Users;
    $usr->storeFormValues( $_POST );

    if( $_POST['password'] == $_POST['conpassword'] ) {
        echo $usr->register($_POST);
    } else {
        echo "Password and Confirm password not match";
    }
}
?>

I am really keen to know how this code is structured, as I used information from several different tutorials combined to create it. Any input would be appreciated.

Data Handling & Security

You use prepared statements, which protects you from SQL injection, which is good. However, the way you handle passwords could be improved to increase security.

It's not a good idea to semi-randomly apply sanitation functions to user input as it results in dirty data.

stripslashes for example really only makes sense when magic quotes is enabled, so you should check that first (and then only strip the slashes in some init file, not every time you access GET). For example, if a user has a password like this: a\'b\\c\'d, stripping slashes reduces the password length from 9 to 6 characters.

Same problem with strip_tags, only much worse. If my password is a<super$secure!password, now my password is a.

You also don't need strip_tags. As XSS protection it's not sufficient anyways (and definitely not needed for passwords, which aren't echoed); You should HTML-encode data when echoing it instead.

There's also really no need to have a length restriction on the password, especially not one that is shorter than what your hashing algorithm allows.

Hashing

Simple sha is really not good enough anymore, as it's too fast. Use bcrypt instead.

Error Handling

Don't echo database error messages directly to the enduser. They won't know what to do with it, and it may provide attackers with information or enable some forms of attacks (such as error based SQL injection).

It's also not a good idea to echo anything in a function, as side-effects like that make it harder to reuse.

Structure

I think your structure is pretty clean, and your code easy to read. Just a couple of small points:

storeFormValues doesn't seem to have any purpose. I wouldn't call the constructor inside a function of the same class, but just use it directly.

I would also not pass a magic array to the constructor, as it's hard to use and reuse (I have to guess how the array must be build and what it must contain, and I have to use the pre-defined array keys).

Instead, just change your constructor to __construct($username, $password). This increases clarity and reduces future bugs.

Misc

remove unused variables such as $correct.
comments such as //store the parameters are not really needed as they don't add any information.
same things should be named in the same way to avoid confusion. For example, registering and logging in are quite similar. But the names userLogin and register are constructed differently. Just use login and register.


Dice roll game that generates random number I just want to know how efficient is my code and is there any better way to do the same program in C?
Can the if statements are replaced with something else or can I merge the two loops together to get roll1 and roll2 within a single loop?

Also if there is any more tricks or things that I could have added to the program to make it more interesting. Also after reading my code, what mistake you think I am making while writing the code or what can I improve to write more efficient programs, things I should keep in mind.

#include <stdio.h>
#include <stdlib.h>
#include <ctype.h>
#include <windows.h>
#include <time.h>

int main()
{
    srand(time(NULL));
    int i, roll1=0, roll2=0, NumberOfRolls, RandomNUM1[50], RandomNUM2[50];
    char nextGuess;

    puts("Welcome to the Dice Roll Game");
    puts("How many times do you want to roll a dice?");
    scanf("%d", &NumberOfRolls);

    for( i=0; i<NumberOfRolls; i++ ) {
        RandomNUM1[i] = ( rand()%6 ) + 1;
        roll1 += RandomNUM1[i];
    }

    printf("\nYou Got %d in your first roll!\n", roll1);
    Sleep(3000);
    printf("\nLet's see if you can guess the value of next roll.\n");
    printf("Will it be Higher/Lower or the same? (press H/L/S)\n");
    scanf(" %c", &nextGuess);
    nextGuess = toupper(nextGuess);

        for( i=0; i<NumberOfRolls; i++ ) {
        RandomNUM2[i] = ( rand()%6 ) + 1;
        roll2 += RandomNUM2[i];
    }

    if(nextGuess=='H'){
        if(roll1<roll2){
            printf("\nYou are such a player, you guessed it right! It's %d", roll2);
        }
        else if(roll1>roll2){
            printf("\nUh-Oh! Bad Luck! First roll was higher, It's %d", roll2);
        }
        else if(roll1==roll2){
            printf("\nUh-Oh! Bad Luck! Both the rolls are same, It's %d", roll2);
        }
    }

    if(nextGuess=='L'){
        if(roll1>roll2){
            printf("\nYou are such a player, you guessed it right! It's %d", roll2);
        }
        else if(roll1<roll2){
            printf("\nUh-Oh! Bad Luck! First roll was lower, It's %d", roll2);
        }
        else if(roll1==roll2){
            printf("\nUh-Oh! Bad Luck! Both the rolls are same, It's %d", roll2);
        }

    }

    if(nextGuess=='S'){
        if(roll1==roll2){
            printf("\nYou are such a player, you guessed it right! It's %d", roll2);
        }
        else if(roll1>roll2){
            printf("\nUh-Oh! Bad Luck! First roll was higher, It's %d", roll2);
        }
        else if(roll1<roll2){
            printf("\nUh-Oh! Bad Luck! Second roll is higher, It's %d", roll2);
        }
    }

    if( (nextGuess!='H') && (nextGuess!='L') && (nextGuess!='S') ){
            printf("\nYour input is invalid. Please enter H/L/S");
        }

    return 0;
}

First, get rid of the arrays RandomNum1 and RandomNum2.
You never use the data you store in them, but doing the storing might lead to out-of-bounds-accesses, as you fail to sanitize the user-input.
Only assume well-formedness of input which is under your control. Test whether scanf succeeds, and whether the result you get is valid.
Try to unify and simplify your conditional code.
Do you know that two string-literals only separated by white-space (including newlines) are fused?
Try to keep to the same style for all your names, I guess variable-names should all be lower pascal-case.
Adding a single whitespace around all binary operators, and after a comma, helps readability.


Determine concurrent access to a function Someone asked here how to determine a function is being called from multiple threads. My take on this is that they are asking about concurrent access not sequential access. The accepted answer therefore seems inadequate. Detecting concurrent access is also a problem I need to solve for debugging purposes so I thought I'd take a stab at a solution.

This takes an RAII-style approach to the solution, requiring only an instantiation of the class at the top of the function in question.

template <std::ostream& stream> class ConcurrentPrint: public std::ostringstream
{
public:
    ~ConcurrentPrint()
    {
        std::scoped_lock lock(printMutex);
        stream << this->str();
    }

private:
    static std::mutex printMutex;
};

template <std::ostream& stream> std::mutex ConcurrentPrint<stream>::printMutex;

class ConcurrencyChecker
{
public:

    ConcurrencyChecker(const std::string& id_) :
    id(id_)
    {
        std::scoped_lock lock(mapMutex);
        if (++numInstances[id] > 1)
        {
            ConcurrentPrint<std::cerr>() << "Warning: concurrent access to " << id << " from " << std::this_thread::get_id() << std::endl;
            abort();
        }
    }

    ~ConcurrencyChecker()
    {
        std::scoped_lock lock(mapMutex);
        --numInstances[id];
    }

private:
    static std::map<std::string, int> numInstances;
    static std::mutex cerrMutex;

    const std::string id;
    std::mutex mapMutex;
};

std::map<std::string, int> ConcurrencyChecker::numInstances = {};

Intended usage:

void functionToCheck()
{
    ConcurrencyChecker check(__func__);

    /* Function body */
}

Working example here

Things I'm interested in:

Are there any flaws in this general approach and is there a better one?
Any general improvements to the code

A few observations off the top of my head

cerrMutex is unused, and should be removed if not needed.

mapMutex is used to guard a static std::map but is itself a regular class member variable. That suggests it probably isn't guarding the map against concurrent access. This is probably a bug.

This seems like a fairly heavyweight tool. The uses of maps and strings all smell bloaty. That's not of itself a killer, but it's worth being at least aware of. If there were the possibility of just using, say, object-local integers for counting (and perhaps a function scope static declaration to use the same counting variables), that would probably be lighter.

On the other hand, in a general use case, concurrency causes problems whenever two threads are interacting with the same resource, not only when executing the same function. For example this tool would be significantly more useful if it could detect different threads concurrently running two methods of a single object.
At the same time, it would be more helpful if it actually did not interfere with two threads each holding their own object of a given class and each running methods on their own object. This is perhaps more of a documentation enhancement suggestion: your rough approach could work by changing what ID you pass, but it would benefit from examples. (And don't go changing it to make it lightweight if it means losing the ability to do this!)

A final thing that is worth thinking about is whether this causes any inadvertant synchronisation that actually masks the very problems you're trying to detect. This is hard to predict, because schedulers can be such a dark art in general, but you could run into a problem like this.

Suppose that you have two threads running a function that does something lightweight, perhaps just increment a shared integer. Without this tool in place, you'd expect them to keep stomping on each other.

Because you're syncing up on mapMutex (which I'm going to assume as above is meant to be static) then each call to the function has to do a lot of extra work in the constructor and destructor of your tool, and that is all under exclusion. If thread 2 starts while thread 1 is still in the constructor, it will fail to take mapMutex and the scheduler will probably send it to sleep for a bit. The window for it to wake up in which thread 1 is in the function and does not have the mutex is actually quite small. What you could get, then, is the function seems to behave fine (albeit slowly) while this object is trying to help with debugging and then goes back into data races for release code when the debug object is removed. Suffice to say, that's unhelpful!

I don't have any suggestions about how to fix this problem, just that it's the sort of thing that is worth being aware of.


Create and store an object of a class I have a class named Employee and another (reg_employee) that instantiates the former class.
I want to know if this code can be improved.
class Employee:

    def __init__(self):
        self.Name = ''
        self.Id = 0
        self.Age = 0
        self.company = 0
        self.title = ''
        self.Level = 0
        self.xp = 0.0
        self.Tasks = []
        self.Projects = []

    def setname(self, name):
        self.Name = name

    def setage(self, age):
        self.Age = age

    def setcompany(self, company):
        self.company = company

    def setxp(self, lvlup):
        self.xp = lvlup

import Employee

class reg_employee:

    def __init__(self):
        self.employees = []

    def read_info(self, name, age, company):
        e = Employee.Employee()
        e.Name = name
        e.Age = age
        e.company = company
        self.employees.append(e)

The code looks fine. However, there are a few things you can do to improve it:

first, avoid using Capital letters for variable. In Python, they should be named in snake_case;
use snake_case for setters (e.g., set_name) and getters as well, or use properties instead. See https://www.python-course.eu/python3_properties.php for more information.
You can avoid instantiating the variable in the __init__.
If you are using Python 3.X, consider specifying the parameters types and return types.

Here is how I would refactor your code:
class Employee:

    @property
    def name(self):
        return self.__name

    @name.setter
    def name(self, name: str):
        self.__name = name

    @property
    def age(self):
        return self.__age

    @age.setter
    def age(self, age: int):
        self.__age = age

    @property
    def company(self):
        return self.__company

    @company.setter
    def company(self, company: str):
        self.__company = company

    @property
    def xp(self):
        return self.__xp

    @xp.setter
    def xp(self, lvlup: float):
        self.__xp = lvlup

However, you should use setters if you need to perform some checks before assigning the value to the variable (e.g., you want to check that the passed age  > 18 or that the title is at least "a bachelor degree", and so forth.
If you do not need those checks, you can simply call the variable, for example:
class Employee:

    def __init__(self):
        self.name = ''
        self.id = 0
        self.age = 0
        self.company = 0
        self.title = ''
        self.level = 0
        self.xp = 0.0
        self.tasks = []
        self.projects = []

employee = Empolyee()
employee.name = 'Foo Bar'
employee.age = 30

Instead of using setters or populating the variables one by one, you can further improve the code by passing the parameters to the Employee class constructor:
class Employee:

    def __init__(self, name:str, age:int, company:int, id:int=None, title:str=None,
                 xp=float=None, task:list=None, projects:list=None):

        self.name = name
        self.id = id
        self.age = age
        self.company = company
        self.title = title
        self.level = level
        self.xp = xp
        self.tasks = tasks
        self.projects = projects

   # Properties here (getters and setters) ...

Then, read_info will become:
def read_info(self, name, age, company):
    e = Employee.Employee(name=name, age=age, company=company)
    self.employees.append(e)

Update
As highlighted by hjpotter, a more suitable approach in this case would use dataclasses.
Therefore, the class can be rewritten as:
from dataclasses import dataclass

@dataclass
class Employee:

    name: str
    age: int
    company: int
    id: int = 0
    title: str = None
    level: int = 0
    xp: float = 0.0
    tasks: list = None
    projects: list = None

The __init__ is created automatically, and a new object can be instantiated as in the previous example:
def read_info(self, name, age, company):
    e = Employee.Employee(name=name, age=age, company=company)
    self.employees.append(e)


RungeKutta method in C, pointers and arrays Now the program works, but is it correct? Sometimes it works in C anyway. Could I improve it somehow (not numerically, just C-wise)? I need to allocate the array as I do right? Since I am returning it and not just using it inside the function.

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

/* Approximates a solution to a differential equation on the form:
   y'(t) + ay(t) = x(t)
   y(0) = b
*/
double* runge_kutta_2nd_order(double stepSize, double a, double b, double (*x) (double), double upto)
{
    int resultSize = ((int) (upto / stepSize)) + 2;
    double yt = b;
    double time;
    double k1,k2,ystar1,ystar2;
    int index = 1;

    //double *results = (double*) malloc(resultSize * (sizeof(double)));
    double *results = malloc(resultSize * sizeof(*results));
    if(results == NULL)
    {
        printf("\nCould not allocate memory. Exiting program.");
        exit(0);
    }

    results[0] = b;

    for(time = 0; time < upto; time += stepSize) //<=
    {
        k1 = x(time) - a * yt;
        ystar1 = yt + stepSize * k1;
        k2 = x(time + stepSize) - a * ystar1;
        ystar2 = yt + (k1 + k2) / 2 * stepSize;
        yt = ystar2;
        results[index] = ystar2;
        index++;
    }
    return results;
}

void free_results(double **r)
{
    free(*r);
    *r = NULL;
}

double insignal(double t)
{
    return exp(t/2)*(sin(5*t) - 10*cos(5*t));
}

int main(void)
{
    int i;
    double *res = runge_kutta_2nd_order(0.01,-1,0,&insignal,10);

    printf("\nRunge Kutta 2nd order approximation of the differential equation:");
    printf("\ny'(t) - y(t) = e^(t/2) * (sin(5t) - 10cos(5t))");
    printf("\ny(0) = 0");
    printf("\n0 <= t <= 10");

    for(i=0; i<1001; i+=100){
        printf("\ni = %lf => y = ", 0.01*i);
        printf("%lf", res[i]);
    }
    printf("\n");

    free_results(&res);

    return 0;
}

Generally, it is preferrable to put the \ns at the end of the corresponding printf instead of relying on them being present on the next one.

printf("\n"); // <-- you don't need to complicate your whole logic just because of this guy!
              //     put him on a line of his own.
printf("\y'(t) - y(t) = e^(t/2) * (sin(5t) - 10cos(5t))\n");
printf("\y(0) = 0\n");
printf("0 <= t <= 10\n");

for(i=0; i<1001; i+=100){
    printf("i = %lf => y = %lf\n", 0.01*i, res[i]);
}

Also, as a general principle I don't like returning malloc-ed values from functions since doing so

forces you to remember to free the data even though the corresponding malloc is hidden away inside the other function and
couples your algorithm (the numeric calculations) with the memory management. It would be preferable if you were free to use any sort of memory instead of being forced into using malloc-free (as you currently are)

One possibility is passing the result array as an argument to your function:

//so you can malloc...
double *result = malloc(/**/);
runge_kutta( /*...*/, result);
free(result);

//...or not
double results[MAX_SIZE];
runge_kutta( /*...*/, result);

Do note that in the previous example you now need to be able to know the length of the result array beforehand. Most of the times this is trivial but in some cases you might need to package this in a subroutine.

Finally, since this is a numerically intensive problem and the list of results is not really needed (since I guess you only need to look at them one at a time and in sequential order) you might want to consider reorganizing things so that you can get away without any allocation for the results array.

For, example, we could have the Runge Kutta function call a callback on each result it gets instead of storing it to an array.

void runge_kutta_2nd_order(
   /*the original arguments and...*/,
   void (*onPoint)(int index, double x_i) /*the callback*/
)
{
   //...
       onPoint(index, ystar2);
   //...
}


Replace EOL in stream I need to replace the windows EOL "\r\n" with just "\n", and I want to this in the fastest way possible, as the software will have a lot of files to upload, many of them with a few thousands lines.  My current approach is:

UnicodeEncoding uniEncoding = new UnicodeEncoding();
using (MemoryStream newStream = new MemoryStream())
{
    using (TextReader stream = new StreamReader(fileInfo.Open(FileMode.Open, FileAccess.Read)))
    {
        string line = null;
        while ((line = await stream.ReadLineAsync()) != null)
        {
            line += "\n";
            await newStream.WriteAsync(uniEncoding.GetBytes(line), 0, line.Length);
        }
    }

    client.UploadFile(newStream, remote, true);
}

Another approach I was considering was to wrap the Stream I get from the file, and just not return the \n when read. I think this would consume less memory, but I am not sure if this would cause more problems as I have never really worked with streams.

Any feedback on my current approach / alternatives would be appreciated.

Performance

The non-Async methods are much faster in case of ReadLine/Write. If you need to run the code in background, use await Task.Run(/* put the whole code here */);

Encoding

You can use the static property Encoding.Unicode. (It is the same as creating a UnicodeEncoding instance without constructor arguments)
Add the Encoding to the stream reader: new StreamReader(fileInfo.Open(FileMode.Open, FileAccess.Read), Encoding.Unicode). Otherwise, if the input file is not unicode encoded, the output becomes a bunch of trash.


Converting timezone for date using java I have written this method for converting a date time zone. How do i reduce the execution time of this method further.

public static Timestamp convertTimeZone(final Timestamp fromDate, final TimeZone fromTZ, final TimeZone toTZ ){
                Long timeInDate  =  fromDate.getTime() ;
                int fromOffset = fromTZ.getOffset(timeInDate);
                int toOffset = toTZ.getOffset(timeInDate);
                Timestamp dateStamp = new Timestamp(fromDate.getTime());
                if( fromOffset >= 0){
                    int diff = 0;
                    if( toOffset > 0){
                        diff = ( fromOffset - toOffset);
                    }else{
                        diff = ( fromOffset + Math.abs(toOffset));
                    }
                    long date = fromDate.getTime() - diff;
                    dateStamp.setTime( date );
                }else{
                    int diff = 0;
                    if( toOffset > 0){
                        diff = ( Math.abs( fromOffset) + toOffset);
                    }else{
                        diff = ( Math.abs( fromOffset) - Math.abs(toOffset));
                    }
                    long date = fromDate.getTime() + diff;
                    dateStamp.setTime( date );
                }
                return dateStamp;
            }

The method can be shortened a bit. It'll probably have some effect on perfomance as well.

public static Timestamp convertTimeZone(final Timestamp fromDate, final TimeZone fromTZ, final TimeZone toTZ ){

  // primitive long should be enough for his task
  final long timeInDate = fromDate.getTime();
  final int fromOffset = fromTZ.getOffset(timeInDate);
  final int toOffset = toTZ.getOffset(timeInDate);

  return new Timestamp(timeInDate + (toOffset - fromOffset));
}


Toggle based on checkbox I have a table which has tasks with several statuses, user can use filter check-boxes to show and hide rows based on these status filters.

 function toggleRows(checkBox, img) {
    var show = $("#" + checkBox).prop("checked");
    var rows = $('tr img[src="../_layouts/15/images/' + img + '.png"]').closest('tr')

    if (show) {
        rows.show();
    }
    else {
        rows.hide();
    }
}

Implementation can be,

toggleRows("taskFinishedCb", "finished");

I would put table's code too but I am not allowed to touch that and only code above needs to be improved.

Personally, I don't like the first two lines of code, but I'm aware that you're only concerned with this function and I wouldn't know how to improve those two lines without seeing the calling function. However:

Do the checkbox and rows share a common (and close) parent?

Example markup:

        <input type="checkbox" value="some value">

In this case, the img and input share the same tr. Meaning you could have this instead:

var $row = $('tr img[src="../_layouts/15/images/' + img + '.png"]').closest('tr');
var show = $('#' + checkBox, $row).prop('checked');

Specifically looking at this code: $('#' + checkBox, $row). Here we are looking for an element with the id specified within the context of $row. The advantage of this is that jQuery doesn't traverse the whole DOM looking for that input, it only looks within the tr. This won't make much of a difference in this case - but it's important to have this approach with performance in mind on client side. You may have also noticed that I renamed rows to $row? A lot of people will prefix a variable with a $ if the variable is a jQuery selector, this gives other developers a bit more visibility.

Ternary operator

Instead of your if/else here you could use a ternary operator to reduce the lines of code:

show ? rows.show() : rows.hide();

Naming variables

The function variable names are a bit ambiguous in my opinion. To me checkBox suggests I'm actually receiving a checkbox not the ID property. The same with img applies here. More suitable ones might be:

function toggleRows(checkboxId, imgName)


Better way for many components? I have coded a system where each 'Player' class requires 5 components (could be more in the future, up to 10, 20 maybe even 30, once we add more and more features). What I want some advice on is a better way of creating these components, I am not saying that my current way is bad, I am just looking for anyone who could throw a fresh idea my way, to give my application a much better feel component wise.

My current way:

_searchesComponent = new PlayerSearchComponent(_playerConnection);
_searchesComponent.InitializeComponent();

_ignoreComponent = new PlayerIgnoreComponent(_playerConnection);
_ignoreComponent.InitializeComponent();

_effectsComponent = new PlayerEffectComponent(_playerConnection);
_effectsComponent.InitializeComponent();

_itemComponent = new PlayerItemComponent(_playerConnection);
_itemComponent.InitializeComponent();

_logComponent = new PlayerLogComponent(_playerConnection);
_logComponent.InitializeComponent();

Here is the base layout of one of my component classes:

namespace MyApp.Players.Players.Effects
{
    using System;
    using System.Collections.Generic;
    using Components;
    using NLog;

    internal sealed class PlayerIgnoreComponent : IDisposable, IComponent
    {
        private static readonly ILogger Logger = LogManager.GetCurrentClassLogger();

        private readonly PlayerConnection _playerConnection;
        private List<int> _ignores;

        public PlayerIgnoreComponent(PlayerConnection playerConnection)
        {
            _playerConnection = playerConnection;
            _ignores = new List<int>();
        }

        public void InitializeComponent()
        {
            using (var databaseConnection = Server.Database.NewDatabaseConnection)
            {
                var playerId = _playerConnection.SelectColumnInt("id");

                databaseConnection.SetQuery("SELECT * FROM `user_ignores` WHERE `user_id` = @userId");
                databaseConnection.AppendParameter("userId", playerId);

                using (var reader = databaseConnection.ExecuteReader())
                {
                    while (reader.Read())
                    {
                        _ignores.Add(reader.GetInt32("ignore_id"));
                    }
                }
            }
        }

        public void Dispose()
        {
            Dispose(true);
        }

        private void Dispose(bool disposing)
        {
            if (!disposing)
            {
                return;
            }

            _ignores.Clear();
            _ignores = null;
        }
    }
}

Fields:

private PlayerConnection _playerConnection;
private PlayerEffectComponent _effectsComponent;
private PlayerSearchComponent _searchesComponent;
private PlayerIgnoreComponent _ignoreComponent;
private PlayerClothingComponent _clothingComponent;

Here is IComponent:

namespace MyApp.Players.Players.Components
{
    public interface IComponent
    {
        void InitializeComponent();
    }
}

Any performance enhancement comments, answers or anything just to overall improve the code is also welcome.

Because the class is sealed, your IDisposable implementation is over-complicated. The whole entire point of the disposing flag is to make sure unmanaged resources get cleaned up correctly even if a class inherits this type and overrides Dispose... but your method isn't virtual, so it's only ever called/callable with Dispose(true).

Scrap that, keep it simple:

public void Dispose()
{
    _ignores.Clear();
    _ignores = null;
}

Actually, if you're using that type correctly, then you gain nothing from explicitly clearing the internal list and nulling it up - the instance is [in theory] about to get collected anyway; IDisposable is making your class much more complex than it needs to be.

Typically you'd make a class implement IDisposable when it's holding on at instance level to a reference that implements IDisposable itself, which does not appear to be the case here (unless PlayerConnection has implications that aren't in your post) - disposing a List<int> is pure overkill.

Moreover, calling Dispose twice should have no ill effect on the calling code, but in your case it throws a rather surprising NullReferenceException, and then you could very well call InitializeComponent again... without getting an expected ObjectDisposedException (you get another NullReferenceException instead).

The IComponent interface seems uncalled for, given how the callers seem to only be calling the concrete types' implementations of the InitializeComponent method.

The implementation is tightly coupled with some static/global-scope database-interacting service, which makes it untestable.

If all "components" load things from a database, it seems you need to look into the repository pattern, and possibly the unit of work as well. Consider using dependency injection to decouple your implementation from its dependencies, and avoid static / global-scope objects in OOP as a general rule of thumb.


BitArray (compacting 32 bool values into one integer) I'm trying to refamiliarize myself with Java and working with it, so I've implemented my own version of a BitArray. Basically, it's an array of integers that maps 32 boolean values to each integer. This means an array of 32 boolean values only takes 4 bytes of space.

I was attempting to implement a Sieve of Eratosthenes and I figured I should implement a BitArray at the same time.

So, the BitArray class is as follows:

public class BitArray {
    private int[] elements;
    private int length;

    public boolean get(int index) {
        return (elements[(int)Math.floor(index / 32f)] >> (index % 32) & 0x01) == 1;
    }

    public void set(int index, boolean value) {
        int actualIndex = (int)Math.floor(index / 32f);
        int elementValue = elements[actualIndex] & ~(0x01 << (index % 32));

        if (value) {
            elementValue = elementValue | (0x01 << (index % 32));
        }

        elements[actualIndex] = elementValue;
    }

    public int getLength() {
        return length;
    }

    public BitArray(int maxSize) {
        length = maxSize;
        elements = new int[(int)Math.ceil(maxSize / 32f)];
    }
}

And to test this BitArray out, I implemented the following code:

public class Main {
    public static void main(String[] args) {
        int maxValue = 10000;

        // All the values will be initialized as false. If we mark them true, it means they are a composite number.
        BitArray bitArrayValues = new BitArray(maxValue);

        for (int i = 2; i < Math.sqrt(bitArrayValues.getLength()); i++) {
            if (!bitArrayValues.get(i)) {
                for (int j = i * i; j < bitArrayValues.getLength(); j += i) {
                    bitArrayValues.set(j, true);
                }
            }
        }

        // All the values will be initialized as false. If we mark them true, it means they are a composite number.
        boolean[] boolArrayValues = new boolean[maxValue];

        for (int i = 2; i < Math.sqrt(boolArrayValues.length); i++) {
            if (!boolArrayValues[i]) {
                for (int j = i * i; j < boolArrayValues.length; j += i) {
                    boolArrayValues[j] = true;
                }
            }
        }

        // Print all the numbers which are still false, which indicates the number is a prime.
        for (int i = 1; i < bitArrayValues.getLength(); i++) {
            if (!bitArrayValues.get(i)) {
                System.out.println(i + " is prime");
            }
        }

        boolean pass = true;

        for (int i = 0; i < maxValue; i++) {
            if (boolArrayValues[i] != bitArrayValues.get(i)) {
                pass = false;
            }
        }

        System.out.println("Arrays are equal? " + pass);
        System.out.println("Done.");
    }
}

I had some trouble getting back into the Java mindset for this, after having done C# and VB.NET for so long. Overall I think it was a successful adventure.

Any comments on either code block are welcome.

public void set(int index, boolean value) {
    int actualIndex = (int)Math.floor(index / 32f);
    int elementValue = elements[actualIndex] & ~(0x01 << (index % 32));

    if (value) {
        elementValue = elementValue | (0x01 << (index % 32));
    }

    elements[actualIndex] = elementValue;
}

You can avoid a bit of recalculation here by caching the shift value. Also, switch to long to increase the range of values the user can enter

public void set(long index, boolean value) {
    final long actualIndex = (long)Math.floor(index / 32f);
    final long mask = 0x01 << (index % 32);
    long elementValue = elements[actualIndex] & ~mask;

    if (value) {
        elementValue |= mask;
    }

    elements[actualIndex] = elementValue;
}


toggle boolean with if statement I currently I have this code
if (this.showAllOnlineFriends === false) {
  this.showAllOnlineFriends = true;
} else {
  this.showAllOnlineFriends = false;
}

When you press a button, it will toggle a boolean on or off. Is there a more readable way to write this?

Value flipping is traditionally just done with a negation:
this.showAllOnlineFriends = !this.showAllOnlineFriends

That is a common, good practice way to do it. It is highly recognizable and easy to understand


Using Barrier to Implement Go's Wait Group I have implemented a WaitGroup class to simulate WaitGroup in Go lang. Then I've found that I can use Barrier in a different manner, to achieve the same thing. My WaitGroup has some additional functionality, but in most scenarios Barrier just would do.

Question: Do you spot any design flaws in this style of using Barrier?

Code:

I have an instance of Barrier:

static Barrier WaitGroupBarrier = new Barrier(0);

Then I define my tasks (or threads) this way in multiple places - so I have not the actual number of tasks/threads:

for (int i = 0; i < 1000; i++) // in multiple places
{
    var t = new Task(() =>
    {
        try
        {
            WaitGroupBarrier.AddParticipant();

            // body
        }
        finally
        {
            WaitGroupBarrier.RemoveParticipant();
        }
    });

    t.Start();
}

And I wait for them to complete, this way:

if (WaitGroupBarrier.ParticipantsRemaining > 0)
{
    Console.WriteLine("waiting...");
    WaitGroupBarrier.SignalAndWait();
}

if (WaitGroupBarrier.ParticipantsRemaining > 0) {
    WaitGroupBarrier.SignalAndWait();
}

The above is a non-atomic operation. Thus if the if branch gets evaluated and there is one remaining, but finishes before SignalAndWait, then this will throw an exception.


Operating multiple columns of one pandas DataFrame using data from another I have a DataFrame of data from a survey that was repeated over several years, asking people about their income and how much money they had in savings. For simplicity, let's pretend it looks like this:

In [1]: nominal.head()
Out[1]:
  year1   ... year11  income1 ... income11    savings1 ... savings11
0 1992    ... NaN     600     ... NaN         100      ... NaN
1 1992    ... 2012    0       ... 100         0        ... 1000
2 1993    ... 2013    155000  ... 211000      490500   ... 60000
3 1993    ... 2013    155000  ... 211000      490500   ... 60000
4 1994    ... 2014    7000    ... 1000        90200    ... 100000

I also have a DataFrame that includes the annual rates of inflation.

In [2]: annual_inflation.head()
Out[2]:
    period    value   ratio to 2014
year
1992  M13 140.300 1.687356
1993  M13 144.500 1.638311
1994  M13 148.200 1.597409
1995  M13 152.400 1.553386

So my goal is to correct all of the income and savings columns for inflation, using the year that each survey was conducted. Therefore generating a DataFrame of the same dimensions as nominal, but with corrected values. (Rounded floats for clarity).

In [*]: real.head(2)
Out[*]:
  year1   ... year11  income1 ... income11    savings1 ... savings11
0 1992    ... NaN     1012.41 ... NaN         168.74   ... NaN
1 1992    ... 2012    0       ... 103.11      0        ... 1031.10

I am able to do this with the following code, but with nested for-loops and a conditional, it is exceptionally slow. Is there a better way? I feel like a .groupby() and/or .apply() should work, but I can't figure it out.

real = nominal.copy()
study_waves = range(11)
years = ['year1', 'year2' ... 'year11']
incomes = ['income1', 'income2' ... 'income11']
savings = ['savings1', 'savings2' ... 'savings11']

for wave in study_waves:
    for row in xrange(len(nominal)):
        year = nominal.loc[row, years[wave]]
        if 1992 <= year <= 2014: # sometimes year is NaN
            old_income = nominal.loc[row, incomes[wave]]
            old_savings = nominal.loc[row, savings[wave]]
            adjustment = annual_inflation.loc[int(year), 'ratio to 2014']
            new_income = old_income * adjustment
            new_savings = old_savings * adjustment
            real.loc[row, incomes[wave]] = new_income
            real.loc[row, savings[wave]] = new_savings

So, don't tell my boss, but I spent almost an entire day trying to improve this code. I have found a much better solution, but I would still love to hear from anyone who is more experienced than me.

real = nominal.copy()
years = ['year1', 'year2' ... 'year11']
incomes = ['income1', 'income2' ... 'income11']
savings = ['savings1', 'savings2' ... 'savings11']

for i in xrange(len(years)):
    interviewed = real[(real[years[i]].notnull())].copy() # to avoid the NaNs
    interview_years = list(interviewed[years[i]].unique())
    for y in interview_years:
        temp = interviewed[(interviewed[years[i]] == y)].copy()
        temp[incomes[i]] = temp[incomes[i]] *\
                           annual_inflation.loc[int(y), 'ratio to 2014']
        temp[savings[i]] = temp[savings[i]] *\
                           annual_inflation.loc[int(y), 'ratio to 2014']
        interviewed[(interviewed[years[i]] == y)] = temp
    real[(real[years[i]].notnull())] = interviewed

Basically the old block was slow because it assessed each column and then each row, looking for elements to manipulate. This block is a lot faster because it starts by slicing out all of the elements that need to be manipulated, then manipulates them all at once, then puts them back in their original place.


Selection Sort Implementation Is this a proper selection sort?
Any code issues?

public static void SelectionSort(ref int[] a)
{
    int n = a.Length;
    for (int j = 0; j < n - 1; j++)
    {
        int iMin = j;
        for (int i = j + 1; i < n; i++)
        {
            if (a[i] < a[iMin])
            {
                iMin = i;
            }
        }
        if (iMin != j)
        {
            int temp = a[j];
            a[j] = a[iMin];
            a[iMin] = temp;
        }
    }
}

//test
int[] ar = new int[] { 90, 90, 71, 82, 93, 75, 81, 54, 36, 102, 99, 34, -56, 103, 78, 796, 52, -56, 5, 215 };
SelectionSort(ref ar);
foreach (int i in ar)
    Debug.WriteLine(i);

The parameter does not need to be a ref, since there is no assignment to the variable a in the SelectionSort method.
There are too few tests.
The current test data looks entirely random, as if you had no idea what the interesting cases were.
The tests should at least cover some interesting edge cases, like:

An empty array
An array with only 1 element
An array with 2 elements
An array with 10 equal elements
An array that is sorted ascendingly
An array that is sorted descendingly
An array that is random
An array containing int.MinValue, 0, int.MaxValue
An array of 5 elements, in which every permutation is tested whether it sorts to the same results

The test code should be presented in the question as real test methods, not just a code snippet.


Python Hangman feedback Just looking for some feedback on a hangman script.  It works perfectly fine; I'm just trying to master the Python language, and the best place way to get better is to ask the true masters!

import random

UNKNOWN_CHARACTER = "*"
GUESS_LIMIT = 7

words = []

def load_words():
    global words
    with open("dictionary.txt") as file:
        words = file.readlines()
    stripped = []
    for word in words:
        stripped.append(word.strip())
    words = stripped

def play():
    word = random.choice(words)
    solved = False
    constructed = ""
    guessed = []
    guess = ""
    for i in range(0, len(word)):
        constructed += UNKNOWN_CHARACTER
    while not solved and len(guessed) < GUESS_LIMIT:
        print("\n" + str(GUESS_LIMIT - len(guessed)) + " errors left...")
        print(constructed)
        valid_guess = False
        while not valid_guess:
            guess = input("Guess a letter: ").lower()
            if len(guess) == 1:
                if guess not in guessed and guess not in constructed:
                    valid_guess = True
                else:
                    print("You've already guessed that letter!")
            else:
                print("Please guess a single letter.")
        if guess in word:
            new_constructed = ""
            for i in range(0, len(word)):
                if word[i] == guess:
                    new_constructed += guess
                else:
                    new_constructed += constructed[i]
            constructed = new_constructed
        else:
            guessed.append(guess)
        solved = constructed == word
    print("\n" + word)

def main():
    load_words()
    keep_playing = True
    while keep_playing:
        play()
        keep_going = input("Continue playing? (y/n): ").lower()
        if keep_going not in ["yes", "y"]:
            keep_playing = False

if __name__ == "__main__":
    main()

Excerpt of dictionary.txt:

logorrheic
logos
logotype
logotypes
logotypies
logotypy
logroll
logrolled
logroller
logrollers
logrolling
logrollings
logrolls
logs
logway
logways
logwood
logwoods
logy
loin
loincloth
loincloths
loins
loiter
loitered
loiterer
loiterers
loitering
loiters
loll
lollapalooza
lollapaloozas
lolled
loller
lollers

List comprehension is a very neat way to construct lists in Python. For instance :

stripped = []
for word in words:
    stripped.append(word.strip())

becomes

stripped = [word.strip() for words in word]

(and then you can get rid of the stripped variable altogether in your code).

The pythonic way to loop over something is not to use range and len (except if you really have to). For instance, we have first :

for i in range(0, len(word)):
    constructed += UNKNOWN_CHARACTER

which can be written :

for i in word:
    constructed += UNKNOWN_CHARACTER

Then one might argue that you can use the * operator here to write :

constructed = UNKNOWN_CHARACTER * len(word)

Sometimes, you might think that you need to use range and len because you need the index corresponding to your iteration. This is what enumerate is for. For instance :

        new_constructed = ""
        for i in range(0, len(word)):
            if word[i] == guess:
                new_constructed += guess
            else:
                new_constructed += constructed[i]

becomes :

        new_constructed = ""
        for i,l in enumerate(word): # i in the index, l is the letter
            if l == guess: # note that we don't need to get the i-th element here
                new_constructed += l # I prefer l to guess here because it's shorter :-P
            else:
                new_constructed += constructed[i]

What would be really awesome here would to be to be able to iterate over 2 containers in the same time. zip allows you to do such a thing.

        new_constructed = ""
        for w,c in zip(word,constructed):
            if w == guess:
                new_constructed += w
            else:
                new_constructed += c

Then, it's pretty cool because we can make things slightly more concise :

        new_constructed = ""
        for w,c in zip(word,constructed):
            new_constructed += w if w == guess else c

If you want to play it cool and re-use list comprehension (or even generator expression (I use fancy words so that you can google then if required)) : you build some kind of list of characters/strings and join them together with join.

        new_constructed = ''.join((w if w == guess else c for w,c in zip(word,constructed)))

This being said, I probably wouldn't build that string this way but I just pointed to point this out so that you can discover new things.

Usually, global variables are frowned upon because they make things hard to track. In your case, it is not so much of an issue but let's try to split the logic in smaller parts. It makes things easier to understand and to test too.

Here, we just need to return the list of words from load_words.

It's interesting to store the letters that have been guessed. However, a list (build with [] and populated with append()) might not be the right container for this. What you really want at the end of the day is just to be able to tell quickly whether some letter has been guessed already. This is what sets are for.

You could store all guesses or just wrong guesses. You've decided to store only wrong guesses (as right guesses can be deduced from the constructed string).

Personnally, I'd rather save all guesses on one hand and the number of wrong guesses on the other hand at it might make things a bit easier later on. I am not saying that what you did was wrong at all, I just want to show a different way to do.

Also, I'm trying to split the logic used to

display what has been found away from the logic used to
know if a letter has been guessed
know if the character has been fully found.

At the moment, the variable constructed in used for these 3 things and can make things hard to understand.

At the end, here is what I came up with :

#!/usr/bin/python

import random

UNKNOWN_CHARACTER = "*"
GUESS_LIMIT = 7

def load_words():
    words = []
    with open("dictionary.txt") as file:
        words = file.readlines()
    return [word.strip() for word in words]

def play(word):
    nb_wrong = 0
    guesses = set()
    while nb_wrong < GUESS_LIMIT:
        print("\n" + str(GUESS_LIMIT - nb_wrong) + " errors left...")
        print(''.join(c if c in guesses else UNKNOWN_CHARACTER for c in word))
        while True:
            guess = input("Guess a letter: ").lower()
            if len(guess) == 1:
                if guess in guesses:
                    print("You've already guessed that letter!")
                else:
                    guesses.add(guess)
                    break # stop when we have a valid guess
            else:
                print("Please guess a single letter.")
        if all(c in guesses for c in word):
            break # stop when all characters are found
    print("\n" + word)

def main():
    words = load_words()
    while True:
        play(random.choice(words))
        if input("Continue playing? (y/n): ").lower() not in ["yes", "y"]:
            break

if __name__ == "__main__":
    main()

(It's highly not tested but the point was more to explain you what I did than to show you a working program)


Implementation of generic unordered list in java This question is a follow-up question of this question. These features are added to the new class -

Generic class
User is capable of setting the size
List interface has been added
Iterator class has been added

As of now I still have some questions after developing this class.

Does this list interface have enough methods/behavior?
Why do I have to type cast from next() while returning data?
When I am testing this class I have to do this Iterator<Integer> itr = ((UnorderedList) ul).iterator(); why?

Thanks to him to open my eyes to design a class to iterate over a structure. My prof of Data Structure even did not specify or taught students important things like this.

If this class is still lacking anything then as always suggestions and advice are appreciated.

//List interface

package lists.arrayLists;

public interface List<T>
{
    boolean isFull();
    boolean isEmpty();
    boolean add(T data);
    void remove(T data);
    boolean find(T data);
    int size();
}

// Unordered List class

package lists.arrayLists.unorderedList;

import lists.arrayLists.List;
import java.lang.IndexOutOfBoundsException;
import java.util.Iterator;
import java.util.NoSuchElementException;

public class UnorderedList<T> implements List<T>, Iterable<T>
{
    private static int MAX_SIZE = 100;
    private int size;
    private Node<T>[] nodeElements;

    public UnorderedList()
    {
        nodeElements = new Node[MAX_SIZE];
    }

    public UnorderedList(int capacity)
    {
        MAX_SIZE = capacity;
        nodeElements = new Node[MAX_SIZE];
    }

    @Override
    public boolean isFull()
    {
        return (size() == MAX_SIZE);
    }

    @Override
    public boolean isEmpty()
    {
        return (size() == 0);
    }

    @Override
    public int size()
    {
        return size;
    }

    @Override
    public boolean add(T data)
    {
        try
        {
            Node<T> node = new Node<>(data);
            nodeElements[size++] = node;
            return true;
        }
        catch (IndexOutOfBoundsException ex)
        {
            System.out.println(ex.getMessage());
            throw new IndexOutOfBoundsException();
        }
    }

    @Override
    public void remove(T data)
    {
        int index = locate(data);
        try
        {
            nodeElements[index] = nodeElements[size() - 1];
            size--;
        }
        catch (IndexOutOfBoundsException ex)
        {
            throw new NoSuchElementException();
        }
    }

    @Override
    public boolean find(T data)
    {
        return (locate(data) >= 0);
    }

    private int locate(T data)
    {
        for (int index = 0; index < size(); index++)
        {
            if (data == nodeElements[index].getData())
            {
                return index;
            }
        }
        return - 1;
    }

    @Override
    public Iterator<T> iterator()
    {
        return new ULIterator<T>();
    }

    private class ULIterator<T> implements Iterator<T>
    {
        private int currentIndex = 0;

        @Override
        public boolean hasNext()
        {
            return (currentIndex < size());
        }

        @Override
        public T next()
        {
            if(hasNext())
            {
                return (T)nodeElements[currentIndex++].getData();
            }
            else
            {
                throw new NoSuchElementException();
            }
        }
    }
}

// Node class

package lists.arrayLists.unorderedList;

public class Node<T>
{
    private T data;

    public Node(T data)
    {
        this.data = data;
    }

    public T getData()
    {
        return data;
    }
}

Does this list interface have enough methods/behavior?

Yes, it is enough. If you want to extend it consider implementing java.util.Collection interface.

Why do I have to type cast from next() while returning data?
When I am testing this class I have to do this Iterator itr = ((UnorderedList) ul).iterator(); why?

Those two questions are related. You need a cast to UnorderedList since you didn't put iterator() function in your List interface. And cast for next() is required since you used (UnorderedList) ul instead of (UnorderedList<Integer>) ul.
If MAX_SIZE is a constant it should have final modifier and shouldn't be changed in UnorderedList(int capacity). You might use nodeElements.length instead.
Node class looks pointless, there is nothing wrong with using T[] nodeElements or  better Object[] nodeElements, since generic arrays are tricky to construct.
You also shouldn't catch IndexOutOfBoundsException when you can just check if index is valid before accessing it.


Calculating the change of a vending machine I would like to know if this function is correct (I know that it works, but I have not got enough knowledge so as to demonstrate that it is correct).

  Indication: a colleccio is defined as a vector< int > always of size 12, so I can represent the number of banknotes and/or coins of value 50, 20, 10, 5, 2, 1, 0.50, 0.20, 0.10, 0.05, 0.02, 0.01 € in positions 0..11.

  Example: if I want to store 5 x 50€ + 2 x 20€ + 0 x 10€ + 1 x 5€ +... et cetera, I would type: colleccio v(12); v[0]=5; v[1]=2; v[2]=0; and so on.

Pre:

No money exchange has been done before the function.
machine is a valid colleccio: it is the content of the machine (how many banknotes and/or coins has inside).
paid is another valid colleccio but, in this case, it is what the user has paid.
possible_change is an empty colleccio.
numeric_change is a valid currency (X.XX€, two decimals, positive value). Its value is the price of the product - what the customer has paid. Its value is valid before computing the function.

Description and post: i_have_change checks if, given machine and paid, exists a possible_change to be returned to the user lately. If it is possible, in possible_change there will be a valid colleccio in which it is stored the most optimum change (that is: the less "currency" items (banknotes/coins), the better). machine should be correctly modified (it doesn't matter if it changes even though I cannot give change).

Code:

bool i_have_change (colleccio& machine, const colleccio& paid,
                    colleccio& possible_change, double numeric_change) {
  vector<double> p(12); //the values of the currency are stored in p.
  p[0] = 50.; p[1]  = 20.;  p[2]  = 10.;
  p[3] =  5.; p[4]  =  2.;  p[5]  =  1.;
  p[6] = .50; p[7]  = .20;  p[8]  = .10;
  p[9] = .05; p[10] = .02; p[11]  = .01;

  //Add `paid` to `machine`
  for (int i = 0; i < paid.size(); ++i)
    machine[i] += paid[i];

  //I start with `numeric_change` (> 0) and I keep on subtracting all $$$
  //as much as I can (they are always <= `numeric_change`)
  for (int i = 0; i < machine.size() and numeric_change > 0; ++i) {
    while (p[i] <= numeric_change and machine[i] >= 1) {
      numeric_change -= p[i];
      ++possible_change[i];
      --machine[i];
    }
  }

  //If I still have `numeric_change`, I have not physical change!
  if (numeric_change != 0) return false;

  //Else, everything is correct and I've got change.
  return true;
}

I see a number of things that may help you improve your code.  Because it makes a lot of things simpler, I'm going to assume that you and your compiler are able to handle C++11.

Post complete code

Here on Code Review, we prefer longer and more complete questions and code which is different from many of the other sister sites.  For that reason, when posting a question here for review, it's often best to include a short driver code showing the context of how your code is intended to be used.  It helps reviewers quickly understand your code and give you a better review.

Use const where practical

The std::vector p gets created and destroyed every time the function is invoked.  That's not very efficient and not at all necessary.  Instead, we can make it static so it's created once and const because it shouldn't change.  We can do that with this single line:

static const std::vector<double> p{ 50, 20, 10, 5, 2, 1, 0.5, 0.2, 0.1, 0.05, 0.02, 0.01 };

Don't use floating point for money

There is a problem using floating point (that is, float or double types) fto represent money values.  For example, when I tried to use your code to return €0.03 I only got €0.02.  This is a fundamental problem with using double (or any floating-point representation) for money values.  An alternative is to keep a number of cents as an integer value internally.  For more depth about floating point issues, I'd recommend the excellent article "What every computer scientist should know about floating-point arithmetic" by David Goldberg.

Simplify expressions

The code currently has these lines:

if (numeric_change != 0) return false;
return true;

We can simplify this to:

return !numeric_change;

However this is something of a problem with floating point values.  Either change the variable to use and integer type or compare to a real value smaller than your minimum change amount.  For example, you might use 0.005.  Much better would be to use integers.

Think carefully about the problem

If the vending machines contains €0.07 and I put in €50.00 for something that costs €10.00 what I would want to have happen is that I would get my €50.00 back and not the €10.00 item.  What the program seems to actually do, however, is return €0.07.  That is, I would expect that the function would return the value false (which it does), and then have possible_change identical to the paid object.  The specification you quote is not very clear on this issue, but it seems like the right behavior to me.

Putting it all together

Using these suggestions, here is what the routine becomes:

bool i_have_change (colleccio& machine, const colleccio& paid,
                colleccio& possible_change, double numeric_change) {
    static const std::vector<unsigned> p{ 5000, 2000, 1000, 500, 200, 100, 50, 20, 10, 5, 2, 1 };

    //Add paid to machine
    for (size_t i = 0; i < paid.size(); ++i)
        machine[i] += paid[i];

    unsigned change = numeric_change * 100;
    //I start with change (> 0) and I keep on substracting all $$$
    //as I can (they are always <= change)
    for (size_t i = 0; i < machine.size() and change > 0; ++i) {
        while (p[i] <= change and machine[i] >= 1) {
            change -= p[i];
            ++possible_change[i];
            --machine[i];
        }
    }

    if (change) {
        // can't make change so return money
        for (size_t i = 0; i < paid.size(); ++i) {
            possible_change[i] = paid[i];
        }
    }
    //If I still have change, I have not physical change!
    return !change;
}

Test code

Since the code for colleccio objects was not given, I created my own to test the code.

#include <vector>
#include <initializer_list>
#include <iostream>
#include <numeric>

class colleccio : public std::vector<int>
{
public:
    colleccio(std::initializer_list<int> w) :
        std::vector<int>{w}
    {}
    colleccio() : std::vector<int>(12) {}
    double value() const { return std::inner_product(v.begin(), v.end(), begin(), 0)/100.0; }
    friend std::ostream& operator<<(std::ostream& out, const colleccio& col) {
        out << "{ ";
        for (const auto n : col)
            out << n << ", ";
        return out << "} = " << col.value();
    }
private:
    static const std::vector<unsigned> v;
};

const std::vector<unsigned> colleccio::v{ 5000, 2000, 1000, 500, 200, 100, 50, 20, 10, 5, 2, 1 };

Now here is a convenience macro and test code in main.

#define SHOW(X) std::cout << # X " = " << (X) << std::endl

int main()
{
    colleccio machine{0, 0, 0, 0,
                      0, 0, 0, 0,
                      0, 0, 0, 7};
    colleccio paid   {1, 0, 0, 0,
                      0, 0, 0, 0,
                      0, 0, 0, 0};
    colleccio change;
    double chg_amount{40.0};

    std::cout << std::boolalpha;
    SHOW(machine);
    SHOW(paid);
    SHOW(change);
    SHOW(chg_amount);
    SHOW(i_have_change(machine, paid, change, chg_amount));
    SHOW(machine);
    SHOW(paid);
    SHOW(change);
    SHOW(chg_amount);
}

Sample output

machine = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 7, } = 0.07
paid = { 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, } = 50
change = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, } = 0
chg_amount = 40
i_have_change(machine, paid, change, chg_amount) = false
machine = { 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, } = 50
paid = { 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, } = 50
change = { 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, } = 50
chg_amount = 40


remove duplicates in a sorted array if duplicates are allowed at most twice Given a sorted array, remove the duplicates in place such that each element appear at most twice and return the new length.

Do not allocate extra space for another array, you must do this in place with constant memory.

For example,
Given sorted array A = [1,1,1,2,2,3],

Your function should return length = 5, and A is now [1,1,2,2,3].

The following is my code:

int rightMost_of_curElem(int A[], int curElem, int ind, int n)
{
    while(ind<n)
    {
        if(A[ind]!=curElem)
            break;
        ++ind;
    }

    return --ind;
}

int remove_duplicates(int A[], int n)
{
    int i=0;
    int unique_num = 0;
    while(i<n)
    {
        int cur_elem = A[i];
        int pos = rightMost_of_curElem(A, cur_elem, i, n);

        if(A[pos]==A[pos-1])
        {
            A[unique_num++] = A[pos];
        }
        A[unique_num++] = A[pos];

        i = pos+1;
    }

    return unique_num;
}

Opps one bug:
 if(A[pos]==A[pos-1])

If pos == 0 (if there is only one first element) it will fail (undefined behavior).
Two Recommendation:
This:
 int i=0;
 while(i<n)
 {
     // Stuff
     i = pos+1;
 }

Seems like a classic for(;;) loop.
 for(int i = 0; i < n; i = pos +1)
 {
 }

Don't use single letter variable names.
If your function is only slightly larger then looking through the code and finding all the uses of i can be hard. Give it a longer more unique name.

So the rest is just some personal preferences. Nothing I could complain about in a code review.
int rightMost_of_curElem(int A[], int curElem, int ind, int n)
This returns the righmost element. Which means the user has basically has the index to beginning an end of a range. Most C/C++ like functions use a concept of beginning to one past the end. So I would have made this return the index of the first element that did not match (and re-named slightly).
Slight improvement is that you pass the index of the current index which you already know equals the value. You should pass current index + 1 (avoids one compare).
int remove_duplicates(int A[], int n)
You hard code the number of elements to retain. In an interview (and in real life) I would have parametrized this.
Using if(A[pos]==A[pos-1]) as a test seems a bit hard to read when testing one or two elements. I would just use the distance between pos and i to find the number.
I would have written like this:
int getNextElement(int* data, int size, int index, int curElem)
{
    for( ; ((index < size) && (data[index] == curElem)) ; ++index)
    { /* No Body */ }

    return index;
}

int removeDuplicates(int* data, int size, int maxElements)
{
    int count = 0;
    int pos   = 0;
    for(int loop = 0; loop < size; loop = pos)
    {
        int pos = getNextElement(data, size, loop + 1, data[loop]);

        int lastElement = min(loop + maxElement, pos);
        memcpy(data + count, data + loop, (lastElement - loop) * sizeof(int));
        count += (lastElement - loop)
        /*
        for(int copy = loop; copy < lastElements; ++copy, ++count)
        {
            data[count] = data[copy];
        }
        */
    }

    return count;
}

int removeDuplicates2(int* data, int size) {return removeDuplicates(data, size, 2);}


Sequential animations This is all about a whole lot of animations which should happen sequentially. That's why I am using promise().done() functions. In the end I'm binding a function to the mousemove event (similar to a dragging feeling on hover).

How can I optimize / shorten it?

function showSpecialNavigation(){
    $('.special-navigation_section').each(function(i) {
        $(this).delay(400*i).fadeIn('fast');
        $(this).find('li').each(function(i2) {
            $(this).delay(400*i).fadeIn('800');
        });
    }).promise().done(function(){
        $(".special-navigation_section").each(function(i3) {
            $(this).find('a.transition_text').fadeIn('800');
        }).promise().done(function(){
            $(".showup1").each(function(i4) {
                $(this).delay(50*i4).fadeIn('fast').delay(10).find('img').fadeIn('fast');
            }).promise().done(function(){
                $(".showup2").each(function(i5) {
                    $(this).delay(50*i5).fadeIn('fast').delay(10).find('img').fadeIn('fast');
                }).promise().done(function(){
                    $(".showup3").each(function(i6) {
                        $(this).delay(50*i6).fadeIn('fast').delay(10).find('img').fadeIn('fast');
                    }).promise().done(function(){
                        $(".showup4").each(function(i7) {
                            $(this).delay(50*i7).fadeIn('fast').delay(10).find('img').fadeIn('fast');
                        }).promise().done(function(){
                            $(".showup5").each(function(i8) {
                                $(this).delay(50*i8).fadeIn('fast').delay(10).find('img').fadeIn('fast');
                            }).promise().done(function(){
                                $(".showup1, .showup2, .showup3, .showup4, .showup5").find('.handle').css({display: 'block'});
                                $(".dragged").each(function(i9) {
                                    $(this).delay(20*i9).animate({width: '130px'}, 300);
                                }).promise().done(function(){
                                    $(".transition").bind("mousemove", function(event){
                                        var dragged         =   $(this).find('.dragged');
                                        var containerWidth  =   $(this).width();

                                        var stopLeft        =   56;
                                        var stopRight       =   132;
                                        var mouse2Container =   event.pageX - $(this).offset().left;
                                        var controller      =   mouse2Container-56;

                                        if((stopRight > mouse2Container) && (mouse2Container > stopLeft)){
                                            dragged.stop(true, false).animate({left: controller}, 200);
                                        }
                                    });
                                });
                            });
                        });
                    });
                });
            });
        });
    });
}

In the interest of eliminating all duplicate code, here's a way to make the nested showupN pieces of code all get executed in one function:

function showSpecialNavigation(){
    var showIndex = 1;
    var maxShowIndex = 5;   // process from .showup1 to .showup5
    var allShows = $();     // initially empty object of .showupNN objects

    function setDrag() {
        allShows.css({display: 'block'});
        $(".dragged").each(function(i9) {
            $(this).delay(20*i9).animate({width: '130px'}, 300);
        }).promise().done(function(){
            $(".transition").bind("mousemove", function(event){
                var dragged         =   $(this).find('.dragged');
                var containerWidth  =   $(this).width();

                var stopLeft        =   56;
                var stopRight       =   132;
                var mouse2Container =   event.pageX - $(this).offset().left;
                var controller      =   mouse2Container-56;

                if((stopRight > mouse2Container) && (mouse2Container > stopLeft)){
                    dragged.stop(true, false).animate({left: controller}, 200);
                }
            });
        });
    }

    function showup() {
        var showList = $(".showup" + showIndex++);
        // accumulate all showupNN we've processed for use later
        allShows = allShows.add(showList);

        // set doneCnt so we know when all are done
        var doneCnt = showList.length;
        showList.each(function(i) {
            $(this).delay(50*i).fadeIn('fast').delay(10).find('img').fadeIn('fast', function() {
                // when last one is done, go to the next level or the next activity
                --doneCnt;
                if (doneCnt == 0) {
                    if (showIndex <= maxShowIndex) {
                        // go to next level of .showupNN
                        showup();
                    } else {
                        // go to next activity after
                        setDrag();
                    }
                }

            });
        });
    }

    var navs = $('.special-navigation_section');
    navs.each(function(i) {
        $(this).delay(400*i).fadeIn('fast');
        $(this).find('li').each(function() {
            $(this).delay(400*i).fadeIn('800');
        });
    }).promise().done(function(){
        navs.find('a.transition_text').fadeIn('800');
    }).promise().done(function(){
        showup();
    });
});


Save model to database I recently asked a question on Software Engineering and I had a comment:

  The updated version has some issues which are out of topic here. You may consider posting it at codereview.stackexchange.com to get some suggestions for improvements.

The code I had asked in that site:

public class GroupBillingPayment
{
    public void Save(IGroupBillingPayment model)
    {
        if (model == null || UserInfo.UserID == 0)
        {
            throw new Exception("GroupBillingPayment object or Current User Id is NULL , Please Contact Administrator.");
        }
        Data.GroupBillingPayment groupBillingPayment = RepositoryManager.GroupBillingPaymentRepository.GetById(model.GroupBillingPaymentID);
        Mapper.Map(model, groupBillingPayment);
        ServiceManager.GroupBilling.IsBillAlreadyCancelled(groupBillingPayment.GroupBillingID, THROW_ERROR);
        groupBillingPayment.UpdatedBy = UserInfo.UserID;
        groupBillingPayment.UpdatedOn = DateTime.Now;
        RepositoryManager.GroupBillingPaymentRepository.Update(groupBillingPayment, false);
        UpdateGroupBilling(groupBillingPayment.GroupBillingPaymentID, groupBillingPayment.GroupBillingID)
    }
}

Could anyone please highlight the issues in this method?

A bit more details about the above method:

ServiceManager and RepositoryManager are private properties, which have been injected into this class via constructor using Autofac.

The Service/Repository managers contain the reference to the classes in that particular layer. Hence ServiceManager can access the service GroupBilling which is in service layer and RepositoryManager can access GroupBillingPaymentRepository.

The classes in service and repository layers are resolved using Autofac and so is UserInfo.

[Parameters] are the necessary parameters required for that method to run. In this case, it is groupBillingPayment.GroupBillingPaymentID and groupBillingPayment.GroupBillingID (have updated the code to reflect this).

if (model == null || UserInfo.UserID == 0)
{
  throw new Exception("GroupBillingPayment object or Current User Id is NULL , Please Contact Administrator.");
}

You should split this into two ifs and two different exceptions.

The first one should only check the parameter passed and throw the ArgumentNullException because we don't throw the Exception directly but some more specific ones that convey more information.

The other one should be the InvalidOperationException which means that the object is in an invalid state which would be when the UserInfo.UserID is 0.

 ServiceManager.GroupBilling.IsBillAlreadyCancelled(
       groupBillingPayment.GroupBillingID,
       THROW_ERROR);

If the second parameter does what I think it does, this is, indicates whether the method should throw an exception or not then this is a very bad idea. If you need a method that does not throw then you should create another one by following the try-something pattern.

  if(ServiceManager.GroupBilling.TryGetBillAlreadyCancelled(
       groupBillingPayment.GroupBillingID,
       out bool canceled))
  {
      ...
  }


Sum of primes less than 2,000,000 I have been attempting the questions at Project Euler and I am trying to find the sum of Primes under two million (question 10)

The sum of the primes below 10 is 2 + 3 + 5 + 7 = 17.
Find the sum of all the primes below two million.

Here is my attempt which does work, but I would like to know if there is any way to improve this code. Any suggestions welcomed!
class SumOfPrimes
{
    static void Main(string[] args)
    {
        Primes primes = new Primes(2000000);
        long sum = 0;
        foreach(int p in primes.list_of_primes){
            sum += p;
        }
        Console.WriteLine(sum);
        Console.ReadLine();
    }
}

class Primes
{
    public HashSet<int> all_numbers = new HashSet<int>();
    public HashSet<int> list_of_primes = new HashSet<int>();
    public HashSet<int> list_of_nonprimes = new HashSet<int>();

    public Primes(int n)
    {
        all_numbers = new HashSet<int>(Enumerable.Range(1, n));
        for (int i = 2; i < Math.Sqrt(n) + 1; i++)
        {
            for (int j = 3; j <= n / i; j++)
                list_of_nonprimes.Add(i * j);
        }
        list_of_primes = new HashSet<int>(all_numbers.Except(list_of_nonprimes));
    }
}

Some other things that you can try, short of trying a totally different algorithm:

rename all_numbers to prime_candidates and remove composite numbers from it. (list_of_nonprimes.Add(i * j); -> prime_candidates.Remove(i*j). Avoiding the Except at the end.
You can also hold which numbers are non primes in an array of bools and lose all the HashSets. Changing:

list_of_nonprimes.Add(i * j)

to

nonprimes[i*j] = true;

Thus avoiding a bunch of hash lookups. After the loops sum up any n s.t. nonprime[n]==false
You can also use a BitArray instead of a bool array. Because it is more space efficient it might reduce cache misses.

Of course, you can only be sure if any of these actually makes any improvement after you try.


Returning information about searches performed by a user I have this simple piece of code from my django project. I feel that it isn't very well written.

Maybe it possible to write this simple function in a better way? Maybe could you give me any advices how to improve this code? I'm trying to learn how to write good quality code.

Or maybe this code is good enough?

import datetime
from accounts.models import UserProfile

def get_search_data(request):
    if request.user.is_authenticated():
        user_email = request.user.email
        user_profile = UserProfile.objects.get(user=request.user)
        user_phone = user_profile.phone_number
    else:
        user_email = ''
        user_phone = ''
    search_date = datetime.datetime.now()
    rent_starts = datetime.datetime.strptime(request.GET.get('date_start'), '%Y-%m-%d')
    rent_ends = datetime.datetime.strptime(request.GET.get('date_end'), '%Y-%m-%d')

    rent_duration = rent_ends - rent_starts
    days_till_rent_start = rent_starts - search_date

    search_date = search_date.strftime('%Y-%m-%d')
    rent_duration = rent_duration.days
    days_till_rent_start = days_till_rent_start.days

    data = {'user_email': user_email, 'user_phone': user_phone,
        'search_date': search_date, 'rent_duration': rent_duration,
        'days_till_rent_start': days_till_rent_start}

    return {'search_data': data}

I use this function to return information about searches user make on page. Function returns data in readable format for user. I give this info to javascript function. This information is used for remarketing.

The code is readable enough and to the point, there is not much to improve. I would just extract date retrieval into its own function so there is no risk of slightly mistyping the format, for instance; and change the formating of the output dict to improve readability:

import datetime
from accounts.models import UserProfile

def extract_date(request, parameter, fmt='%Y-%m-%d'):
    date = request.GET.get(parameter)
    return datetime.datetime.strptime(date, fmt)

def get_search_data(request):
    if request.user.is_authenticated():
        user_email = request.user.email
        user_profile = UserProfile.objects.get(user=request.user)
        user_phone = user_profile.phone_number
    else:
        user_email = ''
        user_phone = ''

    search_date = datetime.datetime.now()
    rent_starts = extract_date(request, 'date_start')
    rent_ends = extract_date(request, 'date_end')
    rent_duration = rent_ends - rent_starts
    days_till_rent_start = rent_starts - search_date

    return {'search_data': {
        'user_email': user_email,
        'user_phone': user_phone,
        'search_date': search_date.strftime('%Y-%m-%d'),
        'rent_duration': rent_duration.days,
        'days_till_rent_start': days_till_rent_start.days,
    }}

However, given that this function does not interact with your models (except to retrieve a phone number that could have been passed to a template earlier on), I'm wondering why you perform this computation server side. All the necessary informations are already available to the client and javascript's Date objects can perform the same kind of manipulations than what you're doing with datetime.


Object Literal Pattern in Typescript My goal is to use the object literal pattern in TypeScript.

Here is an example of the object literal pattern in JavaScript.

I created a simple example of the object literal pattern in TypeScript. The only change I made compared to plain JavaScript was to declare an interface for the object literal.

See the full working example:
Codepen

I'm curious to know if I'm missing out on any TypeScript features or not following any best practices.

interface square {
    squares: NodeListOf<Element>;
    squareColors: Array<string>;
    squareSizer: (el: HTMLElement) => void;
    squareColorChanger: (el: HTMLElement) => void;
    doSquareClick: (element: HTMLElement, index: number, array: Array<HTMLElement>) => void;
    init: () => void;
}

let squareWidget: square = {

    squares: document.querySelectorAll(".square"),

    squareColors: ["square-is-color-red", "square-is-color-orange", "square-is-color-green"],

    squareSizer: function (el) {
        if (el.dataset.enlarged === "false") {
            el.classList.add("square-is-enlarged");
            el.dataset.enlarged = "true";
        } else if (el.dataset.enlarged === "true") {
            el.classList.remove("square-is-enlarged");
            el.dataset.enlarged = "false";
        }
    },

    squareColorChanger: function (el) {
        for (var i = 0; i < el.classList.length; i++) {
            if (el.classList[i].indexOf("square-is-color") !== -1) {
                el.classList.remove(el.classList[i]);
            }
        }

        var randomColorIndex = Math.floor(Math.random() * (squareWidget.squareColors.length));
        el.classList.add(squareWidget.squareColors[randomColorIndex]);
    },

    doSquareClick: function (element, index, array) {
        element.addEventListener("click", function () {
            squareWidget.squareSizer(element);
            squareWidget.squareColorChanger(element);
        });
    },

    init: function () {
       Array.from(squareWidget.squares).forEach(squareWidget.doSquareClick);
    }
}

I'm not going to suggest any major Typescript features you aren't using, as it looks like you don't need any here. Additionally, I'm not going to claim any of my following questions / suggestions are "best practices" as "best practices" seem to change annually at a minimum in the web development world.

Why are you defining an interface for a single object? I don't see any reason to burden yourself with maintaining multiple definitions. You can include the method definitions within the module itself to save yourself from scrolling back up to the interface to check what the definition should be.

This is more of a stylistic preference, but I see no need to preface your methods with square. Since you are accessing the method on the squareWidget object it seems redundant to include square a second time.

When squareWidget is created, the list of squares is immediately created. This is a problem as if you include your code before the HTML has finished loading, the squares added before calling squareWidget.init(), but after the code is parsed will be missed.

In your squareSizer function, you use a dataset value which is not set by the init function. If the value is missing, the method won't do anything. When creating a square, should I have to remember to specify if it is enlarged or not every time? Also, is the dataset used in any code not included in your post? If not, you could replace your checks with the built in classList.toggle()

In squareColorChanger, instead of looping through the classes on the element, it might be better to check for and remove the color classes specified by the module. You can even take this a step further and just use el.classList.remove(...squareWidget.squareColors)

In doSquareClick I expected to see code that handled a click. Instead, the method adds an event listener to the element. Perhaps addListener would be more descriptive? In addition to this, your method asks for more arguments than it uses. You can safely remove the index and array arguments with no effect to your code.

In init the Array.from is unnecessary in newer browsers. forEach is included on NodeLists. Despite this, I would leave it as is until IE dies out completely. One concern I have with the current function is that if I call it twice, two event listeners will be added on all elements. If I add squares to the page after it has loaded due to user interaction I need to call the init method again to activate them which then breaks the existing squares. You could solve this problem several ways. My personal choice would likely be defining a single event listener method which is added on all squares. With this change, calling addEventListener again will have no effect.


Storing statistics as JSON in a simulation game I am working on a choice-based simulation and players have to choose between different locations, I am pushing those choices to an array locationLog and my MakeStats() function below builds an array from it. I tend to break code into bits because it helps me, but want to make sure others can understand it.

var hasAttorney = false;
var hasMoney = true;
var hasAC = true;
var currentlocation = "home";
var locationLog = []; //push locations

//Builds a JSON object with game stats
function MakeStats() {
    let home = 0,
        motel = 0,
        relative = 0,
        shelter = 0,
        apartment = 0;

    for (var i = 0; i < locationLog.length; i++) {
        if (locationLog[i] === "home")
            home++;
        else if (locationLog[i] === "motel")
            motel++;
        else if (locationLog[i] === "relative")
            relative++;
        else if (locationLog[i] === "shelter")
            shelter++;
        else if (locationLog[i] === "apartment")
            apartment++;
    }

    let tempJSON = {
        "location": {
            "home": home,
            "motel": motel,
            "relative": relative,
            "shelter": shelter,
            "apartment": apartment
        },
        "moneyLeft": money,
        "ACLeft": AC,
        "finalDestination": currentlocation,
        "hasAttorney": hasAttorney,
        "hasAC": hasAC,
        "hasMoney": hasMoney
    };
    statJSON = tempJSON;
    localStorage.setItem("playerStats", JSON.stringify(statJSON));
    console.log(statJSON);
}

Some extra points that Sam Onᴇᴌᴀ's good review skipped.

Capitalizing function names

Don't capitalize functions if they are not used to create an object. In JavaScript we only capitalize functions or objects that can be instantiated with the new token. Thus MakeStats should be makeStats.

However if you were to return the object statJSON then you could use the name MakeStats to indicate that it instantiates an object. You can then create the object in 3 ways, each of which return the same object.

    // last 2 lines of MakeStats to be
    return statJSON;
}

const s1 = new MakeStats();
const s2 = new MakeStats;  // with new objects () is not needed if there are no arguments
const s3 = MakeStats();

Quoted property names

I see this a lot, when people create an object that is to be stringified. For some reason  quoted property names are used. Doing so is just adding unneeded noise to the code.

The only time you need to use quotes to define a property is if the property is not a valid javascript variable name. e.g.  {data-one: "one"} will throw an error as data-one is not a valid name, you would thus write it {"data-one": "one"}

Shorthand property names (ES2015)

Use object property shorthand if you are adding existing named variables to an object.

You have

let tempJSON = {
    "location": {
        "home": home,
        "motel": motel,
        "relative": relative,
        "shelter": shelter,
        "apartment": apartment
    },
    "moneyLeft": money,
    "ACLeft": AC,
    "finalDestination": currentlocation,
    "hasAttorney": hasAttorney,
    "hasAC": hasAC,
    "hasMoney": hasMoney
};

can be written as

const tempJSON = {
    location: {home, motel, relative, shelter, apartment},
    moneyLeft: money,
    ACLeft: AC,
    finalDestination: currentlocation,
    hasAttorney,
    hasAC,
    hasMoney
};

the resulting object is identical to the longhand version.

localStorage

You can use simpler notation when accessing localStorage.

You have

localStorage.setItem("playerStats", JSON.stringify(statJSON));

is identical to

localStorage.playerStats = JSON.stringify(statJSON);

The same applies to reading from localStorage

stats = localStorage.playerStats; // Note If that data is not there you get undefined
                                  // Note All data from local storage is a string

Don't trust localStorage

There is no way to guarantee that localStorage content written in one session will be available next time, nor can you trust the data you read back, and many 3rd party extensions have full access to localStorage.

If the data is important, requires trust, or contains personal client data it should be saved on the server.

Warning about cyclic data

JSON.stringify will throw a TypeError: "Converting circular structure to JSON" if the data contains cyclic references.

Cyclic references are everywhere in JavaScript so if you are stringifying data from unknown or unsure sources is pays to wrap the stringify in a try catch so at least you can recover gracefully. As your function is using data from outside its control it would pay to be safe (unless you are in full control of all data).


(Leetcode) Brick wall I would like to know whether I could make my solution to this Leetcode challenge much faster.

  There is a brick wall in front of you. The wall is rectangular and has
  several rows of bricks. The bricks have the same heights but different
  widths. You want to draw a vertical line from the top to the
  bottom and cross the least bricks.

  The brick wall is represented by a list of rows. Each row is a list of
  integers representing the width of each brick in this row from left to
  right.

  If your line goes through the edge of a brick, then the brick is not
  considered as crossed. You need to find out how to draw the line to
  cross the least bricks and return the number of crossed bricks.

  You cannot draw a line just along one of the two vertical edges of the wall, in which case the line will obviously cross no bricks.

  Note -

  The width sum of bricks in different rows are the same and won't exceed INT_MAX.
  The number of bricks in each row is in the range [1, 10,000]. The height of the wall is in the range [1, 10,000]. The total number of
  bricks in the wall won't exceed 20,000.

  Example -

Input: [[1,2,2,1],
        [3,1,2],
        [1,3,2],
        [2,4],
        [3,1,2],
        [1,3,1,1]]

Output: 2

  Explanation -

Here is my solution to this challenge -

import collections

def least_bricks(wall):
    return len(wall) - max(collections.Counter(sum(row[:i + 1]) for row in wall for i in range(len(row[:-1]))).values(), default = 0)

Here is the time taken for the example output -

%timeit least_bricks([[1,2,2,1], [3,1,2], [1,3,2], [2,4], [3,1,2], [1,3,1,1]])

>>> 13 µs ± 1.07 µs per loop (mean ± std. dev. of 7 runs, 100000 loops each)

Here is my Leetcode result (85 test cases) -

Taken from - https://docs.python.org/3/library/collections.html#collections.Counter -

  A Counter is a dict subclass for counting hashable objects. It is a
  collection where elements are stored as dictionary keys and their
  counts are stored as dictionary values. Counts are allowed to be any
  integer value including zero or negative counts. The Counter class is
  similar to bags or multisets in other languages.

There's a lot of repeated summing taking place in this part:

(sum(row[:i + 1]) for row in wall for i in range(len(row[:-1])))

So this is summing the first i+1 elements in the row, with an upper bound that steps through all values except the last. If the row was [1, 2, 3, 4, 5], these are the sums:

1
1+2 = 3
1+2+3 = 6
1+2+3+4 = 10

The sums differ, but only by the last term. It would therefore save effort to use a running sum instead, only adding that last term each time:

1
1+2 = 3
3+3 = 6
6+4 = 10

This can be done with itertools.accumulate:

(s for row in wall for s in itertools.accumulate(row[:-1]))

Another thing in your code that this approach solves is the i+1 summing. Instead of having this addition performed on every iteration, you could iterate through the correct values directly with for i in range(1, len(row)). But using accumulate, this is already taken care of.