Simple Neural Network in Java I had an assignment some weeks ago that consisted of making a simple McCulloch-Pitts neural network. I ended up coding it in a pretty OO style (or the OO style I've been taught), and I felt that my implementation might have been a bit excessive for being a one-off assignment: could this perhaps been implemented in a simpler, less verbose way? Possibly forsaking modularity and such since it's not really a concern. I had never seen any Neural Network implementations in Java beforehand, so I didn't really know what was a reasonable approach for this kind of thing.

This was implemented in Java - you were supposed to use C or C++, but I didn't know either of them so the teacher indulged me. It models a neural network with three input neurons, one output neuron and two layers of hidden neurons. The actual input-output specification is probably not important here, so I'll just explain the main class (NeuralNetwork.java); This class holds information about all neurons in the network and mediates all message passing between neurons: the neurons will in each fire one or more of their outgoing neurons in each timestep, based on the sum of the input of the ingoing neurons in the previous timestep. All of this message passing goes through the network: all the neurons don't know about each other or the network.

So the crux of the problem is simply to coordinate neurons and whether or not they fire their synapses in each timestep, and to calculate the responses of the neurons that they fire "towards" (through the synapses).

package main;

import java.util.HashMap;
import java.util.LinkedList;

public class NeuralNetwork {

//for debugging
int timestep = 0;

HashMap<String,InputNeuron> inputNeurons;
LinkedList<Neuron> internalNeurons;
LinkedList<OutputNeuron> outputNeurons;

public NeuralNetwork
    (HashMap<String,InputNeuron> inputNeurons, LinkedList<Neuron> internalNeurons, LinkedList<OutputNeuron> outputNeurons)
{
    this.inputNeurons = inputNeurons;
    this.internalNeurons = internalNeurons;
    this.outputNeurons = outputNeurons;
    this.outputNeurons.get(0).getOutput();
}

public int getNrTimestep() {
    return timestep;
}

public int[] timestep(LinkedList<String> inputs) {
    System.out.println("Network: timestep: " + timestep);
    for (String inp : inputs) {
        assert inputNeurons.get(inp) != null : "timestep was given a String as key for an InputNeuron that didn't exist";
        inputNeurons.get(inp).fire();
    }
    for (Neuron internal : internalNeurons) {
        internal.nextTimestep();
    }
    for (Neuron outputNeuron : outputNeurons) {
        outputNeuron.nextTimestep();
    }
    int[] output = new int[outputNeurons.size()];
    int count = 0;
    for (OutputNeuron outputNeuron : outputNeurons) {
        output[count++] = outputNeuron.getOutput();
    }
    timestep++;
    resetAll();
    return output;
}

private void resetAll() {
    for (Neuron internal : internalNeurons) {
        internal.reset();
    }
    for (Neuron outputNeuron : outputNeurons) {
        outputNeuron.reset();
    }
}

}

Since you didn't post the code of Neuron, InputNeuron and OutputNeuron so I just give some comments on the style.

Instance variables

private HashMap<String,InputNeuron> inputNeurons;
private LinkedList<Neuron> internalNeurons;
private LinkedList<OutputNeuron> outputNeurons;

Instance variables should be kept private and expose via methods.

Ps: I'm not sure here if you intended to let them package-private for other classess (e.g Neuron, etc) to access them directly.

Prefer reference to objects by their interfaces

It is considered a good practice to reference to objects by their interfaces rather than concrete implementation (if they have one). Here you are using the Collection Framework so it'd be better if you declared the instance variables via interfaces.

private Map<String,InputNeuron> inputNeurons;
private List<Neuron> internalNeurons;
private List<OutputNeuron> outputNeurons;

Make defensive copies

The constructor of NeuronNetwork uses directly the arguments passed to it which could lead to some subtle bugs. It'd be better if you make a copy of those arguments.

public NeuralNetwork(Map<String,InputNeuron> inputNeurons, List<Neuron> internalNeurons,
        List<OutputNeuron> outputNeurons) {
    this.inputNeurons = new HashMap<String, InputNeuron>(inputNeurons);
    this.internalNeurons = new ArrayList<Neuron>(internalNeurons);
    this.outputNeurons = new ArrayList(outputNeurons);
    this.outputNeurons.get(0).getOutput(); // potential IndexOutOfBoundException
}

The last line in the constructor could cause IndexOutOfBoundException if the outputNeurons is empty. I'm not sure the purpose of this line.

Avoid using array as return type if possible

The method timeStep returns an int[] which is an array. IMO this is not good since array is  less convenience and doesn't provides much useful behaviors. Returning a Collection or a  List would be a better choice.

public Collection<Integer> timestep(List<String> inputs) {
    System.out.println("Network: timestep: " + timestep);

    for (String inp : inputs) {
        assert inputNeurons.get(inp) != null : "timestep was given a String as key for an InputNeuron that didn't exist";
        inputNeurons.get(inp).fire();
    }

    for (Neuron internal : internalNeurons) {
        internal.nextTimestep();
    }

    for (Neuron outputNeuron : outputNeurons) {
        outputNeuron.nextTimestep();
    }

    List<Integer> output = new ArrayList<Integer>(outputNeurons.size());
    for (OutputNeuron outputNeuron : outputNeurons) {
        output.add(outputNeuron.getOutput());
    }

    timestep++;
    resetAll();
    return output;
}

Misc

You could rename the method getNrTimestep() to getTimestep() for more elegant.

That's it. Hope these could help.


In 8-puzzle finding nr of steps moving a tile to the right place In 8-puzzle I want to count the number of moves it would take to move a specific tile to it's goal place. The problem is my method for this is >100 lines long which is obviously way too much. I'll only include part of that method that counts steps to move the piece to the right row if the goal row is higher. It gives the idea, it is probably hard to read even with that piece of it only, I did comment profusely though.

To further illustrate what the code does - for state {{0,2,3},{4,1,6},{7,8,5}} the count of steps for moving the 1 tile to first spot would be 5:

    2  3       2     3        2  1  3        2  1  3          1  3       1     3
 4  1  6   =>  4  1  6   =>   4     6   =>      4  6   =>  2  4  6   =>  2  4  6
 7  8  5       7  8  5        7  8  5        7  8  5       7  8  5       7  8  5

And for the code ( or bits and pieces of it):

    // Finding the nr of steps it would take to move tile to it's goal place
public int countSteps(int tileNr) {
    int steps = 0;
    int[] goal = findGoal(tileNr);
    int[] current = findTile(tileNr);
    int[] empty = findTile(0);
    GameState copy = new GameState(this);
    while (goal[0] != current[0] && goal[1] != current[1]) {
        // If the tile needs to be moved upwards (row coord is smaller in
        // goal)
        if (goal[0] < current[0]) {
            if (empty[0] == current[0] - 1 && empty[1] == current[1]) {
                // If empty above current tile, switch them
                steps++;
                current = empty;
                empty[0]++;
            } // Else have to move the empty tile to above current
            else if (empty[1] == current[1]) {
                if (empty[0] < current[0] - 1) {
                    // If empty too high in the right column, move empty
                    // tile down
                    steps++;
                    empty[0]++;
                }
                if (empty[0] > current[0]) {
                    // If empty below current, move out from that that
                    // column
                    steps++;
                    if (empty[1] < 2)
                        empty[1]++;

                    else
                        empty[1]--;
                }
            } else if (empty[0] == current[0]) {
                if (empty[1] < current[1]) {
                    // If empty to the right in right row
                    // move to the left
                    steps++;
                    empty[1]++;
                }
                if (empty[1] > current[1]) {
                    // If empty to the left in right row move to the right
                    // column
                    steps++;
                    empty[1]--;
                }

            }
        }
    }
    return steps;
}

// Finding coords for the tile in goal state
public int[] findGoal(int tileNr) {
    for (int i = 0; i < 3; i++) {
        for (int j = 0; j < 3; j++) {
            if (goalState[i][j] == tileNr)
                return new int[] { i, j };
        }
    }
    return new int[] { -1, -1 };
}

// Finding coords for the tile in current state
public int[] findTile(int tileNr) {
    for (int i = 0; i < 3; i++) {
        for (int j = 0; j < 3; j++) {
            if (state[i][j] == tileNr)
                return new int[] { i, j };
        }
    }
    return new int[] { -1, -1 };
}

Concerns

This logic is complicated and long winded... so much so, that you cut a huge portion of it, or otherwise there is a big bug... You have the condition inside the while-loop:

   // If the tile needs to be moved upwards (row coord is smaller in
   // goal)
   if (goal[0] < current[0]) {

But there is no 'else' condition for that. Did you 'trim' that out?

What if the row is in the other direction? What if it is already in the correct row?

Incremental Suggestion

Generally, when presented with problems like this it often helps to generalize the problem....

For example, if you create a concept called a 'direction', which is a two-value pair, say an int[2] (keeping it consistent with your code). If the target is at coordinate [0,0] and the current position of the tile is at [2,2] then the direction is [-1,-1] (you need to be reducing the row/column coordinates by 1 to get to your target.

Then, your while loop can become:

int[] goal = findGoal(tileNr);
int[] current = findTile(tileNr);
int[] empty = findTile(0);
for (int[] direction = getDirection(current, goal);
           direction[0] != 0 || direction[1] != 0;
           direction = getDirection(current, goal)) {

    if (direction[0] != 0) {
        // find a way to move things in the row axis
        // do this by getting the empty space where we need to move to....
        ......
        // swap our tile with the empty space.
        empty[0] -= direction[0];
        current[0] += direction[0]; // update the current position.
    } else { // direction[1] must be != 0
        // find a way to move things in the column axis
        // do this by getting the empty space where we need to move to....
        ......
        // swap our tile with the empty space.
        empty[1] -= direction[1];
        current[1] += direction[1]; // update the current position.
    }
}
// to exit the loop, direction == [0, 0] ... i.e. we are at our target.

Bigger suggestion

In this case I would consider a significantly different data structure.... :

Create a Tile Object for each Tile.
The GameState instance will then just become a loose collection of Tiles.
Keep the tile location embedded in the Tile instance.
Treat the empty space as a tile, and the 'rule' is that the Empty tile is the only tile that can swap positions with it's neighbour.
You can embed the logic in to methods on the Tile to identify the tile position and directions.

Doing the above can reverse a lot of your game logic, but the benefit will be that you move the logic out of the main methods in to the object orientation....


Thread-safe irregular rate limiter Google's Guava library has a great RateLimiter, but I also needed a way to limit on an irregular basis, such as if an API I'm using has been throttled.  I basically built a wrapper around Guava's RateLimiter, only using the simple acquire() method from it.

import java.util.concurrent.ExecutionException;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;

import com.google.common.util.concurrent.RateLimiter;

public class CustomLimiter {
    private RateLimiter guavaLimiter;
    private Throttle apiThrottle = new Throttle();

    public CustomLimiter() {
        this.guavaLimiter = RateLimiter.create(.8);
    }

    public double acquire() {
        try {
            apiThrottle.waitChoke();
        } catch (Exception e) {
            throw new IllegalStateException("Someone else handle this");
        }
        return guavaLimiter.acquire();
    }

    public void rateLimitNotify() {
        apiThrottle.startChoke();
    }

    class Throttle {
        private final ScheduledExecutorService executor = Executors.newScheduledThreadPool(1);
        private ScheduledFuture future;

        public void startChoke() {
            future = executor.schedule(new EndThrottle(this), 5, TimeUnit.SECONDS);
        }

        public void waitChoke() throws InterruptedException, ExecutionException {
            if(future == null || future.isDone() || future.isCancelled())
                return;
            future.get();
        }
    }

    class EndThrottle implements Runnable {

        private Throttle throttle;
        @Override
        public void run() {
            System.out.println("I feel like I should be doing something with the throttle here");
        }

        public EndThrottle(Throttle throttle) {
            this.throttle = throttle;
        }
    }
}

Multiple threads will be using this limiter and the API, and they may each encounter throttles.  This allows each thread to encounter any throttle notification and tell the limiter, and it only keeps record of the most recently encountered one, since it will also be the last to expire.

Initially, I thought I would need to synchronize everywhere, but in the end, I didn't use any.  Is this code still usable from multiple threads?

As a side question- ScheduledExecutorService seemed like the best way to delay without forcing a block unless something needs to wait, but it asks for a Runnable, which I don't seem to really utilize.  Is this poor design?

Initially, I thought I would need to synchronize everywhere, but in the end, I didn't use any. Is this code still usable from multiple threads?

No. When one thread sets

private ScheduledFuture future;

other may not see it in

    public void waitChoke() throws InterruptedException, ExecutionException {
        if(future == null || future.isDone() || future.isCancelled())
            return;
        future.get();
    }

Making future volatile would help. However, there's another problem with it changing before the test and future.get(). Here, a local variable would help.

  As a side question- ScheduledExecutorService seemed like the best way to delay without forcing a block unless something needs to wait, but it asks for a Runnable, which I don't seem to really utilize. Is this poor design?

I guess, it's fine but you've overcomplicated it. Do you need EndThrottle#throttle? Do you need EndThrottle at all?

Why not just this?

future = executor.schedule(new Runnable(), 5, TimeUnit.SECONDS);

Overall, it doesn't look bad, but concerning thread-safety, there may be more problems.


Logging Window system (I originally asked this on Stack Overflow and was directed here)

So I have 2 classes, one is a PyQT window widget, and the other class is a stack which holds a dictionary pointing to all of the window objects.

Log Window:

from PyQt5.QtWidgets import QTreeView
from PyQt5.QtGui import QStandardItemModel, QStandardItem
from Log.Controllers.Routines.log_get_next_default_index_routine import get_next_nefault_index

class LogWindow(QTreeView):
    def __init__(self,
                 model=None,
                 index=get_next_default_index(),
                 title=None):

        super().__init__()
        self.model = model
        self.index = index
        self.title = title
        self.set_title(title)

    def set_model(self,model):
        if model is not None:
            self.model = model
        else:
            self.model = QStandardItemModel()
        self.setModel(self.model)

    def set_index(self,index):
        if index is not None:
            self.index = index
        else:
            self.index = log_coordinator.get_next_default_index()

    def set_title(self,title):
        if title is not None:
            self.setWindowTitle(title)
        else:
            self.setWindowTitle('Log Window %s' % self.index)

Log Stack:

class LogWindowStack():
    def __init__(self,index=None,log_window=None, log_window_bundle=None):
        self.stack = {}
        self.default_index_counter = 0

        self.populate_index(index,log_window,log_window_bundle)

    def populate_index(self,index,log_window,log_window_bundle):
        if index or log_window is not None:
            self.stack[index] = log_window
        elif log_window_bundle is not None:
            for index,log_window in log_window_bundle:
                self.stack[index]=log_window

    def increment_default_index_counter(self):
        self.default_index_counter = ++self.default_index_counter

There is also a controller which both creates, modifies, and passes values to the constructors of these 2 classes.

Log Controller/Coordinator:

from Log.Controllers.Routines.log_insert_log_entry_routine import insert_log_entry
from Log.Objects.log_entry_obj import LogEntry
from Log.Objects.log_window_obj import LogWindow
from Log.log_window_stack_instance import log_window_stack

def create_log_windows(log_window_indexs):
    log_window_bundle = {}
    if log_window_indexs is not None:
        for index in log_window_indexs:
            log_window_bundle[index] = LogWindow(index=index)
    return log_window_bundle

def create_log_entry(log_entry):
    return LogEntry(log_entry)

def insert_assigned_log_entries(log_window_entry_bundle=None):
    if log_window_entry_bundle is not None:
        for log_window_index,log_entry in log_window_entry_bundle.items():
            insert_log_entry(log_window_index,log_entry)

def distribute_log_entries(log_window_indexs=None,log_entries=None):
    if log_window_indexs or log_entries is not None:
        for index in log_window_indexs:
            for log_entry in log_entries:
                insert_log_entry(index,log_entry)

def push_to_log_window_stack(log_window_bundle):
    for index,log_window in log_window_bundle.items():
        log_window_stack.stack[index] = log_window

def present_log_window_stack(log_window_indexs):
    for index in log_window_indexs:
        log_window_stack.stack[index].show()

def window_index_exists(log_window_index):
    key_exists = False
    for key in log_window_stack.stack.keys():
        if log_window_index == key:
            key_exists = True
    return key_exists

Does this design look sloppy or riddled with potential errors down the line? How can I improve it?

if model is not None:

You do this all over the place. You should change it to:

if model:

@property is the Pythonic way to do getter and setters. For instance, I would do the following for the model variable:

Change:

self.model = model

To:

self._model = model

Create a property:

@property
def model(self):
    return self._model

@model.setter
def model(self, new_model):
    if new_model:
        self._model = new_model
    else:
        self._model = QStandardItemModel()
    self.setModel(self._model)

And do this for the other set functions.


Optimizing Sieve of Eratosthenes for larger numbers I am having difficulty optimizing my code for the Sieve of Eratosthenes. The code works fine for smaller numbers (up to 10,000) but takes a long time to run for numbers over 100,000.

import java.util.Arrays;
import java.util.Scanner;

public class SieveOfErasthostenes {

    static int num = 1;
    static int count = 0;
    int i;
    static int nthPrime;

    public static void main(String[] args) {
        System.out.println("Enter the number of the prime: ");
        Scanner sc = new Scanner(System.in);
        nthPrime = sc.nextInt();
        long start = System.currentTimeMillis();
        boolean[] primes = new boolean[2*nthPrime*(int)Math.log(nthPrime)];
        Arrays.fill(primes,true);
        primes[0]=primes[1]=false;
        for (int i=2;i<Math.sqrt(primes.length);i++) {
            while (count < nthPrime){
                num=num+1; //find the next prime number
                for (i = 2; i <= num; i++){
                    if (num % i == 0) {
                        break; //prime not found
                    }
                }
                if ( i == num){
                    count = count+1; //prime found
                }
            }
        String th;
        if (nthPrime % 10 == 1) {
            th = "st";
        }   else if (nthPrime % 10 == 2) {
            th = "nd";
            }   else if (nthPrime % 10 == 3) {
                th = "rd";
            }   else {
                th = "th";
            }
        long elapsed = System.currentTimeMillis() - start;
        System.out.println("The " + nthPrime + th + " prime number is: " + num);
        System.out.println("Running time: " + elapsed);
        sc.close();
        }
    }
}

Your question implies that you have a sieve of Erastosthenes, but you do not.

The sieve works from known-primes forward to a value with a given magnitude.

In other words, you start with a known prime (2), and then you eliminate all the multiples of 2 until you get to the end-point of the sieve. Then, you move forward from that known prime, to the next prime (which is the next number in your sieve that is prime). You then eliminate all multiples of that next prime too.

This ends up looking like a 2-stage process with a setup stage, and a double-loop stage.

Your code has a setup stage that is not used, and then has a three-loop structure that does things back-to-front.

The setup stage creates the sieve, which is a fixed size, that needs to 'cover' the 'nth' prime, wherever that may be. You may have to cover more than the exact value, which is why the formula 2*nthPrime*(int)Math.log(nthPrime) looks handy, but you should investigate this Wikipedia article about the approximate value of the Nth prime

So, once you have a large enough 'sieve', you then loop through it as follows:

for (int possiblePrime = 2; i < sieve.size; possiblePrime++) {
    if (sieve[possiblePrime]) {
        for (int notPrime = possiblePrime * 2; notPrime < sieve.size; notPrime += possiblePrime) {
            sieve[notPrime] = false;
        }
    }
}

Now, using those loops, you have a basic sieve of Eratosthenes, where all true values left in the sieve are primes. If the sieve is large enough, and you count them as you find them, then you can pull the Nth prime easily.

Note that the basic algorithm can be optimized in a bunch of ways.


Finding locations with a distance between 1 and 5000 I'm wondering how I could refactor this code. It may look a little reiterative and probably there's a shorter method to accomplish the same.

def self.location(distance=100,location)
  if distance.is_a? Integer
    if distance.between?(1,5000)
      distance = distance
    elsif distance < 1
      distance = 1
    elsif distance > 5000
      distance = 5000
    end
  else
    distance = 100
  end
  if location
    within(distance, :origin => location)
  else
    find(:all)
  end
end

Shorter, and while min/max was tricky, IMO, this is easier to understand:

def self.location(distance=100,location)
    distance = 100 unless distance.is_a?(Integer)
    distance = 1 if distance < 1
    distance = 5000 if distance > 5000

    if location
        within(distance, :origin => location)
    else
        find(:all)
    end
end


Arbitrary precision Euler-Mascheroni constant via a Brent-McMillan algorithm with no math module Utilizing the below relation, I am able to compute the Euler constant to great precision on a single thread quickly and simply.

My process is to compute the natural log via the AGM(utilizing Pi an the Ln(2)), and apply the result to the BMM algorithm.

\$\gamma=\lim_{n\to\infty}\mathscr{G}_n=\lim_{n\to\infty}\frac{\sum\limits_{k=0}^\infty \left(\frac{n^k}{k!}\right)^2 (H_k-\log\,n)}{\sum\limits_{k=0}^\infty \left(\frac{n^k}{k!}\right)^2}\$

where \$H_k=\sum\limits_{j=1}^k \frac1{j}\$ is a Harmonic number.

This process is much more efficient than my previous post.

import decimal
from tqdm import tqdm

def agm(a, b):
    """Arithmetic Geometric Mean"""
    a, b = D(a),D(b)
    for x in tqdm(range(prec)):
        a, b = (a + b) / 2, (a * b).sqrt()
    return a

def pi_agm():
    """Pi via AGM and lemniscate"""
    print('Computing Pi...')
    a, b, t = 1, D(0.5).sqrt(), 1/D(2)
    p, pi, k = 2, 0, 0
    while 1:
        an    = (a+b)/2
        b     = (a*b).sqrt()
        t    -= p*(a-an)**2
        a, p  = an, 2**(k+2)
        piold = pi
        pi    = ((a+b)**2)/(2*t)
        k    += 1
        if pi == piold:
            break
    return pi

def lnagm(x):
    """Natural log of via AGM"""
    if x == D(1):
        return 0
    if x == D(2):
        return lntwo()
    m = prec*2
    ln2 = lntwo()
    decimal.getcontext().prec = m
    pi = pi_agm()
    print('Computing Ln({0})...'.format(x))
    twoprec = (D(2)**(2-m))/x
    den = agm(1, twoprec)*2
    diff = m*ln2
    result = ((pi/den) - diff)
    logr = D(str(result)[:m//2])
    decimal.getcontext().prec = prec
    return logr

def lntwo():        #Fast converging Ln 2
    print('Computing Ln(2)...')
    def lntwosum(n, d, b):
        logsum, logold, e = D(0), D(0), 0
        while True:
            logold = logsum
            logsum += (1/((D(b**e))*((2*e)+1)))
            e += 1
            if logsum == logold:
                return (D(n)/D(d))*logsum
    logsum1 = lntwosum(14, 31, 961)
    logsum2 = lntwosum(6, 161, 25921)
    logsum3 = lntwosum(10, 49, 2401)
    ln2 = logsum1 + logsum2 + logsum3
    return ln2

def gamma():
    n = D(prec)
    a = u = -lnagm(n)
    b = v = D(1)
    i = D(1)
    while True:
        k = (n/i)**2
        a *= k
        b *= k
        a += (b/i)
        if (u+a) == u or (v+b) == v:
            break
        u += a
        v += b
        i += 1
    return D(str(u/v)[:prec-3])

if __name__ == "__main__":
    """Setting precision plus 5 to accomodate error"""
    prec = int(input('Precision: '))+5
    decimal.getcontext().prec = prec
    D = decimal.Decimal
    print(gamma())

The single largest optimization I was able to make was to the logarithms. Once the two terms of a AGM are the same, they will not differ again. Therefore you can be sure you have converged. The argument of oscillating convergence does not apply.

I also noticed that, code aside, y=2^x+8; AGM(1, y) converges the fastest out of the numbers. I don't know the exact Math behind AGM convergence rates but I noticed that.(therefore in the script, put precision as 2^x -1 for ultra fast Ln convergence)

Since this is my own answer, I'm fine with me putting gmpy2 in places to make the computations faster.

I computed 212,143 digits of Gamma in 3.5 hours with the script. Usable precision into the thousands is done in less than  breath :) .

import decimal
from tqdm import tqdm
from gmpy2 import mpc
import gmpy2

def agm(a, b):
    """Arithmetic Geometric Mean"""
    a, b = D(a),D(b)
    for i in tqdm(range(prec)):
        a, b = (a + b) / 2, (a * b).sqrt()
        if a == b:
            break
    return a

def pi_agm():
    """Pi via AGM and lemniscate"""
    print('Computing Pi...')
    a, b, t = 1, D(0.5).sqrt(), 1/D(2)
    p, pi, k = 2, 0, 0
    while 1:
        an    = (a+b)/2
        b     = (a*b).sqrt()
        t    -= p*(a-an)**2
        a, p  = an, 2**(k+2)
        piold = pi
        pi    = ((a+b)**2)/(2*t)
        k    += 1
        if pi == piold:
            break
    return pi

def lnagm(x):
    """Natural log of via AGM"""
    if x == D(1):
        return 0
    if x == D(2):
        return lntwo()
    m = prec*2
    ln2 = lntwo()
    decimal.getcontext().prec = m
    pi = pi_agm()
    print('Coverging on Ln({0})...'.format(x))
    twoprec = (D(2)**(2-m))/x
    den = agm(1, twoprec)*2
    diff = m*ln2
    result = ((pi/den) - diff)
    logr = D(str(result)[:m//2])
    decimal.getcontext().prec = prec
    return logr

def lntwo():
    """Fast converging Ln2"""
    print('Computing Ln(2)...')
    def lntwosum(n, d, b):
        logsum, logold = mpc(0), mpc(0),
        e, n, d = mpc(0), mpc(n), mpc(d)
        while True:
            logold = logsum
            logsum += (1/((b**e)*((2*e)+1)))
            e += 1
            if logsum == logold:
                return (n/d)*logsum
    logsum1 = lntwosum(14, 31, 961)
    logsum2 = lntwosum(6, 161, 25921)
    logsum3 = lntwosum(10, 49, 2401)
    ln2 = logsum1 + logsum2 + logsum3
    return D(str(ln2.real)[:-4])

def gamma():
    n = D(prec)
    a = u = mpc(str(-lnagm(n)))
    print('Computing Gamma...')
    b = v = mpc(1)
    i = mpc(1)
    while True:
        k = (n/i)**2
        a *= k
        b *= k
        a += (b/i)
        if (u+a) == u or (v+b) == v:
            break
        u += a
        v += b
        i += 1
    return D(str(u/v)[:prec-3])

if __name__ == "__main__":
    """Setting precision plus 5 to accomodate error"""
    prec = int(input('Precision: '))+5
    gmpy2.get_context().precision=(prec*4)
    decimal.getcontext().prec = prec
    D = decimal.Decimal
    gam = gamma()
    print(gam)


Ruby script implementation I created a script to automate the task of uploading a CSV file to Box. I have a couple of files a Box client and a MySQL client. I create an instance of both on the function.rb file which is the one below. I wonder if there is anything to improve? if there are principles I'm not following or if I'm breaking best practices.
Is there anything that can be improved?
require 'json'
require 'date'
require 'dotenv/load'
require 'fileutils'
require 'csv'
require 'logger'
require './my_sql'
require './box_api'
require 'pry'

begin
  year = ARGV[0]
  month = ARGV[1]
  day = ARGV[2]

  search_timestamp = Time.new(year, month, day).utc

  db_client = MySQL.new(search_timestamp)

  emails = db_client.get_emails_from_db
  return 'No new emails found' if emails.entries.empty?

  box = BoxApi.new(ENV['BOX_USER_ID'])

  date = DateTime.now.strftime("%m-%d-%Y").to_s
  file_name = "access-emails-#{date}"

  CSV.open("./tmp/#{file_name}.csv", "wb") do |csv|
    emails.entries.each do |entrie|
      csv << [entrie.values[0], entrie.values[1]]
    end
  end

  box.upload_file_to_box("./tmp/#{file_name}.csv", file_name, ENV['BOX_FOLDER_ID'])

  FileUtils.rm("./tmp/#{file_name}.csv")

  puts "successfully uploaded CSV file to Box"
rescue StandardError => e
  logger = Logger.new(STDOUT)
  logger.level = ENV.fetch('LOG_LEVEL', Logger::INFO)
  logger.datetime_format = '%Y-%m-%d %H:%M:%S '
  logger.error("msg: #{e}, trace: #{e.backtrace.join("\n")}")
end

I think your script would benefit from some object oriented composition. Basically you have three different concerns.

Config
File backup (select from database and storage in CSV)
File upload

A few things could change now, for instance backup from a different source (different database, cloud etc), upload to a different remote service (e.g. dropbox). Additionally, having small composable objects would make it easier to test this.
Here are some examples how to compose your script more object oriented.
Config
We could use an OpenStruct to store our config data. This way we only need to write our environment variables once, if we want to change them later there is only one place to update them.
require 'ostruct'

config = OpenStruct.new(
  year: ARGV[0],
  month: ARGV[1],
  day: ARGV[2],
  box_user_id: ENV['BOX_USER_ID'],
  box_folder_id: ENV['BOX_FOLDER_ID']
)

FileBackup
We can extract a backup file which just excepts rows and writes them to a CSV file. The dependency injection makes it also easier to test this (e.g. inject the data to write and the test directory)
class BackupFile
  def initialize(rows:, date: DateTime.now.strftime("%m-%d-%Y").to_s, directory: "./tmp")
    @rows = rows
    @date = date
  end

  def save
    CSV.open(full_path, "wb") do |csv|
      rows.each do |entry|
        csv << [entry.values[0], entry.values[1]]
      end
    end
  end

  def full_path
    File.join(directory, filename)
  end

  def delete
    FileUtils.rm(full_path)
  end

  private

  attr_reader :rows, :date

  def file_name
    "access-emails-#{date}"
  end
end

db_client = MySQL.new(search_timestamp)
emails = db_client.get_emails_from_db
return 'No new emails found' if emails.entries.empty?

file = BackupFile.new(emails.entries)
file.save

Upload
The uploader accepts a client, path and remote folder. Also notice that we have an adapter around the BoxApi to implement a common interface upload. If we want to swap it out to upload to Dropbox, we only need to write a DropboxClient adapter which we can inject into the uploader. To test, we can write even a TestClient.
class Uploader
  def initialize(client:, path:, remote_folder:)
    @client = client
    @path = path
    @remote_folder = remote_folder
  end

  def upload
    client.upload(path, file_name, remote_folder)
  end

  private

  attr_reader :client, :path, :remote_folder

  def file_name
    File.basename(path)
  end
end

class BoxClient
  def initialize(client:, box_user_id:)
    @client = client.new(box_user_id)
  end

  def upload(path, file_name, remote_folder)
    client.upload_file_to_box(path, file_name, remote_folder)
  end

  private

  attr_reader :client
end

Error handling
I would move the error handling into the classes directly and also inject the logger. Something like this:
class BoxClient
  def initialize(client:, box_user_id:, logger: Logger.new)
    @client = client.new(box_user_id)
  end

  def upload(path, file_name, remote_folder)
    client.upload_file_to_box(path, file_name, remote_folder)
  rescue BoxError =>
    logger.error("Upload failed: #{e.message}")
  end

  private

  attr_reader :client
end


Python [Discord.py] - Sports Fixture Bot I have created a simple bot using Discord.py which outputs the contents of text files which contain scraped sports fixtures and channel data (scraped data is received from other small scripts I created but haven't included here).
discordbot.py
import discord
import os
from dotenv import load_dotenv

client = discord.Client()

load_dotenv()
TOKEN = os.getenv('DISCORD_TOKEN')
GUILD = os.getenv('DISCORD_GUILD')

client = discord.Client()

@client.event
async def on_ready():
    for guild in client.guilds:
        if guild.name == GUILD:
            break

    print(
        f'{client.user} is connected to the following guild:\n'
        f'{guild.name}(id: {guild.id})'
    )

@client.event
async def on_message(message):
    id = client.get_guild(73194****229982)

    if str(message.channel) == "boxingmma":
        if message.content.find("!boxing") != -1:

            with open('/home/brendan/Desktop/boxingtest.txt', 'r') as file:
                msg = file.read(2000).strip()
                while len(msg) > 0:
                    await message.author.send(msg)
                    msg = file.read(2000).strip()

    if str(message.channel) == "football":
        if message.content.find("!english") != -1:

            with open('/home/brendan/Desktop/splittest.txt', 'r') as file:
                msg = file.read(2000).strip()
                while len(msg) > 0:
                    await message.author.send(msg)
                    msg = file.read(2000).strip()

    if str(message.channel) == "football":
        if message.content.find("!scottish") != -1:

            with open('/home/brendan/Desktop/testing2.txt', 'r') as file:
                msg = file.read(2000).strip()
                while len(msg) > 0:
                    await message.author.send(msg)
                    msg = file.read(2000).strip()

client.run("NzMxOTQ4Mzk5NzY2NjY3NDg2.XwuPsw.iN****-e2yDnRS_uWqff43Thvqw")

The bot is running fine and when commands are run on the relevant discord channel a DM is sent to the user who sent the request to the bot. The biggest issue I have with this code is the repetitive code in the on_message function. The only difference in the three different double if statements is the channel name e.g. Football or Boxing, the command e.g. !scottish, !english, !boxing and the filepath to the scraped data.
I am curious if anyone knows of anyways this could be improved or a way for the repetitive code to be reused? I tried to pass these as positional arguments from another function, for example:
def callall():
    on_message(channel="football", command="!english", filepath="/home/brendan/Desktop/test.txt")

but it appears that after researching this it is not possible to pass arguments to on_message in discord.py
Any suggestions on how the repetitive code can be resued, or ways in which the general program flow can be improved would be much appreciated.
If this would be better over at Stackoverflow instead please let me know as well.
Thanks all.

@client.event
async def on_ready():
    for guild in client.guilds:
        if guild.name == GUILD:
            break

    print(
        f'{client.user} is connected to the following guild:\n'
        f'{guild.name}(id: {guild.id})'
    )

There is a bug if GUILD is not in the client guilds, as it will print out 'client.user' is connected to the last guild in the list. This is especially problematic if client.guilds is empty, as you'll end up with an exception due to never initialising 'guild'.
I would suggest a little more "defensive" code to check for this kind of problem, as good logging here could save you a headache later. It should be fine to do since this code will rarely be run.
@client.event
async def on_ready():
    def get_guild_by_name(guild_name):
        for guild in client.guilds:
            if guild.name == guild_name:
                return guild

    guild = get_guild_by_name()
    if not guild:
        // Code to handle this case. Log, print, throw, whatever you want it to do

        return

    print(
        f'{client.user} is connected to the following guild:\n'
        f'{guild.name}(id: {guild.id})'
    )

id = client.get_guild(73194****229982)

This is an example of a hardcoded value that should really be either in a config file, or assigned to a constant at the top of the file. If you were ever to change the guild for some reason, you'd have to track down all these occurrences and update them. Other places to look at are the hardcoded paths, and the token(!) at the bottom.

if str(message.channel) == "boxingmma":
    if message.content.find("!boxing") != -1:

        with open('/home/brendan/Desktop/boxingtest.txt', 'r') as file:
            msg = file.read(2000).strip()
            while len(msg) > 0:
                await message.author.send(msg)
                msg = file.read(2000).strip()

This block of code repeats a few times, with some minor differences. Since you may add more stats or need to make information available across multiple channels, I would think about how to make adding new commands to a channel as easy as possible. Ideally adding a new command is at most a line long, and obviously doesn't break any other commands.
I think splitting the above code into two functions, one that checks if the command is recognised (the matching) and one that does the work (retrieving the stats). The reasoning behind this separation is that the part of the above code that does the work doesn't change much, but the command matching does.
Some small things of note

x in y is a little more direct than y.find(x) != -1
I don't know why it reads the file in 2000 byte chunks, so I've left it alone. Is this really needed?
while len(msg) > 0: can be reduced to while msg: as containers like strings are truthy when they are not empty.
If you are on a recent enough version of Python, the walrus operator can be used here to remove some repeated code.
I like Path from pathlib, it has prevent a couple of errors I would have otherwise made.

The below code is for demonstration purposes, I haven't run it so I don't know if there are any issues. I also have defined the functions inline, but they would probably be better off outside on_message definition.
@client.event
async def on_message(message):
    id = client.get_guild(73194****229982)

    def matches_channel_command(message, expected_channel, expected_command):
        channel_name = str(message.channel)
        if channel_name != expected_channel:
            return False

        return expected_command in message.content

    async def retrieve_stats(path_):
        with open(path_, 'r') as file:
            msg = file.read(2000).strip()
            while msg:
                await message.author.send(msg)
                msg = file.read(2000).strip()

    if matches_channel_command(message, "boxingmma", "!boxing"):
        await retrieve_stats("/home/brendan/Desktop/boxingtest.txt")
    ...


Different path for grid move (part 2) This is a continued discussion from (Different path for grid move) to optimize for space complexity, and since it is new code and I make a new post.

  Given a m * n grids, and one is allowed to move up or right, find the different number of paths between two grid points.

My major idea is, if move r steps right, u steps up, we can find (1) solutions for r-1 steps right and u steps up, then combine with one final right step (2) solutions for r steps right and u-1 steps up, then combine with one final up step.

I use a dynamic programming method to track count in r-1 steps (using pre_row) and r steps (using cur_row). Here is my code and any advice on code bugs, performance improvements in terms of time complexity, or code style issues are appreciated.

Source code in Python 2.7,

def grid_move(rights, ups):
    pre_row = []
    pre_row.append(0)
    # initialize for zero right and up only
    for i in range(1, ups+1):
        pre_row.append(1)
    cur_row = []
    for r in range(1, rights+1):
        for u in range(0, ups+1):
            if u > 0:
                cur_row.append(pre_row[u] + cur_row[-1])
            else:
                cur_row.append(1)
        pre_row = cur_row

    return cur_row[-1]

if __name__ == "__main__":
    print grid_move(2,3)

The code has a bug: Since you are appending elements, you have to make sure that the array is empty in the beginning. So the cur_row = [] command should be inside the loop.

Also, the number of ways to reach origin from origin is 1. Hence pre_row should have all elements 1 initially. This means that you can just do pre_row = [1] * (ups+1) instead of appending.

Next problem is kind of aesthetic. You are building cur_row at each step and turning pre_row into it in the end. Then it makes sense to fill cur_row before the loop as well, and that will make sure that the code also works for rights == 0.

Once we do these, there is no longer the need to append to cur_row inside the loop. We are sure that the list has the correct size so we can just assign elements.

Finally just remove the if inside the loop.

Combining these we have

def grid_move(rights, ups):
    cur_row = [1] * (ups+1)
    for r in range(1, rights+1):
        pre_row = cur_row
        cur_row[0] = 1
        for u in range(1, ups+1):
                cur_row[u] = pre_row[u] + cur_row[u-1]

    return cur_row[-1]

These should make the code more readable and efficient, and make the recurrence relation clear.


Pick a column for each row by column-header name I am parsing a table:

    |-------|
    |Bar|Foo|
    |-------|
    | 5 | 6 |
    |'f'|'g'|
    | 1 | 8 |
    |_______|

I've tried a number of ways of writing a pick function in Python:

#!/usr/bin/env python

from collections import OrderedDict

def pick0(r, k):
    Bar, Foo = r
    return locals().get(k)

def pick1(r, k, h):
    try:
        return r[h.index(k)]
    except ValueError:
        return None

pick2 = lambda r, k, h: OrderedDict(zip(h, r)).get(k)

pick3 = lambda r, ks, h: map(lambda t: t[0] in ks and t[1] or None,
                             OrderedDict(zip(h, r)).iteritems())

if __name__ == '__main__':
    key = 'Foo'
    headers = 'Bar', 'Foo'
    for row in ((5, 6), ('f', 'g'), (1, 8)):
        # Method 0
        print OrderedDict(Bar=row[0], Foo=row[1]).get(key)

        # Method 1
        print pick0(row, key)

        # Method 2
        print pick1(row, key, headers)

        # Method 3
        print pick2(row, key, headers)

        # Method 4
        print pick3(row, ('Foo', 'Bar'), headers)

The last three methods are generalised. The last method allows you to specify multiple keys to pick out.

The downside of all the functions, except pick0, is that the caller needs to know all the columns and their order. pick0 is even worse as it is not reusable.

I would rather implement something similar to csv.DictReader that would give me each row as a dict, or perhaps as a namedtuple

If the usage is as simple as in the given example, one could also simply do this:

key_index = headers.index(key)
for row in ((5, 6), ('f', 'g'), (1, 8)):
    print row[key_index]

Other comments:

Using locals() in pick0 is dubious. A caller trying to pick k or r would get unexpected results.
lambda is for creating nameless functions. Use def when you want to assign a name.


Extension Methods for Trimming List I'm currently working on a project that I had to create basically a running queue of items. My thought was to do this by a list, but surprisingly there weren't any methods to remove the first sequence or last sequence of items in a list that I found. I wrote the following class with extension methods for List<T>:

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

namespace Extensions
{
    public static class ExtensionClass
    {
        public static bool TrimFirst<T>(this List<T> list, int maxCount)
        {
            try
            {
                if (list.Count <= maxCount)
                {
                    throw new InvalidOperationException
                        ("Items in list <= maxCount, unable to trim list.");
                }

                while (list.Count > maxCount)
                {
                    T first = list.First<T>();
                    list.Remove(first);

                }
                return true;
            }

            catch
            {
                return false;
            }

        }

        public static bool TrimLast<T>(this List<T> list, int maxCount)
        {
            try
            {
                if (list.Count <= maxCount)
                {
                    throw new InvalidOperationException
                        ("Items in list <= maxCount, unable to trim list.");
                }

                while (list.Count > maxCount)
                {
                    T last = list.Last<T>();
                    list.Remove(last);

                }
                return true;
            }

            catch
            {
                return false;
            }
        }

        public static bool ItemExists<T>(this List<T> list, T item)
        {
            foreach (T listItem in list)
            {
                if (item.ToString() == listItem.ToString())
                {
                    return true;
                }
            }

            return false;
        }

    }
}

This seems something that I will likely use in the future, so I would like to optimize it and make it as flexible as possible. Does anyone have any suggestions? For reference, the try/catch/exception are in case I need to modify the code to deal with that in the future, although I was not quite sure about the way that I wrote that part of the code. Anyone that needs this function, feel free to use this code!

I have also added the ItemExists() method since I hate the Any() and Exists() methods' use of a predicate. As far as I know, this method processes the items by enumeration, just at the built-in methods do, except they possibly use a Parallel.ForEach loop. Any suggestions for optimizing this method?

if (list.Count <= maxCount)
{
    throw new InvalidOperationException("Items in list <= maxCount, unable to trim list.");
}

This doesn't seem like an exceptional case to me. If the idea is to trim the list to a certain size and the list is already smaller than that size -- the job is already done! In that case I would replace this with return true;.

while (list.Count > maxCount)
{
    T last = list.Last<T>();
    list.Remove(last);
}

This is the main guts of your method. Since the preceding if statement has already established that list.Count > maxCount, you know that the loop is always going to run at least one. Therefore you could replace this with a do-while loop. This is a negligible performance improvement.

do
{
    T last = list.Last<T>();
    list.Remove(last);
}
while (list.Count > maxCount)

catch
{
    return false;
}

Two problems here.

YAGNI
Using boolean values to indicate whether or not the method was successful is a design flaw. This harks back to the old HRESULT days which were universally loathed. Instead, let the exception do the job of alerting the calling code to errors (this is what exceptions are for). In the case that there is no exception, there is no use for the returned true value.

You could also consider returning a reference to the list itself from the method, allowing people to use method chaining syntax, which is all the rage these days - especially with LINQ..

public static List<T> TrimFirst<T>(this List<T> list, int maxCount)
{
    if (list.Count <= maxCount)
        return list;

    do
    {
        T first = list.First<T>();
        list.Remove(first);
    }
    while (list.Count > maxCount);

    return list;
}

One final thing. It's usually a good idea to defend against NullReferenceExceptions by asserting that your argument is not null.

if (list == null)
    throw new ArgumentNullException("list");

ArgumentNullExceptions are always preferable to NullReferenceExceptions.


Getting value from a property according to some parameters Im curious if my approach is correct in this case:
I would like to get two DateTimes from a public property, but one of the dates i want them only if a user has purchase a feature or if he has not activate a switch, otherwise i want to get the same DateTime twice.
More specifically, the way im getting the values now is this:
public class MainApplication : Application
{
public DateTime DateFrom { get; set; } = DateTime.Now;
public DateTime DateTo { get; set; } = DateTime.Now;
}

AnotherClass
{
            SearchButton.Click += async (sender, e) =>
            {
                await SearchFoNumbersAsync(ApplicationState.DateFrom, ApplicationState.DateTo, DailySearchSwitch.Checked, SeperateCheckBox.Checked, ShowListsCheckBox.Checked).ConfigureAwait(false);
            };
            private async Task SearchFoNumbersAsync(DateTime dateFrom, DateTime dateTo, bool isDailySearchEnabled, bool seperateSearch, bool showDrawTime)
            {
                ((MainActivity)Activity).DisplayLoadingMessage(true, GetString(Resource.String.Common_SearchTitle), GetString(Resource.String.Common_SearchMessage));

                if (isDailySearchEnabled || !PurchaseFunctions.HasPurchasedDateRangeSelection())
                    dateTo = dateFrom;
...
            }
}

So i want to get DateTo only
if (isDailySearchEnabled || !PurchaseFunctions.HasPurchasedDateRangeSelection()), otherwise i want to get the DateFrom value in the place of DateTo
Im sure this approach is not correct so im thinking i should get the DateTo this way:
public class MainApplication : Application
{
    public DateTime DateFrom { get; set; } = DateTime.Now;
    private DateTime DateTo { get; set; } = DateTime.Now;

    public DateTime GetDateTo(bool isDailySearchEnabled)
    {
        if (isDailySearchEnabled || !PurchaseFunctions.HasPurchasedDateRangeSelection())
             return DateFrom;
        else
             return DateTo;
    }

    public void SetDateTo(DateTime dateTime)
    {
        DateTo = dateTime;
    }
}

Is my new approach a good idea?

Probably the direction is correct but the class API is inconsistent for now. DateFrom is available, DateTo is not.
As for me, the following way looks more consistent.
public class MainApplication : Application
{
    public DateTime DateFrom { get; set; } = DateTime.Now;
    public DateTime DateTo { get; set; } = DateTime.Now;

    public DateTime GetSearchDate(bool isDailySearchEnabled)
        => isDailySearchEnabled || !PurchaseFunctions.HasPurchasedDateRangeSelection() ? DateFrom : DateTo;
}


Perl script to handle include files I am trying to extend the JSON format (for private use) to include an include statement. This could be used to include other .json files into a master file, for example.

#! /usr/bin/perl

use warnings;
use strict;
use File::Slurp 'read_file';

my $str=parseFile('t.json');

sub parseFile {
  my ($fn)=@_;

  my $str=read_file($fn);
  my $res='';
  my $i=0;
  while (1) {
    if ($str=~/include\s+"(.*?)"/) {
      $res=$res.$`.parseFile("include/$1.json");
      $str=$';
    } else {
      $res=$res.$str;
      last;
    }
  }
  return $res;
}

It seems to be working for some simple test cases. But is it too complicated or is it too simple?

The $` and $' are very cryptic, I had to look them up in man perlvar to find that they contain whatever was before and after the match. It also turns out that they are discouraged due to performance considerations. Either way the while block really smells.

A simpler approach is to use s///ge to replace the matched pattern with the output of a function call, like this:

sub parseFile {
  my ($fn) = @_;
  my $str = read_file($fn);
  $str =~ s/include\s+"(.*?)"/parseFile("include\/$1.json")/ge;
  return $str;
}

Also consider adding some error handling. For example, what should happen if the file doesn't exist? Ignore / warn / raise error?

It might be a good idea to make parseFile handle multiple parameters:

sub parseFile {
    foreach my $fn (@_) {
        my $str = read_file($fn);
        $str =~ s/include\s+"(.*?)"/parseFile("include\/$1.json")/ge;
        return $str;
    }
}

Maybe you don't really need to handle multiple files, but a side effect of this approach is that it automatically handles the case gracefully if somebody incorrectly calls the method without any parameters.

Finally, notice that I used spaces around operators like = and =~ to improve readability. I borrow this practice from other programming languages where this is a standard, and let's face it, Perl scripts need all the readability aid they can get...

UPDATE

@amon added some excellent points in his comment:

Notice that in the replacement pattern s/include\s+"(.*?)"/parseFile(".\/$1")/ge; I had to escape the delimiter /. Another way is changing the delimiter so I don't have to escape anything: s{include\s+"(.*?)"}{parseFile("include/$1")}ge;
My multi-argument suggestion is kinda broken, because of the return statement: it will only process the first file. It would be better to use Carp::confess "parseFile expects one file name or file handle" if not @_ == 1, and the caller could still map { parseFile($_) } @files if really required


Getting cars and bicycles from a database and writing them into two different arrays I've written a simple class which should:

get 10 top cars and 10 top bicycles from database
write them into 2 different arrays

It works, but I'm trying to find a simpler or clearer solution.

class TopVehicles extends Connection {
    private $sql1 = 'SELECT model, price FROM vehicles WHERE type = \'car\' ORDER BY price DESC limit 10';
    private $sql2 = 'SELECT model, price FROM vehicles WHERE type = \'bicycle\' ORDER BY price DESC limit 10';

    function __construct() {
        parent::__construct();
    }

    public function getTopCars() {
        $stmt = $this->mysqli->prepare($this->sql1);
        $stmt->execute();
        $stmt->store_result();
        $row_cnt = $stmt->num_rows;
        $stmt->bind_result($model, $price);

        while ( $stmt->fetch() ) {
            $_result[] =
            array(
                'Model' => $model,
                'Price' => $price
                );
        }

        $stmt->free_result();
        $stmt->close();
        return $_result;
    }

    public function getTopBicycles() {
        $stmt = $this->mysqli->prepare($this->sql2); /*this is the only line which differs these 2 functions!*/
        $stmt->execute();
        $stmt->store_result();
        $row_cnt = $stmt->num_rows;
        $stmt->bind_result($model, $price);

        while ( $stmt->fetch() ) {
            $_result[] =
            array(
                'Model' => $model,
                'Price' => $price
                );
        }

        $stmt->free_result();
        $stmt->close();
        return $_result;
    }

    function __destruct() {
        parent::__destruct();
    }
}

$obj = new TopProducts();
$_TopCars = $obj->getTopCars();
$_TopVehicles = $obj->getTopBicycles();

It works as expected, but it's messy.

Is it possible to somehow rearrange these 2 methods?

getTopBicycles()
getTopCars()

To stick to the rule: DRY? I've tried to divide them, but I am not able to write proper code. Can you please give me a hint on what I should do?

As far as I see the only difference is the type inside the SQL query. So why not pass that as parameter.

You could also think of creating additional classes for car and bicycle which call the getTop method of another class with the right parameters or if you extend the vehicle class set the corresponding type inside the constructor. Just to give you some more examples what you might be able to do or at least consider.

The following code is probably one of the easiest way to accomplish the DRY principle. But be aware that it's untested code.

class TopVehicles extends Connection {
    private $sql = 'SELECT model, price FROM vehicles WHERE type = ? ORDER BY price DESC limit 10';

    function __construct() {
        parent::__construct();
    }

    public function getTop($type) {
        $stmt = $this->mysqli->prepare($this->sql);
        $stmt->bind_param("s", $type);
        $stmt->execute();
        $stmt->store_result();
        $row_cnt = $stmt->num_rows;
        $stmt->bind_result($model, $price);

        while ( $stmt->fetch() ) {
            $_result[] =
            array(
                'Model' => $model,
                'Price' => $price
                );
        }

        $stmt->free_result();
        $stmt->close();
        return $_result;
    }

$obj = new TopVehicles(); // should be TopVehicles, shouldn't it?
$_TopCars = $obj->getTop('car');
$_TopVehicles = $obj->getTop('bicycle');

UPDATE: Without seeing the final code a code review is rather hard. It's true that you can't bind column names in a prepared statement. But what you could do is to manipulate the SQL string first. So here is a way how to accomplish that:

private $sql = 'SELECT %s FROM %s WHERE type = ? ORDER BY %s limit ?';

public function getTop($table, array $columns, array $orderByColumns, $type, $amount) {
    $sql = sprintf($this->sql, implode($columns, ', '), $table, implode($orderByColumns, ', '));
    $stmt = $this->mysqli->prepare($sql);
    $stmt->bind_param("si", $type, $amount);
    ...

$_TopCars = $obj->getTop('vehicles', array('model', 'price'), array('price DESC', 'model ASC'), 'car', 10);

Note: This snippet is again untested and there is enough room for improvement, but it should provide you with an idea on how you could do it. But make sure that you don't add SQL Injection vulnerabilities by using user input directly. This snippet contains also @Alex L's suggestion to pass the amount as parameter, too.

Note: Your class names also indicate that your code violates the http://en.wikipedia.org/wiki/Liskov_substitution_principle. TopVehicles are no Connection. I'd say it's better to dependency inject the connection.

class TopVehicles {
    private $mysqli;
    private $sql = 'SELECT model, price FROM vehicles WHERE type = ? ORDER BY price DESC limit 10';

    function __construct($mysqli) {
        $this->mysqli = $mysqli;
    }
    ...
}


Creating lists of Card objects I need to create two lists of Card objects in C#.

A Card class is defined as:

public class Card {
    public Suits Suit { set; get; }
    public Values Value { set; get; }
    public static Random rand = new Random();

    public Card() {

    }

    public Card(Suits s, Values v) {
        Suit = s;
        Value = v;
    }

    public override string ToString() {
        return Suit.ToString() + " that has a value of " + Value.ToString();
    }

    public Card RandomCardGenerator() {
        Suit = (Suits)rand.Next(4);
        Value = (Values)rand.Next(1, 14);
        return this;
    }

In a different class called Action:

   static class Action {
        static public List< Card > CreateRandomCardList() {
            List< Card > cardList1 = new List< Card >();

            for (int loop = 0; loop < 12; loop++) {
                cardList1.Add(new Card().RandomCardGenerator());
            }

            return cardList1;
        }

        static public List < Card > CreateFullSetCard() {
            List< Card > cardList2 = new List< Card >();

            for (int s = 0; s < 4; s++) {
                for (int v = 0; v < 13; v++)
                    cardList2.Add(new Card((Suits)s, (Values)v));
            }

            return cardList2;
        }
    }

CreateRandomCardList() is later called upon:

public partial class Form1 : Form {
        public Form1() {
            InitializeComponent();
            PopulateListBox1(Action.CreateRandomCardList());
        }

        private void PopulateListBox1(List< Card > l) {
            listBox1.DataSource = l;
        }
}

I used static for class Action.

It makes sense to create instances of Card, but is it necessary to create an instance of Action? Can I keep it static?

It's unclear what CreateRandomCardList is intended for during gameplay. I know, technically, it's generating random cards but then why just 11 (loop < 12), why not 13?

Nevertheless, I strongly feel, the Action class is clearly misleading. I would instead prefer to have a Deck which knows what cards it holds. Also, Card type should be as simple as possible, perhaps knowing only Suit and Value. RandomCardGenerator does not belong there.

// Deck is general term. You can more appropriately
// name it as DeckOfCards, CardDeck, or anything you deem fit.
public class Deck {
    private Card[] cards;

    // Setup() is purposely part of Deck creation because without cards deck is useless.
    public Deck() {
        // fill up 13 cards
        Setup();
    }

    private Cards[] Setup() {} // CreateFullSetCard()
    private Cards[] Shuffle() {}

    //.. so on
}

The Deck should not be static because

It is going to have it's own state.
If there are > 1 players, each will have their own Deck of cards. Even if it's single player, it should not be static for #1 reason.


Reversing a list without (append) I would like to reverse a list using cdr, car and cons. Since lists in lisp are asymmetrical (can only insert at the beginning), I am interested on how one would write a procedure to do that without using (append). Please review my code.

(define (reverse l)
    (define (aux orig result)
        (if (null? orig) result
                (aux (cdr orig) (cons (car orig) result))))
    (aux l '()))

Is this good enough? Are there better or more efficient ways to do this?

Given that you don't want to use append, this looks great to me. I'm not sure if I can even think of a different way to do it.

My only suggestion would be naming related. l is a terrible name for a variable - it's too close to 1. orig is questionable, since it's not really the original list, it's the list you're currently "popping" (logically, not literally) off of - so you could simply rename both l and orig to something like lst or xs or elems or ....


Sorting a flattened array I'm using C# in .NET 4. The class holds an array of double that represents a 2-D rectangular matrix but (due to business requirements) must be stored flattened (i.e. a double[]). The class also stores the dimensions of the array. I wish to sort the array by the (virtual) first column. Obviously I have to unflatten it, at least partially, in order to do this, right? I don't see anything in LINQ or System.Array that works on a flattened array.

If there's 1 column, System.Array.Sort() works fine and I don't think it can be improved.

If there's 2 columns, it's not hard to split it into two arrays and again use System.Array.Sort(). If you quickly see a way to improve it great, but if not, I'm happy with it as it stands.

I'm not happy with the code for 3 or more columns (it stinks) and suspect there's some way to improve it.

N.B. In real life the class has get-only properties. It's guaranteed that rows and cols are positive, array != null, and rows*cols==array.Length.

public partial class FlattenedArray
{
    public int rows;
    public int cols;
    public double[] array;
    public void sort()
    {
        double[] keys;
        switch(cols)
        {
            case 1:
                System.Array.Sort<double>(array);
                break;
            case 2:
                keys = new double[rows];
                double[] dItems = new double[rows];
                for (int i=0,k=0; i<rows; i++)
                {
                    keys[i] = array[k++];
                    dItems[i] = array[k++];
                }
                System.Array.Sort<double,double>(keys,dItems);
                for (int i=0,k=0; i<rows; i++)
                {
                    array[k++] = keys[i];
                    array[k++] = dItems[i];
                }
                break;
            default:
                keys = new double[rows];
                int[] items = new int[rows];
                double[] temp = new double[rows*cols];
                for (int i=0,k=0; i<rows; i++, k+=cols)
                {
                    keys[i] = array[k];
                    items[i] = k;
                }
                System.Array.Sort<double,int>(keys,items);
                for (int i=0,k=0; i<rows; i++)
                {
                    int spt = items[i];
                    for (int j = 0; j < cols; j++)
                    {
                        temp[k++] = array[spt++];
                    }
                }
                array = temp;
                break;
            }

You can improve the code for 3 or more columns and at the same time use it for 1 or 2 columns, by using LINQ:

public partial class FlattenedArray
{
    public int rows;
    public int cols;
    public double[] array;
    public void sort()
    {
        array = (from i in Enumerable.Range(0, rows)
                        select new List<double>(array.ToList().GetRange(cols * i, cols)))
                        .OrderBy(x => x[0])
                        .SelectMany(x => x)
                        .ToArray();
    }
}

This divides the array into dimensions and sorts it by the first column then flattens it all back to 1D.  It will work for any combination of rows and columns as long the conditions you mentioned hold true.

If the data size gets very large you still might want to use Array.Sort for the 1 column data as this code wouldn't be very efficient for 1 column.


Check if a string is a valid english word, and convert to a valid english word This is an interview question.
For the purpose of the question, a valid English word is a string that begins with an uppercase English letter, and the rest of the string contains only lowercase English letters. I needed to implement two functions:

A function that receives a String and returns true if the String is a valid English word, and false otherwise.
A function that receives a String and convert it to a valid english word. That means that the function removes non-English characters from the string, and also lowercase\uppercase letters that are in the wrong place. Examples:

convertToValidString("A") = "A"
convertToValidString("a") = ""
convertToValidString("Ab") = "Ab"
convertToValidString("AB") = "A"
convertToValidString("Ab5eA") = "Abe"
convertToValidString("aAd") = "Ad"
convertToValidString("a6d") = ""

Below is the code I wrote, please tell me what do you think. My emphasis is mainly about efficiency (run time + short code), and less about coding conventions.

public static boolean isValidString(String s){
    int len = s.length();

    if (len == 0) return true;
    if ( (s.charAt(0) < 65) || (s.charAt(0) > 90) ) return false;
    for (int i = 1; i < len; i++){
        if ( (s.charAt(i) < 97) || (s.charAt(i) > 120) ) return false;
    }

    return true;
}

public static String convertToValidString(String s) {
    StringBuffer ans = new StringBuffer();
    int len = s.length();
    int i = 0;

    while ( (i < len) && ((s.charAt(i) < 65) || (s.charAt(i) > 90)) )
        i++;

    if (i < len){
        ans.append(s.charAt(i));
        i++;
        for (; i < len; i++){
            if ( (s.charAt(i) >= 97) && (s.charAt(i) <= 120) )
                ans.append(s.charAt(i));
        }
    }

    return ans.toString();
}

You should avoid statements on the same line as an if.
So for example

if (len == 0)
    return true;

instead of

if (len == 0) return true;

Also, I find that the (s.charAt(0) < 65) || (s.charAt(0) > 90) are really hard to understand as they use magic numbers.

But, most importantly, are you sure your code is responding to all the cases ?

Let's create some unit test with JUnit and AssertJ :

class TestTestString {
    @Test
    void isValidString_shouldAcceptSimpleWord() {
        assertThat(TestString.isValidString("Hello")).isTrue();
    }

    @Test
    void isValidString_shouldAcceptVeryLongWord() {
        assertThat(TestString.isValidString("Thisincrediblylongwordisylbidercnilongz")).isTrue();
    }

    @Test
    void isValidString_shouldAcceptSingleUppercaseString() {
        String[] values = { "A", "B", "C", "D", "E", "F", "G", "H", "I", "J", "K", "L", "M", "N" };
        for (String s : values) {
            assertThat(TestString.isValidString(s)).isTrue();
        }
    }

    @Test
    void isValidString_shouldAcceptStringWithDiacritics() {
        assertThat(TestString.isValidString("Résumé")).isTrue();
    }

    @Test
    void isValidString_shouldRefuseStringThatStartWithALowerCaseLetter() {
        assertThat(TestString.isValidString("apropos")).isFalse();
    }

}

Without much surprise, the isValidString_shouldAcceptStringWithDiacritics is failing... but so is the isValidString_shouldAcceptVeryLongWord because the 120 isn't the good number !
Never forget to do some unit tests :) (you should also add some tests for numbers and special characters)

To conclude, if I were you, I'd get rid of the current test altogether and use the Character methods that are nicer lookings, namely isUpperCase and isLowerCase :

public static boolean isValidString(final String s){
        int len = s.length();

        if (len == 0)
            return true;
        if (!Character.isUpperCase(s.charAt(0)))
            return false;
        for (int i = 1; i < len; i++) {
            if (!Character.isLowerCase(s.charAt(i)))
                return false;
        }

        return true;
    }

Now, if you rerun the tests, they should be all green.

You can also use the same methods for your convert function ;)


Project Euler #4 in Haskell Project Euler #4 asks:

  Find the largest palindrome made from the product of two 3-digit numbers.

The code is as follows:

module Main where

import Data.List (sortBy)
import Data.Ord (comparing)

palindromes :: [Integer]
palindromes = [calcPalindrome a b c | a <- [1..9], b <- [0..9], c <- [0..9]]
    where
        calcPalindrome a b c = 100001 * a + 10010 * b + 1100 * c

is3Digit :: Integer -> Bool
is3Digit n = n >= 100 && n <= 999

isDiv :: Integer -> Integer -> Bool
isDiv n d = n `mod` d == 0

-- returns all multiples of 2 3-digit numbers
-- for palindromes
isMultiple :: Integer -> Bool
isMultiple n = not $ null threeMultiples
    where
        multiples :: [(Integer, Integer)]
        multiples = do
            d <- [10..99]
            if n `isDiv` (d * 11)
            then return (d * 11, n `div` (d * 11))
            else []
        threeMultiples :: [(Integer, Integer)]
        threeMultiples = filter (\ (m, n) -> is3Digit m && is3Digit n) multiples

problem4 :: Integer -> Integer
problem4 n =
    head $ filter isMultiple $ sortBy (flip compare) $ filter (< n) palindromes

main :: IO ()
main = do
    _ <- getLine
    contents <- getContents
    let cases = map read $ lines contents
    let results = map problem4 cases
    mapM_ print results

I'd appreciate comments on isMultiple and problem4 as they seem rather messy. Any other general comments are welcome too.

Proof technique

I was pleasantly surprised at the speed of this algorithm (generating all 6-digit palindromes, then taking the largest one with two 3-digit factors), as compared to the alternative approach (multiplying all 3-digit pairs, then finding the largest product that is a palindrome).  I would have expected the latter to be faster, as factoring is normally a more expensive operation — but you take advantage of a clever shortcut.  That, I suppose, is part of the point of Project Euler questions.

On the other hand, I would consider the program to be slightly incomplete.  The property that the palindrome is divisible by 11 only holds if it contains an even number of digits.  As a counterexample, 101 ∙ 101 = 10201 is a number within the parameters of the question (a palindrome that is the product of two 3-digit numbers) that is not divisible by 11.  Your palindromes function only generates 6-digit candidates for consideration.  While there is no need for your program to produce a result that requires no human interpretation, it would be desirable to note the assumption (or proof) of the existence of a 6-digit result, at least as a comment.

Implementation

I am puzzled by your boilerplate main function.  Reading an input number as an upper bound on the palindrome candidates seems superfluous.  An excessively large input won't scale the problem to cover 4-digit × 4-digit products, either, as your palindromes function is hard-coded to produce 6-digit candidates only.  So, this would have sufficed:

problem4 :: Int
problem4 = head $ filter isMultiple $ sortBy (flip compare) $ palindromes

main :: IO ()
main = do
    print problem4

Integer is overkill.  The numbers involved fit very comfortably under maxBound::Int.

sortBy (flip compare) is just the same as reverse.  But why reverse anything at all, when you could just generate the palindrome candidates in descending order in the first place?

palindromes :: [Int]
palindromes = [calcPalindrome a b c | a <- [9,8..1], b <- [9,8..0], c <- [9,8..0]]
    where
        calcPalindrome a b c = 100001 * a + 10010 * b + 1100 * c

is3Digit is fine, though I would find is3Digit n = 100 <= n && n < 1000 more aesthetically pleasing.

isMultiple could be better named, and the do … return would be better written as a list comprehension.  (See Do notation considered harmful.)

isProductOfThreeDigitNumbers :: Int -> Bool
isProductOfThreeDigitNumbers n = not $ null threeDigitFactors
    where
        -- All palindromes with an even number of digits are divisible by 11
        factors = [(d, n `div` d) | d <- [11, 22 .. 99 * 11], n `isDiv` d]
        threeDigitFactors = filter (\ (m, n) -> is3Digit m && is3Digit n) factors

Then, problem4 would just be

problem4 :: Int
problem4 =
    head $ filter isProductOfThreeDigitNumbers $ palindromes


Basic Fibonacci sequence in C #include <stdio.h>
#include <stdlib.h>
#define RED     "\x1b[31m"
#define GREEN   "\x1b[32m"
#define YELLOW  "\x1b[33m"
#define BLUE    "\x1b[34m"
#define MAGENTA "\x1b[35m"
#define CYAN    "\x1b[36m"
#define RESET   "\x1b[0m"

int main () {

  int n1 = 1, n2 = 0, i = 0, limit = 0, neg = 0, pos = 0;
        printf("Enter first number > ");
  scanf("%d",&n1);
        printf("Enter the seconed number > ");
  scanf("%d",&n2);
        printf("Enter the number of terms > ");
  scanf("%d",&limit);

  for (i; i < limit; i++ ) {
                if (n2 >=1) { pos ++; printf( RESET "%i\n" RESET, n2); }
                if (n2 <=0) { neg ++; printf( MAGENTA   "%i\n" MAGENTA,  n2); }
    int temp = n2;

    n2 += n1;
    n1 = temp;
  }

        printf(RESET"\n***************************************************"RESET);
        printf("\nPositive numbers in range %i : %i\n",limit,pos);
        printf("Negitive : %i",neg);
  printf("\n***************************************************");
}

Things you could improve

Using Headers

Right now you are defining some items at the top of your source file.

#define RED     "\x1b[31m"
#define GREEN   "\x1b[32m"
#define YELLOW  "\x1b[33m"
#define BLUE    "\x1b[34m"
#define MAGENTA "\x1b[35m"
#define CYAN    "\x1b[36m"
#define RESET   "\x1b[0m"

These are all relatable, and reusable ANSI escape codes to color your text in a terminal, so you could put them all in their own header (color.h) for easy use with other files.

#define BLACKTEXT(x) "\033[30;1m" x "\033[0m"
#define REDTEXT(x) "\033[31;1m" x "\033[0m"
#define GREENTEXT(x) "\033[32;1m" x "\033[0m"
#define YELLOWTEXT(x) "\033[33;1m" x "\033[0m"
#define BLUETEXT(x) "\033[34;1m" x "\033[0m"
#define MAGENTATEXT(x) "\033[35;1m" x "\033[0m"
#define CYANTEXT(x) "\033[36;1m" x "\033[0m"
#define WHITETEXT(x) "\033[37;1m" x "\033[0m"

Then you can just #include "color.h" in all of your files where you need some color in your terminal.
As you can see from my own color.h header file, I don't have a RESET macro.  I didn't find a use for it, since I would always end up resetting the color right after I used it.  The way I have it defined, my macros can do that for me.

User Experience

I find it useless to have to enter a first and second number.

        printf("Enter first number > ");
  scanf("%d",&n1);
        printf("Enter the seconed number > ");
  scanf("%d",&n2);

What is the point of this?  Just start from \$ 0 \$ and \$ 1 \$, and go to \$ n \$ number of terms.

Efficiency/Algorithm

Right now you can't compute that many fibonacci numbers, because of the maximum size of an int; you can only calculate numbers up to 32767.

We aren't expecting negative numbers in our sequence, so we easily increase this restriction, you could use an unsigned int which has a maximum value of 65535, or even better, a long long unsigned int with a maximum value of 18446744073709551615. That's 562967131565294 times larger than your current program!
Your algorithm can be simplified down using method extraction and recursion.

  for (i; i < limit; i++ ) {
                if (n2 >=1) { pos ++; printf( RESET "%i\n" RESET, n2); }
                if (n2 <=0) { neg ++; printf( MAGENTA   "%i\n" MAGENTA,  n2); }
    int temp = n2;

    n2 += n1;
    n1 = temp;
  }

Move the computing of the fibonacci sequence to it's own method.  All you should be passing it is the number of the term you want to compute.

unsigned long long int fibonacci(unsigned long long int n)
{
    if (memoization[n] != -1) return memoization[n];

    return (n < 2)? n : (memoization[n] = fibonacci(n-1) + fibonacci(n-2));
}

You might notice that I have included an array in my previous point named memoization.  We want to use this to decrease computation time with the recursion.  Basically, if we have already computed a number, we put it into the array so we don't have to calculate it the next time we want to find a fibonacci number.  If we haven't found the number, we can indicate that in the array by storing a -1 there instead.

Style

Put all of your variable declarations to separate lines. From Code Complete, 2nd Edition, p. 759:

  With statements on their own lines, the code reads from top to bottom,
  instead of top to bottom and left to right. When you’re looking for a
  specific line of code, your eye should be able to follow the left
  margin of the code. It shouldn’t have to dip into each and every line
  just because a single line might contain two statements.

I can't follow your tabbing scheme.

        printf("Enter first number > ");
  scanf("%d",&n1);
        printf("Enter the seconed number > ");
  scanf("%d",&n2);
        printf("Enter the number of terms > ");
  scanf("%d",&limit);

This makes it harder for me to read.  You should align all of the left characters in a block to make them more readable.
You should declare your parameters as void if you don't take in any arguments.

int main(void)

Standards

You should be declaring i inside of your for loops.(C99)

for (int i = 0; i < limit; i++)

Final code:

Iterative: \$ O(1) \$

The calculation of the fibonacci sequence here is a bit different than my point I made above.  I didn't mention it, since it was so different than how you were calculating your sequence.  This is one of the most optimal solutions for calculating a fibonacci sequence.

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

long long unsigned int fibonacci(long long unsigned int n)
{
    return round((1/sqrt(5)) * (pow(((1 + sqrt(5)) / 2), n) - pow(((1 - sqrt(5)) / 2), n)));
}

int main(void)
{
    unsigned int num = 0;

    printf("Enter how far to calculate the series: ");
    if (scanf("%i", &num) <= 0)
    {
        puts("Invalid input.");
        return -1;
    }

    for(unsigned int n = 1; n < num + 1; ++n) printf("%llu\n", fibonacci(n));
}

Recursion: \$ O(n) \$

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

#define ARRAYLENGTH 100 // keep in mind the result must fit in an llu int (fibonacci values grow rapidly)
                        // you will run into issues after 93, so set the length at 100
long long int memoization[ARRAYLENGTH];

long long unsigned int fibonacci(long long unsigned int n)
{
    if (memoization[n] != -1) return memoization[n];

    return (n < 2)? n : (memoization[n] = fibonacci(n-1) + fibonacci(n-2));
}

int main(void)
{
    unsigned int num = 0;

    for(unsigned int i = 0; i < ARRAYLENGTH; i++) memoization[i] = -1; // preset array

    printf("Enter how far to calculate the series: ");
    if (scanf("%i", &num) <= 0)
    {
        puts("Invalid input.");
        return -1;
    }
    if (num < ARRAYLENGTH)
    {
        for(unsigned int n = 1; n < num + 1; ++n) printf("%llu\n", fibonacci(n));
    }
    else
    {
        puts("Input number is larger than the array length.");
        return -2;
    }
}


UPenn Homework 3: skips function I've started learning Haskell by following along the CIS 194 Course from UPenn.
In the first lessons there is talk about wholemeal programming and certain ways idiomatic Haskell code looks like.
Now, I think that my style is still far away from being idiomatic and I don't think I've grasped what wholemeal means.
To see how far away, I'd like some comments on code from the third assignment because that's where I struggled most (until now).

So, how far away from being idiomatic is the code? How's efficiency?

Exercise 1: skips :: [a] -> [[a]]

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

skips :: [a] -> [[a]]
skips list =
  let pos 1 l = l
      pos i lst =
        let tmp = drop (i-1) lst
        in case tmp of
            [] -> []
            x:xs -> x : (pos i xs)
      tmpSkps i acc lst = if i == (length lst)+1
                          then acc
                          else tmpSkps (i+1) ((pos i lst):acc) lst
  in reverse (tmpSkps 1 [] list)

The Lecture Notes advocate "wholemeal" programming, which I would summarize as being the use of higher-order functions.  Your code, on the other hand, suffers from the "indexitis" that you are being encouraged to avoid.

The key insight for this exercise is that it resembles a list-chunking problem.  If you transform "hello!" into, say, chunks of increasing length…

[
    ["h", "e", "l", "l", "o", "!"],
    ["he", "ll", "o!"],
    ["hel", "lo!"],
    ["hell"],
    ["hello"],
    ["hello!"]
]

… then the problem boils down to taking the last element of each substring.

skips :: [a] -> [[a]]
skips xs =
  map (map last . fullChunks) $ zip [1..] (replicate (length xs) xs)
  where
    fullChunks (n, ys)
      | n > length ys = []     -- Discard incomplete chunks
      | otherwise     = chunk : fullChunks (n, rest)
      where (chunk, rest) = splitAt n ys

Unfortunately, you can't use the functions provided by Data.List.Split, since incomplete chunks at the end need to be discarded.


Whiten black contours around a skewed image I have this image:

I want to whiten the black contours (borders) around it without affecting the image content.
Here is the code I used:

import cv2
import numpy as np
import shapely.geometry as shageo

img = cv2.imread('filename.jpg')

# get the gray image and do binaryzation
gray = cv2.cvtColor(img, cv2.COLOR_RGB2GRAY)
gray[gray < 20] = 0
gray[gray > 0] = 255

# get the largest boundry of the binary image to locate the target
contours, _ = cv2.findContours(gray, cv2.RETR_EXTERNAL, cv2.CHAIN_APPROX_SIMPLE)
rect = cv2.minAreaRect(contours[0])
box = cv2.boxPoints(rect)
box = np.int0(box)

poly = shageo.Polygon(box)
h, w = img.shape[:2]
ind = np.zeros((h, w), np.bool)

# check if the point is inside the target or not
for i in range(h):
    for j in range(w):
        p = shageo.Point(j, i)
        if not p.within(poly):
            ind[i, j] = True

# whiten the outside points
img[ind] = (255, 255, 255)
cv2.imwrite('result.jpg', img)

Here is the result:

The code works fine, but it's very slow because of the for loops.

Any suggestions how to avoid the for loops or to make them faster?

After some research, I ended to a better and faster solution. Here is the code:

# import packages
import numpy
import mahotas.polygon
import shapely.geometry as shageo
import cv2
import numpy as np

def get_mask(dims, pts):
    # create a numpy array of zeros with the same dimensions of the image
    canvas = numpy.zeros((dims[0], dims[1]), dtype=int)
    # the points coords in the form of pt(y, x)

    # fill the polygon with ones.
    mahotas.polygon.fill_polygon(pts, canvas)
    return canvas

def find_polygon(img):
    # get the gray image and do binaryzation
    gray = cv2.cvtColor(img, cv2.COLOR_RGB2GRAY)
    gray[gray < 20] = 0
    gray[gray > 0] = 255

    # get the largest boundry of the binary image to locate the target
    contours, _ = cv2.findContours(gray, cv2.RETR_EXTERNAL, cv2.CHAIN_APPROX_SIMPLE)
    rect = cv2.minAreaRect(contours[0])
    box = cv2.boxPoints(rect)
    box = np.int0(box)
    poly = shageo.Polygon(box)
    # return the polygone coords in a list
    return list(poly.exterior.coords)

def main():
    img = cv2.imread('filename.jpg')
    # get the coords of the polygon containing (around) the image.
    coords = find_polygon(img)
    poly_coords = []
    # the coords are floats and sometimes are negaive (-1), so transform them into positive ints.
    for element in coords:
        poly_coords.append(tuple(map(int, map(abs, reversed(element)))))

    mask = get_mask(img.shape, poly_coords)
    # convert the mask into array of 0 and 1.
    binary_mask = np.logical_not(mask).astype(int)
    # reshape the array to be similar to the image dimenstions
    binary_mask = binary_mask.reshape(img.shape[0], img.shape[1], -1)
    # sum the binary mask with the image
    cv2.imwrite('res.jpg', img + binary_mask * 255)

main()

I am sure this code can be optimized more, any suggestions are welcome.

Credits:
1- Drawing polygons in numpy arrays
2- Whiten black contours around a skewed image opencv

Here is the result:


MySQL CLI: Create a MySQL user & DB Please review my Bash code to create an authenticated, all privileged MySQL user plus a DB with the same name, through the MySQL CLI:

read -sp "DB user password:" dbrootp_1       && echo
read -sp "DB user password again:" dbrootp_2 && echo
read -sp "DB user password:" dbuserp_1       && echo
read -sp "DB user password again:" dbuserp_2 && echo

if [ "$dbrootp_1" != "$dbrootp_2" ]; then echo "Values unmatched" && exit 1 fi
if [ "$dbuserp_1" != "$dbuserp_2" ]; then echo "Values unmatched" && exit 1 fi

cat <<-DBSTACK | mysql -u root -p"$dbrootp"
    CREATE USER "$domain"@"localhost" IDENTIFIED BY "$dbuserp";
    CREATE DATABASE "$domain";
    GRANT ALL PRIVILEGES ON "$domain".* TO "$domain"@"localhost";
DBSTACK

Just executing the code, I'd receive 4 different inputs like this:

DB user password:
DB user password again:
DB user password:
DB user password again:

which would make me think that some thing went wrong in the first time, and I'd input the same password again. You are not making it obvious for the user to input root password first.

echo "Enter root user credentials"
read -sp "root user password:" dbrootp_1       && echo
read -sp "root user password again:" dbrootp_2 && echo
echo "Enter new user credentials"
read -sp "new user password:" dbuserp_1       && echo
read -sp "new user password again:" dbuserp_2 && echo

makes it slightly better.

if [ "$dbrootp_1" != "$dbrootp_2" ]; then echo "Values unmatched" && exit 1 fi
if [ "$dbuserp_1" != "$dbuserp_2" ]; then echo "Values unmatched" && exit 1 fi

You may wish to convey exactly which set of password was mismatched. It is more a UX debate though. You might not want to do that as it might cause some security vulnerability! However, look at

function error_quit() {
    local EXITCODE=$1
    local MESSAGE="$2"
    >&2 echo $MESSAGE
    exit $EXITCODE
}

[[ "$dbrootp_1" != "$dbrootp_2" ]] && { error_quit 1 "Values unmatched" }
[[ "$dbuserp_1" != "$dbuserp_2" ]] && { error_quit 1 "Values unmatched" }

Splitting into a function gives you an added benefit so that in future you might expand the script, and use the same minimal exit setup instead of copying it everywhere.

The >&2 directs the error to stderr stream.


Alternate threads to print even and odd numbers I have written the following piece of code to have two threads printing odd and even numbers alternatively. I know this is a common question but I wanted to get this reviewed and understand if it is right or wrong. If wrong, can this be tweaked slightly to get desired result or the approach itself is wrong. Thanks for help.

public class ThreadTest implements Runnable{

    private static int counter = 1;
    private static final Object lock = new Object();

    public static void main(String[] args) {
        // TODO Auto-generated method stub

         Thread t1 = new Thread(new ThreadTest(), "arunabh");
         t1.start();
         Thread t2 = new Thread(new ThreadTest(), "hejib");
         t2.start();

    }

@Override
public void run() {
    while (counter<=100) {
        synchronized (lock) {
            if (counter % 2 == 0) {
                System.out.println(counter +" Written By even Thread-"+ Thread.currentThread().getName());
                counter++;
                try {
                    lock.notify();
                    lock.wait();

                } catch (InterruptedException e) {
                    e.printStackTrace();
                }

            } else if (counter % 2 == 1) {
                System.out.println(counter +" Written By odd Thread-"+ Thread.currentThread().getName());
                counter++;

                try {
                     lock.notify();
                     lock.wait();

                } catch (InterruptedException e) {
                    e.printStackTrace();
                }

            }
        }
    }
  }
}

I have two comments of the tweaking kind:

1) any variable that is shared between threads has to be volatile. this is true even for static variables that have one value throughout the JVM (actually it is per class loader but this is irrelevant here). The reason for that is that threads can locally cache the value of variables.  volatile variables force the thread to read the global value each time.

You might also want to take a look at AtomicInteger which is an int that is guaranteed to be incremented synchronously and that has the same value for all running threads.

2) while synchronized will work, Java concurrency package has several Lock mechanisms that allow for better control over the synchronization process.  of relevancy here are ReentrantLock, ReadWriteLock.


Designing unit tests for paramiko wrapper Before I get to the question, let me note that I'm a trained scientist, not a programmer; I've done my best to self-teach what I've needed to know so far, but in the interest of making my code usable for other people in my research group, I'd like to improve my code. I spent a weekend writing actual documentation for stuff, and the next thing on the list of making this more stable is unit testing, which I think I have a basic understanding of, but have never implemented in anything I've written.

I've developed a wrapper for paramiko that allows me load scientific data from a remote host. I was curious about how to best design a unit test for other scripts that rely on this function, or if one is even necessary? If there are any security holes or performance improvements, suggestions would be very appreciated!

def get_remote_data(filepath, time=False, info=False, copy_to_local=False):
    """
    A paramiko wrapper that gets file from a remote computer. Parses
    hostname from filepath. Works only for netcdf files!

    Keyword Arguments:
    filepath         -- the path of the file with hostname.
    time             -- print time required for loading(default False)
    info             -- print some information about the file after loading
                        (default False)
    copy_to_local    -- copies file to local /tmp/remote_data/ and checks
                        if this file already exists there (default False)

    Example:
    >>> get_remote_data('pgierz@rayo3:/csys/paleo2/pgierz/GR30s.nc')
    # TODO: Find out if this is the right way to do this, and make a unit test.
    Paul J. Gierz, Sat Feb 14 14:20:43 2015
    """
    # Import stuff from your own library:
    from UI_Stuff import print_colors
    if time:
        import time
    import paramiko
    import os
    from scipy.io import netcdf
    if time:
        now = time.time()
    print "Trying to load ".ljust(40) \
          +print_colors.WARNING("{f}").format(f=os.path.basename(filepath)).ljust(100)
    if not copy_to_local:
        # We wish to split the filepath to get the username, hostname, and
        # path on the remote machine.
        user = filepath.split(':')[0].split('@')[0]
        host = filepath.split(':')[0].split('@')[1]
        rfile = filepath.split(':')[1]
        # FIXME: This function only works if .ssh/id_rsa exists
        privatekeyfile = os.path.expanduser('~/.ssh/id_rsa')
        mykey = paramiko.RSAKey.from_private_key_file(privatekeyfile)
        client = paramiko.SSHClient()
        client.load_system_host_keys()
        client.set_missing_host_key_policy(paramiko.AutoAddPolicy())
        client.connect(host, username=user, pkey=mykey)
        sftp = client.open_sftp()
        fileObject = sftp.file(rfile)
        file = netcdf.netcdf_file(fileObject)
        pre_s = "Loaded from "+host
    else:
        pre_s, fileObject = _copy_remote_file(file path)  # This is my own function that creates an rsync process
        file = netcdf.netcdf_file(fileObject)
    if time:
        print pre_s.ljust(40) \
        +print_colors.OKGREEN("{filepath}").format(filepath=os.path.basename(filepath)).ljust(100) \
        +" in ".rjust(0) \
        +print_colors.OKBLUE("{time}").format(time=round(time.time()-now)) \
        +" seconds"
    if info:
        s = print_colors.HEADER("#"*30+" INFO of "+os.path.basename(filepath)+" "+"#"*30)
        print s
        print "Variables: \n"
        for k, v in file.variables.iteritems():
            print k, ":  dimensions -"+str(v.dimensions)+" shape - "+str(v.shape)
        print "Dimensions: \n"
        print file.dimensions
        print print_colors.HEADER("#"*len(s))
    return file

Single responsibility principle

This function does too many things:

maybe it copies a remote file
maybe it prints info

The function may also print timing stats, which probably doesn't make much sense when not copying the remote file.

Unit testing is most effective when testing small independent units that have a single responsibility, well-defined inputs and outputs, and ideally no side effects.

As a first step, it would be good to split this function to multiple smaller functions:

def get_remote_file(filepath, time=False):
    # ...

def get_local_file(filepath):
    # ...

def get_file(filepath, time=False, copy_to_local=False):
    if copy_to_local:
        return get_remote_file(filepath, time)
    return get_local_file(filepath)

def print_file_info(filepath, file):
    # ...

Each of these functions have well-defined, simpler responsibilities.
The get_file function may seem to do multiple things,
but it's ok, as it only orchestrates those things,
it's more of a controller and has trivially simple implementation.

Now that you have smaller units,
you can think about testing them individually, if necessary.
Actually I don't see much to test here,
because of the code is either printing,
or making paramiko API calls.
Testing that the paramiko calls work and you can fetch files will not be unit testing anymore,
because it will require external factors correctly setup,
such as a remote server with a file at a specific location,
which would violate the principle of unit testing.
That is not to say it's not worth testing,
maybe it is, to test your setup,
but it's not "unit testing", it's integration testing.

Code review

There is just one part that might be worth testing:

user = filepath.split(':')[0].split('@')[0]
host = filepath.split(':')[0].split('@')[1]
rfile = filepath.split(':')[1]

I suggest to put this in a function of its own:

def split_ssh_filepath(filepath):
    user = filepath.split(':')[0].split('@')[0]
    host = filepath.split(':')[0].split('@')[1]
    rfile = filepath.split(':')[1]
    return user, host, rfile

And eliminate the repeated calls to filepath.split:

def split_ssh_filepath(ssh_filepath):
    user_host_rfile = ssh_filepath.split(':')[:2]
    user, host = user_host_rfile[0].split('@')[:2]
    rfile = user_host_rfile[1]
    return user, host, rfile