Simple Ruby program: a + b = c I am new to programming and I would like some feedback. This is my first ruby program that simply adds a + b. I know it's probably more complicated than it needs to be, but I wanted to implement ideas that I was learning. Is there anything that I should do that I didn't or did that I shouldn't?

    puts "<<<-----ADDITION----->>>"
    puts "<<<------A+B=C------->>>"
    letters = %i(a b)
    sum = 0

    letters.each do |let|
      define_method(let) do
        begin
          puts "Enter #{let}"
          let = Float(gets.chomp)
          sum = sum + let.to_f
        rescue
      puts "Something went wrong. Try again."
      retry
    end
  end
end

letters.each do |let|
  send(let)
end

puts sum

As philomory said in a comment, this is a neat demonstration of some of Ruby's features, but also a horribly complex way of saying a + b.

In it's most basic formulation, you could just do:

puts gets.chomp.to_f + gets.chomp.to_f

Of course, this leaves out a lot of the stuff your code is currently doing (like telling the user what they're supposed to input). So let's see if we can get all the same functionality, but with a different structure.

First, though, a few notes on your current code (apart from the metaprogramming):

Don't overwrite arguments just to avoid a new variable. You pass let (a symbol) to define_method, but then use it to store the user's chosen number. So now you have a variable named let, presumably short for "letter", which has nothing to do with letters - now it's a floating point number. You have the contents (the input), but you've overwritten its label. Or, you have the answer, but you've forgotten the question, to paraphrase Hitchhiker's Guide.

Besides conflating the concepts of name and value, it's also impractical. You wouldn't, for instance, be able to print something like "You entered a = 23", because you no longer know both the letter and the entered value.
This is redundant

let = Float(gets.chomp)
sum += let.to_f

You create a Float from the input. And then you call to_f on that float - but it's already a float. Either use Float(...) to get a number, or use the .to_f on a string. The latter is the conventional approach, but the former will complain if the input isn't numeric, so that's probably the one you'll want to use here:

sum += Float(gets.chomp)

And we've avoided overwriting let too.
But speaking of code complaining: You might want to be more specific in your exception handling. Your current rescue block doesn't distinguish between the sorts of errors that might arise. It's one thing to complain to the user if their input is bad, but what if the error is about something else? For instance, if I run your code in my editor, it'll run without an interactive terminal. So gets immediately returns nil, which causes a NoMethodError since chomp isn't a method on nil. And since gets returns immediately, the script goes into a retry-loop, spewing "Something went wrong. Try again." as fast as it can until I kill it.

Obviously, this isn't how the script is meant to be run, but it goes to show how telling the user to "try again" isn't always a solution. Your code just assumes it's always the user's fault, and if it tries enough times, the problem will get fixed. But the error could also be a mistake in the code, like writing get instead of gets - that'd also cause the script to loop indefinitely, even though there's nothing the user can do to salvage things.

Now, for alternative approaches: While define_method is a really neat Ruby feature, it's not called for in this case. But a generalized method for getting a float as user input would be useful. Then you can use that for the rest of the code. For instance, the script could be:

def read_float(prompt)
  puts prompt
  Float(gets.chomp)
rescue ArgumentError # only rescue this type of error
  puts "Please enter a number"
  retry
end

puts %w(a b).reduce(0) { |sum, var| sum + read_float("Enter #{var}") }

Yes, I'm being a little clever with the reduce and everything. A more straight-forward and instantly readable way would be to say:

a = read_float("Enter a")
b = read_float("Enter b")
puts a + b

which is about as to-the-point of "A + B = C" as you can get.


A structure for a text messages dictionary with system notifications/errors I would like to arrange a system messages dictionary for an application in JavaScript.
As an initial intention, I thought something about the following structure:
export const SYS_MESSAGES = Object.freeze({

    fileSizeExceeded: {
        code: "FILE_SIZE_EXCEEDED",
        text: "The file size exceeds allowed limit"
    }

});

But then I realized, the structure could be even more simple:
export const SYS_MESSAGES = Object.freeze({

    FILE_SIZE_EXCEEDED: "The file size exceeds allowed limit"

});

Which option of two structures is a preferred-one from maintenance/style perspective?

I consider that the second option is better for the following:

The first option is more verbose than the second, and it is in a way that there is no additional information which allow other developers or even you to obtain meaningful information.
The second option has a constant style name which represents what you are trying to tell us, "hey, this is a constant and its value is ..." and so, it clearly reflects a shorter and descriptive code style.
Having less properties (in your objects) implies less space, and less space computed implies less execution time. (even if it is very small).
Less content helps you to debug faster.

Well, I hope it helped you.


Retrieve filetype:pdf from library I retrieved all the books in pdf from my library with a recursive function.

import os

def walk(dirname):
    for name in os.listdir(dirname):
        path = os.path.join(dirname, name)

        if os.path.isfile(path) and path.endswith('pdf'):
            books.append(path)
        if os.path.isdir(path):
            walk(path)
def main():
    dir = '~/Documents/MyLibrary/'
    global books
    books = []
    walk(dir)
    print(books)

if __name__ == "__main__":
    main()

Similarly to a feature in most UNIX shells, Python has a globbing module, called glob.

It even supports the extended globbing (using ** to recursively descend into sub-folders) since Python 3.5, which you can use like this:

import os
from glob import glob

if __name__ == "__main__":
    dir = os.path.expanduser('~/Documents/MyLibrary/')
    books = glob(os.path.join(dir, "**/*.pdf"), recursive=True)
    print(books)

Here I also used the os.path module to correctly join the paths and expand the ~ symbol to the users home directory.

Alternatively, you could use the pathlib.Path object (introduced in Python 3.4), which also supports globbing:

from pathlib import Path

if __name__ == "__main__":
    dir = Path('~/Documents/MyLibrary/').expanduser()
    books = list(dir.glob('**/*.pdf'))
    print(books)

Note the list call, which is necessary, because the Path.glob method returns a generator (in contrast to glob.glob, which returns a list).


User and picture models I have the following models:

User
Picture
Variant

User has_many pictures, picture has_many variants.

Variant has value price and I am trying to find the price of all pictures of selected (e.g. for user id 15) current user.

First I tried like this:

def value(user)

  total = 0

  user.pictures.each do |picture|

    picture.variants.each do |variant|
      total += variant.price
    end

  end

  total.to_f

end

How could I improve this code so that I can avoid the N+1 problem?

You could do this with just one query. There are variations how to construct the query but I would probably do something like this:

picture_ids = user.pictures.select(:id) # Will be used as a subquery if you use select
Variant.where(picture_id: picture_ids).sum(:price)

This will only generate one query, and sum all the prices using sql, so you don't lose performance.

EDIT: As pointed out in the comment below, if you are using MySQL (or have a SQL server which does not handle subqueries that good) you can use pluck instead of select. That will make as separate query to fetch all the picture_ids and everything else will still work the same way.


Grouping filenames by basename and extension I have created a function that parses a directory tree, and returns a list of lists that groups specific files within for later program specific parsing.  I feel like it could be done better, considering I've used dummy variables, and was hoping some of you could help.

def get_output_files(path) -> list:
    """
    This routine crawls through a directory tree, and checks for groups of job files, and groups based on:
        .sh   - run script file
        .hin  - hamiltonian creation input file
        .in   - iterate input file
        .hout - hamiltonian creation output file
        .out  - iterate output file
    to be used for parsing.

    Checks for grouping based on base name of file, if file suffix does not exist, then returns None in place.
    Example:  directory "/home/test" contains the following files:
        run.sh
        run.hin
        run2.in
        run.hout
        run2.out

    will return the following list:
        [
        [run.sh, run.hin, None, run.hout, None],
        [None, None, run2.in, None, run2.out]
        ]
    """
    jobs = []
    suffixes = ['.sh', '.hin', '.in', '.hout', '.out']
    for root, dirs, files in os.walk(path):
        for file in files:
            base, end = file.rsplit('.', 1)
            dummy_list = []
            for suffix in suffixes:
                if base + suffix in files:
                    dummy_list.append(os.path.join(root, base + suffix))
                else:
                    dummy_list.append(None)
            jobs.append(dummy_list)
    real_jobs = []
    for num, job in enumerate(jobs):
        if not all(element is None for element in job):
            real_jobs.append(job)
    return jobs

Don't worry about dummy variable names, naming is hard, and it can be fixed up later.

For my solution, there are a number of changes:

code is simplified by having a short inner function
the "yield" operator produces results one at a time, vs a list of results being processed in batch
the original code would process job files multiple times. IE if beer.sh and beer.hin were in a directory, the job would be scanned twice.
a simple test is added; modules added in PEP8 style; documentation; name clarification

source: mglob.py with test

'''
mglob.py -- print job files in directory

TEST USAGE:
    python -m unittest mglob
'''

import glob
import os
import unittest

def find_jobs(path):
    def find_job_paths(root, files, base):
        """
        given a job base filename, list related job files that exist
        """
        JOB_SUFFIXES = ['.sh', '.hin', '.in', '.hout', '.out']
        for suffix in JOB_SUFFIXES:
            newfile = base + suffix
            if newfile in files:
                yield os.path.join(root, newfile)
            else:
                yield None

    for root, dirs, files in os.walk(path):
        # process each file base name once
        bases = set(os.path.splitext(file_)[0]
                    for file_ in files)
        for base in bases:
            job_paths = list(find_job_paths(root, files, base))
            if any(job_paths):
                yield job_paths

class TestFind(unittest.TestCase):
    def test_simple(self):
        result = list(find_jobs('temp'))
        self.assertEquals(
            result, [
                [None, None, 'temp/run2.in', None, 'temp/run2.out'],
                ['temp/run.sh', 'temp/run.hin', None, 'temp/run.hout', None],
            ],
            result)


Rotate an image (2D array) by 90 degrees recursively with low memory usage I'm looking for comments on coding style, passing arguments, ...etc.

#include <iostream>

using namespace std;

void printImage(int** img, int N){

    //check if image pointer is NULL
    if(img == NULL){
        return;
    }

    //check if any of the columns of the image are null (since its essentially an array of pointers)
    for(int i=0; i<N ; i++){
        if(img[i] == NULL){
            return;
        }
    }

    for(int i=0; i<N ; i++){
        for(int j=0; j<N ; j++){
            cout<<img[i][j]<<",";
        }
        cout<<endl;
    }
}

// ROTATE IMAGE BY 90 DEGREES RECURSIVELY
void do_rotateImageInplace(int** img, int N, int row, int col){

    //base condition --> return: do this by check if we need to go deeper into the "peels of the onion :)"
    if(row > N-row-1 || col < 0 || N==1)
    {
        return;
    }

    // Tmp usage --> to store row or column being rotated
    int* tmp = new int[N-row];

    //backup row
    for(int i=row; i<N-row ; i++){
        tmp[i]=img[i][col];
    }

    //copy row into col
    for(int i=N-row-1; i>=row ; i--){
        img[i][col] = img[row][i];
    }

    //swap tmp with end row
    for(int i=N-row-1 ; i>=row ; i-- ){
        int tmpVal = img[col][i];
        img[col][i] = tmp[N-i-1];
        tmp[N-i-1] = tmpVal;
    }

    // Swap end row into first col
    for(int i=N-(row)-2 ; i>=row ; i--){
        int tmpVal = img[i][row];
        img[i][row] = tmp[N-i-1];
        tmp[N-i-1]=tmpVal;
    }

    // Swap first column backup into first row
    for(int i=row+1; i<N-(row)-1 ; i++){
        img[row][i] = tmp[i];
    }

    delete[] tmp;

    do_rotateImageInplace(img, N, row+1, col-1);
}

void rotateImageInplace(int** img, int N){

    // Check if image pointer is NULL
    if(img == NULL){
        return;
    }

    // Check if any of the columns of the image are null (since its essentially an array of pointers)
    for(int i=0; i<N ; i++){
        if(img[i] == NULL){
            return;
        }
    }

    // If image is less than 2 in size
    if(N<2){
        return;
    }

    do_rotateImageInplace(img, N, 0, N-1);
}

int main(){

    //TEST CASES:
    int N = 5;
    int** img = new int*[N];
    int val = 0;
    for(int i=0; i<N ; i++){
        img[i] = new int[N] {val++,val++,val++,val++,val++};
    }

    cout<<"******* original image ******"<<endl;
    printImage(img, N);

    rotateImageInplace(img, N);

    cout<<"******* rotated image ******"<<endl;
    printImage(img, N);

    for(int i=0; i<N ; i++){
        delete img[i];
    }
    delete[] img;

}

First of all, I don't see why do_rotateImageInplace shall be recursive. And indeed, it is a tail recursive, and as such is trivially converted into a loop.

Second, the comments are seriously misleading. This is what they say:

//backup row
//copy row into col
    (hmm... now `col` is permanently destroyed)
//swap tmp with end row
    (hmm... the comment above says that tmp istelf is a row. Why would I want to swap rows?)

etc.

The only useful comment is about peeling the onion. And a good comment really wants to be a function name. So

for (int layer = 0; layer < N/2; layer++)
    rotate_layer_inplace(img, N, layer);

seems like what you are trying to do.


Reddit account creator I'm trying to make a program that will create an account on Reddit using an HttpWebRequest. I ONLY want it to do that. (I don't care about it storing a cookie or anything else.) I just want it to be a simple program where you type a username into Username_TextBox.Text and a password into Password_TextBox.Text and then click the CreateAccount_Button and voilà, new account created!

I followed a tutorial on YouTube that poorly explained how everything about a HttpWebRequest worked.

Currently, the program will create a Reddit account as long as the username hasn't been taken already. I just don't want any excess code that is useless. I want any extra code removed that isn't contributing to my goal mentioned above.

using System.IO;
using System.Net;
using System.Text;
using System.Windows.Forms;
using System;

namespace RedditAccountCreate
{
    public partial class RedditAccountCreate : Form
    {
        public RedditAccountCreate()
        {
            InitializeComponent();
        }

        CookieContainer CreateAccountCookie;

        private void CreateAccount_Button_Click(object sender, EventArgs e)
        {
            string POSTData = "op=reg&dest=https%3A%2F%2Fwww.reddit.com%2F&user=" + Username_TextBox.Text + "&passwd=" + Password_TextBox.Text + "&passwd2=" + Password_TextBox.Text + "&email=&api_type=json";
            CookieContainer TemporaryCookie = new CookieContainer();
            UTF8Encoding Encoding = new UTF8Encoding();
            byte[] ByteData = Encoding.GetBytes(POSTData);

            HttpWebRequest POSTRequest = (HttpWebRequest)WebRequest.Create("https://www.reddit.com/api/register/" + Username_TextBox.Text);
            POSTRequest.Method = "POST";
            POSTRequest.KeepAlive = true;
            POSTRequest.CookieContainer = TemporaryCookie;
            POSTRequest.ContentType = "application/x-www-form-urlencoded";
            POSTRequest.Referer = "http://www.reddit.com/";
            POSTRequest.UserAgent = "Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/41.0.2228.0 Safari/537.36";
            POSTRequest.ContentLength = ByteData.Length;

            Stream POSTRequestStream = POSTRequest.GetRequestStream();
            POSTRequestStream.Write(ByteData, 0, ByteData.Length);
            POSTRequestStream.Close();
            HttpWebResponse POSTResponse = default(HttpWebResponse);

            POSTResponse = (HttpWebResponse)POSTRequest.GetResponse();
            TemporaryCookie.Add(POSTResponse.Cookies);
            CreateAccountCookie = TemporaryCookie;
            StreamReader POSTRequestReader = new StreamReader(POSTResponse.GetResponseStream());

            string POSTRequestCode = POSTRequestReader.ReadToEnd();
        }

    }
}

Here's the link to the YouTube tutorial that I followed, in case anyone wants it.

Some points about your code

extract the code to create an account to a separate class. In this way you can reuse it without problems. This can even be a static method.
variables should be named using camelCase casing. See the naming guidelines.
as you don't use neither the TemporaryCookie nor the CreateAccountCookie you should remove these varaibles.
the assignment of POSTRequest.KeepAlive = true; is superflous because the default of the KeepAlive property is true.
string POSTRequestCode = POSTRequestReader.ReadToEnd(); is superflous because it is never used.
because Stream is implementing IDisposable you should enclose it in a using statement. This will take care that the Stream is closed and disposed.
you don't use the HttpWebResponse POSTResponse so you can remove it too

Implementing the mentioned points will lead to:

private static string postDataToFormat = "op=reg&dest=https%3A%2F%2Fwww.reddit.com%2F&user={0}&passwd={1}&passwd2={1}&email=&api_type=json";
public static void CreateRedditAccount(string userName, string passWord)
{
    string postData = string.Format(postDataToFormat, userName, passWord);

    UTF8Encoding Encoding = new UTF8Encoding();
    byte[] postValue = Encoding.GetBytes(postData);

    HttpWebRequest POSTRequest = (HttpWebRequest)WebRequest.Create("https://www.reddit.com/api/register/" + userName);
    POSTRequest.Method = System.Net.WebRequestMethods.Http.Post;
    POSTRequest.ContentType = "application/x-www-form-urlencoded";
    POSTRequest.Referer = "http://www.reddit.com/";
    POSTRequest.UserAgent = "Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/41.0.2228.0 Safari/537.36";
    POSTRequest.ContentLength = postValue.Length;

    using (Stream POSTRequestStream = POSTRequest.GetRequestStream())
    {
        POSTRequestStream.Write(postValue, 0, postValue.Length);
    }

}


Shell script for creating CR questions Inspired by Tool for creating CodeReview questions.

This script generates a markdown document suitable for a CR question from a list of files. For each file it finds, it sends the following to stdout:

The relative path to the file, in bold.
The byte count and line count, in parentheses.
The contents of the file, indented four spaces on each line.

./mdproj.sh (186 bytes in 7 lines)

for path in $(find "$@"); do
    bytes="$(wc -c < "$path") bytes"
    lines="$(wc -l < "$path") lines"
    echo -e "\n**$path** ($bytes in $lines)\n"
    sed "s/^/    /" < "$path"
done

The script takes the same options as find. Example usage:

mdproj.sh -name *.h -or -name *.cpp > whatever.md

Update / known issues

No spaces, backquotes, backslashes, asterisks, or other weird characters are allowed in file names.
Watch out for shell expansion when invoking the script. Arguments should be enclosed in quotes in cases where the the shell would expand them.

I note three potential problems with your approach, both dealing with special characters in filenames, as well as  one comment on the example usage.

for with find

Consider a directory structure where some paths have spaces in them, say:

Source
├── Constants.h
└── Main Loop.c

(One could argue the wisdom of using spaces in paths, but your script should be able to deal with them anyway.) What would find . -type f output?

./Source/Constants.h
./Source/Main Loop.c

What would for path in $(find . -type f); do echo $path; done output?

./Source/Constants.h
./Source/Main
Loop.c

That's right; for splits on whitespace, breaking your paths. This is a notorious issue. Fortunately, that page provides a solution:

find . -type f -print0 | while IFS= read -r -d '' path; do echo $path; done

echo -e

Look at this line:

echo -e "\n**$path** ($bytes in $lines)\n"

You're substituting path in, and then telling echo to interpret the resulting text. That suggests that if path has an escape sequence in it, echo will interpret it. Indeed, it does:

% path='x\ay'
% touch "$path"
% echo -e "$path"
xy

(The \a was interpreted as "sound the bell.") Instead of using echo -e, consider passing the special characters directly to echo. You can make Bash interpret ones before passing it to echo by using $'':

% echo $'\n'"**$path** ($bytes in $lines)"$'\n'

Markdown

Markdown cares about special names, too. What if I had a file named a** b **c? Your script would give me a header:

**a** b **c**

  a b c

But that's probably not what you intended. You probably want to enclose it in backticks:

**`a** b **c`**

  a** b **c

But even that's not enough. What if my file names have backticks, like, say, a` b `c?

**`a` b `c`**

  a b c

That's no good! You have to then double-backtick it:

**``a` b `c``**

  a` b `c

Essentially, you have to count the maximum number of consecutive backticks and enclose it in that-number-plus-one of backticks.

Example usage

You suggest

mdproj.sh -name *.h -or -name *.cpp > whatever.md

but that will only work when there are exactly zero or one files whose name end with .h and exactly zero or one files whose name end with .cpp. Consider how it expands when there are no files:

mdproj.sh -name *.h -or -name *.cpp > whatever.md

That's right; Bash leaves them in place, passing them on to find, where it looks for files of the form *.h and *.cpp. Since there are no such files (otherwise, Bash would have substituted them in) find will not find the files, and so nothing will happen. What if there is example.h and example.cpp?

mdproj.sh -name example.h -or -name example.cpp > whatever.md

That works fine. What if there are multiple?

mdproj.sh -name foo.h bar.h -or -name foo.cpp bar.cpp > whatever.md

Uh oh. -name takes only one value, but you're passing it two. My find dies with this error:

find: bar.h: unknown option

I always use -regex for that sort of thing:

mdproj.sh -regex '.*\.h' -or -regex '.*\.cpp' > whatever.md

(If I was being cleverer, I might use -E and merge them into one regular expression.)


Reading Multidimensional arrays I am building a cypher program which uses a matrix of numbers and letters. A message is encoded according to the row and column each letter of the message is found in the matrix.

I am trying to find a nice way of iterating over the 2D matrix (6*6).

 for char in range (0, len(message), 1):
  for i in range(0, len(rows), 1):
    for y in range(0, len(cols), 1):
      if matrix[i][y] == message[char]:
         cyphermessage.append(i)
         cyphermessage.append(y)
         cyphermessage.append(" ")

But this method uses a 3-tier for loop which makes it \$\mathcal{O}((N)*3)\$, plus the the cypher append messages are quite ugly.

Don't search – find. Specifically, don't iterate through all matrix entries for each character. Instead, build a dictionary that maps the characters in the matrix to a pair of indices.

Also, don't loop over indices (in message) unless you have to. Now we have:

position_lookup = dict()
for x in range(len(matrix)):
  for y in range(len(matrix[x])):
    position_lookup[matrix[x][y]] = (x, y)

for char in message:
  if char in position_lookup:
    x, y = position_lookup[char]
    cyphermessage.extend([x, y, " "])

Technically, this behaves different from the code you wrote: consider what happens when one character occurs multiple times in the matrix – I only use the last occurrence, while you append all possibilities.


How to check minimize or make if-condition on object loop more correct? I am looping through an object and finding whether or not the object has the "key," along with some further stuff.

I feel that there is some other correct way to do this apart from this:

var initialLocalesGen = function(data){

    var status;

    if(!$.isEmptyObject(data)){

        console.log(data);

        if(data.hasOwnProperty("username") && data.hasOwnProperty("name")){
            var headerHtml = Handlebars.compile($("#header-template").html());
            $("#userInfoBar").append(headerHtml({"username" : data["username"], "name" : data["name"]}));
        }

        if(data.hasOwnProperty("allLocales")){
            var formHtml = Handlebars.compile($("#locale-template").html());
            $("form legend").after(formHtml(data["allLocales"])).end().find("span.submit").css({display:"block"});
        }

        if(data.hasOwnProperty("status") && parseInt(data["status"]) === 0){
                currentStatus = 0;
                var object1 = {"status" : data["status"] , "userLocales": data["userLocales"]}
                userLocalesProcess(object1);
        }

        if(data.hasOwnProperty("status") && parseInt(data["status"]) === 1){
                currentStatus = 1;
                userChangeProcess({"changeLocales" : data["changeLocales"],"userLocales" : data["userLocales"] });
                popupMaker({"message":data["message"],"status":data["status"]});
        }

        if(data.hasOwnProperty("status") && parseInt(data["status"]) === 2){
                currentStatus = 2;
        }

    }

    console.log("currentStatus:", currentStatus);

}

Could anyone help me minimize the condition or show me an even better way of doing it?

You could merge the last three ifs. (I assume there are no other states.)

if(data.hasOwnProperty("status")){
        currentStatus =  parseInt(data["status"]);
        switch(currentStatus ) {
             case 0:
                  var object1 = {"status" : data["status"] , "userLocales": data["userLocales"]}
                  userLocalesProcess(object1);
                  break;
             case 1:
                  userChangeProcess({"changeLocales" : data["changeLocales"],"userLocales" : data["userLocales"] });
                  popupMaker({"message":data["message"],"status":data["status"]});
                  break;
             default:
                  break;
        }

}

I think there is nothing else you can do here. All other ifs are totally different. Maybe you could extract method or variables do communicate the meaning of every branch, but beside that I thing we are done.

Of course, if is also suitable

...
if (currentStatus ==0) {
    var object1 = {"status" : data["status"] , "userLocales": data["userLocales"]}
    userLocalesProcess(object1);
} else if (currentStatus ==1){
    userChangeProcess({"changeLocales" : data["changeLocales"],"userLocales" : data["userLocales"] });
    popupMaker({"message":data["message"],"status":data["status"]});
}
...

Maybe you should just stay with obj.property == null. According to the link above it seems to be faster, but at least it is easier to read.


Clojure MapReduce Reducer This program forms the reducer of a Hadoop MapReduce job. It reads data in from stdin that is tab delimited.

foo    1
foo    1
bar    1

and outputs

foo    2
bar    1

Any suggestions for improvements?

(use '[clojure.string :only [split]])
(def reducer (atom {}))

(defn update-map [map key]
  (merge-with + map {key 1}))

(doseq [line (line-seq (java.io.BufferedReader. *in*))]
  (let [k (first (split line #"\t"))]
    (swap! reducer update-map k)))

(doseq [kv @reducer]
  (println (format "%s\t%s" (first kv) (second kv))))

probably a bit too late to help OP, but in case anyone else stumbles upon this question, here's a nice succinct way of doing it, using the frequencies function:

(doseq [[word freq] (frequencies
                      (map
                        #(re-find #"^[^\t]+" %) ;; just get the first non-tab characters
                        (line-seq (java.io.BufferedReader. *in*))))]
  (println (str word "\t" freq)))


jQuery vertical div slider In my project I need to do an automatic sliding vertical slider:

var number = 0;
var count = $(".home-header-boxes .col-6").children().length;
window.setInterval(
    function() {
        number = number + 104; // 104px div height
        $(".home-header-boxes .col-6 .home-header-boxes-profile-wrapper").each(function(i) {
            $(this).css({
                "transform": "translateY(-" + number + "px)",
            })
        })

        if (number == count * 104) {
            number = 0;
            $(".home-header-boxes .col-6 .home-header-boxes-profile-wrapper").each(function() {
                $(this).css({
                    "transform": "translateY(0px)",
                })
            })
        }
    }, 3000);

I wonder if there is a simpler way to do this vertical slider, because sometimes the transformY is not going up fluently. It is disappearing and showing.

Example jsfiddle

I don't see anything wrong with the smoothness of the animation.  However, I would simplify the code by introducing another container inside the .slide-item-wrapper; that whole container moves up, so that you don't have to animate each .box-item separately.  Furthermore, I would eliminate the special case for rewinding the animation to its original position.

var offset = 0;
var count = $(".slide-item-wrapper > * > *").length;
window.setInterval(
  function() {
    offset = (offset - 104) % (count * 104); // 104px div height (incl margin)
    $(".slide-item-wrapper > *").css({
      "transform": "translateY(" + offset + "px)",
    });
  }, 3000);
body {
  margin: 100px
}

.slide-item-wrapper {
  width: 300px;
  height: 84px;
  overflow: hidden;
}

.slide-item-wrapper > * {
  transition: .5s ease-in-out;
}

.box-item {
  display: flex;
  justify-content: center;
  align-items: center;
  background: red;
  color: white;
  width: 300px;
  height: 84px;
  margin-bottom: 20px;
}

      Lipsum

      Lipsum2

      Lipsum3

      Lipsum4

      Lipsum5

      Lipsum6


Node.js backup cron job I'm looking for some general feedback about the layout and structure of my code. Basically just any feedback, as I'm not super happy about this code, but I'm not sure how to improve it further.

'use strict';

/* jshint node: true */

var fs         = require('fs');
var os         = require('os');
var nodemailer = require('nodemailer');
var rimraf     = require('rimraf');
var config     = require('../config');
var dropbox    = require('../lib/dropbox-sync');
var mysql      = require('../lib/mysql-backup');
var tarball    = require('../lib/tarball');
var transport  = nodemailer.createTransport('sendmail');

// This is the FILE name of the backup file
var backupFilename  = config.dropbox.mysql_prefix + 'backup-' + Date.now();

// This is the absolute file path to save the file to on Dropbox
var dropboxFilename = ['', config.server, config.dropbox.mysql_folder, backupFilename].join('/');

// Connect to Dropbox with our credentials
try {
  dropbox.connect({
    key: config.dropbox.key,
    secret: config.dropbox.secret
  });
} catch (err) {
  failWithDropboxError(err);
}

// Connect to MySQL with our credentials
try {
  mysql.connect({
    host: config.mysql.hostname,
    user: config.mysql.username,
    password: config.mysql.password
  });
} catch (err) {
  failWithMySQLError(err);
}

// Generate a MYSQL backup
mysql.backup(backupFilename, function (err, pathname, stats) {
  if (err) {
    return failWithMySQLError(err);
  }

  tarball.create(pathname, '/tmp/' + backupFilename + '.tar.gz', function (code) {
    rimraf.sync(pathname);

    dropbox.send('/tmp/' + backupFilename + '.tar.gz', dropboxFilename, function (err, response) {
      fs.unlinkSync('/tmp/' + backupFilename + '.tar.gz');

      if (err) {
        failWithDropboxError(err);
      } else {
        console.log(response);
        console.log('The MySQL backup has been completed successfully.\n');
        console.log('Backed up %d from %d databases.\n', stats.tables, stats.databases);
        console.log('The backup file has been saved to your Dropbox: %s', '');
      }
    });
  });
});

function failWithMySQLError (err) {
  console.error('The MySQL backup job has encountered a fatal error and could not continue.\n');
  console.error('We were unable to create the MySQL backup.\n');
  console.error(err);

  process.exit(1);
};

function failWithDropboxError (err) {
  console.error('The MySQL backup job has encountered a fatal error and could not continue.\n');
  console.error('We were unable to sync the MySQL backup to Dropbox.\n')
  console.error(err);

  process.exit(1);
};

You code looks good and well formatted. There are only a few minor issues:

It seems that you have tried to check it with JSHint but there are some issues that JSHint reports about your code. In particular, semicolons and unused variables.
It is not clear when the character "\n" is required in messages, and when it is not. It looks like sometimes you just forgot it.
This one looks inconsistent. It looks like there is an error:

console.log('The backup file has been saved to your Dropbox: %s', '');

This actually a bad pattern:

if (err) {
    return failWithMySQLError(err);
}

it shortens a code a little but adds impression that the value of the function is important, while it is not.
else is redundant here:

if (err) {
    failWithDropboxError(err);
} else {

and as you see, the long lines complicate the code review.


Duplicate Payments using SQL Working on identifying duplicate vendor payments using SQL in MS Access. The business logic uses four column; Vendor #, Invoice #, Invoice Date and Invoice Amount. There are 5 core reviews, if all are the Exact matches (EEEE) or if at least one field is different (DEEE,EDEE,EEDE,EEED). I have it working for all 5, however its slow and I would like to improve on the code itself. I put an example of EEEE and DEEE below. Thoughts on a better way to write it?

Table SCHEMA:

ID - AutoNumber
VendorCol - Short Text
VendorNameCol - Short Text
InvoiceNoCol - Short Text
InvAmountCol - Number
InvoiceDateCol - Date/Time

EEEE:

SELECT *
  FROM [Payment History] AS PH1
  WHERE Exists
 (Select *
 From [Payment History] AS PH2
 Where PH2.[ID] <>PH1.[ID]
 AND PH2.[VendorCol] = PH1.[VendorCol]
 AND PH2.[InvoiceNoCol] = PH1.[InvoiceNoCol]
 AND PH2.[InvoiceDateCol] = PH1.[InvoiceDateCol]
 AND PH2.[InvAmountCol] = PH1.[InvAmountCol])
 ORDER BY [InvAmountCol] DESC , [InvoiceDateCol] DESC;

Example of desired result:
EEEE Example

DEEE: Different vendor # but all else equal

SELECT *
FROM [Payment History] AS PH1
 WHERE Exists
 (Select *
 From [Payment History] AS PH2
 Where PH2.[ID] <>PH1.[ID]
 AND PH2.[VendorCol] <> PH1.[VendorCol]
 AND PH2.[InvoiceNoCol] = PH1.[InvoiceNoCol]
 AND PH2.[InvoiceDateCol] = PH1.[InvoiceDateCol]
 AND PH2.[InvAmountCol] = PH1.[InvAmountCol])
 ORDER BY [InvAmountCol] DESC , [InvoiceDateCol] DESC;

Example of desired result:
DEEE Example

I can post the other ones, but figured this would get most there.

You are needlessly complicating this a little, checking a single table for multiple records can be done the same way as checking multiple tables for the same record. You want to use a join.

You also use PH1 and PH2 as aliases, but what do they mean. Your aliases should be meaningful. I replaced PH1 with base, as it's the base table of our search, and PH2 with duplicate, as it contains the duplicate records.

Last but by no means least, you should avoid using SELECT * in SQL in general. If the schema of the table changes then the query will happy continue to run, even though the data may now be incorrect or irrelevant. SELECT * also tends to run slower than typing out the column names, although I am not 100% sure this is also the case in Access SQL.

Here is your first query after I finished tinkering:

SELECT
    base.[VendorNameCol] AS [Vendor Name],
    base.[InvoiceNoCol] AS [Invoice #],
    base.[InvoiceDateCol] AS [Invoice Date]
    base.[InvAmountCol] AS [Invoice Amount]
FROM [Payment History] AS base
INNER JOIN [Payment History] AS duplicate
    ON base.[ID] <> duplicate.[ID]
    AND base.[VendorCol] = duplicate.[VendorCol]
    AND base.[InvoiceNoCol] = duplicate.[InvoiceNoCol]
    AND base.[InvoiceDateCol] = duplicate.[InvoiceDateCol]
    AND base.[InvAmountCol] = duplicate.[InvAmountCol])
ORDER BY
    [InvAmountCol] DESC,
    [InvoiceDateCol] DESC;


Double event listener for single function With your help in this question,
I've got this code to show the pop-up, and than hide it when close button is clicked:

<script>
    var applycouponPopupId = "popup-wrapper";
    window.addEventListener("load", function() {
        document.getElementById(applycouponPopupId).classList.add("modal-overlay_visible");
    });
    document.addEventListener('click', function(e) {
        if (!e.target.classList.contains("modal-overlay__close")) return;
        document.getElementById(applycouponPopupId).classList.remove("modal-overlay_visible");
    });
</script>

Now i need to change the code to close the pop-up on any click or scroll.
According to this answer, i converted code to the following:

<script>
    var applycouponPopupId = "popup-wrapper";
    window.addEventListener("load", function() {
        document.getElementById(applycouponPopupId).classList.add("modal-overlay_visible");
    });

    function mouseMoveHandler() {
        document.getElementById(applycouponPopupId).classList.remove("modal-overlay_visible");
    }

    ("click scroll".split(" ")).forEach(function(e) {
        document.addEventListener(e, mouseMoveHandler, false);
    });
</script>

Please tell if i did everything right.

The only suggestion I have is..

"click scroll".split(" ") ... why? what was that person thinking?

['click', 'scroll'].forEach() is both faster and shorter.

Aside from that, looks great!


Find the longest common subsequence algorithm - low speed I've designed an algorithm to find the longest common subsequence.

These are steps:

Pick the first letter in the first string.
Look for it in the second string and if its found, Add that letter to
common_subsequence and store its position in index, Otherwise
compare the length of common_subsequence with the length of lcs
and if its greater, asign its value to lcs.
Return to the first string and pick the next letter and repeat the
previous step again, But this time start searching from indexth letter
Repeat this process until there is no letter in the first string to
pick. At the end the value of lcs is the Longest Common
Subsequence.

This is an example:

X=A, B, C, B, D, A, B
Y=B, D, C, A, B, A

Pick A in the first string.
Look for A in Y.
Now that there is an A in the second string, append it to common_subsequence.
Return to the first string and pick the next letter that is B.
Look for B in the second string this time starting from the position of A.
There is a B after A so append B to common_subsequence.
Now pick the next letter in the first string that is C. There isn't a C next to B in the second string. So assign the value of common_subsequence to lcs because its length is greater than the length of lcs.

Repeat the previous steps until reaching the end of the first string. In the end the value of lcs is the Longest Common Subsequence.

The complexity of this algorithm is \$\theta(n*m)\$.

I implemented it on two methods. The second one is using a hash table, but after implementation I found it's much slower compared to the first algorithm. I can't understand why.

The first algorithm:

import time
def lcs(xstr, ystr):
    if not (xstr and ystr): return # if string is empty
    lcs = [''] #  longest common subsequence
    lcslen = 0 # length of longest common subsequence so far
    for i in xrange(len(xstr)):
        cs = '' # common subsequence
        start = 0 # start position in ystr
        for item in xstr[i:]:
            index = ystr.find(item, start) # position at the common letter
            if index != -1: # if common letter is found
                cs += item # add common letter to the cs
                start = index + 1
            if index == len(ystr) - 1: break # if reached to the end of ystr
        # updates lcs and lcslen if found better cs
        if len(cs) > lcslen: lcs, lcslen = [cs], len(cs)
        elif len(cs) == lcslen: lcs.append(cs)
    return lcs

file1 = open('/home/saji/file1')
file2 = open('/home/saji/file2')
xstr = file1.read()
ystr = file2.read()

start = time.time()
lcss = lcs(xstr, ystr)
elapsed = (time.time() - start)
print elapsed

The second one using hash table:

import time
from collections import defaultdict
def lcs(xstr, ystr):
    if not (xstr and ystr): return # if strings are empty
    lcs = [''] #  longest common subsequence
    lcslen = 0 # length of longest common subsequence so far
    location = defaultdict(list) # keeps track of items in the ystr
    i = 0
    for k in ystr:
        location[k].append(i)
        i += 1
    for i in xrange(len(xstr)):
        cs = '' # common subsequence
        index = -1
        reached_index = defaultdict(int)
        for item in xstr[i:]:
            for new_index in location[item][reached_index[item]:]:
                reached_index[item] += 1
                if index < new_index:
                    cs += item # add item to the cs
                    index = new_index
                    break
            if index == len(ystr) - 1: break # if reached to the end of ystr
        # update lcs and lcslen if found better cs
        if len(cs) > lcslen: lcs, lcslen = [cs], len(cs)
        elif len(cs) == lcslen: lcs.append(cs)
    return lcs

file1 = open('/home/saji/file1')
file2 = open('/home/saji/file2')
xstr = file1.read()
ystr = file2.read()

start = time.time()
lcss = lcs(xstr, ystr)
elapsed = (time.time() - start)
print elapsed

Firstly, your algorithm is incorrect try:

lcs("AAAABCC","AAAACCB"), the LCS should be "AAAACC", but your algorithm finds "AAAAB".

Secondly your algorithm is O(n^2*m) not O(n*m). Since you don't elaborate as to why you think your algorithm is theta(n*m) I can't really guess where your analysis has gone wrong.

Your second version attempts to optimize the process of searching through the string by using a list of pre-calculated positions. This means you don't have to scan through all the positions in the string with different characters. However, you lose the ability to skip all the position before your starting index. For long strings with few distinct characters, you end up losing out.


MVVM, WPF Ribbon V4, with Prism This weekend I've been having one heck of a time getting WPF Ribbon v4 working with MVVM and Prism (using unity). After much trial and error, I believe I have it working. I was hoping someone could take a look at it and give me some feedback.

RibbonRegionAdapter.cs

public class RibbonRegionAdapter : RegionAdapterBase<Ribbon>
{
    private Ribbon _ribbonTarget;

    public RibbonRegionAdapter(IRegionBehaviorFactory regionBehaviorFactory)
        : base(regionBehaviorFactory)
    {

    }

    protected override void Adapt(IRegion region, Ribbon regionTarget)
    {
        _ribbonTarget = regionTarget;

        region.Views.CollectionChanged += delegate {
            foreach (RibbonTab tab in region.Views.Cast<RibbonTab>())
            {
                if (!_ribbonTarget.Items.Contains(tab))
                {
                    _ribbonTarget.Items.Add(tab);
                }
            }
        };
    }

    protected override IRegion CreateRegion()
    {
        return new SingleActiveRegion();
    }
}

BootStrapper.cs - To register our regionAdapter

    protected override RegionAdapterMappings ConfigureRegionAdapterMappings()
    {
        RegionAdapterMappings mappings = base.ConfigureRegionAdapterMappings();

        if (mappings != null)
        {
            mappings.RegisterMapping(typeof(Ribbon), this.Container.Resolve<RibbonRegionAdapter>());
        }

        return mappings;
    }

CarRibbonTab.xaml

<ribbon:RibbonTab x:Class="CarManager.Modules.CarModule.Views.CarRibbonTab"
               xmlns:ribbon="clr-namespace:Microsoft.Windows.Controls.Ribbon;assembly=RibbonControlsLibrary"
             xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
             xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
             xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
             xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
             mc:Ignorable="d"
             Header="Officers">
    <ribbon:RibbonGroup Header="New">

    </ribbon:RibbonGroup>
    <ribbon:RibbonGroup Header="Manage">
        <ribbon:RibbonButton Label="Make"
                                LargeImageSource="..\Resources\make.png" />
        <ribbon:RibbonButton Label="Inventory"
                                LargeImageSource="..\Resources\Inventory.png" />
        <ribbon:RibbonButton Label="Assignments" />
    </ribbon:RibbonGroup>
</ribbon:RibbonTab>

CarRibbonTab.cs - Code behind for the View

public partial class CarRibbonTab: RibbonTab
{
    public CarRibbonTab()
    {
        InitializeComponent();
    }
}

Shell.xaml - Just showing the ribbon control

<ribbon:Ribbon DockPanel.Dock="Top" Title="CarManager" prism:RegionManager.RegionName="RibbonRegion">
    <ribbon:Ribbon.ApplicationMenu>
        <ribbon:RibbonApplicationMenu SmallImageSource="Images\Icon.png">
            <ribbon:RibbonApplicationMenuItem Header="Exit"
                                              ImageSource="Images\ExitIcon.png"/>
        </ribbon:RibbonApplicationMenu>
    </ribbon:Ribbon.ApplicationMenu>

</ribbon:Ribbon>

CarModule.cs - registering the view with the region

    public class CarModule: IModule
    {
        private readonly IRegionManager  _regionManager;
        private readonly IUnityContainer _container;
        public MenuItemViewModel MenuItem;

        public CarModule(IUnityContainer container, IRegionManager regionManager)
        {
            _container     = container;
            _regionManager = regionManager;
        }

        public void Initialize()
        {
            //Ribbon
            _container.RegisterType<Object, CarRibbonTab>("CarRibbonTab");

            _regionManager.AddToRegion("RibbonRegion", _container.Resolve<CarRibbonTab>());
        }
    }

Not sure whether we have many Prism specialists on Code Review and I haven't worked with it for a while, so maybe my comments are incorrect at points.

I do not really like the way you're handling region.Views.CollectionChanged event. This event provides a lot of information in eventArgs but you're ignoring it. Firstly you do not handle removing views at all. Secondly instead of iterating through all views every time I would use those from eventArgs (NewItems property).
I believe generally it is better to have views isolated from regions in terms of concrete types. Your view is not isolated since it has to inherit RibbonTab class. Maybe it is not an issue here and I'm not sure whether it should be changed somehow, it just looks strange to me.


Filter to convert duration to hours — in controller or model? I have this before_action method :
  def convert_duration_reminder
    allowed_duration_units = %(hour day)
    if allowed_duration_units.include? params[:store][:store_setting_attributes][:reminder_unit]
      store_settings_params = params[:store][:store_setting_attributes]
      @duration_reminder = store_settings_params[:reminder_duration].to_i.send(store_settings_params[:reminder_unit]).to_i / 3600
    else
      redirect_to store_settings_store_path(id: current_store.id)
    end
  end

I have a little bit of logic inside. The @duration_reminder is after used in an update_attributes.
Does it belongs in the controller or the model? I think it belongs in the controller because I'm using params and I have a redirect and methods are short (except line length...) and it doesn't process any data in database.
In the book Rails 4 action, it says:

Models contain the domain logic of your application. This logic
dictates how the records in your database are retrieved, validated, or
manipulated. In Rails appli- cations, models define the code that
interacts with the database’s tables to retrieve and set information
in them. Domain logic also includes things such as validations or
particular actions to be performed on the data.

Controllers interact with the models to gather information to send to the view. They’re the
layer between the user and the database. They call methods on the
model classes, which can return single objects representing rows in
the data- base or collections (arrays) of these objects. Controllers
then make these objects available to the view through instance
variables. Controllers are also used for permission checking, such as
ensuring that only users who have special permission to perform
certain actions can perform those actions, and users without that
permission can’t.

It could go either way; controller or model. Or a service object, though that'd just be a cop-out.

However, note that a "model" is just structured data and associated methods - it doesn't have to be in a database. It's just the most common use of models in a Rails app. But really, the MVC pattern itself doesn't care about databases; a model's a model.

But I'd say there are other issues to tackle first. Like line length. Also that the method is doing several things:

It's validating params
It's converting input
It's assigning values
It's redirecting

That's way too much, if you ask me, though the first two kinda go together. It's also got a magic number (though 3600 is a fairly well-known one), and a magic array.

You're also saying, in a comment, that it "converts time to minutes". But dividing seconds by 3600 yields hours - not minutes. So there's something weird there too.

Anyway, I'd do something like:

DURATION_UNITS = %w(day hour).freeze
SECONDS_PER_HOUR = 60 * 60

def reminder_duration
  duration = params[:store][:store_setting_attributes][:reminder_duration]
  unit = params[:store][:store_setting_attributes][:reminder_unit]
  if DURATION_UNITS.include? unit
    duration.to_i.send(unit).to_i / SECONDS_PER_HOUR
  end
end

You'll either get a number or nil back. You can then assign or redirect as you see fit.

Still not doing great on the line length, though.

I'd consider adding a (non-ActiveRecord) model called Store or Settings or something, and instantiate that with the params hash. Then let the class handle things from there. Something like:

class Store
  DURATION_UNITS = %w(day hour).freeze
  SECONDS_PER_HOUR = 60 * 60

  def initialize(params_hash)
    ...
  end

  def reminder_duration
    ...
  end
end

Then your controller might look like:

before_action :load_settings

def some_action
  if @settings.reminder_duration
    # do stuff
  else
    redirect_to ...
  end
end

private

def load_settings
  @settings = Store.new(params[:store]) # or something
end

Point is you add a level of abstraction and indirection. Your controller is no longer dealing with a complex params hash; that's all hidden away.

That being said, you've still got a really complex params hash, so that Store class (or whatever you call it) still risks becoming a ball of mud. There's only such much you can do to keep things straight; I'd recommend trying to pare that down to something more manageable first.


Parse a string as a list of words and parse characters I'm trying to create a list from a string with items alternating between words and parse characters like ['Hello', ' ', 'World']

Is there a built in function, existing module, or simpler way to achieve something like below? I'm interested in variable chars for parsing.

sample.txt

def parse_chars(string, chars):
    parse_set = {c for c in chars}
    string_list = []
    start = 0
    for index, char in enumerate(string):
        if char not in parse_set:
            if index - start > 0:
                word = string[start:index]
                string_list.append(word)
            string_list.append(char)
            start = index + 1
    document_len = len(string)
    if start != document_len:
        word = string[start:document_len]
        string_list.append(word)
    return string_list

filename = 'sample.txt'
with open(filename) as document_open:
    document_string = document_open.read()
alphanumeric = (map(chr, range(48, 58)) +
                map(chr, range(65, 90)) +
                map(chr, range(97, 123)))

print parse_chars(document_string, alphanumeric)

  [' ', 'A', ' ', 'space', ' ', 'then', ' ', '3', ' ', 'blank', ' ', 'lines', '\n', '\n', '\n', '3', ' ', 'blank', ' ', 'spaces', ' ', ' ', ' ', 'end']

The documentation for re.split says:

  If capturing parentheses are used in pattern, then the text of all groups in the pattern are also returned as part of the resulting list.

For example:

>>> import re
>>> re.split('( )', 'hello world')
['hello', ' ', 'world']

If the string starts or ends with a separator, you get an empty string:

>>> re.split('( )', ' a b c ')
['', ' ', 'a', ' ', 'b', ' ', 'c', ' ', '']

You probably don't want these empty strings, so you should filter them out:

>>> [w for w in re.split('( )', ' a b c ') if w]
[' ', 'a', ' ', 'b', ' ', 'c', ' ']

So your parse_chars function would become:

[w for w in re.split('([^0-9A-Za-z])', string) if w]

For example:

>>> [w for w in re.split('([^0-9A-Za-z])', '10 green bottles!') if w]
['10', ' ', 'green', ' ', 'bottles', '!']


Using CakePHP models/controllers properly I'd like to know if I'm doing this properly. Basically, I have 3 models: Student, Course, StudentCourse. I'm running into problems when I'm trying to sign a student up for a course (I've removed a lot of the non-important code - auth component, view, etc).

// Courses Controller (ie: /courses/signup/6)
class CoursesController extends AppController {
        public function signup($id = null) {
            $this->Course->id = $id;
            if (!$this->Course->exists()) {
                throw new NotFoundException(__('Invalid course'));
            }

            // Let's check to see if the user can signup for the course
            if($this->Course->canSignupForCourse($this->Auth->user('id'))) {

            } else {
                $this->Session->setFlash(__('You cannot signup for that course.'), 'flash_error');
                $this->redirect(array('controller' => 'pages', 'action' => 'calendar'));
            }
    }
}

// Course Model:
class Course extends AppModel {
    // Class 'hasMany' StudentCourse
    // Class 'habtm' Student

    public function isOwnedBy($course, $user) {
        return $this->field('id', array('id' => $course, 'student_id' => $user)) === $course;
    }

    public function isApproved() {
        $course = $this->read(null);

        if($course['Course']['approved_by'] != 0) {
            return true;
        }

        return false;
    }

    public function isComplete() {
        $course = $this->read(null);

        if($course['Course']['completed'] != 0) {
            return true;
        }

        return false;
    }

    public function canSignupForCourse($studentId) {

        if(! $this->StudentCourse->isSignedUpForCourse($studentId) && $this->isApproved() && $this->isComplete()) {
            return true;
        }

        return false;
    }

}

// Student Model
class Student extends AppModel {
    // Student 'hasMany' Course
    // Student 'hasMany' StudentCourse
}

// StudentCourse Model
class StudentCourse extends AppModel {
    // StudentCourse 'belongsTo' Student
    // StudentCourse 'belongsTo' Course
    public function isSignedUpForCourse($studentId = null, $courseId = null) {
        return $this->find('count', array('conditions' => array('StudentCourse.course_id' => $courseId, 'StudentCourse.student_id' => $studentId))) != 0 ? TRUE : FALSE;
    }
}

Basically, am I going about this the right way, are my relationships correct, or, is there a better/easier way to figure out if a student can signup for a course.

To me your code and relations looks good! You removed the redundancy in your database by splitting the tables up. Your cake really looks cakes way to me!

Just make sure you actually do something with the exception thrown and it should be all good.

Edit : I know this is a little late, but I wanted to complete my answer for the sake of completeness.

Your code style looks pretty good to me. The functions names are well chose. What I mean by that is by putting verb first you make the function a lot easier to find and understand.

Also, putting the logic in the model is the good choice. Continue to keep thin controllers.


The Beauty and the Strings This originally appeared in Facebook's hacker cup 2013. Solving it was fun, though I find myself looping through similar data quite often.

Challenge:

  Print the maximum beauty of strings.

Specifications:

  The beauty of a string is the sum of the beauty of the letters within.
  The beauty of each letter is an integer between 1 and 26, inclusive, and no two letters have the same beauty.
  Lettercase is irrelevant.
  Your program should accept as its first argument a path to a filename.
  Each line in this file has a sentence.
  Print out the maximum beauty of each sentence.

This means calculate the maximum possible beauty a string can have.
e.g. In test case, "AbBCcC" assign 26 to C, as it occurs the most in the string, followed by 25 to B, and 24 to A. For a maximum beauty of 152.

Solution:

import java.io.File;
import java.io.FileNotFoundException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Scanner;
import java.util.Set;

public class BeautifulStrings {
    public static void main(String[] args) throws FileNotFoundException {
        Scanner input = new Scanner(new File(args[0]));

        while (input.hasNextLine()) {
            printMaximumBeauty(input.nextLine());
        }
    }

    private static void printMaximumBeauty(String line) {
        System.out.println(
            computeMaximumBeauty(line
                .replaceAll("[^a-zA-Z]", "")
                .toLowerCase()
                .toCharArray()
            )
        );
    }

    private static int computeMaximumBeauty(char[] line) {
        int beauty = 0;
        int beautyVal = 26;
        int count = 0;
        Set<Character> uniqueCharacters = new HashSet<>();
        List<Integer> appearanceCounts = new ArrayList<>();

        for (char c : line) {
            uniqueCharacters.add(c);
        }

        for (char u : uniqueCharacters) {
            for (char c : line) {
                if (u == c) {
                    count++;
                }
            }

            appearanceCounts.add(count);
            count = 0;
        }

        Collections.sort(appearanceCounts, Collections.reverseOrder());

        for (int i : appearanceCounts) {
            beauty += i * beautyVal--;
        }

        return beauty;
    }
}

In an attempt to reduce the amount of loops and work done I thought of a different implementation for computeMaximumBeauty. I think maybe there's a way to do it all in one stream, but I'm not yet well versed with Java 8 (unfortunately all the Java challenges from the challenge site, CodeEval, require Java 7). This isn't a comparative review, so feel free to pick the better one (or both) and pick at its weaknesses or suggest complete alternatives if this approach misguided.

private static int computeMaximumBeauty(char[] chars) {
        int beauty = 0;
        int beautyVal = 26;
        Map<Character, Integer> beautyMap = new HashMap<>();
        Map<Character, Integer> valueSorted;

        for (char c : chars) {
            beautyMap.put(
                c, beautyMap.containsKey(c) ? beautyMap.get(c) + 1 : 1
            );
        }

        valueSorted =  beautyMap.entrySet().stream()
            .sorted(comparing(Entry::getValue))
            .collect(toMap(
                Entry::getKey,
                Entry::getValue,
                (e1, e2) -> e1, LinkedHashMap::new
            )
        );

        beautyVal -= valueSorted.size() - 1;

        for (Map.Entry<Character, Integer> e : valueSorted.entrySet()) {
            beauty += e.getValue() * beautyVal++;
        }

        return beauty;
    }

Sample Input:

ABbCcc
This is from Facebook Hacker Cup 2013.
Ignore punctuation, please :)
Sometimes, test cases are hard to make up.
CodeReview is love. CodeReview is life.

Sample Output:

152
551
491
729
724

Unit testing

Hi again @Legato, I'll probably bug you with unit testing until you start writing them ;-)

@Test
public void test_ABbCcc() {
    assertEquals(152, computeMaxBeauty("ABbCcc"));
}

@Test
public void test_This_is_from_Facebook_Hacker_Cup_2013_() {
    assertEquals(551, computeMaxBeauty("This is from Facebook Hacker Cup 2013."));
}

@Test
public void test_Ignore_punctuation__please___() {
    assertEquals(491, computeMaxBeauty("Ignore punctuation, please :)"));
}

@Test
public void test_Sometimes_test_cases_are_hard_to_make_up_() {
    assertEquals(729, computeMaxBeauty("Sometimes, test cases are hard to make up."));
}

@Test
public void test_CodeReview_is_love__CodeReview_is_life_() {
    assertEquals(724, computeMaxBeauty("CodeReview is love. CodeReview is life."));
}

The first implementation

It's good that the logic is decomposed to 2 methods (as opposed to not decomposed at all).

The printMaximumBeauty method converts the input string to char[].
I think it would be better to pass the string to computeMaximumBeauty and let it do whatever it wants with it.
For the record, there's a trade-off when converting to a char[] as opposed to accessing characters with .charAt(index):

Converting to char[] creates a new array, so doubles the space
Accessing characters with .charAt(index) incurs a boundary check every time

The computeMaximumBeauty method is very inefficient:

It iterates over the characters once to find unique ones
For each unique character it iterates over the characters again to count

You could combine these two steps.

You also incur some performance penalty due to autoboxing / unboxing, but that may be negligible.

Lastly, don't declare variables at the top. Java is not C.
Declare variables the latest possible, and in the smallest scope possible.
For example count could be declared in the for loop where it's used,
and then you wouldn't need to reset it in every cycle.

The second implementation

This method counts the appearances better, by doing it one pass.
Note that this line will do two map lookups when c is already in the map:

beautyMap.containsKey(c) ? beautyMap.get(c) + 1 : 1

First in .containsKey, then again in .get.
To reduce the lookups, you can do one .get and a null-check.

When calculating the beauty, you iterate over entries, but use only the values:

for (Map.Entry<Character, Integer> e : valueSorted.entrySet()) {
    beauty += e.getValue() * beautyVal++;
}

If you don't need the keys, then it would be better to iterate over the values instead of the entries.
And if you don't need the keys here, then probably you don't need them at all.
So instead of a map, a list would have been enough, and probably simplify your logic.

This implementation also declares variables at the top, which is not so good.
And it also incurs some performance penalties due to autoboxing / unboxing.

Suggested implementation

Here's an alternative implementation that's simpler.
It uses the fact that there can be at most 26 unique values,
so the appearance counts can be stored in a simple int[].

private int computeMaxBeauty(String text) {
    String sanitized = text.replaceAll("[^a-zA-Z]", "").toLowerCase();

    int[] counts = new int[26];
    for (int i = 0; i < sanitized.length(); ++i) {
        ++counts[sanitized.charAt(i) - 'a'];
    }

    Arrays.sort(counts);

    int sum = 0;
    for (int i = 25; i >= 0 && counts[i] > 0; --i) {
        sum += counts[i] * (i + 1);
    }
    return sum;
}


HackerRank - Filter huge list for elements that occur more than a given number of times I'm trying to solve this problem on HackerRank, which basically requires the solution to read an input like this (comments are mine)
3                      # the number of cases, each of which consists of a pair of lines below
9 2                    # case 1, the two numbers are N and K
4 5 2 5 4 3 1 3 4      # case 1, the N numbers; filter out those that occur less than K times
9 4                    # case 2
4 5 2 5 4 3 1 3 4      # case 2
10 2                   # case 3
5 4 3 2 1 1 2 3 4 5    # case 3

and the desired output is
4 5 3
-1
5 4 3 2 1

where the second line signifies that there's no number in case 2 which occurs 4 times or more, first and third lines list those elemnts of cases 1 and 3 which all occur the prescribed number of times or more (in this case the threshold is 2 for both case 1 and case 2).
I came up with an algorithmically correct solution, but apparently it has poor performance, as it fails for timeout on this input (expected output)
import Control.Monad (liftM2)
import Data.List (sort,groupBy)
import Data.List (foldl')
main :: IO()
main = do
  nCasesStr <- getLine
  let nCase = read nCasesStr :: Int
  sequence_ $ replicate nCase $ do
    firstLine <- getLine
    secondLine <- getLine
    let [_,k] = asInts $ firstLine
    let list  = asInts $ secondLine      -- LINE A
    let outList = processList k list
    if null outList
      then putStr $ show (-1)
      else mapM_ (putStr . (++" ") . show) outList -- LINE B
    putStrLn ""
    where
      asInt :: String -> Int
      asInt s = read s :: Int
      asInts :: String -> [Int]
      asInts = map asInt . words

processList :: Int -> [Int] -> [Int]
processList k = map fst . filter ((>= k) . snd) . foldl' step []
  where
    step acc@((n,c):ac) val
      | n == val = (n,c + 1):ac
      | otherwise = (n,c):step ac val
    step _ val = [(val,1)]

I'm especially worried about the lines marked as -- LINE A ad -- LINE B: they both traverse the full long input and output lines respectively; then the processList function traverses the input too. Traversing the input only once while spitting out output, however, would not change the BigO complexity, but just reduce it by a factor of 3, am I right?
As regards the processList function, I'm quite happy I had the idea of using a fold (the three of them don't seem to have a consistent difference in performances, as regards the bulk test cases), but maybe I've chosen the wrong stepping function?
Or maybe folding is not the right tool in this case?

It seems that the suggestion I got in the comment is enough to pass the HR test:
import Data.Maybe (fromJust)
import Data.List (foldl', nub)
import qualified Data.Map as M

main :: IO()
main = do
  nCasesStr <- getLine
  let nCase = read nCasesStr :: Int
  sequence_ $ replicate nCase $ do
    k    <- (str2int. last . words) <$> getLine
    list <-    map str2int . words  <$> getLine
    let outList = processList k list
    if null outList
      then putStr $ show (-1)
      else mapM_ (putStr . (++" ") . show) outList
    putStrLn ""
    where
      str2int = read :: String -> Int

processList :: Int -> [Int] -> [Int]
processList k = nub . (filter <$> (flip (\x -> (k <=) . fromJust . M.lookup x . list2map)) <*> id)
  where
    list2map = foldl' step M.empty
    step map num = M.insertWith (+) num 1 map


Nested Skips (UPenn) Exercise 1: skips :: [a] -> [[a]]

  The nth list in the output should contain every nth element from the
  input list. For example, skips "hello!" == ["hello!", "el!", "l!",
  "l", "o", "!"].

The question UPenn Homework 3: skips function and it's associated answers inspired me to write a solution in Javascript. I used the opportunity to (finally) better understand high order array functions. If you would be so kind to critique:

Code Layout, specifically if nesting function skips() makes any sense and why.
Do higher order functions provide more than just readability, are they more efficient?
Can I make the nested function skips() better (readable/faster)?
Anything at all.

function skip(str, cb) {
  var input = str.split('');

  cb(input.map(skips));

  function skips(val, index, arr) {
    var res = [];
    var i = index;
    var iterator = i + 1;
    for (i; i < arr.length; i += iterator){
      res.push(arr[i]);
    }
    return res;
  }
}

skip('hello!', console.log);

Ok, first off: Passing in console.log as a paramater seems unnecessary. The function should have one task:

  Generate the skips

Then once you have the skips you can do whatever you want with it. So:

function skip(str, cb) -> function skip(str)

and:

skip('hello!', console.log); -> console.log(skip('hello'));

I would also not use a function within a function. I would try keeping the functions relatively small. Split the functions:

function skipHelper(val, index, arr) {
  var res = [];
  var i = index;
  var iterator = i + 1;
  for (i; i < arr.length; i += iterator){
    res.push(arr[i]);
  }
  return res;
}

function skip(str) {
  var input = str.split('')
  return input.map(skips);
}

Also, given that str.split('') isn't too much to explicitly write out, change:

  var input = str.split('')
  return input.map(skips);

To just:

  return str.split('').map(skips);

Also, your code doesn't run exactly as described. Add a .join('') to return res; and this should fix it:

return res; -> return res.join('');

All in all:

function skipHelper(val, index, arr) {
  var res = [];
  var i = index;
  var iterator = i + 1;
  for (i; i < arr.length; i += iterator){
    res.push(arr[i]);
  }
  return res.join('');
}

function skip(str) {
  return str.split('').map(skipHelper);
}

console.log(skip('hello!'));

There are probably some Javascript language specific stylistic choices I am missing, but this is for general code structure.


Multi jQuery Sliders, Help me write this in better code I have three sliders with different min, max values, everything is working fine from following the tutorials on the jQuery UI website.

$("#yield").slider({
    range: true,
    min: 15.2,
    max: 308,
    step: 0.3,
    values: [75.8, 241.4],

    slide: function (event, ui) {
        $("#yield-value").val(ui.values[0] + " - " + ui.values[1]);
    }
});
$("#yield-value").val($("#yield").slider("values", 0) + " - " + $("#yield").slider("values", 1));

$("#tensile").slider({
    range: true,
    min: 37.7,
    max: 345,
    step: 0.3,
    values: [81.2, 243.3],

    slide: function (event, ui) {
        $("#tensile-value").val(ui.values[0] + " - " + ui.values[1]);
    }
});
$("#tensile-value").val($("#tensile").slider("values", 0) + " - " + $("#tensile").slider("values", 1));

$("#elongation").slider({
    range: true,
    min: 1.0,
    max: 77,
    step: 0.1,
    values: [14.5, 43.2],

    slide: function (event, ui) {
        $("#elongation-value").val(ui.values[0] + " - " + ui.values[1]);
    }
});
$("#elongation-value").val($("#elongation").slider("values", 0) + " - " + $("#elongation").slider("values", 1));

here is my working fiddle here - http://jsfiddle.net/barrycorrigan/Sqg8A/1/

My problem is that I think my jQuery is written badly for multiple sliders.

Can anyone help me write this code better. Because there is only 3 sliders in the example but on the actual client website there is still 10 to go.

Any help to make my code cleaner somehow would be greatly appreciated.

When you have code that repeats itself like that, you want to try and separate the logic from the values. Something like this would work, and you just have to add more to the settings object when needed. It's not jQuery that's badly written, you just have to change the way you're using it. http://jsfiddle.net/Sqg8A/5/

$(function() {
    var settings = {
        "yield" : {
            range: true,
            min : 15.2,
            max : 308,
            step : 0.3,
            values : [75.8, 241.4]
        },
        "tensile" : {
            range: true,
            min : 37.7,
            max : 345,
            step : 0.3,
            values : [81.2, 243.3]
        },
        "elongation" : {
            range: true,
            min: 1.0,
            max: 77,
            step: 0.1,
            values: [14.5, 43.2]
        } //Add more if needed
    }

    $.each(settings, function(key, value){
        var _key = key;
        $( "#" + _key ).slider(
            $.extend(value, {
                slide: function( event, ui ) {
                    $( "#" + _key + "-value" ).val( ui.values[ 0 ] + " - " + ui.values[ 1 ] );
                }
            })
        );
        $( "#" + _key + "-value" ).val( $( "#" + _key ).slider("values", 0) + " - " + $( "#" + _key ).slider("values", 1));
    });
});


Time limit exceeded for Sherlock and Queries Hackerrank challenge Here is my C solution for Sherlock and Queries @ Hackerrank.

  Watson gives to Sherlock an array: \$A_1, A_2, ..., A_N\$. He also gives to
  Sherlock two other arrays: \$B_1, B_2, ..., B_M\$ and \$C_1, C_2, ..., C_M\$.

  Then Watson asks Sherlock to perform the following program:

for i = 1 to M do
    for j = 1 to N do
        if j % B[i] == 0 then
            A[j] = A[j] * C[i]
        endif
    end do
end do

  Can you help Sherlock and tell him the resulting array A? You should
  print all the array elements modulo \$(10^9 + 7)\$.

  Input Format

  The first line contains two integer numbers \$N\$ and \$M\$. The next line
  contains \$N\$ integers, the elements of array A. The next two lines
  contains \$M\$ integers each, the elements of array B and C.

  Output Format

  Print \$N\$ integers, the elements of array A after performing the program
  modulo \$(10^9 + 7)\$.

  Constraints

  \$1 ≤ N, M ≤ 10^5\$
   \$1 ≤ B[i] ≤ N\$
   \$1 ≤ A[i], C[i] ≤ 10^5\$

  Sample Input

4 3
1 2 3 4
1 2 3
13 29 71

  Sample Output

13 754 2769 1508

How should I optimize it as it is showing teminated due to time out?

#include<stdio.h>

int main()

{

    int n,m,i,j;

    scanf("%d %d ",&n,&m);

    long int a[n+1],b[m+1],c[m+1];
    for(i=1;i<=n;i++)
        scanf("%ld",&a[i]);

    for(i=1;i<=m;i++)
        scanf("%ld",&b[i]);

    for(i=1;i<=m;i++)
        scanf("%ld",&c[i]);

    for(i=1;i<=m;i++)
    {
        for(j=1;j<=n;j++)
        {
            if(j%b[i]==0)
            {
                a[j]=(a[j]*c[i])%1000000007;
            }
        }

    }

    for(i=1;i<=n;i++)
        printf("%ld ",a[i]%1000000007);

        return 0;

}

The time complexity of your solution is
\$ O(NM) \$. It is too slow. You need a more efficient algorithm. Here is a pseudo code of a better one:

// Reads the input.
n = read_input()
m = read_input()
a = read_input()
b = read_input()
c = read_input()
// Creates an auxilary array to remove duplicates from b
totalProduct = an array of n + 1 elements filled with 1
for (i = 1; i <= m; i++)
    if (b[i] <= n)
        totalProduct[b[i]] = totalProduct[b[i]] * c[i] % MOD
for (divisor = 1; divisor <= n; divisor++)
    // Iterates over all number that are divisible by the divisor
    for (number = divisor; number <= n; number += divisor)
        a[number] = a[number] * totalProduct[divisor] % MOD
print(a)

The correctness of this algorithm is obvious (it simply does what is written in the problem statement). So why is it efficient? It is fast because its time complexity is $$ O\big(M + { N \over 1 } + { N \over 2 } + { N \over 3 } + \dotsb + { N \over N }\big) = O(M + N \log N). $$
Why do we need to remove duplicates from b? If we don't do it, the time complexity will be \$ O(M + N^2) \$ if all elements of b are equal to 1.


A 'slow sort': repeatedly swap the first misordered pair A 'slow sort' is probably the slowest practical sort. It compares each element with the elements after it, and swaps immediately if one is greater than the other, and goes back to the start of the array.

For example:

  CBAD -> BCAD -> ACBD -> ABCD

My code:

function go() {
   var arrLen=Math.floor(Math.random()*20)+30;
   var arr=Array.from({length:arrLen}).map(()=>Math.floor(Math.random()*20)); // make random array
   txtIn.textContent=arr; //display
   //txtOut1.textContent=arr.sort(function(a,b) {return -(aa-b); // native sort
   var srcPos=0,dstPos,tmp; // declare variables
   while (srcPos<arrLen-1) {
        dstPos=srcPos+1;
        while (dstPos<arrLen) { // three swap routines
             if (arr[srcPos]>arr[dstPos]) {tmp=arr[srcPos];arr[srcPos]=arr[dstPos];arr[dstPos]=tmp;srcPos=0;break}
             //if (arr[srcPos]>arr[dstPos]) {arr[srcPos]^=arr[dstPos];arr[dstPos]^=arr[srcPos];arr[srcPos]^=arr[dstPos];srcPos=0;break}
             //if (arr[srcPos]>arr[dstPos]) {arr[srcPos]+=arr[dstPos];arr[dstPos]=arr[srcPos]-arr[srcPos];arr[srcPos]+=arr[dstPos];srcPos=0;break}
             srcPos++;
             dstPos++;
             }
       }
   txtOut2.textContent=arr; //display
}
data

native sort

slowsort

<button onclick='go();'>go</button>

I have included three swap types, tmp, XOR and add, and commented out two of them. The native sort includes a EMCA2015 arrow function version.

It seems to work okay - admittedly I haven't tested it for other data types, but any feedback would be appreciated.

There are a few terms one can use to grade code. Correctness, efficiency, readabilty, reuse testability and few other objectively measurable metrics come to mind.

Efficiency isn't really a concern here, and due to the simplicity of the algorithm, neither is correctness. However, it was hard to check whether the code is correct because it's nearly not readable. Also, it's not reusable, at all, as the sorting method is embedded inline and therefore not testable.

With this small overview in mind, let's review the code in detail.

Correctness

The outer while loop will only exit when srcPos == arrLen - 1. This may only happen if srcPos does not get set to 0, which again only happens if two elements weren't in order. Therefore when we have srcPos == arrLen - 1, all elements where lesser or equal to their predecessors and the array is sorted.

Efficiency

This was an inverse goal. While it's already slow, I accidentally misread the code (see the next section) and read:

...
   var srcPos=0,dstPos,tmp; // declare variables
   while (srcPos<arrLen-1) {
        dstPos=srcPos+1;
        while (dstPos<arrLen) { // three swap routines
             if (arr[srcPos]>arr[dstPos]) {tmp=arr[srcPos];arr[srcPos]=arr[dstPos];arr[dstPos]=tmp;srcPos=0;break}
             //if (arr[srcPos]>arr[dstPos]) {arr[srcPos]^=arr[dstPos];arr[dstPos]^=arr[srcPos];arr[srcPos]^=arr[dstPos];srcPos=0;break}
             //if (arr[srcPos]>arr[dstPos]) {arr[srcPos]+=arr[dstPos];arr[dstPos]=arr[srcPos]-arr[srcPos];arr[srcPos]+=arr[dstPos];srcPos=0;break}

             dstPos++;
             }
             srcPos++; // whoops!
       }
 ...

That's even worse, since we end up with a lot of unnecessary comparisons. See "But this can be made worse" below.

It's already slow...

Your code restarts for every inversion and end up with \$\mathcal O(n^3)\$. That's already bad, but you can do worse.

But this can be made worse

Just increase src only in the outer while. Here's the analysis for the case:
The worst runtime we can get is for an inverse array, e.g. [5,4,3,2,1]:

[5,4,3,2,1]
[4,5,3,2,1]
[3,5,4,2,1]
[2,5,4,3,1]
[1,5,4,3,2]
...

For \$n\$ elements, we need \$1\$ comparison to move the largest element to the second position, \$2\$ elements to move the second largest to the third, and so on. In summary, we have $$1 + 2 + \ldots + (n-2) + (n - 1) = \frac{n(n-1)}{2}$$ comparisons to get the smallest value to the front. However, contrary to bubble sort, we always reset srcPos to zero. Therefore, the second smallest value won't take less time to get to the front:

[1,4,5,3,2] -- 4 comparisons for 1, 1 comparison for 5-4
[1,3,5,4,2] -- 4 comparisons for 1, 2 comparisons for 4-5, 4-3
[1,2,5,4,3] -- 4 comparisons for 1, 3 comparisons for 3-5, 3-4, 3-2

Although we should be able to only look at the subarray starting at index one, we always have the initial 4 comparisons. So for the second element, we have

$$((n-1)+1) + ((n-1)+2) + \ldots + ((n-1)+(n-3)) + ((n - 1)+(n-2))
 = (n-1)(n-2) + \frac{(n-1)(n-2)}{2}$$
comparisons. It's similar for the rest:

[1,2,5,4,3]
[1,2,4,5,3] -- 4 comparisons for 1, 3 comparisons 2, 1 comparison for 5-4
[1,2,3,5,4] -- 4 comparisons for 1, 3 comparisons 2, 2 comparison for 4-5, 4-3

If we complete the induction, we get

$$ \sum_{i=0}^n \frac{(n-i)(n-i-1)}{2} + (n-i-1)\sum_{j=1}^i (n-j) $$
which is inefficient enough. Good job there :).

Readability, reuse and testability

Readability really suffers since the swap logic is put into a single line. Also, there are several values with short names that are not related to your algorithm at all.

Furthermore, we cannot use the sort outside of that snippet. That prevents

sorting more than one array
custom sort (for example reverse, lexicographic, etc.)
testing

So first of all let's make a function:

function slowSort(arr) {
    var src = 0;
    var dest;
    var tmp;

    while(src < arr.length - 1) {
        dest = src + 1;
        while(dest < arr.length) {
            if(arr[src] > arr[dest]) {
                temp = arr[dest];
                arr[dest] = arr[src];
                arr[src] = temp;
                src = 0;
                break;
            }
            dest++;
            src++; // place this one in the outer to make it worse, but
                   // keep in mind to set `src = -1` instead.
        }
    }
}

That's a lot easier to read. Also, we can now

sort multiple times
get improvements/speed regressions for every call site
add additional comparison methods, e.g.

function slowSort(arr, comp) { ... }

test the function

Bottom line

Always try to make your code reusable, testable and readable.


Shorten JavaScript Code due to duplicate variables without polluting global namespace I am at a loss as to how to simplify the following two functions that share variables without polluting the global namespace. (Partial code for a carousel). I would like to streamline it.

Note:
snapshotCheck(); is called once on window load.
snapshotScroll(); is called on click events.

//FIRST FUNCTION
function snapshotScroll(amount, refresh) {
  var imageSum = snapshots.total_entries; //Inner Box
  var dl = $j('#snapshots')[0]; //Outer Box
  var c = $j('#photo-carousel')[0];
  var l = ((dl.style.left) ? parseInt(dl.style.left) : 0);
  var pos = l + parseInt(amount);
  var pos0 = pos;
  var viewport = $j('#photo-carousel')[0];
  var inView = Math.floor(viewport.clientWidth / snapshotWidth); //How many snapshots in view
  var maxPos = (imageSum * -snapshotWidth);
  if(pos > 0) { // prevent left side from going too far right
      pos = 0;
  } else if(pos < c.clientWidth - dl.clientWidth) {
      pos = c.clientWidth - dl.clientWidth;
      if(pos == 8) { // To account for 8px variation, set position to initial pos0 for animation.
          pos = pos0;
      }
  }

  $j(dl).animate({"left" : pos}, 1200); // Animate scroll
  // Toggle "filter" class (reduces opacity) depending on position.
  // if position is max, toggleClass of #next.
  // if position is 0, toggleClass of #prev.
  $j('#next').toggleClass('filter', (pos - maxPos) == (c.clientWidth))
  $j('#prev').toggleClass('filter', (pos >= 0))

  if(imageSum > inView) {
     dl.style.width = maxPos * -1 + "px";
  }
} // end snapshotScroll();

//SECOND FUNCTION
function snapshotCheck(){ // Check if snapshot is in view
  var c = $j('#photo-carousel')[0]; // #snapshots parent container
  var dl = $j('#snapshots')[0]; // ol container holding snapshots
  var imageSum = snapshots.total_entries;
  var l = ((dl.style.left) ? parseInt(dl.style.left) : 0);
  var maxPos = (imageSum * -snapshotWidth);
  var setWidth;
  var viewport = $j('#photo-carousel')[0]
  var inView = Math.floor(viewport.clientWidth / snapshotWidth); // # of snapshots in view
  (setWidth = function(){ // set initial width in order to retrieve position of currentSnapshot
    if(imageSum > inView) {
      dl.style.width = maxPos * -1 + "px";
    }
  })();
  if(typeof currentSnapshot != 'undefined') {
    var aSnapshot = $j('#' + currentSnapshot.snapshot.id + '_slide');  // jQuery object, array
    var theSnapshot = $j('#' + currentSnapshot.snapshot.id + '_slide')[0];  // first DOM element
    var pos = -aSnapshot.position().left; // a negative value, set to first position carousel (in view)
    $j(dl).animate({"left" : pos}, 1200); // Animate scroll
  }
  $j('#next').toggleClass('filter', (pos - maxPos) == (c.clientWidth))
  $j('#prev').toggleClass('filter', (pos >= 0))
  } // end snapshotCheck();

If those variables are constant (ie, they don't change between invocations of of the functions), you can put the functions and variables into a closure:

(function() {
    //FIRST FUNCTION
    var imageSum = snapshots.total_entries; //Inner Box
    var dl = $j('#snapshots')[0]; //Outer Box
    ...
    window.snapshotScroll = function (amount, refresh) {
        ...
    } // end snapshotScroll();

    //SECOND FUNCTION
    window.snapshotCheck = function (){ // Check if snapshot is in view
        ...
    } // end snapshotCheck();
})();


Printing out a binary tree level by level Below is my implementation for an interview question of printing a binary tree level by level. I've also included some methods for creating a binary tree from a vector in my solution.  Please review my solution.

#include <iostream>
#include <unordered_map>
#include <vector>
#include <algorithm>
#include <type_traits>

template <typename T>
struct Node
{
  T data;
  Node<T>* left;
  Node<T>* right;

  Node (T d, Node<T>* l = nullptr, Node<T>* r = nullptr) : data(d), left(l), right(r) {}
};

template<typename T>
void deleteTree ( Node<T>* n)
{
  if ( !n )
    return;
  deleteTree(n->left);
  deleteTree(n->right);
  delete n;
}

template <typename T>
void printBinaryTree (Node<T>* n )
{
  std::unordered_map<int, std::vector<Node<T>*> > m;
  printBinaryTree (n, m, 0);
  for ( auto& i : m )
  {
    std::cout << " Level " << i.first << ": ";
    for ( auto& j : i.second )
      std::cout << j->data << "  ";
  }
  std::cout << std::endl;
}

// print a binary tree level by level
template <typename T>
void printBinaryTree (Node<T>* n, std::unordered_map<int, std::vector<Node<T>*> >& m, int level )
{
  if (n)
  {
    ++level;
    printBinaryTree ( n->left ,m,level);
    printBinaryTree ( n->right,m,level);

    m[level].push_back(n);
  }
}

template <typename ItType>
auto buildBinaryTree (ItType start, ItType end) -> Node<typename std::iterator_traits<ItType>::value_type>*
{
  using T =typename std::iterator_traits<ItType>::value_type;
  auto first = start;
  auto last = std::prev(end);

  if ( first > last ) // error case
    return nullptr;
  else if ( first < last )
  {
    auto mid = first +  ( last - first ) / 2; // avoid overflow
    Node<T>* n = new Node<T>(*mid);
    n->left = buildBinaryTree ( first, mid);
    n->right = buildBinaryTree ( mid+1, end);
    return n;
  }

  // equal
  return new Node<T>(*first);
}

template <typename T>
Node<T>* buildBinaryTree (std::vector<T>& v)
{
  if  ( v.empty() )
   return nullptr;

  std::sort(v.begin(), v.end());
  return buildBinaryTree (v.begin(), v.end());
}

int main()
{
  std::vector<int> v { 1,2,3,4,5,6,7,8,9,10 };
  Node<int>* root = buildBinaryTree ( v );
  printBinaryTree (root);
  deleteTree(root);
}

I got the following output:

Level 1: 5   Level 2: 2  8   Level 4: 4  7  10   Level 3: 1  3  6  9

Printing the layers in a non-deterministic order is probably undesirable behaviour.  You used an unordered_map, which guarantees the order of neither its keys nor its values.  To impose order on the values (the nodes of each level), you store a vector in the map, which complicates the data structure.

The traditional way to do a breadth-first traversal of a tree is to use a queue, and to work iteratively on elements of the queue rather than recursively on elements of the tree.  Not only is this a less complex data structure than an unordered map of ints to vectors of nodes, it also stores nodes from no more than two levels of the tree at a time, rather than all nodes of the tree at once.

#include <queue>
#include <utility>   // for std::pair

template <typename T>
void printBinaryTree(const Node<T> *n) {
    if (nullptr == n) {
        return;
    }
    int level = 0;

    // Use a queue for breadth-first traversal of the tree.  The pair is
    // to keep track of the depth of each node.  (Depth of root node is 1.)
    typedef std::pair<const Node<T>*,int> node_level;
    std::queue<node_level> q;
    q.push(node_level(n, 1));

    while (!q.empty()) {
        node_level nl = q.front();
        q.pop();
        if (nullptr != (n = nl.first)) {
            if (level != nl.second) {
                std::cout << " Level " << nl.second << ": ";
                level = nl.second;
            }
            std::cout << n->data << ' ';
            q.push(node_level(n->left,  1 + level));
            q.push(node_level(n->right, 1 + level));
        }
    }
    std::cout << std::endl;
}

As a final touch, pay attention to the const-correctness of the function's parameter.


Solution to Max Range Sum challenge on CodeEval The challenge in CodeEval is to take a number of days and iterate over the gains and losses and determine which lengths will yield the highest possible profit.  I'm wondering if I could write this any better?  It seems like sort of clunky code to me.

file = ARGV[0]

File.open(file).each do |line|

    duration, days = line.chomp.split(";")
    days = days.split(" ").map! { |num| num = num.to_i }
    duration = duration.to_i - 1

    profit = 0
    max_profit = 0

    days[0..(days.length - duration)].each_with_index do |day, index|

        days[index..(index + duration)].inject(0) do |change, total|
            profit = change + total
        end

        max_profit = profit if profit > max_profit
    end

    puts max_profit

end

Just for clarity, I'd split the code into a couple of methods. One to parse input, and one to find the best yield.

To tackle the latter first: Have a look at Enumerable#each_cons, and Enumerable#reduce (aka inject).

Given an array of gains/losses (values) and the length to examine (days), you can find the "best streak" like so:

values.each_cons(days).map { |streak| streak.reduce(&:+) }.max

In English: Take each consecutive "streak" of days values; map each of these streaks to its sum; take the maximum of these sums. As an example:

days = 3
values = [1, 2, -1, 3, 4]

streaks = values.each_cons(days).to_a              # => [ [1, 2, -1], [2, -1, 3], [-1, 3, 4] ]
sums = streaks.map { |streak| streak.reduce(&:+) } # => [2, 4, 6]
max = sums.max                                     # => 6

There are two other things to handle, to solve the task: Return zero if the best gain is actually a loss, or if there are not enough values to satisfy days.

In all, you might do something like:

def best_streak(days, values)
  return 0 if values.count < days
  gain = values.each_cons(days).map { |streak| streak.reduce(&:+) }.max
  gain > 0 ? gain : 0
end

As for parsing the input, your current code is ok, though I'd still handle it separately.

def parse_line(line)
  days, *values = line.strip.split(/[;\s]/)
  [days.to_i, values.map(&:to_i)]
end

This basically just splits the line in to its parts, treating the first number as the number of days, and the rest as the values.

To put it all together, you can do something like:

File.open(ARGV[0]).each do |line|
  days, values = parse_line(line)
  puts best_streak(days, values)
end

However, it seem that in the CodeEval context, input is passed on STDIN; not in a file. So really it should probably be:

STDIN.each_line do |line|
  days, values = parse_line(line)
  puts best_streak(days, values)
end

There may be a more efficient way to do all this, but this is probably the most readable.


Lock Using "Interlocked" vs lock Statement ("Monitor") A followup question can be found here, based on this question and it's answer.

Considering:

Interlocked.CompareExchange generates a memory barrier,
Intended to be used with very fast operations (like generating an
id),
SpinWait starts sleeping in milliseconds after 10 or so spins (and
does some other smart stuff),

What are characteristics (pros & cons) of this code?

public class SpinMutex
{
    const long Locked = 1;
    const long Unlocked = 0;

    long _locked;

    public SpinMutex() { _locked = Unlocked; }

    public void Lock()
    {
        SpinWait.SpinUntil(() => Interlocked.CompareExchange(ref _locked, Locked, Unlocked) == Locked);
    }

    public bool Lock(int millisecondsTimeout)
    {
        return SpinWait.SpinUntil(() => Interlocked.CompareExchange(ref _locked, Locked, Unlocked) == Locked, millisecondsTimeout);
    }

    public bool Lock(TimeSpan timeout)
    {
        return SpinWait.SpinUntil(() => Interlocked.CompareExchange(ref _locked, Locked, Unlocked) == Locked, timeout);
    }

    public void Unlock()
    {
        Interlocked.Exchange(ref _locked, Unlocked);
    }
}

And some extra points to consider:

Does it help if SpinMutex be defined as struct instead of a
class (I'm aware there would be hurdles with say readonly fields
of struct type and the like)?
Should Interlocked.CompareExchange be used in Unlock too, instead of
Interlocked.Exchange to ensure generating of a memory barrier? Or
Interlocked.Exchange generates a memory barrier itself? Is a memory barrier needed at all at Unlock?

First and foremost I think your implementation is broken: Interlocked.CompareExchange returns the original value at the location. So in your lock methods you should spin until the original value was Unlocked and not Locked.

As for your first question: Besides that "better" is a very vague term - no I don't think making it a struct is better for the simple reason that you have a mutable structure (it can change it's own state between locked and unlocked) and mutable structs are generally best to be avoided - the value semantics of it can trip you up pretty quick.

For your second question: As per MSDN all Interlocked methods make use of the appropriate barriers.

In general:

I would check that the lock was actually locked in Unlock (and throw if it wasn't) - unlocking an already unlocked lock indicates a potentially serious bug (lock/unlock should always be in matching pairs).
I'd consider implementing IDisposable and check that the lock is not locked when being disposed. Any object owning a lock instance should then dispose of it - this adds another check that the locking/unlocking is done correctly.


Compute Hamming Distance How can I make this code snippet better? Those for loops in if don't please me but they are covered with code lines that is dependent to if before and after.

public int hammingDistance(int x, int y) {
    String binx = Integer.toBinaryString(x);
    String biny = Integer.toBinaryString(y);

    int dif = 0;
    StringBuilder sb = new StringBuilder();

    if (binx.length() > biny.length()) {
        dif = binx.length() - biny.length();

        for (int i = 0; i < dif; i++) {
            sb.append(0);
        }

        biny = sb.toString().concat(biny);
    } else if (biny.length() > binx.length()) {
        dif = biny.length() - binx.length();

        for (int i = 0; i < dif; i++) {
            sb.append(0);
        }

        binx = sb.toString().concat(binx);
    }

    int dist = 0;

    for (int i = 0; i < binx.length(); i++) {
        if (binx.charAt(i) != biny.charAt(i)) {
            dist++;
        }
    }

    return dist;
}

You're missing a big simplification.  The hamming distance is the count of ones in x^y, so simply perform that xor and count the set bits.  There should be no need to convert to string.