x64 Assembly - checking for largest prime factor Using x64 assembly, I solved the Largest Prime Factor problem on Project Euler.

The problem is as follows:

  The prime factors of 13195 are 5, 7, 13 and 29.

  What is the largest prime factor of the number 600851475143 ?

The answer is 6857.

I think I have a top notch algorithm to do the job (speed wise), which doesn't actually check if a number is prime. The algorithm just reduces the number they gave you instead.

p3 proc

    mov rbx, 600851475143          ; The original number
    mov rcx, 1                     ; The starting position of the counter
    mov rdx, 0                     ; Just to clear garbage data

    mainloop:

        mov rax, rbx           ; rbx hold the number, and saves it, because the number in rax will change, and a backup needs to be stored

        inc rcx                ; Increment counter
        noinc:                 ; This is where the loop starts at each iteration that the previous division operation had no remainder
        mov rbx, rax           ; At this instruction, we know that rax has been reduced, and save it into rbx

        cmp rax, rcx           ; These two lines check if the counter rcx reached its maximum value, and if so, the program quits and returns the result in rax
            je done
        mov rdx, 0             ; makes sure there is no junk data in rdx

        div rcx                ; Perform division to check for a remainder
        cmp rdx, 0
            jz noinc           ; If there is no remainder: Reduce the value and dont increment next iteration
            jnz mainloop       ; If there is a remainder, jump back to the top of the loop, increment the value, and try the division again.

    done:                      ; Exit
        ret
p3 endp

I am a beginner in assembly, and this is one of my first x64 assembly programs. Are there any standard conventions that I am not following, or general things that could have been done better? Any advice? Is there a simpler way to do what I did?

Please be brutal and detailed in your response.

One optimisation that would make your code run up to twice as fast, is to use the observation that a prime factor of n cannot be greater than n/2. So you can limit the number of times you iterate through your loop.

Additionally, you know that once you have tested for divisibility by 2, then subsequent trial divisions can skip all even numbers. If you continue down this road, you'll eventually implement a prime sieve using all the same tricks.

One thing I would suggest is to use jmp instead of jnz at the bottom of the loop. It might or might not make a performance difference, but you'll avoid the CPU (and your readers) from having to think about the condition.

It probably doesn't matter, but you could move the mov rdx, 0 up above the noinc label because whenever you jz noinc, you already know that rdx is zero. This sort of thing really needs to be clearly documented, of course.


Binary Search given sorted array Unit test I have implemented Binary search code for a given sorted array, written unit test too, Please review my BST unit test code and let me know your comments for improvements.

class BinarySearch {
    public int binSearch(int[] sortedArray, int key) {
        return search(0, sortedArray.length - 1, key, sortedArray);
    }

    private static int search(int start, int end, int key, int[] sortedArray) {
        int mid = start + ((end-start)/2);

        if (mid >= start && mid <= end) {
            if (sortedArray[mid] == key) {
                return mid;
            } else if (sortedArray[mid] > key) {
                return search(start, mid-1, key, sortedArray);
            } else if(sortedArray[mid] < key){
                return search(mid+1, end, key, sortedArray);
            }
        }

        return -1;
    }
}

public class BinarySearchTest {

    private BinarySearch binarySearchCUT;
    private static final int UNSUCCESSFUL = -1;

    @Before
    public void setUp(){
        binarySearchCUT = new BinarySearch();
    }

    @Test
    public void testShouldReturnUnsuccessfulOnEmptyArray() {
        assertEquals(UNSUCCESSFUL, binarySearchCUT.binSearch(new int[]{}, 0));
    }

    @Test
    public void testShouldReturnUnsuccessfulOnLeftBound() {
        assertEquals(UNSUCCESSFUL, binarySearchCUT.binSearch(new int[]{1, 2, 4, 7, 8, 12, 15, 19, 24, 50, 69, 80, 100}, 0));
    }

    @Test
    public void testShouldReturnUnsuccessfulOnRightBound() {
        assertEquals(UNSUCCESSFUL, binarySearchCUT.binSearch(new int[]{1, 2, 4, 7, 8, 12, 15, 19, 24, 50, 69, 80, 100}, 101));
    }

    @Test
    public void testShouldReturnSuccessfulOnLeftBound() {
        assertEquals(0, binarySearchCUT.binSearch(new int[]{1, 2, 4, 7, 8, 12, 15, 19, 24, 50, 69, 80, 100}, 1));
    }

    @Test
    public void testShouldReturnSuccessfulOnRightBound() {
        assertEquals(12, binarySearchCUT.binSearch(new int[]{1, 2, 4, 7, 8, 12, 15, 19, 24, 50, 69, 80, 100}, 100));
    }

    @Test
    public void testShouldReturnSuccessfulOnMid() {
        assertEquals(7, binarySearchCUT.binSearch(new int[]{1, 2, 4, 7, 8, 12, 15, 19, 24, 50, 69, 80, 100}, 19));
    }

    @Test
    public void testShouldReturnSuccessfulOnMidGreaterThanGivenNumber() {
        assertEquals(5, binarySearchCUT.binSearch(new int[]{1, 2, 4, 7, 8, 12, 15, 19, 24, 50, 69, 80, 100}, 12));
    }

    @Test
    public void testShouldReturnSuccessfulOnMidLesserThanGivenNumber() {
        assertEquals(10, binarySearchCUT.binSearch(new int[]{1, 2, 4, 7, 8, 12, 15, 19, 24, 50, 69, 80, 100}, 69));
    }

}

BinarySearch

The BinarySearch class should be public since it contains utility methods that are generally useful. To do this, write public class BinarySearch instead of class BinarySearch.

The binSearch method should be static since it does not access any fields from the BinarySearch class. After all, that class doesn't have any fields that could be accessed. To do this, write public static int binSearch instead of public int binSearch.

When you make the binSearch method static, you no longer need the new BinarySearch() in the unit test. This is good since it sounds weird to have a "new binary search". It's confusing to hear this, since "binary search" is an algorithm and not a countable thing. It makes more sense to speak of "the binary search algorithm", avoiding the word "new". On the other hand, "new house" or "new sheet of paper" sounds much more natural.

The helper method currently is:

private static int search(int start, int end, int key, int[] sortedArray) {

The order of the parameters is not the usual one. Usually the array comes first, before its indexes. In this case, this would mean:

private static int search(int[] sortedArray, int start, int end, int key) {

When you compare your code with the predefined Arrays.binarySearch and the helper function, you can see that the code is quite similar, which is a good sign.

One useful thing that the predefined Arrays.binarySearch does is that if the element is not found, it returns a hint about the index where it would be found. This is useful to see the closest existing value.

BinarySearchTest

Your existing unit tests look good. You can leave out the word test at the beginning of the method names to make the names a little shorter. They are quite long, but that's ok.

There are test cases missing:

An element from the middle cannot be found, for example 5 in [1, 2, 6, 10].
The array contains duplicate values, for example 3 in [1, 2, 3, 3, 3, 3, 3, 3, 3].

In the latter case, it is unspecified whether binSearch should return 2 or 3 or any other index. All you can do in your unit test is to check that `array[binSearch(array, key)] == key.


Simple authentication and registration of users I have created a simple authentication script in python that can add users and
provides a login function. I've tried to thow as much security into this as possible, which probably wasn't the best idea.
To my knowledge, the code works fine and withstands as much invalid input as I can find (although I haven't written any proper tests). I can see straight away that I've repeated myself quite a lot, but I'm not entirely sure how to cut it down.
Usage:

login(username, password)
Checks if this username/password combination is valid.
Returns bool
add_user(username, password)
Adds a user to the password file.
Returns bool

import hashlib, random, string

USERNAME_CHARACTERS=string.digits+string.ascii_letters
PASSWORD_CHARACTERS=string.digits+string.ascii_letters+string.punctuation

SALT_CHARACTERS="0123456789abcdefghijklmnopqrstuv"
SALT_CHARACTER_BITS=5
SALT_LENGTH=16
PEPPER_LENGTH=1

FILE_PATH="./shad"
FILE_SEPERATOR=":"

def int_to_string(num):
    string=""
    while num!=0:
        c=num%(2**SALT_CHARACTER_BITS)
        string+=SALT_CHARACTERS[c]
        num-=c
        num//=2**SALT_CHARACTER_BITS
    return string

def generate_new_salt():
     return int_to_string(random.getrandbits(SALT_CHARACTER_BITS*SALT_LENGTH))
def generate_new_pepper():
    return int_to_string(random.getrandbits(SALT_CHARACTER_BITS*PEPPER_LENGTH))
def generate_all_peppers():
    for i in range(1,SALT_CHARACTER_BITS*PEPPER_LENGTH):
        yield int_to_string(i)

def do_hash(string):
    return hashlib.sha3_256(string.encode()).hexdigest()

def get_user_data(username):
    with open(FILE_PATH) as h:
        data=h.read()
    for line in data.split("\n"):
        l=line.split(FILE_SEPERATOR)
        if l[0]==username:
            return {"user":l[0],
                    "pass":l[1],
                    "salt":l[2]}
    raise ValueError("Username not found: "+username)

def is_valid_username(username):
    for i in username:
        if not(i in USERNAME_CHARACTERS):
            return False
    return True

def is_valid_password(username):
    for i in username:
        if not(i in PASSWORD_CHARACTERS):
            return False
    return True

def add_user(username, password):
    if not(is_valid_username(username)):
        print("Invalid username")
        return False
    elif not(is_valid_password(password)):
        print("Invalid password")
        return False
    with open(FILE_PATH) as h:
        data=h.read()
    for line in data.split("\n"):
        l=line.split(FILE_SEPERATOR)
        if l[0]==username:
            print("Username already exists")
            return False

    salt=generate_new_salt()
    data=FILE_SEPERATOR.join([username, do_hash(generate_new_pepper()+salt+password), salt])+"\n"
    with open(FILE_PATH, "a") as h:
        h.write(data)
    return True

def login(username, password):
    if not(is_valid_username(username)):
        print("Invalid username")
        return False
    elif not(is_valid_password(password)):
        print("Invalid password")
        return False
    data=get_user_data(username)
    for pepper in generate_all_peppers():
        if do_hash(pepper+data["salt"]+password)==data["pass"]:
            return True
    return False

Cryptography

As others have mentioned, I would use bcrypt

Format

You have used good function names and variable names. However, you can add doc strings for each function.

def add_user(username, password):
    """"Function to add a user. Returns True for successful, or False for failure"""
    #Function code here

Additionally, you could improve your compliance with PEP8 (the Python Style Guide) by adding white-space around your equals symbols. This would greatly improve the readability of your code. Also, a more relaxed vertical spacing would improve the look of your code.

__main__

Not to sure if you want to create an default function for your code. But you could add (if __name__ == "__main__":)


An attempt at a toy Vm I usually use c++ so it may not be best practice for c.
This is a stack based toy vm and as a result it is very primitive, and it has no bitwise
instructions
64k might be a bit overkill for a toy vm.
coding is hard
#include <stdio.h>
#include "vm.h"

int main() {
    Vm *vm = new_vm();
    i32 buffer[] = {
        0x00000A01, /* push 0x0A(\n) */
        0x00004301, /* push 0x43(C)  */
        0x00004201, /* push 0x42(B)  */
        0x00004101, /* push 0x41(A)  */
        0x00000009, /* output */
        0x00000002, /* pop */
        0x00000009,
        0x00000002,
        0x00000009,
        0x00000002,
        0x00000009,
        0x00000000  /* halt */
    };
    for (int i = 0; i < sizeof(buffer); i++) {
        vm->mem[vm->pc+i] = buffer[i];
    }
    run_vm(vm);
    free_vm(vm);
    return 0;
}

vm.h
#ifndef VM_H_
#define VM_H_

#include <stdlib.h>
#include <stdint.h>
#include <stdbool.h>

typedef uint32_t i32; /* other numbers */
typedef int32_t si32; /* stack pointer */
typedef unsigned char byte;

typedef struct {
    i32 mem[0xffff]; /* approx. 64k */
    si32 sp;
    i32 pc;

    i32 raw;
    byte opc;
    i32 param;

    bool running;
} Vm;

Vm *new_vm();
void reset_vm(Vm *vm);
void free_vm(Vm *vm);
void run_vm(Vm *vm);

#endif

vm.c
#include <stdio.h>
#include "vm.h"

Vm *new_vm() {
    Vm *ret = (Vm*)malloc(sizeof(Vm));
    ret->pc = 1024; /* add space for the stack */
    ret->sp = -1;
    ret->running = true;
    return ret;
}

void reset_vm(Vm *vm) {
    vm->running = true;
    for (int i = 0; i < 0xffff; i++) {
        vm->mem[i] = 0;
    }
    vm->sp = -1;
    vm->pc = 1024;
}
void free_vm(Vm *vm) {
    free(vm);
    vm = NULL;
}

static void fetch(Vm *vm) {
    vm->raw = vm->mem[vm->pc++];
}

static void decode(Vm *vm) {
    /* style of opcode
     * 24 bits for parameter
     * a byte for the opcode
    */
    vm->opc = vm->raw & 0xff;
    vm->param = (vm->raw & 0xffffff00) >> 8;
}

static void execute(Vm *vm) {
    switch(vm->opc) {
        case 0x00: /* halt */
            vm->running = false;
            printf("Halt\n");
        break;
        case 0x01: /* push */
            vm->mem[++vm->sp] = vm->param;
        break;
        case 0x02: /* pop */
            vm->mem[vm->sp--] = 0;
        break;
        case 0x03: /* store */
            vm->mem[ vm->mem[vm->sp - 1] ] = vm->mem[vm->sp];
        break;
        case 0x04: /* load */
            vm->mem[vm->sp + 1] = vm->mem[ vm->mem[vm->sp] ];
            ++vm->sp;
        break;
        case 0x05: /* add */
            vm->mem[vm->sp + 1] = vm->mem[vm->sp] + vm->mem[vm->sp - 1];
            ++vm->sp;
        break;
        case 0x06: /* sub */
            vm->mem[vm->sp + 1] = vm->mem[vm->sp - 1] - vm->mem[vm->sp];
            ++vm->sp;
        break;
        case 0x07: /* mul */
            vm->mem[vm->sp + 1] = vm->mem[vm->sp] * vm->mem[vm->sp - 1];
            ++vm->sp;
        break;
        case 0x08: /* div */
            vm->mem[vm->sp + 1] = vm->mem[vm->sp - 1] / vm->mem[vm->sp];
            ++vm->sp;
        break;
        case 0x09: /* outc */
            printf("%c", vm->mem[vm->sp]);
        break;
        case 0x0A: /* inpc */
            vm->mem[++vm->sp] = getchar();
        break;
    }
}

void run_vm(Vm *vm) {
    while(vm->running) {
        fetch(vm);
        decode(vm);
        execute(vm);
    }
}

Missing Error Checking

I usually use c++ so it may not be best practice for c.

In C++ when memory allocation fails in new an exception is thrown, this is not the case in the C programming language when using malloc(), calloc() or realloc(). An additional check is required after any memory allocation call. The check is to see if the memory returned is NULL or not, if the allocation fails references through the pointer are Unknown Behavior.
Vm *new_vm() {
    Vm *ret = (Vm*)malloc(sizeof(Vm));
    if (!ret)
    {
        fprintf(stderr, "Allocation of the Virtual Machine failed.\n");
        return ret;
    }

    ret->pc = 1024; /* add space for the stack */
    ret->sp = -1;
    ret->running = true;
    return ret;
}

In main():
#include <stdio.h>
#include <stdlib.h>
#include "vm.h"

int main() {
    Vm *vm = new_vm();
    if (vm == NULL)
    {
        fprintf(stderr, "Exiting Toy Virtual Machine.\n");
        return EXIT_FAILURE;
    }
    i32 buffer[] = {
            0x00000A01, /* push 0x0A(\n) */
            0x00004301, /* push 0x43(C)  */
            0x00004201, /* push 0x42(B)  */
            0x00004101, /* push 0x41(A)  */
            0x00000009, /* output */
            0x00000002, /* pop */
            0x00000009,
            0x00000002,
            0x00000009,
            0x00000002,
            0x00000009,
            0x00000000  /* halt */
    };
    for (int i = 0; i < sizeof(buffer); i++) {
        vm->mem[vm->pc+i] = buffer[i];
    }
    run_vm(vm);
    free_vm(vm);
    return EXIT_SUCCESS;
}

Include Only Necessary Headers
In the code as posted, stdlib.h is included in vm.h, stdlib.h is not necessary in main(), only in vm.c. To reduce the scope of the header files and source files only include what is needed. Amoung other things, this will reduce compile times, and may also reduce linking problems.
Missing Default Case in Switch Statement
It is generally a good programming practice to have a default : case statement in a switch statement to handle cases that haven't been specified yet:
static void execute(Vm *vm) {
    switch(vm->opc) {
        default:
            fprintf(stderr, "Unknown Opcode in execute(). 0x%x\n", vm->opc);
            return;
        case 0x00: /* halt */
            vm->running = false;
            printf("Halt\n");
            break;
        ...
    }

Then all possible paths through the function have been implemented. This is true in either C or C++ and most other programming languages that have a switch statement.
Use an ENUM for the Opcodes
The code would be much more readable if less numeric constants and more symbolic constants were used. In C there are 2 ways to do this, to create single symbolic constants use macro definition
#define SYMBOL  VALUE

or to use enums
typedef enum {
    HALT = 0x00,
    PUSH = 0x01,
    POP = 0x02,
    ...
    INPUTCHAR = 0x0A
} OPCODE;

typedef struct {
    i32 mem[0xffff]; /* approx. 64k */
    si32 sp;
    i32 pc;

    i32 raw;
    OPCODE opc;
    i32 param;

    bool running;
} Vm;

Use Unsigned Types as Indexes
The stack pointer index is currently a signed integer and is initialized to -1, this is what I consider to be a bad practice since stack[-1] will cause Unknown Behavior. It would be better to use size_t or unsigned as the stack pointer index. This will force a change in a number of areas, but here is what I would recommend:

Initialize running to false rather than true.
Initialize sp to zero
Only index the stack and increment the stack pointer if running is true
At the beginning of run_vm(Vm *vm) before the loop set running to true
Change the implementation of reset_vm(Vm *vm) to match all of the above

Type Names and Variable Names
Initially I was confused about Vm, whether it was a Virtual Memory or a Virtual Machine, this was true of other variables and types as well. Well written code is self documenting and doesn't need a lot of comments, type names and variable names play a big part in this. In my opinion Vm should be renamed VirtualMachine, sp should be renamed StackPointer, pc should be renamed ProgramCounter etc.
Use Library Functions Where Available
C++ contains std::memset(), and memset() in C predates C++. The function reset_vm() should use memset() rather than the loop it is using to reset the memory. The function memset() should be faster than the current loop.
void reset_vm(Vm *vm) {
    vm->running = true;
    memset(&vm->mem[0], 0, sizeof(*vm->mem[0]) * 0xffff);
    vm->sp = -1;
    vm->pc = 1024;
}


Batch upload users to nextcloud on docker with bash csv I have this bash script that adds users to NextCloud from a csv file. It expects to be run from the same directory as the docker-compose.yml file.

I'm pretty new to bash scripting and I'm wondering if I am doing things correctly or if there are pieces that could be improved in terms of efficiency, correctness, style? Any comments are appreciated really.

Thanks in advance!

#!/bin/bash

# Handle printing errors
die () {
  printf '%s\n' "$1" >&2
  exit 1
}

usage () {
  echo ""
  echo "batch_users.sh"
  echo "SYNOPSIS"
  echo "batch_users [-p] [file]"
  echo "DESCRIPTION"
  echo "The batch_users script adds a batch of users to an instance of NextCloud running inside of a docker container by reading a list from a csv file."
  echo ""
  echo "-p, --password    Set users password. If no option is passed, the default password is nomoremonkeysjumpingonthebed ."
  echo ""
  echo "csv file should be formatted in one of the following configurations."
  echo "username,Display Name,group,email@address.domain,"
  echo "username,Display Name,group,"
  echo "username,Display Name,"
  echo "username,"
  echo ""
  echo "EXAMPLES"
  echo "The command:"
  echo "batch_users.sh -p 123password321 foobar.csv"
  echo "will add the users from foobar.csv and assign them the password 123password321"
  echo "The command:"
  echo "batch_users.sh foobar.csv"
  echo "will add the users from foobar.csv and assign them the default password."
  echo ""
  echo "batch_users will return 0 on success and a positive number on failure."
  echo ""
}
# flags
password=nomoremonkeysjumpingonthebed
while :; do
  case $1 in
    -h|-\?|--help)
      usage # Display a usage synopsis.
      exit
      ;;
    -p|--password)
      if [ "$2" ]; then
        password=$2
        shift
      else
        die 'Error: "--password" requires a non-empty option argument.'
      fi
      ;;
    --password=?*)
      password=${1#*=} # Delete everything up to = and assign the remainder.
      ;;
    --password=) # Handle the case of empty --password=
      die 'Error: "--password" requires a non-empty option argument.'
      ;;
    --)
      shift
      break
      ;;
    -?*)
      printf 'WARN: Unknown option (ignored): %s\n' "$1" >&2
      ;;
    *) # Default case. No more options, so break out of the loop
      break
  esac
  shift
done

# Check to see if there was at least one argument passed.
# If not, print error and exit.
if [[ $# -eq  0 ]]
then
  die 'Error: Expected at least one argument, but no arguments were supplied.'
fi

# Check to see if the file passed in exists.
# If not, print error and exit.
if [[ ! -e $1 ]]
then
  die "Couldn't find file ${1}."
  exit 1
fi

input_file="$1"

while IFS=, read -r f1 f2 f3 f4
do
  # check --password flag
  # f1, f2, f3 exist?
  if [[ -n $f1  && -n $f2 && -n $f3 ]]
  then

    sh -c "docker-compose exec -T --env OC_PASS=${password} --user www-data app php occ \
      user:add --password-from-env --display-name=\"${f2}\" --group=\"${f3}\" \"$f1\" " < /dev/null

  elif [[ -n $f1 && -n $f2 ]]
  then
    # f1 and f2

    sh -c "docker-compose exec -T --env OC_PASS=${password} --user www-data app php occ \
      user:add --password-from-env --display-name=\"${f2}\" \"$f1\" " < /dev/null

  elif [[ -n $f1 ]]
  then
    #only f1

    sh -c "docker-compose exec -T --env OC_PASS=${password} --user www-data app php occ \
      user:add --password-from-env \"$f1\" " < /dev/null

  else
    #error
    die "Expected at least one field, but none were supplied."
  fi

  # If there is a fourth value in the csv, use it to set the user email.
  if [[ -z ${f4+x} ]]
  then
    break
  else
    sh -c "docker-compose exec -T --user www-data app php occ\
      user:setting \"$f1\" settings email \"${f4}\" " < /dev/null
  fi
done <"$input_file"
exit 0

Review
This code looks pretty good.  I know very little about the docker-compose command being executed, but I can look at the script style.
Shellcheck spots no issues, which is always a good start.
I see nothing in here that really demands Bash rather than standard (POSIX) shell - all those [[ can be easily converted to [.
I recommend setting -u and probably -e.  In any case, consider what the effects of any individual command failing should be.  Within the loop, I recommend continuing to the next iteration, but remember whether any iteration failed.  One way I do that is
status=true   # until a failure
for i in ....
do
    some_command  || status=false
done

# terminate with appropriate exit code
exec $status

Instead of using many echo commands in usage(), it's easier to copy a single here-document:
usage() {
    cat <<'EOF'
batch_users.sh
SYNOPSIS
...
EOF
}

if [[ ! -e $1 ]]
then
  die "Couldn't find file ${1}."
  exit 1
fi

Braces not required here for $1, and exit 1 is unreachable.  Mere existence of the file is insufficient: we need it to be readable.  Here's my version:
[ -r "$1" ] || die "Couldn't find file $1."

We need to be very careful when composing strings to be interpreted by other commands, especially shells:

    sh -c "docker-compose exec -T --env OC_PASS=${password} --user www-data app php occ \
      user:add --password-from-env --display-name=\"${f2}\" --group=\"${f3}\" \"$f1\" " < /dev/null

$password in particular could contain almost anything (including quote characters), so that's really not safe in the inner sh instance.  Do we really need that shell, or can we simply invoke docker-compose directly?  The latter is much easier to get right.  If we really need the sh, then we'll need to be a lot more careful about constructing that command (I think we need to use a printf that supports %q conversion).
We can avoid the repetition by using ${var+} to conditionally include the optional arguments:
if [ "$f1" ]
then
    docker-compose exec -T --env OC_PASS="$password" \
                   --user www-data app php occ \
                   user:add --password-from-env \
                   ${f2:+"--display-name=$f2"} \
                   ${f3:+"--group=$f3"} \
                   "$f1" \
                   </dev/null \
         || status=false
else
    #error
    echo "Expected at least one field, but none were supplied." >&2
    status=false
    continue
fi

I think we have an unnecessary password exposure,. --password-from-env allows passing the password in environment, so that it doesn't appear in command-line arguments.  But we've immediately lost that advantage when we wrote --env OC_PASS="${password}".  I think the correct thing to do is to put the password into environment before the loop:
OC_PASS=$password
export OC_PASS

And then specify to copy it without giving a new value: --env OC_PASS.  We could also ditch $password and assign directly to OC_PASS.  It would be worth not setting a default, so that users can use the same environment variable to avoid exposing the password using when invoking the script.

Modified code
#!/bin/sh

set -eu

# Handle printing errors
die () {
    printf '%s\n' "$1" >&2
    exit 1
}

usage () {
    cat <<'EOF'
batch_users.sh
SYNOPSIS
batch_users [-p] [file]
DESCRIPTION
The batch_users script adds a batch of users to an instance of NextCloud
running inside of a docker container by reading a list from a csv file.

-p, --password    Set user password. If no option is passed, the password
                  should be passed in the OC_PASS environment variable.

csv file should be formatted in one of the following configurations:
username,Display Name,group,email@address.domain,
username,Display Name,group,
username,Display Name,
username,

EXAMPLES
The command:
    batch_users.sh -p 123password321 foobar.csv
will add the users from foobar.csv and assign them the password 123password321
The command:
    batch_users.sh foobar.csv
will add the users from foobar.csv and assign them the default password.

batch_users will return 0 on success and a positive number on failure.
EOF
}

# flags
while true
do
    case $1 in
        -h|-\?|--help)
            usage # Display a usage synopsis.
            exit
            ;;
        -p|--password)
            if [ "$2" ]; then
                OC_PASS=$2
                export OC_PASS
                shift
            else
                die 'Error: "--password" requires a non-empty option argument.'
            fi
            ;;
        --password=?*)
            OC_PASS=${1#*=} # Delete everything up to = and assign the remainder.
            export OC_PASS
            ;;
        --password=) # Handle the case of empty --password=
            die 'Error: "--password" requires a non-empty option argument.'
            ;;
        --)
            shift
            break
            ;;
        -?*)
            printf 'WARN: Unknown option (ignored): %s\n' "$1" >&2
            ;;
        *) # Default case. No more options, so break out of the loop
            break
    esac
    shift
done

# Was exactly one filename argument passed?
[ $# -eq 1 ] || die "$0: Expected a filename argument"

# Is the file readable?
[ -r "$1" ] || die "$1: missing or not readable"

[ "$OC_PASS" ] || die "$0: No password specified. Run with --help for more info."

status=true                     # until a command fails

while IFS=, read -r f1 f2 f3 f4
do
    if [ "$f1" ]
    then
        docker-compose exec -T --user www-data --env OC_PASS \
                       app php occ \
                       user:add --password-from-env \
                       ${f2:+"--display-name=$f2"} \
                       ${f3:+"--group=$f3"} \
                       "$f1" \
                       </dev/null \
            || status=false
    else
        echo "Expected at least one field, but none were supplied." >&2
        status=false
        continue
    fi

    # If there is a fourth value in the csv, use it to set the user email.
    if [ "$f4" ]
    then
        docker-compose exec -T \
                       --user www-data app php occ \
                       user:setting "$f1" settings email "$f4" \
                       </dev/null \
            || status=false
    fi
done <"$1"

exec $status

Further suggestions

Perhaps it would be useful to add a --verbose option that shows what is to be done for each input line.
Does the input really need to come from a file?  It would be useful to be able to provide it on standard input (so we could filter a list using grep, for example).


Predefined values for some values in a template function like with a #define macro (e.g. template) I want to overload an operator (e.g. operator<<) with different types, but also change only 1 or 2 values in the function itself.

I want to get some fancy output of a vector with the primitive integer types.
I have tried to create a template function like template<int width, typename T> operator<<... but this would need to set the width and typename explicitly with each call of the operator<< e.g.

I have a following minimal code example (which is not the shortest tbh):

main.cpp

#include <iostream>
#include <vector>

#include "utils.h"

using namespace std;

int main(int argc, char* argv[]) {
  vector<int8_t> vec_s_1 = {1, 2, 3, -1, -2, -123};
  vector<uint8_t> vec_u_1 = {1, 2, 3, 127, 128, 129};
  vector<int16_t> vec_s_2 = {1, 2, 3, -1, -2, -123};
  vector<uint16_t> vec_u_2 = {1, 2, 3, 127, 128, 129};
  vector<int32_t> vec_s_3 = {1, 2, 3, -1, -2, -123};
  vector<uint32_t> vec_u_3 = {1, 2, 3, 127, 128, 129};
  vector<int64_t> vec_s_4 = {1, 2, 3, -1, -2, -123};
  vector<uint64_t> vec_u_4 = {1, 2, 3, 127, 128, 129};

  cout << "vec_s_1: " << vec_s_1 << endl;
  cout << "vec_u_1: " << vec_u_1 << endl;
  cout << endl;
  cout << "vec_s_2: " << vec_s_2 << endl;
  cout << "vec_u_2: " << vec_u_2 << endl;
  cout << endl;
  cout << "vec_u_3: " << vec_u_3 << endl;
  cout << "vec_s_3: " << vec_s_3 << endl;
  cout << endl;
  cout << "vec_s_4: " << vec_s_4 << endl;
  cout << "vec_u_4: " << vec_u_4 << endl;

  return 0;
}

utils.h

#ifndef MODULO_SEQUENCE_UTILS_H
#define MODULO_SEQUENCE_UTILS_H

#include <vector>
#include <iomanip>
#include <ostream>
#include <sstream>

using namespace std;

template<typename T>
ostream& operator<<(ostream& os, const vector<T>& obj);

#endif // MODULO_SEQUENCE_UTILS_H

utils.cpp

#include "utils.h"

#define GET_FUNCTION(WIDTH, TYPENAME, MASK) \
template<> ostream& operator<<(ostream& os, const vector<TYPENAME>& obj) { \
  size_t size = obj.size(); \
  os << "["; \
  for (size_t i = 0; i < size; ++i) { \
    if (i > 0) { \
      os << ", "; \
    } \
    stringstream ss; \
    ss << "0x" << hex << uppercase << setw(WIDTH) << setfill('0') << (obj[i] & MASK); \
    os << ss.str(); \
  } \
  os << "]"; \
  return os; \
}

GET_FUNCTION(2, int8_t, 0xFF)
GET_FUNCTION(2, uint8_t, 0xFF)
GET_FUNCTION(4, int16_t, 0xFFFF)
GET_FUNCTION(4, uint16_t, 0xFFFF)
GET_FUNCTION(8, int32_t, 0xFFFFFFFF)
GET_FUNCTION(8, uint32_t, 0xFFFFFFFF)
GET_FUNCTION(16, int64_t, 0xFFFFFFFFFFFFFFFF)
GET_FUNCTION(16, uint64_t, 0xFFFFFFFFFFFFFFFF)

So far it is working like expected and the output is like I wanted to have it. It is solved by calling #define for each function separately (which is a really looking ugly code).

The output looks like this:

vec_s_1: [0x01, 0x02, 0x03, 0xFF, 0xFE, 0x85]
vec_u_1: [0x01, 0x02, 0x03, 0x7F, 0x80, 0x81]

vec_s_2: [0x0001, 0x0002, 0x0003, 0xFFFF, 0xFFFE, 0xFF85]
vec_u_2: [0x0001, 0x0002, 0x0003, 0x007F, 0x0080, 0x0081]

vec_u_3: [0x00000001, 0x00000002, 0x00000003, 0x0000007F, 0x00000080, 0x00000081]
vec_s_3: [0x00000001, 0x00000002, 0x00000003, 0xFFFFFFFF, 0xFFFFFFFE, 0xFFFFFF85]

vec_s_4: [0x0000000000000001, 0x0000000000000002, 0x0000000000000003, 0xFFFFFFFFFFFFFFFF, 0xFFFFFFFFFFFFFFFE, 0xFFFFFFFFFFFFFF85]
vec_u_4: [0x0000000000000001, 0x0000000000000002, 0x0000000000000003, 0x000000000000007F, 0x0000000000000080, 0x0000000000000081]

I need the different widths of the leading zeros in the output plus the mask for getting the exact hex number for each type.

My question is: How can/could I achieve the same result with only using templates?

The trick is to use a byte_repeater like here.

template<typename T, std::size_t count, int byte>
struct byte_repeater;

template<typename T, int byte>
struct byte_repeater<T, 1, byte> {
    static constexpr T value = byte;
};

template<typename T, std::size_t count, int byte>
struct byte_repeater {
    static constexpr auto value = (byte_repeater<T, count-1, byte>::value << CHAR_BIT) | byte;
};

Add a convenient function make_mask

template<typename T, int mask>
struct make_mask {
    static constexpr auto value = byte_repeater<typename std::make_unsigned<T>::type, sizeof(T), mask>::value;
};

And then the ostream<< operator becomes:

template<typename T>
std::ostream& operator<<(std::ostream& os, const std::vector<T>& obj)
{
    static constexpr auto width = sizeof(T) * 2;
    static constexpr auto mask = make_mask<T,0xFF>::value;
    os << "[";
    std::for_each(obj.begin(), obj.end() - 1, [&os](const auto& elem) {
        os << "0x" << std::hex << std::uppercase << std::setw(width) << std::setfill('0') << (elem & mask) << ", ";
    });
    os << "0x" << std::hex << std::uppercase << std::setw(width) << std::setfill('0') << (obj.back() & mask);
    os << "]";
    return os;
}

LIVE DEMO


Optimizing performance of hashing array algorithm's implementation I wrote the following C++ code and since I am a novice in C++ and do not really have anybody who can give me advice, I think this site would be a good place to ask.

The algorithm takes the first two elements a1 and a2 of an array, calculate the difference a2 - a1. Then the first two elements a1 and a2 will be removed from the array and the calculated difference will be inserted in front of the array. This will be repeated until there is only one element left. The last element of the array will be the hash value of the array.

The initial array will be mutated q times with the following formula: increase all elements in the segment [lj,rj] by vj, after each mutation the hash value should be printed.

The input will look something like following

7
4 2 -5 10 4 -2 6
4
2 4 -8
5 7 2
3 3 -1
3 7 3

where the first line indicates the length of the array. The next line will be the content of the array, the third line would be q and every following line is lj, rj, vj.

This is how my code looks like at the moment. I would appreciate every critic.

#include <vector>
#include <iostream>

#define ll long long

ll __hash__(const std::vector<int> &arr) {
    ll front = arr[0];
    for(ll i = 1; i < arr.size(); i++){
        front = arr[i] - front;
    }
    return front;
}

int main() {
    std::vector<int> arr;
    ll size;
    std::cin >> size;
    for (ll i = 0; i < size; ++i) {
        ll c;
        std::cin >> c;
        arr.push_back(c);
    }
    ll q;
    std::cin >> q;
    for (ll i = 0; i < q; ++i) {
        ll l, r, v;
        std::cin >> l >> r >> v;
        for(ll j = l - 1; j < r; ++j){
            arr[j] += v;
        }
        std::cout << __hash__(arr) << std::endl;
    }
    return 0;
}

The Good

The code is fairly readable, horizontal spacing is good.

The variables are declared as they are needed.

You didn't use using namespace std; which is very good for a beginner.

Macros in C++

Macros are included in C++ to support some backward compatibility with the C programming language that it grew out of, but they generally aren't used in C++ because they aren't type safe. Replacements for macros are inline functions, and lambda expressions or closures.

An alternate to what is in the code is to use typedef. A second alternative is to use using ll = long long;.

The best thing might be to find what types C++ supports and use that instead. You can find a list of the types provided by the cstdint header file here.

Another alternative is to just use long long rather than shortening it. C++ is not a scripting language and C++ coders are used to things like this.

Beginning a Variable or Function with Double Underscore.

Function names and variables should start with letters rather than underscores, double underscore is reserved for special usage.

Variable Names

Single letter variable names make the code very hard to understand, I don't even have a clue what c, q, l, r or v are or I would try to guess at their meanings.

Variables i and j are used as indexes into the vector, it would be better if these variables were of type size_t rather than long long because size_t is defined as unsigned and can't be negitive. It would very rare to have an array or vector that would require the address space of a long long.

One Variable Declaration Per Line

The code contains this line

        ll l, r, v;

To make the code more maintainable each variable should be declared and initialized on it's own line.

        ll l = 0;
        ll r = 0;
        ll v = 0;

Vertical Spacing

To make the code more readable it needs some vertical spacing, one line between logical blocks:

#include <vector>
#include <iostream>

using ll = long long;

ll __hash__(const std::vector<int> &arr) {
    ll front = arr[0];
    for(ll i = 1; i < arr.size(); i++){
        front = arr[i] - front;
    }
    return front;
}

int main() {
    std::vector<int> arr;

    ll size;
    std::cin >> size;
    for (ll i = 0; i < size; ++i) {
        ll c;
        std::cin >> c;
        arr.push_back(c);
    }

    ll q;
    std::cin >> q;
    for (size_t i = 0; i < q; ++i) {
        ll l, r, v;
        std::cin >> l >> r >> v;
        for(size_t j = l - 1; j < r; ++j){
            arr[j] += v;
        }
        std::cout << __hash__(arr) << std::endl;
    }

    return 0;
}


An echo program, mostly in C and completely from scratch This is an echo program with no runtime or standard library. It's meant to be compiled with -nostdlib on an amd64 Linux system.

static signed long mywrite(int fd, const void *buf, unsigned long count) {
    signed long retval;
    __asm__ __volatile__(
        "syscall" :
        "=a" (retval) :
        "a" (1), "D" (fd), "S" (buf), "d" (count) :
        "rcx", "r11", "memory"
    );
    return retval;
}

static void myexit(int status) __attribute__((__noreturn__));
static void myexit(int status) {
    __asm__ __volatile__(
        "syscall" :
        :
        "a" (60), "D" (status) :

    );
    __builtin_unreachable();
}

static unsigned long mystrlen(const char *str) {
    const char *pos = str;
    while(*pos) ++pos;
    return pos - str;
}

static void writearg(char *str, char end) {
    unsigned long size = mystrlen(str) + 1;
    unsigned long written = 0;
    str[size - 1] = end;
    do {
        signed long result = mywrite(1, str + written, size - written);
        if(result < 0) myexit(1);
        written += result;
    } while(written < size);
}

void _start(void) __attribute__((__naked__, __noreturn__));
void _start(void) {
    __asm__(
        "pop %rdi\n\t"
        "mov %rsp, %rsi\n\t"
        "jmp mymain"
    );
}

static void mymain(int argc, char *argv[]) __attribute__((__noreturn__, __used__));
static void mymain(int argc, char *argv[]) {
    if(argc <= 1) {
        myexit(mywrite(1, "\n", 1) != 1);
    }
    int i;
    for(i = 1; i < argc - 1; ++i) {
        writearg(argv[i], ' ');
    }
    writearg(argv[i], '\n');
    myexit(0);
}

Some of my concerns:

Is my program's behavior fully compliant with the standard for echo?
Am I making any unwarranted assumptions that could make my code not work on a future release of Linux (or compiler)? In particular, are popping argc and the way I'm overwriting null terminators in argv values okay?
Since my code is Linux-on-amd64-only anyway, are there any other assumptions that I can make? For example, can I assume that Linux will always have continuous argv values, and so just make one big write call after swapping out all the nulls, instead of one per argument? (I know I'd still have to loop write in case of partial writes. I also know I could just copy the strings around myself, but I'd rather write them from where I got them.)
Instead of having _start as an assembly stub and my real code in mymain, is there any way I can put my real code in _start but still be able to safely get ahold of the command-line arguments?

Is my program's behavior fully compliant with the standard for echo?

Code does not process the string as in the OPERANDS section.  In particular:

Code does not support \c: "Suppress the <newline> that otherwise follows the final argument in the output. ..."

  Am I making any unwarranted assumptions that could make my code not work on a future release of Linux (or compiler)? In particular, are popping argc and the way I'm overwriting null terminators in argv values okay?

I see no trouble with argc.

Overwriting the null terminators in argv may/may not be OK, but is not needed.  I could foresee future restrictions.  Alternative: write the argv[i] and then the separator.

  Other issues

No comment.


DRYing two very similar SQL conditional inserts I have two very similar SQL statements

INSERT INTO user_profile (user_id, setting_id, value)
    SELECT id, 18, true
    FROM users
        WHERE NOT EXISTS (
            SELECT user_id, setting_id FROM user_profile
            WHERE id=user_id AND setting_id=18);

INSERT INTO user_profile (user_id, setting_id, value)
    SELECT id, 16, true
    FROM users
        WHERE NOT EXISTS (
            SELECT user_id, setting_id FROM user_profile
            WHERE id=user_id AND setting_id=16);

I'm repeating everything except for the setting_id. Is there a way to combine these two statements?

You'll want to create a stored procedure that takes your setting ID

    CREATE OR REPLACE FUNCTION InsertUserProfile(id INTEGER)
RETURNS void AS $$
 BEGIN
   INSERT  INTO user_profile
     ( user_id ,
       setting_id ,
       value
     )
     SELECT  id ,
       id ,
       true
     FROM    users
     WHERE   NOT EXISTS ( SELECT user_id ,
            setting_id
           FROM   user_profile
           WHERE  id = id
            AND setting_id = @ID );
 END;
 $$ LANGUAGE plpgsql;

You'll then want to call it like this:

SELECT usp_InsertUserProfile(16)


TDD: String Calculator Kata String Calculator

  Create a simple String calculator with a method int Add(string numbers).

  The method can take 0, 1 or 2 numbers, and will return their sum (for an empty string it will return 0).
  For example "" or "1" or "1,2".
  Start with the simplest test case of an empty string and move to 1 and two numbers
  Remember to solve things as simply as possible so that you force yourself to write tests you did not think about.
  Remember to refactor after each passing test.

  Allow the Add method to handle an unknown amount of numbers.
  Allow the Add method to handle new lines between numbers (instead of commas).

  The following input is ok: "1\n2,3" (will equal 6).
  The following input is NOT ok: "1,\n" (not need to prove it - just clarifying).

  Support different delimiters

  to change a delimiter, the beginning of the string will contain a separate line that looks like this: "//[delimiter]\n[numbers…]".
  For example, "//;\n1;2" should return three where the default delimiter is ';'.
  The first line is optional. All existing scenarios should still be supported.

  Calling Add with a negative number will throw an exception "negatives not allowed" - and the negative that was passed. If there are multiple negatives, show all of them in the exception message.

import re

def string_calculator(string):
    """
    Sums numbers in a string separated by delimiters.

    >>> string_calculator("")
    0
    >>> string_calculator("42")
    42
    >>> string_calculator("473,12")
    485
    >>> string_calculator("1\\n2,3")
    6
    >>> string_calculator("//;\\n1;2")
    3
    >>> string_calculator("-42, 2")
    Traceback (most recent call last):
    ...
    ValueError: negatives not allowed ['-42']
    >>> string_calculator("-42,31,-312")
    Traceback (most recent call last):
    ...
    ValueError: negatives not allowed ['-42', '-312']
    """
    if re.match('-[0-9]+', string):
        raise ValueError("negatives not allowed {}".format(
            re.findall('-[0-9]+', string)))
    separators = ',', '\\n'
    if "//" in string:
        sep_info = re.findall('//.+\n', string)[0]
        separators = sep_info.replace('//','').replace('\\n','')
        string = string.replace(sep_info, '')
    if not string:
        return 0
    if not any(separator in string for separator in separators):
        return int(string)
    return sum(int(number) for number in re.split('|'.join(separators), string))

There's a redundant if branch:

if not any(separator in string for separator in separators):
    return int(string)

removing it still passes all the tests, because

re.split('|'.join(separators), string)

returns a list containing the list itself if there are no separators in the string.


Generate nth prime number in Python Today I had an interview, where I was asked to solve this problem:

  Generate nth prime number. Given a signature below, write python logic
  to generate the nth prime number:

def nth_prime_number(n):
  # n = 1 => return 2
  # n = 4 => return 7
  # n = 10 => return 29

I wrote this code, but couldn't get through:

def nth_prime_number(n):
    if n==1:
        return 2
    count = 1
    num = 3
    while(count <= n):
        if is_prime(num):
            count +=1
            if count == n:
               return num
        num +=2 #optimization

def is_prime(num):
    factor = 2
    while (factor < num):
        if num%factor == 0:
             return False
        factor +=1
    return True

The overall feedback was that the code quality could be improved a lot, and that I should be more optimal in my approach. How can I improve this code?

Your is_prime() function checks if num is a multiple of any number below it. This means that it checks if it is a multiple of 2, 4, 6, 8, 10, etc. We know that if it isn't a multiple of 2, it won't be a multiple of 4, etc. This goes for all other numbers, if it isn't a multiple of 3, it won't be a multiple of 27 (3x3x3).

What you need to do is check if num is a multiple of any prime number before it.

def nth_prime_number(n):
    # initial prime number list
    prime_list = [2]
    # first number to test if prime
    num = 3
    # keep generating primes until we get to the nth one
    while len(prime_list) < n:

        # check if num is divisible by any prime before it
        for p in prime_list:
            # if there is no remainder dividing the number
            # then the number is not a prime
            if num % p == 0:
                # break to stop testing more numbers, we know it's not a prime
                break

        # if it is a prime, then add it to the list
        # after a for loop, else runs if the "break" command has not been given
        else:
            # append to prime list
            prime_list.append(num)

        # same optimization you had, don't check even numbers
        num += 2

    # return the last prime number generated
    return prime_list[-1]

I'm sure someone else here will come up with an even more efficient solution, but this'll get you started.


Wrapping a javascript "class"? Below is a simple "class" for displaying messages.  It stores the output html div and then displays messages to it as needed.

It works fine.  For readability, and encapsulation purposes, I'd like the componenets of the "class" in a contianer of sorts or a javascript object?  What is the simplest way to to do this.

Or...is there a simple way to do this?

Currently the only container is the comments I've surrounded the code with.

/**
 *      Module  :       Model
 *      Name    :       -Message
 *      Input   :       div via the constructor
                        message type via display
 *      Output  :       TextStream via display to Control.sendInner
 *      Notes   :       Symmetric, client holds message location so needs extra variable
 */

var Message = function( div )
{
    this.div = document.getElementById( div );
};

Message.prototype.messages =
{
    name:       'Please enter a valid name',
    email:      'Please enter a valid email',
    pass:       'Please enter passoword, 6-40 characters',
    url:        'Pleae enter a valid url',
    title:      'Pleae enter a valid title',
    tweet:      'Please enter a valid tweet',
    empty:      'Please complete all fields',
    empty_bm:   'Please complete all fields',
    same:       'Please make emails equal',
    taken:      'Sorry, that email is taken',
    validate:   'Please contact support to reset your password'
};

Message.prototype.display = function( type )
{
    Control.sendInner( this.div, this.messages[type] )
};

/**      Message End        */

You could wrap your whole file in a namespace, as described here.

Of all the possibilities, this is the one I like the most:

var YourNamespace = (function () {
   var Message = function (div) {
      ...
   }
   var aPrivateFunction = function () { ... }
   ...
   return {
      'Message': Message
   };
})();

This lets you define private functions, like aPrivateFunction, not visible outside YourNamespace. You can access Message as YourNamespace.Message.


Function to check that two objects have "equivalent" values I have two objects, a bet and a match. A match has 2 teams and both teams get a score at the end of the match. A bet has two scores set up before the match begins. Here I need to know if the bet specified the same winner as it's set on the match (once it's done).

// isGoodWinner
//
// @description :: return true if has bet on the good winner (or equality)
// @param       :: match (required): the match instance
//                 bet (require): the bet concerned
function isGoodWinner(match, bet) {
  var teamAWins = match.scoreTeamA > match.scoreTeamB;
  var teamBWins = match.scoreTeamA < match.scoreTeamB;
  var equality = match.scoreTeamA === match.scoreTeamB;

  var betOnTeamA = bet.scoreTeamA > bet.scoreTeamB;
  var betOnTeamB = bet.scoreTeamA < bet.scoreTeamB;
  var betEquality = bet.scoreTeamA === bet.scoreTeamB;

  return ((teamAWins && betOnTeamA) ||
        (equality && betEquality) ||
        (teamBWins && betOnTeamB));
}

I think my naming is poor and I don't know how to do it easier, do you have any advice?

Another question I have is: how can I be sure to unit test each case?

Personally, I think your naming is just fine. However, the only thing I'd change is equality. That name is very general.

Luckily, since we are talking about races, we can use information about races to name the variables. As you probably know, in a race, when two or more teams end with the same exact score, it is called a tie. Therefore, you should rename that variable to tie.

And, now that I think about it, your function could use some renaming too. isGoodWinner doesn't really tell you what the function is doing; what is a "good winner"? I would name it to isWinningBet. That name tells you if the bet (not the winner) was on the winning team.

You have a few extra variables here. The ones in question are teamAWins and teamBWins, and betOnTeamA and betOnTeamB.

These can be simplified. If someone did not be on team A, they bet on team B (ties don't matter because the variable holding the tie will handle that). And, if team A did not win, then team B win. Both of these apply visa-versa, too.

That being said, we can reduce the variables you need:

var teamAWins = match.scoreTeamA > match.scoreTeamB;
var equality = match.scoreTeamA === match.scoreTeamB;

var betOnTeamA = bet.scoreTeamA > bet.scoreTeamB;
var betEquality = bet.scoreTeamA === bet.scoreTeamB;

Good job writing documentation for your function. I don't often see JavaScript code with documentation, even thought it should.

And, good job using the === operator. Most of the time, people will use the == operator in JavaScript which is a bad idea on a few levels.

Your design is a little confusing.

Judging by these variables:

var betOnTeamA = bet.scoreTeamA > bet.scoreTeamB;
var betOnTeamB = bet.scoreTeamA < bet.scoreTeamB;
var betEquality = bet.scoreTeamA === bet.scoreTeamB;

a person can bet different amounts on both teams. However, your function declares that, if they bet more on, say, team A, then their full bet is on team A.

Now, I don't know what the rest of your code is supposed to do, but I assume you are going to have some payback for winning bets. Well, if a user bets (for example) 10 on team A and 5 on team B, then team B wins, how are you supposed to know which amount to payback?

You wouldn't want to payback based on a 15 bet because the person bet 15 in total, you'd want to payback based on a 5 bet because that bet was on the winning team.

There are two ways you could fix this:

Have the function return an object, saying if the bet was on a winning team and if so, which team the bet was on.
Change your bet design so a bet can only be placed on a single team.

Personally, I'd go with the second option. This "aligns" your objects more, so one object isn't doing the work or holding the information of another. And, it makes more sense in real life: at a real race, you could place a single bet that applied to multiple teams; you place multiple bets, one for each team.


Determine if element is a select, if so fill it with data from another object The following snippet is an object that handles each of my app's data management views. When a view is created in App.partialView(), I have hard-coded each element that is used to edit that view's data. Some of those elements are select lists that contain data from another view within the same App.dataManager(), never from another manager.

My issue is with the process in which I fill those select lists. I am already looping through each view within the manager, but I need to get the data from the select list's parent data source (another view) so I ultimately have to loop through the views within the existing loop.

Does this appear to be the only way to handle my issue, am I missing something that's right in front of me, or am I out of left field picking daisies?

EDIT: I'm not one to be offended by criticism and I am open to suggestions for improvement in my overall use of JavaScript. I am self taught, so any advice is welcome.

var App = {};
App.dataManager = function() {
  var a = arguments[0];
  var _this = this;
  this.name = a.name;
  this.views = a.views;
  this.tab = a.tab;

  this.views.forEach(function(view) {
    view.dataManager = _this;
  });

  this.tab.on("shown.bs.tab", function() {
    _this.getData();
  });

  this.getData = function() {
    this.views.forEach(function(view) {
      api
        .get(view.dataUri)
        .then(function(data) {
          view.data = data;
        })
        .then(function() {
          fillTable(view);
        })
        .then(function() {

          //***This is the block in question***

          _this.views.forEach(function(v) {
            v.elements.forEach(function(e) {
              if (e.node.tagName === "SELECT" && e.dataParent) {
                var dP = _this.views.find(function(d) {
                  return d.name === e.dataParent;
                });
                if (dP && dP.data) {
                  var dPData = dP.data.filter(function(d) {
                    return d.Enabled;
                  });
                  v.loadDropdownList(e.node, dPData);
                }
              }
            });
          });
        })
        .then(function() {
          if (!view.listeners) {
            view.addButton.addEventListener("click", function() {
              addButtonHandler(view);
            });
            view.updateButton.addEventListener("click", function() {});
            view.listeners = true;
          }
        });
    });
  };

  function addButtonHandler(view) {
    //sanitized for review
  }

  function fillTable(view) {
    //sanitized for review
  }

  function toggleHideButtons(row) {
    //sanitized for review
  }

  function toggleShowButtons(row) {
    //sanitized for review
  }

  function toggleEntity(row, view) {
    //sanitized for review
  }
};

I tried to decipher your code, and express it in pseudo-code:

for each view:
    request view data from server
    for each view:
        for each <select> element with dataParent in view:
            find view where view.name == dataParent
            if found and has data:
                load <select> with data from that view

So you load data for each view, but then you also go through all other views to pass this data to them when they happen to need it. However you don't only pass the data from currently loaded view, but you try to cover all the views (some of which have not been loaded yet).

So the first optimization to make is to eliminate this third loop through all the views. Just passing the data from currently loaded view to the views that need it:

for each view (loadedView):
    request view data from server
    for each view:
        for each <select> element where dataParent == loadedView.name:
            load <select> with data from that loadedView

That's better, but we still need to loop through all the views to find out which ones need our data. Perhaps there are hundreds of views and only three need the data.

A better approach would be to use a publish-subscribe model. Instead of having to loop through all the views, each view that needs data would subscribe itself on the view who's data it needs. When the data for that view is loaded, it would publish this data to all its subscribers

For example:

viewA = new View();
viewB = new View();
viewC = new View();
viewA.addListener(viewB);
viewA.addListener(viewC);
// load view A, which will also push data to views B & C
viewA.loadData();

Each view would have an array of its subscribers. .addListener() method would add a new item to this array. And in loadData(), after the data has been fetched from server, we'd loop through this subscribers array and call a method like addData() on each view.


A Class to Supply an Open Database Connection during the life of a Web Request For some database requests, I like to use raw ADO.NET.

In the context of a web request, I have created a class which provides an open IDbConnection object.

I use a Dependency Injection library ("DI") to do this, scoping the object instantiated from this class to a web request. So, disposal of the object is handled by the DI container.

The abstraction for this class is simple and looks like this:

public interface IDbConnectionManager : IDisposable
{
    IDbCommand BuildCommand(DbParameter[] parameters, string query);
    IDbConnection GetOpenConnection();
}

And the concrete implementation, written for Sql Server, looks like this:

public class DbConnectionManager : IDbConnectionManager
{
    public SqlConnection DbConnection;
    public string ConnectionString { get; set; }

    public DbConnectionManager(string connectionString)
    {
        ConnectionString = connectionString;
    }

    public IDbConnection GetOpenConnection()
    {
        return GetOpenSqlConnection();
    }

    private SqlConnection GetOpenSqlConnection()
    {
        if (ReferenceEquals(DbConnection, null))
        {
            DbConnection = new SqlConnection(ConnectionString);
        }

        if (DbConnection.State != ConnectionState.Open)
        {
            DbConnection.Open();
        }

        return DbConnection;
    }

    public IDbCommand BuildCommand(DbParameter[] parameters, string query)
    {
        if (parameters == null) throw new ArgumentNullException(nameof(parameters));
        if (string.IsNullOrWhiteSpace(query)) throw new ArgumentException(nameof(query));

        var command = new SqlCommand { Connection = GetOpenSqlConnection() };
        command.Parameters.AddRange(parameters);
        command.CommandText = query;

        return command;
    }

    public void Dispose()
    {
        DbConnection?.Dispose();
    }
}

You can see the BuildCommand method uses the GetOpenSqlConnection for its connection.

The main reason I have taken this approach is because DbConnections are expensive. But I'm not sure whether that relates to creating a connection or opening one.

I'm aware that connection pooling is available (if enabled), but I figured for simple web requests (think API), with perhaps just 2 queries to the database, that this would probably be a good approach.

And as can be seen, there's not a lot to the code.

I just wanted a bit of feedback on this code/approach.
And if you can see any potential problems with it, by all means let me know that too.

As a last comment, I am aware that a developer could manually call Dispose or use it in a using block (thereby calling Dispose).
This would be on me as lead developer to ensure that this doesn't happen and that devs understand that the DI container disposes of the object.

As far as I know, in ADO.NET the connection pooling is enabled by default, and if you don't need to pool connections, you'll have to explicitly disable pooling.

So, for your implementation, everything seems okay to me, but DbConnectionManager seems to be specific for handling the connection! so, BuildCommand method will be odd in this class!

I would prefer to rename it to something covers all exposed operations and keep everything simpler.

for GetOpenSqlConnection() this could be unnessary, since you can do it in the property level.

What I suggest is to make DbConnection & ConnectionString static, so you ensure you only have a single instance of SqlConnection. and create two constructors one takes SqlConnection, and the other one takes connectionString. you would have something like this :

public class DbConnectionManager : IDbConnectionManager
{
    // make it static to have a single instance
    private static SqlConnection DbConnection
    {
        get => DbConnection is null ? new SqlConnection(ConnectionString) : DbConnection;
        set => DbConnection = value;
    }

    // store the connectionString of SqlConnection, for connection backup.
    private static string ConnectionString { get; set; } = DbConnection.ConnectionString;

    public DbConnectionManager(string connectionString)
        : this(new SqlConnection(connectionString)) { }

    public DbConnectionManager(SqlConnection dbConnection)
    {
        DbConnection = dbConnection;
        ConnectionString = dbConnection.ConnectionString;
    }
}

in both constructors, they're initiating a new DbConnection and the ConnectionString is just backup, in case your actual DbConnection is lost, you can re-initiate it with the connectionString that you've stored.

doing that, it'll eleminate the need of :

private SqlConnection GetOpenSqlConnection()
{
    if (ReferenceEquals(DbConnection, null))
    {
        DbConnection = new SqlConnection(ConnectionString);
    }

    if (DbConnection.State != ConnectionState.Open)
    {
        DbConnection.Open();
    }

    return DbConnection;
}

for :

public IDbCommand BuildCommand(DbParameter[] parameters, string query)
{
    if (parameters == null) throw new ArgumentNullException(nameof(parameters));
    if (string.IsNullOrWhiteSpace(query)) throw new ArgumentException(nameof(query));

    var command = new SqlCommand { Connection = GetOpenSqlConnection() };
    command.Parameters.AddRange(parameters);
    command.CommandText = query;

    return command;
}

Since there is no actual execution is going here, opening a connection here is not needed, because you built the command and return the instance back to execute it somewhere else. this may requires you to create two public methods for opening and closing connection before you call the execute methods such as command.ExecuteNonQuery().

What I would do is maybe make this method private, and create public methods for each SQL execution type such as ExecuteNonQuery and ExecuteScalar and ExecuteReader, with the same arguments of BuildCommand has, and all will be from within the same class.
Something like this :

public void ExecuteNonQuery(DbParameter[] parameters, string query)
{
    var command = BuildCommand(parameters, query);

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

    command.ExecuteNonQuery();
}

The best approach for that will be creating a static property of SqlCommand then you initiate it, use it across the class, dispose it whenever you're done.

another question got in my mind is this :

if (parameters == null) throw new ArgumentNullException(nameof(parameters));

why you made parameters required? suppose you need to execute a query with no parameters such as SELECT * FROM table, then, you would have to adjust the current implementation or create new method for that. So, keeping it optional will come in handy.


Locating matching files with input folder and file prefix This is my first more-than-1-line script. It takes an input folder and a file prefix and gets all the matching files. For the first of the files, the script grabs the first line and appends an extra label string and puts that into $data. For all files (including the first), it takes the second line, adds the timestamp found from the filename, and appends a that as new line to $data. When it's done, it writes the output.

The problem is it's pretty slow. 1 directory might have 10,000 files. In that case, the script takes about 3-4 minutes to complete. When doing this over 10 such directories, it begins to get quite annoying.

So, I'm hoping someone here could help speed things up, preferably with explanations so I also get better.

for cs in {47..52}; do
  csdirnm="CASE$cs";
  tsdirnm="eta_at_x2";
  label='flow-time';
  dir="$csdirnm/$tsdirnm/";
  files=$(ls -tr $dir); # -tr sort on time created, reversed (newest last).
  i=0;
  for f in $files ; do
    fn=$dir$f;
    if [[ $i = 0 ]]; then
      data="$(sed -n 1p $fn) $label";
      ((i++));
    fi
    d=$(sed -n 2p $fn);
    t=$(echo $fn | perl -pe 's|.*?(\d+\.\d+)|\1|');
  data+="\n$d $t";
  done;
  echo -e "$data">"$csdirnm/${csdirnm}_${tsdirnm}_20T.time-series";
done;

The problem is almost certainly this line here:

t=$(echo $fn | perl -pe 's|.*?(\d+\.\d+)|\1|');

If you are invoking the Perl interpreter for each line, you will struggle.

A close second is that, for each file, you invoke 2 subshells, and two other program (sed and echo).

My recommendation is for you to actually rewrite the whole thing in Perl...

....

but, you may find it faster to use sed

t=$(echo $fn | sed -e '$s@.*[^0-9]\([0-9]\+\.[0-9]\+\).*@\1@g')

The above expression takes advantage of the fact that there has to be some non-digit character before the first date digit.

But, as I say, it is my suggestion that you do the whole thing in Perl, and avoid having to do all the execs.

EDIT: In Perl

#!/usr/bin/perl -w
#
#for cs in {47..52}; do
#  csdirnm="CASE$cs";
#  tsdirnm="eta_at_x2";
#  label='flow-time';
#  dir="$csdirnm/$tsdirnm/";
#  files=$(ls -tr $dir); # -tr sort on time created, reversed (newest last).
#  i=0;
#  for f in $files ; do
#    fn=$dir$f;
#    if [[ $i = 0 ]]; then
#      data="$(sed -n 1p $fn) $label";
#      ((i++));
#    fi
#    d=$(sed -n 2p $fn);
#    t=$(echo $fn | perl -pe 's|.*?(\d+\.\d+)|\1|');
#    data+="\n$d $t";
#  done;
#  echo -e "$data">"$csdirnm/${csdirnm}_${tsdirnm}_20T.time-series";
#done;

use strict;

foreach my $dirnum (47 .. 52) {
  my $csdirnm = "CASE$dirnum";
  print "Processing Dir $csdirnm\n";
  my $tsdirnm = "eta_at_x2";
  # Use ls to get the file ordering right.
  my $subdir = "$csdirnm/$tsdirnm";
  next unless -d $subdir; #Skiip non-existant dirs
  open DIRLIST, "-|", "ls -tr $subdir" or die "Unable to list files in $subdir";
  my $outfile = "${csdirnm}/${csdirnm}_${tsdirnm}_20T.time-series";
  open REPORTFILE, ">", $outfile or die "Unable to write to report file $outfile";

  my $file;
  my $cnt = 0;
  while ($file = <DIRLIST>) {
    chomp $file;
    print "Processing $file\n";
    my $datafile = "$subdir/$file";
    open DATA, "<", $datafile or die "Unable to read file $datafile";
    my $line1 = <DATA>;
    my $line2 = <DATA>;
    close DATA;
    print REPORTFILE $line1 unless $cnt;
    print REPORTFILE $line2;
    $cnt++;
  }

  close DIRLIST;
  close REPORTFILE;
}


Avoiding nested callbacks when using MongooseJS I'm diving into the world of nodejs and mongo. I am playing around with a simple app that uses MongooseJS to talk to Mongo. I have been looking at examples and some GitHub projects for references, but the pattern seems very sloppy to me. Is this approach considered normal, or is there a better way?

Here is a sample from the app I am building with my daughter. Those nested callbacks feel like an anti-pattern to me.  Am I wrong? Are there better ways to do it?

//userController.js when a new user registers an account is made and a document is cloned that will be their new "pet";

var Pet = require("models/pet.js");
var User = require("models/user.js");

exports.register = function (req, res, next) {
    var user = new User({
        username: req.body.username,
        password: req.body.password
    });

    user.save(function (err) {
        if (err) {
            next(err);
            return;
        }

        Pet.findOne({
            "name": "Egg",
            "ownerId": "<<system>>"
        }, function (err, pet) {
            if (err){
                next(err);
                return;
            }
            Pet.clonePetForUser(user._id, pet._id, function (err) {
                if (err) {
                    next(err);
                    return;
                }
                res.json({message: 'User created'});
            })
        });
    });
};

Yes you're right, there are better solutions. For example you could use promises instead of callbacks since mongoose operations can return promises according to the documentation.

Your example with promises:

var Pet = require("models/pet.js");
var User = require("models/user.js");

exports.register = function (req, res, next) {

    var user = new User({
        username: req.body.username,
        password: req.body.password
    });

    user.save()
        .then(function () {
            return Pet.findOne({
                "name": "Egg",
                "ownerId": "<<system>>"
            });
        })
        .then(function (pet) {
            return Pet.clonePetForUser(user._id, pet._id);
        })
        .then(function () {
            res.json({message: 'User created'});
        })
        .catch(function (err) {
            console.error(err);
            next(err);
        });

});

You can write your own method to return a promise. Instead of:

petSchema.statics.clonePetForUser = function (userId, petId, callback) {
     this.find({ }, callback);
};

You can write:

petSchema.statics.clonePetForUser = function (userId, petId) {
    return this.find({ });
};


Creating efficient PDO functions I have the following functions:

 function getExample($id, $otherid) {
            global $db;
            $query = 'SELECT * FROM examples WHERE id = :id AND otherid= :otherid';
            try {
                $statement = $db->prepare($query);
                $statement->bindValue(':id', $id);
                $statement->bindValue(':otherid', $otherid);
                $statement->execute();
                $result = $statement->fetchAll();
                $statement->closeCursor();
                return $result;
            } catch (PDOException $e) {
                $error_message = $e->getMessage();
                display_db_error($error_message);
            }
        }

 function getSecExample($id) {
            global $db;
            $query = 'SELECT * FROM examples WHERE id= :id';
            try {
                $statement = $db->prepare($query);
                $statement->bindValue(':id', $id);
                $statement->execute();
                $result = $statement->fetchAll();
                $statement->closeCursor();
                return $result;
            } catch (PDOException $e) {
                $error_message = $e->getMessage();
                display_db_error($error_message);
            }
        }

Is there a way to make this one function instead of two? I want it to be secure and done with the best practices.

I'm not sure why you think that forcing this into one function will make it a "best practice," and that doesn't seem right anyways.

I suppose one way you could do it is:

 WHERE id = :id AND otherid LIKE :otherid

If it were to be in the second function, :otherid would be a wildcard (%). Otherwise just have your value. Check out this.


Constructor to transform an Outlook contact into a Dynamics CRM contact This is my constructor to transform an Outlook contact into a Dynamics CRM contact:

public Contact(ContactItem contactItem)
{
    LogicalName = EntityLogicalName;
    FirstName = contactItem.FirstName;
    LastName = contactItem.LastName;
    //if no last name, use the first name
    if (String.IsNullOrEmpty(LastName))
    {
        LastName = contactItem.FirstName;
        //if no First name, use the Full Name.
        if (String.IsNullOrEmpty(LastName))
        {
            LastName = contactItem.FullName;
            //if no last name or first name, use the company name
            if (String.IsNullOrEmpty(LastName))
            {
                LastName = contactItem.CompanyName;
                //If no last name, no first name and no company name, use the email address.
                if (String.IsNullOrEmpty(LastName))
                {
                    LastName = contactItem.Email1Address;
                    //if no Last name, no first name, no company name, no email address, default to naam niet bekend.
                    if (String.IsNullOrEmpty(LastName))
                    {
                        LastName = @"Naam niet bekend";
                    }
                }
            }
        }
        else
        {
            FirstName = "";
        }
    }

    Address1_City = contactItem.BusinessAddressCity;
    Address1_Country = contactItem.BusinessAddressCountry;
    Address1_Line1 = contactItem.BusinessAddressStreet;
    Address1_PostalCode = contactItem.BusinessAddressPostalCode;
    Telephone1 = contactItem.BusinessTelephoneNumber;
    Telephone2 = contactItem.HomeTelephoneNumber;
    Fax = contactItem.BusinessFaxNumber;
    Address2_Line1 = contactItem.HomeAddressStreet;
    Address2_City = contactItem.HomeAddressCity;
    Address2_PostalCode = contactItem.HomeAddressPostalCode;
    Address2_Country = contactItem.HomeAddressCountry;
    EMailAddress1 = contactItem.Email1Address;
    EMailAddress2 = contactItem.Email2Address;
    EMailAddress3 = contactItem.Email3Address;
}

As you can see, I got 5 nested ifs to determine what I should use for LastName. I don't like how it looks, but the only other option I can see is putting them in sequence instead of nested. However, that would mean that instead of usually having only 1-3 ifs, it would be forced into doing 5 ifs every single time. during tests with 1800 records, it already felt noticeably slower if it got to the literal string.

Is there some really easy way (or some really obscure way) of doing this which I missed?

You could create a helper function with a loop:

public string getFirstNonNullOrEmpty(params string[] names) {
    foreach (string name in names) {
        if (String.IsNullOrEmpty(name)) {
            continue;
        }
        return name;
    }
    return @"Naam niet bekend";
}

Usage:

LastName = getFirstNonNullOrEmpty(contactItem.LastName, contactItem.FirstName,
    contactItem.FullName, contactItem.CompanyName, contactItem.Email1Address);

You might also need this because of the else branch:

if (String.IsNullOrEmpty(contactItem.LastName))
    && !String.IsNullOrEmpty(contactItem.FirstName)) {
    FirstName = "";
}

You might want to use IsNullOrWhiteSpace instead of IsNullOrEmpty.

(I'm not too familiar with C# and don't have a C# compiler right now so the syntax might be invalid.)


Improving and optimizing squeeze() function I am currently reading The C Programming Language by Dennis Richie and Brian Kernighan, and I came to implement the function squeeze (s1, s2) as an exercise. squeeze() deletes each character in s1 that matches any character in the string s2. While coding squeeze(), I thought that I can extend its capability to act like trim() method in Java, and then the my code was already done, and tested.

However, even though it is implemented in C, I believe that the code below can be improved and optimized. Please help me do so.

char *squeeze (char *str1, char *str2) {

     int i, j;
     int str1Len = strlen (str1);
     int toBeSubtractLen = 0;

     char chr1 = '\0';
     char chr2 = '\0';

     for (i = 0; i < str1Len; i++) {
          chr1 = str1 [i];
          for (j = 0; j < strlen (str2); j++) {
               chr2 = str2 [j];
               if (chr1 == chr2) {
                    toBeSubtractLen++;
               }
          }
     }

     char *finalStr;
     finalStr = malloc ((str1Len - toBeSubtractLen) + 1);
     if (finalStr == NULL) {
          printf ("Unable to allocate memory.\n");
          exit (EXIT_FAILURE);
     }

     int indx = 0;
     for (i = 0; i < str1Len; i++) {
          chr1 = str1 [i];
          for (j = 0; j < strlen (str2); j++) {
               chr2 = str2 [j];
               if (chr1 == chr2) {
                    break;
               }
          }
          if (chr1 != chr2) {
               finalStr[indx] = chr1;
               indx++;
          }
     }
     return finalStr;

} /* end of squeeze() */

Samples:

str1 = ",AaBbCcDdEeFfGgHhIiJjKkLlMmAaAaAaNnOoPpQqRrSsTtUuVvWwXxYyZz"
str2 = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
str3 = "abcdefghijklmnopqrstuvwxyz"
name = "ChristopherM.Sawi"
birthday = "August14,1988"

Outputs:

1. squeeze-ing str1 by str2 yields: "<space>,<tab>abcdefghijklmaaanopqrstuvwxyz"
2. squeeze-ing str1 by str3 yields: "<space>m<tab>ABCDEFGHIJKLMAAANOPQRSTUVWXYZ"
3. squeeze-ing str2 by str3 yields: "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
4. squeeze-ing name by str3 yields: "C<space>M.<space>S"
5. squeeze-ing birthday by str1 yields: "141988"

You want to avoid repeat iteration over the 2nd string, which is O(N).  Do this N times and you have an O(N2) algorithm.

One trick you could use is to build a lookup table of the chars in str2.  This would cost O(N) and some extra memory upfront, but then subsequent lookups in the table could be brought down to O(1).

Here's a simple example.  The trade-off is this takes a bit more memory and makes some assumptions about the range of char.  It also clobbers the buffer of str1:

#include <limits.h>   // for UCHAR_MAX

char *squeeze (char *str1, char *str2)
{
   char present[UCHAR_MAX + 1] = {0};
   char *src, *dst;

   // Build lookup table of chars in str2
   //
   while (*str2)
      present[(unsigned char)*str2++] = 1;

   // Iterate through str1, removing chars along the way.
   //
   src = dst = str1;
   while (*src)
   {
      // Is *src in str2?
      //
      if (present[(unsigned char)*src])
      {
         // Yes, remove this char.. (Advance src but not dst.)
         //
         ++src;
      }
      else
         *dst++ = *src++;
   }

   // NUL terminator
   //
   *dst = 0;

   return str1;
}


Vimeo API - Loading videos I know my way around jQuery and JavaScript a little bit but as for optimising, I'm a bit weak but willing to learn new stuff.

I'm currently loading videos onto our site from Vimeo when a user clicks the appropriate image area.  This is working fine, but I feel it's not an entirely performant way to go about doing it.

Does anyone see any issues with the code I've written below which could be done better?

var videoData = [
{
    'player_id':'video1',
    'videoURL':'<?php the_field('two_vimeo_video_url'); ?>',
    'width':'1000',
},
{
    'player_id':'video2',
    'videoURL':'<?php the_field('six_vimeo_video_url'); ?>',
    'width':'1000',
}];

function loadVideo(target, videoid) {
    $.getJSON('http://www.vimeo.com/api/oembed.json?url=' + encodeURIComponent(videoData[videoid]['videoURL']) + '&api=1&player_id='+ videoData[videoid]['player_id'] +'&width='+videoData[videoid]['width']+'&byline=0&color=ffffff&title=0'+'&callback=?', function(data){

        $(target).find('.video-container').html(data.html); // puts an iframe embed from vimeo's json

        $(target).closest('iframe').load(function(){

            $f(player).addEvent('ready', function(id){
                var vimeoVideo = $f(videoid);
            });
        });
    });
}

$(function(){
    // Create array to store values
    var vimeoArray = [];
    var videoContainer = $('.video-container');

    // loop through st
    $(videoContainer).each(function(index, value) {

        // get the image with the data attr on each loop
        var dataAttr = $(this).attr('data-video');

        // if dataAttr is not false
        if ( dataAttr !== undefined) {

            // push data attribute value into array
            vimeoArray.push(dataAttr);

            // Store last element of array on iteration
            var videoid = vimeoArray[vimeoArray.length-1];

            // attach click handler to the parent of the scoped element
            $(this).parent().click(function(){

                // load the video
                loadVideo(this, videoid);
                $(this).unbind('click');
                $(this).find('.b-studio__image').hide();

            });
        }
    });
});

I've created a jsfiddle as well with all the code and some dummy data. Any suggestions would be really helpful.

Just a small point, but in general, its best not to create anonymous functions.
So for example:

$(function(){
    // Create array to store values
    var vimeoArray = [];
    var videoContainer = $('.video-container');

    // etc...

... should be contained inside a named function, and then called.
See Beware anonymous functions.


Extract the title from a webpage using the python 3 standard lib The aim of this module is to extract the title from possibly broken hypertext. Without using any awesome third party modules like bs4 or lxml.

#-*-coding:utf8;-*-
#qpy:3
#qpy:console

'''
Extract the title from a web page using
the standard lib.
'''

from html.parser import HTMLParser
from urllib.request import urlopen
import urllib

def error_callback(*_, **__):
    pass

def is_string(data):
    return isinstance(data, str)

def is_bytes(data):
    return isinstance(data, bytes)

def to_ascii(data):
    if is_string(data):
        data = data.encode('ascii', errors='ignore')
    elif is_bytes(data):
        data = data.decode('ascii', errors='ignore')
    else:
        data = str(data).encode('ascii', errors='ignore')
    return data

class Parser(HTMLParser):
    def __init__(self, url):
        self.title = None
        self.rec = False
        HTMLParser.__init__(self)
        try:
            self.feed(to_ascii(urlopen(url).read()))
        except urllib.error.HTTPError:
            return
        except urllib.error.URLError:
            return
        except ValueError:
            return

        self.rec = False
        self.error = error_callback

    def handle_starttag(self, tag, attrs):
        if tag == 'title':
            self.rec = True

    def handle_data(self, data):
        if self.rec:
            self.title = data

    def handle_endtag(self, tag):
        if tag == 'title':
            self.rec = False

def get_title(url):
    return Parser(url).title

print(get_title('http://www.google.com'))

Looks pretty clean to me, good job! Here's what I recommend:

The to_ascii function is small and readable, but in my opinion the is_string and is_bytes functions are overkill:

def to_ascii(data):
    if isinstance(data, str):
        ...
    elif isinstance(data, bytes):
        ...
    else:
        ...

We can make to_ascii less complex by returning immediately:

def to_ascii(data):
    if isinstance(data, str):
        return data.encode("ascii", errors="ignore")
    elif isinstance(data, bytes):
        return data.decode("ascii", errors="ignore")

We can then leave out the else clause:

def to_ascii(data):
    if isinstance(data, str):
        return data.encode("ascii", errors="ignore")
    elif isinstance(data, bytes):
        return data.decode("ascii", errors="ignore")
    return str(data).encode("ascii", errors="ignore")

error_callback is pretty much useless. Maybe you planned on adding more functionality to the Parser class, but as is, you can just leave it out.
I don't expect a parser's constructor to automatically parse the data I pass. How about:

class Parser(HTMLParser):
    def __init__(self, data):
        self._data = data
        ...

    def parse(self):
        self._parsed = self.feed( ... )


Reduce logic in Web API controller I have the following scenario - i have a grid on web page that is populated with data.  User can check a check-boxes in grid and data is saved to DB on navigating to next page.  If they then navigate back to the page and remove a selection the data is deleted from the DB and also if the original selection is still checked the data is not saved to the DB twice.  It is a MVC 5 application with SQL server db using Entity Framework - this particular logic is contained in my Web API controller.

The following is my code for the API method:

 public HttpResponseMessage SaveCarModelSelect(CarModelSelectViewModel model)
    {
        var car = _carService.GetById(model.carId);

        var splitSelectedCars = model.SelectedCars.Split(',').Select(sValue => sValue.Trim()).ToList();

        // We only need to check if there are Car Models saved in the DB
        if (car.CarModels.Count > 0)
        {
            var carsToRemove = CarModelsToRemoveFromDb(car, splitSelectedCars);

            foreach (var carToRemove in carsToRemove )
            {
                var carIdToDelete = car.RemoveCar(carToRemove);
                _carService.Delete(carIdToDelete);
            }
        }

        foreach (var singleSelectedCar in splitSelectedCars)
        {
            var savedCarsInDb = car.CarModels.Select(c=> c.ModelId)
              .ToList();

            if (!savedCarsInDb.Contains(singleSelectedCar))
            {
                riskAppraisal.AddCar(singleSelectedCar);
            }
        }

        _carService.Update(car);

        return Request.CreateResponse(HttpStatusCode.OK);
    }

The private method for CarModelsToRemoveFromDb is below

    private static IEnumerable<string> CarModelsToRemoveFromDb (Car car, IEnumerable<string> splitSelectedCars)
    {
        var savedCarsInDb = car.CarModels.Select(c=> c.ModelId)
              .ToList();

        return savedCarsInDb.Except(splitSelectedCars, StringComparer.OrdinalIgnoreCase);
    }

I'm guessing has anyone got suggestions to improve this - all the functionality is working as expected.  Where I call the AddCar method I suppose the checking database logic could be wrapped in another if the .Count is > 0 but then if I had the AddCar(singleSelectedCar) outside that it would get added twice.  The Goal I am trying to get to is make my API method thin and separate out other logic into their own methods.

Any suggested improvements appreciated.

To clean up your controller I came up with this:

entire logic is moved to your CarService
method CarModelsToRemoveFromDb is also moved to CarService
CarServicehas two overloads for method Update

Methods for CarService:

private IEnumerable<string> CarModelsToRemoveFromDb (Car car, IEnumerable<string> splitSelectedCars)
{
    var savedCarsInDb = car.CarModels.Select(c=> c.ModelId)
        .ToList();

    return savedCarsInDb.Except(splitSelectedCars, StringComparer.OrdinalIgnoreCase);
}

public void Update(CarModelSelectViewModel model)
{
    var car = GetById(model.carId);
    var splitSelectedCars = model.SelectedCars.Split(',').Select(sValue => sValue.Trim()).ToList();

    if (car.CarModels.Count > 0)
    {
        var carsToRemove = CarModelsToRemoveFromDb(car, splitSelectedCars);

        foreach (var carToRemove in carsToRemove )
        {
            var carIdToDelete = car.RemoveCar(carToRemove);
            Delete(carIdToDelete);
        }
    }

    foreach (var singleSelectedCar in splitSelectedCars)
    {
        var savedCarsInDb = car.CarModels.Select(c=> c.ModelId)
            .ToList();

        if (!savedCarsInDb.Contains(singleSelectedCar))
        {
            riskAppraisal.AddCar(singleSelectedCar);
        }
    }

    Update(car);
}

public void Update(Car car)
{
    //your current logic
}

Controller:

public HttpResponseMessage SaveCarModelSelect(CarModelSelectViewModel model)
{
    _carService.Update(model);
    return Request.CreateResponse(HttpStatusCode.OK);
}

Logic that checks count of Car (car.CarModels.Count > 0) can also be separated out to private method, same for loop foreach (var singleSelectedCar in splitSelectedCars).

I hope it can give you some sense of direction.


Saving temporary file for plotting audio waveform I am developing a web application in Flask - Python.

My goal is to let the user upload an mp3/wav file. This file will (for the scope of this question) get processed by the server and a picture showing the waveform of the audio file will be shown to the user.

My concern is that I am not experienced with neither Flask nor web applications. I would like this audio file to be saved somewhere temporarily, then get deleted. Similarly, I would like the picture of the waveform to just be shown without being saved after the user sees it. This is because I expect all the mp3/wav files to be different - there is no point in saving them.

Right now I have this method that gets called when a user uploads file:

def handle_file(file):
  filename = secure_filename(file.filename)
  filepath = os.path.join(app.config['UPLOAD_FOLDER'], filename)
  file.save(filepath)
  mir.save_plot(filepath)
  os.remove(filepath)
  plot_filepath = PLOT_FOLDER + 'audio.png'
  return plot_filepath

As you can see, I first save the audio file to a directory in the server, then process it (mir.save_plot does that), then I remove it, and then I return the plot_filepath which will be used in the template to show the final plot picture.

The mir.py file is the following:

import librosa
import plot as plt

def save_plot(filename):
    y, sr = librosa.load(filename)
    plt.plot(y, 'audio', 'time', 'amplitude')

The plot.py file:

import matplotlib.pylab as plt

def plot(vector, name, xlabel=None, ylabel=None):
    plt.figure()
    plt.plot(vector)
    plt.xlabel(xlabel)
    plt.ylabel(ylabel)
    plt.plot()
    plt.savefig('static/plots/' + name)

I feel like my design is poor and there is a better way to do this. Am I right?

The rest of the code:

def allowed_file(filename):
  return '.' in filename and \
    filename.rsplit('.', 1)[1].lower() in ALLOWED_EXTENSIONS

@app.route('/', methods=['GET', 'POST'])
def index():
  if request.method == "GET":
    return render_template('index.html', request="GET")
  else:
    if 'file' not in request.files:
      flash('No file part')
      return redirect(request.url)
    file = request.files['file']
    if file.filename == '':
      flash('No selected file')
      return redirect(request.url)
    if file and allowed_file(file.filename):
      plot_filepath = handle_file(file)
    return render_template('index.html', request="POST", filepath=plot_filepath)

The allowed_file() and majority of index() code are from the Flask docs.

Instead of saving file you can create io.BytesIO object and use it instead of file.

Also

    def allowed_file(filename):
      return (
          '.' in filename
          and os.splitext(filename)[1].lower() in ALLOWED_EXTENSIONS
      )

Also maybe you should check content-type header and not only extension.


Parsing mathmatical functions I have finally finished a Ruby calculator project, which is not based on eval. Instead it parses input char by char. The project is hosted on GitHub.

However, I find a specific part of the program very annoying since whenever I have to add something I have to rewrite a portion of it again.

def trig
  case @look
  when 'c'
    match_all('cos')
    case @look
    when 'h'
      match_all('h(')
      value = cosh(calculate)
    when '('
      match('(')
      value = cos(calculate)
    else
      expected('cos() or cosh()')
    end
  when 's'
    match('s')
    if @look == 'q'
      match_all('qrt(')
      value = sqrt(calculate)
    else
      match_all('in')
      case @look
      when 'h'
        match_all('h(')
        value = sinh(calculate)
      when '('
        match('(')
        value = sin(calculate)
      else
        expected('sin() or sinh()')
      end
    end
  when 'r'
    match_all('root')
    base = get_number
    match('(')
    value = calculate ** (1.0/base)
  when 't'
    match_all('tan')
    case @look
    when 'h'
      match_all('h(')
      value = tanh(calculate)
    when '('
      match('(')
      value = tan(calculate)
    else
      expected('tan() or tanh()')
    end
  when 'l'
    match('l')
    case @look
    when 'n'
      match_all('n(')
      value = log(calculate)
    when 'o'
      match_all('og')
      if digit? @look
        base = get_number
      elsif @look == "("
        base = 10
      else
        expected("integer or ( ")
      end
      match('(')
      value = log(calculate, base)
    else
      expected('ln() or log()')
    end
  when 'e'
    match_all('exp(')
    value = exp(calculate)
  when 'a'
    match_all('arc')
    case @look
    when 'c'
      match_all('cos')
      case @look
      when 'h'
        match_all('h(')
        value = acosh(calculate)
      when '('
        match('(')
        value = acos(calculate)
      else
        expected('arccos() or arccosh()')
      end
    when 's'
      match_all('sin')
      case @look
      when 'h'
        match_all('h(')
        value = asinh(calculate)
      when '('
        match('(')
        value = asin(calculate)
      else
        expected('arcsin() or arcsinh()')
      end
    when 't'
      match_all('tan')
      case @look
      when 'h'
        match_all('h(')
        value = atanh(calculate)
      when '('
        match('(')
        value = atan(calculate)
      else
        expected('arctan() or arctanh()')
      end
    end
  else
    raise InvalidInput, "unexpected input: \"#{@look}\""
  end
  match(')')
  value
end

The @look variable will contain the next character. The match_all function will simply match the input character by character. For example, it would match c and if it works it continues to match o and so on, but if it doesn't it will output an error.

Here's a toy program to illustrate the separation of user input collection from the string matching that determines which calc to use, as we discussed in the comments.  Note that the class NamedCalculations has no knowledge of IO with the user.  All the IO is encapsulated in CmdLineCalculator.

When I find a match, I just evaluate it with the argument pi.  In your real application, you'd need to do further process to gather the user's input for the argument.

Also note how if we decide we want to change which calculations we support (one of the most likely changes for the application), it becomes a 1 line change, and the user input processing code doesn't need to be touched.

class NamedCalculations

  # a hash of named calcs
  # eg, {cos: Math.method(:cos), sin: Math.method(:sin) }
  def initialize(calc_definitions)
    @calc_definitions = calc_definitions
    @calc_names = @calc_definitions.keys.map(&:to_s)
  end

  def method_for(calc_name)
    calc_name = calc_name.to_sym
    raise "Invalid Calculation Name" unless @calc_definitions.key? calc_name
    @calc_definitions[calc_name]
  end

  def exact_match?(input)
    @calc_names.any? {|name| input == name}
  end

  def partial_match?(input)
    @calc_names.any? {|name| name.include? input}
  end

end

class CmdLineCalculator

  def initialize(named_calcs)
    @named_calcs = named_calcs
    @input = ''
  end

  def start
    begin
      system("stty raw -echo")
      collect_user_input while not calc_complete?
      show_answer
    ensure
      system("stty -raw echo")
    end
  end

  private

  def collect_user_input
    @input += next_input_char
    validate_input
    echo_last_char
  end

  def validate_input
    raise "Invalid Calculation" unless @named_calcs.partial_match? @input
  end

  def next_input_char
    STDIN.getc
  end

  def echo_last_char
    STDOUT.putc @input[-1]
  end

  def calc_complete?
    @named_calcs.exact_match? @input
  end

  def show_answer
    selected_method = @named_calcs.method_for(@input)
    value_at_pi = selected_method.call(Math::PI)
    STDOUT.puts "\nThe #{@input}(pi) = #{value_at_pi}"
  end

end

my_calcs = {cos: Math.method(:cos), sin: Math.method(:sin) }
client = CmdLineCalculator.new(NamedCalculations.new(my_calcs))
client.start


Avoid redundancy of HTML entities in .NET framework I have to store various HTML named character references for the following purposes:

Escaping special characters myStringBuilder.Replace("À", WebHelper.Agrave)
Insert spaces in legacy reports ReportButtons.Controls.Add(New LiteralControl(WebHelper.Space))

This is the way I used to store until now:

public static class WebHelper
{
    #region [ Fields ]

    public const string Agrave = "&Agrave;";

    public const string Space = "&nbsp;";

    #endregion
}

Is this Single-Generic-Helper-Class the best way to solve this problem? Should all my programs reference it?

Why not just use the Server.HtmlEncode function?

Here is a brief description of what it does:

  Any ASCII code character whose code is greater-than or equal to 0x80
  is converted to &#<number>, where <number> is the ASCII character
  value.

It also converts common ascii characters such as <, >, &, and " to their html character references.