HMAC-SHA1 implementation I am trying out a small piece of code that would generate HMAC-SHA1. I have been asked to code the HMAC implementation myself using the OpenSSL libs for SHA1 calculation.

After 'wiki'ing for the algorithm, here is what I have below.  I have used input with RFC 2104 specified test values:

  key =         0x0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b
  key_len =     16
      bytes
  data =        "Hi There"
  data_len =    8  bytes    digest =
      0x9294727a3638bb1c13f48ef8158bfc9d

But the output I get is:

675b0b3a1b4ddf4e124872da6c2f632bfed957e9*

Is this really not the way to implement HMAC? How else can this be done?

#include <openssl/evp.h>
#include <openssl/bn.h>
#include <openssl/sha.h>
#include <openssl/err.h>
#include <openssl/conf.h>
#include <openssl/engine.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>     /* for memset() */
#include <unistd.h>

#define IPAD 0x36
#define OPAD 0x5C

#define SHA1_DIGESTLENGTH 20
#define SHA1_BLOCK_LENGTH 64
#define COUNTER_LENGTH 8

typedef unsigned          char uint8_t;
typedef unsigned short     int uint16_t;
typedef unsigned           int uint32_t;

/**
 * Key
 */
#define SECRET { 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b, 0x0b , 0x0b , 0x0b , 0x0b , 0x0b , 0x0b , 0x0b , 0x0b , 0x0b}
#define COUNTER {  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 }
/*
 * hmac sha1.
 */
int main(int argc, char *argv[]){
    unsigned char ipad[SHA1_BLOCK_LENGTH];
    unsigned char opad[SHA1_BLOCK_LENGTH];
    unsigned char algKey[SHA1_BLOCK_LENGTH];
    unsigned char msgBuffer[SHA1_BLOCK_LENGTH + COUNTER_LENGTH];
    unsigned char valBuffer[SHA1_BLOCK_LENGTH + 20];
    unsigned int i;
    uint8_t ctr[] = "Hi There";
    uint8_t hash[20];
    uint8_t key[]= SECRET;
    int len = 16;
    memset(hash, 0, sizeof(hash));
    memset(algKey, 0, sizeof(algKey));
    strncpy((char *)algKey, (char *)key, len);

    memset(ipad, IPAD, sizeof(ipad));
    memset(opad, OPAD, sizeof(opad));

    for (i = 0; i < SHA1_BLOCK_LENGTH; i++){
        ipad[i] ^= algKey[i];
    }

    for (i = 0; i < SHA1_BLOCK_LENGTH; i++){
        opad[i] ^= algKey[i];
    }

    memset(msgBuffer, 0, sizeof(msgBuffer));

    strncpy((char *)msgBuffer, (char *)ipad, SHA1_BLOCK_LENGTH);
    strncpy((char *)msgBuffer + SHA1_BLOCK_LENGTH, (char *)ctr, COUNTER_LENGTH);

    EVP_MD_CTX mdctx, md_ctx;
    const EVP_MD *md;

    unsigned char md_value[EVP_MAX_MD_SIZE];
    unsigned int md_len;

    OpenSSL_add_all_digests();

    md = EVP_get_digestbyname("sha1");

    if(!md) {
        printf("Unknown message digest\n");
        exit(1);
    }

    EVP_MD_CTX_init(&mdctx);
    EVP_DigestInit_ex(&mdctx, md, NULL);
    EVP_DigestUpdate(&mdctx, msgBuffer,(SHA1_BLOCK_LENGTH + COUNTER_LENGTH) );
    EVP_DigestFinal_ex(&mdctx, md_value, &md_len);
    EVP_MD_CTX_cleanup(&mdctx);

    printf("Digest is: ");
    for(i = 0; i < md_len; i++) printf("%02x", md_value[i]);
    printf("\n");

    unsigned char *copy;
    copy = malloc(md_len);
    strncpy((char*)copy, (char *)md_value, md_len);

    memset(valBuffer, 0, sizeof(valBuffer));
    strncpy((char *)valBuffer, (char *)opad, SHA1_BLOCK_LENGTH);
    strncpy((char *)valBuffer + SHA1_BLOCK_LENGTH, (char *)copy, md_len);

    EVP_MD_CTX_init(&md_ctx);
    EVP_DigestInit_ex(&md_ctx, md, NULL);
    EVP_DigestUpdate(&md_ctx, valBuffer, SHA1_BLOCK_LENGTH + 20);
    printf( "updated Digest..\n");
    EVP_DigestFinal_ex(&md_ctx, md_value, &md_len);
    EVP_MD_CTX_cleanup(&md_ctx);

    printf("Digest is: ");
    for(i = 0; i < md_len; i++) printf("%02x", md_value[i]);
    printf("\n");

    return 0;
}

You can make these #defines:

#define SHA1_DIGESTLENGTH 20
#define SHA1_BLOCK_LENGTH 64
#define COUNTER_LENGTH 8

into a more concise enum:

typedef enum { COUNTER=8, SHA1_DIGEST=20, SHA1_BLOCK=64 } Length;

This could also be done with SECRET and COUNTER, but each hex value would need a name.

If you use the enum recommendation, you may need to rename the second COUNTER to avoid possible name-clashing.  Perhaps rename it based on what the counter is for.
You don't need your own typedefs for these:

typedef unsigned           char uint8_t;
typedef unsigned short     int uint16_t;
typedef unsigned           int uint32_t;

Prefer to use the typedefs already defined in <stdint.h>.
For unformatted output with a newline, use puts() instead of printf():

puts("Some text...");

I'd recommend splitting this program into separate functions.  This will make it easier to tell how the it works and make it easier to maintain.  A function should have one purpose, andmain() should have minimal implementation (such as acquiring input and calling other functions).
There's no need to call exit(1) within main().  Just return 1.

The return 0 is also not needed at the end.  Reaching this point implies successful termination, so the compiler will do this return for you.


Nice way to using cast on TextView Question is short. Assuming that I don't need reference itself, would this cast

((TextView) findViewById(R.id.someTextView)).setText("lala");

be better (or at least not worse) than:

TextView tv=(TextView) findViewById(R.id.someTextView);
tv.setText("lala");

The latter would be my preferred method. It enhances readability, and allows you to use that object more than once without casting it every time (more efficient).

Though, if you are only casting to use one function inside the object, then there's no point in doing the latter (you would just be creating needless variables to only set one value in it once).


Terminating a C loop when maximum hardware limit reached In response to the SO question C: f>0 vs Perl: $f>0?, the use of overflow to terminate the loop is referred to as "poor programming practice". Paraphrased:

  Your c code is actually exiting because of
  poor programming practice. You're
  relying on fib being a long long int,
  which has an upper limit of 9.22337204
  × 10^18. When you loop around to the
  93rd iteration fib becomes 1.22001604 ×
  10^19, which overflows and becomes
  negative.

Substantially the same code:

cat >fib.c <<EOF
#include <stdio.h>
int main(){
   for (long long int temp, i=1, prev=0, fib=1; fib>0 ; i++, temp=fib, fib+=prev, prev=temp)
      printf("%lli: %lli\n", i, fib);
   }
EOF
gcc -std=c99 -ofib fib.c
./fib

The code was intended to be a one time "throw-away" program which was evidenced by the use of single letter variable names in the original version. The use of the overflow being indicated by fib becoming negate was used to terminate the loop.

Without knowing in advance what the maximum number of iterations the hardware architecture could handle what would be good "programming practice" to terminate the loop knowing that long long int is limited yet desiring as many values as possible?

Use unsigned rather than signed because once there is overflow the effect is undefined.
Whereas with unsigned there is no overflow but modulo arithmetic plus it produces an additional value.

cat >fib2.c <<EOF
// Fibinocci numbers
#include <stdio.h>
int main(){
   for (unsigned long long ii=1, fib=1, prev=0, temp;
         fib>=prev;  // Wrap indicates max value per implementation
         ii++, temp=fib, fib+=prev, prev=temp)
      printf("%llu: %llu\n", ii, fib);
   }
EOF
gcc -std=c99 -ofib2 fib2.c
./fib2


Iterative binary search I have made a simple binary search algorithm with the assumptions that the input array is sorted and has only unique values.

def binary_search(input_array, value):
    """
    Iterative binary search in sorted array of unique int values.

    :param input_array: Sorted array of unique int values.
    :type input_array: list<int>
    :param value: The int value to search in input_array
    :type value: int
    :return: The index of value or -1 if it doesn't exist
    :rtype: int
    """
    result = -1
    previous_index_down = 0
    previous_index_up = len(input_array)
    current_index = int(math.floor(len(input_array[previous_index_down+1:previous_index_up]) / 2))
    while True:
        if input_array[current_index] == value:
            result = current_index
            break
        elif current_index == previous_index_down or current_index == previous_index_up:
            break
        elif input_array[current_index] > value:
            previous_index_up = current_index
            current_index = current_index - int(math.ceil(len(input_array[previous_index_down:previous_index_up+1]) / 2))
        elif input_array[current_index] < value:
            previous_index_down = current_index
            current_index = current_index + 1 + int(math.floor(len(input_array[previous_index_down+1:previous_index_up]) / 2))

        if previous_index_down == len(input_array) - 1:
            break
    return result

For test I used various inputs like the following:

test_list = [1,3,9,11,15,19,29]
test_val1 = 25
test_val2 = 15
test_val3 = 29
print binary_search(test_list, test_val1)
print binary_search(test_list, test_val2)
print binary_search(test_list, test_val3)

The results appear to be correct, but I am wondering if the implementation can be improved in terms of style and length. Also I don't know if there are inputs that may make it fail.

Your lines are too long which makes things a little difficult to read. The next few points will help with that.
In my opinion your variable names are too verbose. current_index could be replaced with i and most programmers will understand that as an indexing variable.
When generating your current index, there is no reason to take the length of an array slice. The length of array[start:end] will generally be end -
start. When that isn't true its because you are slicing outside of array bounds which probably means your search is over anyway. In any case, you don't really care about array length. You just want the mid point between the two indexes.
there is no reason to pass the result of math.floor to int since both do essentially the same thing (int(2.6) gives 2). In fact, you can avoid both of these entirely by just using the integer division operator // (in python2 integer division is the default for 2 integers: 3/2==1
while True is ok in some limited cases but I generally avoid it when I can because it can easily create infinite loops if you forget a break somewhere... and here you have a lot of breaks. Its not too hard to come up with a condition that will effectively bound your search, and reduce the number of conditions that you have to check for.

Following the above points gives much lighter looking code thats for the most part functionally the same:

def binary_search(input_array, value):
    result = -1
    # simplified variable names and index initializer
    down = 0
    up = len(input_array)
    i = (up - down+1) // 2
    while down < i < up:    # refactored out `while True`
        if input_array[i] == value:
            result = i
            break
        # remove `break` conditions now handled in `while`
        elif input_array[i] > value:
            up = i
            # remove unnecessary string slice, int cast, and math.floor calls
            i = i - (up+1 - down) // 2 # integer division
        elif input_array[i] < value:
            down = i
            i = i + 1 + len(input_array[down+1:up]) // 2 # integer division!

    return result


I'm deallocating objects created but I'm unsure whether my code is leaking memory or not I have a simple application which requires the user to input details about students and their grades. I have two different classes Student and Database. The Student class contains info about students, such as, their names and grades:

Student.h file:

#pragma once
#ifndef STUDENT_H
#define STUDENT_H
#include <string>

class Student {
private:
    std::string name;

public:
    Student(std::string name);

    const std::string getName() const;

    ~Student();
};

#endif // !STUDENT_H

Student.cpp file:

#include "student.h"
#include <string>
#include <iostream>

Student::Student(std::string name) {
    this->name = name;
}

const std::string Student::getName() const {
    return this->name;
}

Student::~Student() {
    std::cout << "Student destructor\n";
}

Database.h file:

#pragma once
#ifndef DATABASE_H
#define DATABASE_H
#include <string>
#include "student.h"
#include <vector>

class StudentDatabase {
private:
    std::vector<Student*> student_list;

public:
    void add_student(Student* stud_obj);

    ~StudentDatabase();
};

#endif // !DATABASE_H

Database.cpp file:

#include "database.h"
#include "student.h"
#include <string>
#include <iostream>
#include <vector>

void StudentDatabase::add_student(Student* stud_obj) {
    std::cout << "ADD STUDENT METHOD\n";
    StudentDatabase::student_list.push_back(stud_obj);
}

StudentDatabase::~StudentDatabase() {
    std::cout << "Database destructor\n";

    std::vector<Student*>::iterator iter;

    for (iter = student_list.begin(); iter != student_list.end(); iter++) {
        delete* iter;
    }
}

Main.cpp file:

#include <iostream>
#include <string>
#include "database.h"
#include "student.h"

int main()
{
    std::string input;
    std::string name;

    StudentDatabase* database = new StudentDatabase();

    for (;;) {
        std::cout << "0: Add student \n";

        std::cout << "Enter a number (or q to quit) and press return... \n";

        std::cin >> input;
        std::cin.ignore();

        if (input == "q") {
            break;
        }

        else if (input == "0") {
            std::cout << "Enter student's first name: ";
            std::getline(std::cin, name);

            Student* student_obj = new Student(name, surname, student_number, grade);
            database->add_student(student_obj);
        }
    }

    delete database;
    return 0;
}

My question is: am I correctly deallocating memory for Students from ~StudentDatabase()?

I know smart pointers are more appropriate but I want to make sure I understand raw pointers properly.

You don't need to use new everywhere. Inside your main function, the StudentDatabase could very well reside on the stack so you don't need to delete it yourself.

One of the issues with how StudentDatabase is written now is ownership semantics. Once a student pointer is added to the database, the database asumes ownership upon it and deletes it in the destructor. What if the same student pointer were to be added to multiple databases, or used after the database is deleted?

Smart pointers help prevent such errors.


Quasi-random sequences: how to improve style and tests? The requirements for this one were (original SO question):

Generate a random-ish sequence of items.
Sequence should have each item N times.
Sequence shouldn't have serial runs longer than a given number (longest below).

The solution was actually drafted by another user, this is my implementation (influenced by random.shuffle).

from random import random
from itertools import groupby # For testing the result
try: xrange
except: xrange = range

def generate_quasirandom(values, n, longest=3, debug=False):
  # Sanity check
  if len(values) < 2 or longest < 1:
    raise ValueError

  # Create a list with n * [val]
  source = []
  sourcelen = len(values) * n
  for val in values:
    source += [val] * n

  # For breaking runs
  serial = 0
  latest = None

  for i in xrange(sourcelen):
    # Pick something from source[:i]
    j = int(random() * (sourcelen - i)) + i
    if source[j] == latest:
      serial += 1
      if serial >= longest:
        serial = 0
        guard = 0
        # We got a serial run, break it
        while source[j] == latest:
          j = int(random() * (sourcelen - i)) + i
          guard += 1
          # We just hit an infinit loop: there is no way to avoid a serial run
          if guard > 10:
            print("Unable to avoid serial run, disabling asserts.")
            debug = False
            break
    else:
      serial = 0
    latest = source[j]
    # Move the picked value to source[i:]
    source[i], source[j] = source[j], source[i]

  # More sanity checks
  check_quasirandom(source, values, n, longest, debug)

  return source

def check_quasirandom(shuffled, values, n, longest, debug):
  counts = []
  # We skip the last entries because breaking runs in them get too hairy
  for val, count in groupby(shuffled):
    counts.append(len(list(count)))
  highest = max(counts)
  print('Longest run: %d\nMax run lenght:%d' % (highest, longest))

  # Invariants
  assert len(shuffled) == len(values) * n
  for val in values:
    assert shuffled.count(val) == n

  if debug:
    # Only checked if we were able to avoid a sequential run >= longest
    assert highest <= longest

for x in xrange(10, 1000):
  generate_quasirandom((0, 1, 2, 3), 1000, x//10, debug=True)

I'd like to receive any input you have on improving this code, from style to comments to tests and anything else you can think of.

A couple of possible code improvements that I noticed:

sourcelen = len(values) * n

This seems unnecessarily complicated to me. I mean, after a second of thinking the reader of this line will realize that len(values) * n is indeed the length of source, but that's still one step of thinking more than would be required if you just did sourcelen = len(source) (after populating source of course).

That being said, I don't see why you need to store the length of source in a variable at all. Doing for i in xrange(len(source)): isn't really less readable or less efficient than doing for i in xrange(sourcelen):, so I'd just get rid of the variable altogether.

source = []
for val in values:
  source += [val] * n

This can be written as a list comprehension like this:

source = [x for val in values for x in [val]*n]

Using list comprehensions is usually considered more idiomatic python than building up a list iteratively. It is also often more efficient.

As Fred Nurk points out, the list comprehension can also be written as

source = [val for val in values for _ in xrange(n)]

which avoids the creation of a temporary list and is maybe a bit more readable.

j = int(random() * (sourcelen - i)) + i

To get a random integer between x (inclusive) and y (exclusive), you can use random.randrange(x,y), so the above can be written as:

j = randrange(i, len(source) - i)

(You'll also need to import randrange instead of random from the random module). This makes it more immediately obviously that j is a random number between i and len(source) - i and introduces less room for mistakes.


Zero-padding a number to have six digits This is a switch statement in JavaScript. I have a feeling that it can be done in a shorter way.

switch(n.length){
  case 1:
    n = '00000' + n;
  break;
  case 2:
    n = '0000' + n;
  break;
  case 3:
    n = '000' + n;
  break;
  case 4:
    n = '00' + n;
  break;
  case 5:
    n = '0' + n;
  break;
}

function foobar(n)
{
    var zeroes = "000000";
    return zeroes.substr(n.length) + n.toString();
}


Euler 357 solution efficiency I have a Ruby program which should solve the following problem:

  Consider the divisors of 30: 1,2,3,5,6,10,15,30. It can be seen that
  for every divisor d of 30, d+30/d is prime.

  Find the sum of all positive integers n not exceeding 100 000 000 such
  that for every divisor d of n, d+n/d is prime.

require 'prime'

class Integer
    def factors # returns an array of all factors of self
        return (1..self).collect { |n| n if ((self/n) * n) == self }.compact
    end
end

puts "Started at #{Time.now}."
counter = 0

1.upto(100000000) do |n|
    factors = n.factors
    counter += n if factors.all? { |d| ( (d+n) / d ).is_a? Prime }
end

p counter
puts "Ended at #{Time.now}."

Problem is, while the code runs, it takes so long (not finished after several hours) that I can't actually test if I get the right answer. Is there any way to make the code more efficient so that it completes in a shorter amount of time? The problem is question #357 of Project Euler. All Euler problems should take less than one minute to solve if an efficient algorithm is being used.

n.is_a? Prime

Doesn't work. I don't know enough Ruby to tell you why, but it doesn't. Use Prime.prime? instead.

Further, (d + n) / d is not the right calculation. You want d + n/d.

You can simplify

return (1..self).collect { |n| n if ((self/n) * n) == self }.compact

to

(1..self).select { |n| self / n * n == self }

which is even simpler as

(1..self).select { |n| (self % n).zero? }

One can do even better by making this lazy:

(1..self).lazy.select { |n| (self % n).zero? }

Then one can precompute the primes for later:

primes = Array.new(max+1) { |i| false }
Prime.each do |n|
    break if n > max
    primes[n] = true
end

1.upto(max) do |n|
    factors = n.factors
    counter += n if factors.all? { |d| primes[d + n/d] }
end

One can also note that only numbers one below primes actually need to be checked, since 1 is a factor of every number. This simplifies things:

primes = Array.new(max+1) { |i| false }
Prime.each do |prime|
    break if prime > max
    primes[prime] = true

    n = prime - 1
    factors = n.factors
    counter += n if factors.all? { |d| primes[d + n/d] }
end

One also only need the first square root of factors, since larger numbers are paired with the smaller numbers anyway:

(1..Math.sqrt(self) + 1).lazy.select { |n| (self % n).zero? }

This now takes about 100 seconds for n = 100_000_000.


Splitting URLs into hierarchy-level directories The goal is to split a URL like http://q.com/a/b/c to:

['http://q.com/a', 'http://q.com/a/b', 'http://q.com/a/b/c']

My code:

import urlparse

def get_domain_with_protocol(url):
    url_parts = urlparse.urlparse(url)
    return "{scheme}://{netloc}/".format(
        scheme=url_parts.scheme,
        netloc=url_parts.netloc
    )

def get_url_directories_combinations(url):
    url = url.rstrip('/')
    path = urlparse.urlparse(url).path[1:]
    parts = path.split('/')
    domain_with_protocol = get_domain_with_protocol(url)

    url_combinations = [domain_with_protocol + '/'.join(parts[:index + 1])
                        for index in range(len(parts))]
    return url_combinations

print get_url_directories_combinations('http://example.com/a/b/c/')

I think this code is ugly and a more Pythonic approach might be possible. There are libraries like hyperlink and posixpath etc. which can be used for path manipulation etc.

How would you improve this code? I'm open to using well-tested, popular libraries if that means less code and more stability.

All in all, this is not bad. A few remarks:

Python 2

Why choose Python 2? Python 3 has a lot of advantages, and will get more support in the future. Even if you need to code for Python 2, you can make your code compatible with both versions:

from __future__ import print_function
try:
    import urlparse
except ModuleNotFoundError:
    import urllib.parse as urlparse

docstring

Adding a docstring can better explain what the different methods do.

range

You don't need the range as list, so you might as well use xrange.

To make the code Python 3 compatible, you'd have to add xrange = range to the except clause on the import, so this might not be worth it in this case, but in general, use the iterable version as much as possible.

'/'

You do a lot of operations with '/'.

you remove a trailing / if it exists
you remove the starting / from urlparse.urlparse(url).path with [1:]
you add a trailing / in get_domain_with_protocol to the first part of the url

You can combine 1 and 2 by doing path.strip('/'). Or you can drop both 2 and 3, and iterate over range(1, len(parts).

generator

Instead of returning a list, you can also make a generator:

for index in range(len(parts)):
    yield domain_with_protocol + '/'.join(parts[:index + 1])

iteration 1

In general I try not to do things like range(len(parts)), but use enumerate. Here you could do for index, _ in enumerate(parts)

iteration 2

I try to avoid iterating over the index, and try to use generators as intermediate product instead of lists. Imagine parts would be an iterable instead of a list, your approach would not work.
In Python 3, you could use itertools.accumulate, but in Python 2, you'd have to write your own accumulator:

def accumulate_parts(parts, sep='/'):
    parts_iter = iter(parts)
    substring = next(parts_iter)
    yield substring
    for part in parts_iter:
        substring += sep + part
        yield substring

def get_url_directories_accumulate(url):
    path = urlparse.urlparse(url).path
    parts = path.strip('/').split('/')
    domain_with_protocol = get_domain_with_protocol(url)
    for substring in accumulate_parts(parts):
        yield domain_with_protocol + substring

timings

I've timed these variations both in Python 2 and Python 3, and all of them are within a few % of each other, so you can pick the one that suits you best, and you'll be able to understand in a few months/years.

code

Full code and timings can be found here.


Safety, sharing, and laziness Is there a more succinct way to write code like this that has a lot of sharing between branches of the case statement?

(This is the main part of my solution to one of the Round 1 problems of the Facebook Hacker Cup 2012. The round is over.)

type Memoizable a = a -> a

recoveries :: Int -> Memoizable (String -> Int)
recoveries m = go
  where go self ('0':_) = 0
        go self [] = 1
        go self ss = flip rem 0xfaceb00c $ case analysis ss of
          HasMany
            | cValid -> cCase + bCase + aCase
            | bValid -> bCase + aCase
            | aValid -> aCase
          Has2
            | bValid -> bCase + aCase
            | aValid -> aCase
          Has1
            | aValid -> aCase
          _
            | otherwise -> 0
          where a = digitToInt (first ss)
                b = a * 10 + digitToInt (second ss)
                c = b * 10 + digitToInt (third ss)
                aValid = a <= m
                bValid = b <= m
                cValid = c <= m
                aCase = self (drop 1 ss)
                bCase = self (drop 2 ss)
                cCase = self (drop 3 ss)

-- not to be confused with functions of same name from Control.Arrow
first (x:_) = x
second (_:x:_) = x
third (_:_:x:_) = x

data Analysis = Empty | Has1 | Has2 | HasMany

-- a dumb little function to help with safety and sharing
analysis :: [a] -> Analysis
analysis [] = Empty
analysis [_] = Has1
analysis [_,_] = Has2
analysis _ = HasMany

Hm, why did you write go as a separate function? It is - as you state - the same as recoveries, why the local definition?

For making it more succinct, I think it's easiest to have an intermediate data structure that carries both pieces of information you seek in the case: Whether there are enough digits and what number the digits so far would form.

The first data structure that comes to mind is a list of numbers - we just need to construct it. In this case, scanl is our friend:

import Data.List (scanl)
import Data.Char (digitToInt)

type Memoizable a = a -> a

recoveries :: Int -> Memoizable (String -> Int)
recoveries _ _    ('0':_) = 0
recoveries _ _    []      = 1
recoveries m self ss      = result `rem` 0xfaceb00c
  where result = case tail (scanl addDigit 0 ss) of
          (_:_:c:_) | c <= m  -> aCase+bCase+cCase
          (_:b:_)   | b <= m  -> aCase+bCase
          (a:_)     | a <= m  -> aCase
          _                   -> 0
        addDigit x c = x*10 + digitToInt c
        aCase = self (drop 1 ss)
        bCase = self (drop 2 ss)
        cCase = self (drop 3 ss)

Unless I've made a mistake (can't test), this should do the same as your code, and now we can write everything cleanly using pattern matches and a few guards on the intermediate list.

You could get rid of aCase, bCase and cCase as well if you would zip the result of scanl with tails ss. But while this is more general, I felt it would have made the code harder to understand, so I left it like this.


Safe cracker string with all combinations Imagine a safe with a 4-digit code, and accepting a continuous stream of code entries, such that when the 4 digits are seen in the right sequence, the safe opens. Generate a short string that contains all possible code combinations, so that when you feed it to the safe, it will inevitably open. The string should be as short as possible, and not contain the same code sequence twice.

As an exercise while learning Scala,
I implemented a general version of this logic using an arbitrary symbol alphabet and customizable code length.
For example, to generate a string for a 4-digit safe with numeric symbols,
you would call SafeCracker.genCrackerString("0123456789", 4).
I'm looking for a review of all aspects of this code,
and the unit tests too.

object SafeCracker {

  def genCrackerString(symbols: String, codeLength: Int) = {
    require(symbols.nonEmpty)
    require(codeLength > 0)

    val combinations = math.pow(symbols.length, codeLength)

    def cracker(prefix: String, used: Set[String]): String = {

      def findSuffixCombo(num: Int): (String, String) = {
        val suffix = getNth(symbols, num)
        val combo = (prefix + suffix).takeRight(codeLength)

        if (!used.contains(combo)) (suffix, combo)
        else findSuffixCombo(num + 1)
      }

      if (used.size == combinations) prefix
      else {
        val (suffix, combo) = findSuffixCombo(0)
        cracker(prefix + suffix, used + combo)
      }
    }

    val first = symbols(0).toString * codeLength

    cracker(first, Set(first))
  }

  def getNth(symbols: String, num: Int) = {
    val base = symbols.length

    def getNth(prefix: String, num: Int): String = {
      if (num == 0) {
        if (prefix.isEmpty) symbols(0).toString
        else prefix
      } else {
        val index = num % base
        val digit = symbols(index)
        getNth(digit + prefix, num / base)
      }
    }
    getNth("", num)
  }
}

Unit tests:

import org.junit.runner.RunWith
import org.scalatest.FunSuite
import org.scalatest.junit.JUnitRunner

@RunWith(classOf[JUnitRunner])
class SafeCrackerTest extends FunSuite {

  test("cracker invalid if no symbols") {
    intercept[IllegalArgumentException](genCrackerString("", 1))
  }

  test("cracker invalid if code length < 1") {
    intercept[IllegalArgumentException](genCrackerString("123", 0))
  }

  test("cracker 123, len=1") {
    assert("123" == genCrackerString("123", 1))
  }

  test("cracker 1, len=3") {
    assert("111" == genCrackerString("1", 3))
  }

  test("cracker 12, len=2") {
    assert("112122" == genCrackerString("12", 2))
  }

  test("cracker 12, len=3") {
    assert("111211221222" == genCrackerString("12", 3))
  }

  test("cracker 0123456789, len=4") {
    assert(10294 == genCrackerString("0123456789", 4).length)
  }

  test("getNth 0123456789abcdef, 255") {
    assert("ff" == getNth("0123456789abcdef", 255))
  }
}

Your algorithm is suboptimal.  According to this website, a de Bruijn sequence for SafeCracker.genCrackerString("0123", 3) should be 66 characters long:

000100200301101201302102202303103203311121131221231321332223233300

However, your output is 74 characters long:

00010020030011012013011121021131031122022123023121320321330331322232332333

A simpler example is genCrackerString("12", 2), which your unit test says should be "112122".  However, "11221" is shorter.  So, your unit tests are invalid as well.


Unit (Integration) Test for code that interacts with a database I am trying to create some (I believe these are Integration not Unit) tests for code that interacts with a database and I am not sure this is the right approach.

I would appreciate any reviews and possible improvements.

Using: VS2010 Unit Testing Framework

This is the function I want to test:

public IDbConnection OpenThirdPartyDatabase(string path, string password)
{
    var provider = GetProvider(path, password);
    return DBObjectFactory.GetObject().GetConnection(provider.Name, true, true);
}

Functions used by OpenThirdPartyDatabase

private static IDBProvider GetProvider(string path, string password)
{
    var providers = DBProviders.GetObject();
    var providerExists = providers.Cast<IDBProvider>().Any(dbProvider => dbProvider.Name == "MyProviderName");

    return providerExists ? providers.GetDBProvider("MyProviderName") : CreateProvider(path, password);
}

private static IDBProvider CreateProvider(string path, string password)
{
    IDBProvider provider = new DBAccessProvider("MyProviderName", path, "", password);
    DBProviders.GetObject().Add(provider);

    return provider;
}

public IDbConnection GetConnection(string ProviderName, bool OpenConnection, bool UseConnectionPool)
{
    IDBProvider dbProvider = DBProviders.GetObject().GetDBProvider(ProviderName);
    if (dbProvider == null)
        throw new UndeclaredDBProviderException(ProviderName);
    else
    {
        IDbConnection newConnection = null;
        if ((connectionPool.ContainsConnection(ProviderName)) && (UseConnectionPool))
            newConnection = connectionPool.GetConnection(ProviderName);
        else
        {
            newConnection = dbProvider.GetConnection();
            if (UseConnectionPool)
                connectionPool.AddConnection(ProviderName, newConnection);
        }
        if ((OpenConnection) && (newConnection.State == ConnectionState.Closed))
            newConnection.Open();
        return newConnection;
    }
}

Tests:

/// <summary>
/// Test Connection with valid path and password
/// </summary>
[TestMethod]
public void ThirdPartyConnection_Valid()
{
    var testConnection = _thirdPartyConnManager.OpenThirdPartyDatabase("Data\\test.mdb", "pass");
    Assert.AreNotEqual(testConnection, null);
}

/// <summary>
/// Test Connection With Invalid Password
/// </summary>
[TestMethod]
[ExpectedException(typeof(OleDbException),"Not a valid password.")]
public void ThirdPartyConnection_ShouldThrowOleDbException_InvalidPassword()
{
    var testConnection = _thirdPartyConnManager.OpenThirdPartyDatabase("Data\\test.mdb", "invalidpassword");
}

/// <summary>
/// Test Connection with invalid path
/// </summary>
[TestMethod]
[ExpectedException(typeof(OleDbException))]
public void ThirdPartyConnection_ShouldThrowOleDbException_InvalidPath()
{
    var testConnection = _thirdPartyConnManager.OpenThirdPartyDatabase("InvalidPath\\Data\\test.mdb", "pass");
}

/// <summary>
/// Test Connection String
/// </summary>
[TestMethod]
public void ThirdPartyConnection_ConnectionString()
{
    var testConnection = _thirdPartyConnManager.OpenThirdPartyDatabase("Data\\test.mdb", "pass");
    Assert.AreEqual(testConnection.ConnectionString,"Expected Connection String","Not expected Connection String");
}

I don't claim to be an expert, but here's my experience.

Apart from that i see you are using a constant string "MyProviderName" in your code and you would do well to move that to a const string for easy refactoring and future changes.

Also i suggest you use the more common "ArgumentNullException" vs creating your own custom "UndeclaredDBProviderException" as you want to capture that a null parameter was passed to the function.

I would also suggest you run the code through FxCop and fix any errors.


Vehicle inheritance exercise I'm learning about using inheritance and super in Ruby. This is a continuation of one of the exercises that is in a book I am reading.

I set up my Vehicle class to be the superclass. From there, MyCar and MyTruck inherit the Vehicle class's methods, by using the super method.

Both subclasses also have a MAX_SPEED constant, which is different for each. Each subclass also has its own .speed_up method, which outputs different strings depending on how fast the vehicle is going.

Is my code formatted correctly? It works, but I wonder how I can improve it, and if I am also using best practices in this case.

Should the year, color, model, and gasoline arguments be in the MyCar and MyTruck subclasses? Or should they remain in the Vehicle class?

The code below is also in my GitHub repository.

require 'date'

class Vehicle
  attr_accessor :speed, :on, :color
  attr_reader :year, :model, :gallons
  def initialize(year, color, model, gallons)
    @year = year
    @model = model
    @gallons = gallons
    @color = color
    @speed = 0
    @on = false
  end

  @@miles_per_gallon = 0

  def self.mileage(tank, miles)
    @@miles_per_gallon = miles / tank
  end

  def brake
    if self.speed == 0
      puts "You aren't moving! Yet you keep pushing the brake..."
    elsif self.speed >= 10
      self.speed -= 10
      puts "You slow down to #{self.speed} kilometers an hour."
    end
  end

  def shut_off
    if self.on == false
      puts "Your car is already off."
    else
      self.on = false
      puts "You turn off your car. The engine falls silent."
    end
  end

  def turn_on
    if self.on == false
      self.on = true
      puts "The car's engine rumbles and sparks to life."
    else
      puts "Your car is already running!"
    end
  end

  def drive(distance)
    puts "You drive #{distance} miles."
    Vehicle.mileage(self.gallons, distance.to_i)
    puts "When you stop in the gas station, you find that your car gets #{@@miles_per_gallon} miles per gallon."
  end

  def look_at
    if Time.new.hour >= 17 && Time.new.hour < 19
      puts "The red light of the setting sun sparkles off the paint of your #{model}."
      puts "You feel nostalgic."
    elsif Time.new.hour >= 19 && Time.new.hour < 6
      puts "Your #{model} carves a dark outline in the moon's pale light."
      puts "You feel dangerous. Perhaps you should go find a street race?"
    else
      puts "You gaze at the body of your #{self.year} #{self.model}. Its #{color} paint sparkles in the sunlight."
      puts "You feel happy."
    end
  end

  def repaint(color)
    self.color = color
    puts "You changed your car's color to #{self.color}!"
    puts "Mmm, sexy."
  end

end

class MyCar < Vehicle

  def initialize(year, color, model, gallons)
    super(year, color, model, gallons)
  end

  MAX_SPEED = 180

  def speed_up
    if self.speed != MAX_SPEED
      self.speed += 10
      puts "You speed up to #{self.speed} kph."
      if self.speed == 120
        puts "The engine purrs, the wind flows past the open window next to your seat."
        puts "You reach serenity..."
      end
    else
      puts "The engine roars, putting forth maximum effort."
      puts "The speedometer is pinned to the max!"
    end
  end

end

class MyTruck < Vehicle

  def initialize(year, color, model, gallons)
    super(year, color, model, gallons)
  end

  MAX_SPEED = 140

  def speed_up
    if self.speed != MAX_SPEED
      self.speed += 10
      puts "You speed up to #{self.speed} kph."
      if self.speed == 120
        puts "The engine rumbles, the wind flows past the open window next to your seat."
        puts "You feel calm. If you had a dog, he'd probably be sticking his head out of the window."
      end
    else
      puts "The engine roars, putting forth maximum effort."
      puts "Your fuel gauge needle slowly trickles downward..."
    end
  end

end

MyTruck and MyCar do not need their own initialize, the behaviour looks the same as the one in MyVehicle.

if self.on == false may be replaced with if !self.on or predicates

def on?
  on
end
def off?
  !on
end

see http://ruby-for-beginners.rubymonstas.org/objects/predicates.html .

Using self.class.mileage(self.gallons, distance.to_i) allows to overwrite class method mileage.

self.speed == 0 may be replaced with self.speed.zero? because
0, 1 and the likes are Integer objects and respond to zero?, although this is opinionated, i.e. not every object respond to zero?.


Lowest Common Ancestor in Binary Tree (Iterative) In the below code I've implemented a method to find the lowest common ancestor of a binary tree.
This is an iterative approach using this pseudocode.
Please suggest any improvements that can be made.
class Node:
    def __init__(self, data=None, left=None, right=None):
        self.data  = data
        self.left  = left
        self.right = right

def lowest_common_ancestor(root, node1, node2):
    parent = {root: None}
    stack = [root]

    while node1 not in parent or node2 not in parent:
        node = stack[-1]
        stack.pop()
        if node.left:
            parent[node.left] = node
            stack.append(node.left)
        if node.right:
            parent[node.right] = node
            stack.append(node.right)

    ancestors = set()
    while node1:
        ancestors.add(node1)
        node1 = parent[node1]
    while node2 not in ancestors:
        node2 = parent[node2]

    return node2.data

def main():
    '''
    Construct the below binary tree:

            30
           /  \
          /    \
         /      \
        11      29
       /  \    /  \
      8   12  25  14

    '''
    root = Node(30)
    root.left  = Node(11)
    root.right = Node(29)
    root.left.left  = Node(8)
    root.left.right = Node(12)
    root.right.left  = Node(25)
    root.right.right = Node(14)

    print(lowest_common_ancestor(root, root.left.left, root.left.right))       # 11
    print(lowest_common_ancestor(root, root.left.left, root.left))             # 11
    print(lowest_common_ancestor(root, root.left.left, root.right.right))      # 30

if __name__ == '__main__':
    main()

Unit tests
Turn these:
print(lowest_common_ancestor(root, root.left.left, root.left.right))       # 11
print(lowest_common_ancestor(root, root.left.left, root.left))             # 11
print(lowest_common_ancestor(root, root.left.left, root.right.right))      # 30

into assertions of the form assert 11 == ..., and you'll immediately have a unit test, which is generally of greater value.
Pop
node = stack[-1]
stack.pop()

should not be necessary; that's what pop already does:
node = stack.pop()


CSS selector for image captions I have this CSS selector:

.img + *{
}

Which, from my understanding, will make the browser look for all the elements and then select only those which follow an element with the class img. Is there a better way of writing this selection to improve performance?

Relevant HTML structure:

    <random-element>content is here</random-element>

My recommendation would be to not worry about the performance (see: http://www.kendoui.com/blogs/teamblog/posts/12-09-28/css_tip_star_selector_not_that_bad.aspx for actual tests).

If you're still not convinced that it's not as bad as everyone makes it out to be, then your only real option is to modify the contents of <random-element />.

    <random-element>
content is here
</random-element>

Which allows your CSS to be:

.img + #foo { ... }

Or just

#foo { ... }


A more efficient enqueue algorithm in Java So I have this simple code in Java. It enqueue (adds) and element to the end of the queue (implemented by an ArrayList) without changing the original queue.

public class MyQueue<T>{
    private List<T> body;

    // some constructors and helper functions.

    //copy constructor
    public MyQueue(List<T> list){
        this.body = list;
    }

    //this is the function
    public MyQueue<T> enqueue(T obj){
        List<T> temp = new ArrayList<T>(body);
        temp.add(obj);
        return new Queue<T>(temp);
    }

The whole idea is to make enqueue faster and more efficient, and again, as you notice, without changing the value of the original queue.  Any ideas?

UPDATE
For the sake of completing the idea.

1- This is an assignment so university, the skeleton provided is not to be changed, the task is to make the function enqueue faster (i do realize i am copying twice and thats the slow part).

2- As for the helper functions, they are simple:

public T peek(){
if(body.isEmpty()){
   thrown new NoSuchElementException();
}
return body.get(0);
}

public int size(){
return body.size();
}

It seems like what you are trying to accomplish here is to make your Queue class immutable. That in itself is good, but there are a couple of issues with your approach:

Creating a new class for this is not needed. There already exists classes for that. Use Collections.unmodifiableList to create one.
Your class does not implement the List interface (or the Queue interface for that matter). I guess there is a way to access the embedded List<T> such as getList hidden among "some constructors and helper functions", but if all that does is to return this.body; then your entire list is still accessible and modifiable.
The List<T> body field could be, and should be, declared final.

Instead of your entire Queue class, you might want to use this static method:

public static <E> List<E> addToList(List<E> oldList, E newElement) {
    List<E> temp = new ArrayList<E>(oldList);
    temp.add(newElement);
    return Collections.unmodifiableList(temp);
}

I should add however, that there is a difference between an unmodifiable list and what I understood about your approach. An unmodifiable list in Java does not allow any change to the list at all, such as list.set(index, obj).


API class to connect with web php today I coded a simple API class in C# with just a WebClient instance in each void and just simple voids to do simple things, can anyone give me any support in how I can improove it? Thanks.

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

namespace uTune.Base.Api
{
    class API
    {
        /// <summary>
        /// Runs a website script vue a url.
        /// </summary>
        /// <param name="url">URL to run.</param>
        public void runScript(string url)
        {
            WebClient client = new WebClient();
            client.DownloadString(url);
        }

        /// <summary>
        /// Runs a website script with the option of multiple parameters
        /// </summary>
        /// <param name="url">URL to run.</param>
        /// <param name="parameters">A list of parameters to include.</param>
        public void runScriptWithParameters(string url, Dictionary<string, string> parameters)
        {
            string urlToRun = url;
            bool firstParam = true;

            if (parameters.Count > 0)
            {
                foreach (KeyValuePair<string, string> parameter in parameters)
                {
                    if (firstParam)
                    {
                        urlToRun += "?" + parameter.Key + "=" + parameter.Value;
                        firstParam = false;
                    }
                    else
                    {
                        urlToRun += "&" + parameter.Key + "=" + parameter.Value;
                    }
                }
            }

            runScript(urlToRun);
        }

        /// <summary>
        /// Returns the pages content after a script has been executed.
        /// </summary>
        /// <param name="url">URL to get the content from.</param>
        /// <returns>Webpages content.</returns>
        public string getScriptReturn(string url)
        {
            WebClient client = new WebClient();
            return client.DownloadString(url);
        }

        /// <summary>
        /// Returns the pages conect after a script has been executed.
        /// </summary>
        /// <param name="url">URL to get the content form.</param>
        /// <param name="parameters">Parameters included in the URL.</param>
        /// <returns>Webpages content</returns>
        public string getScriptReturnWithParameters(string url, Dictionary<string, string> parameters)
        {
            string urlToRun = url;
            bool firstParam = true;

            if (parameters.Count > 0)
            {
                foreach (KeyValuePair<string, string> parameter in parameters)
                {
                    if (firstParam)
                    {
                        urlToRun += "?" + parameter.Key + "=" + parameter.Value;
                        firstParam = false;
                    }
                    else
                    {
                        urlToRun += "&" + parameter.Key + "=" + parameter.Value;
                    }
                }
            }

            return getScriptReturn(urlToRun);
        }
    }
}

both methods, runScriptWithParameters and getScriptReturnWithParameters have a lot of code in common which should be extracted to a separate method.
using string concatenation by using += will create each time a new string because strings are immutable. A better way would be to use a StringBuilder instead.
based on the .NET naming guidelines methods should be named using PascalCase casing.
the check for if (parameters.Count > 0) is not needed, because if you get the enumerator and there are no items to enumerate over, the loops is just ending.
you don't do any null validation on the methods parameters of your public methods. Omitting these will leak implementation details.
you should use the var type whenever the type is obvious form the right hand of the assignment.

Implementing some of the mentioned points will lead to

private string ComposeUrlParameters(Dictionary<string, string> parameters)
{
    var builder = new StringBuilder(1024);
    var firstParameter = true;
    foreach (var parameter in parameters)
    {
        if (firstParameter)
        {
            builder.Append("?");
            firstParameter = false;
        }
        else
        {
            builder.Append("&");
        }
        builder.AppendFormat("{0}={1}",parameter.Key, parameter.Value);
    }

    return builder.ToString();
}

which then can be called like so

public void RunScriptWithParameters(string url, Dictionary<string, string> parameters)
{
    if (url == null) { throw new ArgumentNullException("url"); }
    if (parameters == null) { throw new ArgumentNullException("parameters"); }

    var urlToRun = url + ComposeUrlParameters(parameters);
    RunScript(urlToRun);
}

You could compose the url parameters with linq as well like so

private string ComposeUrlParameters(Dictionary<string, string> parameters)
{
    if (parameters.Count == 0)
    {
        return string.Empty;
    }

    var builder = new StringBuilder(1024);
    var firstParameter = parameters.First();

    builder.AppendFormat("?{0}={1}",firstParameter.Key, firstParameter.Value);

    foreach (var parameter in parameters.Skip(1)
    {
        builder.AppendFormat("&{0}={1}",parameter.Key, parameter.Value);
    }

    return builder.ToString();
}


More than one answer by the same user I hacked together this little query on the Data Explorer

The query finds questions where an answerer has posted two or more answers (which in itself is not necessarily a bad thing), it gives you the link to the question, information about which user posted the multiple answers, and how many answers by that user that was posted.

select question.Id as [Post Link],
   answer.owneruserid as [User Link],
   count(answer.id) as answers
from Posts question
join Posts answer on (answer.parentid = question.id)
where answer.OwnerUserId != 0
group by answer.OwnerUserId,
  question.id, answer.owneruserid
having count(answer.id) >= 2
order by answers desc

The last two group by clauses seem quite useless to me in a way, as they will always only be one unique version of them after grouping by answer.OwnerUserId. My experience from mySQL is that it simply picks a seemingly random value if not grouping by it, but in the SEDE I get this result:

  Column 'Posts.Id' is invalid in the select list because it is not contained in either an aggregate function or the GROUP BY clause.

Is there a way to remove these extra GROUP BY's? Or is adding these "the way it's supposed to be done"?
How is this SQL overall, regarding performance, naming, structure, conventions and everything?
Are there any alternative ways to rewrite this query?

You don't need to perform any join at all.

SELECT ParentId AS [Post Link]
     , OwnerUserId AS [User Link]
     , COUNT(Id) AS Answers
    FROM Posts
    WHERE
        ParentId IS NOT NULL           -- ← Answers only
        AND OwnerUserId <> 0
    GROUP BY ParentId, OwnerUserId
    HAVING Count(id) > 1
    ORDER BY Answers DESC;

Your instinct probably tells you that you need a JOIN to obtain the question titles. However, in Stack Exchange Data Explorer, there is a special feature: provide a PostId in a column named [Post Link], and it will take care of the presentation for you (implicitly doing a JOIN).


Expense Sharing Calculation The following is my solution to the problem presented on this CodeReview post:

Friends A, B, C, D go for a trip. They spend on various expenses. Cost of the expense is shared.
Example :

A spends 100 for breakfast for A, B, C and D
D spends 500 for cab for B and C
B spends 300 for lunch for A, B and C

Write a program to calculate how much each should get or each should give to one another. App should be scalable that number of friends can change

Design:
We need two data sets, which each respectively:

record transactions
tally amount due to each person

Simply strip off the info we need from the transactions and record either how much each person is owed, or how much each person should give.
Any input is welcome!
using System;
using System.Collections.Generic;

namespace CodeReview
{
    class Transaction
    {
        class Account
        {
            private int totalDebt = 0;
            private Dictionary<char, int> debtors = new Dictionary<char, int>();

            public Account(char initialDebtor, int initialDebt)
            {
                debtors.Add(initialDebtor, initialDebt);
            }

            public Dictionary<char, int> Debtors
            {
                get
                {
                    return debtors;
                }
            }

            public int TotalDebt
            {
                get
                {
                    foreach (char debtor in debtors.Keys)
                    {
                        totalDebt += debtors[debtor];
                    }

                    return totalDebt;
                }
            }
        }

        static void Main(string[] args)
        {
            Queue<Tuple<char, int, List<char>>> transactions = new Queue<Tuple<char, int, List<char>>>();
            Dictionary<char, Account> ledger = new Dictionary<char, Account>();

            transactions.Enqueue(Tuple.Create('A', 100, new List<char>() { 'A', 'B', 'C', 'D' }));
            transactions.Enqueue(Tuple.Create('D', 500, new List<char>() { 'B', 'C' }));
            transactions.Enqueue(Tuple.Create('B', 300, new List<char>() { 'A', 'B', 'C' }));

            while (transactions.Count > 0)
            {
                Tuple<char, int, List<char>> transaction = transactions.Dequeue();

                foreach (char beneficiary in transaction.Item3)
                {
                    if (!beneficiary.Equals(transaction.Item1))
                    {
                        if (ledger.ContainsKey(beneficiary))
                        {
                            Dictionary<char, int> debtors = ledger[beneficiary].Debtors;

                            if (debtors.ContainsKey(transaction.Item1))
                            {
                                debtors[transaction.Item1] += transaction.Item2;
                            }
                            else
                            {
                                debtors.Add(transaction.Item1, transaction.Item2);
                            }
                        }
                        else
                        {
                            ledger.Add(beneficiary, new Account(transaction.Item1, transaction.Item2));
                        }
                    }
                }
            }

            foreach (char beneficiary in ledger.Keys)
            {
                Account account = ledger[beneficiary];

                Console.WriteLine("User " + beneficiary + " owes $" + account.TotalDebt + ":");

                foreach (char debtor in account.Debtors.Keys)
                {
                    int debt = account.Debtors[debtor];

                    Console.WriteLine(" - $" + debt + " to " + debtor);
                }
            }

            Console.ReadKey();
        }
    }
}

The main issue I can see, is the excessive use of generic data types like tuples and dictionaries instead of domain classes. That may be a matter of taste, but I prefer meaningful named domain classes like your Account class.
In the Account class, the dictionary is exposed as public member. IMHO internal data structures should be encapsulated. The class should be extended by appropriated methods to manipulate that data structures. That would automatically move code like

if (debtors.ContainsKey(transaction.Item1))
{
    debtors[transaction.Item1] += transaction.Item2;
}
else
{
    debtors.Add(transaction.Item1, transaction.Item2);
}

into the domain class. Imagine use have to add items somewhere else, that kind of code must be duplicated each time.

Instead of

if (!beneficiary.Equals(transaction.Item1))
{
    ...
}

You could use

 if (beneficiary.Equals(transaction.Item1))
 {
     continue;
 }

 ....

to reduce nesting.

The instance variable debtors should be read-only.
Another small point: I would use strings to store the users because you can store names and you do not run into problems if the group becomes greater than 26 persons :P - of course, that is not relevant for the algorithm.

Generall Approach

Your approach is, to build up two dictionaries (the ledger and the debtors within the account class). The data structure of nested dictionaries matches the requirements exactly. If the requirements change, you have to restructure your whole code.

It is better to transform the data in a simple generally usable data structure and implement your algorithm for that data structure.

Such a simple data structure may be a list of Transactions:

public class Transaction
{
    public Transaction(string payer, string payee, int amount)
    {
        Payer = payer;
        Amount = amount;
        Payee = payee;
    }

    public string Payer { get; }
    public int Amount { get; }
    public string Payee { get; }
}

The algorithms could be implemented in a class like Bank:

public class Bank
{
    private readonly List<Transaction> myTransactions = new List<Transaction>();
    private readonly HashSet<string> myPayers = new HashSet<string>();
    private readonly HashSet<string> myPayees = new HashSet<string>();

    public void AddTransaction(string payer, string payee, int amount)
    {
        myTransactions.Add(new Transaction(payer, payee, amount));
        myPayers.Add(payer);
        myPayees.Add(payee);
    }

    public void PrintPayees()
    {
        foreach (var payee in myPayees)
        {
            var trans = myTransactions.Where(t => t.Payee == payee);
            Console.WriteLine($"User {payee} owes ${trans.Sum(t => t.Amount)} total:");
            foreach (var groupedTransaction in trans.GroupBy(t => t.Payer))
                Console.WriteLine($"- ${groupedTransaction.Sum(t => t.Amount)} from {groupedTransaction.Key}");
        }
    }

    public void PrintPayer()
    {
        foreach (var payer in myPayers)
        {
            var trans = myTransactions.Where(t => t.Payer == payer);
            Console.WriteLine($"User {payer} gets ${trans.Sum(t => t.Amount)} total:");
            foreach (var groupedTransaction in trans.GroupBy(t => t.Payee))
                Console.WriteLine($"- ${groupedTransaction.Sum(t => t.Amount)} to {groupedTransaction.Key}");
        }
    }
}

Usage base on the list of tuples:

void Main()
{
    var bank = new Bank();

    foreach (var transaction in new[] { Tuple.Create("A", 100, new [] { "A", "B", "C", "D" }),
                                        Tuple.Create("D", 500, new [] { "B", "C" }),
                                        Tuple.Create("B", 300, new [] { "A", "B", "C" }) })
    {
        var payer = transaction.Item1;
        var amount = transaction.Item2;
        foreach (var payee in transaction.Item3)
            if (payee != payer)
                bank.AddTransaction(payer, payee, amount);
    }

    bank.PrintPayees();
    bank.PrintPayer();
}


Pick sequences of numbers to maximize the points It is a single-player game. In the beginning, there is a row of integers. In one move the player can take a certain amount of adjacent numbers (let's denote it T). Then the player's points increase with the sum of the chosen numbers. The player can take S moves at most. The goal is to determine the maximum points that can be reached and we have to give the specific moves to reach it.

Example

The numbers: 1 6 8 7 6 2 1 8
S = 2 (max moves)
T = 3 (adjacent numbers to take in one move)

The max points is 32, it can be done like this:
1 6 8 7 6 2 1 8 (we take the bold numbers)
So the two moves are 2 and 6 (they indicate start indexes in the row)
I wrote a recursive code in c# that works if we have few numbers, but takes too long if e.g. has 3000 numbers, 100 max moves and T = 20.

My idea is to first count the points we will get for each number. Then try every possible combination, and store the one that gives me the most points.
So my question is how it can be made it faster?

class Value
{
    public int idx = -1;
    public int num = -1;
    public Value(int idx, int num)
    {
        this.idx = idx;
        this.num = num;
    }
}

class Program
{
    const int N = 8;
    static int S = 2;
    static int T = 3;
    static int[] numbers = new int[N] { 1, 6, 8, 7, 6, 2, 1, 8 };
    static List<Value> values = new List<Value>();
    static int maxPoint = -1;
    static List<Value> result;

    static void Main(string[] args)
    {
        CountInitValues();
        Solve(values, new List<Value>(), 0, T);
    }

    static void Solve(List<Value> remaining, List<Value> numSoFar, int pointSoFar, int StepsRemain)
    {
        if (remaining.Count == 0 || StepsRemain == 0)
        {
            if (pointSoFar > maxPoint)
            {
                maxPoint = pointSoFar;
                result = new List<Value>(numSoFar);
            }
        }
        else
        {
            List<Value> newNum = new List<Value>(numSoFar);
            for (int i = 0; i < remaining.Count; i++)
            {
                List<Value> newRemaining = new List<Value>();
                newNum.Add(remaining[i]);
                newRemaining.AddRange(remaining.Take(i - T));
                newRemaining.AddRange(remaining.Skip(i + T));
                int ujPont = pointSoFar + remaining[i].num;
                int ujLepes = StepsRemain - 1;
                Solve(newRemaining, newNum, ujPont, ujLepes);
                newNum.RemoveAt(newNum.Count - 1);
            }
        }
    }

    static void CountInitValues()
    {
        for (int i = 0; i < N - T + 1; i++)
        {
            int newValue = 0;
            for (int j = 0; j < T; j++)
            {
                newValue += numbers[i + j];
            }
            Value ee = new Value(i, newValue);
            values.Add(ee);
        }
    }
}

Some general points

The code looks good, and the steps you take to calculate the result are sound. I would like to mention a few general points for reviewing the code, then move on to some tips that could improve the code.

Include usings

This helps others when reviewing the code.

using System.Collections.Generic;
using System.Linq;

Include the results when you run the program

This also helps reviewing, but more importantly: it helps you
testing.  Pressing "Run" and seeing what the results are is quicker
than attaching the debugger and needing a break point to check.

class Value
{
    // ...
    public override string ToString() { return $"({idx}, {num})"; }
}

static void Main(string[] args)
{
    CountInitValues();
    Solve(values, new List<Value>(), 0, T);
    System.Console.Out.WriteLine ("The result is: {0}", string.Join(", ", result.Select(i => i.ToString())));
}

Names

Use descriptive names for fields and variables with large scopes

Example: N, S and T are clear if you have the problem description
at hand, but not when you are looking at them in the code itself.
You could change them to numbers, maxMoves and numbersToTake,
so that their meaning is clear at one glance.

Write smaller functions

This helps readability, and especially in the case of Solve(), this could make it more clear what the code is doing. An example:

Split CountInitValues()

static void CountInitValues()
{
    for (int i = 0; i < N - T + 1; i++)
    {
        Value ee = new Value(i, CountValuesFrom(i));
        values.Add(ee);
    }
}

static int CountValuesFrom(int startIndex)
{
    int newValue = 0;
    for (int j = 0; j < T; j++)
    {
        newValue += numbers[startIndex + j];
    }
}

Bug

You use T for the number of steps, instead of S

This gives no different result in the example, because after 2
steps, you can't take another set of 3 values any more.  But in
different sets, this will give false results.

Solve(values, new List<Value>(), 0, T);

should be

Solve(values, new List<Value>(), 0, S);

An Object Oriented issue

Solver can be a separate class with non-static fields

If you extract a class Solver, and use non-static fields, you can
create more of them in a testing method (or in Main()), give them
different values to calculate results, and test those results.

class Solver
{
    int N;
    int S;
    int T;
    int[] numbers;

    List<Value> values = new List<Value>();
    int maxPoint = -1;
    List<Value> result;

    public Solver(int N, int S, int T, int[] numbers)
    {
        this.N = N;
        this.S = S;
        this.T = T;
        this.numbers = numbers;
    }

    public List<Value> Run()
    {
        CountInitValues();
        Solve(values, new List<Value>(), 0, S);
        return result;
    }

    void Solve(List<Value> remaining, List<Value> numSoFar, int pointSoFar, int StepsRemain)
    {
        // ...
    }

    void CountInitValues()
    {
        // ...
    }
}

You can call the test like this, then:

class Program
{
    const int N = 8;
    static int S = 2;
    static int T = 3;
    static int[] numbers = new int[N] { 1, 6, 8, 7, 6, 2, 1, 8 };

    static void Main(string[] args)
    {
        var result = new Solver(N, S, T, numbers).Run();
        System.Console.Out.WriteLine ("The result is: {0}", string.Join(", ", result.Select(i => i.ToString())));
    }
}

But also like this:

class Program
{
    const int N = 8;
    static int S = 2;
    static int T = 3;
    static int[] numbers = new int[N] { 1, 6, 8, 7, 6, 2, 1, 8 };

    static void Main(string[] args)
    {
        var result = new Solver(N, S, T, numbers).Run();
        System.Console.Out.WriteLine ("The result for S = 2, T = 3 is: {0}", string.Join(", ", result.Select(i => i.ToString())));
        result = new Solver(N, 3, 2, numbers).Run();
        System.Console.Out.WriteLine ("The result for S = 3, T = 2 is: {0}", string.Join(", ", result.Select(i => i.ToString())));
    }
}

Optimizing the algorithm

It does not matter in which order you take groups

If you pick numbers from indices (2, 3, 4), and then from indices
(6, 7, 8), you will get the same result as when you first pick (6,
7, 8) and then (2, 3, 4).  Also, when picking (1, 2, 3) then (4, 5,
6), you already have the same number of points as when picking (2,
3, 4) and then (1, 5, 6).

Knowing this, you can skip tests that try to take numbers from the
left of where you took numbers:

void Solve(/*...*/)
{
    List<Value> newRemaining = new List<Value>();
    newNum.Add(remaining[i]);
    newRemaining.AddRange(remaining.Take(i - T));
    newRemaining.AddRange(remaining.Skip(i + T));
}

can now be

void Solve(/*...*/)
{
    List<Value> newRemaining = new List<Value>();
    newNum.Add(remaining[i]);
    newRemaining.AddRange(remaining.Skip(i + T));
}


Creating the start of a Caesar cipher My code takes an input, uses the input to take a number of letters from the start of the alphabet and put them at the end. I intend to use this to create a Caesar style cipher.

Is there a way to do this more elegantly?

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

namespace ConsoleApplication
{
    class Program
    {
        static void Main()
        {
            GettingInput gi = new GettingInput();
            var input = gi.GetInput();
            Console.WriteLine("Your input:\n{0}", input);
            List<char> alphabet = new List<char>();

            for (char c = 'a'; c <= 'z'; c++)
            {
                alphabet.Add(c);
            }
            for (int i = 0; i < input; i++)
            {
                alphabet.Add(alphabet[i]);
            }

            for (int i = input ; i > 0; i--)
            {
                alphabet.Remove(alphabet[i-1]);
            }

            for (int i = 0; i < 26; i++)
            {
                Console.WriteLine(alphabet[i]);
            }

            Console.ReadLine();
        }
    }
}

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

namespace ConsoleApplication
{
    public class GettingInput
    {
        public int GetInput()
        {
            Console.WriteLine("Please input an int:");
            while (true)
            {
                try
                {
                    int input = Convert.ToInt32(Console.ReadLine());
                    return input;
                }
                catch (Exception ex)
                {
                    Console.WriteLine(ex);
                    Console.WriteLine("Please input a valid int:");
                }

            }

        }

    }
}

All feedback welcome!

Instead of create the whole list and modify it subsequently, you could calculate the value directly.

The code could like that:

static void Main()
{
    GettingInput gi = new GettingInput();
    var input = gi.GetInput();
    Console.WriteLine("Your input:\n{0}", input);

    var first = (int)'a';
    var last = (int)'z';
    var length = last - first;

    var query = Enumerable
        .Range(0, length)
        .Select(i => (char)(first + ((input + i) % length)));

    foreach (var number in query)
        Console.WriteLine(number);

    Console.ReadLine();
}

You could use int.TryParse(Console.ReadLine(), out input) instead of catching the exception


Delete all comments in a C program I am new to C. And I just want to know if my code is horrible or not. I tried to make program to delete all comments in C program. It's from "The C Programming Language Second Edition" book, Exercise 1-23.

Here is my solution :

#include <stdio.h>
#define MAXCHAR 1000
int c = 0;
int d = 0;
int newFileText[MAXCHAR];
int longDisable = 0;
int shortDisable = 0;
int i = 0;

//Function declarations
void printArray(int a[], int b);

void myputchar(int a[], int c);

int main(void) {
    while ((c = getchar()) != EOF) {
        if (i > MAXCHAR)i = 0;
        if (c == '/') {
            d = getchar();
            if (d == '/') shortDisable = 1;
            else if (d == '*') longDisable = 1;
        } else if (c == '*') {
            d = getchar();
            if (d == '/') {
                c = getchar();
                if (!(c > 0)) {
                    break;
                }
                longDisable = 0;
            }
        } else if (c == '\n') {
            shortDisable = 0;
        }
        if (shortDisable || longDisable);
        else {
            myputchar(newFileText, c);
            i++;
        }
        /*printf("\nShort disable: %d", shortDisable);
        printf("\nLong disable: %d", longDisable);*/
    }
    printArray(newFileText, i);
    return 0;
}

void printArray(int theArray[], int lengthSoFar) {
    for (int i = 0; i < lengthSoFar; i++) {
        printf("%c", theArray[i]);
    }
}

void myputchar(int a[], int c) {
    int i = 0;
    while (a[i] != '\0') {
        i++;
    }
    a[i] = c;
}

Bugs

When processing an expression like 10/2 or 10*2, the 2 gets discarded from the output.

It looks like you attempted to implement a circular buffer for the output, but it actually fails miserably if the output exceeds 1000 bytes.  One problem is performance: myputchar() performs a linear scan of the buffer to find the end of the string, where it can write the next character.  A second problem is buffer overflow: myputchar() makes no attempt to ensure that the location where it stores the next character lies within the buffer.  The third problem is that when you do if (i > MAXCHAR)i = 0, you haven't made any attempt to print the buffer contents before trying to reuse the buffer.

C grammar

In C, a /* or // that appears within a "string literal" should be treated literally.  It should not be interpreted as a start-of-comment marker.

Furthermore, within a //-style comment, any /* or */ needs to be disregarded.  Similarly, within a /* */-style comment, any // should be disregarded.  That is, the correct output for the following C99 program…

#include <stdio.h>
int main() {
    puts("First " /* asdf // */ "example"
    );
    puts("Second " // /*
         "example"
         // */
    );
    puts("Third " /* "/" */ "example");
}

… should be

First example
Second example
Third example

(I also got this wrong in Rev 1 of this answer.  I've corrected the bug, but this illustrates a point that C requires a stateful parser.)

Organization

The variables c, d, newFileText, longDisable, shortDisable, and i are global; they should all be local to the main function.

It is customary to put the main function last, so that you don't need to declare the functions it uses.

Input / output

You read one character at a time with getchar(), but you attempt to print the output in chunks of up to 1000 characters.  Writing in chunks could be more efficient than writing a character at a time, but it is certainly more difficult to implement correctly.  However, after you go through the effort of accumulating the chunks, you then proceed to print the buffer one character at a time using printf("%c", theArray[i]), which negates any performance advantage you could have gained.

Therefore, you would have been better off printing the output one character at a time to begin with, without any attempt at buffering.  To do so, you could use putchar(…), which is simpler than printf("%c", …).

If you did want to efficiently write a chunk of bytes at a time, you could use the write() function instead of the loop in printArray().

Suggested solution

This solution addresses the more serious logic bugs, without attempting to handle string literals correctly.

#include <stdio.h>

int main(void) {
    int c, d;
    int longDisable = 0, shortDisable = 0;

    while ((c = getchar()) != EOF) {
        switch (c) {
          case '/':
          case '*':
            d = getchar();
            if (!longDisable && c == '/' && d == '/') {
                shortDisable = 1;
            } else if (!shortDisable && c == '/' && d == '*') {
                longDisable = 1;
            } else if (!shortDisable && c == '*' && d == '/') {
                longDisable = 0;
            } else if (!(shortDisable || longDisable)) {
                putchar(c);
                if (d != EOF) putchar(d);
            }
            break;

          case '\n':
            shortDisable = 0;
            /* fall through... */
          default:
            if (!(shortDisable || longDisable)) {
                putchar(c);
            }
        }
    }
}


React Hooks - Did I write this correctly? I am brand new to writing hooks and I am having a little trouble getting my head wrapped around it.  Initially, I wanted to write this with one event listener that passed the event and a separate param to distinguish itself from the other incoming arguments.

With hooks, I was confused as to how I should pass a separate param and even more confused on how I would go about putting the logic into deciphering what each argument was.

So... Basically, I am calling two arrow functions in render and using separate arguments to dictate what iconType the onMouseOver is effecting.

I guess my question is, is this an acceptable way to write react hooks?  This is my first component with any kind of state in my project (a simple navbar).  I wan to make sure I am on the right path.

import React, { useState, useEffect } from 'react';
import '../styles/HomeNavBar.css';
import logo from '../styles/images/pulse_logo.png' // relative path to image

export default function HomeNavBar() {

 const [isTrue, handleMouseOver] = useState(false)
 const [iconType, setArg] = useState('')

 return (

 handleMouseOver(true), () => setArg('services')} onMouseLeave = {() => handleMouseOver(false), () => setArg('')} className="link-holder">
            {isTrue === false && iconType !== 'services' ?
Services
 :
Services
}

 handleMouseOver(true), () => setArg('careers')} onMouseLeave = {() => handleMouseOver(false), () => setArg('')} className="link-holder">
          {isTrue === false && iconType !== 'careers'  ?
Careers
 :
Careers
}

 handleMouseOver(true), () => setArg('blog')} onMouseLeave = {() => handleMouseOver(false), () => setArg('')} className="link-holder">
            {isTrue === false && iconType !== 'blog'  ?
Blog
 :
Blog
}

About

  );
}
````

Probably the biggest issue I see here is naming. In particular, isTrue is very vague - perhaps is isHovered would be better? And conventionally you use set... rather than handle... with useState so this should be setIsHovered. Similarly, I think you should rename setArg to setIconType.

Also, I don't think you can pass 2 seperate functions as a prop. You maybe want to instead do this:

<div
  onMouseEnter={() => {
    setIsHovered(true)
    setIconType('careers')
  }}
>
</div>

Other than that, it looks fine to me.


The question could benefit from some context. You don't need to answer every one of the following, but it would be helpful for reviewers.

Why have you written a morse code converter?
Is this going to be used by anyone else or is it a personal project? Is it a homework question? From this we can tell if the review should focus on finding security holes, or just annoying bugs.
What will the input look like? Can we assume nobody is going to call convert_to_word with a letters that aren't dot, dash, or space?

def convert_to_morse_code(word):
    ...

def convert_to_word(word):
    ...

This looks a little funny, as they both take a "word", yet neither does. One takes a string which may have complete sentences, and the other takes a string containing morse code. I'd suggest adding a docstring for these functions which gives a brief explanation about the expected input.

if letternum.isalnum():
    string += f'{morse_code[letternum.upper()]} '
else:
    string += '  '

What happens if somebody comes along and helpfully adds some international morse code symbols such as "?" or "+" to the dictionary? This code will ignore those. I think the dictionary should be the source of truth, not isalnum.

if letternum.upper() in morse_code:
    string += morse_code[letternum.upper()] + ' '
else:
    string += '  '

The above possibility shows that letternum is no longer an appropriate name. Lets update it the name to c or char, short for character, which is what it is. The other change I would propose here is to use the dictionary method .get as it has a handy parameter 'default value'. If we set the default value as '  ' (for when the character is not a symbol we know how to write in morse code) this takes the place of the else branch.

for char in word:
    string += morse_code.get(char, ' ') + ' '

I think it would be worth noting in the docstring for this function that unrecognised characters are skipped, and this is the line responsible.

for code in word:
    if code == '/':
        word = word.replace(code, '   ')

In general it is a bad idea to modify the thing you are looping over. It can lead to hard to spot bugs. The other thing to note is that the first time '/' is encountered, all occurrences of it are replaced. This doesn't need to be in a loop.

    for key in morse_code.keys():
        if morse_code[key] == code:
            string += key

This loop does a little more work than is necessary. It will get slower and slower each time a new symbol is added to the dictionary. The performance loss will honestly not be noticeable, but if you think the solution sounds nicer you can try implementing it.

The solution is to build a new map which is the inverse of morse_code, where all the keys become values and all the values become keys.

morse_code = {...}
code_to_char = {code: char for char, code in morse_code.items()}
...

    if code in code_to_char:
        string += code_to_char[code]

There are some small problems to which you will need to decide on the answer.

convert_to_word(convert_to_morse_code("Hello, world!")) outputs 'HELLO WORLD '. Is that ok? Should the output match the input more closely? Why is there trailing space?
convert_to_morse_code("HI LOW") outputs '.... ..   .-.. --- .-- '. Some variations exist, such as using a slash to indicate a space ('.... .. / .-.. --- .--') or seven dots (.... .. ....... .-.. --- .--). Could your code have that as an optional feature?
convert_to_word works on some weird input, and not on others. But it does not ever tell the user when something is wrong. I'd prefer if convert_to_word('--- -, .') failed in some way rather than returning 'OE'.


Jquery Geocoding Script The rules for this script are simple:

If geocode is hit, then just geocode
If geocode and submit is hit, then geocode and then submit
If an autosuggest link is hit, then geocode instantly
In this case, do not re-geocode later when the button is hit

This is my code, based on help and advice from here:

http://jsfiddle.net/spadez/Jfdbz/14/

$(function () {
    var lastQuery = null,
        lastResult = null, // new!
        autocomplete,
        processLocation = function (callback) { // accept a callback argument
            var input = $("#loc"),
                lat = $("#lat"),
                long = $("#lng");
            var query = $.trim(input.val()),
                geocoder;
            // if query is empty or the same as last time...
            if (!query || query === lastQuery) {
                if (callback) {
                    callback(lastResult); // send the same result as before
                }
                return; // and stop here
            }

            lastQuery = query; // store for next time

            geocoder = new google.maps.Geocoder();
            geocoder.geocode({
                address: query
            }, function (results, status) {
                if (status === google.maps.GeocoderStatus.OK) {
                    lat.val(results[0].geometry.location.lat());
                    long.val(results[0].geometry.location.lng());
                    lastResult = true; // success!
                } else {
                    alert("Sorry - We couldn't find this location. Please try an alternative");
                    lastResult = false; // failure!
                }
                if (callback) {
                    callback(lastResult); // send the result back
                }
            });
        },
        ctryiso = $("#ctry").val(),
        options = {
            types: ["geocode"]
        };

    if (ctryiso !== '') {
        options.componentRestrictions = {
            'country': ctryiso
        };
    }
    autocomplete = new google.maps.places.Autocomplete($("#loc")[0], options);

    google.maps.event.addListener(autocomplete, 'place_changed', processLocation);

    $('#search').click(function (e) {
        var form = $(this).closest('form');
        e.preventDefault(); // stop the submission

        processLocation(function (success) {
            if (success) { // if the geocoding succeeded, submit the form
                form.submit();
            }
        });
    });

    $('#geosearch').click(function (e) {
        var form = $(this).closest('form');
        e.preventDefault(); // stop the submission
        processLocation();
    });
});

I think I have reached the point where it works correctly, and I can't seem to make it any more structured, any more readable, any more efficient or compact. However, I am new to javascript and I would be really open to hear if anyone feels that they can improve on this.

I must admit I a bit pedantic and get a lot of satisfaction out of the refining process!

EDIT: I pasted the code wrong - fixed

Just a few things in no particular order:

Don't use long as a variable name. It's a reserved word in Javascript and can cause confusion.
If you're going to disable the country input in your form, you should probably just put the US keyword straight in your code. That is if you don't intend on enabling it.
You set ctryiso = $("#ctry").val() and then test ctryiso !== ''. Since you hard coded the country value in, that will always be true. Therefore, unnecessary test.
You don't need to initialize your lastQuery and lastResult to null. Any falsy value will do just fine in JS, including undefined.
If you look at the jQuery source code you'll see that the .click() simply calls another method and passes the values on. You can save a few function calls if you just call that other method directly.

Like so:

$('#search').click(function (e) {
    //Do stuff
}

//Do this one instead
$('#search').on('click', function (e) {
    //Do stuff
}

The 'click' can be replaced with any event of you're choosing. This gives you enormous power and control when setting up custom events.


Relaying stdin data from one thread to another I'd like to know if this piece of code is thread-safe. I'm learning something about threads, queues and synchronization, and I'd like to make sure this is correct, before moving forward to next step.

The code is very simple. It just runs two threads (producer and consumer). Producer reads data from stdin (until "0" is entered) and pushes into a queue.
Consumer, just prints queue contents each 10 secs.

The way I tried to make it thread-safe:

using an atomic boolean variable, running,  in the consumer loop
push to and pop from queue using mutex variable, m_queueMutex.

I cannot use c++11 standard. That's why I used boost library.

The code compiles and works as expected using GCC. To compile:
g++ -g -o queues queues.cpp  -lboost_thread -lboost_system -lboost_chrono

Any other advice will be very welcome.

Code:

#include <iostream>
#include <queue>
#include <string>
#include <boost/thread.hpp>
#include <boost/thread/scoped_thread.hpp>
#include <boost/chrono.hpp>
#include <boost/atomic.hpp>

std::queue<std::string> myqueue;
boost::atomic<bool> running;
boost::mutex m_queueMutex;

void push(std::string& val) {

    boost::mutex::scoped_lock lock(m_queueMutex);
    myqueue.push(val);
}

std::string pop() {
    std::string val = "";

    boost::mutex::scoped_lock lock(m_queueMutex);

    if (!myqueue.empty()){
        val = myqueue.front();
        myqueue.pop();
    }

    return val;
}

void wait(int seconds) {
  boost::this_thread::sleep_for(boost::chrono::seconds(seconds));
}

void consume () {
    while (running || !myqueue.empty()) {
        if (!myqueue.empty()) {
            std::string dt = pop();

            wait(10);
            std::cout << "[CONS] -- Got: " << dt << std::endl;
        }
        wait (1);
    }

    std::cout << "[CONS] -- Quiting.. " << std::endl;
}

void produce() {
    std::string data;

    do {
        std::cout << "# ";
        std::cin >> data;
        if (data != "0") {
            push(data);
            std::cout << "[PROD] -- Add: " << data << std::endl;
        }
    } while (data != "0");

    std::cout << "[PROD] -- Quiting.. " << std::endl;

    running = false;
}

int main() {
    std::cout << "Started... " << std::endl;

    running = true;

    boost::thread producer = boost::thread(produce);
    boost::thread consumer = boost::thread(consume);
    //boost::scoped_thread<> producer((boost::thread(produce)));
    //boost::scoped_thread<> consumer((boost::thread(consume)));

    producer.join();
    consumer.join();

    std::cout << "Ended... " << std::endl;
}

Right away I can see a potential problem. It appears that it is possible for the consumer thread to access "myqueue" outside of any lock.

void consume () {
while (running || !myqueue.empty()) {

This will probably still function just fine, but it is only as thread-safe as the queue's implementation for 'empty'. Unless the queue itself can guarantee that such an operation is safe, then the code is not thread safe. It is up to the specifics of the queues implementation, and the optimizations done by the compiler / processor, weather or not this would work as expected.

One other minor thing I notice is that the queue is being checked for empty twice. Once in the consumer before the global function pop() is called, and again in the pop function itself. This is a bit confusing to read, and should not be necessary. It may be better to lock the queue first, and then process items later, like so.

void consume()
{
bool hasWork = true;
while (running && hasWork)
{
    std::string item = "";
    mutex.lock();
    if (!myqueue.empty())
    {
        item = myqueue.pop();
    }
    else
    {
        hasWork = false;
    }
    mutex.unlock();

    // Do something with "item"
}
}

The idea here is that you are locking the queue only when you are performing operations on it, and leaving it unlocked while actually doing work on whatever was pulled form the queue. This is especially important when the individual work items may take significant time.

Other than that, it looks like you've got the right idea about needing to protect access to the queue's push and pop methods, it's just that you've got to protect all the other methods that touch 'myqueue' also.

Again, this would probably still work just fine on most implementations, but it's better to know you're safe.