Sum of absolute difference of values and corresponding indices of an array I am solving questions on arrays from here.

  Problem: You are given an array of N integers, A1, A2 ,…, AN. Return maximum value of:

  f(i, j) for all 1 ≤ i, j ≤ N.

  f(i, j) is defined as |A[i] - A[j]| + |i - j|, where |x| denotes
  absolute value of x.

  Solution approach:

  |A[i] - A[j]| + |i - j| gives us 4 cases:

Case1:|A[i] - A[j]| + |i - j| = -A[i]+ A[j] - i + j = -(A[i]+ i) + ( A[j] + j)
Case2:|A[i] - A[j]| + |i - j| = -A[i]+ A[j] + i - j = -(A[i] - i) + (A[j]-j)
Case3:|A[i] - A[j]| + |i - j| = A[i]- A[j] - i + j  = (A[i] - i) - (A[j] -j)
Case4:|A[i] - A[j]| + |i - j| = A[i]- A[j] + i - j  = (A[i] + i) - (A[j] + j)

  Cases 1 and 4 are equivalent and can be find as the difference between
  the max and min value of A[i]+i

  Cases 2 and 3 are equivalent and can be find as the difference between
  the max and min value of A[i]-i

How can I perfect this code?

def max_abs_diff(array):
    """ returns sum of absolute difference of values and corresponding indices of an array"""
    max1=[]#array to store maximum values given by A[i] + i
    min1=[]#array to store minimum values given by A[i] + i
    max2=[]#array to store maximum values given by A[i] - i
    min2=[]#array to store minimum values given by A[i] - i

    for i, elem in enumerate(array):

        max1.append(elem + i)
        min1.append(elem + i)
        max2.append(elem - i)
        min2.append(elem - i)

    return max(max(max1) - min(min1) , max(max2)-min(min2))

Test cases:

print max_abs_diff([2,2,2]) == (2)
print max_abs_diff([2,-2,2])== (5)
print max_abs_diff([-4,-2,-3,-4,-5]) == (6)
print max_abs_diff([-1]) == (0)
print max_abs_diff([-5, 1, -3, 7, -1, 2, 1, -6, 5]) == (18)
print max_abs_diff( [6, -3, -2, 7, -5, 2, 1, -7, 6]) == (20)
print max_abs_diff([-5, -2, -1, -4, -7]) == (8)

First of all, I want to say this is really well presented. It just looks neat, with docstrings and comments and such. If you really want to perfect it, check the Python conventions in PEP 8 and linked documents. For example,

  An inline comment is a comment on the same line as a statement. Inline comments should be separated by at least two spaces from the statement. They should start with a # and a single space.

and

  The docstring is a phrase ending in a period. It prescribes the function or method's effect as a command ("Do this", "Return that"), not as a description; e.g. don't write "Returns the pathname ...".

I also very much appreciate the case analysis that has gone into the code to define those four cases, and so to reduce the naively quadratic function to a linear one. (Refering back to comments, that case analysis would not be at all amiss in a comment just after the docstring explaining the internal logic of the function)

Because, per the code and your case analysis, max1 and min1 always contain the same data, you could make the code shorter and reduce its space requirements by only having one of those. (And likewise for max2 and min2) However, see the next section.

In terms of the code itself, I suggest getting rid of those arrays. If they are only filled so that you can reduce them to a single maximum or minimum value, you might as well just track the rolling maximum or minimum as you go. This definitely saves space and probably saves time.

I would name the rolling reductions max_sum and max_difference rather than max1 and max2.

In terms of testing examples, do look for pathological cases. One case that always deserves to be checked, although it's not always obvious what the behaviour should be, is the case of an empty list.

Finally, it is generally worth using Python 3 in preference to Python 2 for new code, and especially practice code.


Simple Form Starts Long Running Task and Watches Progress Want to level up my skills. It hits a php script that starts a long running export that can take few minutes to few hours. The export script outputs a progress file that this form's javascript uses for the progress bar.

How maintainable is this? What can I do to improve?

<link rel="stylesheet" href="/css/minified.css">

<form id="exportForm">
  <input id="submitBtn" class="btn btn-primary" type="submit" value="Start Export">
</form>

  <progress id="progressBar" class="" value="0"></progress>

Is there any reason you're using global variables rather than passing function arguments? e.g.

exportProgress = data; progressHandler();

instead of progressHandler(data); ?

It's a much better practice to avoid globals as much as possible.
Instead of polling for progress every second, you could subscribe to SSE (Server Side Events), thus getting the latest progress updates and no unneeded traffic.

In JS this amounts to:

window.onload = function (e) {
    var source = new EventSource("get_progress");
    source.onmessage = function(event) {
        try {
            const response = JSON.parse(event.data);
            progressHandler(response)
            if (response.completed) {
                source.close();
            }
        } catch (error) {
            console.log(error);
        }
    }
}

function progressHandler(response) {
     progressBar.val(response.progress / response.total);
     if(response.completed) reset();
}


Java Doubly Linked List Implementation I recently submitted an implementation of a singly linked list and tried to apply some of the suggestions on this implementation. I decided not to implement the List interface as I only wanted to practice what I believed to be the most important methods. I'm wondering how I could tidy this up some more and improve upon it.

Singly Linked List: Java Singly Linked List Implementation

DoublyLinkedList.java

public class DoublyLinkedList<T> {

    private class Node {

        private T data;
        Node next, prev;

        public Node() {
        }

        public Node(T data) {
            this.data = data;
        }
    }

    private Node head, last;
    private int size;

    public DoublyLinkedList() {
        this.size = 0;
    }

    public void addToBeginning(T data) {
        Node temp = new Node(data);
        if (head == null) {
            head = temp;
            last = head;
        } else {
            temp.next = head;
            head.prev = temp;
            head = temp;
        }
        size++;
    }

    public void add(T data) {
        Node temp = new Node(data);
        if (head == null) {
            head = temp;
            last = head;
        } else {
            last.next = temp;
            temp.prev = last;
            last = temp;
        }
        size++;
    }

    public void add(T data, int index) {
        Node temp = new Node(data);
        if (index < 0 || index > size) {
            throw new IndexOutOfBoundsException();
        } else if (index == 0) {
            addToBeginning(data);
        } else if (index == size) {
            add(data);
        } else {
            Node current = head;
            for (int i = 0; i < index; i++) {
                current = current.next;
            }
            Node previous = current.prev;
            previous.next = temp;
            temp.prev = previous;
            temp.next = current;
            current.prev = temp;
            size++;
        }
    }

    public T get(int index) {
        if (index < 0 || index >= size) {
            throw new IndexOutOfBoundsException();
        } else {
            Node current = head;
            for (int i = 0; i < index; i++) {
                current = current.next;
            }
            return current.data;
        }
    }

    public void delete(int index) {
        if (index < 0 || index >= size) {
            throw new IndexOutOfBoundsException();
        } else if (index == 0) {
            head = head.next;
            head.prev = null;
            size--;
        } else if (index == size - 1) {
            last = last.prev;
            last.next = null;
            size--;
        } else {
            Node current = head;
            for (int i = 0; i < index; i++) {
                current = current.next;
            }
            Node previous = current.prev;
            Node next = current.next;
            previous.next = current.next;
            next.prev = previous;
            size--;
        }
    }

    public int getSize() {
        return size;
    }

    public boolean isEmpty() {
        return size == 0;
    }
}

There is one thing missing: iteration.

There is no way for the user code to get all items stored in the list without doing get(int) for each item which will result in O(n^2) time complexity.

Building the ListIterator without remove support is simple enough you just need to have it keep a reference to the current node.


Reverse all lines in a file The script below should open a file, read and reverse every line, then overwrite the source file.

#include <stdio.h>
#include <fcntl.h>
#include <unistd.h>
#include <stdlib.h>
#include <string.h>
#include <sys/stat.h>

// reverses the order of characters in every line of SRC file

void validate_cmdline_args(int argc, char **argv) {
  if(argc != 2) {
    printf("Usage: %s SRC\n", argv[0]);
    exit(1);
  }
}

void reverse_string(char *str) {
  char *end = strchr(str, (int)'\0') - 1;
  char tmp;
  while(str < end) {
    tmp = *str;
    *str++ = *end;
    *end-- = tmp;
  }
}

void process_line(char *output_content, char *line) {
  reverse_string(line);
  strcat(output_content, line);
  strcat(output_content, "\n");
}

int main(int argc, char **argv) {
  validate_cmdline_args(argc, argv);

  char* file_content = NULL;
  char* output_content = NULL;
  char *file_content_free_pointer = NULL;

  int file_descriptor;
  if((file_descriptor = open(argv[1], O_RDONLY)) == -1)
    goto errno_error;

  struct stat file_stat;
  if(fstat(file_descriptor, &file_stat) == -1)
    goto errno_error;

  if((file_content = malloc((size_t)file_stat.st_size+1)) == NULL)
    goto errno_error;
  file_content_free_pointer = file_content;

  if(read(file_descriptor, file_content, (size_t)file_stat.st_size) == -1)
    goto errno_error;
  file_content[(int)file_stat.st_size] = '\0';

  if(close(file_descriptor) == -1)
    goto errno_error;

  if((output_content = malloc((size_t)file_stat.st_size+2)) == NULL)
    goto errno_error;
  output_content[0] = '\0';

  char* delim = "\n";
  char* line = NULL;

  while((line = strsep(&file_content, delim)))
    process_line(output_content, line);

  if((file_descriptor = open(argv[1], O_WRONLY | O_TRUNC)) == -1)
    goto errno_error;

  if(write(file_descriptor, output_content, (size_t)file_stat.st_size) == -1)
    goto errno_error;
  if(close(file_descriptor) == -1)
    goto errno_error;

  free(file_content_free_pointer);
  free(output_content);

  return 0;
errno_error:
  perror(NULL);
  free(file_content_free_pointer);
  free(output_content);
  return 1;
}

It's solely for fun and practicing Linux system programming, so I've decided not to divide it into smaller functions.

Here's what I find thought-provoking:

instead of reopening a file I'd prefer to open it in such a way that it becomes truncated just before writing,
this constant error-checking is a madness - can you propose a nicer solution?
I find this flow, with almost every single line beginning with if ... != NULL/-1 etc. a bit distracting
I'm not 100% sure if the code is devoid of off-by-one errors (ok, now I'm sure).

Let's get the broad picture right first.

Do you know Schlemiel the Painter? No? Well, read about him on Joel's blog.
Don't follow in his footsteps; they are too deep because he always restarts from the beginning.
You might want to take that opportunity to make your code tolerate zero-bytes.

Now, having gotten that off the agenda, some other gross wastefulness: you are allocating memory on the order of twice the files size. But you could easily avoid that: Only read in a line, transform it, write it to the output.

There you have two basic good choices:

Modify the file in-place, as your transformation does not affect how much space each line needs. You don't even need any additional disk-space.
Write the output to a temporary file, and then atomically update it by moving: rewrite existing file so that it gets replaced by new version atomically, only once fully written

Now, about reducing the tedium of repeating the exact same code. That's one of the (few) places where the C preprocessor shines:

#define TRY(cond) if(cond) {} else goto error;
// Use it
#undef TRY

Now lots of other small things:

Curiously, one normally doesn't extract a functions precondition-checking into another function, as then the precondition is no longer there to see, and anyway the boilerplate is normally longer than what was extracted: main → validate_cmdline_args.
Why do you use strchr to find the end of a string, instead of the better-known (and probably more efficient) strlen?
For completeness, casting the second argument, a character-literal, to int is completely useless because it's already an int.
There's no reason to define tmp in reverse_string outside the loop. Actually, wouldn't that be a good place for a for-loop?

void reverse_string(char* str) {
    for(char* end = str + strlen(str) - 1; str > end; ++str, --end) {
        char tmp = *str;
        *str = *end;
        *end = tmp;
    }
}

Strictly speaking, both your original and my replacement for reverse_string are UB if strlen(str) == 0, because that would create a pointer one before the string. That can be fixed by replacing the first 1 with !!*str or adding an if-condition, but it doesn't matter in practice.
It might make sense to output the filename given to your program on error. That might be useful for anyone trying to understand what went wrong, so perror(argv[1]).


Check PHP validation code I have made a form in HTML and the form data should be send via email.
For that purpose I have created a PHP script.

I did read a lot about the importance of form validation in order to prevent hackers on using the form to redirect to different website and other malicious use.

Can someone please check if I secured the form from hackers attacks?

Form:

            <form method="post" action="send_form.php">

                    Full name:
                    <input type="text" class="form-control" id="input-name" name="full_name" placeholder="Name Surname" required>

                    Email address:
                    <input type="email" class="form-control" id="input-email" name="email" placeholder="email@domain.xy" required>

                    Wedding date:
                    <input type="text" class="form-control" id="input-date" name="date" placeholder="dd/mm/yyy" required>

                    Location:
                    <input type="text" class="form-control" id="input-location" name="location" placeholder="Town, restaurant">

                    <button type="submit" class="btn btn-primary" name="submit">Submit</button>

            </form>

<?php
// check if form was submited
if (isset($_POST['submit'])) {
    $name = test_input($_POST['full_name']);
    $emailFrom = test_input($_POST['email']);
    $date = test_input($_POST['date']);
    $location = test_input($_POST['location']);

    $emailTo = "xxx";
    $subject = "Form was submited at xxx by: ".$emailFrom;
    $txt = "
        name: $name \n
        email: $emailFrom \n
        date: $date \n
        location: $location";

    if (mail($emailTo, $subject, $txt)) {
        // return to the main page after form submited
        header("Location: xxx");
    }
}
// remove empty spaces and convert html characters
function test_input($data) {
    $data = trim($data);
    $data = stripslashes($data);
    $data = htmlspecialchars($data);
    return $data;
}
?>

First of all you must understand that validation and sanitization are not synonyms. But completely different processes that have very little in common.
Whereas sanitization indeed does prevent the malicious use, validation is merely for your and/or your customer's convenience. A properly sanitized data do not require any validation to be safe.
Sanitization
Speaking of the former, there is not so much to sanitize in this form. At least from the list you was able to gather:

there is no HTML output, so it makes no sense convert html characters.
empty spaces do no harm whatsoever
stripslashes always has been useless in the business of preventing any malicious use, while nowadays it's absolutely useless at all

I don't know what harm could be done to the code like this - may be it's better to ask on the security-related site of the network. May be it would be useful to check the input data length and to validate the email address.
Validation
Is another matter. You could validate the email address, so it won't allow a bogus email. Same goes for the date - whether it is correct and could be understood.
But, as it was said before, it's entirely up to you, whether to bother with validation or not.


Validating Credit Card Numbers This is one of the puzzles I was trying to solve, where the question is to validate the credit card number with following characteristics:

  It must contain exactly 16 digits.
  It must start with a 4,5 or 6
  It must only consist of digits (0-9).
  It may have digits in groups of 4 , separated by one hyphen "-".
  It must NOT use any other separator like ' ' , '_', etc.
  It must NOT have  4 or more consecutive repeated digits.

I have go through some of the solutions here and here as well, but the characteristics for validating are different from what I was working on, hence posting this.

total_count = int(raw_input())
numbers_list = []
#refernece list to check the starting series
start_list = [4,5,6]

for count in range(total_count):
    numbers_list.append(raw_input())

#condition 1, validates the starting series
def val_start(num):
    if int(num[0]) in start_list:
        return True
    else:
        return False

#to check if individial elements of the list are of length=4
#4321-5555-67899-9991, splitted to ["4321","5555","67899","991"] which is invalid
def val_group(num):
    for val in num.split("-"):
        if len(val) != 4:
            return False

    return True

#condition 2, validates the length of the number
def val_len(num):
    check = num
    check2 = True
    if "-" in num:
        check = "".join(num.split("-"))
        check2 = val_group(num)

    if ((len(check) == 16) and check2):
        return True
    else:
        return False

#condition 3, validates if input consists only number
def val_isdigit(num):
    if not num.isdigit():
        for ch in num:
            if not (ch.isdigit() | (ch == "-")):
                return False
    return True

#condition 4, validates the repetition of any number for 4 consective times
def val_rep(num):
    res = "".join(num.split("-"))
    for i in range(len(res)):
        try:
            if (res[i] == res[i+1]):
                if (res[i+1] == res[i+2]):
                    if (res[i+2] == res[i+3]):
                        return False
        except IndexError:
           pass
    return True

for num in numbers_list:
    #returns all the values into a list
    result = [val_start(num), val_len(num),val_isdigit(num), val_rep(num)]
    if False in result:
        print("Invalid")
    else:
        print("Valid")

I am looking for an optimised way for the function val_rep(), which validates the repetition of any number for 4 consecutive times.

First of all, you should encapsulate your functionality in a function, unless you really want to use your code only once. At the moment you would have to duplicate your code somewhat just to check a second credit card. So instead let us write a function with a doc string:

def is_valid_card_number(sequence):
    """Returns `True' if the sequence is a valid credit card number.

    A valid credit card number
    - must contain exactly 16 digits,
    - must start with a 4, 5 or 6
    - must only consist of digits (0-9) or hyphens '-',
    - may have digits in groups of 4, separated by one hyphen "-".
    - must NOT use any other separator like ' ' , '_',
    - must NOT have 4 or more consecutive repeated digits.
    """

    #  do your stuff here

However, credit card numbers follow a fixed pattern, and therefore are a perfect candidate for regular expressions:

import re

PATTERN='^([456][0-9]{3})-?([0-9]{4})-?([0-9]{4})-?([0-9]{4})$'

This might seem arcane, so let us have a look at that pattern. Anything inside parentheses is considered a group. So we have four groups: ([456][0-9]{3}) and three times ([0-9]{4}). The hyphens in between have ? and are therefore optional. If they exist, they must occur once, so -- is not allowed.

The […] indicates are character class. Only characters inside that class are allowed at that point. The ^ indicates that we're describing the start of our string, not some point in the middle. So we have to start with 456. That requriement is now met. We're followed by 3 ({3}) digits. Our first group therefore consists of four digits, and so do the others. The $ makes sure that there is nothing left: our string has to stop there.

The re documentation provides more information.

We now have met most of our conditions:

we have 16 digits
we have groups of four
we only have digits or hyphens
we start with a 4, 5 or 6

We're missing the four consecutive digits. This is now tricky, since it's not clear whether the hyphens are part of the run. If they are only invalid four 4 character groups, we can simply check whether the current character * 4 is the complete group:

import re

PATTERN='^([456][0-9]{3})-?([0-9]{4})-?([0-9]{4})-?([0-9]{4})$'

def is_valid_card_number(sequence):
    """Returns `True' if the sequence is a valid credit card number.

    A valid credit card number
    - must contain exactly 16 digits,
    - must start with a 4, 5 or 6
    - must only consist of digits (0-9) or hyphens '-',
    - may have digits in groups of 4, separated by one hyphen "-".
    - must NOT use any other separator like ' ' , '_',
    - must NOT have 4 or more consecutive repeated digits.
    """

    match = re.match(PATTERN,sequence)

    if match == None:
        return False

    for group in match.groups:
        if group[0] * 4 == group:
            return False
    return True

If the repeated digits may also not span hyphens/groups it gets a little bit more complicated, but not too much. This is left as an exercise, though.

That being said: make sure to handle multiple numbers, not a single one. That way you will be able to check your code easier. But that depends. If it's a single use script, you probably don't have a need for a function.


Sieve of Eratosthenes prime number finder up to n in C++ I'be been learning c++ recently and as an exercise I wanted to write a program to find numbers up to n using sieve of Eratosthenes.

I'm new to c++ and programming in general so I'm not sure if this is wildly inefficient or not. I would love some feedback for my code performance wise or better programming practices in general.

#include <iostream>
#include <vector>
#include <cmath>
using namespace std;

int square(int a){
    return a*a;
}

int main(){

    cout << "Enter a number ";
    int MAXIMUM;
    cin>> MAXIMUM;
    int sqrtMaximum = 0;

    //populate allPrimes from 1-MAXIMUM
    vector<int> allPrimes(MAXIMUM);
    for(int i = 1; i<MAXIMUM; i++){
        allPrimes[i] = i;
    }

    //get the square root closest to MAXIMUM (>=)
    if(square(sqrt(MAXIMUM)) < MAXIMUM){
        sqrtMaximum = sqrt(MAXIMUM)+1;
    }
    else{sqrtMaximum = sqrt(MAXIMUM);}

    vector<int> primesOfSqrt = {2,3};

    //adding numbers to list of prime numbers under sqrt
    bool prime = true;
    for(int i = 3; i<sqrtMaximum; i+=2){
        prime = true;
        for(int x : primesOfSqrt){
            if(i % x == 0){
                prime = false;
                break;
            }
        }
        if(prime){
            primesOfSqrt.push_back(i);
        }
    }
    for(int x:primesOfSqrt){cout<<x<<"|||||";}cout<<endl;//print prime numbers under sqrt

    //removing multiples of the prime numbers found under the sqrt
    int factor = 0;
    allPrimes[1] = 0;
    for( int j : primesOfSqrt){
        factor = MAXIMUM/j;
        for(int x = 2; x <= factor; ++x){
            allPrimes[j*x] = 0;
        }
    }
    for(int x = 1; x<MAXIMUM; x++){
        if(allPrimes[x]!=0){cout<<allPrimes[x]<<'\t';}
    }
    char ch; cin>>ch;
}

I used 2 vectors to keep track of prime numbers, one for prime numbers less than the sqrt of n and another to remove multiples of those prime numbers, which I did by setting it to 0, but for bigger numbers i would be setting certain indexes to 0 multiple times, which would only increase the bigger the number gets. I'm not sure how to go about optimizing it or if using a different container would be best so any pointers there would be appreciated as well.

lastly I'm not sure if I am actually doing sieve of eratosthenes right, I'm pretty sure I am, but if I'm not I would like to know.

What you are doing here is first calculating primes under sqrt and then crossing out the multiples like sieve says. However the standard approach is to calculate primes as you go and cross out multiples of each prime :

vector<bool> isPrime(MAXIMUM, true);   //we only need bool, less memory usage

isPrime[0] = false;
isPrime[1] = false;

for (int i = 4; i < MAXIMUM; i+=2) { // even numbers are handled first for efficiency
    isPrime[i] = false;
}

for(int i = 3; i < MAXIMUM; i+=2){
    if (isPrime[i]) {
        //the usual is for(int j = i * 2; j < MAXIMUM; j += i) { but I am using a bit more optimized loop
        for(int j = i * i; j < MAXIMUM; j += 2 * i) {
            isPrime[j] = false;
        }
    }
}

for(int x = 1; x<MAXIMUM; x++){
    if(isPrime[x]){cout<< x <<'\t';}
}

This way code is much shorter and if you try it, also much faster.


Find part of string in array, replace entire row with that part of string if found Following is the string: (I'm getting that from config file so its not constant):

string sequence = "Concat({ACCOUNT_NUM},substring(FormatDate(yyyyMMddHHmmss,DateNow())
,2,12), GetLast(GetNextSequence(seq_relation),1))";

It contains multiple custom methods and I want them somewhere in the same order as they appear in the above string. Following is the strategy I applied:

string[] arbitrary = sequence .Split('(').ToArray();

string[] methodsNmore = arbitrary.Take(arbitrary.Length - 1).ToArray();

string[] array2 = methodsNmore.Where(strr => strr.Contains(',')).ToArray();

string[] methods = array2.Select(str => str.Substring(str.LastIndexOf
                                         (',') + 1, str.Length - str.LastIndexOf
                                         (',') - 1)
                                        ).ToArray();

for (int i = 0; i < methods.Length; i++)
{
   string row = Array.Find(methodsNmore, item => item.Contains(methods[i]));

   int ii = Array.IndexOf(methodsNmore, row);

   methodsNmore[ii] = methods[i];
}

The resulting array, methodsNmore, now contains only the names of methods in the same order as in above string sequence.

Is there any other elegant way of doing it?

You can use a regular expression:

string[] names =
  Regex.Matches(sequence, @"([A-Za-z_]\w*)\(").Cast<Match>()
  .Select(m => m.Groups[1].Value).ToArray();


Grab Facebook Graph share count and abbreviate it I have this code that requests the graph API from Facebook to respond with a shared count for a particular URL (could be multiple on the page). Graph returns a full number (135934) and I take that number and abbreviate it into more friendly numbers (135K). Basically I'm looking for any help on improving the code to be more concise or if there's a better way to do something then to learn how.

 Some Article

 Some Article 2

 Some Article 3

My first remark is that you're explicitely requiring jQuery for your code to execute but only use it for jQuery.getJSON(), while it may be used elsewhere.

So instead of:

        } else {
        var c = document.getElementsByClassName('box-counter');
        numb = c.length;
        for (var i = 0; i < numb; i++) {
            var tempURL = c[i].parentNode.attributes['data-url'].nodeValue;
            jQuery.getJSON('http://graph.facebook.com/'+tempURL,function(data) {
                theShares.push(data.shares);
                insertDuring();
            });
        }
    }

you may simply write:

        } else {
        jQuery('.box-counter').each( function(i) {
            jQuery.getJSON(
                'http://graph.facebook.com/' + this.parent().data('url'),
                function(data) {
                  theShares.push(data.shares);
                  insertDuring();
                }
            );
        }
    }

You may also replace this:

function insertDuring(){
    if(numb == theShares.length){
        var c = document.getElementsByClassName('box-counter');
        for (var i = 0; i < numb; i++) {
            // your internal code here...
        }
    }
}

by this simpler code:

function insertDuring(){
    jQuery('.box-counter').each( function(i) {
        // your internal code here, unchanged...
        this.innerHTML = theNewShares[i];
    }
}

Now regarding your abridgeNumbers() function:

function abridgeNumbers(theString,theLimit,theAbbrev,i,k){
    for (var j = 0; j < theLimit; j++) {
        if(typeof theNewShares[i] === 'undefined'){
            theNewShares[i] = theString.charAt(j);
        } else {
            if(k == true){
                theNewShares[i] = theNewShares[i] +"."+theString.charAt(j);
            } else {
                theNewShares[i] = theNewShares[i] + theString.charAt(j);
            }
        }
    };
    theNewShares[i] = theNewShares[i] + theAbbrev;
}

You can simplify the code using the ternary operator, so improving readability, like this:

function abridgeNumbers(theString, theLimit, theAbbrev, i, k) {
    for (var j = 0; j < theLimit; j++) {
        if(typeof theNewShares[i] === 'undefined') {
            theNewShares[i] = theString.charAt(j);
        } else {
            theNewShares[i] += (k ? "." :"") + theString.charAt(j);
        }
    }
    theNewShares[i] += theAbbrev;
}

(BTW you may notice that the ";" is not needed at the end of the for block)

In a more advanced way you may also increase performance. In the code above, each step executes the test about theNewShares[i], while it is useful only once. So you may deport it before the loop, using a short notation which ensures to define theNewShares[i] as empty string when it is yet undefined.

function abridgeNumbers(theString, theLimit, theAbbrev, i, k) {
    theNewShares[i] |= '';
    for (var j = 0; j < theLimit; j++) {
        theNewShares[i] += (k ? "." :"") + theString.charAt(j);
    }
    theNewShares[i] += theAbbrev;
}

Finally beyond those purely technical aspects you may look at my second answer below about how to enhance the "conversion" strategy to get friendly representation of the numbers.


Nesting loops on same array but skipping same element I'm having a bit of trouble trying to find a more Rubyist way to achieve the following. Essentially, I want to try and iterate over every element e and apply e.method(n) for every \$n \in \text{array}\$, \$n \ne e\$. In order to determine whether or not \$n = e\$, I'll have to use an index comparison (really just test for reference equality as opposed to functional equality).

arr = [413, 321, 654, 23, 11]
(0...arr.length).each do |outer_i|
    (0...arr.length).each do |inner_i|
        next if outer_i == inner_i
        arr[outer_i].apply arr[inner_i]
    end
end

This reeks of Java/C++ and I can tell that this is not the Ruby way, but I can't seem to find an alternative. Any ideas to improve its Ruby-ness? I was thinking of Array#product but I'm not sure where to go from there.

Note that you are just doing a permutation of two elements from a set, and there is an abstraction in the core for that, Array#permutation(n):

arr.permutation(2).each { |x, y| x.apply(y) }


Writing to MemoryStream with foreach As you can see here:

20% CPU in a 3 minutes and 20 seconds excessive 40 executions async/await at a time for 9700 times caused quite a bit of CPU usage.

Code:

public MemoryStream getMessageBody()
{
    #region Get MessageBody
    IEnumerable<BytesWraper> MessageBody = GetMessageBodySource();
    if (MessageBody == null) {
        return null;
    }
    #endregion
    #region Write MessageBody's Data to a MemoryStream so we can convert it later to a String
    MemoryStream memoryStream = new MemoryStream(ContentLength == -1 ? 0 : ContentLength);
    try {
        foreach (BytesWraper bytes in MessageBody) {
            memoryStream.Write(bytes.Value, 0, bytes.Length);
        }
    } catch (Exception) {
        return null;
    }
    #endregion
    if (ConnectionClosed()) {
        _request.Dispose();
    }
    MessageBodyLoaded = true;
    return memoryStream;
}

Is there a way here to reduce the amount of usage by this foreach call? Perhaps not need to do it at all?

All im then doing with the result is as follows:

return MessageBody != null ? CharacterSet.GetString(MessageBody.GetBuffer(), 0, (int)MessageBody.Length) : null

(where MessageBody is getMessageBody())

Have you tried something like this (?):

MemoryStream memoryStream = new MemoryStream(MessageBody.SelectMany(mb => mb.Value).ToArray());


Forming error messages from a multidimensional associative array I'm working with an input array, which is 3 levels deep, and creating error message strings accordingly in a separate output array.  Below is the code.

$e['invalid']['key'][] = 'a123';
$e['invalid']['key'][] = 'a456';
$e['invalid']['color'][] = 'red';
$e['missing']['key'][] = 'b72';
$e['missing']['color'][] = 'blue';
$e['missing']['color'][] = 'green';
echo '
' . print_r($e, 1) . '
';

function generateErrorMessages($e)
{
    $errors;
    foreach ($e as $type => $array)
    {
        foreach ($array as $key => $values)
        {
            foreach ($values as $v)
            {
                switch($type)
                {
                    case 'invalid':
                                    $errors[$type][$key][] = $v . ' is not a valid ' . $key;
                                    break;
                    case 'missing':
                                    $errors[$type][$key][] = $v . ' is a required ' . $key . ' - other functions have dependencies on it';
                                    break;
                    default:
                                    // do nothing
                }
            }
        }
    }
    return $errors;
}

$msgs = generateErrorMessages($e);
echo '
' . print_r($msgs, 1) . '
';

While this achieves the desired outcome, the nested foreach loops in the generateErrorMessages() method seem onerous and difficult to read.  Is there a more concise way to generate the error messages?

Here is a screenshot of the input array:

Here is a screenshot of the output array:

In my opinion, this code is all right.  As any explicit code, these loops are quite straightforward to follow. And making it more concise will likely make it more cryptic as well. I would only make a minor brush-up, removing the unnecessary default clause and make the proper indentation. Variable interpolation is also more natural to read, in my opinion.

Also, variables must be explicitly initialized, just a mention is not enough. If you need an empty array then you must initialize an empty array. Although in the present code the $errors variable doesn't exist before the loop, the code could evolve over time, and there could be the possibility that a variable with the same name will be used above. If you just mention it, as $errors; it will keep the previous value. If you initialize it as $errors = []; you will always know it's an empty array.

function generateErrorMessages($e)
{
    $errors = [];
    foreach ($e as $type => $array)
    {
        foreach ($array as $key => $values)
        {
            foreach ($values as $v)
            {
                switch($type)
                {
                    case 'invalid':
                        $errors[$type][$key][] = "$v is not a valid $key";
                        break;
                    case 'missing':
                        $errors[$type][$key][] = "$v is a required $key - other functions have dependencies on it";
                        break;
                }
            }
        }
    }
    return $errors;
}


How to speed up interval lookup for a piecewise defined function? At the innermost loop of my software is a lookup of a function value from a piecewise defined function with interpolation between the sample points.

Illustration:

*                                                *
   *                               *
          *      *
|--|------|------|--------X---------|-------------|

There are several different interpolation methods in between the sample points.

My current implementation consists of two arrays, one for the interval endpoints and one for the function values. Searching is done via std::upper_bound if the array is large and find_if if the arrays are small. Based on the access patterns the speed improved tremendously by adding a cache value for the previous positions interval and checking if it is still valid for the new position

[1%] if (x >= *cache && x < *(cache + 1)) {
    outside = false;
    position = cache;
} else if (x < *begin || x > *end) {
    outside = true;
} else {
    outside = false;

    if (n_ < 64) {
[3%]    position = std::find_if(begin, end, std::bind2nd(std::greater_equal<argument_type>(), x)) - 1;
    } else {
        position = std::upper_bound(begin, end, x) - 1;
    }
    cache = position ;
}

The lookup currently still takes about 5% of total execution time, which seems too much.
I have indicated costs of the most expensive lines which is the comparison with the cached position [1%] and the linear search [3%].

Is there a faster way to lookup the interval which contains X?

Edit

I have included the switch after profiling since Linear search can faster for small arrays (and there are a lot of small arrays).

Edit
It seems the std::find_if implemenation is actually faster than binary search after all.
I have made a test here: http://ideone.com/Fo5ZeW

#include <iostream>
#include <ctime>
#include <algorithm>
using namespace std;

double* binary_search8(double* arr, const double& x)
{
  if (x<arr[4]) {
    if(x<arr[2]) {
      if(x<arr[1]) {
        return arr;
      } else {
        return arr + 1;
      }
    }
    else { //2..3
      if(x<arr[3]){
        return arr + 2;
      } else {
        return arr + 3;
      }
    }
  } else { // 4..7
    if(x<arr[6]) {
      if(x<arr[5]) {
        return arr + 4;
      } else {
        return arr + 5;
      }
    } else { //6..7
      if(x<arr[7]) {
        return arr + 6;
      } else {
        return arr + 7;
      }
    }
  }
}

double* binary_search16(double* arr, const double& x)
{
    if (x<arr[8]) return binary_search8(arr, x);
    else return binary_search8(arr+8, x);
}

int main() {
double test[16] = {0, 10, 20, 30, 40, 50, 60, 70, 80, 90, 100, 110, 120, 130, 140, 150};

  vector<double> test_vals;
  size_t num_elem = 10000000;
  for (size_t i=0; i<num_elem; i++)
    test_vals.push_back(rand()%150);

  clock_t begin, end;
  double elapsed_secs;

  // Linear search
  double linavg = 0;
  begin = clock();
  for (size_t i=0; i<num_elem; i++) {
    linavg += *(find_if(test, test+16, bind2nd(std::greater_equal<double>(), test_vals[i]))-1);
  }
  end = clock();
  elapsed_secs = double(end - begin) / CLOCKS_PER_SEC;
  cout << "linear secs: " <<elapsed_secs << ", testval = " << linavg << std::endl;

  // Binary search
  double binavg = 0;
  begin = clock();
  for (size_t i=0; i<num_elem; i++) {
    binavg += *binary_search16(test, test_vals[i]);
  }
  end = clock();
  elapsed_secs = double(end - begin) / CLOCKS_PER_SEC;
  cout << "binary secs: " <<elapsed_secs << ", testval = " << binavg << std::endl;

  // Binary search 2
  double binavg2 = 0;
  begin = clock();
  for (size_t i=0; i<num_elem; i++) {
    binavg2 += *(std::upper_bound(test, test+16, test_vals[i])-1);
  }
  end = clock();
  elapsed_secs = double(end - begin) / CLOCKS_PER_SEC;
  cout << "binary2 secs: " <<elapsed_secs << ", testval = " << binavg2 << std::endl;

  return 0;
}

It'll take some testing to be entirely certainly, but a few possibilities to consider:

if (x >= *cache && x < *(cache + 1)) {

I'd try replacing this with:

if (static_cast<unsigned>(x-*cache) < static_cast<unsigned>(*(cache+1) - *cache)) {

[Technically, unsigned is a place-holder here. It should really be something like std::make_unsigned<input_type>::type. That is, if the input is (say) long long, you want unsigned long long, not just unsigned int.]

This basically replaces two comparisons (that must be executed serially, thanks to && imposing a sequence point) with one plus a little extra math. The math can typically be done in parallel, and the branch becomes more predictable as well.

if (x < *begin || x > *end) {
    outside = true;
} else {
    outside = false;

I'd at least try just assigning the result of the comparison:

outside = x < *begin || x > *end;

Again, you can try the same "trick" as above, and get something like:

outside = static_cast<unsigned>(x-*begin) <= static_cast<unsigned>(*end-*begin);

As before, this replaces two relatively unpredictable comparisons with one relatively predictable one.

if (n_ < 64) {
[3%]    position = std::find_if(begin, end, std::bind2nd(std::greater_equal<argument_type>(), x)) - 1;
    } else {
        position = std::upper_bound(begin, end, x) - 1;
    }

If you're spending significantly more time in the linear search than the binary search, I'd try reducing the cut-off between the two, perhaps to 32 instead of 64.

As with any micro-optimization, none of these is really guaranteed to be effective. It's even possible your compiler may be incorporating one or more of them into the code it generates, so they'll do no good at all, or even that its optimizer won't recognize the resulting patterns in the code as well, so they could actually hurt performance. The only real way to find out is some testing and profiling.


Script to clean up old files that should only run one instance I'm writing a bash script for deleting old files on a Linux server.  Here are some relevant facts about this script:

As currently written, it does work as expected.  However, it's entirely possible there's some edge case I haven't considered (which is part of my reason for posting here).
It will be run as root once a day, scheduled via cron.
Only one instance should run at a time.  If another instance is started, it should just exit.
It will delete any file whose mtime is over 30 days old, from the time of find's invocation.
Each run of the script will take a few hours.
The script will be running on a single CentOS 7 node, but it operates on a distributed file system exposed to hundreds of others.

Here is the actual script:

#!/usr/bin/env bash

NUM_PROCS=4
LIMIT_TIME=30 # days
PIDFILE="/tmp/cleanup.pid"
PID_NOW="$$"

function save-pid() {
    echo "$1" > "$PIDFILE"

    if [[ "$?" -ne 0 ]]; then
        # If we couldn't save the PID to the lockfile...
        echo "Failed to create PID file for PID $1 in $PIDFILE; exiting"
        exit 1
    fi
}

function clean-up() {
    rm "$PIDFILE"
}

cd /scratch

if [[ -f "$PIDFILE" ]]; then
    # If the lock file exists... (the script is either running another
    # instance or improperly exited earlier)
    read PID_SAVED < "$PIDFILE" # Get the PID of the running cleanup script

    if ps --pid "$PID_SAVED" &> /dev/null; then
        # If a PID with this process exists...
        echo "Cleanup script is already running with PID $PID_SAVED; exiting"
        exit 1
    else
        save-pid "$PID_NOW"
    fi
else
    save-pid "$PID_NOW"
fi

trap clean-up HUP INT QUIT ILL ABRT BUS SEGV PIPE TERM ERR
# If any further command raises a problematic signal or exits in error,
# clean up $PIDFILE on the way out

ls | xargs -P "$NUM_PROCS" -I{} \
    find {} -type f -mtime "+$LIMIT_TIME" -delete

clean-up

I'll consider any advice you have to offer, but I'm specifically curious about how I can improve my file locking and error handling.

General impressions - nice neat code, well commented.  It's good that you use trap to clean up on exit.

Shellcheck picks up a couple of oversights:

206596.sh:11:11: note: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. [SC2181]
206596.sh:22:1: warning: Use 'cd ... || exit' or 'cd ... || return' in case cd fails. [SC2164]
206596.sh:27:5: note: read without -r will mangle backslashes. [SC2162]
206596.sh:44:1: warning: Use 'find .. -print0 | xargs -0 ..' or 'find .. -exec .. +' to allow non-alphanumeric filenames. [SC2011]

Let's do a walk-through:

#!/bin/bash

Why bash?  I don't think there's anything here that can't be achieved with standard POSIX shell.
NUM_PROCS=4

What's special about 4?  If it's the number of cores you have available, then we could automate that:

NUM_PROCS=$(getconf _NPROCESSORS_ONLN)

PIDFILE="/tmp/cleanup.pid"

I'm not sure about CentOS, but on the platforms I'm more familiar with, pidfiles normally live under /run (or /var/run on older installations).
PID_NOW="$$"

Never modified; I think it's clearer to simply use $$ where it's needed.
echo "$1" > "$PIDFILE"

if [[ "$?" -ne 0 ]]; then

Instead of testing $?, we should use the result directly (and since we never call this with anything other than our own pid, let's just embed that in the function rather than passing as argument).  Give the function a POSIX-compliant name, and declare it without the deprecated function keyword:

save_pid() {
    if ! echo $$ >"$PIDFILE"
    then
        # If we couldn't save the PID to the lockfile...
        echo >&2 "Failed to create PID file for PID $$ in $PIDFILE; exiting"
        exit 1
    fi
}

Note that the message is printed to standard error (stream 2) rather than standard output.
cd /scratch

There's no checking that this command succeeded.  We could append || exit 1 or similar, but we can catch more errors by setting shell flags -e and -u at the beginning of the script.

There's really no need to change working directory at all; we could just omit this command, and use the directory name where we need it.
if [[ -f "$PIDFILE" ]]

The comment says we're testing for existence, so we should be using -e (or change the comment).  And we can use the standard [ command; there's nothing to be gained from Bash-specific [[.
    read PID_SAVED < "$PIDFILE"

Shellcheck complains about using read without -r option.  Whilst we don't expect to see shell escapes in this file, there's no harm in pacifying the warning.  Or, we could just interpolate the file directly:

if ps --quick-pid "$(cat $PIDFILE)" &> /dev/null

Or just test for the existence in procfs:

if [ -e "/proc/$(cat $PIDFILE)" ]

We could add further tests to ensure that we don't have a re-used pid that's running a different program, but I won't pursue that here.
This structure can be simplified:

if a
then
    if b
    then exit 1
    else do-c
    fi
else do-c
fi

It's exactly equivalent to

if a && b
then exit 1
fi
do-c

That simplification gives us:

if [ -e "$PIDFILE" ] && [ -e "/proc/$(cat $PIDFILE)" ]
then
    # A process exists with our saved PID
    echo >&2 "Cleanup script is already running with PID $PID_SAVED; exiting"
    exit 1
fi

save_pid

trap clean-up HUP INT QUIT ILL ABRT BUS SEGV PIPE TERM ERR

Instead of listing all these signals, we just want to clean up however the shell exits:

trap clean_up EXIT

ls | xargs -P "$NUM_PROCS" -I{} \

It's dangerous to pipe the output of ls like that.  Files not under your control can contain almost anything, including newlines and other significant space.  I don't think we really want to ignore dot-files, either.  I recommend find -maxdepth 1 -mindepth 1, which can then write safely to xargs using --print0.  We might want to eliminate symlinks from the listing, although we know that the inner find won't follow them without -H or -L.
save_pid and clean_up are only used once each now, so could be usefully inlined.

Modified code

#!/bin/sh

set -eu

LIMIT_TIME=30 # days
NUM_PROCS=$(getconf _NPROCESSORS_ONLN)
PIDFILE="/var/run/cleanup.pid"

# check pidfile
if [ -e "$PIDFILE" ] && [ -e "/proc/$(cat $PIDFILE)" ]
then
    # A process exists with our saved PID
    echo >&2 "Cleanup script is already running with PID $PID_SAVED; exiting"
    exit 1
fi

# write pidfile
    if ! echo $$ >"$PIDFILE"
then
    # If we couldn't save the PID to the lockfile...
    echo >&2 "Failed to create PID file for PID $$ in $PIDFILE; exiting"
        exit 1
    fi

trap 'rm "$PIDFILE"' EXIT

# Now start working
find /scratch -maxdepth 1 -mindepth 1 \! -type l --print0 \
     | xargs -0 -P "$NUM_PROCS" -I{} \
             find {} -xdev -type f -mtime "+$LIMIT_TIME" -delete

After all that, consider that you might be reinventing a wheel.  Most likely, you have a tmpreaper package available that can be configured to do exactly this task.


ActiveRecord object for billing and shipping addresses I have an Order object that belongs_to a BillingAddress and a ShippingAddress. I want to present my user with only ShippingAddress fields and a checked checkbox indicating that the billing address matches the shipping address. If the user unchecks the box, the BillingAddress fields appear.
This design proved painful to implement, and my Order object has a lot of callbacks I would rather not have.
class Order < ActiveRecord::Base
  attr_accessor :bill_to_shipping_address

  belongs_to :billing_address,  class_name: 'Address'
  belongs_to :shipping_address, class_name: 'Address'

  accepts_nested_attributes_for :billing_address, :shipping_address

  after_initialize  :set_billing_to_shipping_address
  before_validation :set_billing_address
  after_validation  :clear_billing_address_errors

  # Init the object with option checked
  def set_billing_to_shipping_address
    self.bill_to_shipping_address ||= '1'
  end

  # Copy shipping address attrs to billing address
  def set_billing_address
    self.billing_address = self.shipping_address if bill_to_shipping_address?
  end

  def bill_to_shipping_address?
    bill_to_shipping_address == '1'
  end

  # If shipping address matches billing, we copy the attrs, and thus duplicate errors too.
  # We only need to show the user one set of errors if addresses are the same, so remove them for billing address.
  def clear_billing_address_errors
    if bill_to_shipping_address?
      self.errors.messages.each { |k,v| self.errors.messages.delete(k) if k.to_s.split('.')[0] == 'billing_address' }
    end
  end
end

This feels smelly, and I have four methods along with three registered callbacks  to satisfy this need. I have no logic in the controller and the form is relatively simple.
= form_for @order do |f|
    # ...
    = f.label :bill_to_shipping_address, class: 'checkbox' do
      #{f.check_box :bill_to_shipping_address} Use my shipping address as my billing address.

This doesn't feel right. I'm also hacking around the error messages.
Questions:

How can I improve my design?

Would switching the relationships help?
Order has_one :billing_address and has_one :shipping_address instead of belongs_to. Nested forms will feel more natural; in that, a parent creates children, not the other way around.

I'm reading a fair bit of refactoring books, but I can never map their examples to my own object design. I'm not that experienced I guess.

Solution still relying on callbacks

validations in rails accept a "condition" option:

validates :shipping_address, presence: true, unless: :ship_to_billing_address?

for simplicity and consistency, i would advise to use a "real" attribute instead of a virtual one for ship_to_billing_address (attributes accessor generated from boolean columns integrate nicely with checkboxes, and rails automatically generates a ship_to_billing_address? method).

In order to ease access to the shipping address, you could either override the shipping_address method :

def shipping_address
  ship_to_billing_address? ? billing_address : super
end

...or create another method, if you fear overrides. Problem is, this approach could induce confusion (modifying in place a shipping address that is in fact a billing address without knowing it, for instance)

Solution without callbacks

Have an eye on DCI architecture. Some gems implement it. Using multiple form objects with different validation rules is another option.


Saving the values of all variabled declared in a block Often when running exprimental code to empirically investigate an algorithm, you tweak a parameter, then rerun the code, and generate a bunch of saved models for various parameters.

To make sure I don't loose what the parameters were set to I defined a macro that saves there values.

module Util
using JLD

export @param_save #...

function names_candidates(blk::Expr)
    names_in_block = Vector{Symbol}()
    for a in blk.args
        typeof(a) <: Expr || continue
        if a.head == :(=)
            push!(names_in_block, a.args[1])
        else #Recurse, so we captured things in blocks or behind `const`
            append!(names_in_block, names_candidates(a))
        end
    end
    names_in_block
end

macro param_save(filename, blk::Expr)
    names_in_block =  names_candidates(blk)
    quote
        $(esc(blk))
        names_defined = Set($(names_in_block)) ∩ Set(names(current_module()))
        names_and_vals =[(string(name), eval(name)) for name in names_defined]
        println("Paramaters -- saving to $($filename)")
        println("----------")
        println(join(("$n = $v" for (n,v) in names_and_vals),"\n"))
        JLD.save($filename, Base.flatten(names_and_vals)...)
        println("----------")
    end
end
#...
end #module

Usage:

using Util

base_name = "paper_v1"
param_save_fn =  "../models/adagram/$(base_name).params.jld"
output_fn = "../models/adagram/$(base_name).adagram_model"#"file to save the model (in Julia format)"
@assert !isfile(output_fn)
@param_save param_save_fn begin
        nprocessors = nprocs()
        train_fn  =  "../data/corpora/WikiCorp/tokenised_lowercase_WestburyLab.wikicorp.201004.txt" #"training text data"
        output_fn = output_fn #file to save the model (in Julia format)"
        dict_fn = "../data/corpora/WikiCorp/tokenised_lowercase_WestburyLab.wikicorp.201004.1gram" #"dictionary file with word frequencies"

        window = 10 #"(max) window size" C in the paper
        min_freq  = 20 #"min. frequency of the word"
        remove_top_k = 0 #"remove top K most frequent words"
        dim  = 300 #"dimensionality of representations"
        prototypes = 5 #"number of word prototypes" T in the paper
        alpha = 0.15 #"prior probability of allocating a new prototype"
        d  = 0.0 #"parameter of Pitman-Yor process" D in paper
        subsample = 1e-5 #"subsampling treshold. useful value is 1e-5"
        context_cut  = true #"randomly reduce size of the context"
        epochs = 1 #"number of epochs to train"
        initcount = 1. #"initial weight (count) on first sense for each word"
        stopwords = Set{AbstractString}() #"list of stop words"
        sense_treshold = 1e-10 #"minimal probability of a meaning to contribute into gradients"
        save_treshold = 0.0 #"minimal probability of a meaning to save after training"
end

Output:

Paramaters -- saving to ../models/adagram/paper_v1btempexample.params.jld
----------
train_fn = ../data/corpora/WikiCorp/tokenised_lowercase_WestburyLab.wikicorp.201004.txt
alpha = 0.15
subsample = 1.0e-5
sense_treshold = 1.0e-10
initcount = 1.0
nprocessors = 1
epochs = 1
window = 10
output_fn = ../models/adagram/paper_v1btempexample.adagram_model
min_freq = 20
prototypes = 5
context_cut = true
stopwords = Set{AbstractString}()
remove_top_k = 0
save_treshold = 0.0
dict_fn = ../data/corpora/WikiCorp/tokenised_lowercase_WestburyLab.wikicorp.201004.1gram
dim = 300
d = 0.0
----------

Writing code with macros is hard.
It took me a few shots to get the escaping all right.

Is this reasonable code?
AFAICT julia doesn't have a method like pythons locals() that lists all variables in a given scope.
So I have to work around that by finding possible variables in the AST then cross referencing that against all variables in the module.

Since this will be used at toplevel anyway, I would just wrap all the parameters in a module.

macro param_save(file, blk)
    modname = gensym()
    Expr(:toplevel,
        Expr(:using, :JLD),
        Expr(:module, false, modname, esc(blk)),
        quote
            JLD.save($file, Dict(
                string(n) => eval($modname, n)
                for n in names($modname, true)
                if n ≠ $(Meta.quot(modname))))
        end)
end


Light Security Local-Machine Password Encryption I have a program that makes a call to an SQL database. I cannot use SSH to connect to it (as far as I am aware) or any other more secure method, so Java's SQL connector was used. As the connection requires a username/password, and I want to store the user/pass for ease of making the connection. I knew I was not going to store the password in plain text, but I couldn't find a method of doing so in a secure manner in a platform-independent way in core Java, so I rolled my own (not advised in crypto, I know).

Below is the code that encrypts a message of any length with a key of any length.

private static String xor(String message, String key){
    if(message.isEmpty())
        return "";
    byte[] xorBlob=new byte[message.length()];
    long seed=0;
    for(char c: key.toCharArray())
        seed+=c;
    new Random(seed).nextBytes(xorBlob);
    char[] mArr=message.toCharArray();
    for(int i=0;i<mArr.length;i++)
        mArr[i]=(char)(mArr[i]^xorBlob[i]);
    return new String(mArr);
}

To put the above code into words:
It sums up all the char values of the given key into an int. That int is then used as the seed for the Random function, which then spits out an array of bytes the length of the message. The message and that byte array are then xor'd together.

It's rudimentary, but I figured for opt-in local storage of a password for the current user, it should suffice.

Is this a good method of doing so? Is there a way I can improve this code, for either more security or just better code to more fit industry standards?

EDIT:
Here's the code that calls the above function:

FileWriter writer=new FileWriter(config);
writer.write(String.format("username=%s%n", username));
writer.write(String.format("password=%s%n", xor(password, username)));

I'd suggest using built-in libraries, especially if you're looking to solve something for production/industry. Quick search shows that there are two to consider that should be available within JDK, the JCA (Java Cryptography Architecture) for local storage of secrets, and JSSE (Java Secure Socket Extension) for comunicating with the remote service.

    The JCA is a major piece of the platform, and contains a "provider" architecture and a set of APIs for digital signatures, message digests (hashes), certificates and certificate validation, encryption (symmetric/asymmetric block/stream ciphers), key generation and management, and secure random number generation, to name a few...

    The Java Secure Socket Extension (JSSE) enables secure Internet communications. It provides a framework and an implementation for a Java version of the SSL, TLS, and DTLS protocols and includes functionality for data encryption, server authentication, message integrity, and optional client authentication...

Utilizing the documentation for local encryption, and examples scattered about, may cause one to write code similar to...

//  Helpful resources
// https://docs.oracle.com/javase/8/docs%2Ftechnotes%2Fguides%2Fsecurity%2Fcrypto%2FCryptoSpec.html#PBEEx
// https://stackoverflow.com/questions/4644415/java-how-to-get-input-from-system-console
// https://www.tutorialspoint.com/check-if-a-string-is-empty-or-null-in-java
// https://stackoverflow.com/questions/18142745/how-do-i-generate-a-salt-in-java-for-salted-hash

import javax.crypto.spec.PBEKeySpec;
import javax.crypto.spec.PBEParameterSpec;
import javax.crypto.SecretKeyFactory;
import javax.crypto.Cipher;
import javax.crypto.SecretKey;

import java.security.SecureRandom;

import java.io.Console;

class pass_enc {

  public static void main(String[] args) throws Exception {
    Console console = System.console();
    if (console == null) {
      System.out.println("Unable to fetch console!");
      return;
    }

    PBEKeySpec pbeKeySpec;
    PBEParameterSpec pbeParamSpec;
    SecretKeyFactory keyFac;

    // Salt
    byte[] salt = new SecureRandom().nextBytes(salt);

    // Iteration count
    int count = 1000;

    // Create PBE parameter set
    pbeParamSpec = new PBEParameterSpec(salt, count);

    // Prompt for encryption password
    // Use char array because String objects are immutable
    // Convert to SecretKey object using PBE key factory
    char[] password = console.readPassword("Enter a passphrase: ");
    // char[] password = System.console.readPassword("Enter a passphrase: ");
    pbeKeySpec = new PBEKeySpec(password);
    keyFac = SecretKeyFactory.getInstance("PBEWithHmacSHA256AndAES_256");
    SecretKey pbeKey = keyFac.generateSecret(pbeKeySpec);

    // Create PBE Cipher
    Cipher pbeCipher = Cipher.getInstance("PBEWithHmacSHA256AndAES_256");

    // Initalize PBE Cipher with key and parameters
    pbeCipher.init(Cipher.ENCRYPT_MODE, pbeKey, pbeParamSpec);

    // Cleartext to be encrypted
    byte[] cleartext = "This is an example".getBytes();

    // Encrypt cleartext
    byte[] ciphertext = pbeCipher.doFinal(cleartext);

  }

}

... though the compiler will complain about salt being void; Java isn't a language that I write a whole lot in but perhaps the above code will get ya a little closer to something that preforms the necessary steps more securely. Edits and comments are certainly welcomed if someone does figure out how to get Oracle's example code working as intended.


Probability suite for an array of elements I wrote a function that can be used to generate a probability distribution for an array of elements (probGen()), and another that can be used to select an element from an array with a specified probability (probSelect()). I want to optimise my code.

probGen.php

<?
    require_once("randX.php");  #"randX()" generates a random floating point number in a specified range.
    error_reporting(E_ERROR | E_WARNING | E_PARSE);

    function probGen(array $arr, float $control = 0.01)
/*
*   Generates a valid, random probability distribution for a given array of elements, that can be used in conjunction with "probSelect()".
*   Input:
        $arr: An array of elements.
        $control: A value that decides how much mass is allowed to be unilaterally dumped onto one element. A high value would permit distributions where most of the mass is concentrated on one element.
        If an invalid value is provided, the default is used.
*   Output: An associative array where the keys are the elements in the original array, and the values are their probabilities.
*/
    {
        $control = ($control <= 1 && $control >= 0)?($control):(0.00001);   #Use the default value if an invalid number is supplied.
        static $result = [];    #Initialises $result with an empty array on first function call.
        static $max = 1;    #Initialises $max with 1 on first function call.
        foreach ($arr as $value)
        {
            $x = randX(0, $max);    #Random probability value.
            $result[$value] = ($result[$value] + $x)??0;    #Initialise the array with 0 on first call, and on subsequent calls increment by $x to assign probability mass.
            $max -= $x;     #Ensures that the probability never sums to more than one.
        }
/*
*   After the execution of the above code, there would be some leftover probability mass.
*   The code below adds it to a random element.
*/
        $var = array_values($arr);
        if($max <= $control)    #To limit concentration of most of the probability mass in one variable.
        {
            $result[$var[rand(0,(count($var)-1))]] += $max; #Selects a random key and adds $max to it.
            return $result;
        }
        else
            return probGen($arr, $control);
    }
?>

probSelect.php

<?
    require_once("confirm.php");

    function probSelect(array $arr)
/*
*   A function to select an element from an array with indicated probabilites.
*   Input: An associative array whose keys are the elements to be selected from, and whose values are the associated probabilities.
*   Output: The selected element, or "NULL" if an invalid probability distribution was supplied.
*/
    {
        if(confirm($arr))
        {
            $var = lcg_value(); #The random float that would be used to select the element.
            $sum = 0;
            foreach ($arr as $key => $value)
            {
                $sum += $value;
                if($var <= $sum)
                    return $key;
            }
        }
        else
        {
            print("ERROR!!! The supplied probability distribution must sum to 1. <br>");
            return null;
        }
    }
?>

Dependencies

The required functions in case they are necessary for evaluating performance:

randX.php

<?
    function randX(float $a, float $b): float
/*
*   Generates a random number between between two real numbers (both inclusive).
*   Input: Two floating point numbers.
*   Output: A random floating point number.
*/
    {
        $max = max($a, $b);
        $min = min($a, $b);
        $x = $max - $min;
        $y = lcg_value()*$x;
        return ($min+$y);
    }
?>

confirm.php

<?
    function confirm(array $arr)    #Confirms that the supplied array has a valid probability distribution
    {
        return (array_sum($arr) == 1)?true:false;
    }
?>

A few little things:

Don't put comments in between function declarations and curly braces (or anything similar to that like classes or methods)

That's hard to read and an eye sore. Put your function comments above the function itself. Ideally using standard docblock format.

/**
* Generates a valid, random probability distribution for a given array of elements, that can be used in conjunction with "probSelect()".
*
* @param   array    $arr        An array of elements
* @param   float    $control    A value that decides how much mass is allowed to be unilaterally dumped onto one element. A high value would permit distributions where most of the mass is concentrated on one element.
* @return  array                An associative array where the keys are the elements in the original array, and the values are their probabilities.
*/
function probGen(array $arr, float $control = 0.01)
{

Use better variable names

What does $arr mean? I can guess that it is probably an array but my IDE can already tell me that. What's actually in that array? $elements would be a better name based on the function comments. A better name would be $elementsOfSomething where something accurately describe the characteristic of those elements.

Always use curly braces for control structures

Although it is perfectly valid syntax to omit curly braces when a control structure only contains one line of code. But it is a best practice to always use them as they make the code more rreadable and prevent future errors. Future you or another developer may want to add a line to a control block and introduce a hard to find bug because they didn't realize the curly braces weren't there.

if($var <= $sum)
    return $key;

should be:

if($var <= $sum) {
    return $key;
}

Use echo over print()

print() is an alias of echo but there are minor differences between the two. Although they don't come into play here, it is the PHP convention to use echo for outputting content.

Don't output content from your functions

Your functions that do work and output content but only when there is an error are inconstant and doing too much. If you have an error, let the function report that back through a special return value (like false or null) or by throwing an exception. Let the calling code worry about reporting back the error and let the function focus on doing one thing and one thing only (just like a good OOP class should be doing).

You can simply statements that check for a Boolean to return a Boolean

Your statement return (sum($arr) == 1)?true:false; is checking if a statement is true and returning true if it is. false if it is false. So you can return the result of your conditional without having to explicitly return true or false because you already have that value:

return (sum($arr) == 1); // will return true or false

An alternative randx() function

The internet seems like it is already full of example functions that will generate a random float. They tend to be similar to the one below. Is there any reason you did not choose to go this route? It has fewer function calls so it should be more performant.

function frand($min, $max, $decimals = 0) {
  $scale = pow(10, $decimals);
  return mt_rand($min * $scale, $max * $scale) / $scale;
}

echo "frand(0, 10, 2) = " . frand(0, 10, 2) . "\n";

The PHP community prefers // to # for comments

Although # is a valid syntax for a one line comment in PHP, it is common practice to use //.


Java => Caesar Cypher I already asked for help in optimizing a simple Caesar Cypher code in java here : Cesar code in Java
I tried to implement all the advice from that subject and now I am asking again.
Do you have any other advice to optimize this code ? What can be improved ?

Here is the code, thank you in advance :

Cypher.java :

package fr.thomascloarecd.cypher;

import java.util.Scanner;

public class Cypher {

    private static Scanner sc = new Scanner(System.in);

    private static String cypherMethod = "";

    public static void main (String[] args){
        while(!(cypherMethod.equals("stop") || cypherMethod.equals("2"))) {
            System.out.println("Choose a cypher's method and type it :" +
                    "\n (1) - caesarCypher" +
                    "\n (2) - stop => To stop the program");

            cypherMethod = sc.nextLine().toLowerCase();

            if (cypherMethod.equals("1") || cypherMethod.equals("caesarcypher")) {

                String encryptOrDecrypt = "";
                while(!(encryptOrDecrypt.equals("back") || encryptOrDecrypt.equals("3"))) {
                    System.out.println("\nWhat do you want to do (type it) :" +
                            "\n (1) - encrypt" +
                            "\n (2) - decrypt" +
                            "\n (3) - back => To go back in the cypher's method summary");

                    encryptOrDecrypt = sc.nextLine().toLowerCase();

                    if (encryptOrDecrypt.equals("1") || encryptOrDecrypt.equals("encrypt")) {
                        System.out.println("\nThen, what's the sentence that you want to encrypt ?");

                        CaesarCypher caesarcypher = new CaesarCypher(sc.nextLine(), encryptOrDecrypt);

                        System.out.println("\nEncrypted with caesar cypher : "+caesarcypher.translatedSentence());

                    } else if (encryptOrDecrypt.equals("2") || encryptOrDecrypt.equals("decrypt")) {
                        System.out.println("\nThen, what's the sentence that you want to decrypt ?");

                        CaesarCypher caesarcypher = new CaesarCypher(sc.nextLine(), encryptOrDecrypt);

                        System.out.println("\nDecrypted with caesar cypher : "+caesarcypher.translatedSentence());
                    }
                }
            } else if (cypherMethod.equals("2") || cypherMethod.equals("stop")) {
                System.exit(0);
            }
        }
        sc.close();
    }
}

CaesarCypher.java :

package fr.thomascloarecd.cypher;

import java.text.Normalizer;

class CaesarCypher {
    private String userSentence;
    private String encryptOrDecrypt;

    CaesarCypher(String pUserSentence, String pEncryptOrDecrypt){
        userSentence = Normalizer.normalize(pUserSentence, Normalizer.Form.NFD).replaceAll("[^\\p{ASCII}]", "").toLowerCase();
        encryptOrDecrypt = Normalizer.normalize(pEncryptOrDecrypt, Normalizer.Form.NFD).replaceAll("[^\\p{ASCII}]", "").toLowerCase();
    }

    String translatedSentence(){
        char[] ALPHABET = {'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z'};

        StringBuilder translatedSentence = new StringBuilder();

        for(int i = 0; i <= userSentence.length()-1; i++){
            for(int j = 0; j < ALPHABET.length; j++) {
                if (userSentence.charAt(i) == ALPHABET[j]) {
                    translatedSentence.append(ALPHABET[(26 + j + integerDependOnAction()) % 26]);
                    break;
                } else if (j == 25) {
                    translatedSentence.append(userSentence.charAt(i));
                    break;
                }
            }
        }
        return translatedSentence.toString();
    }

    private int integerDependOnAction() {
        if (encryptOrDecrypt.equals("encrypt") || encryptOrDecrypt.equals("1")) {
            return 1;
        } else if (encryptOrDecrypt.equals("decrypt") || encryptOrDecrypt.equals("2")) {
            return -1;
        }
        return 0;
    }
}

My first observation was - without actually reading anything - WOW that's a lot of code for such a simple task.

Then after the first read-through I was confused because I couldn't say if this was due to over-engineering or under-engineering.

It looks over-engineered because it includes a lot of (possibly unnecessary?) functionality (things like a "huge" menu system and Unicode normalization) just for demonstrating Caesar's. OTOH it looks under-engineered, because there is a lot of very verbose code and repetition.

I'll concentrate on the actually encryption class first.

A glaring problem here, is that you are missing the point of object-oriented programming. Having a class that you just instantiate to do one thing and then destroy it again isn't really the point of OOP.

It would be more sensible to have a class that, once instantiated, is used multiple times. Something like this:

CaesarCypher cypher = new CaesarCypher();

System.out.println(cyhper.encrypt("Example"));
System.out.println(cyhper.decrypt("Fybnqmf"));

The next problem would be the interface. Passing the information if you want to encrypt or decrypt as a String is very problematic, especially since those strings are tightly coupled to your menu.

Imagine this were a real production encryption system: The menu is changed in some way (for example, menu items are renumbered or translated), but the developer doing it doesn't know that requires changing the encryption class, or there is simply a small typo in one of the strings, then there will be no way to know something is wrong. The class doesn't report any errors, instead it just sends out everything unencrypted, which is potentially catastrophic.

Instead you should be using a parameter that only allows two possible values to represent encryption and decryption, for example a boolean or an enumeration:

enum CyperDirection { ENCRYPT, DECRYPT; }

Or use two separate methods as I did in the example above.

And even is you do use a String, then you shouldn't be calculating the "integerDependOnAction" inside the loop, but just once at the start.

The ALPHABET variable should actually be a constant (a final static field) in the class.

On to the actual algorithm:

A StringBuilder isn't really needed here. Internally a StringBuilder is just a char array, that is "automatically" extended if needed. But since you know how long the resulting string will be you don't need the automatic extending, so either at least initialize the StringBuilder with a capacity (new StringBuilder(userSentence.length())) or (because you are working character by character anyway) just use a char array of the correct length.

Your outer loop would be nicer as for (int i = 0; i < userSentence.length(); i++) { - just as you did in the inner loop. And don't forget proper formatting: a space after the for keyword and before the opening bracket.

Inside the loop you have a bug: the condition j == 25 can never be true.

In the end the class could look something like this:

class CaesarCypher {
    private static final char[] ALPHABET = {'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z'};

    private int shift;

    // This is just an example, why not to use a "one time" class: You can
    // configure it - in this case by allowing a different size shift.
    public CaesarCypher(int shift) {
        if (shift <= 0) {
            throw new IllegalArgumentException("shift must be a positive integer greater than zero.");
        }
        this.shift = shift;
    }

    public CaesarCypher() {
        this(1);
    }

    public String encrypt(String text) {
        return translate(text, this.shift);
    }

    public String decrypt(String text) {
        return translate(text, -this.shift);
    }

    private String normalize(String text) {
        return Normalizer.normalize(text, Normalizer.Form.NFD).replaceAll("[^\\p{ASCII}]", "").toLowerCase();
    }

    private String translate(String text, int shiftBy) {

        String normalizedText = normalize(text);

        char[] result = new char[normalizedText.length()];

        for (int i = 0; i < normalizedText.length(); i++) {
            for (int j = 0; j < ALPHABET.length; j++) {
                if (normalizedText.charAt(i) == ALPHABET[j]) {
                    result[i] = ALPHABET[(j + shiftBy) % ALPHABET.length];
                    break;
                }
            }
            // 0 is the default value for char. If the current element of the
            // result array is still zero, then original character wasn't in the
            // ALPHABET. In this case just use the original character.
            if (result[i] == 0) {
                result[i] = normalizedText.charAt(i);
            }
        }

        return new String(result);
    }
}

Finally a few short words the menu system: The nested ifs and long conditions ((cypherMethod.equals("1") || cypherMethod.equals("caesarcypher"))) makes the code very unreadable.

If the goal of this exercise was to show case the Caesar's cypher, then you may want to "cut back" on the menu system and use tests to demonstrate your code.

But if you consider the menu part of the exercise then you should consider maybe building a "proper" small framework to make the menu system more readable and maintainable.


Predict new ratings for each user based on their pearson correlation with other users I am new to R and programming. I have a set of ratings for 45000 users and 40 odd movies. I need to predict new ratings for each user based on their pearson correlation with other users. I also need to store the set of similar users for each user-movie combination.The code that I have managed to write is this

# Matrix of users and ratings
x <- matrix(rnorm(1:100), nrow = 10 , ncol = 30)
df = list()

# correlation matrix
cor_mat <- cor(x)

# similarity limits
upper = 1
lower = 0.4

# empty matrix to store predicted values
final_x = matrix(NA,nrow = 10,ncol = 30)

for (i in 1:ncol(x)){

for( j in 1:nrow(x)){

sim_user = which(cor_mat[i,] >= lower & cor_mat[i,] < upper)

final_x[i,j] = t(x[sim_user,j]) %*%
      cor_mat[sim_user,j]/sum(cor_mat[sim_user,j])

df[[length(df)+1]] = cbind.data.frame(i,j,sim_user,cor_mat[sim_user,j])

 }
}

Questions:

I am looping over each element of the matrix which works fine but seems pretty novice to me. Can something better be done?
I have heard of the foreach package but read that it adds value only when a single operation takes a long time to execute which is not the case here. Will it still provide me good performance?

You have incorrect indexing in the loop. Better version with mapply and correct indexing:

df <- mapply(function(i, j) {
  sim_user = which(cor_mat[i,] >= lower & cor_mat[i,] < upper)
  final_x[j, i] = t(x[j, sim_user]) %*% (cor_mat[j, sim_user]/sum(cor_mat[j, sim_user]))
  cbind(i, j, sim_user, cor_mat[j, sim_user])
}, 1:ncol(x), 1:nrow(x))

This version already 44 times faster than the loop version:

    test replications elapsed relative user.self sys.self user.child sys.child
1   loop          100    2.21     44.2      2.21        0         NA        NA
2 mapply          100    0.05      1.0      0.05        0         NA        NA


Count lines of scala code (omitting empty lines) in a directory, traversing any subdirectories I need to simply count lines of Scala code in a project, which includes some package(i.e. directory) hierarchy.

I'm not sure about the performance of the code below. Can you help improve it, if possible?

Also, in Java, we have reduce() in the Stream API, and I didn't find its counterpart in the Scala library, so ended with foldLeft().

import java.io.File
import scala.io.Source

object CountLoc {
  def main(args: Array[String]): Unit = {
    val parentDir: File = new File(System.getProperty("user.dir"))

    def traverse(dir: File): Int = {
      val all = dir.list().map(f => new File(dir.getAbsolutePath + "/" + f))
      val srcs = all.filter(!_.isDirectory).filter(_.getName.endsWith(".scala"))
      val dirs = all.filter(_.isDirectory)
      srcs.foldLeft(0)((i, f) => i + Source.fromFile(f).getLines().filterNot(_.isEmpty).toList.size) +
        dirs.toList.foldLeft(0)((i,d) => i + traverse(d))
    }

    println("LOC: " + traverse(parentDir))
  }
}

Instead of collecting all the source files in one sweep, and all the sub-directories in another sweep, you could move the foldLeft() to the top level and deal with each element as it is encountered.

def traverse(dir :File) :Int =
  dir.listFiles().foldLeft(0){ case (sum,file) =>
    if (file.isDirectory)
      sum + traverse(file)
    else if (file.getName.endsWith(".scala"))
      sum + io.Source.fromFile(file).getLines().count(_.nonEmpty)
    else
      sum
  }


Memory leak in a scraper This is a scraper written in CoffeeScript for NodeJS. It is run in an interval (set to 5 seconds here to dramtically show the leak). Somewhere this code leaks memory. I already tried nulling a few things for manually deleting references. Where is the reference kept that produces the leak?

request = require 'request'
jsdom   = require 'jsdom'
util    = require 'util'

scrapeURL   = 'http://kwlpls.adiwidjaja.com/index.php'
jqueryUrl   = 'http://code.jquery.com/jquery-1.6.1.min.js'
scrapeDivId = "cc-m-externalsource-container-m8a3ae44c30fa9708"

scrapeDelay = 5 * 1000 # 5 seconds
data = []

fetch = (callback) ->
    request { uri: scrapeURL }, (error, response, body) ->
        if error and response and response.statusCode isnt 200
            util.log 'Error when contacting #{scrapeURL}'

        # Fake Browser
        jsdom.env
            html: body,
            scripts: [jqueryUrl],
            (err, window) ->
                processPage(window, (result) ->
                    callback(err, result)
                )

processPage = (window, callback) ->
    # cleanup
    result           = {}
    result.cities    = []
    result.parkings  = []

    $    = window.jQuery
    rows = $('table').children()
    num  = $(rows).size()

    rows.each (i, row) ->
        if i > 0 and i isnt num - 1 # cut off header and footer
            processRow($, row, (item, city) ->
                result.parkings.push(item) if item?
                result.cities.push(city) if city?
                item = null
                city = null
                callback(result) if i is num - 2
            )

processRow = ($, row, callback) ->
    elements  = $(row).children('td')
    item      = {}
    city      = null
    nameStr   = elements?.eq(0).html()
    nameStr  ?= ""
    item.kind = nameStr.substring 0, 2
    item.name = nameStr.substring 3

    if elements.size() > 2
        free           = elements?.eq(2).html()
        spaces         = elements?.eq(1).html()
        item.free      = free
        item.spaces    = spaces
        item.status    = "open"
    else if elements.size() > 0
        item.status    = "closed"
    else
        header      = $(row).children().first().html()
        currentCity = header.split(' ')[1]
        city        = {}
        city.name   = currentCity

    item.city = currentCity
    if item.name is "" or not item.name? then item = null

    # cleanup
    elements = null
    $ = null

    callback(item, city)

cacheJson = () ->
    fetch(
        (err, result) ->
            util.log err if err?
            data = result ? []
            util.log 'Fetched ' + data?.parkings?.length + ' entries'
    )

scrapeIntervalId = setInterval cacheJson, scrapeDelay

I got here reading about garbage collection and memory leakage in CS. It's an old post but I 'll give my 2 cents anyway. When emptying an array it's often recommended to avoid using array = [], but use array.length = 0.

Also, in functions that you don't indent to return, perhaps it's better to implicitly return undefined so that to avoid returning functions just because they appear at the very end of your CS function call.


Avoiding code duplication in mapping layer between dataRecord and The following code is for mapping a dataRecord to a class. I don't like this solution since it requires to duplicate the logic of mapping for each field in every method which access to the same table.

What do you think about? What about using automapper?

public IEnumerable<Model.Accounts.User> GetUsers(int IDUser)
{
    using (var sqlClient = CreateSqlManagerInstance("ListUser", CommandType.StoredProcedure))
    {
        sqlClient.AddParameterWithValue("IDUser", IDUser);
        return sqlClient.ExecuteResultSet().Select(
           dataRecord => new GModel.Accounts.User
           {
               IDUser = dataRecord.GetInt("IdUtente"),
               FirstName = dataRecord.GetString("Nome"),
               LastName = dataRecord.GetString("Cognome"),
               //[...]
           }).ToArray();
    }
}

I believe the easiest way to avoid duplication each time you read from the database is to create a constructor which receives a DataRecord as a parameter, and does the heavy lifting:

public class User {

    public User() {}

    public User(DataRecord dataRecord) {
        IDUser = dataRecord.GetInt("IdUtente");
        FirstName = dataRecord.GetString("Nome");
        LastName = dataRecord.GetString("Cognome");
        //[...]
    }

    //...
}

And then:

public IEnumerable<Model.Accounts.User> GetUsers(int IDUser)
{
    using (var sqlClient = CreateSqlManagerInstance("ListUser", CommandType.StoredProcedure))
    {
        sqlClient.AddParameterWithValue("IDUser", IDUser);
        return sqlClient.ExecuteResultSet().Select(
           dataRecord => new GModel.Accounts.User(dataRecord)).ToArray();
    }
}

You could also use an automatic mapping library, but before you choose to do that, you might  want to look at some O/R mapping library, like NHibernate for c#, which should replace the mapping in your code altogether.


Generate random binary arrays I am fairly new to C++ programming and I, so far, love it. I just have been trying to make sure what I am doing is fast, unique, readable, and efficient. Is there anyway I can improve this code, or is there ways I can improve my coding convention (in C++)? Here I wrote a program that generates a 20 random vectors of binary digits (20 to test "randomness").

#include <stdbool.h>
#include <cstdlib>
#include <iostream>
#include <ctime>

void returnarray(bool* array, int size)
{
    for (int i = 0; i < size; i++)
    {
        std::cout << array[i];
    }
    std::cout << "\n";
}

bool* assignzero(int size)
{
    bool* b = new bool[size];
    for (int i = 0; i < size; i++)
    {
        b[i] = false;
    }

    return b;
}

void generate()
{
    int p = 9;
    int t = rand() % p;
    bool* b = assignzero(p);

    for (int x = 0; x < t; x++)
    {
        b[rand() % p] = true;
    }

    returnarray(b, p);
}

int main()
{
    long inc;
    srand(time(0));
    while (inc < 20)
    {
        generate();
        inc += 1;
    }
}

Initialise your variables

You aren't initialising inc.  This is probably working for you, because the compiler is setting it to 0.  This isn't guaranteed though, so you should be explicitly initialising it before you use it:

long inc = 0;
srand(time(0));
while (inc < 20)

Cleanup after yourself

As I mentioned in the comments, you're also leaking memory:

bool* b = new bool[size];

the allocated array is never cleaned up by your application.  You should have a paired delete somewhere in the code when you're done with the array:

delete [] b;

Expressive names

Think about using more expressive names.  You're referring to the array of bools as both 'b' and 'array' in different parts of your code.  One of the names tells your there's going to be more than one, whilst the other name hints at the type.

Your returnarray method actually prints it to the console.  If you get the names right, your program will be a lot easier to follow.


Laravel 5 controller to list appointments by date range and by label I am starting to get into OOP using PHP and MVC. I just want to know if this code is good practice. Is this how I should be using my controller and model, in its simplest form?

AppointmentsController:

public function index()
{
    $appointmentsByLabel        = Appointment::findAppointmentsByLabel('2');
    $appointmentsBetweenDates   = Appointment::findAppointmentsBetweenDates('2016-02-01', '2016-03-31');

    return view('appointments', compact('appointmentsBetweenDates', 'appointmentsByLabel'));
}

Model (Appointment):

public static function findAppointmentsBetweenDates($start, $end)
{
    $appointments = Appointment::whereBetween('start',array($start,$end))->get();

    return $appointments;
}

public static function findAppointmentsByLabel($value)
{
    $appointments = Appointment::where('label_id', '=', $value)->get();

    return $appointments;
}

The structure looks ok to me (I'm not that familiar with laravel, but this is basically how MVC in a web context generally looks). A couple of minor points:

you don't really need one-time variables such as $appointments, just return directly.
findAppointments* could be renamed to find*. It's in the Appointment class, so it's expected that it will find an appointment, unless noted otherwise. This also avoids the odd phrasing of Appointment::findAppointments and makes it Appointment::find.
be as explicit with your variable names as possible (here, it's not so important, but it's good practice, and for larger methods it does get important). eg value could be label, or even labelId, and start/end could be startDate/endDate (it may seem that this is a contradiction with the point above, but it's really not; the variables will be used inside the method, missing the direct context of the method name, while the methods will be called either directly on the class as you are doing, or on a variable, which will be named after the class, giving them direct context).
You can use self:: instead of ClassName::, even in static methods.


Unit Of Work/Repository usage In a project I'm working on, the data access layer has evolved to something that somewhat resembles the Unit Of Work and Repository patterns, at least they started with those names, I'm not entirely sure with how far they've drifted, if they can still be called that.

For classes data access, an implementation IWorkUnitFactory is passed as a constructor parameter from the IoC container. This is a rather simple interface:

public interface IWorkUnitFactory
{
    IWorkUnit Begin();
}

The IWorkUnit interface it returns looks like this:

public interface IWorkUnit : IDisposable
{
    T Insert<T>(T entity) where T : class;
    T Load<T>(int id) where T : class;
    void Delete<T>(int id) where T : class;
    void Delete<T>(T entity) where T : class;
    T Update<T>(T entity) where T : class;
    T InsertOrUpdate<T>(T entity) where T : class;
    IQueryable<T> Query<T>() where T : class;
    IEnumerable<T> Execute<T>(string queryName, IDictionary<string, object> parameters);

    void Commit();
    void Rollback();
}

My IWorkUnitFactory implementation for NHibernate basically just creates an NHibernate session, and passes it to the NHibernate implementation of IWorkUnit, which is just a rather thin wrapper around the NHibernate session, which starts a transaction in its constructor, and rolls it back in the Dispose implementation unless the Commit method is called.

Then, anywhere data access happens, it follows this pattern:

using(var workUnit = WorkUnitFactory.Begin())
{
    //Boring data access code here
    workUnit.Commit();
}

Next up are the Repository classes. These don't have any common inheritance or interface implementations, they just follow a few conventions, the main one being taking an IWorkUnit in its constructor. These classes contain the interaction with the IWorkUnit for operations like GetById, Find, Insert, Update, etc, and are responsible for small bits of logic (the Update method updates the LastUpdated field on the entity, Find determines what fields to search based on its parameters, etc).

So, for example, a Repository for the User entity might look something like this:

public class UserRepository
{
    private readonly IWorkUnit workUnit;
    public UserRepository(IWorkUnit workUnit)
    {
        this.workUnit = workUnit;
    }
    public virtual User GetByEmail(string email)
    {
        return workUnit.Query<User>().FirstOrDefault(u => u.Email == email);
    }
    public virtual User Update(User user, User updater)
    {
        user.ModifiedBy = updater;
        user.Modified = DateTimeOffset.Now;
        return workUnit.Update(user);
    }
}

Then, code to update a user would look something like this:

using(var workUnit = WorkUnitFactory.Begin())
{
    var userRepo = new UserRepository(workUnit);
    var userToEdit = userRepo.GetByEmail("test@example.com");

    //Do some stuff to userToEdit here

    userRepo.Update(userToEdit, currentRequestUser); //currentRequestUser is just whoever is performing the action
    workUnit.Commit();
}

Any feedback on better naming for these things, or better methods? This has served me rather well, with the only thing I don't really like is creating the repository instances like that, But I can't think of a way to handle transactions and keep the somewhat lower level data access code away from higher level business logic.

If all your business classes (e.g., User) could inherit from a common class or interface that has Modified and ModifiedBy properties, you could make your Repository a generic Repository<T>. Class-specific methods like GetByEmail would be replaced with generalized methods that take Expressions. Would that address your concern about "creating the repository instances like that", while keeping the business-level concerns (e.g., the fact that you're retrieving by email and not something else) away from your data access code?

Here's an untested and even uncompiled example of what I'm talking about.

public interface IBusinessEntity
{
    User ModifiedBy { get; set; }
    DateTime Modified { get; set; }
}

public class Repository<T> where T : IBusinessEntity
{
    private readonly IWorkUnit workUnit;
    public Repository(IWorkUnit workUnit)
    {
        this.workUnit = workUnit;
    }
    public virtual T Get(Expression<Func<T,bool>> expression)
    {
        return workUnit.Query<T>().FirstOrDefault(expression);
    }
    public virtual T Update(T objToUpdate, User updater)
    {
        objToUpdater.ModifiedBy = updater;
        objToUpdate.Modified = DateTimeOffset.Now;
        return workUnit.Update(objToUpdate);
    }
}

As for your naming conventions, they make sense to me.

Just one more idea: Unless your application will always be used in just one timezone, and hosted in the same timezone, you might consider storing your Modified property as UTC.