Rust version of basic functional prime stream - can this be made less clunky? I'm looking at implementing the basic functional not-quite-Eratosthenes prime stream in Rust.  I like to try it when I start learning a language.

Here's the bog standard Haskell version:

primes :: [Integer]
primes = sieve (2 : [3, 5..])
  where
    sieve (p:xs) = p : sieve [x|x <- xs, x `mod` p /= 0]

in Rust, I think a reasonable transliteration is something like:

struct Primes {
    iter: Option<Box<Iterator<Item=u32>>>
}

impl Iterator for Primes {
    type Item = u32;

    fn next(&mut self) -> Option<<Self as Iterator>::Item> {
        let mut iter = self.iter.take().unwrap();
        let res = iter.next().unwrap();
        self.iter = Some(Box::new(iter.filter( move |x| x % res != 0)));
        Some(res)
    }
}

fn main() {
    let primes = Primes { iter: Some(Box::new(2..)) };
    for p in primes.take(20) {
        println!("{}", p);
    }
}

Leaving aside the algorithm (of course there are nicer ways to do primes!),
this feels a bit verbose - is there a simpler way to construct an iterator with accumulating filters?  The Option of Box feels quite clunky, but if I understand right, that's the best way to replace a field on self.

On a first note, your code will panic once your iterator reaches std::u32::MAX, which admittedly would take a long time. You can fix this by returning None when this happens, which can be done easily with ?.

let mut iter = self.iter.take()?;
let res = iter.next()?;

I can't think of a better way to accumulate actual filter calls, but if I were to implement this sort of algorithm, I would probably do something like this instead:

struct Primes {
    found: Vec<u32>,
    iter: std::ops::RangeFrom<u32>,
}

impl Primes {
    fn new() -> Self {
        Primes {
            found: Vec::new(),
            iter: (2..),
        }
    }
}

impl Iterator for Primes {
    type Item = u32;

    fn next(&mut self) -> Option<<Self as Iterator>::Item> {
        let Primes { found, iter } = self;
        let res = iter.find(|x| found.iter().all(|p| x % p != 0))?;
        found.push(res);
        Some(res)
    }
}

Note that in your code, all the previous primes are stored in the closures, so doing it explicitly this way gives a better view of the costs of this method. Additionally, I can imagine the continuously nested dynamic dispatch adding up in costs, so this method removes any need for that.


Calculate the amount of savings I will have in n months I have the following problem and I am new in Java:
I have an initial capital of 10 000. I invest this initial capital of 10 000 in the bank for a interest rate per month of 0.04/12. My net income is 3000 per month and my expenses per month is 2000. I invest also my monthysavings = net income - expenses = 3000 - 2000 = 1000 in the bank for the interest rate of 0.04/12.
I would like to calculate the amount of savings I will have in 300 months.
I did the following code:

public class RetCalcJava {

public double futureCapital(
      double interestRate
    , int nbOfMonths
    , int netIncome
    , int currentExpenses
    , double initialCapital) {

double  monthlySavings = netIncome - currentExpenses;
double iniVal=0;
double finalVal =0;
double intermediate = 0;

for (int i=0; i< nbOfMonths; i++){
    System.out.println("Boucle" + i);

    if (i ==0) {
        iniVal = initialCapital * (1 + interestRate) + monthlySavings;
        intermediate = iniVal;

        System.out.println("intival" + iniVal);
    }
    else {
        finalVal =  intermediate * (1 + interestRate) + monthlySavings;
        intermediate = finalVal;
        System.out.println("finalVal" + finalVal);
    } }    return finalVal;

}
}

The main class is :
public class RetCalcMain {

public static void main(String[] args) {
    RetCalcJava retcal = new RetCalcJava();
    retcal.futureCapital(0.04/12, 300, 3000,2000,10000);

    }
}

This code works. What do you think about my code? Is it optimized? What should I change?

Welcome to Code Review. Few suggestions:

Formatting: the code should be formatted to make it easier to read. Many IDEs support auto-formatting, or you can use an online formatter.
Naming: the class name RetCalcJava shouldn't include Java as suffix, the extension .java already tells what type of file is. It's also hard to understand what a class called RetCalc does, consider a more meaningful name. I am not sure what Boucle means.
Typo and spacing: iniVal, println("intival" + iniVal). There are some typos and the result of the print is "intival1000", it needs a space.
Static method: The method futureCapital does not need to use instance variables, so it can be static. By making it static we don't need to instantiate the class.
Formatting the output: the capital can be formatted as 541,267.20 instead of 541267.1989622512 using System.out.printf and %,.2f.
Accumulation:
iniVal = initialCapital * (1 + interestRate) + monthlySavings;
intermediate = iniVal;
System.out.println("intival" + iniVal);
for (int i = 1; i < nbOfMonths; i++) {
    //...
    finalVal = intermediate * (1 + interestRate) + monthlySavings;
    intermediate = finalVal;
    // ...
}
return finalVal;

The temporary variables intermediate and initVal are not necessary, see the code below.

Revised code
public class RetCalc {
    public static double futureCapital(double interestRate, int months, int netIncome, int currentExpenses,
            double initialCapital) {
        double monthlySavings = netIncome - currentExpenses;
        double capital = initialCapital;
        for (int i = 0; i < months; i++) {
            capital = capital * (1 + interestRate) + monthlySavings;
            if (i == 0) {
                System.out.printf("Initial value: %,.2f%n", capital);
            } else {
                System.out.printf("Month: %d Capital: %,.2f%n", i + 1, capital);
            }
        }
        return capital;
    }

    public static void main(String[] args) {
        double futureCapital = RetCalc.futureCapital(0.04 / 12, 300, 3000, 2000, 10000);
        System.out.printf("Future capital: %,.2f%n", futureCapital);
    }
}

Output:
Initial value: 11,033.33
Month: 2 Capital: 12,070.11
...
Month: 300 Capital: 541,267.20
Future capital: 541,267.20


Python script for parsing links I wrote a test that parses links from a web page, and in a loop, clicks on each of them, and then returns to the main page. But, each iteration is accompanied by parsing the entire web page again and again and overwriting the links array, although only one link is needed per iteration. I understand that this is inefficient. How can I optimize this?

I tried parsing the links only once and then iterating through them in a loop. After the first iteration, it goes back (to the main page) and tries to click on the second link, but it's not interactive (I think it's because of the web elements that are stored in the links array changing every time you go to the page).

def setUp(self):
    self.driver = webdriver.Chrome()
    self.driver.get('https://yandex.ru')

def test_01(self):
    driver = self.driver
    links = []
    time.sleep(3)
    links = driver.find_elements_by_css_selector("a")
    for i in range(len(links)):
        links = driver.find_elements_by_css_selector("a")
        links[i].click()
        driver.get('https://yandex.ru')
        time.sleep(3)

When you navigate away from the page the WebElements get invalidated as DOM changes therefore I would recommend going for List Comprehension and convert the list of WebElements into the list of links text.
Once done you should be able to use normalize-text() function just in case the text belongs to child element or has starting/trailing spaces
And last but not the least consider using Explicit Wait just in case the link is populated via AJAX call.

Example code:

driver.get("http://yandex.ru")
links = [link.text for link in driver.find_elements_by_css_selector("a")]
for link in links:
    WebDriverWait(driver, 10) \
        .until(expected_conditions
        .presence_of_element_located(
        (By.XPATH, "//a[normalize-space()='" + link + "']"))).click()
    # do what you need here
    print(driver.title)
    driver.back()
driver.quit()


Pandoc batch conversion I wrote a simple bash script in order to batch convert Markdown files via pandoc and it is working for me. But I have the feeling the code is dirty. Could you help me improve it?

#!/bin/bash

rm -r out; mkdir out

for f in src/*.md
do
    filename=$(basename "$f")
    filename="${filename%.*}"
    echo "Processing $filename"
    pandoc "$f" -o "out/$filename.epub"
    pandoc "$f" -o "out/$filename.pdf"
done

overall and good stuff

This is pretty good.  I definitely wouldn't call the code dirty.  Good stuff:

The indentation is good
putting double quotes around variable substitutions is a best practice
using $() for command substitution is the best practice also
the #! is good
providing meaningful output to the user is nice
the variable names are ok

suggestions

I get putting rm -r out; mkdir out on one line sounds good because they're related, but it doesn't help the readability here.  If you want to tie them together so that the mkdir doesn't run unless rm succeeds then you could do rm -r out && mkdir out.  Otherwise I'd put them on two lines.  Breaking up things with blank lines, as you've already done, is enough to make clear which things belong together.
for f in src/*.md certainly works most of the time, but will break if there's any white space in a filename.  Fixing this involves using find.  This answer is part of a duped question, but it may be a bit easier to follow.
Having f and filename as variables is a bit confusing.  For something this short it is pretty harmless which is why I said they were ok above.  If you want to tweak this f might make more sense as fullpath or fqfn.  And filename might be better as basename or base.
add if ! which pandoc... near the top to catch if pandoc is missing.

if ! which pandoc > /dev/null; then
        echo you need pandoc
        exit 1
fi

further reading

shellcheck is handy.  You can run it locally or in your CI pipeline or manually paste something into their site.
bash best practices by Yaroslav Tkachenko is a nice quick read on bash best practices.
google shell style guide is even more in-depth with best practices.
env is good for portability but this is rarely an issue with bash.  It might become a more current problem if Apple dumps bash after adopting zsh as the default.


Noughts and crosses command line game in Python 3.5 Based on this Code Review answer, I have created a noughts and crosses game in Python, which asks for an index (1 to 9) and updates the grid, and displays it on the screen. I have done some very untidy programming in places, and would please like some help on how to make it more readable, and avoid repeating statements, such as print_grid(). Also I would like to make the 'Change player' part more simple. Any other constructive criticism or improvements also gratefully recieved.

winning_lines = [(0, 1, 2), (3, 4, 5), (6, 7, 8), (0, 3, 6),
             (1, 4, 7), (2, 5, 8), (0, 4, 8), (2, 4, 6)]

def get_turn(player):
    return int(input("Player %d: " % player).strip()) - 1

def format_player(num):
    return ' OX'[num]

def print_grid(board):
    """Formats and prints the grid."""

    print('\n'
          '{}│{}│{}\n'
          '─┼─┼─\n'
          '{}│{}│{}\n'
          '─┼─┼─\n'
          '{}│{}│{}\n'
          '\n'
          .format(*board))

def check_for_win(board, player):
    """Returns True for win, anything else, False"""
    for winning_line in winning_lines:  # Take all possible winning lines one by one
        for pos in winning_line: # Test each index of the winning line
            if board[pos] != format_player(player):
                break
        else: return True
    return False

def play_game():
    player = 1
    board = [" "] * 9

    while True:
        print_grid(board)
        place = get_turn(player)
        if place in range(9) and board[place] == " ":
            board[place] = format_player(player)

            if check_for_win(board, player):
                print_grid(board)
                return player

            # Change player

            if player == 1:
                player = 2
            else:
                player = 1

winner = play_game()

print("Player %d wins!" % winner)

The functions get_turn, format_player and play_game have no docstrings. What do they do? What do they return?
In check_for_win, the value of format_player(player) does not change and so you could remember it in a local variable to avoid recomputing it on each time around the loop.
check_for_win could be simplified using the built-in function all, like this:

def check_for_win(board, player):
    """Returns True if player has won, otherwise False"""
    player = format_player(player)
    for winning_line in winning_lines:
        if all(board[pos] == player for pos in winning_line):
            return True
    return False

and then that could be futher simplified using the built-in function any, like this:

def check_for_win(board, player):
    """Returns True if player has won, otherwise False"""
    player = format_player(player)
    return any(all(board[pos] == player for pos in winning_line)
               for winning_line in winning_lines)

If the player's input is incorrect, there is no message explaining what the problem was.
Repeatedly switching between player 1 and player 2 can be done using itertools.cycle, like this:

from itertools import cycle
for player in cycle((1, 2)):
    # etc.

I think that printing the winner should be the responsibility of the play_game function, not of the top-level code.
There is no check to see if the game is tied. If the board is full but there is no winner then the game continues to prompt player 2 for a move.


snapping code: from separate checks to 1 formula I'm working on a 3D-project with three.js and webGl. I've written a function to check for 2 objects to be near each other and thus "snap" to each other.

The objects don't move on the Y-axis, so I only have to check the 4 sides of each object against the side of all other objects in the scene. At the moment I'm doing that with this code:

for (var i = 0; i < elements.length; i++) {

            var newBox = updateBoundingBox(elements[i]);

            var snapped = null;

            if (newBox.min.x - box.max.x < 5 && newBox.min.x - box.max.x > -box.size().x / 2//5
                && box.max.z > newBox.min.z && box.min.z < newBox.max.z) {

                activeElement.position.x = newBox.min.x - ((box.max.x - box.min.x) / 2) - 0.5;
                box = updateBoundingBox(activeElement);
                snapped = [elements[i], "X"];
            }

            if (newBox.min.z - box.max.z < 5 && newBox.min.z - box.max.z > -box.size().z / 2//5
                && box.max.x > newBox.min.x && box.min.x < newBox.max.x) {

                activeElement.position.z = newBox.min.z - ((box.max.z - box.min.z) / 2) - 0.5;
                box = updateBoundingBox(activeElement);
                snapped = [elements[i], "Z"];

            }

            if (box.min.x - newBox.max.x < 5 && box.min.x - newBox.max.x > -box.size().x / 2
                && box.max.z > newBox.min.z && box.min.z < newBox.max.z) {

                activeElement.position.x = newBox.max.x + ((box.max.x - box.min.x) / 2) + 0.6;
                box = updateBoundingBox(activeElement);
                snapped = [elements[i], "X"];

            }

            if (box.min.z - newBox.max.z < 5 && box.min.z - newBox.max.z > -box.size().z / 2
                && box.max.x > newBox.min.x && box.min.x < newBox.max.x) {

                activeElement.position.z = newBox.max.z + ((box.max.z - box.min.z) / 2) + 0.5;
                box = updateBoundingBox(activeElement);
                snapped = [elements[i], "Z"];

            }

            if (snapped != null) {

                activeElement.snapped.push(snapped);
                debug.snapped = true;
            }

}

But, it is very obvious that the 4 if-statements are almost identical, so I was wondering if it was possible to write them in a better/shorter way so I can just loop over them?

1) Do not hard-code the distance that will define 'near', but rather make it a variable/parameter.
2) Split the solution : 1) do we 'approximatively' overlap ? and 2) are we on the left or right, and are we up or down ?
3) Use as much 'standard' code as you can : here 'update' will define a BBox based on a list of points, 'expand' and 'overlaps' will do what-you-guess.

var nearDistance = 5;
var newBox new BoundingBox();
var boxSize = null;

for (var i = 0; i < elements.length; i++) {
            var thisElement = elements[i];

            // newBox is now the BBox of thisElement
            newBox.update(thisElement);

            // do we approximatively overlap ?
            newBox.expand(nearDistance);
            var overlaps = newBox.overlaps(box);
            if (!overlaps) continue;

            // are we on the left/right, up/down ??

            newBox.expand(-nearDistance);
            boxSize = box.size() ;

            var newXPosition = -1, newZPosition = -1;

            if ( newBox.min.x - box.max.x > -boxSize.x / 2) {
                // left
                newXPosition = newBox.min.x - ((box.max.x - box.min.x) / 2) - 0.5;
            } else {
                // right
               newXPosition = newBox.max.x + ((box.max.x - box.min.x) / 2) + 0.6;
            }

            if (newBox.min.z - box.max.z > -boxSize.z / 2 ) {
                // up
                newZPosition = newBox.min.z - ((box.max.z - box.min.z) / 2) - 0.5;
            } else {
                // down
                newZPosition= newBox.max.x + ((box.max.x - box.min.x) / 2) + 0.6;
            }

            //adjust position
            activeElement.position.x = newXPosition ;
            activeElement.position.z = newZPosition;

            box.update(activeElement);

            activeElement.snapped.push( /* thisElement was snapped */);
            debug.snapped = true;
            }
}


Create two vehicles, move them on a grid based on user input I have a grid and a class Vehicle, which will have starting point(X, Y on the grid) and direction(one of N,E,S,W) taken from user and there will be commands, L & R turns the vehicle 90 degrees around left and right respectively and M moves the vehicle one unit to faced direction.

  For instance, if the command is M and the current direction is E, the vehicle moves from (x,y) to (x+1,y) and preserve its direction.
  If the command is R, and the current direction is E, vehicle preserves its position but direction changes to S.

  My input consists of 5 lines:

  The first line defines the limits of the grid; X and Y separated by space
  The second line defines the current position and facing direction for the
  first vehicle; X, Y and Dir are separated by space
  The third line defines the commands for the first vehicle, which is a line of string
  The fourth and fifth lines are the same as second and third except they
  are for the second vehicle

  Note: Vehicles are sent sequentially. If the second vehicle attempts to move to the occupied spot of the first vehicle, the command will be skipped. If any move command makes any of the vehicles move out of the grid, that command will be skipped as well. Inputs are always in expected style, so there is no need to validate inputs.

As an example:

Inputs:

6 6
1 3 E
RMLLMRMRM
1 1 N
LMLML

Output:

2 2 S
0 0 E

My code:

directions = ['N','E','S','W']
movement = {'N': (0,1), 'E': (1,0), 'S': (0,-1), 'W':(-1,0)}
commands = {'L': 'turn_left', 'R': 'turn_right', 'M': 'move'}

GRID_MAX_X, GRID_MAX_Y = map(int, raw_input().split())

first_vehicle_x = None
first_vehicle_y = None

class Vehicle():
    def __init__(self, x, y, face):
        self.x = x
        self.y = y
        self.dir = face

    def turn_left(self):
        self.dir = directions[(directions.index(self.dir)-1)%len(directions)]

    def turn_right(self):
        self.dir = directions[(directions.index(self.dir)+1)%len(directions)]

    def move(self):
        new_x = self.x + movement[self.dir][0]
        new_y = self.y + movement[self.dir][1]

        if new_x != first_vehicle_x or new_y != first_vehicle_y:
            if new_x in xrange(GRID_MAX_X+1):
                self.x = new_x
            if new_y in xrange(GRID_MAX_Y+1):
                self.y = new_y

vehicle_one_pos = raw_input().split()
vehicle_one_commands = raw_input()

vehicle_one = Vehicle(int(vehicle_one_pos[0]), int(vehicle_one_pos[1]), vehicle_one_pos[2])
for command in vehicle_one_commands:
    eval("vehicle_one.{0}()".format(commands[command]))

first_vehicle_x = vehicle_one.x
first_vehicle_y = vehicle_one.y

vehicle_two_pos = raw_input().split()
vehicle_two_commands = raw_input()

vehicle_two = Vehicle(int(vehicle_two_pos[0]), int(vehicle_two_pos[1]), vehicle_two_pos[2])
for command in vehicle_two_commands:
    eval("vehicle_two.{0}()".format(commands[command]))

print vehicle_one.x, vehicle_one.y, vehicle_one.dir
print vehicle_two.x, vehicle_two.y, vehicle_two.dir

About my code, it feels like repetition on doing same things for each vehicle but wasnt't sure how to handle without it since I need position of vehicle_one for vehicle_two.

Any comment is always appreciated, thanks in advance.

Use functions
Having code at the top-level of a module is bad practice as it impairs both reusability and testing: as soon as the module is imported, the code gets executed which is not necessarily what you want.
Instead you should use functions and wrap the remaining top-level code in an if __name__ == '__main__': statement.
Having functions would also mean that you need to stop relying on global variables such as GRID_MAX_X, GRID_MAX_Y, first_vehicle_x or first_vehicle_y. Which is a good thing as currently they make your code tightly coupled. Instead, you should pass those values of interest as parameters to objects or functions.
Bug
Requirements state that:

Vehicles are sent sequentially. If the second vehicle attempts to move to the occupied spot of the first vehicle, the command will be skipped. If any move command makes any of the vehicles move out of the grid, that command will be skipped as well.

You effectively skip the command if the second vehicule tries to move to the position of the first one, but you don't skip it if the move would put the vehicule out of the boundaries of the map. Instead you make the vehicule glide along that boundary.
Also note that you can easily compare the position of the two vehicules using tuple equality (new_x, new_y) != vehicule_one_position and check that a number is within bounds using extended comparisons: 0 <= new_x <= grid_max_x.
Indexables
Strings are iterables and indexables, no need to split them in a list or a tuple. I would however encapsulate changing directions in its own class to better separate concerns and avoid a small amount of repetitions.
eval
There is one saying that eval is evil. It has its uses, but never in such simple cases. Instead, you should know that you can reference functions and methods just by their name so you can build the commands dictionnary in the Vehicule class using
{
    'L': self.turn_left,
    'R': self.turn_right,
    'M': self.move,
}

And be able to do commands[command]() directly, without the need for eval.
Use @property
For simpler data extraction, @property methods can help format them and appear as a simple attribute access. This help make a cleaner design.
Proposed improvements
class Directions:
    """Circular buffer of possible directions"""
    DIRECTIONS = 'NESW'

    def __init__(self, start):
        self.index = self.DIRECTIONS.index(start)

    def next(self):
        self.index = (self.index + 1) % len(self.DIRECTIONS)

    def previous(self):
        self.index = (self.index - 1) % len(self.DIRECTIONS)

    @property
    def current(self):
        return self.DIRECTIONS[self.index]

class Vehicle():
    MOVEMENT = {'N': (0, 1), 'E': (1, 0), 'S': (0, -1), 'W':(-1, 0)}

    def __init__(self, x, y, facing, grid, obstacle):
        self.x = x
        self.y = y
        self.facing = Directions(facing)
        self.grid_width, self.grid_height = grid
        self.obstacle = obstacle

    @property
    def direction(self):
        return self.facing.current

    @property
    def position(self):
        return (self.x, self.y)

    def parse_commands(self, commands):
        action = {
            'L': self.facing.previous,
            'R': self.facing.next,
            'M': self.move,
        }
        for command in commands:
            action[command]()

    def move(self):
        offset_x, offset_y = self.MOVEMENT[self.facing.current]
        x = self.x + offset_x
        y = self.y + offset_y

        if (x, y) != self.obstacle and 0 <= x <= self.grid_width and 0 <= y <= self.grid_height:
            self.x = x
            self.y = y

def setup_and_move_vehicule(grid, obstacle):
    x, y, facing = raw_input().split()
    vehicule = Vehicule(int(x), int(y), facing, grid, obstacle)
    vehicule.parse_commands(raw_input().strip())
    return vehicule.position, vehicule.direction

def main():
    grid = map(int, raw_input().split())
    v1_pos, v1_dir = setup_and_move_vehicule(grid, None)
    v2_pos, v2_dir = setup_and_move_vehicule(grid, v1_pos)
    print v1_pos[0], v1_pos[1], v1_dir
    print v2_pos[0], v2_pos[1], v2_dir

if __name__ == '__main__':
    main()


Beginnings of a Poker hand classifier For practicing/learning object orientation, I made a Card class and am practicing doing stuff with it such as shuffling decks and dealing hands etc...

I have a method that receives a list item which essentially is a hand of 5 cards then tells the user how about the hand such as pairs and three of a kind however it feels incredibly sloppy and would like to know if it's improvable.

private static string CheckHand(List<Card> hand)
{
    string[] tempnames = { hand[0].Face, hand[1].Face, hand[2].Face, hand[3].Face, hand[4].Face };
    int sameCardCount = 0;

    for (int i = 0; i < hand.Count; i++)
    {
        for (int k = 0; k < hand.Count; k++)
        {
            if (k == i) { }
            else if (tempnames[i] == hand[k].Face)
            {
               sameCardCount++;
            }
        }
    }
    switch (sameCardCount)
    {
        case 12:
        return "you have four of a kind!";
        case 6:
        return "you have three of a kind!";
        case 4:
        return "you have two pairs!";
        case 2:
        return "you have a pair!";
        default:
        return "nothing special here..";
    }
}

The card object and enum suit is like so:

class Card
{
    public string Face { get; set; }
    public Suit Suitc { get; set; }
    public Card(Suit suit,string face)
    {
        Face = face;
        Suitc = suit;
    }

    public override string ToString()
    {
        string card = "(" + Face + " " + Suitc + ")";
        return card;
    }
}
enum Suit
{
    CLUB, DIAMOND, HEART, SPADE
}

As far as I can test, it works as intended. But with the double loop, the return values are a bit messy in my opinion. How would you have done it?

Note: you do not have to provide extracts of code if you don't wish to. I'm just trying to help myself improve my problem solving.

Much of your code can be made shorter and more clear with LINQ:

string[] tempnames = { hand[0].Face, hand[1].Face, hand[2].Face, hand[3].Face, hand[4].Face };

vs

var faces = hand.Select(c => c.Face).ToArray();

But we actually don't need this at all, because your main algorithm can be made more clear with LINQ as well. What do you want?  To divide the cards up into groups based on their face value.  So do that!

var cardCounts = (from card in hand         // We're processing each card
                 group card by card.Face    // make a sequence of groups of cards with the same face
                 into g                     // process each group
                 let count = g.Count()      // get the size of each group
                 orderby descending count   // sort the groups by size, big to small
                 select count).Take(2).ToList();  // get the top two sizes

Before you read on: Why did I only take the highest two counts?

And now you can just read off the results:

switch(cardCounts[0])
{
  case 1: return "nothing";
  case 2: return cardCounts[1] == 1 ? "one pair" : "two pair";
  case 3: return cardCounts[1] == 1 ? "three of a kind" : "full house";
  case 4: return "four of a kind";
  default: Debug.Fail("How'd we get here?"); return "";
}

LINQ make the code read more like its meaning rather than a bunch of loops and counters that seem completely disconnected from the domain at hand. What are you doing logically?  Grouping the cards by face value and counting how many cards are in each group.  So make the code actually do that.

Remember, your goal here is to correctly implement the rules. The rules do not say "consider all ordered pairs of cards; assign one point for each identical card in that set of pairs. If the point total is 12 then..." and so on, but that's what you wrote.  Which is more likely to be correct, code that resembles the logic of the rules, or code that does some completely other thing that you hope is equivalent to the rules? Which do you suppose will be easier to maintain?

Exercise: Now add flushes, straights, and straight flushes. Can you use LINQ?

Exercise: Come up with a better representation for the value of a hand other than a string describing it

Exercise: Make a class for hands. What operations do you need to perform on hands?

Exercise: Cards don't change their kind, but you've modeled them as having setters. Make the card type an immutable struct, not a mutable class.

Exercise: Implement IComparable<Card> on cards.

Exercise: Given two hands, which, if either, is the winning hand?  Implement IComparable<Hand> on Hand. Note that you'll need more than just the kind of hand it is! Suppose there are two hands, both are two pair, and both are pair of kings, pair of queens. One of them still could win.  Read the rules of poker carefully; that's your specification. Again, try to make the code read like the rules.

Exercise: Given a sequence of cards, generate all possible five-card hands.  The signature of your method should be static IEnumerable<Hand> AllHands(IEnumerable<Card> cards).  If there are fewer than 5 cards in the sequence then clearly the resulting sequence should be empty.  If there are exactly 5 then you should get exactly one hand.  If there are 6 cards then you should get 5 hands. If there are 7 cards then how many hands should you get?

Exercise: given a sequence of 7 cards, find the highest 5-card hand in it.

Exercise: given n two-card hands and five community cards, describe how to divide the pot. Note that you have to represent ties somehow.  Again, remember, you are implementing the rules of poker, and those include ties.

Exercise: What if there are multiple pots, due to an all-in situation? Now how to divide the pots?

Exercise: Implement the rest of the rules of poker: ante, betting, raising, etc.

Exercise: Write an AI that plays poker, and a user interface that lets humans play against the AI.


Phrase matching between two Strings in Ruby I've written a phrase-matching method to return the longest matching phrase from two Strings in Ruby. My method looks like this and works as expected:

class String
  def phrase_in_common(other)
    n = [size, other.size].min
    while n > 0 do
      other_words = other.downcase.split.each_cons(n)
      string_words = split.each_cons(n)
      matching_words = string_words.find { |cons_words| other_words.include?(cons_words.map(&:downcase)) }
      return matching_words&.join(' ') if matching_words
      n -= 1
    end
  end
end

>> string = "Hello world, please come to my house next week"
>> other = "Hello friends, please come to my party next week"
>> string.phrase_in_common(other)
=> "please come to my"

My question is, can (and should) this be accomplished in a more Ruby way, perhaps by replacing the while with an Enumerable method of some kind?

To clarify, the phrase-matching should be based on words, so for example:

>> "this phrase".phrase_in_common("his phrase")
=> "phrase"

Also, note that the method is case-insensitive in matching and uses the case of the subject String for the return value:

>> "Greatest Show On The Earth".phrase_in_common("Silliest Show on the Earth")
=> "Show On The Earth"

It seems that you are trying to get the longest common subsequence.
There are several implementation of this algorithm and you can find an example on Rosetta Code.

A working recursive implementation implementation could be:

# Split your sentence into words and manipulate the content. E.g.
# - downcase all chars
# - split by some chars such as space, punctuation and so on

def split_into_words(string)
  string.downcase.split(/[\s\.,;\'\"]/)
end

def longest_subsequence(xstr, ystr)
  return '' if xstr.empty? || ystr.empty?

  x, *xs = xstr
  y, *ys = ystr

  if x == y
    x + " " + longest_subsequence(xs, ys)
  else
    [longest_subsequence(xstr, ys), longest_subsequence(xs, ystr)].max_by {|x| x.size}
  end
end

def phrase_in_common(string1, string2)
  xstr = split_into_words(string1)
  ystr = split_into_words(string2)
  longest_subsequence(xstr, ystr).strip
end

This would pass all your specs, but it returns '' instead of nil when the longest substring is empty.

You can slightly change this implementation to include int inside class String eventually

Edit 2017-10-04

As remarked by moveson, in Rails we can easily make this method to return nil instead of an empty string:

def phrase_in_common(string1, string2)
  xstr = split_into_words(string1)
  ystr = split_into_words(string2)
  longest_subsequence(xstr, ystr).strip.presence
end


Extracting meta data from nupkg file with F# looks like C# Linq The code snippet below examines a nuget package file (zip file with meta) and extracts all file names referenced inside. The final result should be a Set<string> with all file names. My problem is that the code looks like a nested Linq query in c# and the usage of Seq.iter blocks me to build the Set collection. Can you please give me some kind of advice how to improve the F# code? Am I missing some fundamental concept?

getDependencies nugetPackagesFile
                |> Seq.iter(fun (name,ver) ->
                    name + "." + ver
                    |> fun pkgName ->
                        !! ("./src/packages/*/" + pkgName + ".nupkg")
                        |> Seq.iter(fun pkg ->
                            global.NuGet.ZipPackage(pkg).GetFiles()
                            |> Seq.iter(fun file -> filename file.Path |> printfn "%s")))

Current output:

Elders.Cronus.dll
Elders.Cronus.pdb
Elders.Cronus.DomainModeling.dll
Elders.Cronus.DomainModeling.pdb
log4net.dll
log4net.xml
log4net.dll
log4net.xml
log4net.dll
log4net.xml
log4net.dll
log4net.xml
log4net.dll
log4net.xml
log4net.dll
log4net.xml
log4net.dll
log4net.xml
Elders.Multithreading.Scheduler.dll
Elders.Multithreading.Scheduler.pdb
protobuf-net.dll
protobuf-net.pdb
protobuf-net.xml
protobuf-net.dll
protobuf-net.pdb
protobuf-net.xml
protobuf-net.dll
protobuf-net.pdb
protobuf-net.xml

Without being able to actually test (I can only guess what getDependencies, nugetPackagesFile, !! and filename are), something along the following should produce your desired set of dependencies:

getDependencies nugetPackagesFile
|> Seq.collect (fun (name, ver) ->
       !! (sprintf "./src/packages/*/%s.%s.nupkg" name ver)
       |> Seq.collect (fun pkg -> NuGet.ZipPackage(pkg).GetFiles())
       |> Seq.map (fun f -> filename f.Path))
|> Set.ofSeq

or, depending on your taste (kind of DFS vs. BFS)

getDependencies nugetPackagesFile
|> Seq.collect (fun (name, ver) -> !! (sprintf "./src/packages/*/%s.%s.nupkg" name ver))
|> Seq.collect (fun pkg -> NuGet.ZipPackage(pkg).GetFiles())
|> Seq.map (fun f -> filename f.Path)
|> Set.ofSeq

where Seq.collect and map correspond to IEnumerable.SelectMany respectively Select in C#


UIViewController without interfacebuilder I have a template I've been using with Swift for UIViewController and UIView subclasses without a .xib or .storyboard. I want to know how this stacks up against other people's implementations. One weird thing about it is you have to use contentView to access the view, which is kind of similar to UITableViewCell.

Example of a view controller / view that was made using the template:

MyViewController:

import UIKit

class MyViewController: UIViewController {

    // MARK: - View

    var contentView: MyView {
        guard let contentView = self.view as? MyView else {
            fatalError("Cannot create content view")
        }
        return contentView
    }

    // MARK: - View lifecycle

    override func loadView() {
        self.view = MyView()

        // Relationships between view controller and view
        contentView.addingButton.addTarget(self, action: #selector(myButtonTapped), for: .touchUpInside)
    }

    override func viewDidLoad() {
        super.viewDidLoad()

        // Do any additional setup after loading the view.
    }

    // MARK: Actions

    func amyButtonTapped(sender: UIButton) {
        // ...
    }

    // MARK: - Memory manager

    override func didReceiveMemoryWarning() {
        super.didReceiveMemoryWarning()
        // Dispose of any resources that can be recreated.
    }
}

MyView

import UIKit

class OneWayView: UIView {

    // MARK: - Subviews var
    let myButton: UIButton = {
        let button = UIButton(type: .custom)
        button.translatesAutoresizingMaskIntoConstraints = false
        button.setTitle(NSLocalizedString("myButton", comment: ""), for: .normal)
        button.setTitleColor(.orange, for: .normal)
        button.setTitleColor(.lightGray, for: .highlighted)
        return button
    }()

    // MARK: - View lifecycle

    override init(frame: CGRect) {
        super.init(frame: frame)

        // Setup this view
        addSubview(myButton)
        myButton.centerXAnchor.constraint(equalTo: centerXAnchor).isActive = true
        myButton.centerYAnchor.constraint(equalTo: centerYAnchor).isActive = true
    }

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

Grouping all of the view components into a single view can make views easier to reuse, which can be useful in some circumstances.

However, it is not necessary to create a separate view for each view controller, and view controllers often have individual properties for the subview.

While there is nothing technically wrong with your code, it does add an extra burden of having to define a view class for each view controller.

It is also not the "standard" approach, so it might seems out of place to other developers working on the code, which may or may not be a factor to you.

Usually the views are kept as simple as possible, with the individual controls defined on the view controller instead of the view. The view controller becomes the point of reuse instead of the view. View controller composition can be used to create a hierarchy of views.

Most of the code you have shown in the view can be moved into the view controller, which will remove the need to define a class for each view for each view controller.

When in doubt, aim for code that looks the least unusual, or even code that looks boring or obvious.

For example, instead of creating a OneWayView, create a OneWayViewController which contains the same logic as the previous view and view controller.

class OneWayViewController: UIViewController {

    // Probably better to keep this as a weak property.
    let myButton: UIButton = {
        let button = UIButton(type: .custom)
        button.translatesAutoresizingMaskIntoConstraints = false
        button.setTitle(NSLocalizedString("myButton", comment: ""), for: .normal)
        button.setTitleColor(.orange, for: .normal)
        button.setTitleColor(.lightGray, for: .highlighted)
        return button
    }()

    override func viewDidLoad() {
        super.viewDidLoad()

        // Add the button to the root view, which is just a plain view.
        view.addSubview(myButton)

        // Setup constraints.
        myButton.centerXAnchor.constraint(equalTo: centerXAnchor).isActive = true
        myButton.centerYAnchor.constraint(equalTo: centerYAnchor).isActive = true

        // Add target actions.
        myButton.addTarget(self, action: #selector(myButtonTapped), for: .touchUpInside)
    }
}

This new view controller can be composed with other view controllers:

class MainViewController: UIViewController {

    override func viewDidLoad() {
        super.viewDidLoad()

        let viewController = OneWayViewController()
        viewController.willMoveToParentViewController(self)
        addChildViewController(viewController)

        let oneWayView = viewController.view
        view.addSubview(oneWayView)

        // Add constraints to position the oneWayView...
    }
}

On the other hand, if you prefer to stick to your original plans, you could use Swift generics to make the view controller more reusable:

class BaseViewController<T: UIView>: UIViewController {

    var contentView: T {
        guard let contentView = self.view as? T else {
            fatalError("Cannot create content view")
        }
        return contentView
    }

    override func loadView() {
        self.view = T()
    }
}


Make HTML/Javascript Geolocation function more linear I'm checking users locations with HTML Geolocation before unlocking the front door.  Geolocation takes a few seconds in addition to the time it takes the user to approve it, so currently I have a callback for what to do next.

function getLocationWithCallbacks(callback, error_callback) {
  navigator.geolocation.getCurrentPosition(
      function (position) {
        let location = { "latitude": position.coords.latitude, "longitude": position.coords.longitude };
        callback(location);
      },
      function (error) {
        if (error.code === error.PERMISSION_DENIED)
          error_callback()
      }
  )
}

And I call this function like so..

getLocationWithCallbacks(unlock_request, get_location_denied);

However, I'm not sure if there's a more logical way than this.  It would be nice if events happened in a more linear fashion like ("pseudo code")...

function getLocation() {
  navigator.geolocation.getCurrentPosition(
   function(position) { return ['success', position] },
   function(error) { return ['failure', error] }
}

And invoke it with...

let response = getLocation();
response[0] === 'success' ? unlock_request(response[1]) : get_location_denied(response[1])

However I believe this would involve adding promises and await statements which might make the original callback method more desirable.

I'm just not sure what's the cleanest, most logical way of implementing a getLocation function?

Most native functions require callback instead of Promise because they support older browsers, so what you have to do is to pack the method in a Promise

function getLocation() {
    return new Promise(function(resolve, reject){
        navigator.geolocation.getCurrentPosition(
            function(position) { resolve(position); },
            function(error) { reject(error); }
        }
    });
}

Now are able to determine what should happen with the position and the error.

const response = getLocation()
    .then(position =>
        ( {"latitude": position.coords.latitude,
           "longitude": position.coords.longitude} ))
    .then(latLong => { unlock_request(latLong) },
          error => { get_location_denied(error[1]) });

You know that for this request that you want to get the latitude and longitude out of position so you just return a new object with longitude and latitude, the smart thing with promises is that is it a builder pattern, so can call then again and now the parameter will be what was returned in the last then, so all power is given to response rather than getLocation()


Estimating Pi with random darts on dartboard - high complexity issues I've been trying to write nice snippet of code to simulate pi estimation by randomly throwing darts on a dartboard. While running the following code on high but reasonable numbers my mac doesn't plot.

When looking at it I don't find the source of such a high complexity.

I checked similar questions like
this but haven't been able to find straightforward answer.

My guess is that the line plotting real pi is computationally intense - but that's just a hunch.

I'd also appreciate any comment regarding style / efficiency.

import numpy as np
import random
import math
from matplotlib import pyplot as plt

def estimatePi(r,w,h,N):
    center = (w/2.0,h/2.0)
    in_circle = 0
    for i in range(N):
        x = random.uniform(0.0,w)
        y = random.uniform(0.0,h)
        distance = math.sqrt((x-center[0])**2+(y-center[1])**2)
        if distance <= r:
            in_circle += 1
    outOfCircle=N-in_circle
    ratio = float(in_circle)/N
    #ratio = ((r**2)*pi)/(w*h)  // *(w*h)
    #ratio*(w*h) = ((r**2)*pi) // : r**2
    pi = ratio*(w*h)/(r**2)
    return pi

#run, aggregate results:
PiEstimation=[]
num_darts=[]
loopcount = 1000001
i=10000

while i <loopcount:
    result=estimatePi(3,10,10,i)
    num_darts.append(i)
    PiEstimation.append(result)
    i += 15000

# plot:
plt.title('Estimating the Value of Pi - Dartboard Simulation')
plt.plot([0,100000000], [3.14,3.14], 'k-',color="red", linewidth=2.0)
plt.ylabel('Pi Estimation')
plt.xlabel('Number of Darts')
plt.errorbar(num_darts,PiEstimation, yerr=.0001,ecolor='magenta')
plt.show('hold')

Performance

The biggest simple performance improvement would be to use xrange() instead of range() for the loop counter in estimatePi().  Note that i is unused; it is customary to use _ as the name of a "throwaway" variable.

I don't see much point in the r, w, and h parameters.  If you make the dartboard a unit circle centered at the origin, then you could do away with the math.sqrt().

outOfCircle is never used.  Its naming is also inconsistent with in_circle.

Plot quality

For a program that aims to visualize the accuracy of the technique to estimate π, you're being awfully sloppy by plotting a horizontal line at y = 3.14 rather than at math.pi.  The easier way to plot a horizontal line is to use axhline().

It makes no sense to use an errorbar plot here, with an arbitrarily chosen yerr=.0001, since you have just one sample at each x.

Looping

Neither loop is as expressive as it could be.

In estimatePi(), you can calculate in_circle using sum() with a generator expression.  (When coerced into an integer, True is treated as 1, and False as 0.)

To make the lists num_darts and PiEstimation, you can use range() and a list comprehension, respectively.

Suggested solution

Take care to follow PEP 8 naming conventions.

from math import pi
from random import uniform
import matplotlib.pyplot as plt

def estimate_pi(n):
    in_circle = sum(
        uniform(-1, 1)**2 + uniform(-1, 1)**2 <= 1
        for _ in xrange(n)
    )
    return 4.0 * in_circle / n

darts = range(10000, 1000001, 15000)
pi_estimations = [estimate_pi(n) for n in darts]

plt.title('Estimating the Value of Pi - Dartboard Simulation')
plt.ylabel('Pi Estimation')
plt.xlabel('Number of Darts')
plt.axhline(y=pi, color="red", linewidth=2.0)
plt.plot(darts, pi_estimations, marker='o')
plt.show('hold')

This runs in under a minute on my machine.


Merging every two objects into single single object making group by a property C# LINQ I have a object list like below. I want to join every two rows into single row based on column B. It is sure that only two rows would be there for every single column B value.
Input

Output

However, my solution works. but I am looking for more better solution. I am not much happy with my solution.
My solution:
var groupByItems = items.GroupBy(x => x.ColumnB).Select(x => new MappingClass
    {
        ColumnA= x.FirstOrDefault().ColumnA,
        ColumnB= x.FirstOrDefault().ColumnB,
        ColumnC= x.Where(r=> !string.IsNullOrEmpty(r.ColumnC)).Select(r=>r.ColumnC).FirstOrDefault(),
        ColumnD= x.Where(r => !string.IsNullOrEmpty(r.ColumnD)).Select(r => r.ColumnD).FirstOrDefault(),
    }).ToList();

Now groupByItems object returns me two rows as expected.

If you want to reduce the iteration to get the value for ColumnC and ColumnD and make the code cleaner, you can try using Aggregate instead.
var output = input
             .GroupBy(d => new {d.ColumnA, d.ColumnB})
             .Select(
                 g => g.Aggregate(
                     new MappingClass {ColumnA = g.Key.ColumnA, ColumnB = g.Key.ColumnB},
                     (result, next) =>
                     {
                         result.ColumnC ??= next.ColumnC;
                         result.ColumnD ??= next.ColumnD;
                         return result;
                     }))
              .ToList();


Script to generate templated PHP code I've created a script to create a module in our MVC framework. The structure of the module is like this (for this example, the module is called test):

test >
    Test.php
    controllers >
        IndexController.php
    models >
        TestModel.php
    open >
       css >
       js >
    views >

Here's what I have for the script. If can have an optional flag of -o to allow the script to remove and override the folder is it already exists.

#!/bin/bash

OVERRIDE=false

while getopts ":o" opt; do
        case $opt in
                o ) OVERRIDE=true
        esac
done

# Check we're in the right directory.

if [ ${PWD##*/} != "modules" ]; then
        echo "Make sure you're in the modules directory"
        exit 1
fi

echo "Enter the module name you want:"

# Get the name, and format it into the formats we need.
read moduleName
lowercaseModuleName=${moduleName,,}
uppercaseFirstModuleName=${lowercaseModuleName^}

# Check if the directory exists already.
if [ -d $lowercaseModuleName ]
    then
        if [ "$OVERRIDE" = true ]; then
                echo "Directory already exists, overridding it because of the -o flag"
                rm -rf $lowercaseModuleName
        else

                echo "Directory '$lowercaseModuleName' already exists, please remove if and try again. To have it overridden please use the -o flag"
                exit 1;
        fi
fi

# Make dir.
echo "Adding module $lowercaseModuleName..."
mkdir $lowercaseModuleName
cd $lowercaseModuleName

# Make root class
echo "Adding Root class..."
cat > $uppercaseFirstModuleName.php << EOF
<?php

class $uppercaseFirstModuleName extends Layer_Shared_Controller {

        public function preDispatch(){

        }
}
EOF

# Make controllers
echo "Adding IndexController..."
mkdir controllers
cd controllers

cat > IndexController.php << EOF
<?php

class IndexController extends $uppercaseFirstModuleName {

        /**
         * Index action.
         */

        public function indexAction(){

        }
}
EOF

# Add model
echo "Adding model..."
cd ../
mkdir models
cd models

cat > $uppercaseFirstModuleName"Model.php" << EOF
<?php

class ${uppercaseFirstModuleName}Model extends Layer_Model {

}
EOF

# Add open folder
echo "Adding open folder..."
cd ../
mkdir open
cd open
mkdir css js
cd ../

# Add view folder
echo "Adding view folder..."
mkdir views
cd ../

echo "Done! Module $uppercaseFirstModuleName created."

Is there anything bad/suboptimal, or that could be done easier?

just a few ideas from the top of my head.

I would define a variable with your start directory at the top of the script, so to that you can always refer to absolute paths in contrast to using cd ../ often. In case you ever change the structure of your script, this will ensure that everything keeps working.

MODULE_DIR=$(readlink -f $lowercaseModuleName)

Now the directory creation can be simplified:

# Add open folder
echo "Adding open folder..."
mkdir -p ${MODULE_DIR}/open/{css,js}/

# Add views folder
echo "Adding view folder..."
mkdir ${MODULE_DIR}/views

As for the structure, I'd probaly create all the directories in one 'paragraph' and add the templates in a second step, just so that you one place where you can clearly see which directories are being created. But that might just be my personal tast.

In the template creation I'd use absolute paths here as well, using the new MODULE_DIR variable. Keeps you from having to navigate around a lot using cd which might cause confusion later on.

Hope this helps!


Getting the smallest possible integer ratio between two numbers I wrote this code to find the smallest possible integer ratio between two numbers:

void ratio(int a, int b) {
    int min = Math.min(a, b);
    int max = Math.max(a, b);
    if(max % min == 0) {
               System.out.println("1" + " " + max / min);
               return;
        }
    for (int i = 2; i <= min; i++) {
        while(max % i == 0 && min % i == 0) {
            max /= i;
            min /= i;
        }
    }
    System.out.println(max + " " + min);
}

Is there any better/efficient way to do this? Am I missing any good algorithm?

You need to find the GCD and then divide both numbers by that.

Euclid's algorithm is the classic way to do this.

int gcd(int p, int q) {
    if (q == 0) return p;
    else return gcd(q, p % q);
}

void ratio(int a, int b) {
   final int gcd = gcd(a,b);
   System.out.println(a/gcd + " " + b/gcd);
}

If you want to swap a and b to have the biggest first then that's easy too

void ratio(int a, int b) {
   final int gcd = gcd(a,b);
   if(a > b) {
       showAnswer(a/gcd, b/gcd);
   } else {
       showAnswer(b/gcd, a/gcd);
   }
}

void showAnswer(int a, int b) {
   System.out.println(a + " " + b);
}


Balanced smileys check algorithm Problem

  Your friend John uses a lot of emoticons when you talk to him on
  Messenger. In addition to being a person who likes to express himself
  through emoticons, he hates unbalanced parenthesis so much that it
  makes him go :(

  Sometimes he puts emoticons within parentheses, and you find it hard
  to tell if a parenthesis really is a parenthesis or part of an
  emoticon.

  A message has balanced parentheses if it consists of one of the
  following:

  An empty string ""
  One or more of the following characters: 'a' to 'z', ' ' (a space) or ':' (a colon)
  An open parenthesis '(', followed by a message with balanced parentheses, followed by a close parenthesis ')'.
  A message with balanced parentheses followed by another message with balanced parentheses.
  A smiley face ":)" or a frowny face ":("
  Write a program that determines if there is a way to interpret his message while leaving the parentheses balanced.

I'm working on this balanced smileys checking algorithm, and my current solution is very naive, just enumerate all possible ways to interpret :) and :(. I'm wondering if any smarter ideas in terms of algorithm time complexity perspective.

def procee_raw_string(source):
    result = []
    for i,v in enumerate(source):
        if v == '(':
            if i > 0 and source[i-1] == ':':
                result.append(source[i-1:i+1])
            else:
                result.append(v)
        elif v == ')':
            if i>0 and source[i-1] == ':':
                result.append(source[i-1:i+1])
            result.append(')')
    return ''.join(result)

def check_balance(source):
    stack = []
    for s in source:
        if s == '(':
            stack.append(s)
        else: # s is ')'
            if len(stack) == 0 or stack[-1] != '(':
                return False
            else:
                stack.pop(-1)
    if len(stack) > 0:
        return False
    return True

def generate_balance_string(source, index, prefix, result):
    if index >= len(source):
        result.append(prefix[:])
        return
    if source[index] == ':' and source[index+1] == '(':
        prefix.append('(')
        generate_balance_string(source, index+2, prefix, result)
        prefix.pop(-1)
        generate_balance_string(source, index+2, prefix, result)
    elif source[index] == ':' and source[index+1] == ')':
        prefix.append(')')
        generate_balance_string(source, index+2, prefix, result)
        prefix.pop(-1)
        generate_balance_string(source, index+2, prefix, result)
    elif source[index] == '(':
        prefix.append('(')
        generate_balance_string(source, index+1, prefix, result)
        prefix.pop(-1)
    elif source[index] == ')':
        prefix.append(')')
        generate_balance_string(source, index+1, prefix, result)
        prefix.pop(-1)

if __name__ == "__main__":
    raw_smile_string = 'abc(b:)cdef(:()'
    process_string = procee_raw_string(raw_smile_string)
    result = []
    generate_balance_string(process_string, 0, [], result)
    for r in result:
        print check_balance(r)

Additional thoughts,

I am not sure if we could do such simple check for balance?

If number of close parenthesis (excluding the smileys) are ever more than the number of open parenthesis, then parentheses are not balanced.
If number of open parenthesis at the end (excluding the frowns) are more than the close ones, then also the parentheses are not balanced.

A couple of minor style points:

In check_balance() the return can be simplified from:

if len(stack) > 0:
    return False
return True

to:

return len(stack) <= 0

In generate_balance_string() there is a long if/elif string.  But the first
element does not follow the pattern and has an embedded return it does not need.
You can drop the return and make the following if an elif to make your code more uniform:

def generate_balance_string(source, index, prefix, result):
    if index >= len(source):
        result.append(prefix[:])
    elif source[index] == ':' and source[index + 1] == '(':
        ...


Calculating n-ary Cartesian Product too slow The method gets called extensively throughout my programm, is there any way I can speed it up or improve other aspects?

The code calculates the n-ary Cartesian Product of a List containing n Lists, so that for example:
Product([[1],[2,3],[4,5]]) yields [1,2,4],[1,2,5],[1,3,4],[1,3,5]
public static IEnumerable<T[]> Product<T>(T[][] items)
    {
        T[] currentItem = new T[items.Length];
        static IEnumerable<T[]> go(T[][] items, T[] currentItem, int index)
        {
            if (index == items.Length)
            {
                yield return currentItem;
                yield break;
            }
            else
            {
                foreach (T item in items[index])
                {
                    currentItem[index] = item;
                    foreach(var j in go(items, currentItem, index + 1))
                    {
                        yield return j;
                    }
                }
            }

        }
        return go(items, currentItem, 0);
    }

A possible problem i can think of, is the
foreach(var j in go(items, currentItem, index + 1))
       {
           yield return j;
       }

section, since it takes n yields for every (single) array that is part of the n-ary Cartesian Product.

The implementation contains one small but important bug (or feature?), it yields the same array instance from each loop.
Consider this example.
int[][] numbers = new[] { new[] { 1 }, new[] { 2, 3 }, new[] { 4, 5 } };

foreach (int[] arr in Product(numbers))
{
    Console.WriteLine(string.Join(", ", arr));
}

The output is perfect
1, 2, 4
1, 2, 5
1, 3, 4
1, 3, 5

But if you want to store the result as jagged array and process it later:
foreach (int[] arr in Product(numbers).ToArray())
{
    Console.WriteLine(string.Join(", ", arr));
}

You'll get this output
1, 3, 5
1, 3, 5
1, 3, 5
1, 3, 5

That's because it's an array of four references to the same array.
Next small thing is one redundant line of code
yield break;

You don't need it because previous line is the last statement in the method. The method exits after it.
Fix is simple
public static IEnumerable<T[]> Product<T>(T[][] items)
{
    T[] currentItem = new T[items.Length];
    static IEnumerable<T[]> Go(T[][] items, T[] currentItem, int index)
    {
        if (index == items.Length)
        {
            yield return (T[])currentItem.Clone();
        }
        else
        {
            foreach (T item in items[index])
            {
                currentItem[index] = item;
                foreach (T[] j in Go(items, currentItem, index + 1))
                {
                    yield return (T[])j.Clone();
                }
            }
        }
    }
    return Go(items, currentItem, 0);
}

That's it.
Performance while keeping it as returning arrays will be slow because e.g. for 10 arrays of 10 items it will yield 10*10*10*10*10*10*10*10*10*10 = 10000000000 arrays of 10 items each. But you want up to 15. It won't be fast at all.
But let's try to eliminate the recursion.
Consider an alternative implementation of the same
public static IEnumerable<T[]> MyProduct<T>(T[][] items)
{
    int length = items.Length;
    int[] indexes = new int[length];

    while (true)
    {
        T[] arr = new T[length];
        for (int i = 0; i < length; i++)
        {
            arr[i] = items[i][indexes[i]];
        }
        yield return arr;

        int row = length - 1;
        indexes[row]++;
        while (indexes[row] == items[row].Length)
        {
            if (row == 0)
                yield break;
            indexes[row] = 0;
            row--;
            indexes[row]++;
        }
    }
}

To understand this implementation imagine a digital clock. What if you make 3 arrays 24,60,60 items and fill it with numbers from 0 to 23/59. The Product will yield time to display for each second of the day - 86400 arrays of 3 items each. Then imagine how clock counts time on display and you'll catch the idea of this implementation.
Looks like it works
1, 2, 4
1, 2, 5
1, 3, 4
1, 3, 5

And finally let's make a benchmark for both implementations. Using Benchmark.NET of course. For example with 6 arrays of 6 items each.
[MemoryDiagnoser]
public class MyBenckmark
{
    private int[][] numbers;

    [GlobalSetup]
    public void Setup()
    {
        numbers = new int[6][];
        for (int i = 0; i < 6; i++)
        {
            numbers[i] = Enumerable.Range(i * 6, 6).ToArray();
        }
    }

    [Benchmark]
    public void RunProduct()
    {
        foreach(var _ in Product(numbers)) ;
    }

    [Benchmark]
    public void RunMyProduct()
    {
        foreach (var _ in MyProduct(numbers)) ;
    }

    public static IEnumerable<T[]> Product<T>(T[][] items)
    {
        T[] currentItem = new T[items.Length];
        static IEnumerable<T[]> Go(T[][] items, T[] currentItem, int index)
        {
            if (index == items.Length)
            {
                yield return (T[])currentItem.Clone();
            }
            else
            {
                foreach (T item in items[index])
                {
                    currentItem[index] = item;
                    foreach (T[] j in Go(items, currentItem, index + 1))
                    {
                        yield return (T[])j.Clone();
                    }
                }
            }
        }
        return Go(items, currentItem, 0);
    }

    public static IEnumerable<T[]> MyProduct<T>(T[][] items)
    {
        int length = items.Length;
        int[] indexes = new int[length];

        while (true)
        {
            T[] arr = new T[length];
            for (int i = 0; i < length; i++)
            {
                arr[i] = items[i][indexes[i]];
            }
            yield return arr;

            int row = length - 1;
            indexes[row]++;
            while (indexes[row] == items[row].Length)
            {
                if (row == 0)
                    yield break;
                indexes[row] = 0;
                row--;
                indexes[row]++;
            }
        }
    }
}

Go!
BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19042
Intel Core i7-4700HQ CPU 2.40GHz (Haswell), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=5.0.102
  [Host]     : .NET Core 3.1.11 (CoreCLR 4.700.20.56602, CoreFX 4.700.20.56604), X64 RyuJIT
  DefaultJob : .NET Core 3.1.11 (CoreCLR 4.700.20.56602, CoreFX 4.700.20.56604), X64 RyuJIT

|       Method |      Mean |     Error |    StdDev |     Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------- |----------:|----------:|----------:|----------:|------:|------:|----------:|
|   RunProduct | 28.509 ms | 0.2488 ms | 0.2077 ms | 6750.0000 |     - |     - |  20.08 MB |
| RunMyProduct |  1.274 ms | 0.0103 ms | 0.0086 ms |  712.8906 |     - |     - |   2.14 MB |

You vote.


JavaScript function to calculate the average of an array I have this function that calculates average. I don't want it to look like this. I am hoping for a less cliche average function because I see that most people's look like this.

function mean(array) {
    var num=0;
    for (var i=1;i<=array.length;i++) num=num+array[i];
    var divide=num/array.length;
    alert(divide);
}

Can anyone change this up to be better than before? It doesn't have to be more efficient but just look better.

First of all, your code is incorrect. Arrays have a starting index of 0, not 1. Furthermore, array.length is one more than the last filled index (because the length is 1 based, and the array is 0 based. Yes, this is confusing.) So you're starting by not including the first item, and then also adding up an undefined item.

And what happens if you get an empty array? You divide by zero, which is MADNESS!! MADNESS I TELL YOU!

Second, there's a reason the code looks the same as everywhere: You have followed the definition of the average; the sum of components divided by the number of components. There's no shame in this, you should actually be proud that you've managed to conform to the standard.

If you really want, you can make the loop implicit. ES5 (ECMAScript 5, the latest js standard) defined an Array.prototype.reduce function (click link for the reference.) It's a perfect fit for your needs, but doesn't change the core algorithm.

Third, your mean function is doing something unexpected: Instead of just calculating the mean, it displays it as well. This does not follow expectations (or at least my expectation), as a function which calculates the mean should do that, and just that. That translates to a very important design aspect, which says that a function should do one thing, one thing only, and do it well. Because imagine if Math.sqrt, for instance, starts displaying the square roots instead of giving you them. Like division by zero, it's madness all over again!


Cancelling Local WebClient ASync Operation I had previously asked this question on SO.  The answer pointed me towards using the task parallel library for multiple parallel downloads.  I'm not actually utilizing this exact code, but it got me to rethink my design.

The one open issue that wasn't really addressed (and I didn't ask it), cancelling the a local WebClient.  It isn't as simple as just calling WebClient.CancelASync(); since the scope of WebClient is long gone by the time you need to cancel.

This isn't my code or even how I am approaching the problem, but just part of the test example I put together to see how this works.  It seems to work, although it means having to wait for an event callback before the cancel is called.  So I was wondering if there was another alternative.

private bool pendingCancel = false;
private Queue<Uri> queue = LoadQueue();

public void ASyncDownload()
{
    if (queue.Count == 0) return;

    var uri = queue.Dequeue();

    WebClient client = new WebClient();
    client.DownloadProgressChanged
        += (sender, e) =>
        {
            if (pendingCancellation)
            {
                ((WebClient)sender).CancelAsync();
                return;
            }

            //do something to report progress
        };

    client.DownloadDataCompleted
        += (sender, e) =>
        {
            if (!e.Cancelled || pendingCancel)
            {
                if (e.Error == null)
                {
                    // do something with e.Results
                }
                else
                {
                    // report error
                }
            }
            else
            {
                // report cancellation
            }
        };

    client.DownloadDataAsync(uri);
}

My thought process is for any long running downloads, waiting until the next item in the queue to exit would not be appropriate, so the idea would be to call CancelASync() on the sender of the DownloadProgressChanged event handler.

Is this the best alternative short of putting a webClient field in the class?  And are there dangers or possible unpredictable behavior that I have not found in my testing?

I am not able to comment on your post due to a lack of reputation, but when I did something similar to this, I used a CancellationToken. It's been around for quite some time now (.NET 4.0), and is generally accepted in most .NET Async APIs. It should achieve what you're trying to do. The documentation for the concept is located here.

You don't have to put a WebClient in your class (as a field) if you don't want to. Most of the interactions I've seen with web servers (in C#) behave very similar to the Entity Framework in that you generally send a request, which yields some form of IEnumerable that you have to deal with. So, you could wire up a service util for handling the common bits for a web client, and generate a unique one per request. I did something similar to that, and it worked well, but you need to be cautious of shared state in that scenario (construction really needs to be construction - not use of a singleton - if you're generating a unique client every time).

I hope this helps!


Utilities for Various Builtins #2 This is the final iteration of an iterative review. The previous iteration can be found here: Utilities for various builtins #1

This code solves the following issue:

var i = 9
i.sqrt();     // 3
Math.sqrt(i); // 3

And other similar dilemmas. It also saves you from having to write your own mean and max function. I've also been told this is known as "syntatic sugar".

It's also worth it's weight in keypresses.

/**
 * Adds some utilities to the base classes.
 * Note that this is modular and is able to modify any class it is given.
 * @author FreezePhoenix
 * @version 1.1.0
 */

/**
 * @type {Object}
 * @constant
 */
const XtraUtils = {};
/**
 * @class
 * @classdesc Enables utilities to be added to classes.
 */
class Utility {
   /**
    * Create a new utility
    * @param {Object} ofWhat               - First part of the filter to test if it is a constructor.
    * @param {Function} ofWhat.constructor - Second part of the filter.
    * @return {Boolean}                    - false means something went wrong, true means it succeded.
    * @this Utility
    * @throws {TypeError} The first argument of Utility must be a class.
    */
    constructor(ofWhat) {
        if (typeof ofWhat == 'function' && typeof new ofWhat() === 'object') {
            this.utils = {};
            Object.defineProperty(this, 'aidsIn', {
                value: ofWhat,
                writable: false
            });
            this.Activated = [];
            this.OverWritten = {};
            return true;
        } else {
            console.error(
                'new Utility expects an object template to be passed as the first argument.'
            );
            return false;
        }
    }
    /**
     * Activates the utilities in this Utility instance.
     * @this Utility
     */
    activate() {
        for (const x in this.utils) {
            if (this.aidsIn.prototype[x]) {
                this.OverWritten[x] = this.aidsIn.prototype[x].valueOf();
            }
            this.aidsIn.prototype[x] = this.utils[x];
            this.Activated.push(x);
        }
    }
   /**
    * @this Utility
    */
    deactivate() {
        this.Activated.forEach((item, index) => {
            if (this.OverWritten[item]) {
                this.aidsIn.prototype[item] = this.OverWritten[item];
            } else {
                delete this.aidsIn.prototype[item];
            };
        });
        this.Activated = [];
    }
    /**
     * @this Utility
     */
    addUtil(name, util) {
        this.utils[name] = util;
    }
}
XtraUtils.Number = new Utility(Number);
XtraUtils.String = new Utility(String);
XtraUtils.Array = new Utility(Array);
XtraUtils.Number.addUtil('map', function(in_min, in_max, out_min, out_max) {
    return (this - in_min) * (out_max - out_min) / (in_max - in_min) + out_min;
});
XtraUtils.Number.addUtil(function(inputBase = 10, outputBase = 10) {
    parseInt(this, inputBase).toString(outputBase);
}, 'toBaseN');
XtraUtils.Number.addUtil('pow', function(n) {
    return Math.pow.apply(Math, this, n);
});
XtraUtils.Number.addUtil('ceil', function() {
    return Math.ceil.apply(Math,this);
});
XtraUtils.Number.addUtil('floor', function() {
    return Math.floor.apply(Math,this);
});
XtraUtils.Number.addUtil('sqrt', function() {
    return Math.sqrt.apply(Math,this);
});
XtraUtils.Array.addUtil('max', function() {
    return Math.max.apply(Math, this);
});
XtraUtils.Array.addUtil('min', function() {
    return Math.min.apply(Math, this);
});
XtraUtils.Array.addUtil('mean', function() {
    let total = 0;
    var isFirst = true;
    this.forEach(function(item) {
        if (typeof item === 'string') {
            throw Error('Item must be all numbers');
        }
        total += item;
    });
    const mean = total / this.length;
    return mean;
});

The Utility class should keep track of its own activation state, otherwise calling .activate() multiple times on the same instance (without calling .deactivate() in between) might cause some problems; also, in the current code, calling .addUtil() after .activate() leaves the instance in a limbo state, where the method name is added to this.utils but isn't actually available on the aided class's prototype, even though the instance is activated. I can imagine this leading to some frustrating debugging on a more complex project. I'd suggest that calling .addUtil() on an activated Utility instance should either immediately activate the added function, or throw an Error (this sounds more consistent with your approach).

(also, as a side note, I wouldn't call this syntactic sugar. Sugar is just simpler way of writing something. e.g. { ...obj } is sugar for Object.assign({}, obj); it doesn't actually add new functionality, which your code certainly does)


PHP function to convert a Portuguese word from plural to singular I know, this sounds really difficult, but it is really easy.

I needed to convert a single Portuguese word in the plural into singular. I know there's a right name for that, but it is escaping me.

The rules are simple, and will compile them from http://www.easyportuguese.com/portuguese-lessons/plural/ (but applying in reverse):

If the word ends in a vowel, remove the s at the end
Words ending in ões, ães and ãos should end with ão
Words ending in is, remove the is and add l to the end
Special case: accents should be removed, if needed. The only cases I saw were anéis and pastéis, which have to be anel and papel.
Words ending in ns get it replaced with m
Words ending with [rsz]es should lose the es
Special case: words ending in eses need the first e replaced with ê, like in meses => mês
Some words are always used in the plural, like óculos, parabéns and férias.

Below, here's the code:

function plural_to_singular($string)
{
    if(preg_match('/^(?:[oó]culos|parab[eé]ns|f[eé]rias)$/iu', $string))
    {
        return $string;
    }

    $regexes = array(
        '[õã]es' => 'ão',
        '[áó].*eis' => 'el',
        '[eé]is' => 'el',
        '([^eé])is' => '$1l',
        'ns' => 'm',
        'eses' => 'ês',
        '([rzs])es' => '$1',
        's' => ''
    );

    foreach($regexes as $fragment => $replace)
    {
        $regex = '/' . $fragment . '$/ui';
        if(preg_match($regex, $string))
        {
            return preg_replace($regex, $replace, $string);
        }
    }

    return $string;
}

You can try it on http://sandbox.onlinephpfunctions.com/code/7947a0efd16f361e89491e4a64f71b578d2278df with some testcases

In your opinion, what can I improve?

Is there any obvious butchering or performance killer?

Let me start by saying, that I have respect for Mike Brant, and have been enjoying his posts for quite a while now.  However, I disagree with some of his review.

$regex_config can not store the the replacement values as associative keys unless the regex patterns that use the same replacement value are merged.  This is not explained in the ... (yatta-yatta).  The key clash would be on el.
Simply throwing 1 at the end of preg_replace() is NOT going to provide the desired output.  Declaring a replacement limit on the call will only limit the replacements PER array element. The damage is evident in this output: meses => mês = mê
Most trivially, array_values() doesn't need to be called because preg_replace() is "key ignorant" regarding the array inputs.

For this process to maintain accuracy, there needs to be a return as soon as a replacement occurs on the input string.  To avoid calling multiple replacements, iterate the array of pattern-replacement pairs.
You can avoid using capture groups and shorten your replacement strings in a couple places by implementing the \K metacharacter (restart fullstring match).  This way you don't need to use $1 or rewrite a literals from the pattern into the replacement.
If you need to add case-sensitivity to your replacement process, you can check the last character of the incoming string.  If it is uppercase, assume the whole string is in CAPS and call mb_strtoupper().
I don't have a sample string to test against ~[áó].*eis$~iu, but I wonder if this is accurate/correct and my Portuguese is not too sharp.
After my implementation of \K you can see that two pairs of patterns are making the same replacement.  If you don't expect to be making lots of future adjustments to this set of regex patterns, you could combine the patterns with a pipe.  Here's what I mean: '~(?:[áó].*eis|[eé]is)$~iu' => 'el', and '~(?:[rzs]\Kes|s)$~iu' => ''
I am using the regex patterns as the keys because they will all logically be unique.  the same cannot be said about the replacement values (not without merging anyhow).

Code: (Demo)

function is_allcaps($string)
{
    $last_letter = mb_substr($string, -1, 1, 'UTF-8');
    return $last_letter === mb_strtoupper($last_letter, 'UTF-8');
    // otherwise use cytpe_upper() and setlocale()
}

function plural_to_singular($string)
{
    // quick return of "untouchables"
    if(preg_match('~^(?:[oó]culos|parab[eé]ns|f[eé]rias)$~iu', $string))
    {
        return $string;
    }

    $regex_map = [
        '~[õã]es$~iu' => 'ão',
        '~(?:[áó].*e|[eé])is$~iu' => 'el',
        '~[^eé]\Kis$~iu' => 'l',
        '~ns$~iu' => 'm',
        '~eses$~iu' => 'ês',
        '~(?:[rzs]\Ke)?s$~iu' => ''
    ];

    foreach ($regex_map as $pattern => $replacement)
    {
        $singular = preg_replace($pattern, $replacement, $string, 1, $count);
        if ($count)
        {
            return is_allcaps($string) ? mb_strtoupper($singular) : $singular;

        }
    }
    return $string;
}

$words = array(
    'óculos' => 'óculos',
    'papéis' => 'papel',
    'anéis' => 'anel',
    'PASTEIS' => 'PASTEL',
    'CAMIÕES' => 'CAMIÃO',
    'rodas' => 'roda',
    'cães' => 'cão',
    'meses' => 'mês',
    'vezes' => 'vez',
    'luzes' => 'luz',
    'cristais' => 'cristal',
    'canções' => 'canção',
    'nuvens' => 'nuvem',
    'alemães' => 'alemão'
);

foreach($words as $plural => $singular)
{
    echo "$plural => $singular = " , plural_to_singular($plural) , "\n";
}


C++11 Logging Architecture To create a Logging system for a larger application, I came up with the following draft. The log sinks aren't detailed yet, but the factory method for logger creation and a rough draft of the logger itself are. Is this a good start and worth continuing? Or are there pitfalls you can see?

class LoggingSink; // abstract base class
class ConsoleSink : public LoggingSink; // some concrete logging sink
class LogfileSink : public LoggingSink; // ... too

/***********************************************/
class Logger {
public:
    void log(const std::string& msg) {
        for (LoggingSink ls : Sinks)
            ls.log(msg);
    }

private:
    std::list<std::reference_wrapper<LoggingSink>> Sinks;
    friend class LoggerFactory;
};

/***********************************************/
class LoggerFactory {
public:
    std::unique_ptr<Logger> create() {
        std::unique_ptr<Logger> result = std::make_unique<Logger>();
        result->Sinks.assign(StandardAssignedSinks.begin(), StandardAssignedSinks.end());
        return result;
    }

    void addStandardSink(LoggingSink& ls) {
        Sinks.push_back(std::reference_wrapper<LoggingSink>(ls));
    }

private:
    std::list<std::reference_wrapper<LoggingSink>> StandardAssignedSinks;
};

/***********************************************/
int main() {
    ConsoleSink cs;
    LogfileSink lfs("mylogfile.txt");

    LoggerFactory lf;
    lf.addStandardSink(cs);  // adding as reference for polymorphism
    lf.addStandardSink(lfs); // perhaps the addition of some standard sinks should be done in the constructor of LoggerFactory???

    std::unique_ptr<Logger> myLogger = lf.create();

    myLogger->log("Hello Log, I'm going to be injected soon!");
    MyClass myClassInstance(myLogger); // constructor dependency injection

    MyClass myOtherInstance(lf.create()); // direct DI
}

I'm also unsure about the friend class. This is something I never really liked with factories. Any ideas how to improve here?

As noted in the comments:

I would make the factory retain ownership:

class LoggerFactory {

typedef std::map<std::string, std::unique_ptr<Logger>>  Container;
typedef Container::iterator                             iterator;
typedef Container::const_iterator                       const_iterator;
typedef Container::value_type                           value_type;

typedef std::list<std::reference_wrapper<LoggingSink>>  SinkHolder;

public:
    Logger& get(std::string const& loggerName = "default")
    {
        iterator find = cont.find(loggerName);
        if (find == cont.end())
        {

            std::unique_ptr<Logger> result = std::make_unique<Logger>();

            // Personally I would not do this.
            // I would pass the iterators to the constructor
            // of the Logger object.
            result->Sinks.assign(std::begin(standardAssignedSinks),
                                 std::end(standardAssignedSinks));

            auto insertR = cont.insert(value_type(loggerName, result));
            if (!insertR.second)
            {    throw std::runtime_error("Insertion Failed");
            }
            find = insertR.first;
        }
        return *(find->second);
    }

    void addStandardSink(LoggingSink& ls) {
        standardAssignedSinks.empace_back(ls);
    }

private:
    SinkHolder standardAssignedSinks;
    Container  cont;
};

Now you can have multiple loggers. But you can re-use loggers (as each logger has its own name. Most of the time people should be using the default logger.

Building a streamming logger is not that difficult it just requires an intermediate buffer object to accumulate the message. When it is destroyed it sends the message to the logger.

class Logger {
    public:
        template<typename T>
        LogStreamBuffer  operator<<(T const& data) // Member so left hand side  implied as Logger
        {
            // This is not perfect
            // This may cause a copy (and thus a destruction)
            // If there is a copy/destruction cycle an extra message will be
            // sent. You can code around this problem.
            // This is just to get you started.

            return LogStreamBuffer(*this) << data;
        }
    // OTHER STUFF
};
class LogStreamBuffer
{
    Logger&            parent;
    std::stringstream  buffer;

    public:
        LogStreamBuffer(Logger& parent)
           : parent()
        {}
        ~LogStreamBuffer()
        {
            // When the object is destroyed.
            // Log the accumulated message with the parent.
            parent.log(buffer.str());
        }
        template<typename T>
        LogStreamBuffer&  operator<<(T const& data)
        {
            // Anything you log is just appended to the message.
            buffer << data;
            return *this;   // return a reference to yourself.
                            // So you can chain messages.
        }
};

// Usage:
int main()
{
     LoggerFactory   factory;
     Logger          logger = factory.get("MyLogger");

     logger << "This is a test: " << 1 << " Got it";
       //   ^^^
       //   Creates a LogStreamBuffer
       //   As an invisable temporary object.
       //   The subsequent `operator<<` will accumulate
       //   data in the buffer. When the statement is
       //   finished (at the ;) all temporaies will be
       //   destroyed. This calls the LogStreamBuffer
       //   destructor and logs your data to the Logger
       //   object.

}


Ruby-like squish in Haskell squish is an onomatopoeic string function provided by Ruby on Rails that works on strings.

Given a string, squish condenses all the consecutive spaces into one.1 (Imagine that you take a string at both ends and compress it with both hands, spaces will be merged and you will hear the typical squish sound).

I went I little overboard and wrote three variations of this function:

squishAll that will squish together all repeated chars:
squishAll "aaabbbccc" -> "abc"
squishOn that will squish together any specified char:
squishOn 'a' "aaabbbccc" -> "abbbccc"
squish for spaces only: squish "foo   bar   baz" -> "foo bar baz"

import Data.List
import Data.Foldable

squishAll :: Eq a => [a] -> [a]
squishAll = (map head) . group

squishOn :: Eq a => a -> [a] -> [a]
squishOn item = Data.List.concat . (map firstIfEqualElseAll) . group
  where firstIfEqualElseAll (x:xs) = if x == item then [x] else (x:xs)

squish = squishOn ' '

main = print $ [squishAll "aaabbbccc", squishOn 'a' "aaabbbccc", squish "foo   bar   baz"]

1 The Ruby function that this is modelled on would also remove any spaces at the start and at the end.  I've chosen not to implement this.

I have no complaints about this.

Esthetically it seems that squishOn 'a' "aaabcbcbc" should create the groups:

[ "aaa", "bcbcbc" ]

instead of:

[ "aaa", "b", "c", "b", "c", "b", "c" ]

Due to laziness it may not make any difference. This is something that perhaps should be benchmarked. To form the groups the first way use the groupBy function from Data.List like this:

groupBy (\x y -> x == y || (x /= 'a' && y /= 'a')) input

So, two characters are in the same group if they are equal or if they are not both the character to be squished. Of course, if you do this you'll have to switch the groups back together differently with something like:

concatMap f (groupBy (\x y -> ...) ...)
  where f ('a':xs) = "a"
        f xs       = xs

(Substitute the literal 'a' with the squishing character.)

This alternate approach probably makes more sense if you were to define squishing on ByteString or Text.


Wrapper class for calling external API I have a PHP cronjob script. I need to send requests and get responses from some external API.

So, there is code for the API class. In this class, I connect to the API and  call public methods of this class from another class.

class Api {
private $authApi;
private $logger;

function __construct()
{
    global $_AUTH_API;
    $this->authApi = $_AUTH_API;
    $this->logger = new CustomLogs();
}

public function getBrands()
{
    $options = [
        'request' => 'get_brands',
    ];

    $brands = $this->requestApi($options);

    return $brands->rows;
}

public function get****() {}

/**
 * @param $data
 *
 * @return array|mixed|object
 * @throws Exception
 */
private function requestApi($data)
{
    $data = $this->authApi + $data;

    $data = [
        'data' => json_encode($data)
    ];

    $ch = curl_init();
    curl_setopt($ch, CURLOPT_URL, "https://api.com/api/");
    curl_setopt($ch, CURLOPT_SSL_VERIFYPEER,FALSE);
    curl_setopt($ch, CURLOPT_SSL_VERIFYHOST,2);
    curl_setopt($ch, CURLOPT_POST, 1);
    curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
    curl_setopt($ch, CURLOPT_POSTFIELDS, $data);

    $answer = json_decode(curl_exec($ch));
    curl_close($ch);

    if (empty($answer)) {
        $this->logger->log->error('Empty answer from API');
        throw new Exception('Empty answer from API');
    } else {
        if ($answer->success != true) {
            $this->logger->log->error('API error: ', (array) $answer );
            throw new Exception('API error: ' . $answer->message  );
        } else {
            return $answer;
        }
    }
}
}

How can different API responses be handled correctly? What should be written in Logs? Do I need to throw Exceptions? What should be returned to external code? (i.e. in getBrands())

What would you change in this code?

First of all, it's a job for at least three separate functions (that should
belong to the different classes at best, but that would be a different story), each throwing its own exceptions.

protected function sendRequest($data)
{
    $ch = curl_init();
    curl_setopt($ch, CURLOPT_URL, $this->apiUrl);
    curl_setopt($ch, CURLOPT_SSL_VERIFYPEER,FALSE);
    curl_setopt($ch, CURLOPT_SSL_VERIFYHOST,2);
    curl_setopt($ch, CURLOPT_POST, 1);
    curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
    curl_setopt($ch, CURLOPT_POSTFIELDS, $data);
    $data = curl_exec($ch);
    if ($error = curl_error($ch)) {
        throw new Exception($error);
    }
    curl_close($ch);
    return $data;
}
protected function jsonDecode($json)
{
    $data = json_decode($json);
    if (json_last_error()) {
        throw new Exception(json_last_error_msg());
    }
    return $data;
}
protected function requestApi($data)
{
    $data = $this->authApi + $data;
    $data = [
        'data' => json_encode($data)
    ];
    $response = $this->sendRequest($data);
    $answer = $this->jsonDecode($response);

    if (isset($answer->success) && $answer->success == true))) {
        return $answer;
    }
    if (isset($answer->message) {
        throw new \Exception('API error: ' . $answer->message  );
    }
    throw new \Exception("Unknown API response: $response" );
}

As of the logger, I would call it in the exception handler/catch block and thus avoid repetitive calls.


std::ostream wrapper for network logging device I want to be able to write logging data out to a network device with the same ease as using std::cout, so I implemented an ostream wrapper. The wrapper uses the C file IO functions to simulate writing to the device. I am looking for feedback on whether or not I have implemented the ostream API correctly.

device_ostream.hpp:

#ifndef DEVICE_OSTREAM_HPP__
#define DEVICE_OSTREAM_HPP__

#include <ostream>

#define BUF_SIZE 100

class device_buffer : public std::streambuf
{
public:
  device_buffer();
  ~device_buffer();
  int overflow(int ch = EOF);
  int sync();

private:
  char buffer_[BUF_SIZE];
  FILE* fp_;

};

class device_ostream : public std::ostream {
public:
  device_ostream() : std::ostream(new device_buffer) {}

  ~device_ostream() {
    delete rdbuf();
  }
};

#endif // DEVICE_OSTREAM_HPP__

device_ostream.cpp:

#include "device_ostream.hpp"

device_buffer::device_buffer() : fp_(0) {
  memset(buffer_, 0, sizeof(buffer_));
  setp(buffer_, buffer_+BUF_SIZE-1);
  fp_=fopen("test.bin","wb");  // simulate our device
}

device_buffer::~device_buffer() {
  if(fp_)
    fclose(fp_); // simulate disconnection from device
}

// ostream fwk notification that buffer is now full
// so write out data now
int device_buffer::overflow(int ch) {
  if(ch != EOF) {
    *pptr() = ch;
    pbump(1);
  }
  return sync();
}

// function to sync up with hardware device.  ie we actually write out data here
int device_buffer::sync() {
  if(fp_) {
    fwrite(buffer_, 1, size_t(pptr() - pbase()), fp_);
    // reset put ptr
    setp(pbase(), epptr());
    return 0;  // success
  }
  else
    return EOF; // return EOF on device failure
}

Code to exercise:

#include "device_ostream.hpp"

int main () {

  device_ostream os;
  os.write("WRITE", 5);

  os << "Hi there, how are you today???" << "\n";
  os << "Number: " << 33 << std::endl;  // std::endl causes sync to be called even if buffer not full yet
  os << "ABCDEFGHIJKLMNOPQRSTUVWXYZ" << std::endl;
  os << "abcdefghijklmnopqrstuvwxyz" << std::endl;
  os << "ABCDEFGHIJKLMNOPQRSTUVWXYZ" << std::endl;

  if(os)
    os << "abcdefghijklmnopqrstuvwxyz" << std::endl;

  return 0;
}

You should not do this:

class device_ostream : public std::ostream {
public:
  device_ostream() : std::ostream(new device_buffer) {}

  ~device_ostream() {
    delete rdbuf();
  }
};

You do not know if the user of the class has reset the buffer while you were not watching.

device_ostream   data;
....
MyImprovedStreamBuffer   myBuffer;
data.rdbuf(&myBuffer);               // Your destructor will not play well with that.

I would make it an internal member.

class device_ostream : public std::ostream
{
    device_buffer   buffer;
  public:
    device_ostream()
        : std::ostream(nullptr)
    {
        rdbuf(&buffer);
    }
};

You should validate the return code of C functions.

fwrite(buffer_, 1, size_t(pptr() - pbase()), fp_);

Does not guarantee that it will write everything you asked for. Personally I would use a lower level write function so I can accurately check the error state and compensate accordingly.

std::size_t written = 0;
std::size_t size    = pptr() - pbase();

while(written != size)
{
    std::size output = write(fp_, buffer_ + written, size - written);
    if (output == -1)
    {
        if (errno == EINTR || errno == EAGAIN)
        {
             continue; // Ignore the error and try again.
        }
        // Something bad happened.
        abort(1); // or set the state of your stream to bad in
                  // some way (I used abort() simply because I am
                  // to lazy to look up what to do. (you should).
    }
    written += output;
}

The value returned by overflow is incorrect.

According to the specs:

  In case of success, the character put is returned, converted to a value of type int_type using member traits_type::to_int_type.
  Otherwise, it returns the end-of-file value (EOF) either if called with this value as argument c or to signal a failure (some implementations may throw an exception instead).

On success you return 0.

The value returned by sync is incorrect.

According to the specs:

  Returns zero, which indicates success.
  A value of -1 would indicate failure.

On failure you return EOF. Yes I get that it is -1. But your making assumptions. Follow specs to the letter.

Things like this scare me:

  else
    return EOF; // return EOF on device failure

I have been bitten a couple of times were macros expand to more than a single statement. Which renders the block incorrect. As a result I always use '{}' for sub blocks. I recommend it for everything it will prevent those impossible to find errors.

Since we now live in 2014 (nearly 2015). You should be using the modern version of the compiler. So add override to functions that override virtual functions.

int overflow(int ch = EOF)  override;
int sync()                  override;

Definitely worth overriding the method streamsize xsputn (const char* s, streamsize n);