Evaluating Parser State The "parser state" of a module in rubberduck can be one of several values:

//note: ordering of the members is important
public enum ParserState
{
    /// <summary>
    /// Parse was requested but hasn't started yet.
    /// </summary>
    Pending,
    /// <summary>
    /// Project references are being loaded into parser state.
    /// </summary>
    LoadingReference,
    /// <summary>
    /// Code from modified modules is being parsed.
    /// </summary>
    Parsing,
    /// <summary>
    /// Parse tree is waiting to be walked for identifier resolution.
    /// </summary>
    Parsed,
    /// <summary>
    /// Resolving identifier references.
    /// </summary>
    Resolving,
    /// <summary>
    /// Parser state is in sync with the actual code in the VBE.
    /// </summary>
    Ready,
    /// <summary>
    /// Parsing could not be completed for one or more modules.
    /// </summary>
    Error,
    /// <summary>
    /// Parsing completed, but identifier references could not be resolved for one or more modules.
    /// </summary>
    ResolverError,
}

Now, Rubberduck uses each modules' state, and determines what the "overall" state is - and then displays that value in a status bar.

The rules are:

If all modules have the same state, overall state is that value.
If any module is in an error state, overall state is that error value.
Overall state can only be "ready" when all modules are "ready".
Overall state can only be "parsed" when all modules are "parsed".
Otherwise return the state of the most advanced non-ready module.

The code started simple, got complicated, then too simple, and now looks like this - basically it went back to "too complicated":

private static readonly ParserState[] States = Enum.GetValues(typeof(ParserState)).Cast<ParserState>().ToArray();
private ParserState EvaluateParserState()
{
    var moduleStates = _moduleStates.Values.ToList();
    var state = States.SingleOrDefault(value => moduleStates.All(ps => ps == value));

    if (state != default(ParserState))
    {
        // if all modules are in the same state, we have our result.
        Debug.WriteLine("ParserState evaluates to '{0}' (thread {1})", state, Thread.CurrentThread.ManagedThreadId);
        return state;
    }

    // error state takes precedence over every other state
    if (moduleStates.Any(ms => ms == ParserState.Error))
    {
        Debug.WriteLine("ParserState evaluates to '{0}' (thread {1})", ParserState.Error,
        Thread.CurrentThread.ManagedThreadId);
        return ParserState.Error;
    }
    if (moduleStates.Any(ms => ms == ParserState.ResolverError))
    {
        Debug.WriteLine("ParserState evaluates to '{0}' (thread {1})", ParserState.ResolverError,
        Thread.CurrentThread.ManagedThreadId);
        return ParserState.ResolverError;
    }

    // intermediate states are toggled when *any* module has them.
    var result = moduleStates.Min();
    if (moduleStates.Any(ms => ms == ParserState.Parsing))
    {
        result = ParserState.Parsing;
    }
    if (moduleStates.Any(ms => ms == ParserState.Resolving))
    {
        result = ParserState.Resolving;
    }

    if (result == ParserState.Ready && moduleStates.Any(item => item != ParserState.Ready))
    {
        result = moduleStates.Except(new[] {ParserState.Ready}).Max();
    }

    Debug.Assert(result != ParserState.Ready || moduleStates.All(item => item == ParserState.Ready));

    Debug.WriteLine("ParserState evaluates to '{0}' (thread {1})", result,
    Thread.CurrentThread.ManagedThreadId);
    return result;
}

Does anything stick out? Could it be simplified? Any/all feedback welcome!

If I understand the rules correctly, the last rule "Otherwise return the state of the most advanced non-ready module." isn't met by your method. Having states {ParserState.LoadingReference, ParserState.Parsed, ParserState.Parsing, ParserState.Ready} I would expect the returned state equals to ParserState.Parsed but your method results in ParserState.Parsing.
The rules 3 and 4 are superflous because they are satisfied by rule 1.
Because any error state takes precedence over every other state you should chewck this first. If all states are of the same error state it will result in the same result, but checking first the error states will be faster.
this

if (result == ParserState.Ready && moduleStates.Any(item => item != ParserState.Ready))
{
    result = moduleStates.Except(new[] {ParserState.Ready}).Max();
}

looks strange. result equals to the Min() of the modulStates, so let us assume it would equal to ParserState.Ready then it is garanteed that the second condition will result in true because you already checked if all states are the same.  Because of the the check of the same values the second condition of Debug.Assert(result != ParserState.Ready || moduleStates.All(item => item == ParserState.Ready)); is superflous as well.
You should use the same variables name for the same type when using lambdas. One time you have ps => ps == value and the other times you use ms => ms == ParserState.Error.
The method relies on a class variable which I would change. Add a parameter to that method declaration so it can stand on its own.

Implementing the mentioned points will lead to

private ParserState EvaluateParserState(IList<ParserState> moduleStates)
{
    // error state takes precedence over every other state
    if (moduleStates.Any(ms => ms == ParserState.Error))
    {
        Debug.WriteLine("ParserState evaluates to '{0}' (thread {1})", ParserState.Error,
        Thread.CurrentThread.ManagedThreadId);
        return ParserState.Error;
    }
    if (moduleStates.Any(ms => ms == ParserState.ResolverError))
    {
        Debug.WriteLine("ParserState evaluates to '{0}' (thread {1})", ParserState.ResolverError,
        Thread.CurrentThread.ManagedThreadId);
        return ParserState.ResolverError;
    }

    var state = States.SingleOrDefault(value => moduleStates.All(ms => ms == value));

    if (state != default(ParserState))
    {
        // if all modules are in the same state, we have our result.
        Debug.WriteLine("ParserState evaluates to '{0}' (thread {1})", state, Thread.CurrentThread.ManagedThreadId);
        return state;
    }

    var result = moduleStates.Except(new[] { ParserState.Ready }).Max();

    Debug.Assert(result != ParserState.Ready);

    Debug.WriteLine("ParserState evaluates to '{0}' (thread {1})", result,
    Thread.CurrentThread.ManagedThreadId);
    return result;
}


Closures with validation rules for domain object properties It is nice to be able to set your data in a domain object like so:

$order->setId($id);
$order->setCustomer($customer);
$order->setItems($items);

And then be able to do

if( !$order->validate() ) {
    // Invalid data
}

Which will validate all the set data and tell you if it was valid or not. I have an implementation of this I came up with but it is sort of messy and limited to only certain common validation rules like checking the type, length/range and format of the data.

I have been doing a lot of Javascript lately and I like the idea of the anonymous functions and I just found out you can do them in PHP 5.3+ and you can use the $this keyword from PHP 5.4.

abstract class AbstractEntity {

    private $id = 0;
    private $validations = array();

    public function setId($id) {
        $this->id = $id;

        // A function to validate the Id
        $this->addValidation( function() {
            return ( $this->id > 0 );
        });
    }

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

    public function validate() {
        foreach($this->validations as $validation) {
            if( $validation() === false ) {
                return false;
            }
        }

        return true;
    }

    protected function addValidation($function) {
        $this->validations[] = $function;
    }

}

The Order class:

class Order extends AbstractEntity {

    private $customer;
    private $items = array();
    private $status;

    public function setCustomer(Customer $customer) {
        $this->customer = $customer;

        // Make sure the customers account is active
        $this->addValidation( function() {
            return ( $this->customer->isActive() );
        });
        // Do as many complex validations as needed
    }

    public function setStatus($status) {
        $this->status = $status;

        // The status has to be 'Pending' or 'Complete'.
        $this->addValidation( function() {
            return ( ($this->status === 'Pending') || ($this->status === 'Complete') );
        });

}

Doing it this way validations of all different levels of complexity can be done. I was thinking of adding in another parameter to the addValidation() method so you can group validation rules together so they execute in a certain order.

It needs extra functionality like setting error messages but that should not be too hard to implement.

I was also thinking of maybe making the addValidation() method public so you can add validations outside of the class that will execute when the validate() method is called. This could be handy in some scenarios.

Is this a good or bad idea? I would like to see some opinions before I go and change all my code to using this sort of implementation.

my two cents :

I think tying validation activation to setters is not really good. What if you never set any value on your object ? It will be considered valid ! And what happens if you set the property more than once ?
Some implementations use external validator objects, which allow more flexibility (validation rules often vary, according to which user requests saving for instance). When you call validate(), you either pass a validator instance or let the object use a default validator. As a side note, coming from Rails / ActiveRecord, I can tell that complex projects often do not turn well when your domain object's lifecycle is so tightly tied to a single validation scheme !
Speaking DDD lingo, some of your problems might be efficiently solved using Value objects. For instance, your Id property could be a value object that has a isValid() method (it would be responsible for its own validation). Wether or not this is a good idea depends on your domain.


Creating a unique file name in C# I find that I am often writing files where the names either:

Need to be incremented (rather than overwriting a file that already exists in the directory with the same name)
Need to be timestamped

And, so, I've created the following methods to do so:

public static string CreateDatedFileName(string directory, string filename, string DateTimeFormat = "yyyy-MM-dd", bool AppendNumberIfNameAlreadyExists = true)
{
    if (!CheckIfDirectoryExists(directory))
        throw new ArgumentException(string.Format("The specified directory - {0} - does not exist", directory));

    string formatedTemplate = "{0} - {1}.{2}";

    var fileNameWithoutExtension = filename.Split('.')[0];
    var fileExtension = filename.Split('.')[1];
    var DateValue = DateTime.Now.ToString(DateTimeFormat);

    string newname = String.Format(formatedTemplate, fileNameWithoutExtension, DateValue, fileExtension);
    string path = Path.Combine(directory, newname);

    if (!AppendNumberIfNameAlreadyExists)
        return Path.Combine(directory, newname);
    else
        return CreateFileName(directory, newname, true);
}

public static string CreateFileName(string directory, string filename, bool AppendNumberIfNameAlreadyExists = true)
{
    if (!CheckIfDirectoryExists(directory))
        throw new ArgumentException(string.Format("The specified directory - {0} - does not exist", directory));

    string formatedTemplate = "{0}{1}.{2}";

    var fileNameWithoutExtension = filename.Split('.')[0];
    var fileExtension = filename.Split('.')[1];

    string newname = String.Format(formatedTemplate, fileNameWithoutExtension, string.Empty, fileExtension);
    string path = Path.Combine(directory, newname);
    int AppendDigit = 0;

    while (AppendNumberIfNameAlreadyExists && CheckIfFilePathExists(path))
    {
        ++AppendDigit;
        newname = String.Format(formatedTemplate, fileNameWithoutExtension, "_" + AppendDigit, fileExtension);
        path = Path.Combine(directory, newname);
    }

    return path;
}

public static bool CheckIfDirectoryExists(string directory)
{
    return System.IO.Directory.Exists(directory);
}

public static bool CheckIfFilePathExists(string path)
{
    return System.IO.File.Exists(path);
}

I'm just curious to know if this is an efficient way to accomplish this and, if not, I'd really like to learn why so as to become a better coder overall.

The only way to guarantee that the generated filename is truly unique is to actually create the file.  Any other method is vulnerable to a race condition.  Therefore, if you are serious about uniqueness, you should change

public static string CreateFileName(string directory, string filename, bool AppendNumberIfNameAlreadyExists = true)

to

/// Creates a file in the directory with the given filename
///
/// <param name="AppendNumberIfNameAlreadyExists">
/// If true, then a numbered suffix (starting from 0) is appended
/// to the filename if necessary to avoid a name clash.
///
/// If false, then an IOException is thrown if the file already exists.
/// </param>
public static FileStream CreateUniqueFile(string directory, string filename, bool AppendNumberIfNameAlreadyExists = true)


Counting the lines of a compressed file This is a Python script for counting the lines of a compressed file:

#!/usr/bin/env python3

import gzip, bz2, zipfile, os, sys

def openZip(zfile):
    f = zipfile.ZipFile(zfile)
    firstFile = f.filelist[0].filename
    if firstFile.endswith("/"):
        raise Exception("This zip file contains a directory.")
    return f.open(firstFile)

class compressedFile:
    fileOpenerDict = {".gz":gzip.open, ".bz2":bz2.open, ".zip":openZip}

    def __init__(self, pFilePath):
        self.filePath = os.path.abspath(pFilePath)
        self.fileStem, self.ext = os.path.splitext(self.filePath)
        self.ext = self.ext.lower()
        try:
            self.fileOpener = self.fileOpenerDict[self.ext]
        except KeyError:
            print("Only gz, zip, and bz2 files allowed.")
            exit(1)

    def countLines(self):
        nLines = 0
        with self.fileOpener(self.filePath) as fh:
            for line in fh:
                nLines = nLines + 1
        return nLines

if __name__ == "__main__":
    c = compressedFile(sys.argv[1])
    n = c.countLines()
    print("N lines: %d" % n)

Coding style

You should follow PEP8, the official coding style guide of Python.
The most notable violations:

Don't put all those imports on a single line, use multiple lines
Use CamelCase for class names
Use snake_case for function and variable names

Simplify and improve __init__

Don't create class member attributes that you don't need.
For example self.fileStem is completely unused,
and self.ext could be replaced with a local variable.
Don't pollute your class with unnecessary elements.

Exception handling is best for dealing with anomalies.
If the script is used with a file with an unsupported extension,
that's not really an anomaly.
It would be better to handle this with an if statement instead.

It's not a good idea to exit from the constructor of a class:
the main method of the program should be in charge of shutting down or not.
It would be better to define a custom exception,
make the constructor raise it, and let the main method handle it.

The custom exception can be as simple as this:

class UnsupportedCompressionException(Exception):
    pass

The constructor with the above suggestions applied:

def __init__(self, filepath):
    ext = os.path.splitext(filepath)[1].lower()
    if ext not in self.fileOpenerDict:
        raise UnsupportedCompressionException("Only gz, zip, and bz2 files allowed.")
    self.filepath = filepath
    self.fileOpener = self.fileOpenerDict[ext]

The main method handling the exception:

if __name__ == "__main__":
    try:
        c = CompressedFile(sys.argv[1])
        n = c.count_lines()
        print("N lines: %d" % n)
    except UnsupportedCompressionException as e:
        print(e)
        exit(1)

Improve count_lines

It would be better to do the counting and return the result from inside the with.
This is because in general it's best to limit all code to the smallest possible scope.

The line loop variable is unused.
It's customary to use _ as the loop variable name in such case.

Instead of count = count + 1, it's more common and recommended to write count += 1.

As a result, the method becomes:

def count_lines(self):
    with self.fileOpener(self.filepath) as fh:
        count = 0
        for _ in fh:
            count += 1
        return count


Calculating a Score in VBA based on selected ShapeStyle. Is there a better/shorter way to do this? I'm calculating a score based on whether or not certain colors shape is selected. Some of the shapes are in groups, some are not. For the shapes which are in groups, only one of them will count towards the score. I have figured out how, as you can see below. However, I have 9 more shapes/scores to add to the Sub. I'm still somewhat new at VBA, and I am wondering if there is a better/shorter way to write this formula. Thanks in advance.

Sub Lead_Score()
With Sheet1

Dim Cust As Integer, Vend As Integer, Sus As Integer, Pros As Integer, RelScore As Integer, Qual As Integer, NonQual As Integer, QualScore As Integer, Score As Integer
Cust = .Shapes("CustomerButton").ShapeStyle
Vend = .Shapes("VendorButton").ShapeStyle
Sus = .Shapes("SuspectButton").ShapeStyle
Pros = .Shapes("ProspectButton").ShapeStyle
Qual = .Shapes("QualifiedButton").ShapeStyle
NonQual = .Shapes("NonQualifiedButton").ShapeStyle

Dim Customer As Long, Vendor As Long, Prospect As Long, Suspect As Long, Qfied As Long, NQfied As Long
Customer = 0
Vendor = 0
Prospect = 15
Suspect = 5
Qfied = 15
NQfied = 0

If Cust = 34 Then
    RelScore = Customer
End If
If Vend = 34 Then
    RelScore = Vendor
End If
If Sus = 34 Then
    RelScore = Suspect
End If
If Pros = 34 Then
    RelScore = Prospect
End If
If Qual = 34 Then
    QualScore = Qfied
End If
If NonQual = 34 Then
    QualScore = NQfied
End If

Score = RelScore + QualScore
    Debug.Print Score

End With
End Sub

You deleted this before I could answer on SO:

Option Explicit
Sub Lead_Score()

    Dim shp As Shape
    Dim RelScore As Long
    Dim QualScore As Long
    For Each shp In ThisWorkbook.Sheets(1).Shapes 'change (1) for your sheet name.
        RelScore = MyRelScore(shp)
        QualScore = MyQualScore(shp)
    Next shp
    Dim Score As Long
    Score = RelScore + QualScore
    Debug.Print Score

End Sub
Private Function MyRelScore(shp As Shape) As Long

    Select Case shp.Name
        Case "CustomerButton", "VendorButton"
            If shp.ShapeStyle = 34 Then MyRelScore = 0
        Case "ProspectButton"
            If shp.ShapeStyle = 34 Then MyRelScore = 15
        'Case ...
    End Select

End Function
Private Function MyQualScore(shp As Shape) As Long

    Select Case shp.Name
        Case "SuspectButton"
            If shp.ShapeStyle = 34 Then MyQualScore = 5
        Case "QualifiedButton"
            If shp.ShapeStyle = 34 Then MyQualScore = 15
        Case "NonQualifiedButton"
            If shp.ShapeStyle = 34 Then MyQualScore = 0
        'Case ...
    End Select

End Function


Trim files by removing bytes from the beginning in bash I have files in a store folder, which I want to trim to a particular length by removing bytes from the beginning. My app uses a temp folder for the files manipulation. All files from the previous operations must be taken out form temp, so I use the folder for temporary storing the trimmed files. I also log the errors.
#!/bin/bash

set -euo pipefail

data_service='/path/to/data-service'
store="${data_service}/data/store"
temp="${data_service}/data/temp"
log="${data_service}/logs/log.txt"
max_size=$(( 200000 * 24 ))

{

# Checks if there are any files in 'temp'. It should be empty.
if [ "$( ls -A "${temp}" )" ]
then
    echo "$( date +"%Y-%m-%d %H:%M:%S" ) [ERROR] Temp folder is not empty!" 1>&2
    exit 1
fi

# Loops over all the files in 'store'
for file_path in "${store}/"*
do

    # Trim bigger then 'max_size' files from 'store' to 'temp'
    if [ "$( wc -c < "${file_path}" )" -gt "${max_size}" ]
    then
        file_name="$( basename "${file_path}" )"
        tail -c "${max_size}" "${file_path}" > "${temp}/${file_name}"
    fi

done

# Move all the truncated files back to 'store'
if [ "$( ls -A "${temp}" )" ]
then
    mv "${temp}/"* "${store}/"
fi

} 2>> "${log}"

Any potential problems or ways to improve the code?
Because these are my first steps in writing in bash, the specific things I googled for this code were:

Multiplication and assigning the result to a variable:
max_size=$(( 200000 * 24 ))

Check if there are files in a folder:
if [ "$( ls -A "$temp" )" ]; then .... fi

Get the date and time in a formatted string
$( date +"%Y-%m-%d %H:%M:%S" )

Get the byte size of a file:
"$( wc -c < "$file_path" )"

Copy the last N bytes of a file
tail -c "$max_size" "$file_path" > "$temp/$file_name"

Group commands and redirect STDERR to a log file
{ ... } 2>> log-file

Echo to STDERR of the group
echo 'Error message' 1>&2

Some suggestions
Get the byte size of a file
Instead of:
"$( wc -c < "$file_path" )"

use stat:
stat -c %s "$file_path"

=> better query the file system than count the number of bytes in files, some of which can be pretty large
Redirect or copy console output to a file
Instead of enclosing the whole block between brackets, I would set up file redirection from the beginning eg:
#!/bin/bash
LOG_FILE="/tmp/test.log"
exec > >(tee -a "${LOG_FILE}") 2>&1

# this should show on console AND in the log file
echo "Hello world $(date)"

based on this example
This is more flexible, because once it's set up you don't have to bother about appending your commands, and you can disable or redefine the behavior on the fly if desired.
And the benefit is that since we are redirecting both stdout and stderr, any errors will show up in the log file as well.
Misc
I think parsing the output of ls to count files in the directory is a bit primitive. Let me think but hopefully someone will come up with a better way.


A value set data structure for rapid statistics queries in Java - follow-up (See the previous iteration.)

Now all operations of the data structure work in exact constant time, yet it came with the price: the state of this set maybe non-sensical. A user may end up with the set with negative number of elements in it. Basically, the client programmer must keep track of how he/she work with this set.

StatisticsNumberSet.java

package net.coderodde.stat;

/**
 * This class provides the means for rapid statistics queries.
 *
 * @author Rodion "rodde" Efremov
 * @version 1.618 (Mar 28, 2017)
 */
public class StatisticsNumberSet {

    /**
     * Holds the sum of all elements in this set.
     */
    private double sum;

    /**
     * Holds the sum of square of each element in this set.
     */
    private double squareSum;

    /**
     * Holds the number of elements currently in this set.
     */
    private int size;

    /**
     * Adds the given number to this set.
     *
     * @param number the number to add.
     */
    public void add(double number) {
        checkNumberIsFinite(number);
        sum += number;
        squareSum += number * number;
        size++;
    }

    /**
     * Converts and adds the given number to this set.
     *
     * @param number the number to add.
     */
    public void add(Number number) {
        add(number.doubleValue());
    }

    /**
     * Removes the given number from this set.
     *
     * @param number the number to remove.
     */
    public void remove(double number) {
        checkNumberIsFinite(number);
        sum -= number;
        squareSum -= number * number;
        size--;
    }

    /**
     * Converts and removes the given number from this set.
     *
     * @param number the number to remove.
     */
    public void remove(Number number) {
        remove(number.doubleValue());
    }

    /**
     * Resets the state of this set to an empty set.
     */
    public void clear() {
        sum = 0.0;
        squareSum = 0.0;
        size = 0;
    }

    /**
     * Returns the number of elements in this set.
     *
     * @return the size of this set.
     */
    public int size() {
        return size;
    }

    /**
     * Computes the average of this set.
     *
     * @return the average.
     */
    public double getAverage() {
        return sum / size;
    }

    /**
     * Computes the variance of this set.
     *
     * @return the variance.
     */
    public double getVariance() {
        double step1 = squareSum - sum * sum / size;
        return step1 / (size - 1);
    }

    /**
     * Computes the standard deviation of this set.
     *
     * @return the standard deviation.
     */
    public strictfp double getStandardDeviation() {
        return Math.sqrt(getVariance());
    }

    /**
     * Returns the textual representation of the state of this set.
     *
     * @return the textual representation.
     */
    @Override
    public String toString() {
        return "[Size = " + size + ", average = " + getAverage() +
               ", s.d. = " + getStandardDeviation() + ", variance = " +
                getVariance() + "]";
    }

    public static void main(String[] args) {
        StatisticsNumberSet set = new StatisticsNumberSet();

        set.add(1);
        set.add(1.0f);
        set.add((byte) 3);

        System.out.println(set);

        set.remove(1.0f);

        System.out.println(set);

        set.clear();

        System.out.println(set);
    }

    private void checkNumberIsFinite(double number) {
        if (Double.isNaN(number)) {
            throw new IllegalArgumentException("number is NaN");
        }

        if (Double.isInfinite(number)) {
            throw new IllegalArgumentException("number is +Inf or -Inf");
        }
    }
}

As always, any critique is much appreciated.

This is not a Set. A Set contains no duplicate elements. You should rename the class, perhaps to a Bag or a MultiSet.

I'm also worried about this bit:

/**
 * Computes the variance of this set.
 *
 * @return the variance.
 */
public double getVariance() {
    double step1 = squareSum - sum * sum / size;
    return step1 / (size - 1);
}

/**
 * Computes the standard deviation of this set.
 *
 * @return the standard deviation.
 */
public strictfp double getStandardDeviation() {
    return Math.sqrt(getVariance());
}

Either strictfp cascades, and you calculate the variance twice, but in different ways for toString(), or it doesn't, and in that case, no math is being done in this method and the keyword can go.


Fast find exist in list or not I want to determine whether or not the selected element exists in the list. This is the current procedure that I'm using for finding the element exist in a list or array. Is there any fast procedure for finding an element with any of these?

int[] array = { 1,2,9,3,5,6,7,8,10};

boolean isFound = false;
for(int i =0; i< array.length;i++) {
    if(array[i] == 4 || array[i] == 9) {
      isFound = true;
      break;
    }
}
System.out.println(isFound);

This strictly depends on the use case. If your list:

Doesn't change
Holds a lot of data
Is being searched many times
And you don't care about where the element is

Then it may be worth keeping a cached sorted copy of the list and using Arrays.binarySearch() on it.

If those conditions are not met, then you're better off just doing a linear search. Of course you can't go lower than O(n) time, because you have to check every element if the list is not sorted.


Quickly updating a mass of objects int X = 1;
int Y = 0;
while (X != 99 - 1 || Y != 99 - 1)
{
    Y++;
    if (Y > 99 - 1)
    {
        X += 1;
        Y = 1;
    }
    Layer1ID[X, Y].Update(X, Y);
    Layer2ID[X, Y].Update(X, Y);
}

This must update a total of 19602 objects each tick. Is there any way to speed this up? Threading maybe?

Objects(Ground and buildings) are stored in Layer1 and Layer2 ID Arrays of size 99x99. Most of them do nothing during the update method.

I've only recently began programming, so I apologize for the minimal information and ignorance.

Edit: Solved my own problem w/ help

if (ObjectManager._r.Next(0, 5) == 3)
{
    NeedUpdate = new List<Point>();
    for (int x = 1; x < Layer1ID.GetLength(0); x++)
    {
        for (int y = 1; y < Layer2ID.GetLength(1); y++)
        {
            if (Layer1ID[x, y].Updates || Layer2ID[x, y].Updates)
            {
                NeedUpdate.Add(new Point(x, y));
            }
        }
    }
}
foreach (Point P in NeedUpdate)
{
    Layer1ID[P.X, P.Y].Update(P.X, P.Y);
    Layer2ID[P.X, P.Y].Update(P.X, P.Y);
}

While not answering your question, I would like to point out problems with your loop conditions.

First, the condition X != 99 - 1 looks strange. It is the same as X < 98 in this context. The same holds for Y. Since Y < 98, if you increment Y by one, Y is still Y < 99 and will never be Y > 98!

Arrays go from 0 to N-1. Let us assume that you have declared something like

const int N = 99;

Layer[,] layer = new Layer[N, N];

You would make two nested loops

for (int x = 0; x < N; x++) {
    for (int y = 0; y < N; y++) {
        layer[x,y].Update(x,y);
    }
}

In .NET 4.0 there is a parallel for statement, however if you are updating the UI, what your Update statement suggests, then a parallel processing is of little help. Only "pure" calculations will experience an increased calculation power when using parallel processing. Also, the single tasks must have a substantial amount of work to process; otherwise the overhead associated with parallel processing will outweigh the benefits.

See Parallel.For Method in .NET 4.0 vs the C# For Loop.


IsDatabaseUp returns true or throws exception I want to write an ASP.NET Web API endpoint that allows clients to check if database is running.

The method below is check if database is up by establishing a connection and running a dummy sql statement. If exception is thrown, the database is down. Thus, it always return true when it is up.

public interface IDatabaseHealthStatus {
  bool IsDatabaseUp();
}

public class DatabaseHealthStatus :IDatabaseHealthStatus {

    public bool IsDatabaseUp(){

      using(var con = new SqlConnection(connectionString)
      using(var cmd= new SqlCommand("SELECT 1", con)
      {
          con.Open();
         cmd.ExecuteReader();
         return true;  //always returns true, unless exception throws
      }
    }

}

The calling method looks like below.

API controller action method:

[HttpGet]
public IHttpActionResult IsDatabaseUp(){

  databaseHealthStatus.IsDatabaseUp(); //return value is ignored. Exception is handled by global exception handler, if any
  return OK();

}

Questions:

1: IsDatabaseUp() returns boolean type, but it always returns true (not false), unless it throws exception, which might be confusing (that is, bool, only returns true, and exception). I don't want to catch exception within the method, and return false. Because I want global exception handler to handle it and log the exception. This way it makes the code cleaner by reducing try/catch block. But in this case, unless comment is put for the method, it might be confusing.

Is this the best way to return the result? What do you think?

The naming convention Is… implies that the function is a predicate, returning either true or false.

If you wanted to throw an exception when the database is down, then you might name the function RequireDatabaseUp(), returning void, instead.  But that seems silly: why not just try performing a real SQL operation instead?

In summary, if you want to provide a function whose sole purpose is to determine whether the database is available, make it return true or false.  If you prefer to have an exception thrown on failure, don't bother providing such a function at all.


Multithreading algorithm for high performance parallel computing and Producer-Consumer queue using POSIX threads The following is the producer-consumer algorithm for a piece of software that I'm upgrading to take advantage of multi-core processing. The intended platform is some flavor of Linux running on an EC2 HPC cc4x.large cluster, which feature 2 x Intel Xeon X5570, quad-core “Nehalem” architecture processors (2*4=8 cores). The software runs a genetic algorithm for optimizing artificial neural networks.

My dominating concern is performance. RAM and HD capacity are not an issue, but CPU time and anything else that delays processing is. Right now I've made a few (noted below), hopefully trivial, compromises to make the program portable to Mac OS X, which is on my home computer that I use for coding/debugging. I note a few other minor issues in the comments, most notably an uncertainty about thread-safeness in the consumer function. This is my first time working with threads. Advice/criticism of any kind is much appreciated.

#include <iostream>
#include <pthread.h>
#include <semaphore.h>
#include <vector>

#define NUM_THREADS 3     //will be >= to # of cores
#define N           30

//globals
sem_t* producer_gate;
sem_t* consumer_gate;
pthread_mutex_t access_queued =PTHREAD_MUTEX_INITIALIZER;
int queued;
int completed;

//a dummy class for testing thread-safeness
class the_class
{
public:

    void find_num()
    {
        //make sure completion is non-instant and variable
        double num = rand();
        for(double q=0; 1; q++)
            if(num == q)
                return;
    }
};

//the consumer function for handling the parallelizable code
void* Consumer(void* argument)
{

    std::vector <the_class>* p_v_work = (std::vector <the_class>*) argument ;

    while(1)
    {
        sem_wait(consumer_gate);
        pthread_mutex_lock (&access_queued);
        int index = queued;
        queued--;
        pthread_mutex_unlock (&access_queued);
        (*p_v_work)[index-1].find_num();
        completed++;                            //<-- thread safe??
        std::cout << "\n" << index;
        if(completed == N)
            sem_post(producer_gate);
    }

    return NULL;
}

int main ()
{
    srand(5);

    //holds data to be processed and the methods for processing it
    std::vector <the_class> work(N);
    std::vector <the_class>* p_work = &work;

    sem_unlink("producer_gate");
    sem_unlink("consumer_gate");
    //can't use `sem_init();` under Mac OS X so use sem_open instead
    producer_gate = sem_open("producer_gate", O_CREAT, 0700, 0);
    consumer_gate = sem_open("consumer_gate", O_CREAT, 0700, N);
    completed   = 0;
    queued      = N;

    pthread_attr_t attr;
    pthread_attr_init (&attr);
    pthread_attr_setdetachstate (&attr, PTHREAD_CREATE_DETACHED); //no joining necessary

    pthread_t threads[NUM_THREADS];
    for(int q=0; q<NUM_THREADS; q++)
        //on OS X, a pthread_t must be provided or a bus error occurs. relevant to performance?
        pthread_create(&threads[q], &attr, Consumer, (void*) p_work );

    for( int q=0; q < 4; q++)
    {
        sem_wait(producer_gate);
        std::cout << "\nDONE\n";

        ////////////////////////////////////////////////////////
        //  Summate work done and layout work for next iteration
        ////////////////////////////////////////////////////////

        completed   = 0;
        queued      = N;
        for(int q=0;q<N;q++)
            sem_post(consumer_gate);  //some way to just set this instead of incrementing?
    }

    sem_wait(producer_gate);
    std::cout << "\n\nCompleted (:   !!\n";

}

Style

I see something that looks like a thread safe queue abstraction, which is mysteriously implemented as a collection of globals and one argument. These are a fairly well-known subspecies of container, so write one (or find an existing one that suits your purposes)

template <typename T>
class MTQueue
{
public:
  // simple operations will lock & unlock on every call
  void push(T const &);
  T pop();

  // ...
};

Just pass a pointer to this to your thread function, and your threads can pop at will.
Note this could trivially use a std::queue or any other container internally (and you may prefer FIFO rather than LIFO) - all you're really doing is bundling some container with a mutex and enforcing the lock semantics when you change (or inspect) it.

I also see something that looks like a barrier (your use of semaphores), but I suspect your consumers can start consuming as soon as there's something in the queue, so you just need a way for the producer to wait until they're all done. We could add a method to wait until the work queue is empty, but that doesn't tell us the work is complete.  Should there be a results queue? If so, use another instance of the same type: the producer can be popping results in parallel, it doesn't need to wait until the last one is calculated.

Performance

There are a couple of approaches to making it fast:

Batching

Consider the relative cost of the synchronization involved in popping an item from the queue, and actually doing the work: it might be faster overall to add a more elaborate interface so you can push/pop multiple items with only the same synchronization overhead, eg.

  // more elaborate interface to amortise synchronization cost?
  void push(std::vector<T> const &);
  void pop(std::vector<T> &out);

or you could just switch to pushing/popping a batch of work object instead of single jobs: same effect, may leave you with a simpler and more reuseable queue.

Alternative Synchronization Strategies

Mutual exclusion is only one way of sharing data across multiple cores, and to be honest it's probably a good fit here (especially if you calibrate the batch size correctly).
However, if you really want to get stuck in, look up lock free or non-blocking queues as well - for some workloads and core counts, they might suit you better.


Finding the number of occurrences of a string in another string I have written a program, in which I have to find the number of occurrences (overlapping + non-overlapping) of a string in another string.

// variable to keep track of word
int i=0;
for(vector<string>::iterator it = p.begin(); it!=p.end(); it++){
    frequency[i]=0;
    int start=0;
    while(true){
        start = s.find(*it, start);

        if(start==-1){  // not found
            break;
        } else{
            frequency[i]++;
            start++;
        }
    }

    i++;
}

I am finding the starting index start of the match, and then again searching for the string starting from 'start+1' this time, and so on. The length of the string s, in which searching has to be performed is 50,00,000. And total number of keywords (stored in vector p here) is 500 (each having length of 5000).

I don't like nested loops.  They are generally unnecessry
and are often confusing. So instead of your two loops, I would extrac the
inner loop to a function, for example:

int key_search(const std::string& s, const std::string& key)
{
    int count = 0;
    size_t pos=0;
    while ((pos = s.find(key, pos)) != std::string::npos) {
        ++count;
        ++pos;
    }
    return count;
}

This searches the inputs string s for occurances of string key and returns
the number of matches.  A few things are worth noting:

The parameters are passed as const references - const because you don't change them and references for efficiency.
I used string::npos as the idicator of failure from strings find
method, instead of -1 (-1 would give a warning from the compiler if you have
sign conversion warnings enabled).
I didn't use a while(true) loop but instead used the return from s.find
directly to control the loop.
I pre-incremented variables instead of post-increment (++pos, not pos++)
which can be more efficient (although with built in types it makes no
difference).
pos (your start) is of type size_t, not int. Or you could use std::string::size_type, which is the also size_t.
It is normal to prefix your use of standard library things with std::, so
string becomes std::string etc.

The main loop now just needs to call key_search

int i = 0;
for (auto it = keys.begin(); it != keys.end(); ++it){
    frequency[i] = key_search(s, *it);
    ++i;
}

With C++11 you can use auto to simplify the iterator definition.  You don't
show what frequency is, but I'm assuming it is an array (which are generally
best avoided in C++).


Django - Filtering with default I have a method visible on a QuerySet class which should return all articles an user can see. If the user is not logged in, all articles with min_age should'nt be visible, otherwise, if the user is logged in, all articles which min_age is lower than the user's age should be visible. I wrote this code but I think it can be improved. These raise AttributeError aren't good practice, I know, but how can I improve it?

    def visible(self, request=None):
        articles = self.filter(hidden=False)

        try:
            if request is not None:
                if request.user.is_authenticated:
                    if request.user.userprofile.has_filled_out:
                        articles = articles.filter(min_age__lte=request.user.userprofile.age)
                    else:
                        raise AttributeError
                else:
                    raise AttributeError
            else:
                raise AttributeError
        except AttributeError:
            articles = articles.filter(min_age=None)

        return articles

Just put all those conditions into single IF using AND/OR. return limited queryset in ELSE branch.


The right way to use IPC::Open3 in Perl Here is the function which does exactly what is specified by its name:

sub runCmd_getStdoutStderrAndExitCode(@)
{
  my @cmd = @_;

  # fork a child
  my ($child_pid, $child_stdout, $child_stderr);
  eval
  {
    $child_pid = open3(\*STDIN, # is /dev/null when collectl is a daemon, UPDATE: can pass 'undef' here
                       $child_stdout,
                       $child_stderr,
                       @cmd);
  };
  if ($@ =~ /^open3:/)
  {
    syslog(LOG_ERR, 'Error in "%s" : %s', "@cmd", $@);
    return (undef, undef, -1);
  }

   # result buffers
  my $stdout = '';
  my $stderr = '';

  # read into result buffers avoid blocking
  my $selector = IO::Select->new();
  $selector->add($child_stdout, $child_stderr);
  while (my @ready = $selector->can_read(SOME_TIMEOUT_CONSTANT))
  {
    foreach my $fh (@ready) {
      my $target_buf = ($fh == $child_stdout)
                     ? \$stdout
                         : \$stderr;
      if (!sysread($fh, $$target_buf, 4096, length($$target_buf)))
      {
        $selector->remove($fh);
        close $fh;
      }
    }
  }
  close $_ for ($selector->handles());

  my $finished = waitpid($child_pid, WNOHANG);
  return ($stdout, $stderr, $?) if $finished;

  if (kill 'TERM' => $child_pid) {
    # hang until SIGTERM is processed then bury child process
    waitpid($child_pid, 0);
  } else {
    syslog(LOG_WARNING, 'Error sending TERM to child process "%s"', "@cmd");
  }
  return ($stdout, $stderr, -1);
}

After the script successfully completes its job and before Perl interpreter exits, I get this message in the terminal:

  stty: standard input: Bad file descriptor

It would be nice to have your opinion about the code correctness, and get a piece of advice about how to avoid that message, as a bonus.

UPDATE: The message mentioned gone away when I replaced '\*STDIN' arg with 'undef'.

Major update: It appears that this code is not functioning properly. It mixes stderr and stdout from child process, and returns everything back as stdout. I have no clue, why it is so, and I started looking towards replacing my own bicycle with IPC::Run.

UPDATE 2: Finally, here is valid and working replacement of aforementioned bicycle masterpiece. The only disadvantage, in my opition, is that IPC::Run is not part of standard Perl distribution:

sub _runCmd_getStdoutStderrAndExitCode(@)
{
  use IPC::Run qw(run timeout);

  my @cmd = @_;
  my ($out, $err, $ok);

  my @args = (\@cmd, \*STDIN, \$out, \$err);
  push @args, timeout(SOME_TIMEOUT_CONSTANT) if SOME_TIMEOUT_CONSTANT;
  eval{ $ok = run @args };
  if ($@)
  {
    return ($out, $err, -1);
  }
  else
  {
    return ($out, $err, ($ok? 0 : $?));
  }
}

Thanks for your comments!

as it say in the IPC::Open3 documentation, the filehandle for STDERR cannot be autovivified. You need to

use Symbol qw/ gensym /; my $stderr = gensym;

to create a file handle before opening your command.


Logging system safety I've created a logging system with one-use passwords (sent via SMS API). What I would like to know is if it's "safe". I know it is hard to estimate safety but I am curious if it is safer than regular password. I would appreciate any advice to improve following code.

Scheme is:

Logging:

Loggin at a webpage -> Ask external server for password -> Store the password in session var and when form is sent compare it with the sent one

External script:

When asked with proper apikey -> give random password and send it via SMS API

The code is below:

External script:

    <?php

   $apiKey='foobar123';

   if ($_GET['api']!=$apiKey) {

       header('HTTP/1.0 404 Not Found');
       die();
   }

    $ile=12; //char number

    $onepass='';
    for($i=0;$i<$ile;$i++)
    {
        $k=rand(1,10);
        $onepass.=$letter[$k];
    }
    if (!isset($_POST['ip'],$_POST['ip'])) {echo 'error'; die();}
    $ip=$_POST['ip'];
    $www=$_POST['www'];
    echo $onepass;

    include('sender.php'); //sending API

    ?>

Logging script (at page foo.com)

    <?php
    session_start();

    $sessi='1231dsahsda8';
    $www="foo.com";
    $ip=$_SERVER['REMOTE_ADDR'];

    if(isset($_GET['destroyer'])){
        if($_GET['destroyer']=='yes'){
            session_destroy(); echo 'Logged out - Log in again'; die();
        }
        if($_GET['destroyer']=='no'){
            session_destroy(); echo 'New pass has been sent!'; session_start();
        }
    }

    if (isset($_POST['password'])){
        if($_POST['password']==$_SESSION['pass']) $_SESSION[$sessi]="log";
        else echo'Wrong Password Send again';
    }

    if ($_SESSION[$sessi]!="log") {
        if(!isset($_SESSION['pass'])){
            $adres='http://www.foobar.com/index.php?api=foobar123';
            $c = curl_init();
            curl_setopt($c, CURLOPT_URL, $adres);
            curl_setopt($c, CURLOPT_POST, 1);
            curl_setopt($c, CURLOPT_POSTFIELDS, "ip=$ip&www=$www");
            curl_setopt($c, CURLOPT_RETURNTRANSFER, 1); /
            $dane=curl_exec($c);
            curl_close($c);
            $_SESSION['pass']=$dane;
            echo 'Password has been sent';
        }
        echo'
Log in
<form action="?a" method="post"><input type="password" name="password" value=""/><input type="submit" value="Login" /></form>';
    }
    else{//authorised
         echo 'Logged
Logout

This probably makes integration and debugging harder:

if ($_GET['api']!=$apiKey) {
   header('HTTP/1.0 404 Not Found');
   die();
}

The error message is misleading, if someone use an older or wrong API key they probably will spend some time searching the error in a wrong place since it indicates that the URL is wrong, not the key. 403 Forbidden would be more convenient here.
This should be in a configuration file, it might change more often than the code itself:

$apiKey='foobar123';

This generates a random number:

$k=rand(1,10);

Documentation of mt_rand says the following:

  mt_rand — Generate a better random value

So I guess using mt_rand would we better. Furthermore, the linked page contains another interesting thing:

  [...]

  Caution

  This function does not generate cryptographically secure values,
  and should not be used for cryptographic purposes. If you need a
  cryptographically secure value, consider using openssl_random_pseudo_bytes() instead.

It really depends on the application and the sensitivity of your data which is more appropriate.
This usually referred as one-time password:

  one-use passwords

This check could be at the beginning of the script:

if (!isset($_POST['ip'],$_POST['ip'])) {echo 'error'; die();}

It's unnecessary to create an one-time password if the ip is empty.

Additionally, I guess the following is the same:

if (!isset($_POST['ip'])) {echo 'error'; die();}

You could eliminate the comment here with better variable naming:

$ile=12; //char number

Just rename it to $passwordLength.

See also: Clean Code by Robert C. Martin, Bad Comments, p67: Don’t Use a Comment When You Can Use a Function or a Variable
Two typos:

$adres should be $address
$dane to $done

I'd rename $sessi to a descriptive name. What does this variable store, what's its purpose? Put that into the name.
I'd change log to something more descriptive here:

$_SESSION[$sessi]="log";

logged_in would be easier to understand since it describes a state not an action (like log).


Rule based link processing Here is piece of code with few if ... else, I appreciate if anyone can clean it.

def process_link(post_script_link_list,read_str):
    links_list1 = ['Wq','Ws','Wi','Wd']
    bad_list = ['How long','how long','How much','how much']
    links_list2 = ['Qd']
    result = ''
    for item in bad_list:
        if item in read_str:
            return
    sent_split = read_str.split()
    for linking in post_script_link_list:
        sub_link = re.search('\((.*?)\)',linking).group(1)
        if sub_link in links_list1:
            if ('Who').lower() in sent_split or 'Who' in sent_split:
                result = 'person'
                break
            elif (('Where').lower() in sent_split or 'Where' in sent_split) and 'What' not in read_str and 'what' not in read_str:
                result = 'location'
                break
            elif ('Why').lower() in sent_split or 'Why' in sent_split:
                result = 'reason'
                break
            elif ('How many').lower() in read_str or 'How many' in read_str:
                result = 'count'
                break
            elif (('When').lower() in sent_split or 'When' in sent_split) and 'What' not in read_str and 'what' not in read_str:
                result = 'time'
                break
            elif (('How').lower() in sent_split or 'How' in sent_split) and ('How long').lower() not in read_str and 'How long' not in read_str and ('How much').lower() not in read_str and 'How much' not in read_str:
                result = 'manner'
                break
        elif sub_link in links_list2:
            check_yn = verify_yesno(post_script_link_list)
            if check_yn == 1:
                result = 'Yes/No'
                break
            elif check_yn == 0:
                result = 'noresult'
                break
            break
        else:
            result = 'noresult'
    return result

For lines like

if ('Who').lower() in sent_split or 'Who' in sent_split:

you have some extra brackets and can do some manual constant folding, to get

if 'who' in sent_split or 'Who' in sent_split:

Perhaps you meant

if 'who' in read_str.lower().split():

?

Assuming so, this would simplify the main part to

if  'who' in read_str.lower().split()::
    result = 'person'
    break
elif 'where' in read_str.lower().split(): and 'what' not in read_str.lower():
    result = 'location'
    break
elif 'why' in read_str.lower().split()::
    result = 'reason'
    break
elif 'how many' in read_str.lower():
    result = 'count'
    break
elif 'when' in read_str.lower().split(): and 'what' not in read_str.lower():
    result = 'time'
    break
elif 'how' in read_str.lower().split(): and 'how long' not in read_str.lower() and 'how much' not in read_str.lower():
    result = 'manner'
    break

This can be encoded in a data structure:

# Give this a better name
def process(read):
    in_notin_result = (
        ('who',      [],       'person'),
        ('where',    ['what'], 'location'),
        ('why',      [],       'reason'),
        ('how many', [],       'count'),
        ('when',     ['what'], 'time'),
        ('how',      ['how long', 'how much'], 'manner'),
    )

    words = read.split()

    for in_sent, notin_read, result in in_notin_result:
        if in_sent in words and all(disallowed not in read for disallowed in notin_read):
            return result

    return None

This says that each if is of the form

word which must be in read.split()
some words (0, 1 or 2) which must not be in read
a result value

We put that in the data structure and loop over it:

for in_sent, notin_read, result in in_notin_result

We check the in_sent in words and that disallowed not in read for every disallowed in the words which must not be in read. If this matches, we return result, otherwise we go to the next iteration of the loop.

This can be simplified:

elif sub_link in links_list2:
    check_yn = verify_yesno(post_script_link_list)
    if check_yn == 1:
        result = 'Yes/No'
        break
    elif check_yn == 0:
        result = 'noresult'
        break
    break

to

elif sub_link in links_list2:
    check_yn = verify_yesno(post_script_link_list)
    result = 'Yes/No' if check_yn else 'noresult'
    break

Note further that

('How long').lower() not in read_str and 'How long' not in read_str and ('How much').lower() not in read_str and 'How much' not in read_str:

will always be True because of the first filter agains bad_strings.

I would short-circuit the return instead of breaking. It seems to me that you want to return noresult on no result, but you default to ''. If you just default to 'noresult this would be easier.

for linking in post_script_link_list:
    sub_link = re.search('\((.*?)\)', linking).group(1)

    if sub_link in ['Wq','Ws','Wi','Wd']:
        process_result = process(read_str)
        if process_result is not None:
            return process_result

    elif sub_link in ['Qd']:
        return 'Yes/No' if verify_yesno(post_script_link_list) else 'noresult'

return 'noresult'

but if you do really want to distinguish these cases you'll need to use your original variant:

result = ''
for linking in post_script_link_list:
    ...

    else:
        result = 'noresult'

return result

It's not clear because I don't know what the code is meant to do.

This is what I've ended with. I don't know if it works:

# Give this a better name
def process(read):
    in_notin_result = (
        ('who',      [],       'person'),
        ('where',    ['what'], 'location'),
        ('why',      [],       'reason'),
        ('how many', [],       'count'),
        ('when',     ['what'], 'time'),
        ('how',      [],       'manner'),
    )
    words = read.split()
    for in_sent, notin_read, result in in_notin_result:
        if in_sent in words and all(disallowed not in read for disallowed in notin_read):
            return result
    return None

def process_link(post_script_link_list, read_str):
    read_str = read_str.lower()

    for item in ('how long', 'how much'):
        if item in read_str:
            return

    for linking in post_script_link_list:
        sub_link = re.search('\((.*?)\)', linking).group(1)

        if sub_link in ['Wq','Ws','Wi','Wd']:
            process_result = process(read_str)
            if process_result is not None:
                return process_result

        elif sub_link in ['Qd']:
            return 'Yes/No' if verify_yesno(post_script_link_list) else 'noresult'

    return 'noresult'


Animation that cycles through background colors I am doing a background image animation using the function. It works, but I think I am doing more coding than what it requires. Can anyone please help me to minimize it?

Don't recommend the CSS animations, I am working for IE9.

Live

var call = function () {

var x = 0;

    return function () {
        x++;
        x = x > 4 ? x=1 : x;
    $('#small').prop('class','').addClass('s'+x)
    .text(x)
    .animate({opacity: 0}, 3000, function() {
        call();
    });

        var y = x+1;

        $('#big').prop('class','').addClass('s'+y);

        $('#small').css({
            opacity:1
        })

    }

}();

call();
#big {
    width: 100px;
    height: 100px;
    position:relative;
    color:#fff;
  }

#small {
    position:absolute;
    left:0;
    top:0;
    width:100%;
    height:100%;
    color:#fff;
}

.s1,
.s5{
    opacity:1;
    background:gray;
}

.s2 {
    background:blue;
    opacity:1;
}

.s3 {
    background:yellow;
    opacity:1;
    color:#000;
}
.s4 {
    background:brown;
    opacity:1;
}

The #big and #small divs are actually the same size.  Therefore, I would rename them to #outer and #inner.  You can use the same CSS rules for both.

There's no need to specify the opacity in the stylesheet, since the opacity will be controlled by the jQuery animation.

I'd renumber your classes starting from 0 so that you can take advantage of the modulo operator.  The JavaScript code just looks cleaner with %.

The function-returned-by-a-function would be easier to understand if you make the outer scope into an object.  It also deserves to have a better name than call.

All of the jQuery operations on $('#small') should be done in the same chain.

Clearing the class then calling addClass() is a roundabout way of just setting the class property.

function() { call(); } could just be simplified to call.

function ColorAnimation(speed) {
    var x = 0;
    var self = this;

    this.run = function() {
        x = (x + 1) % 4;
        var y = (x + 1) % 4;
        $('#outer').prop('class', 's' + y);
        $('#inner').prop('class', 's' + x)
                   .text(x + 1)
                   .css({ opacity: 1 })
                   .animate({opacity: 0}, speed, self.run);
    };
};

new ColorAnimation(3000).run();
#outer, #inner {
    width: 100px;
    height: 100px;
    position:relative;
    color:#fff;
}

.s0 {
    background:gray;
}
.s1 {
    background:blue;
}
.s2 {
    background:yellow;
    color:#000;
}
.s3 {
    background:brown;
}


Conditional functions or multiple similar definitions? In my program(C++), I'm going to use callback functions to process input from the keyboard and mouse and constantly draw a scene. How these functions process information will change depending on the state of the program; like I would have certain functionality assigned to several keys, say the arrow keys move the highlighter in the main screen, but I want their functionality to change in another screen. I'm using OpenGL with GLUT, and it's possible to change the keyboard function for example by assigning a different function pointer to it.

The method I used in the past was to define multiple processing functions, each one for a specific task. For example, I have a keyboard for the main menu, but when a different screen is viewed, the function is changed by assigning the appropriate function pointer:

glutKeyboardFunction(MainMenueKeyboardFuncPtr);

But now, I'm wondering if using conditional statements within one function is a better approach.

My question is: are there any performance issues with the conditional functions approach? I'm thinking to define a single function, say for the keyboard, to process keyboard input in all situations:

void KeyInputProcess(int key)
{
   select(SCREEN)
 {

   case MAIN_SCREEN:

  select(key)
   {
 /*process input*/
   }
   break;

    case OPTIONS_SCREEN:

     select(key)
     {
      /*process differently from MAIN_SCREEN*/
     }
     break;
 }

}

Would the checking that takes place every time the function is called have a significant impact on the performance? Which approach is better if I want to maximize performance?

My question is: are there any performance issues with the conditional functions approach?

In a word: no.

The additional performance cost is an extra select statement, whose cost probably depends on the number of switch options in the select.

I say "no" because I expect that the performance cost for a few extra instructions is measured in nanoseconds: and there aren't enough keystrokes per second for that to make a difference. Also the actual work you do (i.e. calling OpenGL functions) is probably/hopefully more significant than the performance of the code required to get there; it's like, "Q: Is calling a subroutine expensive? A: Probably not as expensive as executing/running the subroutine."

The befit of using "function pointers" (or perhaps a different subclass for each screen) is more to do with the code's maintainability and readability: for example, it would let you have different people creating each screen; to be contrasted with having a single KeyInputProcess function, where all handling for every screen is defined in the same function.

Taking the trouble to create an abstract base class for a screen (with concrete subclasses for specific screens, and a virtual KeyInputProcess method) may make more code more maintainable (easier to add new functionality without changing existing functionality, nor requiring previously-simple methods to become too complicated)
On the other hand, for a smaller project with one programmer, defining many abstract and concrete classes might be 'over-engineering' (the Wikipedia article for over-engineering says, "See also: YAGNI"). Beware that if you practice YAGNI then, later when you do need it, you ought to be good at refactoring: i.e. adding structure later to previously less-structured code, where I'd regard "conditional statements within one function" as 'less structured' and "abstract base class with subclasses" as 'more structured'.

  Which approach is better if I want to maximize performance?

Possibly a function pointer: because it's only one or two opcodes to dispatch to the right function, instead of testing several switch statements (assuming that the compiler implements select as "if else if else if else if").

It's also possible that a function pointer isn't as fast: it dispatches to a different location i the code, which may cause slowness due to memory pages and CPU caching; whereas instead a monolithic KeyInputProcess function might have all its code located on the same page of memory.

In either case I doubt whether the performance difference is significant in this case (handling keystrokes).


All-in-one number base converter I coded this program as a challenge for /r/dailyprogrammer. Basically this program can convert any number from any base to another base including negative bases as long as the input is coherent. Did I follow the Python 3 standard or commit any obvious mistakes? I am also wondering if I can efficiently filter out nonsensical input without making explicit cases for  each one?

For example, if you input

  -15-10

...that's nonsense because you can't have a negative number if you're in a negative base, if I understand correctly.

The program is run by giving it 3 inputs:

The number
The base the number you just inputted is in
The base you'd like your output to be in

import sys

def convertToBase10(n, base):
    number = []
    negative = False

    # If n is a negative number (it's a string at this moment of the code)..
    # then replace the minus sign with a blank character to correctly convert string to int...
    # so the algorithm can continue.
    if n[0] == '-':
        n = n.replace('-', '')
        negative = True

    for i in n:
        number.append(int(i))

    number = number[::-1]
    if negative:
        for i in range(len(number)):
            number[i] = number[i] * -1
            number[i] = number[i] * (base ** i)
    else:
        for i in range(len(number)):
            number[i] = number[i] * (base ** i)

    return sum(number)

def convertToAnyBase(n, base):
    a = 0
    i = 0

    # Special case: If n is less than zero and base is greater than zero...
    #               then we have to take the absolute value of n before dividing it...
    #               using the // operator. Reason is because taking e.g. math.floor(-2.5)...
    #               returns -3.0 which is one off the actual value (we actually want -2 in this example).
    #               After we get the correct n, we can now negate it.
    if n < 0 and base > 0:
        while n < 0:
            remainder = n % base
            n = abs(n)
            n //= base
            n = -n

            # If the base is negative, remainder will be a negative number.
            # Add the absolute value of the base to the remainder and add one to n.
            # https://en.wikipedia.org/wiki/Negative_base#Calculation
            if remainder < 0:
                remainder += abs(base)
                n += 1

            a += remainder * (10 ** i)
            i += 1

        return -a

    while n != 0:
        remainder = n % base
        n //= base

        # If the base is negative, remainder will be a negative number.
        # Add the absolute value of the base to the remainder and add one to n.
        # https://en.wikipedia.org/wiki/Negative_base#Calculation
        if remainder < 0:
            remainder += abs(base)
            n += 1

        a += remainder * (10 ** i)
        i += 1
    return a

def main():
    x = sys.argv[1]
    baseIn = int(sys.argv[2])
    baseOut = int(sys.argv[3])

    if baseIn == 0 or baseOut == 0:
        print("Cannot have bases of 0. Exiting...")
        raise SystemExit

    print("You entered " + x + " in base " + str(baseIn) + ".")

    n = convertToBase10(x, baseIn)

    if baseOut == 1:
        ones = str(1) * n
        print("Your number in base " + str(baseOut) + " is " + str(ones) + ".")
    elif baseOut == -1:
        ones = str(1) * n
        print("Your number in base " + str(baseOut) + " is " + str('-' + ones) + ".")
    else:
        # If both bases are ten then we have already calculated its value.
        if abs(baseIn) == 10 and abs(baseOut) == 10:
            print("Your number in base " + str(baseOut) + " is " + str(n) + ".")
        else:
            print("Your number in base " + str(baseOut) + " is " + str(convertToAnyBase(n, baseOut)) + ".")

if __name__ == "__main__":
    main()

A couple of simplifications are possible that will make this code more Pythonic.

Instead of declaring an empty number = [] and then appending elements,
use a list comprehension:

number = [int(i) for i in reversed(n)]

Notice that I used reversed(...) instead of number[::-1],
which seems a bit more intuitive to me.

Instead of this:

if n[0] == '-':
    n = n.replace('-', '')

I'd find it more clear:

if n.startswith('-'):
    n = n[1:]

At this point it's also clear that n is not a good name.
n is usually used as a number,
but here it's a string.
Maybe "string" or "numString" would be better.

number is also not a good name.
Since it's a list, making the name plural would be more natural.

Instead of this:

        number[i] = number[i] * -1
        number[i] = number[i] * (base ** i)

You could write in a shorter, more natural way:

        number[i] *= base ** i * -1

Similar to the previous point, this can be shortened:

        n = abs(n)
        n //= base
        n = -n

To this:

        n = abs(n) // base * -1

  Basically this program can convert any number from any base to another base including negative bases as long as the input is coherent.

For input arguments 255 10 16 it gives me:

You entered 255 in base 10.
Your number in base 16 is 165.

Normally it should be ff, or at least (15, 15) or something...


Merge sort implementation with various improvements So I was working on merge sort today, and was trying to speed it up. I would like your opinion on the implementation and if it's possible to make it faster. Besides that I was wondering if there are any mistakes that i am making code wise, things that are considered a bad practice.

I like making algorithms for fun, and then when i make them I always try to improve them. One thing that I have noticed which i couldn't wrap my head around was that when you give a reverse sorted array as input it works faster than when you don't.

public static void mergeSort(int[] array) {
    mergeDivider(array,0,array.length-1);
}

private static void mergeDivider(int[] array, int lowBound, int highBound) {
    if(lowBound < highBound) {
        int middle = (lowBound + highBound)/2;

        if((highBound - lowBound) <= 43){
            //Found online that insertion sort is faster for n <= 43
            mergeSortInsertionSortSpeedUp(array,lowBound,highBound);
        } else {
            //Normal divide and conquer
            mergeDivider(array, lowBound, middle);
            mergeDivider(array, middle + 1, highBound);

            if(array[middle] > array[middle + 1]){
                mergeSortMerger(array, lowBound, middle, highBound);
            }
        }
    }
}

//Merge method
private static void mergeSortMerger(int[] array, int lowBound, int middle, int highBound) {
    //Doesn't make seperate copies for left and right only makes a temporary array
    int left_index = lowBound, right_index = middle + 1,temp_index = 0;
    int[] temp_holder = new int[highBound - lowBound + 1];

    while(left_index <= middle && right_index <= highBound){
        if(array[left_index] < array[right_index]){
            temp_holder[temp_index++] = array[left_index++];
        } else {
            temp_holder[temp_index++] = array[right_index++];
        }
    }

    while(left_index <= middle){
        temp_holder[temp_index++] = array[left_index++];
    }
    while(right_index <= highBound){
        temp_holder[temp_index++] = array[right_index++];
    }

    //Put everything in the original array
    for(int x = lowBound, k = 0; x <=highBound;x++,k++){
        array[x] = temp_holder[k];
    }
}

private static void mergeSortInsertionSortSpeedUp(int[] array, int left, int right){
    for(int x = left; x <= right;x++){
        int temp = array[x];
        int before = x - 1;
        while(before >= left && array[before] > temp){
            array[before+1] = array[before];
            before--;
        }
        array[before+1] = temp;
    }
}

Mandatory stability note: the sequence

    if(array[left_index] < array[right_index]){
        temp_holder[temp_index++] = array[left_index++];
    } else {
        temp_holder[temp_index++] = array[right_index++];
    }

causes loss of stability (equal elements are merged in the wrong order).
The code still does unnecessary copying (from tmp back to array). It is unnecessary because it can be avoided. Allocate a temporary array once, then

    merge_sort subrange 0 of arr into corresponding subrange of tmp
    merge_sort subrange 1 of arr into corresponding subrange of tmp
    merge subranges from tmp to arr

The insertion sort implementation is suboptimal. Testing two conditions in

    while(before >= left && array[before] > temp)

can be avoided. Take a look at the Alex Stepanov's technique (he is talking about quicksort, but the first 4 methods, which you are are only interested in, are equally applicable to your case).
The algorithms of this kind generally look better at the semi-open ranges. Try to rewrite the code with the assumption that highBound does not belong to the range.


Idiomatic use of computed properties in Polymer 2 I'm trying to wrap my head around how computed observables / properties in Polymer 2 work and should be used. I've created a repro analogous to my real code that zooms in on the thing I'd like to see reviewed: my use of computed properties in a Polymer 2 component.

Some things that might be helpful while reviewing:

I've explicitly left out CSS and styling because I want to focus on getting the code right.
We're targeting modern browsers, specifically for IE only Edge.

The main thing I'm worried about is hasSelectedPerson and its underlying "private" function. For one thing, it feels weird that I have to ask for a person as an argument, when it's also available as a property on the class.

<link rel="import" href="//polygit.org/polymer+:master/components/paper-button/paper-button.html">

<dom-module id="people-template">

  <template>
    <template is="dom-repeat" items="[[people]]" as="person">
      <paper-button raised on-click="_onSelectClick" item="[[person]]">
        [[person.name]]
      </paper-button>
    </template>
    <template is="dom-if" if="[[hasSelectedPerson]]">

Selected: [[selectedPerson.id]] [[selectedPerson.name]]

    </template>
  </template>

</dom-module>

You can also check it on plnkr.co where it's wrapped in a plain index file to see it actually running.

So, to summarize:

Any specific comments on my use of computed?
Any generic comments on this component?

Any specific comments on my use of computed?

I understand it's an annoying thing to have to repeat the dependencies for computed function but that's how Polymer can infer when that property has to be refreshed. If you had to use only the function name then it had to analyze your JavaScript code and it's an definitely a complex/slow task (it can't simply execute it to keep track of what you're reading because code may have side effects and/or multiple execution paths). The easiest workaround is to have you explicitly name the dependencies. Notably View.js does it for you.

Think positive: at least you do not have to add observers to all the dependencies and to manually invalidate the computed property...

  Any generic comments on this component?

Few things you can improve. First of all a non primitive default value shouldn't be an immediate value but a function. That value instance is shared across all instances of your component. Imagine you have two instances of <people-template> and in one of them you push() a new item in people. That new item is added to all the default instances! You should simply rewrite is as:

people: {
    type: Array,
    value: function () {
        return [
            { id: 1, name: "John Doe" },
            { id: 2, name: "Mary Havenstein" },
            { id: 3, name: "Janie van Staffelen" }
        ];
    }
}

There are, of course, cases when this shared value behavior is what you need (for example to implement a custom iron-meta-like component).

You set a new value for the property using this.set(), it's perfectly allowed but in this simple case you may also just use:

this.selectedPerson = event.composedPath()[0].item;

Of course you may decide to mark selectedPerson as read-only, in this case keep this.set() in-place.

I find that getting the model using composedPath()[0] is little bit fragile. dom-repeat template lets you use an easier syntax (see also Data binding helper elements):

this.selectedPerson = event.model.item;

Note that item is the name of the item used in the dom-repeat template, in your case you specified another name with as="person" then you need to use it:

this.selectedPerson = event.model.person;

I find person != null much more clear than return !!person if person is not supposed to be a boolean value.

In general I'd suggest to use a prefix for your custom components and to drop -template. Reasoning: they're all templates then you repeat the obvious and using a prefix you highly reduce possible collisions with 3rd party components.

You have a computed property hasSelectedPerson you use to hide a paragraph. Few things might be optimized here:

dom-if template does not need a boolean property, any falsy value will do the job then simply if="[[selectedPerson]]" is sufficient.
dom-if to hide a single element without any complex logic (for example lists) is an overkill and a (small) performance hit. You can use the hidden attribute (eventually complimented by a specific *[hidden] CSS rule when there isn't browser support) to achieve the same result: <p hidden$="[[selectedPerson]]">...</p>.


Doubly linked list implementation in C++ I made this C++ doubly linked list implementation and it seems to work right, handle everything well, and not have any errors. However, I've been working with it for a while and would like some fresh eyes to take a look and see if I am missing anything large or if my code contains any 'gotchas'.

Side note: I ran valgrind on several tests of this implementation and never got any memory leaks so I suspect it is handling that end of this correctly.

DLL.H

#ifndef DLL
#define DLL

class DoublyLinkedNode {
public:
    int info;
    DoublyLinkedNode *next, *prev;

    DoublyLinkedNode() {
        next = prev = 0;
    }

    DoublyLinkedNode(int nu, DoublyLinkedNode *ne = 0, DoublyLinkedNode *pre = 0) {
        info = nu;
        next = ne;
        prev = pre;
    }
};

class DoublyLinkedList {
private:
    DoublyLinkedNode *head, *tail;
public:
    DoublyLinkedList() {
        head = tail = 0;
    }

    ~DoublyLinkedList();

    bool isEmpty() const {
        return head == 0;
    }

    void addToHead(int);
    void addToTail(int);
    int removeFromHead();
    int removeFromTail();
    void remove(int);
    bool isInList(int) const;

};

#endif

DLL.cpp

#include <iostream>
#include "DLL.h"

DoublyLinkedList::~DoublyLinkedList() {
    for (DoublyLinkedNode *p; !isEmpty();) {
        p = head->next;
        delete head;
        head = p;
    }
}

void DoublyLinkedList::addToHead(int n) {
    if(head == 0){
        head = new DoublyLinkedNode(n);
        tail = head;
        return;
    }

    DoublyLinkedNode *tmp = new DoublyLinkedNode(n, head);
    head->prev = tmp;
    head = tmp;

}

void DoublyLinkedList::addToTail(int n) {
    if (tail != 0) {
        tail = new DoublyLinkedNode(n, 0, tail);
        tail->prev->next = tail;
    } else {
        head = tail = new DoublyLinkedNode(n);
    }
}

int DoublyLinkedList::removeFromHead() {
    int el = head->info;
    if(head == tail) {
        delete head;
        head = tail = 0;
    } else {
        head = head->next;
        delete head->prev;
        head->prev = 0;
    }
    return el;
}

int DoublyLinkedList::removeFromTail() {
    int el = tail->info;
    if(head == tail) {
        delete tail;
        head = tail = 0;
    } else {
        tail = tail->prev;
        delete tail->next;
        tail->next = 0;
    }
    return el;
}

void DoublyLinkedList::remove(int n) {
    if (head != 0) {

        if(head == tail) {
            delete head;
            head = tail = 0;
        }

        if(head->info == n) {
            removeFromHead();
            return;
        }

        if(tail->info == n) {
            removeFromTail();
            return;
        }

        DoublyLinkedNode *tmp;
        tmp = head;
        while(tmp != 0) {
            if (tmp->info != n) {
                tmp = tmp->next;
            } else {
                tmp->prev->next = tmp->next;
                tmp->next->prev = tmp->prev;
                delete tmp;
                tmp = 0;
                //return;
            }
        }
    }
}

bool DoublyLinkedList::isInList(int el) const {
    DoublyLinkedNode *tmp;
    for (tmp = head; tmp != 0 && !(tmp->info == el); tmp = tmp->next);
    return tmp != 0;
}

Unusual API

Some method names are a bit unusual in a linked list, here are some more common alternatives:

addToHead -> addFirst, unshift, prepend
addToTail -> add, addLast, append
removeFromHead -> removeFirst, shift
removeFromTail -> removeLast, pop
isInList -> contains

Information hiding

Users of DoublyLinkedList don't need to be aware of DoublyLinkedNode.
This implementation detail can be hidden.
I suggest to make it private by moving it out of the header file.

Naming

The naming is quite terrible at places. Here's the worst part:

DoublyLinkedNode(int nu, DoublyLinkedNode *ne = 0, DoublyLinkedNode *pre = 0) {
    info = nu;
    next = ne;
    prev = pre;
}

It would be better to call "ne" -> "next" and "pre" -> "prev", and "nu" -> "info" and use this-> on the left hand sides:

DoublyLinkedNode(int info, DoublyLinkedNode *next = 0, DoublyLinkedNode *prev = 0) {
    this->info = info;
    this->next = next;
    this->prev = prep;
}

"info" is an unusual name for the payload in a linked list node.
"value" or "data" would be better.

Instead of DoublyLinkedNode, I'd call it simply Node.
As it can be private, and it's unlikely to need any other kind of node in the linked list implementation.

Constructor initialization

There's a simpler way of initializing variables in the constructor:

DoublyLinkedNode(int info, DoublyLinkedNode *next = 0, DoublyLinkedNode *prev = 0) : info(info), next(next), prev(prev) {}

Use nullptr

Instead of setting pointers to 0 to null them out,
it's recommended to use nullptr.
You might need to enable this in your compiler,
for example with g++ you can pass the --std=c++0x flag.

Avoid external references when you don't really need them

This loop in the destructor makes references to head and isEmpty defined elsewhere:

for (DoublyLinkedNode *p; !isEmpty();) {
    p = head->next;
    delete head;
    head = p;
}

It would be more clear and simple to avoid such external references:

for (DoublyLinkedNode * node = head; node != nullptr;) {
    DoublyLinkedNode * toDelete = node;
    node = node->next;
    std::cout << "deleting: " << toDelete->info << "\n";
    delete toDelete;
}

For loop without body

The for loop in this method doesn't have a body.
This is not recommended.

bool DoublyLinkedList::isInList(int el) const {
    DoublyLinkedNode *tmp;
    for (tmp = head; tmp != 0 && !(tmp->info == el); tmp = tmp->next);
    return tmp != 0;
}

The reason the loop doesn't have a body is because of the !(tmp->info == el) in the loop condition.
That condition doesn't belong there.
It's recommended to put in the loop condition only what concerns the iteration process, in this example checking if the loop variable tmp has reached the end of the list or not.

The condition !(tmp->info == el) is about something else,
it's about evaluating the return value of the function.
That condition would be much better in the loop body, like this:

bool DoublyLinkedList::isInList(int el) const {
    for (DoublyLinkedNode * node = head; node != nullptr; node = node->next) {
        if (node->info == el) {
            return true;
        }
    }
    return false;
}

Also note that this alternative also saves an extra boolean check.


Perfect Hash Family Non-Distinct t Columns Calculator Perfect Hash Families (PHFs) are a widely studied combinatorial object. Essentially, they are 4-tuples (N; t, k, v), where the PHF is an N by k array of v symbols. For any t columns chosen, there is at least one row of those chosen columns that has no symbol duplicated in that row.

I want to be able to count how many "non-distinct" t-column choices there are in a given PHF. My code is below, with some sample inputs that I made up:

from itertools import combinations

def get_column(phf, c):
    return [x[c] for x in phf]

# returns the PHF as if we restrict to the "cs" columns (a list)
def combine_columns_into_array(phf, cs):
    r = []
    for c in cs:
        r.append(get_column(phf, c))
    return list(map(list, zip(*r)))

# returns the number of t-column choices that have no distinct rows
def count_non_distinct_combinations(phf, t):
    k = len(phf[0])
    true_count = 0
    # get all combinations
    for c in combinations([i for i in range(k)], t):
        b = False
        choices = combine_columns_into_array(phf, list(c))
        for row in choices:
            # if the row is completely distinct
            if len(row) == len(set(row)):
                b = True
        # if all rows are non-distinct:
        if not b:
            true_count += 1
    return true_count

phf = [
    [4, 2, 3],
    [2, 1, 3],
    [1, 1, 4]]

# first 2 columns have no distinct rows
phf2 = [
    [4, 4, 3],
    [2, 2, 3],
    [1, 1, 4]]

print(count_non_distinct_combinations(phf, 2)) # 0
print(count_non_distinct_combinations(phf2, 2)) # 1

I plan to use this code as a metric for how "good" the array is - what I mean by this is that I want, given the number of rows N, try to reduce this metric as much as possible. Therefore, speed/efficiency is the most important part of any review of this code.

For a start, use iterators. Don't convert to lists constantly:

get_column can be simplified to (use generator rather than list):

def get_column(phf, c):
    return (x[c] for x in phf)

combine_columns_into_array to:

def combine_columns_into_array(phf, cs):
    return zip(*[get_column(phf, c) for c in cs])

In count_non_distinct_combinations:

for c in combinations(range(k), t):

Rather than:

for c in combinations([i for i in range(k)], t):

To further simplify, combine_columns_into_array can be merged with get_column:

def combine_columns_into_array(phf, cs):
    return zip(*[[x[c] for x in phf] for c in cs])

Combining combine_columns_into_array into count_non_distinct_combinations:

def count_non_distinct_combinations(phf, t):
    k = len(phf[0])
    true_count = 0
    # get all combinations
    for c in combinations(range(k), t):
        b = False
        for row in zip(*[[x[i] for x in phf] for i in c]):
            # if the row is completely distinct
            if len(row) == len(set(row)):
                b = True
        # if all rows are non-distinct:
        if not b:
            true_count += 1
    return true_count

The zip(*[[x[i] for x in phf] for i in c]) can be simplified to ([x[i] for i in c] for x in phf)

The whole

k = len(phf[0])
true_count = 0
# get all combinations
for c in combinations([i for i in range(k)], t):
    b = False
    choices = combine_columns_into_array(phf, list(c))
    for row in choices:
        # if the row is completely distinct
        if len(row) == len(set(row)):
            b = True
    # if all rows are non-distinct:
    if not b:
        true_count += 1
return true_count

section can be simplified (I think) to:

true_count = 0
# get all combinations
for c in combinations(range(len(phf[0])), t):
    s = [[x[i] for i in c] for x in phf]
    # if all rows are non-distinct:
    if not [len(i) for i in s] == [len(set(i)) for i in s]:
        true_count += 1
return true_count

And so to:

def count_non_distinct_combinations(phf, t):
    true_count = 0
    for c in combinations(range(len(phf[0])), t):
        if not any(len(set(i)) == len(i) for i in ([x[i] for i in c] for x in phf)):
            true_count += 1
    return true_count

I admit I lost readability somewhere along the line, but this new 6 line version takes 55.3% of the time needed to complete the phf2 case, so its almost twice as fast as your version. There may be improvements that can be done to the algorithm, but I'll stop here.


C++ Matrix Multiplication Using Classes I'm trying to write a C++ multiplication program using classes (for the first time - I usually try to avoid OOP) and am having trouble freeing my memory. What I'd ideally like is a matrix class with a function for allocating, initializing, multiplying, and freeing my matrices. What I currently have is

#include <iostream>
#include <cstdio>
#include <ctime>
#include <stdlib.h>

using namespace std;

class matrix
{
   static const int nrows = 40000;
   static const int ncols = 40000;

   // Allocate matrices for multiplication
   double (*matrix_a)[ncols] = (double (*)[ncols])malloc(sizeof(double) * (nrows) * (ncols));
   double (*matrix_b)[ncols] = (double (*)[ncols])malloc(sizeof(double) * (nrows) * (ncols));
   double (*matrix_c)[ncols] = (double (*)[ncols])malloc(sizeof(double) * (nrows) * (ncols));

   public:
      matrix()
      {
         // Initialize matrices
         for (int i = 0; i < nrows; i++)
         {
            for (int j = 0; j < nrows; j++)
            {
               matrix_a[i][j] = (double)rand() / (double)RAND_MAX;
               matrix_b[i][j] = (double)rand() / (double)RAND_MAX;
            }
         }
      }

      void multiplication()
      {
         // Multiply matrices
         for (int i = 0; i < nrows; i++)
         {
            for (int j = 0; j < ncols; j++)
            {
               matrix_c[i][j] = 0.0;
               for (int k = 0; k < ncols; k++)
               {
                  matrix_c[i][j] += matrix_a[i][k] * matrix_b[k][j];
               }
            }
         }
      }

      void free()
      {
         free(matrix_a);
         free(matrix_b);
         free(matrix_c);
      }
};

int main()
{
   matrix matMul = matrix();
   matMul.multiplication();
   matMul.free();
   return 0;
}

This currently works if you cut out the free() public function (and call to it within main), but I'd like to have this more like my intended description. If anyone could help me mold this into a more close format to my desired program, it would be much appreciated! Thanks in advance.

I will be assuming that you use at least C++11. It also seems that you are coming from C, is that correct?

double (*matrix_a)[ncols] = (double (*)[ncols])malloc(sizeof(double) * (nrows) * (ncols));:

There is no reason to use dynamic allocation, ncols and nrows are constant expressions!. Just use double matrix_a[ncols][nrows]; or better std::array<std::array<double,ncols>,nrows> matrix_a;. That said you usually want to have dynamic sizes or your matrix may be larger than the stack limit. Then you should use std::vector<std::vector<double>> matrix_a{nrows, {ncols}}. C-style arrays and dynamic allocation should always be limited as much as possible. Using proper class objects wrapping them is much safer. std::vector will take care of all memory management for you. If you are concerned that std::vector will layout rows non-continuously, then use std::vector<double> matrix_a{nrows*ncols} instead and properly access the elements by ncols*row+col.

Also, in C++, you do not use malloc and free. Instead you use new and delete. The difference is that new not only allocates memory but also constructs the object in that memory. In most cases you would need to manually construct the object in the allocated space (although for double it is technically ok here).
You should not declare a free method, but rather everything freeing memory belongs in the destructor, which is automatically called as soon as the object itself goes out of scope or is destroyed:

~matrix()
{
     free(matrix_a); // should be "delete[] matrix_a;"
     free(matrix_b); // ^^^
     free(matrix_c); // ^^^
}

Note that you would not need a destructor if you were using std::array or std::vector.

If you stick to dynamic memory allocation, you probably also need to implement a custom copy and/or move constructor and assignment operator.
The whole idea of a class is that it represents a certain closed/separate entity. Your class does not follow this, it is merely a wrapper to call the multilication method. If you were following OOP principles, you would create a class matrix holding one of the matrices with an overloaded operator+(const matrix&), operator*(const matrix&) and so on, then create two matrix instances in main and call * on them. Like so:

class Matrix {
private:
    std::size_t ncol;
    std::site_t nrow;
    std::vector<double> storage;
public:
    Matrix() : storage() {}
    Matrix(std::size_t ncol, std::size_t nrow) : ncol(ncol), nrow(nrow), storage(ncol*nrow) {}

    std::size_t getColCount() const { return ncol; }
    std::size_t getRowCount() const { return nrow; }

    double getAt(std::size_t col, std::size_t row) { [implement this] }
    Matrix operator+(const Matrix& other) const { [implement this] }
    Matrix operator*(const Matrix& other) const { [implement this] }
    [other operators...]

    static Matrix randomMatrix(std::size_t ncol, std::size_t nrow) { [ implement this ] }
};

int main() {
    Matrix matrixA = Matrix::randomMatrix();
    Matrix matrixB = Matrix::randomMatrix();
    Matrix matrixC = matrixA*matrixB;
}


Convert a bitarray to integer in Python I've been searching for methods to convert a bitarray into integer. Now I'm using this code:
def bin2int(bin_array):
    int_ = 0
    for bit in bin_array:
        int_ = (int_ << 1) | bit
    return int_

where bin_array is a bitarray randomly filled.
I'm curios about other methods that you know of, like functions from bitarray or numpy...

There is a method in bitarray.util called ba2int:
from bitarray import bitarray
from bitarray.util import ba2int

ba = bitarray('1001')  # 9

i = ba2int(ba)

print(f'Value: {i} type: {type(i)}')
# Output: Value: 9 type: <class 'int'>

From the doc:

ba2int(bitarray, /, signed=False) -> int
Convert the given bitarray into an integer. The bit-endianness of the
bitarray is respected. signed indicates whether two's complement is
used to represent the integer.


Pong-like game in C++ and Allegro5 I've been working on this pong game for the last 10 days, and I'd like to know of your opinions about it, what could I change to make my code better?

 #include <stdio.h>
#include <allegro5/allegro.h>
#include "allegro5/allegro.h"
#include "allegro5/allegro_image.h"
#include <allegro5/allegro_native_dialog.h>
#include <allegro5/allegro_primitives.h>
#include <allegro5/color.h>
#include <allegro5/allegro_color.h>
#include <allegro5/allegro5.h>
#include <allegro5/color.h>
#include <allegro5/allegro_font.h>
#include <allegro5/allegro_ttf.h>
#include <iostream>
int p1score = 0;
int p2score=0;

ALLEGRO_EVENT ev;
int main(){
srand (time(NULL));
int d = 200;
int f = 200;
int x = 700;
int y = 100;
int a = 100;
int b = 100;
int ballx = 200;
int bally = 200;
int ballXSpeed = 10;
int ballYSpeed = 5;
bool ballmoving = true;
al_init_image_addon();
ALLEGRO_BITMAP *Player1;
ALLEGRO_BITMAP *Player2;
ALLEGRO_BITMAP *ball;
ALLEGRO_DISPLAY *display = NULL;

ALLEGRO_EVENT_QUEUE *eventqueue = NULL;
ALLEGRO_TIMER *timer = NULL;

bool playing = true;
al_init();
al_init_primitives_addon();
display = al_create_display(800, 600);
al_install_keyboard();
eventqueue = al_create_event_queue();
timer = al_create_timer(1.0 / 60.0);
al_register_event_source(eventqueue, al_get_keyboard_event_source());
al_register_event_source(eventqueue, al_get_display_event_source(display));
al_register_event_source(eventqueue, al_get_timer_event_source(timer));
Player1 = al_load_bitmap("brick.png");
Player2 = al_load_bitmap("brick.png");
int P1W = al_get_bitmap_width(Player1);
int P1H = al_get_bitmap_height(Player1);
int P2W = al_get_bitmap_width(Player2);
int P2H = al_get_bitmap_height(Player2);

ball = al_load_bitmap("ball.png");
int ballw = al_get_bitmap_width(ball);
int ballh = al_get_bitmap_height(ball);
while (playing){
    ballYSpeed = rand() % 7;

    al_clear_to_color(al_map_rgb(255, 255, 0));
    al_wait_for_event(eventqueue, &ev);
    al_start_timer(timer);
    if (ev.type == ALLEGRO_EVENT_TIMER){

        ballx = ballx + ballXSpeed;
        bally = bally + ballYSpeed;
        if (ballx < x+P1W && ballx+ballw>x &&bally<y+P1H && ballx+ballh>y){
            ballXSpeed = ballXSpeed * -1;
            ballYSpeed = ballYSpeed*-1;
        }
        if (ballx < a + P2W && ballx+ballw>a && bally <b + P2H &&bally+ballh>b){
            ballXSpeed = ballXSpeed*-1;
            ballYSpeed = ballYSpeed*-1;
        }
        if (ballx > 800 || bally > 600){
            ballx = 400;
            bally = 400;
            p2score++;
            std::cout << "P2 :  " << p2score << std::endl;
        }
        if (ballx<-1 || bally<-1){
            ballx = 400;
            bally = 400;
            p1score++;
            std::cout << "P1:   " << p1score << std::endl;
        }
    }
        if (ev.type = ALLEGRO_EVENT_KEY_DOWN){
            switch (ev.keyboard.keycode){
            case ALLEGRO_KEY_UP:
                y -= 20;
                break;
            case ALLEGRO_KEY_DOWN:
                y += 20;
                break;
            case ALLEGRO_KEY_W:
                b -= 20;
                break;
            case ALLEGRO_KEY_S:
                b += 20;
                break;

            }
        }

        al_draw_bitmap(Player1, x, y, 0);
        al_draw_bitmap(Player2, a, b, 0);
        al_draw_bitmap(ball, ballx, bally, 0);
        al_flip_display();
    }

al_flip_display();
al_rest(5);
}

I see a number of things that could help you improve your program.

Fix the formatting

Perhaps the indentation problem is partly a cut-and-paste problem when you put it into the question, but the resulting formatting is not good, with very little indenting of code.

Fix the bugs

In this line

if (ev.type = ALLEGRO_EVENT_KEY_DOWN)

It's likely that you don't actually mean to assign the value but to test it, so it should be written instead like this:

if (ev.type == ALLEGRO_EVENT_KEY_DOWN)

Eliminate unused variables

Unused variables are a sign of poor code quality, so eliminating them should be a priority.  In this code, d, f and ballmmoving are all unused according to my compiler also tells me that.  Your compiler is probably also smart enough to tell you that, if you ask it to do so.

Eliminate global variables

My rewrite of this code uses no global variables, so clearly they are neither faster nor necessary.  Eliminating them allows your code to be more readable and maintainable, both of which are important characteristics of well-written code.  Global variables introduce messy linkages that are difficult to spot and error prone.

Check return values for errors

The calls to al_load_bitmap, al_create_event_queue, etc. can fail.  You should check the return values to make sure they did not.

Eliminate "magic numbers"

This code is littered with "magic numbers," that is, unnamed constants such as 400, 7, 600, etc.  Generally it's better to avoid that and give such constants meaningful names.  That way, if anything ever needs to be changed, you won't have to go hunting through the code for all instances of "800" and then trying to determine if this particular 800 means the width of the window or some other constant that happens to have the same value.

Break up the code into smaller functions

Rather than having everything in one long function, it would be easier to read and maintain if each discrete step were its own function.

Think of the user

Right now there is no graceful way to end the game.  This could easily be addressed by adding these three lines to the switch statment:

case ALLEGRO_KEY_Q:
    playing = false;
    break;

Additionally, the ball and the players can go off the top or bottom of the screen, making playing the game a lot more difficult and less enjoyable.

Also, I'm not an Allegro expert, but the game does not seem very responsive even though my machine is fast and has lots of memory.  Something is awry.

Use objects

Each of the players, the field and the ball could all be objects which could simplify some things here.  Consider transforming the code into a more object-oriented design.