Functions for outputting the rows and columns of a matrix I often want to iterate either row-wise or column-wise though a matrix in Julia, so I've created a pair of functions to help:

function cols(x::Matrix)
    function _it()
        for ii in 1:size(x,2)
            produce(x[:,ii])
        end
    end
    Task(_it)
end

function rows(x::Matrix)
    function _it()
        for ii in 1:size(x,1)
            produce(x[ii,:])
        end
    end
    Task(_it)
end

Example of use:

AA = [1 2 3; 1 2 3]

println("Columns are:")
for ii in cols(AA)
    println(ii)
end
println("\nRows are:")
for ii in rows(AA)
    println(ii)
end

Outputs:

Columns are:
[1,1]
[2,2]
[3,3]

Rows are:
[1 2 3]
[1 2 3]

Possible issues:

It seems like this could be one function, that just takes the dimension as a parameter.
This might confound the JIT, which would mean losing out out a bunch of speed. (The Julia JIT can normally vectorize for loops).
I feel like there should be a inbuild function for this, but the closest I know of is mapslice.

I think the most performant solution here would be to create an iterator:

julia> immutable EachRow{T<:AbstractMatrix}
           A::T
       end
       Base.start(::EachRow) = 1
       Base.next(itr::EachRow, s) = (itr.A[s,:], s+1)
       Base.done(itr::EachRow, s) = s > size(itr.A,1)
done (generic function with 48 methods)

julia> AA = [1 2 3; 1 2 3]
       for ii in EachRow(AA)
         println(ii)
       end
[1 2 3]
[1 2 3]

EachCol would be coded similarly.

This is advantageous over your task-based iterator design because Julia is not able to infer the return types from anonymous tasks.  This means that within your loop, the iteration variable ii is typed as Any, so the compiler isn't able to emit efficient type-specific code. The custom iterator is type-stable, so that means that Julia knows that the iteration variable is always going to be an array, allowing it to compile specific and optimized instructions.

To answer a few of your questions: We could similarly make an EachDim iterator which also stored the dimension parameter, but this is a little trickier to write in a type-stable manner.  Since indexing into columns returns a 1-dimensional column vector and indexing rows returns a 2-dimensional row vector, simply using an integer argument 1 or 2 to specify which dimension would result in a type-instability.  We'd need to ensure that the dimension information is stored in the type-domain.  One way to do this would be to use a parametric type… but doing this nicely requires the call-overloading feature from the development 0.4 version.

Also note that Julia's JIT doesn't "vectorize" for loops in the way you're using the word.  It can be thought of more as a just-barely-ahead-of-time compiler that compiles specialized machine code for each function based upon the types of the arguments.  This is why type-stability is so important; when Julia can infer the types precisely the compiled code is typically very similar to what you'd get from equivalent C code.


Checking if string characters can be arranged to form another string I am working on a code project that checks if all the characters in str1 can be arranged to form another string (str2).  If all the characters in str2 are present in str1 (including any repeated characters in str2, i.e. if str2 has two 'a' characters, str1 must correspondingly have two as well), then the function returns true.

For example, here are some test cases below and the intended return:

str1 = 'rkqodlw'
str2 = 'world'
Expected: true

str1 = 'cedewaraaossoqqyt'
str2 = 'codewars'
Expected: true

str1 = 'katas'
str2 = 'steak'
Expected: false

str1 = 'scriptjava'
str2 = 'javascript'
Expected: true

I have a working code solution, but I'd like to refactor for optimal performance as there are a quite a number of random tests (of unknown length) that the code must test against and my code isn't completing the tests in the recommended timeframe.

function stringscrambling(str1, str2) {
  var arr1 = str1.split('');
  var arr2 = str2.split('');
  var index;
  var l = arr2.length;

  while (l--) {
    index = arr1.indexOf(arr2[l]);
    if (index > -1) {
      arr1.splice(index, 1);
    } else {
    return false;
    };
  }
  return true;
}

I originally used a basic for loop (with the array length cached as a variable and not in the for statement) as follows:

for (var i = 0, l = arr2.length; i < l ; i++) {
    index = arr1.indexOf(arr2[i]);
    if (index > -1) {
      arr1.splice(index, 1);
    } else {
      return false;
    };
  }
  return true;
 }

but tried the while-loop in reverse as I read that specific looping would offer a bit better in terms of benchmarked performance. The change in loop offered a negligible performance upgrade.

Are there any other specific tricks I can implement on my code that can speed it up and still retain the original functionality?

The way to improve your performance is to use a better algorithm, specifically, sort both strings and compare:

function stringscrambling(str1, str2) {
 var sort1 = str1.split('').sort().join('');
 var sort2 = str2.split('').sort().join('');

 return sort1 === sort2;
}


Raspberry-Pi morse-code LED I am new to programming, python and raspberry-pi, but I am keen to learn more. After doing a few basic tutorials I though it might be best to give it a shot and just come up with a task in which I could practice my current skills.
I prepared a csv file with morse-code and wrote a little script to access the csv and translate it into a blinking LED.
It works! Now I would like to learn a bit more and improve my code. How would a simpler/faster/neater solution look like?

I would appreciate any kind of constructive criticism. (I made a few comments for myself. I hope it is not too annoying)

import RPi.GPIO as GPIO
import time
import csv

GPIO.setmode(GPIO.BCM)
GPIO.setup(18, GPIO.OUT)

#cleaning out the csv

with open('Morse.csv') as csvfile:
    readCSV = csv.reader(csvfile, delimiter=',')

    letters = [] #a new list with letters only
    codes = [] #a new list with morse code only

    next(readCSV) #jump over the column description line and an empty line
    next(readCSV)

    for row in readCSV: #fill up the new lists

        letter = row[0]
        code = row

        code.remove(row[0])

        letters.append(letter) #append letters only
        codes.append(code) #append everything but letters

LetterInput = input('what do you want to morse? ')

MorseCode = [] #a new list where the final code for the LED input is saved

for SingLetter in LetterInput: #iterate through the input

    SingLetUp = SingLetter.upper() #capitalize letters so they can be found in csv

    if SingLetUp not in letters: #jump over spaces and special characters
        continue

    MorseDex = letters.index(SingLetUp) #find code that corresponds with letter by matching indices
    SingCode = codes[MorseDex]
    MorseCode.append(SingCode) #fill up list

#iterate through list in list to seperate single words

for pos in MorseCode:
    for sing in pos:
        print(sing)

        x = 'short'
        y = 'long'
        z = 'none'

#LED blinks while parsing through the list

        if sing == x:
            GPIO.output(18, GPIO.HIGH)
            time.sleep(0.2)
            GPIO.output(18, GPIO.LOW)
            time.sleep(0.5)

        if sing == y:
            GPIO.output(18, GPIO.HIGH)
            time.sleep(1)
            GPIO.output(18, GPIO.LOW)
            time.sleep(0.5)

        if sing == z:
            GPIO.output(18, GPIO.LOW)
            time.sleep(1)

GPIO.output(18, GPIO.LOW)
GPIO.cleanup()

And here is the csv file

letter,position 1,position 2,position 3,position 4,position 5
,,,,,
A,short,long,none,none,none
B,long,short,short,short,none
C,long,short,long,short,none
D,long,short,short,none,none
E,short,none,none,none,none
F,short,short,long,short,none
G,long,long,short,none,none
H,short,short,short,short,none
I,short,short,none,none,none
J,short,long,long,long,none
K,long,short,long,none,none
L,short,long,short,short,none
M,long,long,none,none,none
N,long,short,none,none,none
O,long,long,long,none,none
P,short,long,long,short,none
Q,long,long,short,long,none
R,short,long,short,none,none
S,short,short,short,none,none
T,long,none,none,none,none
U,short,short,long,none,none
V,short,short,short,long,none
W,short,long,long,none,none
X,long,short,short,long,none
Y,long,short,long,long,none
Z,long,long,short,short,none
1,short,long,long,long,long
2,short,short,long,long,long
3,short,short,short,long,long
4,short,short,short,short,long
5,short,short,short,short,short
6,long,short,short,short,short
7,long,long,short,short,short
8,long,long,long,short,short
9,long,long,long,long,short
0,long,long,long,long,long

I'm aware that you are a beginner. So I will try to improve your code starting from the most obvious problems and try advance step by step.

Variable names

Names like x, y, z are an absolute no-go (unless naming coordinates). This is like naming them guesswhat. Names should describe what the variable holds as close to natural language as possible. So legal names for x, y, z would be long, short, none. Aloso other names are far from perfect. for SingLetter in LetterInput: should read for letter in message:. You generate a lot of temporary variables which requires you to invent a lot of names. Try avoid some temporaries and/or name temporaries 'less important' than variables you want to use later on. SingLetUp = SingLetter.upper() could read letter = letter.upper(). When reading SingLetUp I do think of 'singing', 'letting' and 'up'. There is no need to save letters in names, if a long name is needed, let it be. But a letter is always a single one, a group of letters shall be named 'word' or 'message' anyway.

Temporaries

As already said, you tend to use too many of them. one case where it makes the code bloated and thus less readable is

x = 'short'
y = 'long'
z = 'none'

and the only later use

if sing == x:

this useless. if named correctly this would read

short = 'short'
# ...
if symbol == short:

which shows the redundancy. it could simply read

if symbol == 'short':

saving some lines without any negative implications.

Do not repeat yourself

This is one of the most important rules to consider. In

if sing == x:
    GPIO.output(18, GPIO.HIGH)
    time.sleep(0.2)
    GPIO.output(18, GPIO.LOW)
    time.sleep(0.5)

if sing == y:
    GPIO.output(18, GPIO.HIGH)
    time.sleep(1)
    GPIO.output(18, GPIO.LOW)
    time.sleep(0.5)

you have two code blocks differing by just one value. all the other parts are required(!) to be identical. this is asking for a function definition (I'm not sure if you already learned that). We do something like (still not perfect)

def gpio_out(length):
    GPIO.output(18, GPIO.HIGH)
    time.sleep(length)
    GPIO.output(18, GPIO.LOW)
    time.sleep(0.5)

and use it like

if symbol == 'short':
    gpio_out(0.2)

if symbol == 'long':
    gpio_out(1)

Do not use 'magic values' in the code but define named constants

The most obvious use cases is to change the GPIO pin. The value 18 is appearing 7 times in your source and must be maintained to match. forgetting to change one will lead to subtle errors. So we do

morse_gpio = 18

and use this name throughout the code. the same applies to all other constants, especially the time values.

About morse code and your implementation

the most frequent letters in western languages are 'e', 't', 'n', ... that is why Morse assigned short codes to these letters. you are outputting many 'none' instead of a single pause separating letters. we fix this by not outputting anything on 'none' but inserting a single pause.

for symbol in pos:
    if symbol == 'short':
        gpio_out(0.2)
    if symbol == 'long':
        gpio_out(1)
    if symbol == 'none':
        pass
time.sleep(1)

Reading the csv file

While this might have been part of your assignment - csv is a little heavy here. this is implicitely shown in your code as you never use the header names. Another hint is that you have to use fill values of none. However you could read such a simple file in python directly. Also we get rid of this superfluous 'none' immediately. Nowhere in the code we are expecting a fixed length code, quite contrary we have special handling for it.

letters = []    #a new list with letters only
codes = []      #a new list with morse code only
with open('Morse.csv') as codefile:
    next(codefile)  #jump over the column description line and an empty line
    next(codefile)
    for line in codefile:
        words = line.strip().split(',')
        letters.append(words[0])    # append letter
        code = [x for x in words[1:] if x != 'none']
        codes.append(code)          # append effective code only

here i used list [comprehension https://docs.python.org/3/tutorial/datastructures.html#list-comprehensions]

Use the right data structures

If you want to look up code for a letter you want to write code = codes[letter] which is clean and readable. so we use a dict for storing our codes

codes = dict()
with open('Morse.csv') as codefile:
    next(codefile)  #jump over the column description line and an empty line
    next(codefile)
    for line in codefile:
        words = line.strip().split(',')
        code = [x for x in words[1:] if x != 'none']
        codes[words[0]] = code          # append effective code only

and rewrite the access

MorseCode = [] #a new list where the final code for the LED input is saved

for SingLetter in LetterInput: #iterate through the input

    SingLetUp = SingLetter.upper() #capitalize letters so they can be found in csv

    if SingLetUp not in letters: #jump over spaces and special characters
        continue

    MorseDex = letters.index(SingLetUp) #find code that corresponds with letter by matching indices
    SingCode = codes[MorseDex]
    MorseCode.append(SingCode) #fill up list

to

morse_code = [] #a new list where the final code for the LED input is saved

for letter in message.upper(): #iterate through the input

    if letter in codes:     # ignore spaces and special characters
        morse_code.append(codes[letter])    # fill up list

Structure your code

Avoid so called spaghetti code, that is many lines of code at the same name space. As already stated about your variable names, have good names and make clear what scope they have. when using functions local variables are not visible outside. this improves readability and avoids name collisions. so we define some functions of already identified code blocks

def read_codes():
    #[...]
    return codes

def encode(message, codes):
    #[...]
    return code

def output_code(code):
    #[...]

and use it them like

codes = read_codes()
message = input('what do you want to morse? ')
code = encode(message, codes)
output_code(code)

Thas limits visibility of name like code or letter. also it adds testability. we can now call encode from some testcases without needing interactive user input.

Further down on internal data representation

When doing debugging you most probably do not want to print [['short', 'long'], ['short'], ['long']] more likely you want to print .- . -. So why not represent the symbols as shorter characters througout the code? We improve our existing function

def read_codes():
    codes = dict()
    with open('Morse.csv') as codefile:
        next(codefile)  #jump over the column description line and an empty line
        next(codefile)
        for line in codefile:
            words = line.strip().split(',')
            code = [x for x in words[1:] if x != 'none']
            codes[words[0]] = code          # append effective code only
    return codes

to

for line in codefile:
    words = line.strip().split(',')
    shortcode = {'long':'-', 'short':'.'}
    code = [shortcode[x] for x in words[1:] if x != 'none']
    codes[words[0]] = ''.join(code)          # append effective code only

here we do not only replace longer strings by characters but also join this characters to a string. this string allows better logging. For print(encode("Hello world!", codes)) we get ['....', '.', '.-..', '.-..', '---', '.--', '---', '.-.', '.-..', '-..']. Wouldn't it be nice to have '.... . .-.. .-.. --- .-- --- .-. .-.. -..' where the blanks represent the pause between letters? this would make the output loop better as it would not do an additional pause at the end. So somewhere in the code we should do ' '.join(code), either just before outputting or in encoding. your choice. for debugging I'd prefer to have it in encoding.

def encode(message, codes):
    code = []    # a new list where the final code for the LED input is saved
    for letter in message.upper(): #iterate through the input
        if letter in codes: #jump over spaces and special characters
            code.append(codes[letter]) #fill up list
    return ' '.join(code)

of course we now have to fix the output

def output_code(code):
    GPIO.setmode(GPIO.BCM)
    GPIO.setup(18, GPIO.OUT)

    for symbol in code:
        print(symbol, end='', flush=True) # debug output

        if symbol == '.':
            gpio_out(0.2)
        elif symbol == '-':
            gpio_out(1)
        else:
            time.sleep(1)

    GPIO.output(18, GPIO.LOW)
    GPIO.cleanup()

Note that we also encapsulated all GPIO stuff in this function. there is no neet to initialize GPIOs before outputting.

Main guard

In python modules may be run directly or they may be imported by some other module e. g. a unit test. So there is a pattern to avoid hassle on import

if __name__ == '__main__':
    # stuff only executed on direct execution but not on import

current state of our module now

import RPi.GPIO as GPIO
import time

symbol_long = 1
symbol_short = 0.2
symbol_pause = 0.5
letter_pause = 1
morse_gpio = 18

def read_codes():
    codes = dict()
    with open('Morse.csv') as codefile:
        next(codefile)  #jump over the column description line and an empty line
        next(codefile)
        for line in codefile:
            words = line.strip().split(',')
            shortcode = {'long':'-', 'short':'.'}
            code = [shortcode[x] for x in words[1:] if x != 'none']
            codes[words[0]] = ''.join(code)          # append effective code only
    return codes

def encode(message, codes):
    code = []    # a new list where the final code for the LED input is saved
    for letter in message.upper(): #iterate through the input
        if letter in codes: #jump over spaces and special characters
            code.append(codes[letter]) #fill up list
    return ' '.join(code) #

def gpio_out(length):
    GPIO.output(morse_gpio, GPIO.HIGH)
    time.sleep(length)
    GPIO.output(morse_gpio, GPIO.LOW)
    time.sleep(symbol_pause)

def output_code(code):
    GPIO.setmode(GPIO.BCM)
    GPIO.setup(morse_gpio, GPIO.OUT)
    for symbol in code:
        print(symbol, end='', flush=True) # debug output

        if symbol == '.':
            gpio_out(symbol_short)
        elif symbol == '-':
            gpio_out(symbol_long)
        else:
            time.sleep(letter_pause)
    GPIO.output(morse_gpio, GPIO.LOW)
    GPIO.cleanup()

if __name__ == '__main__':
    codes = read_codes()
    print(codes) # debug output
    message = input('what do you want to morse? ')
    code = encode(message, codes)
    print(code) # debug output
    output_code(code)

Still to do

Better comments - comment what is not obvious.
Docstrings - explain what your functions do. Better encapsulation - avoid globals and pass parameters to functions.
unit testing - worth to look at.


Checking for surrounding mines Am trying to create a minesweeper game, I have the following code which has to many nested ifs, I want to change the code to take out the nested ifs and make it better to read. But I am stuck on how to change the nested ifs. Any help would be great. thanks

   /*method to check cells around a cell and set its value.*/
   public void setCellValues(){
            for(int i = 0; i<side; i++){
                for(int j = 0; j<side; j++){
                     if(cells[i][j].getValue() != -1){
                         if(j>=1 && cells[i][j-1].getValue() == -1) cells[i][j].incrementValue();
                         if(j<= limit && cells[i][j+1].getValue() == -1) cells[i][j].incrementValue();
                         if(i>=1 && cells[i-1][j].getValue() == -1) cells[i][j].incrementValue();
                         if(i<= limit && cells[i+1][j].getValue() == -1) cells[i][j].incrementValue();
                         if(i>=1 && j>= 1 && cells[i-1][j-1].getValue() == -1) cells[i][j].incrementValue();
                         if(i<= limit && j<= limit && cells[i+1][j+1].getValue() == -1) cells[i][j].incrementValue();
                         if(i>=1 && j<= limit && cells[i-1][j+1].getValue() == -1) cells[i][j].incrementValue();
                         if(i<= limit && j>= 1 && cells[i+1][j-1].getValue() == -1) cells[i][j].incrementValue();
                     }
                }
            }
        }
        /*This method starts chain reaction. When user click on particular cell, if cell is empty (value = 0) this
        method look for other empty cells next to activated one. If finds one, it call checkCell and in effect,
        start next scan on its closest area.
         */
        public void scanForEmptyCells(){
            for(int i = 0; i<side; i++){
                for(int j = 0; j<side; j++){
                    if(!cells[i][j].isNotChecked()){
                        if(j>=1 && cells[i][j-1].isEmpty()) cells[i][j-1].checkCell();
                        if(j<= limit && cells[i][j+1].isEmpty()) cells[i][j+1].checkCell();
                        if(i>=1 && cells[i-1][j].isEmpty()) cells[i-1][j].checkCell();
                        if(i<= limit && cells[i+1][j].isEmpty()) cells[i+1][j].checkCell();
                        if(i>=1 && j>= 1 && cells[i-1][j-1].isEmpty()) cells[i-1][j-1].checkCell();
                        if(i<= limit && j<= limit && cells[i+1][j+1].isEmpty()) cells[i+1][j+1].checkCell();
                        if(i>=1 && j<= limit && cells[i-1][j+1].isEmpty()) cells[i-1][j+1].checkCell();
                        if(i<= limit && j>= 1 && cells[i+1][j-1].isEmpty()) cells[i+1][j-1].checkCell();
                    }
                }
            }
        }

Procedural way

I usually suggest doing it in a way such that you don't have to perform to many checks. Also, I think that iterating on cells and updating the contour when you find a mine is logically simpler than detecting adjacent mines for each cell.

I will not write code for you (you are trying to learn something in the end), but I will outline the overall algorithm, and offer an object-oriented approach to the problem - which has smaller checks than your current solution and is substantially more readable.

Phase 1: corners

Write, for each corner, a single if statement. If it is a mine, update the three surrounding cells if they are not mines.

Phase 2: borders (- corners)

Iterate over the four borders (with 4 for loops), updating the 5 adjacent cells if they are not mines.

Phase 3 (final): the center of the board

For each cell in the center, if it is a mine, update all 8 surrounding cells if they are not mines.

Object-oriented way

Let every cell be an instance of a class Cell¹. A Cell knows (and exposes) whether it is a mine or not. It also has a public method incrementNeighborCounter()².

Done.

Really, we're done. (Yes, you need to implement Cell, but the essential part of programming [designing] has been done already, and you seem capable of writing the code of Cell).

How does it work? You go through the board once, using the pattern described in the "Procedural way" above. But you don't check if a cell is a mine or not before telling it to increment its counter (because that has been delegated to how a Cell presents itself [mines can even count their neighboring mines too now]), you just call incrementNeighborCounter() and move ahead. Part of the logic is now in Cell and your code is simpler, lighter on the brain and less likely to contain bugs.

¹ - It could be an Interface or an Abstract Class, just don't go overboard and add unnecessary complexity.

² - Name it whatever you feel like, this is not an exact science. Just try to be descriptive and use the name as documentation.

Just in case you are wondering, a very simple Cell implementation could use a boolean for being a mine or not and an integer for counting how many mine neighbors it has.


Jasmine unit tests for a function to set attributes on an HTML element I'm new to unit testing so maybe I have this all wrong, but it seems kind of useless. I want to reduce bugs and integrate some tests with a build process however I'm not seeing the benefit at the moment. How can I improve the quality of tests, like the one below?

/* script */
SetAttributes = function(el, attrs) {
  /**
   * @method simple for in loop to help with creating elements programmatically
   * @param {object} el - HTMLElement attributes are getting added to
   * @param {object} attrs - object literal with key/values for desired attributes
   * @example SetAttributes(info,{
   *    'id' : 'utswFormInfo'
   *    'class' : 'my-class-name'
   * });
   */

  for (let key in attrs) {
    if (attrs.hasOwnProperty(key)) {
      el.setAttribute(key, attrs[key]);
    }
  }

  return el;
};

/** \/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/ **/

/* unit test */
describe("SetAttributes is for setting multiple attributes at a single time.", function() {

  /** SetAttributes */
  it("should not be null because it returns an HTMLElement with newly added attributes.", function() {
    expect(SetAttributes(document.createElement('div'), {
      defer: true,
      src: null,
      type: 'text/javascript'
    })).not.toBe(null);
  });
  it("should not be a number because it returns an HTMLElement with newly added attributes.", function() {
    expect(SetAttributes(document.createElement('div'), {
      defer: true,
      src: null,
      type: 'text/javascript'
    })).not.toBe(0);
  });
  it("should not be a string because it returns an HTMLElement with newly added attributes.", function() {
    expect(SetAttributes(document.createElement('div'), {
      defer: true,
      src: null,
      type: 'text/javascript'
    })).not.toBe('div');
  });
  it("should not be undefined because it returns an HTMLElement with newly added attributes.", function() {
    expect(SetAttributes(document.createElement('div'), {
      defer: true,
      src: null,
      type: 'text/javascript'
    })).not.toBe(undefined);
  });
  it("should not be a false value because it returns an HTMLElement with newly added attributes.", function() {
    expect(SetAttributes(document.createElement('div'), {
      defer: true,
      src: null,
      type: 'text/javascript'
    })).not.toBeFalsy();
  });
});

First, your function should follow JS conventions and be camelCase, not PascalCase. The latter is for constructors, but this is just a function. So for the following I'll call it setAttributes instead.

As for your tests/spec: You should structure - and name - them differently. Treat the descriptions as documentation you can read through.

E.g. the describe call should just be describe("setAttributes()"). It's a headline, not a paragraph. The following tests are what constitute the actual description. So, your describe call should not itself do anything but say what's being described. That could be a concept (e.g. describe("user login flow")) but in this case, it's a named function, so let's "describe setAttributes()":

Your function has one primary purpose: It sets multiple attributes. However, you aren't actually testing that! I'd say the most important test would be:

it("sets an element's attributes from an object of names and values", function () {
  var element = document.createElement('div');
  setAttributes(element, {
    style: 'color: red',
    id: 'testElement'
  });
  expect(element.getAttribute('style')).toEqual('color: red');
  expect(element.getAttribute('id')).toEqual('testElement');
});

(Sidenote: There are two expectations in this test case, but you should usually aim for just one expectation for each test case, if you can. This forces you to be precise in your tests, and not test too much at once. Same rule as for writing functions: Do one thing, and do it well. Here, having two expectations isn't too bad, since they're not really testing different things, but in most other cases, aim for just 1.)

Secondly, setAttributes() should return the element it was given:

it("returns the element", function () {
  var element = document.createElement('div');
  expect(setAttributes(element, {})).toEqual(element);
});

And done. That's your function's specification.

For now, anyway. If you have bugs later, start by writing a test case triggers the bug, but also describes how it should work (and therefore fails when you run it), and then fix you code until the tests start passing again. That way, you test suite grows naturally, based on actual bugs encountered.

What you're doing right now is testing the negative again and again, which, yes, is pointless. Your tests basically say that the return value is not specifically zero, it's not a specific string, it's not a given boolean, etc.. You can do that forever.

In short: There's an infinite number of things it shouldn't be, and you can't check them all.

So what you want to check is the positive instead: Does it do what it should do. I.e. do I get the element back that I passed in? If yes, then you can be pretty confident that you're not going to get numeric zero all of a sudden. Especially since you keep calling your function with the same arguments; unless JavaScript is suddenly completely broken, you will get the same result. And it'll never be zero, a string, a boolean, or... etc..

You basically only want to test negatives when you don't know the exact value you're expect, but know that's there's one thing it certainly shouldn't be. Again, consider how your spec is worded; in some cases you want it to document what definitely won't happen, and then cover what will happen in another test.

So if you for instance have a function that is made to be resilient against bad input, you could make spec that says that it won't throw a TypeError on bad input, and one that says it will return null (or something).

So in all:

describe('setAttributes()', function () {
  it("sets an element's attributes from an object of names and values", function () {
    var element = document.createElement('div');
    setAttributes(element, {
      style: 'color: red',
      id: 'testElement'
    });
    expect(element.getAttribute('style')).toEqual('color: red');
    expect(element.getAttribute('id')).toEqual('testElement');
  });

  it("returns the element", function () {
    var element = document.createElement('div');
    expect(setAttributes(element, {})).toEqual(element);
  });
});

Try reading the spec out loud:

  setAttributes() sets an element's attributes from an object of names and values [and] returns the element


Simple C# application that transfers new files (modified in the last 24 hours) from one directory to another I'm new to C#, so I'm eager for any and all criticism/advice.

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace FileTransfer
{
    class Program
    {
        private static string targetPath;
        public static bool running;

        static void Main(string[] args)
        {
            var function = new Functions();
            running = true;
            int num = 0;

            while (running)
            {

                Console.WriteLine("These files in Customer Orders are new:");
                ModifiedFiles newFiles = new ModifiedFiles();
                num = 0;

                foreach (var file in newFiles.modified())
                {
                    Console.WriteLine("\""+file+"\"");
                    num++;
                }

                Console.WriteLine("Transfer {0} file(s) to Home Office? y/n", num);
                string answer = Console.ReadLine();

                if (answer == "y")
                {
                    targetPath = @"C:\Users\Student\Desktop\Home Office";

                    num = 0;
                    foreach (var file in newFiles.modified())
                    {
                        File.Copy(file.FullName, Path.Combine(targetPath, file.Name), true);
                        num++;
                    }

                    Console.WriteLine("{0} file(s) transfered.", num);
                    function.exit();
                }

                else if (answer == "n")
                {
                    function.exit();
                }

                else
                {
                    function.wrong();
                }
            }
        }
    }
    class ModifiedFiles
    {
        public string sourceDir;
        public IEnumerable<FileInfo> modified()
        {
            sourceDir = @"C:\Users\Student\Desktop\Customer Orders";
            var directory = new DirectoryInfo(sourceDir);
            DateTime from_date = DateTime.Now.AddDays(-1);
            DateTime to_date = DateTime.Now;
            var files = directory.GetFiles()
              .Where(file => file.LastWriteTime >= from_date && file.LastWriteTime <= to_date);
            return files.ToList();
        }
    }
    class Functions
    {
        public void wrong() { Console.WriteLine("---Input must be 'y' or 'n'."); }

        public void exit()
        {
            Console.WriteLine("Would you like to exit the program? y/n");
            string answerExit = Console.ReadLine();
            if (answerExit == "y")
            {
                Console.WriteLine("See yah!");
                Environment.Exit(0);
            }
            else if (answerExit == "n")
            {
                // Loop back to beginning
            }
            else
            {
                this.wrong();
            }

        }
    }
}

A few notes, not in order of importance, just as I see them. Keep in mind this is just my opinions but I think they're important especially since you're starting out. You can form bad habits later :)

Your classes and methods should always explicitly define their level of accessebility. e.g. class Functions should be public class Functions
Always check if a file or directory exists before opening: var directory = new DirectoryInfo(sourceDir); should be:

DirectoryInfo directory;
if (Directory.Exists(sourceDir))
    directory = new DirectoryInfo(sourceDir);

User input should always be compare in one case for example answerExit == "y" should be answerExit.ToLower() == "y" Unless you want to make response case sensitive.
System directories should not be explicitly entered. For example : "C:\Users\Student\Desktop\Customer Orders" should be Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.Desktop),"Customer Orders");
Your Functions class is superfluous. Those methods directly relate to activities in Main, they can't be used separately or anything else and should definitely be static.
Your method modified() return an IEnumerable and I think that this line also return an IEnumerable

var files = directory.GetFiles()
      .Where(file => file.LastWriteTime >= from_date &&
      file.LastWriteTime <= to_date);` So why return a .ToList()?

I think that's a good start.


Comma Code (project from "Automate the Boring Stuff with Python") I'm currently learning Python using the book mentioned in the title. I just finished reading about lists, tuples, functions and methods, and one of the projects following this chapter is to create a program which takes in a list as an input and outputs a corresponding string, as the following example showcases:

Input:  ['apples', 'bananas', 'tofu', 'cats']

Output: apples, bananas, tofu, and cats

This should of course work for any list. So I've come up with the following code:

def listToString(someList):
    for i in range(len(someList)):
        newString = str(someList[i])

        if i == (len(someList)-1):
            print('and ' + str(someList[i]))
        else:
            print(newString, end=', ')

someList = ['apples','bananas','tofu','cats']
listToString(someList)

I feel like I haven't used everything (for example, some methods) which were taught in the chapter to solve the problem. Is there a more efficient way to code this project?

Ludisposed's answer is already a great improvement, however it fails to take into account some cornercases:

It only works with lists of strings.
For lists of length 1, like ["foo"], it returns " and foo" (your code has the same problem).
It raises an IndexError on an empty list.

This code avoids these problems:

def list_to_string(a_list):
    if not a_list:
        return ""
    elif len(a_list) == 1:
        return str(a_list[0])
    else:
        body = ", ".join(map(str, a_list[:-1]))
        return '{0} and {1}'.format(body, a_list[-1])

This does not currently have the Oxford comma, like your code. But you can easily add it as another special case (don't add it when len(a_list) == 2 and add it otherwise). Thanks to @DavidK for pointing this out in the comments.


Escape user input for use in JS regex I am building a highlight feature for search results on my website and need to escape the user's input so that I could highlight the matching string within the original text.

The function

function sanitize_for_regex(s){
    var escaped = '';
    for(var i = 0; i < s.length; ++i){
        switch(s[i]){
            case '{':
            case '}':
            case '[':
            case ']':
            case '-':
            case '/':
            case '\\':
            case '(':
            case ')':
            case '*':
            case '+':
            case '?':
            case '.':
            case '^':
            case '$':
            case '|':
                escaped+= '\\';
            default:
                escaped+= s[i];
        }
    }
    return escaped;
}

How it's used

var input_from_user = 'Hey + wi$ll th<is \'" { tex|t / \\ mess w?>ith you?';

var highlighted_text = original_text.replace(new RegExp('('+sanitize_for_regex(input_from_user)+')', 'gi'), '$1');

My test cases show that it's working quite well but I would like to get some feedback from other professionals.

Does this function look like it will escape all injection "attempts"? (I quoted attempts because the user is usually not aware of attempting to break anything)

Could the performance be improved?

I'm not a fan of "sanitizing" data.  There isn't a clear definition of what "sanitizing" means, other than that it takes untrustworthy input and somehow makes it valid.  It might entail discarding the invalid parts of the input — which is not what you are doing here.  Escaping is a clearer term to describe what you are doing.

Repeated string concatenation is considered poor practice for performance.  Since JavaScript strings are immutable, it would be better to devise a solution that constructs the string "all at once".

The documentation on developer.mozilla.org suggests the following solution for escaping a regular expression:

  Escaping user input to be treated as a literal string within a regular
  expression can be accomplished by simple replacement:

function escapeRegExp(string){
  return string.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); // $& means the whole matched string
}

I strongly recommend replacing your custom solution with the standard recipe (including a citation).


Validating a StreamingPin Recently I was thinking about the keyless entry on my car and how simple it has to be, so I wrote a C# programme to validate a StreamingPin, that is, a pin that is entered character-by-character, and resets as soon as an invalid character is input. If the correct sequence of characters is entered, then the pin is validated as correct, but if another character is entered afterwards, the pin is incorrect.

Regardless of what happens before the correct sequence of characters, as soon as the valid sequence is entered the pin is valid.

class StreamingPin
{
    public bool IsCorrect => (_lastCorrect + 1) == _pin.Length;

    private int _lastCorrect = -1;
    private string _pin;

    public StreamingPin(string pin)
    {
        if (string.IsNullOrEmpty(pin))
        {
            throw new ArgumentException($"The value provided for {nameof(pin)} cannot be null or an empty string.");
        }

        _pin = pin;
    }

    public void ProcessCharacter(char c)
    {
        var currentIndex = _lastCorrect + 1;

        if (_pin[currentIndex] == c)
        {
            _lastCorrect = currentIndex;
        }
        else if (_pin[0] == c)
        {
            _lastCorrect = 0;
        }
        else
        {
            _lastCorrect = -1;
        }
    }
}

It takes \$O(1)\$ memory, and runs in \$O(n)\$ time where \$n\$ is the number of characters processed until the correct sequence is found.

No matter now many invalid characters are entered before a valid pin, there is no lockout procedure.

Test code:

var sp = new StreamingPin("PinNumber");
var input = "p2gsvpinnumberPinNumbessfeER#VvsdinNumberPinNumberdfvlj4kF4wfV";
var lastIndex = -1;

while (!sp.IsCorrect)
{
    lastIndex++;
    Console.WriteLine(input[lastIndex]);
    sp.ProcessCharacter(input[lastIndex]);
}

Console.WriteLine("Pin found");

_pin overflow

You're relying on the caller to prevent a buffer overflow.  Consider if the calling code was this:

var sp = new StreamingPin("PinNumber");
var input = "p2gsvpinnumberPinNumbessfeER#VvsdinNumberPinNumberdfvlj4kF4wfV";
var lastIndex = -1;

while (!sp.IsCorrect)
{
    lastIndex++;
    Console.WriteLine(input[lastIndex]);
    sp.ProcessCharacter(input[lastIndex]);
}
sp.ProcessCharacter(input[lastIndex]);

Your ProcessCharacter method doesn't check that accessing _lastCorrect + 1 isn't going to overflow _pin.  This seems like the responsibility of the StreamingPin class.  I'd say, if they start adding letters after a valid pin, it should reset to not correct anymore (rather than throwing an exception that could give the caller information they might not know, such as how long the PIN is).

One way to achieve this would be to add a wrap to the increment:

var currentIndex = (_lastCorrect + 1) % _pin.Length;

readonly

Since you're not updating the pin after construction, I'd also consider making it readonly.


Formatting date difference in human-friendly terms I've written a function to take the difference between two dates and display it in a particular format. I've used a lot of if-else statements, which seems a little messy. Is there a way to shorten this up? Would using the ternary operator make sense?

$date1 = '2018-04-30 10:36:29';
$date2 = '2018-04-30 10:35:29';

echo dateDiff($date1, $date2);

function dateDiff($date1, $date2)
{
    $date_1 = new DateTime($date1);
    $date_2 = new DateTime($date2);

    $diff = $date_1->diff($date_2);

    if($diff->days > 365){
      return $date_1->format('Y-m-d');
    }

    elseif($diff->days < 366 AND $diff->days > 7){
      return $date_1->format('M d');
    }

    elseif($diff->days > 2 AND $diff->days < 8){
      return $date_1->format('L - H:i');
    }

    elseif($diff->days == 2) return "Yesterday ".$date_1->format('H:i');

    elseif($diff->days < 2 AND $diff->days > 0 OR $diff->days == 0 AND $diff->h > 1) return $date_1->format('H:i');

    elseif($diff->days == 0 AND $diff->h < 1 AND $diff->i >= 1) return $diff->i." min ago";

    elseif($diff->days == 0 AND $diff->h < 1 AND $diff->i < 1) return "just now";

    else return $error = "Error!";

}

Chained ternary expressions in PHP are a major pain in the ass — don't use them!

You have a lot of tests that are redundant, since earlier tests will have already eliminated longer periods.  The error case at the end also seems pointless.

Use consistent indentation and braces for readability and safety — please don't skimp.

function dateDiff($date1, $date2)
{
    $date_1 = new DateTime($date1);
    $date_2 = new DateTime($date2);
    $diff = $date_1->diff($date_2);

    if ($diff->days > 365) {
        return $date_1->format('Y-m-d');
    } elseif ($diff->days > 7) {
        return $date_1->format('M d');
    } elseif ($diff->days > 2) {
        return $date_1->format('L - H:i');
    } elseif ($diff->days == 2) {
        return "Yesterday ".$date_1->format('H:i');
    } elseif ($diff->days > 0 OR $diff->h > 1) {
        return $date_1->format('H:i');
    } elseif ($diff->i >= 1) {
        return $diff->i." min ago";
    } else {
        return "Just now";
    }
}

Be sure to use consistent capitalization for "yesterday" and "just now".


Age-Calculator Program This is a code to calculate somebody's age. It takes 2 inputs:

A specific date (to calculate your age at a specific time or the current time).
The birth date.

print('''# This is Age Calculator Program.
# This program is written by Mahmoud Naguib and it will be improved.
# You can contact me at : https://www.facebook.com/naguibarea
--------------------------------------------------------------------''')
import re

while True:
    spefic_date = input('Enter a spefic date (like this format: day/month/year): ')
    birth_date = input('Enter the birth date (like this format: day/month/year): ')
    print('--------------------------------------------------------------------')
    # This line makes sure that the user's inputs are correct
    # if not it will print a message and the user will enter them again.
    if re.search(r'\d{1,2}/\d{1,2}/\d{4}', spefic_date) is None or re.search(r'\d{1,2}/\d{1,2}/\d{4}', birth_date) is None:
        print('You have entered a wrong format! ')
        print('--------------------------------------------------------------------')
        continue
    # This will convert the user's input into lists to use them in assigning values.
    spefic_date = spefic_date.split('/')
    birth_date = birth_date.split('/')

    # This line assigns spefic year's, month's and day's values.
    spefic_year, spefic_month, spefic_day = int(spefic_date[2]), int(spefic_date[1]), int(spefic_date[0])
    # This line specifies birth year's, month's and day's values.
    birth_year, birth_month, birth_day = int(birth_date[2]), int(birth_date[1]), int(birth_date[0])
    # These lines are for math rules.
    if spefic_day < birth_day:
        spefic_day += 30
        spefic_month -= 1
    if spefic_month < birth_month:
        spefic_month += 12
        spefic_year -= 1
    # These lines calculate years, months and days.
    year = str(spefic_year - birth_year) + ' years, '
    month = str(spefic_month - birth_month) + ' months and '
    day = str(spefic_day - birth_day) + ' days. '
    # These lines are for grammar rules.
    if spefic_year - birth_year < 2:
        year = year.replace('years', 'year')
    if spefic_month - birth_month < 2:
        month = month.replace('months', 'month')
    if spefic_day - birth_day < 2:
        day = day.replace('days', 'day')
    print('Your age is: ' + year + month + day)
    print('--------------------------------------------------------------------')

I don't have much time for the review so I'll focus on regular expressions for the time being.

About re

You should probably be using match instead of search because you want the pattern to be at the beginning of the string. Read more
If you are re-using the same regexp multiple times like you are doing here, it might be worth compile-ing it.
You could use the logic used for matching to split your result as well. If you do so, you do not need to change things in multiple places if the format changes;

Like this :

>>> date = "14/07/1789"
>>> m = re.match(r'(\d{1,2})/(\d{1,2})/(\d{4})', date)
>>> m.groups()
('14', '07', '1789')

Even better, you could use named elements. If the format changes (because your code is used in a different country where dates have a different format for instance), only the regexp will change, the code will still be ok because it will be accessing elements by their name and not by their position.

Like this :

>>> date = "14/07/1789"
>>> m = re.match(r'(?P<day>\d{1,2})/(?P<month>\d{1,2})/(?P<year>\d{4})', date)
>>> m.groups()
('14', '07', '1789')
>>> m.groupdict()
{'month': '07', 'day': '14', 'year': '1789'}

Also, the whole logic converting strings to dates (or to triples (day, month, year)) could be extracted in a function on its own.

You might also decide that you want to reuse already existing tools for this such as datetime.strptime

Better user experience

When I input 2 dates which is usually quite painful, I'd rather have my program to fail if the first date is incorrect rather than having to input the second date to know it.


Ugly number implementation in Scala I am trying to implement ugly number sequence generation in Scala.

Ugly numbers are numbers whose only prime factors are 2, 3 or 5. The sequence  1, 2, 3, 4, 5, 6, 8, 9, 10, 12, 15...

I have implemented using var keyword like java implementation and it is working fine. Here is the ideone link of complete code.

Can someone suggest better of implementing it using Scala idioms and without using mutable values?

/**
 * Ugly numbers are numbers whose only prime factors are 2, 3 or 5. The sequence
 * 1, 2, 3, 4, 5, 6, 8, 9, 10, 12, 15,
 * shows the first 11 ugly numbers. By convention, 1 is included.
 * Write a program to find and print the 150th ugly number.
 *
 */
object UglyNumbers extends App {
  var uglyNumbers = List(1)
  val n = 11

  var i2 = 0;
  var i3 = 0;
  var i5 = 0;

  // initialize three choices for the next ugly numbers
  var next_multiple_2 = uglyNumbers(i2) * 2;
  var next_multiple_3 = uglyNumbers(i3) * 3;
  var next_multiple_5 = uglyNumbers(i5) * 5;

  for (i <- 0 to n) {
    val nextUglyNumber = min(next_multiple_2, next_multiple_3, next_multiple_5)
    uglyNumbers = uglyNumbers :+ nextUglyNumber

    if (nextUglyNumber == next_multiple_2) {
      i2 = i2 + 1
      next_multiple_2 = uglyNumbers(i2) * 2
    }

    if (nextUglyNumber == next_multiple_3) {
      i3 = i3 + 1
      next_multiple_3 = uglyNumbers(i3) * 3
    }

    if (nextUglyNumber == next_multiple_5) {
      i5 = i5 + 1
      next_multiple_5 = uglyNumbers(i5) * 5
    }
  }

  for (uglyNumber <- uglyNumbers)
    print(uglyNumber + "   ")

  def min(a: Int, b: Int, c: Int): Int = (a, b, c) match {
    case _ if (a <= b && a <= c) => a
    case _ if (b <= a && b <= c) => b
    case _ => c
  }
}

First off, I'd like to mention, that my Scala experience is limited, so please do point out any errors.

You should implement your algorithm in it's own class implementing an appropriate interface/trait (probably Iterator, but there may be better ones).

That would also avoid having the awkward hard-coded limit to 11 elements in the main algorithm.

I'm not a fan of the repeated code for the three base numbers. Off the top of my head I'm not sure how to avoid it, but would assume it would include using case classes.

You should also have a look at your variable names. You have a mixture of short cryptic names (n, i3, etc.) and two different styles (camel case and underlines). You should always use descriptive names (maxIterations, index3) and use a single style (preferably camel case as this is the convention for Scala).

BTW, List has a min method, so there is no need to roll your own:

val nextUglyNumber = List(next_multiple_2, next_multiple_3, next_multiple_5).min;


Achieving a never-ending loop of game tasks The following is a basic implementation of a loop which is intended to launch a series of games in a loop (generic ones that can be overridden, but that have always have finite durations).  The intention would be that a game can broadcast events to clients and would always broadcast its completion.  When there are no clients listening it can Shutdown, and restart again when some clients begin listening again

Is it a good way to approach this?  Does it matter that I do not await the _gameScheduler.Start(this) call? Am I dealing with the task loop correctly?  Is the timer used correctly?

public class Table
{
    private readonly GameScheduler _gameScheduler;
    public Game Game { get; set; }
    public Table()
    {
        _gameScheduler = new GameScheduler();
        Task schedulerTask = _gameScheduler.Start(this);
    }
    public void Close()
    {
        _gameScheduler.Shutdown();
    }
}

public class GameScheduler
{
    private bool _running = true;
    public async Task Start(Table t)
    {
        while (_running)
        {
            await RunGame(t);
        }
    }

    async Task RunGame(Table table)
    {
        var game = new Game();
        table.Game = game;
        table.Game.Start();
        await table.Game.GameCompletionSource.Task;
    }
    public void Shutdown() { _running = false; }
}

public class Game
{
    System.Threading.Timer _countdownTimer;
    private int _timeLeft = 15000;
    private int _countdownInterval = 1000;
    public TaskCompletionSource<bool> GameCompletionSource;
    public void Start()
    {
        _countdownTimer = new System.Threading.Timer(GameCountdown, null, 0, _countdownInterval);
    }
    public void GameCountdown(object state)
    {
        if (_timeLeft <= 0)
        {
            _countdownTimer.Dispose();
            Finish();
        }
        else
        {
            _timeLeft -= _countdownInterval;
        }
    }
    private void Finish()
    {
        GameCompletionSource.SetResult(true);
    }
}

I just want to say that I review C# code style a lot on this site, and this code made me very happy for many seconds, until I spotted this:

public async Task Start(Table t)

So close!

t should, of course, have a much more descriptive name here. You've even done that elsewhere, so it is most likely an oversight on your part.

I don't like underscore notation personally as it smells strongly Hungarian, but it's a commonly-accepted system in C# so I don't have a leg to stand on.


How could this Java CLABE control digit calculation be enhanced I recently wrote the following Java 1.8 snippet to generate a CLABE control digit based on the following subheading on the CLABE page on Wikipedia and I was curious on how it could be improved.

private void generateCLABEControlDigit() {
    final int[] expectedResults = {0,1,2,3,6,0,0,0,0,3,7,8,9,5,9,1,7};
    final String incompleteClabe = "03218000011835971";
    final String completeClabe = "032180000118359719";

    char[] clabeChars = incompleteClabe.toCharArray();
    int[] weightFactors = {3, 7, 1};
    int[] results = new int[clabeChars.length];
    int count = 0;

    for (int i = 0; i < incompleteClabe.length(); i++) {
        int clabeInt = Integer.parseInt(String.valueOf(clabeChars[i]));
        int weightFactor = weightFactors[count];
        int result = clabeInt * weightFactor % 10;
        results[i] = result;
        if(count == 2) {
            count = 0;
        } else {
            count++;
        }
    }

    final int controlDigit =  (10 - Arrays.stream(results).sum() % 10) % 10;

    assert Boolean.TRUE.equals(Arrays.equals(expectedResults, results));
    assert completeClabe.equals(incompleteClabe + controlDigit);
}

I have included the incomplete CLABE and complete expected CLABE, these are also present within the Wiki article.

This snippet normally takes a String value and would return an integer, this being the generated control digit. However, for purposes of the review I've simply refactored it so it can be simply lifted into a class and run along with the expected outcome (See the assert).

The CLABE comes into the system as a String, which resulted in the not so elegant one liner.

int clabeInt = Integer.parseInt(String.valueOf(clabeChars[i]));

There are certainly a few things that can be improved.

Mainly the amount of intermediate (array) variables is just unnecessary. They use up memory, which means things get big and slow. The adding up can be performed within the loop.

Computers don't like branching (if statements), so if you can remove branches from a loop and replace them by a calculation (such as % 3) then that's always a boon, especially if the result is more readable as well.

The integer count is not well named. If it would be used in my code below, it would be named weightIndex or something similar.

I think the method is better named calculateCLABEControlDigit but I'm a bit lazy and won't rename it here. "Generate" is a bit much for such a calculation in my opinion.

public class CLABE {
    private static final String INCOMPLETE_CLABE = "03218000011835971";
    private static final String COMPLETE_CLABE = "032180000118359719";
    private static final int[] WEIGHT_FACTORS = {3, 7, 1};

    public static char generateCLABEControlDigit(final String incompleteClabe) {
        int sum = 0;
        for (int i = 0; i < incompleteClabe.length(); i++) {
            final int digitValue = Character.digit(incompleteClabe.charAt(i), 10);
            final int weightFactor = WEIGHT_FACTORS[i % 3];
            sum = (sum + (digitValue * weightFactor) % 10) % 10;
        }

        final int controlDigitValue =  (10 - (sum % 10) % 10);
        return Character.forDigit(controlDigitValue, 10);
    }

    public static void main(String[] args) {
        String clabe = INCOMPLETE_CLABE + generateCLABEControlDigitLessMod(INCOMPLETE_CLABE);
        if (clabe.equals(COMPLETE_CLABE)) {
            System.out.println("Yay!");
        }
    }
}

Now just to spook you a bit, some modular operations can be removed from the loop. For multiplication and addition, the final digit will remain correct after all. That is, as long as the sum is below Integer.MAX_VALUE. Of course you'd need a very long CLABE before you can add up to that (with 9 x 7 = 63 being the highest value that may be added per digit).

The double modulus is required for the final subtraction, as the sum can be 0 (an all zero incomplete CLABE) and because the Java mod operator is terrible when it comes to negative left-operand values. Actually, it's not a modulus operator at all. It is called the rest operator, as it may return negative values even if the modulus is positive. That's OK though because it is not in the loop.

    public static char generateCLABEControlDigitLessMod(final String incompleteClabe) {
        int sum = 0;
        for (int i = 0; i < incompleteClabe.length(); i++) {
            final int digitValue = Character.digit(incompleteClabe.charAt(i), 10);
            final int weightFactor = WEIGHT_FACTORS[i % 3];
            sum += digitValue * weightFactor;
        }

        final int controlDigitValue =  (10 - (sum % 10) % 10);
        return Character.forDigit(controlDigitValue, 10);
    }

Of course this speedup doesn't make any difference for this particular method. However, adding or removing modular operations is an interesting speedup trick to use in cases where it does matter, so I thought it was interesting enough to show you.

The testing with the vectors from Wikipedia (or preferably a banking standard) should be done by a Unit test.

Besides that, I'd probably make sure that the input is consisting just of digits before running any code on it by using a guard statement such as:

if (!incompleteClabe.matches("\\d*")) {
    throw new IllegalArgumentException("Not a valid CLABE sans control digit");
}

or even:

if (!incompleteClabe.matches("\\d{17}")) {
    throw new IllegalArgumentException("Not a valid CLABE sans control digit");
}

The CLABE class as specified should be made final and should not be instantiated as it only contains static methods. That is performed by adding a private, no argument constructor to it.

Finally, I might also create a validateCLABEControlDigit(final String fullClabe): boolean method that uses the other method. I'll leave the implementation up to you...


Nested if statements dependant on nulls As far as readability and best practises go I am not sure how best to write this helper method:

Here are two options I have tried:

private decimal? selectTolerance(decimal? globalTolerance, decimal? localTolerance)
{
     if(localTolerance == null)
     {
               if(globalTolerance == null){
                   return null;
               }else{
                   return globalTolerance;
               }
     }
    else
     {
               if(globalTolerance == null){
                    return localTolerance;
               }else{
                   if(globalTolerance < localTolerance){
                       return globalTolerance;
                   }else{
                       return localTolerance;
                   }
               }
         }
 }

Alternatively,

private decimal? selectTolerance(decimal? globalTolerance, decimal? localTolerance)
{
           if (localTolerance == null && globalTolerance == null)
           {
               return null;
           }

           if (localTolerance != null && globalTolerance == null)
           {
               return localTolerance;
           }

           if (localTolerance == null && globalTolerance != null)
           {
               return globalTolerance;
           }

           if (globalTolerance < localTolerance)
           {
               return globalTolerance;
           }
           else
           {
               return localTolerance;
           }
  }

Which of these are better? Any other alternatives?

Methods should always be named in PascalCase.

You can condense the logic to the following:

private decimal? SelectTolerance(decimal? globalTolerance, decimal? localTolerance)
{
    if (localTolerance == null || globalTolerance == null)
    {
        return localTolerance ?? globalTolerance;
    }
    return Math.Min(globalTolerance.Value, localTolerance.Value);
}


Copy a FileInputStream to a file, either as text or as a string of bytes I have written a small program that reads input from a text file and then writes this output to a text file. The method contains a switch which decides whether to output to the file as text or as a literal string of the bytes. How could I refactor this code to only include a single if statement block instead of two to switch between these two modes? I am also interested if there is a more optimal way of writing this code overall.

I have tried to create just one switch at the beginning of the code which would change the output files from output-text-txt to output-bytes.txt (which are located in the resources folder) as well as changing the writing method (whether it writes the integer or string literals). I encounter difficulty when I need to write the bytes because the method asks for either an integer or a string and I am not sure how I would change these writing methods with just one if statement at the beginning of the code (due to it asking for int c as a parameter)

public class ByteStreamTest {

    private static File input;
    private static File output;

    public static void main(String args[]) throws Exception {
        new ByteStreamTest().write(true);
        new ByteStreamTest().write(false);
    }

    public ByteStreamTest() {}

    public void write(boolean asText) {

        input = new File(getClass().getClassLoader().getResource("input.txt").getPath());

        if (asText) {
            output = new File(getClass().getClassLoader().getResource("output-text.txt").getPath());
        } else {
            output = new File(getClass().getClassLoader().getResource("output-bytes.txt").getPath());
        }

        FileInputStream in = null;
        FileWriter fw = null;

        try {

            in = new FileInputStream(input);
            fw = new FileWriter(output);

            int c;

            while ((c = in.read()) != -1) {
                if (asText) {
                    fw.write(c);
                } else {
                     fw.write(String.valueOf(c) + System.getProperty("line.separator"));
                }
            }

            in.close();
            fw.close();

        } catch (Exception e) {
            e.printStackTrace();
        }
    }
}

This isn't really a question that has to do with the functionality of the code, I am simply interested in refactoring the code to include as little if statements as possibly optimizing it further.

Please note I am not an expert in Java, I do know something about object oriented programming.

Java is an object oriented programming language. To write good object oriented programs it's best to keep 5 programming principles in mind. Together these 5 principles are called SOLID programming. The 5 programming principles are

The Single Responsibility Principle
The Open-closed principle
The Liskov substitution principle
The Interface segregation principle
The Dependency inversion principle

The single responsibility principle states:

  that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

This program violates the single responsibility principle in 2 ways, first the function main() is included in the class and second the write() function is too complex (does too much) and should be simplified.

The function write() might be better named copy(). It should do only one thing, which is copy the input file to the output file. There could be 2 copy() functions, one for text and one for strings. The decision on which way to write to the output file should be outside the copy functions.

Opening and closing the files should be outside the write function. The program should only open the input file once and possibly reset it to the beginning the second time.

The ByteStreamTest constructor could open the input file.
There could be a Boolean member of the ByteStreamTest class that is either public and gets modified by main() or private with accessor functions.


Go string formatting I'm very new to Go, and am trying to get some experience by re-writing some of my Python code in Go.  Below is a function that takes an IP address (ipv4) in integer form and returns the string version. From 572779703 to 34.35.236.183 for example.

func int2ip(num int) (string) {
    result   := [4]int{}
    for i, n := range []uint{24, 16, 8, 0} {
        result[i] = (num >> n) & 255
    }
    return fmt.Sprintf("%d.%d.%d.%d", result[0], result[1], result[2], result[3])
}

I'm not only curious if there is a more straightforward or concise way to do this with Go, but am very curious if there is a way to use the []result array in Sprintf() without having to type out every index.

Something similar to:

fmt.Sprintf("%d.%d.%d.%d", result)

The proper way of doing this is to use the tools the language already gave you.

IPv4 usually are stored as big endian numbers, and Go already have an IP type:

func int2ip(num int) net.IP {
    var b [4]byte
    binary.BigEndian.PutUint32(b[:], uint32(num))
    return net.IPv4(b[0], b[1], b[2], b[3])
}

//or if you don't care about returning net.IP
func int2ipstr(num int) string {
    var b [4]byte
    binary.BigEndian.PutUint32(b[:], uint32(num))
    return fmt.Sprintf("%d.%d.%d.%d", b[0], b[1], b[2], b[3])
}

playground

//edit adding more details following @Schism's advice

Using Sprintf is usually fine and they worked on optimizing it fairly well, if you want to squeeze every drop of performance out of it, you could use a more "buffered" alternative like :

func int2ipb(num int) string {
    b, buf := [4]byte{}, make([]byte, 0, 15) //15 = max number of bytes in an ip
    binary.BigEndian.PutUint32(b[:], uint32(num))
    for _, n := range b {
        buf = append(buf, '.')
        buf = append(buf, strconv.Itoa(int(n))[:]...)
    }
    return string(buf[1:])
}


How to make a loop of nested if-statements to find the smallest number of something? I'm a beginning programmer and wrote a little code for a mathematical question. There are 6 rounds where each time the pile of cupcakes is divided into five equal groups, with one extra cupcake (which is discarded). Then one fifth is taken away and the next round starts. I've written the code, but there must be a way to write this shorter, a loop instead of nested if-statements.
My code:
 import sys

getal = 1

while True:
    if getal % 5 == 1:
        a = (getal - 1)/5
        if (a*4) % 5 == 1:
            b = (a*4-1)/5
            if (b*4) % 5 == 1:
                c = (b*4-1)/5
                if (c*4) % 5 == 1:
                    d = (c*4-1)/5
                    if (d*4) % 5 == 1:
                        e = (d*4-1)/5
                        if (e*4) % 5 == 1:
                            print(getal)
                            sys.exit()
                        else:
                            getal = getal + 1
                    else:
                        getal = getal + 1
                else:
                    getal = getal + 1
            else:
                getal = getal + 1
        else:
            getal = getal + 1
    else:
        getal = getal + 1
        continue

Thanks in advance!
PS: getal is Dutch for number (the number of cupcakes at the start, which is the result of my question).

I'm not sure what your code is doing. As it currently stands, it will print the first number that satisfies the requirement of being composed of various parts, all divisible by 5 with a remainder of 1.
If that is your objective, then you should certainly compute the number as an equation: (0*5)+1 *5 ... etc.
If you are working up to determining whether any given number meets the condition, then you can certainly continue as you are doing.
Here are some tips to improve your efficiency and clarity:

Get rid of the continue at the bottom of the loop. It is not needed.

Get rid of the various statements getal = getal + 1 in your else branches. By inspection, I can see that this adjustment is always done whenever you loop (since you sys.exit() on success) so just move the statement to the top of the loop, and adjust your incoming variable:
 getal -= 1
 while True:
     getal += 1
     # many, many if statements

Perform your multiplication as part of your adjustment to getal, not as a separate part of your if statements:
 a = ((getal - 1) / 5) * 4
 if a % 5 == 1:

Consider negating your tests. Instead of looking for "good" values, look for "bad" values. Because you are testing for "failure," and because the response to failure is to jump away (continue), you can replace nesting with a series of tests at the same level:
 if a % 5 != 1:
     continue
 b = ...
 if b % 5 != 1:
     continue
 c = ...
 if c % 5 != 1:
     continue


Merging rectangles in C++ I have the following code for separating out overlapping rectangles vs.
non-overlapping rectangles in C++ 11:

#include <stdio.h>
#include <opencv/cv.h>
#include <opencv/highgui.h>
#include <opencv/cxcore.h>
#include <math.h>
#include <array>

int main() {
    auto detected = get_detected();
    std::vector <Rect> singles;
    std::vector <Rect> overlaps;
    for (auto rect_1: detected) {
        bool isSingle = false;
        for (auto rect_2: detected) {
            if (rect_1 == rect_2) {
                continue;
            }
            bool intersects = ((rect_1 & rect_2).area() > 0);
            if (intersects) {
                isSingle = false;
                break;
            }
        }
        if (isSingle) {
            singles.push_back(rect_1);
        } else {
            overlaps.push_back(rect_1);
        }
    }

}

How do I make this cleaner? I'm thinking of doing singles.push_back(std::move(rect_1));, but I'm not sure if that's a good idea.

I would love some guidance in the right direction.

Bug
        if (intersects) {
            isSingle = false;
            break;
        }

This looks like a bug. The variable isSingle is already false. Setting it to false here does nothing.
Code Review:
You use std::vector but don't include its header.
    std::vector <Rect> overlaps;

You should fix that.

References and Const correctness.
This is a big part of C++.

If you are not modifying an object, prefer to use reference than making a copy of an object.

Prefer const references over normal references to prevent accidental modification.
for (auto rect_1: detected) {

Here you are making a copy of the object from the array detected into the object rect_1. Personally I would use a const reference to prevent the overhead of a copy:
for (auto const& rect_1: detected) {

The code looks mostly fine:
I think the only bug is here:
            if (rect_1 == rect_2) {
                continue;
            }

This tells you if two rectangles are equivalent not if they are the same rectangle. If you have two distinct but otherwise identical rectangle then by my logic they would overlap and thus not be in the single's list.
So you need a way to determine if these are the same object. Personally I would do that by changing your for loops to use references and then comparing the objects' addresses.
    for (auto const& rect_1: detected) {
              ^^^^^^
        ...
        for (auto const& rect_2: detected) {
                  ^^^^^^
            if ( & rect_1 ==  & rect_2) {
                ^^^          ^^^
                continue;
            }

            test(rect_1, rect_2);

Your other alternative is to make sure you never compare an object with itself. This also brings into mind that you do twice as many comparisons as you need to. You compare every rectangle against every other rectangle. But if A & B overlap then we already know that B & A will overlap.
for(std::size_t loop = 0l; loop <  detected.size(); ++loop) {
    for(auto check = loop + 1; check < detected.size(); ++check) {
        test(detected[loop], detected[check]);

in the code below "...object addresses" -- aren't rect_1 and rect_2 references already? If so, why can't we just do if (rect_1 == rect_2)

No. Because the == is supposed to compare the equality (is the state the same) of two objects. It is not supposed to be a test of equivalence (is it the same object).
Note: You can declare operator== to test if the objects are the same but that would be terrible practice and would cause a lot of issues with people expecting the normal operation. So don't do that. The above statement is said assuming you are following the normal idioms of the language.
I assume you are coming from a Java like background. The term reference in C++ has a different meaning than in Java. A reference means another name for an object (they are not like Java references (which in C++ we would call pointers). So a reference is another name for an object but the test operator== will compare if the two objects' state are the same (not if they are the same object).

Also, if I use auto const&, I would have to change my return type to vector<Rect&>...is that idiomatic? I think the OpenCV API expects vector<Rect>

No. You can pass a reference to the vector and it will copy the object into the vector. You don't want to hold references in a vector.


What is better to use for mapping value to Map of functions: tail recursion or foreach? I want to map functions from my functions library (Map called as chain) to input string str. Also that functions (Twitter.removeRT, ...) are regex which replace substrings in input str. I think it is better to save that regex-functions in Map like in my example.

So, the code:

Tail recursion variant

  def filterTwitter2 (str: String): String = {

    @scala.annotation.tailrec
    def recFilter(str: String, chain: Map[String, (String) => String]): String = {
      chain.headOption match {
        case Some(v) =>
          val filteredString = v._2(str)
          recFilter(filteredString, chain.tail)
        case None => str
      }
    }

    val chain = Map[String,(String) => String](
      "f1"-> Twitter.removeRT,
      "f2"-> Twitter.removeNickName,
      "f3"-> Twitter.removeURL,
      "f6"-> Emoticons.removePunctRepetitions,
      "f7"-> Emoticons.removeHorizontalEmoticons,
      "f9"-> Emoticons.normalizeEmoticons,
      "f10"-> Beautify.removeCharRepetitions,
      "f12"-> Beautify.removeNSpaces
    )

    recFilter(str, chain)
  }

foreach variant

  def filterTwitter (str: String): String = {
    var tmp = str
    val chain = Map[String,(String) => String](
      "f1"-> Twitter.removeRT,
      "f2"-> Twitter.removeNickName,
      "f3"-> Twitter.removeURL,
      "f6"-> Emoticons.removePunctRepetitions,
      "f7"-> Emoticons.removeHorizontalEmoticons,
      "f9"-> Emoticons.normalizeEmoticons,
      "f10"-> Beautify.removeCharRepetitions,
      "f12"-> Beautify.removeNSpaces
    )
    chain.foreach {
      case (name, func) => tmp = func(tmp)
    }
    tmp
  }

So, the questions:

Is it normal to save functions in map? What can be better for it?
What better: tail recursion variant or variant with foreach?
Maybe there is any better solution for that problem?

You never use your Map to lookup a filter function by its key. If you don't need any of the keys then you don't really need a Map.

def filterTwitter(str :String) :String =
  List(Twitter.removeRT
      ,Twitter.removeNickName
      ,Twitter.removeURL
      ,Emoticons.removePunctRepetitions
      ,Emoticons.removeHorizontalEmoticons
      ,Emoticons.normalizeEmoticons
      ,Beautify.removeCharRepetitions
      ,Beautify.removeNSpaces
      ).foldRight(str)(_(_))

EXPLANATION

The 1st underscore is an element from the List that is being folded. Because this is a fold Right, it will start with the last element (removeNSpaces) and work toward the head (removeRT).

The 2nd underscore is the result from the previous invocation and it is being passed as an argument to the filter function. (Actually it's a little more complicated than that, but this is an easy way to think about it.)

So this is what's going down:

removeNSpaces(str)             ===> resStr1
removeCharRepetitions(resStr1) ===> resStr2
normalizeEmoticons(resStr2)    ===> resStr3
. . .
removeRT(prevResStr)           ===> finalResStr


Seeding a PRNG - Custom Setter I have a PRNG-Class called Generator. Now, after creating a new instance of it:

var g:Generator = new Generator();

Custom function: set_seed()  -  Please review this approach.

I call g.set_seed(); which has the following logic:

public function set_seed(s:Number = 0):void
{
    seed = (s > 0) ? s : new Date().getTime();
}

Pro: I can call it without any arguments, looks more beautiful in the classes I'm using it.
Con: set_seed looks awful and I feel I should be using Flash's Getter/Setter approach.

(I'm getting the seed with: get_seed();)

I would expect the generator to seed itself (using the current time) in its constructor.

Therefore, I would not use a default parameter in your set_seed method.

Also, in your current code, you treat all the negative values the same as zero. I wouldn't recommend that. Many random seeders can handle negative seeds as well, if you don't want to do that: Throw an Exception/Error!

Therefore, I would use this:

public function set_seed(s: Number): void
{
    if (s < 0) {
        throw new Error("Seed must be positive");
    }
    seed = s;
}

For comparison and inspiration, check Java's java.util.Random class. Note that it has two constructors available, one for initializing to System's seed and one for initializing to a specific seed. That is an approach I would recommend.


Babel code in React component to display tasks I'm trying to think of ways to clean up this component in my app so it looks more readable to other devs. This is the best I came up with so far. It's not that bad, but I think it could be a lot better. What do you guys think? How would you clean this code up to make it more readable?

const DisplayTasks = ({ tasksArray, removeTask, crossOutTask }) => {
  return (

        {tasksArray.map((task, index) => (

 crossOutTask(index) } key={index} >

            { task.crossedOut
              ? {task.title}
              : task.title }

            <button id="removeButton" onClick={ event => removeTask(event, index) } >
              Remove
            </button>

        ))}

  );
};
```

This looks pretty good. I don't really have any big comments, since I found it pretty easy to read. Nits:

you could remove the wrapping div with the id orderedList... The ol tag serves the same purpose, plus it's not a very specific name. I think it probably makes more sense to add a class to the ol with a value like tasks.
Generally don't use the index as a key. React uses these to figure out what to redraw. If you have some sort of ID, use that; if not, you can probably just use the task title, or a title + index. You want this to be specific... if the key too specific the component will get redrawn too much (generally not a problem). If the key is to general, components will be "stale"... not redrawn with the proper values.

Look good!


Function for store data - C++ Given the following definition for a NODE in a linked list:

struct NODE {
int data;
NODE* next;
};

I want a function to store the nodes with odd data to a given file (each line should contain a single integer number) [use c++ classes from ]

I wrote this code but there is some problems with the output .. Where is the wrong or what is the best solution for the question??

void storeOddElements(NODE *head, char *FileName)
{
    NODE *p = head;
    int count = 0;

    ofstream fp(FileName, ios::out);
    if(!fp.is_open()) return;

    while (p != NULL)
    {
        if ((p->data % 2) == 1)
            count++;
        p = p->next;

    }

    int odd_store[count], i=0;
    while (p !=NULL) {
        if ((p->data % 2) == 0)
            p = p->next;
        else
        {
            odd_store[i] = p->data;
            p = p->next;
            i++;
        }
    }

    int temp;
    for (int z = 0; z <= i-1; z++) {
        for (int j = i+1; j <= i; j++) {
            if (odd_store[z] <= odd_store[j]) {
                continue;
            }
            else
            {
                temp = odd_store[z];
                odd_store[z] = odd_store [j];
                odd_store[j] = temp;
            }
        }
    }
    for (int x = 0; x <= i; x++) {
        fp << odd_store[x] << endl;
    }
    fp.close();
}

Why are you passing a file name?

void storeOddElements(NODE *head, char *FileName)

A file stream is the same concept as a stream. So writ the code to use a stream. Then you can pass std::cout while you are testing then swap in std::ofstream object when you are ready to pass a file.

Technically this is illegal C++;

int odd_store[count];

To be legal count must be determined at compile time. Though some compilers support this as an extension. It is better to use std::vector<> for this situation.

BUT why are you counting and storing the output values into an array in the first place? That seems like a redundant waste of time. Just print them directly to the file stream.

Then you sort the stored values. I see no requirement that the data be sorted.

Looking at your sort you implement a swap operation rather than using std::swap(). But you don't even need to implement a sort function there is a standard one std::sort that implements a better algorithm than the bubble sort you are using.

Bu the easiest way is to store them in a sorted container when you retrieve them from the list std::set would work for that.

Lastly the copy from the container to the output uses a loop. Learn how to use the standard algorithm or the range based for to make the code easier to read.

Also prefer to use "\n" rather than std::endl. Using std::endl will only slow your code down.

Personally I would write it like this:

void storeOddElements(NODE *head, std::ostream& output)
{
    std::copy_if(NodeIterator(head), NodeIterator(),
                 std::ostream_iterator<int>(output, "\n"),
                 [](auto const& val){return val % 2 == 1;});
}

Now you only need to define a NodeIterator that can iterate over a list of Node. :-)

OK Got bored. Here is a NodeIterator.

class NodeIterator
{
    Node* current;
    public:
        NodeIterator(Node* begin = nullptr)
            : current(begin)
        {}
        NodeIterator& operator++()   {current = current->next;return *this}
        NodeIterator  operator++(int){NodeIterator result(*this); (*this)++; return result;}

        bool operator==(NodeIterator const& other) const {return current == other.current;}
        bool operator!=(NodeIterator const& other) const {return !(*this == other);}
        int operator*()                       {return current->data;}

        // iterator traits
        using difference_type   = long;
        using value_type        = int;
        using pointer           = int*;
        using reference         = int&;
        using iterator_category = std::forward_iterator_tag;
};

If we need to sort the data before output then we can use:

void storeOddElements(Node *head, std::ostream& output)
{
    std::set<int>  sortedData;
    std::copy_if(NodeIterator(head), NodeIterator(),
                 std::inserter(sortedData, sortedData.end()),
                 [](auto const& val){return val % 2 == 1;});

    std::copy(std::begin(sortedData), std::end(sortedData),
              std::ostream_iterator<int>(output, "\n"));
}


Days in Month solution The code, written in Rust, solves the Days in Month problem in CodeChef. It is also one of my first few programs written in Rust.

The code is accepted so that there are no logical mistakes. I am more interested in getting review feedback on style. I am not particularly happy with the code as is; it is rather bulky.

use std::io;

fn read_line(stdin: &io::Stdin) -> io::Result<String>
{
    let mut input = String::new();
    match stdin.read_line(&mut input)
    {
        Ok(_) => Ok(input.trim().to_string()),
        Err(e) => Err(e)
    }
}

fn to_day_of_week(input: &str) -> Option<usize>
{
    if input == "mon"
    {
        Some(0)
    }
    else if input == "tues"
    {
        Some(1)
    }
    else if input == "wed"
    {
        Some(2)
    }
    else if input == "thurs"
    {
        Some(3)
    }
    else if input == "fri"
    {
        Some(4)
    }
    else if input == "sat"
    {
        Some(5)
    }
    else if input == "sun"
    {
        Some(6)
    }
    else
    {
        None
    }
}

pub fn codechef_nw1()
{
    let stdin = io::stdin();
    let read_first_line_result = read_line(&stdin);
    if read_first_line_result.is_ok()
    {
        let first_line = read_first_line_result.unwrap();
        let parse_first_line_result = first_line.parse::<i32>();
        if parse_first_line_result.is_ok()
        {
            let t = parse_first_line_result.unwrap();
            for _i in 0..t
            {
                let read_test_line_result = read_line(&stdin);
                if read_test_line_result.is_ok()
                {
                    let test_line = read_test_line_result.unwrap();
                    let find_space_result = test_line.find(' ');
                    if find_space_result.is_some()
                    {
                        let space_index = find_space_result.unwrap();
                        let test_token_1 = &test_line[..space_index];
                        let test_token_2 = &test_line[space_index + 1..];
                        let parse_day_result = test_token_1.parse::<i32>();
                        if parse_day_result.is_ok()
                        {
                            let days = parse_day_result.unwrap();
                            let day_of_week_result = to_day_of_week(test_token_2);
                            if day_of_week_result.is_some()
                            {
                                let mut day_of_week = day_of_week_result.unwrap();
                                let mut stats = [0, 0, 0, 0, 0, 0, 0];
                                for _d in 0..days
                                {
                                    stats[day_of_week] = stats[day_of_week] + 1;
                                    day_of_week = day_of_week + 1;
                                    day_of_week = day_of_week % 7;
                                }
                                println!("{} {} {} {} {} {} {}", stats[0], stats[1], stats[2], stats[3], stats[4], stats[5], stats[6]);
                            }
                        }
                    }
                }
            }
        }
    }
}

to_day_of_week can be heavily simplified by using match instead of if-else:

fn to_day_of_week(input: &str) -> Option<usize> {
    match input {
        "mon" => Some(0),
        "tues" => Some(1),
        "wed" => Some(2),
        "thurs" => Some(3),
        "fri" => Some(4),
        "sat" => Some(5),
        "sun" => Some(6),
        _ => None,
    }
}

The bulkiness of your code can be somewhat helped if you provide additional functions and use ? instead of unwrap. Using unwrap is discouraged, as far as I know. If you don't want to use ? or additional functions, early returns prevent your code from getting too much indented.


Dice-rolling Python script I completed writing a dice roll script in Python but I thought it looked too messy. Is there anything I should change here?

import random, os

class Dice:
    result = []
    total = 0

    def __roll_(sides=1):
        return random.randint(1, sides)

    def roll(sides=1, times=1):
        for time in range(0, times):
            Dice.result.append(Dice.__roll_(sides))
            Dice.result = Dice.result[len(Dice.result) - times:len(Dice.result)]
        Dice.sumResult()
        return Dice.result

    def sumResult():
        Dice.total = 0
        for num in range(0, len(Dice.result)):
            Dice.total += Dice.result[num]
        return Dice.total

    def saveResult(directory=''):
        if directory == '':
            savetxt = open('savedResult.txt', 'a+')
        else:
            savetxt = open(os.path.join(directory, 'savedResult.txt'), 'a+')
        savetxt.write(str(Dice.result) + '\n')
        savetxt.close()

    def saveTotal(directory=''):
        if directory == '':
            savetxt = open('savedTotal.txt', 'a+')
        else:
            savetxt = open(os.path.join(directory, 'savedTotal.txt'), 'a+')
        savetxt.write(str(Dice.total) + '\n')
        savetxt.close()

Your class is not a class, self is totally missing. You have to rewrite the whole thing.
Internal methods start with one single underscore _roll.
You can access lists from the end with negative indices, len in unnesseccary.
Never change the internal state of a instance and return a value. Do the one or the other.
You can join with empty strings, if is unneccessary.
Open files with the with-statement. Never use the string representation of python objects like lists or dicts for other purposes than debugging.
Remember the naming conventions in PEP-8.

import random
import os

class Dice:
    def __init__(self, sides=1):
        self.sides = sides
        self.result = []
        self.total = 0

    def _roll(self):
        return random.randint(1, self.sides)

    def roll(self, times=1):
        self.result[:] = [self._roll() for time in range(times)]
        self.sum_result()

    def sum_result(self):
        self.total = sum(self.result)

    def save_result(self, directory=''):
        with open(os.path.join(directory, 'savedResult.txt'), 'a') as txt:
            txt.write('%s\n' % ', '.join(map(str, self.result)))

    def save_total(directory=''):
        with open(os.path.join(directory, 'savedTotal.txt'), 'a') as txt:
            txt.write('%d\n' % self.total)


Fast Number Factorization in Python Here's my implementation to factorize a positive integer:

def is_prime(n):
    if n == 1:
        return False
    if n % 2 == 0:
        return False
    i = 3
    while i * i <= n:
        if n % i == 0:
            return False
        i += 2
    return True

# @timeit
def prime_factors(n):
    prime_factor_list = []
    while not n % 2:
        prime_factor_list.append(2)
        n //= 2
    while not n % 3:
        prime_factor_list.append(3)
        n //= 3
    i = 5
    while n != 1:
        if is_prime(i):
            while not n % i:
                prime_factor_list.append(i)
                n //= i
        i += 2

    return prime_factor_list

How can I improve this code in speed and make it pythonic?

prime_factors()

The single greatest improvement to performance that you could make would be to remove the call to is_prime() altogether.  If all factors smaller than i have been removed from n already, then i could not possibly be composite.

The next improvement you could make would be to turn prime_factors() into a generator.  The function would no longer need to resize and recopy prime_factor_list whenever the list needs to grow beyond its estimated size.  Furthermore, the caller could start making use of initial results before the entire factorization is finished.

Using three lines (i = 5, while n != 1, and i += 2) to structure the loop — something that would be succinctly written in C as a for (i = 5; n != 1; i += 2) header — is un-Pythonic.  Such counting loops are usually better written using some kind of range(), xrange(), enumerate(), or something from itertools.  Here, itertools.chain() and itertools.count() would be appropriate.

Without changing the spirit of your algorithm, I would write:

import itertools

def prime_factors(n):
    for i in itertools.chain([2], itertools.count(3, 2)):
        if n <= 1:
            break
        while n % i == 0:
            n //= i
            yield i

is_prime()

As mentioned above, you don't need is_prime() at all, if your only goal is to implement prime_factors().

However, if you wanted to implement is_prime() anyway, you could tighten it up a bit.  My main recommendation would be to avoid performing the i * i multiplication with each iteration.  You would probably be better off computing the \$\sqrt{n}\$ limit just once.

Then, as with the other function, you should express the loop more Pythonically using range() or xrange().  Actually, you wouldn't need an explicit loop; you could use any() instead.

So, if we stick to your algorithm in spirit, we could tighten up the implementation like this:

def is_prime(n):
    if n < 3 or n % 2 == 0:
        return n == 2
    else:
        return not any(n % i == 0 for i in range(3, int(n**0.5 + 2), 2))


Sort ObservableCollection after added new item For a WPF application, I have to sort the items in ObservableCollection after Added new item.

public void UpdateSource(ObservableCollection<SomeType> source, SomeType newItem)
{
    source.Add(newItem);
    SortSource(source, newItem);
}

private void SortSource(ObservableCollection<SomeType> source, SomeType item)
{
    var oldIndex = source.IndexOf(item);
    var list = source.OrderBy(_=>_.SomeProperty).ToList();
    var newIndex = list.IndexOf(item);

    source.Move(oldIndex, newIndex);
}

In order to keep the observable functionality for binding element, the ObservableCollection reference cannot be changed, so I used an assistance list to help me sort the souce.

Any improvement about this?

Your code unnecessarily sorts the whole collection on each insert (which is likely going to be O(n log n)). It also produces two events for each insert (one for Add and one for Move), which also isn't necessary.

What I would do:

Find the index where the inserted item belongs.
Insert the item at that index.

A simple implementation for that could look like this:

public static void AddSorted<T>(this IList<T> list, T item, IComparer<T> comparer = null)
{
    if (comparer == null)
        comparer = Comparer<T>.Default;

    int i = 0;
    while (i < list.Count && comparer.Compare(list[i], item) < 0)
        i++;

    list.Insert(i, item);
}

This could be made faster by using binary search to find the index. Though the total complexity is still going to be O(n), even with binary search, because Insert is O(n) (it has to move all items after the inserted item).

This means that using binary search would help the most if you're often inserting items at or near the end of the collection.

Other notes about your code:

public void UpdateSource(ObservableCollection<SomeType> source, SomeType newItem)

I don't think Update is the right name here, this is not a general update, something like AddToSource would be better.

source.OrderBy(_=>_.SomeProperty)

_ as a parameter name of a lambda is commonly used when you don't care about the value of that parameter. If you do care about it (like here), use something like x, or, even better, a meaningful name.


Unit testing Backbone Model I would like to hear other people's opinion on how I unit tested a functionality in a Backbone Model. As you see below, the code under test, upon being called to toggle, toggles its completed state and saves the new state:

// js/models/todo.js

  var app = app || {};

  // Todo Model
  // ----------
  // Our basic **Todo** model has `title` and `completed` attributes.

  app.Todo = Backbone.Model.extend({

    // Default attributes ensure that each todo created has `title` and `completed` keys.
    defaults: {
      title: '',
      completed: false
    },

    // Toggle the `completed` state of this todo item.
    toggle: function() {
      this.save({
        completed: !this.get('completed')
      });
    }

  });

To unit test this, as you can see below, I stub Backbone.sync method and make sure that the Model is saved with the correct state:

describe("todo", function() {
    it("should save the toggled completed state", function() {
        var stub = sinon.stub(Backbone, "sync", function(method, model) {
            expect(model.get("completed")).to.equal(true);
        });

        var todo = new app.Todo({completed: false});

        todo.toggle();

        expect(stub.calledOnce).to.equal(true);

        Backbone.sync.restore();
    });
});

I'm not really familiar with mocha. If I understand correctly, the true and false values here closely releated, in the sense that they should be opposites:

var stub = sinon.stub(Backbone, "sync", function(method, model) {
    expect(model.get("completed")).to.equal(true);
});

var todo = new app.Todo({completed: false});

In that case, to avoid mistakes, it would be better to include that logic in the code, for example:

var initial_state = false;
var stub = sinon.stub(Backbone, "sync", function(method, model) {
    expect(model.get("completed")).to.equal(initial_state);
});

var todo = new app.Todo({completed: !initial_state});

And maybe add another test for the inverse, for initial_state = true.

Instead of this:

var todo = new app.Todo({completed: false});
todo.toggle();

Since you only use the todo instance once, I would combine the two lines to shorten the code:

new app.Todo({completed: false}).toggle();

Instead of this .to.equal(true):

expect(stub.calledOnce).to.equal(true);

I think it would be more natural (and shorter) with .to.be.ok():

expect(stub.calledOnce).to.be.ok();