Structure of parser in Clojure I'm working on my first Clojure project, which is a parser. Since it's my first project I'm unsure of many aspects, from my general approach to solving this problem to the small implementation details. I also don't know anyone proficient in Clojure so any guidance is welcome.

In higher terms, my implementation will take a string (which is one message to be parsed) and break it into fields. Each field it will apply some function to (often a dictionary lookup), and finally return a dictionary of results.

(use 'clojure.string)

;; This defines the breakdown of each line in the file. The line contains an entry and start with something
;; like "1AXYZ ". In this case the first two characters ("1A") are the "Record Type" and the next four
;; ("XYZ ") are the "Entering Firm". This data structure holds that information.
(def parser
  {:shared '(  ; These are the entries shared amongst all types of message

          ;; Field-Name  num-chars
             Record-Type 2
             Entering-Firm 4
             Clearing-Number 4
             Symbol 11
             Lot-Indicator 1
             ;;...
             )

   :1A     '(      ; this is one type of message, indicated with record-type=1A

          ;; Field-Name  num-chars
             Filler 9
             Poss-Dupe 1
             Booth-Booked-Indicator
             Order-Time 6
             Order-Date 8
             ;; ...
             )
   }
)

;; if no implementation is found for a field, just return a map to its contents.
(defn default [header x]
  {(str header) (trim (join x))})

;; most of the fields that dont use the default function will just be a dictionary lookup
(defn Poss-Dupe [[x]] (default 'Poss-Dupe ({\1 "Odd Lot" \D "Rule 902" \E "Poss Dupe Rule 902"
                                            \F "Auto Execution" \G "Poss Dupe of Auto-Execution"
                                            \H "Non-auto Execution" \I "Poss Dupe of Non-auto Execution"
                                            \0 "" \space ""}
                                               x (str "ERROR: " x))))

(defn Booth-Booked-Indicator [[x]] (default 'Booth-Booked-Indicator ({\0 "Post" \1 "Booth" \2 "Booked"} x (str "ERROR: " x))))

(defn Order-Time [x] (default 'Order-Time (->> x (partition 2) (map join) (join ":")  ))) ; "HHMMSS" -> "HH:MM:SS"

(defn Order-Date [x] (default 'Order-Date
                       (join "-" (map join (list (take 4 x) (take 2 (drop 4 x)) (drop 6 x)))))) ; "YYYYMMDD"->"YYYY-MM-DD"

(defn Filler [x] nil)

(defn try-call [f subseq]
  (try
    ((eval f) subseq)
    (catch Exception e  ; f not defined -> call default
      (default f subseq))))

;; Where the recursion happens
(defn grab-call [[f n & rest-instructions] line]
  (let [[field rest-line] (split-at n line)]
    (if (or (empty? line) (not f))
      nil
      (merge (try-call f field) (grab-call rest-instructions rest-line)))))

;;; Entry point
(defn parse-line [line]
  (grab-call (concat (:shared parser)
                     ((keyword (subs line 0 2)) parser))
    line ))

If you care to know the particulars of what is being parsed, that can be found at NYSE MRO specs.

There is a lot of weird stuff here.

It's strange to have no ns form for defining your namespace and requiring the libraries you need. Putting a bare use at the top is gross.
Don't use an entire namespace. Either require with an alias, like (:require [clojure.string :as s]) ... (s/join ...), or else require and refer the specific stuff you use: `(:require [clojure.string :refer [join trim]]).
The uppercase function names should be lowercase: poss-dupe, or even possible-duplicate, not Poss-Dupe
There should be a newline after the parameter definitions for most functions: (defn foo [x] \newline (...))
The eval is very suspicious, but I don't want to read your whole program and figure out what you should be doing instead. Probably ns-resolve or something.
(if (or (empty? x) f) nil y) is a bad way to write (when (and (seq x) f) y)
Usually calling keyword to look stuff up in a map suggests that you shouldn't be using keywords as keys, but in your case it looks fine-ish. However, instead of ((keyword whatever) m), it would be more readable to write (get m (keyword whatever)).


Inversion count using merge sort count = 0

def merge_sort(li):

    if len(li) < 2: return li
    m = len(li) / 2
    return merge(merge_sort(li[:m]), merge_sort(li[m:]))

def merge(l, r):
    global count
    result = []
    i = j = 0
    while i < len(l) and j < len(r):
        if l[i] < r[j]:
            result.append(l[i])
            i += 1
        else:
            result.append(r[j])
            count = count + (len(l) - i)
            j += 1
    result.extend(l[i:])
    result.extend(r[j:])
    return result

unsorted = [10,2,3,22,33,7,4,1,2]
print merge_sort(unsorted)
print count

Rather than a global count, I would suggest using either a parameter, or to return a tuple that keeps the count during each recursive call.  This would also assure you thread safety.

def merge_sort(li, c):
    if len(li) < 2: return li
    m = len(li) / 2
    return merge(merge_sort(li[:m],c), merge_sort(li[m:],c),c)

def merge(l, r, c):
    result = []

Since l and r are copied in merge_sort, we can modify them without heart burn. We first reverse the two lists O(n) so that we can use s.pop() from the correct end in O(1) (Thanks to  @ofer.sheffer for pointing out the mistake).

    l.reverse()
    r.reverse()
    while l and r:
        s = l if l[-1] < r[-1] else r
        result.append(s.pop())

Counting is separate from the actual business of merge sort. So it is nicer to move it to a separate line.

        if (s == r): c[0] += len(l)

Now, add what ever is left in the array

    rest = l if l else r
    rest.reverse()
    result.extend(rest)
    return result

unsorted = [10,2,3,22,33,7,4,1,2]

Use a mutable DS to simulate pass by reference.

count = [0]
print merge_sort(unsorted, count)
print count[0]


Handling two datepickers in a form with angularjs Here's I am using simple form with angularjs. Everything works fine. I am having watchgroup for EndDateFrom & EndDateTo datepickers. When it's defined I am logging the value to console. I am constructing an expression based on my if conditions if value is defined from datepickers, But i feel my code is little messy. Is there any better way to format this to make more simple.

I know we can't use switch case for this, but just checking if there is any other way to make less messy

$scope.$watchGroup(['$scope.EndDateFrom', '$scope.EndDateTo'], function () {

        if (!angular.isUndefined($scope.EndDateFrom) && !angular.isUndefined($scope.EndDateTo)) {

           return console.log(('expr://' + $filter('date')(new Date($scope.EndDateFrom), 'MM/dd/yyyy') + ' and ' + $filter('date')(new Date($scope.EndDateTo), 'MM/dd/yyyy')));
            // $scope.Test1 = 'expr://' + $filter('date')(new Date(disbursementsScheduleVm.processEndDateFrom), 'MM/dd/yyyy') + ' and ' + $filter('date')(new Date(disbursementsScheduleVm.processEndDateTo), 'MM/dd/yyyy');
        }
        else if(!angular.isUndefined($scope.EndDateFrom))
        {
            return console.log(('expr://' + $filter('date')(new Date($scope.EndDateFrom), 'MM/dd/yyyy')));
        }
        else if (!angular.isUndefined($scope.EndDateTo))
        {
            return console.log(('expr://' + $filter('date')(new Date($scope.EndDateTo), 'MM/dd/yyyy')));
        }

    });

Would something like that work better?

$scope.$watchGroup(['$scope.EndDateFrom', '$scope.EndDateTo'], function () {
  const FORMAT = 'MM/dd/yyyy';
  const dates = [];

  if (!angular.isUndefined($scope.EndDateFrom))
    dates.push($filter('date')(new Date($scope.EndDateFrom), FORMAT));

  if (!angular.isUndefined($scope.EndDateTo))
    dates.push($filter('date')(new Date($scope.EndDateTo), FORMAT));

  const expression = `expr://${dates.join(' and ')}`;
  console.log(expression);
  return expression;
});

Array.join() will help avoid branching. In your case we don't really save any branches, but in more complex cases it may save a lot.
I left angular.isUndefined(...) untouched since I don't know how different it is from the native JavaScript's analogue, but if it's equivalent you may want to use $scope.EndaDateFrom !== undefined instead.
Original return console.log is counter-intuitive. Is it returning a value, or logging it to the console, or both? I split that into two separate operations.
FORMAT is a reusable constant.
expr://${dates.join(' and ')} uses string interpolation to create result object.

Update 1

`expr://${dates.join(' and ')}`

in older versions of JavaScript should be replaced with a classic equivalent

'expr://' + dates.join(' and ')


What is a better approach for operating on an arbitrary map value in Scala? Coming from an imperative background, I have written the following code in Scala.  I need to attempt to find a value in a map.  If the value exists, I need to pass the value to another function and return the result, otherwise I need to return None.

def foo(index: Int): Option[FooBar] = {
  val mylookup: Map[Int, String] = this.generateMap()
  var out: Option[String] = None

  if (mylookup.containsKey(index)) {
    out = Some(mylookup(index))
  } else if (mylookup.containsKey("static_value")) {
    out = Some(mylookup("static_value"))
  }

  if (!out.getOrElse("").isEmpty) {
    return this.doSomeCall(out.get).makeFooBar()
  } else {
    return None
  }
}

I don't feel like this is the best way to accomplish this in Scala.  Any ideas on how to approach this using the idiomatic nature of the language?

myLookup.get(index).orElse(myLookup.get(static_value)).map { doSomeCall(_).makeFooBar }

From the Scala doc of Option.map:

  Returns a scala.Some containing the result of applying f to this scala.Option's value if      this scala.Option is nonempty. Otherwise return None.

This is exactly the type of task functional programming is made for. This simple chaining of Options using map is related to the fact that Option is a monad.


Minimum number of coins to make change I am new to programming and I am taking CS50 2014 online through iTunes University. This is the change-making problem in Problem Set 1:

  Write a program that first asks the user how much change is owed and then spits out the minimum number of coins with which said change can be made. Assume that the only coins available are quarters (25¢), dimes (10¢), nickels (5¢), and pennies (1¢).

I am pretty sure that my code is super "hacky." I would love understandable input on how I could have made a better algorithm or how I should have broken down the problem or "You did 'x' here"..."it would be better if you did 'y'."

#include <stdio.h>
#include <cs50.h>
#include <math.h>

int main(void)
{

    // declare float
    float money;

    // Get user input and check if input is positive
    do
    {
        printf("Please input the some amount of money you would like to make change for:  \n");
        money = GetFloat();
    } while (money < 0);

    // Get the Dollars from the Float
    int getDollars = money;

    // Get the cents from the Float
    float getCents = roundf((money * 100) - (getDollars * 100));
    int convertCents = getCents;

    // Greedy Algorithm for Dollars
    int getHundreds = getDollars / 100;
    int getFifties = ((getDollars) - (100 * getHundreds)) / 50;
    int getTwenties = ((getDollars) - (50 * getFifties) - (100 * getHundreds)) / 20;
    int getTens = ((getDollars) - (20 * getTwenties) - (50 * getFifties) - (100 * getHundreds)) / 10;
    int getFives = ((getDollars) - (10 * getTens) - (20 * getTwenties) - (50 * getFifties) - (100 * getHundreds)) / 5;
    int getOnes = ((getDollars) - (10 * getTens) -(5 * getFives) - (20 * getTwenties) - (50 * getFifties) - (100 * getHundreds)) / 1;

    //Greedy Algorithm for Cents
    int getQuaters = convertCents / 25;
    int getDimes = ((convertCents) - (getQuaters * 25)) / 10;
    int getNickles = ((convertCents) - (getQuaters * 25) - (getDimes * 10)) / 5;
    int getPennies = ((convertCents) - (getQuaters * 25) - (getDimes * 10) - (getNickles * 5)) / 1;

    printf("\n");
    if (convertCents == 0)
    {
        printf("The least possible steps you can make change from $%i.%i0 is %i. \n", getDollars, convertCents, (getHundreds + getFifties + getTwenties + getTens + getFives + getOnes + getQuaters + getDimes + getNickles + getPennies));
    }
    else if (convertCents < 10 && convertCents > 0)
    {
        printf("The least possible steps you can make change from $%i.0%i is %i. \n", getDollars, convertCents, (getHundreds + getFifties + getTwenties + getTens + getFives + getOnes + getQuaters + getDimes + getNickles + getPennies));
    }
    else
    {
        printf("The least possible steps you can make change from $%i.%i is %i. \n", getDollars, convertCents, (getHundreds + getFifties + getTwenties + getTens + getFives + getOnes + getQuaters + getDimes + getNickles + getPennies));
    }
}

Also please forgive me I kind of went out of the scope of the problem. I appended it knowing the fact that I would like to use currently circulating paper denominations as well as coins.

Printf format

I noticed that at the end of your program you had 3 cases due to trying to print the number of cents out correctly.  You can do it all in 1 case by using the %02i format.  This format prints an integer with 2 digit width and pads the leading digits with zeros.

Simplifying the logic

Right now, your logic for each denomination gets more and more complicated as you go along because you are subtracting more and more things from the total on each step.  If you simply subtracted the amount from the total right away, you wouldn't have to repeat it on each step.  For example:

int getHundreds = getDollars / 100;
getDollars -= getHundreds * 100;
int getFifties = getDollars / 50;
getDollars -= getFifties * 50;
int getTwenties = getDollars / 20;
getDollars -= getTwenties * 20;
// etc.

Using functions

Now that you have simplified the logic, you can put that logic into a function.  For example:

/**
 * @param        pAmount          Points to amount of money.  Returns the amount
 *                                minus the amount used up by this denomination.
 * @param        denomination     The value of this denomination.
 * @return                        The number of units of this denomination.
 */
int makeChange(int *pAmount, int denomination)
{
    int ret = *pAmount / denomination;

    *pAmount -= ret * denomination;
    return ret;
}

int main(void) {
    // ...
    int remainingDollars = getDollars;
    int getHundreds = makeChange(&remainingDollars, 100);
    int getFifties  = makeChange(&remainingDollars,  50);
    int getTwenties = makeChange(&remainingDollars,  20);
    // etc.
}

Reducing the number of variables

Since you aren't actually printing out how many of each bill/coin you are using, you don't need so many variables.  You only actually need to know how many bills plus coins you need.  So you could change your code to:

    int remainingDollars  = getDollars;
    int numBillsPlusCoins = 0;

    numBillsPlusCoins += makeChange(&remainingDollars, 100);
    numBillsPlusCoins += makeChange(&remainingDollars,  50);
    numBillsPlusCoins += makeChange(&remainingDollars,  20);
    // etc.

Using arrays

There's still a lot of repetition.  It would be nice to not have to copy/paste one line per denomination.  You can use arrays to handle each denomination in a loop instead.  You just need to create an array with the possible bill denominations and one for the coin denominations.

Some other miscellany:

I added my own GetFloat() function because I don't have whatever library you are using.
I removed the use of roundf() and just did the rounding myself.
I used a DIM() macro which returns the dimensions of a given array.  You will encounter this in C a lot for loops which need to loop over a statically defined array.

Here is the final code:

#include <stdio.h>

#define DIM(array)        (sizeof(array)/sizeof(array[0]))

float GetFloat(void)
{
    float ret = 0;
    scanf("%f", &ret);
    return ret;
}

/**
 * @param       pAmount         Points to amount of money.  Returns the amount
 *                              minus the amount used up by this denomination.
 * @param       denomination    The amount for this denomination.
 * @return                      The number of units of this denomination.
 */
int makeChange(int *pAmount, int denomination)
{
    int ret = *pAmount / denomination;

    *pAmount -= ret * denomination;
    return ret;
}

int main(void)
{
    // declare float
    float money;
    const int billDenominations[] = { 100, 50, 20, 10, 5, 1 };
    const int coinDenominations[] = { 25, 10, 5, 1 };

    // Get user input and check if input is positive
    do
    {
        printf("Please input the some amount of money you would like to "
                "make change for:  \n");
        money = GetFloat();
    } while (money < 0);

    // Get the Dollars from the Float
    int numDollars = (int) money;

    // Get the cents from the Float (rounded to nearest cent)
    int numCents = (int) ((money - numDollars) * 100 + 0.5f);

    int remainingDollars = numDollars;
    int remainingCents   = numCents;
    int numBillsAndCoins = 0;

    // Greedy Algorithm for Dollars
    for (int i = 0;i < DIM(billDenominations);i++)
        numBillsAndCoins += makeChange(&remainingDollars, billDenominations[i]);

    // Greedy Algorithm for Cents
    for (int i = 0;i < DIM(coinDenominations);i++)
        numBillsAndCoins += makeChange(&remainingCents, coinDenominations[i]);

    printf("\nThe least possible steps you can make change from "
            "$%i.%02i is %i. \n", numDollars, numCents, numBillsAndCoins);
}


Security of simple client/server desktop app I am writing an application aimed at the desktop but using web technologies. It will have an HTML interface and a server written in Python using Flask will do all of the filesystem interaction. It will have the ability to edit user settings so I want to make it impossible for other users to interfere. This is a little tricky because the server and client communicate using HTTP over a localhost port.

The program is split between 3 files:

run.py - controls the start up and shut down of the server and client.

import os
import json
import time
import signal

secret_file = os.path.join(os.environ['HOME'], '.myapp.secret')
if os.path.exists(secret_file):
    os.remove(secret_file)

os.system("python server.py &")

while not os.path.exists(secret_file):
    time.sleep(0.1)

f = open(secret_file)
secret = json.load(f)

url = 'http://localhost:' + str(secret['port']) + '/' + secret['key']

os.system("python client.py " + url)

os.kill(secret['pid'], signal.SIGINT)

server.py

from flask import Flask, request, Response
import socket
import random
import json
import sys
import os

app = Flask(__name__)
key = ''.join(random.sample(
    "1234567890qwertyuiopasdfghjklzxcvbnmQWERTYUIOPASDFGHJKLZXCVBNM",
    random.randint(8, 24)
))
port = 0
secret_file = os.path.join(os.environ['HOME'], '.myapp.secret')

@app.route('/<authkey>')
def hello(authkey):
    if authkey == key:
        return 'Hello, world!'
    else:
        return Response('Access Denied', 401)

def broadcast(port):
    s = json.dumps({
        'key' : key,
        'port' : port,
    'pid' : os.getpid()
    })
    f = open(
        secret_file,
        'w'
    )
    f.write(s)
    os.chmod(secret_file, 0400)
    f.close()

if __name__ == '__main__':
    sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
    sock.bind(('localhost', 0))
    port = sock.getsockname()[1]
    broadcast(port)
    sock.close()
    app.run(port=port)

client.py

This is a simple GTK window with a webkit widget that renders the page. It renders the URL passed to it by the first argument.

Is this sufficiently secure? Also, are there any other problems that I have not considered?

The important thing you want to enforce is that only the same local user can see the hello world.

You're relying at least on the inability of any random user to

listen in on someone else's sockets.
read another user's files
otherwise learn the secret
execute random python code in your server
And the inability for a remote attacker to connect to your socket.

1, 2 and 4 depend how well you've secured your system (no privilege elevation or random suid files lying around). 4 also depends on the security of flask and the rest of your python stack.

Five can be broken, depending on how your firewall is configured (someone could use connection tracking to get your socket forwarded, even though it is bound to localhost). You'd be better off using unix domain sockets.

The second is broken, because you chmod the file only after you've written to it. That gives a window for the attacker to open the file. In fact the attacker could even write to the file and execute arbitrary commands, which is why you should never use os.system, and you should use urlparse to build the url.

The third is broken in two ways:

the secret appears in the process list
the server won't take the same time to compare strings that are mostly right and mostly wrong. The attacker can guess the length of the secret, and guess the secret letter by letter (timing attack).


C++ Graph Implementation My data set is a list of Edges which are passed as a pair of integers. Based on that, I have the following graph implementation for BFS. Could someone please review my code and comment on errors/omissions/efficiency and other points.

#include <queue>
#include <iostream>
#include <vector>

class Graph {

private:
    int num_of_vertices;
    std::vector<int>* Adj;

public:
    Graph(int V){

        this->num_of_vertices = V;
        Adj = new std::vector<int>[V];
    }

    void addEdge(std::vector<std::pair<int, int>* > edge_list)
    {
        for(auto it = edge_list.begin(); it != edge_list.end(); it++) {
            std::pair<int, int>* p = *it;
            Adj[p->first].push_back(p->second);
        }
    }
    void BFS(int start)
    {
        bool* visited = new bool[this->num_of_vertices]();

        for(int i=0;i<num_of_vertices;i++)
            visited[i]=false;

        std::queue<int> queue;
        queue.push(start);
        std::vector<int>::iterator i;

        while(!queue.empty())
        {
            start = queue.front();

            visited[start] = true;
            queue.pop();
            for (i = Adj[start].begin(); i != Adj[start].end(); i++)
            {
                if (!visited[*i])
                    queue.push(*i);
            }
        }

    }

};

Pointer to vector?

You have two members: a pointer to a vector (which you new, but never delete, which leads to several other problems) and its size. But you're already using a vector, so just do it twice:

std::vector<std::vector<int>> adjacency_vector;

Graph(int num_vertices)
: adjacency_vector(num_vertices)
{ }

Pointer to pair?

Similarly, for addEdge, take a vector of pairs - not a vector of pointers to pairs - and by reference to const:

void addEdge(std::vector<std::pair<int, int>> const& edge_list)

And then just use a range-based for expression to add them all:

for (auto const& edge : edge_list) {
    adjacency_vector[edge.first].push_back(edge.second);
}

Pointer to bool?

You see a trend here, hopefully. In your BFS, you start with:

bool* visited = new bool[this->num_of_vertices]();

But you never delete it, so you're leaking that memory. Prefer:

std::vector<bool> visited(adjacency_vector.size());

Also, this already sets everything to false, so you don't have to manually. Though your original implementation did as well.

What does BFS do?

This is a void function, that does something with local variables queue and visited - and then what? What as the outside observer do I get? This function needs to return... something of value to the caller. Otherwise, it's just spending time doing nothing.

Also, again, prefer range-based-for for the push:

for (auto child : adjacency_vector[start]) {
    if (!visited[child]) {
        queue.push(child);
    }
}

It's just much shorter.


SPOJ Problem 2 - Prime Generator I'm trying to solve this problem. After a couple of tries, this is what I pulled off:

        #include<stdio.h>
    #define primeLimit 100000

    int prime (long int Start2,  long int Stop2 )
    {
       long int a[primeLimit+1];
       long int i,j,k,l;
       for (i=Start2;i<=Stop2;i++)
       {
           a[i-Start2] = 1;
       }

       for (i=Start2;i<=Stop2;i++)
       {
            if (a[i-Start2]!= 0 && i!=1)
            {
                for (j=3; j*j< i;j=j+2)
                {
                    if(i%j==0)
                        break;

                }
                if(j*j > i)
                {
                    printf(" \n %ld",i);
                    l = i;
                    if (i<=46340)
                    {
                    for (k = i*i; k< Stop2;)
                         {
                                while (k<46340 && (k-Start2 <100000))
                                a[k-Start2] = 0;
                                k = k+l;

                         }
                    }
                }
                else
                {
                    a[i-Start2] = 0;
                }
            }
       }

        return 0;
    }

    int main (void)
    {

       long int start,stop,a,look;

       scanf("%ld", &look);

       for (a=1;a<=look;a++)
       {
           scanf("%ld %ld", &start,&stop);
            prime (start,stop);
       }

       return 0;
    }

Here I used if (i<=46340) because the 46340*46340 exceeds the limit of a long int on a 32-bit machine (2,147,483,647). For the same purpose, I used while (k<46340 && (k-Start2 <100000)).

This code exceeds the time limit (6 seconds) of the SPOJ problem rule.

How can this be faster?

The short answer: restrict your sieving to what is necessary (sieve potential factors up to the square root of the upper end of the range, sieve the range itself). The runtime for that is a few milliseconds, compared to several seconds (or even minutes) for sieving all numbers up to the upper end of the range.

An extensive answer with all the bells and whistles is at StackOverflow, where the SPOJ PRIME1 question has already been asked and answered:

How do I efficiently sieve through a selected range for prime numbers?

In addition to what is said there: trial division is the slowest form of primality test ever. The Sieve of Eratosthenes is equally simple but it is the fastest of them all, for small numbers up to 2^64 or thereabouts. Special requirements like that of SPOJ PRIME1 require small complications - e.g. windowed/segmented operation - and there are many complications that can be added to make it even faster if that is desired. See the linked article. However, for SPOJ I would keep things sweet and simple.


Use case of nesting an enum into a struct At our office there is this piece of code (the original author isn't employed anymore, so I can't ask him).
Does anybody have an idea, what he did there? Is there any advantage of using this struct OrderDirection instead directly the enum Direction?
(for me, this is interesting and I just wanted to know why.)
public struct OrderDirection
{
    private enum Direction
    {
        Ascending = 0,
        Descending = 1
    }

    private OrderDirection(Direction direction)
    {
        _direction = direction;
    }

    private readonly Direction _direction;

    public override string ToString()
    {
        switch(_direction)
        {
            case Direction.Ascending: return "asc";
            case Direction.Descending: return "desc";
            default: throw new NotImplementedException(nameof(_direction));
        }
    }

    public static OrderDirection Ascending = new OrderDirection(Direction.Ascending);
    public static OrderDirection Descending = new OrderDirection(Direction.Descending);
}

In my opinion nothing indicate the use of enum.
According to my understanding your object provides the following functionalities from consumer perspective:

Provide two factory methods one for Ascending and another for Descending
Provide custom string representation (abbreviation) for these

From the public interface point of view the following implementation is equivalent with yours
public struct OrderDirection
{
    private readonly string _direction;
    private OrderDirection(string direction)
    {
        _direction = direction;
    }

    public override string ToString() => _direction;

    public static OrderDirection Ascending = new OrderDirection("asc");
    public static OrderDirection Descending = new OrderDirection("desc");
}


Non-concurrent write creates bottleneck I have some efficient methods that recursively creates a tree structure (NSTreeNode) of by looping recursively over a directory structure. The result is displayed in a NSOutlineView. This works well with one exception where I have to non-concurrently update a NSMutableDictionary that holds the structure of the tree. This turns out to be a real bottleneck in my program.

I am wondering if anyone has suggestions for how I can make this method more efficient or avoid the whole method.

-(void) addDictionaryItem:(NSString *) mykey withURL:(NSString *)myurl isDir:(BOOL) myIsDir andLR:(NSString *)LR
{
    NSDictionary *mydict=nil;

    @synchronized(@"concurrentDictAccess") {
        mydict = [self.dict objectForKey:mykey];
    }
    if ( mydict !=nil) {

            NSMutableArray *myarray = [mydict objectForKey:@"myarray"];
            NSMutableArray *myarrayIsDir = [mydict objectForKey:@"isdir"];
            NSMutableArray *myarrayLR = [mydict objectForKey:@"LR"];

            if (![myarray containsObject:myurl]){
                [myarray addObject:myurl];
                [myarrayLR addObject:LR];
                [myarrayIsDir addObject:[NSNumber numberWithBool:myIsDir]];
            }
        }
    else {

        NSMutableArray *arrayOfFiles = [NSMutableArray array];
        [arrayOfFiles addObject:myurl];

        NSMutableArray *arrayIsDir = [NSMutableArray array];
        [arrayIsDir addObject:[NSNumber numberWithBool:myIsDir]];

        NSMutableArray *arrayOfLR = [NSMutableArray array];
        [arrayOfLR addObject:LR];

        NSMutableDictionary *attrDict = [NSMutableDictionary   dictionaryWithObjectsAndKeys:arrayOfLR, @"LR",
                                     arrayOfFiles, @"myarray", arrayIsDir, @"isdir",
                                     nil];
        @synchronized(@"concurrentDictAccess") {
             [self.dict setObject:attrDict forKey:mykey];
        }
    }
}

The performance might be improved if you used more efficient synchronization mechanism.  The @synchronized directive is notoriously inefficient. It's pretty convenient way of doing mutex lock, but there are more efficient alternatives.

A common pattern is to use dedicated GCD serial queue to synchronize access. Even better, one can use the the reader-writer pattern: It's a concurrent GCD queue where you dispatch_sync to read operations to enjoy concurrency on read operations, but use dispatch_barrier_async to write). See the discussion of this pattern in WWDC 2012 video Asynchronous Design Patterns with Blocks, GCD, and XPC (it's in the latter portion of the video).

A couple of other reactions to the provided code snippet:

If you use @synchronized, I wouldn't suggest using string constants like that. The typical convention is to use the object that is being mutated (thus make sure it is instantiated up-front, and then you can do @synchronized (self.dict)).
Note, the provided code is not thread-safe. It has several issues:

One cannot just synchronize the initial read and final write separately. You generally have to wrap the whole routine in some synchronization mechanism. (And obviously, make sure any reads outside of this method are synchronized, too.)
BTW, the current code that adds of the entries to the existing array isn't currently synchronized at all.

This is really unrelated, but I wouldn't suggest three separate arrays for the URL,  LR (?), and isdir. I'd personally rather see an array of objects (either custom object or NSDictionary for which each object has its own URL, LR, and isDir). There are other approaches, too, but having three separate arrays doesn't seem very appealing.
Also unrelated, but I'm surprised that if you call this method and the key/URL both exist, that you don't update the existing record. Right now, you leave the old values there.


Project Euler #7 10001st prime I decided to start working on the Euler project exercises. I'm doing fine for now but the seventh problem is running quite slow and I can't think of anything to make it work faster. It takes around 1300-1500 ms to finish which doesn't satisfy me while I know it can be done in maybe 20-30 milliseconds.

int index = 1;
List<int> primes = new List<int> { 1 };
int i = primes[index - 1];
Stopwatch sw = new Stopwatch();
sw.Start();
while (primes.Count<10001)
{
    i += 2;
    bool isPrime = true;
    for (int j = 2; j < i / 2; j++)
    {
        if (i%j != 0) continue;
        isPrime = false;
        break;
    }
    if (!isPrime) continue;
    primes.Add(i);
    index++;
}
sw.Stop();
Console.WriteLine(primes[10000]);
Console.WriteLine("Time to calculate in milliseconds : {0}", sw.ElapsedMilliseconds);
Console.ReadKey();

bool isPrime = true;
for (int j = 2; j < i / 2; j++)
{
    if (i%j != 0) continue;
    isPrime = false;
    break;
}
if (!isPrime) continue;

This is one of the most primitive and least efficient ways to calculate whether or not a value is prime.

First and foremost, we deserve a method which encapsulates this logic and can be called repeatedly:

bool isPrime(int value)
{
    // return true/false
}

This will allow us to write a set of unit tests around this method to make sure it's correct.  It also allows us to very easily measure exactly how much this part of our total algorithm is costing us in terms of time.  And in the end, it just makes our code more readable, which for me is usually enough of a reason to do anything.

Now, as for the logic here... I've got a few comments.

j < i / 2

Neverminding the fact that we need better variable names, we're calculating far too many numbers.  Consider the prime number 97.  Your algorithm will test every number up to 48, but we could stop at the closest thing to 97's square root, 10.  If no number 10 or smaller divides evenly into 97 whose square root is 9.8 something, then it's prime.

j++

We're checking every divisor.  We could eliminate the even numbers early and just see if our test value is divisible by odd numbers.

if (i%j != 0) continue;

First of all, omitting braces is a capital sin as far as I'm concerned.  You shouldn't consider them optional.  But perhaps more importantly, why the inverse logic?  Your loop body could simply be written as:

if (i % j == 0)
{
    isPrime = false;
    break;
}

But... there's a far more efficient pattern, so let's get back to that isPrime method I was talking about...

Let's start with this logic to deal with a few special cases:

if (value < 2) { return false; } // no number less than 2 is prime
if (value % 2 == 0) { return value == 2; } // no multiple of 2 is prime
if (value % 3 == 0) { return value == 3; } // no multiple of 3 is prime
if (value % 5 == 0) { return value == 5; } // no multiple of 5 is prime

We're also going to go ahead and filter out 7, which is a prime.

if (value == 7) { return true; }

We're taken out a lot of the primes now.  The first line takes out a little over half of all valid numbers (all negatives, 0, and 1).

The second line takes out another half of the remaining by eliminated all the even numbers.

The third line takes out a third of the remaining by eliminating all of the odd multiples of three (even multiples of three were eliminated in previous line).

The fourth line takes out about twenty percent of the remaining by eliminated the multiples of five which were not already knocked out by being even or a multiple of three (so for example, 10, 15, 20 already knocked out, but here we knocked out 25).

The final line deals with 7 so that all of our special cases are handled before we enter this pattern:

for (int divisor = 7; divisor * divisor <= value; divisor += 30)
{
    if (value % divisor == 0) { return false; }
    if (value % (divisor + 4) == 0) { return false; }
    if (value % (divisor + 6) == 0) { return false; }
    if (value % (divisor + 10) == 0) { return false; }
    if (value % (divisor + 12) == 0) { return false; }
    if (value % (divisor + 16) == 0) { return false; }
    if (value % (divisor + 22) == 0) { return false; }
    if (value % (divisor + 24) == 0) { return false; }
}

This loop starts at 7 and increments by 30 on each iteration.  Each line of this loop tests a different off set which makes sure we're not double checking our special cases.  With each line we're testing against a smaller portion of possible values, because the line above it eliminated more.

To be clear, our special cases eliminated...

roughly 50%
half of remaining
1/3rd of remaining
1/5th of remaining

Through the first iteration of this loop, we eliminated by line

1/7th of remaining
1/11th of remaining
1/13th of remaining
1/17th of remaining
1/19th of remaining
1/23rd of remaining
1/29th of remaining
1/31st of remaining

Notice those denominators?  They're primes.

The loop becomes less efficient per iteration (and as you get into requiring more and more iterations of the loop, a sieve starts to become faster at the expense of memory).

But by the end of the first iteration of the loop, we have tested the value against the eleven primes.  And that's the most efficient way to determine if a number is a prime, by testing if it is divisible by primes (which is what sieves allow you to do).

There's no point in checking if 97 is divisible by 4, because if it was going to be divisible by 4, it would have been divisible by 2, and we already checked 2.
There's no point in checking if 97 is divisible by 9, because if it was going to be divisible by 9, it'd be divisibly by 3, which we already checked.
There's no point in checking if 97 is divisible by 25, because if it was going to be divisible by 25, it'd be divisible by 5, which we already checked.

Also, we know that if 97 were divisible by 25, we would have already found the other number that divides in to it.  And to be more clear on that part, let's look at a similar number that isn't prime: 98.

So, 98 is divisible by 49, right?  49 will go into 98 twice.  So naïvely, we might consider in order to determine if 98 is prime, we need to check all the way up to 98 % 49, right?  Wrong.  Numbers factor into pairs.  For every pair, we only need to test the smallest pair against the value.

So for the factors of 98, we have these pairs:

(2, 49)
(7, 14)

For any number, if we break it into pairs like this and look at the smaller number in the pair, the largest smaller number will be less than or equal to the square root of the number we are factoring.

Consider a number that is a perfect square, like 100.  It's factor pairs look like this:

( 2,50)
( 4,25)
( 5,20)
(10,10)

Again, we only need to check the left column.  Here, the largest value out of the left column is 10.  This is the exact square root of 100, and we can stop our checking here.  No need to go all the way up to halfway.

Anyway, long story short, the final isPrime function looks about like this:

bool isPrime(int value)
{
    if (value < 2) { return false; }
    if (value % 2 == 0) { return value == 2; }
    if (value % 3 == 0) { return value == 3; }
    if (value % 5 == 0) { return value == 5; }
    if (value == 7) { return true; }

    for (int divisor = 7; divisor * divisor <= value; divisor += 30)
    {
        if (value % divisor == 0) { return false; }
        if (value % (divisor + 4) == 0) { return false; }
        if (value % (divisor + 6) == 0) { return false; }
        if (value % (divisor + 10) == 0) { return false; }
        if (value % (divisor + 12) == 0) { return false; }
        if (value % (divisor + 16) == 0) { return false; }
        if (value % (divisor + 22) == 0) { return false; }
        if (value % (divisor + 24) == 0) { return false; }
    }

    return true;
}


First missing positive As usual, please be brutally honest with your opinion of my code, and how you would judge it if I were to code this at a top-tier tech company for an interview(think Google, MSFT, Facebook, etc). My algorithm is has a worst time complexity of O(n).

Question:
Given an unsorted integer array, find the first missing positive integer.
For example,

Given [1,2,0] return 3,
and [3,4,-1,1] return 2.*

My code(coded in 22 minutes, with all test cases passed, devised initial algorithm in 2-5 minutes)

  public int firstMissingPositive(int[] A) {
        int minVal=0;
        if(A.length==0){
            return 1;
        }
        Hashtable<Integer, Integer> myTable = new Hashtable<Integer, Integer>();
        for(int i : A){
            if(i<minVal && i>0){
                minVal = i;
            }
            myTable.put(i, i);
        }

        for(int i=0; i<=A.length; i++){
           if( (!myTable.containsKey(minVal-1)) && minVal-1>0){
                return minVal-1;
            }
            else if(!myTable.containsKey(minVal+1)){
                return minVal+1;
            }
            minVal++;
        }
        return minVal;
    }

I have to disagree with palacsint about using HashMap, if anything, I think that you should use a Set implementation such as HashSet. You're never using the value that is stored in the map, and you have no reason to use it either. Therefore, I would go with a Set. (The fact that a HashSet internally uses a HashMap is an entirely different subject) Palacsint is correct though that HashMap is a better alternative than Hashtable.

Reading lines like this makes me sea-sick:

if( (!myTable.containsKey(minVal-1)) && minVal-1>0){

Use spacing properly and remove unnecessary parameters and it becomes:

if(!myTable.containsKey(minVal - 1) && minVal - 1 > 0) {

Overall, I think that you shouldn't need to use Objects for this (except an array of course, which technically is an object). I would only use primitives and use more self-documenting variable names.

An unnecessary operation in your code is that on each iteration you check containsKey twice. Once to see if value - 1 is set and once to see if value + 1 is set. Why not just check if value itself is set?

Your code can be rewritten and optimized into this: (This is a fixed version of ChrisW's approach, I had started the approach when he posted his answer and then I did some modifications to make sure it matched your code).

public int firstMissingPositive(int[] array) {
    boolean[] foundValues = new boolean[array.length];
    for (int i : array) {
        if (i > 0 && i <= foundValues.length)
            foundValues[i - 1] = true;
    }

    for (int i = 0; i < foundValues.length; i++) {
        if (!foundValues[i]) {
            return i + 1;
        }
    }
    return foundValues.length + 1;
}


Least common ancestor for binary search tree I want you to pick my code apart and give me some feedback on how I could make it better or more simple. This code finds a common ancestor for binary search tree.  This code accounts duplicates as well as incomplete tree, and also throws an exception if input element is not in the tree.

public class LeastCommonAncestorBST {

 private TreeNode root;

    private static class TreeNode {
        TreeNode left;
        TreeNode right;
        int item;

        TreeNode (TreeNode left, TreeNode right, int item) {
            this.left = left;
            this.right = right;
            this.item = item;
        }
    }

    public void makeBinarySearchTree(Integer[] a) {
        for (int i : a) {
            addElement(i);
        }
    }

    public void addElement(int element) {
        if (root == null) {
            root = new TreeNode(null, null, element);
        } else {
            TreeNode prevNode = null;
            TreeNode node = root;

            while (node != null) {
                prevNode = node;
                if (element <= node.item) {
                    node = node.left;
                } else {
                    node = node.right;
                }
            }

            if (element <= prevNode.item) {
                prevNode.left = new TreeNode(null, null, element);
            } else {
                prevNode.right = new TreeNode(null, null, element);
            }
        }
    }

    public int leastCommonAncestor(int n1, int n2) {
        TreeNode node  = findLCA(root, n1, n2);
        if (node != null) {
            return node.item;
        } else {
            throw new IllegalArgumentException(" Input was not valid ");
        }
    }

    private TreeNode findLCA(TreeNode node, int n1, int n2) {

        if (node == null) return null;

        if (node.item > n1 && node.item > n2) {
            return findLCA(node.left, n1, n2);
        }

        if (node.item < n1 && node.item < n2) {
            return findLCA(node.right, n1, n2);
        }

        boolean bothExist = doesExist(node, n1) && doesExist(node, n2);

        return bothExist ? node : null;
    }

    private boolean doesExist(TreeNode node, int n) {
        if (node == null) {
            return false;
        }
        if (n < node.item) {
            return doesExist(node.left, n);
        }

        if (n > node.item ) {
            return doesExist(node.right, n);
        }
        return true;
    }
}

Most of the code is pretty good!

I find your interface a bit weird.  Instead of public void makeBinarySearchTree(Integer[] a), I suggest public void addElements(int[] elements) to be consistent with your addElement(int) method.  There's no need to tell everyone that you're making a binary search tree, since that information is encoded into your class name already.

You might want to generalize the code to handle any Comparable, not just ints.  You would get more flexibility for little additional effort.

Instead of throwing IllegalArgumentException, I would throw NoSuchElementException.  In fact, I would change doesExist() to

private TreeNode find(TreeNode start, int element) {
    if (start == null) {
        throw new NoSuchElementException(Integer.toString(element));
    }
    if (element < start.item) {
        return find(start.left, element);
    }
    if (element > start.item) {
        return find(start.right, element);
    }
    // Assertion makes the code more readable
    assert element == start.item;
    return start;
}

That way, you can let the exception propagate up all the way, and it would tell you which of the two numbers passed to leastCommonAncestor() is missing.

Your addElement() is a bit repetitive and can be simplified:

public void addElement(int element) {
    // No matter what happens, we're going to attach this
    // new node somewhere.
    TreeNode newNode = new TreeNode(null, null, element);

    if (root == null) {
        root = newNode;
        return;  // Return early to avoid a layer of nesting
    }

    TreeNode node = root;
    while (true) {
        if (element <= node.item) {
            // Test for null before following the pointer.
            // That relieves you from having to keep track
            // of the parent node, and also avoids having
            // to decide again whether to attach to the
            // left or right.
            if (node.left == null) {
                node.left = newNode;
                return;
            }
            node = node.left;
        } else {
            if (node.right == null) {
                node.right = newNode;
                return;
            }
            node = node.right;
        }
    }
}

Follow-Up Q & A

  Not understanding your feedback - I suggest public void
  addElements(int[] elements) to be consistent with your addElement(int)
  method.

In Java, you use unboxed primitives most of the time.  Boxed types are rare: generally they are only used in Collections (which can't store primitives) and when you want to store a value that might be null.  An Integer[] array is therefore rather unnatural — it's neither an int[] nor a List<Integer>, both of which would be more likely to occur.

  As regards to cleaning up addElement() I had few concerns

  while(true) is said to be bad
  more checks in your method (a) while(true) (b) null check
  (c) comparison, while my code does only 2 of them for most traversal
  part (a) null and (b) compare.

There are two common objections to while (true): one invalid concern about efficiency, and one legitimate concern about style.

Many decades ago, simplistic C compilers might take while (true) literally, and generate code to load 1 into a register, check whether the register was non-zero, and execute a conditional jump.  Modern C compilers and all Java compilers are smart enough to handle while (true) efficiently, generating nothing but an unconditional goto at the end of the loop.  You can verify this by inspecting the bytecode:

$ cat WhileTrue.java
public class WhileTrue {
    private WhileTrue() {}  // Suppress default constructor

    public static void main(String[] args) {
        System.out.println("Before while loop");
        while (true) {
            System.out.println("In while loop");
        }
        // Compiler would detect this as unreachable
        // System.out.println("After while loop");
    }
}

$ javap -c WhileTrue
Compiled from "WhileTrue.java"
public class WhileTrue extends java.lang.Object{
public static void main(java.lang.String[]);
  Code:
   0:   getstatic   #2; //Field java/lang/System.out:Ljava/io/PrintStream;
   3:   ldc #3; //String Before while loop
   5:   invokevirtual   #4; //Method java/io/PrintStream.println:(Ljava/lang/String;)V
   8:   getstatic   #2; //Field java/lang/System.out:Ljava/io/PrintStream;
   11:  ldc #5; //String In while loop
   13:  invokevirtual   #4; //Method java/io/PrintStream.println:(Ljava/lang/String;)V
   16:  goto    8

}

That means that your second objection is invalid, since while (true) is not a check.  In fact, your original version had more conditionals since you had to decide which side to append to after exiting the loop.

As previously mentioned, there is another problem with while (true), which is that it keeps the reader in suspense.  Most programs are not intended to run in an infinite loop.  Therefore, when you see while (true), you immediately suspect that something within the loop must eventually cause it to terminate: a break, a return, or an exception.  Therefore, whenever you feel the urge to write while (true), it's always worthwhile to think about whether it is possible to restructure the code such that the termination condition can be expressed in the loop header, rather than buried in the loop body.  In many instances, it is possible, but here, I think that my proposal is already optimal.


Express a number as a sum of powers of 2 This is my function:

def two_powers(num):
    powers = []
    i = 1
    while i <= num:
        if i & num:
            powers.append(i)
        i <<= 1
    return powers

I have python 3.6(Windows 10 64-bit). I want the result in the form of a list. My problem statement is to express a integer(num) in the form of sum of powers of 2.

Do I have to create a list in the beginning and then append to it each time ? Can I return a list directly without creating it in the beginning ?

This will will speed up my execution time, right ?

List building performance aside, there is a way to get the powers of two out without touching the bits in between, by using the formulas:

isolateLowestSetBit(x) = x & -x
resetLowestSetBit(x) = x & (x - 1)

These have been explained elsewhere.

Using those definitions, you can extract the powers of two like this:

def two_powers(num):
    powers = []
    while num != 0:
        powers.append(num & -num)
        num = num & (num - 1)
    return powers

Since it potentially avoids testing many bits (depending on the size and sparseness of the input) it may be faster. It can also work out not so well, for very dense numbers.


Finding the closest value in a collection in a game-server plugin implementation I have an array of a instances of an enum type. A random double variable is initialized with a value between 0.... to 1, and should be used to get the closest value's enum instance to that randomly set double value.
The enum looks like this:
public enum TargetScope {

    CLEAN(1, 5040),
    FIRST(.888, 5046),
    SECOND(.777,5052),
    THIRD(.666,5058),
    FOURTH(.555, 5064),
    FIFTH(.444, 5070),
    SIXTH(.333, 5076),
    SEVENTH(.222, 5082),
    EIGHTH(.001, 5088);

    private int interfaceBase;
    private double time;

    TargetScope(double time,int interfaceIdBase) {
        this.time = time;
        this.interfaceBase = interfaceIdBase;
    }

    public int getTime() {
        return (int) this.time * TargetManager.TARGET_SEARCH_WAIT;
    }

    public int getInterfaceBase() {
        return this.interfaceBase;
    }

    public double getPercent() {
        return this.time;
    }
}

I have implemented an algorithm which does that, using the array and few temp variables:
public TargetScope getInterfaceScopeByTime() {
    double alpha = (double) this.time / TargetManager.TARGET_SEARCH_WAIT;

    TargetScope[] scopes = TargetScope.values();

    double delta = 1;
    TargetScope scope = null;

    for (int i = 0; i < scopes.length; i++) {
        double distance = Math.abs(scopes[i].getPercent() - alpha);
        if (distance < delta) {
            delta = distance;
            scope = scopes[i];
        }
    }

    return scope;
}

Basically it gets the closest Enum.getPercent() value in the list to alpha, and then returns the final closest TargetScope instance.
But I really feel bad about using this, where Java has an advanced collection API, which can be used better for this case, but I am not really as experienced with Collections API to use them for that.
Is there really a shorter way to perform this?
The idea behind this
I am working on a game-server plugin, which basically makes two players targets and have them fight each other. For a player to get a target, he needs his target-timer to hit 0, in order to let the system search for a target. Originally, the default target timer is 1000, each system cycle is 600ms, so 1000 is 10 minutes.
Each player has an interface that looks like a sliced circle to 8 pieces, something like this:

I need to let the interface know, what slices should be filled with colour, to let the player know when he can get his target. a fully coloured scope (circle) means that the timer hit 0.
Each interface slice has it's own id, as you see in the enum, int interfaceBase.
I need to update the player's interface, by using the player's current left time.
So if his time is 800, I do 800 / 1000, I get 0.8, now from the enums list, the closest enum is called SECOND because the closest value to 0.8 out of the list is 0.777, therefore when the server updates player's interface exactly when the timer is 800, then the interface will display one slice out of 8.
The interface updates every 10 seconds, per player (as in each player has its executor).

This problem can be solved mathematically much better than it can with a list, or collection.

What you have is 9 states, and a timer that counts down. The timer can be expressed as a proportion of 1. Because the states are all equally spaced on 1/9 intervals, you can just do math....

So, if the time is \$x\$ and the total time is \$y\$, then the current portion is \$\frac{x}{y}\$ If you multiply this value by 9, you get something on the scale of 0 to 9 inclusive. you really want:

public int tileID(int currentTime, int totalTime, int tileCount) {
    // we want the shift to happen at less than half-the-time (in the middle of the period).
    double shift = (1.0 / tileCount) / 2;
    double progress = (double)currentTime / (double)(totalTime);
    int tile = (int)(progress * tileCount + shift);
    return tile;
}

shift is needed to make the (int) truncation work. Your comment has made me think, and, in reality, it is not the most readable/understandable code. In fact it is broken, and shift should just be 0.5 always... Let me re-do it in the form of a round() instead of a truncation:

public int tileID(int currentTime, int totalTime, int tileCount) {
    double progress = (double)currentTime / (double)totalTime;
    int tile = (int)Math.round(progress * tileCount + 0.3);
    // because your tiles are in the opposite order: tileCount - 1 - tile
    return tileCount - 1 - tile;
}

Then, you can use this with:

TargetScope tile = TargetScope.values[tileId(this.time,
              TargetManager.TARGET_SEARCH_WAIT, TargetScope.values[].length)];


Readability of two rectangular point function samples I have two implementations of the same function. Which of these is easier to understand?

Code sample A

function makeRectanglePoint(box, index){

    var boxToUse;

    function randomPoint(a, b){
        return Math.random() * Math.abs(a - b) + Math.min(b, a)
    }

    if(index % 2){ boxToUse = box.p1 } else { boxToUse = box.p2 }

    if(index < 2){
        return new Point(randomPoint(box.p1.x, box.p2.x), boxToUse.y);
    } else {
        return new Point(boxToUse.x, randomPoint(box.p1.y, box.p2.y));
    }

}

Code sample B

function makeRectanglePoint(rectangle, pointIndex){

    function getRectangleEdge(){

        function getPointByIndex(index) {
            switch (index) {
                case 0: return rectangle.p1;
                case 1: return new Point(rectangle.p2.x, rectangle.p1.y);
                case 2: return rectangle.p2;
                case 3: return new Point(rectangle.p1.x, rectangle.p2.y);
            }
        }

        function Line(p1, p2){
            this.p1 = p1;
            this.p2 = p2;
        }

        var endPointIndex = (pointIndex + 1) % 4;
        return new Line(
            getPointByIndex(pointIndex),
            getPointByIndex(endPointIndex)
        );
    }

    function generateRandomPointOnEdge(line){

        function generateRandomBetween(a, b){
            return Math.min(a, b) + Math.random() * Math.abs(a - b);
        }

        return new Point(
            generateRandomBetween(line.p1.x, line.p2.x),
            generateRandomBetween(line.p1.y, line.p2.y)
        );

    }

    return generateRandomPointOnEdge(getRectangleEdge());
}

I'd say version A wins simply by being more succinct. Version B does however provide some extra detail, such as the fact that the point being added is being added on an edge (not inside a rectangle). Still, the structure of version A is a lot more straightforward to read.

Both version could use comments though!

I'd also replace the 1-line if..else with a simple ternary:

boxToUse = index % 2 ? box.p1 : box.p2;

Speaking of, shouldn't that be pointToUse instead of boxToUse? In either case, though, I'd ask "to use for what exactly?"

I don't know what this is used for, but I imagine a function that returns a random number in a given range would be useful elsewhere, so you might consider extracting it. Regardless, its name shouldn't be randomPoint, since it doesn't return a point - it just returns a number.

As for version B (just to give it a once over):

Using a constructor for Line is overkill. Just use an object literal: {p1: p1, p2: p2}
The switch is tricky. It has no default case and instead relies on its input being 0-3

All in all it's hard to say much more, since I don't know the context.

But a 3rd way might be better still. I'd recommend something like

// Returns a point lying on one of the rectangle's edges
// The edge is given as a number:
// 0: top edge
// 1: right edge
// 2: bottom edge
// 3: left edge
function makeRectanglePoint(rect, edge) {
  // generate a random number within the rect's height or length
  var rand = edge % 2 ? random(rect.p1.y, rect.p2.y) : random(rect.p1.x, rect.p2.x);

  switch(edge % 4) {
    case 0: return new Point(rand, rect.p1.y); // top
    case 2: return new Point(rand, rect.p2.y); // bottom
    case 1: return new Point(rect.p2.x, rand); // right
    case 3: return new Point(rect.p1.x, rand); // left
  }

  // handle errors (i.e. edge not being an integer or being negative)
  throw new Error("Edge index should be an integer between 0 and 3 (inclusive)";
}


Project Euler 22: Names scores Description of challenge:

  Using names.txt (right click and 'Save Link/Target As...'), a 46K text
  file containing over five-thousand first names, begin by sorting it
  into alphabetical order. Then working out the alphabetical value for
  each name, multiply this value by its alphabetical position in the
  list to obtain a name score.

  For example, when the list is sorted into alphabetical order, COLIN,
  which is worth \$3 + 15 + 12 + 9 + 14 = 53\$, is the 938th name in the
  list. So, COLIN would obtain a score of \$938 × 53 = 49714\$.

  What is the total of all the name scores in the file?

I quickly created a solution: it was fairly easy, but I am specifically concerned about the performance, which I am unsure of. What I do know is:

string.split(): \$O(n)\$
Arrays.sort(): \$O(n \log n)\$ Worst Case
Remaining code: \$O(nm)\$, where \$n\$ is the length, and \$m\$ is the average length of the names

The code is below:

import java.io.BufferedReader;
import java.io.FileReader;
import java.io.IOException;
import java.util.Arrays;

public class ProjectEuler22 {

    private static final String filePath = "names.txt";
    private static final int ASCII_VALUE_TO_INT = 64; // naming

    public static void main(String[] args) {
        long time = System.nanoTime();
        String[] names = readAllNames().split("\",\"");
        names[0] = names[0].substring(1);
        int length = names.length; // extra "
        names[length - 1] = names[length - 1].substring(0,
                names[length - 1].length()); // extra "
        Arrays.sort(names);
        int total = 0;
        for (int i = 0; i < length; i++) {
            total += getValueOfName(names[i], i + 1);
        }
        time = System.nanoTime() - time;
        System.out.println("The result is: " + total + "\nTime taken (ns): "
                + time);
    }

    private static String readAllNames() {
        try (BufferedReader reader = new BufferedReader(
                new FileReader(filePath))) {
            return reader.readLine();
        } catch (IOException e) {
            e.printStackTrace();
            return null;
        }
    }

    /**
     * Gets the value of the given string. If there are characters other than
     * [A-Z], then it will ignore it. Note that the problem file only contains
     * names with all caps.
     *
     * @param string
     *            the name to get the value of
     * @param i
     *            the index of the name
     * @return the value of the name
     */
    private static int getValueOfName(String string, int i) {
        char[] array = string.toCharArray();
        int sum = 0;
        for (char c : array) {
            if (Character.isUpperCase(c)) {
                sum += c - ASCII_VALUE_TO_INT;
            }
        }
        return sum * i;
    }

}

Output:

   The result is: 871198282
 Time taken (ns): 19757146

Time taken seems to be around 20_000_000 nanoseconds.

Specific concerns:

Is there a better way to solve this? (As said above)
Where it says the // naming comment, is the naming fine? Anywhere else where the naming is off?
Anything else?

Well, there's a potentially more efficient way:

Read the whole file into a byte-array (It's not even a measly 64K), thus avoiding any conversion overhead and most object-allocations, extract the start-indices of all the names, and sort them.

Is it equally nice? Probably not. And it will only speed it up by a constant factor, because more just isn't there to be had.
Anyway, measure it because while going down a level gives opportunities for optimization, it's easy to get it wrong.
If you don't want to go quite that far, here's an easy one: Strip the trailing " from the last name, and you can drop the test whether you actually have a letter in getValueOfName.
Getting to your constants name, that's atrocious. Because, you know, using a named constant for that at all is so bad.
You can simply use a character-literal: 'A'-1
You fail to close the file in readAllNames if newing the BufferedReader fails. Admittedly that's unlikely in such a small program, but anyway.
Also, why do you catch IOException there, but then return null which immediately causes a NPE in turn in main?


Chain Responsibility Pattern to filtering messages Is this a good implementation of a Chain Responsibility Pattern? This code attempts to filter a message received (for instance, in a chat application).

import urlmarker
import re

bad_words = []
with open("bad-words.txt") as file:
    for entry in file:
        entry = entry.strip()
        bad_words.append(entry)

class Message(object):

    def __init__(self, content):
        self.content = content.split()
        self.can_broadcast = True
        self.construct_word()

    def construct_word(self):
        self.content = ' '.join(self.content)

class Filter():

    def __init__(self, successor = None):
        self.successor = successor

    def has_successor(self):
        if self.successor is not None:
            return True
        return False

    def filter_message(self, message):
        if self.has_successor():
            self.successor.filter_message(message)

class Slang_Filter(Filter):

    def filter_message(self, message):
        has_bad_words = False

        for word in message.content:
                for bad_word in bad_words:
                    if bad_word == word.lower():
                        has_bad_words = True
                        break

        if has_bad_words == False:
            if self.has_successor():
                self.successor.filter_message(message)
        else:
            message.can_broadcast = False

class Website_Filter(Filter):

    def filter_message(self, message):
        has_websites = False

        if re.findall(urlmarker.URL_REGEX, message.content) is None:
            if self.has_successor():
                self.successor.filter_message(message)
        else:
            message.can_broadcast = False

def main():
    new_message = Message("This is full of fucking shit www.google.com")
    slang_filter = Slang_Filter()
    website_filter = Website_Filter(slang_filter)
    website_filter.filter_message(new_message)
    print new_message.can_broadcast

if __name__ == '__main__':
    main()

Chain of Responsibility

Rather than building the chain on instantiation, make the creation of the chain a function.
This can allow you to chain the creation, which is easier to read and nicer to look at.

I'd also make the chain return, rather than mutate a foreign object.
This removes side effects, which lead to hard to understand code, and potential bugs.

has_successor is a poor function, it's simpler to read and understand using if self._next is not None, rather than using that abstraction.
And so I'd remove it, I'd favour writing out the non-function way anyway.

And so I'd come to:
class Filter(object):
    _next = None
    def add_filter(self, next_filter):
        self._next = next_filter
        return next_filter

    def filter_message(self, message):
        if self._next is None:
            return True
        return self._next.filter_message(message)

class SlangFilter(Filter):
    def filter_message(self, message):
        for word in message.content:
            for bad_word in bad_words:
                if bad_word == word.lower():
                    return False

        if self._next is None:
            return True
        return self._next.filter_message(message)

class Website_Filter(Filter):
    def filter_message(self, message):
        if re.findall(urlmarker.URL_REGEX, message.content) is not None:
            return False

        if self._next is None:
            return True
        return self._next.filter_message(message)

This then lets you create the chain as:
def main():
    new_message = Message("This is full of fucking shit www.google.com")
    filter = WebsiteFilter()
        .add_filter(SlangFilter())
    print filter.filter_message(new_message)

Going forward, you'd want to change your classes in a couple of other ways too.

Rather than relying on a global bad words variable, SlangFilter should be passed these words on instantiation.
Rather than manually re-creating in, you can use it to reduce code complexity, and for a performance boost, if you were to change the list of words to a set.
Your code shouldn't rely on an unneeded model, as a string is good enough for this.

And so I'd use:
import urlmarker
import re

class Filter(object):
    _next = None
    def add_filter(self, next_filter):
        self._next = next_filter
        return next_filter

    def filter_message(self, message):
        if self._next is None:
            return True
        return self._next.filter_message(message)

class SlangFilter(Filter):
    def __init__(self, bad_words):
        self._bad_words = {word.lower() for word in bad_words}

    def filter_message(self, message):
        for word in message.lower().split():
            if word in self._bad_words:
                return False

        if self._next is None:
            return True
        return self._next.filter_message(message)

class Website_Filter(Filter):
    def filter_message(self, message):
        if re.findall(urlmarker.URL_REGEX, message) is not None:
            return False

        if self._next is None:
            return True
        return self._next.filter_message(message)

def main():
    with open("bad-words.txt") as file:
        bad_words = [word.strip() for word in file]

    message = "This is full of fucking shit www.google.com"
    filter = WebsiteFilter()
        .add_filter(SlangFilter(bad_words))
    print filter.filter_message(message)

if __name__ == '__main__':
    main()


Simple JavaScript quiz I'm learning JavaScript and have tried making a simple quiz from scratch.

JS Bin

//Define global variables
    var count = 0,
          zCount = 0,
          yCount = 0,
          sumPoints = 0,
          aQuestion,
          allQuest,
          aChoices,
          bChoices,
          allQuestions,
          finishedDoc,
          someValue;

    allQuestions = [
            {
            question: "What is the capital of Peru?",
            choices: ["Buenos Aires", "La Paz", "Lima", "Quito"],
            correctAnswer: 2
        },
            {
            question: "What is the capital of Bolivia?",
            choices: ["La Paz", "Buenos Aires", "Quito", "Lima"],
            correctAnswer: 0
        },
            {
            question: "What is the capital of Ecuador?",
            choices: ["Lima", "Quito", "Buenos Aires", "La Paz"],
            correctAnswer: 1
        },
            {
            question: "What is the capital of Argentina?",
            choices: ["Quito", "La Paz", "Lima", "Buenos Aires"],
            correctAnswer: 3
        }
    ];

    function renderQuestions() {
        aQuestion = document.getElementById("thisQuestion");
        aChoices = document.getElementsByTagName("label");
        bChoices = document.getElementsByTagName("input");
        if(count < allQuestions.length){
            allQuest = allQuestions[count].question;
            count++;
        }
        else{
            alert("You have a total of " + sumPoints + "/4" + " correct answers!");
            finished();
        }
        aQuestion.innerHTML = allQuest;
        //render the choices and structure
        for(var i = 0; i < allQuestions.length; i++){
            aChoices[i].innerHTML = allQuestions[zCount].choices[i];
        }
        zCount++;

    }
    renderQuestions() //render first question
    function checkAnswer() {

        someValue = allQuestions[yCount].correctAnswer;
        if(bChoices[someValue].checked == true){
            alert("That's correct!");
            sumPoints += 1;
        }
        else{
            alert("Nope, sorry!");
        }
        yCount++;
        renderQuestions();
    }

    function finished(){
        finishedDoc = document.getElementById("thisCont");
        finishedDoc.innerHTML = "<h1>Finished!<\/h1>";
    }

Here are some critiques. I focused on what is most important for a beginner to think about.

Use Objects

You have conceptually clear units of data you should consider breaking into formally declared objects with constructors. Take a look at this link, especially "Using an Object Constructor". In the long run, this will keep you from writing spaghetti code that is hard to debug, longer than it has to be, and lacking in conceptually-based structure. For example, I would think instead of your allQuestions array you should make the questions with an object constructor method and put them in the array. If you ever want to do anything with the questions later, this will save you a headache.

function Question(q, choices, answer) {
    this.question = q;
    this.choices = choices;
    this.correctAnswer = answer;
}

allQuestions = [...];    // using the class you defined above

Structure

Your structure is confusing. You declare a function, call that function, then declare some other functions. Why not keep all your function declarations together?

Timing

It can be tempting to just dump invocation of your functions into the <script> body, but it's cleaner to link these with an onload event.  You can read more here. This way you have tighter control and understanding of when/why functions execute. For example you wouldn't want to display the quiz when you hadn't yet set up the necessary HTML elements to do so.

Avoid global variables when possible

Read more in this Stack Overflow about why folks generally recommend not using global variables. They result in a crowded namespace and things happening that you don't expect, where the problem can be hard to track down. I know this is just a small project, but it's best to develop coding habits that are sustainable in larger projects. It's not actually more work, and it saves you a lot of debugging later.

Seems like many of your global variables do not need to be global. For example, aChoices is only used in one function. That's probably true for many of the others.


Finding all non-empty directories and their files on an SFTP server with Paramiko The purpose of the following function is to find all non-empty directories, and the files in those non-empty directories. It recursively checks each directory on an SFTP server to see if it has any files, and if it does, adds it to a default dict using the path as the key. The function uses paramiko.SFTPClient and stat. I am specifically concerned about the performance; it is rather slow.

Prereqsuite information

sftp.listdir_attr returns a list of SFTPAttributes which represent either files, directories, symlinks, etc., and contain a st_mode, which is used to determine if it is a directory or file. This can throw an IOException for example if you don't have permissions to inspect the path.
stat.S_ISDIR will inspect the mode to determine if its a directory

The function in question:

def recursive_ftp(sftp, path='.', files=None):
    if files is None:
        files = defaultdict(list)

    # loop over list of SFTPAttributes (files with modes)
    for attr in sftp.listdir_attr(path):

        if stat.S_ISDIR(attr.st_mode):
            # If the file is a directory, recurse it
            recursive_ftp(sftp, os.path.join(path,attr.filename), files)

        else:
            #  if the file is a file, add it to our dict
            files[path].append(attr.filename)

    return files

Use:

import paramiko
import stat
transport = paramiko.Transport((host, port))
transport.connect(username=username, password=password)
sftp = paramiko.SFTPClient.from_transport(transport)

files = recursive_ftp(sftp)

If we have an SFTP server that looks like this:

/foo
----a.csv
----b.csv
/bar
----c.csv
/baz

The function will return a dictionary like so:

{
    './foo': ['a.csv', 'b.csv'],
    './bar': ['c.csv']
}

There is nothing obviously wrong with your implementation that could explain a slow behaviour. The slowest part here being the use of listdir_attr, you might want to check with other means if its speed matches what your network has to offer.
That being said, there are a few changes you can do to improve a bit on your end:

use an helper function so files will not be both a return value and modified in place;
use paramiko simulation of a working directory to remove the need for os.path;
use list-comprehension to remove the need for defaultdict.

I'm also wondering whether you really want to list everything that is not a directory or only regular files (i.e. no symlinks, no block devives, etc.) You can change the proposed list-comprehension accordingly.
Proposed improvements
def _sftp_helper(sftp, files):
    stats = sftp.listdir_attr('.')
    files[sftp.getcwd()] = [attr.filename for attr in stats if stat.S_ISREG(attr.st_mode)]

    for attr in stats:
        if stat.S_ISDIR(attr.st_mode):  # If the file is a directory, recurse it
            sftp.chdir(attr.filename)
            _sftp_helper(sftp, files)
            sftp.chdir('..')

def filelist_recursive(sftp):
    files = {}
    _sftp_helper(sftp, files)
    return files

You can adapt easily to include back the optional path parameter into filelist_recursive.


Python: OOP design for event pricing updates This was an interview question, I was supposed to refactor the EventPricingUpdate class to reduce the amount of nested ifs.
Here are the basic rules:

EventPricingUpdate() is responsible for updating the price of a list of events after each day.
Each event's price will be reduced by 10 for each day.
The price of each event must be in the range of 50 <= price <= 500.
The field days_from_start represents how many days before the event begins. If days_from_start < 7, then the effect of pricing change will be doubled. For example, a regular event's price will be reduced by 10 * 2 each day in the last 6 days.
There are special event_types that behavors differently.

"music" events' price will always go up by 10 each day, and up by 10 * 2 each day in the last 6 days.
"construction" events' price will never change.
"sports" events' price will drop by double the regular amount(20 instead of 10).

The class should support easily adding new conditions for a new event_type.

As you can see below, I had trouble refactoring the nested ifs in the update() method, what are some good approach to refactor it?

class Event(object):
    def __init__(self, price, days_from_start, event_type):
        self.price = price
        self.days_from_start = days_from_start
        self.event_type = event_type

class EventPricingUpdate(object):
    def __init__(self, events):
        self.events = events

    def update(self):
      for event in self.events:
        if event.event_type == 'construction':
          continue
        elif event.event_type == 'music':
          if event.days_from_start < 7 and event.price <= 480:
            event.price += 20
          elif event.price <= 490:
            event.price += 10
        elif event.event_type == 'sports':
          if event.days_from_start < 7 and event.price >= 90:
            event.price -= 40
          elif event.price >= 70:
            event.price -= 20
        elif event.days_from_start < 7 and event.price >= 70:
            event.price -= 20
          elif event.price >= 60:
            event.price -= 10
        event.days_from_start -= 1
      return self.events

After a small check, it seems the best solution is to add a condition per type of event, where you adjust accordingly.

Since all events have their effect 'doubled' on the last 7 days, you can use a single if for it

Lastly, you use a single if as well to check if you can update the event, regarding final price

Also I added a list comprehension to avoid looping through 'construction' events, whose value never changes

class EventPricingUpdate(object):
    ORIGINAL_REDUCTION = 10

    def __init__(self, events):
        self.events = events

    def events_to_update(self):
        return [event for event in self.events if event.event_type != 'construction']

    def update(self):
        for event in self.events_to_update():
            reduction = self.ORIGINAL_REDUCTION

            if event.event_type == 'music':
                reduction = reduction * -1

            if event.event_type == 'sports':
                reduction = reduction * 2

            if event.days_from_start < 7:
                reduction = reduction * 2

            # if you use python 2 this if needs a slight change
            if 500 >= event.price - reduction >= 50:
                event.price -= reduction

            event.days_from_start -= 1

        return self.events

# Fill some events
events = [
    Event(event_type='construction', days_from_start=10, price=100),
    Event(event_type='music', days_from_start=10, price=100),
    Event(event_type='sports', days_from_start=10, price=100)
]

EventPricingUpdate(events).update()

# Check update
assert events[0].price == 100
assert events[1].price == 110
assert events[2].price == 80

As you can see, is easy to plug new events, just a new condition on the loop

If you want to go further on the refactoring, you will add the reduction on the list comprehension method


What’s your opinion on a Throw() method? Lately I seem to run into a situation very frequently where I want to write something like the following:

var x =
    condition1 ? value1 :
    condition2 ? value2 :
    condition3 ? value3 :
    condition4 ? value4 :
    throw new InvalidOperationException();

Suppose the conditions are not simply constants so I can’t use switch, but they are not complex enough expressions to warrant saying that ?: is too unreadable.

Of course the above construct doesn’t compile because throw is a statement, not an expression. Therefore, I considered writing a method that does exactly that:

/// <summary>Throws the specified exception.</summary>
/// <typeparam name="TResult">The type to return.</typeparam>
/// <param name="exception">The exception to throw.</param>
/// <returns>This method never returns a value. It always throws.</returns>
public static TResult Throw<TResult>(Exception exception)
{
    throw exception;
}

[...]

var x =
    condition1 ? value1 :
    condition2 ? value2 :
    condition3 ? value3 :
    condition4 ? value4 :
    Ut.Throw<MyType>(new InvalidOperationException());

Are there any problems with this approach that I’m not seeing?

I would avoid using such statement. It doesn't seem to be a right place for this construction inside ?: operators and I do not see major benefits in using it. I would consider using following options:

1)

Type variable;
if (condition1) variable = value1;
else if (condition2) variable = value2;
...
else throw new InvalidOperationException();

This has a disadvantage that in each line you have variable = but anyway compiler will let you know if you haven't initialized variable value before using it so it won't bother me much.

2) Also if your value1, value2, etc cannot be null then I would consider using this:

var variable =
    condition1 ? value1 :
    condition2 ? value2 :
    condition3 ? value3 :
    condition4 ? value4 :
    null;
if (variable == null) throw new InvalidOperationException();

P.S.: Of course I cannot be sure without knowing the context but if you have to write such statements frequently then it already seems to be wrong for me, maybe something may be changed on higher level?


Generate Symmetric Ascii Have you got any suggestions to improve this code?
The idea is to generate a one-line string that is visually symmetric.

Revisions at Github

Omit the extra == true in conditions:

if(randomBool())

Use the ternary if operator to compact your code:

left_side.push(randomChoice(randomBool() ? chars : double_chars))

If this seems to dense for you, you can also extract the inner expression like this:

var options = randomBool() ? chars : double_chars;
left_side.push(randomChoice(options))


Creating rows from JSON data I am getting data from JSON and then create rows of it:

        $.each(data.GetwebMethodResult, function (index, item) {
        $("#TableABC").append(FunABC(item.aID, item.b, item.c, item.d, item.e, item.f));
        });

function FunABC(a, b, c, d, e, f) {
    var row =
        "
" +
        "
" +
        "       " +
        "
" + a + "
" +
        "       	" +
        "
" +
        "
";

    return row;
}

Is there any better way of doing it?

Not really. It hard to optimize something with little logic. You could do this instead:

var row =
    "
 \

 \
            \

" + a + "
 \
           	 \

 \

";

But aside from that looks good!


Interacting with database in MVC When using MVC 4 Razor (I am new to web development), I get concerned about my understanding of interactions between the database and the client. Much like all appilications these days, there is a lot of data that is presented to the user and in different fashions. I have looked around, and I can't find a concrete C# example, as they're mostly PHP.

Below is an example of loading data into a jQuery DataTables plugin.

Is this the correct way of formatting data to JSON  and loading it into a jQuery function?
If I wanted to update/delete/insert to the database from the client, is there a way to do that within JavaScript, or should I be doing it within C# somehow? (I have looked around and really, there isn't a ton of documentation)

Note: If it is easier to answer my question without concerning the use of jQuery DataTables, that's completely fine. I'm looking for a broader spectrum of information.

HTML

            Sequence
            ID
            Name
            Ticker #
            Salary
            Bonus
            Allocation
            Begin Date
            End Date

Home Controller - C#

    public string GetAssociateFromDb()
    {
        var dt = new DataTable();
        string connString = ConfigurationManager.ConnectionStrings["DEFCOMP"].ConnectionString;
        using (var connection = new SqlConnection())
        {
            connection.ConnectionString = connString;
            using (var cmd = new SqlCommand("SELECT * FROM FUND", connection))
            {
                connection.Open();
                var myAdapter = new SqlDataAdapter(cmd);
                myAdapter.Fill(dt);
                return JsonConvert.SerializeObject(dt);
            }
        }
      }

In the _Layout

        <link href="../../Content/jQueryDataTables1.10.css" rel="stylesheet" type="text/css" />

I really feel bad about recommending this, but based on what you're doing you should investigate the use of Webforms in ASP.NET.

Webforms are the most straightforward way to provide easy CRUD operations on a database in C# while exposing the operations safely to the user. You also will work entirely with C# and the ASP.NET elements in html, so there's no need to manually use javascript with most basic tasks.

If you want to get more advanced then you can go with MVC5 in ASP.NET but that doesn't have the same level of builtin controls, data table displays, and similar that webforms has baked into itself. MVC5 does give you more power and flexibility in return, but if you need to get something done in a hurry or in a very basic form then you'll probably be fine with Webforms.

The problem with webforms, and why I feel bad recommending it, is that it tends to not encourage good N-Tier design because it both allows you to do things like embed SQL queries in your views (which is blasphemy) and a lot of the examples display exactly that behavior.

It is possible to setup a good distribution of responsibilities into multiple tiers with webforms, which is what I'm doing at my current job, but it doesn't do a great job of encouraging good design. That being said, it does encourage getting things done in a hurry, so that can sometimes outweigh the drawbacks.


Happy Number Program I wrote this program, to determine if any given number is happy, and would like to know of any improvements that can be made to make it better. I have tested it, but whether or not it works perfectly I have no idea, there might be some numbers that get through, although I doubt it.

import java.util.HashSet;
import java.util.Scanner;
import java.util.Set;

public class HappyNumber
{
    public static void main(String[] args)
    {
        System.out.print("Please enter a number: ");
        int number = new Scanner(System.in).nextInt(), value;

        Set<Integer> unique = new HashSet<Integer>();

        while (unique.add(number))
        {
            value = 0;
            for (char c : String.valueOf(number).toCharArray())
                value += Math.pow(Integer.parseInt(String.valueOf(c)), 2);
            number = value;
        }

        System.out.println(number == 1 ? "Happy" : "Not Happy");
    }
}

Apart from what @RubberDuck said, you can also do a bit less work by not
converting from/to String, but iterating over the digits one by one
directly.  I've also moved the value to where it is initialised, that
way its scope is better visible.

After making those changes I arrive at the following snippet.  isHappy
or (isHappyNumber) can be reused and the main function only does
interface stuff.

import java.util.HashSet;
import java.util.Scanner;
import java.util.Set;

public class HappyNumber
{
    public static boolean isHappy(int number)
    {
        Set<Integer> unique = new HashSet<Integer>();

        while (unique.add(number))
        {
            int value = 0;
            while (number > 0)
            {
                value += Math.pow(number % 10, 2);
                number /= 10;
            }
            number = value;
        }

        return number == 1;
    }

    public static void main(String[] args)
    {
        System.out.print("Please enter a number: ");
        int number = new Scanner(System.in).nextInt();
        System.out.println(isHappy(number) ? "Happy" : "Not Happy");
    }
}


Printing a rectangle I have an assignment as follows:

  Write a program which reads from the keyboard two integers n and m,
  and a character c from the keyboard. This program should deﬁne and
  call a function:
  print rectangle(n, m, c) which prints a rectangle of
  size n x m consisting of the character c

My solution is:

n=int(input("Enter the lenght of the rectangle: "))
m=int(input("Enter the width: "))
c="c"
def print_rect(n, m, c):
    for a in range(m):
        print (n*c)
print_rect(n, m, c)
input("Press enter to close")

I am sure there are alternative ways to do this assignment. How would you code this assignment? Do you think my solution is OK?

I think the assignment wants you to read the character from input as well.

When you are printing the input you are composing the string m times. More optimal solution is if you do that 1 time and output it m times:

def print_rect(n, m, c):
    row=n*c
    for a in range(m):
        print (row)

I would even do it with this oneliner:

print((n * c + '\n') * m, end="")