A class to generate, save and load unique transaction reference numbers for an application This code is a class I use in my financial market trading bot (note: I'm a daytrader-come-beginner programmer, not the other way around).

Although I have the application running successfully, it has major design flaws which I intend to now address ASAP.

An API requires that a unique transaction ref is submitted with each new order, otherwise, that order will be rejected. I have very little idea if this is a competent or robust design, I could not find any guidance on the issue elsewhere.

I'm very much open to refactoring or a completely new design.

class UniqueTransactionRef:
    """Transaction reference number iterator

    Loads, generates and saves unique transaction ref data for ``Bot`` orders

    :ivar file: absolute path the the .json transaction ref data file
    :ivar num: an integer representing the last generated transaction ref number
    """

    default_file = r"C:\Bot\Bot\trans_ref.json"

    def __init__(self, file=None):
        """Initialize the UniqueTransactionRef object

        :param file: an absolute path to a .json file containing the transaction
        reference number, as an integer
        """
        self.file = file if file else self.default_file
        self.num = 0
        self.load()

    def __iter__(self):
        return self

    def __next__(self):
        """Increment the self.num attribute. Save the value to the local file.

        :returns: a string formatted version of the self.num class attribute
        """
        self.num += 1
        self.save()
        return str(self.num)

    def get(self):
        """Call the ``next`` function on self """
        return next(self)

    def load(self):
        """Load the transaction ref data from the local file

        :raises errors.UniqueTransactionRefError: Cannot find file
        """
        try:
            with open(self.file) as f:
                self.num = json.load(f)
        except FileNotFoundError:
            raise errors.UniqueTransactionRefError(
                self.file,
                num=self.num,
                message="Cannot find file",
                )

    def save(self):
        """Save self.num to the designated local file

        :raises UniqueTransactionRefError:
        """
        try:
            with open(self.file, "w") as f:
                json.dump(self.num, f)
        except Exception as e:
            raise errors.UniqueTransactionRefError(self.file,
                                                     num=self.num,
                                                     message=e)
unique_trans_ref = UniqueTransactionRef()  # module level variable

Simplified example usage:

class Bot:
    def __init__(self, unique_trans_ref):
        self.unique_trans_ref = unique_trans_ref

    def construct_order():
        trans_ref = next(self.unique_trans_ref)

The server requires a unique ID to detect replays in the case of network failures, support idempotency, and to report order confirmations in a way the client can make sense of. So yes, the server offers a sensible API.

You can send sequential numbers, or a random guid. Consider using import uuid.

Consider using a numeric prefix of yyyymmdd, with strftime('%Y%m%d') + '%06d' % num. This will help when you have issues with deleted file, backups, switching client laptops, and so on. It makes the transaction IDs more human readable. (It also makes them easier to guess, if you're concerned about a possible attacker.)

Thank you for using with for I/O. Consider acquiring a lock before saving updated counter, if you're concerned that multiple daemons or users might run instances of your code concurrently.

There's no need for default_file, as you can just put that default in the signature:

    def __init__(self, file=r"C:\Bot\Bot\trans_ref.json"):

You catch a subset of the things that can go wrong with file I/O and JSON parsing. It does not appear well motivated, given that the client doesn't try anyway, so it doesn't care whether it gets fatal exception A or B. Perhaps you have a use case which requires a custom exception, but you have not shown it. Likely you don't need it at this point. I would just let library load and open throw as they will, and caller will (correctly) receive fatal exception if one happens. Later, when caller implements an actual error recovery strategy, you will have new requirements in hand for what exception details must be returned, and you can make a better informed design decision about how UniqueTransactionRef should interact with exceptions.

Write the simplest code you can get away with (it's easier to test!). When you get a new concrete requirement in hand, make the code a little fancier.


Implementation of hash map I was asked to implement a hash map in this phone interview (screen-share), so time was limited and this is what I came up with. I'd appreciate if someone can take a minute to critique/review it and help me improve.

Here is an online version of the code.

#include <list>
#include <iostream>
using namespace std;

const int SIZE = 10;

class Node{
public:
    Node(){}
    Node(int k, int v):key(k), value(v){}
    int key, value;
};

class HashMap {
private:
    list<Node*> data[SIZE];

public:
    ~HashMap();
    Node* get(int key);
    void put(int key, int value);

    int hashFn(int val){ return val % 13; }
};

Node* HashMap::get(int key){
    int hashValue = hashFn(key);
    int bucket = hashValue % SIZE;
    list<Node*>::iterator it = data[bucket].begin();

    while(it != data[bucket].end()){

        Node ** d = &(*it);
        if((*d)->key == key){
            return *d;
        }

        it++;
    }
    return NULL;
}

void HashMap::put(int key, int value){
    int hashValue = hashFn(key);
    int bucket = hashValue % SIZE;
    Node* node = this->get(key);
    if(node == NULL){
        data[bucket].push_front(new Node(key, value));
    }
    else{
        node->value = value;
    }
}

HashMap::~HashMap(){
    for(int i = 0; i < SIZE; ++i){
        list<Node*>& val = data[i];
        for(list<Node*>::iterator it = val.begin(); it != val.end(); it++){
            Node* n = *it;
            delete n;
        }
    }
}

int main(){
    HashMap map;
    cout << "Finding 5: " << map.get(5) << endl;  // -1
    map.put(5, 10);
    map.put(5, 11);
    cout << "Finding 5: " << map.get(5)->value;  // 11
    return 1;
}

Some obvious problems

It leaks memory (every newly allocated Node) when the Hashmap leaves the scope.
A delete/remove function is missing.
It doesn't replace the value within the existing Node on repeated puts with the same key but instead adds an additional Node every time.
The size is fixed and therefore the hashmap get quickly degenerates into linked list search.
Key and value types aren't generic.

Some positive things

It implements a hashmap
uses hash-lookup for search
returns correct results


A program that accepts numbers and computes their sum, and displays smallest and largest number I am self learning programming by reading "Programming: Principles, and Practice" by Bjarne Stroustrup. I have solved a drill. Please review my code and give suggestions.

Drill

  Write a program that:

  Accepts numbers with units m, cm, ft, and in and rejects other units.
  Keeps a record of all the numbers in a single unit and computes the      sum of the numbers.
  Prints the smallest and the largest numbers.

My solution:

#include <iostream>
#include <algorithm>

using namespace std;

int main()
{
    double temp{0};
    string temp_unit;
    vector<double> numbers;
    double sum{0};

    constexpr double cm_to_m = 0.01;
    constexpr double ft_to_m = 0.3048;
    constexpr double in_to_m = 0.0254;

    cout<<"Enter an integer with its unit. E.g 2m or 2 m \n";
    while(cin>>temp>>temp_unit){
        if(temp_unit=="m"){
            numbers.push_back(temp);
        }
        else if(temp_unit=="cm"){
            numbers.push_back(temp*cm_to_m);
        }
        else if(temp_unit=="ft"){
            numbers.push_back(temp*ft_to_m);
        }
        else if(temp_unit=="in"){
            numbers.push_back(temp*in_to_m);
        }
        else{
            cout<<"Incorrect Unit\n";
        }
    }
    sort(numbers.begin(),numbers.end());
    for(auto& i:numbers){
        cout<<i<<" ";
        sum+=i;
    }
    cout<<"\nNumber of elements "<<numbers.size()<<endl;
    cout<<"Sum of numbers: "<<sum<<endl;
    cout<<"Smallest number: "<<numbers[0]<<endl;
    cout<<"Largest number: "<<numbers[numbers.size()-1]<<endl;
    return 0;
}

Use maps to handle mappings

instead of having constants for each individual ratio, you might as well have a single constant that holds both the ratios and their respective strings, as well as being able to map one to the other.

Consider this:

const std::unordered_map<std::string, double> unit_ratios = {
  {"m", 1.0},
  {"cm", 0.01},
  {"ft", 0.3048},
  {"in", 0.0254}
};

int main() {
  std::vector<double> numbers;

  double temp;
  std::string temp_unit;
  while(cin>>temp>>temp_unit){
    try {
      numbers.push_back(temp * unit_ratios.at(temp_unit));
    }
    catch(const std::out_of_range&) {
      cout<<"Incorrect Unit\n";
    }
  }
...
}

Doesn't that look nicer? all the names and ratios nicely bundled together in one spot.

That's also why I've set it as a global constant in my example: The ratios and their name have nothing to do with the main function, they just are, so they should exist out of context.

Declare variables as late as possible

double sum{0}; should be declared as close as possible to the first use.

Use algorithms when possible.

You can use std::accumulate to calculate the sum:

double sum = std::accumulate(numbers.begin(), numbers.end(), 0.0);

Use front() and back() to access the first and last elements of a vector

You could make an argument that numbers[0] is as good as numbers.front(), but not for the last element.

cout<<"Smallest number: "<< numbers.front() << endl;
cout<<"Largest number: "<< numbers.back() << endl;

Do you really need to sort?

Sorting is a O(NlogN) operation, whereas std::max_element and std::min_element are both O(N).

Since you are only doing these two lookups through the vector, you might be better off just using these instead.

If you can use c++11, then std::minmax_element is even better, since you only have to do one pass.

don't use using namespace std;

Just... just don't. Please see every other C++ question on this site as to why.


Calculate the number of moves requires for a knight chess piece It's possible to work out the number of moves required to move from (0, 0) to (A, B) for a knight on an infinite chess board in O(1) time.

This is an attempt at a solution to do it in O(n) time, where n is the number of moves required.

#define ABS(a) ({ typeof (a) _a = (a); _a > 0 ? _a : -_a; })

int movesRequired(int A, int B) {
    int X = 0, Y = 0;
    int count = 0;
    if (!(((A-X) % 2) ^ ((Y-B) % 2)))
        return -1;
    while (X != A && Y != B) {
        int distX = A-X;
        int distY = B-Y;
        if (ABS(distX) > ABS(distY)) {
            X += distX>0?2:-2;
            Y += distY>0?1:-1;
        } else {
            X += distX>0?1:-1;
            Y += distY>0?2:-2;
        }
        count++;
    }
    return count;
}

It returns the minimum moves required, or -1 if it's not possible.

What I'm interested in:

As far as I am aware, this runs in O(n) time. However, I may not be correct. Does it?
Is there a way to improve my algorithm so it runs in O(n) time?

\$O(1)\$ Discussion

  Is it possible to calculate the distance in \$O(1)\$ time complexity?

Yes, it is.

This problem is called the "Knight's distance" problem, and googling it find a number of references.

This problem was posed as part of the South African "Computer Olympiad" in 2007 for high-school students to solve.... (I know, kids these days.... :) ). The specification was such that some input values for the programs were large enough that \$O(n)\$ solutions would fail to compute in the required time limits, so to get full marks you needed to (devise, and) compute the \$O(1)\$ system.

The problem description, and enough of the description of the \$O(1)\$ solution are available. In case the .za net goes dead, here's the description of the solution:

  4 The Knights Who Say Ni

  The (x, y)-coordinates in the 50% constraints are small enough to
  generate a full grid of the minimum number of moves required to get
  from any point to (0,0). The grid can be generated by a breadth-first
  search (BFS).

  We know that getting to (0,0) requires 0 moves. We therefore know that
  the points (1,2), (2,1) and the other six movements each require 1
  move. From each of those points with a shortest distance of 1, we can
  take another step in each direction for a shortest path of 2. However,
  some moves will bring us back to a point we have already found a
  shorter distance for and we therefore discard such moves. We continue
  this until the grid is full and then use the grid to find the
  distances for each knight.

  To get 100%, a lot of scribbling on paper is required to make some key
  observations and work out some formulae. The first thing to note is
  that the grid is symetrical along the x, y axis and the lines y = ±x.
  You can therefore convert all points (x, y) into an equivalent
  point such that the new 0 ≤ y ≤ x.

  The magic formula is:

  $$
f(x, y) = \left\{
 \begin{array}{ll}
  2 \left\lfloor \frac{y - \delta}{3} \right\rfloor  + \delta & \mbox{if } y > \delta \\
  \delta - 2 \left\lfloor \frac{\delta - y}{4} \right\rfloor & \mbox{otherwise}
 \end{array}
\right.
$$

  where \$δ = x − y\$.

Are there mistakes

Yes.... All squares are accessible to the knight. Why do you have a restriction?

I put your code in to an Ideone that loops through the 100 targets in positions (0,0) through (9,9).... and its results are disappointing... Does your code work?

NO. it does not. It produces wildly wrong values.....

Now, the algorithm I referenced above does not seem to be producing good values either (at least a short range....), but seriously? Your code hardly works at all :(

Lots, and lots of -1 distances, and lots and lots of 0.....


Binary to decimal in python binaryToDecimal = lambda binary: sum([[int(2**index) for index in range(len(binary))][-index-1] for index, char in enumerate(binary) if int(char)])

I wrote a simple function that converts binary strings into decimal integers. How can this be improved/simplified?

I know about the int function, I wanted to implement the conversion myself.

Of course, the most efficient solution would be to let python parse the binary string as integer ;)

binaryToDecimal = lambda x: int(x,2)

But you already know that, so what about simplifying your code like this:

binaryToDecimal = lambda binary: sum(1<<(len(binary)-index-1) for index, char in enumerate(binary) if char=='1')

removing the creation of the inner list comprehension just to pick one value
not using power but bitshift (integer computation, faster, more accurate)
don't create a list to pass to sum, pass a generator comprehension (it's faster, saves memory)
in your condition, don't convert your number to integer, just test against the char value (it's faster)

Keep in mind that your parsing has no error checking!


Laravel routes/controller I'm a clean guy when I code and I don't like typing the same thing over and over again, so I will try to be simple.

In my Laravel project I have two routes for every major page on the website, for example:

Route::get('/artigos','GlobalController@gestorArtigos');
Route::get('/artigos/{id}','GlobalController@gestorArtigos');

When it receives an ID in the controller:

 public function gestorArtigos(Request $request , $id = '') {
    $MyOutput = view('artigos.art_gestor_base');
    if($id){
        return $MyOutput->with('id',$id);
    }
    return $MyOutput->with('request',$request);
}

This is completely fine and I have no intention of changing this. What does bother me is the fact that I have to repeat the same function every single time I create a new page. The only differences are the function name and the value of $MyOutput, because the views change.

I need help to compact the function to a 'global' one so I don't have to repeat the code, just call it.

I don't think you need to pass $request to the view, because you can access it anyway with Laravel's helper request() function.

For the routes you can use optional parameters, something like.

Route::get('/artigos/{id?}','GlobalController@gestorArtigos');

For the views and repetitive checks you can always use view composers or middlewares depends on the case really.

Eventually you could create trait with function to perform this repetitive task and include it in controller, for instance:

trait RepetitiveTask
{
    public function fireView($output, $id)
    {
        return ($id) ? $output->with('id', $id) : $output;
    }
}

class YourController
{
    use RepetitiveTask;

    public function gestorArtigos(Request $request)
    {
        return $this->fireView(view('artigos.art_gestor_base'), $request->id);
    }
}

However i would use middlewares or view composers instead.

Whatever you do, make sure you keep the logic out of the views.


Proving even-ness in Agda I've been just getting started with Agda. I tried to prove, as exercise, evenness.

even : ℕ → Bool
even 0             = true
even 1             = false
even (suc (suc x)) = even x

x : even 12 ≡ true
x = refl

But asserting truth using _≡_ seems like a really bad way. What would be the best way of doing the same thing?

One thing you can do is define an inductive predicate describing what it means for a number to be even. You could for instance write:

data isEven : ℕ → Set where
  ZisEven  : isEven 0
  SSisEven : {n : ℕ} → isEven n → isEven (2 + n)

You can then prove that your even function is sound: whenever it returns true (which is what Data.Bool's T means) then the input was indeed an even number.

even-sound : (n : ℕ) → T (even n) → isEven n
even-sound zero          prf = ZisEven
even-sound (suc zero)    ()
even-sound (suc (suc n)) prf = SSisEven (even-sound n prf)

Now, you can write the following test rather than your previous equality: even-sound generates the proof that isEven 12.

y : isEven 12
y = even-sound 12 _

If you want to be even more precise in the way you specify even, you can also prove that it is complete: whenever an input is even, it indeed returns true.

even-complete : (n : ℕ) → isEven n → T (even n)
even-complete 0             ZisEven        = _
even-complete (suc (suc n)) (SSisEven prf) = even-complete n prf

Now, the specification of Even-ness I have chosen is a bit arbitrary, you may want to prove that it is equivalent to other formulations.

First, we can describe evenness in terms of oddness and reciprocally using mutually defined predicates:

mutual

  data isOdd′ : ℕ → Set where
    OisOdd′    : isOdd′ 1
    SEvenIsOdd : {n : ℕ} → isEven′ n → isOdd′ (suc n)

  data isEven′ : ℕ → Set where
    ZisEven′   : isEven′ 0
    SOddIsEven : {n : ℕ} → isOdd′ n → isEven′ (suc n)

Or we can use divisibility by 2 as the fundamental notion:

open import Data.Nat.Divisibility
isEven′′ : (n : ℕ) → Set
isEven′′ n = 2 ∣ n

Edit: I have included the proofs that these definitions are equivalent in a gist in case you want to peek.


Processing form values for each day of week I am using PHP, and I am getting the value back from the form. Since I am working with time, I have an array by (1 x 4) x 7, and I feel like I could do a better job at it. I just don't know exactly how should I approach the problem

$schedule = array(
    round(abs(strtotime($_POST['mon'][1]) - strtotime($_POST['mon'][0])) / 3600, 2),
    round(abs(strtotime($_POST['mon'][3]) - strtotime($_POST['mon'][2])) / 3600, 2),

    round(abs(strtotime($_POST['tue'][1]) - strtotime($_POST['tue'][0])) / 3600, 2),
    round(abs(strtotime($_POST['tue'][3]) - strtotime($_POST['tue'][2])) / 3600, 2),

    round(abs(strtotime($_POST['wed'][1]) - strtotime($_POST['wed'][0])) / 3600, 2),
    round(abs(strtotime($_POST['wed'][3]) - strtotime($_POST['wed'][2])) / 3600, 2),

    round(abs(strtotime($_POST['thu'][1]) - strtotime($_POST['thu'][0])) / 3600, 2),
    round(abs(strtotime($_POST['thu'][3]) - strtotime($_POST['thu'][2])) / 3600, 2),

    round(abs(strtotime($_POST['fri'][1]) - strtotime($_POST['fri'][0])) / 3600, 2),
    round(abs(strtotime($_POST['fri'][3]) - strtotime($_POST['fri'][2])) / 3600, 2),

    round(abs(strtotime($_POST['sat'][1]) - strtotime($_POST['sat'][0])) / 3600, 2),
    round(abs(strtotime($_POST['sat'][3]) - strtotime($_POST['sat'][2])) / 3600, 2),

    round(abs(strtotime($_POST['sun'][1]) - strtotime($_POST['sun'][0])) / 3600, 2),
    round(abs(strtotime($_POST['sun'][3]) - strtotime($_POST['sun'][2])) / 3600, 2),
);

Well, every time you see a repeated code, you could think of a loop.

if I am not mistaken, only a weekday is changed, so it could be used to form the loop:

$schedule = [];
$weekdays = ['mon','tue','wed','thu','fri','sat','sun'];
foreach ($weekdays as $day)
{
    $schedule[] = round(abs(strtotime($_POST[$day][1]) - strtotime($_POST[$day][0])) / 3600, 2),
    $schedule[] = round(abs(strtotime($_POST[$day][3]) - strtotime($_POST[$day][2])) / 3600, 2),
}


Is there a better way to search directories for a file or files? Edit:

The purpose of this method is to search a specific directory for a given file type, ex. (*.txt, *.pdb, *.exe") then move the file(s) to a given destination directory and delete the original file from the source directory.

Is there a better way to write the following code?:

/// <summary>
/// Moves a specific file type from a given
/// source directory to a destination directory.
/// </summary>
/// <param name="sourceDir">The source directory.</param>
/// <param name="destDir">The destination directory.</param>
/// <param name="fileExt">Type of file to be moved.</param>
private static void MoveFilesToDirectory(string sourceDir, string destDir, string fileExt)
{
    try
    {
        if ( !Directory.Exists(sourceDir) )
        {
            Console.WriteLine("{0} does not exist.", sourceDir);
            return;
        }

        if ( !Directory.Exists(destDir) )
        {
            Directory.CreateDirectory(destDir);
        }

        foreach ( String dir in Directory.GetDirectories(sourceDir) )
        {
            foreach ( String file in Directory.GetFiles(dir, fileExt) )
            {
                string fileName = Path.GetFileName(file);
                string moveFileTo = Path.Combine(destDir, fileName);

                if ( !File.Exists(moveFileTo) )
                {
                    Directory.Move(file, moveFileTo);
                }

                File.Delete(file);
            }

            MoveFilesToDirectory(dir, destDir, fileExt);
        }
    }
    catch ( IOException ex )
    {
        Console.WriteLine(ex.Message);
    }
}

static void Main(string[] args)
    {
        String mask = "*.txt";
        String source = @"c:\source\";
        String destination = @"c:\destination\";

        String[] files = Directory.GetFiles(source, mask, SearchOption.AllDirectories);
        foreach (String file in files)
        {
            File.Move(file, destination + new FileInfo(file).Name);
        }
    }

Add in your own directory exists verification logic... but this should be all you need at the core.  I haven't verified this code, but these built in functions should do what you want.


Sum of all arguments ever passed to counter Q4. Define a function make_counter that returns a counter function,
  which takes an immutable key and a numerical value argument and
  returns the sum of all arguments ever passed to counter with that same
  key.

Below is the solution:

def make_counter():
    """Return a counter function.

    >>> c = make_counter()
    >>> c('a', 3)
    3
    >>> c('a', 5)
    8
    >>> c('b', 7)
    7
    >>> c('a', 9)
    17
    >>> c2 = make_counter()
    >>> c2(1, 2)
    2
    >>> c2(3, 4)
    4
    >>> c2(3, c('b', 6))
    17
    """
    dict = {}
    def counter(key, value):
        if key in dict:
            dict[key].append(value)
        else:
            dict[key] = [value]
        lst = dict[key]
        total = 0
        length = len(lst)
        for index in range(length):
            total = total + lst[index]
        return total
    return counter
    """-m doctest gives some weird failure despite all above test case pass"""

Can we improve this solution?

Choose another name for the dictionary besides dict.  Don't ever hide the name of a built-in type.

Then...

d = {}
def counter(key,value):
    d[key] = d.get(key,0) + value
    return d[key]

You're done.  I see no need to build lists of all the passed values.


Time Tracker Project I am working on a little Python project to track how much time I am spending coding and doing things outdoors. Coding subtracts and outdoors stuff adds.

My code is looking pretty bad. Any suggestions on how to make it better?

import tkinter as tk
import time
import json
import atexit
from flask import Flask

root = None
start_time = 0
end_time = 0
_add = False
_sub = False
entries_num = 0
_click = False
name = ''
_name = None
run_txt = None

def simplify(_time):
    return time.strftime('%H:%M:%S', time.localtime(_time))

def simple_mode(mode):
    if mode == 0:
        return "Add"
    elif mode == 1:
        return "Subtract"

def start():
    global start_time
    start_time = int(time.time())
    run_txt.configure(text="Running", fg="green")

def end():
    global end_time
    end_time = int(time.time())
    run_txt.configure(text="Not Running", fg="red")
    time_manager()

def find_mode():
    if _add and not _sub:
        return 0
    elif _sub and not _add:
        return 1

# 0 is add 1 is subtract
def time_manager():
    global entries_num
    entries_num += 1
    mode = simple_mode(find_mode())
    entry_raw = {'name': _name.get(), 'mode': mode, 'start': simplify(start_time), 'end': simplify(end_time), 'total_time': simplify(end_time-start_time-(-2211688800))}
    entries.update({str(entries_num): entry_raw})
    display()

def add():
    global _add
    global _sub
    _add = True
    _sub = False

def sub():
    global _add
    global _sub
    _add = False
    _sub = True

def display():
    height = 1
    width = 5
    cells = {}
    for i in range(height):  # Rows
        for j in range(width):  # Columns
            b = tk.Label(root, text="")
            b.grid(row=i + 16 + entries_num, column=j)
            cells[(i, j)] = b
    cells[(0, 0)]['text'] = entries[str(entries_num)]['name']
    cells[(0, 1)]['text'] = entries[str(entries_num)]['total_time']
    cells[(0, 2)]['text'] = entries[str(entries_num)]['mode']
    cells[(0, 3)]['text'] = entries[str(entries_num)]['start']
    cells[(0, 4)]['text'] = entries[str(entries_num)]['end']

class MainApplication(tk.Frame):
    def __init__(self, parent, *args, **kwargs):
        tk.Frame.__init__(self, parent, *args, **kwargs)
        self.parent = parent
        title_lbl = tk.Label(root)
        title_lbl['text'] = 'TimeTracker'
        title_lbl.configure(font=('Roboto', 20))
        title_lbl.grid(row=0, column=0, sticky='ew')
        srt_btn = tk.Button(root, text="Start", command=start, font=('Roboto', 8)).grid(row=10, column=0, sticky='sw')
        end_btn = tk.Button(root, text="End", command=end, font=('Roboto', 8)).grid(row=10, column=0, sticky='se')
        add_btn = tk.Button(root, text="Add Time", command=add, font=('Roboto', 8)).grid(row=11, column=0, sticky='se')
        sub_btn = tk.Button(root, text="Subtract Time", command=sub, font=('Roboto', 8))
        sub_btn.grid(row=12, column=0, sticky='se')
        global _name
        _name = tk.Entry(root, width=50)
        _name.grid(row=10, column=2)
        _name.insert(0, "Entry Name")
        _name.configure(state=tk.DISABLED)

        def on_click(event):
            _name.configure(state=tk.NORMAL)
            _name.delete(0, tk.END)

            # make the callback only work once
            _name.unbind('<Button-1>', on_click_id)

        on_click_id = _name.bind('<Button-1>', on_click)
        global run_txt
        run_txt = tk.Label(root)
        run_txt.configure(text="Not Running", fg="red")
        run_txt.grid(row=11, column=2)

def save():
    with open('data.json', 'w') as fb:
        _json = json.dumps(entries)
        fb.write(_json)

if __name__ == "__main__":
    root = tk.Tk()
    with open('data.json', 'r') as fr:
        text = fr.read()
        entries = json.loads(text)
        keys = entries.keys()
        for key in keys:
            entries_num = int(key)
            display()

    root.title("TimeTracker")
    root.minsize(200, 400)
    atexit.register(save)
    MainApplication(root).grid()
    root.mainloop()

The way you're handling add "modes" is a little clunky. It seems that only one of _add or _sub should ever be true at the same time. By making these two separate variables, you need to make sure that invariant is maintained everywhere. You've definitely helped make sure it's safe by using the add and sub functions, but those are band-aids on an awkward design.

The easiest way to simplify this is just to have one variable. Something like:

_is_adding = True

If this is true you're adding, if it's not, you're subtracting. Now find_mode can be much simpler:

def find_mode():
    return 0 if _is_adding else 1

And add and sub simply becomes:

def add():
    global is_adding
    is_adding = True

def sub():
    global is_adding
    is_adding = False

Although,

I think you're overusing globals here. In some cases, like UI callbacks, they may be the cleaner (but not only) option. find_mode for example though itself does not need to rely on globals. If you can cleanly pass data in instead of relying on globals, you should. I'd change it to:

def mode_number(is_adding: bool) -> int:  # I added some type hints in and renamed it
    return 0 if is_adding else 1

. . .

mode = simple_mode(mode_number(_is_adding))

The gain here is incredibly small because of how simple the functions involved are. In general though, it's a good habit to get into. Functions that explicitly take as arguments all the data they need, and return all the data that they produce are much easier to understand.

simple_mode and find_mode are kind of odd though if you think about them. Why translate add/sub into a number just to then translate it back to "Add"/"Subtract"? I think this makes more sense:

def format_mode(is_adding: bool) -> str:
    return "Add" if is_adding else "Subtracting"

. . .

mode = format_mode(_is_adding)

I think simplify is a bad name for a function. What's it "simplifying"? If I'm writing a function that turns something into a string, I usually call it format_* (although that's just a habit of mine). Give some description though. I'd probably call it format_time, and adding some simple type hints make the types clearer:

def format_time(_time: int) -> str:
    return time.strftime('%H:%M:%S', time.localtime(_time))

I'm not sure I 100% agree with calling the parameter _time to avoid the name collision, since a prefix _ typically means a "private" attribute, but I can't offhand think of a better name either, since the function is pretty broad in its purpose.

end_time-start_time-(-2211688800))

What the heck is -2211688800? That seems to offset the date by 70.1 years. Whatever it's purpose I see two problems:

If that's meant to offset today's date to reach a certain format, it'll be outdated from the second it's written. You should see if you can automate the creation of that number.
Magic numbers, especially large unusual like that should be named. Something like:

TIME_OFFSET = 2211688800

. . .

end_time - start_time - (-TIME_OFFSET)

Also, x - (-y) is the same as x + y (unless x and y are some custom type with odd math laws). It should just be:

end_time - start_time + TIME_OFFSET

The Flask import seems to be unneeded.


Simple Go TCP server and client I'm a Go (and programming in general) newbie and the task was to program a Go server that would accept incoming messages, take each line, reverse its order and case and send it back. Besides doing that I also wrote a client (just for the hell of it and to help me test my server). I wonder if some things could've been done better and just want feedback on my code.

I'm particularly interested in proper usage of those break statements I used to forcefully finish Scan() loops and if I really have to send end of line symbols from my client as a hardcoded measure (rather than use some other function that automatically adds end of line symbols when reading input).

Server:

func reverse(s string) string {
    runes := []rune(s)
    for i, j := 0, len(runes)-1; i < j; i, j = i+1, j-1 {
        runes[i], runes[j] = runes[j], runes[i]
    }
    return string(runes)
}

func swapCase(s string) string {
    return strings.Map(func(r rune) rune {
        switch {
        case unicode.IsLower(r):
            return unicode.ToUpper(r)
        case unicode.IsUpper(r):
            return unicode.ToLower(r)
        }
        return r
    }, s)
}

func handleServerConnection(c net.Conn, i int) {
    for {
        // scan message
        scanner := bufio.NewScanner(c)
        for scanner.Scan() {
            msg := scanner.Text()
            log.Printf("Client %v sends: %v", i, msg)
            msgNew := swapCase(reverse((msg)))
            c.Write([]byte(msgNew + "\n"))
            log.Printf("Client %v receives: %v", i, msgNew)
            fmt.Println("---")
        }
        if errRead := scanner.Err(); errRead != nil {
            log.Printf("Client %v disconnected...", i)
            fmt.Println("---")
            return
        }
    }
}

func main() {
    log.Println("Server launched...")
    // listen on all interfaces
    ln, _ := net.Listen("tcp", ":9999")
    i := 0
    for {
        // accept connection on port
        c, _ := ln.Accept()
        i++
        log.Printf("Client %v connected...", i)
        fmt.Println("---")
        // handle the connection
        go handleServerConnection(c, i)
    }
}

Client:

func main() {
    i := 0 // connection index
    // connect to server
    for {
    connect:
        c, errConn := net.Dial("tcp", "127.0.0.1:9999")
        if errConn != nil {
            continue
        } else {
            i++
            if i <= 1 {
                log.Println("Connected to server...")
                fmt.Println("---")
            } else {
                log.Println("Reconnected to server...")
                fmt.Println("---")
            }
        }
        for {
            // read in input from stdin
            scannerStdin := bufio.NewScanner(os.Stdin)
            fmt.Print("Server message: ")
            for scannerStdin.Scan() {
                text := scannerStdin.Text()
                fmt.Println("---")
                // send to server
                _, errWrite := fmt.Fprintf(c, text+"\n")
                if errWrite != nil {
                    log.Println("Server offline, attempting to reconnect...")
                    goto connect
                }
                log.Print("Server receives: " + text)
                break
            }
            // listen for reply
            scannerConn := bufio.NewScanner(c)
            for scannerConn.Scan() {
                log.Println("Server sends: " + scannerConn.Text())
                break
            }
            if errReadConn := scannerStdin.Err(); errReadConn != nil {
                log.Printf("Read error: %T %+v", errReadConn, errReadConn)
                return
            }
            fmt.Println("---")
        }
    }
}

Your reverse, and swapCase functions are good. The native use of runes is great, and the functions are clear, etc. If I was to really nit-pick, I would suggest a real function (instead of an anonymous) for the switch statement.

func swapRuneCase(r rune) rune {
    switch {
    case unicode.IsLower(r):
        return unicode.ToUpper(r)
    case unicode.IsUpper(r):
        return unicode.ToLower(r)
    }
    return r
}

That makes it clearer, and potentially, later, reusable.

It also makes the swapCase function look like:

func swapCase(s string) string {
    return strings.Map(swapRuneCase, s)
}

Interestingly, it also leads to a more rune-based server handler function...

Here's your function:

func handleServerConnection(c net.Conn, i int) {
    for {
        // scan message
        scanner := bufio.NewScanner(c)
        for scanner.Scan() {
            msg := scanner.Text()
            log.Printf("Client %v sends: %v", i, msg)
            msgNew := swapCase(reverse((msg)))
            c.Write([]byte(msgNew + "\n"))
            log.Printf("Client %v receives: %v", i, msgNew)
            fmt.Println("---")
        }
        if errRead := scanner.Err(); errRead != nil {
            log.Printf("Client %v disconnected...", i)
            fmt.Println("---")
            return
        }
    }
}

That function is doing too many things .... hmmm actually it's just got a useless outer loop. The "scan" loop is all you need. The scanner.Scan() will only terminate the loop when the connection is closed (the c Reader returns an error - presumably EOF, but any other error has the same consequence... c is not readable). So, remove the outer loop, and, we know that the scanner will return an error too, so the condition at the end is not useful. This leaves:

func handleServerConnection(c net.Conn, i int) {
    // scan message
    scanner := bufio.NewScanner(c)
    for scanner.Scan() {
        msg := scanner.Text()
        log.Printf("Client %v sends: %v", i, msg)
        msgNew := swapCase(reverse((msg)))
        c.Write([]byte(msgNew + "\n"))
        log.Printf("Client %v receives: %v", i, msgNew)
        fmt.Println("---")
    }
    log.Printf("Client %v disconnected...", i)
    fmt.Println("---")
}

Note that the fiddling you do with the newline is necessary because you can't "reverse" it too.

Your mixing of both log and fmt in things like log.Printf and fmt.Println is confusing. Use one of them, not both. log is probably the better option. Also, you should be adding \n to the fmt.Printf format strings (you don't need to do that for log.Printf - but you can if you want). There's too much logging! You're not inspecting the error value for Write(...), and you're not closing the Conn which will lead to "CLOSE_WAIT" sockets in the OS network stack. In general, though. I would rationalize it all down to:

func handleServerConnection(c net.Conn, i int) {
    defer func() {
        if err := c.Close(); err != nil {
            log.Printf("Closing client %v connection returned error: %v\n", i, err)
        }
    }()

    // scan message
    scanner := bufio.NewScanner(c)
    for scanner.Scan() {
        msg := scanner.Text()
        msgNew := swapCase(reverse((msg)))
        if _, err := c.Write([]byte(msgNew + "\n")); err != nil {
            log.Printf("Client %v has closed writer: %v\n", i, err)
            break
        }
        log.Printf("Client %v\n received: %v\n returned: %v\n", i, msg, msgNew)
    }
    log.Printf("Client %v disconnected...\n", i)
}

Right, that's it for the handler function.

Your main function is structurally OK, but you MUST check the error state from the Listen call:

ln, _ := net.Listen("tcp", ":9999")

By ignoring the error, if you start your program twice, the second program will panic with a nil pointer reference on the ln.Accept() call (since ln will be nil)

That Accept() call should also have it's error checked too!

Again, using both log and fmt is confusing.

In general, you should specialize your network services/processes when you can. You use net.Listen and ln.Accept when you should probably use ListenTCP and AcceptTCP (you get a TCPListener instead of a Listener) - and your connection is a TCPConn and not a Conn (you should change the parameter type on the handler function too).

Your client side has similar issues, but I think you can figure them out as you go based on the above.


Implementation of partitionM I needed a monadic version of partition today. I settled on the following version:

partitionM :: Monad m => (a -> m Bool) -> [a] -> m ([a], [a])
partitionM p xs = bimap (map fst) (map fst) . partition snd . zip xs <$> mapM p xs

Then I realized it's already implemented here:

partitionM :: Monad m => (a -> m Bool) -> [a] -> m ([a], [a])
partitionM f [] = return ([], [])
partitionM f (x:xs) = do
    res <- f x
    (as,bs) <- partitionM f xs
    return ([x | res]++as, [x | not res]++bs)

This second version doesn't have to pull in extra packages and doesn't create intermediate lists, but mine is shorter and doesn't have explicit recursion. What do you think of my solution in comparison with the library's implementation?

This second version doesn't have to pull in extra packages…

Data.Bifunctor is part of base since 4.8.0.0, so I'm not sure where you get the "extra packages" from. Yes, Data.Bifunctor was originally in the bifunctors package, but it was transferred into base in 2015.

However, compared to the extras version, your variant is easy to read, as the [x | res] … [x | not res] slightly obfuscates the result. I'd probably bind map fst, but that's personal preference.

  and doesn't create intermediate lists

It generates a lot of intermediate lists. I haven't checked yet, but as [x|res]++as depends on res cannot get simplified to x:as or as at compile time by rules, so it might create a thunk […]++as. In the end, you have \$\mathcal O(n) \$ additional \$\mathcal O(1)\$-sized lists.

Since this is a hidden implementation detail, I don't really like Neil's solution too much. Something along

partitionM :: Monad m => (a -> m Bool) -> [a] -> m ([a], [a])
partitionM f [] = return ([], [])
partitionM f (x:xs) = do
    res <- f x
    (as,bs) <- partitionM f xs
    return $ if res then (x : as,     bs)
                    else (    as, x : bs)

would at least support the optimizer and get rid of those intermediate lists, but is even more verbose. Your variant uses an additional list, sure, but it's easier to read and to understand IMHO.


Constructing polygons from the edges in a Voronoi diagram I am working on a Java program which creates a map using Voronoi. I am using a Java library which generates Voronoi and it is very fast.

The problem I am facing is that, then, I have to scan every Voronoi edge to know what point is on the left and on the right of the edge to create the polygon which contains each point.

This is the class which contains every Voronoi edge:

public class GraphEdge
{
    public double x1, y1, x2, y2;

    public int site1;
    public int site2;
}

The coordinates x1, y1, x2, y2 are the edge start and end coordinates and site1 and site2 are the indexes of the points which are on the left and on the right of the edge. So, to create the polygon which contains every point I do this:

for(int n = 0; n < xValues.length; ++n){
        polygonsList.add(new GPolygon());
        for(GraphEdge mGraphEdge : edgesList){
            if( (xValues[mGraphEdge.site1] == xValues[n] || xValues[mGraphEdge.site2] == xValues[n])
                && (yValues[mGraphEdge.site1] == yValues[n] || yValues[mGraphEdge.site2] == yValues[n]) ){
                polygonsList.get(n).addPoint((int)mGraphEdge.x1, (int)mGraphEdge.y1);
                polygonsList.get(n).addPoint((int)mGraphEdge.x2, (int)mGraphEdge.y2);
            }
        }
    }

Where xValues and yValues are the points coordinates from which I generate the Voronoi diagram and GPolygon is a Polygon class I created which extends from java.awt.Polygon.

These are the times I measured:

Voronoi Time: 283 ms (time to generate Voronoi diagram)
Polygon Search Time: 34589 ms (time to complete the for loop which generates the polygons)
Polygon Fill Time: 390 ms (time to fill the polygons and save to image, which is optional)
Points quantity: 26527 (number of points from which Voronoi is generated)
Map Generation Finished
Polygon quantity: 26527 (number of polygons, one for each point)

As you can see, the time is really significant compared to the others. How can I speed up the for loop? What other alternatives do I have?

To improve the speed of your code, you'd really have to profile it, and know what the bottleneck is (use a profiler, and find out in what methods most of the time is spent).

Here's what I would have a look at (no promise it will yield significant improvements).

polygonsList.get(n) is evaluated twice per iteration.
The polygon class you're using (I assume java.awt.Polygon) takes ints as coordinates, while the library calculates them in doubles. Yo may want to consider writing a custom Polygon class that takes doubles, in order to avoid the casts.
if you only need the Polygon class for drawing, you may even forego that entirely and make a class that can draw a line given a GraphEdge. This will avoid creating Polygon objects, and you can reuse the same drawing instance.


BouncyCastle Rijndael 256 Implementation I modified the implementation from this website to include a salt:

package de.xxx.yyy.main;

import static org.junit.Assert.*;

import org.bouncycastle.crypto.BlockCipher;
import org.bouncycastle.crypto.BufferedBlockCipher;
import org.bouncycastle.crypto.DataLengthException;
import org.bouncycastle.crypto.InvalidCipherTextException;
import org.bouncycastle.crypto.PBEParametersGenerator;
import org.bouncycastle.crypto.digests.SHA3Digest;
import org.bouncycastle.crypto.engines.RijndaelEngine;
import org.bouncycastle.crypto.generators.PKCS12ParametersGenerator;
import org.bouncycastle.crypto.modes.CBCBlockCipher;
import org.bouncycastle.crypto.paddings.PKCS7Padding;
import org.bouncycastle.crypto.paddings.PaddedBufferedBlockCipher;
import org.bouncycastle.crypto.params.ParametersWithIV;
import org.bouncycastle.util.encoders.Base64;
import org.junit.Test;

public class test {

    private char[] password = "0123456789abcdef0123456789abcdef".toCharArray();
    private byte[] salt = "0123456789".getBytes();
    private int iterationCount = 5;

    @Test
    public void testEncryptRijndael() throws DataLengthException, IllegalStateException, InvalidCipherTextException {

        PKCS12ParametersGenerator pGen = new PKCS12ParametersGenerator(new SHA3Digest(256));
        char[] passwordChars = password;

        final byte[] pkcs12PasswordBytes = PBEParametersGenerator.PKCS12PasswordToBytes(passwordChars);
        pGen.init(pkcs12PasswordBytes, salt , iterationCount );

        BlockCipher engine = new RijndaelEngine(256);
        CBCBlockCipher cbc = new CBCBlockCipher(engine);
        BufferedBlockCipher cipher = new PaddedBufferedBlockCipher(cbc, new PKCS7Padding());

        ParametersWithIV aesCBCParams = (ParametersWithIV) pGen.generateDerivedParameters(256, 256);
        cipher.init(true, aesCBCParams);

        byte[] input = "Hallo ich bin ein Test".getBytes();
        byte[] cipherText = new byte[cipher.getOutputSize(input.length)];

        int cipherLength = cipher.processBytes(input, 0, input.length, cipherText, 0);
        cipher.doFinal(cipherText, cipherLength);

        String result = new String(Base64.encode(cipherText));
        System.out.println("testEncryptRijndael result : " + result);
        assertEquals("cMoMSNMNsikAkLjaheE6iD48Xkfvo7Y6gS8/zroGfHc=",result);
    }

    @Test
    public void testDecryptRijndael() throws DataLengthException, IllegalStateException, InvalidCipherTextException {
        PKCS12ParametersGenerator pGen = new PKCS12ParametersGenerator(new SHA3Digest(256));
        char[] passwordChars = password;

        final byte[] pkcs12PasswordBytes = PBEParametersGenerator.PKCS12PasswordToBytes(passwordChars);
        pGen.init(pkcs12PasswordBytes, salt , iterationCount );

        BlockCipher engine = new RijndaelEngine(256);
        CBCBlockCipher cbc = new CBCBlockCipher(engine);
        BufferedBlockCipher cipher = new PaddedBufferedBlockCipher(cbc, new PKCS7Padding());

        ParametersWithIV aesCBCParams = (ParametersWithIV) pGen.generateDerivedParameters(256, 256);
        cipher.init(false, aesCBCParams);

        byte[] output = Base64.decode("cMoMSNMNsikAkLjaheE6iD48Xkfvo7Y6gS8/zroGfHc=".getBytes());
        byte[] cipherText = new byte[cipher.getOutputSize(output.length)];

        int cipherLength = cipher.processBytes(output, 0, output.length, cipherText, 0);
        int outputLength = cipher.doFinal(cipherText, cipherLength);
        outputLength += cipherLength;

        byte[] resultBytes = cipherText;
        if (outputLength != output.length) {
            resultBytes = new byte[outputLength];
            System.arraycopy(
                    cipherText, 0,
                    resultBytes, 0,
                    outputLength
                    );
        }

        String result = new String(resultBytes);
        System.out.println("testDecryptRijndael result : " + result);
        assertEquals("Hallo ich bin ein Test", result);
    }
}

Is this a good implementation, or are there some mistakes, logical errors or security risks?

It's good that you used a more interesting test string instead of "value" and that in testEncryptRijndael you replaced assertNotNull with a more strict assertEquals.

Since your tests are different from the originals, it would be better to rename them to testEncryptRijndaelWithSalt and testDecryptRijndaelWithSalt, respectively. And keep the originals too.

In both the original tests and yours I don't see the point of the logging and printing statements. I think you can drop that. They do no harm, but they might invite the bad habit of reading test outputs instead of focusing on adding proper assertions.

There's a lot of duplication in the encryption and decryption cases. Perhaps you could move that logic to a common private helper method. That can reduce possible typing errors.

I don't see the point of this if block:

 if (outputLength != output.length) {
     // ...
 }

The thing is, in a unit test everything should be predictable: you should already know at this point the outcome of that condition. If outputLength should be equal to output.length, then drop the if but keep the block of code. Otherwise delete the whole thing. You can add an assertion for the condition as a sanity check.


Find the missing link Thee million men write down their name and the person to their left and the person on the far left throws out his paper.  Reconstruct the list.

A List takes a while. Don't try 3 million unless you have some time.

Up front we do NOT know the remaining man on left is 0.  That is what we are looking for. The remaining here is 0 as the was test data easy to create.

public static void TAOCPthreeMillionMenTest()
{
    int numberOfMen = 30000;  // test at less than 3 million or get killed
    List<PairInt> pairs = new List<PairInt>(numberOfMen);
    for (int i = 0; i < numberOfMen; i++)
    {
        pairs.Add(new PairInt(i, i + 1));
    }
    // shuffle left
    Random rand = new Random();
    int temp;
    int j;
    //shuffl up
    for (int i = numberOfMen - 1; i > 0; i--)
    {
        j = rand.Next(i + 1);
        if (i != j)
        {
            temp = pairs[i].Rght;
            pairs[i].Rght = pairs[j].Rght;
            pairs[j].Rght = temp;
            temp = pairs[i].Left;
            pairs[i].Left = pairs[j].Left;
            pairs[j].Left = temp;
        }
    }
    foreach (PairInt pair in pairs)
    {
        //Debug.WriteLine(pair.Left + " " + pair.Rght);
        if (pair.Left == pair.Rght)
            Debug.WriteLine("pair.Left == pair.Rght");
    }
    Debug.WriteLine("");
    Debug.WriteLine(TAOCPthreeMillionManLeft(pairs));
    Debug.WriteLine("");
}
public static int TAOCPthreeMillionManLeft(List<PairInt> pairs)
{
    List<PairInt> lineEmUp = new List<PairInt>();
    int lineEmUpCountMax = 0;
    int lineEmUpCountMaxCount = 0;
    PairInt matchRght;
    PairInt matchLeft;
    int count = 0;
    foreach (PairInt pair in pairs)
    {
        count++;
        //Debug.WriteLine("new   " + pair.Left + " " + pair.Rght);
        //foreach (PairInt exist in lineEmUp)
        //{
        //    Debug.WriteLine("exist " + exist.Left + " " + exist.Rght);
        //}
        //Debug.WriteLine("");
        matchRght = lineEmUp.FirstOrDefault(x => x.Rght == pair.Left);
        matchLeft = lineEmUp.FirstOrDefault(x => x.Left == pair.Rght);
        if (matchRght != null)
        {
            if (matchLeft != null)
            {
                matchRght.Rght = matchLeft.Rght;
                lineEmUp.Remove(matchLeft);
            }
            else
            {
                matchRght.Rght = pair.Rght;
            }
        }
        else if (matchLeft != null)
        {
            matchLeft.Left = pair.Left;
        }
        else
        {
            lineEmUp.Add(pair);
            if (lineEmUpCountMax < lineEmUp.Count)
            {
                lineEmUpCountMax = lineEmUp.Count;
                lineEmUpCountMaxCount = count;
            }
            //Debug.WriteLine("lineEmUp.Count " + lineEmUp.Count);
        }
    }
    // lineEmUp should have only one entry
    if (lineEmUp.Count != 1)
    {
        Debug.WriteLine("");
        Debug.WriteLine("lineEmUp.Count != 1");
        foreach (PairInt pair in lineEmUp)
        {
            Debug.WriteLine(pair.Left + " " + pair.Rght);
        }
        Debug.WriteLine("");
    }
    return lineEmUp[0].Left;
}

public class PairInt
{
    public int Left { get; set; }
    public int Rght { get; set; }
    public PairInt(int left, int rght)
    {
        Left = left;
        Rght = rght;
    }
}

I tried a double dictionary but could not make it work.

Not sure if I get the problem, but one solution for reconstructing the original list (even 3.000.000 items) with a dictionary is:

public static void ReconstructList(List<PairInt> pairs)
{
    var leftRightDict = pairs.ToDictionary(p => p.Left, p => p.Rght);
    var idx = 0;
    while (leftRightDict.TryGetValue(idx, out idx))
        Console.WriteLine(idx);
}

Update:

The following code gets the ID of the left-most men:

public static void ReconstructList(List<PairInt> pairs)
{
    var rightLefttDict = pairs.ToDictionary(p => p.Rght, p => p.Left);
    // just start with any men
    var right = pairs[0].Rght;
    while (rightLefttDict.TryGetValue(right, out right)) {}
    Console.WriteLine(right);
}


improvement of a database register function Here is a function in a django rest api, that takes data in json and registers a category for a given website. I'd like to merge two try...except parts that check 'site_id' and 'category_id' but I am not sure how to do it. Also any other suggestion to make it pythonic is appreciated:

@myql_view(['POST'])
def register_category(request):

    try:
        site_id = request.data['site_id']
    except Exception as e:
        print('Request error or missing site_id:', e)
        return Response(status=status.HTTP_400_BAD_REQUEST)

    try:
        category_id = request.data['category_id']
    except Exception as e:
        print('Request error or missing category_id', e)
        return Response(status=status.HTTP_400_BAD_REQUEST)

    cursor = connection.cursor()
    try:
        cursor.execute('INSERT INTO productCategory (site_id, category_id) VALUES (%s, %s)',
                       [site_id, category_id])
    except Exception as e:
        print('Could not register product', e)
        return Response(status=status.HTTP_409_CONFLICT)
    finally:
        cursor.close()

    return Response(status=status.HTTP_200_OK)

You can actually merge them pretty easily. They're very similar, with just a minor difference in the strings that determine what key failed. But luckily you can get the bad key by using the message attribute of the exception:]

try:
    site_id = request.data['site_id']
    category_id = request.data['category_id']
except Exception as e:
    print('Request error or missing {}:'.format(e.message), e)
    return Response(status=status.HTTP_400_BAD_REQUEST)

Now they both work the same except that the missing key is taken out of your exception e and formatted into the message you print for the user.

I also think you should except KeyError, not just any old Exception. Using the latter would mask typos, while the former deals specifically with the typeof exception you expect to appear.


Scheme function to duplicate every element of a list a specified number of times This code takes a list and a integer and duplicates every element in the list the specified number of times.

(define (super-duper source count)
        (define (next-super source left)
                (if (zero? left)
                        (super-duper (cdr source) count)
                        (cons (super-duper (car source) count) (next-super source (- left 1)))))

        (if (pair? source)
                (cons (super-duper (car source) count) (next-super source (- count 1)))
                source))

(display(super-duper '((x y) t) 3))

I think you need to decide weather to repeat only on the first level of the list, or if you want to do a tree walk for lists inside lists. As is, this code produces a weird combination of both in an exponential explosion of the list, The following sample returns over 2 million leaf elements for an input of length 26 and a repeat of 10.

(leaf-count (super-duper '(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) 10))
->> 2576420

(define (leaf-count Tree)
  (cond ((null? Tree) 0)
        ((not (pair? Tree)) 1)
        (else (+ (leaf-count (car Tree))
                 (leaf-count (cdr Tree))))))


Is there any memory leak for the implementation of array of char arrays Basically, I declared a char array to hold a maximum size of 100 char arrays.

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

int add_to_urls(char **urls, char *url) {
    for (int i = 0; i < 100; i++) {
        if (urls[i] == NULL) {
            urls[i] = malloc(sizeof(strlen(url) + 1));
            strcpy(urls[i], url);
            break;
        } else {
            //make sure we don't add same site twice
            if (strcmp(urls[i], url) == 0) {
                printf("%s already in list\n", urls[i]);
            }
        }
    }
    return 0;
}

void free_urls(char **urls) {
    for (int i = 0; i < 100; i++) {
        if (urls[i] != NULL) {
            free(urls[i]);
        } else break;
    }
}

int main() {
    char *urls[100] = {NULL};

    char *url = "www.youtube.com";
    add_to_urls(urls, url);

    url = "www.reddit.com";
    add_to_urls(urls, url);

    url = "www.reddit.com";
    add_to_urls(urls, url);

    printf("URL: %s\n", urls[0]);
    printf("URL: %s\n", urls[1]);

    free_urls(urls);

    return 0;
}

I'm not sure if there's any conventions of making such an array container, as I am learning how to use c. The priority is to check if there's any memory leak, what I did is iteratively release the urls[i] and urls itself will be automatically handled (I suspect).

The second thing is about improvements, what do you think might be inappropriate causing any potential troubles?

You made an error here:
        urls[i] = malloc(sizeof(strlen(url) + 1));

strlen(url) + 1 is of type size_t, so you will allocate enough space for that, which could well be less than the amount you actually wanted (strlen(url) + 1).
If we fix that bug, and also change url to const char* (because it points at string literals, which are always const), then we get a clean Valgrind run from the test program.

What does the return value from add_to_urls indicate?  If you meant it to be true when the url was added, then the break should be replaced with return 1.
Always check the result of malloc() is not null.
Errors and warnings should be printed to the standard error stream, not to stdout.
Your main() doesn't check that we didn't add a duplicate.  In fact we did, but we never printed it.
Avoid writing constants (such as 100) in separate places if they need to agree with each other.
Instead of the break in free_urls(), we can make the test part of the loop's condition.

Improved code
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int add_to_urls(char **urls, const char *url, size_t max_list)
{
    for (size_t i = 0;  i < max_list;  i++) {
        if (!urls[i]) {
            urls[i] = malloc(strlen(url) + 1);
            if (!urls[i]) {
                fprintf(stderr, "Failed to alloc for %s\n", url);
                return 0; /* failed allocation */
            }
            strcpy(urls[i], url);
            return 1;           /* we added it */
        }
        //make sure we don't add same site twice
        if (strcmp(urls[i], url) == 0) {
            fprintf(stderr, "%s already in list\n", url);
            return 0;           /* already present */
        }
    }
    return 0; /* ran out of room */
}

void free_urls(char **urls, size_t max_list) {
    for (size_t i = 0;  i < max_list && urls[i];  i++) {
        free(urls[i]);
    }
}

int main() {
    char *urls[100] = {NULL};
    const size_t url_len = sizeof urls / sizeof *urls;

    add_to_urls(urls, "www.youtube.com", url_len);
    add_to_urls(urls, "www.reddit.com", url_len);
    add_to_urls(urls, "www.reddit.com", url_len);

    for (size_t i = 0;  i < url_len && urls[i];  i++) {
        printf("URL[%zd]: %s\n", i, urls[i]);
    }

    free_urls(urls, url_len);
}


Matching a type T to a list of case statements I have a compile-time switch_ template that matches a given type T to a list of case_ statements (of which default_ is a special case, no pun intended). A case<Key, Value is two-parameter class for which its Key is matched against the class T using a match<T, Key> variable template, which is a modified std::is_same_v.

#include <cstdint>      // uint8_t, uint16_t, uint32_t, uint64_t
#include <type_traits>  // conditional_t, is_same_v, true_type, false_type

namespace meta {

template<auto N>
using int_c = std::integral_constant<decltype(N), N>;

struct true_;
struct false_;

template<class Key, class Value>
struct case_;

template<class Value>
using default_ = case_<true_, Value>;

template<class T, class Key>
constexpr auto match = std::is_same_v<T, Key>;

template<class T> constexpr auto match<T, true_> = true;
template<class T> constexpr auto match<true_, T> = true;

template<class T> constexpr auto match<T, false_> = false;
template<class T> constexpr auto match<false_, T> = false;

template<class T, class... Cases>
struct switch_;

template<class T, class... Cases>
using switch_t = typename switch_<T, Cases...>::type;

template<class T, class Key, class Value>
struct switch_<T, case_<Key, Value>>
:
        std::conditional<match<T, Key>, Value, false_>
{};

template<class T, class Key, class Value, class _, class Else>
struct switch_<T, case_<Key, Value>, case_<_, Else>>
:
        std::conditional<match<T, Key>, Value, Else>
{};

template<class T, class Key, class Value, class Head, class... Tails>
struct switch_<T, case_<Key, Value>, Head, Tails...>
:
        std::conditional<match<T, Key>, Value, switch_t<T, Head, Tails...>>
        //                                     ^^^^^^^^ <= can this recursion be avoided?
{};

}   // namespace meta

using namespace meta;

template<auto N>
using unsigned_ = switch_t<         // will return associated type of first match, case_ has "break" semantics
    int_c<N>,
    case_<int_c<8>, uint8_t>,
    case_<int_c<16>, uint16_t>,
    case_<int_c<32>, uint32_t>,
    case_<int_c<64>, uint64_t>,
    default_<unsigned>              // will always match, so put this last!
>;

int main()
{
    static_assert(std::is_same_v<unsigned_<  8>, uint8_t>);
    static_assert(std::is_same_v<unsigned_< 16>, uint16_t>);
    static_assert(std::is_same_v<unsigned_< 32>, uint32_t>);
    static_assert(std::is_same_v<unsigned_< 64>, uint64_t>);
    static_assert(std::is_same_v<unsigned_<128>, unsigned>);
}

Live Example using gcc 7.1 in C++17 mode.

Question: Please comment on the clarity of the code. In particular, I am curious if the compile-time recursion over the variadic parameter pack can somehow be avoided by mapping it to tuple or integer_sequence land where there are efficient compiler-intrinsics that pay off for long lists.

UPDATE: See this link for the updated code with changes that reflects the accepted answer.

I don't think returning the artificial false_ in case of no match is a good idea. Just return nothing, and you won't need false_ at all. Replace

: std::conditional<match<T, Key>, Value, false_>

with

: std::enable_if<match<T, Key>, Value>

It's interesting that while you support an explicit default-case, the last case is always treated as the default case anyway, if it isn't the only case. Oops?

template<class T, class Key, class Value, class _, class Else>
struct switch_<T, case_<Key, Value>, case_<_, Else>>
:
        std::conditional<match<T, Key>, Value, Else>
{};

By the way, if you restrict the default-case to being the last case, which you should consider doing anyway, you can replace match with std::is_same and some changes to switch_, making default_ its own class-template. Might be worth it.

What I failed to find is any way to avoid the recursion, sorry. I hope you can make good use of my feedback anyway.

Modified code:

namespace meta {

// Only for the example-program:
template<auto N>
using int_c = std::integral_constant<decltype(N), N>;

template<class Key, class Value>
struct case_;

template<class Value>
struct default_;

template<class T, class... Cases>
struct switch_
{
    static_assert(!sizeof...(Cases), "this meta::switch_ is malformed");
}

template<class T, class Value>
struct switch_<T, default_<Value>>
: std::enable_if<true, Value>
{}

template<class T, class Key, class Value, class... Tail>
struct switch_<T, case_<Key, Value>, Tail...>
: std::conditional_t<std::is_same<T, Key>(),
    std::enable_if<true, Value>,
    switch_<T, Tail...>>
{}

template<class T, class... Cases>
using switch_t = typename switch_<T, Cases...>::type;

}   // namespace meta


Slice a list or NumPy array into consecutive tuples My function needs two params(a,b) and if...break, which seems redundant. Can it be made prettier?

def slice_list(lst,num):
    """
    Given a list or Numpy array，
    get consecutive tuples based on specific window.
    """
    rst = []
    for i,_ in enumerate(lst):
        a = i
        b = i + num
        if len(lst[a:b])<num:
            break
        rst.append(tuple(lst[a:b]))
    return rst

Example:

>>> lst = np.array([10, 11, 12, 13, 26, 28])
>>> slice_list(lst,4)
[(10, 11, 12, 13), (11, 12, 13, 26), (12, 13, 26, 28)]

I believe that what you are looking for is already available as an itertools recipe; even though pairwise only allow you to return couples and not tuple of arbitrary length.

You will need to modify it so that:

tee will return num iterators;
you advance each of these iterators by one more element than the previous one (see the consume recipe for that).

This can lead to the following code:

import itertools

def advance(iterator, step):
    next(itertools.islice(iterator, step, step), None)

def tuplewize(iterable, size):
    iterators = itertools.tee(iterable, size)
    for position, iterator in enumerate(iterators):
        advance(iterator, position)
    return zip(*iterators)

Usage being:

>>> for t in tuplewize(lst, 4):
...   print(t)
...
(10, 11, 12, 13)
(11, 12, 13, 26)
(12, 13, 26, 28)

However, you are using numpy so we may come up with a better numpy approach:

numpy.roll allows us to advance the nth element on top of the list;
numpy.stack allows us to concatenate the rolled arrays into a single 2D array;
numpy.transpose allows us to convert a "list of lists" into a "list of tuples".

Full code being:

import numpy as np

def tuplewize(array, size):
    if size < 2:
        return np.array([array])

    stack = np.stack([np.roll(array, -i) for i in range(size)])
    return np.transpose(stack)[:-size+1]

Usage being:

>>> tuplewise(lst, 4)
array([[10, 11, 12, 13],
       [11, 12, 13, 26],
       [12, 13, 26, 28]])

And as @Peilonrayz indicated in a comment, a third possibility is to use the more_itertools package and its windowed function which has extended capabilities; and as such more overhead, so depending on your needs it may or may not suit you.


Resize renderer on browser window size change For a game, there is some WebGLRenderer object that wraps around canvas element. There is need to update it's size when browser window size changes. Here goes my current code written using BaconJS. What do you think about it ? Would you improve it in any way or maybe do it other way? I have read that listening to resize event is just slow for game purposes.

# Make the property of window object
rWindow = Bacon.constant window

# Sample window by tick event
sTickWindow = rWindow.sampledBy app.land.stream('tick')

# Look for width and height changes on every tick
sWidth = sTickWindow.map('.innerWidth').skipDuplicates()
sHeight = sTickWindow.map('.innerHeight').skipDuplicates()
sSize = Bacon.combineAsArray sWidth, sHeight

# Resize renderer when size is changed
Bacon.onValues rRenderer, sSize, (renderer, size) ->
    # Apply size update to renderer
    renderer.setSize size...

Note that app.land.stream('tick') is basically EventStream of requestAnimationFrame invocations.

Update

To make the understanding easier, I have made fiddle for it. Open dev console and try to resize the Result window rather then browser window.

I think your code style is fine. If you are happy with this method, I think you've written it pretty well.

As potential upgrades, here are two alternative methods that better obey the Rules of Clarity and Simplicity.

First, is there a reason you're using so much machinery to get the window size? This would be clearer with fewer moving parts. Anyone coming to your codebase knowing only JavaScript/CoffeeScript will understand what is happening here:

# setup canvas
canvas = document.createElement 'canvas'
document.body.appendChild canvas

# update ui every frame
tick = ->
  resize()
  requestAnimationFrame tick
requestAnimationFrame tick

resize = ->
  # only resize canvas if necessary
  canvas.width = window.innerWidth if canvas.width isnt window.innerWidth
  canvas.height = window.innerHeight if canvas.height isnt window.innerHeight

There's a fiddle of this here.

You could also employ modules that throttle redraws and polyfill requestAnimationFrame
like this example. I would argue that this method would also be clearer for a newcomer to your codebase.

raf = require 'raf' # requestAnimationFrame polyfill
throttle = require 'lodash.throttle'

# only redraw every 200 milliseconds
raf().on 'data', throttle(resize, 200)

See it in action (as JavaScript) on RequireBin here.


Could this QuickSort be made more consise and "nicer"? After seeing this example of a "primitive" QS implementation in F# by Scott W

let rec quicksort2 = function
   | [] -> []
   | first::rest ->
        let smaller,larger = List.partition ((>=) first) rest
        List.concat [quicksort2 smaller; [first]; quicksort2 larger]

He has an example in C# which is far longer, I figured that this approach should be able to be more closely modeled than his C# example so this was my best attempt:

private static IEnumerable<T> Quicksort<T>(IEnumerable<T> list)
            where T : IComparable<T>
        {
            if ((list == null) || !list.Any())
                return Enumerable.Empty<T>();

            var smallerAndLarger = list.Skip(1).GroupBy(n => n.CompareTo(list.First()));

            return Quicksort(smallerAndLarger.FirstOrDefault(x => x.Key < 0))
                   .Concat(new List<T>(new[] { list.First() }))
                   .Concat(Quicksort(smallerAndLarger.FirstOrDefault(x => x.Key >= 0)));
        }

Now depending on how you count my solution is "equally" short as the F# one. But it does have more noise, that I cannot argue with. I wonder, can it be made clearer and perhaps even shorter than this?

I'm more worried about the efficiency than conciseness.

Here are some sample inputs:

Equal new int[n]
Alternating Enumerable.Range(0, n).Select(x => x % 2).ToArray()
Sorted Enumerable.Range(0, n).ToArray()
Reverse sorted Enumerable.Range(0, n).Reverse().ToArray()

\begin{array}{ r | r | r | r | r }
 n & \text{Equal} & \text{Alternating} & \text{Sorted} & \text{Reverse sorted} \\
\hline
  10,000 & 7.9\text{s} & 1.8\text{s} & 7.8\text{s} & 7.7\text{s} \\
  100,000 & \text{N/A} & \text{N/A} & \text{N/A} & \text{OOM} \\
\end{array}

N/A is where I gave up waiting after a minute.

By comparison, Array.Sort on the same machine:

\begin{array}{ r | r | r | r | r }
 n & \text{Equal} & \text{Alternating} & \text{Sorted} & \text{Reverse sorted} \\
\hline
  100,000 & 4\text{ms} & 5\text{ms} & 2\text{ms} & 3\text{ms} \\
\end{array}

One advantage of QuickSort is that it can be done in-place, which is not happening here. If you're OK with allocating more memory, why not use Merge sort with its worst-case \$O(n \log n)\$ time guarantee?

Another issue is the choice of the pivot. Choosing the first element will give quadratic performance for sorted inputs. One option is to shuffle the input array before sorting; another is to choose the pivot at random (these are not the only options).

Robert Sedgewick* has a great page on QuickSort which touches on a lot of implementation issues. He also has a couple of implementations in Java, which translate easily to C#.

*Who did his Ph.D on QuickSort under Don Knuth, so well worth listening to :)


Binary search a sorted list of integers This code is meant to do a binary search of a sorted list containing integers.

I'm doing this to improve my style and to improve my knowledge of fundamental algorithms/data structures for an upcoming coding interview.

def bin_search(a, item):
    first = 0
    last = len(a) - 1
    found = False

    while first <= last and not found:
        mid = (first + last) // 2
        if a[mid] == item:
            found = True
        else:
            if item < a[mid]:
                last = mid - 1
            else:
                first = mid + 1

    return found

Looks pretty good to me.  Personally, I would get rid of the found variable and just return True if item is found.

I would also use a more descriptive name for the list.  a just doesn't say list to me.  If you want to keep it short, use something like l or borrow Haskell's convention and use something like as or xs.

Finally, if you wanted, you could use an if/elif/else instead of the if/else with nested if/else inside.  But I can't think of any compelling reason to do so.

As an answer to an interview question, I think you'll want to do a better job of indicating coverage of the case where the list is empty.  Your code covers this case perfectly (last will be -1 and first will be 0, so first <= last is False), but it's not obvious.  So it probably wouldn't hurt to add an explicit check or add a comment with a brief explanation why that case is covered.  Then there's no doubt that you considered that case.


Lazy Pirate Request Response pattern using zeromq and C#/.NET bindings I'm a zeromq newb and have implemented the REQ/RES Lazy Pirate pattern with C#. It's essentially a port from the C example given in the documentation. It's harnessed by tests, and seems to be work to my expectations. I'm interested to know if I'm understanding the pattern correctly, or if I can improve the API. If anyone knows of a pre-existing solution, I'd love to know too. Any advice would be great.

using System;
using System.Text;
using ZMQ;

namespace zeromq.net {
    public class LazyPirateClient : ReqRes {
        private readonly string _address;
        private readonly long _timeOut;
        private readonly int _retries;
        private bool _responseReceived;
        private int _attempt;

        public LazyPirateClient(string address, long timeOut, int retries) {
            _address = address;
            _timeOut = timeOut;
            _retries = retries;
        }

        public override void Send(Func<byte[]> byteProvider) {
            _responseReceived = false;
            using (var context = new Context(1)) {
                _attempt = 0;
                do {
                    using (var requester = context.Socket(SocketType.REQ)) {
                        _attempt++;
                        requester.Connect(_address);
                        requester.Send(byteProvider(), SendRecvOpt.NOBLOCK);
                        var item = requester.CreatePollItem(IOMultiPlex.POLLIN);
                        item.PollInHandler += item_PollInHandler;
                        context.Poll(new[] { item }, _timeOut);
                        requester.Linger = 0;
                    }
                } while (_attempt < _retries && !_responseReceived);

                if (!_responseReceived)
                     throw new PermenantFailException(_attempt);
            }
        }

        private void item_PollInHandler(Socket socket, IOMultiPlex revents) {
            _responseReceived = true;
            var bytes = socket.Recv(SendRecvOpt.NOBLOCK);
            OnReceived(new ResponseReceivedEventArgs(bytes, _attempt));
        }
    }

     public abstract class ReqRes {
        public abstract void Send(Func<byte[]> byteProvider);
        public delegate void ResponseReceived(object sender, ResponseReceivedEventArgs e);
        public event ResponseReceived ReceivedHandler;

        public void OnReceived(ResponseReceivedEventArgs e) {
            var handler = ReceivedHandler;
            if (handler != null) handler(this, e);
        }
     }

It seems zeromq will release sent data itself. With GC supported in C#, this feature is useless. The byteProvider parameter seems inappropriate, so why not just use IEnumerable<byte>?


Cropping and combining two images on server I've coded following two methods to combine images that are HTTP POSTed to the server:

// Crops two squares out of two separate images
// And then combines them into single image that's returned as byte[]
private byte[] combineImages(HttpPostedFileBase[] itemPayload)
{
    try
    {
        using (var ms = new MemoryStream())
        using (var bitmap = new Bitmap(800, 400, PixelFormat.Format24bppRgb))
        using (var img1 = Image.FromStream(itemPayload[0].InputStream))
        using (var img2 = Image.FromStream(itemPayload[1].InputStream))
        using (var g = Graphics.FromImage(bitmap))
        {
            RectangleF img1Params = getCropParams(img1);
            g.DrawImage(img1, new RectangleF(0, 0, 400, 400), img1Params, GraphicsUnit.Pixel);

            RectangleF img2Params = getCropParams(img2);
            g.DrawImage(img2, new RectangleF(400, 0, 400, 400), img2Params, GraphicsUnit.Pixel);

            var encoderParameters = new EncoderParameters(1);
            encoderParameters.Param[0] = new EncoderParameter(Encoder.Quality, 90L);
            bitmap.Save(ms, ImageCodecInfo.GetImageDecoders().Single(codec => codec.FormatID == ImageFormat.Jpeg.Guid), encoderParameters);
            return ms.ToArray();
        }
    }
    catch (Exception ex)
    {
        return new byte[0];
    }
}

private RectangleF getCropParams(Image img)
{
    var rect = new RectangleF();
    if (img.Width > img.Height)
    {
        rect.X = (img.Width / 2) - (img.Height / 2);
        rect.Width = (img.Width / 2) + (img.Height / 2) - rect.X;
        rect.Y = 0;
        rect.Height = img.Height;
    }
    else
    {
        rect.X = 0;
        rect.Width = img.Width;
        rect.Y = (img.Height / 2) - (img.Width / 2);
        rect.Height = (img.Height / 2) + (img.Width / 2) - rect.Y;
    }

    return rect;
}

It produces results I expect, but I was curious if my implementation could be improved. I am specifically interested in three things:

Do you see any potential memory leaks?
Do you think code will be performing well when it comes to scalability
Is there a component like ImageResizer that can do this?

Expanding on my comment above, I'd also follow Microsoft's naming guidelines and have the method names be PascalCased. Plus, I'm not a huge fan of mutable structs, preferring to initialize them via provided constructors. So I came up with the following:

public static class ImageCombiner
{
    private static readonly EncoderParameters encoderParameters = new EncoderParameters(1);

    private static readonly RectangleF sizeLocation1 = new RectangleF(0.0F, 0.0F, 400.0F, 400.0F);

    private static readonly RectangleF sizeLocation2 = new RectangleF(400.0F, 0.0F, 400.0F, 400.0F);

    private static readonly ImageCodecInfo jpgDecoder = ImageCodecInfo.GetImageDecoders().Single(codec => codec.FormatID == ImageFormat.Jpeg.Guid);

    private static readonly byte[] emptyImage = new byte[0];

    static ImageCombiner()
    {
        encoderParameters.Param[0] = new EncoderParameter(Encoder.Quality, 90L);
    }

    // Crops two squares out of two separate images
    // And then combines them into single image that's returned as byte[]
    public static byte[] CombineImages(IList<HttpPostedFileBase> itemPayload)
    {
        try
        {
            using (var ms = new MemoryStream())
            using (var bitmap = new Bitmap(800, 400, PixelFormat.Format24bppRgb))
            using (var img1 = Image.FromStream(itemPayload[0].InputStream))
            using (var img2 = Image.FromStream(itemPayload[1].InputStream))
            using (var g = Graphics.FromImage(bitmap))
            {
                g.DrawImage(img1, sizeLocation1, GetCropParams(img1), GraphicsUnit.Pixel);
                g.DrawImage(img2, sizeLocation2, GetCropParams(img2), GraphicsUnit.Pixel);
                bitmap.Save(ms, jpgDecoder, encoderParameters);
                return ms.ToArray();
            }
        }
        catch
        {
            return emptyImage;
        }
    }

    private static RectangleF GetCropParams(Image img)
    {
        return img.Width > img.Height
            ? new RectangleF(
                    (img.Width / 2) - (img.Height / 2),
                    0.0F,
                    2 * (img.Height / 2),
                    img.Height)
            : new RectangleF(
                    0.0F,
                    (img.Height / 2) - (img.Width / 2),
                    img.Width,
                    2 * (img.Width / 2));
    }
}


SQL iteration-Insertion plus renaming Base Info

Two tables: tData, tData2
Exactly the same columns
About 200,000 records
SQL Server 2008 R2

Logic

At first sight we need to insert tData rows into tData2. What else?

We need a renamed version of a column inside another (tData2)
with the condition checking it won't be an existing item when it's renamed.
Here's the code:

INSERT INTO [tData2]
(
    [Key],
    Info1,
    Info2
)
SELECT
    REPLACE([Key],'_',' '),
    Info1,
    Info2
FROM
    [tData]
WHERE
    (REPLACE([Key],'_',' ') = [Key]) OR
    (REPLACE([Key],'_',' ') NOT IN (SELECT [Key] FROM [tData]))

The problem is it's really slow for me on a top-notch 64 bit system. It has taken more than an hour so far and it's still executing.

How to speed it up? Any alternatives? Any ideas?

I'm not a DBA/DBMS guru, so just some ideas:

Turn off indexes on tData2 during the migration.
Do it in smaller chunks for smaller transaction log. (Batch commit on large INSERT operation in native SQL?, How to implement LIMIT with Microsoft SQL Server?)
Make sure that [tData].[Key] has an index.
I'd consider replacing the subselect with a join if it's possible at all.


Repeatedly open and close a PDF document to perform various operations I am trying to implement the Execute Around pattern described in Kent Beck's Smalltalk Best Practice Patterns. An example in Java could be found here.

Basically, I am repeatedly opening and closing a pdf document while performing various operations, something like,

public void Parse()
{
    // Open the document
    PdfLoadedDocument loadedDocument = new PdfLoadedDocument("plan.pdf");

    List<string> annotations = Build(loadedDocument);
    // work with annotations.

    // Close the document
    loadedDocument.Save();
    loadedDocument.Close();
}

I would like to move the opening and closing the document in a centralized place, as I have tens of similar methods. All these methods open the document, perform an action, and close the document, and it's easy to forget to close the document.

Here is what I tried:

public void BuildAnnotations()
{
    List<string> annotations = null;

    ExecuteAction("plan.pdf", (PdfLoadedDocument loadedDocument) =>
    {
        annotations = Build(loadedDocument);
        // work with annotations
    });
}

private void ExecuteAction(string path, Action<PdfLoadedDocument> perform)
{
    PdfLoadedDocument loadedDocument = new PdfLoadedDocument(path);

    try
    {
        perform(loadedDocument);
    }
    catch(Exception e)
    {
        Console.WriteLine($"An error occured. {e}");
    }

    loadedDocument.Save();
    loadedDocument.Close();
}

My question is, is passing a lambda to an Action delegate a good idea? I am not that familiar with delegates, Actions, and lambdas (other than using them in linq queries). Are there any other better alternatives?

IMO there is a problem with the try...catch statement. If an exception is thrown you'll have to deal with communicating the error to the client. You can't rethrow it because then the Save() and Close() aren't called, so you'll have to have a kind of logging or event to notify through and that could make it all unnecessarily complicated. Besides that: if an exception is thrown, you may not want to save the document in an unknown state, but you should close it anyway.

You can handle that in the following way:

  try
  {
    perform(loadedDocument);
    loadedDocument.Save();
  }
  finally
  {
    loadedDocument.Close();
  }

Here the document is only saved if the process went well, but closed in any case. And the caller can handle the exception as needed.

Alternatively you can make a wrapper like:

  class PdfDocumentExecuter : IDisposable
  {
    private PdfLoadedDocument document;

    public PdfDocumentExecuter(string fileName)
    {
      document = new PdfLoadedDocument(fileName);
    }

    public void Execute(Action<PdfLoadedDocument> action)
    {
      action(document);
      document.Save();
    }

    public T Execute<T>(Func<PdfLoadedDocument, T> function)
    {
      T result = function(document);
      document.Save();
      return result;
    }

    public void Dispose()
    {
      if (document != null)
      {
        document.Close();
        document = null;
      }
    }
  }

called as:

  using (PdfDocumentExecuter executer = new PdfDocumentExecuter("path"))
  {
    annotations = executer.Execute(Build);
  }


Improving Watir::Browser for my needs I want to:

use Watir::Browser methods without browser. instance prefix
expand abilities of Watir::Browser using own methods and overload existing
since I don't need more than one browser launched at the same time, using Module seems to have more sense than class

Here is my try:

require "watir"

module Session_FF
    class << self; attr_accessor :browser end
    def self.element *args
        es = browser.elements *args
        raise "found #{es.size} nodes instead of one" if es.size > 1
        es.first
    end
    def self.method_missing m, *args, &block
        browser.send m, *args, &block
    end
end
def session_FF &block
    Session_FF::browser = Watir::Browser.new :ff
    Session_FF.module_eval &block
ensure
    Session_FF::browser.quit if Session_FF::browser
end

session_FF do
    goto "http://localhost/"
    element(text:"test").click
    #
    #
    #
end

Seems to be more complex, than I expected. How do you think?

I don't know if you can make it less complex in terms of lines of code, but I'd recommend structuring it a bit differently.

You've already got class << self around the attr_reader, so you might as well wrap the remaining methods in it.

You've also got the session_FF method that's independent of the module it "belongs to", and which messes with the module's static variables. Just for convenience, it'd be nice to keep things more encapsulated.

Here's my take:

module Session_FF
  class << self
    def element *args
      elements = browser.elements *args
      raise "found #{es.size} nodes instead of one" if elements.size > 1
      elements.first
    end

    def method_missing m, *args, &block
      browser.send m, *args, &block
    end

    # lazily instantiate a browser
    def browser
      @browser ||= Watir::Browser.new :ff
    end

    # quit and nil the browser instance
    def quit
      @browser.quit if @browser
      @browser = nil
    end
  end

  # included method
  def session_FF &block
    Session_FF.module_eval &block
  ensure
    Session_FF.quit
  end
end

which you can then include in your test code:

include Session_FF

session_FF do
  element(text: "test").click
  # ...
end

I'm guessing "FF" means "Firefox", so I'd probably just call the proxy method firefox, and the module FirefoxSession, but that's a different story.


Node.js register function var argon2 = require('argon2')
var nano = require('nano')('http://127.0.0.1:5984')

var users = nano.db.use('users')

var private = {
  generateHash: (password, callback) => {
    argon2.generateSalt().then((salt) => {
      argon2.hash(password, salt).then((hash) => {
        callback(hash)
      })
    })
  }
}

var public = {
  register: (username, password, callback) => {
    var calledback = false
    users.list((error, body) => {
      if (error) {
        calledback = true
        callback('dbError')
      } else {
        body.rows.forEach((value) => {
          if (value.id === username) {
            calledback = true
            callback('userExists')
          }
        })
      }
      if (!calledback) {
        private.generateHash(password, (hash) => {
          users.insert({
            _id: username,
            hash: hash
          }, (error) => {
            if (error) {
              calledback = true
              callback('dbError')
            } else {
              calledback = true
              callback(false)
            }
          })
        })
      }
    })
  }
}

module.exports = public

I made a function for registrating a user in a CouchDB database. How could I make the code more readable?

Use const and let

You're using ES6, so use these features. These are better than var as they communicate their intent better and are not hoisted.

Don't mix promises and callbacks

This one I can't really understand: You use a promise in one method (generateHash) but then try to use callbacks afterwards. Promises compose much better than callbacks for a little extra overhead and are much more readable (and would also make a huge improvement to your code). If you still want to use callbacks, that's completely fine, but try to stick to one - don't mix.

I've chosen to use Promises in the code I'm giving back to you, so I've wrapped your users.list function in a Promise. If you'd prefer to use callbacks, that's perfectly fine, but you may want to use async or a similar library to prevent the hacks you made with callbacked = false etc. That sort of thing is very brittle and does not scale very well at all, and makes it hard to read.

userExists return value

This makes things stringly typed. I've left it in as I'm not sure what you would prefer to use, but may I suggest using false to indicate register was unsuccessful (but did not error), such as when the user already exists, or return a Promise.reject(error) when there is actually an error. On the note of returning false, I'm not sure why you return false when registration was a success, but I'd advise against it.

Promises chain

You can return promises from inside of a then "callback" and that value will be resolved and then passed to the next promise in the chain. This means you can simplify your generateHash method to not have so much cyclomatic complexity. This is further improved by not using callbacks.

Don't split your functions into private and public

There's no need to attach these functions to these objects, and they have no bearing on what is actually exported or imported. To simplify things, you could let generateHash be a free function and then just export an object with register in.

Use function statements where possible

Unless you transpile with Babel, using function expressions will cause those functions to lose their names. This is fine for adhoc functions like arrow functions passed to map, but for library functions it may be more helpful to have those names in a stack trace.

Note that newer engines automatically assign the binding name to the function, but that can't be relied upon.

Reviewed code

const argon2 = require('argon2')
const nano = require('nano')('http://127.0.0.1:5984')

const users = nano.db.use('users')

function generateHash(password, callback) {
  return argon2.generateSalt()
    .then((salt) => argon2.hash(password, salt))
    .then(callback)
}

// Reviewers note: this wraps users.list in a promise. You
// may want to consider using Bluebird for this instead.
function listUsers() {
  return new Promise((resolve, reject) => {
    users.list((error, body) {
      if (error) {
        return reject(error)
      }

      return resolve(body)
    }
  })
}

function insertUser(userObject) {
  return new Promise((resolve, reject) => {
    users.insert(userObject, (error) => {
      if (error) {
        return reject(error)
      }

      // Reviewers note: I am not sure why you return false on success?
      return resolve(false)
    })
  })
}

module.exports = {
  register(username, password) {
    return listUsers()
      .then(body => body.rows.some(value => value.id === username))
      .then(doesUserExist => {
        if (doesUserExist) {
          return 'userExists'
        }

        return generateHash(password)
          .then(hash => insertUser({ _id: username, hash })
      })
  }
}


Shortest path algorithm in 0-1-unoriented graph I have an unoriented graph with n vertices and m edges, 1 <= n, m <= 100000. All edges have 0 or 1 weights (two vertices are connected even if the corresponding edge has 0 weight). Vertices indexing starts with 1. I need to find length of the shortest path from vertex A to vertex B.

Input data example:

4 5 1 3
1 2 1
2 3 0
3 4 1
4 1 1
1 3 0

From the first line I get: n = 4, m = 5, A = 1, B = 3. Then I get the elements of the adjacency list. The first number in each line is from-vertex, the second - to-vertex, and the last is the weight of current edge. 0 or 1.

In the output I must receive the length of the shortest path or -1 if the path doesn't exist.

Here is my solution using Dijkstra's algorithm:

#include <iostream>
...

using std::cout;
using std::cin;
using std::endl;
using std::vector;
using std::pair;
using std::string;
using std::map;

size_t INF = 200000;

int main() {
    size_t nVert,
        mEdges,
        start,
        end;
    cin >> nVert >> mEdges >> start >> end;
    vector<vector<pair<size_t, size_t>>> ajList(mEdges);
    size_t edStart, edEnd, weight;
    for (size_t vert = 0; vert < mEdges; ++vert) {
        cin >> edStart >> edEnd >> weight;
        ajList.at(edStart - 1).push_back(pair<size_t, size_t>(edEnd, weight));
    }

    vector<size_t> minLen(nVert, INF),
        pred(nVert);
    minLen[start - 1] = 0;
    vector<char> used(nVert);
    for (size_t vert = 0; vert < nVert; ++vert) {
        size_t currentVert = -1;
        for (size_t j = 0; j < nVert; ++j) {
            if (!used[j] && (currentVert == -1 || minLen[j] < minLen[currentVert]))
                currentVert = j;
        }
        if (minLen[currentVert] == INF)
            break;
        used[currentVert] = true;

        for (size_t j = 0; j < ajList[currentVert].size(); ++j) {
            int to = ajList[currentVert][j].first - 1,
                len = ajList[currentVert][j].second;
            if (minLen[currentVert] + len < minLen[to]) {
                minLen[to] = minLen[currentVert] + len;
                pred[to] = currentVert;
            }
        }
    }

    if (minLen[end - 1] == INF)
        cout << -1;
    else
        cout << minLen[end - 1];
    return 0;
}

I know that Dijkstra is the fastest algorithm for searching the shortest path from one vertex to all other vertices, but in my case it takes too long.

How can I optimize this code to make it working faster (this is the main problem)?
If you see some bugs in this snippet, please, let me know.

You should extract reading the data and the Dijkstra to their own method. It helps debugging. You don't have enough information to make a meaningful heuristic for A* anyway.

When filling the adjacency matrix you do it as if it is directional while your post says it's not:

for (size_t vert = 0; vert < mEdges; ++vert) {
    cin >> edStart >> edEnd >> weight;
    ajList.at(edStart - 1).push_back(pair<size_t, size_t>(edEnd, weight));
    ajList.at(edEnd - 1).push_back(pair<size_t, size_t>(edStart, weight));
}

I suggest you use a priority queue of some sort; it lets you remove the current node in \$O(\log n)\$ instead of \$O(n)\$ (the first inner for loop in your code).