Finding legal knight moves in a chessboard in APL I wrote a function that takes as input a vector with two integers between 1 and 8 representing a position in a chessboard and that should output a vector where each cell is a similar vector of integers, with the positions that a knight in the input position could reach.

E.g. for the input 1 1, my function should output [2 3] [3 2] (I'm using the [] to represent the boxes of the cells).

This is what I wrote:

knight_moves ← {
  ⍝ Monadic function, expects a vector with 2 integers
  ⍝ Given a chessboard position, find the legal knight moves
  signs ← , ∘.,⍨(¯1 1)
  offsets ← ((⊂⌽),⊂) 2 1
  moves ← , signs ∘.× offsets
  locations ← moves + ⊂⍵
  valid ← ^/¨(1∘≤∧≤∘8) locations
  valid/locations
}

This works and gives the expected result for a series of test cases. Since I am quite new to APL, I wanted to know what could be written in a cleaner way.

This question has been followed-up here.

Initial impression

Your code is already quite good, using idiomatic APL in short clear lines that each do a single job well. Your variable names are such that you don't really need comments other than the fine description you already have at the top.

Describe your result

You might want to add a third comment describing the result structure:

  ⍝ Returns a vector of 2-element vectors

Remove unnecessary parenthesis

The vector (¯1 1) could be written as ¯1 1

Adopt a naming convension

Consider a naming convention that makes it easier for the reader to distinguish syntactic classes; mainly variables and functions, but maybe even monadic operators and dyadic operators. One such scheme that some people like is:

variables lowerCamelCase
Functions UpperCamelCase
_Monadic _Operators _UnderscoreUpperCamelCase
_Dyadic_ _Operators_ _UnderscoreUpperCamelCaseUnderscore_

Being that you seem to prefer snake_case: An equivalent such scheme could be used too:

variables lower_snake_case
Functions Upper_snake_case
_Monadic _Operators _Underscore_upper_snake_case
_Dyadic_ _Operators_ _Underscore_upper_snake_case_underscore_

Alternatively, the cases could be swapped: My father used lowercase for functions and uppercase for variables according to the German (and previous Danish) orthography that specifies lowercase verbs and uppercase nouns, and this may also look more natural with things like X f Y rather than x F y.

Interestingly, Stack Exchange's syntax colourer seems to make a distinction between uppercase and lowercase identifiers.

Consider naming complex functions

You use two non-trivial trains. Consider giving them meaningful names, which also allows you to remove their parentheses:

  Dirs ← (⊂⌽),⊂
  offsets ← Dirs 2 1

  In_range ← 1∘≤∧≤∘8
  valid ← ^/¨In_range locations

This isn't necessarily required in this case, but could be relevant with more involved code.

Improve performance by keeping arrays flat

To avoid the overhead of pointer chasing, you can implement your function using only flat arrays, and then, as a finalising step, restructure the data as required. Here is a direct translation of your code to flat-array code:

knight_moves_flat←{
⍝ Monadic function, expects a vector with 2 integers
⍝ Given a chessboard position, find the legal knight moves
⍝ Returns a 2-column table
  signs← ,[⍳2] ,⍤1 0⍤0 1⍨ (¯1 1)
  offsets ← (⌽,[1.5]⊢) 2 1
  moves ← ,[⍳2] signs (×⍤1⍤1 2) offsets
  locations ← moves (+⍤1) ⍵
  valid ← ^/(1∘≤∧≤∘8) locations
  ↓valid⌿locations
}

Compare the performance:

      ]runtime -compare knight_moves¨all knight_moves_flat¨all

  knight_moves¨all      → 7.4E¯4 |   0% ⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕
  knight_moves_flat¨all → 5.0E¯4 | -34% ⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕

The price here is that the code becomes somewhat more involved and less clear.

For an alternative algorithm with even better performance, see Roger Hui's blog post 2019 APL Problem Solving Competition: Phase I Problems Sample Solutions.

Ultimate performance through lookups

If you need to call the function many (more than 100) times, you can get the ultimate performance by pre-computing all the results (by any means). This is because the input domain is rather limited. With only 64 valid arguments, you pay a 64-fold setup cost, but after that, the only costs will be looking up an argument in a list of valid arguments and then picking the corresponding result from a list of results. However, in this case, where the argument already is a proper argument for ⊃, you can simply use the argument directly to pick a result from a vector of vectors of results, thus avoiding even the lookup cost:

all ← ⍳ 8 8
results ← ↓knight_moves¨all
knight_moves_pick ← ⊃∘results

Throughput increases with almost two orders of magnitude compared to the flat edition:

      ]runtime -c knight_moves_flat¨all knight_moves_pick¨all

  knight_moves_flat¨all → 4.4E¯4 |   0% ⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕⎕
  knight_moves_pick¨all → 5.2E¯6 | -99%

Since the result-picking is almost free in comparison to actually computing each result, the setup cost is paid off after less than 100 applications, and is certainly negligible in the above comparison where each expression is run well over 10000 (1002) times. Instead, you pay though additional storage space being required:

      (⍪,⎕SIZE)⎕NL 3
knight_moves       2800
knight_moves_flat  3512
knight_moves_pick 19088

The fully expanded text representation of the function is also unreadable:

knight_moves_pick ← ⊃∘(((2 3)(3 2))((3 1)(2 4)(3 3))((2 1)(3 2)(2 5)(3 4))((2 2)(3 3)(2 6)(3 5))((2 3)(3 4)(2 7)(3 6))((2 4)(3 5)(2 8)(3 7))((2 5)(3 6)(3 8))((2 6)(3 7)))(((1 3)(3 3)(4 2))((1 4)(4 1)(3 4)(4 3))((1 1)(1 5)(3 1)(4 2)(3 5)(4 4))((1 2)(1 6)(3 2)(4 3)(3 6)(4 5))((1 3)(1 7)(3 3)(4 4)(3 7)(4 6))((1 4)(1 8)(3 4)(4 5)(3 8)(4 7))((1 5)(3 5)(4 6)(4 8))((1 6)(3 6)(4 7)))(((2 3)(1 2)(4 3)(5 2))((1 1)(2 4)(1 3)(5 1)(4 4)(5 3))((2 1)(1 2)(2 5)(1 4)(4 1)(5 2)(4 5)(5 4))((2 2)(1 3)(2 6)(1 5)(4 2)(5 3)(4 6)(5 5))((2 3)(1 4)(2 7)(1 6)(4 3)(5 4)(4 7)(5 6))((2 4)(1 5)(2 8)(1 7)(4 4)(5 5)(4 8)(5 7))((2 5)(1 6)(1 8)(4 5)(5 6)(5 8))((2 6)(1 7)(4 6)(5 7)))(((3 3)(2 2)(5 3)(6 2))((2 1)(3 4)(2 3)(6 1)(5 4)(6 3))((3 1)(2 2)(3 5)(2 4)(5 1)(6 2)(5 5)(6 4))((3 2)(2 3)(3 6)(2 5)(5 2)(6 3)(5 6)(6 5))((3 3)(2 4)(3 7)(2 6)(5 3)(6 4)(5 7)(6 6))((3 4)(2 5)(3 8)(2 7)(5 4)(6 5)(5 8)(6 7))((3 5)(2 6)(2 8)(5 5)(6 6)(6 8))((3 6)(2 7)(5 6)(6 7)))(((4 3)(3 2)(6 3)(7 2))((3 1)(4 4)(3 3)(7 1)(6 4)(7 3))((4 1)(3 2)(4 5)(3 4)(6 1)(7 2)(6 5)(7 4))((4 2)(3 3)(4 6)(3 5)(6 2)(7 3)(6 6)(7 5))((4 3)(3 4)(4 7)(3 6)(6 3)(7 4)(6 7)(7 6))((4 4)(3 5)(4 8)(3 7)(6 4)(7 5)(6 8)(7 7))((4 5)(3 6)(3 8)(6 5)(7 6)(7 8))((4 6)(3 7)(6 6)(7 7)))(((5 3)(4 2)(7 3)(8 2))((4 1)(5 4)(4 3)(8 1)(7 4)(8 3))((5 1)(4 2)(5 5)(4 4)(7 1)(8 2)(7 5)(8 4))((5 2)(4 3)(5 6)(4 5)(7 2)(8 3)(7 6)(8 5))((5 3)(4 4)(5 7)(4 6)(7 3)(8 4)(7 7)(8 6))((5 4)(4 5)(5 8)(4 7)(7 4)(8 5)(7 8)(8 7))((5 5)(4 6)(4 8)(7 5)(8 6)(8 8))((5 6)(4 7)(7 6)(8 7)))(((6 3)(5 2)(8 3))((5 1)(6 4)(5 3)(8 4))((6 1)(5 2)(6 5)(5 4)(8 1)(8 5))((6 2)(5 3)(6 6)(5 5)(8 2)(8 6))((6 3)(5 4)(6 7)(5 6)(8 3)(8 7))((6 4)(5 5)(6 8)(5 7)(8 4)(8 8))((6 5)(5 6)(5 8)(8 5))((6 6)(5 7)(8 6)))(((7 3)(6 2))((6 1)(7 4)(6 3))((7 1)(6 2)(7 5)(6 4))((7 2)(6 3)(7 6)(6 5))((7 3)(6 4)(7 7)(6 6))((7 4)(6 5)(7 8)(6 7))((7 5)(6 6)(6 8))((7 6)(6 7)))

It is interesting to note that just parsing the giant constant takes about as long as computing it.


Find The One Element In An Array That is Different From The Others https://www.codewars.com/kata/find-the-stray-number/train/javascript

I solved this challenge by sorting from least to greatest, and checking if the first element in the array matched the second element in the array.

If the first element does not match, then the different element is the first element.

If the first element does match, then the different element is the last element.

function stray(numbers) {
  numbers = numbers.sort((a, b) => a - b);
  if (numbers[0] !== numbers[1]) {
    return numbers[0];
  }
  else {
    return numbers[numbers.length - 1];
  }
}
console.log(stray([17, 17, 3, 17, 17, 17, 17]));

I'm wondering if / how this can be done with the filter() method instead?

This is a similar idea to the other answers here, but the implementation is a bit different.

First of all, we can assume that the array's length is at least 3, since it needs to have at least two of the same values and one different value.

Let's start by handling the case where the stray value is not in the first element. We could simply write:

a.find(v => v != a[0])

That is, find an element that's different from the first element. But what if the stray element comes first in the array? We can check if the first two elements differ. If they do, then the stray is either in the first or second position, so the third element is not a stray. In this case, we can check against the third element instead of the first; otherwise we check against the first element as before, thus:

a.find(v => a[0] != a[1] ? v != a[2] : v != a[0])

This is a bit code-golfey and not very readable, so I wouldn't recommend it in production, but it may be of some interest as a curiosity.

It may be worth noting that this solution appears to perform quite well, and can be further optimized by doing the inequality check on the first two elements before invoking find, and by using the third parameter to find to access the array, making the callback a pure function and eliminating the need to reference the array via the closed-over variable, for example:

a.find(a[0] != a[1] ?
    (v, i, a) => v != a[2] :
    (v, i, a) => v != a[0])


Poker equity calculation algorithm I writing some specific poker equity calculator. Basically, equity calculator tells how much one hand is more profitable than other. I have written such calculator and it works, works fast, but there is a one not optimal place where a lot of time is spent and it definitely can be optimized. I am asking you about the advice how to optimize it.

Below is the simplified structure of poker equity calculator. For sample purpose, I have made it for the game when both Players hands contain one from 52 cards, 4 cards are on board(turn), one(river) board card left. Let say that player who's sum of the board and hand card is higher - wins.

#include "stdafx.h"
#include <vector>
#include <iostream>
#include <ctime>
#include <string>

using namespace std;

static const int card_count = 52;

// Checks that board contains the card. A lot of time spent here. Need to be optimized.
bool Contains(const std::vector<int>& board, int card)
{
    for (int boardCard : board)
    {
        if (boardCard == card)
        {
            return true;
        }
    }

    return false;
}

// Fake function that evaluates hand strength. This is just for example real evaluator is different. So no need to optimize this.
size_t GetHandStrength(const std::vector<int>& board, int card, int b)
{
    size_t stren = 0;

    for (int boardCard : board)
    {
        stren += boardCard;
    }

    return stren + card + b;
}

// Returns equity for the board
float GetEquity(const std::vector<int>& board)
{
    float equity = 0;

    for (int c1 = 0; c1 < card_count; c1++) // Player1 card
    {
        if (Contains(board, c1))
        {
            continue;
        }

        for (int c2 = 0; c2 < card_count; c2++)  // Player2 card
        {
            if (c1 == c2 || Contains(board, c2))
            {
                continue;
            }

            size_t usedBoards = 0; // number of boards are possible for this players and board cards
            float currentEquity = 0;

            for (int b = 0; b < card_count; b++) // Iterating river board cards
            {
                if (Contains(board, b))
                {
                    continue;
                }

                if (b == c1 || b == c2) // Check that there are no conflict with players cards
                {
                    continue;
                }

                size_t p1stren = GetHandStrength(board, c1, b);
                size_t p2stren = GetHandStrength(board, c2, b);

                if (p1stren > p2stren)
                {
                    currentEquity += 1.0;
                }
                else if (p1stren < p2stren)
                {
                    currentEquity -= 1.0;
                }

                usedBoards++;
            }

            currentEquity /= usedBoards;
            equity += currentEquity;
        }
    }

    return equity;
}

int main()
{
    clock_t begin = clock();
    std::vector<int> board = { 0, 1, 2, 21 }; // Some turn random board (4 cards)
    float equity = GetEquity(board);;
    cout << "equity: " << equity <<  endl;
    return 0;
}

When iterating over the hand and board cards I need to check that board doesn't already contains the card, so I need to iterate over the board cards again and again. This is done by the bool Contains(const std::vector<int>& board, int card) also checks like if (b == c1 || b == c2) What can you suggest to optimize this part?

Since you're doing an O(N³) operation, with a linear search, a big performance boost will happen if we can eliminate that search.

There are two similar ways to go about it; both replace the search with an indexed lookup.

Change the type of board from a vector that holds the cards to a vector (or array) of card_count slots that holds a count of how many of that card is in the board (using a count for future expansion to multiple decks).
In GetEquity, fill a vector or array of card_count elements with counts for the cards in board, then just check that container for a nonzero to see if the card is contained in the board.  This has the additional advantage of being able to add c1 and c2, so the inner loop checks only need to check one condition (rather than 2 or 3) to see if a card is available.

Here's an updated GetEquity using ideas from option 2:

float GetEquity(const std::vector<int>& board)
{
    std::vector<int> counts(card_count);
    for (auto b: board)
        ++counts[b];

    float equity = 0;

    for (int c1 = 0; c1 < card_count; c1++) // Player1 card
    {
        if (counts[c1])
        {
            continue;
        }
        ++counts[c1];

        for (int c2 = 0; c2 < card_count; c2++)  // Player2 card
        {
            if (counts[c2])
            {
                continue;
            }
            ++counts[c2];

            size_t usedBoards = 0; // number of boards are possible for this players and board cards
            float currentEquity = 0;

            for (int b = 0; b < card_count; b++) // Iterating river board cards
            {
                if (counts[b])
                {
                    continue;
                }

                size_t p1stren = GetHandStrength(board, c1, b);
                size_t p2stren = GetHandStrength(board, c2, b);

                if (p1stren > p2stren)
                {
                    currentEquity += 1.0;
                }
                else if (p1stren < p2stren)
                {
                    currentEquity -= 1.0;
                }

                usedBoards++;
            }

            currentEquity /= usedBoards;
            equity += currentEquity;

            --counts[c2];
        }
        --counts[c1];
    }

    return equity;
}


Scala list to tuple conversion def toPairs[A](xs : Seq[A]) : Seq[Tuple2[A,A]] = {
      @tailrec
      def accumulator(as : Seq[A], accum: Seq[Tuple2[A,A]]) : Seq[Tuple2[A,A]] = as match {
        case Nil => accum
        case x :: tail => tail match {
          case Nil => accum
          case _ =>  accumulator(as.tail, accum :+ (x,tail.head))
        }
      }

      accumulator(xs, Seq.empty[Tuple2[A,A]])
    }

I use this code for the following use case: for a Seq[A] i.e. List(1,2,3) I want a Seq of Tuples of A i.e. List((1,2)(2,3)). Is this the most optimal, idiomatic way in Scala, what could be improved?

For the record: the order of the elements is important and is expected to be congruent with the example in the preceding paragraph.

I find the way you go back and forth between as and x a bit annoying.  You should be able to collapse the nested match into one match that covers three cases.

The :+ operator, which appends an element to a sequence, should be avoided at all costs, because it involves traversing to the end of the sequence.  That makes your algorithm O(n2), when it should ideally be O(n).

Anyway, the solution can be much simpler than that.

def toPairs[A](xs: Seq[A]): Seq[(A,A)] = xs.zip(xs.tail)


Can I speed up this simple versioning/backup script? I'm writing a simple automatic backup/versioning bash script.

It should basically mirror a directory structure somewhere, and then copy files over there when they've been altered. The ghost files should then be named filename.YYYYMMDD-HHMMSS.ext (the included date/time being the time of last modification).

This is what I came up with - it seems to work already, sort of. but since I've never written shell scripts before, I suspect I might be doing some things fundamentally wrong and inefficiënt.

How can I make this faster? The iterating over files seems to be really slow.
Is this robust? It seems to "lock up" without error messages sometimes, and I can't explain or understand why. What am I doing wrong?

I intend to run this as a cron job, every 30 minutes or so.

#!/bin/sh

# $1 : backup "root"
# $2 : from: directory to be backed up
# $3 : to: destination

rpath=$1

for f in $(find $2); do
  r=$(./rel.sh $rpath $f)
  if [ -f $f ]
  then
    basename=$(basename $f)
    dir=$(dirname $r)
    name=$(echo $basename | cut -d'.' -f1)
    ext=$(echo $basename | cut -d'.' -f2)
    mod=$(stat --format=%y $f | awk -F'.' '{printf $1}' | sed 's/[-:]//g' | sed 's/ /-/g')

    if [ ! -f "$3/$dir/$name.$mod.$ext" ]
    then
      echo "+f   $mod $r"
      # cp $f "$3/$dir/$name.$mod.$ext"
    fi
  elif [ -d $f ]
  then
    if [ ! -d "$3/$r" ]
    then
      echo "+dir $r"
      mkdir "$3/$r"
    fi
  fi
done
exit

Implementing solutions to problems like this is good for exercise purposes. It is good for learning.

But it is almost never good to re-invent the wheel in production systems. Please have a look at well-established data synchronization software such as rsync and perform some research around it and other data synchronization/backup/snapshot techniques.

So, your question is good until "I intend to run this as a cron job, every 30 minutes or so." :-) I am not that bash expert myself, to others might point out the potential weaknesses of your approach.


Improving a function from using 1-D arrays to 2-D arrays as argument The problem:

I have a function that writes an array it takes as argument to a div, in a single line.

Example:
writeToOutput([1,2,3]) outputs
    1,2,3

The aim is to improve this function, to be able to take an array of arguments, and display them on several lines

Example: writeToOutput([[1,2,3],[4,5,6]]) outputs

1,2,3
4,5,6

An obvious solution is to use writeToOutput([[1,2,3]]) to replicate the previous results, however, the function call with a 1-D array is already used by several scripts, and I would like them to keep working.

The code:

function writeToOutput(values, outputName) {
  var outputDiv = document.getElementById(outputName)
  var outputText = "";

  if (!Array.isArray(values[0])) {
    values = [values]
  }

  var valuesSubArray;
  for (var i = 0; i < values.length; i++) {
    valuesSubArray = values[i];
    outputText = "";
    for (var j = 0; j < valuesSubArray.length; j++) {
      outputText += valuesSubArray[j]
    }
    if (i > 0) {
      outputDiv.innerHTML += "</br>";
    }
    outputDiv.innerHTML += outputText;
  }
}

What I am interested in:

As you may have guessed, I don't really care about writing numbers to a div, but I have a legacy function that takes a 1-D array, and I would like it to deal with 2-D arrays without breaking existing implementation.

I am interesting in figuring out if the trick with Array.isArray(values[0]) is the best way to do this, or if there is more performant, more widely used, etc. ways to do this. (The project is using jQuery).

Altought this stub has been made to sugarcoat the 1-D -> 2-D array, I am eager to learn, and if there is an obvious improvement to be made, please let me know!

Regarding your main question
From my point of view, yes, your current solution to allow legacy 1-D arrays to work with your new version is the best one I can think.
Rather funny: I just proposed a close solution in a PHP context question, less than 24 hours ago!
Another obvious method might be to keep the legacy function as is, and to write an almost identical one (simply expecting only 2-D arrays) with an alternative name.
But it doesn't seem better at all, notably because in case of changes regarding the core job they do, these two functions should be both updated!
Now since you asked for other possible improvements
I know that your example is not a real one, but since you implemented it "seriously" I propose you some points you might consider.
Performance
for (var i = 0; i < values.length; i++) {
  // ...
}

With the above code (your outer loop), values.length is evaluated for each step while looping, that may become significative when length is high.
A well known best practice is to avoid it, using this instead:
for (var i = 0, n = values.length; i < n; i++) {
  // ...
}

This way, values.length is evaluated only once.
Simplification
Anyway, instead of the above code you can use this alternative construction, resulting in a simpler code:
values.forEach(function(valuesSubArray, i) {
  // ...
});

NOTE: with this construction, you can also drop the previous declaration var valuesSubArray;.
Again simplification, and probably also performance
outputText = "";
for (var j = 0; j < valuesSubArray.length; j++) {
  outputText += valuesSubArray[j]
}

The above code (your inner loop) can be replaced by this simple statement:
outputText = valuesSubArray.join('');
NOTE: this results in 123 for a sub-array like [1, 2, 3], like does your outputText += valuesSubArray[j]. If you rather want to get 1,2,3 as stated by the initial explanation in your OP, you must use outputText = valuesSubArray.join(','); instead.
More simplification
At a higher level we can observe that the whole desired result outputDiv.innerHTML structure is the same as the outputText's one`:

each element is the current outputText
the separator is
 instead of a comma

So we can use join('
') at this level, applying it to an array of the generated outputTexts. This array can be obtained using reduce():
values.reduce(
  function(result, valuesSubArray) {
    result.push(valuesSubArray.join(','));
    return result;
  },
  []
);

NOTE: we don't need using i any longer, nor previously declaring outputText.
Finally
We can completely drop any variable declaration, and achieve the whole job with a "one-liner":

function writeToOutput(values, outputName) {
  if (!Array.isArray(values[0])) {
    values = [values]
  }

  document.getElementById(outputName).innerHTML =
    values.reduce(
      function(result, valuesSubArray) {
        result.push(valuesSubArray.join(','));
        return result;
      },
      []
    )
    .join('
');
}

writeToOutput([[1, 2, 3], [4, 5, 6]], 'outputs');


Asynchronous login form handler I have an app that user submit the log in form , when it sent the data to server app create a connection for its account.

In this connection i have an integer field named as state.
the state value is : 1 for connecting, 2 for connected and 0 for failed.

I show a dialog to user display  Connecting ... and then check the state of connection. if its return 0 or 2 dismiss the dialog and show the related message else if it doesn't change after 15 sec dismiss dialog and change the state to 0 !
I do the logic in this way :

final AsyncTask<IImConnection, Void, Boolean> task = new CheckSignInTask();
timeOut = new TimeOut(task);
handler.postDelayed(timeOut, 60 * 1000);
task.execute(conn);

CheckSignInTask class :

 private class CheckSignInTask extends AsyncTask<IImConnection, Void, Boolean> {

        private IImConnection connection;

        @Override
        protected Boolean doInBackground(IImConnection... params) {
            try {
                connection = params[0];
                if (connection != null){
                    while (connection.getState() == ImConnection.LOGGING_IN) {
                        // do nothing while connection state not change

                    }
                    return true;
                }
                else{
                    return false;
                }

            } catch (RemoteException e) {
                e.printStackTrace();  //To change body of catch statement use File | Settings | File Templates.
            }
            return true;  //To change body of implemented methods use File | Settings | File Templates.
        }

        protected void onPreExecute() {
            dialog = new ProgressDialog(AccountActivity.this);
            dialog.setMessage("Signing in...");
            dialog.show();
        }

        protected void onPostExecute(final Boolean result) {
            if (dialog.isShowing()) {
                dialog.dismiss();
            }
            if (result) {
                try {
                    if (connection.getState() == ImConnection.LOGGED_IN) {
                        isSignedIn = true;
                        setResult(RESULT_OK);
                        finish();
                    } else {
                        isSignedIn = false;
                        deleteAccount();
                        displayLoginError("Login failed ! maybe your username/password is incorrect");
                    }
                } catch (RemoteException e) {
                    e.printStackTrace();  //To change body of catch statement use File | Settings | File Templates.
                }
            }
            else{
                recreate();
            }
        }

        @Override
        protected void onCancelled() {
            if (dialog.isShowing()) {
                dialog.dismiss();
            }
            super.onCancelled();
        }
    }

TimeOut class :

 public class TimeOut implements Runnable {
        private AsyncTask task;

        public TimeOut(AsyncTask task) {
            this.task = task;
        }

        @Override
        public void run() {
            if (task.getStatus() == AsyncTask.Status.RUNNING) {
                task.cancel(true);
                deleteAccount();
                try {
                    if (dialog != null && dialog.isShowing()) {
                        dialog.dismiss();
                    }
                } catch (java.lang.IllegalArgumentException iae) {
                    //dialog may not be attached to window if Activity was closed
                }
                displayLoginError("Login failed ! check your internet connetction or maybe your username/password is incorrect");
            }
        }
    }

Its work ! but im not sure is it a good approach ?

Disclaimer: not an android developer....

Thoughts on the Problem

A few thoughts, right off the top.

  In this connection i have an integer field named as state.
  the state value is : 1 for connecting, 2 for connected and 0 for failed

First - that should probably be implemented as an enum, rather than as an integer

Second - that suggests that you are trying to implement a state machine (unsurprising; you are trying to implement a login protocol, and protocols almost always demand state machines).  I'd encourage you to refactor the code and make the state machine explicit; you might want to consider stateless4j as part of the implementation.

Third - doInBackground() needs to be checking for cancellation.  Normally, I would expect that check to be the condition in the while loop.

while (! isCancelled()) {
    if (connection.getState() == ImConnection.LOGGING_IN) {
        ...
    }
}

With the while loop written the way that you have it, it looks as though you are busy spinning while waiting for the state to change.  I'm not keen on that at all; Thread.sleep(), SystemClock.sleep(), something to free up CPU to do the work.  I'd probably go with Thread.sleep() myself, since I would want this worker thread to respond quickly to the cancel() command (which almost certainly interrupts() the running thread).

It's not really clear to me that you need the cancellation to be running on a timer separate from the CheckSignInTask - why not have the task check the time out itself? (If because Reasons[tm], those reasons should be clearly documented in the code so that the next coder to come along doesn't get too clever).

Style notes

What's the point of assigning a member variable if you are only using it in one place?

        protected Boolean doInBackground(IImConnection... params) {
        try {
            IImConnection connection = params[0];

Also, it's disconcerting that you are passed an array of connections, but you only ever manipulate the first one?

public class TimeOut implements Runnable {
        private AsyncTask task;

        public TimeOut(AsyncTask task) {
            this.task = task;
        }

You planning on changing what task you are timing out later?  Of course not.  So make that explicit

public class TimeOut implements Runnable {
        private final AsyncTask task;
        ...

Try to avoid magic numbers

handler.postDelayed(timeOut, 60 * 1000);

Is that a minute?  A thousand hours? 60 millenia?

handler.postDelayed(timeOut, TimeUnit.MINUTES.toMillis(1));

or even better

final long LOGIN_PROTOCOL_TIMEOUT = TimeUnit.MINUTES.toMillis(1);
handler.postDelayed(timeOut, LOGIN_PROTOCOL_TIMEOUT);


Mastermind: Evaluating the guess The evaluate_guess function below returns the evaluation of a guess with respect to Mastermind game rules:

# sample secret code and guess
secret_code = ["a", "a", "c", "a"]
guess = ["c", "a", "a", "b"]

def evaluate_guess(secret_code, guess):
    score = []
    temp_list = secret_code.copy()

    for i in range(len(secret_code)):
        if guess[i] == temp_list[i]:
            score.append(1)
            temp_list[i] = None

        elif guess[i] in temp_list:
            score.append(0)
            temp_list[temp_list.index(guess[i])] = None

    score.sort(reverse=True)
    return score

print(evaluate_guess(secret_code, guess))

>>> [1, 0, 0]

Without changing the essence of the algorithm: Can I implement the evaluate_guess function in a more Pythonic way? Is it possible to implement the algorithm in a purely functional fashion?
Is there a more straightforward algorithm?

Most of the solutions provided so far is hard to read, and you need to really think before you understand what is happening, so I would like to propose an alternative approach as a response to a more straightforward algorithm:

def evaluate_guess(secret_code, guesses):

    score = []
    remaining_secrets = []
    remaining_guesses = []

    # Check exact matches
    for guess, secret in zip(secret_code, guesses):
        if guess == secret:
            score.append(1)

        else:
            remaining_guesses.append(guess)
            remaining_secrets.append(secret)

    # Check wrong placing
    for guess in remaining_guesses:

        if guess in remaining_secrets:
            score.append(0)
            remaining_secrets.remove(guess)

    return score

The only magic in this code is the joining of secret_code and guesses using zip which joins them index for index. This allows for, in my opinion, code that is easier to understand instead of list comprehensions, ifilters, index searching, sorting, sliceing and other stuff.


Generate a random discrete signal I currently have this code to generate a discrete signal with random amplitudes, but a fixed frequency:

import matplotlib.pyplot as plt
import numpy as np

# constants
t_len = 0.5
freq = 10.0
dt = 0.001

# set the see for reproducible results
rng = np.random.RandomState(0)

# create the random values to interpolate between
assert dt < freq
white_size = int(t_len * freq)
white_vals = rng.uniform(low=-1, high=1, size=white_size)

# setup the interpolation
white_noise = np.zeros(int(t_len / dt))
assert white_size < white_noise.shape[0]

# how can I avoid using this for-loop?
for w_i in xrange(white_size):
    white_noise[int(w_i/freq/dt):int((w_i+1)/freq/dt)] = white_vals[w_i]

# plot the result for visual verificaition
plt.plot(white_noise)
plt.show()

It generates something like this:

Like it says in the code comments, how do I get rid of this for-loop and vectorize the operation? I feel like I should be using reshape to accomplish this, but I'm not sure how.

I figured out that this assignment could be rephrased as a vectorized multiplication which eliminates the need for a for-loop and costly indexing of individual groups of elements.

import matplotlib.pyplot as plt
import numpy as np
import ipdb

# constants
t_len = 0.5
freq = 10.0
dt = 0.001

# set the see for reproducible results
rng = np.random.RandomState(0)

# create the random values to interpolate between
assert dt < freq
white_size = int(t_len * freq)
white_vals = np.random.uniform(low=-1, high=1, size=white_size)

# do the interpolation
tot_size = int(t_len / dt)
step_size = int(1 / freq / dt)
n_shape = (white_size, step_size)
white_noise = (white_vals[:, None] * np.ones(n_shape)).reshape(tot_size)

assert white_vals.shape[0] < white_noise.shape[0]

# plot the result for visual verificaition
plt.plot(white_noise)
plt.show()


jQuery functions to add corresponding entries of lists Thanks to this post, I was able to create a working piece of javascript that fits my needs. I have a div/ul/li based table where each "row" has 2 values that I want to have the sum for. Currently the code does exactly what I want, but somehow I have the feeling that the javascript is not coded in the most efficient way since I am repeating a lot of code. Any suggestions?

$(calculateSum1);
 function calculateSum1() {
 var sum1 = 0;
 // searching li for equal class and add the values
 $(".pr1").each(function() {
    var value = $(this).text();
    // add only if the value is number
    if(!isNaN(value) && value.length != 0) {
        sum1 += parseFloat(value);
    }
});
 $('#result1').text(sum1);
};

$(calculateSum2);
 function calculateSum2() {
 var sum2 = 0;
 // searching li for equal class and add the values
 $(".pr2").each(function() {
    var value = $(this).text();
    // add only if the value is number
    if(!isNaN(value) && value.length != 0) {
        sum2 += parseFloat(value);
    }
});
 $('#result2').text(sum2);
};

$(calculateSum3);
 function calculateSum3() {
 var sum3 = 0;
 // searching li for equal class and add the values
 $(".pr3").each(function() {
    var value = $(this).text();
    // add only if the value is number
    if(!isNaN(value) && value.length != 0) {
        sum3 += parseFloat(value);
    }
});
 $('#result3').text(sum3);
};

$(calculateSum4);
 function calculateSum4() {
 var sum4 = 0;
 // searching li for equal class and add the values
 $(".pr4").each(function() {
    var value = $(this).text();
    // add only if the value is number
    if(!isNaN(value) && value.length != 0) {
        sum4 += parseFloat(value);
    }
});
 $('#result4').text(sum4);
};
div.col, div.price {float:left}
div.col {width:25%;margin;none;}
ul {list-style-type: none;margin:0;}

Price

150

199

249

299

+

+

+

+

+

Add costs

27

20

30

39

=

=

=

=

=

Totaal

I think you could rewrite the code in a shorter way.

You can write the calculateSum# function to be generic.

This you can do by providing the selector or directly the jQuery object you need to loop and sum values.

And instead to directly write the result, you could return the computation.

Of course you could pass the result selector or jQuery element and do the job, but if you return the result, the code is more changeble in the future.

Then you create an object literal with the keys as your element selector, and the values as the result element selector.

And finally create an anonymous function to pass on jQuery document ready step $(myFuncOnDocReadyHere...), where you put all together.

If you prefer to avoid sharing the object with elements you could place it inside the on doc ready function, to keep it "private".

Here is my changes:

var summableAndResult = {
 ".pr1": "#result1",
 ".pr2": "#result2",
 ".pr3": "#result3",
 ".pr4": "#result4"
}

function calculateSum ($list) {
 var sum = 0;

 $list.each(function() {
  var value = $(this).text();
  // test isNaN only if string is not empty
  if (value.length != 0 && !isNaN(value)) {
   sum += parseFloat(value);
  }
 });

 return sum;
}

$(function(){

 for(var el in summableAndResult) {
  var sum = calculateSum($(el));
  $(summableAndResult[el]).text(sum);
 }

});
div.col, div.price {float:left}
div.col {width:25%;margin;none;}
ul {list-style-type: none;margin:0;}

Price

150

199

249

299

+

+

+

+

+

Add costs

27

20

30

39

=

=

=

=

=

Totaal


Haskell game server auth: cookie key generate&check C++ code version (description copied from it)

gameKeys id is authentication function of an MMO game server. When someone wants to play with friends, he sends request to server, server calls generateId, then gameKeys id and returns it as JSON using encodeJSON to player via API. When a friend establishes connection, he sends the game id and his key, server calls gameKeys id, where id is user data, and then checks user's key.

Algorithm is:

use current time and internal counter to generate id if it's not provided, or use provided id
make fast crypto hash using Blake2 function of id and secret(Yes, I know about HMAC and using as key, but in this case it doesn't matter)
split 64-bit hash to four 16-bit values, one hash for one user
make JSON object from id and keys and return it

I want to make stable code: exploit-free, no memory leaks and high speed (for high load and DDOS prevention). I'm experimenting now, so the algorithm was first written in JavaScript, then in C++ and now in Haskell.

I think Haskell is best in writing stable code.

Please review the Haskell code; I think it can be improved (I'm new to Haskell). Code compiles and works same as JS and C++. Also please advise me, which programming language is better for this: JS, C++ , Haskell or Go?

Here's the Haskell code; install dependencies cabal install blake2 hex split unix-time json to run it:

{-# LANGUAGE DeriveDataTypeable #-}
import Data.IORef
import System.IO.Unsafe
import qualified Crypto.Hash.BLAKE2.BLAKE2b as Blake2
import qualified Data.ByteString.Char8 as Char8
import Data.Hex
import Data.Char
import Data.List.Split
import Data.UnixTime
import Text.JSON.Generic

topsecret = "TOPSECRET"

dateCounter :: IORef Int
dateCounter = unsafePerformIO (newIORef 0)

hashBytes :: Char8.ByteString -> Char8.ByteString
hashBytes = Blake2.hash 64 mempty;
hashHex :: String -> String
hashHex =  (map toLower) . Char8.unpack . hex . hashBytes . Char8.pack;
makeKeys :: String -> [String]
makeKeys = (chunksOf 32) . hashHex;

data GameAuth = GameAuth
  { id :: Int
  , keys :: [String]
  } deriving (Show, Data, Typeable)

gameKeys id = GameAuth id (makeKeys ((show id)++"|keys|"++topsecret))

generateId = do
  time <- getUnixTime
  counter <- readIORef dateCounter
  modifyIORef dateCounter addCounter
  return $ shiftTime time counter
shiftTime time counter  = (fromEnum (utSeconds time))*1000 + counter
addCounter c = (c+1) `mod` 1000

main = do
  id <- generateId
  putStrLn (encodeJSON (gameKeys id))

Here's the node.js version; it needs blakejs (run npm i blakejs to install it):

const TOPSECRET = "TOPSECRET", blake = require("blakejs").blake2bHex;
let dateCounter = 0;

function gameKey(checkId){
  let id = checkId || (Date.now()/1000>>0)*1000+(dateCounter++%1000),
    keys = blake(`${id}|keys|${TOPSECRET}`).match(/.{32}/g);
  return {id, keys};
}
console.log(gameKey());

As modifyIORef notes, use modifyIORef' for counters.

I would wrap the unsafe usage of unsafePerformIO into a safer interface.

withCounter :: (Int -> IO a) -> IO a
withCounter action = let counter = unsafePerformIO $ newIORef 0 in do
  i <- readIORef counter
  modifyIORef' counter (+1)
  action i

generateId = withCounter $ \date -> do
  time <- fromEnum . utSeconds <$> getUnixTime
  return $ time * 1000 + mod date 1000

The || logic doesn't seem to be in the Haskell code yet.

I wouldn't give a name to a private function used only once.

Many of your brackets are redundant. The hint tool can tell you this.

It seems you want to use some but not all of what you define in other code. The code that uses this could make a difference in the review.

Data.Unique may interest you for id generation if you do not actually care about your exact behavior.


Wordpress $wpdb->row Query Check Looking at $wpdb class, I found get_row, and I am unsure if the function automatically prepares the query. I believe it does not and I may have to reexamine my code to prevent a future disaster.

Here is what my code looks, and I feel that it is vulnerable:

$wpdb->get_row("SELECT * FROM `my_table` WHERE column = '".$_GET['fromUrl']."'", ARRAY_A);

However, I have tried injecting a malicious input with the follow: ' or column = 'value'

And it seems not to work, should I go ahead and prepare my statements anyway?

Yes, you should prepare the statement or at least SQL escape the value of $_GET['fromUrl']. It's good practice to be paranoid about any input potentially provided by the user as it will be wrong at some point.

The wpdb-reference says:

  Briefly, though, all data in SQL queries must be SQL-escaped
  before the SQL query is executed to prevent against SQL injection
  attacks.

The prepared query would look something like this

$wpdb->get_row(
       $wpdb->prepare("SELECT * FROM `my_table` WHERE column = %s",
                      $_GET['fromUrl']),
       ARRAY_A);

which in my opinion is more readable, particularly the SQL query is easier to follow.

Furthermore, inspecting the user input may allow you provide the user better experience by creating meaningful notifications describing what has happened and how the user can cope with the inconvinience.


Python Code Review - Project Euler #12 I just figured out Project Euler problem #12. This the first one I've done in Python, so I'm putting my code out there to hopefully get some constructive criticism.

If anyone is interested, would you mind looking over my solution and letting me know how I can improve on it, either through optimization or style?

Thanks!

from math import*

def divisorCount(n):
    nDivisors = 0

    integerSqrt = int(sqrt(n))

    if sqrt(n) == integerSqrt:
        nDivisors += 1

    for i in range(1, integerSqrt - 1):
        if n % i == 0:
            nDivisors += 2

    return nDivisors

def main():

    divisors = 0

    incrementTriangle = 1
    triangleNumber = 1

    while True:

        divisors = divisorCount(triangleNumber)

        if divisors < 500:
            incrementTriangle += 1
            triangleNumber = triangleNumber + incrementTriangle
        elif divisors >= 500:
            print "The first triangle number w/ 500+ divisors is", triangleNumber
            return 0

main()

Cygal's answer covers style, but you also asked for optimisation so here goes.

Your divisorCount can be a lot more efficient. The number of divisors can be calculated cia the following formula:

d(n) = prod((k + 1)) for a^k in the prime factorisation of n

For example, consider the prime factorisation of 360:

360 = 2^3 * 3^2 * 5^1

The exponents in the prime factorisation are 3, 2, and 1, so:

d(360) = (3 + 1) * (2 + 1) * (1 + 1)
       = 24

Note that 360 indeed has 24 factors: [1, 2, 3, 4, 5, 6, 8, 9, 10, 12, 15, 18, 20, 24, 30, 36, 40, 45, 60, 72, 90, 120, 180, 360].

Combine this with the fact that a number n can have at most one prime factor greater than sqrt(n), and you get something like the following:

# Calculates the number of positive integer divisors of n, including 1 and n
def d(n):
    divisors = 1
    stop = math.sqrt(n)

    primes = # Calculate prime numbers up to sqrt(n) in ascending order
    for prime in primes:
        if prime > stop:
            break

        exponent = 0
        while n % prime == 0:
            n /= prime
            exponent += 1

        if a != 0:
            divisors *= (a + 1)
            stop = max(stop, n)

    # At this point, n will either be 1 or the only prime factor of n > root
    # in the later case, we have (n^1) as a factor so multiply divisors by (1 + 1)
    if n > root:
        divisors *= 2

    return divisors

Since you are going to be calling this function many times consecutively, it makes sense to pre-calculate a list of primes and pass it to the divisor function rather than calculating it each time. One simple and efficient method is the Sieve of Eratosthanes which can be implemented very easily:

# Compute all prime numbers less than k using the Sieve of Eratosthanes
def sieve(k):
    s = set(range(3, k, 2))
    s.add(2)

    for i in range(3, k, 2):
        if i in s:
            for j in range(i ** 2, k, i * 2):
                s.discard(j)

    return sorted(s)

This function can also fairly easily be modified to compute a range in pieces, e.g. to calculate the primes to 1M and then to 2M without recalculating the first 1M.

Putting this together your main loop will then look something like this:

primes = [2, 3, 5]
triangularNumber = 0
increment = 1
maxDivisors = 0
while maxDivisors < 500:
    triangularNumber += increment
    increment += 1

    if math.sqrt(triangularNumber) > primes[-1]:
       primes = sieve(primes, primes[-1] * 4)

    divisors = d(triangularNumber, primes)
    maxDivisors = max(divisors, maxDivisors)

print triangularNumber, "has", maxDivisors, "divisors."

As far as performance goes, here are the triangular numbers with the largest number of divisors and the times (in seconds) taken to get there (py1 is your code, py2 is using primes, c++ is the same as py2 but in c++):

TriNum      Value     d(n)  py1  py2  c++
 12375      76576500   576    0    0    0
 14399     103672800   648    0    0    0
 21735     236215980   768    1    0    0
 41040     842161320  1024    4    0    0
 78624    3090906000  1280  128   13    9
 98175    4819214400  1344  350   23   15
123200    7589181600  1512  702   38   23
126224    7966312200  1536  749   41   24
165375   13674528000  1728   -    74   42
201824   20366564400  1920   -   107   61
313599   49172323200  2304   -   633  153
395199   78091322400  2880   -   790  259
453375  102774672000  3072   -   910  358

There are probably other optimisations you can make based on the properties of triangular numbers, but I don't know the math. :)


Fast .slk (SYLK) file parser For my project I need a (partial) .slk (SYLK) parser which need to load 10+ fairly large tables (30.000-50.000 lines) quickly. The subset is very limited to just the following format:

  ID;PWXL;N;E

Always first and it's only important if "ID" is there

  B;X79;Y838;D0

The size of the table, always comes before "C" rows. X is the width and Y is the height.

  C;X25;K0.25
  C;X26;K"always"
  C;X26;Y32;K"never"

X is the column and an optional Y sets the row (if missing the last row or 0 is used). The "K" part can either be in quotes or without. Either way the storage format is std::string (for now).

Now for the actual code

std::vector<std::vector<std::string>> table_data;
std::unordered_map<std::string, int> header_to_column;
std::unordered_map<std::string, int> header_to_row;

std::ifstream stream(path);

std::string line;

size_t position = 0;
size_t length = 0;

size_t column = 0;
size_t row = 0;

const auto parse_int_part = [&]() {
    position++;
    length = line.find_first_of(';', position) - position;
    position += length;
    return std::stoi(line.substr(position - length, length));
};

if (std::getline(file, line)) {
    if (line.substr(0, 2) != "ID") {
        std::cout << "Invalid SLK file, does not contain \"ID\" as first record" << std::endl;
        return;
    }
} else {
    return;
}

while (std::getline(stream, line)) {
    position = 2;

    switch (line.front()) {
        case 'B':
            while (true) {
                switch (line[position]) {
                    case 'X':
                        columns = parse_int_part();
                        break;
                    case 'Y':
                        rows = parse_int_part();
                        break;
                    default:
                        table_data.resize(rows, std::vector<std::string>(columns));
                        goto nextline;
                }
                if (position < line.size() - 1) {
                    position++;
                }
            }
        case 'C':
            while (true) {
                switch (line[position]) {
                    case 'X':
                        column = parse_int_part() - 1;
                        break;
                    case 'Y':
                        row = parse_int_part() - 1;
                        break;
                    case 'K': {
                        position++;
                        if (line[position] == '\"') {
                            position++;
                        }

                        length = line.size() - position - ((line.back() == '\r') ? 1 : 0);
                        const std::string part = line.substr(position, length - ((line[position + length - 1] == '\"') ? 1 : 0) );

                        if (row == 0) {
                            header_to_column.emplace(part, column);
                        }

                        if (column == 0) {
                            header_to_row.emplace(part, row);
                        }
                        table_data[row][column] = part;
                        goto nextline;
                    }
                }
                if (position < line.size() - 1) {
                    position++;
                }
            }
        case 'E':
            goto exitloop;
    }
    nextline:
    position = 2;
}
exitloop:;

I use some gotos which are probably ok to use here since the other solutions are a lot more verbose, but are apparently bad practice? And since the file may have "\r\n" line endings those have to be handled manually, but it feels like there should be a better way.

How could the parsing be improved while keeping the same performance?

Edit
I've changed the example so it can be run easily and a simple [test .slk].3.

I gave it a try compiling it on my machine, but there are too many things still missing, so this will be mainly based on visual inspection of the code.
Functions
I would break out various pieces of this into separate named functions. It will greatly improve readability, and likely they'll be small enough that they'll be inlined, anyway, so it probably won't hurt speed. First, reading the header line could be its own function:
bool check_SLK_header(std::fstream file)
{
    bool header_is_good = false;
    std::string line;
    if (std::getline(file, line)) {
        if (line.substr(0, 2) == "ID") {
            header_is_good = true;
        }
    }
    return header_is_good;
}

I would leave printing the error message to the caller of the main function (which you haven't named, so I can't reference it). That separates out the logic of parsing the file from the display of data to the user, which is usually a good thing.
Next I would break out the various cases of the switch statement into their own functions, too. I would name them something like parse_dimensions_record() for the B case, and parse_cell_record() for the C case. I'll talk about the E case below.
Avoid Infinite Loops
This pattern of writing an infinite loop that has some special "break-out" criteria hidden in the middle of it is extremely hard to read and follow. This is one of the problems with using goto, break (outside of a switch statement), continue, etc. It obscures the flow of the code. You can almost always rewrite an infinite loop better with an actual exit criteria. Here's how I'd write the parse_dimensions_record() function for the B case:
void parse_dimensions_record(const std::string& line, std::vector<std::vector<std::string>>& table_data)
{
    int columns = -1;
    int rows = -1;
    size_t position = 2;
    while (((columns < 0) && (rows < 0)) && (position < line.size() - 1))
    {
        switch (line [ position ])
        {
            case 'X':
                columns = parse_int_part();
                break;
            case 'Y':
                rows = parse_int_part();
                break;
        }
        position++;
    }

    if ((columns >= 0) && (rows >= 0))
    {
        table_data.resize(rows, std::vector<std::string>(columns));
    }
}

It now becomes clear that there are some edge cases that might warrant handling. First, does it make sense to have 0 columns or 0 rows? If not, you should be returning an error and stop parsing, probably, when you hit that case. If so, then you can also probably end early, as there's no point in reading the rest of the file, as there's no columns or rows to hold any data.
You also don't handle malformed data, such as an invalid specifier in the B record, for example. Is it safe to ignore those? Does it indicate a corrupted file? You probably need to return an error code and handle that case in some way.
I mentioned above that I'd talk about the E case below. Well here we are. The main loop for parsing the file isn't an infinite loop which is good, but you still don't need the goto you have in there. You can end it by just keeping around a variable that says whether you've hit the end record. Something like this:
bool hit_end = false;
while ((std::get line(stream, line)) && (!hit_end)) {
    position = 2;

    switch (line.front()) {
        case 'B':
            parse_dimensions_record(line, table_data);
            break;
        case 'C':
            parse_cell_record(line, table_data);
            break;
        case 'E':
            hit_end = true;
            break;
        default:
            // Got some sort of invalid record here. Should probably stop parsing
            break;
    }
}

This is much easier to read and understand and avoids the various goto calls that made the other version difficult to understand.


Matching socks from a bag of socks I was reading the book Algortihms to Live By and was inspired by it to take a shot at simulating How to pair socks from a pile? (sans the efficiently part at the moment).

import random
from operator import add

def get_pair_of_socks(num_of_socks):
    return random.sample(range(num_of_socks), num_of_socks)

def index_to_pull_sock_from(bag_of_socks: list):
    return random.randint(a=0, b=len(bag_of_socks) - 1)

def attempt_counts_matching_socks(num_of_socks_to_consider):
    # Keep attempt counts in this list.
    attempt_counts = []

    # Generate pairs of random socks.
    socks = get_pair_of_socks(num_of_socks_to_consider) + get_pair_of_socks(num_of_socks_to_consider)

    while len(socks) != 0:
        # Pick one pair from the bag..
        first_pair = socks.pop(index_to_pull_sock_from(socks))

        # Pick a second pair..
        random_pick = index_to_pull_sock_from(socks)
        second_pair = socks[random_pick]

        # We did an attempt..
        attempt_count = 1

        # If they matched, perfect. We will never enter this block.
        # Otherwise loop until you do find the match..
        while second_pair != first_pair:
            # Increment the attempt_count whenever you loop..
            attempt_count = attempt_count + 1
            random_pick = index_to_pull_sock_from(socks)
            second_pair = socks[random_pick]

        # Remove the second matching pair from the bag..
        socks.pop(random_pick)

        # Keep the number of attempts it took you to find the second pair..
        attempt_counts.append(attempt_count)

    return attempt_counts

num_of_iterations = 1000
pair_of_socks = 10

# Initalise a list of length `pair_of_socks`
attempt_counts_so_far = [0] * pair_of_socks

for _ in range(num_of_iterations):
    # Get attempt counts for 1 iteration..
    attempt_counts_single_iteration = attempt_counts_matching_socks(pair_of_socks)

    # Add the attempt counts aligned by index. We will be dividing by the total number of iterations later for averages.
    attempt_counts_so_far = list(map(add, attempt_counts_so_far, attempt_counts_single_iteration))

average_takes = list(map(lambda x: x / num_of_iterations, attempt_counts_so_far))
print(average_takes)
# [18.205, 16.967, 14.659, 12.82, 11.686, 9.444, 7.238, 4.854, 2.984, 1.0]

Your get_pair_of_socks seems to be initializing a pile of socks, so you have the wrong name.

If I understand things correctly, you initialize a pile of "left" socks, then initialize a pile of "right" socks (socks don't have left and right, but the labels make it easier to keep track of what's going on), and then put the pile of "left" socks on top of the "right" pile. So each half of the final pile has only one copy of each sock pair. This is an odd situation to be simulating. I recommend initializing the pile in one go with random.sample(list(range(number_of_socks_to_consider))*2,2*number_of_socks_to_consider).

I think you could just take the sock on the top of the pile without affecting the results, rather than the complicated pop statement you're currently using. (A random element from a random permuted set isn't any more random than the first element from a randomly permuted set).

first_pair seems to be the first sock in the attempted pair, while second_pair is the  second sock, which, again, is poor naming (perhaps you are confusing "pair" with "partner").

I don't think you're following the algorithm describes in that link: that involves taking a sock, then taking the rest of the socks in order, while you are taking a sock, then checking the remaining socks with replacement, which again is making it take longer.

You don't need a separate case to handle the first sock you try to pair with the sock you picked. You can just do this:

    first_pair = socks.pop(index_to_pull_sock_from(socks))
    attempt_count = 0
    while True:
        attempt_count = attempt_count + 1
        random_pick = index_to_pull_sock_from(socks)
        second_pair = socks[random_pick]
        if second_pair == first_pair:
                break


Berlin clock kata in Elixir I'm starting with Elixir and as exercise I wrote the Berlin Clock kata:

The clock is made up of 5 rows.
On the very top of the clock is a lamp that blinks to show the seconds. It turns on for one second, then off for one second, and so on.
The next 2 rows represent the hours. The upper most of these rows represents 5 hour blocks and is made up of 4 red lamps. The second row represents 1 hour blocks and is also made up of 4 red lamps.
The final two rows represent the minutes. The upper most row repesents 5 minute blocks and is made up of 11 lamps- every 3rd lamp red but the rest yellow. The second row represents 1 minute blocks and is made up of 4 yellow lamps.

Here's a picture:

The full code is available here.

import Integer, only: :macros

defmodule BerlinClock do
    def parse (time) do

        [h, m, s] = String.split(time, ":")
            |> Enum.map fn n -> Integer.parse(n)
            |> elem(0) end

        seconds = get_seconds(s)
        single_minutes = get_single_minutes(m)
        fives_minutes = get_fives_minutes(m)
        single_hours = get_single_hours(h)
        fives_hours = get_fives_hours(h)

        [seconds, fives_hours, single_hours, fives_minutes, single_minutes]
    end

    def get_fives_hours(h) do
        number_of_r = div(h, 5)
        String.ljust(String.duplicate("R", number_of_r), 4, ?O)
    end

    def get_single_hours(h) do
        number_of_r = rem(h, 5)
        String.ljust(String.duplicate("R", number_of_r), 4, ?O)
    end

    def get_fives_minutes(m) do
        number_of_y = div(m, 5)
        String.ljust(create_fives_minutes(number_of_y), 11, ?O)
    end

    def create_fives_minutes(1) do
        "Y"
    end

    def create_fives_minutes(n) when n < 1 do
        ""
    end

    def create_fives_minutes(n) when n > 1 do
        if rem(n,3) == 0 do
            light = "R"
        else
            light = "Y"
        end
        [create_fives_minutes(n-1), light ] |> Enum.join
    end

    def get_single_minutes(m) do
        number_of_y = rem(m, 5)
        String.ljust(String.duplicate("Y", number_of_y), 4, ?O)
    end

    def get_seconds(s) do
        cond do
            s |> even? -> "Y"
            s |> odd?  -> "O"
        end
    end
end

Tail Recursion
Erlang (and hence Elixir) pride in the tail recursion idioms they use. You've got one recursion in your code but it is not a tail recursion!

This means that the recursive part is not the last thing in the function, and the tail recursion optimization cannot be used. Although your code's recursion is limited to up to 11 hits, it still misses the point of the Elixir idiom.

A more idiomatic pattern is using an accumulator, and recursing on that:

def create_fives_minutes(m) when m >= 0, do: _create_fives_minutes(m, "")

def _create_fives_minutes(0, acc), do: acc

def _create_fives_minutes(m, acc) when rem(m,3) == 0 do
  _create_fives_minutes(m-1, "R" <> acc)
end

def _create_fives_minutes(m, acc), do: _create_fives_minutes(m-1, "Y" <> acc)

if code smell
In Elixir if and cond are seldom used, and are even considered a code smell - if you can solve the problem using pattern matching - it is considered more idiomatic. Consider get_seconds - the following is more idiomatic in Elixir than using cond:

def get_seconds(s) when even?(s), do: "Y"
def get_seconds(_), do: "O"

Also see above how I've avoided the if in the create_fives_minutes above using a guard condition (when rem(m,3))

Code Duplication
You've got several methods, which do essentially the same thing. Making a method which takes the duplicate code will remove the need for them entirely - something like:

def build_field(pattern, repeat \\ 1, field_length) do
    String.ljust(String.duplicate(pattern, repeat), field_length, ?O)
end

Now - your main code can look something like this:

    seconds = get_seconds(s)
    single_minutes = build_field("Y", rem(m, 5), 4)
    fives_minutes = build_field(create_fives_minutes(div(m, 5)), 11)
    single_hours = build_field("R", rem(h, 5), 4)
    fives_hours = build_field("R", div(h, 5), 4)

and all the helper methods are no longer needed.

Indentation and readability
When parsing the time string you've got a piping block which looks like this:

    [h, m, s] = String.split(time, ":")
        |> Enum.map fn n -> Integer.parse(n)
        |> elem(0) end

This is a very misleading indentation, since you indent elem(0) at the same level as the Enum.map.

A more readable way to put it should be:

    [h, m, s] = String.split(time, ":")
        |> Enum.map fn n ->
            Integer.parse(n)
            |> elem(0)
        end

Showing that |> elem(0) is inside the Enum.map function, and not after it.

Playing with the idioms
You can parse the time string using the recursive method idioms, which would look like this:

def _parse_time("", acc), do: acc

def _parse_time(":" <> s, acc), do: _parse_time(s, acc)

def _parse_time(s, acc) do
    {num, s} = Integer.parse(s)
    _parse_time(s, [num | acc])
end

and used like this:

    [s, m, h] = _parse_time(time, [])

It is not worse or better than your solution, but it uses a different Elixir idiom.


Haskell Convert 1-800 Numbers This little app converts 1-800 numbers (with letters) to actual numbers. I'm very frustrated by the insertDashes function as it's very brute force. I'm sure there's a far more elegant solution. I'd appreciate any feedback on anything else though.

module Dial
    ( enter,
    stripDashes
    ) where

import Data.Char (ord, toLower, isNumber, intToDigit, isPunctuation)
import System.IO
import Data.List

enter = do
    putStrLn "Enter a phone number"
    num <- getLine
    let converted = insertDashes $ convertLetters $ stripDashes num
    putStrLn $ "You may dial " ++ converted

stripDashes :: String -> String
stripDashes xs = [ x | x <- xs, not (elem x  "-") ]

insertDashes :: String -> String
insertDashes xs = take 1 xs ++ ['-'] ++ take 3 (drop 1 xs) ++ ['-'] ++ take 3 (drop 4 xs) ++ ['-'] ++ take 4 (drop 7 xs)

getDigit :: Char -> Int
getDigit letter
    | (ord letter >= 97) && (ord letter <= 99) = 2
    | ord letter <= 102 = 3
    | ord letter <= 105 = 4
    | ord letter <= 108 = 5
    | ord letter <= 111 = 6
    | ord letter <= 115 = 7
    | ord letter <= 118 = 8
    | ord letter <= 122 = 9
    | otherwise = 0

convertLetters :: String -> String
convertLetters xs = map (\x -> if (isNumber x) then x else (intToDigit $ getDigit $ toLower x)) xs

Type signatures

Good job on providing all type signatures. Unfortunately, you forgot one: enter. Always include type signatures on top-level functions.

Remove unused imports

You use neither Data.List, nor System.IO.

Prefer character comparison instead of ord x > n

In getDigit, you compare characters by their ord values. That's not easy on the eye. It's a lot easier to read if you just compare the letter with characters:

getDigit :: Char -> Int
getDigit letter
    | letter >= 'a' && letter <= 'c' = 2
    | letter <= 'f' = 3
    | letter <= 'i' = 4
    | letter <= 'l' = 5
    | letter <= 'o' = 6
    | letter <= 's' = 7
    | letter <= 'v' = 8
    | letter <= 'z' = 9
    | otherwise = 0

There are other ways to write getDigit, but I they differ on personal preference only.

Use the standard library

stripDashes is filter (/= '-') in disguise:

stripDashes xs = [ x | x <- xs, not (elem x  "-")]
               = [ x | x <- xs, x /= '-']
               = filter (/= '-') xs

Therefore use filter instead of a list comprehension:

stripDashes :: String -> String
stripDashes = filter (/= '-')

By the way not (elem x y) is notElem x y.

Prefer local bindings instead of long lines

This is personal preference, but I think that

convertLetters :: String -> String
convertLetters xs = map toDigit xs
  where
    toDigit x
     | isNumber x = x
     | otherwise  = intToDigit $ getDigit $ toLower x

is easier to read.

Use hlint to get rid of redundant brackets

You have a lot of redundant brackets, which can make the code hard to read. hlint can report them to you. It also reports the not (elem x y) == notElem x y hint, by the way.

Prefer pure functions if possible

You have enter for a complete transformation. It would be great if there was a non-IO variant too, e.g.

convert :: String -> String
convert = insertDashes . convertLetters . stripDashes

That way you can export enter, conver and stripDashes.

Use splitAt instead of take/drop combinations

Finally, we have a look at insertDashes. First of all, let's write bindings:

insertDashes :: String -> String
insertDashes xs = group1 ++ ['-'] ++ group2 ++ ['-'] ++ group3 ++ ['-'] ++ group4
   where
      group1 = take 1 xs
      group2 = take 3 (drop 1 xs)
      group3 = take 3 (drop 4 xs)
      group4 = take 4 (drop 7 xs)

That looks like a perfect job for splitAt:

-- do not include this function in your code, it's part of the Prelude
splitAt n xs = (take n xs, drop n xs)

However, if we were to rewrite insertDashes with splitAt, it wouldn't get any better immediately:

import Data.List (intercalate)

insertDashes :: String -> String
insertDashes xs = group1 ++ ['-'] ++ group2 ++ ['-'] ++ group3 ++ ['-'] ++ group4
   where
      (group1,ys1) = splitAt 1 xs
      (group2,ys2) = splitAt 3 ys1
      (group3,ys3) = splitAt 3 ys2
      (group4,_  ) = splitAt 4 ys3

Ugh. Not really any better. So instead let's write another function:

splitAtNs :: [Int] -> [a] -> [[a]]
splitAtNs []     xs = [xs]
splitAtNs (n:ns) xs = as : splitAtNs ns bs
  where (as, bs) = splitAt n xs

Now  insertDashes is

insertDashes :: String -> String
insertDashes xs = group1 ++ ['-'] ++ group2 ++ ['-'] ++ group3 ++ ['-'] ++ group4
   where
      (group1:group2:group3:group4:_) = splitAtNs [1,3,3,4] xs

Which almost looks fine. There is one more improvement, though. The function intercalate from Data.List brings it down to a single line:

insertDashes :: String -> String
insertDashes = intercalate "-" . take 4 . splitAtNs [1,3,3,4]


Javascript get item in object code So I have some Javascript code that resembles the following:

var mylibrary = new (function ()
{

    this._getLibraryObj = function ()
    {
        var newLibraryObj = {};
        var libraryData = window.specifiedLibraryData;
        // Add head librarian details
        // open library
        // etc. etc. turn into an object from data

        return newLibraryObj ;
    };

    this.item = function (item)
    {

        if (this.obj && item in this.obj)
        {
            return this.obj[item];
        }
        else
        {
            this.obj = this._getLibraryObj();
            if (item in this.obj)
            {
                return this.obj[item];
            }
            return null;
        }
    };

    return this;
})();

What annoys me is that this.item(item) function repeats the item in this.obj[item] and I find it a little hard to read.  I was hoping for it to not have to run _getLibraryObj too often.

Question: How would you rewrite this.item() to be short and sweet (DRY and readable)?

EDIT:- From alex's post I realised I left out one constraint: If the item isn't in the library object then it might need to _getLibraryObj() again as the data may have changed

Solution:- I ended up using @Alex Nolan's fifth example (with || instead of && I find it easier to read.):

this.item = function(item)
{
    if(!this.obj || !item in this.obj)
    {
        this.obj = this._getLibraryObj();
    }

    return this.obj[item] || null;
}

Another downfall of your this.item() function is that if item isn't in this.obj, this._getLibraryObj() will be called every time.

Try

this.item = function(item) {
    if (this.obj) {
        return this.obj[item] || null;
    } else {
        this.obj = this._getLibraryObj();
        return this.item(item);
    }
};

Or if you want it even more minimal:

this.item = function(item) {
    if (this.obj)
        return this.obj[item] || null;

    this.obj = this._getLibraryObj();
    return this.item(item);
};

If you switch up the statements a little bit you get the most readable IMHO:

this.item = function(item) {
    if (!this.obj)
        this.obj = this._getLibraryObj();

    return this.obj[item] || null;
};

One final way to do it would be to do:

this.item = function(item) {
    this.obj = this.obj || this._getLibraryObj();

    return this.obj[item] || null;
};

EDIT:

In light of your comment about needing to re-call getLibraryObj

this.item = function(item) {
    if (!(this.obj && item in this.obj))
        this.obj = this._getLibraryObj();

    return this.obj[item] || null;
};

or:

this.item = function(item) {
    this.obj = this.obj && item in this.obj ? this.obj : this._getLibraryObj();

    return this.obj[item] || null;
};

or a one-liner:

this.item = function(item) {
    return (this.obj = (this.obj && item in this.obj ? this.obj : this._getLibraryObj()))[item] || null;
};


Triggering the execution of a job at the end of a timed campaign I have a task that checks for campaigns that are supposed to be finished by checking the end_at date. If finished, then set the status of the campaign to finished, and send the owner of the campaign an email.

The way I see it, is approach 1 and 2 may have the worker queued more than once in the worker pool.

Approach 3, always makes sure that there is only one instance running at a time.
Approach 3 also means making sure the worker is run once on every deploy.

I have 3 scenarios to consider implementing this:

APPROACH #1 - Run the cron job. Cron job determines which campaigns the worker should process

# CRON via whenever gem that runs every 3 minutes
campaigns = Campaign.where("properties -> 'end_at' <= ? AND status = ?", Time.now.utc, 'running')

campaigns.each do |campaign|
  FinishCampaignWorker.perform_async(campaign.id)
end

# FinishCampaignWorker class
class FinishCampaignWorker
  include Sidekiq::Worker

  sidekiq_options({
    retry: false,
    backtrace: true
  })

  def perform(campaign_id)
    begin
      campaign = Campaign.unscoped.find(campaign_id)

      # Do not update campaign if deleted or deactivated
      if campaign.present? && !%w(deleted deactivated).include?(campaign.status)
        campaign.status = 'finished'

        UserMailer.post_campaign_notification(campaign.id, campaign.user.id).deliver

        campaign.save
      end
    rescue
      Airbrake.notify(
        error_class:      'Finish campaign worker',
        error_message:    "#{$!}",
        environment_name: Rails.env
      )
    end
  end
end

APPROACH #2 - Cron triggers the worker to run every 3 minutes. Let the worker handle most of the logic

# CRON via whenever gem that runs every 3 minutes
FinishCampaignWorker.perform_async

# FinishCampaignWorker class
class FinishCampaignWorker
  include Sidekiq::Worker

  sidekiq_options({
    retry: false,
    backtrace: true
  })

  def perform
    begin
      campaigns = Campaign.where("properties -> 'end_at' <= ? AND status = ?", Time.now.utc, 'running')

      campaigns.each do |campaign|
        campaign = Campaign.unscoped.find(campaign_id)

        # Do not update campaign if deleted or deactivated
        if !%w(deleted deactivated).include?(campaign.status)
          campaign.status = 'finished'

          UserMailer.post_campaign_notification(campaign.id, campaign.user.id).deliver

          campaign.save
        end
      end
    rescue
      Airbrake.notify(
        error_class:      'Finish campaign worker',
        error_message:    "#{$!}",
        environment_name: Rails.env
      )
    end
  end
end

APPROACH #3 - Run the worker from console/chef/other on first deployment, let the worker handle most of the logic, and then reschedule the worker 3 minutes after completion

# FinishCampaignWorker class
class FinishCampaignWorker
  include Sidekiq::Worker

  sidekiq_options({
    retry: false,
    backtrace: true
  })

  def perform
    begin
      campaigns = Campaign.where("properties -> 'end_at' <= ? AND status = ?", Time.now.utc, 'running')

      campaigns.each do |campaign|
        campaign = Campaign.unscoped.find(campaign_id)

        # Do not update campaign if deleted or deactivated
        if !%w(deleted deactivated).include?(campaign.status)
          campaign.status = 'finished'

          UserMailer.post_campaign_notification(campaign.id, campaign.user.id).deliver

          campaign.save
        end
      end
    rescue
      Airbrake.notify(
        error_class:      'Finish campaign worker',
        error_message:    "#{$!}",
        environment_name: Rails.env
      )
    end

    FinishCampaignWorker.perform_at(3.minutes.from_now)
  end
end

Your problem is an integration and management problem, rather than a programming problem (though there are some issues there too).
Approach 3
Approach-3 is a problem, and I would not use it for your task. Repetitive tasks require a well designed and reliable infrastructure to handle them. The quirks of time are such that things like daylight-saving, system clock resets, and other issues will play havoc with things. For example, What will your task do when you 'spring forward' in daylight savings... will it fail because the next scheduled time never happens? In fall, will it go idle for over an hour when the same hour is repeated?
Cron (and on many Linux distributions, Anacron) has very well documented practices and behaviours for these types of issues, and the predictable, and reasonable use case makes a lot of sense. Use Cron.
Additionally, if there is an issue sending a mail, or anything requiring rescue in Approach 3, you will fail to reschedule, and your program will break.
Approach 2
This is actually quite similar to Approach 1, but it just shifts more logic in to the process method, and it mixes up the responsibilities in that method. SOLID principles suggest having just a single responsibility in each method, so I don't like this approach. Additionally, you can only process one campaign at a time using this approach.
Too much happening in one place, and possible opportunities for parallelism are removed.
Approach 1
I prefer this approach for the relative simplicity of the single-campaign method, and the way the campaigns are processed.
Improvements
Having said that, there are problems still. The biggest is that your program is not defensive enough. Even though you expect only one instance of the program to be running at any one time, you should still program defensively and assume there could be more.
Cron (anacron) will ensure that it does not start a second instance of the program if the previous instance has not yet completed, so cron will not cause  a problem, but you may have some mix up happening in your testing, or you may run it yourself when cron has it running already, leading to problems from there.
I am not very familiar with RoR, but the following looks like it could lead to conflicts and race conditions with other running processes:

  # Do not update campaign if deleted or deactivated
  if campaign.present? && !%w(deleted deactivated).include?(campaign.status)
    campaign.status = 'finished'

    UserMailer.post_campaign_notification(campaign.id, campaign.user.id).deliver

    campaign.save
  end

Posting the campaign e-mail could be a slow task and (even if it was fast) there is the possibility that another instance of the program may be processing the same campaign at the same time. If you are using a database for your campaigns you should consider a different approach. I recommend:

a transactional approach which checks to see whether the campaign is state='finished', updates it to finished, and returns the previous state, and then commits the transaction.
this guarantees the state will be marked as finished in a single 'atomic' operation
it saves the Campaign state before sending the mail.
there is only a short period of time while the campaign is 'locked'
if the previous state was not 'finished' then send the mail.

At worst case, now, the only risk is that the mail fails to send. That should be logged in a reliable way that allows you to (manually?) identify, resolve, and resend the mails.
Note that a failed mail will fail whether you mark the Campaign complete or not, and repeatedly resending the mail will not make it suddenly work. There is no reason to save the campaign after the mail-send.


Tidying and deduplicating files listed in a DataGridView I have the following code that searches through folder directories in a DataGridView table, and puts all files of the wanted format into a list, it also gathers a list of their last modified date for later use in the application.

The code works, but it is sore on the eyes. I want to tidy up the following loops to improve efficiency - what I mean is that I have a For loop within a For loop that creates the list of filenames, then I have two separate Do Until loops that search through the list from start to finish to pick out file names that need adjustment.

I would be very interested to learn a better way of achieving the same result, as my knowledge of efficiency in coding is quite elementary. Basically, can this be done in one or two loops, as the idea of looping through the Lists twice seems inefficient?

Public Class
Private Sub btnDirectory_Click(sender As Object, e As EventArgs) Handles btnDirectory.Click

Dim FileNames As New List(Of String)
Dim FileDates As New List(Of Date)

Dim DocNo As String
Dim rowCheck As String
Dim ProjectNo As String = "1111"
Dim FileNameCheck As String
Dim str As String

Dim k As Integer = 0
Dim i As Integer
Dim j As Integer

'The first loop grabs all files of the wanted format from a datagridview table containing all directories to be checked

For Each rw In Background.Table1.Rows

    rowCheck = Background.Table1(0, k).Value
    If Not String.IsNullOrEmpty(rowCheck) Then

    For Each file As String In My.Computer.FileSystem.GetFiles(Background.Table1(0, k).Value)
        CorrectType = False
        FileNameCheck = IO.Path.GetFileNameWithoutExtension(file)
        If FileNameCheck.Contains(ProjectNo) AndAlso FileNameCheck.Contains("-") AndAlso Not String.IsNullOrEmpty(FileNameCheck) AndAlso FileNameCheck.Contains(" ") Then
            DocNo = FileNameCheck.Substring(0, FileNameCheck.IndexOf(" "))
            If FileNameCheck.Substring(0, FileNameCheck.IndexOf("-")) = ProjectNo AndAlso CountLetters(DocNo) = 3 Then
                FileNames.Add(FileNameCheck)
                FileDates.Add(IO.File.GetLastWriteTime(file))
            End If
        End If

    Next
    End If

    k += 1

Next

'The next loop tidies up the file formats that contain a "-00-" in their names

 j = FileNames.Count
 i = 0
 Do
     str = FileNames(i)
     If str.Contains("-00-") Then
         FileNames(i) = RemoveChar(str, "-00-") ' RemoveChar is a function that replaces "-00-" with a "-"
     End If
     i += 1
 Loop Until i = j

 i = 0
 j = FileNames.Count

 'Finally, this loop checks that no two files have the exact same name, and gets rid of one of them if that is the case

 Do

     Dim st1 As String = FileNames(j - 1)
     Dim st2 As String = FileNames(j - 2)

     If st1 = st2 Then
         FileNames.RemoveAt(j - 1)
         FileDates.RemoveAt(j - 1)
     End If
     j -=  1
 Loop Until j = 1

 End Sub

End Class

I found some code that you could tidy up a bit right here

    j = FileNames.Count
    i = 0
    Do
        str = FileNames(i)
        If str.Contains("-00-") Then
            FileNames(i) = RemoveChar(str, "-00-") ' RemoveChar is a function that replaces "-00-" with a "-"
        End If
        i += 1
    Loop Until i = j

that seems like a lot of work for such a simple task.

use a for loop that goes from the base index to the FileNames.Count value.

Then use a Replace instead of creating a function that does a replace.  here is some code that does the same thing

keep in mind that a replace will only replace a string that it can find, so you don't have to check for the substring in your string before you call the Replace function on the string.

For i As Integer = 0 To FileNames.Count
    FileNames(i).Replace("-00-", "-")
Next


Decode the Morse Code From Code Wars:

  This kata is part of a series on the Morse code. After you solve this kata, you may move to the next one.

  In this kata you have to write a simple Morse code decoder. While the Morse code is now mostly superceded by voice and digital data communication channels, it still has its use in some applications around the world.
  The Morse code encodes every character as a sequence of "dots" and "dashes". For example, the letter A is coded as ·−, letter Q is coded as −−·−, and digit 1 is coded as ·−−−−. The Morse code is case-insensitive, traditionally capital letters are used. When the message is written in Morse code, a single space is used to separate the character codes and 3 spaces are used to separate words. For example, the message HEY JUDE in Morse code is ···· · −·−− ·−−− ··− −·· ·.

  NOTE: Extra spaces before or after the code have no meaning and should be ignored.

  In addition to letters, digits and some punctuation, there are some special service codes, the most notorious of those is the international distress signal SOS (that was first issued by Titanic), that is coded as ···−−−···. These special codes are treated as single special characters, and usually are transmitted as separate words.

  Your task is to implement a function that would take the morse code as input and return a decoded human-readable string.

  For example:

decodeMorse('.... . -.--   .--- ..- -.. .')
#should return "HEY JUDE"

  NOTE: For coding purposes you have to use ASCII characters . and -, not Unicode characters.

  The Morse code table is preloaded for you as a dictionary, feel free to use it:

  Coffeescript/C++/Go/JavaScript/PHP/Python/Ruby/TypeScript: MORSE_CODE['.--']
  C#: MorseCode.Get(".--") (returns string)
  Elixir: morse_codes variable
  Haskell: morseCodes ! ".--" (Codes are in a Map String String)
  Java: MorseCode.get(".--")
  Kotlin: MorseCode[".--"] ?: "" or MorseCode.getOrDefault(".--", "")
  Rust: self.morse_code
  All the test strings would contain valid Morse code, so you may skip checking for errors and exceptions. In C#, tests will fail if the solution code throws an exception, please keep that in mind. This is mostly because otherwise the engine would simply ignore the tests, resulting in a "valid" solution.

  Good luck!

  After you complete this kata, you may try yourself at Decode the Morse code, advanced.

I wrote a program that works, but it was essentially done with ad hoc patches to cover the edge cases/exceptions the program initially didn't cover. I'd appreciate a critique of my code, such as best practices and the program logic itself.

# "Code Wars: Decode the Morse Code"

def decodeMorse(morse_code):
    clear_text = ''
    char = ''
    index = 0
    length = len(morse_code)
    delim1 = " "    # Next character delimiter.
    delim2 = "   "  # Next word delimiter.
    while index < (length):
        if morse_code[index] != delim1:
            # Build the character to be added to the clear text.
            char += morse_code[index]
        else:
            # When the delimiter is encountered.
            if char != '':
                # To cover the program encountering erroneous whitespace.
                clear_text += MORSE_CODE[char]
                    # Add said character to clear text
                char = ''
                    # Reset "char".
                if index < (length-2):
                    # If it is possible to encounter a space.
                    if morse_code[index:(index+3)] == delim2:
                        # When a space is encountered.
                        clear_text += " "
                        index += 2
                        if index == length-1:
                            # If the last character in the code is a space, assign a control value to "char"
                            char = ""
        index += 1

    if char != "":
        # If the last character isn't a space.
        clear_text += MORSE_CODE[char]
            # Add the final character to the clear text.
    return clear_text

Iterating in Python

In Python if you find yourself writing something like:

while index < (length):

There is good chance you are not writing Pythonic code.  Looping over an index is just not needed that often.  I will start with showing some ways to improve your code.  And then at the end will show a more Pythonic solution.

I would suggest changing your looping statement to:

for index, di_dah in enumerate(morse_code.strip() + ' '):

enumerate() is used to iterate over an object and give the index of of the current element.  The str.strip() removes leading and trailing spaces, and the + ' ' at the end removes the need special casing after the loop.  This fairly simple change can greatly simplify the code needed inside the loop:

def decodeMorse2(morse_code):
    clear_text = ''
    char = ''

    for index, di_dah in enumerate(morse_code.strip() + ' '):
        if di_dah != ' ':
            char += di_dah
        elif char:
            clear_text += MORSE_CODE[char]
            char = ''
            if index < len(morse_code) - 2:
                if morse_code[index: index + 3] == '   ':
                    clear_text += " "

    return clear_text

More Pythonic?

I don't have access to the test cases, but I read the spec I think this would do the job:

def decodeMorse(morse_code):
    return ' '.join(
        ''.join(MORSE_CODE[char] for char in word.split())
        for word in morse_code.strip().split('   ')
    )

How does this work?

This code uses two nested loops in the form of generator expressions.  It uses str.split() to initially split the words in the outer loop and then again to split the characters in the inner loop.  Then does a character lookup and finally uses str.join() to build the sentence.


Get average of each nested array in dynamic object I'm iterating this JSON using a for loop:

var movieObj = {
    "cat": {
        "moveName": "Breath",
        "rating": [4, 5, 4, 3]
    },
    "cat1": {
        "moveName": "shallow",
        "rating": [5, 5, 5, 5, 4]
    }
}

Is there any way to improve this code?

var sum;

Array.prototype.getSum = function (ele) {
    sum = ele.reduce(function (a, b) {
        return a + b
    });
    return sum / ele.length
}

for (var key in movieObj) {
    var b = movieObj[key];

    if (Array.prototype.getSum(b['rating']) < 5) {
        console.log(b['moveName']);
    }
}

First off, you should ensure that each key is a properly defined member of movieObj, not part of the prototype:

for (var key in movieObj) {
    if (movieObj.hasOwnProperty(key)) {
        // ...
    }
}

Instead of declaring getSum on Array, you can use the builtin reduce function:

var sum = function(previous, current) {
    return previous + current;
};

var ratings = movieObj[key].rating;

if (ratings.reduce(sum) / ratings.length < 5) {
    // ...
}

Finally, your console.log statement can use direct property access, instead of square bracket notation.  I believe moveName is a typo, and should in fact be movieName, but correct me if I'm wrong there:

console.log(movieObj[key].movieName);

Putting it all together gives:

var sum = function(previous, current) {
    return previous + current;
};

for (var key in movieObj) {
    if (movieObj.hasOwnProperty(key)) {
        var ratings = movieObj[key].rating;

        if (ratings.reduce(sum) / ratings.length < 5) {
            console.log(movieObj[key].movieName);
        }
    }
}


Simple REST API server This is a chunk of code I wrote for a job interview at some point - it is a remote key-value storage REST API supporting CRUD operations.

Maybe anyone has some feedback on this? It would be very helpful for me.

I did not attach the tests, since it might be a little bit too much code. You don't have to review both files - even commenting on just one can be extremely helpful.

remotekvs.py

# -*- test-case-name: tests.remotekvs_test.RemoteKVSTest -*-

import json
import inspect

from twisted.internet import reactor
from twisted.web.resource import Resource
from twisted.web.server import Site

from keyvaluestorage import KeyValueStorage

class RemoteKVSResource(Resource):
    """
    This is a Twisted Resource object used to serve requests for
    KeyValueStorage operations. It supports POST, GET, PUT and DELETE
    request methods and replies in JSON.
    """
    isLeaf = True

    def __init__(self):
        self.kvs = KeyValueStorage()

    def render(self, request):
        operations = {
            'POST': 'create',
            'GET': 'read',
            'PUT': 'update',
            'DELETE': 'delete'}
        if request.method in operations.keys():
            response = {
                'success': True}
            try:
                operation = getattr(self.kvs, operations[request.method])
                args = {k: v[0] if isinstance(v, list) else v
                        for k, v in request.args.items()}
                result = operation(**(args))
                if result is not None:
                    response['result'] = result
            except (TypeError, KeyValueStorage.KeyAlreadyExistsError,
                    KeyValueStorage.InvalidKeyError), exception:
                response['success'] = False
                response['error'] = exception.__class__.__name__
                if isinstance(exception, TypeError):
                    response['error_message'] = (
                        "Valid request parameters: %s." % (
                            ', '.join(inspect.getargspec(operation)[0][1:])))
                else:
                    response['error_message'] = str(exception)
        else:
            response = {
                'success': False,
                'error': 'InvalidRequestMethod',
                'error_message': (
                    "Valid request methods: POST, GET, PUT, DELETE.")}
        return json.dumps(response)

if __name__ == '__main__':  # pragma: no cover
    site = Site(RemoteKVSResource())
    reactor.listenTCP(2048, site)
    reactor.run()

keyvaluestorage.py

# -*- test-case-name: tests.keyvaluestorage_test.KeyValueStorageTest -*-

class KeyValueStorage(object):
    """The most basic key-value storage, supports all 4 CRUD operations."""

    class KeyAlreadyExistsError(Exception):
        """Key you are trying to create already exists in the database."""

    class InvalidKeyError(Exception):
        """Key you are trying to access does not exist in the database."""

    def __init__(self):
        self.storage = {}

    def create(self, key, value):
        if key in self.storage.keys():
            raise self.KeyAlreadyExistsError(
                ("Key already exists. KeyValueStorage does not allow "
                 "duplicate keys."))
        self.storage[key] = value

    def read(self, key):
        try:
            return self.storage[key]
        except KeyError:
            raise self.InvalidKeyError(
                "Key does not exist. You can not read by a nonexistent key.")

    def update(self, key, value):
        if key not in self.storage.keys():
            raise self.InvalidKeyError(
                "Key does not exist. You can not update by a nonexistent key.")
        self.storage[key] = value

    def delete(self, key):
        try:
            del self.storage[key]
        except KeyError:
            raise self.InvalidKeyError(
                "Key does not exist. You can not delete by a nonexistent key.")

I'd write the error handling this way:

def render(self, request):
    operations = {
        'POST': 'create',
        'GET': 'read',
        'PUT': 'update',
        'DELETE': 'delete',
    }
    if request.method not in operations:
        response = {
            'success': False,
            'error': 'InvalidRequestMethod',
            'error_message': ("Valid request methods: %s" % (', '.join(operations.keys()))),
        }
        request.setResponseCode(http.NOT_ALLOWED)
    else:
        response = {'success': True}
        ...
    return json.dumps(response)

Note the changes:

Since the error handler is simpler than the main processing code, put it first to get it out of the way.
if request.method not in operations reads better than if request.method not in operations.keys() and might be slightly more efficient.
Reuse operations.keys() for the error message.
Set HTTP status 405 ("Method Not Allowed").  (The constant is in twisted.web.http.)

In KeyValueStorage, I suggest raising the standard KeyError instead of making your own exception classes.


Determine whether any permutation of a given array exists such that the sum of all subarrays of length K are equal I was asked this question in an interview.

  You are given an array consisting of N integers. You need to determine whether any permutation of a given array exists such that the sum of all subarrays of length K are equal. N is divisible by K i.e. N mod K = 0.
  If the array is [1,2,1,3,2,3] and K = 2, it is False.
  If the array is [1,2,1,3,2,3] and K = 3, it is True.

My logic was this.

  If N == K, return True.
  Else store each of the distinct element in the array in a
  dictionary. If the number of distinct elements in the dictionary
  is greater than K, return False.
  Else store the count of each distinct element in the dictionary.
  If the count of any distinct element is less than (N/K), return False.
  Finally return True.

Here is my code in Python:

def check(nums, n, k):
if n == k:
    return True
else:
    my_dict = {}
    for i in nums:
        if i not in my_dict:
            my_dict[i] = 1
        else:
            my_dict[i] += 1
    if len(my_dict) > k:
        return False
    count = int(n/k)
    for i,j in my_dict.items():
        if j < count:
            return False
    return True

Am I doing the right approach ?

Use the collections.Counter instead of keeping a count yourself.

When you have counted all the elements from the list, check if any of their counts is not a factor of \$ N \$ and return False.

Therefore:

from collections import Counter

def check(nums, n, k):
    if n == k:
        return True
    counts = Counter(nums)
    if len(counts) > k:
        return False
    return all((n % x == 0 for x in counts.values()))


Call method by condition I apologize in advance for my English.
I have a user search method. It's parameters - login and strict. The last one determines, which method should have been used for search - Equals (strict is true) or Contains (strict is false). How should I implement it? I could do two different methods, but I think it would be code repeating.
    public async Task<List<UserFindInfoDTO>> FindUserByLoginAsync(string login, bool strict)
    {
        var users = await context.Users
            .Where(u => u.Nickname.Equals(login)) // change on "Contains" if strict is false
            .Select(u => new UserFindInfoDTO
            {
                Status  = u.Status,
                Id = u.Id,
                MiniAvatar =  u.Image,
                Nickname = u.Nickname
            })
            .ToListAsync();
            ...
        return users;
    }

The lambda expression you pass to Where() is where you define the condition, so this is where it makes the most sense to put all the logic for this.
.Where(u => strict ? u.Nickname.Equals(login) : u.Nickname.Contains(login))

Or for higher performance you can conditionally pass a different lambda expression:
Func<User,bool> checker = strict
  ? (Func<User,bool>)((User u) => u.Nickname.Equals(login))
  : (Func<User,bool>)((User u) => u.Nickname.Contains(login));

...

.Where(checker)