OO and non-OO usage of Mysqli I'm starting to learn the object oriented aspect of PHP. In this regard I made a small "exercise" of sorts "translating" one of my PHP functions into OO mode of PHP. I'm asking if there are there any recommendations you can give me concerning my predicament.

Example of non-OO PHP:

<?php
function pristup($servername, $username, $password, $dbname, $sql){

    $conn=new mysqli($servername, $username, $password, $dbname);

    if($conn->connect_error){

        die("Neuspela konekcija: ".$conn->connect_error);

    }

    $result = $conn->query($sql);
    if ($result == TRUE) {
    //echo "Uspela konekcija";
} else {
    echo "Neuspešno izvršavanje upita: " . $conn->error;
}
    return $result;

    $conn->close();

}

?>

Example of this done in an object oriented manner:

<?php

class konekcija{

    private $servername;
    private $username;
    private $password;
    private $dbname;
    private $sql;

    //Setter functions
    public function setVal($par1, $par2, $par3, $par4, $par5){ //Setovanje vrednosti za upit.
    $this->servername = $par1;
    $this->username = $par2;
    $this->password = $par3;
    $this->dbname = $par4;
    $this->sql = $par5;

}

    //Getter functions
    public function getServername() {
        return $this->servername;
    }

    public function getUsername() {
        return $this->username;
    }

    public function getPassword() {
        return $this->password;
    }

    public function getDBname() {
        return $this->dbname;
    }

    public function getSQL() {
        return $this->sql;
    }

    //Function that executes query.
    public function pristup($server_name, $user_name, $pass_word, $db_name, $sql_query){

        $conn=new mysqli($server_name, $user_name, $pass_word, $db_name);

        if($conn->connect_error){

            die("Neuspela konekcija: ".$conn->connect_error);

        }

        $result = $conn->query($sql_query);

        if ($result == TRUE){

        echo "Uspela konekcija";

        }
        else{

                echo "Neuspešno izvršavanje upita: " . $conn->error;

            }

        return $this->$result;

        $conn->close();

    }

}

$kon = new konekcija(); //Creation of an object.

//Setting values.
$kon -> setVal("localhost", "root", "", "test", "CREATE TABLE example(
            id INT(6) UNSIGNED AUTO_INCREMENT PRIMARY KEY,
            firstname VARCHAR(30) NOT NULL,
            lastname VARCHAR(30) NOT NULL,
            email VARCHAR(50),
            reg_date TIMESTAMP
            )");

//Getting values and inserting them into class method.
$kon -> pristup($kon->getServername(), $kon->getUsername(), $kon->getPassword(), $kon->getDBname(), $kon->getSQL());

?>

This works, and I'm asking if this code is any good. Is there a better/more established way of doing this?

Most of the code in this class just makes no sense and written only because it has been shown in some tutorial as an example for some other class.
In case of a database connection class, there is no use for the database credentials' getters. You never need a database host anywhere outside of the database class. Hence, getServername() method will never be used. Neither such class variables are actually needed as you only need them in the constructor. And of course,  there shouldn't be a setVal() method whic is way too localised to be a part of the database class.
From the above, it can be clearly seen that there is no useful functionality in this class, and for a reason: mysqli is already a class, and not the worst one.
In your case, it would be much, much better to learn how to use vanilla mysqli first.
OO usage of mysqli is simple. All you need to know is the main rule of the database interaction in PHP:
A database connection must be created strictly once!
Therefore, you have to create the mysqli object first, and then use this instance for all the database interactions in your script. The simplest code for this would take just three lines:
mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT); // enable error reportig
$conn= new mysqli($host, $user, $pass, $db, $port); // the actual connection
$conn->set_charset($charset); // set connection charset

After this, you can use the instance of mysqli class to run SQL queries OO style:
$sql = "CREATE TABLE example(
        id INT(6) UNSIGNED AUTO_INCREMENT PRIMARY KEY,
        firstname VARCHAR(30) NOT NULL,
        lastname VARCHAR(30) NOT NULL,
        email VARCHAR(50),
        reg_date TIMESTAMP
        )";
$conn->query($sql);

And it would be the perfect example of using mysqli OO style.
Other OO usage  examples of mysqli can be found in this answer


Removes course objects from the course list I have implemented those two methods described as in 1 and 2.
I appreciate any kind of review to improve my method implementation.

  Implement the method toString.
  toString returns a string representation of the list
  Implement the method remove .
  remove removes a given course from the list.
  If the course is not in the list, the list is not updated.

  When the code is executed, the following printout is generated:

  <10, Algebra>

  <20, Algorithms>

  <30, Electronics>

  <40, Physics>

  <50, Programming>

  After the implemented methods call.

  <10, Algebra>

  <20, Algorithms>

  <50, Programming>

public class CourseList {

    private static final int CAPACITY = 5;
    // courses
    private Course[] courses;
    // the number of courses
    private int courseCount;

    public CourseList() {
        courses = new Course[CAPACITY];
        courseCount = 0;
    }

    // add adds a course to the list
    public void add(Course course) {
        courses[courseCount] = course;
        courseCount++;
    }
    public void remove (Course course) {
        int courseIndex = -1;
        for (int i = 1; i < courseCount; i++)
            if (courses[i].equals(course)) {
                courseIndex = i;
                break;
            }
        if (courseIndex != -1) {
            for (int i = courseIndex; i < courseCount - 1; i++)
                courses[i] = courses[i + 1];
            courses[courseCount - 1] = null;
        }
    }

    public String toString(){
        StringBuilder sb = new StringBuilder();
        for(int i = 0; i< courses.length; i++){
            sb.append(courses[i]);
            sb.append("\n");
        }
        return sb.toString();
    }

    public static void main(String[] args) {
        CourseList list = new CourseList();
        list.add(new Course(10, "Algebra"));
        list.add(new Course(20, "Algorithms"));
        list.add(new Course(30, "Electronics"));
        list.add(new Course(40, "Physics"));
        list.add(new Course(50, "Programming"));
        System.out.println(list);
        list.remove(new Course(40, "Physics"));
        list.remove(new Course(30, "Electronics"));
        System.out.println(list);

    }
}

Your toString() method iterates up to courses.length rather than courseCount, so you will end up outputting nulls if the CourseList is not filled to capacity.

Just a minor note: most StringBuilder methods are designed to allow chaining, like this:

sb.append(courses[i]).append("\n");

In your remove() method, you fail to decrement courseCount, so a subsequent add() will result in an awkward null gap.

You should never omit the "optional" braces of a for loop.  Someday, you will contribute to a coding accident, and it will be your fault.  Not to mention, the missing braces make the code look ugly and hard to read.

The method would be clearer and more reusable if you broke out part of the code into an indexOf(Course) method (which could be public or private — your choice).

public void remove(Course course) {
    int courseIndex = this.indexOf(course);
    if (courseIndex >= 0) {
        for (int i = courseIndex; i < courseCount - 1; i++) {
            courses[i] = courses[i + 1];
        }
        courses[--courseCount] = null;
    }
}


A small backup script in Python I work on ubuntu machine and my backup requirements are straightforward. My only requirement is the usual copy paste, but only changed files (ie. the files whose modification-time OR size has changed) should get replaced.

Since I could not find such option in the default nautilus copy-paste (it only allows a merge with a blanket replace), I decided to write a backup script in python myself that I would like to get reviewed. Here is the script for backup.py:

 #!/usr/bin/env python
#@module: backup.py
#@description: Script to take backup to a fixed location
#@author: Prahlad Yeri
#@copyright: MIT Licensed

#from __future__ import print_function
import os
import os.path
import sys
import time
from datetime import datetime
import shutil

backup_loc = '/media/username/1tera/backup'
#backup_loc = '/tmp/backup'

locations = ['/home/username/docs',
    '/home/username/source',
    '/home/username/scripts',
    '/home/username/library',
    '/home/username/programs',
    '/home/username/staging',
    '/home/username/soft',
    '/home/username/Desktop',
    '/home/username/Downloads',
    '/home/username/movies',
    '/home/username/songs',
    ]

if __name__ == "__main__":
    #loop thru the folders
    start = time.clock()
    num=0
    for s in locations: #[0:1]:
        #print s + "\n"
        #files = os.listdir(s)
        print 'listing for '  + s
        for (root, dirs, files) in os.walk(s):
            subpath = root.replace('/home/prahlad','')
            for f in files:
                filename = os.path.join(root, f)
                dfilename = backup_loc + subpath + os.sep + f
                link = ''
                if os.path.islink(filename):
                    link = os.readlink(filename)

                if not os.path.exists(dfilename):
                    #check dirs
                    if not os.path.exists(backup_loc + subpath):
                        os.makedirs(backup_loc + subpath)
                        print 'creating directory: ' + backup_loc + subpath

                    #just copy the files
                    print 'copying from: ' + filename
                    print 'to: ' + dfilename
                    if link == '':
                        shutil.copy2(filename, dfilename)
                    else:
                        os.symlink(link, dfilename)
                    num+=1
                else:
                    sz = os.path.getsize(filename); lm = datetime.fromtimestamp(os.path.getmtime(filename)).timetuple()
                    dsz = os.path.getsize(dfilename); dlm = datetime.fromtimestamp(os.path.getmtime(dfilename)).timetuple()

                    if (sz == dsz and lm == dlm):
                        print 'skipped: ' + dfilename
                        #time.sleep(3)
                    else:
                        #copy the files
                        print 'copying from: ' + filename
                        print 'to: ' + dfilename
                        if link == '':
                            shutil.copy2(filename, dfilename)
                        else:
                            os.symlink(link, dfilename)
                        num+=1

    mins = (time.clock() - start)
    #print "All files copied in %d minutes" % mins
    print "{0} files copied in {1} minutes".format(int(num), round(mins))

I can't think of any way the file size could change without the mtime
changing too. On the other hand, it does no harm to check, aside from
making the code a little more complex.

You use the expression backup_loc + subpath quite often, so I would do
this:

subpath = root.replace('/home/prahlad/','')   # note extra slash
backup_path = os.path.join(backup_loc, subpath)
...
dfilename = os.path.join(backup_path, f)
...
if not os.path.exists(backup_path):
    os.makedirs(backup_path)
    print 'creating directory: ' + backup_path

For these lines, I would do one thing per line, and move the complex
expression into a function:

sz = os.path.getsize(filename); lm = datetime.fromtimestamp(os.path.getmtime(filename)).timetuple()
dsz = os.path.getsize(dfilename); dlm = datetime.fromtimestamp(os.path.getmtime(dfilename)).timetuple()

As follows:

def file_mtime(path):
    return datetime.fromtimestamp(os.path.getmtime(path)).timetuple()
...
sz = os.path.getsize(filename)
lm = file_mtime(filename)
dsz = os.path.getsize(dfilename);
dlm = file_mtime(dfilename)

However, since you're only comparing one timestamp to another, and not
doing anything else with the timestamps, I don't see why you couldn't
just do this:

lm = os.path.getmtime(filename)
...
dlm = os.path.getmtime(dfilename)

time.clock() returns time in seconds, not minutes; and num is
already an int; so:

mins = round((time.clock() - start) / 60)
print "{0} files copied in {1} minutes".format(num, int(mins))


Lock-free thread-safe Fibonacci number generator Here is my implementation of a lock-free thread-safe Fibonacci number generator. Is it correct? The idea is to use an immutable holder for previous and current numbers (since it's hard to change two values atomically at the same time) and AtomicReference which points to the current Fibonacci number.

Lock-free

public class FibonacciSequence {

    private static class FibonacciNumber {
        protected final BigInteger prev;
        protected final BigInteger curr;

        public FibonacciNumber(BigInteger prev, BigInteger curr) {
            this.prev = prev;
            this.curr = curr;
        }

        public FibonacciNumber next() {
            return new FibonacciNumber(curr, prev.add(curr));
        }

        public BigInteger value() {
            return curr;
        }
    }

    private static final class FirstFibonacciNumber extends FibonacciNumber {

        public FirstFibonacciNumber() {
            super(null, BigInteger.valueOf(0L));
        }

        public FibonacciNumber next() {
            return new FibonacciNumber(curr, BigInteger.valueOf(1L));
        }

    }

    private final AtomicReference<FibonacciNumber> currentFibNumberRef;

    public FibonacciSequence() {
        currentFibNumberRef = new AtomicReference<>(new FirstFibonacciNumber());
    }

    public BigInteger next() {
        while (true) {
            FibonacciNumber currFibNumber = currentFibNumberRef.get();
            if (currentFibNumberRef.compareAndSet(currFibNumber, currFibNumber.next()))
                return currFibNumber.value();
        }
    }

Testing

    public static void main(String[] args) {
        testFibonacciNumberCorrectness();
        // todo add thread-safe test
    }

    private static void testFibonacciNumberCorrectness() {
        int[] some = {0, 1, 1, 2, 3, 5, 8, 13, 21, 34, 55, 89, 144,
                       233, 377, 610, 987, 1597, 2584, 4181, 6765, 10946,
                       17711, 28657, 46368, 75025, 121393, 196418, 317811};
        FibonacciNumber currFibNumber = new FirstFibonacciNumber();

        for (int num : some) {
            assert num == currFibNumber.value().intValue();
            currFibNumber = currFibNumber.next();
        }
    }
}

I have taken this problem on as a little study in to lock efficiency, performance, and contention. As a result, I am posting a second answer with some additional information, and a different sort of review.

Note: The full console output from my harness is here in pastebin.com

First, my conclusion is that lock-free fib generator is a mistake. Reentrant locks are also a problem, and that the best performance comes from plain old synchronization.

So, to test this all, I created a basic interface:

package fibgen;

import java.math.BigInteger;

public interface FibGen extends AutoCloseable {

    /**
     * Get the next Fibonacci number in this sequence
     * @return the next number
     */
    public BigInteger next();

    /**
     * How many retries were needed in spin loops.
     * @return the retry count
     */
    public long retryCount();

    /**
     * because there may need to be some clean up in some generators.
     */
    public void close();

}

Right, that's a tool to generate Fibonacci numbers in a way that is independent of the implementation.

Now, putting your original question code in to that framework, so you can see what I did using a familiar starting point, I got:

package fibgen;

import java.math.BigInteger;
import java.util.concurrent.atomic.AtomicLong;
import java.util.concurrent.atomic.AtomicReference;

public class FibonacciSequence implements FibGen {

    @Override
    public void close() {
        // nothing
    }

    private static class FibonacciNumber {
        protected final BigInteger prev;
        protected final BigInteger curr;

        public FibonacciNumber(BigInteger prev, BigInteger curr) {
            this.prev = prev;
            this.curr = curr;
        }

        public FibonacciNumber next() {
            return new FibonacciNumber(curr, prev.add(curr));
        }

        public BigInteger value() {
            return curr;
        }
    }

    private static final class FirstFibonacciNumber extends FibonacciNumber {

        public FirstFibonacciNumber() {
            super(null, BigInteger.valueOf(0L));
        }

        public FibonacciNumber next() {
            return new FibonacciNumber(curr, BigInteger.valueOf(1L));
        }

    }

    private final AtomicReference<FibonacciNumber> currentFibNumberRef;
    private final AtomicLong retries = new AtomicLong();

    public FibonacciSequence() {
        currentFibNumberRef = new AtomicReference<>(new FirstFibonacciNumber());
    }

    @Override
    public BigInteger next() {
        while (true) {
            FibonacciNumber currFibNumber = currentFibNumberRef.get();
            if (currentFibNumberRef.compareAndSet(currFibNumber, currFibNumber.next()))
                return currFibNumber.value();
            retries.incrementAndGet();
        }
    }

    @Override
    public long retryCount() {
        return retries.get();
    }

}

OK, now you know how it hangs together, let's look at some of my output. After doing a lot of output, and running on my dual-core-with-hyperthreading laptop, I get the best results from 6 strategies as:

       fibgen.FibGenQueue Retries        0 Statistics: 50000 BigIntegers with hashXOR 212738101 - Next=7.632us Calc=0.610us all in 104.814ms
 fibgen.FibonacciSequence Retries    74188 Statistics: 50000 BigIntegers with hashXOR 212738101 - Next=5.206us Calc=1.280us all in 91.571ms
        fibgen.FibGenLock Retries        0 Statistics: 50000 BigIntegers with hashXOR 212738101 - Next=6.338us Calc=0.551us all in 91.333ms
       fibgen.FibGenRolfl Retries  4451265 Statistics: 50000 BigIntegers with hashXOR 212738101 - Next=4.677us Calc=0.618us all in 72.015ms
        fibgen.FibGenSync Retries        0 Statistics: 50000 BigIntegers with hashXOR 212738101 - Next=4.695us Calc=0.611us all in 67.888ms

Let me explain the above results, using your generator:

 fibgen.FibonacciSequence Retries    74188 Statistics: 50000 BigIntegers with hashXOR 212738101 - Next=5.206us Calc=1.280us all in 91.571ms

Your generator needed to spin-loop almost 75,000 times in order to generate 50,000 values in the sequence. A quality-checking XOR function has some value 212738101 (this is a good thing, I will explain later), and the average latency on the 'next()' call was 5.2 microseconds, the time to calculate the XOR function was an average of 1.3 microseconds, and the whole sequence was completed, in many threads, in 91.5 milliseconds.

Put another way, you ran 4 threads at 100% for 91.5 milliseconds, and you generated 125,000 fibonacci numbers (but only used 50,000).

Note, when I ran your code, I did it 10 times (after warmups), with the following results:

 fibgen.FibonacciSequence Retries    70284 Statistics: 50000 BigIntegers with hashXOR 212738101 - Next=7.813us Calc=0.677us all in 132.730ms
 fibgen.FibonacciSequence Retries    88643 Statistics: 50000 BigIntegers with hashXOR 212738101 - Next=6.030us Calc=0.695us all in 107.690ms
 fibgen.FibonacciSequence Retries    81224 Statistics: 50000 BigIntegers with hashXOR 212738101 - Next=7.501us Calc=0.879us all in 121.851ms
 fibgen.FibonacciSequence Retries    90017 Statistics: 50000 BigIntegers with hashXOR 212738101 - Next=7.992us Calc=0.749us all in 113.700ms
 fibgen.FibonacciSequence Retries    89161 Statistics: 50000 BigIntegers with hashXOR 212738101 - Next=7.785us Calc=0.964us all in 117.558ms
 fibgen.FibonacciSequence Retries    87775 Statistics: 50000 BigIntegers with hashXOR 212738101 - Next=5.022us Calc=0.912us all in 93.947ms
 fibgen.FibonacciSequence Retries    80012 Statistics: 50000 BigIntegers with hashXOR 212738101 - Next=6.034us Calc=0.650us all in 105.458ms
 fibgen.FibonacciSequence Retries    71632 Statistics: 50000 BigIntegers with hashXOR 212738101 - Next=4.862us Calc=0.640us all in 110.539ms
 fibgen.FibonacciSequence Retries    74188 Statistics: 50000 BigIntegers with hashXOR 212738101 - Next=5.206us Calc=1.280us all in 91.571ms
 fibgen.FibonacciSequence Retries   105237 Statistics: 50000 BigIntegers with hashXOR 212738101 - Next=6.891us Calc=0.905us all in 103.621ms

Note that the fastest run (91.5ms) has the slowest 'Calc' time. I am not sure why this happened. I believe the variances I see in your code are in part caused by the increased thrashing your algorithm puts on the garbage collector. The other algorithms result in more stable performance.

Compare your fastest time to the alternatives. I suggested one in the answer code I supplied before. In my test framework, it looks like:

package fibgen;

import java.math.BigInteger;
import java.util.concurrent.atomic.AtomicLong;
import java.util.concurrent.atomic.AtomicReference;

public class FibGenRolfl implements FibGen {

    @Override
    public void close() {
        // Nothing here..
    }

    protected final AtomicReference<BigInteger> nextRef = new AtomicReference<>(BigInteger.ONE);
    protected final AtomicReference<BigInteger> currRef = new AtomicReference<>(BigInteger.ZERO);
    protected final AtomicLong retries = new AtomicLong();

    @Override
    public BigInteger next() {
        BigInteger current = null;
        long tries = -1L;
        do {
            // spin loop while someone else is updating the reference.
            // updating is indicated by a null value in the reference.
            tries++;
            current = currRef.getAndSet(null);
        } while (current == null);
        BigInteger next = nextRef.get();
        nextRef.set(next.add(current));
        currRef.set(next);
        if (tries > 0L) {
            retries.addAndGet(tries);
        }
        return current;
    }

    @Override
    public long retryCount() {
        return retries.get();
    }
}

Now, I claimed this is an improvement over your code, and, is it? It produces the result:

fibgen.FibGenRolfl Retries  4451265 Statistics: 50000 BigIntegers with hashXOR 212738101 - Next=4.677us Calc=0.618us all in 72.015ms

It ran about 25% faster overall than your code. The individual average latency on each next() call is slightly faster at 4.7us (instead of 5.2). The big difference, is that your code calculations are significantly slower, so my code 'spins' a lot faster.... While your code retried 75,000 times, my code retried 4.5million times. The difference is that your code computes and throws-away a fib number, whereas my code uses it as a spin-lock and only computes the fib number when it is free to do so.... hopefully that reduces memory churn... maybe. I create no garbage in my spins, your code makes two objects each wasted cycle.

So, I run all threads at 100%, but I get results slightly faster, on average, and my overall run time is reduced. I believe the imprved latency is because the BigInteger.add operation is relativey slow, and in your code, the 'latency' of each call is in increments of that time. For example, if an add takes 4us, then your code will return in either 4us, 8us, 12us, 16us, etc. My code, on the other hand, can 'delay' for partial amounts of time, and then return after a single 4us calculation.

But, if the partial delay is beneficial, would a blocking system be better than a spin system? Note that your algorithm kept 4 threads busy for 91.5ms, and mine kept threads busy for 72ms. Combined that's 370ms of CPU time vs. 290ms CPU time

I tried using a Reentrant lock instead, with the code:

package fibgen;

import java.math.BigInteger;
import java.util.concurrent.locks.ReentrantLock;

public class FibGenLock implements FibGen {

    @Override
    public void close() {
        // Nothing here..
    }

    private BigInteger nextRef = BigInteger.ONE;
    private BigInteger currRef = BigInteger.ZERO;

    private final ReentrantLock lock = new ReentrantLock();

    @Override
    public BigInteger next() {
        lock.lock();
        try {
            BigInteger current = currRef;
            currRef = nextRef;
            nextRef = current.add(currRef);
            return current;
        } finally {
            lock.unlock();
        }
    }

    @Override
    public long retryCount() {
        return 0;
    }
}

That code is has about the same latency and overall elapsed time as your code, but, in terms of used CPU time, it blocks all threads except one, so it is 4-times more efficient than your code.

Now, the fastest result was using plain-jane synchronization. I believe it is fastest because there's only one memory-barrier activity (just one synchronization - whereas the Reentrant lock has two - a lock, and an unlock).

Here's the code:

package fibgen;

import java.math.BigInteger;

public class FibGenSync implements FibGen {

    @Override
    public void close() {
        // Nothing here..
    }

    private BigInteger nextRef = BigInteger.ONE;
    private BigInteger currRef = BigInteger.ZERO;

    private final Object lock = new Object();

    @Override
    public BigInteger next() {
        synchronized(lock) {
            BigInteger current = currRef;
            currRef = nextRef;
            nextRef = current.add(currRef);
            return current;
        }
    }

    @Override
    public long retryCount() {
        return 0;
    }
}

Note that this runs in 67ms, and has the same latency as the tight spin-lock mechanism. The major difference is in the fact that it wastes no CPU cycles. It's much more efficient.

That's the best algorithm.

As an aside, I through it would be interesting to try a queue, where one thread feeds Fib numbers in to the queue, and the other threads read it out. It was significantly slower than the alternatives, more complicated, and just generally ugly... here's the code for your reference:

package fibgen;

import java.math.BigInteger;
import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicLong;

public class FibGenQueue implements FibGen, Runnable {

    private final ArrayBlockingQueue<BigInteger> queue = new ArrayBlockingQueue<>(64);
    private AtomicBoolean active = new AtomicBoolean(true);
    private AtomicLong retries = new AtomicLong();

    public FibGenQueue() {
        Thread t = new Thread(this, "FibGen");
        t.setDaemon(true);
        t.start();
    }

    @Override
    public BigInteger next() {
        try {
            do {
                BigInteger fib = queue.poll(1, TimeUnit.SECONDS);
                if (fib != null) {
                    return fib;
                }
                retries.incrementAndGet();
            } while (active.get());
        } catch (InterruptedException ie) {
            Thread.currentThread().interrupt();
        }
        throw new IllegalStateException("Attempt to read from an invalid queue");
    }

    @Override
    public long retryCount() {
        return retries.get();
    }

    @Override
    public void run() {
        BigInteger current = BigInteger.ZERO;
        BigInteger next = BigInteger.ONE;
        try {
            while(true) {
                if (queue.offer(current, 1, TimeUnit.SECONDS)) {
                    BigInteger tmp = next.add(current);
                    current = next;
                    next = tmp;
                } else {
                    if (!active.get()) {
                        return;
                    }
                }

            }
        } catch (InterruptedException ie) {
            Thread.currentThread().interrupt();
        } finally {
            queue.clear();
            active.set(false);
        }

    }

    @Override
    public void close() {
        active.set(false);
    }

}

Now, how was this all tested? Using the following framework, and some Java-8 magic:

package fibgen;

import java.math.BigInteger;
import java.util.ArrayList;
import java.util.List;
import java.util.function.Supplier;
import java.util.stream.Collectors;
import java.util.stream.IntStream;

public class FibTester {

    private static final class BigIntCollector {
        private long nextTime = 0L;
        private long calcTime = 0L;
        private long runTime = -1L;
        private int count = 0;
        private int hasXOR = 0;

        public void accumulate(int index, FibGen gen) {
            long start = System.nanoTime();
            BigInteger big = gen.next();
            long next = System.nanoTime();
            hasXOR ^= big.hashCode();
            long done = System.nanoTime();
            nextTime += next - start;
            calcTime += done - next;
            count++;
        }

        public BigIntCollector merge (BigIntCollector toMerge) {
            nextTime += toMerge.nextTime;
            calcTime += toMerge.calcTime;
            count += toMerge.count;
            hasXOR ^= toMerge.hasXOR;
            return this;
        }

        @Override
        public String toString() {
            double factor = (1000.0 * count);
            return String.format("Statistics: %d BigIntegers with hashXOR %d - Next=%.3fus Calc=%.3fus all in %.3fms", count, hasXOR,
                    nextTime / factor, calcTime / factor, runTime / 1000000.0);
        }

        public void runTime(long time) {
            runTime = time;

        }

    }

    public static final String benchmark(Supplier<FibGen> supplier, int volume) {
        long start = System.nanoTime();
        try (FibGen gen = supplier.get()) {

            BigIntCollector coll = IntStream.range(0, volume).parallel()
                .collect(BigIntCollector::new, (c, i) -> c.accumulate(i, gen), (a,b) -> a.merge(b));

            coll.runTime(System.nanoTime() - start);
            return String.format("%25s Retries %8d %s", gen.getClass().getName(), gen.retryCount(), coll.toString());

        }
    }

    public static void main(String[] args) {
        List<Supplier<FibGen>> suppliers = new ArrayList<>();
        suppliers.add(FibonacciSequence::new);
        suppliers.add(FibGenRolfl::new);
        suppliers.add(FibGenQueue::new);
        suppliers.add(FibGenSync::new);
        suppliers.add(FibGenLock::new);
        dump(suppliers, 100);
        for (Supplier<FibGen> supplier : suppliers) {
            System.out.println(benchmark(supplier, 1000));
        }
        for (int i = 0; i < 100; i++) {
            for (Supplier<FibGen> supplier : suppliers) {
                benchmark(supplier, 1000);
            }
        }
        for (Supplier<FibGen> supplier : suppliers) {
            System.out.println();
            for (int i = 0; i < 10; i++) {
                System.out.println(benchmark(supplier, 50000));
            }
        }
    }

    private static void dump(List<Supplier<FibGen>> suppliers, int count) {
        FibGen[] gens = suppliers.stream().map(s -> s.get()).collect(Collectors.toList()).toArray(new FibGen[suppliers.size()]);
        StringBuilder sb = new StringBuilder();
        for (int i = 1; i <= count; i++) {
            sb.setLength(0);
            sb.append(String.format("%5d ", i));
            for (FibGen fg : gens) {
                sb.append(String.format("%20s ", fg.next()));
            }
            System.out.println(sb.toString());
        }

    }

}


Reimplementation of C++ vector I previously attempted to make a C++ vector here, yet it was not very successful. Now I have made a basic reimplementation of it, so I'm checking that it is fine, and that I will not have to re make it again.

Note: I also implemented an allocator class, but it works the same as the std:: equivalent.

template < typename _Ty, typename _Alloc = allocator<_Ty> > class vector
{
public:
    typedef vector<_Ty, _Alloc> _Myt;
    typedef _Ty value_type;
    typedef _Alloc  allocator_type;
    typedef value_type *pointer;
    typedef const value_type *const_pointer;
    typedef value_type *iterator;
    typedef const value_type *const_iterator;
    typedef value_type &reference;
    typedef const value_type &const_reference;
    typedef std::size_t size_type;
    typedef std::ptrdiff_t difference_type;

    vector()
        : __size(0), __capacity(20), __data(_Alloc().allocate(20))
    {
    }

    vector(const _Myt &_Rhs)
        : __size(_Rhs.__size), __capacity(_Rhs.__size + 20), __data(_Alloc().allocate(_Rhs.__size))
    {
        int count = 0;
        for (iterator i = &_Rhs.__data[0]; i != &_Rhs.__data[_Rhs.__size]; ++i, ++count)
        {
            _Alloc().construct(&__data[count], *i);
        }
    }

    ~vector()
    {
        if (!empty())
        {
            for (iterator i = begin(); i != end(); ++i)
            {
                _Alloc().destroy(i);
            }
            _Alloc().deallocate(__data, __capacity);
        }
    }

    _Myt &push_back(const value_type &_Value)
    {
        if (++__size >= __capacity)
        {
            reserve(__capacity * 2);
        }
        _Alloc().construct(&__data[__size - 1], _Value);
        return *this;
    }

    _Myt &push_front(const value_type &_Value)
    {
        if (++__size >= __capacity)
        {
            reserve(__capacity * 2);
        }
        if (!empty())
        {
            iterator _e = end(), _b = begin();
            std::uninitialized_copy(_e - 1, _e, _e);
            ++_e;
            std::copy_backward(_b, _e - 2, _e - 1);
        }
        _Alloc().construct(&__data[0], _Value);
        return *this;
    }

    _Myt &reserve(size_type _Capacity)
    {
        int count = 0;
        if (_Capacity < __capacity)
        {
            return *this;
        }
        pointer buf = _Alloc().allocate(_Capacity);
        for (iterator i = begin(); i != end(); ++i, ++count)
        {
            _Alloc().construct(&buf[count], *i);
        }
        std::swap(__data, buf);
        for (iterator i = &buf[0]; i != &buf[__capacity]; ++i)
        {
            _Alloc().destroy(i);
        }
        _Alloc().deallocate(buf, __capacity);
        __capacity = _Capacity;
        return *this;
    }

    iterator begin()
    {
        return &__data[0];
    }

    iterator end()
    {
        return &__data[__size];
    }

    size_type size()
    {
        return __size;
    }

    size_type capacity()
    {
        return __capacity;
    }

    bool empty()
    {
        return !__data;
    }

    const_pointer data()
    {
        return __data;
    }

private:
    pointer __data;
    size_type __size, __capacity;
};

STOP USING __ its reserved for the implementation.

Basically stop using underscore until you know the rules.
This code is basically all broken because of this. You code technically is all undefined.
We told you this before. STOP IT.

BUG:

vector(const _Myt &_Rhs)
    : __size(_Rhs.__size), __capacity(_Rhs.__size + 20), __data(_Alloc().allocate(_Rhs.__size))

You are only allocating size space. But your capacity is 20 bytes larger.
Why 20 bytes? Should this not be 20 objects 20 * sizeof(_Ty)

BUG:

You use the allocator construct routines when you are doing placement new. This means the space has not been constructed before. If the location has already got an element in it you must copy over it using the copy constructor.

Thus in

_Myt &push_front(const value_type &_Value)

This line:

   _Alloc().construct(&__data[0], _Value);

You are constructing into location 0 a new object without calling the old objects destructor. If your type is anything non trivial this will screw up any memory management.

BUG

bool empty()
{
    return !__data;  // Both constructors allocate memory.
}                    // So this will never be true.

I think you mean

    return size != 0;

You have a potential leak situation in your reserve().
You provide the strong exception gurantee. BUT if the copy fails (with an exception) then you will leak the buffer. You need to hold the buffer in a smart pointer until you swap so that if there is an exception you gurantee that you deallocate the unused buffer.


MVC - Control Class - Architecture / Design - Structure Review - PHP The client side version in JavaScript is here

This is the server side version of my control module.  How does the structure look.

PostScript

Because this is a small prototype/app I have preferences or things I will consider when I will have a working prototype, they are - testing, try/catch/throw handling of errors, advanced abstractions / design patterns, defensive coding against programmer error / malicious users. Auto-Loading, optimization  Will add these later if the project matures.

<?php
function __autoload( $class_name ) { include 'class.' . $class_name . '.php'; }
$object_c = new CMachine();
$object_c->invoke();

class CMachine
{
    public function invoke()
    {
        $pipe = $this->getPipe();
        switch( $pipe['model'] )
        {
            case 'MUserNew': case 'MUserExist':
                $model_object = new $pipe['model']( new SDB(), new SUniversals() , new SText( $pipe['page'] ), new SMessage() );
                $this->send( $model_object->invoke( $pipe['args'] ) );
                break;
            case 'MUserTry':
                $model_object = new $pipe['model']( new SDB(), new SText( $pipe['page'] ) );
                $test = $model_object->invoke( $pipe['args'] );
                $this->send( $test );
                break;
            case 'MUserAny': case 'MOrb':
                $model_object = new $pipe['model']( new SDB() );
                $this->send( $model_object->invoke( $pipe['args'] ) );
                break;
            default:
                echo " -> Undefined Module Requested";
        }
    }
    private function send( $string_send )
    {
        echo "|A|" . $string_send;
    }
    private function getPipe()
    {
        return json_decode( $_POST['pipe'], true );
    }
}

Would separate those includes a bit differently. I'd use a newline after each include then use a double new line when I wanted to section them off. All one line as you are doing, and not even spaces between them, could make someone think that you have only three includes. Speaking of includes. You are loading all of these classes that you aren't going to use. Take a look a lazy initialization. Assuiming you only need to use those classes once in this class, and that is in that switch statement, then you can just include the correct one via case and avoid the overhead of those other unneeded classes.

Would have to agree with Corbin about this looking more like a dispatcher/router. These are okay, just letting you know that what you have isn't a controller.

Would suggest moving the send() method out of the switch. It doesn't really appear to change much and is just causing you to rewrite code. Assign whatever paramters you need to in the switch and just call the method outside of it. To prevent the default case from doing this, since you mentioned wanting to use try/catch, I would throw an error there instead of using echo.


A small to medium sized pygame game Github Link To Code

Pastebin Link

I am well aware of how terrible the SettingsMenu class is, it's still a work in progress and is the way it is so it can actually work.
Can't paste it directly in the post because it would be mostly code.

I feel like this function needs work, but I've been over it a couple times and can't find anything meaningful to do.

def title_screen():
    background = assets_base_path + 'title_screen.png'
    font_size = 0
    frames = 0
    enter_game = False
    while not enter_game:
        for event_ in pygame.event.get():
            if event_.type == pygame.QUIT:
                quit()
            elif event_.type == pygame.KEYDOWN:
                if event_.key == pygame.K_KP_ENTER or pygame.K_SPACE:
                    enter_game = True
            # print(event_)

        game_display.blit(pygame.image.load(background), (0, 0))
        font = pygame.font.Font(fonts_path + 'Futura.ttf', font_size)
        game_display.blit(font.render(str('An RPG'), True, Color.Black), (500, 111))
        # game_display.blit(font.render(str(frames), True, Color.Goldenrod), (0, 0))

        if font_size < 65: font_size += 1
        if font_size == 65:
            game_display.blit(font.render(str('Press Enter To Play'), True, Color.Black), (345, 400))

        pygame.display.update()
        frames += 1
        sleep(0.013)

I also feel like there's room to improve in this section. Seems like there should be a better way to do it, but I'm not sure what it would be.

rect_ = copy(player)
    if active_keys['w']: rect_.y -= config.player_movespeed_vertical
    if active_keys['s']: rect_.y += config.player_movespeed_vertical
    if active_keys['a']: rect_.x -= config.player_movespeed_horizontal
    if active_keys['d']: rect_.x += config.player_movespeed_horizontal
    rect_.refresh_rect()
    pillar_player_collisions_list = pygame.sprite.spritecollide(rect_, active_pillars, False)
    wall_player_collisions_list   = pygame.sprite.spritecollide(rect_, active_walls, False)
    if len(pillar_player_collisions_list + wall_player_collisions_list) != 0:
        # print('Collisions With Pillars')
        pass
    else:
        # print('No Collisions With Pillars')
        if active_keys['w']: player.y -= config.player_movespeed_vertical
        if active_keys['s']: player.y += config.player_movespeed_vertical
        if active_keys['a']: player.x -= config.player_movespeed_horizontal
        if active_keys['d']: player.x += config.player_movespeed_horizontal
        player.refresh_rect()

Thanks!

With respect to your title_screen function:

You are not using the standard python functions for operating on paths. Try using pathlib:

from pathlib import Path

Assets_base_path = Path('/path/to/assets')

def title_screen():
    background = Assets_base_path / 'title_screen.png'

In your while not enter_game loop, you are setting the enter_game
status from the event loop, but that won't affect anything until the entire loop is completed. I'd suggest moving that code down to the bottom
of the loop.
Your code calls this line:

game_display.blit(pygame.image.load(background), (0, 0))

over and over again. The loaded image should be cached - just load it into a variable outside the loop, instead of reading it from disk each time.
You have this code:

font = pygame.font.Font(fonts_path + 'Futura.ttf', font_size)
game_display.blit(font.render(str('An RPG'), True, Color.Black), (500, 111))

And a similar statement elsewhere in the function. I would suggest writing a helper function that does this work for you. Something like this:

def show_text(locn, text, *, color=None, font=None):
    global Game_display

    if font is None:
        font = get_current_font()

    if color is None:
        color = Color.Black

    Game_display.blit(font.render(text, True, color), locn)

Then you can just write:

  futura = pygame.font.Font(Fonts_path / 'Futura.ttf', font_size)
  # NB: Might have to use str() if pygame.font doesn't support pathlib

  show_text((500, 111), 'An RPG', font=futura)

In your collision-detection code, you have repeated code. I'd suggest that you write a function to handle updating your rectangle position:

future = copy(player)
move_rect(future)
future.refresh_rect()

Furthermore, you are concatenating two lists and then discarding the result, when all you really need to do is check the lists separately:

if pillar_hits or wall_hits:
    pass
else:  # No hits

Finally, you might consider just keeping the future-player variable, which is a copy() of player, when you accept the move:

else:  # No hits
    player = future


Word Counter Project I recently read K&R The C Programming Language and wanted to write a small program to count the occurrence of each word in the input (std-input - I piped a file with all shakespeare pieces in there).

Since I wanted to learn how to split source files into multiple files with this project I pretty much outsources every function I could think of.
This is the part I'm most unsure, whether its bad practice or not (I include multiple libraries from the std-lib multiple times. Is this a bad thing?). You'll see.

find_words.c

#include <stdio.h>
#include <ctype.h>
#include <stdlib.h>
#include "word_finder.h"
#include "binary_tree.h"

#define MAX_WORD 20

int main(int argc, char *argv[])
{
    char word[MAX_WORD];
    struct tnode *root = NULL;
    int min_count = 1500;
    int max_count = 20000;

    if (argc > 3) {
        printf("Usage: ./find_words [min_count] [max_count] <INPUT_PIPE\n");
        return 0;
    }

    if(argv[1] != NULL)
        min_count = atoi(argv[1]);
    if(argv[2] != NULL)
        max_count = atoi(argv[2]);

    while(get_word(word, MAX_WORD) != 0)
        if(isalpha(word[0]))
            root = addtree(root, word);

    treeprint(root, min_count, max_count);

    return 1;
}

word_finder.h

int get_word(char *word, int limit);

word_finder.c

#include <ctype.h>
#include <stdio.h>
#include "io.h"

int get_word(char *word, int size)
{
    int c;
    while((isspace(c = getch())))
        ;

    if (c == EOF)
        return 0;

    int i = 0;
    do
    {
        *(word+i++) = c;
    } while (isalpha(c = getch()) && i < size-1);

    *(word+i) = '\0';
    ungetch(c);

    return i;
}

io.h

int getch(void);
void ungetch(int);

io.c

#include <stdio.h>
#include "io.h"

#define BUFSIZE 100

/* Buffer can hold up to BUFSIZE - 1 characters */
char buf[BUFSIZE];
int bufhead = 0;
int buftail = 0;

int getch(void)
{
    if (bufhead == buftail)
        return getchar();
    else {
        int temp = buf[buftail];
        buftail = (buftail + 1) % BUFSIZE;
        return temp;
    }
}

void ungetch(int c)
{
    if ((bufhead + 1) % BUFSIZE == buftail % BUFSIZE) {
        fprintf(stderr, "Buffer full, dropped %c.\n", c);
    } else {
        buf[bufhead] = c;
        bufhead = (bufhead + 1) % BUFSIZE;
    }
}

binary_tree.h

struct tnode {
    char *word;
    int count;
    struct tnode *right;
    struct tnode *left;
};

struct tnode *addtree(struct tnode *, char *);
void treeprint(struct tnode *, int, int);

binary_tree.c

#include "binary_tree.h"
#include <stdlib.h>
#include <stdio.h>
#include <string.h>

static char *strduplicate(const char *);
static struct tnode *talloc(void);

struct tnode *addtree(struct tnode *node, char *word)
{
    int cond;

    if(node == NULL) {  //The word doesnt exist in the tree
        node = talloc();
        node->word = strduplicate(word);
        node->count = 1;
        node->left = node->right = NULL;
    } else if((cond = strcmp(word, node->word)) == 0) {
        node->count++;
    } else if(cond < 0) {
        node->left = addtree(node->left, word);
    } else {
        node->right = addtree(node->right, word);
    }

    return node;
}

static struct tnode *talloc(void)
{
    return (struct tnode *) malloc(sizeof(struct tnode));
}

static char *strduplicate(const char *s)
{
    char *p;

    p = (char *) malloc(strlen(s)+1);
    if(p != NULL)
        strcpy(p,s);

    return p;
}

void treeprint(struct tnode *node, int min_count, int max_count)
{
    if(node != NULL) {
        treeprint(node->left, min_count, max_count);
        if(node->count >= min_count && node->count <= max_count)
            printf("%4d %s\n", node->count, node->word);
        treeprint(node->right, min_count, max_count);
    }
}

I'm sorry for the long post (I'm not 100% sure if this is allowed - I'll delete the post immediately if it's not).

"whether its bad practice or not" to sub-divide.  No, not bad practice - in fact good in certain areas.  It often comes down to maintenance - how many functions per file and the optimal answer is highly problem dependent.  IMO, too often files need fewer functions than code needs fewer files.
ungetch() have troubles as it ungets a EOF.  This special value should not be unget-able.  Further – I have doubts that when char is signed, that these 2 functions properly handle some char values as the real getc() returns unsigned char and EOF.  Example: If (char) 255 is un-gotten, on get, it may have the value of EOF.   Recommend making your unget buffer unsigned char.
Unclear to the whole need for user unget/get functionality as standard library ones well handle 1 level of get/unget.
The whole addtree(), should input be alphabetical, devolves into a linked list.  Consider AVL trees.
Below test i against size after the assignment.  Recommend test before to cope with pathological size values like 1.  Recommend re-write - avoiding code like (word+i++).  Further, consider size_t rather than int for array indexes.

// from
do {
  *(word+i++) = c;
} while (isalpha(c = getch()) && i < size-1);

// to
if (size > 0) size--;
while (i < size) {
  word[i++] = c;
  c = getch()
  if (!isalpha(c)) {
    break;
  }
}

MAX_WORD is fuzzy.  Suggest MAX_WORD_SIZE.
ungetch() and getch() look way too much like standard functions.  Suggest io_ungetch() and io_getch().
Cast not needed.

// return (struct tnode *) malloc(sizeof(struct tnode));
return malloc(sizeof(struct tnode));

// p = (char *) malloc(strlen(s)+1);
p = malloc(strlen(s)+1);

Magic number 4.  Why 4?

// printf("%4d %s\n", node->count, node->word);
printf("%d %s\n", node->count, node->word);

If a function does not modify a pointer's contents, use const.

// void treeprint(struct tnode *node, int min_count, int max_count)
void treeprint(const struct tnode *node, int min_count, int max_count)

Minor: .h file void ungetch(int); --> void ungetch(int ch);  Also add a little documentation of the .h files, it goes a long way in understanding.
Minor: spelling: doesnt --> doesn't
Pedantic point: char may be signed and is...() defined for unsigned char and EOF, not signed char.

// if(isalpha(word[0]))
if(isalpha((unsigned char) word[0]))


Simplify 2 filters on the same array I have an array of number objects, which can be selected only if the orderable quantity is greater than 0.

I have a select all checkbox that is checked or unchecked accordingly with this function:

self.allSelected = function () {
  return self.shownNumbers().filter(function (number) {
    return number.isSelected();
  }).length === self.shownNumbers().filter(function (number) {
    return number.orderableQty > 0;
  }).length;
}

I can also iterate over the array and return the result:

self.allSelected = function () {
  var s = 0, o = 0;
  for (var i = 0; i < self.shownNumbers().length; i += 1) {
    if (self.shownNumbers()[i].isSelected()) s ++;
    if (self.shownNumbers()[i].orderableQty > 0) o ++;
  }
  return s === o;
}

The second one seems more readable and probably is more efficient since it iterates the array only once. Is there a way to use a single filter, map or other Array.prototype built-in function in order to reduce the code and make it more readable or should I keep the single loop option?

Do either of these implementation really do what you seem to say they do (ensure that all visible items that are orderable are selected?)  Perhaps it's hard to tell without understanding whether one can "select" something that is "shown" but not "orderable".  If they can, then I think your logic is wrong and you should be comparing that all three number "shown", "selected", and "orderable" are equal.

I would consider refactoring. Just as you have methods for shownNumbers() why not add methods for getting all selected items and getting all items with orderable quantity.  So, perhaps something like this:

self.allselected = function () {
    return (self.shownNumbers().length === self.filterSelected().length &&
        self.shownNubers().length === self.filterNonzeroQuantity().length);
}

self.getAllSelected = function () {
    return self.shownNumbers().filter(function (number) {
        return number.isSelected();
    }
}

self.getAllOrderable = function () {
    return self.shownNumbers().filter(function (number) {
        return number.orderableQty > 0;
    }
}

Or, if you don't want to refactor then combine the conditions in a single filter.  This may or may not work depending on the my very first comments around method logic.

self.allselected = function () {
    var shownNumbers = self.shownNumbers();
    var selectedAndOrdereable = shownNumbers.filter(function (number) {
        return (number.isSelected() && number.orderableQty > 0);
    };
    return selectedAndOrderable.length === shownNumbers.length;
}

In either case, don't call shownNumbers() repeatedly in single method.  It is unnecessary. Get the result of shownNumbers() and work with it.

Consider naming your method something like areAllOrdereablesSelected() that a) indicates to caller that return value will be boolean (similar isSelected()) and b) better hints are what the underlying logic that the boolean represents is.   I would think allSelected as a method name would do what your first comparison condition in this method does - get all selected elements.

You might be able to optimize for performance and/or memory utilization by implementing loops and counters, but you must decide if it is worth it.


Displaying alert from an action This code works great, but I think it can be reduced.

@if (Session["Success"] != null)
{
    <text>

@Session["Success"]

    </text>
    Session.Remove("Success");
}
else if (Session["Error"] != null)
{
    <text>

@Session["Error"]

    </text>
    Session.Remove("Error");

}

It's a bad idea to modify the session state from inside a view or a partial view. Instead, I would create an alert view model

public class AlertViewModel
{
    public AlertType AlertType { get; set; }
    public string Message { get; set; }
}

public enum AlertType
{
    None,
    Success,
    Error
}

and use that from inside the view/partial view, for example:

@if (Model.AlertType != AlertType.None)
{
    string alertClass = Model.AlertType.ToString().ToLowerInvariant();

    <text>

@Model.Message

    </text>
}

This way, you avoid modifying the session where someone else may not expect it, and you have a cleaner view.

About populating the view: I do not know why you are using the session, but if the value is only needed for one request, you should consider using TempData instead. In any way, just populate the view model in the controller and remove the session value if you need to.


Simple registration with NodeJS and Mongoose I'm new to Node.js (and also backend) for only a week, and now I'm trying to do a simple registration. I found that it's really hard to reduce complexity because pretty much every callback is error-first. Is there any way to reduce this? I found it seems impossible to avoid all of those if-elses.

const User = require('../../models/user')
const ERROR_MESSAGES = require('../../models/constant').ERROR_MESSAGES
const RESPONSE_STATUS = require('../../models/constant').RESPONSE_STATUS
const helper = require('../../helpers/helper')
const validator = require('validator')

module.exports.register = function(req, res) {
    const email = req.body.email
    const password = req.body.password
    const errors = []

    // input validation
    if (!email || email.trim().length == 0) {
        errors.push(ERROR_MESSAGES.EMPTY.EMAIL)
    } else if (!validator.isEmail(email)) {
        errors.push(ERROR_MESSAGES.INVALID.EMAIL)
    }

    if (!password) {
        errors.push(ERROR_MESSAGES.EMPTY.PASSWORD)
    }

    if (errors.length > 0) {
        return res.json(helper.generateResponse(errors, null, RESPONSE_STATUS.REQUEST_DENIED))
    } else {
        User.findOne({email: email}, function(err, user){
            if (err) {
                res.json(helper.generateResponse([ERROR_MESSAGES.UNKNOW], null, RESPONSE_STATUS.REQUEST_DENIED))
            } else if (user) {
                res.json(helper.generateResponse([ERROR_MESSAGES.DUPLICATE.EMAIL], null, RESPONSE_STATUS.REQUEST_DENIED))
            } else {
                var user = new User()
                user.email = email
                user.password = user.generateHash(password)

                var errorValidation = user.validateSync();

                if (errorValidation) {
                    // input validation
                    errors = helper.getMongooseValidationErrors(errorValidation.errors)
                    res.json(helper.generateResponse(errors, null, RESPONSE_STATUS.REQUEST_DENIED))
                } else {
                    user.save(function(error, newUser) {
                        if (error) {
                            res.json(helper.generateResponse([error], null, RESPONSE_STATUS.REQUEST_DENIED))
                        } else {
                            res.json(helper.generateResponse(errors, newUser, RESPONSE_STATUS.SUCCESS))
                        }
                    })
                }
            }
        })
    }
}

Well, the code is fairly simple, but you've made it complex due to a bad design. I would encourage the following:

Use Promises. Mongoose implements promises and so does express, so most of the times you can avoid using callbacks. Also implement your own helpers with promises. Further more you can read up on Async/Await too.
Read up on ES6. Use Arrow-functions, const/let variables, etc. It's not just syntactic sugar, but also more consistent overall.
Destructure package variables and arrays. It shortens your code and makes it easier to read.
Use functions. Functions are your friends. Get used to breaking down problems into small pieces and code into small functions. As a rule of thumb no function should have more that two indentations. If you need to nest many if-else clauses, then function them.
Use semicolons. Even though JS accepts code without semicolons, it's a good practice to use them because it makes the code more readable and less error-prone.

Now, getting back to your code... I made a small refactorization so you can see how it should look, but it needs a complete refactor of architecture as well as code because imo there are many anti-patterns and wrong responsibility assignation, i.e. validations should be frontend, the constants should be in the same file they are used, etc. Nonetheless, this last is more opinion-based.

'use strict';

const User = require('../../models/user'),
    { ERROR_MESSAGES, RESPONSE_STATUS } = require('../../models/constant'),
    helper = require('../../helpers/helper'),
    validator = require('validator');

const register = (req, res) => {
    const email = req.body.email;
    const password = req.body.password;
    const errors = [];

    // input validation
    validate(email, password);

    if (errors.length > 0) {
        return res.json(helper.generateResponse(errors, null, RESPONSE_STATUS.REQUEST_DENIED))
    } else {
        User.findOne({email: email})
        .then(user => {
            if (user) {
                res.json(helper.generateResponse([ERROR_MESSAGES.DUPLICATE.EMAIL], null, RESPONSE_STATUS.REQUEST_DENIED))
            } else {
                createUser(email, password);
            }
        })
        .catch(err => {
            res.json(helper.generateResponse([ERROR_MESSAGES.UNKNOW], null, RESPONSE_STATUS.REQUEST_DENIED))
        });
    }
}

const validate =  (email, password) => {
    if (!email || email.trim().length == 0) {
        errors.push(ERROR_MESSAGES.EMPTY.EMAIL)
    } else if (!validator.isEmail(email)) {
        errors.push(ERROR_MESSAGES.INVALID.EMAIL)
    }

    if (!password) {
        errors.push(ERROR_MESSAGES.EMPTY.PASSWORD)
    }
};

const createUser = (email, password) => {
    let user = new User()
    user.email = email
    user.password = user.generateHash(password)

    const errorValidation = user.validateSync();
    if (errorValidation) {
        errors = helper.getMongooseValidationErrors(errorValidation.errors)
        res.json(helper.generateResponse(errors, null, RESPONSE_STATUS.REQUEST_DENIED))
    } else {
        saveUser(user);
    }
};

const saveUser = user => {
    user.save()
        .then(newUser => res.json(helper.generateResponse(errors, newUser, RESPONSE_STATUS.SUCCESS)))
        .catch(error => res.json(helper.generateResponse([error], null, RESPONSE_STATUS.REQUEST_DENIED)));
};

module.exports = { register };

As I said earlier, I'd suggest a complete refactor not just making syntactic changes like the ones I commented, but also architecture changes. If you need to decide how-to structure the project, etc. Ask other questions in Stackoverflow, maybe :)


Click textbox to show answer and highlight related text Is this JavaScript code robust? It is for an interactive book about a card game. When the reader clicks on the checkbox, an answer is revealed and related text is highlighted. I need to get this right before I write more chapters.

JSBin

<!DOCTYPE html>

   <title>Quiz</title>

<form name="form1">

Q1: 3  5   10
 Click
<input onclick="hlt_showhide(this)" type="checkbox" name=""  answer="1">

A1:  revealed text refers to 2nd & 3rd highlighted numbers

Q2:  7  28  37
 Click
<input onclick="hlt_showhide(this)" type="checkbox" name="" answer="2">

A2:  revealed text refers to 1st & 3rd highlighted numbers

</form>

Here are my thoughts:

Most importantly from the first look at the code: Use proper variable names. No one knows what you mean with EEE, xxx or e2. And while you are at it: in JavaScript it's a convention to start variable names with a lower case letter.
Don't make up attributes such as "answer" - you can't say if an unknown browser can work with them. At least you should adhere to HTML5 and prefix the custom attribute with data-.
name is not a valid attribute for span (or any other object other than form elements). Unfortunately there is no simple, cross-browser alternative for selecting multiple elements. The easiest soltuion with be to use a class and getElementsByClassName, however the latter isn't supported by all browsers yet, but there are implementations for the non-supporters easily available on the web.
The "Click" text is bad for two reasons: A) You can't actually click on it and B) texts such as "click here" is bad UI. Instead wrap the text together with the checkbox in a <label> element, such allows the user to click on the text to activate the checkbox and use a text such as "Show solution" instead.
Finally, there are more elegant ways to switch on the answers. Instead of looking of a specifc ID, set (or remove) a class on an element surrounding the question and use CSS to highlight/show the sub elements. Example: http://jsbin.com/ayoqem/7/edit


Remove duplicates from a sorted array I have solved the LeetCode Remove Duplicates From Sorted Array problem:

  Given a sorted array, remove the duplicates in place such that each element appear only once and return the new length. It doesn't matter what you leave beyond the new length.

I was able to come up with a approach but only when I submitted this solution and got it accepted I found that my solution ranked somewhere around 60%, ranked by run-time. i.e) My solution was only faster that 40% of other contestants solution for the same language.

  int removeDuplicates(vector<int>& nums) {
    if (nums.size() == 0)
    {
        return 0;
    }
    auto start = nums.begin(), end = nums.end();
    int last_value = *(nums.end() - 1);
    for (auto it = nums.begin(); it != end;)
    {
        int find_value = *it;
        auto upper = upper_bound(start, end, find_value); //Find the last occurring index of this number
        iter_swap(start, it);
        ++start;
        if ((upper != end) || ((upper == end) && (find_value == last_value)))
        {
            it = upper;
        }
        else
        {
            ++it;
        }
    }
    return distance(nums.begin(), start);
}

Could you please suggest any performance improvements to this code. And also please go easy on the naming convention and code formatting. This was written for a programming challenge :)

Edit.

The worst case time complexity i believe is O(n Log n) which happens when the entire array is unique. And the best case run time should be O(log n) when the entire array contains duplicates.

So the simplest solutions is to use the available stl function. However there is obviously less to learn there.

#include <algorithm>
#include <vector>

int removeDuplicates(std::vector<int>& nums) {
    nums.erase(std::unique(nums.begin(), nums.end()), nums.end()); ;
    return nums.size();
}


Five functions to get the digits of a number I'm writing a simple function: given a number, it will return a sequence or a collection of the digits in the number (in the correct order). i.e (get-digits-fn 1234567) => (1 2 3 4 5 6 7) / [1 2 3 4 5 6 7]

Below are five attempts at the same function:

(defn get-digits-1 [num]
  (->> [num '()]
       (iterate (fn [[num digits]]
                  (when (> num 0)
                    [(quot num 10) (conj digits (rem num 10))])))
       (take-while some?)
       (last)
       (second)))

(defn get-digits-2 [num]
  (when (> num 0)
    (lazy-seq (concat (get-digits-2 (quot num 10)) '((rem num 10))))))

;; Suggested by Carcigenate
(defn get-digits-3 [num]
  (->> (str)
       (map str)
       (map int)
       (into '())))

(defn get-digits-4 [num]
  (loop [n num
         res '()]
    (if (= n 0)
      res
      (recur (quot n 10) (conj res (rem n 10))))))

(defn get-digits-5 [num]
  (->>
   (iterate (fn [[n digits]]
              [(quot n 10) (conj digits (rem n 10))])
            [num '()])
   (drop-while #(not= 0 (first %)))
   (first)
   (second)))

A helper function for testing performance:

(defn quick-bench-get-digits [fn range]
  (quick-bench (->> range
                    (map fn)
                    (map (partial apply vector))
                    (into []))))

The perf results (output truncated to only show execution time mean):

eul> (quick-bench-get-digits get-digits-1 (range 1 100000))
Execution time mean : 129.516521 ms

eul> (quick-bench-get-digits get-digits-2 (range 1 100000))
Execution time mean : 128.637055 ms

eul> (quick-bench-get-digits get-digits-3 (range 1 100000))
Execution time mean : 24.267716 ms

eul> (quick-bench-get-digits get-digits-4 (range 1 100000))
Execution time mean : 25.083393 ms

eul> (quick-bench-get-digits get-digits-5 (range 1 100000))
Execution time mean : 145.430443 ms

It looks like get-digits-3 is the fastest while get-digits-4 is closely behind. (As the numbers increase, get-digits-3 outperforms get-digits-4. i.e try (range 1000000 2000000))

Any way to increase performance more without leaving Clojure land?
If mutability and Java inter-op is allowed, is there a way to increase performance?

p.s. functions 1 and 5 are almost identical. This was incremental exploration.

I'm afraid your code has errors,  so your benchmarking is invalid.

The get-digits-3 function is wrong.
A corrected version runs about twelve times slower than
get-digits-4.

Repairing get-digits-3

Your get-digits-3 doesn't work:

=> (get-digits-3 123456789)
()

The function chain should be applied to the argument num:

(defn get-digits-3 [num]
  (->> num
       (str)
       (map str)
       (map int)
       (into '())))

But this fails:

=> (get-digits-3 123456789)
ClassCastException java.base/java.lang.String cannot be cast to java.base/java.lang.Character  clojure.lang.RT.intCast (RT.java:1213)

The culprit is the (map int) line. You can't apply int to strings. If you drop the offending line, you get the character values:

=> (get-digits-3 123456789)
(57 56 55 54 53 52 51 50 49)

Notice, too, that they are in reverse order. So we

subtract the value of character 0 and
conj onto the other end by using a vector.

This gives us ...

(defn get-digits-3 [num]
  (->> num
       (str)
       (map int)
       (map #(- % (int \0)))
       (into [])))

=> (get-digits-3 123456789)
[1 2 3 4 5 6 7 8 9]

Benchmarking

I went about this differently:

I used a single example instead of a range.
I used Criterium's bench rather than its quick-bench.

The results were as follow:

For your original (faulty) version of get-digits-3:

=> (bench (get-digits-3 123456789))
...
             Execution time mean : 228.998292 ns

For the corrected version:

=> (bench (get-digits-3 123456789))
...
             Execution time mean : 3.440024 µs

And for get-digits-4:

=>(bench (get-digits-4 123456789))
...
             Execution time mean : 247.502418 ns

This confirms that your faulty get-digits-3 is about as fast as get-digits-4. And get-digits-3 ignores its argument, so is bound to get relatively faster as the numbers get bigger. You can see that the corrected version is nowhere near competitive.


Form Validation (JQuery) I made a simple form validation. It's working alright. The thing is I want my code to be optimized. Can somebody show me some pointers to improve my code? Thanks in advance.

$(document).ready(function(){

$('.submit').click(function(){
     validateForm();
});

function validateForm(){
    var nameReg = /^[A-Za-z]+$/;
    var numberReg =  /^[0-9]+$/;
    var emailReg = /^([\w-\.]+@([\w-]+\.)+[\w-]{2,4})?$/;

    var names = $('#nameInput').val();
    var company = $('#companyInput').val();
    var email = $('#emailInput').val();
    var telephone = $('#telInput').val();
    var message = $('#messageInput').val();

    var inputVal = new Array(names, company, email, telephone, message);

    var inputMessage = ["name", "company", "email address", "telephone number", "message"];

    var textId = ["#nameLabel", "#companyLabel", "#emailLabel", "#telephoneLabel", "#messageInput"];

     $('.error').hide();

        if($.trim(inputVal[0]) === ""){
            $('#nameLabel').after(' Please enter your ' + inputMessage[0] + '');

        }else if(!nameReg.test(names)){
            $('#nameLabel').after(' Letters only');
        }

        if($.trim(inputVal[1]) === ""){
            $('#companyLabel').after(' Please enter your ' + inputMessage[1] + '');
        }

        if($.trim(inputVal[2]) === ""){
            $('#emailLabel').after(' Please enter your ' + inputMessage[2] + '');

        }else if(!emailReg.test(email)){
            $('#emailLabel').after(' Please enter a valid email address');
        }

        if($.trim(inputVal[3]) === ""){
            $('#telephoneLabel').after(' Please enter your ' + inputMessage[3] + '');

        }else if(!numberReg.test(telephone)){
            $('#telephoneLabel').after(' Numbers only');
        }

        if($.trim(inputVal[4]) === ""){
            $('#messageLabel').after(' Please enter your ' + inputMessage[4] + '');
        }

    return false;

}

});

First of all, you shouldn't use $('.submit').click() but $('#myForm').submit() as you will be able to submit forms by pressing enter too.
To check if a string is a number, use !isNan(string) (see this answer for more information).
Email regex are difficult things. Just check if there is someting before the @ (with .+?) and if there is something after the @ (again with .+?) and if that last part contains a . and a couple of characters (from 2 to 6)
It would make the function a lot easier if you put all specific code outside it. E.g:

    validateForm({
        '#nameInput': {
            'valid': function (value) {
                return /^[a-z]+$/i.test(value);
            },
            'label': '#nameLabel',
            'messages': {
                'empty': 'Please enter your name',
                'invalid': 'Letters only',
            },
        },
        '#companyInput': {
            'label': '#companyLabel',
            'messages': {
                'empty': 'Please enter your company',
            },
        },
        // ...
    });


A node CLI to start new projects I was annoyed to manually start new projects each time so I made a Node CLI that automate the process. I'm really not comfortable yet with node, so I wanted some advice. It works but I don't know if I did it the node way or not.

Basically the code under here is my whole application. The file structure is like so:

- app.js
- seed

It's not really complicated. What I do in the app is copy the seed and change the project name in package.json. The seed is a template project, with nothing in it beside a starter-repo. I could have upload it to git but this is more fun.

#!/usr/bin/env node

const spawn = require('child_process').spawn;
const fs = require("fs");
const path = require('path');
const through2 = require('through2');
const copydir = require('copy-dir');

const name = process.argv[2] || "unnamed project";
const projPath = path.join(process.cwd(), name);
// path to my seed project
const seed = path.join(__dirname, "seed");

if (!fs.existsSync(name)) {
    fs.mkdirSync(name);
} else {
    throw new Error(`directory ${name} already exist`);
}

copydir(seed, path.join(process.cwd(), name), function(err) {
    if (err) {
        console.log(err);
        process.exit(1);
    }
    const package = path.join(projPath, "package.json");
    // in my seed project package.json, I have name: "-your-app-name-here-"
    replaceInFile(package, "-your-app-name-here-", name, onCopyDone);
});

function onCopyDone(err) {
    if (!err)
        install(projPath)
    else
        console.log(err)
}

function replaceInFile(someFile, rep, by, cb) {
    fs.readFile(someFile, 'utf8', function(err, data) {
        if (err) {
            return console.log(err);
        }
        let result = data.replace(rep, by);

        fs.writeFile(someFile, result, 'utf8', cb);
    });
}

function install(where) {
    console.log('installing dependencies...')
    const i = spawn('npm', ['install', '--prefix', where]);

    i.stdout.on('data', (data) => {
        console.log(`stdout: ${data}`);
    });
}

Primarily what I'd like to know is how I can do it more the Node way, if that makes sense.

Avoiding else

If you can avoid else then it can simplify indentation. For example:

if (!fs.existsSync(name)) {
    fs.mkdirSync(name);
} else {
    throw new Error(`directory ${name} already exist`);
}

The error can be thrown the error if the name exists, but if not just go ahead with the call to fs.mkdirSync():

if (fs.existsSync(name)) {
    throw new Error(`directory ${name} already exist`);
}
fs.mkdirSync(name);

In some cases you may have to manually return or exit if the statement inside the first conditional doesn't return early or terminate the process - e.g. inside onCopyDone().

Use const instead of let when appropriate

It is wise to get in the habit of using const for any value that doesn't get re-assigned. Then if you need to have a variable that changes, switch to using let. This affects variables like result in the callback inside replaceInFile().

Arrow functions

The parentheses are not necessary on arrow functions with only one argument. Also, a single line doesn't need to have brackets - while this would lead to the return value of the statement getting returned, it shouldn't matter in most cases - so this:

i.stdout.on('data', (data) => {
    console.log(`stdout: ${data}`);
});

can be simplified to:

i.stdout.on('data', data => console.log(`stdout: ${data}`));


Iterate list to map entries in Python I have two files, namely:

File1:

CL1 AA  XX  YY  ZZ  SS \n
CL2 3_b AA

File2:

AA  string1
AA  string2
3_b string3

My expected output is:

CL1 AA  string1
CL1 AA  string2
CL2 3_b string3
CL2 AA  string1
CL2 AA  string2

For this I wrote a following code:

import numpy as np
print("Reading Files...")
header = open('File1', 'r')
cl = header.readlines()
infile = np.genfromtxt('File2', dtype='str', skip_header=1)
new_array = []

for j in range(len(infile)):
    for row in cl:
        element = row.split("\t")
        ele_size = len(element)
        for i in range(0, ele_size):
            if np.core.defchararray.equal(infile[j,0], element[i]):
                clust = element[0]
                match1 = infile[j,0]
                match2 = infile[j,1]
                combo = "\t".join([clust, match1, match2])
                new_array.append(combo)

np.savetxt('output.txt',new_array, fmt='%s', delimiter='\t')

This generates the output I desire. But since the file has some  700000 lines in file2 and some 65000 clusters, it takes a huge time to iterate. Can anyone suggest an efficient way to parse it?

Is it possible to keep first file as a list and the second file as a dictionary, and then iterate over key values?

Your code does loop 700 000 multiplied by 65 000 multiplied by the number of elements in each cluster. That is a lot of iterations, and not very useful.

The better approach would be to read the smaller file into memory, and then read the larger file line by line. In addition as you iterate over each row in the smaller file, matching each of the keys, it makes sense to switch from a dict with cluster as key, and the different keys as values, to actually using the keys as keys, and list all the clusters it belongs to.

This approach would leave the lower memory footprint, but should be rather efficient to work with. Here is some code to start you off with. You might need to adjust a little related to splitting on space or tabs, but I get your wanted output using this.

from collections import defaultdict

def build_cluster_dict(filename):
    """Return a dict of keys with all the cluster the key exists in."""

    result = defaultdict(list)
    with open(filename) as infile:
        for line  in infile:
            elements = line.strip().split()

            cluster = elements[0]
            for key in elements[1:]:
                result[key].append(cluster)

    return result

def build_output(cluster_dict, filename, output_filename):

    with open(filename) as infile, open(output_filename, 'w') as outfile:
        for line in infile:
            key, text = line.strip().split()

            if key in cluster_dict:
                for cluster in cluster_dict[key]:
                    outfile.write('{}\t{}\t{}\n'.format(cluster, key, text))

def main():

    cluster_dict = build_cluster_dict("cluster.txt")

    print (cluster_dict)

    build_output(cluster_dict, "file2.txt", "output.txt")

if __name__ == '__main__':
    main()

Note that I've left out the use of numpy, as I don't see the need for it in this context. I've also used a double with statement to open both the in- and out-file at the same time in the same context. I left the print (cluster_dict) in there just to see the intermediate list it generates. For your test files this gave the following output (somewhat formatted):

defaultdict(<type 'list'>,
            {'AA': ['CL1', 'CL2'],
             'SS': ['CL1'],
             'YY': ['CL1'],
             'XX': ['CL1'],
             '3_b': ['CL2'],
             'ZZ': ['CL1']})

Addendum: Locate erroneous input line

In comments OP said their was a problem in the key, text line, and to detect this these lines:

       for line in infile:
           key, text = line.strip().split()

can be replaced with:

        for line_number, line in enumerate(infile):
            try:
                key, text = line.strip().split()
            except ValueError:
                print("Error on line {}: {}".format(line_number, line))
                ## Option a) Use empty text
                #key = line.strip()
                #text = ""
                # Option b) Continue with next line
                continue

This code will catch the error situation, and as it stands it will display an error message with the offending line. I've set it to use option b), that is continue with next line. If you want to use an empty string and write the output to file file, uncomment option a), and comment out option b).


Solution to part 1 of Day 7 Advent of Code in Scala Does someone want to review my solution to part 1 of Day 7 Advent of Code in Scala?
The assignment is here: http://adventofcode.com/day/7
Without repeating the whole text, in short:

Given a text file of the following form (simplified):

x AND y -> a
x -> 1
y -> 0

calculate the value of a. Here is an example of a full input file.

My solution:

import scala.io.Source

object Day7 {

  sealed trait Expr

  case class Variable(name: String) extends Expr

  case class Const(value: Int) extends Expr

  case class Not(expr: Expr) extends Expr

  case class And(left: Expr, right: Expr) extends Expr

  case class Or(left: Expr, right: Expr) extends Expr

  case class LShift(expr: Expr, by: Int) extends Expr

  case class RShift(expr: Expr, by: Int) extends Expr

  case class Assignment(v: Variable, expr: Expr) extends Expr

  case object Command {
    def parse(s: String): Assignment = {
      val assignmentRegex = """(.+) -> (.+)""".r
      def innerParse(s: String): Expr = {
        val rShiftRegex = """(\w+) RSHIFT (\d+)""".r
        val lShiftRegex = """(\w+) LSHIFT (\d+)""".r
        val orRegex = """(\w+) OR (\w+)""".r
        val andRegex = """(\w+) AND (\w+)""".r
        val notRegex = """NOT (\w+)""".r
        val constRegex = """(\d+)""".r
        val varRegex = """(\w+)""".r

        s match {
          case rShiftRegex(l, n) =>
            RShift(innerParse(l), n.toInt)
          case lShiftRegex(l, n) =>
            LShift(innerParse(l), n.toInt)
          case orRegex(l, r) =>
            Or(innerParse(l), innerParse(r))
          case andRegex(l, r) =>
            And(innerParse(l), innerParse(r))
          case notRegex(e) =>
            Not(innerParse(e))
          case constRegex(n) =>
            Const(n.toInt)
          case varRegex(n) =>
            Variable(n)
        }
      }
      s match {
        case assignmentRegex(l, r) => Assignment(Variable(r), innerParse(l))
        case _ => throw new Exception(s"Unrecognized command: $s")
      }
    }
  }

  class Environment(mappings: Map[String, Expr]) {

    val cache = new scala.collection.mutable.HashMap[Expr, Int]()

    private def eval(e: Expr): Int = {
      cache.get(e) match {
        case Some(i) => i
        case None => {
          val result = e match {
            case Variable(name) => {
              println(s"evaluating $name")
              eval(mappings(name))
            }
            case Const(value) => value
            case Not(expr) => ~eval(expr)
            case And(l, r) => eval(l) & eval(r)
            case Or(l, r) => eval(l) | eval(r)
            case LShift(expr, n) => eval(expr) << n
            case RShift(expr, n) => eval(expr) >> n
          }
          cache += (e -> result)
          result
        }
      }
    }

    def call(name: String): Int = {
      eval(mappings(name))
    }
  }

  def main(args: Array[String]) = {
    val lines = Source.fromFile("input-day7.txt").getLines()
    val x = lines.map(Command.parse).map((a) => a match {
      case Assignment(Variable(v), e) => (v -> e)
    })
    val m = x.toMap
    println(new Environment(m).call("a"))

  }

}

It seems to me that your general approach is solid. One small change I noticed you could make would be to move the Regex declarations outside of the functions parse and innerParse. The reason for doing this is that it is expensive to compile the Regexs with every call to these functions. By moving them out we only construct them once. This is supported by the Scala Regex documentation which can be read here.

case object Command {
  val assignmentRegex = """(.+) -> (.+)""".r
  val rShiftRegex     = """(\w+) RSHIFT (\d+)""".r
  val lShiftRegex     = """(\w+) LSHIFT (\d+)""".r
  val constRegex      = """(\d+)""".r
  val varRegex        = """(\w+)""".r
  val notRegex        = """NOT (\w+)""".r
  val andRegex        = """(\w+) AND (\w+)""".r
  val orRegex         = """(\w+) OR (\w+)""".r

  def parse(s: String): Assignment = {
    def innerParse(s: String): Expr = {
      // ...
    }
    // ...
  }
  // ...
}


Is my design still a state design pattern, or some abomination? I was trying to review state design pattern, and tried to create flexible implementation of it so it could reuse it in future.

So I came out with this:

public interface FsmState<S extends Enum<?>,INPUT> {
    Result<S> action(INPUT input);
}

and this:

public class Result<S extends Enum<?>> {

    public boolean isCompleted() {
        return isCompleted;
    }

    public S getNextState() {
        return nextState;
    }

    public Result(boolean isCompleted, S state) {
        super();
        this.isCompleted = isCompleted;
        this.nextState = state;
    }

    private final boolean isCompleted;

    private final S nextState;
}

and for now this is way how you can use it:

public class Fsm {
    static enum SomeEnum {
        A, B, C
    }

    static class aState implements FsmState<SomeEnum, String> {

        @Override
        public Result<SomeEnum> action(String input) {

            if (input.equals("B")) {
                return new Result<SomeEnum>(false, SomeEnum.B);
            }
            return new Result<SomeEnum>(false, SomeEnum.A);
        }
    }

    static class bState implements FsmState<SomeEnum, String> {

        @Override
        public Result<SomeEnum> action(String input) {
            if (input.equals("B")) {
                return new Result<SomeEnum>(false, SomeEnum.B);
            }
            if (input.equals("C")) {
                return new Result<SomeEnum>(false, SomeEnum.C);
            }
            throw new RuntimeException("Invalit input");
        }
    }

    static class cState implements FsmState<SomeEnum, String> {

        @Override
        public Result<SomeEnum> action(String input) {
            if (input.equals("A")) {
                return new Result<SomeEnum>(true, SomeEnum.C);
            }
            return new Result<SomeEnum>(false, SomeEnum.C);

        }
    }

    private Map<SomeEnum, FsmState<SomeEnum, String>> statesMap = new HashMap<Fsm.SomeEnum, FsmState<SomeEnum, String>>();

    public Fsm() {
        statesMap.put(SomeEnum.A, new aState());
        statesMap.put(SomeEnum.B, new bState());
        statesMap.put(SomeEnum.C, new cState());
    }

    boolean run(String input) {
        FsmState<SomeEnum, String> action = statesMap.get(SomeEnum.A);
        for (char ch : input.toCharArray()) {
             ;
            Result<SomeEnum> state = action.action("" + ch);
            if (state.isCompleted()) {
                return true;
            }
            action = statesMap.get(state.getNextState());

        }
        return false;

    }

    public static void main(String[] args) {
        System.out.println(new Fsm().run("AAABBCA"));
    }
}

For each Enum element you create implementation of FsmState interface, where based on input you do transition to next state.

It is still a bit messy code, and I'm fully aware of that. But I was wondering if it can be called a state pattern?

First of all all design patterns are well defined. Every deviation of it will distort your implementation and negate advantages into disadvantages. That is because all known design patterns in general were NOT MADE UP. They were IDENTIFIED to be the best model structures to represent some of the real world structures.

The state pattern is a behavioural pattern. Behaviour is not existent for types only objects can behaves. The state pattern consists of a holder object and an association to a state object that is known abstract. Every single state decides on his own what is the following state dependent on the state of the holder object. So far your implementation as well.

But currently you are only hopping from one state to the other without doing something. You are currently not manipulating any object local variables of the holder object what will be the target of the state pattern. The manipulation is done in the for-loop. The holder object provides the information to the action() method of each state object. So here you distribute responsibilities between the holder object and the state object. My recommendation is to let the state objects decide on their own, when to get the next character. Because the "input" variable is not in object scope you should put it there. Doing this in the run() method will lead to an error prone and not thread enabled solution. So here I would pass the "input" through the constructor and make it object local available. No Parameter for the run() method anymore. For each Thread instantiate a fresh Fsm object with new input.

Furthermore you have at least one other "state" representation: isCompleted(). This should be represented as a state as well and not with a separate construct in the result.

What your implementation explicitly defines is the reuse of the state objects (caching).

In a special case you will not face any problem. That is if only behaviour is encapsulated in the state object no "state". So the state objects must not have object local variables to be modified (and of course no class variables too). BTW this is another pattern called flyweight. Its primary feature is immutability. So in your implementation you should ensure that your state objects are immutable.

If you have state within your state objects then you have to consider an initialization mechanism for your stateful state objects or always create new state objects. If you don't do that you introduce distributed states because the execution of the next state does not only depend on the state of the holder object. It also depends on the state of the following state object that the predecessor is not aware of. But every state object must be aware of ANY state to decide to what next state is to delegate. So if you have stateful state objects you must newly instantiate or initialize them or make them a flyweight. As your state objects hold no state you currently have no problem.

But one other thing is allowed. You may parametrize a successor with a result of a predecessor. I am not sure about any use cases but may pass information around because the current state object can decide on its own what to do. But in your example your are not passing anything around.

OK, what about the enum? Under the condition your state objects remain flyweights the enum is an unnecessary code fragment. You can remove the enum and introduce constants for each state object. If you have the intention to have stateful state objects I recommend to avoid reusing state objects. Of course you can initialize them but this is error prone. In this case it's also not necessary to have an enum.

Currently you are returning a complex object that consists of a boolean and the enum (the TYPE!!!) of the new state. I recommend to return nothing (void). The boolean as already stated can be seen as a separate state construct beside the existing state pattern which should be represented as a separate state object. The enum is obsolete when you use state object constants or newly instantiate state objects. The return value is obsolete if you provide a setState() method on the holder object. I didn't recognize until now that you haven't got a currentState variable held by the holder object. That has to be introduced.

Finally to make your Fsm (whatever this will be) generic you should provide a set of start states (public Set<..> getStartStates()) on the holder object. They will be accessible to the client object that uses an instance of Fsm. The client may pass one start state into the constructor.

As well you should provide a set of end states. Your loop should then only look like this:

while (!getEndStates().contains(currentState)){ currentState.action() }.

One thing I won't go into: To make Fsm generic I assumed to have flyweight state objects. If you do not have flyweights you will go deep into meta modelling. Then you would introduce a type level. BTW your enum was somehow something like this but under the assumption of flyweights unnecessary and under the assumption of stateful state objects insufficient.

To answer your question finally: It's not the standard way to implement the state pattern and it has its pitfalls because of that.


PHP passing messages back to Ajax I have removed a lot of the code, but you should get an idea of what I am doing:

<?php

$upOne = realpath(__DIR__ . '/..');
require_once $upOne . '/vendor/autoload.php';

use phpseclib\Net\SFTP;

$errors = array();
$fileData = array();
$inputArray = array();
$config = new CONFIG\Config();

if (empty($_POST['emailAdd'])) {
    $errors['emailAdd'] = '- Please input your Email address';
} else {
    if (!filter_var($_POST['emailAdd'], FILTER_VALIDATE_EMAIL)) {
        $errors['emailAdd'] = '- Please input your Email address';
    } else {
        $email = filter_var($_POST['emailAdd'], FILTER_SANITIZE_EMAIL);
        array_push($inputArray, $email);
    }
}

//Additional validation
...

if (!empty($errors)) {
    echo json_encode("failed");
} else {
    try {
        $dbh = new PDO("mysql:host=".$config::DB_HOST.";dbname=".$config::DB_NAME."", $config::DB_USER, $config::DB_PASSWORD);

        $stmt = $dbh->prepare("INSERT INTO emirates_user("...");

        if($stmt->execute()) {

            $lastId = $dbh->lastInsertId();

            if (isset($_FILES)) {
                foreach($_FILES as $file) {
                    $upload = UPLOAD\Upload::factory($config::TEMP_DIR);
                    $upload->file($file);

                    if(!$upload->get_errors()) {
                        $fileData[$results['full_path']] = $results['mime'];
                    }
                    else {
                        echo json_encode("failed");
                        break;
                    }
                }

                $date = date('_dmY_H:i:s');
                $pdf = new PDF\PdfGenerator($fileData, $inputArray, $date);

                if($pdf) {
                    $command = '/usr/bin/python /srv/www/vhosts/someurl/html/php/somefile.py';
                    exec($command, $retval);

                    if(isset($retval[0])  && $retval[0] === 'success') {
                        $mail = new PHPMailer;

                        .....

                        if(!$mail->send()) {
                            echo json_encode("failed");
                        } else {
                            $lstmt = $dbh->prepare("...");
                            ...

                            if($lstmt->execute()) {
                                echo json_encode("success");
                            }
                        }
                    } else {
                        echo json_encode("failed");
                    }
                }

            } else {
                echo json_encode("failed");
            }
        } else {
            echo json_encode("failed");
        }
    }
    catch(Exception $e) {
        echo json_encode("failed");
    }
}

First of all, I validate the user input.  If this fails, I return a failed message back to my Ajax.  If all is ok, the process begins.  This script inserts into a database, generates a PDF, SFTP's the file using a Python script, sends an email etc.  All of these actions could potentially go wrong.  I have handled things via conditional statements to make sure the code is executed correctly.  If not, then I return a failed message again.  If it makes it to the very end, I return a success message.

My Ajax is like so (not the whole function, just the done and failure sections):

.done(function (response) {
    if(response == 'failed'){
        window.location.replace('failure.html');
    } else {
        window.location.replace('success.html');
    }
}).fail(function (jqXHR, textStatus) {
    console.log(textStatus);
});

So if the response is failed, I display a failed page.

Really, I am just seeking advice as to whether this type of logic is ok.  Should I maybe return more detail information about the error so I know what it relates too?  If so, how would I handle this in Ajax?

There are a few things that you might consider here, to improve readability, maintainability, and (indirectly through those qualities), reliability.

As a guideline, if a piece of code goes over 10 lines or statements, it is probably about time to break it up into functions.

In this case, I might consider breaking it up into following functions:

public function getEmailInput() {}
public function getFileInput() {}
public function insertIntoDatabase() {}
public function generatePdf() {}
public function sendConfirmationEmail() {}
//Maybe some others, as you see fit

This will allow give you a little more freedom to update the steps as that inevitably becomes necessary without as much risk of breaking other parts or having to remember how the entire program works.

On the JavaScript side, it looks like this is the kind of script that needs to fail safe, so I might consider checking for success rather than failure, like so:

.done(function (response) {
    if(response === 'success'){ //make sure we got the response we needed
        window.location.replace('failure.html');
    } else {
        window.location.replace('success.html');
    }
}).fail(function (jqXHR, textStatus) {
    console.log(textStatus);
});