CodeFights: Frisbees Description

  You and your friends are seizing another summer day, passing around the frisbee in the park. You have the most fun when the game is fair, so as much as possible, you would all like to ensure that everyone receives the disc an equal number of times. You also like to challenge yourselves, so when possible, you'll go for the longest pass you can throw.

  So each friend will throw according to the following rules:

  Pass to the friend who has held the frisbee the least amount of times.
  In the event of a tie, pass to the furthest among these friends.
  If there's also a tie for furthest, pass to the one of these friends with the lowest index.

  Given an array friends containing info about each player (where they're standing and how far they can throw), as well as the index of the starting player and the total number of passes, your task is to find which player will be holding the frisbee after all the passes are complete.

  NOTE: Because you and your friends are really good at frisbee, it's safe to assume that all passes will be completed successfully; none of you will fumble a throw or catch.

Example

For friends = [[152, 213, 276], [274, 259, 151], [40, 57, 130], [203, 87, 189], [43, 182, 163]], numberOfPasses = 19, and startingPlayer = 4, the output should be frisbees(friends, numberOfPasses, startingPlayer) = 4

Visual example

Constraints

2 ≤ friends.length ≤ 5000
friends[i].length = 3
0 ≤ friends[i][0] ≤ 400
0 ≤ friends[i][1] ≤ 400
0 ≤ friends[i][2] ≤ 500
0 ≤ numberOfPasses ≤ 2500
0 ≤ startingPlayer < friends.length

Code

def make_player_throws_list(friends):
    distance = lambda f, s: (f[0] - s[0])**2 + (f[1] - s[1])**2
    return [
        [
            (i, distance(f, s))
            for i, f in enumerate(friends)
            if distance(f, s) <= s[2]**2 and f != s
        ]
        for s in friends
    ]

def frisbees(friends, number_of_passes, starting_player):
    frisbee_held = {i: 0 for i in range(len(friends))}
    player_possible_throws_list = make_player_throws_list(friends)

    for _ in range(number_of_passes):
        frisbee_held[starting_player] += 1
        starting_player = min(
                                player_possible_throws_list[starting_player],
                                key = lambda x: (frisbee_held[x[0]], -x[1], x[0])
                             )[0]
    return starting_player

builtin

you can use some parts of the standard library:

math.hypot for the distance function
collections.defaultdict for the frisbees_held
itertools.combinations for generating the combinations in the distance matrix

imports:

from itertools import combinations, islice
from collections import namedtuple, defaultdict
from math import hypot

players

you can use a class, or even a namedtuple to represent a player, to simplify the building of the distance matrix

Player = namedtuple('Player', 'name x y range')
players = [[152, 213, 276], [274, 259, 151], [40, 57, 130], [203, 87, 189], [43, 182, 163]]
players = [Player(i, x, y, range_) for i, (x, y, range_) in enumerate(players)]

[Player(name=0, x=152, y=213, range=276),
 Player(name=1, x=274, y=259, range=151),
 Player(name=2, x=40, y=57, range=130),
 Player(name=3, x=203, y=87, range=189),
 Player(name=4, x=43, y=182, range=163)]

distance matrix

you calculate the distance between the players a few times, you can reduce that by doing something like this:

def distance_generator(players):
    for p1, p2 in combinations(players, 2):
        dist = hypot((p1.x - p2.x), (p1.y - p2.y))
        if dist < p1.range:
            yield p1, p2, dist
        if dist < p2.range:
            yield p2, p1, dist

here a dict of dicts might be a more appropriate data structure than a list of lists

def build_distance_matrix(players):
    distance_matrix = defaultdict(dict)
    for p1, p2, dist in distance_generator(players):
        distance_matrix[p1.name][p2.name] = dist
    return dict(distance_matrix)

{
    0:
    {
        1: 130.38404810405297,
        2: 192.04166214652486,
        3: 135.9301291105103,
        4: 113.3225485064645,
    },
    1: {0: 130.38404810405297},
    3: {
        0: 135.9301291105103,
        1: 186.07794065928394,
        2: 165.73774464496614,
        4: 186.07794065928394,
    },
     4: {0: 113.3225485064645, 2: 125.03599481749245},
     2: {4: 125.03599481749245},
}

throwing

you can model a game as an endless series of people throwing the frisbee to eachother. The min(...) you use is the correct way to do this, but can be formatted clearer

def game(distance_matrix, start_player):
    frisbees_held = defaultdict(int)
    target = start_player
    while True:
        frisbees_held[target] += 1
        targets = distance_matrix[target]
        yield target
        target = min(
            targets,
            key=lambda x: (
                frisbees_held[x], # number times held
                -targets[x], # furthest
                x # lowest index
            )
        )

final selection

then you can use the nth itertools recipe

def nth(iterable, n, default=None):
    "Returns the nth item or a default value"
    return next(islice(iterable, n, None), default)

def frisbees(friends, numberOfPasses, startingPlayer):
    players = [Player(i, x, y, range_) for i, (x, y, range_) in enumerate(friends)]
    distance_matrix = build_distance_matrix(players)
    g = game(distance_matrix, startingPlayer)
    return nth(g, numberOfPasses)


Asp.Net Custom Server Control: PostBack handling I wrote a custom server control. The data in it can be manipulated on the client and the changes will be written into the hidden field. The data represents a property of the control and should be retrieved on a postback. I achieved this by using the value changed event to set the property and all its dependencies.
Is this a good a way to do this? Or can this code be improved?

public class Control : CompositeControl {

  private bool mProperty;
  private HiddenField hiddenField;

  public virtual bool Property {
    get {
      return mProperty;
    }
    set {
      mProperty = value;
      if (contentPanel != null) contentPanel.Visible = value;
      if (hiddenField != null && hiddenField.Value != value.ToString().ToLower()) hiddenField.Value = value.ToString().ToLower();
    }
  }
  protected override void CreateChildControls() {
    Controls.Clear();
    CreateControlHierarchy();
    ClearChildViewState();
  }
  protected virtual void CreateControlHierarchy() {
    CreateHiddenField();
    CreateContent();
  }

  protected virtual void CreateHiddenField() {
    hiddenField = new HiddenField();
    hiddenField.ID = "hiddenField";
    hiddenField.Value = Property.ToString().ToLower();
    hiddenField.ValueChanged += hiddenField_ValueChanged;
    Controls.Add(hiddenField);
  }
  protected virtual void CreateContent() {
    contentPanel = new Panel();
    contentPanel.ID = "content";
    contentPanel.Vsiible = Property;
    Controls.Add(contentPanel);
  }
  void hiddenField_ValueChanged(object sender, EventArgs e) {
    Property = Convert.ToBoolean(hiddenField.Value);
  }
  protected override void OnInit(EventArgs e) {
    EnsureChildControls();
    base.OnInit(e);
  }
}

You can just expose hiddenField.Value in the property.

public bool Property
{
    get
    {
        EnsureChildControls();
        return Convert.ToBoolean(hiddenField.Value);
    }
    set
    {
        EnsureChildControls();
        hiddenField.Value = value.ToString();
    }
}

Property will expose the current value on postback by it self.

You could also expose an event on your control that you refire in the ValueChanged event of the hidden field.

public event EventHandler PropertyChanged;

...

void hiddenField_ValueChanged(object sender, EventArgs e)
{
    ContentPanel.Visible = Property;
    if (PropertyChanged != null) PropertyChanged(this, EventArgs.Empty);
}

I moved the visibility here since it's more related to the value changing than the property itself.


Setting Pokemon stats I set the stats (.ie. name, attack, defense, HP) in the __init__ method. However, I have chosen to build the self.attacks dictionary with a separate method.

class Pokemon(object):

     def __init__(self, name, attack, defense, HP):
                self.name = name
                self.attack = attack
                self.defense = defense
                self.HP = HP
                self.attacks = {}

     def Moves(self,move_1,damage_1,move_2,damage_2):
                self.attacks[move_1] = damage_1
                self.attacks[move_2] = damage_2

pikachu = Pokemon('Pikachu',40,10,50)
pidgey = Pokemon('Pidgey',50,30,60)

pikachu.Moves('tackle',50,'thunderbolt',70)
pidgey.Moves('gust',80,'tackle',50)

The code works as expected but I'm not sure if this proper use of classes. I believe it is and have seen some examples do similar things. I just wanted to check if I am headed in the right direction.

Edit:  The reason I would like to know is because the Pokemon will be getting many more attributes and I need a way to assign those attributes in groups to avoid assigning fifty or so variables every time I create a new instance of Pokemon.

You need to step away from programming and design your classes.
Pokemon stats aren't just one number. And so you should design for this.

First things first, Pokemon have multiple stats, the main ones are:

Each Pokemon of the same breed have the same base stats.
Say you have two Pikachu, their base stats are exactly the same.
And so you should define this on a Pokemon's class.
They have 'genes', better know as IVs - individual values.
These change for each and every Pokemon, two Pikachu are unlikely to have the same IVs.
They also don't change, ever. And so it'd be a good idea to make these immutable.
They learn things by fighting, and gain EVs - effort values.
These aren't immutable all the time, but have a limit, and become immutable at that point.
Pokemon have a nature, which also change a Pokemon's stats.

And so, if you want the program to be correct, you need to correctly handle the above.
But you need to handle all of them differently:

For base stats, you want to use Composition over inheritance.
For IVs, you want to use a namedtuple, or just a tuple.
For EVs, you want to have a user defined type.

And so you don't need to take 50 arguments in the creation of a Pokemon. You need 4 arguments, and if you want to pass the moves, 5.
To start you off you'd need something like:

from collections import namedtuple

Stats = namedtuple('Stats', 'hp attack defense sp_atk sp_def speed')

# TODO: add limits on input
class EV(object):
    def __init__(self, hp, attack, defense, sp_atk, sp_def, speed):
        self.hp = hp
        self.attack = attack
        self.defense = defense
        self.sp_atk = sp_atk
        self.sp_def = sp_def
        self.speed = speed

class Pokemon(object):
    _base_stats = Stats(0, 0, 0, 0, 0, 0)
    def __init__(self, iv, ev, nature, level):
        # TODO: check input is within correct bounds
        self._iv = Stats(*iv)
        self._ev = EV(*ev)
        self._nature = nature
        self.level = level
        # TODO: take more data, such as the pokemon's name.

    @property
    def max_hp(self):
        base = self._base_stats.hp
        iv = self._iv.hp
        ev = self._ev.hp
        level = self.level
        return int(((2 * base + iv + int(ev/4)) * level) / 100) + level + 10

    def _calculate_stat(self, stat):
        base = getattr(self._base_stats, stat)
        iv = getattr(self._iv, stat)
        ev = getattr(self._ev, stat)
        level = self.level
        # TODO: correct nature
        nature = 1
        return int((int(((2 * base + iv + int(ev/4)) * level) / 100) + 5) * nature)

    @property
    def attack(self):
        return self._calculate_stat('attack')

    @property
    def defense(self):
        return self._calculate_stat('defense')

class Pikachu(object):
    _base_stats = Stats(35, 55, 40, 50, 50, 90)

def pokemon(base, *args, **kwargs):
    return type('Pokemon', (base, Pokemon), dict())(*args, **kwargs)

pikachu = pokemon(Pikachu, [0, 0, 0, 0, 0, 0], [0, 0, 0, 0, 0, 0], None, 100)
print(pikachu.max_hp)
print(pikachu.attack)

Even if you don't use the previous, then I'd still recommend that you move the data into smaller more logical datatypes.
But either way you should want to then change the way you're storing your moves.
Each move should be an object, it has a name, damage, type, miss chance, special animation. You need to contain all of this in each move.
But you also need to have an instance of each move, so that every move in the game doesn't share the same PP.
And so to create each move, you may want to do the same as above with the Pikachu class.
After this, it'd make more sense to store these instances in a list rather than a dictionary.

And so you should instead have something like:

class Tackle(object):
    name = 'tackle'
    damage = 70

pikachu = pokemon(..., [Tackle()])

Going further, if this is too static, then you can use type to define these classes. And allow you to off-load storing the data in a database.

You should want to take the moves in the constructor of the class as it normally makes saving and loading game state easier.
If you did want to keep your Moves function then you should change it to add_move and only take one move, not two.


Output human readable time I wrote a little function which formats a ms timestamp in a human readable way. I know there are lots of scripts out there, but I needed a very simple one, which only outputs minutes and hours.

I was wondering if there is any way to shorten or improve this code snippet, because to me it seems to be very long for the few things it actually does.

function prettyTime(ms) {
      var sec = ms / 1000,
          time,
          timeUnit;

      if(sec > 3600) {
          // Hours
          time = Math.round(sec / 3600);
          timeUnit = ' hr';
      } else if(sec > 60) {
          // Minutes
          time = Math.round(sec / 60);
          timeUnit = ' min';
      } else {
          return 'less than 1 min';
      }

      if(time > 1) {
          timeUnit += 's';
      }

      return 'about ' + time + timeUnit + 'ago';
}

alert(prettyTime(7600001));

Hope this might be helpful -

function getRelativeTime(ms){
    var SECOND_MS = 1000;
    var MINUTE_MS = 60 * SECOND_MS;
    var HOUR_MS = 60 * MINUTE_MS;
    var DAY_MS = 24 * HOUR_MS;
    var WEEK_MS = 7 * DAY_MS;
    var MONTH_MS = 30 * DAY_MS;

    var lookup = ["months", "weeks", "days", "hours", "minutes", "seconds"];
    var values = [];
    values.push(ms / MONTH_MS); ms %= MONTH_MS;
    values.push(ms / WEEK_MS); ms %= WEEK_MS;
    values.push(ms / DAY_MS); ms %= DAY_MS;
    values.push(ms / HOUR_MS); ms %= HOUR_MS;
    values.push(ms / MINUTE_MS); ms %= MINUTE_MS;
    values.push(ms / SECOND_MS); ms %= SECOND_MS;

    var pretty = "about ";
    for(var i=0 ; i <values.length; i++){
        var val = Math.round(values[i]);
        if(val <= 0) continue;

        pretty += val + " " + lookup[i] + " ago";
        break;
    }
    return pretty;
}

getRelativeTime(10000);

Output :

about 10 seconds ago

You can easily customize above code for your "xx hours and xx mins ago" use-case. Just change the for loop like this -

for(var i=0 ; i <values.length; i++){
    var val = Math.round(values[i]);
    if(val <= 0) continue;

    pretty += val + " " + lookup[i];

    var nextval = Math.round(values[i+1]);
    if(i+1 < values.length && nextval > 0){
      pretty += " and " + nextval + " " + lookup[i+1] + " ago";
    }
    else {
      pretty += " ago";
    }
    break;
}

Output

getRelativeTime(100000);

about 2 minutes and 40 seconds ago

getRelativeTime(1000000);

about 17 minutes and 40 seconds ago

UPDATE:

  Math.round is causing wrong result in above code, Math.floor is more
  appropriate.


Go Quiz exercise, part of Gophercises This is the first exercise from https://gophercises.com/.
Basically the idea is to parse a CSV file in a form of 'question:answer' and count the number of correct answers. Also you have a time out range in which you must finish it.
I would appreciate any comments, good or bad :) Thanks a lot !
package main

import (
        "encoding/csv"
        "os"
        "fmt"
        "flag"
        "time"
)

const Ready = "yes"

var correctAnswers int

func getFileContent(filename string) (records[][] string, err error) {
        fd, err := os.Open(filename)

        if err != nil {
                exit("Problem with opening the file")
        }

        reader := csv.NewReader(fd)
        return reader.ReadAll()
}

func outputResult(totalQuestions, correctAnswers int) {
        fmt.Println("Number of total questions:", totalQuestions)
        fmt.Println("Number of corrected answers:", correctAnswers)
}

func exit(message string) {
        fmt.Println(message)
        os.Exit(3)
}

func provideQuestions(questions[][] string, done chan bool) {
        for index, value := range questions {
                question, answer := value[0], value[1]
                fmt.Println("Question N:=", index+1, "=>", question)

                var userAnswer string
                fmt.Scanf("%s", &userAnswer)

        if userAnswer == answer {
            correctAnswers ++
        }
        }

        done <- true
}

func main() {
    fileName := flag.String("name of csv file", "problems.csv", "The name of the csv file - default to 'problems.csv'")
    allowedTime := flag.Int("allowed time", 10, "The duration in seconds for which the quizz must be finished.")
    flag.Parse()

    var readyToStart string
    fmt.Println("Type 'yes' if you are ready to start")
    fmt.Scanf("%s", &readyToStart)

    if readyToStart != Ready {
        fmt.Println("Come back later if you are ready")
        os.Exit(0)
    }

    records, err := getFileContent(*fileName)

    if err != nil {
            exit("Reader problem.")
    }

    done := make(chan bool)
    timer := time.NewTimer(time.Duration(*allowedTime) * time.Second)

    go provideQuestions(records, done)

    select {
    case <- done:
            fmt.Println("Great job")
            fmt.Println("You finished with all questions.")
    case <- timer.C:
            fmt.Println("Time expired")
        }

    outputResult(len(records), correctAnswers)

}

Your function returns error, but actually you don't return it and exit in the function body when opening the file:
func getFileContent(filename string) (records[][] string, err error) {
        fd, err := os.Open(filename)

        if err != nil {
                exit("Problem with opening the file")
        }

        reader := csv.NewReader(fd)
        return reader.ReadAll()
}

From the point of error handling it will be more consistent to return error:
fd, err := os.Open(filename)
if err != nil {
   return nil, err
}

The file was open, but not closed: defer fd.Close().
I don't remember flag package, but if it is not ensure the presence of parameter you need to check filename != nil:
records, err := getFileContent(*fileName)

Here you have created new goroutine, but returns the result via global variable correctAnswers:
done := make(chan bool)
go provideQuestions(records, done)

It stops working if you decide to improve your game and allow more than one person to play on the same time. You will have race condition on this variable. Much better return the value from goroutine using channel.
You already have such channel - done, just change its type to channel of int and return the number of correct answers using it.


Guessing game in NASM Win64 Assembly Still learning Assembly. This program is a simple guessing game to test most of the new things I've learned! Any advice and all topical comments on code optimization and conventions is appreciated!

Compiled as follows using VS2017 x64 Native Tools Command Prompt:

> nasm -g -fwin64 guess.asm

> cl /Zi guess.obj msvcrt.lib legacy_stdio_definitions.lib

guess.asm

;; bits 64
default rel

extern time, srand, rand
extern printf, scanf

%macro str 2
    %2: db %1, 0
%endmacro

; use DQ to define a quad-word, DD to define a double-word, DW to define a word, and DB to define a byte.
section .rdata          ; immutable predefined variables
    str "Enter your guess (1-100): ", prompt
    str "%u", scan_fmt
    str "ERROR! Input was not a number!", scan_fail
    str {"Too high!", 10}, too_high
    str {"Too low!", 10}, too_low
    str {"You guessed it!", 13, 10}, congrats

; use RESQ to reserve a number of quad-words, RESD to reserve a number of double-words, RESW to reserve a number of words, and RESB to reserve a number of bytes.
section .bss            ; mutable undefined variables
    target: resb 4      ; 4 bytes of storage (enough for a 32bit number)

section .text
    global main
main:
    stack_reserve: equ 40      ; 32 + 8
    sub     rsp, stack_reserve ; shadow space for callees + 8 bytes for stack alignment

    ;; calculate the random number
    xor     rax, rax
    mov     rcx, rax        ; clear rax and rcx
    call    time
    mov     rcx, rax
    call    srand
    call    rand

    ;; rand = (rand % 100) + 1
    xor     rdx, rdx        ; clear rdx
    mov     rcx, 100
    div     rcx
    inc     rdx
    mov     [target], rdx
    ;mov     rcx, scan_fmt
    ;call    printf
.check:
    lea     rcx, [prompt]
    call    printf

    ; memory from rsp+0..31 has potentially been stepped on by printf
    ; leave RSP where it is, ready for another call

    ;;; scanf into that 8-byte block of stack space above the shadow space, or into our *own* shadow space
    lea     rdx, [rsp+32]        ; stack addresses are normally 64-bit, can't get away with edx
    lea     rcx, [scan_fmt]
    mov     dword [rdx], 0       ; instead of error check, set n = 0 in case of I/O error
    call    scanf
    cmp     eax, 1               ; success = exactly 1 conversion
    jnz     .scanf_fail          ; error check

    mov     r8d, [rsp+32]        ; r8d: 32-bit unsigned number to be checked

    cmp     r8d, [target]
    jg      .too_high
    jl      .too_low

    lea     rcx, [congrats]
    call    printf
.end:
    ;xor     eax,eax              ; return 0
    movzx   eax, byte [rsp+32]   ; return EXIT_SUCCESS(0) or EXIT_FAILURE(1)
    add     rsp, stack_reserve
    ret
.too_high:
    lea     rcx, [too_high]
    call    printf
    jmp     .check
.too_low:
    lea     rcx, [too_low]
    call    printf
    jmp     .check
.scanf_fail:
    lea     rcx, [scan_fail]
    call    printf
    jmp     .end

The usage of target is not consistent:

target: resb 4      ; 4 bytes of storage (enough for a 32bit number)
...
mov     [target], rdx ; store 8 bytes

That's easy to improve, just use edx.

Using rand like this is not very random, it's some function of the time in seconds. Seconds are long, you could easily start the program twice in the same second. Maybe that's fine for a simple number guessing game. On not-too-old processors there is a simpler and more random way to get a random number though:

.rand_retry:
    rdrand eax
    jnc .rand_retry

But that doesn't work on some older processors (Intel processors older than Ivy Bridge, for example Nehalem or Core2, and AMD processors older than Excavator), and can actually be slower than calling rand. Seeding the random number generator with the TSC value is more portable and less predictable than using time:

rdtsc ; read TSC into edx:eax
mov ecx, eax
call srand
call rand

Since you asked for performance tips (this program is obviously limited by waiting for user input but you can still learn useful things from this):

This remainder calculation is inefficient:

mov     rcx, 100
div     rcx     ; 64bit div, quite slow
inc     rdx

There are other options. For example, for a 232 input range (which rand normally does not deliver but rdrand does), a very similar-looking fixed-point scale can be used:

mov     ecx, 100
mul     ecx
inc     edx

The mul, combined with using the upper half of the result, effectively multiplies the input by 100 / 232 so 0xFFFFFFFF maps to 99 and so on. This method can be adjusted for smaller ranges such as up to RAND_MAX by multiplying by a higher constant, namely 100 * 232 / (RAND_MAX + 1)

The too low/too high cases could be less branchy and can share some code - admittedly they call an IO function anyway, but still. For example:

    cmp     r8d, [target]
    jne     .not_equal
...
.not_equal:
    lea     rcx, [too_high]
    lea     rdx, [too_low]
    ;; if "less than", use the too_low string
    cmovl   rcx, rdx
    call    printf
    jmp     .check


Equivalents permutations function My function is supposed to essentially give the cartesian product of all the letters in a string with the caveat that you have to give the permutations of every equivalent of each character in a string (capital and lowercase for example). This is what I came up with but I believe it has a time complexity of \$O(n*k^n)\$, \$k\$ being the number of equivalents of each character.

import string
from pprint import pprint

equivs = {x[0]:x for x in list(zip(string.ascii_lowercase,string.ascii_uppercase))}

st = "missifsffs"

def perms(st):
    st = st.lower()
    out = [""]
    for let in st:
        out = [y+x for y in out for x in equivs[let]]
    return out
print(perms("miss")) #['miss', 'misS', 'miSs', 'miSS', 'mIss', 'mIsS', 'mISs', 'mISS', 'Miss', 'MisS', 'MiSs', 'MiSS', 'MIss', 'MIsS', 'MISs', 'MISS']
print(perms("okay")) #['okay', 'okaY', 'okAy', 'okAY', 'oKay', 'oKaY', 'oKAy', 'oKAY', 'Okay', 'OkaY', 'OkAy', 'OkAY', 'OKay', 'OKaY', 'OKAy', 'OKAY']

print(perms(st))

For starter, you shouldn't use abreviations for your variables/functions names: perms for permutations, st for string, let for letter, equiv for equivalence_table… You gain nothing doing so, except obfuscating what is going on which impairs readability.

Then I find that your function has a wrong name, as permutations refers more naturally to swapping the order of the letters. I would rather use something along the line of case_swap or character_equivalence. I find these names more descriptive in constructs of the form for word in <function_name>('I got it.').

Now, what about the behaviour of your function when given a string with characters that are not in string.ascii_lowercase or string.ascii_uppercase, as the spaces or the dot in my previous example? You should handle such cases where let is not a key in equivs. And the simplest to do so is to fallback to using only let in such cases: equivs[let] can become equivs.get(let, let).

An other thing, that I particularly like with Python, is how it is simple to write lazy functions. Instead of building whole lists of results and storing them in memory, you can just yield intermediate results as you compute them. And I have the feeling that such function would benefit from being writen as a generator since I believe it will most likely be called to iterate over its result. And to do that without computing a whole bunch of intermediary string before the first usefull result, we can:

use a recursive function to prepend the equivalents of the first letter to the computed equivalents of the rest of the string; or
ask python to generate the cartesian product by providing ourselves a list of equivalent values for each letter.

The recursive function would look like:

def swap_case(sentence, equivalence_table):
    letter, *rest = sentence
    equivalents = equivalence_table.get(letter, letter)
    if not rest:
        yield from equivalents
    else:
        for end in swap_case(rest, equivalence_table):
            for symbol in equivalents:
                yield symbol + end

but recursion has its limits, especially in the number of function calls you can perform. So I would prefer to use the product generator from itertools to leverage fast iteration optimized in C:

import itertools

def swap_case(sentence, equivalence_table):
    equivalent_letters = (equivalence_table.get(l, l) for l in sentence)
    for characters in itertools.product(*equivalent_letters):
        yield ''.join(characters)

Here the join is needed since itertools.product generates lists of values taken from the provided iterables.

You can also sacrifice a bit of readability to continue iterating in C rather than using a for loop in Python by using map to apply ''.join to each list yielded by itertools.product:

def swap_case(sentence, equivalence_table):
    equivalent_letters = (equivalence_table.get(l, l) for l in sentence)
    return map(''.join, itertools.product(*equivalent_letters))

All in all, both version doesn't change the time complexity of the algorithm since you still have to generate the same amount of values. But the memory usage should be drastically lower.

You may also have noted that I changed the signature to include the equivalence table as a parameter. This is twofold:

easier testing as the function does not relly on a global variable;
parametrizable output as we may want some equivalences in some cases but not in others.

And if we want to easily provide various equivalence tables, we should as well write a function that makes it for us. Looking back at how swap_case will handle it, we should provide the equivalent letters as a value for each of the symbols as a key. Let's also provide an option to include/exclude uppercase letter as well:

def build_equivalence_table(use_uppercase=True, **kwargs):
    values = string.ascii_lowercase
    if use_uppercase:
        values = map(''.join, zip(values, string.ascii_uppercase))
    table = dict(zip(string.ascii_lowercase, values))
    for letter, equivalents in kwargs.items():
        table[letter.lower()] += equivalents
    # At this points, keys of `table` are only lowercase letters
    # let's include all the variants as well
    return {variant: group for group in table.values() for variant in group}

Usage being:

>>> build_equivalence_table(a='@4', t='7', l='!1', e='3')
{'p': 'pP', 'k': 'kK', 'r': 'rR', 'V': 'vV', 'U': 'uU', 'I': 'iI', 'i': 'iI', 'J': 'jJ', 'A': 'aA@4', 'n': 'nN', 's': 'sS', 'K': 'kK', 'D': 'dD', '1': 'lL!1', 'j': 'jJ', 'z': 'zZ', 'B': 'bB', 'w': 'wW', 'C': 'cC', 'P': 'pP', 'N': 'nN', 'Y': 'yY', 'G': 'gG', 'g': 'gG', 'q': 'qQ', 'T': 'tT7', 'h': 'hH', 'M': 'mM', '7': 'tT7', '!': 'lL!1', 'F': 'fF', 'd': 'dD', 'X': 'xX', 'l': 'lL!1', 'f': 'fF', 't': 'tT7', 'y': 'yY', 'e': 'eE3', 'x': 'xX', 'v': 'vV', 'b': 'bB', 'H': 'hH', 'Z': 'zZ', 'L': 'lL!1', 'S': 'sS', 'c': 'cC', 'a': 'aA@4', 'Q': 'qQ', '4': 'aA@4', 'W': 'wW', 'E': 'eE3', 'R': 'rR', 'o': 'oO', '@': 'aA@4', 'm': 'mM', '3': 'eE3', 'u': 'uU', 'O': 'oO'}


How can I make placing JButtons from an array more elegant? I am trying to place JButtons from an array onto a JFrame. The way I'm doing it is having it test how many more buttons are left, and if buttons have been placed at the edge of the frame. The end result is an ugly piece of code.

JButton[] grid = new JButton[2501];

JFrame MapFrame = new JFrame();

public void makeMap() {

    MapFrame.setBounds(40, 0, 750, 773);

    int x = 0;
    int y = 0;
    for (int i = 0; i < grid.length; i++) {
   // grid is the JButton Array

        if (x > 749) {
            x = 0;
            y = y + 15;
        }
        grid[i] = new JButton();
        grid[i].setBounds(x, y, 15, 15);
        MapFrame.add(grid[i]);

        x = x + 15;
    }
    MapFrame.setVisible(true);
    MapFrame.repaint();
}

The code just looks so bulky with variables being changed in different braces, and with so many braces. How could I make this more elegant? (Please don't recommend layouts, as, none of them fit my requirements.)

In order to find a more elegant solution, we first need to identify the problems; only then can we solve them:

Magic Numbers
One of the confusing things about this snippet is that we constantly (pun intended) come across magic values such as 15 and 749. What if the map gets bigger in the future, or the dimensions of the buttons change? Solution: Define constants.
Note: I used the Java naming convention for constants, which is SHOUTY_CASE, although I dislike it, because unified coding standards when sharing code trump personal preferences.

private static final int NUMBER_OF_BUTTONS = 2501;
private static final int BUTTON_SIDE = 15;

Unnecessary use of array
Why are you copying every button into your grid before adding them to the mapFrame? If you aren't accessing them from the array later on, you can get rid of all the access-by-index complexity.

JButton button = new JButton();
button.setBounds(x, y, BUTTON_SIDE, BUTTON_SIDE);
mapFrame.add(button);

Function doing too many things
The empty lines that you have used to split your code into sections is a code smell: it indicates that your function is doing too many different things, and therefore needs to be divided into several functions, each doing one thing. To quote Robert C. Martin, author of Clean Code: A Handbook of Agile Software Craftsmanship:

  Functions should do one thing. They should do it well. They should do it only.

Using complex syntax for simple things
Code like the following two examples from your code snippet

x = x + 15;
y = y + 15;

can be shortened by using the combined += operator.

Refactored

private static final int NUMBER_OF_BUTTONS = 2501;
private static final int BUTTON_SIDE = 15;
private JFrame mapFrame = new JFrame();

public void createAndShowMap() {
    mapFrame.setBounds(40, 0, 750, 773);
    addButtons();
    mapFrame.setVisible(true);
}

private void addButtons() {
    int leftOffset = 0;
    int topOffset = 0;
    for (int i = 0; i < NUMBER_OF_BUTTONS; i++) {
        if (isBeyondEndOfLine(leftOffset)) {
            leftOffset = 0;
            topOffset += BUTTON_SIDE;
        }
        addButtonAt(leftOffset, topOffset);
        leftOffset += BUTTON_SIDE;
    }
}

private boolean isBeyondEndOfLine(int x) {
    return x >= mapFrame.getBounds().width;
}

private void addButtonAt(int x, int y) {
    JButton button = new JButton();
    button.setBounds(x, y, BUTTON_SIDE, BUTTON_SIDE);
    mapFrame.add(button);
}


Time conversion program optimization I have just finished working on a problem on hackerrank dealing with time conversion in C++.

I'm still fairly new to programming as I only have a few semesters of programming classes under my belt and had a pretty challenging time with this program.

Below is a link to the problem itself and my code.
I solved the problem but I feel like my time complexity is unnecessarily high.  It works to solve all the test cases the site provides but I am looking to become a better developer so I want to learn to write more efficient and effective code.

Input Format

  A single string containing a time in 12-hour clock format (i.e.:hh:mm:ssAM  or hh:mm:ssPM), where 01 <= hh <= 12.

Output Format

  Convert and print the given time in 24-hour format, where 00 <= hh <= 23.

Please tell show me how I can optimize this code for a better run time.

Link to the problem

#include <cmath>
#include <cstdio>
#include <vector>
#include <iostream>
#include <algorithm>
#include <sstream>
#include<iomanip>
using namespace std;

int main(){
    string time;
    cin >> time;
    char delim = ':'; // delimiter for extracting colons from input
    stringstream newTime; // stream to read string that holds time info
    newTime << time;
    int hours,minutes,seconds; // variables to hold data
    string ampm;
    newTime >> hours;
    newTime >> delim;
    newTime>> minutes;
    newTime >> delim;
    newTime>> seconds;
    newTime>> ampm;
    if(ampm == "PM"){ // for changing hours to 0-23 scale
        switch(hours){
            case 1:hours = 13;
            break;
            case 2:hours = 14;
            break;
            case 3:hours = 15;
            break;
            case 4:hours = 16;
            break;
            case 5:hours = 17;
            break;
            case 6:hours = 18;
            break;
            case 7:hours = 19;
            break;
            case 8:hours = 20;
            break;
            case 9:hours = 21;
            break;
            case 10:hours = 22;
            break;
            case 11:hours = 23;
            break;
        }
    }
    else if(ampm == "AM" && hours == 12 ){ // for changing 12am to 00am
        hours = 0;

    }
      //use of iomanip functions below to received desired output
    cout <<  setw(2)<< setfill('0') << hours << ":" << setw(2)<< setfill('0') << minutes << ":" << setw(2)<< setfill('0') << seconds << endl;
    return 0;
}

If you were writing this for real world use1, I think the right way would be to use std::get_time and std::put_time, something on this order:

#include <iostream>
#include <iomanip>
#include <ctime>

int main() {
    struct std::tm t;
    std::cin >> std::get_time(&t, "%I:%M:%S%p");
    std::mktime(&t);
    std::cout << std::put_time(&t, "%H:%M:%S");
}

(Note: it's open to question whether the call to mktime is really necessary here, but at worst it's still pretty harmless).

Switch Statement

Looking more specifically at your code, the thing that probably sticks out the most is the big switch statement. I'd eliminate that in favor of a tiny bit of math:

if (ampm == "PM")
    hours += 12;

Reading Delimiters

Right now, the code to initially read the data is made somewhat ugly and unreadable (and arguably failure-prone) by it's somewhat poor handling of delimiters. It currently just reads a character and assumes it's a delimiter. In real life, you probably want to at least compare what you read to what you expected so ensure that the input is correctly formatted (and react appropriately if it's not).

Personally, I think I'd write a little operator overload like this:

std::istream &operator >> (std::istream &is, char const *pat) {

    char ch;
    while (isspace(static_cast<unsigned char>(is.peek())))
        is.get(ch);

    while (*pat && is && *pat == is.peek() && is.get(ch))
        ++pat;

    // if we didn't reach the end of the pattern, matching failed (mismatch, premature EOF, etc.)
    if (*pat) {
        is.setstate(std::ios::failbit);
    }

    return is;
}

At least to me, this supports much cleaner code to read the data:

cin >> hour >> ":" >> minute >> ":" >> second >> ampm;

...and the operator overload takes care of reading data, comparing to what was passed, and setting the stream's failbit if it didn't match. As it stands right now, I've also included skipping any leading white-space like normal extraction operators do, but it would be trivial to eliminate that if you didn't want it (but it shouldn't make any difference for solving the HackerRank problem).

1. HackerRank is apparently using an old/broken standard library, so it won't accept this, even when you try to tell it you want to use C++14.


Sliding window iteration in Python I'm creating a small library of Python utilities, and I'd like feedback on a function which allows iterating over an arbitrary iterable in a sliding-window fashion.

Relevant parts of iteration.py:

import collections
import itertools

def sliding_window_iter(iterable, size):
    """Iterate through iterable using a sliding window of several elements.

    Creates an iterable where each element is a tuple of `size`
    consecutive elements from `iterable`, advancing by 1 element each
    time. For example:

    >>> list(sliding_window_iter([1, 2, 3, 4], 2))
    [(1, 2), (2, 3), (3, 4)]
    """
    iterable = iter(iterable)
    window = collections.deque(
        itertools.islice(iterable, size-1),
        maxlen=size
    )
    for item in iterable:
        window.append(item)
        yield tuple(window)

Relevant parts of the test file iteration_test.py:

import doctest
import unittest

import iteration
from iteration import *

class TestSlidingWindowIter(unittest.TestCase):
    def test_good(self):
        self.assertSequenceEqual(
            list(sliding_window_iter([1, 2, 3, 4], 2)),
            [(1, 2), (2, 3), (3, 4)]
        )

    def test_exact_length(self):
        self.assertSequenceEqual(
            list(sliding_window_iter(["c", "b", "a"], 3)),
            [("c", "b", "a")]
        )

    def test_short(self):
        self.assertSequenceEqual(
            list(sliding_window_iter([1, 2], 3)),
            []
        )

    def test_size_one(self):
        self.assertSequenceEqual(
            list(sliding_window_iter([1, 2, 3, 4], 1)),
            [(1,), (2,), (3,), (4,)]
        )

    def test_bad_size(self):
        with self.assertRaises(ValueError):
            list(sliding_window_iter([1, 2], 0))

def run():
    if not doctest.testmod(iteration)[0]:
        print("doctest: OK")
    unittest.main()

if __name__ == "__main__":
    run()

I'm primarily looking for feedback in these areas:

Is the code Pythonic? I'm primarily a C++ developer, so Python idioms don't come naturally to me; that's one thing I'm constantly trying to improve.
Are there any potential performance problems?
Am I reinventing a wheel and something like this already exists?

I'll welcome any other feedback too, of course.

Looks pretty good to me. You are taking advantage of the standard library, are following the relevant style guides and you even have tests, good job!

Personally, I am more in favor of using import from, at least for well-known standard library functions and maybe for utility functions that are used everywhere throughout a project. This way you can more often fit the code on one line easily, making it more readable.

from itertools import islice
from collections import deque

One small improvement is that your initial slice can be one larger, the argument behaves just like a range or slice argument and is exclusive of the end (i.e islice(x, 4) takes the first four elements since it is analogous to x[0:4], x[:4] and range(0, 4)). However, this way you need to deal with the last element in a special way, so YMMV:

def sliding_window_iter(iterable, size):
    """..."""
    iterable = iter(iterable)
    window = deque(islice(iterable, size), maxlen=size)
    for item in iterable:
        yield tuple(window)
        window.append(item)
    if window:
        # needed because if iterable was already empty before the `for`,
        # then the window would be yielded twice.
        yield tuple(window)

This also reveals one behavior which you should at least be aware of (which seems to be the case, since you have a test for it). Arguable, sliding_window_iter([1, 2], 3) could yield (1, 2), i.e. the function maybe shouldn't swallow any data. In any case, the chosen behavior should be documented in the docstring.


Click handlers for SVG polygons How can I write this shorter?

$('#pietra_svg #p1_4 polygon#p1').click(function() {
  $('#apla_p1').fadeIn("slow");
  return false;
});
$('#pietra_svg #p1_4 polygon#p2').click(function() {
  $('#apla_p2').fadeIn("slow");
  return false;
});
$('#pietra_svg #p1_4 polygon#p3').click(function() {
  $('#apla_p3').fadeIn("slow");
  return false;
});
$('#pietra_svg #p1_4 polygon#p4').click(function() {
  $('#apla_p4').fadeIn("slow");
  return false;
});
$('#pietra_svg #p5 polygon#a').click(function() {
  $('#apla_p5').fadeIn("slow");
  return false;
});
$('#pietra_svg #p5 polygon#b').click(function() {
  $('#apla_p5').fadeIn("slow");
  return false;
});
$('.close').click(function() {
  $('.apla').fadeOut("slow");
  return false;
});

You can use an array in a similar way here. Just leave the last one, as it doesn't fit the same pattern.

$.each(
  [
    { poly: 'p1', id: '#apla_p1' },
    { poly: 'p2', id: '#apla_p2' },
    { poly: 'p3', id: '#apla_p3' },
    { poly: 'p4', id: '#apla_p4' },
    { poly: 'a', id: '#apla_p5' },
    { poly: 'b', id: '#apla_p5' }
  ],
  function(o){
    var id = o.id; // copy to closure
    $('#pietra_svg #p1_4 polygon#' + o.poly).click(function() {
      $(id).fadeIn("slow");
      return false;
    });
  }
);

$('.close').click(function() {
  $('.apla').fadeOut("slow");
  return false;
});


Arranging 8 queens on an 8x8 board Print all the ways of arranging eight queens on an 8x8 board. Rules:
  Queens should not share the same row, column, of any diagonal.

Any comment on my solution?
I didn't write unit tests but I checked visually the solutions.

public class solution {

  public static void printMatrix(char[][] board) {
    // 8x8 board
    int n = board.length;
    System.out.println("------------------------------");
    for (int i = 0; i < n; i++) {
      for (int j = 0; j < n; j++) {
        System.out.print(board[i][j] + "|");
      }
      System.out.println();
    }
    System.out.println("------------------------------");

  }

  public static void updateBoard(int row, int col, boolean[][] logicalBoard) {
    // Turn all the cases corresponding to the row, column and diagonals of the element added to
    // false
    // 8x8 board
    int n = logicalBoard.length;

    // update the row
    for (int j = 0; j < n; j++) {
      logicalBoard[row][j] = false;
    }

    // update the column
    for (int j = 0; j < n; j++) {
      logicalBoard[j][col] = false;
    }

    // update the diagonals
    int r = row;
    int c = col;
    while (r >= 0 && c >= 0) {
      logicalBoard[r][c] = false;
      r--;
      c--;
    }

    r = row;
    c = col;
    while (r < 8 && c < 8) {
      logicalBoard[r][c] = false;
      r++;
      c++;
    }

    r = row;
    c = col;
    while (r < 8 && c >= 0) {
      logicalBoard[r][c] = false;
      r++;
      c--;
    }

    r = row;
    c = col;
    while (r >= 0 && c < 8) {
      logicalBoard[r][c] = false;
      r--;
      c++;
    }
  }

  public static char[][] copyArray(char[][] original) {
    char[][] copy = new char[original.length][];
    for (int i = 0; i < original.length; i++) {
      copy[i] = original[i].clone();
    }
    return copy;
  }

  public static boolean[][] copyArray(boolean[][] original) {
    boolean[][] copy = new boolean[original.length][];
    for (int i = 0; i < original.length; i++) {
      copy[i] = original[i].clone();
    }
    return copy;
  }

  public static void arrangeQueens(char[][] board, boolean[][] logicalBoard, int rowNumber) {
    if (rowNumber == 8) {
      printMatrix(board);
      return;
    }

    int n = board.length;
    for (int column = 0; column < n; column++) {
      if (logicalBoard[rowNumber][column]) {
        char[][] newBoard = copyArray(board);
        boolean[][] newLogicalBoard = copyArray(logicalBoard);
        newBoard[rowNumber][column] = 'Q';
        newLogicalBoard[rowNumber][column] = false;
        updateBoard(rowNumber, column, newLogicalBoard);
        arrangeQueens(newBoard, newLogicalBoard, rowNumber + 1);
      }
    }
  }

  public static void arrangeQueens() {
    int N = 8;
    char[][] board = new char[8][8];
    boolean[][] logicalBoard = new boolean[8][8];
    for (int i = 0; i < N; i++) {
      for (int j = 0; j < N; j++) {
        board[i][j] = ' ';
        logicalBoard[i][j] = true;
      }
    }
    arrangeQueens(board, logicalBoard, 0);
  }

  public static void main(String[] args){
    arrangeQueens();
  }

}

Some points:

public class solution {

Java's conventions say that class names should be PascalCase, not camelCase:

public class Solution {

would be correct.

Another thing is that you print results like:

 | | | | | | |Q|
 |Q| | | | | | |
 | | | |Q| | | |
 | |Q| | | | | |
Q| | | | | | | |
 | | | | | |Q| |
 | | |Q| | | | |
 | | | | |Q| | |

Why not do:

| | | | | | | |Q|
| |Q| | | | | | |
| | | | |Q| | | |
| | |Q| | | | | |
|Q| | | | | | | |
| | | | | | |Q| |
| | | |Q| | | | |
| | | | | |Q| | |

Therefore, printMatrix() will now be:

public static void printMatrix(char[][] board) {
    // 8x8 board
    int n = board.length;
    System.out.println("------------------------------");
    for (int i = 0; i < n; i++) {
        System.out.print("|");
        for (int j = 0; j < n; j++) {
            System.out.print(board[i][j] + "|");
        }
        System.out.println();
    }
    System.out.println("------------------------------");
}

Since this is Java, you might as well have some OOP in your code. It does not look like OOP at all. After some redesigning with OOP, your code will look like this:

public class Solution {

    public static void main(String[] args) {
        List<QueenBoard> solutions = new NQueensProblem(8).getAllSolutions();
        for(QueenBoard board : solutions) {
            System.out.println(board.toString());
        }
    }

}

class NQueensProblem {

    private int size;

    private List<QueenBoard> solutions;

    public NQueensProblem(int size) {
        this.size = size;
        this.solutions = new LinkedList<QueenBoard>();
        solve(size);
    }

    public void solve(int size) {
        QueenBoard board = new QueenBoard(size);
        LogicalBoard logicalBoard = new LogicalBoard(size);
        solve(board, logicalBoard, 0, size);
    }

    public List<QueenBoard> getAllSolutions() {
        return solutions;
    }

    private void solve(QueenBoard board, LogicalBoard logicalBoard,
            int rowNumber, int size) {
        if (rowNumber == size) {
            solutions.add(board);
            return;
        }
        for (int column = 0; column < size; column++) {
            if (logicalBoard.getPossible(rowNumber, column)) {
                QueenBoard newBoard = new QueenBoard(board);
                newBoard.setHasQueen(rowNumber, column, true);
                LogicalBoard newLogicalBoard = new LogicalBoard(logicalBoard);
                newLogicalBoard.setPossible(rowNumber, column, false);
                updateBoard(rowNumber, column, newLogicalBoard, size);
                solve(newBoard, newLogicalBoard, rowNumber + 1, size);
            }
        }
    }

    private void updateBoard(int row, int col, LogicalBoard logicalBoard, int size) {
        // update the row
        for (int j = 0; j < size; j++) {
            logicalBoard.setPossible(row, j, false);
        }
        // update the column
        for (int j = 0; j < size; j++) {
            logicalBoard.setPossible(j, col, false);
        }
        // update the diagonals
        int r = row;
        int c = col;
        while (r >= 0 && c >= 0) {
            logicalBoard.setPossible(r, c, false);
            r--;
            c--;
        }
        r = row;
        c = col;
        while (r < 8 && c < 8) {
            logicalBoard.setPossible(r, c, false);
            r++;
            c++;
        }
        r = row;
        c = col;
        while (r < 8 && c >= 0) {
            logicalBoard.setPossible(r, c, false);
            r++;
            c--;
        }
        r = row;
        c = col;
        while (r >= 0 && c < 8) {
            logicalBoard.setPossible(r, c, false);
            r--;
            c++;
        }
    }

}

class QueenBoard {

    private boolean[][] board;
    int size;

    public QueenBoard(int size) {
        this.size = size;
        this.board = new boolean[size][size];
    }

    public QueenBoard(QueenBoard oldBoard) {
        this.size = oldBoard.size;
        this.board = new boolean[size][size];
        for(int i = 0; i < size; i++) {
            for(int j = 0; j < size; j++) {
                this.board[i][j] = oldBoard.board[i][j];
            }
        }
    }

    public void setHasQueen(int x, int y, boolean value) {
        board[x][y] = value;
    }

    public boolean hasQueen(int x, int y) {
        return board[x][y];
    }

    @Override
    public String toString() {
        StringBuilder result = new StringBuilder();
        result.append("------------------------------\n");
        for (int i = 0; i < size; i++) {
            result.append("|");
            for (int j = 0; j < size; j++) {
                result.append((board[i][j] ? "Q" : " ") + "|");
            }
            result.append("\n");
        }
        result.append("------------------------------");
        return result.toString();
    }

}

class LogicalBoard {

    private boolean[][] board;
    int size;

    public LogicalBoard(int size) {
        this.size = size;
        board = new boolean[size][size];
        for(int i = 0; i < size; i++) {
            for(int j = 0; j < size; j++) {
                this.board[i][j] = true;
            }
        }
    }

    public LogicalBoard(LogicalBoard oldBoard) {
        this.size = oldBoard.size;
        this.board = new boolean[size][size];
        for(int i = 0; i < size; i++) {
            for(int j = 0; j < size; j++) {
                this.board[i][j] = oldBoard.board[i][j];
            }
        }
    }

    public void setPossible(int x, int y, boolean value) {
        board[x][y] = value;
    }

    public boolean getPossible(int x, int y) {
        return board[x][y];
    }

}

Major changes:

The board is now a class
The logical board is now a class
Instead of printing all the solutions while solving, you get all the solutions through getAllSolutions() once it is finished solving the puzzle, and then print them one by one with System.out.println(board.toString())
The solving methods are now non-static

This way, it looks much more Java-like.

Also:

private void updateBoard(int row, int col, LogicalBoard logicalBoard, int size) {
    // update the row
    for (int j = 0; j < size; j++) {
        logicalBoard.setPossible(row, j, false);
    }
    // update the column
    for (int j = 0; j < size; j++) {
        logicalBoard.setPossible(j, col, false);
    }
    // update the diagonals
    int r = row;
    int c = col;
    while (r >= 0 && c >= 0) {
        logicalBoard.setPossible(r, c, false);
        r--;
        c--;
    }
    r = row;
    c = col;
    while (r < 8 && c < 8) {
        logicalBoard.setPossible(r, c, false);
        r++;
        c++;
    }
    r = row;
    c = col;
    while (r < 8 && c >= 0) {
        logicalBoard.setPossible(r, c, false);
        r++;
        c--;
    }
    r = row;
    c = col;
    while (r >= 0 && c < 8) {
        logicalBoard.setPossible(r, c, false);
        r--;
        c++;
    }
}

looks over-repetitive. Try creating multiple methods to handle that:

private void updateBoard(int row, int col, LogicalBoard logicalBoard, int size) {
    updateRow(row, logicalBoard, size);
    updateColumn(col, logicalBoard, size);
    updateDiagonal(row, col, -1, -1, logicalBoard, size);
    updateDiagonal(row, col, 1, 1, logicalBoard, size);
    updateDiagonal(row, col, 1, -1, logicalBoard, size);
    updateDiagonal(row, col, -1, 1, logicalBoard, size);
}

private void updateRow(int row, LogicalBoard logicalBoard, int size) {
    for (int j = 0; j < size; j++) {
        logicalBoard.setPossible(row, j, false);
    }
}

private void updateColumn(int col, LogicalBoard logicalBoard, int size) {
    for (int j = 0; j < size; j++) {
        logicalBoard.setPossible(j, col, false);
    }
}

private void updateDiagonal(int row, int rowIncrement, int col, int colIncrement, LogicalBoard logicalBoard, int size) {
    while (row >= 0 && col < 8) {
        logicalBoard.setPossible(row, col, false);
        row += rowIncrement;
        col += colIncrement;
    }
}

This looks much nicer, and is less repetitive.


Function to print command-line usage for a program The following function works well to print out a help message for the program I have written. It prints a multi-line string message that describes the command line usage with some examples:

def usage():

    print(
        """
Usage examples:

Test deployment:
       $ fab [noinput] test deploy

Staging deployment:
       $ fab [noinput] staging deploy

Production deployment:
       $ fab [noinput] production deploy
""")

Unfortunately, in order to make the message look good in the output, the code block currently looks ugly with the string message making the indentation of the code look wrong. Is there a way to improve the code so that it is more readable, similar to the following code (which does not work - it is just an example) where the indentation is more readable?

def usage():

    print(
        """
        Usage examples:
        Test deployment:
            $ fab [noinput] test deploy
        Staging deployment:
          $ fab [noinput] staging deploy
        Production deployment:
          $ fab [noinput] production deploy
        """
    )

How can the readability be improved while still producing results that look good in the output?

You can use textwrap.dedent to remove common whitespace from the start of all lines in a multiline string:

>>> import textwrap
>>> print(textwrap.dedent(
        """
        Usage examples:
        Test deployment:
            $ fab [noinput] test deploy
        Staging deployment:
            $ fab [noinput] staging deploy
        Production deployment:
            $ fab [noinput] production deploy
        """
))

Usage examples:
Test deployment:
    $ fab [noinput] test deploy
Staging deployment:
    $ fab [noinput] staging deploy
Production deployment:
    $ fab [noinput] production deploy

As it says in the docs:

  This can be used to make triple-quoted strings line up with the left edge of the display, while still presenting them in the source code in indented form.


Sum of primes between two number from optimized Sieve of Eratosthenes Here is the code that I wrote to give me sum of prime numbers between n and m:

class TestClass {
    final static int MAX=1000000;
    final static boolean[] isPrime=isPrime();
    public static void main(String args[] ) throws Exception {
        BufferedReader keyboard= new BufferedReader(new InputStreamReader(System.in));
        int t=Integer.parseInt(keyboard.readLine());

        while(t>0 && t<=100){
            String[] tempInt=keyboard.readLine().split(" ");
            int n=Integer.parseInt(tempInt[0]);
            int m=Integer.parseInt(tempInt[1]);

            int sum=primeSum(n,m);
            System.out.println(sum);
            t--;
        }
    }

private static int primeSum(int n, int m) {
        int sum=0;

        for(int i=n;i<=m;i++){
            if(isPrime[i]){
                sum=sum+i;
            }
        }
        return sum;
    }

    private static boolean[] isPrime(){
        int maxFactor= (int)Math.sqrt(MAX);
        boolean[] isPrime=new boolean[MAX + 1];
        int len=isPrime.length;
        Arrays.fill(isPrime,true);
        isPrime[0]=false;
        isPrime[1]=false;
        for(int i=0;i<=maxFactor;i++){
            if(isPrime[i]){
                for(int j=i+i;j<len;j+=i){
                    isPrime[j]=false;
                }
            }
        }
        return isPrime;

    }

}

Input:

2
1 99999
10000 99999

Output:

454396537
448660141

Now I'm trying to further optimize sieve by just taking odd number what usually in practice. Here is the optimized sieve function  that I have written:

private static boolean[] isPrime(){
        int root=(int) Math.sqrt(MAX)+1;
        int limit=(MAX-1)/2;
        boolean[] isPrime=new boolean[limit];
        Arrays.fill(isPrime, true);
        root = root/2 -1;
        for(int i = 0; i < root ; i++){
            if(isPrime[i]){
                for( int j = 2*i*(i+3)+3 , p = 2*i+3; j < limit ; j=j+p ){
                    isPrime[j]=false;
                }
            }
        }

    return isPrime;
}

Which I was able to do. I tested the above function till MAX=100. Here is the Ideone link.

Test results:

    truetruetruefalsetruetruefalsetruetruefalsetruefalsefalsetruetruefalsefalsetrue
falsetruetruefalsetruefalsefalsetruefalsefalsetruetruefalsefalsetruefalse
truetruefalsefalsetruefalsetruefalsefalsetruefalsefalsefalsetruefalse

i.e 3 5 7 9̶ 11 13 1̶5̶ 17 19 ̶2̶1̶  23 2̶5̶ ̶2̶7̶  29 31 ̶3̶3̶ ̶3̶5̶ ̶  37 3̶9̶ ̶  so on..

Now what really bugging me is the indexing I did in primeSum() method for this optimized sieve:

private static int primeSum(int n, int m) {
        int sum;
        if(n>0 && n<=2){
            sum=2;
        }else
            sum=0;
        //System.out.println(sum);
        for( int i = (n-3)/2; i <= (m-3)/2 ; i++){
            if(isPrime[i]){
                //System.out.println(i);
                sum=sum+2*i+3;
            }
        }
        return sum;
    }

But obviously, this indexing of n is failing for n<3, so I have to do this to get this code working:

if(n>0 && n<=2){
            sum=2;
            n=n+2;
        }

But then it still fails for the cases when I've to find it between ranges:

1 2
1 1
2 2

So should I again include these cases and deal with it separately like this?

private static int primeSumInRange(int n, int m) {
        int sum;
        if(m ==1) return 0;
        if(n<=2){
            sum=2;
            n=n+2;
        }else
            sum=0;
            if(m>2){
        for( int i = (n-3)/2; i <= (m-3)/2 ; i++){
            if(isPrime[i]){
                sum=sum+2*i+3;
            }
        } }
        return sum;
    }

Is my way of doing the indexing i in primeSum() method proper? Or can I improve it? What are the other possible way of indexing here?

What you did is called Sieve of Sundaram and saves half the memory. You can save factor 8 if you use a BitSet (or implement it manually e.g. using a long[]). For small numbers it costs some time, but for big numbers it may be faster when random access is involved (less memory means that more fits in cache). When dealing with really big primes, you want both optimizations.

private static boolean[] isPrime(){
    int root=(int) Math.sqrt(MAX)+1;
    int limit=(MAX-1)/2;
    boolean[] isPrime=new boolean[limit];
    Arrays.fill(isPrime, true);
    root = root/2 -1;

You really shouldn't do things like this. The problem is already confusing by the fact that you start with a number but work with an index which is half as big (and rounded down and in your case I guess additionally decremented). It's much better to use one variable for one thing.

    for(int i = 0; i < root ; i++){
        if(isPrime[i]){
            for( int j = 2*i*(i+3)+3 , p = 2*i+3; j < limit ; j=j+p ){

Is it correct? How do you know that you should iterate exactly up to root? Why 2*i*(i+3)+3? The first value to cross out is usually p*p, which you need to transform to index.

I wouldn't use the p = 2*i+3 formula. When compared to p = 2*i+1, you save one bit, bit it gets even more confusing.

What about something like

int numberToIndex(int oddNumber) {
    // Asserts cost nothing unless switched on.
    assert (oddNumber & 1) == 1;
    // The right shift is just a faster division by 2
    return oddNumber >> 1;
}

int indexToNumber(int index) {
    return 2 * index + 1;
}

? By using these functions, you could make it nearly readable.

    truetruetruefalsetruetruefalsetruetruefalsetruefalsefalsetruetruefalsefalsetruefalsetruetruefalsetruefalsefalsetruefalsefalsetruetruefalsefalsetruefalsetruetruefalsefalsetruefalsetruefalsefalsetruefalsefalsefalsetruefalse

Can you read it? Do you want to read it? Maybe you should write a formatting method and get something like

3, 5, 7, -9, 11, 13, -15

where the negatives denotes non-primes (or leave them out or whatever debugging output suits you). In any case you should write a test, so that you don't have to look at the output every time you change anything. You can test some probes like

assertTrue(isPrime(127));
assertFalse(isPrime(129));

and test some small primes in loop (and get the expected value using some trivial method). Finally, you can verify that there are 105_097_565 primes which fits in int.

I wouldn't call the array isPrime as isPrime[0] must be false as 0 is no prime. So let's call it isPrimeIndex.

  Now what really bugging me is the indexing i did in primeSum() method for this optimized sieve

Sure, the 2 needs a special handling and then you should make sure that m and n are odd. I guess that you code is wrong and I'd try something like this instead:

private static int primeSum(int n, int m) {
    if (m < 2) {
        return 0; // no primes below 2
    }
    if (n < 2) {
       n = 2; // no primes below 2
    }
    // Add 2 if in range.
    int sum = n <= 2 && 2 <= m ? 2 : 0;
    // Round up to next odd in a very hacky way.
    n += ~n & 1;
    // Round down to next odd.
    m -= ~m & 1;
    for (int i = numberToIndex(n), p = n; i<=numberToIndex(m); ++i, p+=2) {
        sum += isPrimeIndex[i] ? p : 0;
    }
}

How does my hacky rounding work: If n is odd, then ~n is even and ~n & 1 is 0, so nothing changes. If n is even, then ~n is odd and ~n & 1 is 1, so n gets incremented. You could use something like n += (n % 2) == 0  ? 1 : 0 which is equivalent (but slower) or whatever suits you.

  So should i again include these cases and deal with it separately ? like this

My strategy is to handle special cases upfront (here: deal with the two) and normalize everything ASAP (here: ensure that m and n are odd). This tends to make things simple.


Calculate working days and excluding specific dates (Holidays) I read some questions/answers related to functions in C# to calculate the working days; some use an extended code to achieve that.

I have a data table with more than 50,000 rows and I required an effective method to calculate this information.

I created the following extension in C# that return a number of days (Integer), between two dates; excluding dates (Based on a list of DateTime)

/// <summary> Get working days between two dates (Excluding a list of dates - Holidays) </summary>
/// <param name="dtmCurrent">Current date time</param>
/// <param name="dtmFinishDate">Finish date time</param>
/// <param name="lstExcludedDates">List of dates to exclude (Holidays)</param>
public static int fwGetWorkingDays(this DateTime dtmCurrent, DateTime dtmFinishDate, List<DateTime> lstExcludedDates)
{
    Func<DateTime, bool> workDay = currentDate =>
    (
        currentDate.DayOfWeek == DayOfWeek.Saturday ||
        currentDate.DayOfWeek == DayOfWeek.Sunday ||
        lstExcludedDates.Exists(evalDate => evalDate.Date.Equals(currentDate.Date))
    );

    return Enumerable.Range(0, 1 + (dtmFinishDate - dtmCurrent).Days).Count(intDay => workDay(dtmCurrent.AddDays(intDay)));
}

Does there exists some way to improve this method.

Bug
There is a bug in this method. It counts non-working days because the workDay condition needs to be negated.
return
    Enumerable
        .Range(0, (finishDate - current).Days)
        .Count(day => !isExcludedDate(current.AddDays(day)));

Naming
Then comes the naming. What's with the prefixes fw, dtm, lst? In C# we don't use the hungarian notation. You should use either the camelCase or PascalCase naming where appropriate.

Closures
You could take advantage of closures and simplify the Count
public static int GetWorkingDays(this DateTime current, DateTime finishDateExclusive, List<DateTime> excludedDates)
{
    Func<int, bool> isWorkingDay = days =>
    {
        var currentDate = current.AddDays(days);
        var isNonWorkingDay =
            currentDate.DayOfWeek == DayOfWeek.Saturday ||
            currentDate.DayOfWeek == DayOfWeek.Sunday ||
            excludedDates.Exists(excludedDate => excludedDate.Date.Equals(currentDate.Date));
        return !isNonWorkingDay;
    };

    return Enumerable.Range(0, (finishDateExclusive - current).Days).Count(isWorkingDay);
}

Depending on how long the List<DateTime> excludedDates is, you may consider using a HashSet<DataTime>.
Upper range

1 + (dtmFinishDate - dtmCurrent).Days

Most .NET APIs consider the upper range exclusive thus it might be a good idea to follow this style by removig the +1 and renaming the finishDate to finishDateExclusive.


Merge sort Python implementation Is this efficient?

"""
merge sort algorithm in python
"""

def merge_sort(array):
    if len(array) == 1 or len(array) < 1:  # base case
        return array

    int_mid = len(array) / 2

    left = merge_sort(array[:int_mid])
    right = merge_sort(array[int_mid:])

    return merge_arrays(left, right)

def merge_arrays(left, right):
    left_index = 0
    right_index = 0
    new_array = []
    size = (len(left) + len(right))

    for i in range(size):
        if left[left_index] <= right[right_index]:
            new_array.append(left[left_index])
            left_index += 1
        else:
            new_array.append(right[right_index])
            right_index += 1

        if right_index >= len(right):
            for i in range(left_index, len(left)):
                new_array.append(left[i])
            break
        elif left_index >= len(left):
            for i in range(right_index, len(right)):
                new_array.append(right[i])
            break
    return new_array

if __name__ == '__main__':
    print merge_sort([8, 5, 3, 4, 6])
    print merge_sort([10, 9, 8, 7, 6, 5, 4])
    print merge_sort([1, 2, 3, 4, 5])
    print merge_sort(['b', 'c', 'a'])
    print merge_sort([54, 26, 93, 17, 77, 31, 44, 55, 20])
    print merge_sort([])

No, it's not very efficient...  There are operations you typically want to minimize if you want a performing solution, and memory allocation is one of the top ones.  But your merge_arrays function is creating a new list every time it gets called, which is not a good thing.  An efficient implementation of mergesort will do the sorting in-place (with a copy of the list before if the original has to be kept unchanged), and will reuse the merge buffer for all merging operations.

def mergesort(array, left=0, right=None):
    if right is None:
        right = len(array)
    if right - left <= 1:
        return

    mid = left + (right - left) // 2
    mergesort(array, left, mid)
    mergesort(array, mid, right)
    merge(array, left, mid, right)

def merge(array, left, mid, right):
    buffer = array[left:mid]
    read_left = 0
    read_right = mid
    write = left
    while read_left < len(buffer) and read_right < right:
        if array[read_right] < buffer[read_left]:
            array[write] = array[read_right]
            read_right += 1
        else:
            array[write] = buffer[read_left]
            read_left += 1
        write += 1
    while read_left < len(buffer):
        array[write] = buffer[read_left]
        read_left += 1
        write += 1

Notice how the left part is the only one copied out into the buffer, as that is all that is needed.  Notice also the loop structure, which is substantially different from yours.  It is a matter of taste, but the single while loop for the merging, with an extra loop to copy items that could be left in the buffer (notice that any item left on the right side is already in the right place) seems to be preferred in most production implementations I've seen, and I find it more clear, but YMMV.

The above code is creating a new buffer for every merge operation, which makes the code a little simpler.  A possible way of reusing a buffer is to have it be a static variable of merge.  Static variables are a little funny in Python, but it could look something like this:

def merge(array, left, mid, right):
    left_len = mid - left
    if len(merge.buffer) < left_len:
        merge.buffer.extend([None] * (len(buffer) - left_len))
    for write, read in enumerate(range(left, mid)):
        merge.buffer[write] = array[read]
    read_left = 0
    read_right = mid
    write = left
    while read_left < left_len and read_right < right:
        if array[read_right] < buffer[read_left]:
            array[write] = array[read_right]
            read_right += 1
        else:
            array[write] = buffer[read_left]
            read_left += 1
        write += 1
    while read_left < len(buffer):
        array[write] = buffer[read_left]
        read_left += 1
        write += 1
merge.buffer = []

I wouldn't be surprised though if, this being Python, the explicit looping took longer than the previous version.  But this would be preferred in a lower level language.

Another possible optimization is to scan the left half of the array prior to the copying into the buffer, to avoid moving around items that are already in the right place:

def merge(array, left, mid, right):
    while array[left] < array[mid]:
        left += 1
    ...

and the rest of the code would stay the same.


Count number of square integers in a given range of ints (faster function?) The function below passes most test cases:
function numOfSquares(a, b) {
    const isSquare = function (n) {
        return n > 0 && Math.sqrt(n) % 1 === 0;
    };
    let squareCount = 0;
    for (let i = a; i <= b; i++){
        if (isSquare(i)) {
            squareCount++;
        }
    }
    return squareCount
}

numOfSquares(3, 9) //out: 2
numOfSquares(17, 24) //out: 0
The time complexity of this is O(n)... right?
The problem is that this function does not pass the test cases being provided due to the time complexity. For example, in one case, the input could be numOfSquares(465868129, 988379794) (expected out: 9855). In my case, using the function above, it seems to run... forever.
How can I make this function more efficient, and thus faster?

You are correct that your solution is O(n). However, there is a possible solution for O(1). If you find the root of the first square (first) and the last square (last), then all of the numbers between first and last will yield squares in the range.
function numSquares(min,max) {
    const first = Math.ceil(Math.sqrt(min));
    const last = Math.floor(Math.sqrt(max));
    return last-first+1;
}

console.log(numSquares(465868129, 988379794));


Parse a GET or POST string in C This function takes a string from an HTTP POST or GET string, finds the specified variable, and allocates memory for the result and puts it in a string. The destination is given as an address of an empty pointer.

username=johndoe&password=password123

Would produce:

password123

when finding variable password.

void httpString(char **dest, char *input, const char *find) {
char *start;
char *o_input = input;
const char *o_find = find;
size_t length = 0;
size_t i = 0;
while (*input) {
    if (*input == '&' || input == o_input) {
        if (*input == '&') {
            input++;
            if (*input == 0) {
                return;
            }
        }
        while (*input == *find) {
            if (*input == 0 || *find == 0) {
                return;
            }
            input++;
            find++;
            if (*input == '=' && *find == 0) {
                input++;
                if (*input == 0) {
                    return;
                }
                start = input;
                while (*input != '&' && *input) {
                    input++;
                    length++;
                }
                *dest = malloc(length + 1);
                input = start;
                while (*input != '&' && *input) {
                    (*dest)[i] = *input;
                    input++;
                    i++;
                }
                (*dest)[i] = 0;
                return;
            }
        }
    }
    find = o_find;
    input++;
}
}

Any feedback related to how this function can be improved would be greatly appreciated. I am worried about potential edge cases where a memory access violation could occur.

The easiest way to improve this function is to use the C standard library. As it is, it's difficult to read; you can't understand what it does at a glance. And it's unnecessary difficult, because most of the building blocks are already available for free in <string.h>:

find a string in another string with strstr;
find a character in a string with strchr;
copy a string with strcpy or a substring with memcpy

Once you have simplified your code, you'll have more brain space to care for not so negligeable things such as testing memory allocations:

void httpString(char **dest, char *input, const char *find) {
    char* found = strstr(input, find);
    if (!found) {
        printf("find not found!");
        return;
    }
    char* assign = found + strlen(find);
    if (*assign != '=') {
        printf("ill-formed!");
        return;
    }
    char* value = assign + 1;
    char* end_value = strchr(value, '&');
    if (!end_value) end_value = strchr(value, 0);
    int length = end_value - value;

    *dest = (char*) malloc(length + 1);
    if (!*dest) {
        printf("Not enough memory");
        return;
    }
    memcpy(*dest, value, length);
    (*dest)[length] = 0;
}


Range checking function Here is the problem: Verilog code and testbench for detecting the range of the input given number. If input number is between 12 to 49 then output is '1' else '0'. Range should be easily programmable.
I'm using EDA playground, cadence xcelium 20.09 simulator and EP wave is also present. Is it the appropriate code for the question asked? and also-
Does Verilog read integers normally? for example if I'm inputting a binary number then I should mention the number of bits and then the binary bit like 4b'0011. I directly inputted 12 in the if loop, so it automatically detects that it is an integer?
Here is the link: https://www.edaplayground.com/x/7sQM
Design
module range_detect(y,x);
  input [9:0]x;
  output reg y;
  always @(x)
    begin
      if(x>=12 && x<=49)
        begin
        y=1;
          $display("Input: %d lies between 12 and 49",x);
        end
      else
        begin
        y=0;
          $display("Input: %d does not lie between 12 and 49",x);
        end
    end
endmodule

Testbench
module testbench;
  reg [9:0]in;
  wire out;
  range_detect rd(out,in);
  initial
    begin
      $dumpfile("dump.vcd");
      $dumpvars(1,testbench);
      in=12;
      #2 in=11;
      #2 in=51;
      #2 in=34;
      #2 in=61;
      #2 in=19;
      #2 in=18;
      #2 $stop;
    end
endmodule

Verilog is flexible with how it interprets integers.  It interprets 5 and 4'b0101 as the same numeric value.  Yes, it interprets 12 as an integer in the if statement.
Your code simulates properly on edaplayground, and you have done a good job of separating the design code from the testbench.  Here are some specific recommendations.
If you want your design code to be synthesizable, it would be best to move the $display statements into the testbench.  This requires you to replicate the logic in the testbench as well, but this is necessary when you create a testbench checker anyway.
For debugging purposes, it is helpful to display the simulation time as well: $time.
Also for debugging, it is better to dump signals throughout the hierarchy.  Currently, you just dump the testbench signals.  If you call $dumpvars without arguments, you will also get the design variables dumped to your VCD file.
An alternate approach to driving specific values to your input (12, 11, 51, etc.) is to drive random values in a repeat loop.  This allows you to easily change the number of values you drive and the range.  The code {$random} % 60 below returns a value in the range 0-59.
Your rd instance uses connections-by-position, which is fine for a small number of ports (2), like you have.  But, it is preferable to use connections-by-name to make the code more readable and to help prevent connection errors.  For example: .x (in)
In your design, it is preferable to use ANSI style port declarations to cut down on duplicate port names.
It is preferable to use an implicit sensitivity list always @* because it automatically triggers the block when the appropriate signals change.  This avoids a common Verilog problem where people forget to add or remove signals from the list when they change the code body.
To make the range programmable, one common way to do it is to use parameters.  They can be declared in both the design and the testbench.  For example, you can use MIN and MAX instead of 12 and 49 (all caps is a convention).  It is also helpful to add comments describing the legal values for the parameters.
Here is an alternate way to code everything:
module range_detect (
    input [9:0] x,
    output reg y
);

parameter MIN = 12;
parameter MAX = 49;

always @* begin
    if ((x>=MIN) && (x<=MAX)) begin
        y = 1;
    end else begin
        y = 0;
    end
end
endmodule

module testbench;
    reg [9:0] in;
    wire out;

    // MIN must be smaller than MAX.
    // MAX must be smaller than 1024.
    parameter MIN = 12;
    parameter MAX = 49;

    range_detect #(
        .MIN (MIN),
        .MAX (MAX)
    )
    rd (
        .x  (in),
        .y  (out)
    );

    always @* begin
        if ((in>=MIN) && (in<=MAX)) begin
            $display($time, "  Input: %0d lies between %0d and %0d", in, MIN, MAX);
        end else begin
            $display($time, "  Input: %0d does not lie between %0d and %0d", in, MIN, MAX);
        end
    end

    initial begin
        $dumpfile("dump.vcd");
        $dumpvars;
        repeat (10) #2 in = {$random} % 60;
        #2;
    end
endmodule

You can simply change the parameter values in the testbench and they will propagate into the design.

I use 4-space indentation because I think it is easier to see the different levels of your code's logic.
I add parentheses around terms like (x>=MIN) so that I never have to think about which operators have higher precedence, and it also helps me visually parse the code.


Construct URLs out of filenames of zero-byte images and open them in Firefox for re-downloading It's a problem with FlashGot: it sometimes download zero-byte files. I think it's because I didn't use something like DownThemAll! to limit the number of concurrent downloads, but I wrote this script anyway to solve my problem.

The script basically sets up a RunspacePool and in each of the spawned thread, checks if either of the constructed Danbooru image URL is valid and appends it to a list that becomes the input to Firefox.

I guess the main problem I encountered was a scoping issue. I can not use custom functions inside a script block unless it's defined in there, which is what I did, or its definition is passed as a parameter to the script block and sourced, which is ugly.

I would be interested in reviews that point out any cargo cult programming, performance quirks and/or violation of "best practices". I'm still a beginner so I'd appreciate more if feedback comes with working code rather than hand-waving.

<#
    .SYNOPSIS
      Opens the Danbooru URLs of zero-byte images in Firefox for re-downloading by FlashGot.
#>

#Requires -Version 3.0
#Requires -Modules Microsoft.PowerShell.Utility, Microsoft.PowerShell.Management, CimCmdlets

Set-StrictMode -Version Latest

$PrepareUrl = {

    param (
        [string]$domain,
        [string]$file
    )

    begin {
        function Get-UrlStatusCode([string]$Url) {
            try {
                [int](Invoke-WebRequest -Uri $Url -UseBasicParsing -DisableKeepAlive -Method Head).StatusCode
            }
            catch [Net.WebException] {
                [int]$_.Exception.Response.StatusCode
            }
        }
    }

    process {
        $url = "$domain/data/$file"
        if ((Get-UrlStatusCode -Url $url) -ne 200) {
            $url = "$domain/cached/data/$file"
            if ((Get-UrlStatusCode -Url $url) -ne 200) {
                $url = $null
            }
        }
        Write-Output $url
    }

}

#region Setting Up Jobs
$domain = 'https://danbooru.donmai.us'
$sourceDir = 'C:\fakepath'
Write-Progress -Activity 'Getting the filenames' -Status "Searching under $sourceDir" -SecondsRemaining -1
filter isEmptyImage {
    if ($_.PSIsContainer -eq $false -and $_.Length -eq 0 -and $_.Name -match '__.+\.(jpg|png|gif)') { $_ }
}
$emptyFiles = Get-ChildItem -Path $sourceDir -Recurse | isEmptyImage | Select-Object -ExpandProperty Name
$NumberOfLogicalProcessors = (Get-CimInstance Win32_Processor).NumberOfLogicalProcessors
$rp = [runspacefactory]::CreateRunspacePool(1, 2 * $NumberOfLogicalProcessors)
$rp.Open()
$jobs = New-Object System.Collections.Generic.List[System.Object]
foreach ($file in $emptyFiles) {
    $job = [powershell]::Create().AddScript($PrepareUrl).AddArgument($domain).AddArgument($file)
    $job.RunspacePool = $rp
    $jobs.Add(
        (New-Object PSObject -Property @{
            Job = $job
            Result = $job.BeginInvoke()
        })
    )
}
#endregion
#region Collecting Jobs
$urlList = New-Object System.Collections.Generic.List[System.Object]
$jobCount = $jobs.Count
while ($jobs.Count -gt 0) {
    $WriteProgressParams = @{
        Activity = 'Waiting for jobs to complete'
        Status = "$($jobs.Count) jobs remaining"
        PercentComplete = (($jobCount - $jobs.Count) / $jobCount * 100)
    }
    Write-Progress @WriteProgressParams # splatting
    Start-Sleep -Milliseconds 50
    foreach ($job in $($jobs | Where-Object { $_.Result.IsCompleted -eq $true })) {
        $result = [string]$job.Job.EndInvoke($job.Result)
        if ($result) {
            $urlList.Add($result)
        }
        $job.Job.Dispose()
        [void]$jobs.Remove($job)
    }
}
#endregion
if ($urlList) {
    Start-Process -FilePath 'C:\Program Files\Mozilla Firefox\firefox.exe' -ArgumentList $urlList
}
$rp.Close()
$rp.Dispose()

Set-StrictMode -Off

I don't see anything really wrong with your script. It looks quite functional, and I don't see anything superfluous, though it is very verbose. If it weren't for the fact that I don't see a ; at the end of every line I'd assume you're more accustomed to C# than PowerShell from how structured your script is, not that there's anything wrong with that at all.

All that said, if it were me I think I'd have done a couple of things differently. Nested If statements are hard to read in my opinion. I would prefer to use a Switch statement instead to define cases.

process {
    Switch ($true){
        {(Get-UrlStatusCode -Url "$domain/data/$file") -ne 200} {"$domain/data/$file";continue}
        {(Get-UrlStatusCode -Url "$domain/cached/data/$file") -ne 200} {"$domain/cached/data/$file";continue}
        default {$null}
    }
}

You may note that I output the strings directly in the case scriptblocks; that is because Write-Output is implied, and everything not otherwise directed (such as capturing it in a variable, or using Write-Host or Out-File which redirect the output to the screen or a file respectively) is returned from a function to the pipeline.

For your Get-ChildItem command I would suggest using the -file parameter. That way the FileSystem provider only returns files to PowerShell and you don't have to filter out folder later. This will help speed things up in general, and also simplifies your filter. Kudos on using a filter by the way, almost nobody does it and it's really under appreciated IMHO. Along those same lines I would strongly suggest letting the provider filter files as well using -include, then passing it to your filter. It would look something like this:

filter isEmptyImage {
    if ($_.Length -eq 0 -and $_.Name -match '__.+\.(jpg|png|gif)') { $_ }
}
$emptyFiles = Get-ChildItem -Path $sourceDir -Recurse -Include '*.jpg','*.png','*.gif' -File | isEmptyImage | Select-Object -ExpandProperty Name

This way the File System provider only returns files that end in .jpg, .png, and .gif, and PowerShell has less to sort through.

Lastly, PowerShell is pretty bad about adding and removing things from arrays. It rebuilds its array each time, so I think I would avoid that by capturing the jobs all at once, and then changing the While clause to be a little more intrusive.

$jobs = foreach ($file in $emptyFiles) {
    $job = [powershell]::Create().AddScript($PrepareUrl).AddParameters(@{domain=$domain;file=$file})
    $job.RunspacePool = $rp
    [PSCustomObject]@{
        Job = $job
        Result = $job.BeginInvoke()
    }
}

Or if you want to get fancy you could simply do:

$Jobs = $emptyFiles | %{ [powershell]::Create().AddScript($PrepareUrl).AddParameters(@{domain=$domain;file=$_}) | %{ $_.RunspacePool = $rp;$_ } } | Select *,@{l='Result';e={$_.Job.BeginInvoke()}}

Then we change over to a Do/While loop instead of just a While loop:

$jobsLeft = $jobs | Where-Object { $_.Result.IsCompleted }
$urlList = Do {
    $jobsLeft = $jobs | Where-Object { $_.Result.IsCompleted }
    $WriteProgressParams = @{
        Activity = 'Waiting for jobs to complete'
        Status = "$($jobsLeft.Count) jobs remaining"
        PercentComplete = (($jobs.Count - $jobsLeft.Count) / $jobs.Count * 100)
    }
    Write-Progress @WriteProgressParams # splatting
    Start-Sleep -Milliseconds 50
    foreach ($job in $jobsLeft) {
        [string]$job.Job.EndInvoke($job.Result)|?{$_}
        [void]$job.Job.Dispose()
        [void]$jobs.Remove($job)
    }
} while ($jobsLeft.Count -gt 0)

Those are my suggestions, take'em or leave'em.


Create a pop-up class I'm new to OOP, so I tried to learn from tutorials how to create a class in JavaScript/jQuery. But tutorials don't give you feedback from experienced coders. Is the approach to create a pop-up OK (see below)? I know it works (it's tested), but is it prone to errors or does it have redundancies?

var Popup = function (){
    /// PROPERTIES
    var new_id="div"+Date.now();
    var defaults = {
        id: new_id,
        container: "body",
        height: 200,
        width: 200,
        text: "
 created with \"id\" = "+new_id,
        x: 100,
        y: 100,
        position: "absolute",
        bgcolor: "red"
    };
    this.settings=defaults;
    /// METHODS
    this.create=function (options){
        $.extend(this.settings,options);
        var html= "
"+
                this.settings.text+
                "
";
        $(this.settings.container).append(html);
        this.update();
    };
    this.update=function (options) {
        $.extend(this.settings,options);
        $("#"+this.settings.id).css({
            "height":this.settings.height+"px",
            "width":this.settings.width+"px",
            "position":this.settings.position,
            "top":this.settings.y,
            "left":this.settings.x,
            "background-color":this.settings.bgcolor
        });
    };
};

/* Popup use:

var pop=new Popup(); // new popup is declared
pop.create({bgcolor:"green"}); // popup appears green instead of read
pop.update({bgcolor:"blue"}); // popup changes to blue

*/

(I know there are a tons of Popup plugins that I could use, but that's irrelevant for my learning goals)

For example, I've realized that this line will cause problems:

this.settings=defaults;

because when "settings" are changed, "defaults" change too (!). Thus, the better alternative is:

this.settings=$.extend(defaults);

or a JavaScript "clone" function like this.

Most of your code looks good
The one improvement is do not overwrite the initial settings, instead extend from defaults and the options provided. Use $.extend for this

 popup.settings = $.extend(defaults, options);

Also store the reference of this into a variable so that it will be used inside event handlers and anonymous callbacks.

var Popup = function() {
    var popup = this,
        new_id = "div" + Date.now(),
        defaults = {
            id: new_id,
            container: "body",
            height: 200,
            width: 200,
            text: "
 created with \"id\" = " + new_id,
            x: 100,
            y: 100,
            position: "absolute",
            bgcolor: "red"
        };

    popup.create = function(options) {
        popup.settings = $.extend(defaults, options);
        var html = "
" +
            popup.settings.text +
            "
";
        $(popup.settings.container).append(html);
        popup.update();
    };

    popup.update = function(options) {
        popup.settings = $.extend(defaults, options);
        $("#" + popup.settings.id).css({
            "height": popup.settings.height + "px",
            "width": popup.settings.width + "px",
            "position": popup.settings.position,
            "top": popup.settings.y,
            "left": popup.settings.x,
            "background-color": popup.settings.bgcolor
        });
    };
};

var pop = new Popup(); // new popup is declared

pop.create({
    bgcolor: "green"
}); // popup appears green instead of read

pop.update({
    bgcolor: "blue"
}); // popup changes to blue


String Calculator kata in JavaScript using TDD I'm looking for some advice in TDD for my implementation of the String Calculator kata described by Roy Osherov. Any advice regarding refactoring, structuring and polishing of the code is welcome. Still a novice at JS, so please go easy on me.

I have implemented the first 5 rules excluding the delimiter rule.
I intend to do this kata everyday as a practice to get better.

The rules for the kata can be found here :
http://osherove.com/tdd-kata-1/

Tests :

// StringCalculator-spec.js
describe("String Calculator",  function(){
    var calculator;

    beforeEach(function() {
      calculator = new StringCalculator();
    });

    it("should return zero when '' is passed", function(){
      expect(calculator.add('')).toEqual(0);
    });

    it("should return the number itself when a single number is passed", function(){
      expect(calculator.add('1')).toEqual(1);
    });

    it("should return the sum of the numbers if two are given", function(){
      expect(calculator.add('1,2')).toEqual(3);
    });

    it("should return the sum of an unknown amount of numbers", function(){
      // creates a random array
      randomArray = (length, max) => [...new Array(length)]
      .map(() => Math.round(Math.random() * max));
      // creates a random number between 1 and 100
      randy = Math.floor((Math.random() * 100) + 1);
      res = randomArray(randy,randy);
      // sums up an array
      sum = res.reduce((pv, cv) => pv+cv, 0);
      // converts the array to a string
      arg = res.join();
      expect(calculator.add(arg)).toEqual(sum);
    });

    it("should allow \\n in between the input number string", function(){
      expect(calculator.add("1\n2,3")).toEqual(6);
    });

    it("should not allow negative numbers", function(){
      expect( function(){ calculator.add("-1,2,3"); } ).toThrow(new Error("negatives not allowed"));
    });
  });

Code :

// StringCalculator.js
function StringCalculator(){
    }
    StringCalculator.prototype.add = function(string_numbers) {
      this.number = string_numbers;
      if (this.number.includes('-')) {
        throw new Error('negatives not allowed');
      }
      // replaces \n with ,
      numbers = this.number.replace(/(\r\n|\n|\r)/gm,",");
      if (numbers == '') {
        return 0;
      } else if(numbers.length === 1) {
        return (parseInt(numbers));
      } else {
        res = numbers.split(',');
        var total = 0;
        var arrayLength = res.length;
          for (var i = 0; i < res.length; i++) {
            total = total + parseInt(res[i]);
          }
          return total;
      }
    };

beforeEach(function() {
  calculator = new StringCalculator();
});

I usually recommend not using things like beforeEach (and similarly afterEach). The problem is that your tests are now implicitly dependent on an operation that's not referenced or even called anywhere in the test body. It's not immediately clear that a calculator was defined for each test. It's also not clear if it's a per test instance or a shared instance.

Tests should be independent and self-contained. The general rule I follow  is I should be able to cut and paste a test to any part of the suite and still have it work, or remove it without affecting other tests. You can have shared utility functions that are called explicitly in the test body so that it's clear to the reader what happened, where something was obtained, etc.

function StringCalculator(){...}
StringCalculator.prototype.add = function(string_numbers) {...}

Although the rules mention "a method" which immediately would imply an instance and therefore classes, you don't necessarily need one. You'll only need a function that takes an input and gives an output. Keep it simple.

it("should return the sum of an unknown amount of numbers", function(){
  // creates a random array
  randomArray = (length, max) => [...new Array(length)]
  .map(() => Math.round(Math.random() * max));
  // creates a random number between 1 and 100
  randy = Math.floor((Math.random() * 100) + 1);
  res = randomArray(randy,randy);
  // sums up an array
  sum = res.reduce((pv, cv) => pv+cv, 0);
  // converts the array to a string
  arg = res.join();
  expect(calculator.add(arg)).toEqual(sum);
});

Never forget to use var, let or const if you don't want these variables to be defined in the global space (or at least the top level of the module). I usually recommend const whenever possible to ensure nothing has manipulated the variable since creation and let if you need to mutate it at some point (i.e. counters). Both are also block-scoped, eliminating guesswork and hoisting.

    var total = 0;
    var arrayLength = res.length;
      for (var i = 0; i < res.length; i++) {
        total = total + parseInt(res[i]);
      }
      return total;

You can use array.reduce to accumulate values. It iterates through an array and uses the callback return value as first argument of the next call. An initial value can be supplied, otherwise iteration starts at the second value with first value as initial value.

Also, always check for NaN when converting strings to numbers. NaN passes through calculations, NaNing everything along the way. It's annoying to track the source of NaN and it may be very far from where it's spotted. Guarding near wherever it potentially comes up is a good practice.

Updating your logic:

function add(stringNumbers){
  const numbers = stringNumbers
    .replace(/(\r\n|\n|\r)/gm, ',') // Normalize delimiter
    .split(',')                     // Split values
    .map(n => parseInt(n, 10))      // Convert to integers

  // Check if numbers are ok. There's probably better APIs
  // to check if something in the array is invalid.
  if(numbers.some(n => Number.isNan(n))) throw new Error('Not a number')
  if(numbers.some(n => n < 0)) throw new Error('Negatives not allowed')

  // Add them all up
  return numbers.reduce((s, n) => s + n)

}