Similar statements need duplication removed How would you refactor this?

ExpensiveQuery must only run when necessary.

DECLARE @city VARCHAR(10)
DECLARE @status INT

IF @status = 5
BEGIN
    SELECT c.Name, c.Phone
    FROM Customers c
    WHERE c.City = @city
    AND c.Status = 5
END
ELSE IF @status = 10
BEGIN
    SELECT c.Name, c.Phone
    FROM Customers c
    WHERE c.City = @city
    AND c.Status = 10
    AND c.SignUpDate NOT IN (EXEC ExpensiveQuery)
END

I am not sure your quest for de-duplication is warranted,
the only approach I know of is this:

DECLARE @city VARCHAR(10)
DECLARE @status INT

SELECT c.Name, c.Phone
FROM Customers c
WHERE c.City = @city
AND c.Status = @status
AND ( ( c.Status = 5 )
OR    ( c.Status = 10  AND c.SignUpDate NOT IN (EXEC ExpensiveQuery) ) )


Fastest way to iterate over Numpy array I wrote a function to calculate the gamma coefficient of a clustering. The bottleneck is the comparison of values from dist_withing to dist_between. To speed this up, I tried to adapt and compile it using Cython (I dealt with C only few times). But I don't know, how to rapidly iterate over numpy arrays or if its possible at all to do it faster than

for i in range(len(arr)):
    arr[i]

I thought I could use a pointer to the array data and indeed the code runs in only half of the time, but pointer1[i] and pointer2[j] in cdef unsigned int countlower won't give me the expected values from the arrays. So, how to properly and speedy iterate over an array? And where else can be made improvements, even if in this case it would not make such a difference concerning runtime-speed?

# cython: profile=True
import cython

import numpy as np
cimport numpy as np

from scipy.spatial.distance import squareform

DTYPE = np.float
DTYPEint = np.int

ctypedef np.float_t DTYPE_t
ctypedef np.int_t DTYPEint_t

@cython.profile(False)
cdef unsigned int countlower(np.ndarray[DTYPE_t, ndim=1] vec1,
                             np.ndarray[DTYPE_t, ndim=1] vec2,
                             int n1, int n2):
    # Function output corresponds to np.bincount(v1 < v2)[1]

    assert vec1.dtype == DTYPE and vec2.dtype == DTYPE

    cdef unsigned int i, j
    cdef unsigned int trues = 0
    cdef unsigned int* pointer1 = <unsigned int*> vec1.data
    cdef unsigned int* pointer2 = <unsigned int*> vec2.data

    for i in range(n1):
        for j in range(n2):
            if pointer1[i] < pointer2[j]:
                trues += 1

    return trues

def gamma(np.ndarray[DTYPE_t, ndim=2] Y, np.ndarray[DTYPEint_t, ndim=1] part):
    assert Y.dtype == DTYPE and part.dtype == DTYPEint

    if len(Y) != len(part):
        raise ValueError('Distance matrix and partition must have same shape')

    # defined locals
    cdef unsigned int K, c_label, n_, trues
    cdef unsigned int s_plus = 0
    cdef unsigned int s_minus = 0

    # assigned locals
    cdef np.ndarray n_in_ci = np.bincount(part)
    cdef int num_clust = len(n_in_ci) - 1
    cdef np.ndarray s = np.zeros(len(Y), dtype=DTYPE)

    # Partition should have at least two clusters
    K = len(set(part))
    if K < 2:
        return 0
    # Loop through clusters
    for c_label in range(1, K+1):
        dist_within = squareform(Y[part == c_label][:, part == c_label])
        dist_between = np.ravel(Y[part == c_label][:, part != c_label])
        n1 = len(dist_within)
        n2 = len(dist_between)

        trues = countlower(dist_within, dist_between, n1, n2)
        s_plus += trues
        s_minus += n1 * n2 - trues

    n_ =  s_plus + s_minus

    return (<double>s_plus - <double>s_minus) / <double>n_ if n_ != 0 else 0

Edit1: Passing just the pointers, instead of the arrays to the time-critical function (>99% of time is spent there) made a ~ 10% speed-up. I guess some things just cannot be made faster

@cython.profile(False)
@cython.boundscheck(False)
@cython.nonecheck(False)
cdef unsigned int countlower(double* v1, double* v2, int n1, int n2):
    ''' Function output corresponds to np.bincount(v1 < v2)[1]'''
    ''' The upper is not correct. It rather corresponds to
    sum([np.bincount(v1[i] < v2)[1] for i in range(len(v1))])'''
    cdef unsigned int trues = 0

    cdef Py_ssize_t i, j
    with nogil, parallel():
        for i in prange(n1):
            for j in prange(n2):
                if v1[i] < v2[j]:
                    trues += 1
    return trues

1. Introduction

This question is difficult because:

It's not clear what the function countlower does. It's always a good idea to write a docstring for a function, specifying what it does, what arguments it takes, and what it returns. (And test cases are always appreciated.)
It's not clear what the role of the arguments n1 and n2 is. The code in the post only ever passes len(v1) for n1 and len(v2) for n2. So is that a requirement? Or is it sometimes possible to pass in other values?

I am going to assume in what follows that:

the specification of the countlower function is Return the number of pairs i, j such that v1[i] < v2[j];
n1 is always len(v1) and n2 is always len(v2);
the Cython details are not essential to the problem, and that it's OK to work in plain Python.

Here's my rewrite of the countlower function. Note the docstring, the doctest, and the simple implementation, which loops over the sequence elements rather than their indices:

def countlower1(v, w):
    """Return the number of pairs i, j such that v[i] < w[j].

    >>> countlower1(list(range(0, 200, 2)), list(range(40, 140)))
    4500

    """
    return sum(x < y for x in v for y in w)

And here's a 1000-element test case, which I'll use in the rest of this answer to compare the performance of various implementations of this function:

>>> v = np.array(list(range(0, 2000, 2)))
>>> w = np.array(list(range(400, 1400)))
>>> from timeit import timeit
>>> timeit(lambda:countlower1(v, w), number=1)
8.449613849865273

2. Vectorize

The whole reason for using NumPy is that it enables you to vectorize operations on arrays of fixed-size numeric data types. If you can successfully vectorize an operation, then it executes mostly in C, avoiding the substantial overhead of the Python interpreter.

Whenever you find yourself iterating over the elements of an array, then you're not getting any benefit from NumPy, and this is a sign that it's time to rethink your approach.

So let's vectorize the countlower function. This is easy using a sparse numpy.meshgrid:

import numpy as np

def countlower2(v, w):
    """Return the number of pairs i, j such that v[i] < w[j].

    >>> countlower2(np.arange(0, 2000, 2), np.arange(400, 1400))
    450000

    """
    grid = np.meshgrid(v, w, sparse=True)
    return np.sum(grid[0] < grid[1])

Let's see how fast that is on the 1000-element test case:

>>> timeit(lambda:countlower2(v, w), number=1)
0.005706002004444599

That's about 1500 times faster than countlower1.

3. Improve the algorithm

The vectorized countlower2 still takes \$O(n^2)\$ time on arrays of length \$O(n)\$, because it has to compare every pair of elements. Is it possible to do better than that?

Suppose that I start by sorting the first array v. Then consider an element y from the second array w, and find the point where y would fit into the sorted first array, that is, find i such that v[i - 1] < y <= v[i]. Then y is greater than i elements from v. This position can be found in time \$O(\log n)\$ using bisect.bisect_left, and so the algorithm as a whole has a runtime of \$O(n \log n)\$.

Here's a straightforward implementation:

from bisect import bisect_left

def countlower3(v, w):
    """Return the number of pairs i, j such that v[i] < w[j].

    >>> countlower3(list(range(0, 2000, 2)), list(range(400, 1400)))
    450000

    """
    v = sorted(v)
    return sum(bisect_left(v, y) for y in w)

This implementation is about three times faster than countlower3 on the 1000-element test case:

>>> timeit(lambda:countlower3(v, w), number=1)
0.0021441911812871695

This shows the importance of finding the best algorithm, not just speeding up the algorithm you've got. Here an \$O(n \log n)\$ algorithm in plain Python beats a vectorized \$O(n^2)\$ algorithm in NumPy.

4. Vectorize again

Now we can vectorize the improved algorithm, using numpy.searchsorted:

import numpy as np

def countlower4(v, w):
    """Return the number of pairs i, j such that v[i] < w[j].

    >>> countlower4(np.arange(0, 20000, 2), np.arange(4000, 14000))
    45000000

    """
    return np.sum(np.searchsorted(np.sort(v), w))

And this is six times faster still:

>>> timeit(lambda:countlower4(v, w), number=1)
0.0003434771206229925

5. Answers to your questions

In comments, you asked:

"What does vectorizing mean?" Please read the "What is NumPy?" section of the NumPy documentation, in particular the section starting:

  Vectorization describes the absence of any explicit looping, indexing, etc., in the code - these things are taking place, of course, just “behind the scenes” (in optimized, pre-compiled C code).

"What is meshgrid?" Please read the documentation for numpy.meshgrid.

I use meshgrid to create a NumPy array grid containing all pairs of elements x, y where x is an element of v and y is an element of w. Then I apply the < function to those pairs, getting an array of Booleans, which I sum. Try it out in the interactive interpreter and see for yourself:

>>> import numpy as np
>>> v = [2, 4, 6]
>>> w = [1, 3, 5]
>>> np.meshgrid(v, w)
[array([[2, 4, 6],
       [2, 4, 6],
       [2, 4, 6]]), array([[1, 1, 1],
       [3, 3, 3],
       [5, 5, 5]])]
>>> _[0] < _[1]
array([[False, False, False],
       [ True, False, False],
       [ True,  True, False]], dtype=bool)
>>> np.sum(_)
3


AnimationFrameController: requestAnimationFrame made easy I am looking for feedback on what I can improve to a tool I have been using for several months now, my AnimationFrameController. I find requestAnimationFrame an incredibly hassle and hard to control, so this controller does it for you. Are there better/more efficient ways to do this? The below is an example of how to use the code and the ES6 version of the source.

The main reason I built this is to be able to quickly throw together animation or things you can do every frame, but in the requestAnimationFrame function you get exactly one call, and you better hope it calls all animations etc... With my animationFrameController its as easy as going AF.add( myFunction ) and it will be called continuously until it gets removed (by AF.remove) or it returns false itself and cancels itself out. The animationFrame ends when no more handlers remain.

"use strict";

/*
 * AnimationFrameController 1.0
 */

class AnimationFrameController {

  constructor( autostart = true ){

    this.autostart = true;
    this.paused = true;
    this.time = 0;
    this.fps = 0;

    this.callees = [];
    this.calleesTime = [];

  }

  get time(){
    return this._time;
  }

  set time(v){
    this.fps = (v - this.time) / 1000;
    this._time = v;
    return this._time;
  }

  get paused(){
    return this._paused;
  }

  set paused(v){
    this._paused = v;
    if( this._paused === false ){
      this.loop( this.time );
    }
  }

  add( ...handlers ){

    handlers.forEach( handler => {
      if(handler instanceof Function){
        this.callees.unshift( handler );
        this.calleesTime.unshift( this.time );
      }
    });
    if(this.callees.length && this.autostart && this.paused) this.paused = false;

  }

  remove( ...handlers ){

    handlers.forEach( handler => {
      let index = -1;
      while((index = this.callees.lastIndexOf( handler )) >= 0){
        this.callees.splice( index, 1 );
        this.calleesTime.splice( index, 1 );
      }
    });
    if(this.callees.length <= 0) this.paused = true;

  }

  loop( time ){

    try {

      if( !this.paused ){

        window.requestAnimationFrame( this.loop.bind(this) );

        let delta = time - this.time;
        this.time = time;

        this.callees.map(( handler, index ) => {
          let progress = time - this.calleesTime[index];
          return handler( delta, progress ) === false ? handler : true;
        }).forEach(handler => {
          return handler !== true ? this.remove(handler) : 0;
        });

      }

    } catch(e) {

      this.paused = true;
      console.warn('An error has occurred while running the AnimationFrame.', e);

    }

  }

}

/* IMPLEMENTATION */

var AF = new AnimationFrameController;
var button = document.getElementById('clickme');
var animation = false;
var x = 0, y = 0;

button.addEventListener('click', function( event ){
  var fromX = x;
  var fromY = y;
  var toX = Math.round( Math.random() * (window.innerWidth - button.clientWidth) );
  var toY = Math.round( Math.random() * (window.innerHeight - button.clientHeight) );
  AF.remove( animation );
  AF.add(animation = function( delta, progress ){
    var now = progress / 2000;
    if( now >= 1 ){
      x = toX;
      y = toY;
      return false;
    } else {
      x = fromX + (toX - fromX) * now;
      y = fromY + (toY - fromY) * now;
    }
  });
});

AF.add(function(){
  button.style.left = x + 'px';
  button.style.top = y + 'px';
});
<input type="button" value="click me" id="clickme" style="position:absolute;" />

(You can find an ES5 version over at github if you want to try it in al older browser: https://github.com/jasperlinsen/AnimationFrameController)

What about two AnimationFrameControllers

That will work just fine?

As a games/visualisation programmer I would not use your  AnimationFrameController. It is just a callback stack, with ugly bits and would be difficult to use (you are catching my throws), GC unfriendly, carries a bad bug if I use  AnimationFrameController.pause = false; and should be a singleton

A breakdown of your code.

Don't use class, its is really bad and makes it so hard to write good javascript.
AND
You will only ever need one instance of AnimationFrameControler so it should be a singleton.

class AnimationFrameController {
  constructor( autostart = true ){
    this.autostart = true;
    this.paused = true;
    this.time = 0;
    this.fps = 0;

callees & calleesTime do not need to be exposed, programmers will just side step your add/remove function trying to take short-cuts then if you ever change the behaviour you will have a list of upset emails. "Why your code break?" Don't expose what should not be exposed.

Also why keep two arrays, why not just a simple object added to callees array {callback : callback, time : time}

    this.callees = [];
    this.calleesTime = [];
  }

Just bad a function call to get a exposed value. Use get to get abstract values not object properties.

  get time(){
    return this._time;
  }

  set time(v){
    this.fps = (v - this.time) / 1000;
    this._time = v;
    return this._time;
  }

Again pause is a property of AnimationFrameController why force a function call to get its value.

  get paused(){
    return this._paused;
  }

BUG: This function is will generate additional requestAnimationFrame loops on top of the current loops.

  set paused(v){
    this._paused = v;
    if( this._paused === false ){ // if not paused
      this.loop( this.time );     // call loop which on the 3rd line requests an animation fame without checking if
                                  // there is one currently pending..
    }
  }

  add( ...handlers ){
    handlers.forEach( handler => {
      if(handler instanceof Function){
        this.callees.unshift( handler );
        this.calleesTime.unshift( this.time );
      }
    });

Bad style this code would not pass a linter without a pile of opt outs. Always add { }

    if(this.callees.length && this.autostart && this.paused) this.paused = false;
  }
  remove( ...handlers ){
    handlers.forEach( handler => {

Why let for a function scope? Incorrect scope declaration can lead to bugs, use var if the variable is function scoped.

      let index = -1;
      while((index = this.callees.lastIndexOf( handler )) >= 0){
        this.callees.splice( index, 1 );
        this.calleesTime.splice( index, 1 );
      }
    });

Missing {}

    if(this.callees.length <= 0) this.paused = true;
  }
  loop( time ){

This try catch would be very annoying during development, completely unacceptable in release code.

You are not responsible for the functions you call and you should not attempt to circumvent any error handling system they have in  place by adding a cover all try catch. If there is a potential error to be thrown by your code then check for it and handle it. Don't use try catch unless you are at the application level.

try {

  if( !this.paused ){

    window.requestAnimationFrame( this.loop.bind(this) );

Is there any particular reason you use let here???

    let delta = time - this.time;
    this.time = time;

The next section is just wrong for way too many reasons. This is an animation controller, the number one enemy of animations is GC and you are just creating dead memory for no reason. Also forEach is SLOW SLOW SLOW just use a standard for loop it is much more efficient.

    this.callees.map(( handler, index ) => {
      let progress = time - this.calleesTime[index];
      return handler( delta, progress ) === false ? handler : true;
    }).forEach(handler => {
      return handler !== true ? this.remove(handler) : 0;
    });

Above code rewrite. The following will not use up memory, and be an order of magnitude quicker.

    var i; // << hoist
    for (i = 0; i < callees.length; i += 1) {
        if (callees[i]( delta, time - this.calleesTime[i] ) === false) {
            this.remove(callees[i]);
            i--; // so you don`t skip a item
        }
    }

  }

} catch(e) {
  this.paused = true;
  console.warn('An error has occurred while running the AnimationFrame.', e);

}


Prime number sieve using ranges::view::ints I wrote a simple prime number generator in C++17.  The function generate_primes(max) generates all prime numbers up to max.  I aimed for maximum portability.  Here's the code:

#include <cstdint>
#include <iostream>
#include <set>
#include <range/v3/view/iota.hpp>

namespace view = ranges::view;

using number_t = std::uint_fast32_t;
using numbers_t = std::set<number_t>;

numbers_t generate_primes(number_t max)
{
    if (max < 2)
        return {};

    numbers_t primes{view::ints(number_t(2), max)};
    for (number_t n = 2; n * n <= max; ++n) {
        if (primes.find(n) != primes.end()) {
            for (number_t m = 2; m <= max / n; ++m)
                primes.erase(n * m);
        }
    }
    return primes;
}

I used the ints utility from the Ranges library to populate primes.  view::ints(number_t(2), max) lazily generates all the integers of type number_t that is greater or equal to 2 and that is less than or equal to max in ascending order.  In C++20, the Ranges library is standardized.

Example usage:

void print(std::ostream& os, const numbers_t& nums)
{
    // convert to unsigned long so that it can be portability printed
    for (unsigned long n : nums)
        os << n << "\n";
}

int main()
{
    print(std::cout, generate_primes(100));
}

Output:

2
3
5
7
11
13
17
19
23
29
31
37
41
43
47
53
59
61
67
71
73
79
83
89
97

It may well be more efficient to compute std::sqrt(max) once upfront than to multiply n * n every time around the loop.

Removing an element by value from the set is O(log n), where n is the set size at the time.  This will likely make this method slower than writing to elements in a fixed-size storage (such as a std::vector that's never resized), and creating the return set from that.  Also, find() is much slower than vector lookup.

I think we can probably test m * n <= max instead of the more expensive m <= max / n, if we can arrange that the multiplication doesn't overflow.  It will be fine if 2 * n < std::numeric_limits<number_t>::max() - max.  Also, no need to start at 2 * n when removing - we can start at n * n instead, as all the lower multiples have already been removed due to their smaller factor(s).

And a tiny improvement to the test code - we can stream a single character '\n' to output rather than the string "\n".


Scala tic-tac-toe no mutable state The following was an exercise for me to try to understand how state can be passed down the stack without immutability.

I'm used to writing java code and this is just a way for me to understand.

I've made no attempt to make the computer play a clever game. It just selects a random move. Player is always X. Player always starts first. Moves are made by entering the 0 based index of the grid

 0 | 1 | 2
---+---+---
 3 | 4 | 5
-----------
 6 | 7 | 8

However I found it difficult to write and find the piece that calculates if there is any winning line very messy to read.

import scala.annotation.tailrec
import scala.io.StdIn
import scala.util.{Success, Failure, Random,Try}

/**
 * A simple expirement in tic tac toe the idea
 * is to try to have something that contains state
 * without having vars.
 */
object ttt extends App {

  object Player extends Enumeration {
    val X, O = Value
  }

  val grid: List[Option[Player.Value]] =
    None :: None :: None ::
      None :: None :: None ::
      None :: None :: None ::
      Nil

  val winningLines = List(0, 1, 2) ::
    List(3, 4, 5) ::
    List(6, 7, 8) ::
    List(0, 3, 6) ::
    List(1, 4, 7) ::
    List(2, 5, 8) ::
    List(1, 4, 8) ::
    List(2, 4, 6) :: Nil
  @tailrec
  def play(next: Player.Value, grid: List[Option[Player.Value]]): String = {
    val sliceHorizontal = (g:List[Option[Player.Value]]) =>
      g.slice(0, 3) ::
      g.slice(3, 6) ::
      g.slice(6, 9) :: Nil

    val toPrint = (p: Option[Player.Value]) => if (p.isEmpty) "   " else " " + p.get.toString() + " "
    val gridPrint = (g: List[List[Option[Player.Value]]]) => g.map(_.map(toPrint).mkString("|")).mkString("\n---+---+---\n")+"\n\n"
    println(gridPrint(sliceHorizontal(grid)))

    val emptyCells = (g: List[Option[Any]]) => g.zipWithIndex.
      filter(_._1.isEmpty).
      map(_._2)

    lazy val randomCell = Random.shuffle(emptyCells(grid)).head

    def playerChoice(g: List[Option[Player.Value]]): Int = {
      print("Please enter selection, ")
      val result = Try(StdIn.readInt());
      result match {
        case Success(v) => if (emptyCells(g).contains(v)) v else playerChoice(g)
        case Failure(_) => playerChoice(g)
      }
    }

    if (emptyCells(grid).isEmpty) {
      "Draw"
    } else {
      val newGrid = if (next == Player.O) {
        grid.updated(randomCell, Some(Player.O))
      } else {
        grid.updated(playerChoice(grid), Some(Player.X))
      }

      def listOfDuplicates[A](x: A, length: Int): List[A] = {
        if (length < 1) Nil
        else x :: listOfDuplicates(x, length - 1)
      }

      def allEqual[A](first: A, seq: List[A]):Boolean = {
        if (seq isEmpty) true
        else if (first != seq.head) false
        else allEqual(first, seq.tail)
      }

      //Okay this is very messy not used to this
      val hasWon= (p: Player.Value) => winningLines.
        foldLeft(false)((i, s) => i || allEqual(Some(p), s.map(newGrid(_))))
      if (hasWon(Player.X)) {
        println(gridPrint(sliceHorizontal(newGrid)))
        "X Wins"
      } else if (hasWon(Player.O)) {
        println(gridPrint(sliceHorizontal(newGrid)))
        "O Wins"
      } else play(if (next == Player.O) Player.X else Player.O, newGrid)
    }
  }

  println(play(Player.X, grid))
}

How can I clean this up and make it more idiomatic? easier to read? Also for tic-tac-toe it doesn't matter much but for a large game if it wasn't tail recursive I guess I couldn't use this strategy for holding state between moves.

I won't comment about your algorithm, as I understand that that's not the focus of this review - suffice it to say that there are better ways of checking winning than comparing against hard-coded winning lines, even in Scala.

Also, Scala has a beautiful and extensive standard library, especially for collections - learn and use it!

In no particular order:

Don't explicitly construct lists - use factory methods

i.e., List.apply(values: T*), a variadic method which can be invoked with syntax sugar as List(value1, value2, ... valueN), as well as similar fill and tabulate.

In fact, fill makes your listOfDuplicates function redundant. There's also a forall method taking a boolean predicate and a list and returning whether all elements of the list satisfy the predicate or not, which makes your allEqual method kind of redundant.
Use appropriate prompts and notification when interacting with the user

For example, when you recurse in playerChoice, you do not let the user know why you're prompting him/her for input again. let him/her know that the position is occupied or invalid before asking him/her again.
Use IndexedSeq for random-access immutable collections

Here I recommend using IndexedSeq instead of Vector specifically to ensure that we type to interfaces - you might want to swap out the implementation for an array later on. This'll also make you update the function signatures.

Why IndexedSeq instead of List? You use updated, a function which accesses elements by index. This is \$O(n)\$ for a List, but amortized \$O(1)\$ for Vector or Array, which follow the IndexedSeq interface.
collect instead of chained filter and map

collect allows you to combine filter and map, essentially allowing you to do pattern-matching in situ. Leads to 1 less intermediate collection (seeing that you aren't using lazy .views) and cleaner code.

Use nested/local functions instead of function literals

In Scala, you can put def within def and so on. This'll allow you to get around the ugly List[Option[Any]] you have for emptyCells and replace it with a proper type parameter. You do this for allEqual already, I don't know why you aren't doing it for everything else (did you copy allEqual` from somewhere else?).
Alias long, frequently used types

Type-alias Player.Value to Player and import it to reduce some visual clutter. Also do the same with List[Option[Player.Value]] by aliasing it to Grid to reduce visual clutter even more.

Use more descriptive variable names

grid for g and value/position for v couldn't hurt.
Proper spacing and indentation

Good enough, but somewhat inconsistent. Put it through an IDE autoformatter, e.g., IntelliJ IDEA's.
Some general ideas for improvement

Incorporate a clean quit mechanism, maybe a q at the prompt?
A help menu where you explain how to play would be nice.
Adding player choice is not that difficult - try it. Just a wee bit of initialization code.

All in all, functional programming is about composing functions together to write programs; immutability and stuff is good to have but it isn't primary. You have some repetition, as in the hasWon check - see if you can get rid of it (Hint - you might want a function which can iterate over the values of the Player enum and check if any of them have won).

Also, this

  def hasWon(p: Player) = winningLines.foldLeft(false)((i, s) => i || allEqual(Some(p), s.map(newGrid(_))))

isn't really ugly. It's a mouthful, yes, but that can be improved with better parameter names (something I'm not doing for you right now - I hope I've indicated how to do it on your own). Don't worry - you'll get used to it. I nowadays find that foldLeft easier to read than a couple nested for loops.

Here's the current code (I might throw in more improvements later):

import scala.annotation.tailrec
import scala.io.StdIn
import scala.util.{Success, Failure, Random,Try}

/**
 * A simple expirement in tic tac toe the idea
 * is to try to have something that contains state
 * without having vars.
 */
object TicTacToe extends App {

  object Player extends Enumeration {
    val X, O = Value
    type Player = Value
  }

  import Player._
  type Grid = Seq[Option[Player]]

  val grid: Grid = IndexedSeq.fill(9)(None)

  val winningLines =
    IndexedSeq(
      IndexedSeq(0, 1, 2),
      IndexedSeq(3, 4, 5),
      IndexedSeq(6, 7, 8),
      IndexedSeq(0, 3, 6),
      IndexedSeq(1, 4, 7),
      IndexedSeq(2, 5, 8),
      IndexedSeq(1, 4, 8),
      IndexedSeq(2, 4, 6))

  @tailrec
  def play(next: Player.Value, grid: Grid): String = {
    def sliceHorizontal(g: Grid) =
      IndexedSeq(
        g.slice(0, 3),
        g.slice(3, 6),
        g.slice(6, 9))

    def playerRepr(p: Option[Player]) = p.map(v => s" $v ").getOrElse("   ")
    def gridRepr(g: Seq[Grid]) = g.map(_.map(playerRepr).mkString("|")).mkString("", "\n---+---+---\n", "\n\n")

    def printGrid(g: Grid) = println(gridRepr(sliceHorizontal(g)))
    printGrid(grid)

    def emptyCells[T](g: Seq[Option[T]]) =
      g.zipWithIndex.collect {
        case (value, index) if value.isEmpty => index
      }

    lazy val randomCell = Random.shuffle(emptyCells(grid)).head

    @tailrec
    def playerChoice(g: Grid): Int = {
      print("Please enter selection: ")
      Try(StdIn.readInt()) match {
        case Success(v) =>
          if (emptyCells(g).contains(v)) v
          else {
          println("Position already occupied, please try a different one")
          playerChoice(g)
        }
        case Failure(_) =>
          println("Invalid Position - try a value between 0 and 8")
          playerChoice(g)
      }
    }

    if (emptyCells(grid).isEmpty) {
      "Draw"
    } else {
      val newGrid = if (next == O) {
        grid.updated(randomCell, Some(O))
      } else {
        grid.updated(playerChoice(grid), Some(X))
      }

      def allEqual[A](first: A, seq: Seq[A]): Boolean = seq.forall(_ == first)

      def hasWon(p: Player) = winningLines.foldLeft(false)((i, s) => i || allEqual(Some(p), s.map(newGrid(_))))

      if (hasWon(X)) {
        printGrid(newGrid)
        "X Wins"
      } else if (hasWon(O)) {
        printGrid(newGrid)
        "O Wins"
      } else {
        play(if (next == O) X else O, newGrid)
      }
    }
  }

  println(play(Player.X, grid))
}


Game Of Life implemented with for-loops and a boolean-array There's a Follow-up: Game Of Life rewritten into two classes, PetriDish and Cell

I wrote an implementation of the Game Of Life using the easiest approach, a bunch of loops and a boolean array. Yet I feel like I missed something very elemental.

The basic implementation is:

Check the old generation with loops
Check each neighbor of each cell...with loops
Set the status in the generation
Copy the new generation over the old one

This seems to have some serious downsides, especially the heavy use of loops which makes me a little bit itchy.

private boolean[] cells; // Will be set at construction
private int width;
private int height;

/**
 * Evolve into the next nextGeneration.
 */
private void doGeneration() {
    boolean[] nextGeneration = new boolean[cells.length];

    for (int x = 1; x < width - 1; x++) {
        for (int y = 1; y < height - 1; y++) {
            int neighbors = 0;

            // Check surrounding cells.
            for (int neighborX = x - 1; neighborX <= x + 1; neighborX++) {
                for (int neighborY = y - 1; neighborY <= y + 1; neighborY++) {
                    if (neighborX != x || neighborY != y) {
                        if (cells[neighborX * width + neighborY]) {
                            neighbors++;
                        }
                    }
                }
            }

            int idx = x * width + y;

            switch (neighbors) {
                case 0:
                case 1:
                    nextGeneration[idx] = false;
                    break;

                case 2:
                    nextGeneration[idx] = cells[idx];
                    break;

                case 3:
                    nextGeneration[idx] = true;
                    break;

                default:
                    nextGeneration[idx] = false;
                    break;
            }
        }
    }

    cells = nextGeneration;
}

Edit: The whole application (Slick dependent for input and drawing) can be found at GitHub.

Edit2: There's a bug in the above code. The index should of course be y * width + x. I stumbled against that yesterday when I tried to handle non-square grids, but I just realized what was wrong some minutes ago while riding the bus.

I'd start it with a Cell and a Grid class and probably a two-dimensional Cell array field in the Grid.  Cell provides type-safety and more readable code while  probably contains only a boolean flag.

Anyway, this implementation can be improved. I'd extract out a List<Integer> getSurroundingCells(int cellIndex) and an int countLiveCells(final List<Integer> cellIndexes) method. A boolean getNextValue(int cellIndex, boolean oldValue, int liveNeighborCount) also could help.


Is this a proper quicksort algorithm in Python? import random

def qsort(qslist):

    small = []
    big = []

    if (len(qslist) < 2):
        return qslist

    elif (len(qslist) > 1):

        pivot = qslist[int(random.randrange(len(qslist)))]

        for x in range(len(qslist)):
            if qslist[x] <= pivot:
                small.append(qslist[x])
            else:
                big.append(qslist[x])
        if(len(big) == 0):
            big.append(pivot)
            small.remove(pivot)

return qsort(small) + qsort(big)

qslist = [9,1,8,2,7,3,6,4,5]
print(qsort(qslist))

Please give me advice while picking up Python.  I'm trying to do so while at the same time doing some basic algorithms.

Have I understood how the quicksort algorithm work?
Please give me some general feedback on the Python code.

Some comments:

Write docstrings to document code
Use if/else instead of if/elif if possible
Use list comprehensions
Use random.choice to get a random element from a non-empty sequence

The code in the original questions with the suggested improvements would be as follows:

import random

def qsort(qslist):
    """"Quicksort implementation."""
    if len(qslist) < 2:
        return qslist

    pivot = random.choice(qslist)
    small = [x for x in qslist if x < pivot]
    equal = [x for x in qslist if x == pivot]
    big = [x for x in qslist if x > pivot]

    return qsort(small) + equal + qsort(big)

qslist = [9,1,8,2,7,3,6,4,5]
print(qsort(qslist))

Edit: If you prefer to avoid traversing the array three times, a partition
function to do that very similar to the one in the original question would be:

def partition(qslist):
    """Partition array using a pivot value."""
    small = []
    equal = []
    big = []

    pivot = random.choice(qslist)
    for x in qslist:
        if x < pivot:
            small.append(x)
        elif x == pivot:
            equal.append(x)
        else:
            big.append(x)

    return small, equal, big


Prolog refactor if/else How do I refactor this mess of if/else conditions so that it's more readable?

    ( Reverse_Path = [[]] -> Child = [c(New_Score,G1,Pos),P]
    ; otherwise -> Child = [c(New_Score,G1,Pos),P|Reverse_Path]
    ),
    (  memberchk(Child,NewAgenda) -> addChildren(Others,Reverse_Path,Current,NewAgenda,Target,Result)

    ;  otherwise ->
        (   NewAgenda=[] -> BrandNewAgenda = [Child|NewAgenda]
        ;   otherwise    ->
            (New_Score =< FP -> BrandNewAgenda = [Child|NewAgenda];
            otherwise  -> append(NewAgenda,[Child],BrandNewAgenda)
            )
        ),

    )).

Generally speaking, you can resolve if/else logic by using multiple clauses for the same predicate, as multiple clauses implicitely represent 'or' or 'case', e.g.:
foo([[]], Child) :-
   Child = [c(New_Score,G1,Pos),P].
foo(Reverse_Path, Child) :-
   Child = [c(New_Score,G1,Pos),P|Reverse_Path].
bar(Reverse_Path, Child, NewAgenda, BrandNewAgenda) :-
   memberchk(Child,NewAgenda),
   addChildren(Others,Reverse_Path,Current,NewAgenda,Target,BrandNewAgenda).
bar(Reverse_Path, Child, [], BrandNewAgenda) :-
   BrandNewAgenda = [Child].
bar(Reverse_Path, Child, NewAgenda, BrandNewAgenda) :-
   New_Score =< FP,
   BrandNewAgenda = [Child|NewAgenda].
bar(Reverse_Path, Child, NewAgenda, BrandNewAgenda) :-
   append(NewAgenda, [Child], BrandNewAgenda).

main(Reverse_Path, BrandNewAgenda) :-
   foo(Reverse_Path, Child),
   bar(Reverse_Path, Child, [], BrandNewAgenda).

This is just a rough sketch, there are a lot of loose ends here like variables Others, Target or Result (I guess you are juggling too many variables anyway), but I hope you get the gist of it:

Decompose into multiple simpler predicates that encode alternatives through multiple clauses.
Bind results to "out" variables in the clause head (like Child in foo/2 or BrandNewAgenda in bar/4).
Compose the smaller predicates in a "main" predicate that glues them togehter.


Better Way to Show and Hide Items in Project I have this code snippet below that I am looking for a way to make smaller if possible.  Any help or suggestions would be appreciated, thanks!

Private Sub ShowAlternateRows(Show As Boolean)
    If Show = True Then
        splitter.Visible = True
        lblAlternateActualPercent.Visible = True
        lblAltEstimated.Visible = True
        lblStaticAltEstimated.Visible = True

        Grid.Cols.Item("Alternate Type").Visible = True
        Grid.Cols.Item("Alternate Start").Visible = True
        Grid.Cols.Item("Alternate End").Visible = True
        Grid.Cols.Item("Alternate Hours").Visible = True
        Grid.Cols.Item("Alternate Burn").Visible = True

        Grid.Cols.Item("Start Estimated").Visible = False
        Grid.Cols.Item("End Estimated").Visible = False
        Grid.Cols.Item("Start Actual").Visible = False
        Grid.Cols.Item("End Actual").Visible = False
        Grid.Cols.Item("Estimated Hours").Visible = False
        Grid.Cols.Item("Actual Hours").Visible = False
        Grid.Cols.Item("Burn Estimated").Visible = False
        Grid.Cols.Item("Object Count").Visible = False
        Grid.Cols.Item("Request Count").Visible = False
        Grid.Cols.Item("Resource Count").Visible = False

        lblStaticActual.Visible = False
        lblActual.Visible = False
        lblActualPercent.Visible = False
        lblStaticEstimated.Visible = False
        lblEstimated.Visible = False
        lblStaticFiltered.Visible = False
        lblFiltered.Visible = False
        lblFilteredPercent.Visible = False

    Else
        splitter.Visible = False
        lblAlternateActualPercent.Visible = False
        lblAltEstimated.Visible = False
        lblStaticAltEstimated.Visible = False

        lblStaticActual.Visible = True
        lblActual.Visible = True
        lblActualPercent.Visible = True
        lblStaticEstimated.Visible = True
        lblEstimated.Visible = True
        lblStaticFiltered.Visible = True
        lblFiltered.Visible = True
        lblFilteredPercent.Visible = True

        Grid.Cols.Item("Start Estimated").Visible = True
        Grid.Cols.Item("End Estimated").Visible = True
        Grid.Cols.Item("Start Actual").Visible = True
        Grid.Cols.Item("End Actual").Visible = True
        Grid.Cols.Item("Estimated Hours").Visible = True
        Grid.Cols.Item("Actual Hours").Visible = True
        Grid.Cols.Item("Burn Estimated").Visible = True
        Grid.Cols.Item("Object Count").Visible = True
        Grid.Cols.Item("Request Count").Visible = True
        Grid.Cols.Item("Resource Count").Visible = True

        Grid.Cols.Item("Alternate Type").Visible = False
        Grid.Cols.Item("Alternate Start").Visible = False
        Grid.Cols.Item("Alternate End").Visible = False
        Grid.Cols.Item("Alternate Hours").Visible = False
        Grid.Cols.Item("Alternate Burn").Visible = False
    End If
End Sub

Assuming OPTION STRICT OFF (since I don't know the type of Grid.Cols.Item):

Private Sub ShowAlternateRows(show As Boolean)
    Dim grpShow =
    {
        splitter, lblAlternateActualPercent,
        lblAltEstimated, lblStaticAltEstimated,
        Grid.Cols.Item("Alternate Type"), Grid.Cols.Item("Alternate Start"),
        Grid.Cols.Item("Alternate End"),  Grid.Cols.Item("Alternate Hours"),
        Grid.Cols.Item("Alternate Burn")
    }
    Dim grpHide =
    {
        Grid.Cols.Item("Start Estimated"), Grid.Cols.Item("End Estimated"),
        Grid.Cols.Item("Start Actual"),    Grid.Cols.Item("End Actual"),
        Grid.Cols.Item("Estimated Hours"), Grid.Cols.Item("Actual Hours"),
        Grid.Cols.Item("Burn Estimated"),  Grid.Cols.Item("Object Count"),
        Grid.Cols.Item("Request Count"),   Grid.Cols.Item("Resource Count"),
        lblStaticActual,  lblActual,
        lblActualPercent, lblStaticEstimated,
        lblEstimated, lblStaticFiltered,
        lblFiltered,  lblFilteredPercent
    }
    For Each ctrl in grpShow
        ctrl.Visible = show
    Next
    For Each ctrl in grpHide
        ctrl.Visible = Not show
    Next
End Sub


Splitting an array into stride by 2 i've written a basic function to split an array into stride by 2:

[z] -> [x],[y]

    func splitArray(Z x:[Float64])->(Real:[Float64],Imag:[Float64])
    {
      guard (x.count % 2 == 0) else {"BOMB:: need even number of elements in array \(#file) \(#line) \(#function)"
      return ([0.0],[0.0])
      }
      var Real:[Float64] = zeroD(npts: Int(x.count/2) )
      var Imag:[Float64] = zeroD(npts: Int(x.count/2) )
      var ida:Int = 0, idb:Int = 0
  for elem in stride(from:0, to: x.count-1, by:2)
  {
      Real[ida] = x[elem]
      ida += 1
  }
  for elem in stride(from:1, to:x.count, by:2)
  {
    Imag[idb] = x[elem]
    idb += 1
  }
      return (Real,Imag)
}

Does anyone have a faster map? Sometimes map is not that fast and just hides the loop?

I'm hoping for solutions which have been designed to crunch big data arrays or like google's big table, although not sparse.  Something like a Metal/GPU pipeline? hyper-threading model? an Atomic que/stack? pulling in an external optimized lib like boost?

The algorithm behind map is unknown, sometimes it's faster than a simple loop sometimes not.  It's not deterministic, generally not a good thing, but it's easy to implement.

I've tried unrolling loops in Xcode flags but seems to make no different with an arrays of size MxN>[500,500].

As others have commented, it's not clear what your performance issue is currently and what your performance target is, but the function could certainly be rewritten to be more concise and use only a single "loop", like this:

func split<T>(array:[T])->(real:[T], imag:[T]) {
    // I didn't include your guard because it wasn't necessary for safety in this implementation, but you could include it here if you wished.
    return array.enumerated().reduce(([T](), [T]())) {
        (result:(real:[T], imag:[T]), input:(index:Int, element:T)) in
        input.index % 2 == 0 ? (result.real + [input.element], result.imag) : (result.real, result.imag + [input.element])
    }
}

This implementation uses reduce to loop through the source array once, and accumulate a tuple of two arrays as a result.  The reduce is use on the enumerated() source array, so that the closure received both the element, and the index of that element in the original array.  This way, by checking the mod 2 of the index, we can see if the element was in an even or an odd position and add it to the correct resulting array accordingly.

Lastly, since we don't really need to know what the specific type of the array's element is for this behavior, this implementation is generic and will split any array of anything into two new arrays: one with all the even-indexed elements, and one with all the odd-indexed elements.


Scale-able API development with macro definitions In a couple of months I'll be part of a large(ish) project, and there's good chances my team will be a lot bigger, with at least 2 programmers (probably 3) and some 3-5, or more, consumers of the API we'll build (we're building a game, and they'll script high level logic using a scripting language we've yet to choose).

Now, last project I was in I used the below mentioned technique with "great" success when generating structures for use in mapping configuration files. Problem is, this will be a first timer for me in building an API with several developers and multiple consumers, and I have no idea how well it will scale.

tl;dr:

Is this system intuitive enough and scale-able enough for someone (you!), without pre-existent knowledge of the system, to pick it up and extend it without much issue?
If not, how should I go about building something that achieves the same but lets me skip all the repetitive cruft?
Can you spot any gotchas I've overlooked?

These examples uses python 2.7 and pybind11 (for binding structures to python). When properly linked against python they're buildable and runnable.

#include <pybind11/pybind11.h>

namespace py = pybind11;

#define PLUGIN PYBIND11_PLUGIN

#define PLUGIN_BEGIN(TYPE, NAME, DESCR)         \
    PLUGIN(TYPE)                                \
    {                                           \
        typedef TYPE Type;                      \
        py::module Module (NAME, DESCR);        \
        py::class_<Type> Binding(Module, #NAME);\

#define PLUGIN_END                              \
        return Module.ptr();                    \
    }                                           \

#define PLUGIN_MAP_FIELD(NAME)                \
    Binding.def_readwrite(#NAME, &Type::NAME);\

#define PLUGIN_MAP_PROPERTY(NAME)                                   \
    Binding.def_property(#NAME, &Type::get##NAME, &Type::set##NAME);\

And then you use it as so:

#include <string>

struct SPerson
{
    unsigned int Age;

    std::string getName( void ) { return m_Name; }
    void setName( const std::string name ) { m_Name = name; }

private:
    std::string m_Name;
};

PLUGIN_BEGIN(SPerson, "Person", "A person descriptor")
    PLUGIN_MAP_FIELD(Age)
    PLUGIN_MAP_PROPERTY(Name)
PLUGIN_END

What you've currently got expands to something like this:

PYBIND11_PLUGIN(SPerson)
{
    typedef SPerson Type;
    py::module Module("Person", "A person descriptor")
    py::class_<SPerson> Binding(Module, "\"Person\"");
    Binding.def_readwrite("Age", &Type::Age);
    Binding.def_property("Name", &Type::getName, &Type::setName);
    return Module.ptr();
}

I think you've got a stray # in front of NAME in one place.

Anyway, is there a technical reason you need SPerson and Person to be different identifiers? That seems like it would increase confusion (two names for the same entity, depending on what language you were writing) rather than decreasing it.

And the string "A person descriptor" is just internal documentation, right? It's not important to the code's semantics, i.e., it's just a comment? If so, it would almost certainly be better to omit it. Remember, you're aiming this code at people who will cut-and-paste it and change just what they need in order to make it work; I foresee a lot of

PLUGIN_BEGIN(SPerson, "Person", "A person descriptor")
PLUGIN_BEGIN(SDog, "Dog", "A dog descriptor")
PLUGIN_BEGIN(SVehicle, "Vehicle", "A vehicledescriptor")
PLUGIN_BEGIN(SWeapon, "Weapan", "A person descriptor")
PLUGIN_BEGIN(SEnemy, "Enemy", "A person descriptor")

in your future, unless you take steps now to eliminate those redundant fields. Remember Murphy's Law: Whatever can go wrong, will go wrong. So if you don't want anything to go wrong, you should try really hard to eliminate any element of the system that can go wrong.

PLUGIN_BEGIN(Person)
    PLUGIN_MAP_FIELD(Age)
    PLUGIN_MAP_PROPERTY(Name)
PLUGIN_END

If your users are likely to show any personal initiative whatsoever — if they're likely to want to understand the tool you give them, instead of just using it passively and/or asking you personally if they have questions — then I would also advise not monkeying with pybind11's names for things. Don't say FIELD if what you mean is READWRITE.  This will help the user-programmer when something inevitably breaks and he has to go google the name of the thing; it'll help because there will be one name (readwrite) instead of two (readwrite and field), which means his googling can be twice as effective in the same amount of time.

This will also help you, the library writer, when it comes time to extend the library. Under your current system, how would a user expose a const data member to pybind11? He can't. You need a new macro to deal with const fields. pybind11 has an existing name for such things (readonly), but you haven't made up your own name for them yet. Do you go with FIELD_READONLY, or CONST_FIELD, or suck it up and go with READONLY even though you didn't originally use READWRITE...? It's a bit of a mess. Whereas, if you make a design decision to stick with pybind11's names for things from the get-go, you'll never have to waste your own time on such questions again; the answer is always "go with what pybind11 called it."

Your macro system limits you to one Python class per Python module. That's okay, right?

Syntactical nitpick: Those trailing backslashes are going to bite you one day. (I don't even know if putting a backslash as the last character of a source file is well-defined!) Instead of

#define FOO \
        bar \
        baz \
/* warning: this blank line is semantically significant */

I'd strongly recommend

#define FOO \
        bar \
        baz


Poker Game: Unit testing different poker hands without violating DRY principles I'm currently in the process of refactoring an old poker game which violates many of the SOLID principles.

I was trying to use the TDD approach to refactoring and I found myself having to hard code a player's hand every time I wrote a unit test for a specific poker hand check.

For example, here are the unit tests for a one pair poker hand check:

    [Test]
    public void OnePair_IsLowPair_ReturnsTrue()
    {
        SuperCard[] testHand =
        {
            new CardClub(Rank.Ace),
            new CardSpade(Rank.Ace),
            new CardDiamond(Rank.Five),
            new CardHeart(Rank.Jack),
            new CardClub(Rank.Three)
        };

        bool result = PokerHandEvaluator.OnePair(testHand);

        Assert.That(result == true);

    }

    [Test]
    public void OnePair_IsLowerMiddlePair_ReturnsTrue()
    {
        SuperCard[] testHand =
        {
            new CardClub(Rank.Eight),
            new CardSpade(Rank.Ace),
            new CardDiamond(Rank.Ace),
            new CardHeart(Rank.Jack),
            new CardClub(Rank.Three)
        };

        bool result = PokerHandEvaluator.OnePair(testHand);

        Assert.That(result == true);

    }

    [Test]
    public void OnePair_IsHigherMiddlePair_ReturnsTrue()
    {
        SuperCard[] testHand =
        {
            new CardClub(Rank.Jack),
            new CardSpade(Rank.Queen),
            new CardDiamond(Rank.Ace),
            new CardHeart(Rank.Ace),
            new CardClub(Rank.Three)
        };

        bool result = PokerHandEvaluator.OnePair(testHand);

        Assert.That(result == true);

    }

    [Test]
    public void OnePair_IsHighPair_ReturnsTrue()
    {
        SuperCard[] testHand =
        {
            new CardClub(Rank.Jack),
            new CardSpade(Rank.Deuce),
            new CardDiamond(Rank.Six),
            new CardHeart(Rank.Ace),
            new CardClub(Rank.Ace)
        };

        bool result = PokerHandEvaluator.OnePair(testHand);

        Assert.That(result == true);

    }

This is what the class diagram (at the project's current stage) looks like:

My question is how I should go about extracting a method (or methods) out of the testHand initialization.

One thing that would help with reducing duplication is to use parameterized tests.

Additionally, you might want to have a enum with the different hand types instead of - or in addition to - having one method for each hand type, such as your current PokerHandEvaluator.OnePair.


Heterogenous persistent list in Rust I have implemented a basic, but seemingly working, persistent (as in immutable), heterogenous list type in Rust.

Here's the code:

#[derive(Debug)]
pub enum ConsCell<'a> {
    Char(char),
    Number(u64),
    String(&'a str),
}

pub struct PersistentList<'a, T: 'a> {
    first: Option<&'a T>,
    rest: Option<&'a PersistentList<'a, T>>,
    count: u64,
}

impl<'a, T> PersistentList<'a, T> {
    pub fn new() -> PersistentList<'a, T> {
        PersistentList {
            first: None,
            rest: None,
            count: 0,
        }
    }

    pub fn first(&self) -> &T {
        self.first.unwrap()
    }

    pub fn next(&self) -> Option<&PersistentList<T>> {
        if self.count == 1 {
            return None;
        }

        self.rest
    }

    pub fn cons(&'a self, x: &'a T) -> PersistentList<'a, T> {
        PersistentList {
            first: Some(x),
            rest: Some(&self),
            count: self.count + 1
        }
    }

    pub fn count(&self) -> u64 {
        self.count
    }
}

This can be used like so:

fn main() {
    let list = PersistentList::new();
    let cell = &ConsCell::Number(42);
    let list = list.cons(cell);
    let cell = &ConsCell::String(&"foo");
    let list = list.cons(cell);
    match list.first() {
        ConsCell::Char(c) => println!("{}", c),
        ConsCell::Number(n) => println!("{}", n),
        ConsCell::String(s) => println!("{}", s),
    }
}

The ergonomics of this are not fantastic, however it does seem to work. Is there a more idiomatic way of doing something similar with Rust?

I asked about this in the Rust beginner's IRC channel and someone helpfully pointed out that another approach might be to use trait objects and Box.

By way of illustration:

use std::fmt::Display;

pub trait Item: Display {}

impl Item for String {}
impl Item for bool {}
impl Item for char {}
impl Item for f64 {}
impl Item for u64 {}

pub struct PersistentList<T = Box<Item>> {
    first: Option<T>,
    rest: Option<Box<PersistentList<T>>>,
    count: usize,
}

impl<T> PersistentList<T> {
    pub fn new() -> PersistentList<T> {
        PersistentList {
            first: None,
            rest: None,
            count: 0,
        }
    }

    pub fn first(&self) -> Option<&T> {
        self.first.as_ref()
    }

    pub fn next(self) -> Option<Box<PersistentList<T>>> {
        self.rest
    }

    pub fn cons(self, item: T) -> PersistentList<T> {
        let count = &self.count + 1;
        PersistentList {
            first: Some(item),
            rest: Some(Box::new(self)),
            count,
        }
    }

    pub fn count(&self) -> usize {
        self.count
    }
}

The advantage here is that we can simply implement Item for any given type and our collection will be capable of handling it. Here's a Playground link.

Now I think it's only fair to leave this question open for a while: I'm new to Rust and still learning and I'm unsure of the best practices and idioms--that said, this seems to be a pretty decent approach.


Measuring executing time I've written a small piece of code to measure execution time. As this is my first attempt at something meaningful, I would really like some input on ways to improve my code.

Timer.h

/**
    Purpose: Measuring execution time using chrono.

    @author Rick Regeling
    @version 1.0 2017/01/05
*/

#ifndef TIMER_H
#define TIMER_H

#include <chrono>
#include <iostream>
#include <string>
#include <map>

namespace library {

    // Default unit: std::chrono::milliseconds
    template<typename Unit = std::chrono::milliseconds>
    class Timer {
    public:
        using Clock = std::conditional_t<std::chrono::high_resolution_clock::is_steady,
            std::chrono::high_resolution_clock,
            std::chrono::steady_clock>;

        Timer() = default;
        Timer(const std::string& name) : m_name{ std::move(name) } {}
        ~Timer() {
            m_end = Clock::now();
            output();
        }

        // Disabled operations:
        Timer(const Timer&) = delete;
        Timer(Timer&&) = delete;
        Timer& operator=(const Timer&) = delete;
        Timer& operator=(Timer&&) = delete;
    private:
        /**
            Build a map<const char*, const char*> and find the unit.
            @return The unit name.
        */
        const char* unit_name() {
            const std::map<const char* const, const char*> units{
                { typeid(std::chrono::hours).name(),"h" },
                { typeid(std::chrono::minutes).name(), "m" },
                { typeid(std::chrono::seconds).name(), "s" },
                { typeid(std::chrono::nanoseconds).name(), "ns" },
                { typeid(std::chrono::microseconds).name(), "mms" },
                { typeid(std::chrono::milliseconds).name(), "ms" } };
            auto result = units.find(typeid(Unit).name());
            return result->second;
        }

        /**
            Output the information.
        */
        void output() {
            std::cout << "Elapsed time";
            // Determine if a name is used:
            std::cout << (m_name == "" ? ": " : " (" + m_name + "): ");
            // Calculate the total time and get the unit:
            std::cout << (std::chrono::duration_cast<Unit>
                (m_end - m_start).count()) << unit_name() << '\n';
        }

        // Members:
        const Clock::time_point m_start = Clock::now();
        Clock::time_point m_end{};
        const std::string m_name{};
    };
}
#endif

Example:

#include "Timer.h"
#include <vector>
#include <numeric>

void foo() {
    std::vector<int> v(1'000'000, 42);
    std::accumulate(std::begin(v), std::end(v), 0);
}

int main() {
    // default behavior:
    {
        library::Timer<> timer;
        foo();
    }

    // different unit + description:
    {
        library::Timer<std::chrono::nanoseconds> ex_timer("foo");
        foo();
    }
    return EXIT_SUCCESS;
}

Usage:

Famous Scott Meyers' words: "Make interfaces easy to use correctly and hard to use incorrectly". It should be kept in mind.

It is really cumbersome to use. Users have to create different scopes and they don't have any control over what's being output and where it's being output.

Also, it seems like the class is somewhat one shot, so users can't make multiple runs. Usually people make considerable amount of runs to normalize the numbers and diminish the noise. The interface could emphasize that.

It is needed to keep in mind that the responsibility of this class is to measure the timings. Don't do anything with it, just provide a way to read it.

Alternative solution:

template<typename Func>
class session {
    Func function;
    std::vector<std::chrono::duration<double>> timings;
public:
    session(const Func& f) :
            function(f)
    {}

    session(Func&& f) :
            function(std::move(f)) {}

    template<class ... Args>
    void measure(Args &&... args) {
        auto start = std::chrono::high_resolution_clock::now();
        function(std::forward<Args>(args)...);
        auto end = std::chrono::high_resolution_clock::now();

        timings.push_back(end - start);
    }

    //writes into the range until either exhausts it or timings vector is exhausted
    template<typename Unit, typename OutputIt>
    void get_as(OutputIt first, OutputIt last) {
        auto it = timings.begin();
        for (; it != timings.end() && first != last;
               ++first, ++it) {
            *first = std::chrono::duration_cast<Unit>(*it);
        }
    }
};

As you can see, single measure() call doesn't return the time, but rather get_as() is used to get all of the measurements at once, and throw it into some function that computes average and generally does processing of the collected data. The interface does little dictating on what users should do, and provides enough flexibility. In C++17, there even won't be a need to write

session<T> s(t);

it will be just

session s(t);

If it is so wanted, you can write freestanding measure() function to get timings of one function call.

What is done right:

The code is really generic, which is nice for portability reasons.

Rule of five nailed down.

Bad stuff:

(m_name == "" ? ": " : " (" + m_name + "): ");

This line is like calling for a challenge "Try to understand me!".

 m_end = Clock::now();
 output();

My opinion is that if you don't initialize a member variable during construction, then the member variable is a candidate for local variable and being passed by reference/const reference.

Don't postpone to runtime:

Get the string for units right in the beginning using the fact that units are aliases for std::ratio<>s.


Parsing a list of single numbers and number ranges I have input of a string containing a single number (like: \$3\$) or a range (like: \$1-5\$). Sample input, all together, looks like: "1-5,3,15-16", and sample output for that input looks like "1,2,3,4,5,15". Output doesn't need to be sorted.

I built something to parse this, but it's ugly. How can I improve this?

from itertools import chain
def giveRange(numString:str):
    z=numString.split("-")
    if(len(z)==1):
        return [int(z[0])]
    elif(len(z)==2):
        return list(range(int(z[0]),int(z[1])+1))
    else:
        raise IndexError("TOO MANY VALS!")

def unpackNums(numString:str):
    rList=[]
    rList.extend(set(chain(*map(giveRange,numString.split(",")))))
    return rList
unpackNums("1-2,30-50,1-10")

Since the whole exercise is a string-transformation problem, I suggest performing it using a regex substitution.

import re

def expand_ranges(s):
    return re.sub(
        r'(\d+)-(\d+)',
        lambda match: ','.join(
            str(i) for i in range(
                int(match.group(1)),
                int(match.group(2)) + 1
            )
        ),
        s
    )

I think that expand_ranges would be a more descriptive name than unpackNums.


Creating a matrix from a dyadic product Ax, Ay, Az: [N-by-N]
B=AA (dyadic product)

means

B(i,j)= [Ax(i,j);Ay(i,j);Az(i,j)]*[Ax(i,j) Ay(i,j) Az(i,j)]

B(I,j): a 3x3 matrix.

One way to construct B is:

N=2;
Ax=rand(N); Ay=rand(N); Az=rand(N);
t=1;
F=zeros(3,3,N^2);
for  i=1:N
    for j=1:N
        F(:,:,t)= [Ax(i,j);Ay(i,j);Az(i,j)]*[Ax(i,j) Ay(i,j) Az(i,j)];
        t=t+1;      %# t is just a counter
    end
end
%# then we can write
B = mat2cell(F,3,3,ones(N^2,1));
B = reshape(B,N,N)';
B = cell2mat(B);

Is there a faster way than this, especially when N is large?

There is a problem with vector multiplication in the second loop. You should transpose the second vector before doing multiplication.

F(:,:,t)= [Ax(i,j);Ay(i,j);Az(i,j)]*[Ax(i,j) Ay(i,j) Az(i,j)]';

One of the ways to speed things up is to apply vectorization instead of two for loops, such as with Dyadics.


Find every 6 hours from now I need to find a set of hours, starting from now, every 6 hours.

So if now is midnight, I want [ 0, 6, 12, 18 ].
If now is 15h (3pm), I want [ 15, 21, 3, 9 ]

For now I have brute-forced it:

var now = new Date();
var hours = [];
var h = now.getHours();
// add current hour (tomorrow)
hours.push(h);

// to add 6 hours
var inXh = function(toAdd) {
    var inh = h + toAdd;
    if (inh >= 24) { inh = inh - 24; }
    return inh;
};

// add in 6 hours
hours.push(inXh(6));
// add in 12 hours
hours.push(inXh(12));
// add in 18 hours
hours.push(inXh(18));

It's incredibly ugly and verbose, and I'm sure there is a much more clever way to do it, but right now I'm too tired to calculate modulos.

You are basically calculating the modulo manually by doing

if (inh >= 24) { inh = inh - 24; }

but JavaScript can do it for you, so the inXh() function could be written like this:

var inXh = function(toAdd) {
    return (h + toAdd * 6) % 24;
}

With that, after having declared

var h = new Date().getHours();

you have (at least) two options, choose the one you feel more elegant.

OPTION 1: using for:

var hours = [];
for (var i = 0; i < 4; i++) hours.push(inXh(i));

OPTION 2 (nicer in my opinion): using Array.map():

var hours = [0, 1, 2, 3].map(inXh);

which all together in one line would be

var hours = [0, 1, 2, 3].map(function(toAdd) { return (h + toAdd * 6) % 24; });


WPF - Run an async Task with DispatcherTimer I have a small application that checks some system status and display it details.
I have the following code which is working. But I want to make sure it's optimal:

public partial class MainWindow : Window
{

    // Dictionary to hold info
    Dictionary<string, int> dataHolder = new Dictionary<string, string>
    {
        {"status_code", "" },
        {"memory", "" },
        {"process", "" }
    };

    public MainWindow()
    {
        InitializeComponent();
    }

    private void StartBtn_Click(object sender, RoutedEventArgs e)
    {
        StartBtn.IsEnabled = false;

        System.Windows.Threading.DispatcherTimer dispatcherTimer = new System.Windows.Threading.DispatcherTimer();
        dispatcherTimer.Tick += new EventHandler(updateUI);
        dispatcherTimer.Interval = new TimeSpan(0, 5, 0);
        dispatcherTimer.Start();
    }

    private async void updateUI(object source, EventArgs e)
    {
        await loadInfo();
        StartBtn.IsEnabled = true;

        foreach(KeyValuePair<string, string> entry in dataHolder)
        {
            // Display in the UI
        }
    }

    private async Task loadInfo()
    {
        int page = 1;
        var keys = new List<string>(dataHolder.Keys);
        foreach (string key in keys)
        {
            dataHolder[key] = "";
            //await asyncCall(key, page);
        }

        // Better asynchronously and in parallel
        await Task.WhenAll(keys.Select(key => asyncCall(key, page)));
    }

    private async Task asyncCall(string key, int page)
    {
        var htClient = new HttpClient();
        // code to retrieve data...
    }
}

So as I said before, this code is working but I read somewhere that having an async void method is not good practice as in private async void updateUI(object source, EventArgs e). I could change it to private async Task updateUI(object source, EventArgs e) but then I would not be able to do dispatcherTimer.Tick += new EventHandler(await updateUI);

Also how to start counting (Timer) only after the previous process is done?

Thank you

EDIT: I am using the DispatcherTimer because I want the application to retrieve the status information and update the UI every 5 minutes.

Async void should be avoided, but they are acceptable when in an EventHandler so it's fine.

Why are you copying the keys of the dictionary into a list? Do you want a snapshot at a random point in time?

If you have potentially race conditions, I would change the Dictionary to a ConcurrentDictionary.

public partial class MainWindow : Window
{

    // Dictionary to hold info
    private readonly Dictionary<string, string> _dataHolder = new Dictionary<string, string>
    {
        {"status_code", "" },
        {"memory", "" },
        {"process", "" }
    };

    public MainWindow()
    {
        InitializeComponent();
    }

    private async void StartBtn_Click(object sender, RoutedEventArgs e)
    {
        StartBtn.IsEnabled = false;

        while (true) // Your exit condition instead of the true.
        {
            await LoadInfo();

            foreach (var entry in _dataHolder)
            {
                // Display in the UI
            }

            await Task.Delay(TimeSpan.FromMinutes(5));

            //StartBtn.IsEnabled = true;
        }
    }

    private async Task LoadInfo()
    {
        const int page = 1;

        var keys = new List<string>(_dataHolder.Keys);

        foreach (var key in keys)
        {
            _dataHolder[key] = "";
            //await asyncCall(key, page);
        }

        // Better asynchronously and in parallel
        await Task.WhenAll(keys.Select(key => AsyncCall(key, page)));
    }

    private static async Task AsyncCall(string key, int page)
    {
        var htClient = new HttpClient();
        // code to retrieve data...
    }


Concurrent Queue with enqueue and dequeue using Stack as underlying data structure This is a mix of data structure and multi threading concept based question. (This is only for understanding and learning purpose) The language for the solution : Java

A Queue with FIFO behavior needs to be implemented by having underlying data structure as Stack.
Enqueue and Dequeue need to be parallel and need not block each other, i.e. enqueue thread must not wait for dequeue thread.

During my search (java based solutions) I could only find the blocking version of this problem where enqueue and dequeue are blocking for each other.

Below is my attempt for this, please correct me if I am wrong or in case I missed something.

Two Stacks : addStack and removeStack with individual Lock Objects addLock and removeLock
Enqueue operation : enqueue inside synchronized block on addLock, once item added, invoke notifyAll on addLock
Dequeue operation : If removeStack is empty then addStack is checked if it is empty , if not then all the elements from addStack are popped and pushed to removeStack
pop the element from removeStack and return

Below is the java code :

import java.util.Stack;

/**
 * Queue Implementation with Stack as underlying data-structure and with
 * parallel (almost) enqueue and dequeue
 *
 * @author krishna_k
 *
 */
public class Queue {

    private Stack<Integer> addStack = new Stack<>();
    private Stack<Integer> removeStack = new Stack<>();
    private final Object addLock = new Object();
    private final Object removeLock = new Object();

    public void enqueue(int item) {
        synchronized (addLock) {
            addStack.push(item);
            addLock.notifyAll();
        }
    }

    public int dequeue(){
        int value = 0;
        synchronized (removeLock) {
            if (removeStack.isEmpty()) {
                try {
                    synchronized (addLock) {
                        while (addStack.isEmpty()) {
                            addLock.wait();
                        }
                        while (!addStack.isEmpty()) {
                            removeStack.push(addStack.pop());
                        }
                    }
                }catch(InterruptedException e) {
                    //code to log the exception e
                }
            }
            value = removeStack.pop();
        }
        return value;
    }
}

Let's start by talking about the requirements.

Implement a thread-safe queue with an underlying stack. This is pretty basic, and has many ways to implement. However, the catch here is Enqueue and Dequeue need to be parallel. This is actually problematic, because as long as a lock is used, blocking will occur. So one can either not use locks, or try to limit their affects.

Your Implementation

Let's talk about your implementation. The first clear thing is that you are using locks, which means blocking is likely.

You have 2 locks, a good thought to try and separate the methods. You also have 2 stacks, each corresponding to a lock. Again, good thought, a single lock must be responsible for all the usage of a specific data source.

The usage of  the Stack class however, is a problem. Not only is Stack a outdated collection class, you must also be aware that it extends Vector and each method in it is synchronized. Luckily, as we will see, most of this doesn't have much functionality impact, but rather more of a performance issue. I would recommend using a Dequeue instead.

Another clear part is that it is an int only implementation. In reality, implementing it generically isn't that different or complicated, since an underlying data structure that supports generics is used.

Enqueue

Your enqueue is quite basic. Take the addition lock, add an element, notify about this addition, and release the lock.

The notifying part, tells us that there is waiting somewhere. I'm assuming this is a blocking queue then. This is both not defined in the requirements, and will cause blocking between the threads. Although, the use of notifyAll may cause a problem.

Otherwise, this is quite straightforward. However, it is already obvious that for the dequeue to work, addStack must be accessed, and thus addLock must be touched. This already tells us that blocking will occur.

Dequeue

Now comes the complicated part.

You start by entering a synchronized block of removeLock, this is quite expected, given how the stacks require usage of the locks.

You then check if the removeStack is empty or not. If it isn't you simply pop and return that value. This might seem like a basic example of a dequeue implementation, but that is not true. Because values are not added directly into removeStack, so it being empty means nothing about the actual queue.

Now, if removeStack is empty... then comes the problematic part. The moment addLock is used, we know blocking between the two method will occur.

If addStack is empty, you wait on addLock until it is notified. This is a blocking queue operation. Which again, is not specified as needed. A non-blocking queue operation would be simply to throw some exception indicating that no value is available. Let's return to the use of notifyAll. If multiple dequeue calls are done, all of them will reach the wait, since the locks are not actually taken while waiting. When enqueue is called and notifyAll is done, only one element was added, however all the waiting threads will wake up, and compete on getting the lone element. Perhaps it is best to just use notify.

if addStack is not empty, you copy the values from it to removeStack, and then pop from it and return a value. This still causes blocking between enqueue and dequeue. Luckily, if enqueue runs longer, the blocking will be decreased, since the values will already be in removeStack.

Catching of InterruptedException here is not really a good idea. When you have a blocking method (method that waits), throwing an InterruptedException is important, since this exception is thrown when the thread is interrupted and the user likely wants to stop the operations running.

Summary

Although it has some good logic behind it, in reality this implementation both doesn't accomplish the full requirements, and adds some complications. However, because stack is LIFO, the complexity in dequeue cannot be avoided. Luckily, this blocking is actually pretty short. So it might be even negligible enough to not care about it.

If enqueue is used more than dequeue, the blocking in dequeue will be longer for the first call, but because removeStack stores stuff, subsequent runs may be faster. If dequeue is used more than enqueue, dequeue will simply be blocked most of the time, because it is waiting.

Unfortunately using stack is not a good option for implementing a queue, because the 2 are completely reversed. There are more ways to implement, however they will all be blocking. So this really depends on how each method is used in relation to the other (frequency of calls, amount of read/write threads).

Snapshot on addStack

An example for another implementation is to use a single stack, and limit the holding of the lock by taking a snapshot of it:

private Stack<Integer> stack = new Stack<>();

public int dequeue() {
    Stack<Integer> reverse = new Stack<>();
    Stack<Integer> copy;
    synchronized(lock) {
        copy = new Stack<>(stack);
    }

    while (!copy.isEmpty()) {
        reverse.push(copy.pop());
    }

    if (reverse.isEmpty()) {
        throw new NoSuchElementException();
    }

    return reverse.pop();
}

Of course, if stack has a lot of elements, the synchronized block may take some time. However it is just a complexity of o(n).

Not using a stack

Of course not using a stack is always an option. There are many other data structures which can be used to implement a queue. However, the real problem is Enqueue and Dequeue need to be parallel.

There are 2 ways to safely deal with data in a concurrent environment:

Locks
Atomic operations

Locks, inherently can cause blocking. However, atomic operations, do not. So a true non-blocking implementation lays in using atomic operation.

Lock-free queue

The concept of a lock-free queue is to implement a queue, which is safe for use in concurrent environments, without using locks, which only leaves us with atomic operations for use.

This is actually well known, but difficult to implement. Depending on the amount of read/write threads, the difficulty can increase.

When working with a single writer thread, and a single reader thread it is actually quite simple.


Add a number to a number to be prime number in python The program is to get the nearest prime number to a number while adding a number. I solved the problem but I wish to optimize the code.
import time
def isprime(a ,d):
    b=a
    c=0
    f=b**(0.5)
    for i in range(1,int(f)+1):
        if a%i==0 and c<=1:
            c+=1
    if c==1:
        return d.append(b)
    else:
        b=a+1
        isprime(b,d)

start=time.time()
b=[89, 54,36, 74, 44, 19, 12]   # Input
d=[]
for i in b:
    isprime(i,d)     #function call
print(d)             #output is [89, 59, 37, 79, 47, 19, 13]
stop=time.time()
print(stop-start)    #0.0001347064971923828 Seconds

Help with Code optimization. I'm just a beginner I know code is of lower. Help to learn bit of coding.

To be cheeky, the ultimate optimization is "don't use Python for numerics", and the next-best optimization is "use a library rather than writing this yourself".
It's worth having a read through https://stackoverflow.com/questions/4114167/checking-if-a-number-is-a-prime-number-in-python
Also consider calling into https://docs.sympy.org/latest/modules/ntheory.html#sympy.ntheory.primetest.isprime .
Otherwise:
Roots
I do not trust Python to do the right thing here:
b**(0.5)

Just call isqrt instead, which will be more likely to get an optimized implementation. This is confirmed here: https://stackoverflow.com/questions/327002/which-is-faster-in-python-x-5-or-math-sqrtx
Variable names
You need to get out of the habit of single-letter variable names. I have no idea what b means other than inferring based on context.
Time measurement
Use timeit. Your current implementation only has a single iteration for each of the input values and is thus extremely vulnerable to CPU thread-switching noise, etc.


Simple 3…2…1 countdown in iOS app I would like to display a 3...2...1 countdown on my iOS app (before to take a picture) but I didn't find an elegant solution. I don't want to make a Countdown app with an NSTimer. For me the important is to keep all code in the same method, use a system like Block.

self.infoLabel.text   = [NSString stringWithFormat:@"3"];
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(1 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{

self.infoLabel.text = [NSString stringWithFormat:@"2"];
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(1 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{
    self.infoLabel.text = [NSString stringWithFormat:@"1"];

    dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(1 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{
        self.infoLabel.hidden = YES;

            // DO WHAT I WANT AFTER 3 SECs
        });
    });
});
});

Do you have a better solution? t works okay, but it's not really elegant link up dispatch_after like I did.

The problem with any sort of timer in which each "tick" is a measurement from the previous "tick" is that in software, these "ticks" are not guaranteed to be exact.  This means that if your first tick is off by a tenth of a second, every remaining tick will be off by that amount or more.  Your second tick is scheduled based on your first tick.  This means the actual amount of total time passed is in exact, and the more ticks you have, the more error prone your total time is going to be.

So, for a start, I want to show you what a countdown object class might look like.  Then I'll walk through how we'd use this.

CRCountdown.h

typedef void (^CRCountdownCompletion)(void);

@interface CRCountdown : NSObject

- (void)startCountdownWithInterval:(NSTimeInterval)interval
                             ticks:(NSUInteger)ticks
                        completion:(CRCountdownCompletion)completion;

@property (readonly) NSUInteger ticksRemaining;
@property (readonly) NSTimeInterval interval;

@end

CRCountdown.m

@interface CRCountdown()

@property NSTimer *timer;
@property (readwrite) NSTimeInterval interval;
@property (copy) CRCountdownCompletion completion;

@end

@implementation CRCountdown

- (void)startCountdownWithInterval:(NSTimeInterval)interval ticks:(NSUInteger)ticks completion:(CRCountdownCompletion)completion {
    self.completion = completion;
    self.interval = interval;
    self.timer = [NSTimer scheduledTimerWithTimeInterval:(interval * ticks) target:self selector:@selector(countdownComplete:) userInfo:nil repeats:NO];
}

- (void)stopCountdown {
    [self.timer invalidate];
}

- (NSUInteger)ticksRemaining {
    if (self.timer.isValid) {
        NSTimeInterval timeRemaining = [self.timer.fireDate timeIntervalSinceDate:[NSDate date]];
        return timeRemaining / self.interval;
    } else {
        return 0;
    }
}

- (void)countdownComplete:(NSTimer *)timer {
    if (self.completion) {
        self.completion();
    }
}

@end

Now, this will guarantee that our completion event happens at as accurate a time as possible.  We still can't guarantee that it will happen precisely at a specific moment, but the margin of error is drastically reduced by eliminating the possibility of compounding the error for each one of our "update ticks".

Instead, we set up a repeating NSTimer that's in charge of calling some code to update the UI.  This UI update code can look at ticksRemaining (and if necessary, multiply this by interval) to determine what it should update the UI to represent.

It's not even necessary to pass anything for the completion argument, as our interested object will already be polling ticksRemaining, so whenever this returns 0, we know that the timer is complete (complete-ish anyway).

But we can go one step further with our Countdown object and provide a means for including an update block that is fired on each tick, if we want.  For this, however, we're going to want two NSTimer's ticking away.

First, in our header, let's add another block typedef:

typedef void (^CRCountdownUpdate)(NSUInteger);

We also need to change our startCountdown... method to take an argument of this type:

- (void)startCountdownWithInterval:(NSTimerInterval)interval
                             ticks:(NSUInteger)ticks
                            update:(CRCountdownUpdate)update
                        completion:(CRCountdownCompletion)completion;

This means adding two more properties in our .m, an updateTimer (NSTimer *) and a CRCountdownUpdate:

@property NSTimer *updateTimer;
@property (copy) CRCountdownUpdate update;

We need to modify our countdownComplete: method so it will invalidate the updateTimer (so update never fires after completion):

- (void)countdownComplete:(NSTimer *)timer {
    [self.updateTimer invalidate];
    if (self.completion) {
        self.completion();
    }
}

And add an update method to handle the update ticks:

- (void)countdownUpdate:(NSTimer *)timer {
    if (self.update) {
        self.update(self.ticksRemaining);
    }
}

The final step is modifying our startCountdown... method to match how we updated the header and to take care of setting up the update timer:

- (void)startCountdownWithInterval:(NSTimeInterval)interval ticks:(NSUInteger)ticks completion:(CRCountdownCompletion)completion update:(CRCountdownUpdate)update {
    self.completion = completion;
    self.update = update;
    self.interval = interval;
    self.timer = [NSTimer scheduledTimerWithTimeInterval:(interval * ticks) target:self selector:@selector(countdownComplete:) userInfo:nil repeats:NO];
    if (self.update) {
        self.updateTimer = [NSTimer scheduledTimerWithTimeInterval:interval target:self selector:@selector(countdownUpdate:) userInfo:nil repeats:YES];
    }
}

Now, using this object is as simple as this:

CRCountdownUpdate update = ^(NSUInteger ticks) {
    self.infoLabel.text = [NSString stringWithFormat:@"%lu", (unsigned long)ticks];
};

CRCountdownCompletion completion = ^{
    self.infoLabel.hidden = YES;
    // do whatever else you want
};

CRCountdown *countdown = [[CRCountdown alloc] init];
[countdown startCountdownWithInterval:1.0
                                ticks:3
                               update:update
                           completion:completion];

Final version of the class I'm suggesting looks like this:

CRCountdown.h

typedef void (^CRCountdownCompletion)(void);
typedef void (^CRCountdownUpdate)(NSUInteger);

@interface CRCountdown : NSObject

- (void)startCountdownWithInterval:(NSTimeInterval)interval ticks:(NSUInteger)ticks completion:(CRCountdownCompletion)completion update:(CRCountdownUpdate)update;
@property (readonly) NSUInteger ticksRemaining;
@property (readonly) NSTimeInterval interval;

@end

CRCountdown.m

@interface CRCountdown()

@property NSTimer *timer;
@property NSTimer *updateTimer;
@property (readwrite) NSTimeInterval interval;
@property (copy) CRCountdownCompletion completion;
@property (copy) CRCountdownUpdate update;

@end

@implementation CRCountdown

- (void)startCountdownWithInterval:(NSTimeInterval)interval ticks:(NSUInteger)ticks completion:(CRCountdownCompletion)completion update:(CRCountdownUpdate)update {
    self.completion = completion;
    self.update = update;
    self.interval = interval;
    self.timer = [NSTimer scheduledTimerWithTimeInterval:(interval * ticks) target:self selector:@selector(countdownComplete:) userInfo:nil repeats:NO];
    if (self.update) {
        self.updateTimer = [NSTimer scheduledTimerWithTimeInterval:interval target:self selector:@selector(countdownUpdate:) userInfo:nil repeats:YES];
    }
}

- (void)stopCountdown {
    [self.updateTimer invalidate];
    [self.timer invalidate];
}

- (NSUInteger)ticksRemaining {
    if (self.timer.isValid) {
        NSTimeInterval timeRemaining = [self.timer.fireDate timeIntervalSinceDate:[NSDate date]];
        return timeRemaining / self.interval;
    } else {
        return 0;
    }
}

- (void)countdownUpdate:(NSTimer *)timer {
    if (self.update) {
        self.update(self.ticksRemaining);
        NSString *s = [NSString stringWithFormat:@"%lu", (unsigned long)self.ticksRemaining];
    }
}

- (void)countdownComplete:(NSTimer *)timer {
    [self.updateTimer invalidate];
    if (self.completion) {
        self.completion();
    }

    self.update = nil;
    self.completion = nil;
}

@end

This of course, could also be done with delegation rather than blocks, which might be an appropriate approach.

I've created a github repo with both the block-based and delegation-based approaches.  It can be found here.


Sorting email address according to occurrences of each domain My code:

import re
mbox = open('mailbox.txt')
ndict = {}
for line in mbox:
    domain = re.findall('From [^ ].*@([^ ]*)', line)
    if domain:
        if domain[0] in ndict:
            ndict[domain[0]] += 1
        else:
            ndict[domain[0]] = 1

print_list = [print(i) for i in sorted([(i, ndict[i]) for i in
          list(ndict.keys())], key=lambda x: x[1], reverse=True)]

I don't like how I create a dictionary and then immediately convert it into a list (which I'm doing in order to sort it), as it seems un-Pythonic to me. Is there a more Pythonic way to do this without using dictionaries?

i.e. Can you do this by making a list and appending tuples of the domains and number of occurrences?

I suggest you get a linter such as Prospector or flake8, this can tell if your code is un-Pythonic. Some people prefer hinters like black.

Your code doesn't conform to PEP 8 which is the standard when it comes to Python. Your comprehension is hard to read because it doesn't conform to best practices.
I'd recommend moving your code into a main function and use an if __name__ == '__main__': guard. This reduces global pollution and prevents your code from running accidentally.
When you see something like:

if key in my_dict:
    my_dict[key] += value
else:
    my_dict[key] = default + value

Then you should probably use dict.get which can get the following code:

my_dict[key] = my_dict.get(key, default) + value

In this case you can add more sugar by using collections.defaultdict, as it will default missing keys to 0.

import collections

my_dict = collections.defaultdict(int)
my_dict[key] += 1

Don't use comprehensions with side effects, they're hard to understand and are better expressed as standard for loops. This is as the list you're making is absolutely pointless.
You can use dict.items() rather than your comprehension with dict.keys().

import re
import collections

def main():
    with open('mailbox.txt') as mbox:
        ndict = collections.defaultdict(int)
        for line in mbox:
            domain = re.findall('From [^ ].*@([^ ]*)', line)
            if domain:
                ndict[domain[0]] += 1

        for item in sorted(ndict.items(), key=lambda x: x[1], reverse=True):
            print(item)

if __name__ == '__main__':
    main()

You can replace the majority of your code with collections.Counter.

import re
import collections

def main():
    with open('mailbox.txt') as mbox:
        counts = collections.Counter(
            domain[0]
            for domain in re.findall('From [^ ].*@([^ ]*)', line)
            if domain
        )
        for item in counts.most_common():
            print(item)

if __name__ == '__main__':
    main()


Is this a good approach to loading JavaScript files asynchronously? I want to load JS files asynchronously to speed up page loading, but also need to execute JS code only once the scripts finish loading (so I need a way to implement callbacks).

I based this off of some example code on the Google Pagespeed website:

// Add script elements as a children of the body
function downloadJSAtOnload() {
    var filesToLoad = [
        ["/assets/plugins/typeahead.bundle.min.js", "onTypeaheadLoaded"],
        ["https://apis.google.com/js/client:plusone.js?onload=googlePlusOnloadCallback", ""]
    ];

    filesToLoad.forEach(function(entry) {
        var element = document.createElement("script");
        element.src = entry[0];
        if (entry[1] != "") { // if an onload callback is present (NOTE: DOES NOT SUPPORT NAMESPACES -- http://stackoverflow.com/a/359910/1101095)
            element.onload = function() {
                if (typeof window[entry[1]] != "undefined") {
                    window[entry[1]]();
                }
            };
        }
        document.body.appendChild(element);
    });
}

// Check for browser support of event handling capability
if (window.addEventListener)
    window.addEventListener("load", downloadJSAtOnload, false);
else if (window.attachEvent)
    window.attachEvent("onload", downloadJSAtOnload);
else window.onload = downloadJSAtOnload;

I know there are libraries out there for dependency management, callback functions, etc., but I just want something very lightweight and using a library would mean having another HTTP request.

I'm very new to JavaScript and would really appreciate any feedback or advice you might have!

First of all, I would split you problem into 3 parts:

Loading a js-file asynchronously, and calling a callback-function
Loading several js-files an once asynchronously
Using an onload-Handler, which triggers everything

These are 3 independent parts, which might be reused for other purposes, if seperated correctly.

1. Loading a js-file asynchronously

You're using 2 strings to specify a file to be loaded. But instead of writing

["file.js", "cbFunction"]

you can as well also pass the actual function:

["file.js", cbFunction]

This not only ensures that the function actually exists, it also gives you the opportunity to use anonymous functions:

["file.js", function(){ alert("loaded!"); } ]

However, speaking of segregation of responsibilities, you should write a function on its own, without the need of an Array anymore:

function loadScript(path, scriptLoadedCallback){
    var element=document.createElement('script');    //generate <script>-tag
    element.setAttribute("type","text/javascript");
    element.setAttribute("src", path);

    if(typeof(scriptLoadedCallback) == 'function'){  //makes the callback-function optional
        element.onload = function() {
            return scriptLoadedCallback(true, path); //true = successfull; the path is needed later
        };
        element.onerror = function() {               //you might also call the cb on error
            this.parentNode.removeChild(this);       //remove faulty node from DOM
            return scriptLoadedCallback(false, path);      //false = error; the path is needed later
        };
    }
    document.head.appendChild(fileref);              //insert the node in DOM (end of <head>), and load the script
}

This function looks much clearer, and you can see at first glance what it is doing.

Note that I checked the existence of the callback before I append an onload-attribute. This is a bit more efficient than your code, which always calls the onload-Handler and then does nothing.

You might also want to add the onerror-method, otherwise you will never get notified if the loading failed (This usually happens if the js-file doesn't exist).

If you only have to load a single script, you can call this function directly. No need to generate useless Arrays.

2. Loading several js-files an once

If you only want to load several files, and calling a callback after each file, this is an easy task, using your original array (with functions instead of strings):

var filesToLoad = [
    ["file1.js", cbFunctionToCall],
    ["file2.js"]
];
for(var i=0; i<filesToLoad.length; i++){ //I prefer a for-loop because its more readable
    loadScript(filesToLoad[i][0], filesToLoad[i][1];
}

And that's all.

However, if you want to have one callback-function which is called after all files have been loaded, this gets a bit tricky. You don't know which file is the last one, and which file should trigger the actual callback-function.

function loadScriptArray(contentArray, contentLoadedCallback){
    var contentQuantity = contentArray.length;      //Number of Files that needs to be loaded
    var contentCompleted = 0;                       //Number of Files, that have already been loaded
    var returnParamList = {};                       //List with return-Parameters

    if(contentQuantity == 0){                       //We don't have anything to load
        return contentLoadedCallback({});
    }

    for (var i = 0; i < contentQuantity; i++) {
        loadScript(contentArray[i], function(success, path){   //This anonymous function is called everytime a script is finished
                                                               //The only way to know which script finished, is to pass the path as an parameter
            returnParamList[ identifier ] = returnParam;       //store the returnValue (true=success, false=error)
            contentCompleted++;
            if(contentCompleted == contentQuantity){    //this was the last file
                if(typeof contentLoadedCallback== 'function'){
                    contentLoadedCallback(returnParamList);
                }
            }
        });
    }
}

This is how the function is being called:

loadScriptArray(["file1.js", "file2.js"], function(returnParamList){
    alert("All Scripts finished. \n"+
          "File1: "+returnParamList["file1.js"]+"\n"+
          "File2: "+returnParamList["file2.js"]);
});

This function loads 2 files simultaneously, and calls the callback after the last one has been loaded. Afterwards it prints which files have been loaded successfully, and which loading-operation failed.

This only works, if the initial loadScript()-Function always calls the callback-Function - both if it succeeded or failed. And to report the success back to the caller, the callback-function also needs the filePath as an parameter.

Another advantage is, that you can extend this function to also load other files like images, json, xml and so on.

3. Trigger loading with onload

You code looks fine here, with only one very bad issue: Use brackets, they won't kill you.

There's absolutely no reason to omit brackets, it's just a possible source for annoying errors, which are very hard to be found.


Generating random numbers and decks of cards So I have some code here:
First a simple random # generator and a array-choosing function:

 function Rand(min,max)
 {
 return parseFloat(Math.floor(Math.random()* max-min+1)))+parseFloat(min);
 }

 function Choose(arr)
 {
//Returns an element from an array at random.
return arr[Math.floor(Math.random()*arr.length)];
 }

and second a Card shuffler:

function CardDeck() {
var Cd=["Ace","2","3","4","5","6","7","8","9","10","Jack","Queen","King"];
var H=[];
var S=[];
var D=[];
var C=[];
var Result=[];
var Dk=document.getElementById("Deck Count").value;
for(i=0;i<Cd.length;i++) {
    S[i]=Cd[i]+" of Spades";
    H[i]=Cd[i]+" of Hearts";
    C[i]=Cd[i]+" of Clubs";
    D[i]=Cd[i]+" of Diamonds";}
if (Dk=="4") {P=S.concat(C,D,H);}
else if (Dk=="5") {
    var St=[];
    for(i=0;i<Cd.length;i++) {St[i]=Cd[i]+" of Stars";}
    P=S.concat(C,D,H,St);}
else if (Dk=="6") {
    var Rk=[];
    var Wh=[];
    for(i=0;i<Cd.length;i++) {
        Rk[i]=Cd[i]+" of Rackets";
        Wh[i]=Cd[i]+" of Wheels";}
    P=S.concat(C,D,H,Rk,Wh);}
for(i=0;i<Dk*Cd.length;i++) {
    var Q=Choose(P);
    R=P.indexOf(Q);
    Result[i]=(i+1)+": "+Q;
    P=P.slice(0,R).concat(P.slice(R+1));}
document.getElementById("Cards").innerHTML=Result.join("\n");}

Is there an easy way to make this faster or at least declare the arrays faster instead of just doing a=[],b=[]...

Additionally, is there a way to get a better RNG with a longer period in base JS (no libraries, as this was built on base JS)

First off, your code is borderline unreadable. Use proper indentation. Add whitespace (horizontal and vertical). Follow conventional guidelines (lowercase camelCase names for most things). And give your variables actual names - bytes are cheap.

If you want really compact code, use a minifier - after writing the code in readable, sane manner. This looks like a text dump of a fiendish Excel spreadsheet or something.

For the few things that I could read, and which stand out:

You Rand [sic] function is doing a bunch of unnecessary stuff. You parseFloat something that's guaranteed to be an integer, because you called Math.floor() on it. You parseFloat on min when adding it at the end, but you don't do it when you subtract it from max.

And it's broken (which makes it technically off-topic for CodeReview, by the way).

Firstly, it doesn't even run because you've got one close-parenthesis too many, so there's a syntax error.

Secondly, you're not paying attention to operator precedence: Multiplication precedes addition and subtract. So your code is saying Math.random() * max then subtracting min and finally adding 1. Hence, if you call Rand(9, 10), you're liable to get any number from 1-10, rather than a number from 9-10.

Just say:

function rand(min, max) {
  var span = max - min;
  return Math.round(Math.random() * span) + min;
}

As for Choose [sic], you can use the bitwise-floor trick:

function choose(arr) {
  return arr[Math.random() * arr.length | 0];
}

When appending to and array, use push. Don't just assign to an index. E.g.

spades.push(values[i] + " of Spades");
hearts.push(values[i] + " of Hearts");
clubs.push(values[i] + " of Clubs");
diamonds.push(values[i] + " of Diamonds");

As for "better RNG", that's a StackOverflow question if anything. JavaScript has Math.random() and that's it.