Memory Game in React This is my second game in React (I posted a country guessing game 2 days ago). Please review my code.

Preview

Alert: May work only in Google Chrome (I build here)

import React, { Component } from 'react';
import './MemoryGame.css';

class MemoryGame extends Component {
  constructor(props) {
    super(props);
    //Array of memory images
    this.ImagePieces = ['cat', 'cat', 'dog', 'dog', 'horse', 'horse',
    'pig', 'pig', 'snake', 'snake', 'fish', 'fish'];
    this.tempCheckArr = [];
    this.state = {
      showImg: Array(this.ImagePieces.length).fill('hidden'),
      divClick: true,
      compareImgArr: [],
      counter: 0
    }
    this.checkMatch = this.checkMatch.bind(this);
  }

  //Shuffle memory game images
  componentWillMount() {
    function shuffleArray(array) {
      for (let i = array.length - 1; i > 0; i--) {
          let j = Math.floor(Math.random() * (i + 1));
          [array[i], array[j]] = [array[j], array[i]];
      }
      return array;
    }
    shuffleArray(this.ImagePieces);
  }

  //Check for match function
  checkMatch(key, e) {
    //For later hidding images purposes
    this.tempCheckArr.push(key.toString());

    //Create copy of (compareImgArr) and add img src, for later compare
    const imgSrc = e.target.firstChild.src;
    const compareImgArr = [...this.state.compareImgArr];
    compareImgArr.push(imgSrc);

    //Set current clicked item as 'visible' in main array 'showImg'
    const arr = this.state.showImg
    arr[key] = 'visible';

    //Update state, counter for block user click method
    //after unhidding two pieces
    this.setState({
      showImg: arr,
      compareImgArr: compareImgArr,
      counter: this.state.counter + 1
    });

    //Check if 2 items are clicked - if yes - disable clicking
    if (this.state.counter % 2) {
      this.setState({
        divClick: false
      });
      //Check if pictures are matching
      if (compareImgArr[0] === compareImgArr[1]) {
        this.tempCheckArr = [];
        this.setState({
          compareImgArr: [],
          divClick: true
        });
      } else {
        //If pictures not match turn them back to hidden
        var tempArr = this.state.showImg
        // eslint-disable-next-line
        var firstElement = parseInt(this.tempCheckArr[0]);
        // eslint-disable-next-line
        var secondElement = parseInt(this.tempCheckArr[1]);
        setTimeout(()=>{
          tempArr[firstElement] = 'hidden';
          tempArr[secondElement] = 'hidden';
          this.tempCheckArr = [];
          this.setState({
            showImg: tempArr,
            compareImgArr: [],
            divClick: true
          })
        }, 1500)
      }
    }
  }

  render() {
    return(

Memory Game

          {this.ImagePieces.map((text, i) => {
            return (

 this.checkMatch(i, e) : undefined}>

            )
          })}

    )
  }
}

export default MemoryGame;

Ok, I had quite a few thoughts on this.

State

Your state seems more complex than it needs to be, and some of it is hiding outside of the state variable.
I think all you really need to keep track of are:
 - What order the images are in
 - Which images are currently selected
 - Which images have been guessed correctly

I don't really understand what tempCheckArr is, how it's different from compareImgArr, or why it's an instance variable.

Extract stuff

I think you have too much stuff crammed into one component.
I think you need to extract pieces of logic that can be self contained.

Prime example: shuffleArray

Why is shuffleArray declared inside componentWillMount?
This is creating a new shuffleArray function every time MemoryGame is mounted.
I think shuffleArray should at least be outside of this component, and really should be in a some sort of RandomUtil.js file.

Card component

I would also extract out a Card component that renders a card.
It should know:
 - What image to display
 - Whether or not it's selected
 - Whether or not it's been guessed correctly

This way you can keep your MemoryGame component about managing the state of the game while the Card component focuses on presentation.

Other Thoughts

Your  tag doesn't need a key prop because it's not the top level thing in an array.
Why is ImagePieces declared on this. It seems like it should be part of state.
(Also why is the I in ImagePieces uppercase? It seems like this should be imagePieces)
Why are you shuffling ImagePieces in componentWillMount and not just in the constructor?
It's kinda gross to reach into the DOM to compare the elements src attribute, and it makes it harder to understand what you actually want to compare: that the two images selected are of the same thing
Why are you converting integers to strings when you put them into tempCheckArr and converting them back when you take them out? Wouldn't it be easier to just put integers into the array?

How I would write it

const IMAGES = ["cat", "cat", "dog", "dog", "horse", "horse", "pig",
  "pig", "snake", "snake", "fish", "fish"];

class MemoryGame extends Component {
  constructor(props) {
    super(props);

    // You can simplify your state a lot
    this.state = {
      cards: shuffleArray(IMAGES.slice()),
      selected: [], // indexes which have been selected
      correct: [] // indexes which have been guessed correctly
    };
  }

  // Don't need a componentWillMount

  onCardClick(clickedIndex) {
    const { selected, cards, correct } = this.state;

    if (selected.length === 0) { // selecting a first card
      this.setState({ selected: [clickedIndex] })
    } else if (selected.length === 1) { // they're selecting a second card
      if (cards[selected[0]] === cards[clickedIndex]) {
        // It's a match :)
        // Add selected cards to `correct` and reset selection
        this.setState({
            correct: correct.concat([selected[0], clickedIndex]),
            selected: []
        });
      } else {
        // It's not a match :(
        // Select it for now, and reset selection in a bit
        this.setState({ selected: [selected[0], clickedIndex] });
        setTimeout(() => {
          this.setState({ selected: [] })
        }, 1500);
      }
    }
    // Otherwise they already have 2 selected and we don't wanna do anything
  }

  render() {
    const { correct, selected, cards } = this.state;
    return (

Memory Game

          {cards.map((image, i) => (
            <MemoryCard
              key={i}
              image={image}
              isCorrect={correct.includes(i)}
              isSelected={selected.includes(i)}
              onSelect={() => this.onCardClick(i)}
            />
          ))}

    );
  }
}

// Extracted into it's own component
const MemoryCard = ({ image, isSelected, isCorrect, onSelect }) => (

 {
      // You can only select a card that's not already correct and
      // isn't currently selected
      if (!isCorrect && !isSelected) {
        onSelect();
      }
    }}
  >

);

// Probably in a different file
function shuffleArray(array) {
  for (let i = array.length - 1; i > 0; i--) {
    let j = Math.floor(Math.random() * (i + 1));
    [array[i], array[j]] = [array[j], array[i]];
  }
  return array;
}

export default MemoryGame;


Macro for transforming 2 way matrix into 1 way matrix I have written some code that transforms 2 way matrices into 1 way matrices. Although it's comfortable to use, it's very sluggish and slow in execution. Does anyone have any idea how to improve the performance? I'm aware that the same transformation can be done with standard formulas, but solely for learning reasons I would like to stick with vba.

How to use it - select whole matrix and execute the Sub.

Sub TwoToOneMatrix()
Set Mrange = Selection
colsX = Mrange.Columns.Count
rowsX = Mrange.Rows.Count
Set objects = Range(Mrange.Columns(1).Rows(2), Mrange.Columns(1).Rows(rowsX))
Set titleX = Range(Mrange.Columns(1).Rows(1), Mrange.Columns(1).Rows(1))
Set Rng = Application.InputBox("Select a range", "Obtain Range Object", Type:=8)

'Column 1
Rng.Value = titleX.Value
def = 1
For j = 2 To objects.Rows.Count + 1
    curCountry = Mrange.Columns(1).Rows(j)
    For i = 1 To colsX - 1
        Rng.Offset(def, 0).Value = curCountry
        def = def + 1
    Next i
Next j

'Column 2
def = 1
Rng.Offset(0, 1).Value = "Date"
For j = 1 To objects.Rows.Count
    For i = 2 To colsX
        curDate = Mrange.Columns(i).Rows(1)
        Rng.Offset(def, 1).Value = curDate
        def = def + 1
    Next i
Next j

'Column 3
Rng.Offset(0, 2).Value = "Values"
def = 1
For r = 2 To rowsX
    For c = 2 To colsX
        Rng.Offset(def, 2).Value = Mrange.Columns(c).Rows(r)
        def = def + 1
    Next c
Next r

End Sub

Your description is not very clear, but I see the usual suspects.

Indenting

Always indent your code properly - this makes it easier to read, easier to pick out the logic loops and easier to maintain. In itself, it does not guarantee code correctness.

Option Explicit

Always put Option Explicit at the top of your modules.

Variable names

Use descriptive variable names. For what you tell us the code is intended to do, curDate and curCountry mean nothing. Neither does colsX, rowsX, def, Rng or Mrange.

Use arrays

Using arrays will improve your performance significantly. The reason for this improvement is that you stop switching between the VBA Engine and the Excel Engine (which comes at a cost) and you work purely in the one context. You also stop working with complex objects, which also incur an overhead.

The following is an example of how to adjust the code, but is not intended to be an accurate representation of your code.

Dim originalMatrix as Variant
originalMatrix = Selection.Value '<-- creates an array from the range values - only time to touch Excel
Dim title as String
title = CStr(originalMatrix(LBound(originalMatrix,1), LBound(originalMatrix,2))
Dim finalMatrix(,) As Variant '<-- or whatever type you want here
ReDim finalmatrix(UBound(originalMatrix,1) * UBound(originalMatrix,2),2)

' Adjust following loops to pick the right rows and columns.
For iterator1 = LBound(originalMatrix,1) To UBound(originalMatrix,1)
    For iterator2 = LBound(originalMatrix,2) To UBound(originalMatrix,2)
        finalMatrix(someCounter,1) = someValue1
        finalMatrix(someCounter,2) = someValue2
    Next iterator2
Next iterator1

'.... some code here to select output range and resize to match finalMatrix …
' … and then
outputRange.Value = finalMatrix '<--- again, a light touch against the Excel model

The use of LBound and UBound protect you against any confusion as to whether your array is 1-based or 0-based.You could store these in variables if you wanted the code to read a bit better.


Two functions that move array elements These two functions act on a stack, an array of integers (whose first element is the top of the stack, LIFO).

The first function retrieves the first element and puts it at the end. The other is the opposite.

I would like to combine these two functions into only one function, if possible. (I have a constraint, 25 lines maximum)

int     *reverse(int value, int size, int *stack)
{
    int     value;
    int     i;
    int     *previous;

    value = *stack;
    previous = stack;
    if (!(stack = (int*)malloc(sizeof(int) * size)))
        return (NULL);
    i = 1;
    while (i < size)
    {
        stack[i - 1] = previous[i];
        i++;
    }
    stack[i - 1] = value;
    free(previous);
    return (stack);
}

int     *sv_reverse(int value, int size, int *stack)
{
    int     value;
    int     i;
    int     *previous;

    value = stack[size - 1];
    previous = stack;
    if (!(stack = (int*)malloc(sizeof(int) * size)))
        return (NULL);
    *stack = value;
    i = 1;
    while (i < size)
    {
        stack[i] = previous[i + 1];
        i++;
    }
    free(previous);
    return (stack);
}

I would consider an approach were you think of the array as a circular buffer, with the top of the stack being position 0. Each of your operations becomes a rotation. One position clockwise and one anticlockwise.  Or even both clockwise (or anticlockwise), one a single position rotation and one a size-1 rotations.

If you can change the data structure so that the top of the stack can be any position in the circular buffer the these rotations become trivial, just set the top of value.


Function for inserting data into database I have this function to ease out the task of inserting data into databases.
I am not very sure if it is secure to use it this way.

Any suggestions on improving it?

function insert_into($query,$datatype) {

        //$i=0; num=1; $i=1; num=2; for(i=1;)
        $num=func_num_args()-1;
        global $CONN;//set the global connection variable on.
        for($i=2;$i<=$num;$i++) //assign the 'n' variables to an array
        {
            $values[$i]=mysqli_real_escape_string($CONN,func_get_arg($i));

            if(empty($values[$i])|| !isset($values[$i])|| $values[$i]=="")
            {
                die("
Fatal Error: One or more parameters have an uninitialized or empty variable. You can't use an empty variable as a parameter.
");
            }
        }

        $blanks=str_repeat("?,", count($values)); //get the parameterized values as "?".
        $blanks=trim($blanks,","); //remove the extra ","

         $sql="INSERT INTO  ".$query." VALUES (".$blanks.");"; //make the query

         //set the connection

        $stmt= $CONN->prepare($sql); //prepare the query

        $bind=array();
        foreach ($values as $key => $val) {

            $bind[$key]=&$values[$key];
        }

        array_unshift($bind,$datatype);
        call_user_func_array(array($stmt,'bind_param'),$bind); // bind the variables

        #Replaced with above line ^ $stmt->bind_param($datatype, $xid);

        if($stmt->execute())    // set parameters and execute
    {
            $stmt->close();
        return 1; //sucessful

        } else {
            echo mysqli_error($CONN);
            $stmt->close();
            return 0;
        }
            $stmt->close();
            //close anyway

    }

     //SAMPLE: set_conn(); insert_into("tablename (stuff1, stuff2, stuff3)","sss",$val1,$val2,$val3);
    // set_conn() is another function which sets the connection to the database.
    //The above function returns 1 on success.

Does this have some serious issues like SQL injection or any other vulnerabilities? Thanks!

My suggestion is to simply abandon this piece of code.  I don't know what value that it is adding.  Trying to create some super flexible function that can query any table in any manner is just adding complexity to your application.  How is making this function call easier than just working directly with mysqli in your calling code?

The sending of partial queries and concatenating them to the rest of the query inside this function just serves to obfuscate your code.  Your query is now written in two places rather than one!

Additionally, you have to do things like parse apart the string passed as parameter to figure out how to formulate the query that is to be made. That is added complexity (and potentially fragility) that is just not needed.

I will also offer some thoughts around general coding style that you should considered applying whether or not you use this code.

Avoid using global to make a variable available in function scope.  You are much better off passing the dependency to the code that needs it as a parameter.  This allows you to do things like type hinting the parameters to validate you have the proper dependency before working with it (like a valid mysqli object for example).
Make sure you do not just consider happy path.  For example when working with your DB object, what happens if prepare() fails?  How does your code recover?
You have too much vertical white-space (i.e. empty lines) in your code where there isn't really a need for it.
Some of your lines of code are too long, making your code harder to read.  Consider trying to limit line length to ~80 characters.
Your code indentation (especially around brackets) is inconsistent.  Do this in a consistent fashion so it is clear at a glance where code is nested.
Don't echo out errors to standard output.  Log them!
Don't have unreachable lines of code like you have with your last $stmt->close() call.  Your if-else covers all cases here, so this is totally unnecessary.
You should not use mysqli_real_escape_string() in combination with parametrized prepared statements.  You are going to potentially munge your data by doing this.
A function should only do one thing.  Don't have a function both change data in a database as well as provide HTML-formatted display for error conditions like you have here.  In this case if you have "fatal error", don't die in this function.  Have the function simply report the failure to the calling code (ideally via throwing an Exception) and let the calling code figure out what to do about the error.


Monty Hall implementation To get a feel of Monty Hall problem, I've implemented Monty Hall simulator in Python, using only two strategies: never switch the door or always switch the door. Please review it with an eye towards any problem with the code you can think of, things like better design, flaws in code, etc.

#!/usr/bin/env python3

import random
import sys

def available_doors():
    return [1, 2, 3]

class MontyHallBox(object):
    def __init__(self):
        self.__doors = {1: None, 2: None, 3: None}
        self.__init_doors()

    def reinitialize(self):
        self.__init_doors()

    def __init_doors(self):
        self.__doors = {n: 'goat' for n in range(1, 4)}
        car = random.choice(available_doors())
        self.__doors[car] = 'car'

    def _get_rand_doornum_item(self):
        door_num = random.choice(available_doors())
        item = self.__doors[door_num]
        return door_num, item

    def reveal_noncar(self, initial_door):
        door_num, item = self._get_rand_doornum_item()
        while item == 'car' or door_num == initial_door:
            door_num, item = self._get_rand_doornum_item()
        return door_num

    def reveal_selected(self, door):
        return self.__doors[door]

def run_sim_always_switch(mhb):
    mhb.reinitialize()
    initial_door = random.choice(available_doors())
    noncar_door = mhb.reveal_noncar(initial_door)
    switch_door_set = set(available_doors()) - set([initial_door, noncar_door])
    switch_door = list(switch_door_set)[0]
    item = mhb.reveal_selected(switch_door)
    if item == 'car':
        return 'success'
    return 'fail'

def run_sim_never_switch(mhb):
    mhb.reinitialize()
    initial_door = random.choice(available_doors())
    # this is pointless really as this info is not used by game participant
    _ = mhb.reveal_noncar(initial_door)
    item = mhb.reveal_selected(initial_door)
    if item == 'car':
        return 'success'
    return 'fail'

def run_simn(num, simn_fun):
    mhb = MontyHallBox()
    results = []
    for i in range(num):
        results.append(simn_fun(mhb))
    successes = len(list(filter(lambda x: x == 'success', results)))
    return successes / num

def main():
    num = 10000
    if num < 1:
        print('Simulation has to be ran at least 1 time')
        sys.exit(1)
    print('Always switch, run simulation {} times.'.format(num))
    print('Success ratio: {}'.format(run_simn(num, run_sim_always_switch)))
    print()
    print('Never switch, run simulation {} times.'.format(num))
    print('Success ratio: {}'.format(run_simn(num, run_sim_never_switch)))

if __name__ == '__main__':
    main()

Available doors

Why is this a stand-alone function?  Why are door identifiers hard-coded here?  The MontyHallBox knows what the doors are, and what they are called:

class MontyHallBox(object):
    def available_doors(self):
        return list(self.__doors.keys())

Class Syntax

class MontyHallBox(object):
    ...

is obsolete syntax.  Use:

class MontyHallBox:
    ...

Class Private Names

Why use a double underscore for self.__doors and self.__init_doors()?  This is a Python feature for avoiding name collision in derived classes, and "mangles" the names is a predictable fashion (adding the classname as a prefix):

>>> mhb = MontyHallBox()
>>> mhb._MontyHallBox__doors
{1: 'car', 2: 'goat', 3: 'goat'}
>>>

It does not provide any extra security of the member.

Use set notation

Instead of set([initial_door, noncar_door]), write {initial_door, noncar_door}.

Throw-away variables

The variable i is never used:

    for i in range(num):
        results.append(simn_fun(mhb))

The PEP-8 recommendation is to use _ for these throw-away variables:

    for _ in range(num):
        results.append(simn_fun(mhb))

List comprehension

Building up a list of results should be done using list comprehension, instead of initialization & repeated .append operations.  Instead of:

    results = []
    for i in range(num):
        results.append(simn_fun(mhb))

Use:

    results = [simn_fun(mhb) for _ in range(num)]

Counting

There is no need to build up a long list of results, just to count the result distributions.  Pass the results as they are generated to a counter.

>>> import collections
>>> num = 10000
>>> simn_fun = run_sim_always_switch
>>> counts = collections.Counter(simn_fun(mhb) for _ in range(num))
>>> counts['success']
6717
>>> counts['fail']
3283

or, knowing True is 1 and False is zero, count the successes as you generate them yourself:

>>> success = sum(simn_fun(mhb) == 'success' for _ in range(num))
>>> success
6614

Formatting

Use f-strings to put the variables/expressions directly in the format statements:

    print(f'Always switch, run simulation {num} times.')
    print(f'Success ratio: {run_simn(num, run_sim_always_switch)}')
    print(f'Never switch, run simulation {num} times.')
    print(f'Success ratio: {run_simn(num, run_sim_never_switch)}')

Round results to the desired significant figures with appropriate format codes:

    print(f'Success ratio: {run_simn(num, run_sim_never_switch):.3f}')

Named Constants

If you accidentally wrote the tests as if item == 'Car':, you'd find that nobody wins, and would have to hunt down where in the code you made spelling/capitalization errors.  The IDE won't help you; it can't auto complete strings for you.  If you used named constants ...

CAR = 'car'
GOAT = 'goat'

... and you wrote if item == Car:, instead of nobody winning, the program would crash with an unknown identifier and identity the line where the problem exists, which is much better than having to search through all of the code looking for where logic is going wrong.  But the above named "constants" aren't actually constants; they are variables we hope never get accidentally (or maliciously) changed.

from enum import Enum

Prize = Enum('Prize', 'CAR, GOAT')

Now we have Prize.CAR and Prize.GOAT, which are constants.

Malicious Strategies

A friend can say they've come up with a great strategy - and let you test their code:

import friend

run_simn(10_000, friend.strategy)

And lo and behold, they always win the car!  How is this possible?

Friend strategy 1:

def strategy(mhb):
    initial_door = next(door for door, prize in mhb._MontyHallBox__doors.items()
                        if prize == 'car')
    _ = mhb.reveal_noncar(initial_door)
    item = mhb.reveal_selected(initial_door)
    return 'success' if item == 'car' else 'fail'

Friend strategy 2:

def strategy(mhb):
    initial_door = 1
    goats = { mhb.reveal_noncar(initial_door) for _ in range(10) }

    if len(goats) == 2:
        selected_door = initial_door
    else:
        selected_door = 2 if goats == { 3 } else 3
    item = mhb.reveal_selected(initial_door)

    return 'success' if item == 'car' else 'fail'

Friend strategy 3:

def strategy(mhb):
    return 'success'

They are cheating.  Some friend.

A safer (harder to cheat) simulation approach

Let's start by creating some type aliases, to make clearer code.

from typing import Callable, Counter, List, NewType, Tuple

# Doors are numbered
Door = NewType('Door', int)
Doors = List[Door]

This Door is just a fancy way of saying the int type-hint, and Doors is a fancy way of saying the List[int] type-hint.

Now let's create two methods which the let's the user play the Monty Hall game:

def first_pick(doors: Doors) -> Door:
    """
    Ask the player to pick a door
    """

    return int(input(f"Pick a door [{', '.join(map(str,doors))}]: "))

def second_pick(doors: Doors, selected: int, goats: Doors) -> Door:
    """
    Show some goats behind doors the player didn't pick.
    Ask them again to pick a door.
    """

    print(f"Monty shows you a goat behind door #{', '.join(map(str, goats))}.")
    choice = input(f"Pick a new door [default: {selected}]: ")
    return int(choice) if choice else selected

We pass all the information the player needs to make their choices.  In the first case, just the list of available Doors.  In the second, we add what their original Door selection was, and the Doors Monty Hall revealed goats behind.  No extra MontyHallBox object, or self parameter that the user can peek into to cheat and get extra information.

We can even add types for these two functions up where we are defining the types we're using:

FirstPick = Callable[[Doors], Door]
SecondPick = Callable[[Doors, Door, Doors], Door]

Now, let's build the actual game:

def monty_hall(first: FirstPick = first_pick, second: SecondPick = second_pick, *,
               cars: int = 1, goats: int = 2, reveal: int = 1) -> Tuple[Door, Prize]:
    """
    Play a Monty Hall type game.

    The player picks one of several doors, behind each is either a Car or a Goat.
    Monty Hall shows the player behind one of the doors they didn't pick,
    "Oh look, behind this door is a Goat!  Now, do you want to change your mind?"
    The player now gets alter their selection, but does it pay to change their mind?

    Parameters:
        first: given the doors, pick a door
        second: given the doors, the first pick, and what was revealed, pick a door
        cars: number of cars (default 1)
        goats: number of goats (default 2)
        reveal: number of hidden goats to reveal (default 1)

    Returns:
        The door they choose, and the Prize behind it (a Goat or a Car)
    """

    # Sanity checks
    if cars < 1 or goats < 1:
        raise ValueError("Rigged game!  There must be both cars & goats!")
    if reveal >= goats:
        raise ValueError("Monty is not allowed to reveal all of the goats!")

    # Set up the game
    prizes = [Prize.CAR] * cars + [Prize.GOAT] * goats
    random.shuffle(prizes)
    doors = list(range(1, len(prizes) + 1))
    game = { door: prize for door, prize in zip(doors, prizes) }

    # Ask player to make their initial selection...
    selected = first(doors)

    # Monty reveals some doors
    goats = [ door for door, prize in game.items()
              if prize == Prize.GOAT and door != selected ]
    random.shuffle(goats)
    goats = goats[:reveal]

    # Based on original choice and revealed doors, ask player makes new choice...
    selected = second(doors, selected, goats)

    # ... and give the player their prize
    return selected, game[selected]

Ok, I've beefed up the game a little bit.  We've got a configurable number of cars (default 1), goats (default 2), and doors to reveal (default 1).  We've got first and second functions which default to the first_pick and second_pick user player functions above.  We've got a long doc-string describing the function.  Then we've got some validation on number of cars, goats, and reveals.

Then we have the meat-and-potatoes of the function.  Create the prizes, mix them up, and hide them behind doors.  Call the first method to get the initial door selection.  Then Monty Hall picks some doors with goats to reveal.  The second the second function is called to get the final door selection.  Finally, the resulting prize is returned.

Want to play the game?

def human_verses_monty(**kwargs):
    door, prize = monty_hall(**kwargs)
    print(f"Behind door #{door}, you find a {prize.name}!")

Use human_verses_monty().  Or maybe human_verses_monty(cars=3, goats=3, reveal=2)

I'm using **kwargs for brevity, so I can pass additional keywords through to the underlying monty_hall(...) method.  It would be clearer to list all of the keyword arguments out, but this post is already getting pretty long, and I'm not done yet.

Playing multiple times

Now that we have our safe simulation, let's make it so we can play the game multiple times, and collect the results:

def monty_halls(strategy: SecondPick, games, *,
                initial: FirstPick = random.choice, **kwargs) -> Counter[Prize]:
    return Counter(monty_hall(initial, strategy, **kwargs)[1] for _ in range(games))

The initial door pick defaults to random.choice.  We need to pass in a strategy for the second pick.  The game is played the given number of times, with Door, Prize being returned, from which we select only the Prize with [1], and count the number of times we get each prize in a Counter.

Now we need some strategies:

def always_switch(doors: Doors, selected: Door, goats: Doors) -> Door:
    return random.choice(list(set(doors) - set(goats) - {selected}))

def never_switch(doors: Doors, selected: int, goats: Doors) -> Door:
    return selected

Again, no extra information to allow them to cheat.  And they don't get to break the rules and play incorrectly (asking Monty Hall multiple times to reveal doors) to cheat.

How about function to evaluate various strategies:

def evaluate(strategies: List[SecondPick], games, **kwargs):
    for strategy in strategies:
        prizes = monty_halls(strategy, games, **kwargs)
        wins = prizes[Prize.CAR]
        losses = prizes[Prize.GOAT]
        total = wins + losses
        print(f"{strategy.__name__}: wins {wins / total * 100:.2f} %")
    print()

Let's see it in action:

if __name__ == '__main__':
    print("Standard game (1 car, 2 goats)")
    evaluate([always_switch, never_switch], 10_000)

    print("Alternate game (2 car, 3 goats, 2 reveals)")
    evaluate([always_switch, never_switch], 10_000, cars=2, goats=3, reveal=2)

Running this gives:

Standard game (1 car, 2 goats)
always_switch: wins 66.88 %
never_switch: wins 33.12 %

Alternate game (2 car, 3 goats, 2 reveals)
always_switch: wins 80.14 %
never_switch: wins 39.91 %

Cheaters will be cheaters

It is still possible to cheat.  One method would be to capture the stack frame, and look at the local variables in the calling frame.  We can get around this by using a separate thread or process to execute the strategy calls in, so the caller can't simply walk up the stack looking for gold.  This still isn't fool proof, 'cause the cheater could look for the parent thread, or parent process and try to inspect those.

At least we aren't making it easy to cheat, like the original MontyHallBox approach did.


Simple Data Access Layer I have DatabaseContext class.

public interface IDatabaseContext : IDisposable
{
    SqlConnection Connection { get; }
}

public class DatabaseContext : IDatabaseContext
{
    private readonly string _connectionString;
    private SqlConnection _connection;

    public DatabaseContext(string connectionString)
    {
        _connectionString = connectionString;
    }

    public SqlConnection Connection
    {
        get
        {
            if (_connection == null)
                _connection = new SqlConnection(_connectionString);

            if (_connection.State != ConnectionState.Open)
                _connection.Open();

            return _connection;
        }
    }

    public void Dispose()
    {
        if (_connection != null && _connection.State == ConnectionState.Open)
            _connection.Close();
    }
}

And I use it in PageService class using dependency injection, for example:

public class PageService
    {
        private readonly IDatabaseContext _databaseContext;

        public PageService(IDatabaseContext databaseContext)
        {
            _databaseContext = databaseContext;
        }

        public IEnumerable<CustomPage> GetAllPages()
        {
            IEnumerable<CustomPage> pages;
            using (var db = _databaseContext)
            {
                //using Dapper.Net to get list of pages.
                pages = db.Connection.GetList<CustomPage>();
            }
            return pages;
        }
    }

Do you see any improvement / issue of my DatabaseContext class?

I've never seen GetList() method on SqlConnection so I'm assuming this is your own class?

If that were the case would it not be best to have IDataContext abstract the Connection details away and return an IConnection contract.

public interface IDatabaseContext : IDisposable
{
    IConnection Connection { get; }
}

Doing this you can ensure your calling classes are completely decoupled from the implementation details of a connection.  Otherwise any implementation of IDataContext would have been coupled to a SQLConnection and you lose the benefit of the initial abstraction.


Implementing a generic Comparator I'm familiar with JS, however just today started TypeScript. Experimenting with the type system and generics, I thought to implement a comparator yielding 3 distinct results and created the following:

export function compare<T>(s1: T, s2: T): number {
    let equal = 0;
    let greater = 1;
    let lesser = -1

    // Special cases: consider "undefined" and "null" to be less than defined
    if (nullOrUndefined(s1) && nullOrUndefined(s2)) {
        return equal;
    } else if (nullOrUndefined(s1) ^ nullOrUndefined(s2)) {
        return nullOrUndefined(s2) ? greater : lesser;
    }

    // Ignore case for strings
    if (typeof s1 === 'string') {
        s1 = s1.toLowerCase();
        s2 = s2.toLowerCase();
    }

    // Use JS default
    return s1 === s2 ? equal : s1 > s2 ? greater : lesser;
}

export function nullOrUndefined(input: any): boolean {
    return input === undefined || input === null;
}

My questions are:

Are the variables necessary? I keep going back and forth on this / wonder if it aids clarity in any way. Sometimes feel like it's intuitive, but I even wonder if an enumeration representing comparison results outside could be superior.
My first implementation used the any type, but it feels more appropriate to apply it this way since it should always compare like-items. This feels like a sensical assumption, but would any prove more handy and 'generic' in any case?
For any unfamiliar with JS weirdness, see this article.

Personally, because this function can be simplified, I don't believe the variables are necessary. Since you are using Typescript, an enum would fit well here, but I believe even this is unnecessary. The results of a compare function are well standardized.

enum Result {
  equal = 0,
  greater = 1,
  lesser = -1
}

You are correct that it is more appropriate to apply the function with generics. If somewhere you need the flexibility that any provides, you can simply call the function with compare<any>(a, b) to override the type. I personally have found that any is only useful when first converting a project and will avoid it like the plague if at all possible.

Now, a few comments about the function itself.

I'm not sure how you are compiling this, but it emits three errors in the playground.

nullOrUndefined(s1) ^ nullOrUndefined(s2) - The '^' operator is not allowed for boolean types. Consider using '!==' instead.
s1 = s1.toLowerCase();- Type 'string' is not assignable to type 'T'.
s2 = s2.toLowerCase(); - Property 'toLowerCase' does not exist on type 'T'.

You will have problems if you try to use this function with the built in Array.prototype.sort function because undefined results in the array being treated as a sparse array. See the spec.
To ignore string case, it would be better to use String.prototype.localeCompare.

Here is how I would implement this function. I purposely use == instead of === for comparing null as it also catches undefined.

function compare<T>(a: T, b: T): number {
    if (a == b) return 0;
    if (a == null) return -1;
    if (b == null) return 1;

    // To appease the type system, check both
    // I think it might be a bug that this is required
    if (typeof a === 'string' && typeof b === 'string') {
        return a.localeCompare(b, 'standard', { sensitivity: 'case'});
    }

    return b > a ? -1 : 1;
}


Scraper for parsing email-ID using email sending button I've written a script in python with POST request which is able to send email using send button and then catch the response of that email and finally parse the ID from that. I don't know whether the way I did this is the ideal one but it does parse the email ID from this process. There are 4 email sending buttons available in that webpage and my script is able to scrape them all. Here is what I've tried so far with:

import requests
from lxml import html

main_url = "https://www.yell.com/ucs/UcsSearchAction.do?keywords=pizza&location=London&scrambleSeed=2082758402"

def main_page(url):
    response = requests.get(url, headers = {'User-Agent':'Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.109 Safari/537.36'}).text
    tree = html.fromstring(response)
    for titles in tree.xpath("//div[@class='businessCapsule--callToAction']"):
        title = titles.xpath('.//a[contains(@class,"btn-blue")]/@href')[0] if len(titles.xpath('.//a[contains(@class,"btn-blue")]/@href'))>0 else ""
        process_page(title.replace("/customerneeds/sendenquiry/sendtoone/","").replace("?searchedLocation=London",""))

def process_page(number):
    link = "https://www.yell.com/customerneeds/sendenquiry/sendtoone/"
    payload = {'message':'Testing whether this email really works.','senderPostcode':'GL51 0EX','enquiryTimeframe':'withinOneMonth','senderFirstName':'mth','senderLastName':'iqbal','senderEmail':'habibrahman@gmail.com','senderEmailConfirm':'habibrahman@gmail.com','uniqueAdId':number,'channel':'desktop','ccSender':'true','marketing':'on'}
    headers = {'User-Agent':'Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.109 Safari/537.36'}
    response = requests.post(link, data = payload, headers = headers)
    items = response.json()
    item = items['emailToCustomerUUID']
    print(item)

main_page(main_url)

These are the four email IDs I've parsed from that webpage:

1. 468145be-0ac3-4ff0-adf5-aceed10b7e5e
2. f750fa18-72c9-44d1-afaa-475778dd8b47
3. d104eb93-1f35-4bdc-ad67-47b13ea67e55
4. 90d2db3b-6144-4266-a86c-aa39b4f99d9a

Post Script: I've used my fake email address and put it in this script so that you can test the result without bringing any change to it.

Here are the few improvements I would apply:

match the "Email" links directly via //div[@class='businessCapsule--callToAction']//a[. = 'Email']/@href XPath expression - note how I'm matching the link by Email text
use urlparse to get the "id" number and extract that logic into a separate function
make a "scraper" class to share the same web-scraping session and persist things like headers to avoid repeating them every time you make a request
a better name for process_page would probably be send_email

Improved Code:

from urllib.parse import urlparse

import requests
from lxml import html

def extract_id(link):
    """Extracts the search result id number from a search result link."""
    return urlparse(link).path.split("/")[-1]

class YellScraper:
    MAIN_URL = "https://www.yell.com/ucs/UcsSearchAction.do?keywords=pizza&location=London&scrambleSeed=2082758402"
    EMAIL_SEND_URL = "https://www.yell.com/customerneeds/sendenquiry/sendtoone/"

    def __init__(self):
        self.session = requests.Session()
        self.session.headers = {'User-Agent': 'Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.109 Safari/537.36'}

    def scrape(self):
        response = self.session.get(self.MAIN_URL).text
        tree = html.fromstring(response)
        for email_link in tree.xpath("//div[@class='businessCapsule--callToAction']//a[. = 'Email']/@href"):
            self.send_email(extract_id(email_link))

    def send_email(self, search_result_id):
        payload = {'message': 'Testing whether this email really works.', 'senderPostcode': 'GL51 0EX',
                   'enquiryTimeframe': 'withinOneMonth', 'senderFirstName': 'mth', 'senderLastName': 'iqbal',
                   'senderEmail': 'habibrahman@gmail.com', 'senderEmailConfirm': 'habibrahman@gmail.com',
                   'uniqueAdId': search_result_id, 'channel': 'desktop', 'ccSender': 'true', 'marketing': 'on'}

        response = self.session.post(self.EMAIL_SEND_URL, data=payload)
        items = response.json()
        item = items['emailToCustomerUUID']
        print(item)

if __name__ == '__main__':
    scraper = YellScraper()
    scraper.scrape()


Custom sorting of dates I have implemented a class which implements a list of my custom DateObj. I have sorted the list in a peculiar manner based on the current month. I achieved my desired results, but I'm required to call Collections.sort() four times to achieve it. I feel this could be improved. Any thoughts?

//Custom Date Object
public class DateObj {

private int year;
private int month;
private int date;

DateObj(int year, int month ,int date){
    this.year = year;
    this.month = month;
    this.date = date;
}

public Integer getYear() {return year;}
public Integer getMonth() {return month;}
public Integer getDate() {return date;}
}

The custom sort class:

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

public class CustomListSort {
 static List<DateObj> list = new ArrayList<>();

public static void main(String a[]){

createRandomDateObjs();

Integer CURRENTMONTH = 6;

//Sort Months Descending
Collections.sort(list,(arg0,arg1)-> arg1.getMonth().compareTo(arg0.getMonth()));

//Sort by keeping the object with nearest largest next month first
Collections.sort(list,(arg0,arg1)->CURRENTMONTH.compareTo(arg0.getMonth()));

//Sort the months which were pushed back in an ascending order.
Collections.sort(list,(arg0,arg1)->(arg0.getMonth()<=CURRENTMONTH && arg1.getMonth()<=CURRENTMONTH)? arg0.getMonth().compareTo(arg1.getMonth()):0);

//If months same sort them in ascending manner based on their dates
Collections.sort(list, (arg0,arg1)->(arg0.getMonth() == arg1.getMonth())  ? arg0.getDate().compareTo(arg1.getDate()) : 0);

System.out.println("\nDESIRED OUTPUT");
System.out.println("\nMM - DD - YYYY \n");

for(int i = 0 ; i<list.size();i++)
    System.out.println(list.get(i).getMonth() +" - "+list.get(i).getDate()+" - "+list.get(i).getYear());
}

static void createRandomDateObjs(){
    for(int i = 0 ; i<20 ; i++){
        int y = 1980;
            y+=(int)(Math.random()*55);
        int m = (int) (Math.random()*11);
        int d = (int) (Math.random()*30);
        list.add(new DateObj(++y,++m,++d)); //Pre-increment to change 0's to 1's
}
}

Initially I had implemented it using Comparators but the Lambda made the code much cleaner.

Desired output:

MM - DD - YYYY

7 - 20 - 2023
7 - 18 - 2027
8 - 30 - 2010
8 - 12 - 2020
9 - 21 - 2006
10 - 23 - 2008
11 - 19 - 2000
11 - 16 - 2033
11 - 13 - 1989
12 - 13 - 2019
1 - 10 - 1985
1 - 15 - 2016
4 - 10 - 2021
4 - 13 - 2022
4 - 14 - 2004
5 - 1 - 2025
5 - 4 - 2014
5 - 20 - 2023
5 - 21 - 2022
6 - 29 - 1990

A Comparator is the correct approach, you just need to mod the month by the current month before comparing them. The easiest way to do this is to introduce a static variable currentMonth to hold the current month for all instances of DateObj.

Add DateObj.setCurrentMonth(CURRENTMONTH); to your Main, and use the code below:

//Custom Date Object
class DateObj implements Comparable<DateObj>{

    private int year;
    private int month;
    private int date;
    private static int currentMonth;

    DateObj(int year, int month ,int date){
      this.year = year;
      this.month = month;
      this.date = date;
    }

    public Integer getYear() {return year;}
    public Integer getMonth() {return month;}
    public Integer getDate() {return date;}
    public Integer getCurrentMonth() { return DateObj.currentMonth;}
    public static void setCurrentMonth(int currentMonth){
        DateObj.currentMonth = currentMonth;
    }

    @Override
    public int compareTo(DateObj o) {
        int months = (month+(12-currentMonth))%12 - (o.month+(12-currentMonth))%12;
        if(months == 0)
            return this.date-o.date;
        else{
            return months;
        }
    }
}

EDIT: An alternative approach is to simply define the Comparator in your main body. This avoids the use a static value to hold currentMonth:

    Collections.sort(list, (o1,o2)->{
            int months = (o1.getMonth()+(12-CURRENTMONTH))%12 - (o2.getMonth()+(12-CURRENTMONTH))%12;
            if(months == 0)
                return o1.getDate()-o2.getDate();
            else{
                return months;
            }
    });

Thanks to Mr.Wiggles for the recommendation.


Two and a sub string Challenge:

  Given two strings, A and B. Find if there is a substring that appears
  in both A and B.

Specifications:

  Several test cases will be given to you in a single file. The first
  line of the input will contain a single integer T, the number of test
  cases.

  Then there will be T descriptions of the test cases. Each description
  contains two lines. The first line contains the string A and the
  second line contains the string B.

  For each test case, display YES (in a newline), if there is a common substring.
  Otherwise, display NO.

Implementation:

import java.util.Scanner;
import java.util.Set;
import java.util.HashSet;

public class TwoStrings {
    public static void main(String[] args) {
        Scanner input = new Scanner(System.in);

        for (int i = Integer.parseInt(input.nextLine()); i> 0; i--) {
            String sub1 = toDiscreteString(input.nextLine());
            String sub2 = toDiscreteString(input.nextLine());
            boolean success;
            if (sub1.length() < sub2.length()) {
                success = hasCommonSubString(sub1, sub2);
            } else {
                success = hasCommonSubString(sub2, sub1);
            }
            System.out.println(success ? "YES" : "NO");
        }
    }

    private static boolean hasCommonSubString(String shorter, String longer) {
        for (int i = 0; i < shorter.length(); i++) {
            if (longer.indexOf(shorter.charAt(i)) > -1) {
                return true;
            }
        }
        return false;
    }

    private static String toDiscreteString(String input) {
        Set<Character> set = new HashSet<>();
        StringBuilder result = new StringBuilder();
        for (int i = 0; i < input.length(); i++) {
            if (set.add(input.charAt(i))) {
                result.append(input.charAt(i));
            }
        }
        return result.toString();
    }
}

This is a challenge found on HackerRank and passes all test cases. Is there a more efficient method? Is this overly-convoluted? Java 8 is available.

I am guessing indexOf is O(n). A faster way may be to add all the characters of a string to a set. Then call contains for each character of the second string (I would imagine that contains is on average O(1) for a hash set or O(lg(n)) for a tree set.)

private static boolean hasCommonSubString(String shorter, String longer)
{

    Set<Integer> set = shorter.chars()
                              .boxed()
                              .collect(Collectors.toSet());

    return longer.chars().anyMatch(set::contains);
}


Basic reverse function I am just starting out in my Clojure journey and I wonder if anybody can point out my beginners mistakes in my function below that simply reverses a list.  I know that there is already a reverse function so this is purely for learning.

(defn rev
  ([l]
   (if (= (count l) 1) l (rev '() l)))
  ([l orig]
   (if (= (count orig) 0)
     l
     (rev (conj l (first orig)) (rest orig)))))

In my defence, it does work, but what I am finding myself doing a lot in Clojure is overloading the arguments list to take into account when I need a working list like in this example where I conj new items onto.

Sometimes using arity overloading like you've done is the best way to write a function.

I tried to rewrite your function using destructuring, like this:

(defn rev [[x & more]]
  (if more
    ...

however, it doesn't really work because you need some way of storing your "result sequence" in progress, which in your function is l. Because we're trying to write a reverse function, it's hard to do what we want without having some way of "building up" the sequence as we go and storing it in an intermediate value.

Taking a look at how reverse is actually implemented in Clojure, we can see that a more concise way to express this idea of iterating over a sequence and building up a result sequence is by using reduce with a starting value of '():

(defn reverse [coll]
  (reduce conj '() coll))

The magic behind this is that we're starting with a seq, '(). When you conj something onto a seq, it adds it to the beginning, not the end. So what this does is it starts with an empty seq, then goes through the input list items one by one and adds them to the beginning of the result list, so that you end up with a list that's reversed.

EDIT: Leonid has a more thorough answer here.


Regularised regression code I wrote some code to do regularised linear regression and it works, but I don't like the fact that I've had to double call the functions when plotting, nor the fact that I've sliced those calls to get the parts that I want.

import numpy as np
import matplotlib.pyplot as plt

def fit(phi_fn, xx, yy):
    w_fit = np.linalg.lstsq(phi_fn(xx), yy, rcond=None)[0]
    grid_size = 0.01
    x_grid = np.arange(0,9,grid_size)[:,None]
    f_grid = np.matmul(phi_fn(x_grid),w_fit)
    return(x_grid, f_grid)

def fitreg(phi_fn, xx, yy,lamb):
    yy = np.pad(yy,(0,8),'constant',constant_values=0)
    zz = np.concatenate((phi_fn(xx),lamb*np.identity(8)),axis=0)
    w_fit = np.linalg.lstsq(zz, yy, rcond=None)[0]
    grid_size = 0.01
    x_grid = np.arange(0,9,grid_size)[:,None]
    f_grid = np.matmul(phi_fn(x_grid),w_fit)
    return(x_grid, f_grid)

def phi_poly(xx):
        return np.concatenate([np.ones((xx.shape[0],1)), xx,xx**2,xx**3,xx**4,xx**5,xx**6,xx**7], axis=1)

D = 1
N = 10
mu = np.array([0,1,2,3,4,5,6,7,8,9])

xx = np.tile(mu[:,None], (1, D)) + 0.01*np.random.randn(N, D)
yy = 2*xx + 2*np.random.randn(N,D)

plt.clf()
plt.plot(xx,yy,'kx')

plt.plot(fit(phi_poly, xx, yy)[0], fit(phi_poly, xx, yy)[1], 'b-')
plt.plot(fitreg(phi_poly, xx, yy,1)[0], fitreg(phi_poly, xx, yy,1)[1][:,0], 'r-')
plt.plot(fitreg(phi_poly, xx, yy,10)[0], fitreg(phi_poly, xx, yy,10)[1][:,0], 'g-')
plt.plot(fitreg(phi_poly, xx, yy,0.1)[0], fitreg(phi_poly, xx, yy,0.1)[1][:,0], 'y-')

plt.show()

You can get rid of some of the repetition using a for loop looping over the lambdas and colors and using tuple assignment to at least get rid of one level of having to use indexing as well as the necessity of having to call fitreg twice for each plot:

if __name__ == "__main__":

    D = 1
    N = 10
    mu = np.arange(10)

    xx = mu[:, None] + 0.01 * np.random.randn(N, D)
    yy = 2 * xx + 2 * np.random.randn(N, D)

    plt.clf()
    plt.plot(xx, yy, 'kx')

    x, y = fit(phi_poly, xx, yy)
    plt.plot(x, y, 'b-')
    for lamb, color in zip([1, 10, 0.1], "rgy"):
        x, y = fitreg(phi_poly, xx, yy, lamb)
        plt.plot(x, y[:, 0], '{}-'.format(color))

    plt.show()

I also added a if __name__ == "__main__": guard to allow importing from this script in another script and used numpy.arange for your mu array.

For the construction of xx you don't actually need the numpy.tile, the normal broadcasting rules already do that for you.


Clojure reverse multiple times I have a piece of Clojure code that reverses a string recursively.
It is working but too slow (because I got the problem from Codewars and I cannot validate it before it timeouts).

The algorithm is :

take a string ex. "abc" and reverse it (r "abc") -> "cba"
take the first position and reverse from there "c" + (r "ba") -> "cab"
take the second position and reverse from there "ca" + (r "b") -> "cab"
and so on..

First iteration :

(defn reverse-fun [s]
  (loop [r s p 0]
    (if (= p (count s))
      r
      (let [[b e] (split-at p r)
            e     (reverse e)]
        (recur (apply str (concat b e)) (inc p))))))

However it was too slow. I pass about 31 tests with this one before the timeout. Here is the fastest I have for now :

(defn reverse-fun [s]
  (apply str (loop [r (vec s) p 0]
               (if (= p (count r))
                 r
                 (let [b (subvec r 0 p)
                       e (rseq (subvec r p (count r)))]
                   (recur (into [] (concat b e)) (inc p)))))))

This code passes about 72 tests before there is a timeout.
How can I make this code faster ?

The intermediate reversals seem like a bit of a trick question and are not really necessary to solve this problem. They are a complicated way to describe reordering a sequence indexed
0 ... n by taking items n, 0, n-1, 1, n-2, 2 ...
alternating from which end you take each time.

If you abandon the idea of creating the intermediate strings described it becomes more about how to select the right character from the input string for each position :

(ns reversefun.core)

(defn reverse-fun [s]
  (->> (interleave (reverse s) s)
       (take (count s))
       (apply str)))


Simple routing system This is my first routing system on PHP. As you can see it's very simple. Any advice or tip about it that can make it better? I want to make it better so I could use it in real websites.

First I include db connect, then get the URL, if the URL have special index like post id or post name, then include post.php or if $sp_link[4] == profile then load the profile.php.

<?php
    require_once 'inc/header.php';
    include_once 'inc/nav.php';

    $actual_link = (isset($_SERVER['HTTPS']) ? "https" : "http") . "://$_SERVER[HTTP_HOST]$_SERVER[REQUEST_URI]";
    // echo $actual_link;
    $sp_link = explode("/", $actual_link);
    print_r($sp_link);

    $sql ='SELECT * from posts WHERE post_id=? AND post_name=?';
    $stmt=$pdo->prepare($sql);
    $stmt->execute([@$sp_link[5], @$sp_link[6]]);
    $post=$stmt->fetch();

    if ($sp_link[4] == 'blog' && @$sp_link[5] == '' ) {
        // @include 'blog.php';
        //$page_title = $post->post_title;
    }
    else if ($sp_link[4] == 'profile' && @$sp_link[5] == '' ) {
        @include_once 'profile.php';
    }
    else if ($sp_link[4] == 'login' && @$sp_link[5] == '' ) {
        @include 'login.php';
        $page_title = 'ورود';
    }
    else if ($sp_link[4] == 'signup' && @$sp_link[5] == '' ) {
        @include 'signup.php';
        $page_title = 'ورود';
    }
    else if ($sp_link[4] == 'blog' && @$sp_link[5] == $post->post_id && @$sp_link[6] ==     $post->post_name) {
        @include 'post.php';
        $page_title = $post->post_title;

    }
    else if (($sp_link[4] == '' || $sp_link[4] == 'index' || $sp_link[4] == 'index.php') &&     @$sp_link[5] == '') {
           $page_title = 'سکان آکادمی | آموزش برنامه نویسی';
            include 'home.php';
    }
    else if (($sp_link[4] == 'blog' && $sp_link[5] == '12342') && @$sp_link[6] == '') {
        include_once 'post.php';
    } else {
        include_once '404.php';
    }

Firstly, I would consider switching off error suppression, and add in Exception handling. You could in this example add in a condition to check if each file exists before including it, for instance:

if ($sp_link[4] == 'profile' && @$sp_link[5] == '' && file_exists('profile.php')) {
    include_once 'profile.php';
}

It looks like you're using the query only to return a page title, and only in one instance. Is this correct? If so, amend you query to only return the data that you want, rather than returning the whole dataset from any particular table, for instance, if you only want the title tuple, change it to this:

$sql ='SELECT `title` FROM `posts` WHERE `post_id`=? AND `post_name`=?';
$stmt=$pdo->prepare($sql);
$stmt->execute([$sp_link[5], $sp_link[6]]);
$post=$stmt->fetch(PDO::FETCH_ASSOC);

To improve this further, you could turn this block of PHP into a function or method. Here's how one might achieve this aim:

function simpleSelect($fields = '*', $table = 'posts', $postId = '', $postName = '', $stmt = null){
    if(is_array($fields)){
        $fields = implode(',', $fields);
    }
    $sql = "SELECT `{$fileds}` FROM `{$table}` WHERE `post_id`=? AND `post_name`=?";
    $stmt = $pdo->prepare($sql);
    $stmt->execute([$postId, $postName]);
    return $stmt->fetch(PDO::FETCH_ASSOC);
}

Now the top of your code starting from $sql ='SELECT * from posts WHERE post_id=? AND post_name=?'; and ending with $post=$stmt->fetch(); becomes this:

$post = simpleSelect('title', 'posts', $sp_link[5], $sp_link[6], $stmt);

If you need to select multiple fields from the posts table, then you could do this (imagining that you want title and name fields returned):

$post = simpleSelect(['title', 'name'], 'posts', $sp_link[5], $sp_link[6], $stmt);

Finally, consider moving to an object-orientated approach. Exception handling and OO programming will be covered elsewhere.


Find solutions to a quadratic equation I've started to get far more familiar with C (and coding in general) and the way to use function to my advantage to create more complex programs. As a way of doing something actually useful instead of my "Starting Programming with C: For Dummies" book I decided to make a simple program to help me cheat my way through maths class, a program that calculates the quadratic equation.
However due to the fact that I've never seen a piece of code that is longer than 60 lines outside of a youtube videos and my for Dummies book, I don't exactly know if the way I format functions is readable to other people.
/*El programa calcula la ecuación general de segundo grado,
no tiene mayor proposito más que practicar C y hacer trampa en los exámenes -w-,
trato de usar funciones claras para hacer más legible el código, tómese nota de que me gusta
main en primer lugar main y luego el resto de funciones.*/
//----------------------------------------------------------------------------------------------------------------------------------------------------------------------
#include<stdio.h>
#include<stdlib.h>
#include<math.h>
int a,b,c,total,resultado; //total se usa para el resultado de diferentes opercaciones de la ecuación, resultado es el resultado final de la ecuación
void value_input();
int discriminante(int b, int a, int c);
void display();
//----------------------------------------------------------------------------------------------------------------------------------------------------------------------
void main(){
        value_input();
        b=-b; //como todas las operaciones con "b" son su multiplicación *-1 y su cuadrado, decidí solo volver b negativo (o positivo dependiendo del caso)
        discriminante(b,a,c);

        display();
}
//-----------------------------------------------------------------------------------------------------------------------------------------------------------------------
//value_input solo le pregunta al usuario los valores de a, b y c en la ecuación y los aloca a las variables correspondientes.
void value_input(){
        printf("Dame el valor de a: ");
        scanf("%d", &a);
        printf("Dame el valor de b: ");
        scanf("%d", &b);
        printf("Dame el valor de c: ");
        scanf("%d", &c);
}
//----------------------------------------------------------------------------------------------------------------------------------------------------------------------
//La función calcula la discriminante y devuelve su valor
int discriminante(int b, int a, int c){
        int result;
        b*=b;
        c=4*a*c;
        total=b-c;
        return total;
}
//----------------------------------------------------------------------------------------------------------------------------------------------------------------------
//display calcula el divisor de la ecuación y determina como imprimir la ecuación basado en el discriminante
void display(){
        a*=2; //se calcula el divisor
        //se calcula valor del discriminante y dependiendo de éste se imprime el resultado
        if(total>0){    //es mayor que 0
                total=sqrt(total);
                printf("El resultado es: (%d±%d)/%d\n",b,total,a);
                }
        else if(total<0){       //es menor que 0
                total=abs(total);
                total=sqrt(total);
                printf("El resultado es: (%d±%di)/%d\n",b,total,a);
                }
        else if(total=0){       //es igual a 0
                printf("El resultado es: (%d\n",b/a);
                }
}

I think that it is mostly understandable for anyone that wants to review the code in general, but I think that it can become a little convoluted in some places, specially the display() function.
How could I make it clearer?

I would find it easier to understand if the comments were in English, but I'm not the one who will be making changes later, so write your comments for that person (whatever is most useful to future-you).
What I find actively harmful is the very long lines such as

//----------------------------------------------------------------------------------------------------------------------------------------------------------------------

and

        b=-b; //como todas las operaciones con "b" son su multiplicación *-1 y su cuadrado, decidí solo volver b negativo (o positivo dependiendo del caso)

For the first, I think an empty line would be better; for the second, consider word-wrapping it as a block comment.
The global variables a, b, c, total and resultado make it harder to reason about each function in isolation.  I can't even see where that last one is actually used.  Prefer to pass values to and from functions rather than communicating via globals.
These function declarations would be better as prototypes (i.e. specify that they take no arguments, rather than unspecified ones):
void value_input(void);
void display(void);

I'd declare all the helpers with static linkage (this becomes more important when you write programs with multiple source files, but it's a good habit to form), and I'd accept the arguments to discriminante() in the less surprising order a, b, c.
void main() is incorrect - the main function must return an int, so we need int main(void).  Your compiler should have warned you about this; make sure you're enabling enough warning options (e.g. gcc -Wall -Wextra).
When we read input using scanf(), it's important to use the return value that indicates how many conversions were successful.  I would write a function to get a single value (and accept non-integer coefficients, since that's what we can expect from real equations - pun intended):
double read_double(const char *prompt)
{
    for (;;) {
        printf("%s: ", prompt);
        double x;
        int conversions = scanf("%lf", &x);
        if (conversions == 1) {
            return x;
        }
        if (conversions == EOF) {
            /* not much we can do now */
            fputs("Reading failed\n", stderr);
            exit(1);
        }
        /* consume the invalid input and start again */
        puts("Input should be a valid number");
        scanf("%*[%\n]");
    }
}

discriminante() declares total which it never uses, and returns a value that gets ignored.  There's clearly something wrong there.
This line doesn't do what you think it does:

   else if(total=0){

Thankfully, it's harmless, since if total>0 and total<0 are both false, we know that total is already zero, and the assignment has no effect.
When we know a number n is negative, we can make it positive simply with -n rather than calling abs().

Modified code
Here's what I ended up with after fixing the above:
/* El programa calcula la ecuación general de segundo grado, no tiene
   mayor proposito más que practicar C y hacer trampa en los exámenes
   -w-, trato de usar funciones claras para hacer más legible el
   código, tómese nota de que me gusta main en primer lugar main y
   luego el resto de funciones.
*/

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

static double read_double(const char *prompt);
static double discriminante(double a, double b, double c);
static void display(double a, double b, double d);

int main(void)
{
    const double a = read_double("Dame el valor de a");
    const double b = read_double("Dame el valor de b");
    const double c = read_double("Dame el valor de c");

    const double d = discriminante(a, b, c);

    display(a, b, d);
}

static double read_double(const char *prompt)
{
    for (;;) {
        printf("%s: ", prompt);
        double x;
        int conversions = scanf("%lf", &x);
        if (conversions == 1) {
            return x;
        }
        if (conversions == EOF) {
            /* not much we can do now */
            fputs("Reading failed\n", stderr);
            exit(1);
        }
        /* consume the invalid input and start again */
        puts("Input should be a valid number");
        scanf("%*[%\n]");
    }
}

// La función calcula la discriminante y devuelve su valor
static double discriminante(double a, double b, double c)
{
    return b * b - 4 * a * c;
}

// display calcula el divisor de la ecuación y determina como imprimir
// la ecuación basado en el discriminante
static void display(double a, double b, double d)
{
    printf("El resultado es: ");
    if (d > 0) {
        printf("(%g±%g)/%g\n", -b, sqrt(d), a);
    } else if (d < 0) {
        printf("(%g±%gi)/%g\n", -b, sqrt(-d), a);
    } else {                /* total == 0 */
        printf("%g/%g\n", -b, a);
    }
}


Popup message queueing I have a WPF application where I have a function for generating an overlaying info grid.

I have written some code for queueing the messages(IpopupMessage) so that each displays a certain amount of time

public interface IPopupMessage
{
    string Description { get; set; }
    TimeSpan Duration { get; set; }
    SolidColorBrush MessageColor { get; set; }
    string Title { get; set; }
}

This is the code for displaying and hiding the UI-element

private readonly Queue<IPopupMessage> PopupMessageQueue = new Queue<IPopupMessage>();

public void DisplayPopupmessage(IPopupMessage obj)
{
    this.PopupMessage = obj;
    if (obj.Duration > TimeSpan.Zero) this.HideErrorMessageAfter(obj.Duration);
}

protected async void HideErrorMessageAfter(TimeSpan delayTime)
{
    await Task.Delay(delayTime);
    if (this.PopupMessageIsVisible) this.PopupMessageIsVisible = false;
}

private void CancelPopupMessage()
{
    this.PopupMessageIsVisible = false;
}

private async void UpdatePopupMessage(IPopupMessage obj)
{
    //Adding the message to the queue
    this.PopupMessageQueue.Enqueue(obj);
    //never ending loop
    while (true)
        //PopupMessageIsVisible is a boolean with binding to the UI element
        if ((this.PopupMessageQueue.Count > 0) && (this.PopupMessageIsVisible == false))
        {
            this.PopupMessageIsVisible = true;
            this.DisplayPopupmessage(this.PopupMessageQueue.Dequeue());
        }
        else
        {
            await Task.Delay(TimeSpan.FromSeconds(1));
        }
}

Is there a cleaner more efficient way for this? I have tried some variants with single thread TPL actionblock, but the timer in HideErrorMessageAfter would set PopupMessageIsVisible false and cut the next message short.

I also tried creating a continuous Queue where every queued item would get consumed automatically, but I could not get it to work.

Do you have any tips for making this code more efficient?

I think instead of looping indefinitely over the queue you should only start it when the first popup is queued via the DisplayPopupmessage and loop only as long as there are any popups. After you've shown everything you should exit the loop and  start agian when something arrives.

For this job you should have a dedicated service that knows how to handle it. Currently it looks like the logic is within the popup-message.

I used these models for testing:

interface IPopupMessage
{
    string Message { get; }
    TimeSpan Timeout { get; }
}

class PopupMessage : IPopupMessage
{
    public string Message { get; set; }
    public TimeSpan Timeout { get; set; }
}

A simple service could be implemented like this one to handle the queue:

class PopupMessageService
{
    private bool _started;
    private readonly Queue<IPopupMessage> _popupMessageQueue = new Queue<IPopupMessage>();

    public void DisplayPopupMessage(IPopupMessage popup)
    {
        Console.WriteLine($"Equeued \"{popup.Message}\".");
        _popupMessageQueue.Enqueue(popup);
        Start();
    }

    private async void Start()
    {
        if (_started) { return; }
        _started = true;
        Console.WriteLine("Service started.");
        while (_popupMessageQueue.Any())
        {
            await Task.Run(async () =>
            {
                var msg = _popupMessageQueue.Dequeue();
                Console.WriteLine($"Showing \"{msg.Message}\".");
                await Task.Delay(msg.Timeout);
            });
        }
        _started = false;
        Console.WriteLine("Service stopped.");
    }
}

The real message should have a method like Show(CacellationToken token) that you use to display the message and to hide it via the token.IsCancellationRequested and token.ThrowIfCancellationRequested(). You should handle the OperationCanceledException in order to proceed to the next popup.

It's just for demonstration purposes so I left the Console.WriteLine there.

And here's one more example how to use the above service:

void Main()
{
    Messages();
}

static async void Messages()
{
    var popupMessageService = new PopupMessageService();

    var messages = new IPopupMessage[]
    {
        new PopupMessage { Message = "Foo", Timeout = TimeSpan.FromSeconds(3) },
        new PopupMessage { Message = "Baz", Timeout = TimeSpan.FromSeconds(5) },
        new PopupMessage { Message = "Qux", Timeout = TimeSpan.FromSeconds(1) },
        new PopupMessage { Message = "Bar", Timeout = TimeSpan.FromSeconds(2) },
    };

    foreach (var msg in messages)
    {
        popupMessageService.DisplayPopupMessage(msg);

        // For test purposes assuming a message arrives every 3 seconds.
        await Task.Delay(TimeSpan.FromSeconds(3));
    }
}

The output is:

Equeued "Foo".
Service started.
Showing "Foo".
Service stopped.
Equeued "Baz".
Service started.
Showing "Baz".
Equeued "Qux".
Showing "Qux".
Service stopped.
Equeued "Bar".
Service started.
Showing "Bar".
Service stopped.


Simplifying ToString() implementation Any ideas?

public override string ToString() {
    string returnValue;
    if (!string.IsNullOrWhiteSpace(Country))
        returnValue = string.Format("{0}", Country);
    else
        return returnValue;

    if (!string.IsNullOrWhiteSpace(City))
        returnValue += string.Format(", city {0}", City);
    else
        return returnValue;

    if (!string.IsNullOrWhiteSpace(Street))
        returnValue += string.Format(", street {0}", Street);
    else
        return returnValue;

    if (!string.IsNullOrWhiteSpace(Block))
        returnValue += string.Format(", block {0}", Block);
    else
        return returnValue;

    if (!string.IsNullOrWhiteSpace(Building))
        returnValue += string.Format(", building {0}", Building);

    if (!string.IsNullOrWhiteSpace(Latitude)
             && !string.IsNullOrWhiteSpace(Longitude))
    returnValue += string.Format(", coordinates {0}:{1}", Latitude, Longitude);

    return returnValue;
}

I find that writing it this way makes it more maintainable and relatively efficient.

public class Information
{
    public string Country { get; set; }
    public string City { get; set; }
    public string Street { get; set; }
    public string Block { get; set; }
    public string Building { get; set; }
    public string Latitude { get; set; }
    public string Longitude { get; set; }

    private IEnumerable<Tuple<string[], string>> DataFormatPairs()
    {
        yield return Tuple.Create(new[] { Country },             "{0}");
        yield return Tuple.Create(new[] { City },                "city {0}");
        yield return Tuple.Create(new[] { Street },              "street {0}");
        yield return Tuple.Create(new[] { Block },               "block {0}");
        yield return Tuple.Create(new[] { Building },            "building {0}");
        yield return Tuple.Create(new[] { Latitude, Longitude }, "coordinates {0}:{1}");
    }
    public override string ToString()
    {
        var data = DataFormatPairs()
            .TakeWhile(p => p.Item1.All(s => !String.IsNullOrWhiteSpace(s)))
            .Select(p => String.Format(p.Item2, p.Item1));
        return String.Join(", ", data);
    }
}


How to sort array by divisor sum faster? I am sorting an array by divisor sum in ascending order. If two or more numbers have the same sum of divisors they have to be sorted in ascending order. The problem I have is that my version is not fast enough. Here is my code:

int divisorSum( double number )
{
    double sum = number + 1;
    for( int i = 2; i <= number / 2; i++ )
        if(( int )number % i == 0 )
            sum += i;
    return sum;
}
void sortByDivisorSum( double *array, int length )
{
    for( int i = 0; i < length; i++ )
    {
        for( int j = i + 1; j < length; j++ )
        {
            if(( divisorSum( array[ i ]) > divisorSum( array[ j ])) || (( divisorSum( array[ i ] ) == divisorSum( array[ j ])) && ( array[ i ] > array[ j ])))
            {
                array[ i ] += array[ j ];
                array[ j ] = array[ i ] - array[ j ];
                array[ i ] -= array[ j ];
            }
        }
    }
}
void readArrayy(double *array, int length)
{
    for(int index = 0; index < length; index++)
        scanf("%lf", &array[index]);
}
int main( void )
{
    int length, in;
    scanf( "%d", &length );
    double ar[ length ];
    readArrayy( ar, length );
    sortByDivisorSum( ar, length );
    for( in = 0; in < length; in++ )
    {
        printf( "%.0lf ", ar[in]);
    }
    return 0;
}

How to solve this in a faster way?

Input:

10
24 46 11 36 48 35 27 28 49 6

Output:

6 11 27 35 28 49 24 46 36 48

Many elements of this code can be much faster.

Calculating the sum of divisors will be faster if you:

Iterate until sqrt(number) instead of number / 2, and include divisor pairs (i and number / i)
Change the type of number from double to int

(See also my review of your related question.)

Use a faster sorting algorithm, for example quicksort or merge sort.
And when you, avoid mistakes like if(( divisorSum( array[ i ]) > divisorSum( array[ j ])) || (( divisorSum( array[ i ] ) == divisorSum( array[ j ])) && ( array[ i ] > array[ j ]))), which recomputes the sum of divisors twice for array[i] and array[j].
You could store the results of expensive operations in a variable,
and then you can use that variable multiple times in conditions.


Calculating frequency of a note from a string I've been working on a solution to Pset3 of CS50 Harvard course for last couple of hours and managed to finish it. Although it works as intended I'm unhappy with how clean this code is, mostly because of use of multiple nested conditionals inside the main switch case. The function takes a string, for example "B#3", and converts it to correct frequency.

If interested here is the Pset and here is the frequency table.

// Calculates frequency (in Hz) of a note
int frequency(string note)
{
    const double a4Frequency = 440;
    char key;
    float numOfSemitones = 0;
    double octave = 0;
    bool flat, sharp;

    // Determine octave and whether key is sharp or flat
    if (note[1] == 'b')
    {
        octave = note[2] - '0';
        flat = true;
    }
    else if(note[1] == '#')
    {
        octave = note[2] - '0';
        sharp = true;
    }
    else
        octave = note[1] - '0';

    key = note[0];

    // Base frequency knowing that A4 is 440 hz
    if(((key == 'A') && (octave == 4)) && ((!flat) && (!sharp)))
        return 440;

    // TODO find alternative way to count distance between keys - there is a pattern
    // Count frequency using distance in semitones from A4
    double frequency = 0.0;
    switch(key)
    {
        case 'B':
            if (flat)
                numOfSemitones = 1;
            else
                numOfSemitones = 2;

            frequency = pow(2, (numOfSemitones / 12))  * a4Frequency;
            break;
        case 'A':
            if (flat){
                numOfSemitones = 1;
                frequency = a4Frequency / pow(2, (numOfSemitones / 12));
            }
            else if (sharp){
                numOfSemitones = 1;
                frequency = pow(2, (numOfSemitones / 12))  * a4Frequency;
            }
            else
                frequency = a4Frequency;

            break;
        case 'G':
            if (flat)
                numOfSemitones = 3;
            else if (sharp)
                numOfSemitones = 1;
            else
                numOfSemitones = 2;

            frequency = a4Frequency / pow(2, (numOfSemitones / 12));
            break;
        case 'F':
            if (sharp)
                numOfSemitones = 3;
            else
                numOfSemitones = 4;

            frequency = a4Frequency / pow(2, (numOfSemitones / 12));
            break;
        case 'E':
            if (flat)
                numOfSemitones = 6;
            else
                numOfSemitones = 5;

            frequency = a4Frequency / pow(2, (numOfSemitones / 12));
            break;
        case 'D':
            if (flat)
                numOfSemitones = 8;
            else if (sharp)
                numOfSemitones = 6;
            else
                numOfSemitones = 7;

            frequency = a4Frequency / pow(2, (numOfSemitones / 12));
            break;
        case 'C':
            if (sharp)
                numOfSemitones = 8;
            else
                numOfSemitones = 9;

            frequency = a4Frequency / pow(2, (numOfSemitones / 12));
            break;
    }

    // Multiply or divide frequency to get note in correct octave
    int final;
    if (octave > 4)
    {
        octave = pow(2, (octave - 4));
        final = round(frequency * octave);
    }
    else if (octave == 4)
    {
        octave = 0.0;
        final = round(frequency);
    }
    else
    {
        octave = pow(2, (4 - octave));
        final = round(frequency / octave);
    }
    return final;
}

I'm trying to focus on writing clean code from start, yet this solutions seems a bit hacked to me. How can I make this code more readable? What's the alternative to nested conditionals? What's "illegal" about this code?

Overall this is pretty readable code. It's straightforward to understand, so it would be fairly easy to maintain or debug if there were found to be problems.
Table-driven Development
If you look at the various case statements within your switch you'll notice that they all take the same form, just with different values. This indicates that it might be a good idea to use a table of values to reduce the amount of code. I would build a table of the number of semitone change for each note like this:
int kSemitoneOffsets[] = { -2, 0, 2, 4, 5, 7, 9 };

In the natural case where there are no accidentals (sharps or flats) applied, every case from your switch statements breaks down to this:
frequency = a4Frequency / pow(2, (kSemitoneOffsets [ index ]  / 12));

Here index is calculated from the key character. If we don't mind rearranging the above table so it progresses from A up to G, it ends up looking like this:
int kSemitoneOffsets[] = { 0, -2, 9, 7, 5, 4, 2 };

Now you can calculate the index like this:
int index = key - 'A';

Next, we can handle accidentals by adding or subtracting 1 from the lookup. Putting it all together, it looks like this:
int kSemitoneOffsets[] = { 0, -2, 9, 7, 5, 4, 2 };
int index = key - 'A';
int numOfSemitones = kSemitoneOffsets [ index ];
if (flat)
{
    numOfSemitones--;
}
else if (sharp)
{
    numOfSemitones++;
}
frequency = a4Frequency / pow(2, (numOfSemitones / 12));

Notice that I changed the calculation for B from a multiplication to a division so it was the same form as the other calculations. This is possible because raising a number to a negative power is the same as dividing by that number raised to the positive power.
We can use this same trick to calculate final without the if statement, like this:
final = round(frequency * pow(2, octave - 4));

When octave is less than 4, we get a negative number and end up dividing. When it's 0, we get 1 so the multiplication does nothing.
Remove Unnecessary Code
In addition to switching to using a table, you can also remove unnecessary code. For example the if statement you use to check if the user specifically entered A4. That code is unnecessary because you will correctly calculate the value using the rest of your code. Unless this function is being called inside some tight inner loop, its speed isn't a concern, so that optimization is not very helpful.
Avoid Magic Numbers
Your code also has a bunch of magic numbers. These are hard-coded values in the code that have no indication what their purpose is. You should create named constants for these. You have one for 440Hz, but the values of the semitone offsets are a bit mysterious. In my code I put them in a table with a name so it's more obvious what they are. But in general, values other than 0 or 1 should have a name. I would also comment the relationship of 2^(numSemitones / 12) as it would be non-obvious to people who haven't studied the physics of Western music.
Nitpicks
One other thing I noticed is that in your code you didn't handle the cases of Fb, Cb, E#, or B#. These are valid notes that you'll want to handle. They don't come up often, and they are synonyms (at least in equal temperament) for E, B, F, and C, but they are legit, so I'd recommend handling them. (And if you want to get really technical, you could handle double sharps (Ax4, for example), and double flats (Bbb3, for example).)


Removing duplication in RSpec I’m still relatively new to the TDD scene. I’m writing a Rails app, and some duplication immediately sticks out at me in my first model spec. How else can I approach this?

require 'spec_helper'

describe TimeEntry do

  describe "without a project" do
    let(:entry) { FactoryGirl.build :time_entry }
    it "should not be valid" do
      entry.project = nil
      expect(entry).to_not be_valid
    end
  end

  describe "without a user" do
    let(:entry) { FactoryGirl.build :time_entry }
    it "should not be valid" do
      entry.user = nil
      expect(entry).to_not be_valid
    end
  end

end

I don't think this could get much drier. let is not cached across examples but it is cached within examples, so you can't move that let out of the nested describe blocks.

Maybe it might be easier for you to define tests that test when the model is valid, and then have a catch-all test to fail in any other case? Unless you are testing validation messages (which I don't think you need to be testing individually, and that is out of the scope of a unit test), you don't need to know which individual field failed, only that the model is invalid and X should happen.

describe TimeEntry do
  describe "validity" do
    let(:entry) { FactoryGirl.build :time_entry }
    it "is invalid without a project" do
      entry.project = nil
      expect(entry).to_not be_valid
    end
    it "is invalid without a user" do
      entry.user = nil
      expect(entry).to_not be_valid
    end
  end
end

Alternatively you can do the above.


Will this code make some attacks (XSS, CSRF...) harder to do? I have following code on my login page (now i use it for preventing POST variable resubmit on page refresh):

<?php

$cleaner = md5(uniqid(rand(), TRUE));
$_SESSION["cleaner"]=(empty($_SESSION["cleaner"])) ? $cleaner : $_SESSION["cleaner"];

if($_POST['login_button'] && !empty($_POST['cleaner']) && $_SESSION['cleaner'] == $_POST['cleaner']){

    $cleaner = md5(uniqid(rand(), TRUE));
    $_SESSION['cleaner'] = $cleaner;

     //all other verification/login code

}
?>

<form method="POST" action="">
  <input type="hidden" name="cleaner" value="<?php echo $_SESSION['cleaner']; ?>" />
  <--! all other login html code and fields -->
</form>

Can this code make some attacks harder to do, or if not - what to change?

I haven't had the need to worry about this kind of thing before, so I can't claim that this will answer your question, but maybe it will help. It looks simple enough, but rand() may not be the best tool here. I know that, at least with Java, the random function is easily beaten because it starts with a number from a list and then iterates over that list each time you call it, and the list is always the same. So maybe this is something to look at in PHP too. Also, I was here anyways and I couldn't help but notice a few things.

A couple of minor notes first. One line of code should only do one thing. This helps with legibility, which is a real problem the deeper you nest functions. And what is "cleaner" by the way? This is a really odd name. Would expect "id" or something else, but anyways.

$id = uniqid( rand(), TRUE );
$cleaner = md5( $id );

You should check if the variable is set, not empty. Unless you already know it is set, which you don't because it hasn't been done in this document. Also, the extra parenthesis are unnecessary in this ternary operation.

$_SESSION[ 'cleaner' ] = isset( $_SESSION[ 'cleaner' ] ) ? $cleaner : $_SESSION[ 'cleaner' ];

A more efficient way, instead of creating something you will now never use, is to only create it if you have to. So...

if( isset( $_SESSION[ 'cleaner' ] ) ) {
    $cleaner = $_SESSION[ 'cleaner' ];
} else {
    $id = uniqid( rand(), TRUE );
    $cleaner = md5( $id );
}

$_SESSION[ 'cleaner' ] = $cleaner;

You may notice that I'm now modifying the session variable no matter what. Its not really necessary, but it keeps the session fresh which is sometimes nice.

With this next line I'm assuming that you are checking if the form has been submitted ("login_button" seems kind of obvious). This is a really debatable subject. Many people still do this, I think because they think it ensures that the user is actually using their form instead of their own, but its not that hard to look at the source and create POST parameters with the same keys, so I don't know how much truth there is in that.  I personally find it better to just check the request method, or, if I'm feeling particularly lazy, to see if the POST array is empty. As I said, its really debatable, so you can take it or leave it.

if( $_SERVER[ 'REQUEST_METHOD' ] == 'POST' ) {
//Or the hack
if( $_POST ) {

Next you check for the POST version of "cleaner", this should be another isset() check rather than an empty() check by the way, then you compare the session version to the POST version. So then, this would only be possible, theoretically, if we didn't use the cleaner we created earlier because the session version was set. Think I'm with you so far... But why not then use that earlier version we didn't use instead of creating a new one? To illustrate, a full rewrite might look something like this:

$id = uniqid( rand(), TRUE );
$cleaner = md5( $id );

if( isset( $_SESSION[ 'cleaner' ] ) ) {
    $temp = $_SESSION[ 'cleaner' ];
    if( ! $_SERVER[ 'REQUEST_METHOD' ] || $temp != $_POST[ 'cleaner' ] ) {
        $cleaner = $temp;
    }
}

$_SESSION[ 'cleaner' ] = $cleaner;

Hopefully this helps somewhat, sorry I couldn't really answer your question.


Data Access Layer method with transaction handling I just want to know whether I've done transaction handling well. Also what about the other types of transactions available in .NET with regard to this code snippet?

Also are there any redundant things in this code?

    public void InsertAttendance(BindingList<IAttendance> attendanceList)
    {
        string selectStatement = @"IF NOT EXISTS (SELECT Attendance.Atten_ID FROM Attendance WHERE Atten_ID = @aid) BEGIN INSERT INTO attendance (EID,PID,in_time,out_time,shift) VALUES(@eid,@pid,@inT,@outT,@shift) END ";
        using (SqlConnection sqlConnection = new SqlConnection(db.ConnectionString))
        {
            sqlConnection.Open();
            using (SqlTransaction trans = sqlConnection.BeginTransaction())
            using (SqlCommand sqlCommand = new SqlCommand(selectStatement, sqlConnection, trans))
            {
                sqlCommand.Parameters.Add("@aid", SqlDbType.Int);
                sqlCommand.Parameters.Add("@pid", SqlDbType.Char);
                sqlCommand.Parameters.Add("@eid", SqlDbType.Int);
                sqlCommand.Parameters.Add("@inT", SqlDbType.DateTime);
                sqlCommand.Parameters.Add("@outT", SqlDbType.DateTime);
                sqlCommand.Parameters.Add("@shift", SqlDbType.Char);
                //sqlConnection.Open();

                try
                {
                    foreach (Attendance obj in attendanceList)
                    {
                        sqlCommand.Parameters["@aid"].Value = obj.AttendanceID;
                        sqlCommand.Parameters["@pid"].Value = obj.PointID;
                        sqlCommand.Parameters["@eid"].Value = obj.EmployeeID;
                        sqlCommand.Parameters["@inT"].Value = obj.InTime;
                        sqlCommand.Parameters["@outT"].Value = obj.OutTime;
                        sqlCommand.Parameters["@shift"].Value = obj.ShiftType;

                        sqlCommand.ExecuteNonQuery();
                    }
                    trans.Commit();
                }
                catch { trans.Rollback(); throw; }
            }
        }
    }

First and foremost, I would strongly suggest using an ORM (e.g., Entity Framework or nHibernate) rather than doing all this manual work to update the database.  ORMs take most of the drudge work away and generally make things much easier.

That aside, there are a few things I would change:

As much as possible, utilize the ADO.NET interface methods instead of creating concrete classes directly
While a commit must be performed before leaving scope, rollback happens automatically, so the catch block is unnecessary
The method should not take in BindingList, both because it only requires IEnumerable and because DAL code shouldn't have ties to view concerns
The SQL statement variable should be const
There is no need to prefix variables with sql - this smells like a form of Hungarian Notation

With that in mind, I would modify it to be more like the following:

public void InsertAttendance(IEnumerable<IAttendance> attendanceList)
{
    const string selectStatement = @"IF NOT EXISTS (SELECT Attendance.Atten_ID FROM Attendance WHERE Atten_ID = @aid) BEGIN INSERT INTO attendance (EID,PID,in_time,out_time,shift) VALUES(@eid,@pid,@inT,@outT,@shift) END ";
    using (var connection = new SqlConnection(db.ConnectionString))
    {
        connection.Open();
        using (var trans = connection.BeginTransaction())
        using (var command = connection.CreateCommand())
        {
            command.CommandText = selectStatement;
            var parameters = command.Parameters;

            parameters.Add("@aid", SqlDbType.Int);
            parameters.Add("@pid", SqlDbType.Char);
            parameters.Add("@eid", SqlDbType.Int);
            parameters.Add("@inT", SqlDbType.DateTime);
            parameters.Add("@outT", SqlDbType.DateTime);
            parameters.Add("@shift", SqlDbType.Char);

            foreach (var attendanceItem in attendanceList)
            {
                parameters["@aid"].Value = attendanceItem.AttendanceID;
                parameters["@pid"].Value = attendanceItem.PointID;
                parameters["@eid"].Value = attendanceItem.EmployeeID;
                parameters["@inT"].Value = attendanceItem.InTime;
                parameters["@outT"].Value = attendanceItem.OutTime;
                parameters["@shift"].Value = attendanceItem.ShiftType;

                command.ExecuteNonQuery();
            }
            trans.Commit();
        }
    }
}

Sadly, creating parameters is particularly painful using just IDbCommand, so I continued using the SqlParameterCollection members.

Additional questions/considerations:

Should PointID be char?  An ID suggests a numeric type.
Should ShiftType be char?  This name suggests an enum type, which I suppose you could choose to represent as a character in the database, but it could also be a numeric type.
The SQL statement no-ops if a record already exists, but the method gives no indication when this occurs.  This should probably be re-considered, since it fails to provide any feedback to the caller that the insert did not occur.
What is db?  It seems to be providing the connection string, but we only see the property reference.


Python Euclidean Distance I'm working on some facial recognition scripts in python using the dlib library. dlib takes in a face and returns a tuple with floating point values representing the values for key points in the face. If the Euclidean distance between two faces data sets is less that .6 they are likely the same.

So, I had to implement the Euclidean distance calculation on my own. I found an SO post here that said to use numpy but I couldn't make the subtraction operation work between my tuples. Because this is facial recognition speed is important. If anyone can see a way to improve, please let me know.

Yes, it works :)

def euclidean_dist(data_x, data_y):
    if len(data_x) != len(data_y):
        raise Exception('Data sets must be the same dimension')
    dimensions = len(data_x)
    sum_dims = 0
    for dim in range(0, dimensions):
        sum_dims += (data_x[dim] - data_y[dim])**2

    return sqrt(sum_dims)

You can collapse the summation using sum():

sum_dims = sum((data_x[dim] - data_y[dim]) ** 2
               for dim in range(dimensions))

Or, what if you would "zip" the x and y:

sum_dims = sum((x - y) ** 2 for x, y in zip(data_x, data_y))


Filling a matrix with values I would like to hear your recommendations on when to use JavaScript's const over let, especially when applied to mutable objects as demonstrated in the following comparative code samples:

Strict preference of let:

// Create 100 x 100 zero-filled matrix:
let matrix = new Array(100);
for (let i = 0, length = matrix.length; i < length; ++i) {
  matrix[i] = new Array(100).fill(0);
}

// Fill matrix with values:
function fill(matrix, value) {
  for (let row of matrix) {
    row.fill(value);
  }
}

Strict preference of const:

// Create 100 x 100 zero-filled matrix:
const matrix = new Array(100),
      length = matrix.length;
for (let i = 0; i < length; ++i) {
  matrix[i] = new Array(100).fill(0);
}

// Fill matrix with values:
function fill(matrix, value) {
  for (const row of matrix) {
    row.fill(value);
  }
}

In my opinion, the const semantics in JavaScript are too weak to help static code analysis or to allow for compiler optimizations. Where const is (mis-)used to signal immutability, I'd go with e.g. declaring ALL_CAPS identifiers instead.

Is my thinking flawed? When writing robust, maintainable code, which of the above samples would you prefer? Is there a middle ground?

In my opinion, the const semantics in JavaScript are too weak to help static code analysis or to allow for compiler optimizations. Where const is (mis-)used to signal immutability, I'd go with e.g. declaring ALL_CAPS identifiers instead.

All const does is assure you that the value assigned to it is the same value throughout the scope. If it was assigned an array, it will be the same array and not suddenly change to another array or a different value altogether.

Mutation of the value, however, is a different story. JavaScript has no concept of immutability.Object.freeze does exist, and primitives are immutable, but that's about it.

For your code, if you're aiming to avoid mutation of your arrays, temporary values, counters and all, swap your loop for array.map and create a range function. Create a range, map through it, fill it with values, repeat.

function range(n){
  return Array(n).fill(0);
}

function matrix(dim, value){
  return range(dim).map(v => range(dim).map(v => value));
}

console.log(matrix(10, 2));


Photoshop Color Picker Gradient I'm trying to improve my Color Picker code.  It works like Photoshop and I don't want to use a library.

    for(var yColor = 0; yColor < 255; yColor++)
    {
        var r = 255,
            g = 0,
            b = 0;
        for(var xColor = 0; xColor < 255; xColor++)
        {
            if(r == 255 && g < 255 && b == 0) g += 6;
            else if(r > 0 && g == 255 && b == 0) r -= 6;
            else if(r == 0 && g == 255 && b < 255) b += 6;
            else if(r == 0 && g > 0 && b == 255) g -= 6;
            else if(r < 255 && g == 0 && b == 255) r += 6;
            else if(r == 255 && g == 0 && b > 0) b -= 6;

            if(r > 255) r = 255;
            else if(r < 0) r = 0;
            else if(g > 255) g = 255;
            else if(g < 0) g = 0;
            else if(b > 255) b = 255;
            else if(b < 0) b = 0;

            var rG = r + yColor > 255 ? 255 : r + yColor;
            var gG = g + yColor > 255 ? 255 : g + yColor;
            var bG = b + yColor > 255 ? 255 : b + yColor;

            GAContext.beginPath();
            GAContext.fillStyle = "rgb(" + rG + ", " + gG + ", " + bG + ")";
            GAContext.rect(xColor, yColor, 1, 1);
            GAContext.fill();
        }
    }

If I redraw it, It become a bit laggy.

In my tests, using createImageData and putImageData to fill the individual pixels ran about 3x faster than rect and fill.

You can replace this code:

GAContext.beginPath();
GAContext.fillStyle = "rgb(" + rG + ", " + gG + ", " + bG + ")";
GAContext.rect(xColor, yColor, 1, 1);
GAContext.fill();

With this:

pixelData[0] = rG;
pixelData[1] = gG;
pixelData[2] = bG;
GAContext.putImageData(pixel, xColor, yColor);

And before the loop, add this:

var pixel = GAContext.createImageData(1, 1);
var pixelData = pixel.data;
pixelData[3] = 255;

Test it out here. This Stack Overflow question is relevant. You may also be able to figure out a clever way to do this with gradients that would be much faster.