DB Query within foreach I'd like a general opinion on my piece of code (which does exactly what I want it to do). I feel like I'm using bad practices, like a DB query within a foreach loop, or grabbing the lowest value in a non-elegant way (sorting, then using the object with an index of 0). I'm also not sure on my usage of the DB queries inside my controller, instead of passing it to the model.

Note this is on Laravel v3:

public function get_predict()
{
    $champions = DB::table('champions')
        ->get(array('id','name','released','price_ip','price_rp'));

    $now = strtotime(date('Y-m-d'));

    foreach($champions as &$champion) {
        $id = $champion->id;
        $champion_sales = DB::table('sales')
            ->where('type','=','1')
            ->where('for','=',$id)
            ->get();
        $champion->sales = $champion_sales;

        foreach($champion->sales as &$sale) {
            $dateout = strtotime($sale->date_out);
            $sale->days_since = floor(($now - $dateout)/(60*60*24));
        }

        usort($champion->sales, function($b, $a) {
            return strcmp($b->days_since, $a->days_since);
        });

        if(isset($champion->sales[0]->days_since)) {
            $champion->most_recent = $champion->sales[0]->days_since;
            $champion->last_sale = date('Y-m-d',strtotime($champion->sales[0]->date_out));
        }
    }

    return View::make('champion.predict')->with('champions', $champions);
}

Right, one general opinion coming up:

Static's, at least the way you're using them, are just globals in drag. Don't do that. If you need both the View and DB class(or at least some tables from it) in the object, of which you posted a method: either Inject, or lazy-load them. A call to a static method is bound to be slower, and, by the looks of things DB::table is sort-of a factory method. Why call it time and time again, if it's already created and returned the desired object once before. You can injected/lazy-loaded them, or just assign them to a property.
Anonymous functions (lambda's or closures, whichever name you prefer) aren't my favorite addition to PHP. They feel, to me at least, like a typical PHP afterthought. In essence, they're just syntactic sugar. What they actually do is create an instance of the Closure class. I'm not sure on the internals, but I wouldn't be at all surprised if your code were to create instances of Closure on each iteration of the loop. This isn't that bad in a language like JavaScript, where function objects are very cheap, but in PHP, it'd be far cheaper to define a private method, and pass a callable array to usort. Something along the lines of usort($champion->sales, array($this, 'privateSorterMethod'); will almost certainly be faster, especially since you can declare & assign the array outside of the loop. This gives you more control on when and where the array is constructed: not in the loop, if you don't put it there, that's for sure.
Getting data from the database, and returning the view is doing 2 things, and thus a violation of the Single Responsibility Principle. build your $champions array, and return it back to the controller, or whatever called the get_prediction method. The controller is what should link the Model-layer to the view. That's its job: the controller gets the request, decides what the Model-layer needs to do, and sends the output of the model layer back to the view. The model layer should be unaware of the view and vice-versa.
Performing that second query inside the loop is, in all likelihood, not the best way forward. Consider using JOIN, to get all data in one go.
Maintainability: if I were to be assigned your project some time in the future, I'd have to check the actual DB's, to see what you're getting from the sales table. even if you're getting all fields, specify them.
How are you getting your data? As (assoc-)array's? Objects (Stdclass or another object)? Personally, I prefer data models that are directly mapped to the tables. The properties of these data-models correspond to the field names, and they should have a getter and setter for each property. At least, that's what I'd do.

Notes: Calling a static is slower than calling it once, and assigning its return value for re-use. Adding the getTable methods (see below) does mean added overhead (+1 method call), but the overhead will only be noticeable once.
Dropping the anonymous function means better performance, I've made a test-script, and it using a callable array is 50~100% faster. Caching enabled or not, it's the fastest way to go, still.
Fetching rows as data-models with predefined properties is faster than using single stdClass instances, at least: as far as property access is concerned. Basically because the lookup is done using a hashtable, to get the offset of the corresponding zval*. A bit more details here

Just to clarify what I mean by Inject or lazy-load them:

class Foo
{
    private $table = null;
    privte $view = null;//this is a bad idea anyway, but just as an example
    public function __construct(array $tables = array(), View $view = null)
    {//you can pass DB and or view instance to constructor
        $this->tables = $tables;
        $this->view = $view;
    }
    public function setTables(Table $inject)
    {//if you didn't pass to constructor, or need another DB instance for something:
        $this->table[$inject->getName()] = $inject;
    }
    public function setView(View $view)
    {//this is standard DI (dependency Injection)
        $this->view = $view;
    }
    //This is GUARANTEED to return an instance of DB
    //Unless a fatal error occurred.
    protected function getTable($name)
    {
        if (!isset($this->table[$name]))
        {//if table isn't set, load instance on the fly => lazy-loading
            $this->table[$name] = DB::table($name);//get the instance
        }
        return $this->table[$name];
    }
    private function sortCallback($a, $b)
    {
        return strcmp($b->days_since, $a->days_since);
    }
}


Scanning a .cs file or a directory This is a console application which takes as an argument either a .cs file or a directory. If it's a .cs file, it scans it; if it's a directory, it scans all .cs files within the directory.

A simplified view of a method which chooses whether it's a file or a directory and reports errors is this one:

public static void ProcessPath(string path)
{
    if (path.EndsWith(".cs"))
    {
        if (File.Exists(path))
        {
            Program.ScanFile(path);
        }
        else
        {
            Console.WriteLine("The specified file doesn't exist.");
            Environment.Exit(1);
        }
    }
    else
    {
        if (Directory.Exists(path))
        {
            Program.ScanDirectory(path);
        }
        else
        {
            if (File.Exists(path))
            {
                Console.WriteLine("The path corresponds to a file, but only *.cs files are supported.");
            }
            else
            {
                Console.WriteLine("The specified directory doesn't exist.");
            }

            Environment.Exit(1);
        }
    }
}

How can I rewrite the if/else logic in a more elegant way? Would it improve readability?

In my opinion, the easiest way would be to embrace exceptions. DirectoryNotFoundException and FileNotFoundException are there for a reason, and actually even calling .Exists might not save you from them - the file might be moved/removed between the Exists and the action you're taking next.

You can do:

public static void ProcessPath(string path)
{
    var extension = Path.GetExtension(path);

    if(string.IsNullOrEmpty(extension))
    {
        Program.ScanDirectory(path);
    }
    else
    {
        if(extension != ".cs")
            throw new NotSupportedException("Only .cs files are supported");

        Program.ScanFile(path);
    }
}

And either catch those exceptions higher up and write them to console or (considering you're doing Environment.Exit in your example), just let them propagate and crash the application.


BitShift code compacting I'm new to Java programming and am really enjoying it. I made a bitshift program, but I think it's too long.

I expect the program to:

Let users input the number and degree to be shifted
Specify if it is a right or left shift
Restart the program if the user requires at the end
Ensure exceptions don't crash the program

import java.util.Scanner;
class BitShift{
    int a,b;
    String c;

    static int shiftNo(){
        Scanner sc=new Scanner(System.in);
        int x=sc.nextInt();
        return x;
    }

    static String shiftDir(){
        Scanner sc=new Scanner(System.in);
        String x=sc.nextLine();
        return x;
    }
    }

    class BitShiftObjects1 extends BitShift{

    public static void main(String []args){

        int answer;
        BitShift ob1=new BitShift();

    boolean isValue=false;
    while(isValue==false){
        try{
            System.out.println("Please enter number to be shifted");
            ob1.a=shiftNo();
            System.out.println("Please enter degree to be shifted");
            ob1.b=shiftNo();
            isValue=true;
        }
        catch(java.util.InputMismatchException e){
            System.out.println("ERROR:**Please Input Integer Number**");
            System.out.println("**Please enter from begining**");
            System.out.println("");
        }

    }

    isValue=false;
    while(isValue==false){

            System.out.println("Please enter r or l");
            ob1.c=shiftDir();

            if(ob1.c.equals("r") || ob1.c.equals("l"))
                isValue=true;
            else
                System.out.println("ERROR:**Please enter LOWERCASE r/l**");

        }

        if(ob1.c.equals("r")){
            answer=ob1.a >> ob1.b;
            System.out.println("Your Answer is : "+answer);
                }
        else{
            answer=ob1.a << ob1.b;
            System.out.println("Your Answer is : "+answer);
                    }

        System.out.println("");
        System.out.println("Do you want to enter a new data set??");
        System.out.println("Press y or n");

        isValue=false;
        while(isValue==false){
        ob1.c=shiftDir();

        if(ob1.c.equals("y")){
            System.out.println("**Reinitializing program**");
            isValue=true;
            main(args);
        }
        else if(ob1.c.equals("n"))
            isValue=true;
        else
            System.out.println("ERROR:**Please enter lowercase y or n**");
        }
    }
}

Formating

Correct indentation and spaces make the code a lot easier to read. Just paste your code in any decent IDE and it should manage this for you automatically.

Spaces before curly brackets
Spaces before and after equals sign
Indentation: closing brackets should be on the same level as the beginning of the opening statement
Indentation: Elements on the same level should be indented the same amount

Variable names

a, b, c, x, sc, ob1, those are all bad variable names. The bigger the scope of the variable, the more important it gets that it has a good name. So while sc for scanner is probably still alright, a, b, and c are definitely not.
shiftNo and shiftDir should better be named shiftAmount and shiftDirection. Or, to make them more reusable: getInputString and getInputInteger.
isValue sounds confusing. Maybe recievedInput or something like it. And then, don't do this: while (recievedInput == false), but this: while (!recievedInput)

Comments

Your code is missing comments. If it is self-documenting code this might be alright. But in this case: What are a, b, and c? Does shiftDir shift something? (Maybe a directory?) No, it gets input. The method name and/or the comment should make this clear.

Class Structure

see the answer of Puneet_Techie

In addition to that, you might also want to extract the actual shifting to their own methods. It makes the code cleaner, and it also makes it easier to handle corner cases and to write unit tests.

Corner Cases / Input validation

What if I want to shift 3 by 100? Is the output still correct? Or if I want to shift -4 by 5? Or if I want to shift 3 by -4?

Make sure that you only allow input that you actually want. And that the result is always what you expect it to be.


Fetching detail for a given ID, which might not exist I'm actually working on a piece of code which use the following code:

public Detail GetDetail(int id)
{
    if (!_detail.ContainsKey(id))
    {
        GetDetailForNewObjects(id);
    }

    return _detail[id].Data;
}

The last line throws a NullReferenceException. Since it should not happen that this method is called in the way it returns an exception they propose not to catch it and rethrow a more specific and descriptive one.

I don't agree. The software readability and the maintainability of the cose falls dramatically when we get a NullReferenceException.

How would you write that piece of code?

In this case, I would suggest that this exception should be classified as a 'Boneheaded' exception as defined by Eric Lippert because there is no reason why the code should throw the exception at all.  Exceptions are expensive, as is handling them so I would make every effort to have code that only has them in TRULY exceptional circumstances.

It seems to me like the reason the _detail[id] throws a NullReferenceException is because the GetDetailForNewObjects method had a problem which could be in many forms, but I'll outline two below.

This problem could be one of

Your DB Query (assuming that is what is happening) couldn't find any records.

Solution: return a null object.  Don't try to access something you know has a reasonable possibility of being null, instead check again for null and return null if no object exists after the call.  Let the calling code figure out what to do with a null (maybe search results are null, this is not an exception).

Your DB had a connection issue / timeout

Solution:  Again, THIS is the exception that should be passed up.  We shouldn't handle a DB exception (since there it typically nothing that can be done about it), just to throw a null reference exception.  This behavior would make debugging more difficult since it hides the actual DB connection issue.  Bubble the DB exception upwards so the code / developer can respond to that, instead of an ambiguous NullReferenceException because ultimately, we need to know WHY it was null since clearly we weren't expecting it to be.

In both cases, we as developers know how to handle the situation (given the assumptions I laid out).

I would write the above code something like this:

public Detail GetDetail(int id) {
    if(id < 0) {
        throw new ArgumentException("id", "Argument should have a positive value.");
    } // end if
    Detail returnValue = null; // value store that is only assigned if the call succeeds
    if (!_detail.ContainsKey(id)) {
        GetDetailForNewObjects(id); // assuming this adds the key to the dictionary.
        if(_detail.ContainsKey(id)) { //Cheaper and safer than handling an exception
            returnValue = _detail[id].Data;
        } // end if
    } // end if

    return returnValue;
} // end function GetDetail

Eric Lippert has a great description of when / how to handle different types of exceptions here: http://blogs.msdn.com/b/ericlippert/archive/2008/09/10/vexing-exceptions.aspx

Further reading:

https://softwareengineering.stackexchange.com/questions/133560/how-to-avoid-throwing-vexing-exceptions
Is catching 'expected' exceptions that bad?


JQuery UI Datepicker with beforeShowDay function I am relatively new to JavaScript and JQuery. I wanted a Datepicker that has the following functionality:

Weekends are not selectable
Non working days (bank holidays etc.) are not selectable
The first selectable day must be x number of full working days in the future, taking into account bank holidays and weekends

I looked at few examples and came up with this (the date format is dd/mm):

$(function() {

  var holidays= [[3,1], [22,4], [25,4], [29,4], [2,5], [30,5], [29,8], [26,12], [27,12]];

  var workingDayOffset = 10, selectedDate = new Date();

  function nonWorkingDays(date) {
    for (var j = 0; j < holidays.length; j++) {
      if (date.getMonth() == holidays[j][1] - 1 && date.getDate() == holidays[j][0]) {
        return [false, ''];
      }
    }
  return [true, ''];
  }

  function beforeCurrentDate(date) {
   if(date.getDate() === selectedDate.getDate() && date.getMonth() === selectedDate.getMonth() && date.getFullYear() === selectedDate.getFullYear())
   {
    return [true, ''];
   }
   return [date < selectedDate,''];
  }

  function nonAvailableDays(date) {
    var noWeekend = $.datepicker.noWeekends(date), before = beforeCurrentDate(date), holiday = nonWorkingDays(date);
    return [noWeekend[0] && before[0] && holiday[0], ''];
  }

  for(var i = 0; i < workingDayOffset; i++) {
    selectedDate.setDate(selectedDate.getDate() + 1);
    if(!nonAvailableDays(selectedDate)[0])
    {
      i--;
    }
  }

  Date.prototype.formatDDMMYY=function(){
    var dd = this.getDate(), mm = this.getMonth()+1, yyyy = this.getFullYear();
    if(dd<10){
      dd = '0' + dd;
    }
    if(mm<10){
      mm = '0'+ mm;
    }
  return String(dd + "\/" + mm + "\/" + yyyy);
  };

  $( "#datepicker" ).val(selectedDate.formatDDMMYY());

  $( "#datepicker" ).datepicker({ beforeShowDay: nonAvailableDays, dateFormat: 'dd/mm/yy', showOn: 'button', buttonText: "select", defaultDate: selectedDate,gotoCurrent: true}) ;
});

I am looking for a general critique of the code, but also there are a couple of things that I thought would be easier / cleaner:

In beforeCurrentDate I wanted to do a check that the date was less than selectedDate, in terms of the whole date, not including the time component. Using date > selectedDate would return false even if the date component was the same, due to the time part.  I thought about setting the time on the selected date, but then GMT offset came into play. It seemed to me that it should be easier, in the end I added a fudge that check just the date components for a match.
I am not sure adding Date.prototype.formatDDMMYY is the right way to go.  Again, this felt like it should be more straight forward.

I am interested to hear your thoughts on this.

for 1)

var today = new Date();
today.setTime(0); // resets the time. (midnight)

I believe this will fix the date comparison issue

for 2)

It just sucks that there isn't a built-in for this sort of thing. I would suggest not putting it on the Date prototype just in case it conflicts with some other date function/library down the road. Its seems to be only a matter of preference though.

(if you Google: JavaScript format date you'll see many other examples of formatting the date or https://stackoverflow.com/q/1056728/684890 where there are a few alternatives suggested in case you want some more functionality. )


How to append ajax return data to a table I am trying to append data to a table based on the ajax returned data.

My codes work but it seems ugly. I was wondering if anyone here can help me
to simplfy it. Thanks a lot!

    ajax.callback=function(data){
          for (var i=0; i<dataObj[0].data.length; i++){
              var td=document.createElement('td');
              td.innerHTML=dataObj[0].data[i].ID;
              var td2=document.createElement('td');
              td2.innerHTML=dataObj[0].data[i].test;
              var td3=document.createElement('td');
              td3.innerHTML=dataObj[0].data[i].year;
              var td4=document.createElement('td');
              td4.innerHTML=dataObj[0].data[i].code;
              var td5=document.createElement('td');
              td5.innerHTML=dataObj[0].data[i].Label;
              var td6=document.createElement('td');
              td6.innerHTML=dataObj[0].data[i].contents;
              var td7=document.createElement('td');
              td7.innerHTML=dataObj[0].data[i].test;

              var tr=document.createElement('tr');
              tr.appendChild(td);
              tr.appendChild(td2);
              tr.appendChild(td3);
              tr.appendChild(td4);
              tr.appendChild(td5);
              tr.appendChild(td6);
              tr.appendChild(td7);

              $('#Table').append(tr);

           }
    }

You could do:

ajax.callback=function(data){
    var elements = ["ID", "test", "year", "code", "Label", "contents", "test"];

    for (var i=0; i<dataObj[0].data.length; i++){
        var td;
        var tr=document.createElement('tr');

        for (var j=0; j < elements.length; ++j){
            td = document.createElement('td');
            td.innerHTML=dataObj[0].data[i][elements[j]];
            tr.appendChild(td);
        }

        $('#Table').append(tr);
    }
}


Queue implementation using unique_ptr I am trying out different data structures to learn more about smart pointers. I have created a queue implementation which has push, pop, front, back and size functions.

#include <iostream>
#include <memory>
#include <cstddef>

template<typename T>
class Queue
{
    std::unique_ptr<T[]> q_ptr;
    int front_idx = -1;
    int back_idx = -1;
    int capacity = 0;
public:
    Queue(std::size_t space)
    {
        q_ptr = std::unique_ptr<T[]>(new T[space]);
        capacity = space;
    }
    void push(T value);
    void pop();
    T front() const;
    T back() const;
    std::size_t size() const;
};

template<typename T>
void Queue<T>::push(T value)
{
    if(front_idx == -1)
    {
        front_idx++;
    }
    if(back_idx - front_idx + 1 == capacity)
    {
        std::cerr << "Queue full\n";
        return;
    }
    q_ptr[++back_idx] = value;
}

template<typename T>
void Queue<T>::pop()
{
    if(front_idx == -1)
    {
        std::cerr << "Empty queue\n";
        return;
    }
    q_ptr[front_idx++] = T{};
}

template<typename T>
T Queue<T>::front() const
{
    return q_ptr[front_idx];
}

template<typename T>
T Queue<T>::back() const
{
    return q_ptr[back_idx];
}

template<typename T>
std::size_t Queue<T>::size() const
{
    return back_idx - front_idx + 1;
}

int main()
{
    Queue<int> q1{20};
    q1.pop();
    for(int i = 0; i < 23; i++)
    {
        q1.push(i);
    }
    std::cout << "Queue size: " << q1.size() << "\n";
    std::cout << "Queue front: " << q1.front() << "\n";
    std::cout << "Queue back: " << q1.back() << "\n";
    q1.pop();
    std::cout << "Queue size: " << q1.size() << "\n";
    std::cout << "Queue front: " << q1.front() << "\n";
    q1.pop();
    std::cout << "Queue size: " << q1.size() << "\n";
    std::cout << "Queue front: " << q1.front() << "\n";
    q1.push(12);
    std::cout << "Queue size: " << q1.size() << "\n";
    std::cout << "Queue back: " << q1.back() << "\n";
}

The attempt is to use unique_ptr as an array similar to a raw pointer. Is the usage and approach correct? Can it be improved?
As far as I know, I cannot use shared_ptr this way in C++11 or C++14. Is this the best approach with shared_ptr?

Yes I think your usage of unique_ptr is correct. The only recommendation I would make is to use make_unique for construction:

explicit Queue(std::size_t space) :
    q_ptr(std::make_unique<T[]>(space)),
    capacity(space) {}

This answer provides motivation for its use. I've also used explicit to avoid implicit conversion from size_t to Queue. And I made use of a member initialization list.

I wrote the following tests using googletest:

TEST(QueueTester, testEmpty) {
    Queue<int> queue(0);
    EXPECT_EQ(0, queue.size());
}

TEST(QueueTester, testSingle) {
    Queue<int> queue(1);
    EXPECT_EQ(0, queue.size());
    queue.push(0);
    EXPECT_EQ(1, queue.size());
}

But neither passed. I corrected this by using the following:

int front_idx = 0;

And changing push:

template<typename T>
void Queue<T>::push(T value)
{
    if (back_idx - front_idx + 1 == capacity)
    {
        std::cerr << "Queue full\n";
        return;
    }
    q_ptr[++back_idx] = value;
}

I changed front and back to return references to the elements to allow the user to modify the contents. I also wrote overloads for returning const references to allow reading when the Queue is passed by const reference:

template<typename T>
T &Queue<T>::front()
{
    return q_ptr[front_idx];
}

template<typename T>
T &Queue<T>::back()
{
    return q_ptr[back_idx];
}

template<typename T>
const T &Queue<T>::front() const
{
    return q_ptr[front_idx];
}

template<typename T>
const T &Queue<T>::back() const
{
    return q_ptr[back_idx];
}

I further noticed that push, given a nonzero front_idx, will write past the end of the array pointed to by q_ptr. I took inspiration from this page and modified the code to store elements circularly around the array. Here is the final implementation:

#include <iostream>
#include <memory>
#include <cstddef>

template<typename T>
class Queue
{
    std::unique_ptr<T[]> q_ptr;
    int front_idx = 0;
    int back_idx = -1;
    std::size_t _size = 0;
    std::size_t capacity;
public:
    explicit Queue(std::size_t space) :
        q_ptr(std::make_unique<T[]>(space)),
        capacity(space)
    {
    }
    void push(T value);
    void pop();
    bool full();
    bool empty();
    T &front();
    T &back();
    const T &front() const;
    const T &back() const;
    std::size_t size() const;
};

template<typename T>
void Queue<T>::push(T value)
{
    if (full())
    {
        std::cerr << "Queue full\n";
        return;
    }
    back_idx = (back_idx + 1) % capacity;
    q_ptr[back_idx] = value;
    ++_size;
}

template<typename T>
bool Queue<T>::full()
{
    return _size == capacity;
}

template<typename T>
void Queue<T>::pop()
{
    if (empty())
    {
        std::cerr << "Empty queue\n";
        return;
    }
    q_ptr[front_idx] = T{};
    front_idx = (front_idx + 1) % capacity;
    --_size;
}

template<typename T>
bool Queue<T>::empty()
{
    return _size == 0;
}

template<typename T>
T &Queue<T>::front()
{
    return q_ptr[front_idx];
}

template<typename T>
T &Queue<T>::back()
{
    return q_ptr[back_idx];
}

template<typename T>
const T &Queue<T>::front() const
{
    return q_ptr[front_idx];
}

template<typename T>
const T &Queue<T>::back() const
{
    return q_ptr[back_idx];
}

template<typename T>
std::size_t Queue<T>::size() const
{
    return _size;
}


Handle data sent back over serial by Arduino I have a machine that is being controlled by an Arduino.  If I send the machine the command '9' it will send back JSON with some sensor temperatures in the format {"temperatures":{"bean_temp":110.75,"env_temp":98.15}}.

I am exposing a function called getTemperatures() that I currently call from elsewhere every 1000ms. The serialport library provides a listener for all the data that gets sent back over the wire from the machine (machine.on('data', handleData)). There are other types of data that will get sent back from the machine, such as {"action":"handle_pulled"}. I am filtering the data that comes back over the serial and then assigning temperatures responses to a temperatures variable that I continuously overwrite. I then give a 999 ms delay before calling back that variable.

This works, but seems fragile (and not elegant).

var serialport = require('serialport');

var ports = require('./ports'),
    port = ports.arduino;

var machine = new serialport(port, {
  baudRate: 9600,
  parser: serialport.parsers.readline("\n")
});

machine.on('data', handleData);

var temperatures = {};

machine.getTemperatures = function(callback) {
    sendCommand('9');
    setTimeout(function(){
      callback(temperatures);
    }, 999)
}

function sendCommand(data) {
  machine.write(data);
}

function handleData(dataString){
  data = JSON.parse(dataString);

  if ('temperatures' in data) {
    temperatures = data;
  }
}

module.exports.getTemperatures = getTemperatures;

Nobody wants an explicit setTimeout in their async code; you want data as soon as possible.

So I'd wrap the Arduino interface in an object that is an EventEmitter. Have that object listen to any and all incoming data, and dispatch it as temperature events, leverPulled events, and whatever else, based on content.

For extra points, make the Arduino object's data detection extensible, so it can, by itself, be dumb. It might just parse any given line as JSON, and then look through its registered data detection/classification methods, to see what kind of packet it got. Then it'll dispatch an event with that name to whomever listens.

So something like:

// in arduino.js or whatever you want to call it
var serialport = require('serialport'),
    ports = require('./ports'),
    port = ports.arduino;

var machine = new serialport(port, {
  baudRate: 9600,
  parser: serialport.parsers.readline("\n")
});

var wrapper = new require('events'); // i.e. new EventEmitter

var classifiers = {};

machine.on('data', function (string) {
  try {
    var json = JSON.parse(string);
    for (var type in classifiers) {
      if (!classifiers.hasOwnProperty(type)) continue;

      // classifiers are just function that return true
      // if they recognize the json they're given
      if (classifiers[type](json)) {
        wrapper.emit(type, json);
        break;
      }
    }
  } catch (e) {
    wrapper.emit('error', e); // might want to be more elaborate
  }
});

wrapper.addClassifier = function (type, callback) {
  // might want throw an error if the type already exists
  classifiers[type] = callback;
};

wrapper.send = function (string) {
  machine.send(string);
};

module.exports = wrapper;

Usage would be something like:

var arduino = require('./arduino');

// give the arduino wrapper a way to recognize temperature packets
arduino.addClassifier('temperature', function (json) {
  return ('temperature' in json);
});

module.exports.getTemperature = function (callback) {
  // listen for the next incoming temperature reading
  // and send it straight to the callback
  arduino.once('temperature', callback);

  arduino.send('9');
};

And if you want to check for errors, then you can add a timeout - if the temperature event hasn't fired in, say 500ms from sending the '9' command, something's probably wrong (and you'll want to remove the listener). You'll also want to give getTemperature a callback with the Node-typical err, data parameters, so you can send an error back:

module.exports.getTemperature = function (callback) {
  var timeout = null;

  var listener = function (json) {
    clearTimeout(timeout);
    callback(null, json);
  };

  arduino.once('temperature', listener);

  arduino.send('9');

  setTimeout(function () {
    // if we're here, we didn't hear back from the arduino in time
    arduino.removeListener('temperature', listener);
    callback(new Error('Timed out'), null);
  }, 500);
};


Obtaining prediction bands for regression model I'm trying to generate prediction bands for an exponential fit to some 2-dimensional data (available here).

The data (blue points), best fit found by scipy.optimize.curve_fit (red curve), and lower & upper 95% prediction bands (green curves) can be seen in the image below.

I'd love some confirmation that the code is actually doing things correctly and I haven't missed some step or simply used the wrong statistical tools.

The references I used are basically this post, and this video, both adapted to work with a general function, rather than being hardcoded to work with a linear function of the form \$y = a*x+b\$.

import numpy as np
import matplotlib.pyplot as plt
from scipy.optimize import curve_fit
from scipy import stats

def make_plot(x, xd, yd, popt, upb, lpb):
    # Plot data.
    plt.scatter(xd, yd)
    # Plot best fit curve.
    plt.plot(x, func(x, *popt), c='r')
    # Prediction band (upper)
    plt.plot(x, upb, c='g')
    # Prediction band (lower)
    plt.plot(x, lpb, c='g')
    plt.show()

def func(x, a, b, c):
    '''Exponential 3-param function.'''
    return a * np.exp(b * x) + c

def predband(x, xd, yd, f_vars, conf=0.95):
    """
    Code adapted from Rodrigo Nemmen's post:
    http://astropython.blogspot.com.ar/2011/12/calculating-prediction-band-
    of-linear.html

    Calculates the prediction band of the regression model at the
    desired confidence level.

    Clarification of the difference between confidence and prediction bands:

    "The prediction bands are further from the best-fit line than the
    confidence bands, a lot further if you have many data points. The 95%
    prediction band is the area in which you expect 95% of all data points
    to fall. In contrast, the 95% confidence band is the area that has a
    95% chance of containing the true regression line."
    (from http://www.graphpad.com/guides/prism/6/curve-fitting/index.htm?
    reg_graphing_tips_linear_regressio.htm)

    Arguments:
    - x: array with x values to calculate the confidence band.
    - xd, yd: data arrays.
    - a, b, c: linear fit parameters.
    - conf: desired confidence level, by default 0.95 (2 sigma)

    References:
    1. http://www.JerryDallal.com/LHSP/slr.htm, Introduction to Simple Linear
    Regression, Gerard E. Dallal, Ph.D.
    """

    alpha = 1. - conf    # Significance
    N = xd.size          # data sample size
    var_n = len(f_vars)  # Number of variables used by the fitted function.

    # Quantile of Student's t distribution for p=(1 - alpha/2)
    q = stats.t.ppf(1. - alpha / 2., N - var_n)

    # Std. deviation of an individual measurement (Bevington, eq. 6.15)
    se = np.sqrt(1. / (N - var_n) * np.sum((yd - func(xd, *f_vars)) ** 2))

    # Auxiliary definitions
    sx = (x - xd.mean()) ** 2
    sxd = np.sum((xd - xd.mean()) ** 2)

    # Predicted values (best-fit model)
    yp = func(x, *f_vars)
    # Prediction band
    dy = q * se * np.sqrt(1. + (1. / N) + (sx / sxd))

    # Upper & lower prediction bands.
    lpb, upb = yp - dy, yp + dy

    return lpb, upb

# Read data from file.
xd, yd = np.loadtxt('exponential_data.dat', unpack=True)

# Find best fit of data with 3-parameters exponential function.
popt, pcov = curve_fit(func, xd, yd)

# Generate equi-spaced x values.
x = np.linspace(xd.min(), xd.max(), 100)

# Call function to generate lower an upper prediction bands.
lpb, upb = predband(x, xd, yd, popt, conf=0.95)

# Plot.
make_plot(x, xd, yd, popt, upb, lpb)

There is a nice discussion about confidence/prediction bands in the documentation for the excellent kmpfit module. You need to include the convariance matrix from the fit in the calculation of the bands, which I don't see in your code. There is an example in the link that walks through it.

Here is their implementation:

def confpred_band(x, dfdp, prob, fitobj, f, prediction, abswei=False, err=None):
   #----------------------------------------------------------
   # Return values for a confidence or a prediction band.
   # See documentation for methods confidence_band and
   # prediction_band
   #----------------------------------------------------------
   from scipy.stats import t
   # Given the confidence or prediction probability prob = 1-alpha
   # we derive alpha = 1 - prob
   alpha = 1 - prob
   prb = 1.0 - alpha/2
   tval = t.ppf(prb, fitobj.dof)

   C = fitobj.covar
   n = len(fitobj.params)              # Number of parameters from covariance matrix
   p = fitobj.params
   N = len(x)
   if abswei:
      covscale = 1.0  # Do not apply correction with red. chi^2
   else:
      covscale = fitobj.rchi2_min
   df2 = numpy.zeros(N)
   for j in range(n):
      for k in range(n):
         df2 += dfdp[j]*dfdp[k]*C[j,k]
   if prediction:
      df = numpy.sqrt(err*err+covscale*df2)
   else:
      df = numpy.sqrt(covscale*df2)
   y = f(p, x)
   delta = tval * df
   upperband = y + delta
   lowerband = y - delta
   return y, upperband, lowerband


A class for animating messages as feedback to the user This class provides animation messages to the user.  Looking for general feedback.

I've posted the library I use as well if you have time to look that over.

/***************************************************************************************************
SMessenger - messaging system to provide feedback to the user
*/
    var SMessenger = $A.Class.create({
        Name: 'SMessenger',

        // static constants
        A: {
            DELAYTRANS:           1000,
            DELAY:                2000,
            GRANULARITY:          15,
            GRANULARITY_POSITION: 1,
            OFFSET:               40,
            RADIX:                10,
            UNITS:                'px'
        },

        // holds dome elements
        E: {

            // the dom holds the messages
            dynamic:      '#dynamic'
        },
        init: function () {

            // pull the elements dynamically at init time
            $A.eachChild(this.E.dynamic, function (val) {
                this.E[val.id] = $A.el('#' + val.id);
            }, this);
        },

        // s_ denotes server response
        populateMessage: function (element, type) {
            var period = '';

            // messages from the server will have a period
            // they are denoted by a preceding s_
            if (type.match("s_")) {
                type = type.replace('s_', '');
                period = '.';
            }
            element.innerHTML = this.E['dyn_' + type].innerHTML + period;
        },

        // creates a fading message for the user
        fadingMessage: function (obj) {
            this.populateMessage(obj.response_element[0], obj.state);
            obj.response_element.fade('down', this.A.DELAY);
        },

        // creates a message that fades up and then fades down.
        popMessage : function (obj) {
            var self = this;
            this.populateMessage(obj.response_element, obj.state);
            $A(obj.pane_element).fade('up', this.A.DELAYTRANS, function () {
                $A.setTimeout(function () {
                    $A(obj.pane_element).fade('down', self.A.DELAYTRANS, function () {
                        self.hideInput(false, obj);
                    });
                }, self.A.DELAY);
            });
        },

        // creates a sliding message
        slidingMessage : function (obj) {
            var self = this,
                el = obj.pane_element;
            this.populateMessage(obj.response_element, obj.state);
            el.style.display = 'inline';
            this.hideInput(true, obj);
            $A.peakOut(el, 40, 2000, function () {
                el.style.display = 'none';
                self.hideInput(false, obj);
            });

        },

        // hides or exposes input - enter, button - while the message is being displayed
        hideInput: function (hide, obj) {
            if (hide) {
                if (obj.input_element) {
                    obj.input_element.removeEventListener("keypress", obj.enter);
                }
                obj.cover_element.style.display = 'inline';
            } else {
                if (obj.input_element) {
                    obj.input_element.addEventListener("keypress", obj.enter);
                }
                obj.cover_element.style.display = 'none';
            }
        },

        // resets inputs
        // set the inputs to empty
        // a focus and a blur will trigger event handlers
        resetInput: function (array, checkbox) {
            $A.someIndex(array, function (val) {
                val.value = '';
                val.focus();
                val.blur();
            });
            if (checkbox) {
                checkbox.checked = true;
            }
        }
    }, true);

It looks really good! The only bit I don't like is the nested ifs, but I'm really nit-picking.

       if (hide) {
            if (obj.input_element) {
                obj.input_element.removeEventListener("keypress", obj.enter);
            }
            obj.cover_element.style.display = 'inline';
        } else {
            if (obj.input_element) {
                obj.input_element.addEventListener("keypress", obj.enter);
            }
            obj.cover_element.style.display = 'none';
        }

Maybe....

obj.cover_element.style.display = hide ? 'inline' : 'none';
if (!obj.input_element) return;

if (hide) {
   obj.input_element.removeEventListener("keypress", obj.enter);
}
else {
   obj.input_element.addEventListener("keypress", obj.enter);
}

Putting the return value at the start, instead of protecting both blocks from it being null, makes it slightly more understandable. But I still don't like that section much.


Print BFS implementation in JavaScript Given a graph and a starting vertex print the value of each vertex in a breadth first order.

Let's code out a general BFS algorithm printing each vertex's value in a BFS order. We are given a graph in the form of an adjacency list, as well as a starting vertex.

To analyze the complexity of a graph, we need to consider the number of vertices (V) and the number of Edges (E) as independent variables. Also, we need to consider the type of edge representation for the graph. We will go over the analysis for adjacency list.

Pseudocode
1. Create a queue to hold neighbor vertices and add the start vertex.
2. Create a set to track visited vertices and add the start vertex.
3. While queue is not empty
    4. Dequeue from the queue and set to current.
    5. Loop through all the vertex's neighbors.
        6. For each neighbor, check if the vertex has been visited.
        7. If not visited, then add it to the queue and add mark it as visited.
    8. Operate on current
9. Once the queue is empty, we have completed the BFS

Here's the code in JavaScript:

function graphBFS(graph, start) {
    let queue = new Queue(); // 1
    let visited = new Set(); // 1
    let current;             // 1
    let neighbors;           // 1

    queue.enqueue(start);    // 1
    visited.add(start);      // 1

  // while loop will run a total of V times
    while (queue.length > 0) {                       // 1 (for break condition)
        current = queue.dequeue();                   // 1
        neighbors = graph.neighbors(current);        // 1 (for adjacency list)
        // the for loop will run based on d (degree) on average is E/V
        for (let i = 0; i < neighbors.length; i++) {    // 1
            if (!visited.has(neighbors[i]) {            // 1
            queue.enqueue(neighbors[i]);                // 1
            visited.add(neighbors[i]);                  // 1
        }
        // <-- operating on the current vertex will add additional time
    }
  }
}

Code review
Fail
You should ensure your code is at least able to be parsed before you ask for review. I will assume it an oversight (that even your code formater was glaringly pointing out) and review assuming the missing token.
Incomplete
As an exercise the real complexity is in the graph and the neighbors function for which there is no code to evaluate.
The Queue implementation is missing, JS Arrays are queues, using push and shift to move items through the (abstracted) "queue".
Poor style
The code can be written better.

Use const for variables that don`t change
Its a little noisy. Remove redundant code eg while(foo.length > 0) the length property will never be < 0 so while(foo.length) does the same with less
Use appropriate iterators, for(;;) loops only when you need the index, else use the for(of) loop
There is really no need for the variables current and neighbors however neighbors can be argued to help with readability (see rewrite)
The comments are of no useful value.

Never add comments without relevance to the code. Why the many // 1?
Don't state the obvious. // 1 (for break condition)
Comment must be consistent with the naming and abstractions used. // 1 (for adjacency list) don't you mean neighbours?

Bad indentation is the worst possible readability error you can introduce into your source code. If assessing code, unindented blocks are an automatic fail and there is no argument that can ever justify such poor style.
There is a syntax error, the code will not even make it past the parser. A basic requirement for code review. I am not going to point it out as your runtime environment will tell you.

Rewrite
Two acceptable options of the many possible.
function graphBFS(graph, start) {
    const queue = new Queue();
    const visited = new Set([start]);
    queue.enqueue(start);
    while (queue.length) {
        const neighbors = graph.neighbors(queue.dequeue());
        for (const neighbor of neighbors) {
            if (!visited.has(neighbor)) {
                queue.enqueue(neighbor);
                visited.add(neighbor);
            }
        }
    }
}

Or as
function graphBFS(graph, start) {
    const queue = [start], visited = new Set(queue);
    while (queue.length) {
        for (const item of graph.neighbors(queue.shift())) {
            if (!visited.has(item)) {
                queue.push(item);
                visited.add(item);
            }
        }
    }
}

The latter I would score higher as it demonstrates a higher level of understanding of the problem being solved, reduced dependency and thus more portable and reusable, and reduced use of abstraction in favor of consistency.
EG If the function were to be change to depth first you need only change the function graph.neighbors and not rename neighbor 4 times due to strong coupling with the abstract. An item of neighbors is still meaningful as an item of roots (Or whatever you call the forward edge traverse)


Repetitive wordpress PHP styling I was wondering if this was a decent way of handling this function. Please take a look.

// Use Less Mixins from Theme Options to adjust Stylesheets before Parsing Less to CSS
add_filter( 'less_vars', 'wellies_less_vars', 10, 2 );
function wellies_less_vars( $vars, $handle ) {

    //Necessary for UpTheme Framework
    global $up_options;

    //Grab Theme option and check if it's empty and default to color if it is.
    $linkcolor = empty($up_options->linkcolor) ? '#08c' : $up_options->linkcolor;
    $linkcolorhover = empty($up_options->linkcolorhover) ? '#03c' : $up_options->linkcolorhover;
    $networknavstart = empty($up_options->networknavstartcolor) ? '#333' :$up_options->networknavstartcolor;
    $networknavend = empty($up_options->networknavendcolor) ? '#222' :$up_options->networknavendcolor;
    $sitenavstart = empty($up_options->sitenavstartcolor) ? '#333' :$up_options->sitenavstartcolor;
    $sitenavend = empty($up_options->sitenavendcolor) ? '#222' :$up_options->sitenavendcolor;

    //Set Less Mixins to Variables above
    $vars = array(
        'linkColor' => $linkcolor,
        'linkColorHover' => $linkcolorhover,
        'networkNavBarStart' => $networknavstart,
        'networkNavBarEnd' => $networknavend,
        'siteNavBarStart' => $sitenavstart,
        'siteNavBarEnd' => $sitenavend,
    );
    return $vars;
}
//Enqueue Stylesheets if not on dashboard.
function wellies_css_loader() {
    if ( ! is_admin() )
        wp_enqueue_style('bootstrap', get_bloginfo('template_directory').'/library/css/lib/bootstrap.less',  array(), '', 'screen, projection');
    wp_enqueue_style('themes', get_bloginfo('template_directory').'/library/css/theme.less', array(), '', 'screen, projection');
}
add_action('wp', 'wellies_css_loader');

It is currently working but I don't want to have it slow down the site if I can optimize it somehow.

No, this can be written a lot easier.  There is no need to create variables when you are not going to vary the value.  It actually only makes things harder to read.  Assuming you are using PHP 5.3+ consider this:

// It is odd that you aren't using $vars in your method.
// I'll ignore that for now.
function wellies_less_vars( $vars, $handle ) {

    //Necessary for UpTheme Framework
    // This is not nice, but I'll ignore that too.
    global $up_options;

    //Set Less Mixins to Variables above
    // Grab Theme option and check if it's set and use that, otherwise use a
    // default value.
    // Now there is beauty in the alignment IMO.
    return array(
        'linkColor'          => $up_options->linkcolor            ?: '#08c',
        'linkColorHover'     => $up_options->linkcolorhover       ?: '#03c',
        'networkNavBarStart' => $up_options->networknavstartcolor ?: '#333',
        'networkNavBarEnd'   => $up_options->networknavendcolor   ?: '#222',
        'siteNavBarStart'    => $up_options->sitenavstartcolor    ?: '#333',
        'siteNavBarEnd'      => $up_options->sitenavendcolor      ?: '#222');
}

There is a small difference in functionality, but a large difference in readability.  The ?: uses the value on the left if it evaluates to true (generally a set variable except false), or the value on the right if it is not.


All permutations of the pairs that add to squares Given a number, this program would find "all permutations of the pairs that add to squares" from 1 to number.

  E.g: Given an input number of 16, one of the possible answers would be:

8:1:15:10:6:3:13:12:4:5:11:14:2:7:9:16

  and its reverse.

Note that the function called hasIntegerRoot has been commented out for a purpose. I would appreciate if reviewer would chose me to use hasIntegerRoot over currently used List<Integer> squareList.

I'm looking for code review, best practices, optimizations etc.

Verifying Complexity (where \$n\$ is the input number):

Time: \$O(n!)\$
Space: \$O(n!)\$

public final class PairSquare {

    private PairSquare() {};

    /**
     *  Given a number, this program would find "all permutations of the pairs that add to squares" from 1 to number"
     *
     * @param num        The number upto which adjacent numbers should sum to a sqaure
     * @return           A List of all the lists/chains of integers such that adjacent numbers sum upto a square.
     */
    public static List<List<Integer>> pairSquare (int num) {
        if (num <= 0) throw new IllegalArgumentException("The input number " + num + " should be less than equal to zero.");

        final List<List<Integer>> pairSquaresList = new ArrayList<List<Integer>>();
        final List<Integer> squareList = squareList(num);
        final LinkedHashSet<Integer> elements = new LinkedHashSet<Integer>();
        for (int i = 1; i <= num; i++) {
            elements.add(i);
            getSqaureLists(i, num, elements, pairSquaresList, squareList);
            elements.remove(i);
        }
        return pairSquaresList;
    }

    private static List<Integer> squareList(int num) {
        int limit = num + (num  - 1);
        int n = 2;
        final List<Integer> sqaureList = new ArrayList<Integer>();
        int square;
        while ((square = n * n) <= limit) {
            sqaureList.add(square);
            n++;
        }
        return sqaureList;
    }

//    private static boolean hasIntegerRoot(int x) {
//        double root = Math.sqrt(x);
//        return  root == (int)root;
//    }

    private static void getSqaureLists(int currNum, int num, LinkedHashSet<Integer> elements, List<List<Integer>> pairSquareList, List<Integer> squareList) {
        if (elements.size() == num) {
            pairSquareList.add(new ArrayList<Integer>(elements));
            return;
        }

        for (int i = 1; i <= num; i++) {
            // a number has already been added in the list. deduping.
            if (elements.contains(i))  continue;

//            // checking if the adjacent numbers add up to a square of an int.
//            if (hasIntegerRoot(i + currNum)) {
//                elements.add(i);
//                getSqaureLists(i, num, elements, pairSquareList, squareList);
//            }

            if (squareList.contains(i + currNum)) {
                elements.add(i);
                getSqaureLists(i, num, elements, pairSquareList, squareList);
            }

            elements.remove(i);
        }
        return;
    }

    public static void main(String[] args) {
        List<Integer> list1 = Arrays.asList(8, 1, 15, 10, 6, 3, 13, 12, 4, 5, 11, 14, 2, 7, 9, 16);
        List<Integer> list2 = Arrays.asList(16, 9, 7, 2, 14, 11, 5, 4, 12, 13, 3, 6, 10, 15, 1, 8);

        List<List<Integer>> listOfLists = new ArrayList<List<Integer>>();
        listOfLists.add(list1);
        listOfLists.add(list2);

        Assert.assertEquals(listOfLists, pairSquare(16));
    }
}

Since you don't seem to be concerned with the order of any of the collections of objects you use, only membership in those collections, Set is probably the better Collection to use, instead of List.  I imagine that Set.contains is going to be more efficient than List.contains.

Also, it's not clear to me that recursion is the way to go here.  If I were you, I would try to walk through the code with a low value for the input number and see what it's doing.  It looks like for every integer between 1 and num, you're calling getSqaureLists num x num times.  That seems like overkill.  Recursion typically does some work and then passes a smaller set of work to the recursive call.  You need to clarify how you split up that work.  If that's not working, a iterative solution might be clearer.

You also have some code to check for duplicates.  You shouldn't need to do that.  Whether you a doing an iterative or recursive solution, you should be able to avoid ever testing duplicate values.


Data transformation using Lodash in a more readable way I don't like how transforming the data requires a bunch of nested functions. It's not very readable. Is there a more readable way to do the same transformations in Lodash?

var templates = {
  "fullRenderV80": {
    "stuff": "stuff",
    "screenshots": [
      {
        "device": "iPhone 6",
        "position": 1
      },
      {
        "device": "Watch",
        "position": 2
      }
    ]
  },
  "fullRenderV70": {
    "stuff": "stuff",
    "screenshots": [
      {
        "device": "iPhone 6",
        "position": 1
      },
      {
        "device": "iPad",
        "position": 2
      }
    ]
  }
}

var transformedData = _.groupBy(_.map(templates, 'screenshots'), function(screenshotsArray) {
  return _.reduce(screenshotsArray, function(templateDevicesName, screenshot) {
    return templateDevicesName.concat(screenshot.device)
  }, []).join(' + ')
})

// transformedData is this:
//{
//  "iPhone 6 + Watch": [
//    [
//      {
//        "device": "iPhone 6",
//        "position": 1
//      },
//      {
//        "device": "Watch",
//        "position": 2
//      }
//    ]
//  ],
//  "iPhone 6 + iPad": [
//    [
//      {
//        "device": "iPhone 6",
//        "position": 1
//      },
//      {
//        "device": "iPad",
//        "position": 2
//      }
//    ]
//  ]
//}

Take advantage of lodash's _() wrapper function, which wraps a value, and allows chaining of methods.

function grouper(screenshotsArray){
  return _.reduce(screenshotsArray, function(templateDevicesName, screenshot) {
    return templateDevicesName + ' + ' + screenshot.device;
  }, '');
}

var transformedData = _(templates).map('screenshots').groupBy(grouper).value();


Haskell MultiWayIf extension: When is it considered useful syntactic sugar? The new MultiWayIf extension (available with GHC 7.6) allows guard syntax in an if:

{-# LANGUAGE MultiWayIf #-}
fn :: Int -> String
fn x y = if | x == 1    -> "a"
            | y <  2    -> "b"
            | otherwise -> "c"

But I don't find it better than the old way:

fn :: Int -> String
fn x y | x == 1    = "a"
       | y <  2    = "b"
       | otherwise = "c"

Has it sense in other cases?

Unsurprisingly, it's mainly useful when the if is not the top-level expression. Say:

forM_ [1..100] $ \i ->
  putStrLn $ if | i `mod` 15 == 0  -> "FizzBuzz"
                | i `mod` 3 == 0   -> "Fizz"
                | i `mod` 5 == 0   -> "Buzz"
                | otherwise        -> show i

I happen across similar cases quite often - up to this point this required either deep if trees or mis-using case in some way.


Ignore exceptions when changing column formula in Excel table The new table feature on Excel is excellent and you can specify a column formula as well as exceptions on the column. However, if you want to change the column formula, Excel will rewrite the whole column including the exceptions. I've tried to avoid this issue with the following code:

Private Sub Worksheet_Change(ByVal Target As Range)
    On Error GoTo ErrHandler

    'Prevent from firing if changed cell is outside a table
    If Target.ListObject Is Nothing Then Exit Sub
    'Prevent from firing if cell is changed to a value
    If Val(Target.Formula) = Target.Value Then Exit Sub
    'Prevent from firing if multiple cells where changed
    If Target.Count > 1 Then Exit Sub
    'Disable events
    ToggleWaitMode

    'Prevent change if user already stated calculation as manual
    Dim bCheckCalculationMode As Boolean
    If Application.Calculation = xlCalculationAutomatic Then
        Application.Calculation = xlCalculationManual
        bCheckCalculationMode = True
    End If

    'Declarations
    Dim sTableName As String
    Dim lNbLines As Long
    Dim i As Long
    Dim sFieldName As String
    Dim cel As Range
    Dim sOldFormula As String
    Dim sNewFormula As String
    Dim aExceptionFormulas() As String
    Dim aExceptionAddresses() As String

    sTableName = Target.ListObject.Name
    lNbLines = Range(sTableName).Rows.Count
    sFieldName = "[" & Intersect(Target.ListObject.HeaderRowRange, Target.EntireColumn).Value & "]"

    'Add a new row to get the column formula
    Range(sTableName).ListObject.ListRows.Add
    sOldFormula = Range(sTableName & sFieldName)(lNbLines).Formula

    sNewFormula = Target.Formula

    'Loop on each cell of the column to get each exception formulas and addresses in arrays
    i = 1
    For Each cel In Range(sTableName & sFieldName)
        If cel.Formula <> sOldFormula Then
            ReDim Preserve aExceptionFormulas(1 To i)
            ReDim Preserve aExceptionAddresses(1 To i)
            aExceptionFormulas(i) = cel.Formula
            aExceptionAddresses(i) = cel.Address
            i = i + 1
        End If
    Next cel

    'Delete added row and replace column to new formula
    Range(sTableName).ListObject.ListRows(lNbLines).Delete
    Range(sTableName & sFieldName).Formula = sNewFormula

    If i > 1 Then
        For i = 1 To UBound(aExceptionFormulas)
            Range(aExceptionAddresses(i)).Formula = aExceptionFormulas(i)
        Next i
    End If
    ActiveSheet.Calculate

CleanExit:
    If bCheckCalculationMode Then Application.Calculation = xlCalculationAutomatic
    ToggleWaitMode False
    Exit Sub
ErrHandler:

End Sub

Private Sub ToggleWaitMode(Optional ByVal bWait As Boolean = True)
    Application.EnableEvents = Not bWait
    Application.AutoCorrect.AutoFillFormulasInLists = Not bWait
End Sub

The code works but it's really ugly. Do you think it's possible to optimize it? For example by finding the Inconsistent Formulas?

Edit: I've modified the code accordingly to the great advices of Mat's Mug. I hope it got better. Can the added row be avoided? It's just a workaround to get the column formula.

Private Sub Worksheet_Change(ByVal Target As Range)

The Worksheet.Change event gets fired whenever anything changes anywhere in the worksheet. Code you write in a handler for that event must do its business as fast as possible, or it can significantly affect (negatively) the performance of the whole Excel instance.

Whenever possible, avoid using this event for anything that doesn't need to run whenever anything changes anywhere in the worksheet.

Application.Calculation = xlCalculationManual
If Target.Count = 1 Then
Application.EnableEvents = False

Your indentation is off, the assignment Application.EnableEvents = False should be one Tab further to the right.

I like that you're disabling automatic calculation and other worksheet event handlers, but I don't understand why only one of them is under the If block.

    Application.EnableEvents = True
    End If
    ActiveSheet.Calculate
End Sub

The result of this is that you're calculating the active sheet, but you've disabled automatic calculation of the workbook without reinstating it, and you end up systematically calling ActiveSheet.Calculate, which gives the illusion that automatic calculation is still enabled.

tb = Target.ListObject.Name

If the modified cell is outside of a ListObject, this is where your code blows up, and because you're not handling run-time errors, this is where your user sees a VBA debugger prompt and might accidentally end up in your source code with a puzzled facial expression.

Whenever you start writing a method/procedure/function in VBA, you should start with something like this:

Private Sub Worksheet_Change(ByVal Target As Range)
    On Error GoTo ErrHandler

    'code goes here

CleanExit:
    Exit Sub
ErrHandler:
    Resume CleanExit
    Resume 'unreachable code, for debugging
End Sub

Now you can put a breakpoint on the Resume CleanExit instruction, and can inspect the Err object and then set the Resume dead code as the next statement to be executed, and be taken (F8) exactly on the line the error came from, whenever anything goes wrong with this code.

Dim tb As String
tb = Target.ListObject.Name

If tb is supposed to be the name of a table, then why is it not called tableName? Use descriptive identifiers when naming things!

Dim l As Long
Dim i As Long
Dim s As String
Dim cel As Range
Dim fmla As String
Dim nfmla As String
Dim mfmla() As String
Dim mad() As String

Oh, boy. Need I say more?

I'm not going to review your algorithm (not going to try to wrap my head around those cryptic identifiers), but one thing I can say is that, for the reasons explained at the top of this answer, I'd start the procedure like this:

If Target.ListObject Is Nothing Then Exit Sub

That way you don't bother with anything if the active cell isn't in a table, and the handler can't blow up either when you later try to get the table's name.

Also by only testing for Target.Count = 1 all you're really doing, is ensuring that the current selection only has a single cell. This means if a user selects an entire row (or column) and starts entering data, your code happily sets calculation mode to manual, skipts the entire handler and recalculates the sheet.

Hence, the next thing I'd do after ensuring the Target range has a ListObject, would be to toggle calculation mode and event firing. It's probably best to write a small procedure just for that:

Private Sub ToggleWaitMode(Optional ByVal wait As Boolean = True)
    Application.Calculation = IIf(wait, xlCalculationManual, xlCalculationAutomatic)
    Application.EnableEvents = Not wait
    Application.AutoCorrect.AutoFillFormulasInLists = Not wait
End Sub

That leaves your handler with pretty much only the code that's relevant for its task:

Private Sub Worksheet_Change(ByVal Target As Range)
    On Error GoTo ErrHandler

    If Target.ListObject Is Nothing Then Exit Sub
    ToggleWaitMode

    '...

CleanExit:
    ToggleWaitMode False
    Exit Sub
ErrHandler:
    Resume CleanExit
    Resume 'unreachable code, for debugging
End Sub

I don't like the ReDim Preserve array handling. You know how many cells you're going to be iterating over (Range(tb & s).Count), therefore you know the size of the array you're going to need at the moment you're declaring it. You'll probably gain some performance by doing this, depending on how many cells the range has.


Text Analysis web page For a homework assignment, I made a web page that has a text area. The user inputs some text, hits the analyse button, and the output is supposed to be a list of the frequency of words of a given number of characters.

For example, "one two three" would be two words of three characters and one word of five characters.

It works, but is there room for improvement?

HTML:

Text Input

<textarea id="text" placeholder="type something..." rows="20" cols="80"></textarea>
<button id="analyse">Click to analyze</button>

Results

JavaScript:

function getWordInfo() {
    var text1 = document.getElementById("text").value;
    var text2 = text1.replace(/[^\d\w ]+/g," ");
    var text3 = text2.split(/[ ]+/);
    return text3;
}

function firstLoop() {
    var text = getWordInfo();
    var wordMap = [];
    var i =0;
    var looplength = text.length;
    for ( i ;i < looplength; i++) {
        wordMap.push(text[i].length)
    }
    return wordMap;
}

function objCon(name,count) { //object constructor
    this.name = name;
    this.count = count;
}

function maxNum() {
    var array1 = firstLoop();
    var num = Math.max.apply(Math, array1);
    return num;
}

function objArrCon() { //object array constructor
    var num = maxNum();
    var array1 = [];
    var i = 2;
    for ( i ; i <= num; i++) {
        var myObj = new objCon(i,0);
        array1.push(myObj);
    }
    return array1;
}

function objArrParse() { //updates the object with word info
    var array1 = firstLoop();
    var array2 = objArrCon();
    var loopLength1 = array1.length;
    var loopLength2 = array2.length;
    for (var i = 0; i < loopLength1; i++) {
        for (var m = 0; m < loopLength2; m++) {
            if (array2[m].name === array1[i]) {
                array2[m].count++;
                }
        }
    }
    return array2;
}

function objArrTrun() { //object array truncation
    var array1 = objArrParse();
    var len = array1.length;
    for (var i = 0;i < len; i++) {
        if (array1[i].count === 0) {
            array1.splice(i, 1);
            len--;
            }
        }
    return array1;
}

function formatter() {
    var strAr = objArrTrun();
    var arrayLength = strAr.length;
    var formatStr = [];
    for (var i = 0; i < arrayLength; i++) {
        var str = "Number of characters: " + strAr[i].name + ", Number of words with this length: " + strAr[i].count;
        formatStr.push(str);
    }
    return formatStr;
}

function clearBox(elementID) {
    document.getElementById(elementID).innerHTML = "";
}

function analyseButtonClick() {
    clearBox("results");
    var html = " ";
    var str = formatter();
    var len = str.length;
    for (var i = 0; i < len; i++) {
        html += "
" + str[i] + "
";
}
    document.getElementById("results").innerHTML += html;
}

function init() {
    var button = document.getElementById("analyse");
    button.onclick = analyseButtonClick;
}

window.onload = init;

Here's a different approach, using the DOM instead of innerHTML, and using document fragments to append all at once and improve performance. Also you don't need that many regex, specially if you're copy/pasting them; splitting by any whitespace character and removing empty elements from the resulting array will do.

Then you could make use of the builtin higher-order functions such as map and
reduce to minimize the use of for loops and make the code a bit cleaner:

var doc = document;

var extend = function(a, b) {
  Object.keys(b).forEach(function(k){a[k] = b[k]});
  return a;
};

var elem = function(tag, props) {
  return extend(doc.createElement(tag), props);
};

/**
 * @param words {Array}
 * @returns {Object} Frequency of words per length
 */
var frequency = function(words) {
  return words.filter(Boolean).reduce(function(acc, x) {
    acc[x.length] = ++acc[x.length] || 1;
    return acc;
  },{});
};

var empty = function(x) {
  while (x.hasChildNodes())
    x.removeChild(x.lastChild);
  return x;
};

/**
 * @param freq {Object} Frequence of words per length
 * @returns {Node} Document fragment
 */
var resultOf = function(freq) {
  return Object.keys(freq).reduce(function(frag, k) {
    frag.appendChild(elem('p', {
      textContent: 'Words with '+ k +' chars: '+ freq[k]
    }));
    return frag;
  }, doc.createDocumentFragment());
};

var textarea = doc.getElementById('text');
var button = doc.getElementById('analyse');
var result = doc.getElementById('results');

button.addEventListener('click', function() {
  var res = resultOf(frequency(textarea.value.split(/\s+/)));
  empty(result).appendChild(res);
});

Demo: http://jsbin.com/motoy/1/edit

To learn more about these idioms, I'd recommend reading JavaScript Allonge, and Functional JavaScript. For JavaScript documentation visit the MDN


Play around with wait and notify The following is an a self-tutored attempt to take a leap into understanding simple usage of wait() and notify() methods.

The code is responsible for scanning a directory structure with each directory being scanned by a new thread, for the files present in it. All results to get accumulated in a data collection and displayed at the end. The problem is to detect the end, since this is a chain reaction type.

class MyThreadA implements Runnable {

    private String path;
    private static List<String> FILE_LIST;
    private static AtomicInteger ATOMIC_COUNTER;

    static {
        ATOMIC_COUNTER = new AtomicInteger(0);
        FILE_LIST = new LinkedList<>();
    }

    public MyThreadA(String path) {
        this.path = path;
    }

    @Override
    public void run() {
        try {
            int begin = ATOMIC_COUNTER.getAndIncrement();
            File filePath = new File(path);
            File[] listOfFiles = filePath.listFiles();
            synchronized (System.out) {
                for (File temp : listOfFiles) {
                    if (temp.isFile()) {
                        FILE_LIST.add(temp.getAbsolutePath());
                    } else if (temp.isDirectory()) {
                        Thread t = new Thread(new MyThreadA(temp.getPath()));
                        t.start();
                    }
                }
            }
            MyThreadA.decrementCounter();
            //if(begin == 0)
                try {
                    synchronized (FILE_LIST) {
                        FILE_LIST.wait();
                    }
                    printList();
                } catch (InterruptedException e) {
                    e.printStackTrace();
                }
        } catch (Exception e) {
            e.printStackTrace();
        }
    }

    private static void decrementCounter() {
        if (ATOMIC_COUNTER.decrementAndGet() == 0)
            synchronized(FILE_LIST){
                FILE_LIST.notifyAll();
            }
    }

    private static void printList(){
        for(String temp:FILE_LIST){
            System.out.println(temp);
        }
    }
}

Now, as you can see, all such spawned threads will wait to be notified. I can minimize this by letting the 1st thread only to wait, but that introduces one more critical section. I am looking for if there can be a better solution to this using wait() and notify() constructs only, with minimal concurrency tools.

I understand that there are better tools for solving this, but this is solely for learning purpose.

There is a race condition in your code. When you call t.start() the thread doesn't start immediately which means that it's possible to have a thread waiting to execute but the current thread can call notifyall before the new thread has a chance to increment the counter. This can be fixed by doing the increment in the constructor instead of in run.

On another note, FILE_LIST.add is not safe to call from multiple threads. All the rest around that is safe. So, the for loop can be reduced to:

for (File temp : listOfFiles) {
    if (temp.isFile()) {
        synchronized (FILE_LIST) {
            FILE_LIST.add(temp.getAbsolutePath());
        }
    } else if (temp.isDirectory()) {
        Thread t = new Thread(new MyThreadA(temp.getPath()));
        t.start();
    }
}

When waiting on an object there is the (documented) possibility of a spurious wakeup, which means that the thread woke up without anyone calling notify.

The way to deal with that is to wait in a loop:

synchronized (FILE_LIST) {
    while(ATOMIC_COUNTER.get() > 0)
        FILE_LIST.wait();
}

The condition is inside the synchronized to avoid a race condition (not the one detailed above though).


Checking if entity exist in Google App Engine Datastore using JPA 1 Below is my method in checking if entity already exist.

public boolean isExist(String genreName) throws PersistenceException{
    EntityManager em = EMF.get().createEntityManager();
    boolean flag = false;

    try{
        Genre genre = em.find(Genre.class, genreName);
        if (genre != null)
            flag = true;

    }finally{
        em.close();
    }

    return flag;
}

Is the code above ok? Please suggest.

It looks fine. You could omit the boolean flag if you return immediately when you know the answer:

public boolean isExist(final String genreName) throws PersistenceException {
    final EntityManager em = EMF.get().createEntityManager();
    try {
        final Genre genre = em.find(Genre.class, genreName);
        if (genre != null) {
            return true;
        }
        return false;
    } finally {
        em.close();
    }
}


Looking to shorten method of parsing arguments in BASH I have here a working method of parsing input arguments however it seems redundant. I would like to shorten this code so it doesn't appear so repetitive.

tool.sh <file-to-use>
tool.sh <file-to-use> -o <output-file-name>

# If arguments are set and equals "-o" then set APP_NAME
if [[ -n $3 ]] && [[ $2 = "-o" ]]; then
    APP_NAME=$3

# If argument is set and equals anything besides "-o" throw error and show help menu
elif [[ -n $2 ]] && [[ $2 != "-o" ]]; then
    echo "ERROR: Synatx error."
    bash $0 -h
    exit

# If "-o" is set but they did not give a output name throw error and display help menu
elif [[ -z $3 ]] && [[ $2 = "-o" ]]; then
    echo "ERROR: Did not specify output file name."
    bash $0 -h
    exit
fi

Use the ${variable?error message} syntax to error on empty arguments.  Use shift to remove parsed arguments from the positional parameters, so that the next unseen argument is always $1.  If there are any arguments left at the end, there were too many.

When a message isn't enough, you can use trap … EXIT to call a function on error.  Remember to remove the trap after parsing and at the very start of your handler function.

This version isn't any shorter than yours but it's more generic.  You can add parameters or rearrange their order without altering the numbers in the existing logic.  The error-handling is in one place instead of two.

show_help() {
 trap - EXIT # omitting this can lead to an infinite loop
 exec $0 -h
 exit
}

trap show_help EXIT
error_usage="Usage: $0 input-file [-o output-file]"
input=${1?$error_usage} && shift
[[ $1 = -o ]] && output=${2?$error_usage} && shift 2
[[ -n $1 ]] && echo $error_usage && exit 0
trap - EXIT

echo "input=$input output=$output"

Rather than invoking ourselves to display help, I'd just put the help logic in the show_help function.


Byte Array Constant Time Compare I am writing a function that does a constant time compare of two byte arrays. I want it to be constant time to insure there is no side channel attacks possible.

Does the code protect against timing attacks or other side channel attacks (of which I could fix)
Any other suggestions for improvements

Note: This code is called infrequently so performance is not a big deal

/// <summary>
/// Performs a comparison of two identical sized byte arrays.
/// Performs in constant time to prevent timing/side channel attacks
/// </summary>
public int CompareConstantTime(byte[] bytesA, byte[] bytesB)
{
    if (bytesA == null)
        throw new ArgumentNullException(nameof(bytesA));
    if (bytesB == null)
        throw new ArgumentNullException(nameof(bytesB));
    if (bytesA.Length != bytesB.Length)
        throw new ArgumentOutOfRangeException("byte length must be equal");

    //only the lest significant bit is used
    int aIsLarger = 0;
    int bIsLarger = 0;

    unchecked
    {
        for (int i = 0; i < bytesA.Length; i++)
        {
            int byteA = bytesA[i];
            int byteB = bytesB[i];

            int byteAIsLarger = ((byteB - byteA) >> 8) & 1;
            int byteBIsLarger = ((byteA - byteB) >> 8) & 1;

            aIsLarger = aIsLarger | (byteAIsLarger & ~bIsLarger);
            bIsLarger = bIsLarger | (byteBIsLarger & ~aIsLarger);
        }

        //fixes to standard compaire results (0 if A = B, 1 if A > B, -1: B > A)
        int result = aIsLarger - bIsLarger;
        return result;
    }
}

Some nitpicking:

/// <summary>
/// Performs a comparison of two identical sized byte arrays.
/// Performs in constant time to prevent timing/side channel attacks
/// </summary>

This does not answer arguably the main question one would have: what does it mean for one array to be "larger" than another, when both have equal size? Do you compare sums of the elements? Do you compare first non-equal pair of elements (looks like this one is correct)? Do you use some other even less obvious criteria? You should explain this in above documentation, so it is not necessary to read through obscure boolean logic in order to get the answer.

public int CompareConstantTime(byte[] bytesA, byte[] bytesB)

This method should probably be refactored into static extension method, so it is easier to re-use if needed.

public static int ConstantTimeCompareTo(this byte[] bytes, byte[] otherBytes)

throw new ArgumentOutOfRangeException("byte length must be equal");

There is no range per se at this point. I think base ArgumentException is a better fit.


Stored procedure for processed orders in SQL Server I am struggling with creating a Stored Procedure in SQL Server for getting sales statistics. I have managed to get the information I need out, but the query is a mess and I have probably messed up the table variable by using 3 different ones.

Any ideas on how to merge this triple part-query into one?

BEGIN
SET NOCOUNT ON;

    DECLARE @SalesStats TABLE
    (
        NumberOfProcessed int,
        SellerID int
    )

    DECLARE @1 TABLE(
    NumberOfPaidOrders int,
    SellerID int
    )

    DECLARE @2 TABLE(
    TotalAmountPaid int,
    SellerID int
    )

    --Number of Processed orders. Removing 0 as its test account.

    INSERT INTO @SalesStats (NumberOfProcessed, PO.SellerID)
    SELECT COUNT(PO.OrderId) AS NumberOfProcessed, PO.SellerID
    FROM ProcessedORder AS PO
    WHERE NOT PO.SellerID = 0
    GROUP BY PO.SellerID
    ORDER BY PO.SellerID

    --Number of paid orders

    INSERT INTO @1 (NumberOfPaidOrders, SellerID)
    SELECT COUNT(AA.OrderId) AS NumberOfPaidOrders, AA.SellerID
    FROM AcceptedOrder AS AA
    LEFT JOIN ProcessedORder AS PO ON AA.OrderId = PO.OrderId
    GROUP BY AA.SellerID
    ORDER BY AA.SellerID

     --Total amount paid

    INSERT INTO @2 (TotalAmountPaid, SellerID)
    SELECT SUM(AA.Amount) AS TotalAmountPaid, SellerID
    FROM AcceptedOrder AS AA
    GROUP BY AA.SellerID
    ORDER BY AA.SellerID

    SELECT DISTINCT(SS.SellerID), SS.NumberOfProcessed, one.NumberOfPaidOrders, two.TotalAmountPaid, two.TotalAmountPaid / one.NumberOfPaidOrders AS AverageAmountPaid FROM @SalesStats AS SS
    LEFT JOIN @1 AS one ON SS.SellerID = one.SellerID
    LEFT JOIN @2 AS two ON SS.SellerID = two.SellerID
    ORDER BY SS.NumberOfProcessed DESC
END

Table variables

Because you are using them only once, and as part of a bigger query, there is really no benefits in using a table variable nor a temporary table. The best choice in this case is to use a Common Table Expressions, or CTE.

A CTE basically works like a subquery but unlike a traditional subquery, it has the advantage of reading from top to bottom rather than inside out as traditionally done. Here is a sample:

WITH SalesStats AS (
  SELECT PO.SellerID, COUNT(PO.OrderId) AS NumberOfProcessed
  FROM ProcessedOrder AS PO
  WHERE PO.SellerID <> 0
  GROUP BY PO.SellerID
)
SELECT ss.SellerID, ss.NumberOfProcessed
FROM SalesStats AS SS;

Improper use of ORDER BY

Your INSERT INTO queries has a ORDER BY which contributes nothing and in fact may penalize your performance. The only time you should use an ORDER BY is in the final select.

There are legitimate cases where your INSERT INTO (or an UPDATE) may need an ORDER BY but those are the exceptions, not the rule. By putting in an ORDER BY, you are tying up the optimizer's hands because it has less choices it can make in optimizing the queries. Don't do that.

Lack of schema names

You do not qualify your tables. It's very good practice to always use two-naming as the default --- schema.table rather than just table. Consider that multiple schemas within a database could have a table with same name, that a user could be in their own schema (especially from older database where things worked differently back then), that you are forcing the server to do a search of all schema to bind the tables. I'm going to assume that your tables are in the default dbo schema. Therefore....

SELECT PO.SellerID, COUNT(PO.OrderId) AS NumberOfProcessed
FROM dbo.ProcessedOrder AS PO
WHERE PO.SellerID <> 0
GROUP BY PO.SellerID;

Note that a CTE do not have two-part naming because it is defined in the scope of query. But all real objects (e.g. tables or views or stored procedures) should all be named with schema and object names explicitly.

Column ordering

To be honest, I found it confusing to see aggregates listed first then the grouping columns last. Normally, I would write query vertically, with grouping columns first.

SELECT
  PO.SellerID,
  COUNT(PO.OrderId) AS NumberOfProcessed
FROM dbo.ProcessedOrder AS PO
WHERE PO.SellerID <> 0
GROUP BY PO.SellerID;

In a small query with only few columns it might be OK to have them as one line but for more complicated queries, vertical listing makes it much easier to see what is going out from that query. By listing the grouping columns, it helps to verify the GROUP BY as well.

Unnecessary use of DISTINCT

In all queries up to the query, you group on SellerID. Thus they are already distinct because by definition, grouping generate one row per value in that column, so it's already distinct. Don't force the optimizer to do more work than necessary.

Lack of semicolons

Upon a time, terminating a SQL statement was optional. Now, it's very much recommended that you always terminate a SQL statement. Some people cheat by adding semicolons only when they are required (e.g. before a CTE or a MERGE statement) but that makes for inconsistent and ugly code.

Furthermore, having an explicit terminator help to clearly separate each SQL statement and avoid overlapping due to ambiguity.

Potential division by zero error

You have this expression: two.TotalAmountPaid / one.NumberOfPaidOrders AS AverageAmountPaid

What if the number of paid orders is zero, not NULL? What if you later changed the queries that made it possible to put in zeroes? It's good to always guard against that to make your queries easier to change if you need to do so in future and is as simple as using NULLIF: TotalAmountPaid / NULLIF(NumberOfPaidOrders, 0). This basically substitutes the 0 with a NULL, which causes the results to be NULL without raising any runtime errors.

Putting it all together

With CTEs, we can consolidate this into a single query.

WITH SalesStats AS (
  SELECT
    PO.SellerID,
    COUNT(PO.OrderId) AS NumberOfProcessed, PO.SellerID
  FROM dbo.ProcessedOrder AS PO
  WHERE NOT PO.SellerID = 0
  GROUP BY PO.SellerID
), CountOfPaidOrders AS (
  SELECT
    AA.SellerID,
    COUNT(AA.OrderId) AS NumberOfPaidOrders
  FROM dbo.AcceptedOrder AS AA
  LEFT JOIN dbo.ProcessedOrder AS PO
    ON AA.OrderId = PO.OrderId
  GROUP BY AA.SellerID
), TotalAmountPaid AS (
  SELECT
    AA.SellerID,
    SUM(AA.Amount) AS TotalAmountPaid
  FROM dbo.AcceptedOrder AS AA
  GROUP BY AA.SellerID
)
SELECT
  SS.SellerID,
  SS.NumberOfProcessed,
  C.NumberOfPaidOrders,
  T.TotalAmountPaid,
  T.TotalAmountPaid / NULLIF(C.NumberOfPaidOrders, 0) AS AverageAmountPaid
FROM SalesStats AS SS
LEFT JOIN CountOfPaidOrders AS C
  ON SS.SellerID = one.SellerID
LEFT JOIN TotalAmountPaid AS T
  ON SS.SellerID = two.SellerID
ORDER BY SS.NumberOfProcessed DESC;

Now the optimizer can see how they all are used, and thus form a better execution plan and you don't need to worry about optimizing, beyond ensuring that the optimizer has the indices it needs.

The CTEs only live for a single SQL statement so if you had to modify the procedure that you end up accessing the CountOfPaidOrders (for instance) multiple times within a stored procedure, you do need a temporary table instead, but in this case, I don't see benefits of creating one.


duplicate element removal I have tried to remove the duplicate elements from the array using c language, here I am using 3 for loops to remove the duplicate elements.
How can this optimize the code?
Are there any other simple methods to remove the duplicate elements?
Please help.
#include <stdio.h>

#define MAX_SIZE 100 // Maximum size of the array

int main()
{

 int arr[MAX_SIZE];
 int size;
 int i, j, k;

printf("Enter size of the array : ");
scanf("%d", &size);

printf("Enter elements in array : ");
for(i=0; i<size; i++)
{
    scanf("%d", &arr[i]);
}

for(i=0; i<size; i++)
{
    for(j=i+1; j<size; j++)
    {

        if(arr[i] == arr[j])
        {

            for(k=j; k<size; k++)
            {
                arr[k] = arr[k + 1];
            }

            size--;

            j--;
        }
    }
}

printf("\nArray elements after deleting duplicates : ");
for(i=0; i<size; i++)
 {
    printf("%d\t", arr[i]);
 }

return 0;
 }

The code looks reasonably short and clear, that's good.
It's obvious that you are a beginner though since your code layout is inconsistent in a few places.  Formatting the code consistently is mostly a boring task, that's why several people wrote programs to perform this formatting automatically.  There are many different formatting styles, try one that fits your own liking and use that style consistently.
One of these programs is ClangFormat.
The result of formatting your code with the default settings is:
#include <stdio.h>

#define MAX_SIZE 100 // Maximum size of the array

int main() {

  int arr[MAX_SIZE];
  int size;
  int i, j, k;

  printf("Enter size of the array : ");
  scanf("%d", &size);

  printf("Enter elements in array : ");
  for (i = 0; i < size; i++) {
    scanf("%d", &arr[i]);
  }

  for (i = 0; i < size; i++) {
    for (j = i + 1; j < size; j++) {

      if (arr[i] == arr[j]) {

        for (k = j; k < size; k++) {
          arr[k] = arr[k + 1];
        }

        size--;

        j--;
      }
    }
  }

  printf("\nArray elements after deleting duplicates : ");
  for (i = 0; i < size; i++) {
    printf("%d\t", arr[i]);
  }

  return 0;
}

In this version, the indentation of each line tells the reader which pieces of the code belong together. In your original version, this was not easy to see.
To advanced programmers, the consistently formatted version looks calm and ordered, just like code should look like. Your original version instead looks chaotic. So far for the first impression.

At your current stage of learning to program, you probably didn't learn how to define functions yet. Functions are an important way to structure your code, so that you can highlight the interesting parts of the code. In this case, the most interesting function of the code would be "remove duplicate elements from an array, given by the first element and the size of the array". In C, this is written like this:
// Removes duplicates, returns the new size of the array.
// Keeps the order of the original elements, removing the duplicates that occur later.
static int
remove_duplicates_from_int_array(int *arr, int arr_size)
{
    // TODO: actually implement this function
}

Separating the code that removes the duplicates from the rest of the code makes it possible to test and analyze this part of the code on its own. For example you can write a standard test case:
#include <assert.h>

...

    size = 3;
    arr[0] = 1;
    arr[1] = 3;
    arr[2] = 1;

    size = remove_duplicates_from_int_array(arr, size);

    assert(size == 2);
    assert(arr[0] == 1);
    assert(arr[1] == 3);

With this code, you don't have to enter the size and the elements yourself anymore, which is a huge time-saver.
Even better would be to extract this test code into its own function:
static void
test_remove_duplicates_from_int_array(void) {
    // the code from above
}

This allows you to quickly run this test code at the start of the program, to ensure that the code still works for the basic test cases, before you even start entering your own test cases manually. Again, this saves a lot of time.

When you carefully inspect your code and "run" it with pen and paper, you will notice that the code reads data from outside of the array. This bug needs to be fixed before you should think about performance. Having fast and wrong code is usually worse than slow and correct code.
This bug is not easy to find when you only look at the input and output, since everything seems to work fine. The tricky thing is that reading from outside of an array invokes undefined behavior, and this means that nothing at all is guaranteed when running this program. To really run into this undefined behavior, you have to enter the array size as 100 and actually enter exactly 100 array elements, something you probably never did since it is quite an effort. Therefore, for simpler testing, you should reduce MAX_SIZE to around 5. This allows you to test the edge cases more quickly.
Except for this bug, the code looks reasonable. After you have fixed this bug, you can start thinking about making the code run faster. That's the tricky part though. As long as the array is short, your current approach is simple and easy to understand. Only if your arrays grow larger you need to get concerned about the algorithm's complexity, which is currently \$\mathcal O(\mathrm{size} \cdot \mathrm{size} \cdot \mathrm{size})\$. That is, for an array with 1000 elements, you would need a billion steps.
To design a more efficient algorithm, print out a sheet of paper with 1000 random numbers and remove the duplicates manually, asking yourself what you do in each step and why. One idea may be to sort the numbers, after which it is much easier to remove the duplicates. But then you lose the original order of the numbers, which may or may not be acceptable.
So far for a few ideas, I'm not going to solve everything for you. :)


Can the performance of this canvas drawImage() test be improved? I have a quite simple loop that draws an 8px by 8px image many times in a canvas in a tiling fashion. Currently it's drawing 7500 images each loop.

See the jsfiddle.

var img = new Image();
img.src = 'http://i.imgur.com/3dzaMlv.png';

var W = 8;
var H = 8;
var R = 800/W;
var C = 600/H

var canvas = document.getElementById('canvas');
var ctx = canvas.getContext('2d');

var fps = document.getElementById('fps');
var timePrev = new Date().getTime();
var fpsInterval = 30;
var i = 0;

window.setInterval(function() {

    ctx.clearRect(0, 0, 800, 600);

    for (var r=0 ; r<R ; r++) {
        for (var c=0 ; c<C ; c++) {
            ctx.drawImage(img, r*W, c*H);
        }
    }

    // fps
    if (i % fpsInterval == 0) {
        var timeNow = new Date().getTime();
        var spf = (timeNow - timePrev) / fpsInterval / 1000;
        fps.innerHTML = (1/spf).toFixed(2);
        timePrev = timeNow;
    }
    i++;

}, 1000/60);

On my I7-2700K (3.5GHz) processor and ATI Radeon HD7970, I am getting the following framerates:

Chrome 36.0.1985.143 m  :  ~40 FPS
Firefox 30.0            :  ~55 FPS
IE 11.0.9600.17239      :  ~35 FPS

Is there any way to improve FPS performance here, assuming I do actually need to draw this many tiles on each update?

To clarify, I'm requiring that they be drawn as individual tiles on each update. So, rendering them all to an off-screen canvas wouldn't work. I am essentially disappointed in the number of drawImage() calls that seems to perform well.

Edit: Only now did I notice your comment that you require the tiles to be drawn individually. Since I don't know exactly why that is (I can only review the code I can see), I can't offer you any other advice than what I've already written below.
If you can redraw larger rects using a tiling fill style, or only update some tiles per frame, it'll be an improvement over redrawing everything each frame. But if your exact requirements are "redraw every tile by itself on every frame" then your current code is as probably as good as it gets. But the end result is the same as the much more efficient methods outlined below.

Edit edit: Don't know why I didn't think of it sooner, but off-screen drawing could be an improvement. The link provides a nice simple implementation.

You can draw the background using a single rect with a tiling background, and not do any drawImage calls. The technique is outlined in this StackOverflow answer. In its simplest form:

ctx.fillStyle = ctx.createPattern(image, 'repeat');
ctx.fillRect(0, 0, canvas.width, canvas.height);

Here's a demo - it runs at the target 60fps (in Chrome at least).

Alternatively, you could just set the image as a repeating CSS background image, and not do any JS at all. Just:

canvas { background: url(...) repeat; }

And done. Though I won't guarantee how that'll work with whatever you intend to draw on top of it.

In terms of code review, I noticed a couple of things

Single-letter variables - always better to use full words (the exception being known conventions like the i variable in a loop)
Speaking of, you can also avoid the hard-coded values for R, C, W and H "constants". The canvas' width/height you can get as simply canvas.width and canvas.height. Similarly (once the image has been loaded; see below), you can get its dimensions the same way: image.width and image.height.
You can use Date.now() instead of new Date().getTime()
You don't actually wait for the image to be loaded. It's a tiny image, but the network may be slow. So (as shown above), you should wait for the load event to fire on the image object, before trying to use it:

var image = new Image();
image.src = "...";
image.onload = function () {
  // use the image here
};

In general, though, don't expect exceptional performance from canvas. It's good, but it's not compiled code being mainlined to the GPUs in parallel. You're writing in JS - an interpreted language with a single-threaded runtime - doing compositing on top of an already complex view (the web page). Your many calls to drawImage each require the computer to do a lot of extra work - your loop is not a single operation; it's many individual operations and - depending on how smart the browser is, how much hardware acceleration there is, etc. - it takes time to update the screen after each draw.


Initializing immutable objects with a nested builder I need to initialize several parameters of an object but I also wanted to make it immutable. At the same time I don't want the constructor to take all several parameters because some of them are optional and have default values that the user can change during initialization like an IReadOnlyDictionary so I was experimenting with different patterns.

I found out that there is an interesting fact that if you make the builder nested inside the actual immutable class it then has still access to its private setters. This means you can build the actual object using the actual object without having to create copies in the builder itself and it still is immutable after exporting it, isnt't it?

This is how it looks like:

class Foo
{
    private IDictionary<string, string> _corge =
        new Dictionary<string, string>();

    public Foo() { }

    public string Bar { get; private set; }

    public IReadOnlyDictionary<string, string> Corge =>
        (IReadOnlyDictionary<string, string>)_corge;

    // API v3
    public static Foo Build(Action<Builder> build)
    {
        return Builder.Build(build);
    }

    public class Builder
    {
        private Foo _foo = new Foo();

        private Builder() { }

        // API v1
        public static Builder Create()
        {
            return new Builder();
        }

        // API v2
        public static Foo Build(Action<Builder> build)
        {
            var builder = new Builder();
            build(builder);
            return builder.ToFoo();
        }

        public Builder Bar(string bar)
        {
            _foo.Bar = bar;
            return this;
        }

        public Builder AddCorge(string key, string value)
        {
            _foo._corge.Add(key, value);
            return this;
        }

        public Foo ToFoo()
        {
            // prevents from reusing the builder after "ToFoo"
            var result = _foo;
            _foo = null;
            return result;
        }
    }
}

Usage with API v1:

var fooBuilder = Foo.Builder.Create();
fooBuilder.Bar("baz");
var foo = fooBuilder.ToFoo();
fooBuilder.Bar("QUUX"); // bam!

or with API v2:

var foo2 = Foo.Builder.Build(b => b.Bar("baz"));

or even shorther with API v3:

var foo2 = Foo.Build(b => b.Bar("baz"));

When I think of immutable object I think of get only properties backed with readonly fields.

  public string Bar { get; private set; }

That's not a fully immutable instance you still have the private set but removing the set completely will cause problems in your implementation, because your nested Builder class relies on the fact that it can set them.

What I think is better in this case is to have a private constructor to initalize your get only properties and instead of saving some private instance of the Foo object in the Builder you can save values :

public class Foo
{
    public string Bar { get; }

    public IDictionary<string, string> Corge { get; }

    private Foo(string bar, IDictionary<string, string> corge)
    {
        Bar = bar;
        Corge = corge;
    }

    public class Builder
    {
        private string _bar;
        private readonly IDictionary<string, string> _corge = new Dictionary<string, string>();

        public Builder Bar(string bar)
        {
            _bar = bar;
            return this;
        }

        public Builder AddCorge(string key, string value)
        {
            _corge.Add(key, value);
            return this;
        }

        public Foo Build()
        {
            return new Foo(_bar, _corge);
        }
    }
}

Usage :

Foo foo = new Foo.Builder().Bar("bar").AddCorge("key", "value").Build();

The action delegate doesn't quite fit in my solution so I just removed it.

You can add the public empty constructor if you want to. I personally wouldn't  like to have static methods to create an immutable object, I prefer everything to be instance based.

One builder to rule them all..

One thing that I really don't like about your code is this

var fooBuilder = Foo.Builder.Create();
fooBuilder.Bar("baz");
var foo = fooBuilder.ToFoo();
fooBuilder.Bar("QUUX"); // bam!

You can't use the same builder to create multiple objects and it's not obvious that it will crash unless you check how the Builder class is implemented.

My solution fixes that

var fooBuilder = new Foo.Builder();
fooBuilder.Bar("baz");
var foo1 = fooBuilder.Build();
fooBuilder.Bar("QUUX"); // no bam!
var foo2 = fooBuilder.Build();