Tick module for the game I tried to look up and suck in most of the information about optimizing this operation and this is what I came up with. As it's pretty much core of the game, I really would like to have it performant as much as I can. I would appreciate if someone can take a look at this and possibly find weak spots.

Note: I am using Browserify, hence that module.exports. Don't get confused, it is supposed to run in the browser.

module.exports = (tickModule, app) ->

    # Function to retrieve current timestamp, hopefully using window.performance object
    getTime = if (perf = window.performance)? then -> perf.now() else Date.now

    # Store the reference so there is no need for scope lookup in every tick
    raf = window.requestAnimationFrame

    # Indicates if module is running
    running = false

    # Holds identifier for cancelAnimationFrame call
    requestId = null

    # Timestamp of the last run of the tick
    previous = 0

    # Run the tick loop
    tick = ->
        return unless running

        # Request frame and store identifier
        requestId = raf tick

        # Retrieve current timestamp
        timestamp = getTime()

        # Calculate number of seconds from last tick
        delta = (timestamp - previous) * 0.001

        # Store the timestamp for the next round
        previous = timestamp

        # Emit event with delta
        app.land.emit 'tick', delta, timestamp

    # Start ticking when module start
    tickModule.addInitializer ->
        previous = getTime()
        running = true
        tick()

    # Stop ticking when module stops
    tickModule.addFinalizer ->
        running = false
        window.cancelAnimationFrame requestId

I am thinking about removing that requestId and running, since I am not really planning to stop the ticking once it starts. It was made merely like nice gesture, but it's not that useful for the game I suppose.

Do  you  really  need  a  comment  above  every  line? There are a few good comments like # Function to retrieve current timestamp, hopefully using window.performance object, but comments like # Indicates if module is running are really not useful. They add no value to the code itself, and can be removed. Preferably, you should only be using comments when parts of the code are unclear.

Secondly, your function alias raf is fairly unclear. I'd recommend renaming it to something like requestFrame. Also, if you're creating aliases for functions like this, I'd also recommend creating one for window.cancelAnimationFrame, and other functions like this.

Finally, I'd recommend renaming tick to something like tickLoop, like how it's described in the comment above it.


Fastest way to create random pixel image I want to create a very big gigantic PNG image. The image pixel are generated randomly, being
How can I improve the below code to create a fast random pixel image in java?
import java.io.File;
import java.io.IOException;
import java.awt.image.BufferedImage;
import javax.imageio.ImageIO;
public class RandomImage
{
    public static void main(String args[])throws IOException
    {
        // Image file dimensions
        int width = 30000, height = 24000;

        // Create buffered image object
        BufferedImage img = null;
        img = new BufferedImage(width, height, BufferedImage.TYPE_INT_ARGB);

        // file object
        File f = null;

        // create random values pixel by pixel
        for (int y = 0; y < height; y++)
        {
            for (int x = 0; x < width; x++)
            {
                int a = (int)(Math.random()*256); //generating
                int r = (int)(Math.random()*256); //values
                int g = (int)(Math.random()*256); //less than
                int b = (int)(Math.random()*256); //256

                int p = (a<<24) | (r<<16) | (g<<8) | b; //pixel

                img.setRGB(x, y, p);
            }
        }

        // write image
        try
        {
            f = new File("G:\\Out.png");
            ImageIO.write(img, "png", f);
        }
        catch(IOException e)
        {
        System.out.println("Error: " + e);
        }
    }
}

You can write arrays of Integers directly on the WritableRaster of the BufferedImage:
public static BufferedImage createRandomImage(final int width, final int height) {
    final BufferedImage result = new BufferedImage(width, height, BufferedImage.TYPE_INT_ARGB);

    final long bytesPerPixel = 4L;

    final int[] pixelData = new SplittableRandom().ints(bytesPerPixel * width * height, 0, 256).toArray();

    result.getRaster().setPixels(0, 0, width, height, pixelData);

    return result;
}

This performs more than three times faster on my machine in a very crude benchmark (3.92S vs 1.26S), with the caveat that for very big images like the one you desire, you have a lot of data duplication, because you basically have to allocate twice the amount of memory, since you're writing the random data to a buffer first.
This can easily be solved by writing arrays of random Integers line by line though, which nets comparable performance:
public static BufferedImage createRandomImage(final int width, final int height) {
    final BufferedImage result = new BufferedImage(width, height, BufferedImage.TYPE_INT_RGB);

    final long bytesPerPixel = 4L;

    for (int y = 0; y < height; y++) {
        final int[] pixelData = new SplittableRandom().ints(bytesPerPixel * width, 0, 256)
                                                      .toArray();

        result.getRaster().setPixels(0, y, width, 1, pixelData);
    }

    return result;
}

BufferedImage.setRGB is very slow, because for every pixel you set, it has to go fetch the corresponding data element:
public void setRGB(int x, int y, int rgb) {
    raster.setDataElements(x, y, colorModel.getDataElements(rgb, null));
}

which is a comparatively heavy operation.

Parallelization
Crude parallelization improves performance slightly, but it does not scale linearly with number of cores. On my 24 thread machine, the following yields a 30-40% improvement compared to the loop WritableRaster variant:
public static BufferedImage createRandomImage(final int width, final int height) {
    final BufferedImage result = new BufferedImage(width, height, BufferedImage.TYPE_INT_ARGB);

    final long bytesPerPixel = 4L;

    IntStream.range(0, height).parallel().forEach(y -> {
        final int[] pixelData = new SplittableRandom().ints(bytesPerPixel * width, 0, 256)
                                                      .toArray();
        result.getRaster().setPixels(0, y, width, 1, pixelData);
    });

    return result;
}


Evaluating a completely parenthesized arithmetic expression I tried to solve the following problem in a programming challenge, but the verdict was time limit exceeded.

  Completely parenthesized expression

  Write a program that reads a completely parenthesized expression, and
  prints the result of evaluating it. The three possible operators are
  sum, substraction and multiplication. The operands are natural numbers
  between 0 and 9 (both included).

  Input

  Input has a completely parenthesized expression. That is, parentheses
  always appear around subexpressions that are not digits. For instance,
  the expression 4 + 3 would be written

  ( 4 + 3 )

  The expression 8 * (4 + 3) would be written

  ( 8 * ( 4 + 3 ) )

  The expression (2 − 8) * (4 + 3) would be written

  ((2-8)*(4+3))

  Output

  Print a line with an integer number: the result of evaluating the
  given expression.

  (Source: https://jutge.org/problems/P45102_en/statement)

Some of the public test cases were (each line is a different test case):

9
( 3 + 4 )
( 8 * ( 4 + 3 ) )
( ( 2 - 8 ) * ( 4 + 3 ) )
( ( 3 * 2 ) + 1 )

This is my code, which passed every public test cases, but not the private ones. I ask your help in order to find the bottleneck of my program.

#include <iostream>
#include <sstream>

using std::string;
using std::cin;
using std::cout;

/**
 * Returns the evaluation of a completely parenthesed expression.
 *
 * Preconditions:
 *     * 0 <= i <= j < expr.size()-1.
 *     * expr is a completely parenthesed expression.
 *
 * Postcondition: returns the evaluation of expr[i..j].
 */

int evaluate(const string& expr, int i, int j)
{
    // Skip leading blanks
    while (expr[i] == ' ') {
        ++i;
    }
    while (expr[j] == ' ') {
        --j;
    }

    // Base case
    if (i == j) {
        return expr[i] - '0';
    }
    // Recursive case
    else {
        // Skip first and last parentheses
        ++i;
        --j;
        int open_parentheses = 0;
        int end_i = i;
        // Loop invariant:
        //     * expr[i..end_i) is part of the first subexpression of expr[i..j]
        //     * open_parentheses is the balance of opened and closed parentheses of expr[i..end_i)
        while (open_parentheses > 0 || (expr[end_i] != '+' && expr[end_i] != '-' && expr[end_i] != '*')) {
            if (expr[end_i] == '(') {
                ++open_parentheses;
            }
            else if (expr[end_i] == ')') {
                --open_parentheses;
            }
            ++end_i;
        }
        // Loop ending: expr[end_i] is the 'main' operation of expr[i..j]

        char operation = expr[end_i];

        // By induction hypothesis,
        //     evaluate(expr, i, end_i - 1)
        // and
        //     evaluate(expr, end_i + 1, j)
        // return the values of the first and the second subexpression of expr[i..j]

        if (operation == '+') {
            return evaluate(expr, i, end_i - 1) + evaluate(expr, end_i + 1, j);
        } else if (operation == '-') {
            return evaluate(expr, i, end_i - 1) - evaluate(expr, end_i + 1, j);
        } else {
            int first_expression = evaluate(expr, i, end_i - 1);
            // Multiplication shortcut
            if (first_expression == 0) {
                return 0;
            } else {
                return first_expression * evaluate(expr, end_i + 1, j);
            }
        }
    }
}

int main()
{
    string expr;
    getline(cin, expr);
    cout << evaluate(expr, 0, expr.size() - 1) << "\n";
}

Use Iterators
The int i and int j are unnecessary parameters in int evaluate(const std::string& expr, int i, int j).
The string class is a standard container class. Standard container classes provide the begin() and
end() functions that provide iterators to the first element and the last element of the container.
This StackOverflow.com Question highlights why iterators are good.

This StackOverflow.com question provides a possible example for finding the last character to process.

Using iterators will be faster than using indexing by integer. In C++11 or later the following loop
will advance the iterator until non-white space characters are found in the string.

for (auto CharInExperession : exper)
{
    if (!isspace(*CharInExperssion)) {
        break;
    }
}

Use Standard C++ functions to check for white space
In the example above isspace() is used, this checks for all white space, not just space so it checks for
tabs, spaces and new lines. See isspace for information on how to use it.

Reduce the Complexity of the Code
There is no reason to have the else clause for if(i == j) {, the code in the else clause can be out-dented
because of the return in the if-then portion.


Determining the number of vowels in each word from a file I'm 5 weeks into my first C++ class. I'm curious about how this code could be improved, more streamlined or simply things that I could/should have done better.

The project was:

  Read from a .txt file that has different words on separate lines.
  Determine how many vowels each word has, and print each word AND vowel count to the console. You must write and use a function outside of main().

Here's the code I wrote:

#include <iostream>
#include <string>
#include <fstream>
using namespace std;

//prototypes
void readFile();
void printWords(string);

int main()
{
    readFile();
    return 0;
}//end main

void readFile()
{
    ifstream inFile;
    string words;

    inFile.open("words.txt");

    while(!inFile.eof())
    {
        inFile>>words;
        printWords(words);
    }//end while loop

    inFile.close();
}//end readFile function

void printWords(string str)
{
    char a = 'a', e = 'e', z = 'i', o ='o', u ='u';
    int num_A = 0, num_E = 0, num_I = 0, num_O = 0, num_U = 0;

    cout<< str <<endl;

    for(int i = 0; i < str.length(); ++i)
    {
        if(str.at(i) == a)
        {
            num_A++;
        }
        if(str.at(i) == e)
        {
            num_E++;
        }
        if(str.at(i) == z)
        {
            num_I++;
        }
        if(str.at(i) == o)
        {
            num_O++;
        }
        if(str.at(i) == u)
        {
            num_U++;
        }
    }//end for loop
    cout<< "a: " << num_A <<endl;
    cout<< "e: " << num_E <<endl;
    cout<< "i: " << num_I <<endl;
    cout<< "o: " << num_O <<endl;
    cout<< "u: " << num_U << "\n" <<endl;

}//end printWord function

I feel like there should be a smoother function to extract, count and return the vowels. Then a function to print. Also, I tried to use a switch statement because there were so many if statements, but I couldn't get it to work.

Don't do this:

using namespace std;

See: Why is “using namespace std;” considered bad practice?

Pass by value?

void printWords(string);

You are not planning on modifying so pass by const reference in preference. This will prevent an extra copy. This also tells the compiler you don't want to modify the word and thus the compiler can potentially do optimizations as it does not need to worry about mutations.

Main is special.
You do not need an explicit return. If non is defined then an implicit return 0; is panted for you. In the case where there is no chance of failure I prefer not to use an explicit return.

int main()
{
    readFile();

    return 0;
}//end main

May as well create and open file in a single statement.

    ifstream inFile;
    string words;

    inFile.open("words.txt");

    // I would have used:

    std::ifstream  inFile("words.txt");
    std::string    words;

This is always wrong:

    while(!inFile.eof())

see: Why is iostream::eof inside a loop condition considered wrong?

It should be re-written as:

 while(inFile >> word)
 {
     // STUFF
 }

But you can go further. You can use iterators and loops to achieve the same affect.

 for(std::istream_iterator<std::string> loop(inFile); loop != std::istream_iterator<std::string>(); ++loop)
 {
     // STUFF
 }

Now that you can see a loop using iterators you can look at using some of the standard algorithms.

 std::for_each(std::istream_iterator<std::string>(inFile),
               std::istream_iterator<std::string>(),
               [](std::string const& word){ /* STUFF */ }
              );

Don't manually close a file:

    inFile.close();

see:  My C++ code involving an fstream failed review

As noted above:

void printWords(std::string cosnt& str)
{

If variables do not change then mark them constant.

    char a = 'a', e = 'e', z = 'i', o ='o', u ='u';

I would have used an array here:

    int num_A = 0, num_E = 0, num_I = 0, num_O = 0, num_U = 0;

Prefer not to use std::endl  unless you want to force a flush (you usually don't as it is very costly). Prefer to use "\n" this has the same effect but with no flush.

    cout<< str <<endl;

If you know your access is in bounds then prefer to use operator[] it does no checking and is thus faster. Prefer .at() if you want to force the check.

        if(str.at(i) == a)

Saying all that I would have use a standard function to count vowels.

std::count_if(std::begin(word), std::end(word), [](char l){return isVowel(l);});

bool isVowel(unsigned char l)
{
    static char vowel[] =
                    {    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
                         0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
                         0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
                         0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
                         0, 1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 0, 0, 0, 1, // aeio
                         0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // u
                         0, 1, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, 0, 0, 0, 1, // AEIO
                         0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // U
                         0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
                         0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
                         0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
                         0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
                         0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
                         0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
                         0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
                         0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
    return vowel[l];
}

My solution would be:

#include <string>
#include <fstream>
#include <iostream>

WordInfo class

class WordInfo
{
    std::string     word;
    int             count;
    public:
        WordInfo(std::string const& word)
            : word(word)
            , count(0)
        {
            count   = std::count_if(std::begin(word), std::end(word),
                                    [](char l) {return isVowel(l);});
        }
        friend std::ostream& operator<<(std::ostream& str, WordInfo const& data)
        {
            return str << data.word << " " << data.count;
        }
};

main.cpp

int main(int argc, char* argv[])
{
    if (argc < 2)
    {
        std::cerr << "Error: Wrong usage\n";
        return -1;
    }
    std::ifstream   infile(argv[1]);

    std::copy(std::istream_iterator<std::string>(infile),
              std::istream_iterator<std::string>(),
              std::ostream_iterator<WordInfo>(std::cout, "\n")
             );
}

Note: The std::copy above acts like this:

    std::string     word;

    while(infile >> word)
    {
        std::cout << WordInfo(word) << "\n";
    }


Rewrite from .. where .. select to more compact one This is the code:

public DateTime GibSomeStartDate(IEnumerable<int> partnerNumbers, DateTime startTime)
{
     return (from contract in this.databaseContext.Contract
             where partnerNumbers.Contains(contract.Pnr) && contract.SomeDateTime >= startTime
             select contract.SomeDateTime).Min();
}

databaseContext Instance is child of Entity Framework DbContext

My refactorings objectives:

make it compact
check if from where select finds nothing. In this
situation Min() will cause an Excetion

This can be accomplished with a slight modification to your method signature and an extension method:

Here, the signature now can return null if there are no records selected:

public DateTime? GibSomeStartDate(IEnumerable<int> partnerNumbers, DateTime startTime)
{
    return (from contract in this.databaseContext.Contract
         where partnerNumbers.Contains(contract.Pnr) && contract.SomeDateTime >= startTime
         select contract.SomeDateTime).SafeMin(someDateTime => (DateTime?)someDateTime);
}

This extension method will trap Min's exception and return null instead:

public static U? SafeMin<T, U>(this IEnumerable<T> source, Func<T, U?> selector) where U : struct
{
    try
    {
        return source.Min(selector);
    }
    catch
    {
        return null;
    }
}


Prime Number related class design I am creating a class which should contain all the methods related to Prime numbers. Now in that I have to implement a method isPrime() to check the primality of a number. I have to implement that in two ways one that uses the Sieve of Erastosthene and one that uses the standard for looping. The class should be made in such a way that If I use a specific constructor to instantiate it then it should use the sieve algorithm. If I use the other constructor then it should use the standard way of finding primality (through looping). Now I made this implementation keeping conceptual affinity in mind. I have made inner classes to implement both the types of implementations. Is this is the best way to do this, or should I use separate classes?

import java.util.Arrays;

/**
 * Class for Prime Number related methods.
 */
public class PrimeNumber {

    private PrimeAbstract primeNumber;

    /**
     * If you want the class to use sieve of erastosthene as the
     * base algorithm then you need to use this constructor. You need to pass the
     * limit till which you want the sieve to be initialized as the method parameter.
     * @param limit
     */
    public PrimeNumber(int limit) {
        this.primeNumber = new PrimeNumberWithSieve(limit);
    }

    /**
     * If you want the class to use the classic way of finding out the primality
     * of a number. By classic we mean for looping.
     */
    public PrimeNumber(){
        this.primeNumber = new PrimeNumberClassic();
    }

    /**
     * Returns if a number is prime or not.
     * @param number
     * @return boolean
     */
    public boolean isPrime(int number){
        return primeNumber.isPrime(number);
    }

}

/**
 * abstract class of prime number object
 */
abstract class PrimeAbstract {

    public abstract boolean isPrime(int number);
}

/**
 * This class uses sieve of erastothene as the base
 * algorithm for finding out the primality of a natural number.
 */
class PrimeNumberWithSieve extends PrimeAbstract {

    boolean[] sieve;

    public PrimeNumberWithSieve(int limit){
        sieve = limit<1000?new boolean[1001]:new boolean[limit+1];
        Arrays.fill(sieve, true);
        sieve[0]=sieve[1]=false;
        for (int i=2;i<sieve.length;i++) {
            if(sieve[i]) {
                for (int j=2;i*j<sieve.length;j++) {
                    sieve[i*j]=false;
                }
            }
        }
    }

    /**
     * This method returns if a number is prime or not.
     * It uses the sieve of erastothene method to figure this out.
     * The sieve is declared in the class and initialized on instantiating
     * the class.
     * @param number
     * @return
     */
    public boolean isPrime(int number){
        return sieve[number];
    }
}

/**
 * This class uses standard for looping for finding out the primality of a number.
 */
class PrimeNumberClassic extends PrimeAbstract{

    public PrimeNumberClassic(){
    }

    /**
     * This method returns if a number is prime or not.
     * It uses the classical style of for looping through all the numbers
     * till the sqaure root of the number.
     * @param number
     * @return
     */
    public boolean isPrime(int number) {
        if (number%2==0) return false;
        for(int i=3;i*i<=number;i+=2) {
            if(number%i==0)
                return false;
        }
        return true;
    }

}

Bugs in PrimeNumberClassic

There are multiple bugs in this class : 2 is not prime, while any number <= to 1 are considered prime :(

Some large number may results in infinite loop !!! (I'm going into more details about that at the end of my post)

Before considering the rest of my reviews, you should :

correct them
make unit tests (check JUnit) for your two classes for some cases (2, multiple even numbers not 2, 3, 5, 7, 31, 277, some really big numbers and a negative number for example)

Consider using bigger number

int aren't that big, if you plan on doing math calculations, you should consider switching to long or BigInteger.

Well now, the real review :

Review of PrimeNumber

I don't really get what this class is about. The name makes it sounds like it's storing a prime number but it's not... it actually looks like a Factory of some sort.

I'd consider removing it unless you want to add more features (in which case that may be the object of a future, follow-up, question ^^).

Review of PrimeAbstract

/**
 * abstract class of prime number object
 */
abstract class PrimeAbstract {

    public abstract boolean isPrime(int number);
}

Why is this an abstract class ? It's clearly an interface, also the javadoc is pretty useless.

The name is not very good... why PrimeAbstract ? Aren't PrimeNumberFinder/ PrimeNumberDetector name closer to the intent ?

Review of PrimeAbstract's childs

The code in the implementation really needs to breathe !

Put some spaces in it ;) it's tiring to read it as of now.

As a rule of thumb, put spaces between the = (as well as things like += ofc) sign, the '?' sign and theirs operands as well as between comparators and their operands.

if (number%2==0) return false;

I'm one of the people that don't really care about using { for very simple cases... however putting the return on the same line makes it harder to read IMO.

As said in others questions, any good IDE have a code formatter included solving all previous points (and auto-indenting your code) ;)

The Sieve is okish... I don't really like the fact that it fails with an exception when you use isPrime with a too big number. Also you should make the field private ;)

I'd probably make the sieve lazy and not compute it until isPrime is called for the first time.

1000 is a magic number and should be turned into a constant.

Finally, storing every results in an array is really space-inefficient. Maybe consider storing only the integers that are prime into an HashSet, the contains method with a HashSet is incredibly fast.

I'd optimize PrimeNumberClassic to :

know the results for some very small primes (reducing the complexity for "common" cases)
go for at most sqrt of number... yeah I know you already do that with i*i but it's less clear, likely less performant and it's buggy (if i grows large it'll overflow and you find yourself with a potentially infinite loop or accessing indexes that aren't in your array... no good)

So in the end this implementation may look like something like :

public boolean isPrime(final int number) {
    if (number == 2) {
        return true;
    } else if (number % 2 == 0) {
        return false;
    } else if (number < 36) {
        return number == 3 || number == 5 || number == 7 || number == 11 || number == 13
                || number == 17 || number == 19 || number == 23 || number == 29 || number == 31;
    }

    final int max = (int) Math.round(Math.sqrt(number));
    for (int i = 3; i <= max; i += 2) {
        if (number % i == 0)
            return false;
    }
    return true;
}


Parse and clean log files I have the code below, which works successfully, and is used to parse, clean log files (very large in size) and output into smaller sized files. This would take about 12-14 mins to process 1 GB worth of logs (on my laptop). Can this be made faster? Could Dask or parallelism or asyncio or another help speed this up?
I am new to python and pandas, and I have googled around, but am totally confused and cant seem to adopt any of the examples I saw.
import os
import pandas as pd

asciireg = "[^\x00-\x7F]+"
emailreg = "^\w+(?:[-+.']\w+)*@\w+(?:[-.]\w+)*\.\w+(?:[-.]\w+)*$"

for root, dirs, files in os.walk('.', topdown=True):
    for file in files:
        try:
            for df in pd.read_csv(file, sep='\n', header=None, engine='python', quoting=3, chunksize=1200000):
                df = df[0].str.strip(' \t"').str.split('[,|;: \t]+', 1, expand=True).rename(columns={0: 'email', 1: 'data'})

                mask = (df.email.str.contains(emailreg, regex=True, na=False)) & (~df.data.str.contains(asciireg, regex=True, na=False))
                df2 = df[~mask].copy()
                df = df[mask].copy()
                df2[['email', 'data']].to_csv("errorfile", sep=':', index=False, header=False, mode='a', compression='gzip')
                del df2
                del mask

            for x in "abcdefghijklmnopqrstuvwxyz0123456789":
                df2 = df[df.email.str.startswith(x)]
                if (df.email.size > 0):
                    df2[['email', 'data']].to_csv(x, sep=':', index=False, header=False, mode='a')
        except Exception as e:
            print ("Error: ", file)
            print(str(e))
        else:
            os.remove(file)

Sample log file
"email1@foo.com:datahere2
email2@foo.com:datahere2
email3@foo.com datahere2
email5@foo.com;dtat'ah'ere2
wrongemailfoo.com
email3@foo.com:datahere2

Expected Output

$ cat e

email1@foo.com:datahere2
email2@foo.com:datahere2
email3@foo.com:datahere2
email5@foo.com:dtat'ah'ere2
email3@foo.com:datahere2

$ cat errorfile

wrongemailfoo.com

I think there is quite a lot that could be improved on in your approach.
My main piece of advice is to try and process each line in the data only once, since each line is independent you should be able to do this.
I'm not too familiar with pandas but it seems like there are two main areas of concern.

The section where you clean up the data and filter out all the bad emails, you create a mask by executing two regexs on each line and then read through and make copies of the data frame twice using the mask. At this point you have passed over every line in the data 3 times.

    df = df[0].str.strip(' \t"').str.split('[,|;: \t]+', 1, expand=True).rename(columns={0: 'email', 1: 'data'})
    mask = (df.email.str.contains(emailreg, regex=True, na=False)) & (~df.data.str.contains(asciireg, regex=True, na=False))
    df2 = df[~mask].copy()
    df = df[mask].copy()
    df2[['email', 'data']].to_csv("errorfile", sep=':', index=False, header=False, mode='a', compression='gzip')
    del df2
    del mask

The second section where you breakdown each email into a different file if it is valid. you go through every line in the dataframe for every possible starting letter, and copy the result over to process again. At this point you have gone through each line in the data about 40 times.

for x in "abcdefghijklmnopqrstuvwxyz0123456789":
    df2 = df[df.email.str.startswith(x)]
    if (df.email.size > 0):
        df2[['email', 'data']].to_csv(x, sep=':', index=False, header=False, mode='a')

Running cProfile on the code, when it just has to read one file with 6 lines in it produces this: 336691 function calls (328148 primitive calls) in 0.974 seconds. Nearly a second to just read and process 6 lines into different files is not good.
Rather than taking a pandas approach I have just written a pure python script that sketches out an alternative strategy. Doing the same test with cProfile produces 11228 function calls (11045 primitive calls) in 0.038 seconds. It might not fit your needs exactly but you could look at it for ideas about how to tweak your script.
import re
import logging

EMAIL_REGEX = r"^\w+(?:[-+.']\w+)*@\w+(?:[-.]\w+)*\.\w+(?:[-.]\w+)*$"
OUTPUT_FILES = "abcdefghijklmnopqrstuvwxyz0123456789"

def configure_logging():
    """
    Configure a logger for each possible email start.
    """

    # TODO - Tweak the handlers, output formats and locations
    # to suit your needs

    error_handler = logging.FileHandler("error.log", mode="a")
    error_handler.setLevel(logging.ERROR)
    error_handler.setFormatter(logging.Formatter('%(message)s'))

    for entry in OUTPUT_FILES:
        logger = logging.getLogger(entry)
        handler = logging.FileHandler(f"{entry}.log", mode="a")
        handler.setFormatter(logging.Formatter('%(message)s'))
        handler.setLevel(logging.INFO)
        logger.addHandler(handler)
        logger.addHandler(error_handler)
        logger.setLevel(logging.INFO)

def gather_files():
    """
    Return all the log files that need to be processed.
    """
    # TODO - replace with your own logic to find files.
    return ["test_input.csv"]

def process_log_file(log_file_path):
    """
    For each line in the log file, process it once.
    """
    with open(log_file_path, "r") as log_file:
        for line in log_file:
            process_line(line)

def process_line(line):
    """
    Find the email and user from a line, test if the email is valid. Log the data
    to the appropriate place.
    """

    # TODO you may wish to change to logic
    # to decide if the line is valid or not.

    line = line.strip(' \t"\n')
    data = re.split(r'[,|;: \t]+', line, maxsplit=1)
    logger = logging.getLogger(data[0][0])
    if len(data) == 2 and re.match(EMAIL_REGEX, data[0]):
        logger.info(":".join(data))
    else:
        logger.error(line)

def main():
    """
    Processes each log file in turn.
    """
    for log_file_path in gather_files():
        process_log_file(log_file_path)

if __name__ == "__main__":
    configure_logging()
    main()

```


Scalability of running commands from user input Here is some code I have that has been extracted and shrunk down from a project of mine.

#include <stdio.h>
#include <stdint.h>
#include <time.h>
#include <string.h>

#define streq(x, y) (strcmp((x), (y)) == 0)
#define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))

typedef struct
{
    const char *cmd;
    void* (*fn)(void);
} __attribute__((__packed__)) Command;

void* getTime(void)
{
    return ((void*)((uintptr_t)time(NULL)));
}

void* getDay(void)
{
    time_t t = time(NULL);
    struct tm tm = *localtime(&t);
    return ((void*)((uintptr_t)(tm.tm_wday)));
}

static Command commands[] =
{
    {"time", getTime},
    {"day", getDay},
};

int main(int argc, char *argv[])
{
    for (int i = 0; i < ARRAY_SIZE(commands); ++i)
    {
        Command *p = commands+i;
        if (streq(argv[1], p->cmd)) printf("%d\n", (int)p->fn());
    }
}

When run with some given input, it runs the method associated with that input. Since this is a scaled down version of my project, it can only run two commands.

$ ./test-command day
4

Right now, my implementation should run with \$ O \left( n\right)\$ time complexity.  This would be unacceptable with the hundreds of thousands of commands my project could scale to. I am looking for reviews on scalability and optimization, and how well my Command structure and function pointer template would scale as well.

As requested.

An unstructured array may only be searched in linear order. To get better asymptotics you need a more search-friendly structure. A simplest solution would be to presort the Command array, and apply a binary search to find the command. A definite advantage here is a possibility to use standard library. You may even have the array sorted during build time.

For hundreds of thousands of commands the binary search would complete in about 20 lookups. If this is unacceptable, and your commands' names let you come up with a perfect hash function, hash is a way to go. Beware that a hash function must be really perfect - that is, almost no conflicts and almost no misses. Otherwise, the asymptotic constant would defeat all the advantages of O(1).

Update: assuming a unixish environment, the best way to sort at the build time is to have a commands.c file as

static Commands commands[] = {
#include "command-list.c"
};

with a command-list.txt text file

"time", getTime
"day", getDay

and a Makefile rule saying something like (may require some backslashes)

%.c: %.txt
    sort $< | sed -e 's/.*/{&},/' > $@


Implementing Haskell's `insert` Learn You a Haskell shows the insert function.

  insert takes an element and a list of elements that can be sorted and
  inserts it into the last position where it's still less than or equal
  to the next element. In other words, insert will start at the
  beginning of the list and then keep going until it finds an element
  that's equal to or greater than the element that we're inserting and
  it will insert it just before the element.

ghci> insert 4 [3,5,1,2,8,2]
[3,4,5,1,2,8,2]
ghci> insert 4 [1,3,4,4,1]
[1,3,4,4,4,1]

How's my implementation?

insert' :: (Ord a) => a -> [a] -> [a]
insert' x []     = [x]
insert' x yys@(y:ys) = if x <= y then x : yys else y : insert' x ys

In my experience idiomatic Haskell does not use if/else very much. Guards are definitely the preferred approach. The idea is to have the core of your logic in a function and let the checks and edge cases be handled at the edges. This also tends to end up with cleaner, more open code. Using the guard lets the reader see exactly what is happening without having to look inside the if/else branches.

insert' :: (Ord a) => a -> [a] -> [a]
insert' x []         = [x]
insert' x yys@(y:ys)
  | x <= y           = x : yys
  | otherwise        = y : insert' x ys

If you are concerned about speed, the compiler transforms the guards and the if/else into the same case structure internally. I made a simple main:

main = putStrLn $ show $ insert' 2 $ insert' 4 [3,2,1]

Below is the relevant section from ghc -O2 -ddump-simpl insert.hs. I added comments to shows where your original code is to help you read it.

Rec {
Main.main_$sinsert' [Occ=LoopBreaker]
  :: GHC.Integer.Type.Integer
     -> [GHC.Integer.Type.Integer] -> [GHC.Integer.Type.Integer]
[GblId, Arity=2, Caf=NoCafRefs, Str=DmdType LS]
Main.main_$sinsert' =
  \ (x_akb :: GHC.Integer.Type.Integer)
    (ds_dxR :: [GHC.Integer.Type.Integer]) ->
    case ds_dxR of wild_X8 {
      [] ->
        GHC.Types.:
          @ GHC.Integer.Type.Integer
          x_akb
          (GHC.Types.[] @ GHC.Integer.Type.Integer);  {- [x] case -}
      : y_ake ys_akf ->
        case GHC.Integer.Type.leInteger x_akb y_ake of _ {
          GHC.Types.False ->
            GHC.Types.:
              @ GHC.Integer.Type.Integer
              y_ake
              (Main.main_$sinsert' x_akb ys_akf); {- recurse -}
          GHC.Types.True ->
            GHC.Types.: @ GHC.Integer.Type.Integer x_akb wild_X8 {- return of final list -}
        }
    }
end Rec }


Can delegation be used to set these values at declaration time (thus reducing lines)? This class represents a single entry in a log file of the Common Log Format. Can this be improved? I think I might be able to use delegation to pluck values from the matcher. Ideally I would be able to set these values at declaration time.

Kotlin code

    data class CommonLogEntry(private val entry: String) {
        companion object {
            private val formatter: DateTimeFormatter = DateTimeFormatter.ofPattern("dd/MMM/yyyy:HH:mm:ss Z")
            private fun asDateTime(line: String) = LocalDateTime.parse(line, formatter)
            val pattern: String = "^(\\S+) (\\S+) (\\S+) \\[([\\w:/]+\\s[+\\-]\\d{4})] \"(\\S+) (\\S+)\\s*(\\S+)?\\s*\" (\\d{3}) (\\d+) \"(\\S+)\" \"(\\S+)\""
        }

        val clientIp: String
        val rfc1413identity: String
        val identity: String
        val requestTime: LocalDateTime
        val verb: String
        val resource: String
        val httpVersion: String
        val statusCode: Int
        val payloadSize: Long
        val URI: URI
        val userAgent: String

        init {
            val p = Pattern.compile(pattern)
            val m = p.matcher(entry)
            if (!m.matches() || m.groupCount() != 11)
                throw IllegalArgumentException("Log Pattern doesn't match '$pattern'")

            clientIp = m.group(1)
            rfc1413identity = m.group(2)
            identity = m.group(3)
            requestTime = asDateTime(m.group(4))
            verb = m.group(5)
            resource = m.group(6)
            httpVersion = m.group(7)
            statusCode = m.group(8).toInt()
            payloadSize = m.group(9).toLong()
            URI = java.net.URI.create(m.group(10))
            userAgent = m.group(11)
        }
    }

By moving a few things around you could easily get rid of the init block, shortening the amount of lines.

Key parts is to move the Pattern to the companion object, and use .also on the matcher to make sure it is a correct match.

data class CommonLogEntry(private val entry: String) {
    companion object {
        private val formatter: DateTimeFormatter = DateTimeFormatter.ofPattern("dd/MMM/yyyy:HH:mm:ss Z")
        private fun asDateTime(line: String) = LocalDateTime.parse(line, formatter)
        val pattern: Pattern = Pattern.compile("^(\\S+) (\\S+) (\\S+) \\[([\\w:/]+\\s[+\\-]\\d{4})] \"(\\S+) (\\S+)\\s*(\\S+)?\\s*\" (\\d{3}) (\\d+) \"(\\S+)\" \"(\\S+)\"")
    }

    private val m = pattern.matcher(entry).also {
        if (!it.matches() || it.groupCount() != 11)
            throw IllegalArgumentException("Log Pattern doesn't match '$pattern'")
    }
    val clientIp: String = m.group(1)
    val rfc1413identity: String = m.group(2)
    val identity: String = ...
    val requestTime: LocalDateTime = ...
    val verb: String = ...
    val resource: String = ...
    val httpVersion: String = ...
    val statusCode: Int = ...
    val payloadSize: Long = ...
    val URI: URI = ...
    val userAgent: String = ...

However, I have to question the reason for writing this code. I have a feeling there is already some library out there that can parse these log files, it is the "Common Log Format" after all. Also, if you want to parse log files in general I would strongly recommend the tool Splunk which does have a free license for up to 500 MB of logs per day.


Strongly connected component algorithm in Python 2.7 This is my implementation of Kosaraju's algorithm for detecting strongly connected components, post here for advice. Some special areas looking for advice,

Not sure if my current implementation for adjacent table of graph is efficient?
Any better ideas to reverse the graph to make it more efficient?
I use preVisited and visited to track what nodes are already reached by a strongly connected components, it there a better way?
Any other areas my code could improve, especially for performnace.

from collections import defaultdict
visitTimeCounter = 0

class GraphNode:
    def __init__(self, name):
        self.name=name
        self.connections = [] # list of GraphNode, directional
        self.visitTime = -1

    def DFS(self, visited):
        global visitTimeCounter
        if self.name in visited:
            return
        visited.append(self.name)

        for node in self.connections:
            if node.name in visited:
                pass
            else:
                node.DFS(visited)
        visitTimeCounter += 1
        self.visitTime = visitTimeCounter

if __name__ == "__main__":
    nodeA = GraphNode('A')
    nodeB = GraphNode('B')
    nodeC = GraphNode('C')
    nodeD = GraphNode('D')

    allNodes = [nodeA, nodeB, nodeC, nodeD]

    nodeA.connections.append(nodeB)
    nodeA.connections.append(nodeC)
    nodeB.connections.append(nodeA)
    nodeC.connections.append(nodeD)
    nodeD.connections.append(nodeC)

    visited = []

    for node in allNodes:
        if node.name not in visited:
            node.DFS(visited)

    for node in allNodes:
        print node.name, node.visitTime

    allNodesReverse=[]
    nodeIndex = defaultdict(GraphNode)
    # fork new nodes
    for node in allNodes:
        newNode=GraphNode(node.name)
        newNode.visitTime = node.visitTime
        allNodesReverse.append(newNode)
        nodeIndex[newNode.name]=newNode

    # build conections
    for node in allNodes:
        for neighbour in node.connections:
            nodeIndex[neighbour.name].connections.append(nodeIndex[node.name])

    # verify reverse connections
    for i in allNodesReverse:
        for j in i.connections:
            print i.name, '=>', j.name

    allNodesReverseOrdered = sorted(allNodesReverse, key=lambda x:x.visitTime, reverse=True)
    visited = []
    preVisited = []
    SCResults = []
    for node in allNodesReverseOrdered:
        if node.name not in visited:
            node.DFS(visited)
            # append to new SC
            sc = set()
            for node in allNodesReverseOrdered:
                if node.name in visited and not node.name in preVisited:
                    sc.add(node.name)
            for item in visited:
                preVisited.append(item)
            SCResults.append(sc)

    for sc in SCResults:
        print 'strongly connected component: '
        for i in sc:
            print i

1. Review

The code is not portable to Python 3 due to the use of the print statement.
The code is not organized into functions. This makes it hard to understand and hard to test.
Using a global variable (namely visitTimeCounter) makes code fragile (if the global variable starts out with the wrong value then it goes wrong) and hard to test (because you have to remember to reset the global variable each time you call it).
A graph node isn't the best choice of data structure here (it only has one attribute that you actually use, namely the list of its out-neighbours). A better data structure for this algorithm is a directed graph. A directed graph supports several methods that can be used by this algorithm: adding nodes and edges to the graph; iterating over the nodes; and getting the in- or out-neighbours of a node.

class Graph(object):
    """A directed graph. Nodes may be any hashable values."""

    def __init__(self, edges=()):
        """Initialize graph from optional iterable of edges."""
        self._out = defaultdict(set) # Map from node to its out-neighbours
        self._in = defaultdict(set) # Map from node to its in-neighbours
        for u, v in edges:
            self.add_edge(u, v)

    def add_node(self, u):
        """Add a node u to the graph."""
        self._out[u]
        self._in[u]

    def add_edge(self, u, v):
        """Add a directed edge to the graph, from node u to node v."""
        self.add_node(u)
        self.add_node(v)
        self._out[u].add(v)
        self._in[v].add(u)

    def in_neighbours(self, u):
        """Return the set of in-neighbours of the node u."""
        return self._in[u]

    def out_neighbours(self, u):
        """Return the set of out-neighbours of the node u."""
        return self._out[u]

    def __iter__(self):
        """Return iterator over the nodes of the graph."""
        return iter(self._out)

The constructor makes it very easy to create example graphs:

Graph('AB BA CD DC AC'.split())

(Compare this to the ten lines of code needed to create the graph in the post.)
To make the code for the algorithm easy to understand, it would make sense to closely follow the algorithm description. Wikipedia describes the algorithm like this:

  For each vertex \$u\$ of the graph, mark \$u\$ as unvisited. Let \$L\$ be empty.
  For each vertex \$u\$ of the graph do \$Visit(u)\$, where \$Visit(u)\$ is the recursive subroutine:
  If \$u\$ is unvisited then:

  Mark \$u\$ as visited.
  For each out-neighbour \$v\$ of \$u\$, do \$Visit(v)\$.
  Prepend \$u\$ to \$L\$.

  For each element \$u\$ of \$L\$ in order, do \$Assign(u,u)\$ where \$Assign(u,root)\$ is the recursive subroutine:
  If \$u\$ has not been assigned to a component then:

  Assign \$u\$ as belonging to the component whose root is \$root\$.
  For each in-neighbour \$v\$ of \$u\$, do \$Assign(v,root)\$.

which can be translated directly into Python like this:

def components(self):
    """Return list of strongly connected components."""
    visited = set()         # Set of visited nodes.
    L = []                  # Nodes in topological order.
    def visit(u):
        if u not in visited:
            visited.add(u)
            for v in self.out_neighbours(u):
                visit(v)
            L.insert(0, u)
    for u in self:
        visit(u)
    component = defaultdict(set) # Map from root to its component.
    assigned = set()       # Set of nodes assigned to a component.
    def assign(u, root):
        if u not in assigned:
            component[root].add(u)
            assigned.add(u)
            for v in self.in_neighbours(u):
                assign(v, root)
    for u in L:
        assign(u, u)
    return list(component.values())

It's much easier to compare and check this kind of directly translated code against the algorithm description than it is to check the more complicated translation in the post.

(Note that the directly translated code above has the correct functionality, but not the right complexity: inserting an item at the beginning of a list with L.insert(0, u) is inefficient (it takes time proportional to the length of the list). We can fix this by adding nodes to the end of the list instead (which is efficient). This requires three changes. First, the comment becomes:

L = []           # Nodes in reverse topological order.

Second, L.insert(0, u) becomes L.append(u). Third, the iteration over the list L proceeds in reverse order:

for u in reversed(L):

If you don't like the idea of maintaining this list in reverse order, then an alternative would be to switch L from a list to a collections.deque, which has an efficient appendleft method.)

Some quick tests:

>>> Graph('AB BA CD DC AC'.split()).components()
[{'D', 'C'}, {'B', 'A'}]
>>> Graph('AB BC CD DA AE FB DG'.split()).components()
[{'D', 'B', 'C', 'A'}, {'E'}, {'F'}, {'G'}]
>>> Graph('AB BC CD DE'.split()).components()
[{'D'}, {'B'}, {'E'}, {'C'}, {'A'}]

2. Answers to questions

() is the empty tuple. See the documentation.
Your (1) is correct. If there is a directed edge from \$u\$ to \$v\$, then \$u\$ appears before \$v\$ in the list \$L\$ (unless \$u\$ and \$v\$ belong to the same strongly connected component, in which case they could appear in either order).

But in your (2), I think you have been confused by the wording of the sentence you quoted. When Wikipedia says,

  either way \$v\$ will be prepended to \$L\$ before \$u\$ is

what it means is,

  either way \$v\$ will be prepended to \$L\$ at an earlier time than \$u\$ is prepended to \$L\$.

In other words, if there is a directed edge from \$u\$ to \$v\$, then when step 2 is complete, \$u\$ appears before \$v\$ in the list \$L\$. Hence this has the same interpretation as your (1).
The "in-neighbours" of a node \$u\$ are the nodes \$v\$ such that there is an edge from \$v\$ to \$u\$. So the in-neighbours are the set of nodes that can be reached from \$u\$ by following one edge in the reverse direction.


LeetCode: Add Two Numbers (in Kotlin) (...as I still learning Kotlin) I found this "problem":

  You are given two linked lists representing two non-negative numbers. The digits are stored in reverse order and each of their nodes contain a single digit. Add the two numbers and return it as a linked list.

  Input: (2 -> 4 -> 3) + (5 -> 6 -> 4)

  Output: 7 -> 0 -> 8

This is my solution so far:

fun add(a: IntArray, b: IntArray): Iterable<Int> {
  val result: MutableList<Int> = mutableListOf()
  var remainder = 0
  for (i in 0..Math.max(a.size, b.size) - 1) {
    val sum = a.elementAtOrElse(i, { 0 }) + b.elementAtOrElse(i, { 0 }) + remainder
    remainder = if (sum >= 10) 1 else 0
    result.add(if (remainder >= 1) sum - 10 else sum)
  }
  if (remainder > 0) result.add(remainder)
  return result
}

...test cases:

import org.assertj.core.api.Assertions.assertThat
import org.junit.Test

class AddTwoNumbersTest {
  @Test
  fun test_Add() {
    assertThat(add(intArrayOf(9, 6, 9, 5, 9), intArrayOf(1, 5, 1, 6, 1))).isEqualTo(listOf(0, 2, 1, 2, 1, 1))
    assertThat(add(intArrayOf(4, 1, 8, 7, 2), intArrayOf(9, 4, 1, 0, 3))).isEqualTo(listOf(3, 6, 9, 7, 5))
    assertThat(add(intArrayOf(2, 2, 3, 3), intArrayOf(4, 5, 6, 7))).isEqualTo(listOf(6, 7, 9, 0, 1))
    assertThat(add(intArrayOf(4, 1, 0, 8), intArrayOf(2, 2, 7, 6))).isEqualTo(listOf(6, 3, 7, 4, 1))
    assertThat(add(intArrayOf(2, 7, 1, 0), intArrayOf(0, 0, 8, 1))).isEqualTo(listOf(2, 7, 9, 1))
    assertThat(add(intArrayOf(9, 9, 9), intArrayOf(9, 9, 9))).isEqualTo(listOf(8, 9, 9, 1))
    assertThat(add(intArrayOf(9, 3, 7), intArrayOf(8, 7, 8))).isEqualTo(listOf(7, 1, 6, 1))
    assertThat(add(intArrayOf(6, 6, 6), intArrayOf(7, 8, 9))).isEqualTo(listOf(3, 5, 6, 1))
    assertThat(add(intArrayOf(0, 0, 0), intArrayOf(0, 0, 0))).isEqualTo(listOf(0, 0, 0))
    assertThat(add(intArrayOf(2, 4, 3), intArrayOf(5, 6, 4))).isEqualTo(listOf(7, 0, 8))
    assertThat(add(intArrayOf(0, 1), intArrayOf(0, 1, 2))).isEqualTo(listOf(0, 2, 2))
    assertThat(add(intArrayOf(4, 6), intArrayOf(6, 4))).isEqualTo(listOf(0, 1, 1))
    assertThat(add(intArrayOf(1), intArrayOf(9, 9))).isEqualTo(listOf(0, 0, 1))
    assertThat(add(intArrayOf(), intArrayOf(0, 1))).isEqualTo(listOf(0, 1))
    assertThat(add(intArrayOf(), intArrayOf())).isEmpty()
  }
}

It works so far...so I'm wondering if you can improve this one, or have a better approach. Notice I use everything the language offer.

Using add as a function name seems very ambiguous. The method name used in the Java Solution class from LeetCode seems more appropriate: addTwoNumbers (thereby specifying that the arguments, although not numbers themselves, represent numbers).
You can remove the explicit type specification for var result if you add explicit type arguments to mutableListOf(). I personally find this more readable and less verbose:

var result = mutableListOf<Int>()

You can use start until endExclusive instead of start..endInclusive - 1:

for (i in 0 until Math.max(a.size, b.size))

You can use the slightly shorter getOrElse instead of elementAtOrElse. You can also move trailing lambda arguments out of parentheses:

val sum = a.getOrElse(i) { 0 } + b.getOrElse(i) { 0 } + remainder

You can use an overloaded operator for adding elements to a mutable list:

result += if (remainder >= 1) sum - 10 else sum

I wouldn't use remainder >= 1 or remainder > 0 because it makes it appear that remainder can be greater than 1 which is not true. I recommend using remainder == 1.

Altogether:

fun addTwoNumbers(a: IntArray, b: IntArray): Iterable<Int> {
    val result = mutableListOf<Int>()
    var remainder = 0
    for (i in 0 until Math.max(a.size, b.size)) {
        val sum = a.getOrElse(i) { 0 } + b.getOrElse(i) { 0 } + remainder
        remainder = if (sum >= 10) 1 else 0
        result += if (remainder == 1) sum - 10 else sum
    }
    if (remainder == 1) result.add(1)
    return result
}

However, the "problem" specified in LeetCode does not use arrays which are very different from linked lists. If you convert the Java code it provides you would get the following:

/**
 * Definition for singly-linked list.
 * class ListNode internal constructor(internal var `val`: Int) {
 *     internal var next: ListNode? = null
 * }
 */
class Solution {
    fun addTwoNumbers(l1: ListNode, l2: ListNode): ListNode {

    }
}

Solving such a problem should result in a very different solution than what you've arrived at.


Chop off trailing newline for Swift's NSAttributedString I've made this extension but I'm touchy around NSAttributedStrings, keenly aware that Swift refuses to use the traditional notion of integer lengths on Unicode Strings, what with their graphemes that can compose and overlap. Also, it feels odd to convert to String to check for the newline. I'm not worried about \r because \n is the standard conversion when coming from HTML <p> tags. Also is this too long for something so simple? It feels like I'm missing someething

extension NSAttributedString {
    var trailingNewlineChopped: NSAttributedString {
        get {
            guard self.length > 0 else {
                return self
            }
            let lastCharRange = NSMakeRange(self.length - 1, 1)
            let lastChar = self.attributedSubstringFromRange(lastCharRange).string
            guard lastChar == "\n" else {
                return self
            }
            let mutableCopy = NSMutableAttributedString(attributedString: self)
            mutableCopy.deleteCharactersInRange(lastCharRange)
            return mutableCopy
        }
    }
}

Your method works correctly, as far as I can see.

For a read-only computed property, you don't need to enclose the
getter method in get {  }:

var trailingNewlineChopped: NSAttributedString {
    guard self.length > 0 else {
        return self
    }
    let lastCharRange = NSMakeRange(self.length - 1, 1)
    let lastChar = self.attributedSubstringFromRange(lastCharRange).string
    guard lastChar == "\n" else {
        return self
    }
    let mutableCopy = NSMutableAttributedString(attributedString: self)
    mutableCopy.deleteCharactersInRange(lastCharRange)
    return mutableCopy
}

Checking for a trailing newline character can be done with
the hasSuffix() method on the underlying string.

Instead of creating a mutable copy and removing the last character,
you can extract the first length-1 characters instead.

The method then becomes

var trailingNewlineChopped: NSAttributedString {
    if self.string.hasSuffix("\n") {
        return self.attributedSubstringFromRange(NSMakeRange(0, self.length - 1))
    } else {
        return self
    }
}

You can even omit the implicit self., but that is a matter of personal
preference:

var trailingNewlineChopped: NSAttributedString {
    if string.hasSuffix("\n") {
        return self.attributedSubstringFromRange(NSMakeRange(0, length - 1))
    } else {
        return self
    }
}

NSMakeRange will not be available in Swift 3 anymore, so it could
be a good idea to start replacing that by
NSRange(location:, length:) in your code.

Update: Swift 3 changed how Objective-C methods are imported
into Swift:

extension NSAttributedString {

    var trailingNewlineChopped: NSAttributedString {
        if string.hasSuffix("\n") {
            return self.attributedSubstring(from: NSRange(location: 0, length: length - 1))
        } else {
            return self
        }
    }
}


Printing out all possible combination of printable ASCII 7-bit characters to stdout This takes over 60 minutes to execute. The program is small in size, about 16kb. I am thinking that the repeated calls to printf slows it down significantly, rather than the nested for loops.

I used:

  gcc -Wall -o charsprint charsprint.c ./charsprint

Without removing the printf call: How could this source code be smaller and/or faster? Can the nested for loops be condensed using pointers?

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

/*
* print out all combinations of chars between 1 and 5 chars
* 88 keys
* char combinations:
* basic a-f[26+] 0-9[10+] SPACE TAB CAPSLK ,./;'[]\-=`[14+]
* SHIFT A-F[26+] <>?:"{}|~!@#$%^&*()_+[21+]
* 52+35 = 87 chars available
* 87^5 = 4 984 209 207 possible combinations
*/

char ascii[] = {0x00,0x01,0x02,0x03,0x04,0x05,0x06,0x07,
        0x08,0x09,0x0a,0x0b,0x0c,0x0d,0x0e,0x0f,
        0x10,0x11,0x12,0x13,0x14,0x15,0x16,0x17,
        0x18,0x19,0x1a,0x1b,0x1c,0x1d,0x1e,0x1f,
        0x20,0x21,0x22,0x23,0x24,0x25,0x26,0x27,
        0x28,0x29,0x2a,0x2b,0x2c,0x2d,0x2e,0x2f,
        0x30,0x31,0x32,0x33,0x34,0x35,0x36,0x37,
        0x38,0x39,0x3a,0x3b,0x3c,0x3d,0x3e,0x3f,
        0x40,0x41,0x42,0x43,0x44,0x45,0x46,0x47,
        0x48,0x49,0x4a,0x4b,0x4c,0x4d,0x4e,0x4f,
        0x50,0x51,0x52,0x53,0x54,0x55,0x56,0x57,
        0x58,0x59,0x5a,0x5b,0x5c,0x5d,0x5e,0x5f,
        0x60,0x61,0x62,0x63,0x64,0x65,0x66,0x67,
        0x68,0x69,0x6a,0x6b,0x6c,0x6d,0x6e,0x6f,
        0x70,0x71,0x72,0x73,0x74,0x75,0x76,0x77,
        0x78,0x79,0x7a,0x7b,0x7c,0x7d,0x7e,0x7f
        };

int main(void){
    int i,j,x,y,z;
    i = j = x = y = z = 0x00;
    char result[] = {0x00,0x00,0x00,0x00,0x00,0x00};
    for (i=0x20; i<0x7e; i++){
        result[0] = ascii[i];
        for (j=0x20; j<0x7e; j++){
            result[1] = ascii[j];
            for (x=0x20; x<0x7e; x++){
                result[2] = ascii[x];
                for (y=0x20; y<0x7e; y++){
                    result[3] = ascii[y];
                    for (z=0x20; z<0x7e; z++){
                        result[4] = ascii[z];
                        printf("%s\n", result);
                    }
                }
            }
        }
    }
    printf("\nDone...\n");
    return 0;
}

It can be smaller if you remove the pointless ascii array, like this:

int main(void){
    int i,j,x,y,z;
    i = j = x = y = z = 0x00;
    char result[] = {0x00,0x00,0x00,0x00,0x00,0x00};
    for (i=0x20; i<0x7e; i++){
        result[0] = i;
        for (j=0x20; j<0x7e; j++){
            result[1] = j;
            for (x=0x20; x<0x7e; x++){
                result[2] = x;
                for (y=0x20; y<0x7e; y++){
                    result[3] = y;
                    for (z=0x20; z<0x7e; z++){
                        result[4] = z;
                        printf("%s\n", result);
                    }
                }
            }
        }
    }
    printf("\nDone...\n");
    return 0;
}

And, it would seem logical that puts(result) should be faster than printf("%s\n", result), though the compiler might optimize both to the same thing anyway.


Userscript to redirect insecure pages to HTTPS I am developing a userscript that redirects insecure pages (HTTP) to the https: version.

// ==UserScript==
// @name         Secure Redirect
// @namespace    http://tampermonkey.net/
// @version      0.1
// @description  A simple userscript that redirects pages to https:.
// @author       You
// @match        *
// @grant        none
// ==/UserScript==

(function() {
    'use strict';
    window.onload=()=>{
        if (location.protocol != "https:") {
            window.location.href = "https:" + window.location.href.substring(window.location.protocol.length);
        }
    };
})();

Please tell me if it works and what I can improve on. My GitHub repo for this project is here.

Overview

Pass global variables as parameters to IIFE to make your code more independent and clear
Use addEventListener method to follow recommended way of attaching event listeners
Do you really need to execute redirection code, only when everything is loaded?
I would use regex to replace protocol to make solution robust
Try to not duplicate constant values (in this case protocol)

Suggested solution

(function (w) {
    'use strict';

    var secureProtocol = 'https';

    if ( !w.location.protocol.startsWith(secureProtocol) ) {
        w.location.href = w.location.href.replace(/^[^:]+/, secureProtocol);
    }
})(window);


"Guess number" game in Python What could I improve on this code?

from random import randint
guesses = 0
randomNum = (randint(0,100))
numPlayer = 0
while (numPlayer!=randomNum):
    numPlayer = int(input("Guess the number(0-100)"))
    if (numPlayer>randomNum) :
        print "It's -"
    elif (numPlayer<randomNum) : print("It's +")
    guesses=guesses+1

print ("Well done you guessed my number in %d guesses" % guesses)

I rewrote your code, let me explain my changes line by line:

from random import randint

The same.

from itertools import count

You need to use modules in Python, itertools is incredibly useful and allows to avoid while and use instead the for loop (manual incrementing is terribly un-idiomatic in Python)

def number_guessing(minimum, maximum):

This is a function, you may use it as many times as you want, even in the same program with different arguments. Taking optional parameters for the messages for ultimate customization is left as an exercise for the reader.

randomNum = randint(minimum, maximum)

We use the parameters as literals in the call to randint, not just numbers.

for tries in count(1):

count() just counts, from 1 (the argument) to infinity. tries is just a number that counts the iterations of a loop, but automatically this time.

numPlayer = int(input("Guess the number({}-{})".format(minimum, maximum))

String interpolation, so the user sees a message that contains the appropraite numbers, not always 0 - 100.

    if numPlayer > randomNum:
        print ("My number is smaller"  )
    elif numPlayer < randomNum:
        print ("My number is bigger")

Using spaces around operators, removing unnecessary parentheses, and using messages that seem more understandable to me. (Your homework will make this messages fully customizable, so if you do not like my choices do not worry)

    elif numPlayer == randomNum:
        return tries

If the player guessed correctly our function halts and returns (gives back) how many tries it took for the user to guess the number.

print ("Well done you guessed my number in {} guesses".format(
    number_guessing(0, 50)))

We just call the function we wrote and print its result, that is how many tries it took.

Final code

Just to play around and implement the change I suggested:

from random import randint
from itertools import count

def number_guessing(minimum, maximum):
    randomNum = randint(minimum, maximum)
    for tries in count(1):
        numPlayer = int(input("Guess the number({}-{})".format(minimum, maximum)))
        if numPlayer > randomNum:
            print ("My number is smaller"  )
        elif numPlayer < randomNum:
            print ("My number is bigger")
        elif numPlayer == randomNum:
            return tries

print ("Well done you guessed my number in {} guesses".format(
    number_guessing(0, 2)))


Writing an itoa function Here is a basic itoa function that I've written trying to use recursion:
void itoa(int number, char buffer[])
{
    static int idx=0, size=0;
    if (number == 0) {
        buffer[size] = '\0';
        idx = size = 0; // reset for next call
    }
    else {
        size++, idx++;
        itoa(number/10, buffer);
        buffer[size-idx--] = (number % 10) + '0';
    }
}

How does it look? It is not thread-safe due to the static, but is that an issue for a function such as this? If so, how could I update that? What other ways could I make improvements to it?

How does it look?

Not so good.

Not thread safe.

Does not handle negative numbers well.

Forms "" with 0.

Prone to buffer overflow.

itoa() is not a standard function, yet it commonly returns a pointer.

A test harness
int main(void) {
  char buf[100];
  int test[] = {123, 456, 0, - 42, INT_MAX, INT_MIN};
  int n = sizeof test / sizeof *test;
  for (int i = 0; i < n; i++) {
    itoa(test[i], buf);
    printf("%-11d <%s>\n", test[i], buf);
  }
  return 0;
}

Output
123         <123>
456         <456>
0           <>
-42         <,.>
2147483647  <2147483647>
-2147483648 <./,),(-*,(>

It is not thread-safe due to the static, but is that an issue for a function such as this?

Yes.  Thread safety is expected.

If so, how could I update that? What other ways could I make improvements to it?

I really do not think this is a good place to use recursion given the potential for buffer overflow is a fair complication.
But if one must use recursion, consider adding error checking and test for various sorts of int including 0, INT_MAX, INT_MIN.
static char* itoa_helper(int number, size_t sz, char buffer[]) {
  if (sz == 0) {
    return NULL;
  }
  if (number <= -10) {
    buffer = itoa_helper(number / 10, sz - 1, buffer);
    if (buffer == NULL) {
      return NULL;
    }
    number %= 10;
  }
  *buffer = (char) ('0' - number);
  return buffer + 1;
}

char* itoa_recursive_alt(int number, size_t sz, char buffer[sz]) {
  if (sz == 0 || buffer == NULL) {
    return NULL;
  }
  char *s = buffer;

  if (number >= 0) {
    // Flip pos numbers to neg as neg range is greater.
    number = -number;
  } else {
    sz--;
    if (sz == 0) {
      *buffer = '\0';
      return NULL;
    }
    *s++ = '-';
  }
  s = itoa_helper(number, sz-1, s);
  if (s == NULL) {
    *buffer = '\0';
    return NULL;
  }
  *s = 0;
  return buffer;
}

int main(void) {
  char buf[100];
  int test[] = {123, 456, 0, -42, INT_MAX, INT_MIN};
  int n = sizeof test / sizeof *test;
  for (int i = 0; i < n; i++) {
    // char *s = itoa_recursive_alt(test[i], sizeof buf, buf);
    char *s = itoa_recursive_alt(test[i], sizeof buf, buf);
    if (s == NULL)
      s = "NULL";
    printf("%-11d <%s> <%s>\n", test[i], s, buf);
  }
  return 0;
}

Output
123         <123> <123>
456         <456> <456>
0           <0> <0>
-42         <-42> <-42>
2147483647  <2147483647> <2147483647>
-2147483648 <-2147483648> <-2147483648>


compareTo method for names I'm implementing the compareTo method to set the natural order of an object. My object has three fields that make up a name. fName, lName, middleInitial. The natural ordering should be by lName, fName, middleInitial. I was wondering if this is an efficient way of implementing the compareTo.

    @Override
public int compareTo(Employee o) {
    StringBuilder builder = new StringBuilder();
    String thisName;

    if(!StringUtils.isEmpty(this.lName.trim())){
        builder.append(this.lName);
    }
    if(!StringUtils.isEmpty(this.fName.trim())){
        builder.append(this.fName);
    }
    if(!StringUtils.isEmpty(this.midInitial.trim())){
        builder.append(this.midInitial);
    }
    thisName = new String(builder.toString());
    builder = new StringBuilder();

    if(!StringUtils.isEmpty(o.lName.trim())){
        builder.append(o.lName);
    }
    if(!StringUtils.isEmpty(o.fName.trim())){
        builder.append(o.fName);
    }
    if(!StringUtils.isEmpty(o.midInitial.trim())){
        builder.append(o.midInitial);
    }

    if(!StringUtils.isEmpty(thisName) && !StringUtils.isEmpty(builder.toString())){
        return thisName.compareTo(builder.toString());
    }else{
        return 0;
    }

}

Correctness

How should these names be ordered?

SMITH, ARIEL B.
SMITH, ZINNIA X.
SMITHSON, HARRIET

The way your code works, you smash all the components of each name together, like "SMITHZINNIAX" and "SMITHSONHARRIET", resulting in Harriet Smithson coming before Zinnia Smith.  Thus, you'll end up Ms. Smithson ordered between the two Smiths, violating the expectation that all employees with the last name Smith be listed together.

Therefore, you should be comparing by lName first, proceeding to compare by fName only to break ties.
If one of the employees has no name (lName, fName, and midInitial are all empty strings after trimming), then this compareTo() method will report it as being equal to all other employees!

Performance

That's a huge amount of string manipulation to do a comparison!  That's a bad idea, because if you want to sort a list of employees, you will likely end up doing many of these comparisons, and each comparison will be much slower than it should be.

You should be able to implement this compareTo() method without any string concatenation or trimming.

Design

Why do you need to .trim() any of these name components?  Is your data unclean?  Why are initial and trailing spaces being stored in any of these fields?  Such spaces would not be considered a legitimate meaningful part of a name, so why aren't they either rejected or sanitized in the constructor or setter methods?
Unlike numbers or strings, which have an inherent natural ordering, there is no obvious reason why employees should be ordered according to their names.  What if you want to sort a list of employees by their salary instead?  Instead of writing this Employee.compareTo() method, consider moving this code out into a Comparator class instead:

import java.util.Comparator;

public class Employee {
    public String getLastName() { … }
    public String getFirstName() { … }
    public String getMidInitial() { … }

    public static final Comparator<Employee> COMPARE_BY_NAME = new Comparator<Employee>() {
        @Override
        public int compare(Employee a, Employee b) {
            int cmp;
            if (0 != (cmp = a.getLastName().compareTo(b.getLastName()))) {
                return cmp;
            }
            if (0 != (cmp = a.getFirstName().compareTo(b.getFirstName()))) {
                return cmp;
            }
            return a.getMidInitial().compareTo(b.getMidInitial());
        }
    };
}

Since Java 8, you can also write:

public class Employee {
     …

    public static final Comparator<Employee> COMPARE_BY_NAME =
        Comparator.comparing(Employee::getLastName)
                  .thenComparing(Employee::getFirstName)
                  .thenComparing(Employee::getMidInitial);
    }
}


Class for course information Here's what the question asks:

  Build a class called Course.java. This class should have 4 properties: CourseId, CourseName, Description, and credit hours. Also add the appropriate set and get methods. Add a display() method to display these 4 properties out to the DOS window. Lastly add a main() method for testing. In the main() method instantiate a Course object, fill it with data using the set methods, then call the display method to display the data.

I just want to make sure is my code right and if there are mistake in my code.

public class Course {

//   ========================== Properties ===========================
private int courseid;
private String courseName;
private String description;
private String creditHours;

//   ==========================  Behaviors  ==========================
public void setCourseId(int c) { courseid = c; }
public int getCourseId() { return courseid;}

public void setCourseName(String cn) { courseName = cn; }
public String getCourseName() { return courseName;}

public void setDescription(String d) { description = d; }
public String getDescription() { return description;}

public void setCreditHours(int ch) { ch = 4; }
public int getCreditHours() { return ch;}

//Returning String
public String toString() {
    return courseName + ":" + description + ":" + creditHours;
}

public void display() {
    System.out.println("Course ID             = " + getCourseId());
    System.out.println("Course Name      = " + getCourseName());
    System.out.println("Description      = " + getDescription());
    System.out.println("Credit Hours           = " + getCreditHours());

} //end display()

public static void main(String args []) {

    Course c1;
    c1 = new Course();

    c1.setCourseId(109);
    c1.setCourseName("Intro to Python");
    c1.setDescription("This course intros the Python Prog Lang.");
    c1.setCreditHours(4);

    c1.display();

    //Test out toString() method
    System.out.println(c1);
} //end main
} //end class

Here's the list of things I would change:

Indentation (make it 2 or 4 tabs)
Field naming is inconsistent. For example, courseid should be changed to courseId. Secondly, you should append the prefix course to all fields, or omit it at all. (Don't forget to change your methods as well).
Assuming that this is a POJO (Plain Old Java Object) class, then I would recommend removing the display() method from this class. It's simply not its responsibility to display itself(S from SOLID principles). Instead, you can modify your toString() method to look something like this:

`

public String toString() {
   return "Course ID:" + id + '\n' +
            "Course name:" + name + '\n' +
            "Course description:" + description + '\n' +
            "Credit hours:" + creditHours + '\n';
}

`
And then use it in a different object/function whose responsibility is to present information to the user.

From my personal experience, I can say that the id field should be a String. If you are getting data from a server, and then convert that data to a POJO, the id field may cause an overflow in some situations if it's an integer. Even longdata type may cause an overflow if the id value is very long.
I don't think it's a professional way to declare and initialize an instance of a class the way it was written. You can do both in a single line.

This is how I would have written it:

public class Course {

  private String id;
  private String name;
  private String description;
  private float creditHours;

  public void setId(String id) {
     this.id = id;
  }

  public String getId() {
     return id;
  }

  public String getName() {
     return name;
  }

  public void setCreditHours(float creditHours) {
     this.creditHours = creditHours;
  }

  public void setName(String name) {
     this.name = name;
  }

  public void setDescription(String description) {
     this.description = description;
  }

  public String getDescription() {
     return description;
  }

  public String toString() {
     return "Course ID:" + id + '\n' +
            "Course name:" + name + '\n' +
            "Course description:" + description + '\n' +
            "Credit hours:" + creditHours + '\n';
  }

  public static void main(String args[]) {
    final Course course = new Course();
    course.setCreditHours(15.6f);
    course.setDescription("BlaBla");
    course.setId("12345567878665345453123");
    course.setName("Just a boring class");

    System.out.print(course.getDescription());
  }
}

It contains some other corrections as well.

Hope this helps!

As a side note, though it seems absolutely superfluous, I would like you to know that's it's not my intention to criticize your code, I'm simply expressing my opinion based on the experience I have.


Grouping events in jQuery This is a jQuery time update events for a video player with multiple ids. I feel the code is too long and I need to add more ids to fire events.

 $(document).ready(function() {
           var i = 0;
           var a = 0;
           var b = 0;
           var c = 0;
           var j = 0;

           $("#r-news-week1, #r-news-week2").bind("play", function() {
                if (i < 1) {
                    console.log("Video Started");
                    ga('send', 'event', 'Videos', 'Started', 'Video');
                }
                i = i + 1;
            });

             $("#r-news-week1, #r-news-week2").bind("timeupdate", function() {
                  var currentTime = this.currentTime;
                  if (currentTime > 0.75 * (this.duration)) {
                    if (c < 1) {
                      console.log("watched 75%");
                      ga('send', 'event', 'Videos', 'Watched 75%', 'Video');
                    } c = c + 1;
                  } else if (currentTime > 0.50 * (this.duration)) {
                      if (b < 1) {
                      console.log("watched 50%");
                      ga('send', 'event', 'Videos', 'Watched 50%', 'Video');
                    } b = b + 1;
                  } else if (currentTime > 0.25 * (this.duration)) {
                      if (a < 1) {
                      console.log("watched 25%");
                      ga('send', 'event', 'Videos', 'Watched 25%', 'Video');
                    } a = a + 1;
                  }
              });

            $("#r-news-week1, #r-news-week2").bind("ended", function() {
                if (j < 1) {
                    console.log("Finished 100%");
                    ga('send', 'event', 'Videos', 'Finished 100%', 'Video');
                }
                j = j + 1;
            });

        })

If you can us the timeupdate event to detect start and finish events you could use a single event listener block.  You could also put the events an an array and increment the current position in the video being listened for.  Something similar to this:

function bindVideoEvents(ids) {
    var _events = [
        { time: 0.00, name: 'Started' },
        { time: 0.25, name: '25% Watched' },
        { time: 0.50, name: '50% Watched' },
        { time: 0.75, name: '75% Watched' },
        { time: 1.00, name: 'Finished 100%' }
    ];
    var _evPos = {};

    $.each(ids, function (idx, id) {
        _evPos[idx] = 0;
        $('#' + id).on('timeupdate', function () {
            var currentEvent = _events[_evPos[idx]];
            if (this.currentTime > (currentEvent.time * this.duration)) {
                console.log(currentEvent.name);
                ga('send', 'event', 'Videos', currentEvent.name, 'Video');
                _evPos[idx]++;
            }
        });
    });
}


Object pool pattern for asynchronous socket operations I have written my own object pool class called ObjectPool<T> which uses SpinLock and Stack<T>:

public class ObjectPool<T> where T : class
{
    private int _size;
    private Func<T> _factory;
    private SpinLock _spinLock;

    // storage for the pool objects.
    // the first item is expected to be most often case.
    private T _firstItem;
    private Stack<T> _cache; //Use Queue<T> for first-in-first-out (FIFO)

    public ObjectPool(Func<T> factory) : this(factory, Environment.ProcessorCount * 2) { }

    public ObjectPool(Func<T> factory, int size)
    {
        _factory = factory;
        _size = size;
        _cache = new Stack<T>();
        _spinLock = new SpinLock(false);
    }

    /// <summary>
    /// Produces an instance.
    /// </summary>
    public T Rent()
    {
        bool lockTaken = false;
        _spinLock.Enter(ref lockTaken);

        try
        {
            T instance = _firstItem;
            if (instance == null || instance != Interlocked.CompareExchange(ref _firstItem, null, instance))
            {
                instance = RentFromCache();
            }

            return instance;
        }
        finally
        {
            if (lockTaken)
            {
                _spinLock.Exit(false);
            }
        }
    }

    private T RentFromCache()
    {
        var cache = _cache;

        if (cache.Count > 0)
        {
            T instance = _cache.Pop();

            if (instance != null)
            {
                return instance;
            }
        }

        return CreateInstance();
    }

    private T CreateInstance()
    {
        var instance = _factory();
        return instance;
    }

    /// <summary>
    /// Returns objects to the pool.
    /// </summary>
    public void Return(T obj)
    {
        bool lockTaken = false;
        _spinLock.Enter(ref lockTaken);

        try
        {
            if (_firstItem == null)
            {
                // worst case scenario: two objects may be stored into same slot.
                _firstItem = obj;
            }
            else
            {
                ReturnToCache(obj);
            }
        }
        finally
        {
            if (lockTaken)
            {
                _spinLock.Exit(false);
            }
        }
    }

    private void ReturnToCache(T obj)
    {
        var cache = _cache;

        if (cache.Count >= _size)
        {
            // not a big fan of doing it this way
            return;
        }

        // worst case scenario: two objects may be stored into same slot.
        cache.Push(obj);
    }
}

I'm currently using this object pool to get high performance out of a Socket by making a pool of asynchronous socket operations (ObjectPool<SocketAsyncEventArgs>).

Note: I'm receiving UDP packets, which might help illustrate why some quirks exist (that won't work well for some generic-cases) in this object pool's design, such as using a LIFO pool.

It's working great so far. However, I'd appreciate some expert code review. I am hoping for advice on where it can be improved, further optimized, or if I have missed something important.

Additional Questions:

I'm wondering if ConcurrentStack<T> would provide me with any added benefits?

Because you don't change

private int _size;
private Func<T> _factory;
private Stack<T> _cache;

private SpinLock _spinLock; //Use Queue<T> for first-in-first-out (FIFO)

you should make them readonly.

Correction after the comment of t3chb0t:

Don't make the SpinLock readonly. Further clarification: https://stackoverflow.com/a/9235028/2655508

  The underlying problem is that the C# compiler creates a copy of a readonly value type field when you call a non-static method on it and executes that method on the copy - because the method could have side effects that change the value of the struct - which is not allowed for readonly fields.

private T RentFromCache()
{
    var cache = _cache;

    if (cache.Count > 0)
    {
        T instance = _cache.Pop();

        if (instance != null)
        {
            return instance;
        }
    }

    return CreateInstance();
}

Here you are using var cache only for checking its Count property. I don't see a reason to not use the class level _cache directly. Using the class level variable directly won't confuse the reader of the code. The same applies to ReturnToCache() as well.


Finding unique items in the array I was reading this code where author claims that its the fastest implementation which I doubt, hence I decided to write my version of the same problem.

'use strict';

module.exports = function unique(arr) {
  if (!Array.isArray(arr)) {
    throw new TypeError('array-unique expects an array.');
  }
  return arr.filter(function(element, index) {
    return arr.indexOf(element) === index;
  });
};

The above code looks readable to me but I don't about its performance(I think mine should be faster).

Your implementation is elegant, clean, easy to read.

The other is less so, but there is a significant difference between the two: your version returns a new array of unique values, the other removes duplicate values, modifying the input array.

Both algorithms are \$O(n^2)\$. Your implementation includes one redundant comparison per element: when the indexOf call reaches the current element, it compares it to itself, which is a redundant comparison for your ultimate intent of finding unique values. The other implementation optimized this with the var j = i + 1; step: it compares the current element to all subsequent element, never to itself.

In the end, the performance difference will come down to the implementation of splice and new array creation of the given JavaScript engine. I don't think such comparison will be very useful, except in very rare extreme cases. Also keep in mind that premature optimization is the root of all evil. And once again, there's also the matter of the different behavior of the implementations (return new unique array or mutate the parameter in-place), which makes comparisons problematic.

For what it's worth, in cases when an \$O(n^2)\$ solution to this problem is good enough (as opposed to an \$O(n)\$ solution using \$O(n)\$ extra space), I would go for your version, because it's intuitively easier to read.


Comparing 32- and 64-bit A few days ago I saw this question on Stack Overflow.

I have tried to make a little demo to show the speed advantage of using 64-bit memory operations compared to 32-bit ones. So I wrote this code:

#include <stdio.h>
#include <stdlib.h>
#include <sys/time.h>

#define MEGABYTES 1024 * 1024
#define MEMORY_SIZE 100
#define LOOPS_START 100
#define LOOPS_STOP 10001

int main (int argc, const char * argv[])
{
    clock_t start, stop;
    int64_t* big_pile = malloc(MEGABYTES * MEMORY_SIZE * sizeof(char));

    for (int loops = LOOPS_START; loops < LOOPS_STOP; loops += 100)
    {
        start = clock();

        for (int i = 0; i < loops; i++)
        {
            for (int pos = 0; pos < MEGABYTES * MEMORY_SIZE * sizeof(char) / sizeof(int64_t); pos++) {
                big_pile[pos] = big_pile[pos] ^ big_pile[pos];
            }
        }

        stop = clock();
        double elapsed = (double)(stop - start) / CLOCKS_PER_SEC * 1000;
        double average = elapsed / loops;
        printf("Loops:\t%i\tElapsed time:\t%f\tAverage:\t%f\n", loops, elapsed, average);
    }

    free(big_pile);
}

I compile it under XCode on a MacBook Air running OSX Lion. This allow me to change the build target to 32- or 64-bit. When I run the program (and verify that the program is actually running in the correct bit-mode) I see no real difference in time.

Is this the right way to measure performance difference between 32- and 64-bit?
Is XCode/the compiler playing tricks on me?
Is there anything else wrong?

Note Sorry for the C. C isn't my native tongue ;-)

Solution findings

Writing 100 MB memory executing in 64-bit mode using 64-bit integer pointers takes ~47 seconds. Exceuting in 32-bit mode using 64-bit integer pointers takes ~70 seconds. Executing in 32-bit mode using 32-bit integer pointers takes ~90 seconds.

You are using int64_t, so most of your code gets compiled into the same machine code regardless of the bitness of the target. Also, A^A = 0, so the compiler might be optimizing your entire for loop with a single call to memset with zero.

I would recommend that you move different values around instead of xoring the same values together, and I would recommend that you use int32_t for the 32-bit version, and int64_t for the 64-bit version.


Stack using Linked List in Swift I made this implementation of a stack in Swift that stores its elements as a linked list. It seems to work perfectly well, but I’m wondering how I can improve it to follow the best practices of the language.

struct StackList<T> : CustomStringConvertible {
    private var first: StackNode? = nil // The topmost node in the stack

    /// Add a new element to the top of the stack
    /// - parameter newElement: the new element to be added
    mutating func push(_ newElement: T) {
        let newElement = StackNode(newElement);
        newElement.next = first;
        first = newElement;
    }

    /// Remove and return the topmost element from the stack
    /// - returns: Optional containing the removed element, or nil if the stack was empty
    mutating func pop() -> T? {
        guard let first = first else { return nil }

        let poppedElement = first.element
        self.first = first.next
        return poppedElement
    }

    /// The number of elements in the stack
    var size: Int {
        var count = 0
        var current = first

        // Traverse the list
        while current != nil {
            count += 1
            current = current!.next
        }
        return count
    }

    /// Textual representation of the stack
    var description: String {
        var output = ""

        // Traverse the list
        var currentNode = first;
        while let thisNode = currentNode {
            if output != "" { output += ", " }  // Add commas between elements
            output += String(describing: thisNode.element)

            currentNode = thisNode.next
        }

        return "[\(output)]"
    }

    private class StackNode {
        var next: StackNode? = nil
        var element: T

        init(_ element: T) {
            self.element = element
        }
    }
}

That looks already quite good and clean. Here are my remarks:

private var first: StackNode? = nil // The topmost node in the stack

An optional value is initalized to nil by default, so

private var first: StackNode? // The topmost node in the stack

is sufficient. And with a different variable name it becomes self-explaining:

private var top: StackNode?

There are trailing semi-colons at some places, these are not needed in Swift.

Here

    let newElement = StackNode(newElement)
    newElement.next = first
    first = newElement

you pass one property of the new node as parameter of the init method, and
set the other via accessor, which looks asymmetric to me. With

private class StackNode {
    var next: StackNode?
    let element: T

    init(_ element: T, next: StackNode?) {
        self.element = element
        self.next = next
    }
}

this simplifies to

    let newElement = StackNode(newElement, next: first)
    first = newElement

or even

    first = StackNode(newElement, next: first)

Note also that the element property in StackNode can be a constant.

A better property name for

/// The number of elements in the stack
var size: Int

might be count – that is what all Swift collection types use. The forced
unwrap in

    // Traverse the list
    while current != nil {
        count += 1
        current = current!.next
    }

is safe, but can even be avoided with

    // Traverse the list
    while let node = current {
        count += 1
        current = node.next
    }

A common practice is to put the implementation of protocols into separate
extensions:

extension StackList: CustomStringConvertible {
    var description: String { ... }
}

Naming: class StackNode is defined within the StackList “namespace,”
so you can simply name it Node. And I would name StackList just
Stack – it is an implementation detail that it uses a linked list.

Further suggestions

Make StackList conform to Sequence. A simple implementation would be

extension StackList: Sequence {
    func makeIterator() -> AnyIterator<T> {
        var current = first
        return AnyIterator {
            guard let node = current else { return nil }
            defer { current = node.next }
            return node.element
        }
    }
}

That allows to enumerate all elements easily:

for item in stack { print(item) }

In addition, you can use it in the description method which then simplifies
to

extension StackList: CustomStringConvertible {
    var description: String {
        return "[" + self.map { "\($0)" }.joined(separator: ", ") + "]"
    }
}

Implement a peek() method to get the top element without removing it,
and an isEmpty property.


Parse and extract synonym data from a website I made a method that finds synonyms to words using thesaurus.com and I'm looking for comments and feedback to it. In what ways can I improve it, both when it comes to speed, security, reliability (regardless of how "reliable" it is to rely on a third-party website for look-ups), etc.

    ///
    /// This method relies heavily on thesaurus.com for synonym lookups. It is not completely reliable, but is deemed reliable enough in instances where you dont have your own thesaurus
    ///
    public static string[] GetSynonyms(string word)
    {
        string url = string.Format("http://thesaurus.com/search?q={0}", word);

        HttpWebRequest request = (HttpWebRequest)WebRequest.Create(url);
        HttpWebResponse response = (HttpWebResponse)request.GetResponse();
        if (response.StatusCode == HttpStatusCode.OK)
        {
            List<string> synonyms = new List<string>();
            StringBuilder data = new StringBuilder();
            string line;

            using (StreamReader reader = new StreamReader(response.GetResponseStream()))
            {

                //we know that the synonyms is in the upper-part of the html stream so we do not want to read the entire stream.
                while((line = reader.ReadLine()) != null) {
                    var index = line.IndexOf("");

                    if(index > 0)
                    {
                        index = index + "".Length;
                        synonyms.Add(line.Substring(index).Replace("", ""));
                    }

                    //break when we come to the Antonyms section of the page
                    if (line.Contains("container-info antonyms"))
                    {
                        break;
                    }
                }
            }
            return synonyms.ToArray<string>();
        }
        else
        {
            return null;
        }
    }

Edit: As an example, it now takes about 3.5 seconds to find the synonyms for the word "old".

You're doing things the hard way.   By that I mean, why build a web crawler for this?   Is it a requirement that you only use thesaurus.com?   There are several quality thesaurus services out there with APIs that you could utilize.

I've never used any of them, but a quick web search and a bit of sifting results and these ones appear to be at the head of the thesaurus API ballgame.

Altervista Thesaurus:  Thesaurus is a web service providing search capability for synonyms in different languages.
The synonyms are retrieved from the thesauri dictionaries of OpenOffice according to the relevant licenses.
Big Huge Thesaurus: This site sports a very simple API for retrieving the synonyms for any word. You can access the API by making a GET request...

I also found another Stack Exchange post on the topic, which lists some more options:
https://webapps.stackexchange.com/questions/9156/database-of-english-synonyms

If thesaurus.com is absolutely a requirement, then you're doing the best you can.  You have to send the request, crawl the page, extract the information you want, and format it into something you can use.

thesaurus.com does have an API, but from how they describe it, its difficult to get them to give you access.  You can read more about that here:

thesaurus.com API


Making a directory structure for a web project I have always been into HTML and CSS so I decided to give programming a try. I installed Python because I heard it is the best first programming language. So I made this program just now and I'm super excited and happy about it. I would like to have your criticism or feedback and tell me how I can make it better or add more functionality to it.

import os

name = input("Enter the name of your project here: ");

os.makedirs(name);

os.chdir(name);

os.makedirs('css');
os.makedirs('resources');

os.chdir('resources');

os.makedirs('img');
os.makedirs('js');

os.chdir('../');

html = input("Enter name of html file: ")+".html";
open(html, 'a');

os.chdir('css');

css = input("Enter name of css file: ")+".css";
open(css, 'a');

I think it would be nice, if you could define the directory structure once and then let the program create it. Something like this:

import os

folders = ["css", "resources/img", "resources/js"]

name = input("Enter the name of your project here: ")
for folder in folders:
    os.makedirs(os.path.join(name, os.path.normpath(folder)))
os.chdir(name)
...
# file opening like Peilonrayz suggested

This uses a list and os.path.join, which allows joining filenames with the platform specific file name separator (\ for Windows, / for UNIX), os.path.normpath to convert the / in the path to \ in case you are on Windows and it also uses a for-loop, if you have not seen them, yet.