Review for CSS layout code I want to get my first CSS layout reviewed.
First of all, the related HTML code is as follows -

<!DOCTYPE html>

<html lang="en">
    <head>
        <meta charset="utf-8" />
        <title> Preferences </title>

        <script src="options.js"></script>
        <link rel="stylesheet" href="options.css" />
    </head>

    <body>
        <h1> Search in New Tab - Preferences </h1>
        <form id="preference-form">
                <label for="search-engine"> Search Engine </label>
                <select id="search-engine">
                    <option id="google" value="google" selected="selected"> Google </option>
                    <option id="bing" value="bing"> Bing </option>
                    <option id="yahoo" value="yahoo"> Yahoo! </option>
                    <option id="duckduckgo" value="duckduckgo"> DuckDuckGo </option>
                    <option id="wikipedia" value="wikipedia"> Wikipedia </option>
                </select>

                <br>

                <label for="focus-search-tab"> Make the search tab active </label>
                <input type="checkbox" value="focus-search-tab" id="focus-search-tab" />
        </form>
    </body>
</html>

I wrote the following CSS file in order to provide satisfactory presentation to the HTML -

h1
{
    width : 40%;
    background : #eeeeee;
    box-shadow : 0 0 5px 5px #888;
    margin-top : 10%;
    margin-left : 30%;
    margin-right : 30%;
    padding-top : 10px;
    padding-bottom : 10px;
    text-align : center;
    font-family : "Arial", "Century Gothic", "Lucida Sans Unicode";
}

form
{
    width : 30%;
    background : #eeeeee;
    box-shadow: 0 0 5px 5px #888;
    margin-top : 1%;
    margin-left : 35%;
    margin-right : 35%;
    padding-top : 10px;
    padding-bottom : 10px;
}

label
{
    width : 60%;
    display : inline-block;
    text-align : right;
    margin-top : 10px;
    margin-bottom : 10px;
    margin-right : 10px;
    font-family : "Arial";
}

The resulting HTML document looks like this:

Basically, I wanted to avoid the use of float property. I felt that I could accomplish the same by using combinations of inline-block property, padding and margins. So, I used that. Was that the right way?
Other than that, I would like to know if I did things right. Also, any improvements that make my CSS code better are welcome.

I'm no CSS expert, but two things I would change are:

remove unnecessary spaces (indenting and in ' : ').
condense explicit margin-top, margin-left etc (and padding):

h1{
width:40%;
background:#eee;
box-shadow:0 0 5px 5px #888;
margin:10% 30% 0 30%;
padding:10px 0 10px;
text-align:center;
font-family:"Arial", "Century Gothic", "Lucida Sans Unicode";
}

form{
width:30%;
background:#eee;
box-shadow: 0 0 5px 5px #888;
margin:1% 35% 35%;
padding:10px 0 10px;
}

label{
width:60%;
display:inline-block;
text-align:right;
margin:10px 10px 10px;
font-family:"Arial";
}


Simplify Google places autocomplete address for loop I am using the following code to format an address retrieved using the Google Places API. According to Googles docs they recommend looping through the response based on what you list in the componentForm array. Is there a way to optimize this code and simplify it without having to specify the individual if statements?

var componentForm = {
    street_number: 'short_name',
    route: 'long_name',
    locality: 'long_name',
    administrative_area_level_1: 'short_name',
    postal_code: 'short_name'
};

var place = searchaddress.getPlace();

for (var i = 0; i < place.address_components.length; i++) {

    var addressType = place.address_components[i].types[0];

    if (componentForm[addressType]) {

        if (addressType == 'street_number') {
            var streetNumber = place.address_components[i][componentForm[addressType]];
        }

        if (addressType == 'route') {
            var route = place.address_components[i][componentForm[addressType]];
        }

        if (addressType == 'locality') {
            var locality = place.address_components[i][componentForm[addressType]];
        }

        if (addressType == 'administrative_area_level_1') {
            var state = place.address_components[i][componentForm[addressType]];
        }

        if (addressType == 'postal_code') {
            var zipcode = place.address_components[i][componentForm[addressType]];
        }

    }

}

document.getElementById('fullAddress').value = streetNumber + ' ' + route + ', ' + locality + ', ' + state + ' ' + zipcode;

Use if.else if.else if ladder:
Instead of multiple conditions of if.if.if, you should use if.else if.else if ladder. The advantage of latter is that once a condition is met, the next conditions are skipped. But using only if.if.if means all the ifs are walked through irrespective of any if matching the condition. Read more about it here

It is generally a good idea to save repeating variables like place.address_components[i][componentForm[addressType]] in a variable to avoid its repetition and errors

Use object to store address avoiding all if conditions:
Use object to store the address. place.address_components[i][componentForm[addressType]] is common value to be store. The only thing changing is addressType. So, make addressType as key with value place.address_components[i][componentForm[addressType]]. This will skip the need for if conditions.

You will get address object of like:
{
  "street_number": "1600",
  "route": "Amphitheatre Pkwy",
  "locality": "Mountain View",
  "administrative_area_level_1": "CA",
  "postal_code": "94043"
}

And this can be printed where ever you want by using Object.key
If you want to preserve the naming conventions in code such as state instead of administrative_area_level_1 that can also be done by creating another mapping object

Updated Code

var place = {"address_components":[{"long_name":"1600","short_name":"1600","types":["street_number"]},{"long_name":"Amphitheatre Pkwy","short_name":"Amphitheatre Pkwy","types":["route"]},{"long_name":"Mountain View","short_name":"Mountain View","types":["locality","political"]},{"long_name":"Santa Clara County","short_name":"Santa Clara County","types":["administrative_area_level_2","political"]},{"long_name":"California","short_name":"CA","types":["administrative_area_level_1","political"]},{"long_name":"United States","short_name":"US","types":["country","political"]},{"long_name":"94043","short_name":"94043","types":["postal_code"]}],"formatted_address":"1600 Amphitheatre Parkway, Mountain View, CA 94043, USA","geometry":{"location":{"lat":37.4224764,"lng":-122.0842499},"location_type":"ROOFTOP","viewport":{"northeast":{"lat":37.4238253802915,"lng":-122.0829009197085},"southwest":{"lat":37.4211274197085,"lng":-122.0855988802915}}},"place_id":"ChIJ2eUgeAK6j4ARbn5u_wAGqWA","types":["street_address"]}

var componentForm = {
  street_number: 'short_name',
  route: 'long_name',
  locality: 'long_name',
  administrative_area_level_1: 'short_name',
  postal_code: 'short_name'
};

var addressObj = {}

for (var i = 0; i < place.address_components.length; i++) {
  var addressType = place.address_components[i].types[0];
  if (componentForm[addressType]) {
      addressObj[addressType] = place.address_components[i][componentForm[addressType]]
  }
}

console.log(addressObj)

document.getElementById('fullAddress').innerHTML = addressObj.street_number + ' ' + addressObj.route + ', ' + addressObj.locality + ', ' + addressObj.administrative_area_level_1 + ' ' + addressObj.postal_code;

The above json is copied from https://developers.google.com/maps/documentation/geocoding/intro
Hope it helps. Revert for any doubts.


Typesetting A* in LaTeX using algorithm2e - follow-up (See the previous and initial iteration.)

I have this second version of my LaTeX code. I made it less dry by removing the duplicate keyword definitions shared by the two algorithms being typeset. Also, in the previous version, in the argument of main While loop, OPEN was typeset instead of desired OPEN.

See what I have now:

\documentclass[10pt]{article}

\usepackage{amsmath}
\usepackage[ruled,vlined,linesnumbered]{algorithm2e}

\SetArgSty{textnormal} % Make the While argument non-italic

% Define special keywords.
\SetKw{Nil}{nil}
\SetKw{Is}{is}
\SetKw{Not}{not}
\SetKw{Mapped}{mapped}
\SetKw{In}{in}
\SetKw{ChildNode}{child node}
\SetKw{Of}{of}
\SetKw{Continue}{continue}

\begin{document}

  % A*.
  \begin{algorithm}
        $\text{OPEN} = \{ s \}$ \\
        $\text{CLOSED} = \emptyset$ \\
        $\pi = \{ (s \mapsto$ \Nil $)\}$ \\
        $g = \{ (s \mapsto 0) \}$ \\
        \While{$|\text{OPEN}| > 0$}{
          $u = \textsc{ExtractMinimum}(\text{OPEN})$ \\
          \If{$u$ \Is $t$}{
            \KwRet \textsc{TracebackPath}$(u, \pi)$ \\
          }
          $\text{CLOSED} = \text{CLOSED} \cup \{ u \}$ \\
          \ForEach{\ChildNode $v$ \Of $u$}{
            \If{$v \in \textsc{CLOSED}$}{
              \Continue \\
            }
            $c = g(u) + w(u, v)$ \\
            \If{$v$ \Is \Not \Mapped \In $g$}{
              $g(v) = c$ \\
              $\pi(v) = u$ \\
              \textsc{Insert}$(\text{OPEN}, v, c + h(v))$ \\
            }
            \ElseIf{$g(v) > c$}{
              $g(v) = c$ \\
              $\pi(v) = u$ \\
              \textsc{DecreaseKey}$(\text{OPEN}, v, c + h(v))$ \\
            }
          }
        }
        \KwRet $\langle \rangle$
        \caption{\textsc{AStarPathFinder}$(s, t, w, h)$}
  \end{algorithm}

  % Traceback path.
  \begin{algorithm}
        $p = \langle \rangle$ \\
        \While{$u$ \Is \Not \Nil}{
          $p = u \circ p$ \\
          $u = \pi(u)$ \\
        }
        \KwRet $p$
      \caption{\textsc{TracebackPath}$(u, \pi)$}
  \end{algorithm}

\end{document}

The result looks like this:

Any critique is much appreciated.

Nice.  I mean if you're going for perfection I'm going to write
something but otherwise that looks really good.

The
documentation for algorithm2e
mentions that every line should be terminated with \; - and then has
a flag to disable showing that semicolon, \DontPrintSemicolon - take
that how you will.  At least Emacs highlights that less garishly than
the forced newline.
The variables could be marked via \SetKwData.  If you want to keep
the same style, modify it via \SetDataSty{textnormal}.  Similarly
for the functions via \SetKwFunction.
The style for the captions could also just be set to smallcaps
globally via \let\AlCapNameSty\textsc (works and should be correct,
but I'm not a \$\LaTeX\$ expert by any means.
Also there's the procedure environment (instead of algorithm) that
has some more shortcuts.  Note that I added procnumbered and
\SetAlgoProcName to set the names and numbering back to what is the
default algorithm settings.

Looks like this:

\documentclass[10pt]{article}

\usepackage{amsmath}
\usepackage[ruled,vlined,linesnumbered,procnumbered]{algorithm2e}

\SetArgSty{textnormal} % Make the While argument non-italic
\SetDataSty{textnormal}
\SetFuncSty{textsc}
\let\AlCapNameSty\textsc
\DontPrintSemicolon
\SetAlgoProcName{Algorithm}

% Define special keywords.
\SetKw{Nil}{nil}
\SetKw{Is}{is}
\SetKw{Not}{not}
\SetKw{Mapped}{mapped}
\SetKw{In}{in}
\SetKw{ChildNode}{child node}
\SetKw{Of}{of}
\SetKw{Continue}{continue}

\SetKwData{Open}{OPEN}
\SetKwData{Closed}{CLOSED}

\SetKwFunction{ExtractMinimum}{ExtractMinimum}
\SetKwFunction{TracebackPath}{TracebackPath}
\SetKwFunction{Insert}{Insert}
\SetKwFunction{DecreaseKey}{DecreaseKey}

\begin{document}

% A*.
\begin{procedure}
        $\Open = \{ s \}$ \;
        $\Closed = \emptyset$ \;
        $\pi = \{ (s \mapsto$ \Nil $)\}$ \;
        $g = \{ (s \mapsto 0) \}$ \;
        \While{$|\Open| > 0$}{
        $u = \ExtractMinimum(\Open)$ \;
        \If{$u$ \Is $t$}{
            \KwRet \TracebackPath{$u$, $\pi$} \;
        }
        $\Closed = \Closed \cup \{ u \}$ \;
        \ForEach{\ChildNode $v$ \Of $u$}{
            \If{$v \in \Closed$}{
            \Continue \;
            }
            $c = g(u) + w(u, v)$ \;
            \If{$v$ \Is \Not \Mapped \In $g$}{
            $g(v) = c$ \;
            $\pi(v) = u$ \;
            \Insert{\Open, $v$, $c + h(v)$} \;
            }
            \ElseIf{$g(v) > c$}{
            $g(v) = c$ \;
            $\pi(v) = u$ \;
            \DecreaseKey{\Open, $v$, $c + h(v)$} \;
            }
        }
        }
        \KwRet $\langle \rangle$
        \caption{AStarPathFinder($s$, $t$, $w$, $h$)}
\end{procedure}

% Traceback path.
\begin{procedure}
        $p = \langle \rangle$ \;
        \While{$u$ \Is \Not \Nil}{
        $p = u \circ p$ \;
        $u = \pi(u)$ \;
        }
        \KwRet $p$
    \caption{TracebackPath($u$, $\pi$)}
\end{procedure}

\end{document}


Implementation of Newton's method of finding root of a function The following is an implementation of Newton's method of finding root of a function.

using System;
using System.Drawing;
using ZedGraph;

//Newton's Method
//https://www.youtube.com/watch?v=2GrfaB88w4M&t=350s
//https://stackoverflow.com/questions/12915317/how-to-find-derivative-of-a-function-using-c
//https://www.geeksforgeeks.org/program-for-newton-raphson-method/
namespace _1_22_Newton_Method_of_Root_Finding
{
    public delegate double MyFunction(double x);
    public class RootFinder
    {
        static double EPSILON = 0.01;
        static double h = 1.0e-6;

        public static double Derivative(MyFunction func, double x0)
        {
            double x1 = x0 - h;
            double x2 = x0 + h;
            double y1 = func(x1);
            double y2 = func(x2);
            double der = (y2 - y1) / (x2 - x1);//<==== i am not sure about this.

            return der;
        }

        private static double NextNewton(MyFunction func, double guess)
        {
            return guess - func(guess) / Derivative(func, guess);
        }

        public static double Newton(MyFunction func, double init)
        {
            double x = init;
            double y = NextNewton(func, x);

            //for(int i=0 ; i < 10 ; i++)
            while(Math.Abs(y) >= EPSILON)
            //while(true)
            {
                y = NextNewton(func, x);

                if (x == y)
                {
                    break;
                }

                Console.WriteLine("Root = {0}", y);
                x = y;
            }

            return y;
        }
    }

Kindly, review the source code.

Does this source code have any pitfall?

Math.Abs(y) >= EPSILON

I think this condition is only satisfied for roots around zero on the x-axis. Maybe you confuse yourself by naming the next x as y? A better name would be x1.

The actual continue condition should be Math.Abs(x - x1) >= EPSILON and you then can skip the test if (x == y) break;

Your rather large value of EPSILON may result in imprecise roots now and then.

You can clean up the main algorithm to something like:

public static double Newton(MyFunction func, double init)
{
  double x;
  double x1 = init;

  do
  {
    x = x1;
    x1 = NextNewton(func, x);

    Console.WriteLine("Root = {0}", x1);
  } while (Math.Abs(x - x1) >= EPSILON);

  return x1;
}


Finding the sum of the digits appearing in the leaves via in-order traversal The following text is an in-order traversal of a binary tree with 1023
  nodes. What is the sum of the digits appearing in the leaves?

How can I improve this Clojure code? It looks strange to me to have two recurs in the if.

(ns fun2)

(defn parse [in sum]
 (println "sum" sum)
 (if (> (.length in) 1)
   (let [next-char (subs in 0 1)]
     (if (.matches next-char "[0-9]")
       (recur (subs in 2) (+ sum (Integer/parseInt next-char)))
       (recur (subs in 2) sum)))))

(parse (slurp "/Users/clojure/challange2.txt") 0)

;; ans 331

Here's a version of the function that I find nicer:

(defn parse [s]
  (apply + (->> s
             (partition-all 2)                 ; iterate pairs of chars in s
             (map (comp str first))            ; leaf node is the first in the pair
             (filter (partial re-find #"\d"))  ; keep only digits
             (map #(Integer/parseInt %)))))    ; turn 'em into ints

I imagine it can be further reduced to be more idiomatic Clojure, but I find this pretty readable.

Important assumption: this algorithm only works if the input is an inorder traversal of a complete binary tree.  If the original tree was not complete, a single inorder traversal is not sufficient to reconstruct it and find the leaves.


Compare array with Json Is there any better way to do this?

import json

product_data = [x for x in open('product_strings.txt', 'r', encoding='utf-8').read().split(',')]
json_data = json.load(open('tedata.json', 'r', encoding='utf-8'))

for product_no in json_data:
    for product in product_data:
        if product.lower() in json_data[product_no]['name'].lower():
            print (product.lower(), json_data[product_no]['name'])
        elif product.lower() in json_data[product_no]['des'].lower():
            print (product.lower(), json_data[product_no]['name'])

tedata.json contains (6mb with ~1000 objects)

{
    "12345": {
        "owner": "BMW int",
        "doman": "20/01/2016",
        "des": "a beautiful BMW with improvements with respect to older version of bmw.",
        "dopur": "25/07/2016",
        "purchaser": "Mitsubishi, Japan",
        "name": "BMW-8"
    },
    "12346": {
        "owner": "audi",
        "doman": "20/01/2016",
        "des": "a beautiful skoda with improvements with respect to older version of skoda.",
        "dopur": "25/07/2016",
        "purchaser": "Mokoto, Japan",
        "name": "skoda-1"
    }
}

product_strings file contains (small with ~100 such string)

audi,bmw,jaguar

code for testing:

import json

tedata = """{
    "12345": {
        "owner": "BMW int",
        "doman": "20/01/2016",
        "des": "a beautiful BMW with improvements with respect to older version of bmw.",
        "dopur": "25/07/2016",
        "purchaser": "Mitsubishi, Japan",
        "name": "BMW-8"
    },
    "12346": {
        "owner": "audi",
        "doman": "20/01/2016",
        "des": "a beautiful skoda with improvements with respect to older version of skoda.",
        "dopur": "25/07/2016",
        "purchaser": "Mokoto, Japan",
        "name": "skoda-1"
    }
}"""

product_strings = "audi,bmw,jaguar"

product_data = [x for x in product_strings.split(',')]
json_data = json.loads(tedata)

for product_no in json_data:
    for product in product_data:
        if product.lower() in json_data[product_no]['name'].lower():
            print (product.lower(), json_data[product_no]['name'])
        elif product.lower() in json_data[product_no]['des'].lower():
            print (product.lower(), json_data[product_no]['name'])

##prints ===>bmw BMW-8

Your identifiers could be altered to make things clearer. What you're calling product_data is just a list of names, so perhaps product_names would be better. Then it follows that product should become product_name, as for product_name in product_names: makes perfect sense. And you should only call things x where that's the convention of the domain (e.g. mathematics).

One obvious code simplification is that you can iterate over a dictionary's keys and values simultaneously:

for product_no, product in json_data.items():
    for product_name in product_names:
        ...

This way your comparisons lower down are neater:

if product_name.lower() in product['name'].lower():

Rather than product_name.lower() every time you compare to it, lowercase everything once:

product_names = [name for name in product_strings.lower().split(',')]
                                               # ^ e.g. here

In this case you don't need it at all, as product_strings is already lowercase!

You can do multiple comparisons in one (logical) line, too, reducing duplication:

if (product_name in product['name'].lower()
        or product_name in product['des'].lower()):
    print (product, product_spec['name'])


Parse a csv file and create a dictionary of partial results I have a bunch of .csv files which I have to read and look for data. The .csv file is of the format:

A row of data I will ignore
State,County,City
WA,king,seattle
WA,pierce,tacoma

In every csv file, the order of columns is not consistent. For example in csv1 the order can be State,County,City, in csv2 it can be City,County,State. What I am interested is the State and County. Given a county I want to find out what State it is in. I am ignoring the fact that same counties can exist in multiple States. The way I am approaching this:

with open(‘file.csv’) as f:
    data = f.read()

# convert the data to iterable, skip the first line
reader = csv.DictReader(data.splitlines(1)[1:])
lines = list(reader)
counties = {k: v for (k,v in ((line[‘county’], line[‘State’]) for line in lines)}

Is there a better approach to this?

You're on the right track, using a with block to open the file and csv.DictReader() to parse it.

Your list handling is a bit clumsy, though.  To skip a line, use next(f).  Avoid making a list of the entire file's data, if you can process the file line by line.  The dict comprehension has an unnecessary complication as well.

with open('file.csv') as f:
    _ = next(f)
    reader = csv.DictReader(f)
    counties = { line['County']: line['State'] for line in reader }

Your sample file had County as the header, whereas your code looked for line[‘county’].  I assume that the curly quotes are an artifact of copy-pasting, but you should pay attention to the capitalization.


Vigenere cipher with random salt EDIT:
I've created a repo and uploaded it to Maven (still in dev though).
Any suggestion or pull request is more than appreciated. :)

https://github.com/enrichman/vigenere

I had the need to write a simple encryption algorithm (no security needs, it just needs to look a bit random) and I thought to add a bit of random things to a Vigenere cipher.

The idea was simply to create a random key of a fixed length, use it as key in the Vigenere, then append it to the encrypted text. Then encrypt again the whole text with the secret key.

In this way the same text, encrypted with the same key looks more random.

Any suggestion about the algorithm or implementation?

A simple run:

1)
Plaintext: HELLO
Encrypted: FF22LDAQ
Decrypted: HELLO

2)
Plaintext: HELLO
Encrypted: 7MYUS92X
Decrypted: HELLO

Maybe the last thing to add is a way to add the initial size to the encrypted text, but I don't want it to be fixed in some way.

public class Crypto {

    private static Random rand = new Random();
    private static char[] alphabet = new char[] {
            '0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
            'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M',
            'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z'
    };
    private static String KEY = "DSNJKDNFJKSNDJKNOCNSJCKNK";

    public static void main(String[] args) {

        int size = 3;
        String plaintext = "HELLO";

        System.out.println("Plaintext: " + plaintext);

        String enc = encrypt(plaintext, size);
        System.out.println("Encrypted: "+enc);

        String dec = decrypt(enc, size);
        System.out.println("Decrypted: "+dec);
    }

    public static String encrypt(String plaintext, int size) {

        StringBuilder sb = new StringBuilder();
        for(int i=0; i<size; i++) {
            sb.append(alphabet[rand.nextInt(alphabet.length)]);
        }
        String initVector = sb.toString();

        String firstPass = shift(plaintext, initVector, false);

        return shift((initVector+firstPass), KEY, false);
    }

    public static String decrypt(String encrypted, int size) {
        String firstPass = shift(encrypted, KEY, true);
        return shift(firstPass.substring(size), firstPass.substring(0, size), true);
    }

    private static String shift(String clear, String shifter, boolean backward) {

        while (shifter.length() < clear.length())
            shifter += shifter;
        shifter = shifter.substring(0, clear.length());

        StringBuilder sb = new StringBuilder();
        for(int i=0; i<clear.length(); i++) {
            sb.append(shift(clear.charAt(i), shifter.charAt(i), backward));
        }
        return sb.toString();
    }

    private static char shift(char c, char s, boolean backward) {
        int indexC = indexOf(c);
        int indexS = indexOf(s);

        int indexShifted;
        if(backward) {
            indexShifted = ((indexC - indexS) + alphabet.length) % alphabet.length;
        } else {
            indexShifted = (indexC + indexS) % alphabet.length;
        }

        return alphabet[indexShifted];
    }

    private static int indexOf(char c) {
        int index = -1;
        for(int i=0; i<alphabet.length; i++) {
            if(c == alphabet[i]) {
                index = i;
                break;
            }
        }
        return index;
    }
}

Lacking a good reason to do otherwise, I think I'd use a Vernam cipher instead of a Vigenere. The two offer about the same level of security (none, at least against anybody who knows anything about cryptanalysis), but a Vernam cipher is generally easier to implement, and (probably more importantly, at least for your purposes) with a typical key, a single pass produces output that looks much more random (in particular, it frequently produces a lot of relatively unreadable symbols and such. For example:

#include <iostream>
#include <sstream>

int main() {
    std::istringstream input("This is some input for encryption.");
    std::string key("EncryptionKey");

    unsigned pos = 0;
    char ch;
    while (input.get(ch)) {
        std::cout << char(ch ^ key[pos]);
        ++pos;
        if (pos > key.length())
            pos = 0;
    }
}

Obviously, the code is quite short and simple. The result it produces in this case is:

  ◄♠
  ☺Y↓I∟☺& Yi+▲▬♠Y▬←←O♂%♠♂y5→
  ↔↨^

As already noted, there's little difference in terms of real security--but in terms of producing output that looks strange and off-putting, this seems to me to win by a wide margin.

If you were going to mix two different ciphers, I don't think I'd use the same algorithm, just with two different keys. Rather, I'd use two entirely different types of ciphers. Since Vigenere is a substitution cipher, the obvious choice would be to use a transposition cipher for the second one.


Running shell script as root via external binary I'm not an experienced Linux user and I wanted an easy way to run shell scripts as root from a PHP script, I came up with this:

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

int main(int argc, char *argv[])
{
    if (access(argv[1], F_OK) != -1)
    {
        struct stat filestat;

        if (stat(argv[1], &filestat) == 0)
        {
            if ((filestat.st_uid == 0) && (filestat.st_gid == 0) && (filestat.st_mode & S_IXUSR) && (!(filestat.st_mode & S_IWOTH)))
            {
                char* match = strrchr(argv[1], '.');

                if ((match != NULL) && (strcasecmp(match, ".sh") == 0))
                {
                    if (setuid(0) != -1)
                    {
                        execl("/usr/bin/sudo", "/usr/bin/sudo", argv[1], (char*) NULL);
                        return 0;
                    }
                }
            }
        }
    }

    return 1;
}

Any potential security issues with this? If so how can I improve it?

Example Usage:

Lets say I have a script located at:

/some/path/script.sh

With the following in it:

#!/bin/bash
echo $USER

Now lets say I compile the above C code to a binary and place it at:

/some/path/run-as

and do:

chown root:root /some/path/run-as
chmod 6755 /some/path/run-as

Now I run this PHP script owned by www-data (via browser / local apache web server):

<?php
    echo exec('/some/path/run-as /some/path/script.sh');
?>

I expect the script to output 'root' when ran.

This is an ambitious thing to try, with many pitfalls - especially for environments which are Web-accessible.  I urge you to learn how to configure and use sudo instead - that's not foolproof, but it is a good deal safer than rolling your own.

That said, I'll make some observations:

We're using argv[1] without checking whether any arguments were supplied (we should test argc >= 2 before using argv[1] - possibly check argc == 2, since we ignore any extra arguments).
There's a time-of-check/time-of-use (TOCTOU) race, because we examine the supplied file and later assume the properties haven't changed (we we get to running it).
Using system causes the supplied argument to be parsed as a shell command, which will do word splitting, expand variables and globs, and more.
Using a shell script from a privileged program is vulnerable to the script being changed between the interpreter being started and it opening the script to read commands (e.g. by switching symlinks).

It's quite easy to exploit this script to execute anything I want on your system, if there's a directory I can write to:

ln -s /bin/true 'ls -l ~root; id; cd; echo rm -rf; true .sh'

/some/path/run-as ls*.sh

We can avoid using system() (and accept extra arguments if we choose), by using execv() instead:

            if (match && !strcasecmp(match, ".sh"))
            {
                execv(argv[1], argv+1);
                /* if we got here, exec failed */
                /* so fall through to return 1 */
            }

This does still leave us vulnerable to a user creating a symbolic link to a root-owned file and rewriting it to point to the user's file between the access check and the exec(), though.

In short: please use sudo if you really have to run some things as root.  Configured properly, it can give tight control on the commands you can execute with elevated privileges.  It's not completely attack-proof, but it is much better than writing your own (it has had decades of scrutiny to improve it).


Eliminating repetitions of subsequent items in a list I would like to convert a list A
A = {1, 12, 3, 3, 3, 8, 5, 5 }

into list B
B = {1, 12, 3, 8, 5 }

As you see, items 3 and 5 are repeated in A, and those repetitions are eliminated in B.
The code I've created to do that is the following:
public IEnumerable<int> FilterRepeatingElements(IEnumerable<int> source)
{
    return source
        .Select(x => (IEnumerable<int>)new[] { x })
        .Aggregate((a, b) => a.Last() != b.First() ? a.Concat(b) : a);
}

It works, but I wonder if there's a better more expressive way to do it using "LINQ", either with the classic .NET methods or the ones in a library like morelinq.
Thanks!
EDIT
I have found out that my solution has problem with uses case like this:
FilterRepeatingElements(MoreEnumerable.Random()).Take(10)

MoreEnumerable.Random() generates an infinite sequence.
Executing code like this, hangs the execution.

While aepot answer is a good answer and got my upvote it does only work for type int.  Which I know the question for an example uses int in their code but if wanting it to work for all data types will need to change the code a bit.
public static class EnumerableExtensions
{
    public static IEnumerable<TSource> FilterRepeatingElements<TSource>(this IEnumerable<TSource> source, IEqualityComparer<TSource> comparer)
    {
        using (var enumerator = source.GetEnumerator())
        {
            if (enumerator.MoveNext())
            {
                var item = enumerator.Current;
                yield return item;

                while (enumerator.MoveNext())
                {
                    if (!comparer.Equals(item, enumerator.Current))
                    {
                        item = enumerator.Current;
                        yield return item;
                    }
                }
            }
        }
    }

    public static IEnumerable<TSource> FilterRepeatingElements<TSource>(this IEnumerable<TSource> source)
    {
        return FilterRepeatingElements(source, EqualityComparer<TSource>.Default);
    }
}

This would work for any type and takes the same parameters as linq Distinct does since we are doing a "kind of" distinct.
var data = new int[] { 1, 12, 3, 3, 3, 8, 5, 5 };
var postData = data.FilterRepeatingElements().ToArray();

var items = new string[] { "A", "A", "B", "C", "C", "D", "D" };
var postItems = items.FilterRepeatingElements().ToArray();


Python vs. Java Runtime on Euler 22 - Name Scores I want to know why Java is accepted over Python when it comes to runtime. On performing the following Euler problem, Python takes up less lines and runs faster (Python ~0.05s, Java ~0.3s on my machine).

Could I optimize this Java code in any way? The problem is here (http://projecteuler.net/problem=22)

Python:

def main():
    names = open("names.txt", "r").read().replace("\"", "").split(",")
    names.sort()
    print sum((i + 1) * sum(ord(c) - ord('A') + 1 for c in n) for i, n in enumerate(names))

if __name__ == "__main__":
    main()

Java:

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

public class euler22
{
    public static String readFileAsString(String path) throws IOException
    {
        StringBuilder builder = new StringBuilder();
        BufferedReader reader = new BufferedReader(
                new FileReader(path));
        String buffer = null;

        while((buffer = reader.readLine()) != null)
        {
            builder.append(buffer);
        }

        reader.close();

        return builder.toString();
    }

    public static void main(String[] args) throws IOException
    {
        String[] names = fileAsString("names.txt").replace("\"", "").split(",");
        int total = 0;

        Arrays.sort(names);

        for(int i = 0; i < names.length; ++i)
        {
            int sum = 0;

            for(char c : names[i].toCharArray())
            {
                sum += c - 'A' + 1;
            }

            total += (i + 1) * sum;
        }

        System.out.println(total);
    }
}

Broadly speaking try creating less objects. ;)

You can

read the entire file as a series of lines or as a string with FileUtils.
iterate through each character rather than building an array which you iterate.
as the program is so short, try using -client which has shorter start time.

To maximise the performance

long start = System.nanoTime();
long sum = 0;
int runs = 10000;
for (int r = 0; r < runs; r++) {
    FileChannel channel = new FileInputStream("names.txt").getChannel();
    ByteBuffer bb = channel.map(FileChannel.MapMode.READ_ONLY, 0, channel.size());
    TLongArrayList values = new TLongArrayList();

    long wordId = 0;
    int shift = 63;
    while (true) {
        int b = bb.remaining() < 1 ? ',' : bb.get();
        if (b == ',') {
            values.add(wordId);
            wordId = 0;
            shift = 63;
            if (bb.remaining() < 1) break;

        } else if (b >= 'A' && b <= 'Z') {
            shift -= 5;
            long n = b - 'A' + 1;
            wordId = (wordId | (n << shift)) + n;

        } else if (b != '"') {
            throw new AssertionError("Unexpected ch '" + (char) b + "'");
        }
    }

    values.sort();

    sum = 0;
    for (int i = 0; i < values.size(); i++) {
        long wordSum = values.get(i) & ((1 << 8) - 1);
        sum += (i + 1) * wordSum;
    }
}
long time = System.nanoTime() - start;
System.out.printf("%d took %.3f ms%n", sum, time / 1e6);

prints

XXXXXXXXX took 27.817 ms

Its pretty obtuse, but works around the fact its not warmed up.

You can tell this is the case because if you repeat the code in a loop 10000 times, the time taken is only 8318 ms or 0.83 ms per run.


Solving quadratic equation in Clojure I wrote a function in Clojure to solve quadratic equations using the quadratic formula

The function

(defn solvequadeq [a b c]
 (let [D (- (* b b) (* 4 a c))]
      (if (< D 0) #{}
        (let [sqrtD (Math/sqrt D)
              x1 (/ (+ (- b) sqrtD) (* 2 a))
              x2 (/ (- (- b) sqrtD) (* 2 a))]
              (set [x1 x2])))))

Unit tests

(defn assertFuzzyEquals [act exp]    ;; From codewars.com
  (do (println "asserting equality over " act exp)
  (let [inrange (<= (Math/abs (- act exp)) 1e-2)]
    (if (= inrange false)
      (println "abs(actual - expected) must be <= 1e-2. Expected was " exp " but got: " act))
    (is (= inrange true))))
)

(defn assertSetsFuzzyEqual [act exp]
  (and (is (= (count act) (count exp)))
  (doall (map assertFuzzyEquals (sort (vec act)) (sort (vec exp))))))   ; (1)

(deftest a-test5
  (testing "Solve quadratic equations"
          (assertSetsFuzzyEqual (solvequadeq 1 0 0) #{0})
          (assertSetsFuzzyEqual (solvequadeq 1 -6 9) #{3})
          (assertSetsFuzzyEqual (solvequadeq 1 -4 3) #{1 3})
          (assertSetsFuzzyEqual (solvequadeq 1 -4 14) #{})
))

N.B.: assertFuzzyEquals is borrowed from a task of codewars.com, all the rest is my code.

While any remark, and suggestion is welcome, I would be interested most particularly in the following questions:

Is using a set a good solution for avoiding having the same return value twice, if the two solutions are equal? (I am worrying that they might not be always exactly equal, due to imprecision in calculation.)
If not, than what would be a better approach, to elegantly handle the case of one vs two solutions?
Independently whether sets should be used for this particular problem or not, is there a better way to compare two sets for equality, with a custom function between the elements? (C.f., line (1))

EDIT: Clarification of this last point, as per the answer of Josh.
The question is: what is the best practice, in Clojure, for checking the equality of two sets of floating point numbers?

Let's say, that we have a set with values #{1.0, 2.0}, and want to compare it against the set #{1, 2}.

Attempted solutions:

(= #{1.0, 2.0} #{1, 2})

; result false, since 1.0 != 1

(reduce #(and %1 %2)  (map #(> 0.01 (Math/abs (- %1 %2))) #{1.0,2.0}  #{2,1}))

; returns false, since ordering of elements in the set is not guaranteed

(reduce #(and %1 %2)  (map #(> 0.01 (Math/abs (- %1 %2))) (sort (vec #{1.0,2.0}))  (sort (vec #{2,1}))))

; returns true, because fixes the ordering, but is very complicated...

So, I am asking if there is a better way, to compare two sets, providing a custom function for equality checking (in this particular case, this custom function would be #(> 0.01 (Math/abs (- %1 %2)))).

I am thinking of something like this (pseudo-code only):
(sets-equal #(> 0.01 (Math/abs (- %1 %2))) #{1.0,2.0}  #{2,1}

Is using a set a good solution for avoiding having the same return value twice, if the two solutions are equal? (I am worrying that they might not be always exactly equal, due to imprecision in calculation.)

Instead of a set, IMO a vector would be a better structure.  It is essentially a "list of solutions."  Instead of your (set [x1 x2... simply wrap it in a vector:

(vec (hash-set x1 x2))

There's nothing wrong with using a set, but vectors are more common. Sets tend to be used more for utility in computation, rather than as a way to convey expression results.  There are always exceptions, and you could very well make one here if you chose to do so.

  Independently whether sets should be used for this particular problem or not, is there a better way to compare two sets for equality, with a custom function between the elements? (C.f., line (1))

I'm not the expert on floating-point comparisons, but I wonder if the following would work, or if it's too naive:

(def s1 #{1.0 2.0})
(def s2 #{1 2})
(= (set (map double s1)) (set (map double s2)))  ; true


compare two logic commented one to my logic java I'm trying to review this block of code to make sure logic is fine. The first version that I commented will create duplicated order 'my college developer' but I fixed it but "really not sure" I want to make sure my fix it exactly same as commented code

the old logic "this code include duplicated"

public void setOrderedProduct(String prodName, int eoq) {
    String productCd = getProdCd(prodName);
    // Here only one time one product
    boolean prodPresent = false;
    int indexOfProd = 0;
    int sizeOfItems = items.size();
    for (int i = 0; i < sizeOfItems; i++) {
        PurchaseOrderDTO itemVal = items.get(i);
        if (itemVal.getProdcd().equalsIgnoreCase(productCd)) {
            prodPresent = true;
            indexOfProd = i;
            break;
        }
    }
    itemData.setProdname(prodName);
    itemData.setQty(eoq);
    itemData.setProdcd(productCd);
    double costPrice = ProductView.getProductCost(vendorno, productCd);
    itemData.setPrice(costPrice);
    double extPrice = ProductView.getProductMSRP(vendorno, productCd);
    itemData.setExt(extPrice);

    if (!prodPresent) {
        if (eoq != 0) {
            items.add(itemData);
            // Now add the details to the database
            ProductView.addPurchaseDetails(itemData, vendorno);
        }
    } else {
        if (eoq > 0) {
            items.set(indexOfProd, itemData);
            ProductView.updatePurchaseDetails(itemData, vendorno);
        } else {
            items.remove(indexOfProd);
            ProductView.removePurchaseDetails(itemData, vendorno);
        }
    }
    if (items.size() > 0)
        calculateTotal();
}

my fixed logic

public void setOrderedProduct(String prodName, int eoq) {
    boolean dataPresent = false;
    if (orderedProdNameList.contains((Object) prodName)) {
        dataPresent = true;
    }
    if (dataPresent) {
        int indexOfProdName = orderedProdNameList.indexOf((Object) prodName);
        String productCd = getProdCd(prodName);
        // Here only one time one product
        PurchaseOrderDTO itemData = new PurchaseOrderDTO();

        boolean prodPresent = false;
        itemData.setProdname(prodName);
        itemData.setQty(eoq);
        itemData.setProdcd(productCd);
        double costPrice = ProductView.getProductCost(vendorno, productCd);
        itemData.setPrice(costPrice);
        double extPrice = ProductView.getProductMSRP(vendorno, productCd);
        itemData.setExt(extPrice);
        // Now add the details to the database
        ProductView.addPurchaseDetails(itemData, vendorno);
        if (eoq > 0) {
            items.set(indexOfProdName, itemData);
            ProductView.updatePurchaseDetails(itemData, vendorno);
        } else if (eoq == 0) {
            for (int i = 0; i < items.size(); i++) {
                if (items.get(i).getProdname().equalsIgnoreCase(prodName)) {
                    items.remove(i);
                    ProductView.removePurchaseDetails(itemData, vendorno);
                }
            }
        }

    } else {
        orderedProdNameList.add(prodName);
        String productCd = getProdCd(prodName);
        // Here only one time one product
        PurchaseOrderDTO itemData = new PurchaseOrderDTO();

        boolean prodPresent = false;
        itemData.setProdname(prodName);
        itemData.setQty(eoq);
        itemData.setProdcd(productCd);
        double costPrice = ProductView.getProductCost(vendorno, productCd);
        itemData.setPrice(costPrice);
        double extPrice = ProductView.getProductMSRP(vendorno, productCd);
        itemData.setExt(extPrice);
        items.add(items.size(), itemData);
        ProductView.addPurchaseDetails(itemData, vendorno);
    }
    if (items.size() > 0) {
        calculateTotal();
    }
}

Not sure if your fixed logic actually works. But I see couple of issues with the code.

both if and else block is setting a boolean. But I don't find any use of it in your code. boolean prodPresent = false

How do you get access to items ? is it instance variable ?
The if and else block has lot of common code which can be written outside of the condition.

Here I am not concerned with your logic but tried to organize the second version of the code which you posted. My intention is to avoid repetitive code in both the if-else block

public void setOrderedProduct(String prodName, int eoq) {
PurchaseOrderDTO itemData = new PurchaseOrderDTO();
String productCd = getProdCd(prodName);

itemData.setProdname(prodName);
itemData.setQty(eoq);
itemData.setProdcd(productCd);

double costPrice = ProductView.getProductCost(vendorno, productCd);
itemData.setPrice(costPrice);

double extPrice = ProductView.getProductMSRP(vendorno, productCd);
itemData.setExt(extPrice);

if (orderedProdNameList.contains((Object) prodName)) {
  int indexOfProdName = orderedProdNameList.indexOf((Object) prodName);

  ProductView.addPurchaseDetails(itemData, vendorno);
  if (eoq > 0) {
    items.set(indexOfProdName, itemData);
    ProductView.updatePurchaseDetails(itemData, vendorno);
  } else if (eoq == 0) {
    for (int i = 0; i < items.size(); i++) {
      if (items.get(i).getProdname().equalsIgnoreCase(prodName)) {
        items.remove(i);
        ProductView.removePurchaseDetails(itemData, vendorno);
      }
    }
  }

} else {
  orderedProdNameList.add(prodName);
  items.add(items.size(), itemData);
  ProductView.addPurchaseDetails(itemData, vendorno);
}
 if (items.size() > 0) { calculateTotal(); }
}


Computing average grades for students in a class I wrote up a simple averages calculator for student grades, based on this daily programming challenge.

First, the script asks for the file name for the text file containing the student names and grades. It is formatted like so:

JON 19 14 15 15 16
JEREMY 15 11 10 15 16
JESSE 19 17 20 19 18

The user is then asked how many assignments there are.

While my solution works, it seems awfully verbose. Can you give me tips on "pythonic" ways to simplify the script?

grade_avg_generator.py

"""Simple student grade manager"""

from fileinput import input

def import_data(grades_file, assignments):
    """Split the data into key value pairs."""
    student_grades = dict()
    for line in input(grades_file):
        this_line = line.split()
        student_grades[this_line[0]] = [int(item) for item in this_line[1:assignments + 1]]
    return student_grades

def student_means(student_grades):
    """Generate the average grade for each student and append it."""
    for k, v in student_grades.iteritems():
        grades = v
        grades_mean = float(sum(item for item in grades)) / len(grades)
        v.append(grades_mean)
    student_grades_mean = student_grades
    return student_grades_mean

def class_mean(student_grades_mean):
    """Generate the class average."""
    class_grades = list()
    for k, v in student_grades_mean.iteritems():
        this_avg = v[-1]
        class_grades.append(this_avg)
    class_mean = float(sum(item for item in class_grades)) / len(class_grades)
    return class_mean

def main():
    grades_file = raw_input('File name: ')
    assignments = int(raw_input('How many assignments are there? '))

    student_data = import_data(grades_file, assignments)
    student_data_avg = student_means(student_data)
    class_avg = class_mean(student_data_avg)
    print 'class average: %0.2f' % class_avg
    for k, v in student_data_avg.iteritems():
        grades = v[:-1]
        grades = ' '.join(str(i) for i in grades)
        avg = str(v[-1])
        print 'name: %s | grades: %s | average: %s' % (k, grades, avg)

if __name__ == '__main__':
    main()

For conciseness in Python, the key is to think in terms of applying functions using map and list comprehensions, rather than thinking procedurally.  In this problem, it is very important to define a mean() function.  Once you do that, the calculations themselves are one-liners.

The main objection I had to your code was that you mangled the data structure by appending each student's average at the end of his grades.  Don't do that — student_means() should just return a fresh dict instead.

I've taken out the non-essential prompt for the number of assignments; you can easily put it back.

""" Simple student grade manager"""

from fileinput import input

def mean(list):
    return sum(map(float, list)) / len(list) \
      if len(list) > 0 else float('nan')

def import_student_grades(grades_file):
    """
    Given a filename, reads data into a dict whose keys are student names
    and whose values are grades (as strings).
    """
    line_words = [line.split() for line in input(grades_file)]
    return dict((words[0], words[1:]) for words in line_words)

def main():
    grades_file = raw_input('File name: ')
    student_grades = import_student_grades(grades_file)
    student_means = dict((k, mean(v)) for k, v in student_grades.iteritems())

    print 'class average: %0.2f' % mean(student_means.values())
    for name, grades in student_grades.iteritems():
        print 'name: %(name)s | grades: %(grades)s | average: %(avg)0.2f' % {
            'name': name,
            'grades': ' '.join(grades),
            'avg': student_means[name],
        }

if __name__ == '__main__':
    main()


Using ADO to run sql script (with GO syntax) in inno setup I am using this code to run my sql scripts in inno setup. However ADO gives an error if the script contains GO syntax within it. I added my own code to overcome this problem.

My code below splits the script at each GO statement and run each section separately. I think this could be done in better ways.

[code]
const
 adCmdText = $00000001;
 adExecuteNoRecords = $00000080;

procedure RunScript();
var
 tString: TStringList;
 ADOCommand: Variant;
 ADORecordset: Variant;
 ADOConnection: Variant;
 i, len: Integer;
 strToQuery, str: String;
begin
 try
 ADOConnection := CreateOleObject('ADODB.Connection');
 ADOConnection.ConnectionString :=
  'Provider=SQLOLEDB;' +               // provider
  'Data Source=(local)\SQL2000;' +   // server name
  'Initial Catalog=databasename;' +       // default database
  'User Id=userId;' +                // user name
  'Password=password;';                   // password
 ADOConnection.Open;
 try
  ADOCommand := CreateOleObject('ADODB.Command');
  ADOCommand.ActiveConnection := ADOConnection;
   tString:= TStringList.Create;
   tString.LoadFromFile('C:\sqlscript.sql');
   len:= tString.Count;
   strToQuery:='';
   for i:=0 to len-1 do begin
    str := trim(tString[i]);
    if POS('go', lowercase(str)) = 1  then begin
     //If GO is found run the script added in to strToQuery since last GO
     ADOCommand.CommandText := strToQuery;
     ADOCommand.Execute(NULL, NULL, adCmdText or adExecuteNoRecords);
     strToQuery:='';        //Clear the string to start adding till next GO
     end else strToQuery:= strToQuery +#13#10+ str;  //Add the script to strToQuery
    end;
    ADOCommand.CommandText := strToQuery;
    ADOCommand.Execute(NULL, NULL, adCmdText or adExecuteNoRecords);
 finally
  ADOConnection.Close;
  tString.Free;
 end;
 except
  MsgBox(GetExceptionMessage, mbError, MB_OK);
 end;
end;

Any suggestion to update the code will be much appreciated.

1. Function block

Standalone action like loading a script from file and splitting it into SQL commands by the GO command, I would move into a separate function. That function might have the following prototype:

function LoadScriptFromFile(const FileName: string; out CommandList: TStrings): Integer;
begin

end;

Having such function will isolate parsing and execution parts of the script. You would first load the script from a file and then in the other part of the script iterate the parsed commands and execute each one.

2. Object finalization

Always try to encose object finalization into a separate try..finally block. In your code it applies to the tString object. This part:

ADOConnection.Open;
try
  tString := TStringList.Create;
  ...
finally
  ADOConnection.Close;
  tString.Free;
end;

Should become:

ADOConnection.Open;
try
  tString := TStringList.Create;
  try
    ...
  finally
    tString.Free;
  end;
finally
  ADOConnection.Close;
end;

3. Useless variable

The len variable is used for only one purpose and its value is used only once, so it's actually useless there. I would remove it and write directly:

for i := 0 to tString.Count - 1 do
begin
  ...
end;

4. Summary

Except the above (not so major) things I would say your code is fine. The string concatenation with line breaks you have used should be fine for ADO connected to the SQL Server, I think.

For the sake of completness, here is what I'd write (untested!). Please note, that every SQL command is expected to be "terminated" by the GO command in the input file (that requires also your original code):

function LoadScriptFromFile(const FileName: string; out CommandList: TStrings): Integer;
var
  I: Integer;
  SQLCommand: string;
  ScriptFile: TStringList;
begin
  Result := 0;
  ScriptFile := TStringList.Create;
  try
    SQLCommand := '';
    ScriptFile.LoadFromFile(FileName);

    for I := 0 to ScriptFile.Count - 1 do
    begin
      if Pos('go', LowerCase(Trim(ScriptFile[I]))) = 1 then
      begin
        Result := Result + 1;
        CommandList.Add(SQLCommand);
        SQLCommand := '';
      end
      else
        SQLCommand := SQLCommand + ScriptFile[I] + #13#10;
    end;

    //EDIT: If there is no GO syntax present int the file AND
    //To add the script after the last GO - Govs
    CommandList.Add(SQLCommand);
    Result:= Result + 1;
  finally
    ScriptFile.Free;
  end;
end;

procedure RunScript(const FileName: string);
var
  I: Integer;
  CommandList: TStrings;
  ...
begin
  ...
  // here is already established connection
  CommandList := TStringList.Create;
  try
    if LoadScriptFromFile(FileName, CommandList) > 0 then
      for I := 0 to CommandList.Count - 1 do
      begin
        // execute each command
        ADOCommand.CommandText := CommandList[I];
        ADOCommand.Execute(NULL, NULL, adCmdText or adExecuteNoRecords);
      end;
  finally
    CommandList.Free;
  end;
  ...
end;


C dynamic array implementation using preprocessor I have implemented a generic dynamic array implementation using defines. As I am new to C, I am not sure whether the code has any flaws. The code is as follows,

#define INIT_CAPAC 5
#define INC_CAPAC 5
#define list(t) struct{ t* data; unsigned int count; unsigned int capac;}
#define list_init(lst)  lst.data = malloc(sizeof(*lst.data) * INIT_CAPAC); \
                        lst.count = 0; \
                        lst.capac = INIT_CAPAC;
#define list_append(lst, item) lst.data[lst.count++] = item; \
                               if(lst.count >= lst.capac) { \
                                    realloc(lst.data, sizeof(*lst.data) * (lst.capac + INC_CAPAC)); \
                               }
#define list_count(lst) lst.count
#define list_at(lst, index) lst.data[index]

typedef list(int) int_list_t;

int main() {
    int_list_t arr;
    list_init(arr);

    list_append(arr, 10);
    list_append(arr, 20);
    list_append(arr, 30);

    printf("Count: %d\n", list_count(arr));
    printf("Item at 0: %d\n", list_at(arr, 0));
    printf("Item at 1: %d\n", list_at(arr, 1));
    printf("Item at 2: %d\n", list_at(arr, 2));
}

This implementation seems better than other C solutions because,

We don't store the item size as the part of array structure. In other dynamic array implementations, the size of each element is stored in along with the element count and array capacity.
This is as intuitive as the dynamic arrays in other high level languages like C++, C#.

Here are some things that may help you improve your code.

Use the appropriate #includes

This program requires two headers, both of which should be included:

#include <stdio.h>
#include <stdlib.h>

Check the return value of malloc

If the program runs out of memory, a call to malloc (or realloc) can fail.  The indication for this is that the call will return a NULL pointer.  You should check for this and avoid dereferencing a NULL pointer (which typically causes a program crash).

Don't leak memory

This code leaks memory.  To begin with, the code calls malloc but there is no corresponding call to free.  For a usable general list, there should be something akin to list_destroy().

Be careful with multiline macros

Multiline macros can have odd effects if you're not careful. For instance, if we do this:

for (int i=10; i < 70; ++i)
    list_append(arr, i);

The effect is that the if part of list_append is only executed once rather than once per iteration.  This can be fixed by wrapping all multiline macros in their own block by enclosing them in curly braces.  This will, however, have the side effect of no longer requiring a terminating semicolon if that matters to you.


Efficient String permutations calculation in Java In trying to compute permutations of a given String, what can I do to further improve the code below for memory and/or time efficiency?

import java.util.ArrayList;
import java.util.List;

public class Main {

    public static void main(String[] args) {
        permutateString("ABC");
    }

    private static void permutateString(String str) {

        List<String> results = permutations(new StringBuilder(str), new ArrayList<>());
        System.out.println("Number of permutations: " + results.size()
                            + "\n" + "Permutations: \n" + results);
    }

    private static List<String> permutations(StringBuilder input, List<String> permutationsList) {

        if (input.length() == 0 || input.length() == 1) {
            permutationsList.add(input.toString());
        }
        else {
            char prefix;
            StringBuilder substring;

            for (int i=0; i<input.length(); i++) {
                prefix = input.charAt(i);
                substring = new StringBuilder(input).deleteCharAt(i);
                for (String str : permutations(substring, new ArrayList<>())) {
                    permutationsList.add(String.valueOf(prefix) + str);
                }
            }
        }
        return permutationsList;
    }
}

Why are you giving as parameter to permutations a permutationsList, that you will both mutate and return? The purpose of this method is to generate the permutations of the given input; it shouldn't need to take any list as parameter, and should just return the result. (Also, node that this method is only called by giving it a new array list, which hints that it isn't really needed).

Also, why does it take a StringBuilder as parameter? This method doesn't try build a String with it, it just get its length and the character at a given index. So pass it a String.

Therefore, consider this instead:

private static List<String> permutations(String input) {
    List<String> permutationsList = new ArrayList<>();
    // ...
    return permutationsList;
}

And with this small change, we can clarify the code more. We would have

List<String> permutationsList = new ArrayList<>();
if (input.length() == 0 || input.length() == 1) {
    permutationsList.add(input);
} else {
    // ...
}
return permutationsList;

(We don't need to call toString() anymore since the argument is now a String). The first part handles the base condition of the recursion. Notice that it builds a new list, only to add a single element to it and return it directly. Instead, we can use the optimized Collections.singletonList(o) which directly returns a list containing the single given element. Now we can have:

if (input.length() == 0 || input.length() == 1) {
    return Collections.singletonList(input);
}
List<String> permutationsList = new ArrayList<>();
// ...
return permutationsList;

No need for an else-clause, and the use of the early return saves one level of indentation and adds to clarity.

Then, we can move on to the scope of the variables. So far, the code is:

char prefix;
StringBuilder substring;
for (int i=0; i<input.length(); i++) {
    prefix = input.charAt(i);
    substring = new StringBuilder(input).deleteCharAt(i);
    // ...
}

Why declare the variables outside of the for loop? They are only used inside of it; variables should have the tighter scope possible. Instead, consider:

for (int i=0; i<input.length(); i++) {
    char prefix = input.charAt(i);
    StringBuilder substring = new StringBuilder(input).deleteCharAt(i);
    // ...
}

which saves in lines of code, makes sure prefix and substring have a minimal scope and is overall clearer.

Finally, the inner for loop calculates String.valueOf(prefix) each time, when the result will always be the same.

char prefix = input.charAt(i);
// ...
for (String str : permutations(substring)) {
    permutationsList.add(String.valueOf(prefix) + str);
}

This could be done a single time and reuse the result with

String prefix = String.valueOf(input.charAt(i));
// ...
for (String str : permutations(substring)) {
    permutationsList.add(prefix + str);
}

With all that, the end result is:

private static List<String> permutations(String input) {
    if (input.length() == 0 || input.length() == 1) {
        return Collections.singletonList(input);
    }
    List<String> permutationsList = new ArrayList<>();
    for (int i = 0; i < input.length(); i++) {
        String prefix = String.valueOf(input.charAt(i));
        String substring = new StringBuilder(input).deleteCharAt(i).toString();
        for (String str : permutations(substring)) {
            permutationsList.add(prefix + str);
        }
    }
    return permutationsList;
}

Other comments:

permutateString is a wrong name. The word "permutate" doesn't exist, it should be "permute". But actually, even that would be a wrong name: the method doesn't actually permute the given String. It prints the permutations of it. The difference is that, with a method named permuteString, one would expect it to return the permutations, except this one doesn't, since it prints them. Consider renaming it printPermutationsOf.
If you worry about the memory and time efficiency of the algorithm, then you're likely not using the right tool here. Building all the permutations will be memory intensive (with all the objects to build), and very slow (complexity is O(n!), where n is the length of the String). It is only practical for short Strings: there are already more than 3 millions permutations for a String of length 10. Depending on the use-case, you will either want to filter some of them, or even use a completely different approach that doesn't require building any permutations.


Stopping a thread that runs an action periodically I have a class that (among others) needs to run an action at fixed intervals of time. So I created a thread for this:

struct Logger
{
   std::chrono::milliseconds flush_interval_;
   std::thread flush_thread_;

   Logger(...) : flush_thread_{&Logger::flush_thread_function, this}, ... {}

   auto flush_cb() -> void; // this is the action that need to be ran periodically

   auto flush_thread_function() -> void
   {
       while (true)
       {
           std::this_thread::sleep_for(flush_interval_);
           flush_cb();
       }
   }
};

The problem with this is of course stopping this thread when the object is destroyed. I came with a solution involving condition_variable that seems to work, but since I don't have much experience with multi-threading and I am just getting my head around condition_variable I want to make sure the solution is OK.

This is the part of the class that deals only with this:

class Logger
{
private:
    std::chrono::milliseconds flush_interval_;

    bool flush_stop_ = false;
    std::condition_variable flush_stop_cv_{};
    std::mutex flush_stop_mutex_{};

    std::thread flush_thread_;

public:

    Logger(std::chrono::milliseconds flush_interval = 1s)
        : flush_interval_{flush_interval},
          flush_thread_{&Logger::flush_thread_function, this}
    {
    }

    ~Logger()
    {
        {
            std::lock_guard<std::mutex> lock_guard{flush_stop_mutex_};
            flush_stop_ = true;
        }

        flush_stop_cv_.notify_all();
        flush_thread_.join();
    }
private:

    auto flush_cb() -> void;

    auto flush_thread_function() -> void
    {
        auto prev_flush = std::chrono::system_clock::now();

        while (true)
        {
            auto next_flush = prev_flush + flush_interval_;

            std::unique_lock<std::mutex> lock{flush_stop_mutex_};
            auto stop_reason = flush_stop_cv_.wait_until(lock, next_flush);

            if (stop_reason == std::cv_status::timeout)
            {
                flush_cb();
                prev_flush = std::chrono::system_clock::now();
            }
            else if (flush_stop_)
            {
                // could call flush_cb() one last time here,
                // but I have another mechanism that deals with unflushed log entries
                break;
            }
            else
            {
                // this is a spurious wakeup
                // nothing to do here
            }
        }
    }
};

I am interested if my logic is solid and if the implementation is (thread-) safe. I chose wait_until and this overload in particular because the condition won't tell me if it was a timeout or a spurious wakeup. Another part that I am not entirely sure it's ok is the locking/unlocking of flush_stop_mutex_ in ~Logger() and the use of lock in flush_thread_function. As I understand after waking flush_stop_mutex_ is locked protecting flush_stop_ until the end of the current iteration of while, so I reason it is correct.

Your code is almost correct. Here's your code stripped down:

    while (true)
    {
        auto next_flush = prev_flush + flush_interval_;
        std::unique_lock<std::mutex> lock{flush_stop_mutex_};
        auto stop_reason = flush_stop_cv_.wait_until(lock, next_flush);

        // stopping logic omitted
    }

Notice that the unique_lock is destroyed at the }, and so the mutex is not locked for a little while, until the next time you loop around and hit the std::unique_lock line. If another thread destroys the Logger during this period, it'll notify flush_stop_cv_ but you won't be listening and so you'll miss the notification... and that was your one chance to hear it, so now you'll never stop and your program is frozen.

Solution: If you're waiting for a notification that you absolutely must hear, then absolutely do not stop listening. Don't drop that lock!

    std::unique_lock<std::mutex> lock{flush_stop_mutex_};
    while (true)
    {
        auto next_flush = prev_flush + flush_interval_;
        auto stop_reason = flush_stop_cv_.wait_until(lock, next_flush);

        // stopping logic omitted
    }

Or if you're really worried about holding the mutex for some reason, then check again after taking the lock but before going to sleep:

    while (true)
    {
        auto next_flush = prev_flush + flush_interval_;
        std::unique_lock<std::mutex> lock{flush_stop_mutex_};
        if (flush_stop_) break;  // IMPORTANT LINE ADDED
        auto stop_reason = flush_stop_cv_.wait_until(lock, next_flush);

        // stopping logic omitted
    }

Otherwise, looks good to me.

condition_variable::wait_until is a fairly obscure function, IMO. Here you're assuming that it is implemented correctly and efficiently. I'm not sure that it always is, but I have no evidence to the contrary. The only immediately obvious way to get rid of wait_until is almost certainly worse: You could set another thread to notify the condition variable every flush_interval_ seconds, and detect spurious wakeups by computing std::chrono::system_clock::now() - prev_flush_, instead of by looking for std::cv_status::timeout.


Collection of sorting types A collection of sorting types as following insertion, radix, selection and heap sorts.

How can i improve it?

#include <algorithm>
#include <functional>
#include <type_traits>
#include <iostream>

template <typename Iterator>
void radixSort(Iterator begin, Iterator end)
{
    using value_type = typename std::iterator_traits<Iterator>::value_type;

    for (auto i = 0; i < 32; ++i)
    {
        std::stable_partition(begin, end,
            [&](value_type& value)
        {
            if (i == 31)
                return value < 0;
            else
                return !(value & (1 << i));
        });
    }
}

template<typename Iterator>
void insertionSort(Iterator begin, Iterator end)
{
    using value_type = typename std::iterator_traits<Iterator>::value_type;

    for (auto i = begin; i != end; ++i)
    {
        std::rotate(std::upper_bound(begin, i, *i, std::less<value_type>()), i, std::next(i));
    }
}

template <typename Iterator>
void selectionSort(Iterator begin, Iterator end)
{
    using value_type = typename std::iterator_traits<Iterator>::value_type;

    for (; begin != end; ++begin)
    {
        const auto minElemt(std::min_element(begin, end, std::less<value_type>()));

        if (begin != minElemt)
        {
            std::iter_swap(begin, minElemt);
        }
    }
}

template <typename Iterator>
void heapSort(Iterator begin, Iterator end)
{
    using value_type = typename std::iterator_traits<Iterator>::value_type;

    std::make_heap(begin, end, std::less<value_type>());
    std::sort_heap(begin, end, std::less<value_type>());
}

int main()
{
    int a[] = {9,-6, 7, -8, 6, -9, 1, 3, -4, 2, -1, -5, 5, 8, -3, 4, 0, -2, -7 };

    radixSort(std::begin(a), std::end(a));
    //insertionSort(std::begin(a), std::end(a));
    //selectionSort(std::begin(a), std::end(a));
    //heapSort(std::begin(a), std::end(a));

    for (const auto& i : a)
    {
        std::cout << i << ' ';
    }
}

In radixSort you assume that std::iterator_traits<Iterator>::value_type is a signed 32bit int.

Instead you should check whether it's an integral type and alter the algorithm a bit based on its properties:

template <typename Iterator>
void radixSort(Iterator begin, Iterator end)
{
    using value_type = typename std::iterator_traits<Iterator>::value_type;

    static_assert(std::numeric_limits<value_type>::is_integer, "Wrong type");

    for (auto i = 0; i < std::numeric_limits<value_type>::digits; ++i)
    {
        std::stable_partition(begin, end,
            [&](value_type& value)
        {
             return !(value & (1 << i));
        });
    }

    if(std::numeric_limits<value_type>::is_signed){
        std::stable_partition(begin, end,
            [&](value_type& value)
        {
             return value < 0;
        });
    }

}

Besides that all your algorithms use the standard std::less comparator. you should let the user supply a comparator (a mapping function for radixSort) for types that don't have a default std::less.


A little "infile" Asynchronous Module Definition I like the modular aspect of AMDs (Asynchronous Module Definition; see this Stack Overflow reply) and it's quite neat to structure JS.

But I typically compile all my JS files into one JS file. Therefore I don't really need the asynchronous part. So I thought I could quickly write such "infile" Module Loader.

var load=(function()
{
  var modules={};
  return function(moduleName,dependencies,module)
  {
    if(moduleName.constructor===Array)
    {
      module=dependencies;
      dependencies=moduleName;
      moduleName=undefined;
    }
    if(!((moduleName&&moduleName.constructor===String||moduleName===undefined)
         && dependencies&&dependencies.constructor===Array
         && module&&module.constructor===Function))
      throw "wrong usage";
    var ret = module.apply(null,
      dependencies.map(function(d){return modules[d]||(function(){throw "no module "+d})()})
    );
    if(moduleName)
    {
      if(!ret||ret.constructor!==Object) throw "module should be an object";
      modules[moduleName]=ret;
    }
  }
})();

load('utils',[],function(){return {veryUseful:function(){alert('hey')}}});
load(['utils'],function(utils){utils.veryUseful()});

load is used on one way to define a module and on an other way to call a function with the required modules passed as function arguments.

Demo

The goal here is to allow one to have two pieces of code written in the same file that are guaranteed to be independent.

I'm curious if you have any suggestions on any level. From the concept of such "infile" Module Loader to a hidden neat little JS feature.

I re-read your code several things, I can't say I would want to use it.

Requiring the parameters to be modules seems neat with 1 module or 2. For larger applications, that would become silly.
The most common use case with require is var xxx = load("Xxx") but you do not provide anything of the sort, in fact, load does not return anything
Having load being called in 2 different ways with 2 different ways of execution is wrong, I would suggest to simply use 2 different functions.
Consider rewriting the if block for "wrong usage" with falsey values and/or add some comment, it is pretty much unreadable now

At the very least I would modify the apply call to a call call, so that this has all the modules :

var ret = module.call(modules);

which makes for

load('utils',[],function(){return {veryUseful:function(){alert('hey')}}});
load(['utils'],function(){this.utils.veryUseful()});

and return ret in case the caller wants to work with the module directly.


Find largest palindrome from the product of two 3-digit numbers I think this is a good solution to finding the largest numeric palindrome that can be formed from the product of two 3-digit numbers... But it seems like there are some improvements that we could make which I haven't thought of...

Given int inputs[899] which is initialized by: iota(begin(inputs), end(inputs), 100), Do you guys have any thoughts on this code:

for(auto it = crbegin(inputs); it != crend(inputs) && *it * *it > max_product; ++it) {
    const auto rhs = find_if(it, crend(inputs), [lhs = *it](const auto rhs){
        const auto input = to_string(lhs * rhs);

        return equal(cbegin(input), next(cbegin(input), size(input) / 2U), crbegin(input));
    });

    if(crend(inputs) != rhs) {
        const auto product = *it * *rhs;

        if(product > max_product) {
            max_product = product;
            max[0] = *it;
            max[1] = *rhs;
        }
    }
}

Live Example

This will correctly find 993 and 913 as the pair of 3-digit numbers that would form the largest numeric palindrome.

The main thing I'd suggest is to use functions more to break up your code into more clearly defined subtasks - especially if those subtasks are reusable.

For example, the lambda in your find_if() is:

[lhs = *it](const auto rhs){
    const auto input = to_string(lhs * rhs);

    return equal(cbegin(input), next(cbegin(input), size(input) / 2U), crbegin(input));
}

That would be clearer - and more maintainable and reusable - as:

[lhs](auto rhs){ return is_numeric_palindrome(lhs * rhs); }

Where is_numeric_palindrome() could be defined as:

auto is_numeric_palindrome(int value)
{
    auto const s = std::to_string(value);

    return is_palindrome(s.begin(), s.end());
}

The way you do the test right now can be neither constexpr nor noexcept, because you use to_string(), but we'll fix that shortly.

is_palindrome() is also an operation that could be reusable, and can be defined as:

// Need C++20 for constexpr std::equal()
// could possibly be noexcept, if only conditionally
template <typename BiDiIterator>
constexpr auto is_palindrome(BiDiIterator first, BiDiIterator last)
{
    return std::equal(//...
}

Once you have these functions written you can optimize them at your leisure. For example, you could go back to is_numeric_palindrome() and rewrite it as:

auto is_numeric_palindrome(int value, int base = 10) noexcept
{
    constexpr auto MAX_SIZE = // determine the max buffer size you need

    auto buffer = std::array<char, MAX_SIZE>{};

    auto res = std::to_chars(buffer.data(), buffer.data() + buffer.size(), value, base);

    return is_palindrome(buffer.data(), res.ptr);
}

That should make the whole algorithm quite a bit faster. And as a free side-effect/benefit, you can now check for palindromes in bases other than 10. (Technically you could do that with to_string(), too, I believe.)

And then after that you could devise a way to test if a number is a palindrome in any base without converting it to a string. You'd just need to rewrite is_numeric_palindrome() again. That might make your code even faster; you'd have to profile to be sure.

Moving on....

I think it would make your code more clear to use some better-named variables in the mix. For example, obviously-named variables like lhs and it_end could be defined right at the top of the loop like:

for(auto it = crbegin(inputs); it != crend(inputs) && *it * *it > max_product; ++it) {
    const auto lhs = *it;
    const auto it_end = crend(inputs);

    const auto it2 = find_if(it, it_end, [lhs](auto rhs){ return is_numeric_palindrome(lhs * rhs); });

    if(it2 != it_end) {
        const auto rhs = *it2;
        const auto product = lhs * rhs;

        if(product > max_product) {
            max_product = product;
            max[0] = lhs;
            max[1] = rhs;
        }
    }
}

I think that's much easier to read and mentally parse.

Now onto some more technical concerns....

You never bother to check that your multiplication doesn't overflow... and in fact, in your demo code it does overflow the minimum size of an int. An int is only guaranteed to hold from -32,767 to 32,767 inclusive... your final palindrome is 906,609. Everything probably works on your system because your ints are 32 or 64 bits, but you really should check these things in general. One easy way to do that is just to get the max value from your inputs, and divide numeric_limits<int>::max() by that, and make sure the result is greater than the divisor.

If you want to generalize this algorithm into its own function - which is a good idea - be sure to check whether the input data is sorted, and be sure to handle the situation where the input data sequence is empty (maybe by returning optional<tuple<int, int>>. And do checks to make sure the data is what you expect it to be (for example, no negative values).


Merging directories and keep files that have more lines Goal

My goal is to merge directories. Whenever a file has the same name in two or more directories, then only the one that has the highest number of lines should be kept. If both files have the same number of files and differ then an error message should be thrown. Note that files having more lines are also bigger (in my specific case) which might be another way to compare files having the same name.

My Code

Here is my code that I think works fine

### Parameters ###

GeneralPath="/Users/remi/Documents/Biologie/Vancouver/PhD/Thesis/BackgroundSelection/Simulations/s_and_Pi/outputs/4.0.2_1.0.5/"

cd ${GeneralPath}
Directories=( HR OR OS ) # Array of directories ot be merged with the destination directory
Destination=HS
errorFile="${GeneralPath}MergeDirs.err"

### Do Stuff ###

for d in ${Directories[@]};do
    echo "${d}"
    cd $d
    for f in *; do
        echo "      ${f}"
        if [ ! -f "../${Destination}/${f}" ];then
            echo cp1
            cp ${f} ../${Destination}/
        else
            nblinesFrom=$(wc -l ${f} | awk -F" " '{print $1}')
            nblinesDest=$(wc -l "../${Destination}/${f}" | awk -F" " '{print $1}')
            if [ ${nblinesFrom} -gt ${nblinesDest} ]; then
                echo cp2
                cp ${f} ../${Destination}/
            elif [ ${nblinesDest} -gt ${nblinesFrom} ];then
                echo "Destination is bigger - nothing to do"
            else
                DoTheyDiffer=$(diff ${f} "../${Destination}/${f}" | wc -l)
                if [ ${DoTheyDiffer} -gt 0 ];then
                    echo "${f} and ../${Destination}/${f} diff but have the same number of lines" >> ${errorFile}
                fi
            fi
        fi
    done
    cd ..
done

My code seems quite complicated and I feel like a good combinaison of find -exec, awk, cp and diff might do something much more fancy.

Use More Quotes™.
Use a shebang line (disclosure: I wrote that answer).
Don't use single character variables. Maintainability is the most important feature of code.
Rather than echo cp1 etc., simply use cp -v to print every copy command verbatim.
You don't need to count the number of lines that diff returns, you can simply do if diff foo bar, or the safer option:

diff foo bar
exit_code=$?
if [ "$exit_code" -eq 0 ]
then
    [no difference]
elif [ "$exit_code" -eq 1 ]
then
    [difference]
else
    exit "$exit_code" # WTF
fi

Some people love mixing languages. I think cut -d' ' -f1 is much nicer than even a short awk script.
If you pass the file to wc on standard input, it doesn't print the filename, and so you don't need to process the output at all: wc -l < /path
You can use if cp --no-clobber source destination to try copying the file instead of checking whether the target exists.
I'm not sure I understand why you're copying rather than moving files (unless this is a one-off script, which you're not going to test, and it's only going to take a few seconds anyway).
Be wary of using cd in scripts. It changes the context in a significant way, and makes it harder to reason about what the script will do. Instead, simply do `for path in "$directory"/*.
I can definitely recommend set -o errexit -o noclobber -o nounset -o pipefail. You could also use -o xtrace to make all those logging commands obsolete.
A common convention to get used to is to never end paths with a slash. Firstly because cp a b/ and cp a b and the same as long as b is a directory, and secondly because it makes it more natural to concatenate paths without ending up with double slashes.


A four-item text-based menu in Python I picked up python again, and I am proud with what I have without using if/elif statements throughout the whole thing. I have modified this couple of times, since I kept asking how could I make it better or more organized. My biggest complaint is using the if/elif at the bottom, for some reason when I use.

func = optionMenu.get(test, "INVALID ENTRY")

it would would return blank instead of showing "invalid entry" if I selected option that wasn't on the menu. So, I had to use if/else to band-aid it for now.

#!/usr/bin/env python

import os

def mainMenu():

    print "=" * 25
    print "Please Select An Option"
    print "=" * 25
    print ""
    print "1. option one\n"
    print "2. option two\n"
    print "3. option three\n"
    print "4. filler\n"
    menuOption = input("Select Option: ")

    optionSelect(menuOption)

class selections(object):
    def one(self):
        print "option one"

    def two(self):
        print "option two"

    def three(self):
        print "option three"

    def four(self):
        print "option four"

def optionSelect(test):
    menuSelection = selections()
    optionMenu = {
    1 : menuSelection.one,
    2 : menuSelection.two,
    3 : menuSelection.three,
    4 : menuSelection.four,
    }

    func = optionMenu.get(test)
    if test in optionMenu:
        func()
    else:
        print "ERROR INVALID ENTRY ERROR"
mainMenu()

I think, there is room for some improvement:

1) You are mixing OOP with procedural code

If you choose one paradigm, you should stick with it. Besides: why are you abstracting selections into a class and not the whole menu?

2) DRY - Don't repeat yourself

print "=" * 25
print "Please Select An Option"
print "=" * 25

Why are you repeating the same task? You could refactor the "=" * 25 into a constant: separator = "=" * 25.

Everytime you need a separator, you could use separator, which is more meaningful.

3) You should separate concerns

print "4. filler\n"
menuOption = input("Select Option: ")

Here you are mixing two concerns printing the options and prompting for input. This pollutes your mainMenu. One function should only do one thing (SRP).

4) Your code lacks an overall structure

If you take every point together, this should now be clear.

I came up with the following:

#!/usr/bin/env python2

import os
from collections import namedtuple

class Menu():

    Option = namedtuple('Option', 'label')
    _separator = "=" * 25
    _options = {1: Option("option one"), 2: Option("option two"),
                3: Option("option three"), 4: Option("option four")}

    def print_header(self):
        print "{0}\n Please Select An Option\n{0}\n".format(self._separator)

    def print_mainMenu(self):
        self.print_header()
        for option in sorted(self._options.keys()):
            print "{0} {1}".format(option, self._options[option].label)

    def prompt(self):
        return input("Select Option: ")

    def handle_input(self, chosen_option):
        try:
            print self._options[chosen_option].label
        except KeyError:
            print "Wrong Option"

def main():
    menu = Menu()
    menu.print_mainMenu()
    menu.handle_input(menu.prompt())

if __name__ == '__main__':
    main()

For this example, the use of namedtuple is not necessary, but on the other hand gives you something like a lightweight object which could not only contain lables. On top options[chosen_option].label is very readable.


Optimize mass import of XML to SQLServer in Ruby I have the following Ruby code that's designed to update item, price and stock data for items in a MSSQL database. It's running on a Ruby 1.8.6/Rails 1.2.3 installation, in its own controller (for now)

What I'm looking for is ways to optimize performance.

Right now each item takes about 0,2 seconds (200ms) to process. Edit: There may be 10,000+ items in the XML file, and 10,000,000+ items in the "Items" and "Price" SQL tables.

I've been told that

composing my own SQL queries directly instead of using the models will be faster, since I'm doing quite a few lookups, how much performance would that lend me?
and selecting many rows at once, doing processing, and inserting at once should be faster, instead of doing it one by one. (How would this be done in practice?)

  # counters for statistics
  count = 0
  skips = 0

  # load + parse XML
  file = File.read(XML_PATH)
  doc = REXML::Document.new file

  # loop through products in XML file
  doc.elements.each('products') { |product|
    itemid = product.attributes['id'].to_i
    itemprice = product.elements['price'].text.to_i

    item = Item.find_by_id(itemid)
    if item.nil?
      skips += 1
      next
    end

    # update prices
    price = Price.find(:first, :conditions => { :item_id => itemid })
    # create new price if price not found
    if price.nil?
      price = Price.new
      price.item_id = itemid
    end
    price.price = product.elements['price'].text.to_i
    price.save

    # find + update item stock data
    product.elements.each('stocks/location') { |location|
      item_stock_location_id = location.attributes['location_code']
      item_stock_location = ItemStockLocationCount.find(:first,
                                                    :conditions => {
                                                        :item_stock_location_id => item_stock_location_id,
                                                        :item_id => itemid.to_s
                                                    })

      if item_stock_location.nil?
        item_stock_location = ItemStockLocationCount.new
        item_stock_location.item_stock_location_id = item_stock_location_id
        item_stock_location.item_id = itemid
      end
      item_stock_location.stock_qty = location.elements['stock'].text.to_i
      item_stock_location.save
    }

    # update onsite_stock + offsite_stock
    item_stock_count_on_site = 0
    item_stock_count_off_site = 0
    item_stock_loc_qtys = ItemStockLocationQuantity.find_all({ :item_id => itemid.to_s })
    item_stock_loc_qtys.each { |stock_loc_qty|
      item_stock_count_on_site += stock_loc_qty.stock_qty
      item_stock_count_off_site += stock_loc_qty.stock_qty unless stock_loc_qty.item_stock_location_id == 1
    }
    item.onsite_stock = item_stock_count_on_site
    item.offsite_stock = item_stock_count_off_site

    # update price lock
    item.price_lock = product.elements['price_locked']
    if item.price_lock == 1
      item.vat_price = product.elements['price']
      item.price = (itemprice - ((itemprice * 25)/ (100 + 25)))
    end

    item.save

    count += 1
    # progress bar
    if count%100 == 0
      printf '.'
    end
  }

  puts 'Item Count: ' + count.to_s
  puts 'Items skipped: ' + skips.to_s

As for performance, I definitely think you want to use SQL's Bulk Insert from XML functionality. I don't know how to use it from Ruby, so I'll leave that to another reviewer.

I do want to point out something else though. I had to scroll down two pages to find the end of the loop that this opens up. doc.elements.each('products') { |product|. In fact, the entirety of the code is inside of this loop. Blocks of code should fit on the screen in their  entirety. I call it the "Single Page Principle".

So, the refactoring step that should be taken is to define a method that takes product as a parameter and put all of the logic necessary to handle a single doc.element inside of it.

def update_product!(product)
    #ALL THE CODEZ
end

# loop through products in XML file
doc.elements.each('products') { |product| update_product!(product) }

The next step would be to break down the steps inside of the new update_product method into smaller methods as well. A good start would be to take the code between the comments and place them inside of a method with a good name. This replaces your comment and breaks the code down into logical chunks.

For example:

Instead of this:

# update prices
price = Price.find(:first, :conditions => { :item_id => itemid })
# create new price if price not found
    if price.nil?
      price = Price.new
      price.item_id = itemid
    end
    price.price = product.elements['price'].text.to_i
    price.save

    # find + update item stock data
    product.elements.each('stocks/location') { |location|
      item_stock_location_id = location.attributes['location_code']
      item_stock_location = ItemStockLocationCount.find(:first,
                                                    :conditions => {
                                                        :item_stock_location_id => item_stock_location_id,
                                                        :item_id => itemid.to_s
                                                    })

Define this:

def update_price
    # update prices
    price = Price.find(:first, :conditions => { :item_id => itemid })
    # create new price if price not found
    if price.nil?
      price = Price.new
      price.item_id = itemid
    end
    price.price = product.elements['price'].text.to_i
    price.save

    # find + update item stock data
    product.elements.each('stocks/location') { |location|
      item_stock_location_id = location.attributes['location_code']
      item_stock_location = ItemStockLocationCount.find(:first,
                                                    :conditions => {
                                                        :item_stock_location_id => item_stock_location_id,
                                                        :item_id => itemid.to_s
                                                    })
end

And call it from update_product!. Continuing doing this until each method has exactly one responsibility. You should end up with code that looks something like this psuedocode.

def update_product(product)
    update_price
    update_item_stock_data
    update_stock_site
    update_price_lock

    count += 1
    # progress bar
    if count%100 == 0
      printf '.'
    end
end

 # counters for statistics
  count = 0
  skips = 0

  # load + parse XML
  file = File.read(XML_PATH)
  doc = REXML::Document.new file

  # loop through products in XML file
  doc.elements.each('products') { |product| update_product!(product) }

  puts 'Item Count: ' + count.to_s
  puts 'Items skipped: ' + skips.to_s


Remove elements existing in a list built from an enum The objective of this code is to make the difference inbetween two lists of items, that is knowing which items are missing in the second list in regards to the first one.

The first list is built from an enumerator with the following method:

keysToPress = Enum.GetValues(enumType).ToList();

Once this list is built, now the second list, the one we desire to compare has to be retrieved. In my code, I ask a device I am testing the information of a  register. Processing this register returns me a list of type enumType.

// ReadKeyboard method retrieves a list of all the keys that have been
// pressed at least one time during the device test.
// (Read notes to know why the code is missing, I feel like it is not needed)
keysPressed = GetKeyList();

Once both list have been retrieved, I only have to make the difference in between them with the following code:

keysPressed.ForEach(x=> keysToPress.Remove(x));

This way, I remove every item in the keysPressed list from the keysToPress list.

The resulting code is as follows:

private List<enumType> CheckKeysToPress()
{
    List<enumType> keysToPress = Enum.GetValues(typeof(enumType)).Cast<enumType>().ToList();

    // ReadKeyboardMask returns a enumType list
    List<enumType> keysPressed = GetKeyList();

    // Here I try to remove all the items in the keysPressed list from the keysToPress list
    keysPressed.ForEach(x=> keysToPress.Remove(x));

    return keysToPress;
}

As you can see, the return value of the method is a list of all remaining keys to press.

An example for the following code, given the enum:

[Flags]
public enum Keys
{
    NONE = 0x0000,
    UP = 0x0001,
    DOWN = 0x0002,
    LEFT = 0x0004,
    RIGHT = 0x0008,
    OK = 0x0010,
    POWER = 0x0020
}

The keysToPress list would have the following items:

NONE
UP
DOWN
LEFT
RIGHT
OK
POWER

With a reading from the device that returns a list with the following items.

UP
DOWN
LEFT
RIGHT

would output the following list:

NONE
OK
POWER

I want to know, as the information retrieved is a set of bit flags, if it is better to provide the bit flags and then remove those items that match the enum, to optimise and make less error prone the code.

Note: The code regarding GetKeyList() is missing because  it is a function that interacts with a device to retrieve information from it communicating through ModBus protocol.

What I like about the method is that it's implementation is straight forward and easy to understand. There are, however, a few things that may be done done differently.

The name of the method is misleading, as it does not actually check keys, but it build/returns a list of keys not pressed at the current moment.
The result of Enum.GetValues(typeof(enumType)) should never change during runtime, thus running it on every method call must be sub-optimal on recurring calls.
Using the ForEach method and using List<T>.Remove to remove the items looks fine, but List<T> might not be the best data type to remove items from. You could also use a HashSet<T>, which would not go all the way to Array.IndexOf<T>.

I tested around a bit and found that another option - not removing items from a list - is actually marginally faster.

    private readonly IReadOnlyList<Keys> keysToPress = Enum.GetValues(typeof(Keys)).Cast<Keys>().ToList();

    private List<Keys> CheckKeysToPressWithExcept()
    {
        // You could just use LINQ - the method probably won't get any simpler than this.
        return this.keysToPress.Except(GetKeyList()).ToList();
    }

    private List<Keys> CheckKeysToPressWithSet()
    {
        var keysPressed = new HashSet<Keys>(GetKeyList());

        // Or you can construct the resulting list manually.
        // Performs better, but is more code to read.
        var result = new List<Keys>(this.keysToPress.Count);
        foreach (var key in this.keysToPress)
        {
            if (!keysPressed.Contains(key))
            {
                result.Add(key);
            }
        }

        return result;
    }

In a quick benchmark, CheckKeysToPressWithSet was the fastest, a modified version of your CheckKeysToPress that uses the IReadOnlyList<Keys> keysToPress instead of re-creating the enum value list every time, was 2% slower and the LINQ was 14% slower.


Partition function with itertools from itertools import izip_longest

def partition(n, iterable):
   p = izip_longest(*([iter(iterable)] * n))
   r = []
   for x in p:
       s = set(x)
       s.discard(None)
       r.append(list(s))
   return r

print partition(2, range(100))

I saw it from a recruit and the company said there were potential problems, but I can't find out anything wrong with my test case:

print partition(2, range(100))
print partition(1, range(100))
print partition(-1, range(100))
print partition(101, range(100))

The elephants in the room:

Single letter variable names are to be avoided. Each variable should have a suggestive meaningful name.
They probably expected you to write automated tests, just printing a result for visual inspection is not a real way of testing. assert is easy to use and can get you started on automated testing.