Distributing cards to players in C++ I have written this program to deal 7 cards to one player.

#include <iostream>
#include <string>
#include <cstdlib> //for rand and srand
#include <cstdio>
#include <vector>
using namespace std;

string suit[] = {"Diamonds", "Hearts", "Spades", "Clubs"};
string facevalue[] = {"Two", "Three", "Four", "Five", "Six", "Seven",         "Eight", "Nine", "Ten", "Jack", "Queen", "King", "Ace"};
int numberOfCardsDrawn = 0;
string drawnCards[52];

string drawCard () {
    string card;
    int cardvalue = rand()%13;
    int cardsuit = rand()%4;
    card += facevalue[cardvalue];
    card += " of ";
    card += suit[cardsuit];
    return card;
}

bool isMyCardAlreadyDrawn (string card) {
    for(int i = 0; i < 52; i++) {
        if(card.compare(drawnCards[i]) == 0) {
            // if this is true, then both strings are the same
            return true;
        }
    }
    return false;
    // if the code reaches this point, then the card has not   been drawn yet
}

string getCard () {
    srand(time(0));
    string card = drawCard();
    while (isMyCardAlreadyDrawn(card) == true) {
        // keep drawing until an    undrawn card is found
        card = drawCard(); // draw a new card
    }
    drawnCards[numberOfCardsDrawn] = card;
    numberOfCardsDrawn++;
    return card;
}

int main () {
    cout << "Your Cards:\n";
    vector<string> p0; //player 0's card
    const int DEAL_CARDS = 7; //number of cards we can deal to each player
    string choices[] = { "a", "b","c","d","e","f","g" };

    for (int i = 0; i < DEAL_CARDS; i++){
        string p0_getCard = getCard();
        cout <<" " << choices[i] << ")"<< p0_getCard << " ";
        p0.push_back(p0_getCard);
    }
}

However, I will eventually want to have a five-player game (dealing cards to four more vectors), and I feel that extending this code to accomplish that would result in excessive code duplication.  How do I go about improving the code to support that?

I see a number of things which may help you improve your code.

Don't abuse using namespace std

Especially in a very simple program like this, there's little reason to use that line.  Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Don't reseed the random number generator more than once

The program currently calls srand at the top of each call to getCard(), but this is really neither necessary nor advisable.  Instead, just call it once when the program begins and then continue to use rand() to get random numbers.  Better yet, use the next suggestion.

Consider using a better random number generator

You are currently using

int cardsuit = rand()%4;

There are two problems with this approach. One is that the low order bits of the random number generator are not particularly random, so neither with cardsuit be.  On my machine, there's a slight but measurable bias toward 0 with that.  The second problem is that it's not thread safe because rand stores hidden state.  A better solution, if your compiler and library supports it, would be to use the C++11 `std::uniform_int_distribution.  It looks complex, but it's actually pretty easy to use.

Eliminate global variables where practical

Having routines dependent on global variables makes it that much more difficult to understand the logic and introduces many opportunities for error.  Eliminating global variables where practical is always a good idea, whether programming for desktop machines or for embedded systems.  For global variables such as drawnCards and `numberOfCardsDrawn, consider wrapping them in objects to more easily keep them synchronized.

Use object-oriented programming

The game is written much more in the procedural style of C rather than in the object-oriented style of C++.  The cards and the deck could each be an object, with most of the procedures as functions of those objects.  This would reduce coupling and make the program easier to understand.

Eliminate "magic numbers"

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

Use const where practical

In your isMyCardAlreadyDrwan routine, the passed card is never altered, which is just as it should be.  You should indicate that fact by declaring it like this:

bool isMyCardAlreadyDrawn (const std::string& card)


CTCI Chapter 1 : Palindrome Permutation Below is my code for the problem 1.4 in CTCI. I would like a review of my code and if my approach to the problem is correct.

Problem Statement:
Palindrome Permutation: Given a string, write a function to check if it is a permutation of
a palindrome. A palindrome is a word or phrase that is the same forwards and backwards. A
permutation is a rearrangement of letters. The palindrome does not need to be limited to just
dictionary words.

EXAMPLE

Input: Tact Coa

Output: True (permutations:"taco cat'; "atco cta'; etc.)

My Solution (Python):

def checkPalindromeAndPermutation(inputstr):
    lengthOfInputString = len(inputstr)
    counterforodd = 0
    actualCharactersInInput = 0
    inputstr = inputstr.lower()
    hashTable = dict()

    for i in inputstr:
        if i != " ":
            hashTable[i] = hashTable.get(i, 0) + 1
            actualCharactersInInput = actualCharactersInInput + 1
    print(hashTable)

    for item in hashTable:
        # if input has even length, but each character's frequency is not even, then it's not a plaindrome
        if actualCharactersInInput % 2 == 0 and hashTable[item] % 2 != 0:
            return False
        # if input has odd length, but more than one character's frequency is greater than 1 , then it's not a plaindrome
        if actualCharactersInInput % 2 == 1 and hashTable[item] % 2 == 1:
            counterforodd = counterforodd + 1
            if counterforodd > 1:
                return False
    return True

print("Answer : " , checkPalindromeAndPermutation("abc bac"))

Nice implementation.

Here are a couple suggestions

collections.defaultdict

Intead of hashTable[i] = hashTable.get(i, 0) + 1, use collections.defaultdict

charcount = defaultdict(int)

for char in inputStr:
    charcount[char] += 1
actualCharactersInInput = len(inputStr) - charcount[' ']

collections.Counter

Or better yet, use a Counter:

charcount = collections.Counter(inputStr)
actualCharactersInInput = len(inputStr) - charcount[' ']

other

if actualCharactersInInput % 2:
    # odd number of chars
    return sum(count%2 for count in charcount.values()) == 1
else:
    # even number of chars
    return not any(count % 2 for count in charcount.values())


Usage of Decorator pattern in baking program Is this a good usage of Decorator pattern?  I'm trying to use Interfaces instead of Abstract classes.

(Component) Interface

public interface IBake
    {
        string Name { get; }
        double Price { get; }
   }

(Concrete Component)

 public class BakeBase : IBake
    {
        private string m_Name = "Cake Base";
        private double m_Price = 200.00;

        public string Name
        {
            get
            {
               return m_Name;
            }
        }

        public double Price
        {
            get
            {
                return m_Price;
            }
        }

        public string GetCakeCost()
        {
            return "Normal cake is:" + m_Price + "and Tax is: " + (this.m_Price * 0.08f);
        }

    }

Decorator

class CherryDecorator
{
    IBake _iBakeModel;

    public CherryDecorator(IBake iBakeModel)
    {
        _iBakeModel = iBakeModel;
    }

    public string GetName()
    {
        return "Cherry Decorated Cake!";
    }
    public double GetPrice()
    {
        return _iBakeModel.Price + 100;
    }
}

Decorator

class CreamDecorator
    {
        IBake _iBakeModel;

        public CreamDecorator(IBake iBakeModel)
        {
            _iBakeModel = iBakeModel;
        }

        public string GetName()
        {

            return "Cream Decorated Cake!";

        }
        public double GetPrice()
        {

            return _iBakeModel.Price + 50;

        }
    }

Client

 public class Client
{
static void Main(string[] args)
{

    BakeBase bakebase = new BakeBase();
    Console.WriteLine(bakebase.GetCakeCost());
    Console.ReadKey();
    Console.WriteLine(Environment.NewLine);
    CherryDecorator cd = new CherryDecorator(bakebase);
    Console.WriteLine(cd.GetName() + "  " + cd.GetPrice());
    Console.ReadKey();

    Console.WriteLine(Environment.NewLine);
    CreamDecorator crd = new CreamDecorator(bakebase);
    Console.WriteLine(crd.GetName() + "  " + crd.GetPrice());
    Console.ReadKey();

}
}

I'm getting the following, which is not the right total:

You missed a somewhat crucial point in the pattern: the decorator must present the exact same interface as the decorated object.

In other words, any IBake decorator should also implement the IBake interface, so that it's possible to decorate a cake with both cream and cherries!

Here's your CreamDecorator, fixed:

class CreamDecorator : IBake
    {
        public static readonly decimal ExtraCharge = 50;
        private readonly IBake _decorated;

        public CreamDecorator(IBake decorated)
        {
            _decorated = decorated;
        }

        public string Name
        {
            get
            {
                var name = _decorated.Name;
                return name + " +Cream";
            }
        }

        public double Price
        {
            get
            {
                var price = _decorated.Price;
                return price + ExtraCharge;
            }
        }
    }

The idea is that you call the decorated code, and then decorate the result before returning it.

Also since we're talking about money, double is a bad choice of a type for Price. IBake should look like this:

public interface IBake
{
    string Name { get; }
    decimal Price { get; }
}

As for the wrong price being output, there's your bug:

public string GetCakeCost()
{
    return "Normal cake is:" + m_Price + "and Tax is: " + (this.m_Price * 0.08f);
}

The base class doesn't know whether it's decorated or not, and shouldn't make that assumption, it's just another implementation of the IBake interface:

public static readonly decimal TaxRate = 0.08;

public string GetCakeCost()
{
    return string.Format("Price: {0:C2}\nTax: {1:C2}\nTotal: {2:C2}", Price, Price * TaxRate, Price * (1 + TaxRate));
}

Now that looks cleaner, but it's also flawed: Cost is a bad/confusing term, should be Price, SellingPrice, or TotalPrice - Cost could refer to what the bakery is paying to produce the cake, and would be used to calculate profits and gross margins. The tax rate shouldn't be hard-coded into a method that returns a string either.

Another problem is that the decorator would need to override the Price property for the base class to return the correct figure, and that's not how decorators work.

One solution could be to include a TaxAmount member in the IBake interface, and a separate member to return a string representation of the cake's price, which I'd call PriceDetails:

public interface IBake
{
    string Name { get; }
    decimal Price { get; }
    decimal TaxAmount { get; }
    string PriceDetails { get; }
}

The base cake could do it like this:

public decimal TaxAmount
{
    get { return TaxRate * Price; }
}

public string PriceDetails
{
    get { return return string.Format("{0}\nPrice: {1:C2}\nTax: {2:C2}\nTotal: {3:C2}", Name, Price, TaxAmount, Price + TaxAmount); }
}

And then the decorators have nothing to change about these two, so they just call the decorated code:

public static readonly decimal ExtraCharge = 50;

public decimal TaxAmount
{
    get { return _decorated.TaxAmount; }
}

public string PriceDetails
{
    get { return _decorated.PriceDetails; } // add an "Extra" in the detail line?
}

At the end of the day, what you want to ensure is that the client code is always calling into the decorated instance - that's why it's important for decorators to present the same interface as the decorated type, and for all members to be accessed by the client code via that interface.

Here's what your client code could look like in a production application:

public class CakeOrderLine
{
    private readonly IBake _cake;
    public CakeOrderLine(IBake cake)
    {
        _cake = cake;
    }

    public override string ToString()
    {
        return _cake.PriceDetails;
    }
}

An "order line" is injected with an IBake instance, but has no clue how/if it's decorated, it treats all implementations of IBake equally.

Your console client could do this:

var cake = new BakeBase();
var cherryCake = new CherryDecorator(cake);
var creamCherryCake = new CreamDecorator(cherryCake);

And then this:

Console.Write("{0:C2}\n", cake.PriceDetails);
Console.Write("{0:C2}\n", cherryCake.PriceDetails);
Console.Write("{0:C2}\n", creamCherryCake.PriceDetails);

Should output something like this, assuming Price is implemented as above:

Cake Base
Price: $200.00
Tax: $16.00
Total: $216.00

Cake Base +Cherry
Price: $250.00
Tax: $20.00
Total: $270.00

Cake Base +Cherry +Cream
Price: $350.00
Tax: $28.00
Total: $378.00

I'm not a UML-ist, I think this fairly represents the concept:

See the full code here.


Using PHP's __call() to emulate a Class instead of Extending I recently started working on a project that had been dropped by the previous dev. When looking through the existing code, I came across this singleton Database Class.

class Database{
private static $instance;
private $PDOInstance;

// Customize when ready
private function __construct(){
    try{
        $this->PDOInstance = new PDO("mysql:database=testdb;unix_socket=/tmp/mysql.sock", "username" , "password"/* , array driver_options*/);
    }catch(PDOException $e){
        throw $e;
    }
}

public static function getInstance(){
    if(!self::$instance){
        try{
            self::$instance = new self();
        }catch(PDOException $e){
            return false;
        }
    }
    return self::$instance;
}

public function __clone(){
    trigger_error("Cloning forbidden on Singleton", E_USER_ERROR);
}

public function __wakeup(){
    trigger_error("Unserializing forbidden on Singleton", E_USER_ERROR);
}

// PDO Function Aliasing
public function __call($function, $args){
    if(method_exists($this->PDOInstance, $function)){
        return call_user_func_array(array($this->PDOInstance, $function), $args);
    }
    trigger_error("Unknown PDO Method Called: $function()\n", E_USER_ERROR);
}
}

I suppose my question revolves around the use of __call() to alias PDO functions. Is this the best way to go about it, and if not, what should be done instead?

As for why the database class doesn't simply extend PDO, from what I understand, PDO has a public __construct(), and when it gets extended, it isn't possible to change the visibility to private, hence this work around.

My instinct is to scrap this class and just rewrite it as a simple PDO wrapper, but I thought I would ask around first.

A simple workaround, assuming that you want to keep the class a Singleton:

class Database {
    private static $instance;
    private $PDOInstance;

    public static function getInstance(){
        if(!self::$instance){
            try{
                self::$instance = new self();
            }catch(PDOException $e){
                return false;
            }
        }
        return self::$instance->getPDOInstance();
    }

    public function getPDOInstance() {
        return $this->PDOInstance;
    }
}

Well, more of a factory method now, which is silly. But not as silly as __call() / call_user_func_array. If you absolutely need to keep the class's signature as it is, go for it.

If not, a PDO wrapper would be the better approach.


Recursion to map phone number to strings I am trying to solve the following via recursion: In a phone we have each digit mapped to a number.

Example:

1       2(ABC)  3(DEF)
4(GHI)  5(JKL)  6(MNO)
7(PRS)  8(TUV)  9(XYZ)
*       0       #

I need to print out all the possible sequences of a phone number (example: 637-8687).

I did the following that seems to solve it:

static Map<String, String> digits;
    static{
        digits = new HashMap<String, String>();
        digits.put("2", "ABC");
        digits.put("3", "DEF");
        digits.put("4", "GHI");
        digits.put("5", "JKL");
        digits.put("6", "MNO");
        digits.put("7", "PRS");
        digits.put("8", "TUV");
        digits.put("9", "XYZ");
    }

    public static void printSequences(String phoneNumber, Map<String, String> associations){
        if(phoneNumber == null || phoneNumber.isEmpty()){
            throw new IllegalArgumentException();
        }
        if(associations == null || associations.isEmpty()){
            throw new IllegalArgumentException();
        }
        String[] letters = new String[phoneNumber.length()];
        for(int i = 0; i < phoneNumber.length(); i++){
            String seq = associations.get(phoneNumber.charAt(i) + "");
            if(seq == null)continue;
            letters[i] = seq;
        }
        printSequences(letters, 0, letters.length, "");
    }
private static void printSequences(String[] letters, int start, int end, String prefix) {

        if(start == end){
            System.out.println(prefix);
            return;
        }

        String sequence = letters[start];
        if(sequence == null)
            return;

        for(int i = 0; i < sequence.length(); i++){

            printSequences(letters, start + 1, end, prefix + sequence.charAt(i));
        }

    }

The String[] letters has all the relevant digits (extracted from a Map). In this case it is [MNO, DEF, PRS, TUV, MNO, TUV, PRS] for phone 6378687.

This code seems to work but does not for 637-8687.  For 637-8687 it actually prints nothing at all!

I am trying to practice recursion, so could you please help me out with this one?  Also, is recursion actually the best approach on such a problem?

Here are some comments from my side:

The end parameter is unnecessary. You can just test for start >= letters.length instead. Also, you might want to rename start.
It might make sense to wrap your algorithm in a callable method such as private void printSequences(String[] letters), which just kicks off the recursion by calling printSequences(letters, 0, "").
As to your bug, it depends on how exactly you are calling the method above (what is start and end).
Your test for letters[start] == null does not seems necessary if the letters array is well-behaved (i.e., does not contain null).
Recursion is a decent way to solve this problem.
Your bug comes from the fact that when you process "-" as a digit, you generate null in letters. When processing letters, you don't call your function recursively but just return. That way, nothing is ever printed, which explains the behavior you describe. The proper way to handle that case would be to call the method recursively without appending anything to the prefix (or to make sure there are no null entries in letters, which I believe is the better way, see point 7).
I would use a List<String> for letters. That way you don't end up with null entries.
Your IllegalArgumentException should describe the problem. For instance, it could say "Got illegal phoneNumber".
Your digits map should map characters to strings, not strings to strings. That way you don't have to coerce the character into string with phoneNumber.charAt(i) + "".


Sieve of Eratosthenes in C# with LINQ Can someone look over this Sieve of Eratosthenes implementation? It seems almost too easy.

Rather than maintaining a seperate bit[] to track prime/not prime, I'm just removing the noncandidates from the collection completely on each iteration.

Pseudocode

LIST = 2...n

set M = 1
while M < sqrt(n)
   set M = next number in LIST > M
   remove all multiples of M (excluding M itself) from LIST

C#

int cur = 1, total = 1000;
var pc = Enumerable.Range(2, total).ToList();

while(cur <= Math.Sqrt(total))
{
    cur = pc.First(i => i > cur);
    pc.RemoveAll(i => i != cur && i % cur == 0);
}

Console.WriteLine(pc.Max());

It just seems a bit too easy. Results seem right though. In LINQPad 4

Runs total = 100000; in 0.008 secs
Runs total = 1000000; in 0.141 secs
Runs total = 10000000; in 2.973 secs

Yes, it works, but it's slow.

I compared it to this:

bool[] notPrime = new bool[total];
notPrime[0] = true;
notPrime[1] = true;
for (int i = 2; i <= Math.Sqrt(notPrime.Length); i++) {
  if (!notPrime[i]) {
    for (int j = i * 2; j < notPrime.Length; j += i) {
      notPrime[j] = true;
    }
  }
}

(I used Enumerable.Range(2, total - 2) in the Linq code to make it produce the numbers 2 to 99999 rather than 2 to 100001.)

For total = 100000, average for 100 executions:

Linq 27.696966 ms., 0.280000 collections
Array 0.708616 ms., 0.030000 collections

So, it takes a lot of time, and does more garbage collections.


Tower of Hanoi: graphical representation of optimal solution I wrote a program to show graphically the solution of the problem of the Tower of Hanoi. Here is a video of an example run.

The logic of the solving is taken from StackOverflow (see links in docstring), I wrote the part dealing with the graphics, so review is most welcome on the graphical part that I wrote.

The code is pretty simple, tested and documented, so no further introduction is needed.

# -*- coding: utf-8 -*-
import doctest
import time
import pygame

SPACE_PER_PEG = 200

def hanoi(pegs, start, target, n):
    """
    From stackoverflow:
      http://stackoverflow.com/questions/23107610/towers-of-hanoi-python-understanding-recursion
      http://stackoverflow.com/questions/41418275/python-translating-a-printing-recursive-function-into-a-generator

    This function, given a starting position of an hanoi tower, yields
    the sequence of optimal steps that leads to the solution.

    >>> for position in hanoi([ [3, 2, 1], [], [] ], 0, 2, 3): print position
    [[3, 2], [], [1]]
    [[3], [2], [1]]
    [[3], [2, 1], []]
    [[], [2, 1], [3]]
    [[1], [2], [3]]
    [[1], [], [3, 2]]
    [[], [], [3, 2, 1]]

    """
    assert len(pegs[start]) >= n, 'not enough disks on peg'
    if n == 1:
        pegs[target].append(pegs[start].pop())
        yield pegs
    else:
        aux = 3 - start - target  # start + target + aux = 3
        for i in hanoi(pegs, start, aux, n-1): yield i
        for i in hanoi(pegs, start, target, 1): yield i
        for i in hanoi(pegs, aux, target, n-1): yield i

def display_pile_of_pegs(pegs, start_x, start_y, peg_height, screen):
    """
    Given a pile of pegs, displays them on the screen, nicely inpilated
    like in a piramid, the smaller in lighter color.
    """
    for i, pegwidth in enumerate(pegs):

        pygame.draw.rect(
            screen,
            # Smaller pegs are ligher in color
            (255-pegwidth, 255-pegwidth, 255-pegwidth),
            (
              start_x + (SPACE_PER_PEG - pegwidth)/2 , # Handles alignment putting pegs in the middle, like a piramid
              start_y - peg_height * i,         # Pegs are one on top of the other, height depends on iteration
              pegwidth,
              peg_height
            )
        )

def visual_hanoi_simulation(number_of_pegs, base_width, peg_height, sleeping_interval):
    """
    Visually shows the process of optimal solution of an hanoi tower problem.
    """
    pegs = [[i * base_width for i in reversed(range(1, number_of_pegs+1))], [], []]
    positions = hanoi(pegs, 0, 2, number_of_pegs)

    pygame.init()
    screen = pygame.display.set_mode( (650, 650) )
    pygame.display.set_caption('Towers of Hanoi')

    for position in positions:
        screen.fill((255, 255, 255))
        for i, pile in enumerate(position):
            display_pile_of_pegs(pile, 50 + SPACE_PER_PEG*i, 500, peg_height, screen)
        pygame.display.update()
        time.sleep(sleeping_interval)

    pygame.quit()

if __name__ == "__main__":
    doctest.testmod()
    visual_hanoi_simulation(
        number_of_pegs = 4,
        base_width = 30,
        peg_height = 40,
        sleeping_interval = 0.5
    )
    visual_hanoi_simulation(
        number_of_pegs = 8,
        base_width = 20,
        peg_height = 30,
        sleeping_interval = 0.2
    )

Very nice code!

Herea are a few small things:

Since your function hanoi is a generator, you don't need to save it to a variable, you can use it directly in the for loop:

for position in hanoi(pegs, 0, 2, number_of_pegs):

According to PEP8:

  Compound statements (multiple statements on the same line) are
  generally discouraged.

reversed(range(1, number_of_pegs+1)) is the same as range(number_of_pegs, 0, -1)
You calculate 255-pegwidth three times in a row, just use (255 - pegwidth,)*3 and use that.


Creating Two-Dimensional String Array for Plane Seats I'm creating a ticket reservation system in Java, I want to use a Two-dimensional String array for managing the seats. I have enums that hold the plane model and the amount of seats available so the array pulls the rows and columns from there. I just want to see efficient my code is and how can I improve it.
    public String[][] createSeatArray() {
    String[] alphabet = {"a","b","c","d","e","f","g","h","i","j","k","l","m","n","o","p","q","r"
            ,"s","t","u","v","w","x","y","z","aa","bb","cc","dd","ee","ff","gg","hh","ii","jj","kk","ll","mm","nn","oo","pp"};

    String[][] seatArray = new String[this.getNumOfRows()][this.getNumOfColumns()];
    int alphabetPos = 0;
    for(int i = 0; i < this.getNumOfRows(); i++) {
        for (int j = 1; j <= this.getNumOfColumns(); j++){
            String columnToString = String.valueOf(j);
            seatArray[i][j-1] = alphabet[alphabetPos] + columnToString;

            if (this.getNumOfColumns() == 4) {
                if (j == 2) {
                    System.out.print(seatArray[i][j-1] + "       ");
                } else {
                    System.out.print(seatArray[i][j-1] + " ");
                }
            } else {
                if (j == 3) {
                    System.out.print(seatArray[i][j-1] + "       ");
                } else {
                    System.out.print(seatArray[i][j-1] + " ");
                }
            }
        }
        System.out.println("\n");
        alphabetPos++;
    }
    return seatArray;
}

Thank you for any feedback! also keep in mind that if the indentation is off it may be due to bringing it over to stack overflow, on intellij it's perfect.

Here are a few optimization suggestions:

You can get rid of alphabetPos because it is always equals to i.
In fact, while you are at it, your code will be more readable if you rename i to row and j to col or column.

You can also get rid of columnToString and just rely on Java's string concatenation overloads.

 seatArray[i][j-1] = alphabet[alphabetPos] + j;

Note that you use j multiple times as index, subtracting one, and only once as string.
It would make your code more readable if you used zero based index, and only added 1 for the string composition.

You may want to use string formatting.

for (int col = 0; col < this.getNumOfColumns(); col++) {
        seatArray[row][col] = String.format("%s%d", alphabet[row], col + 1);

        if (this.getNumOfColumns() == 4) {
            if (col == 1) {
                System.out.print(seatArray[row][col] + "       ");
            } else {
                System.out.print(seatArray[row][col] + " ");
            }
        } else {
            if (col == 2) {
                System.out.print(seatArray[row][col] + "       ");
            } else {
                System.out.print(seatArray[row][col] + " ");
            }
        }
}

You can simplify your array printing by splitting it in to two parts: printing the seat number and printing the spacer.
The first part is always the same, so no need to repeat it 4 times.
The second part changes only when col and row have certain values, so one condition is enough.

System.out.print(seatArray[row][col] + " "); //this part is always printed the same

//just add a few more spaces if this is an isle seat
if ((getNumOfColumns() == 4 && col == 1) || (getNumOfColumns() != 4 && col == 2)) {
    System.out.print("      ");
}

Finally, to save memory, you can get rid of the alphabet array all together, since the seat letters are sequential and rows 26 and above just have the same letter twice:

String rowStr = String.format("%c", 'a' + (row % 26));
if (row > 25) rowStr += rowStr;
seatArray[row][col] = String.format("%s%d", rowStr, col + 1);


PHP login form with PDO I create login form with session with four files:

connection.php
login.php
plan.php
logout.php

Please check my code and tell about any smokestack.

Database educate (create table users)

CREATE TABLE users(
id INT AUTO_INCREMENT,
username VARCHAR(50) NOT NULL,
password VARCHAR(100) NOT NULL,
PRIMARY KEY(id) );

Connection.php

<?php
$dsn = 'mysql:host=localhost;dbname=educate'; //Data source Name
$username = 'root';
$password = '';
$options = array(PDO::MYSQL_ATTR_INIT_COMMAND=> 'SET NAMES utf8');

 $conn = new PDO($dsn, $username, $password,$options);
 ?>

login.php

<?php
session_start();
if(!empty($_SESSION['username'])) {
header('location:plan.php');
}
require 'connection.php';

if(isset($_POST['login'])) {

$user = $_POST['username'];
$pass = $_POST['password'];

if(empty($user) || empty($pass)) {
$message = 'All field are required';
} else {
$query = $conn->prepare("SELECT username, password FROM users WHERE
username=? AND password=? ");
$query->execute(array($user,$pass));
$row = $query->fetch(PDO::FETCH_BOTH);

if($query->rowCount() > 0) {
  $_SESSION['username'] = $user;
  header('location:plan.php');
} else {
  $message = "Username/Password is wrong";
}

}

}
?>

<!DOCTYPE html>

<?php
if(isset($message)) {
echo $message;
}
?>
<form action="#" method="post">
Username: <input type="text" name="username" placeholder="username">

Password: <input type="password" name="password" placeholder="password">

<input type="submit" name="login" value="Login">

</form>

logout.php

<?php
session_start();
session_destroy();
header('location: login.php');
?>

plan.php

<?php
session_start();

if(isset($_SESSION['username'])) {
echo "Welcome ".$_SESSION['username']."
";
} else {
header('location: login.php');
}

?>
Logout

Always authenticate the user before you take any restricted action.

In login.php you have:

session_start();
if(!empty($_SESSION['username'])) {
header('location:plan.php');
}

This code has 2 vulnerabilities I can think of:

It assumes that the existence of username in the session is proof of authenticity. If your user doesn't log out, anyone who uses his browser much later to come to your site will be admitted; likewise anyone who logs HTTP requests and later looks at the log will find the session cookie and can impersonate the user. To fix, store a valid_until date in the session when the user logs in.  Then when you check for credentials, you only admit the user if the login validity has not expired.
Your site is vulnerable to Cross-Site Request Forgery (CSRF). Suppose your logged-in user visits my website where I have something like:

.

<--! Make user send request to your site without knowing -->
<img src="http://yoursite.com/plan.php?do=evil">

Because browsers send cookies automatically with every request, the user's session cookie will be sent to plan.php when the browser tries to load the image, and your script will treat the request as legitimate, therefore it will act on ?do=evil. To fix, set a token in your response headers when the user logs in, and expect the same token back from the request headers from legitimate requests.

The two points above lead to a modification of login.php. When the user logs in, you now do something like:

$_SESSION['username'] = $user;
$_SESSION['valid_until'] = time() + 60*60; //1hr from now
$_SESSION['csrf_token'] = 'long_random_string'; //research how to build this
header("X-CSRF: $_SESSION[csrf_token]");
header('location:plan.php');

When your user comes to my evil site and I try to connect to plan.php, the browser will still send the session cookie but it will not send the CSRF token, so before you grant access you would check for it:

session_start();
$logged_in = false;
if(isset($_SESSION['username'])){
    if(!$_SESSION['valid_until'] >= time()) $logged_in = false;
    $request_headers = apache_request_headers();
    $csrf = isset($request_headers["X-CSRF"])? $request_headers["X-CSRF"] : null;
    if($_SESSION['csrf_token']!==$csrf) $logged_in = false;
}
if($logged_in){/*give access*/}
else{/*block access*/}

By the way, this logic should be moved to a function that returns TRUE if the user is logged in, false otherwise. You can extend it with further checks. For instance you could record the IP address to which the session was granted, and if it doesn't match the IP of the request, deny access (to protect against an attacker stealing the session and CSRF token and trying to re-use it from another location).

In login.php you have:

$user = $_POST['username'];
$pass = $_POST['password'];
...
$query = $conn->prepare("SELECT username, password FROM users WHERE
username=? AND password=? ");
$query->execute(array($user,$pass));

It's great that you use prepared statements to prevent SQL Injection, but this code is evidence that you're storing the passwords in clear text, without hashing. This means that if your database gets stolen, the thief will know your users' credentials, and can use them to impersonate the users on other services. It also means that anyone with access to the database can do the same thing. Instead, when a user registers, hash the password before saving it.

$password = password_hash($_POST['password'],PASSWORD_DEFAULT);

Then when a user logs in, you verify the submitted password against the hashed password. Do something like:

$query = $conn->prepare("SELECT username, password FROM users WHERE username=?");
... // <-execute query and capture password in $db_pass

if($query->rowCount() > 0 && password_verify($_POST['password'],$db_pass)){
    //credentials are correct

}else{/*user unknown or password incorrect*/}


A small api that delays api requests using async/await with Node.js I spent a good couple hours troubleshooting a bug where the yelp-fusion api said I was sending too many requests per second, so I finally found a way to delay each api request to make it work. Please take a look at the code and give me any tips you have. I'd like to see how this could be done in a better way if possible.

Here is my server.js where post requests come through:

  //handle post requests for getting yelp data
  app.post('/post', async function(req, res) {
    let yelpInData = []
    let result = null;
    req.body.forEach((item) => {
      yelpInData.push(item);
    });

    result = await handleYelp(yelpInData);
    //send data back to the front end
    res.send(result);

  });

  //handle yelp api
  async function handleYelp(yelpInData) {
    let yelpData = [];

    for (let i = 0, len = yelpInData.length; i < len; i++) {
      let result = await searchPlace(yelpInData[i]);
      yelpData.push(result);
    }
    //  console.log(yelpData);
    return yelpData;

  }

  function searchPlace(city) {
    //console.log(city)
    return new Promise((resolve) => {
      setTimeout(() => {
        let coords = city.coords.lat + "," + city.coords.lng;
        let name = city.name;

        client.search({term: name, location: coords}).then(response => {

          //name, img url, review count, rating, price, location.display address, is closed,phone
          resolve({
            name: response.jsonBody.businesses[0].name,
            img: response.jsonBody.businesses[0].image_url,
            hours: response.jsonBody.businesses[0].is_closed,
            revcount: response.jsonBody.businesses[0].review_count,
            rating: response.jsonBody.businesses[0].rating,
            price: response.jsonBody.businesses[0].price,
            location: response.jsonBody.businesses[0].location.display_address.toString(),
            phone: response.jsonBody.businesses[0].display_phone,
            url: response.jsonBody.businesses[0].url
          });

        }).catch(e => {
          console.log(e);
        });
      }, 100);
    })
  }

Is setTimeout a good idea for delaying the api requests to yelp or is there a different way to delay? Here is the git repo if you want to run the web app: https://github.com/capozzic1/knockout_map

setTimeout for a delay is as good as any other approach if you're happy to keep the load on the server. Other option is to separate the processing from the scheduling e.g. offload those particular requests to a scheduling service and then drip feed those back into your app for processing.

From a code point of view, there are some small things you can do to tidy it up a bit.... but the biggest issue I see with your code at the moment is there is absolutely zero error handling. Express does have a default error handler which conveniently catches an errors thrown inside middleware, however, just be wary that you do run the risk of leaking internal server details to the client if you let you just leave your errors to bubble up.

Here's a bit of a tidy up & better error handling

// handle post requests for getting yelp data
app.post('/post', async (req, res, next) => {
  try {
    // nice use of map makes handleYelp redundant :)
    const searches = req.body.map(x => searchPlace(x));
    const results = await Promise.all(searches);
    return res.send(results);
  } catch (e) {
    console.error(e); // log internal error
    return next(new Error('Internal Server Error')); // return public error to client
  }
});

function searchPlace(city) {
  return new Promise((resolve, reject) => {
    // use async / await here too :)
    setTimeout(async () => {
      try {
        const name = city.name;
        const location = `${city.coords.lat},${city.coords.lng}`;
        const result = await client.search({ name, location });
        const business = result.jsonBody.businesses[0];
        resolve({
          name: business.name,
          img: business.image_url,
          hours: business.is_closed,
          revcount: business.review_count,
          rating: business.rating,
          price: business.price,
          location: business.location.display_address.toString(),
          phone: business.display_phone,
          url: business.url
        });
      } catch (e) {
        return reject(e);
      }
    }, 100);
  })
}


Program to check if heap order is correct or not There is a Java program to check if a Heap (Min) is in the correct order. Heap Order means the parent is less than both left child and right child.

package testers;

import heap.Heap;
import java.util.List;

public class HeapTester
{
    public static void main(String[] args)
    {
        Heap<Integer> heap = new Heap <>();
        heap.add(50);
        heap.add(25);
        heap.add(26);
        heap.add(15);
        heap.add(10);
        heap.add(16);
        heap.add(3);
        heap.add(7);
        heap.add(10);
        heap.add(12);
        heap.add(2);
        System.out.println(heap);
        System.out.println(hasHeapOrder(heap.copy()));
    }

    private static <T extends Comparable<T>> boolean hasHeapOrder(List<T> list)
    {
        if(list.size() < 2) return true;

        boolean hasOrder = true;

        int parent = 0;
        int leftChild = 2 * parent + 1;
        int rightChild = 2 * parent + 2;

        while(leftChild < list.size())
        {
            if(list.get(parent).compareTo(list.get(leftChild)) > 0)
            {
                hasOrder = false;
                break;
            }
            if(rightChild < list.size())
            {
            if(list.get(parent).compareTo(list.get(rightChild)) > 0)
                {
                    hasOrder = false;
                    break;
                }
            }

            parent++;
            leftChild = 2 * parent + 1;
            rightChild = 2 * parent + 2;
        }
        return hasOrder;
    }
}

Avoid flag variables if you don't need them

The hasOrder flag variable is unnecessary.
When you set this variable to false,
you could return false instead.
If the end of the loop is reached, you can return true.

Avoid unnecessary special handling

The special treatment for the case list.size() < 2 is unnecessary.
The rest of the implementation handles that case naturally.

Don't repeat yourself

The computation logic of the left and right child indexes is duplicated twice.
You could eliminate that by changing the loop to while (true), and making this computation the first step of the loop instead of last:

while (true) {
  int leftChild = 2 * parent + 1;
  int rightChild = 2 * parent + 2;

  if (leftChild >= list.size()) {
    break;
  }

  // ...

Test using a unit test framework

Testing by printing stuff is not very useful.
You have to read the output to verify it's correct,
which takes mental effort, and error-prone.
It's better to use a proper unit testing framework,
where test cases will give you simple yes-no answers per test case;
no need to re-interpret passing results.

Test all corner cases

Only one case is "tested".
You need more tests to verify that hasHeapOrder correctly returns true or false depending on the input.
I suppose you have an implementation of Heap such that heap.copy() returns a correctly heap-ordered list, and so hasHeapOrder returns true.
At the minimum,
you should verify that hasHeapOrder returns false for lists like 4, 1, 2 and 4, 5, 2.


Reader Writer SpinLock I'm quite new to C++ and want to focus on writing performant multithreaded code because I will try to port our company internal GUI framework which is currently implemented in C#. So I'd love to get some recommendations about code style and Performance on a reader-writer spin lock that favors writers I wrote:

namespace UI
{
    namespace Threading
    {
        /**
         * \brief ReaderWriterSpinLock which favors writers
         */
        class ReaderWriterSpinLock
        {
        public:
            ReaderWriterSpinLock() = default;
            ~ReaderWriterSpinLock() = default;
            ReaderWriterSpinLock(ReaderWriterSpinLock&) = delete;
            ReaderWriterSpinLock(ReaderWriterSpinLock&&) = delete;
            ReaderWriterSpinLock& operator =(ReaderWriterSpinLock&) = delete;
            ReaderWriterSpinLock& operator =(ReaderWriterSpinLock&&) = delete;

            void AcquireReaderLock()
            {
                int computedValue, initialValue;
                do
                {
                    int count{0};
                    while (m_waitingWriters != 0 || (initialValue = m_lockCount) < 0)
                    {
                        if (++count % MAX_SPIN_COUNT == 0)
                            std::this_thread::yield();
                    }
                    computedValue = initialValue + 1;
                }
                while (!m_lockCount.compare_exchange_strong(initialValue, computedValue));
            }

            void ReleaseReaderLock()
            {
                assert(m_lockCount > 0);
                --m_lockCount;
            }

            void AcquireWriterLock()
            {
                int computedValue, initialValue;
                ++m_waitingWriters;
                do
                {
                    int count{0};
                    while (m_lockCount != 0)
                    {
                        if (++count % MAX_SPIN_COUNT == 0)
                            std::this_thread::yield();
                    }
                    initialValue = 0;
                    computedValue = -1;
                }
                while (!m_lockCount.compare_exchange_strong(initialValue, computedValue));
                --m_waitingWriters;
            }

            void ReleaseWriterLock()
            {
                assert(m_lockCount == -1);
                ++m_lockCount;
            }

        private:
            static constexpr int MAX_SPIN_COUNT = 1000;
            std::atomic<int> m_lockCount{0};
            std::atomic<int> m_waitingWriters{0};
        };
    }
}

Explanation: A negative m_lockCount denotes the active writers (because there can only be one writer at a time the minimum is -1) and a positive count denotes the active readers. When aquiring the reader lock it spins (and yields every MAX_SPIN_COUNT spins) until there are no waiting writers (because it should favor those) and there are no active writers (m_lockCount >= 0), then it tries to update the lock count and repeats the process if it fails. When releasing the lock, the lock count can get decremented without further checks. The methods for the writer lock are quite similar with the difference of temporarily incrementing the m_waitingWriters instead of checking them in the acquire method.

I know that you should normally rely on library functions but in my test it was ten times faster for acquiring the writer lock and twice as fast for readers+writers (with four readers trying to acquire the reader lock in a loop) than the boost::shared_mutex in combination with the boost::shared_lock and boost::unique_lock. So it would actually be worth it.

Also in my application the thread count equals the processor count so the yield should rarely be necessary and the spin lock will only be used for methods which only need a few cycles.

I also wanted to explicitly delete the move and copy constructors because it makes sense for such a class (even though they are deleted implicitly because of the atomic fields). Is this a good idea?

On Threading

lockfree code is rarely easy to understand.

I would beg to differ. It usually just hides the complex situations.
On Spin Locks.
Spin locks are generally not a good idea. You should be really sure that a thread caught in a spin lock will escape quickly (otherwise you are going to melt your processor).
But you mitigate the problem by using std::this_thread::yield() so this is not such a huge problem. So it is not what I would call a classic spin lock. More a spin with yield.
Guaranteed escape
The only issue I see is that you don't guarantee escape of the lock for a particular thread.
Some particular unlucky thread may be caught trying to obtain a lock while other threads zip passed and keep getting the lock forcing the unlucky thread to keep re-trying to get out of the acquire.
So your spin lock has the potential for some resource starvation (or worst case making the code serial).
This is usually achieved by maintaining an order.
Documentation
You need to put the explanation into the code (as comments). Putting it as an explanation for this site was nice and allowed me to understand the code. But without it the code is undecipherable.
RAII
You have designed your code that requires matched calls to method (this is bad practice). Also you have not show RAII lockers for this class so we must assume its use is not exception safe.
All four of your methods:
        void AcquireReaderLock()
        void ReleaseReaderLock()
        void AcquireWriterLock()
        void ReleaseWriterLock()

Should be private members. You should only allow access to these methods via an RAII lock guard. Look at std::lock for an example.
Best practice
Minor best practice violations:
// One declaration per line
int computedValue, initialValue;

// Why not initialize these on declaration.
initialValue = 0;
computedValue = -1;
// There is no need to ever set them again.


Armstrong number generator This is my code to generate all possible Armstrong numbers between the two given numbers. The logic uses string instead of integer to separate the digits to optimize the code.

import java.util.Scanner;

public class ArmstrongNumberGenerator {
    public static void main(String[] args) {
        Scanner scanner = new Scanner(System.in);
        int firstNumber;
        int lastNumber;
        int sum = 0;
        try {
            System.out.println("\nYou will have to enter initial and final number between which all the armstrong numbers you want to generate\n");
            System.out.println("\nEnter the initial number\n");
            firstNumber = scanner.nextInt();
            System.out.println("\nEnter the final number\n");
            lastNumber = scanner.nextInt();
            if (firstNumber == lastNumber) {
                System.out.println("both initian and final numbers are same , no range to generate armstrong numbers");
            } else {
                if (firstNumber > lastNumber) {
                    System.out.println("initial number is greater than final number so i will alter them and make a range from " + lastNumber + " to " + firstNumber);
                    int temp = firstNumber;
                    firstNumber = lastNumber;
                    lastNumber = temp;
                }
                do {
                    String s = Integer.toString(firstNumber);
                    char[] c = s.toCharArray();
                    for (int i = 0; i < s.length(); i++) {
                        sum = ( int ) (sum + Math.pow((c[i] - 48), c.length));
                    }

                    if (sum == firstNumber) {
                        System.out.println("Number " + firstNumber + " is Armstrong");
                    }
                    ++firstNumber;

                    sum = 0;
                } while (firstNumber < lastNumber);

            }
        } catch (Exception e) {
            System.out.println("invalid data");
        }

    }
}

The first number is included in the range but the last number is not. That inconsistency is odd. You should document the limitations you set to the input. If your limitations make documentation hard, it's a sign of bad programming.

Knowing what I wrote above, right now you to check for both equality and greater than between firstNumber and lastNumber. Just check if (firstNumber > lasNumber) instead and tell the user that "firstNumber must be smaller than lastNumber."

FirstNumber and lastNumber are not descriptive variable names. LowerLimit and upperLimit would be better.

You're not prepared for negative input.

Using firstNumber as both the lower limit and loop counter makes the variable name to be incorrect in both uses. It's really never the lowerLimit nor the number being checked. Add a separate variable and use a loop for (int candidate = lowerLimit; candidate <= upperLimit; candidate++) { ...

You should separate the algorithm from main method that reads the input to a static utility method that operates on integers. Reading code that is nested four deep is difficult.


Checking if two dates are within 15 minutes of each other The following code compares two dates and checks if they are within 15 minutes of each other, with a tolerance of a second. All of the test cases below pass with the existing solution. The question is can it be improved by making it more efficient and/or elegant given the following test cases.

TestId, whenCreated, whenToCompareTo, expected Outcome

[(1, "01/01/2017 15:00:00", "01/01/2017 15:15:00", true)]
[(2, "01/01/2017 15:00:00", "01/01/2017 15:00:00", true)]
[(3, "01/01/2017 15:00:00", "01/01/2017 15:16:00", false)]
[(4, "01/01/2017 15:00:00", "01/01/2017 14:59:59", false)]
[(5, "01/01/2017 15:00:00", "01/01/2017 15:14:59", true)]
[(6, "01/01/2017 15:00:00", "01/01/2017 15:16:01", false)]
[(7, "2017-01-01T15:00:00.0000001", "2017-01-01T15:15:00.0000002", true)]
[(8, "2017-01-01T15:15:00.5170000", "2017-01-01T15:15:00.4537587", true)]
[(9, "2017-01-01 15:00:00", "2017-12-31 15:00:00", false)]
[(10, "2018-05-06T13:30:43.2200000", "2018-05-06T13:30:42.9978916", true)]

And here is the code that is doing the comparison:

public bool IsTokenAlive(DateTime timeToCompareTo)
{
    var isDateEarlier = DateComparison(timeToCompareTo, Created);
    var isCompareTimeOfDayEarlier = DateTimeOfDayComparison(timeToCompareTo, Created);
    var isExpired = AbsoluteSecondsComparison(timeToCompareTo, Created);
    var isAlive = isDateEarlier && (isCompareTimeOfDayEarlier || GetTotalSeconds(timeToCompareTo, Created) < 1) && isExpired;

    return isAlive;
}

private static bool DateComparison(DateTime timeToCompareTo, DateTime created)
{
    return DateTime.Compare(timeToCompareTo.Date, created.Date) >= 0;
}

private static bool DateTimeOfDayComparison(DateTime timeToCompareTo, DateTime created)
{
    //We are interested to compare up to the seconds, we are not interested in milliseconds
    return TimeSpan.Compare(StripMilliseconds(timeToCompareTo).TimeOfDay, StripMilliseconds(created).TimeOfDay) >= 0;
}

private static bool AbsoluteSecondsComparison(DateTime timeToCompareTo, DateTime created)
{
    return GetTotalSeconds(timeToCompareTo, created) < MaxTimeInSecondsTokenIsAlive;
}

private static DateTime StripMilliseconds(DateTime dateTime)
{
    return dateTime.AddTicks(-(dateTime.Ticks % TimeSpan.TicksPerSecond));
}

private static double GetTotalSeconds(DateTime timeToCompareTo, DateTime created)
{
    return (timeToCompareTo - created).Duration().TotalSeconds;
}

The Created property is set when the object is created and it will always be the timestamp of the server.

Naming:

Method names should contain a verb and describe exactly what they are doing. For example, look at DateComparison. That sounds more like a type name to me, and I have no idea what the method will do by its name alone.

StripMilliseconds and GetTotalSeconds are much better.

I'd suggest this is as an alternate solution:

public static class TokenValidator
{
    private const int aliveDurationMinutes = 15;

    /// <summary>
    /// Checks if the token is still alive with a 1-second tolerance.
    /// </summary>
    public static bool CheckTokenAlive(DateTime created, DateTime timeToCompare)
    {
        TimeSpan elapsedTime = timeToCompare - created;

        return (elapsedTime.TotalSeconds > -1 && elapsedTime.TotalSeconds < 0) // up to 1 second before
               || (elapsedTime.TotalSeconds >= 0 && Math.Floor(elapsedTime.TotalSeconds) <= aliveDurationMinutes * 60); // up to 15 minutes later
    }
}

I use Math.Floor on the TotalSeconds to add the 1-second tolerance. You can be 0.9999 of a second over and still evaluate as true. You could have done a strictly less than with ((aliveDurationMinutes * 60) + 1) instead.


Reading and writing of pixels of BufferedImages I'm working on a project that takes in a video file and does some image processing on each individual frame. The first step in that processing is to take each source frame and generate three monochromatic images based upon the average red, green, and blue values of the image. If that doesn't make sense, I've drawn up an example image for illustration:

I achieve this step by passing in a BufferedImage (along with a frame number) to a function that loops over the individual pixels and extracts the color data (via getRGB()), and then setRGB()s it into the the three new images as is appropriate.

Unfortunately the performance is killing me, with it taking up to 4 seconds to render the three images per frame (and, by the way, this is with test footage whose resolution is only 856x480! I'll need to be able to use it on footage of 1080p!) For a 30 second clip at 30FPS, this first step takes around an hour! (~4 seconds x 30 seconds x 30 frames = 3600 seconds) I admit, my 1.4GHz dual core really isn't cut out for this kind of stuff, but if I can render 4 minutes of 1080p 60FPS footage in 10 minutes using Virtualdub, then I know I can squeeze more performance out of this thing.

The code I wrote is below. I'm using Rhino to script Java, but I think it should be clear even to those who normally don't use JavaScript.

importPackage(java.io)
importPackage(javax.imageio)
importPackage(java.awt.image)
importPackage(java.awt)

function makeMonochromaticImages(img, n) {
  var w = img.getWidth()
  var h = img.getHeight()

  // These are the three monochromatic images produced.
  var red   = new BufferedImage(w, h, BufferedImage.TYPE_INT_RGB);
  var green = new BufferedImage(w, h, BufferedImage.TYPE_INT_RGB);
  var blue  = new BufferedImage(w, h, BufferedImage.TYPE_INT_RGB);

  // I've cached the constructor to avoid constant overload resoloution
  var _Color = Color['(int,int,int)']

  for(var x = 0; x < w; ++x) {
    for(var y = 0; y < h; ++y) {
      var c = new Color(img.getRGB(x, y))
      var r = c.getRed()
      var g = c.getGreen()
      var b = c.getBlue()

      var red_grayscaled   = (new _Color(r, r, r)).getRGB()
      var green_grayscaled = (new _Color(g, g, g)).getRGB()
      var blue_grayscaled  = (new _Color(b, b, b)).getRGB()

      red.setRGB(x, y, red_grayscaled)
      green.setRGB(x, y, green_grayscaled)
      blue.setRGB(x, y, blue_grayscaled)
    }
  }

  ImageIO.write(red, "png", new File("work\\newframes\\" + (n) + "-r.png"))
  ImageIO.write(blue, "png", new File("work\\newframes\\" + (n) + "-g.png"))
  ImageIO.write(green, "png", new File("work\\newframes\\" + (n) + "-b.png"))
}

I've already considered some other potential options such as manually converting between Integer and RBG, but this seems really messy. I could also make it multi-threaded and render twice as much at once and fully utilize my two cores, but then the code is still technically slow. I guess I'm looking more for a big picture idea (but I'm also not shying away from micro-optimizations if they will have major impact on performance. Remember for a 856x480 image 410880 pixels have to be processed, so I'm actually creating a lot of Color objects.)

The problem comes from iteratively accessing your image object in your current manner.  Take a look below.  This is substantially faster.

import java.awt.image.BufferedImage;
import java.awt.image.DataBufferByte;

public class FastRGB {

private final int width;
private final int height;
private final boolean hasAlphaChannel;
private int pixelLength;
private final byte[] pixels;

public FastRGB(BufferedImage image) {
    pixels = ((DataBufferByte) image.getRaster().getDataBuffer()).getData();
    width = image.getWidth();
    height = image.getHeight();
    hasAlphaChannel = image.getAlphaRaster() != null;
    pixelLength = 3;
    if (hasAlphaChannel) {
        pixelLength = 4;
    }
}

public int getRGB(int x, int y) {
    int pos = (y * pixelLength * width) + (x * pixelLength);
    int argb = -16777216;
    if (hasAlphaChannel) {
        argb = (((int) pixels[pos++] & 0xff) << 24);
    }
    argb += ((int) pixels[pos++] & 0xff);
    argb += (((int) pixels[pos++] & 0xff) << 8);
    argb += (((int) pixels[pos++] & 0xff) << 16);
    return argb;
}
}


Cosine function period I'm using the cosine function in C++ to simulate flashing for a sprite in my game. The method looks like this:

(anything in the sf namespace is part of the SFML graphics library)

void Player::update(const float& deltaTime)
{
    mAccumulatedTime += deltaTime;
    float opacity = abs(cosf(5*mAccumulatedTime)) * 255;
    static int numFlashes = 0;
    if (opacity == 255) {
        cout << ++numFlashes << endl;
    }
    mSprite.setFillColor(sf::Color(255, 255, 255, opacity));
}

So every time opacity is equal to 255 (basically the passing of one full period),  numFlashes should be incremented. The problem is, cos() isn't perfect, meaning it doesn't exactly reach 1 and 0, so the if condition is rarely met. If I use rough checking like if (opacity > 255*0.9999), then numFlashes becomes really high, really fast.

Does anyone know a way to accurately check when a full period has passed? Or is that just not possible?

One may use the following closed formula to compute numFlashes:

numFlashes = 5 * mAccumulatedTime / pi.

This follows from the fact that the period of the function abs(cos(x)) is pi and, if an oscillating function in variable x has a period T, then the number of oscillations, n, is given by the formula:

n = x / T.

Thus, your function definition may be corrected, and even simplified, as follows:

const float PI = acosf(-1);

void Player::update(const float& deltaTime)
{
    mAccumulatedTime += deltaTime;
    float opacity = abs(cosf(5*mAccumulatedTime)) * 255;
    int numFlashes = 5 * mAccumulatedTime / PI;
    cout << numFlashes << endl;
    mSprite.setFillColor(sf::Color(255, 255, 255, opacity));
}


htaccess for URL remapping, caching, and compression I have been collecting snippets from all over the place and then put them all together creating the following .htaccess file.

I really don't understand if what I have done below is good/should work as expected, so your opinion is really appreciated.

I also don't understand the bit just after #Leverage Browser Caching.  Can anyone please explain it to me?

# Date 2 2 2014

ErrorDocument 404 /404

RewriteEngine On
RewriteBase /

    # Redirect if it begins with www.
RewriteCond %{HTTP_HOST} ^www\.(.*)$
RewriteRule ^ http://%1%{REQUEST_URI} [R=301,L]

    # Remove enter code here.php; use THE_REQUEST to prevent infinite loops
    # By puting the L-flag here, the request gets redirected immediately
RewriteCond %{THE_REQUEST} ^GET\ (.*)\.php\ HTTP
RewriteRule (.*)\.php$ $1 [R=301,L]

    # Remove /index
    # By puting the L-flag here, the request gets redirected immediately
    # The trailing slash is removed in a next request, so be efficient and dont put it on there at all
RewriteRule (.*)/index$ $1 [R=301,L]

    # Remove slash if not directory
    # By puting the L-flag here, the request gets redirected immediately
RewriteCond %{REQUEST_FILENAME} !-d
RewriteCond %{REQUEST_URI} /$
RewriteRule (.*)/ $1 [R=301,L]

    # Add .php to access file, but don't redirect
    # On some hosts RewriteCond %{REQUEST_FILENAME}.php -f will be true, even if
    # no such file exists. Be safe and add an extra condition
    # There is no point in escaping a dot in a string
RewriteCond %{REQUEST_FILENAME}.php -f
RewriteCond %{REQUEST_URI} !(/|\.php)$
RewriteRule (.*) $1.php [L]

    # Change the sitemap extension from .php to .xml
RewriteRule ^sitemap.xml(.*)$ /sitemap.php?$1

    # Leverage Browser Caching
    # 480 weeks
<FilesMatch "\.(ico|pdf|flv|jpg|jpeg|png|gif|js|css|swf)$">
    Header set Cache-Control "max-age=290304000, public"
</FilesMatch>

    # 2 DAYS
<FilesMatch "\.(xml|txt)$">
    Header set Cache-Control "max-age=172800, public, must-revalidate"
</FilesMatch>

    # 2 HOURS
<FilesMatch "\.(html|htm)$">
    Header set Cache-Control "max-age=7200, must-revalidate"
</FilesMatch>

<IfModule mod_deflate.c>
    #The following line is enough for .js and .css
AddOutputFilter DEFLATE js css
AddOutputFilterByType DEFLATE text/plain text/xml application/xhtml+xml text/css application/xml application/rss+xml application/atom_xml application/x-javascript application/x-httpd-php application/x-httpd-fastphp text/html

    #The following lines are to avoid bugs with some browsers
BrowserMatch ^Mozilla/4 gzip-only-text/html
BrowserMatch ^Mozilla/4\.0[678] no-gzip
BrowserMatch \bMSIE !no-gzip !gzip-only-text/html

Leverage browser caching is designed to prevent browsers from using a cache for too long. If your website is dynamic, this is necessary to prevent users' browsers from storing your website for too long.

The way your file does this is by checking the type of file, and making a browser's cache of it expire after a specified amount of time. Images the other files you listed generally change little, so that part of you code is good unless you create dynamic images often. Your XML and TXT file deadline is okay unless you have XML news feeds or similar. HTML expiration date is also good as those files are usually the most often edited.

Overall, your file structure is good. I am sorry if this did not answer your question.

Edit:
Note that a browser doesn't have to obey you request to revalidate the page.


Spring MVC dynamically adding form elements I made a simple form with an option of adding one text field dynamically on mouse click. And I will be grateful for suggestions on how I may improve the code.

@Controller
public class MyController {

public ArrayList<NewField> fieldArrayList;
@RequestMapping(value="/create", method= RequestMethod.GET)
public String myForm(Model model, @ModelAttribute NewField newField) {

    fieldArrayList = new ArrayList<>();
    fieldArrayList.add(newField);
    model.addAttribute("dataForm", new DataForm());

    return "create";
}

@RequestMapping(value="/create", method=RequestMethod.POST)
public String formSubmit(Model model, HttpServletRequest hsr, @ModelAttribute DataForm dataForm) {

    if (hsr.getParameterValues("lastField") != null) {

        String[] lastF = null;
        lastF = hsr.getParameterValues("lastField");
        List<String> d = Arrays.asList(lastF);

        int count = d.size() + 1;
        for (int i = 2; i < count+1; i++) {
            NewField newField = new NewField();
            String s1 = hsr.getParameter("p_new_" + String.valueOf(i));
            String s2 = hsr.getParameter("l_" + String.valueOf(i));
            newField.setStr1(s2);
            newField.setStr2(s1);
            fieldArrayList.add(newField);
        }
    }
    model.addAttribute("dataForm", dataForm);
    model.addAttribute("fieldArrayList", fieldArrayList);

    return "view";
}

<!DOCTYPE HTML>

    <title>Getting Started: Handing Form Submission</title>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />

Form

<form action="#" th:action="@{/create}" th:object="${dataForm}" method="post">

Id: <input type="text" th:field="*{id}" />

Name: <input type="text" th:field="*{content}" />

<input type="submit" value="Submit" />

</form>
<input type="text" id="p_new" size="20" name="p_new" value="" placeholder="Input Value" />
<input type="button" id="addNew" value="Add New field"/>

Use interface List instead of concrete class for the fieldArrayList
field.
Make the fieldArrayList field private.
@RequestMapping(value="/create", method= RequestMethod.GET). Requests using GET should only retrieve data, not create.
Extract the calls of hsr.getParameterValues and hsr.getParameter methods in separate methods.
Don't use meaningless variable names such as d, s1, s2.
Format code with your IDE. For example, in IDEA you can use the Ctrl+Alt+L hotkey.
Extract attribute and parameter names in constants.
@RequestMapping(value="/create", method=RequestMethod.POST) can be replaced with @PostMapping("/create").


Validate destinations of a piece in a board game I'm programming a board game called Djambi, where most of its pieces can move like the queen from chess. I created a list to save all valid destinations of a piece, which can be in 8 different directions, so I used 8 for loops, but the code of each loop is almost identical:

This is the loop for possible movements to the right:

count = 0
for j in range(y+1,9):
    if P == 3 and count == 2:
        break

    if re.search(r'[■]',board[x][j]):
        pos = str(x)+str(j)
        destinations.append(pos)
    elif re.search(r'\d', board[x][j]):
        if re.search(r'' + chip[0], board[x][j]) or P == 1 or P == 0:
            break
        else:
            pos = str(x)+str(j)
            destinations.append(pos)
            break
    elif re.search(r'[░]',board[x][j]):
        if P == 0:
            pos = str(x)+str(j)
            destinations.append(pos)
            break
        else:
            break
    else:
        if P == 2:
            pos = str(x)+str(j)
            destinations.append(pos)
        else:
            continue

    count += 1

The loop for movements to the left can be simplified with the one above, but the problem is for the loops for movements up

count = 0
for i in range(x+1,9):
    if P == 3 and count == 2:
        break

    if re.search(r'[■]',board[i][y]):
        pos = str(i)+str(y)
        destinations.append(pos)
    elif re.search(r'[\d]',board[i][y]):
        if re.search(r'' + chip[0], board[i][y]) or P == 1 or P == 0:
            break
        else:
            pos = str(i)+str(y)
            destinations.append(pos)
            break
    elif re.search(r'[░]',board[i][y]):
        if P == 0:
            pos = str(i)+str(y)
            destinations.append(pos)
            break
        else:
            break
    else:
        if P == 2:
            pos = str(i)+str(y)
            destinations.append(pos)
        else:
            continue

    count += 1

and down, because the order of variables that change is different, also for diagonal movements:

count = 0
for i,j in zip( range(x+1,9) , range(y+1,9) ):
    if P == 3 and count == 2:
        break

    if re.search(r'[■]',board[i][j]):
        pos = str(i)+str(j)
        destinations.append(pos)
    elif re.search(r'[\d]',board[i][j]):
        if re.search(r'' + chip[0], board[i][j]) or P == 1 or P == 0:
            break
        else:
            pos = str(i)+str(j)
            destinations.append(pos)
            break
    elif re.search(r'[░]',board[i][j]):
        if P == 0:
            pos = str(i)+str(j)
            destinations.append(pos)
            break
        else:
            break
    else:
        if P == 2:
            pos = str(i)+str(j)
            destinations.append(pos)
        else:
            continue

    count += 1

x and y are the fixed coordinates of the original position of
the chip
i and j are the variable coordinates
board is a matrix of 9x9 dimensions

I want to know if there is a way to implement a kind of function of method or whatever Python can provide to simplify the code above in one chunk, and call it depending of what is needed, in this case, calling it 8 times.

The code works, I just want to know how to simplify it and make it more understandable.

Yes, you can simplify the implementation of your algorithm by introducing the notion of direction, like you find in mathematics…

In Djanbi game, a token can move in 8 directions. You can define a vector d(d_x, d_y) which defines this notion of direction:

North: d = (0, 1),
East: d = (1, 0),
South: d = (0, -1),
West: d = (-1, 0),
North-East: d = (1, 1),
South-East: d = (1, -1),
North-West: d = (-1, 1),
South-West: d = (-1, -1).

First you can define the Point tuple to simplify coding:

import collections

Point = collections.namedtuple('Point', 'x y')

Then, you can use this named tuple to construct all possible directions:

import itertools

directions = [
    Point(*coord) for coord in itertools.product([-1, 0, 1], [-1, 0, 1])
]
directions.remove(Point(0, 0))

You get:

[Point(x=-1, y=-1), Point(x=-1, y=0), Point(x=-1, y=1), Point(x=0, y=-1), Point(x=0, y=1), Point(x=1, y=-1), Point(x=1, y=0), Point(x=1, y=1)]

For each direction, you can search for the possible moves. But before you need to calculate the next position of a token in a given direction. You can use a simple generator for that:

def next_pos(p, d):
    p0 = p
    while 0 <= p.x < 9 and 0 <= p.y < 9:
        if p != p0:
            yield Point(p.x, p.y)
        p = Point(p.x + d.x, p.y + d.y)

Then you can loop as follow:

# coordinates of the position of the chip
chip = Point(2, 4)

for direction in directions:
    print('In the direction: {0}'.format(direction))
    for pos in next_pos(chip, direction):
        ...

Here, pos is a Point instance that you can use to check if a position is valid…


Redux Toolkit Structure with TS I've just taken the leap over to RTK instead of 'vanilla' redux and am blown away by how much less code I need to write. I'm not entirely sure how to structure my slices, however, and figured I'd ask for a small code review so I do not accidentally start breaking conventions from the start. This is how one of my slices is currently looking:
import { createAsyncThunk, createSlice, PayloadAction } from '@reduxjs/toolkit';
import bundle from '../../bundler';
import {BundleStartAction, BundleCompleteAction, BundlesState } from './bundleSliceInterface.ts'

export const createBundle = createAsyncThunk(
  'bundle',
  async (data: { cellId: string; input: string }, thunkAPI) => {
    const { cellId, input } = data;

    thunkAPI.dispatch(bundleStart({ cellId }));
    const result = await bundle(input);
    thunkAPI.dispatch(
      bundleComplete({ cellId, bundle: { code: result.code, err: result.err } })
    );
  }
);

const initialState: BundlesState = {};
const bundleSlice = createSlice({
  name: 'bundle',
  initialState,
  reducers: {
    bundleStart: (state, action: PayloadAction<BundleStartAction>) => {
      state[action.payload.cellId] = { loading: true, code: '', err: '' };
    },
    bundleComplete: (state, action: PayloadAction<BundleCompleteAction>) => {
      state[action.payload.cellId] = {
        loading: false,
        code: action.payload.bundle.code,
        err: action.payload.bundle.err,
      };
    },
  },
  extraReducers: {},
});

export const { bundleStart, bundleComplete } = bundleSlice.actions;
export default bundleSlice.reducer;

Any feedback or tips on how to structure slices properly is greatly appreciated.
Thanks in advance!

I am blown away by how much less code I need to write

I'm going to blow your mind even more because you actually do not need to dispatch any actions inside createAsyncThunk.  It does that for you!
The "pending" action doesn't have a payload so getting the cellId is a little bit tricky.  You can see the types of the dispatched actions in the docs.  The property that you want is action.meta.arg.cellId.
You get better TypeScript support on your reducer by using the builder callback notation.  It already knows the types of your thunk actions!
The part that I am unclear on is what your bundle function does as it seems like it catches errors on its own?  That's fine but you can also throw errors in the inner function of createAsyncThunk and handle them in the reducer with the createBundle.rejected action.  You can also use the rejectWithValue helper to customize the error payload.
export const createBundle = createAsyncThunk(
  "bundle/create",
  async (data: { cellId: string; input: string }) => {
    const { cellId, input } = data;
    const result = await bundle(input);
    // this is the payload
    return {
      cellId,
      bundle: result
    };
  }
);

const initialState: BundlesState = {};
const bundleSlice = createSlice({
  name: "bundle",
  initialState,
  reducers: {},
  extraReducers: (builder) =>
    builder
      .addCase(createBundle.pending, (state, action) => {
        const { cellId } = action.meta.arg;
        state[cellId] = { loading: true, code: "", err: "" };
      })
      .addCase(createBundle.rejected, (state, action) => {
        //??
      })
      .addCase(createBundle.fulfilled, (state, action) => {
        const { bundle, cellId } = action.payload;
        state[cellId] = {
          loading: false,
          code: bundle.code,
          err: bundle.err
        };
      })
});


Project Euler Problem 1 - Functional approach? I'm trying to learn F# right now coming from C# and I'm finding it a great difficulty to "reconfigure" my mind to the functional programming mindset. So I'm going to attempt a few Project Euler problems to learn how to code functionally.

Here's my attempt at problem 1, now I'm wondering prorimarily, is this an idiomatic approach?

// Project Euler - Problem 1
// If we list all the natural numbers below 10 that are multiples of 3 or 5, we get 3, 5, 6 and 9. The sum of these multiples is 23.
// Find the sum of all the multiples of 3 or 5 below 1000.
#light

open System

let multiples = [for n in 1..1000 do if n % 3 = 0 || n % 5 = 0 then yield n]
let sum = List.sum multiples
printfn "The sum of all multiples are: %d" sum
ignore (Console.ReadKey ())

Some changes:

You don't need #light any more.  I would also use |> more (this is more useful in more complex examples where it helps type inference.

open System
[for n in 1..1000 do if n % 3 = 0 || n % 5 = 0 then yield n]
|> List.sum
|> printfn "The sum of all multiples are: %d"
Console.ReadKey () |> ignore


Lowest open space in memory I am writing a simple compiled programming language. Everything but the bytecode interpreter can run as slow as possible, but I would like the interpreter to fun fast, because that's why I made it compiled.

Now that much of the language is working, I decided to profile it to see where I could improve the interpreter. And I find out that a particular method, the method that enables the interpreter to use space freed up by previously deleted variables, is the major bottleneck.

The function, find_space, takes in the size of the contiguous memory block to find. A nonlocal variable called used_mem stores all the indices that are being used by other variables. To find_space, one must find an index where the next size items are not in used_mem.

The original implementation (also the fastest):

def find_space(size: int) -> int:
    for i in range(256):  # all possible memory indices
        for offset in range(size):  # for index past the starting index in size
            if i+offset in used_mem:  # check that new index is not in the used memory
                break  # oh well, its used, lets move on
            else:
                return i  # it works! lets use it
    return None

My second implementation (slowest):

def find_space(size: int) -> int:
    return next(i for i in range(256) if all(i+offset not in used_mem for offset in range(size)))

My third implementation (middle):

def find_space(size: int) -> int:
    for i in range(256):
        if used_mem.isdisjoint(range(i, i+size)):
            return i
    return None

However, all these implementations still couldn't get the average total time used on calculating the 46th fibonacci number 10 times below 0.086 sec.

I've tried 3 times, and 3rd time is supposed to be the charm. Therefore, I'm asking all you wonderful, talented people of codereview.stackexchange.com to help me in optimizing this code.

How can I optimize this code to run as fast as humanely possible in python?

I'll start with the obligatory:

If you're looking to write a fast interpreter, and you've identified a bottleneck in your memory allocator, a smarter algorithm may save you but at the end of the day rewriting in something lower level is probably the way to go if interpreter speed is that crucial.

The problem with your three snippets is they effectively all do the same thing. By this, I don't mean that the return the same (correct) answer. I mean these three snippets effectively perform the same operations. Just some of the operations are disguised in the second two.

Without having profiling, I can take some guesses about your observed performance. The first is likely fastest because everything is just unrolled into two loops. The second is slowest because of the function call all and the contained generator expression. The third is in the middle because of the (worst case) 256 calls to is_disjoint.

Why are these all the same?

Goes through every index i in range(256), and for each offset from 0 to size checks if it's used (exiting early if it is) returning i if none of them are
Creates a generator that yields an index i (trying 0 to 256) if all indexes from i to i+size are not in used_mem, exiting early when one is) and takes the first yielded i
Goes through every index i in range(256), and checks if every number from i to i+size is not in used_mem (exiting early if one is), returning such an i

Note how these are all just rephrasings of the same algorithm. Your performance isn't going to change much because at the end of the day your algorithm is O(n). Actually, it's a little worse than that. It's really O(n*size), because in the worst case you have to do size comparisons for each of your n "slots" in memory.

You could probably get some mileage out of representing your used_mem as a bitfield where bit i indicates whether slot i is taken (this will be more compact and likely faster to lookup than a set), but...

What you really need is a more efficient algorithm. And fortunately, there is a lot of research in the area of memory allocators. Look through the wikipedia page for many examples of different algorithms for different use cases. One is bound to fit your problem.

One way to reduce your allocator's runtime is with the following simple adjustment: keep track of ranges of memory allocated instead of each slot. So, instead of a set marking each slot as used, maintain a linked list of ranges of slots that are allocated:

used_mem: (start=0, end=10) -> (start=20, end=25) -> (start=30, end=50)
                                    current pointer ^

Additionally, keep a pointer to the last place in the list where you allocated memory.

To allocate memory, start from your current pointer, and see if there is enough space between the last thing allocated (above, slot 25) and the next thing allocated (above, slot 30). If we were allocating 3 slots, we could immediately return slot 26 (and insert (start=26, end=28) into the list, moving the current pointer to after it). If instead, we were allocating something larger (say, 10 slots), then we'd see there's no space between 26 and 30, and we'd move the current pointer to the right and try again.

To reclaim memory, just remove the allocation from the linked list.

This approach has the advantage that of not reconsidering things that have already been allocated (the current pointer), so it typically will find an empty hole faster. Additionally, it considers allocations in chunks, so it can skip large allocations in one loop instead of iterating through each slot individually.


Some simple Perl and Regex Reads from a flat file of three columns, delimited by at least one tab character.  Filters the three columns based on input; returns list of corresponding emails.  Arguments passed like key=value or key= for blank.

#!/bin/perl
use warnings;
use strict;

sub get_list {
   my $cols   = {'list'=>0, 'email'=>1, 'sub'=>2};
   my $list   = '[a-z0-9_\- ]+';
   my $email  = '[a-z0-9@\._\-]+';
   my $subls  = '[a-z0-9\. ]+';
   my $return = $cols->{'email'};
   foreach (@ARGV) {
      my ($key, $value) = split '=';
      $value = q{} if not defined $value;
      if    ($key eq 'list')   { $list  = $value; }
      elsif ($key eq 'email')  { $email = $value; }
      elsif ($key eq 'sub')    { $subls = $value; }
      elsif ($key eq 'return') {
         $return = $cols->{$value}
                   if defined $cols->{$value};
      }
      else  { die "invalid key: $key\n"; }
   }
   my $file = 'input.txt';
   open FH, '<', $file
         or die "Could not open '$file' $!\n";
   my %emails = map { (split /\s*\t\s*/)[$return] => 'tacos' }
                grep m/^${list}\s*\t\s*${email}(\s*\t\s*${subls})?\s*$/gi,
                <FH>;
   close FH;
   return (join ', ', keys %emails);
}

print get_list(@ARGV);
print "\n";

# test input... three columns delimited by at least one tab
# third column may be blank.  nothing should contain spaces
# in real life, but i'm allowing it...

# archive   email@email.com success
# archive       email2@email.com    fail
# archive   email2@email.com    success
# archive   email3@email.com
# archive   email@email.com fail
# taco  email@email.com success
# taco  email2@email.com    fail
# taco  email2@email.com    success
# taco meat     email3@email.com
# tacor email@email.com fail

I realize some people may not be down with the $thing = $value if defined $value; arrangement but I went ahead and used it since the logic wasn't complex.  I prefer it over using space for brackets on one line conditionals.  Also, please note, I'm not trying to validate email addresses with regex and I these are truly the only characters the strings will contain.

for the future person... this is what I ended up with:

#!/bin/perl
use warnings;
use strict;

sub get_list {
   my $file = 'input.txt';
   my %emails;

   my $cols  = {'list'=>0, 'email'=>1, 'sub'=>2, 'subls'=>2};
   my $index = $cols->{'email'};

   my $dlmtr = qr{\s*\t\s*};
   my $list  = qr{[a-z0-9_\- ]+};
   my $email = qr{[a-z0-9@\._\-]+};
   my $subls = qr{[a-z0-9\. ]+};

   foreach (@ARGV) {
      my ($key, $value) = split qr{=};
      $value //= qr{};
      if    ($key eq 'list')   { $list  = $value; }
      elsif ($key eq 'email')  { $email = $value; }
      elsif ($key eq 'sub')    { $subls = $value; }
      elsif ($key eq 'return' && defined $cols->{$value}) {
         $index = $cols->{$value};
      }
      else  { die "invalid key: $key\n"; }
   }

   open FH, '<', $file or die "Could not open '$file' $!\n";
   while (<FH>) {
      chomp;

      # i couldn't get the syntax in the accept answer to parse for me
      # so i changed it to this... also, %emails is better as a hash
      # b/c it prevents emails added multiple times... and for my thing,
      # the first two columns are required but the third may be blank.

      if (m/^${list}${dlmtr}${email}(${dlmtr}(${subls}))?\s*$/gi) {
         my $value   = (split $dlmtr)[$index];
            $value //= q{};
         $emails{ $value } = 'tacos';
      }
   }
   close FH;

   return join ' ', keys %emails;
}

my $cols   = {'list'=>0, 'email'=>1, 'sub'=>2};

I'd call this

my %index_of = { 'list' => 0, 'email' => 1, 'sub' => 2 };

and later, change $return to

my $index = $index_of{'email'};

The following is used oddly:

print get_list(@ARGV);
foreach (@ARGV) {

You are calling the function with parameters @ARGV but you never use the parameters in the function.  Instead, you use @ARGV again.  Better would be to use the function parameters:

my @arguments = @_;
foreach my $argument (@arguments) {
   my ($key, $value) = split {=}, $argument;

I also switched to using a named element instead of letting it default to $_.

Alternately, you could pass nothing into the function and use @ARGV in the foreach as you did.  That would work but isn't a best practice.

You may not have realized but the '=' was a regular expression with single quotes as the delimiters.  I prefer to use curly brackets for regular expressions.  If you wanted to be really plain about it, you could have said qr{=}.

# default patterns that can be overridden by command line arguments
my $list   = qr{[a-z0-9_\- ]+};
my $email  = qr{[a-z0-9@\._\-]+};
my $subls  = qr{[a-z0-9\. ]+};
  $value = qr{} if not defined $value;

Using qr{} here makes it more obvious that these are regular expression patterns.

open my $fh, '<', $file

I'd prefer a variable here rather than a globally scoped bareword.

my $SEPARATOR = qr{\s*\t\s*}
my %emails = map { (split $SEPARATOR)[$return] => 'tacos' }
             grep m{^${list}${SEPARATOR}${email}(${SEPARATOR}${subls})?\s*$}gi,
             <$fh>;

By defining $SEPARATOR, we make the regular expressions clearer and reduce the amount of repeated code.

my @entries = ();
while ( my $line = <$fh> ) {
   @columns = m{\A(${list})${SEPARATOR}(${email})(${SEPARATOR}(${subls}))?\s*\z}gi;
   if ( scalar @columns >= 2 ) {
      push @entries, $columns[$index];
   }
}
close $fh;

return (join ', ', @entries);

This is clearer about what it is doing than the map of the grep results.  Also, it doesn't parse each line twice.  And of course it doesn't define a hash just to get the keys.

In general, I would make more use of paragraphing.  This is extra new lines to separate blocks of code.  This allows you to easily see what pieces of code go together.  For example, note how I separate the return from the file handling code.

You have comments explaining what the file format should be, but you don't explain what the command line arguments should be.  A user would have to read the code to figure it out.

Your function does two distinct things.  First, it reads the command line arguments.  Second, it opens a file and processes it based on the command line arguments.  It would be better if you could separate that into two functions.


Sending large data package with TCP - winsock A small struct is sent beforehand to the client that specifies package_size amount of data should be received which is what p->data_value_1 contains.

Server side:

void Server::sendDataSingleUser(char *data_in, int user, unsigned int package_size){
    package_size_sent_ = 0;
    FD_ZERO(&write_set_);
    FD_SET(sockets_[user], &write_set_);

    while(package_size_sent_ < package_size){
        timeout_.tv_sec = 3;
        timeout_.tv_usec = 0;
        i_result_ = select(0, 0, &write_set_, 0, &timeout_);

        // Socket free, send a package
        if(i_result_ > 0){
            i_result_ = send(sockets_[num_clients_], data_in+package_size_sent_, package_size - package_size_sent_, 0);
            // Move buffer pointer
            if(i_result_ > 0){
                package_size_sent_ += i_result_;
            }

            // send error
            if(i_result_ == SOCKET_ERROR){
                // Handle the error
            }
        }
        // Socket still bussy
        else{
            // Disconnect the client
        }
    }
}

Client side:

*size_p = new char[(unsigned int)p->data_value_1];
int temp_count = 0, data_received, package_size = p->data_value_1;

while(temp_count < package_size){
    data_received = recv(connect_socket_, size_p+temp_count, package_size - temp_count, 0);
    // Move buffer pointer
    if(data_received > 0){
        temp_count += data_received;
    }
}

What can be improved/what's incorrect?

In both read and write you should leave the loop after an error (unless you have explicitly tested and fixed the error).

while(totalRecieved < readSize)
{
    dataRecieved = recv(socket, date + totalRecieved, readSize - totalRecieved);
    if (dataRecieved > 0)
    {
        totalRecieved += dataRecieved;
        continue;
    }
    if ((dataRecieved == -1) && (errno == EAGAIN || errno == EWOULDBLOCK))
    {
        // Simple error just try again.
        continue;
    }
    if (dataRecieved == -1 && errno == EINTR)
    {
        // An interrupt was received.
        // This is usually a signal by another thread to give up
        // but you can ignore it if you want.
        continue;
    }
    // Any other error is bad.
    // looping is just going to cause infinite loops.
    break;
}

if (totalRecieved < readSize)
{
    // BAD THINGS HAVE HAPPENED
}


Generic classes with inheritance/equals/comparison operators I would like to greatly reduce verbosity of following code fragment.

What I'd like to do:

Reduce number of occurrences of MaterialRangedParam<Value> and MaterialParam<Value>. C++ has typedef. C# doesn't. Is there keyword for own type of a class?
Remove operator !=, operator ==, GetHashCode and Equals and make compiler generate them for me (the way C++ would do it).

How can I do that?

Additional info:

I'm already aware that clone method offers functionality similar to ICloneable. I have no need to support ICLoneable at the moment.

Usage pattern:
    Those items are going to be stuffed into several lists, arrays and hashtables, compared to each other, assign is going to be called often.

Advice?

public class MaterialParam<Value> where Value: System.IEquatable<Value>{
    public string name = "";
    public string shaderValueName = "";
    public Value value;// = Value();
    public void assignFrom(MaterialParam<Value> other){
        name = other.name;
        shaderValueName = other.shaderValueName;
        value = other.value;
    }

    public MaterialParam<Value> clone(){
        var result = new MaterialParam<Value>();
        result.assignFrom(this);
        return result;
    }

    public override bool Equals(object obj){
        if(!(obj is MaterialParam<Value>))
            return false;

        MaterialParam<Value> other = (MaterialParam<Value>)obj;
        if((System.Object)other == null){
            return false;
        }

        return (this.name == other.name) &&
            (this.shaderValueName == other.shaderValueName) &&
                (this.value.Equals(other.value));
    }

    public override int GetHashCode(){
        return name.GetHashCode() ^ shaderValueName.GetHashCode()
            ^ value.GetHashCode();
    }

    public static bool operator !=(MaterialParam<Value> a, MaterialParam<Value> b){
        return !(a == b);
    }

    public static bool operator ==(MaterialParam<Value> a, MaterialParam<Value> b){
        return a.Equals(b);
    }
};

public class MaterialRangedParam<Value>: MaterialParam<Value> where Value: System.IEquatable<Value>{
    public Value minValue;// = Value();
    public Value maxValue;// = Value();
    public void assignFrom(MaterialRangedParam<Value> other){
        base.assignFrom(other);
        minValue = other.minValue;
        maxValue = other.maxValue;
    }

    new public MaterialRangedParam<Value> clone(){
        var result = new MaterialRangedParam<Value>();
        result.assignFrom(this);
        return result;
    }

    public override bool Equals(object obj){
        if (!base.Equals(obj))
            return false;

        if(!(obj is MaterialRangedParam<Value>))
            return false;

        MaterialRangedParam<Value> other = (MaterialRangedParam<Value>)obj;
        if((System.Object)other == null){
            return false;
        }

        return (minValue.Equals(other.minValue)) && (maxValue.Equals(other.maxValue));
    }

    public override int GetHashCode(){
        return base.GetHashCode() ^ minValue.GetHashCode() ^ maxValue.GetHashCode();
    }

    public static bool operator !=(MaterialRangedParam<Value> a, MaterialRangedParam<Value> b){
        return !(a == b);
    }

    public static bool operator ==(MaterialRangedParam<Value> a, MaterialRangedParam<Value> b){
        return a.Equals(b);
    }
}

This code looks very Java-like in its style and implementation. Maybe C#1.0 at its best. There are a number of stylistic and idiomatic changes I'd make to make these two classes "play well" in the C# / .NET world of 2015. Without going into exhaustive detail, I employed object immutability, parameterized constructors, implementing IClonaeble, utilizing properties, correctly implementing Equals and ==.

With regards to the two things that  you'd like to do, sorry, neither are possible (nor really desirable) with C# for various reasons that you can find on MSDN.

The code:

public class MaterialParam<TValue> where TValue : IEquatable<TValue>, ICloneable
{
    public readonly string name = string.Empty;

    public readonly string shaderValueName = string.Empty;

    public readonly TValue value; // = Value();

    public MaterialParam()
    {
    }

    public MaterialParam(string name, string shaderValueName, TValue value)
    {
        if (name == null)
        {
            throw new ArgumentNullException("name");
        }

        if (shaderValueName == null)
        {
            throw new ArgumentNullException("shaderValueName");
        }

        if (value == null)
        {
            throw new ArgumentNullException("value");
        }

        this.name = name;
        this.shaderValueName = shaderValueName;
        this.value = value;
    }

    public MaterialParam(MaterialParam<TValue> other)
    {
        if (other == null)
        {
            throw new ArgumentNullException("other");
        }

        this.name = other.name;
        this.shaderValueName = other.shaderValueName;
        this.value = other.value;
    }

    public string Name
    {
        get
        {
            return this.name;
        }
    }

    public string ShaderValueName
    {
        get
        {
            return this.shaderValueName;
        }
    }

    public TValue Value
    {
        get
        {
            return this.value;
        }
    }

    public static bool operator ==(MaterialParam<TValue> a, MaterialParam<TValue> b)
    {
        if (ReferenceEquals(a, b))
        {
            return true;
        }

        if (((object)a == null) || ((object)b == null))
        {
            return false;
        }

        return a.Equals(b);
    }

    public static bool operator !=(MaterialParam<TValue> a, MaterialParam<TValue> b)
    {
        return !(a == b);
    }

    public MaterialParam<TValue> Clone()
    {
       return new MaterialParam<TValue>(this);
    }

    public override bool Equals(object obj)
    {
        if (!(obj is MaterialParam<TValue>))
        {
            return false;
        }

        var other = (MaterialParam<TValue>)obj;

        return (this.name == other.Name)
            && (this.shaderValueName == other.ShaderValueName)
            && this.value.Equals(other.Value);
    }

    public override int GetHashCode()
    {
        return this.name.GetHashCode()
            ^ this.shaderValueName.GetHashCode()
            ^ this.value.GetHashCode();
    }
}

public class MaterialRangedParam<TValue> : MaterialParam<TValue> where TValue : IEquatable<TValue>, ICloneable
{
    public readonly TValue minValue; // = Value();

    public readonly TValue maxValue; // = Value();

    public MaterialRangedParam(TValue minValue, TValue maxValue)
    {
        if (minValue == null)
        {
            throw new ArgumentNullException("minValue");
        }

        if (maxValue == null)
        {
            throw new ArgumentNullException("maxValue");
        }

        this.minValue = minValue;
        this.maxValue = maxValue;
    }

    public MaterialRangedParam(MaterialRangedParam<TValue> other) : base(other)
    {
        if (other == null)
        {
            throw new ArgumentNullException("other");
        }

        this.minValue = other.MinValue;
        this.maxValue = other.MaxValue;
    }

    public TValue MinValue
    {
        get
        {
            return this.minValue;
        }
    }

    public TValue MaxValue
    {
        get
        {
            return this.maxValue;
        }
    }

    public static bool operator ==(MaterialRangedParam<TValue> a, MaterialRangedParam<TValue> b)
    {
        if (ReferenceEquals(a, b))
        {
            return true;
        }

        if (((object)a == null) || ((object)b == null))
        {
            return false;
        }

        return a.Equals(b);
    }

    public static bool operator !=(MaterialRangedParam<TValue> a, MaterialRangedParam<TValue> b)
    {
        return !(a == b);
    }

    public new MaterialRangedParam<TValue> Clone()
    {
        return new MaterialRangedParam<TValue>(this);
    }

    public override bool Equals(object obj)
    {
        if (!(obj is MaterialRangedParam<TValue>))
        {
            return false;
        }

        var other = (MaterialRangedParam<TValue>)obj;

        return base.Equals(other) && this.minValue.Equals(other.MinValue) && this.maxValue.Equals(other.MaxValue);
    }

    public override int GetHashCode()
    {
        return base.GetHashCode() ^ this.minValue.GetHashCode() ^ this.maxValue.GetHashCode();
    }
}