Does this image-processing library break the Single Responsibility Principle? I'm writing a small library to do some image-processing on GPU for WinRT, however, I'm not sure if such design breaks SRP.

One class loads and saves image, renders and maintains filters - isn't it too much?

Code

public class FilteredImageProvider : IDisposable
{
    public event Action ImageSourceChangedEvent;
    GPUImage currentImage;
    SharpDXRenderer renderer;
    FilterContainer filterContainer;
    ImageGPULoaderFactory loaderFactory;

    public FilteredImageProvider()
    {
        this.renderer = new SharpDXRenderer();
        this.filterContainer = new FilterContainer();
        this.loaderFactory = new ImageGPULoaderFactory();
    }

    public ImageSource Image
    {
        get { return renderer.GetRendererSource(); }
    }

    public async Task LoadFromFile(IStorageFile sourceFile)
    {
        var imageLoader = loaderFactory.GetImageLoader(sourceFile);

        if (currentImage != null)
            currentImage.Dispose();

        currentImage = await imageLoader.LoadImage();
        renderer.Load(currentImage);
        ImageSourceChangedEvent();
    }

    public async Task SaveToFile(IStorageFile destinyFile)
    {
        var imageSaver = loaderFactory.GetImageLoader(destinyFile);
        var deviceManager = renderer.GetDeviceManager();
        await imageSaver.SaveImage(currentImage, deviceManager);
    }

    public void Render()
    {
        var filterCompilator = new FilterCompilator();
        var renderedImage = currentImage.GetImageAsEffect();
        var compiledEffect = filterCompilator.CompileFiltersToEffect(renderedImage, filterContainer);

        renderer.Render(compiledEffect);
    }

    public void AddFilter(IFilter filter)
    {
        filterContainer.AddFilter(filter);
    }

    public void RemoveFilter(IFilter filter)
    {
        filterContainer.RemoveFilter(filter);
    }

    public void Undo()
    {
        filterContainer.Undo();
    }

    public void Redo()
    {
        filterContainer.Redo();
    }

    public void Dispose()
    {
        if (currentImage != null)
            currentImage.Dispose();
        renderer.Dispose();
    }
}

Private Fields

I prefer to prefix my private fields with an underscore, and as readonly as possible. If you don't like the underscore, fine - but then be consistent with this, and don't use it only in your constructor - use this whenever you're referring to a private field. It greatly helps telling private fields from parameters.

Personally I don't like sprinkling this everywhere in my code, so I prefer the underscore; I also like my access modifiers explicit:

public class FilteredImageProvider : IDisposable
{
    private readonly SharpDXRenderer _renderer;
    private readonly FilterContainer _filterContainer;
    private readonly ImageGPULoaderFactory _loaderFactory;

    private GPUImage _currentImage;

    //...

Constructor

The readonly fields are still assigned in the constructor:

public FilteredImageProvider()
{
    _renderer = new SharpDXRenderer();
    _filterContainer = new FilterContainer();
    _loaderFactory = new ImageGPULoaderFactory();
}

These three should be injected as constructor arguments - creating these instances within the constructor breaks SRP, at least if you're applying DI principles. SRP is only the S of SOLID. By taking these instances in as constructor arguments, you greatly facilitate the rest of SOLID.

You're mostly exposing cherry-picked encapsulated methods, that works for me. However I find that you shouldn't be newing up the FilterCompilator like you're doing. It should be constructor-injected, and perhaps made a dependency of FilterContainer, or of another type that would deal with everything that can be done with a Filter.

Note: If _renderer is injected in the constructor, then you shouldn't call _renderer.Dispose() since you've put that burden onto the caller (ideally your favorite IoC container).


Substitute IDs to word tokens I writing a function in Python to substitute word tokens in a sentence with IDs.

The sentence is a list of tokens (list_of_tokens).
The IDs are provided in a dictionary mapping tokens with an IDs (tokens2IDs_dict).

However, the functions I've written is not very efficient because of the for-loop. I am wondering if there is any more pythonic and efficient way to perform this mapping.

def tokens2IDs(list_of_tokens, tokens2IDs_dict):
    new_sentence = []
    for token in list_of_tokens:
        if len(token) == 0 or token == " ":
               continue
        new_sentence.append(tokens2IDs_dict.get(token, tokens2IDs_dict.get('UNK')))
    return new_sentence

There is nothing wrong in general with for loops. If you have to iterate over some elements there is no magic that can avoid this. So why is your function "not very efficient"? And why is this because of the for-loop?

However you asked for a pythonic solution. In Python explicit for loops are avoided by using list comprehension. Internally Python again loops over all elements, however slightly more efficient. Better readability is the other big gain.

So let's measure (I did some renaming for readability). I compare

your original explicit for loop
a 1:1 translation to a list comprehension
further improved by extraction of the default lookup
also apply Jean-François Fabres strip()

.

import timeit

def encode(tokens, dictionary):
    ids = []
    for token in tokens:
        if len(token) == 0 or token == " ":
            continue
        ids.append(dictionary.get(token, dictionary.get('UNK')))
    return ids

def encode1(tokens, dictionary):
    ids = [dictionary.get(t, dictionary.get("UNK")) for t in tokens if t and t != " "]
    return ids

def encode2(tokens, dictionary):
    unk = dictionary.get("UNK")
    ids = [dictionary.get(t, unk) for t in tokens if t and t != " "]
    return ids

def encode3(tokens, dictionary):
    unk = dictionary.get("UNK")
    ids = [dictionary.get(t, unk) for t in tokens if t.strip()]
    return ids

def test(tokens, dictionary, result, encoders):
    for encode in encoders:
        assert result == encode(tokens, dictionary)

def time(tokens, dictionary, encoders):
    print(tokens)
    print(encoders[0](tokens, dictionary))
    for encode in encoders:
        print(timeit.timeit(lambda: encode(tokens, dictionary)))

encoders = [encode, encode1, encode2, encode3]

tokens = ["asdf", "qwer", "yxcv", " ", ""]
dictionary = {"UNK": 0, "asdf": 1, "qwer": 2}
test(tokens, dictionary, [1, 2, 0], encoders)

string = "asdwerasfeqtdfgrtzdfhetzufgjhtzui"
tokens = list(string)
dictionary = {k: v for v, k in enumerate(["UNK"] + list(dict.fromkeys(tokens)))}
time(tokens, dictionary, encoders)

tokens = [" "] * len(string)
time(tokens, dictionary, encoders)

tokens = [""] * len(string)
time(tokens, dictionary, encoders)

results on my machine

['a', 's', 'd', 'w', 'e', 'r', 'a', 's', 'f', 'e', 'q', 't', 'd', 'f', 'g', 'r', 't', 'z', 'd', 'f', 'h', 'e', 't', 'z', 'u', 'f', 'g', 'j', 'h', 't', 'z', 'u', 'i']
[7, 9, 6, 11, 10, 12, 7, 9, 8, 10, 5, 14, 6, 8, 15, 12, 14, 1, 6, 8, 2, 10, 14, 1, 3, 8, 15, 4, 2, 14, 1, 3, 13]
7.497583819553256
5.152629452757537
3.7371353656053543
4.100516518577933
[' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ']
[]
1.744785476475954
1.0442649777978659
1.1422674069181085
1.721993056125939
['', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '']
[]
1.2569031789898872
0.6375910267233849
0.7521892953664064
1.699515514075756

which shows:

list comprehension is indeed significantly faster than the explicit for-loop
avoiding the evaluation of the default also has significant impact
strip() is a little slower than your explicit test (while more generic)
for the explicit tests the order of evaluation is important

Altogether this is doubling the speed, but there is no magic to find magnitudes.


Processing lexical analyses of sentences using the Perl split function I have two kinds of lexical analyses of sentences that I need to process.  One type of data comes in a "tagged" format, and the other comes in a "parsed" format.

Tagged

The input (@subsentences) looks like:

5.4_CD Passive_NNP Processes_NNP of_IN Membrane_NNP Transport_NNP 85_CD We_PRP have_VBP examined_VBN membrane_NN structure_NN and_CC how_WRB it_PRP is_VBZ used_VBN to_TO perform_VB one_CD membrane_NN function_NN :_: the_DT binding_JJ of_IN one_CD cell_NN to_TO another_DT ._.

Desired output

5.4 Passive Processes of Membrane Transport 85 We have examined membrane stru....

My code

@finalsentence = split(/_\S+/,$subsentences[$j]);

Parsed

   Parsing [sent. 1 len. 31]:
        nsubj(85-7, Processes-3)
        nn(Transport-6, Membrane-5)
        prep_of(Processes-3, Transport-6)
        nsubj(examined-10, We-8)
        nsubjpass(used-17, it-15)
        xsubj(perform-19, it-15)
        conj_and(examined-10, used-17)
        xcomp(used-17, perform-19)
        dobj(perform-19, function-22)
        prep_of(binding-25, cell-28) <- refer to this for examples below

Desired output (for the last line)

the sent. number (ie. sent. 1 )
the grammar function (ie. prep_of )
the first dependency word (ie. binding )
the second dependency word (ie. cell )

My code

Here is how I do it, but when I check for word boundaries (\b), sometimes they're not defined and on top of that, it's pretty crude:

For the sent. number:

@parsesentcounter = split (/.*sent\.\s/, $typeddependencies[$i]);
@parsesentcounter = split (/\s/, $typeddependencies[$i]);

This (crude method) leaves the sent. number (sent. 1) at $parsesentcounter[2]

For the grammar function:

@grammarfunction = split(/\(\S+\s\S+\s/,$typeddependencies[$i]);

This leaves the grammar function(prep_of) at $grammarfunction[0]

For the dependency words, I do it in a few steps (I think I get lost a bit here):

@dependencywords = split (/,\s+/,$typeddependencies[$i]); ## Take out all commas, there was also a space associated
@dependencywords = split (/-\S+\s+/,$typeddependencies[$i]); ## Take out all -digits and space

This leaves the second dependency word(cell) at $dependencywords[1].

Then for first dependency word:

@firstdependencyword = split(/.*subj\w*.|.*obj\w*.|.*prep\w*\(|.*xcomp\w*\(|.*agent\(|purpcl\(|.*conj_and\(/,$dependencywords[0]);

This leaves the first dependency word (binding) at $firstdependencyword[1]

Match and substitute are far better suited for this problem than split().
To delete all of the _XX things, use a substitute global s/(_\S+)//g;.
So generating your desired output is actually easy.

To see demo of this and how to parse your report, see the code below. Note that if the left hand side contains variables in a list context like:($gramFunc, $dep1, $dep2) then they will be assigned $1,$2,$3 from the match respectively. I check if $dep2 is defined to mean that the match succeeded because if that variable is defined, then the variables $gramFunc and $dep1 are too!

This line $sentNum = $1 if /\[sent\.\s*(\d+)/; has the same meaning as:

if (/\[sent\.\s*(\d+)/)
{
   $sentNum = $1;
}

I cannot assign to $sentNum directly in this case because I want to know if the match worked or not. $1 is undefined if the match fails and the scalar value of /\[sent\.\s*(\d+)/ will be zero. I presume that more such "[sent" lines will appear in the report that you are trying to parse. This statement just updates as necessary the current sent # for the report.

Code:

#!/usr/bin/perl -w
use strict;

my $str = '5.4_CD Passive_NNP Processes_NNP of_IN Membrane_NNP Transport_NNP 85_CD We_PRP have_VBP examined_VBN membrane_NN structure_NN and_CC how_WRB it_PRP is_VBZ used_VBN to_TO perform_VB one_CD membrane_NN function_NN :_: the_DT binding_JJ of_IN one_CD cell_NN to_TO another_DT ._.';

$str =~ s/(_\S+)//g;  # deletes all of the "_XX" tokens
print $str, "\n";

my $sentNum = 0;
while (<DATA>)
{
   $sentNum = $1 if /\[sent\.\s*(\d+)/;

   my ($gramFunc, $dep1, $dep2) = $_ =~ /\s+(\w+)\((\w+).*?(\w+)-/;
   if (defined $dep2)
   {
      printf "sent num=$sentNum %-10s %-15s %-15s\n", $gramFunc, $dep1, $dep2;
   }
}

=program output
5.4 Passive Processes of Membrane Transport 85 We have examined membrane structure and how it is used to perform one membrane function : the binding of one cell to another .
sent num=1 nsubj      85              Processes
sent num=1 nn         Transport       Membrane
sent num=1 prep_of    Processes       Transport
sent num=1 nsubj      examined        We
sent num=1 nsubjpass  used            it
sent num=1 xsubj      perform         it
sent num=1 conj_and   examined        used
sent num=1 xcomp      used            perform
sent num=1 dobj       perform         function
sent num=1 prep_of    binding         cell
=cut

__DATA__
Parsing [sent. 1 len. 31]:
        nsubj(85-7, Processes-3)
        nn(Transport-6, Membrane-5)
        prep_of(Processes-3, Transport-6)
        nsubj(examined-10, We-8)
        nsubjpass(used-17, it-15)
        xsubj(perform-19, it-15)
        conj_and(examined-10, used-17)
        xcomp(used-17, perform-19)
        dobj(perform-19, function-22)
        prep_of(binding-25, cell-28) <- refer to this for examples below


Finding the objects with the cheapest seats i have the following json object

    const file = [
  {
    "seatNumber": "1A",
    "price": "£19.99",
    "available": true,
    "disabilityAccessible": true
  },
  {
    "seatNumber": "2A",
    "price": "£19.99",
    "available": false,
    "disabilityAccessible": false
  },
  {
    "seatNumber": "3A",
    "price": "£19.99",
    "available": false,
    "disabilityAccessible": false
  },
  {
    "seatNumber": "4A",
    "price": "£19.99",
    "available": true,
    "disabilityAccessible": false
  },
  {
    "seatNumber": "5A",
    "price": "£19.99",
    "available": false,
    "disabilityAccessible": false
  },
  {
    "seatNumber": "1B",
    "price": "£12.99",
    "available": true,
    "disabilityAccessible": true
  },
  {
    "seatNumber": "2B",
    "price": "£12.99",
    "available": false,
    "disabilityAccessible": false
  },
  {
    "seatNumber": "3B",
    "price": "£12.99",
    "available": false,
    "disabilityAccessible": false
  },
  {
    "seatNumber": "4B",
    "price": "£12.99",
    "available": false,
    "disabilityAccessible": false
  },
  {
    "seatNumber": "5B",
    "price": "£12.99",
    "available": true,
    "disabilityAccessible": false
  },
  {
    "seatNumber": "1C",
    "price": "£12.99",
    "available": true,
    "disabilityAccessible": true
  },
  {
    "seatNumber": "2C",
    "price": "£12.99",
    "available": true,
    "disabilityAccessible": false
  },
  {
    "seatNumber": "3C",
    "price": "£12.99",
    "available": true,
    "disabilityAccessible": false
  },
  {
    "seatNumber": "4C",
    "price": "£12.99",
    "available": true,
    "disabilityAccessible": false
  },
  {
    "seatNumber": "5C",
    "price": "£12.99",
    "available": true,
    "disabilityAccessible": false
  },
  {
    "seatNumber": "1D",
    "price": "£12.99",
    "available": true,
    "disabilityAccessible": true
  },
  {
    "seatNumber": "2D",
    "price": "£12.99",
    "available": false,
    "disabilityAccessible": false
  },
  {
    "seatNumber": "3D",
    "price": "£12.99",
    "available": true,
    "disabilityAccessible": false
  },
  {
    "seatNumber": "4D",
    "price": "£12.99",
    "available": true,
    "disabilityAccessible": false
  },
  {
    "seatNumber": "5D",
    "price": "£12.99",
    "available": true,
    "disabilityAccessible": false
  },
  {
    "seatNumber": "1E",
    "price": "£8.99",
    "available": true,
    "disabilityAccessible": true
  },
  {
    "seatNumber": "2E",
    "price": "£8.99",
    "available": true,
    "disabilityAccessible": false
  },
  {
    "seatNumber": "3E",
    "price": "£8.99",
    "available": false,
    "disabilityAccessible": false
  },
  {
    "seatNumber": "4E",
    "price": "£8.99",
    "available": true,
    "disabilityAccessible": false
  },
  {
    "seatNumber": "5E",
    "price": "£8.99",
    "available": false,
    "disabilityAccessible": false
  }
]

I am trying to display seatNumber of the cheapest objects. so far i have the following which works but would like to know if there is a better way following big o notation in es6 format

const seats = file.filter( seat => seat.price.replace(/[^0-9.-]+/g,"") == Math.min(...file.map(function ( seat ) { return Number(seat.price.replace(/[^0-9.-]+/g,"")) }) ) ).map(seat => seat.seatNumber);
        console.log(seats)

output is

[ '1E', '2E', '3E', '4E', '5E' ]

Following the little big O.
You ask

"...if there is a better way following big o notation..." (?)

Big O notation is a formalized mathematical convention used to express how a function (mathematical function) behaves as it approaches infinity.
It is used in computer science to classify an algorithms complexity in regard to a definable input metric, usually the size of the input array when dealing with arrays.
You can not "follow" big O notation as it provides no insight into how to reduce an algorithms complexity apart from a comparison.
Find the big O
To classify your function using Big O, first we need to make it at least readable, and convert it to a function. See snippet.
Now count the number of times the function loops over each item in the input array data. Experience lets you do this in your head, but to demonstrate we modify the function to count every time you handle an item in the input array.
Because we need to fit a curve we need at least 3 different input sizes which I do with some random data.

function bestSeats(data) {
     var c = 0; // the counter
     const seats = data.filter( seat =>
         (c++,  // count the filter iterations
         seat.price.replace(/[^0-9.-]+/g, "") == Math.min(
                 ...data.map(function ( seat ) {
                     c += 2;  // this counts as two
                              // once each to map to the arguments of Math.min
                              // once each to find the min
                     return Number(seat.price.replace(/[^0-9.-]+/g, ""))
                 }
             )
         ))).map(seat => (
             c++,  // count each result
             seat.seat
         ));
     return "O(n) = O(" + data.length + ") = " + c;
}

function randomData(size) {
    const data = [];
    while (size--) { data.push({seat:size, price: "$"+(Math.random() * 100 | 0)})}
    return data;
}
console.log("Eval complexity A:" + bestSeats(randomData(10)));
console.log("Eval complexity B:" + bestSeats(randomData(100)));
console.log("Eval complexity C:" + bestSeats(randomData(500)));

The 3 points we can use to find the curve that best fits
 O(10) ~= 211
 O(100) ~= 20,102
 O(500) ~= 500,005

Experience tells me it's a polynomial of some (not too high) order. Using a graphing calculator I found a reasonable fits at 2.15 making your functions big O complexity
\$O(n^{2.15})\$
Which is insanely inefficient. OMDG!!!!
So keeping

"...in es6 format" (?)

in mind a cleaner more readable, less CO2 producing sea level rising approach is to do a two pass scan of the seats. The first pass finds the min price, the second extracts the min price seat numbers.
This example uses for of loops rather than the Array methods like filter and map, because these array methods have a nasty way of blinding people to the insane level of  complexity of what they do, and for of is a little quicker and often allows better optimizations than the array methods, so it's win win for for of
Examples in \$O(n)\$ linear time.
 function bestSeats(seats) {
     var min = Infinity, minVal;
     const result = [];
     for (const seat of seats) {
         const price = Number(seat.price.slice(1));
         if (price < min) {
             min = price;
             minVal = seat.price;
         }
     }
     for (const seat of seats) {
         if (seat.price === minVal) { result.push(seat.seatNumber) }
     }
     return result;
 }

And if you must use the array methods.
 function bestSeats(seats) {
     const price2Num = seat => Number(seat.price.slice(1));
     const min = (min, seat) => price2Num(seat) < min ? price2Num(seat) : min;
     const minPrice = "$" + seats.reduce(min, Infinity);
     return seats.filter(seat => seat.price === minPrice).map(seat => seat.seatNumber);
 }


Implement an algorithm for a room cleaning robot I wanted to ask this question to my interview candidates, hence wanted to first give it a shot to see if this is doable in 45 mins. Here's the code that I was able to come up with in 45 minutes. Can folks please let me know if there are any logic flaws in this code?
Constraints:

The robot does not have any data of the room to start with.
The robot has to traverse the entire area of the room.
The room can have obstacles.
The robot can go front or right or left.

public interface IRobot
{
    public void TurnLeft();
    public void TurnRight();
    public bool Advance(); // This returns true if the robot is able to advance/no obstacle in that step
    public void Clean();
}

public class Node
{
    public bool Visited { get; set; }
    public bool Cleaned { get; set; }
    public Node Front { get; set; }
    public Node Left { get; set; }
    public Node Right { get; set; }
}

public class RobotManager
{
    private IRobot _robot;
    private Node _rootNode;
    private Node _currentNode;

    public RobotManager(IRobot robot)
    {
        Validator.ThrowArgumentNullIfApplicable(robot);
        this._robot = robot;
        this._rootNode = new Node();
    }

    public void Clean()
    {
        Clean(_rootNode);
    }

    private void Clean(Node currentNode)
    {
        if(currentNode.Cleaned)
        {
            return;
        }

        this._robot.Clean();
        currentNode.Cleaned = true;
        this._currentNode = currentNode;

        if(this._robot.Advance())
        {
            currentNode.Front = new Node();
            Clean(currentNode.Front);
        }

        this._robot.TurnLeft();
        if(this._robot.Advance())
        {
            currentNode.Left = new Node();
            Clean(currentNode.Left);
            this._robot.TurnRight();
        }

        this._robot.TurnRight();
        if(this._robot.Advance())
        {
            currentNode.Right = new Node();
            Clean(currentNode.Right);
            this._robot.TurnLeft();
        }

        Console.WriteLine("Finished Cleaning!");
        Reset();
    }

    // This will return the robot back to the starting point
    public void Reset()
    {
        Traverse(this._currentNode);
        this._rootNode = new Node();
    }

    private bool Traverse(Node currentNode)
    {
        if(currentNode == null || currentNode.Visited)
        {
            return false;
        }

        currentNode.Visited = true;
        if(currentNode == _rootNode)
        {
            return true;
        }

        if(currentNode.Front != null)
        {
            this._robot.Advance();
            if(Traverse(currentNode.Front))
            {
                return true;
            }
        }

        if(currentNode.Left != null)
        {
            this._robot.TurnLeft();
            this._robot.Advance();
            if(Traverse(currentNode.Left))
            {
                return true;
            }

            this._robot.TurnRight();
        }

        if(currentNode.Right != null)
        {
            this._robot.TurnRight();
            this._robot.Advance();
            if(Traverse(currentNode.Right))
            {
                return true;
            }

            this._robot.TurnLeft();
        }
    }
}
```

TL;DR - I would not hire you
I am going to be fair here and state that I am judging this code more harshly because you are a not just any junior developer or applicant, but you are actually responsible for testing and technically evaluating potential hires. This implies a certain skill level, and it means that you should be innately able to display what the bar for the quality of the company's code is.
And to be quite frank, as an applicant, if I saw this as the proposed solution, I would genuinely back out of working for this company.
I spent more time reviewing this than you did writing it (if you kept to your own 45 minute limit), so I hope you appreciate that I'm not trying to be mean, but I am trying to be very direct about the many issues I've uncovered. The more I looked at your code, the more started realizing that it is deeply flawed, suffers from multiple reality 101 failures, and most of all it is painfully apparent that you never even tested it, not even once.
I wouldn't hire you (for any position above junior) for several reasons:

You are asking applicants to solve pathfinding, a notoriously difficult problem that has existed for decades and that to this day still has not been optimized (with reasonable performance), and you're giving them 45 minutes to do so. This is sheer insanity. Unless you are hiring for a game developer (or roomba developer) with significant prior experience, the posed challenge just doesn't fit the kind of applicant you're looking for.
Some of the issues I found are related to not grasping the elemental basics of the field you're trying to work with. That's general grounds for rejecting any applicant, let alone the interviewer who created the challenge to begin with. Interviewers who don't understand the subject matter they're interviewing about are not good interviewers.
You never even bothered to test your code. Some of these issues are blindingly obvious from the most barebones example, without even factoring in features like obstacles.
There is not a single situation in which your code works. It's not that you've forgotten to account for fringe cases, or specific features such as obstacle avoidance. Your code fails for a 2x2 empty room.
Furthermore, you also therefore didn't consider that applicants would like to be able to test and debug their code, which in my opinion raises red flags for both your roles as an interviewer and as a developer.

That being said, the code was formatted nicely.
I don't want to make you feel bad. We've all struggled with certain problems at one time or another. There are plenty of projects I've started that have been deeply flawed.
But I would strongly suggest you re-evaluate if you are the right person to evaluate these applicants for technical interviews. In the interest of the company, I do not think that you are the right person for the job without some significant improvements being made.
Test data and visualization tool
This is more a comment on the question rather than the solution, but behavioral logic of this kind is notorious for needing shotgun debugging. It's exceedingly hard if not impossible to see if this algorithm works as intended based on reading the code and not seeing it in action.
This applies to me needing to review your code now, you needing to check your applicants' code, as well as any applicant who'd like to debug their code while developing to find any obvious flaws.
I would strongly advise you to create a default test fixture where you have some predefined room setups to test the robot with, and a visual tool that shows you the room and the robot's progress step by step.
I will try to review your algorithm to some degree, but keep in mind that I did not write an entire test scenario for this. This is just based on what I can interpret by reading the code, and some experience I have with pathfinding logic.
Code structure
Based on the question as presented, I find it counterintuitive that you only add a robot interface without implementation, and then develop the "algorithm for a robot" in another class altogether. I'm not against separating the movement logic from the cleaning logic, but the names leave something to be desired. IRobot (and the presumed Robot class) don't indicate movement, and RobotManager doesn't indicate cleaning.
Especially for an interview question, I find this a really important sticking point. It shows you how well this developer is independently going to be able to keep your code clean.
Stack overflow
Your code relies on recursion, and the recursive stack size is finite. Therefore, you've implemented a natural cap on how big a room can be, and the way it behaves when it reaches that cap (StackOverflowException) is ugly.
I would strongly advice using some kind of iteration instead of recursion here. A for(each) is not going to be appropriate since you don't know the size of the room in advance, but a while would be appropriate here.
DRY and method naming
I don't like the Clean method name, because it is also the method which handles traversal, even though you also have a separate Traverse method. Something has gone awry here. You've duplicated the movement logic in both methods, and implemented them slightly differently.
That's not promoting reusability the way it should.
Your algorithm in general
There are frankly a lot of glaring issues with the logic itself. I can't delve into all of them in full depth. If you had had a visualization tool and test data, some of these would've been easy to spot. I'll elaborate on a few core issues, but quite frankly there's little logic to salvage here when you've noticed all the bugs.
It's important to observe that your Clean method is the only method being called to handle both the cleaning and the room traversal. Traverse is only used after the cleaning has been completed.
private void Clean(Node currentNode)
{
    if(currentNode.Cleaned)
    {
        return;
    }

If you back out of every cleaning operation when the current node has already been cleaned, that means that you are not allowing the robot to cross a cleaned node. That is a significant flaw in your logic. The easiest test case is having a room whose obstacles are laid out like a Bomberman level:

Try traversing all the green tiles in a continuous line without crossing your path.
You might be able to snake around the level and then get into the first dead-end street, and your robot will never turn itself around (it only attempts straight, left and right).
But even if it did turn around, when it tries to exit that dead-end street, it has to drive over a path it already drove over. And what does you code do then? Refuse to continue. It tries to backtrack back into the dead-end street.
Since it is no longer adjacent to any uncleaned and accessible tile, and therefore it will report that the cleaning has been completed.
You might think that my example is a fringe case for a room that's designed to trip up your system. But your logic fails for a completely empty room as well. Your logic is as follows:

Go straight
If you can't, go left
If you can't, go right
In any of the above cases, repeat the logic
In none of the above cases, back out and continue that recursion level's logic.

Placing the robot in a room, it's going to go straight until it can't, turn left once, go straight until it can't again, etc,... Think about what that means in visual terms:

Edit: I just noticed I forgot to number the first straight. Oops! Let's just say I totally intended to zero-index them...
After the straight that is (incorrectly) labeled 6 in the image, since it will never cross a tile that has already been cleaned, your robot has boxed itself in and will never clean the left half of the room (except the outer edge which it cleaned in the beginning).
Node-based traversal
This system just doesn't make any sense. The robot's interface is fine. You are clearly understanding that the robot only knows its own orientation, and therefore must be issued commands relative to its own position (without knowing its location in the room).
But then your node-based system throws all of that out the window and tries to dynamically build a room-scale grid, and issues commands based on the grid (which has the room as a reference point) to the robot (which has itself as a reference point).
You never check the robot's orientation. That's a massive red flag. It makes no sense to actively need to rotate a robot, and then never bother with using that rotation in your logic. Your logic decides to "go left", but since it doesn't know the orientation of the robot, how can it even figure out what "left" means to the robot?
I can't elaborate on the full ramification of this, but it essentially means that you cannot just assume that "node left" == "robot left", since the robot's orientation alters how the robot is oriented in the room. You've mixed two completely different coordinate systems and have not made sure they are compatible with one another.
Node-based traversal 2 - the big one
In my opinion, this is the biggest issue in your algorithm. This is something you should have spotted without even running the code.
Every movement moves onto a new Node(). You are not tracking any meaningful node state.
Let's imagine a 4 tile room, and the robot starts in the bottom right corner.

For brevity, I'm only focusing on the successful movement actions. The first 4 steps are what you intended them to be:

Go straight (new location: top-right)
Go left (new location: top-left)
Go left (new location: bottom-left)
Go left (new location: bottom-right)

I want to focus on what happens in step 3 when you realize that moving to the left was possible:
this._robot.TurnLeft();
if(this._robot.Advance())
{
    currentNode.Left = new Node();
    Clean(currentNode.Left);

The next node is a new Node(). But you actually already knew this location (because you have been there before). But your code makes no effort to check if this is a known location. It always blindly makes a new node.
Therefore, your entire clean-node-tracking logic is defeated:
    if(currentNode.Cleaned)
    {
        return;
    }

Since currentNode is always a newly created node, which by definition could not have been cleaned yet.
You've fallen into the age-old trap of thinking that compilers read intent, speak English, and figure out your system for you. Putting it another way:
var topLeft = new Node();
var topRight = new Node();

topLeft.Right = topRight;

var whatIsThisValue = topRight.Left;

Tell me what will be in whatIsThisValue. Is it your topRight object?
Nope. It's null. Just because left is the opposite of right doesn't mean that your code will automatically understand that "if B is right of A, then A must be left of B". That's just not how it works.
This issue renders your entire floor node state tracking logic, which the entire cleaning logic hinges on to know both how to avoid double work and to figure out when it's done, completely defeated.
Recursive backtracking
Next, we get to the recursive logic. It just doesn't make much sense. You lay out your pathfinding in recursive steps. Each recursive level will at some point attempt going forward, left and right. It doesn't just attempt one of these, it attempts all of them sequentially. When a movement attempt succeeds, it first drills down further, but the remaining movement attempts are still "queued" to happen when the recursive stack starts bubbling up again.
Following the above example, the robot will go straight for a few steps. Each step, that's a new recursive level. But you mustn't forget about the "queued" left/right commands from the first few steps. For now, you've drilled down into a new level, but what drills down will eventually bubble back up. Numbering the levels of recursion, you're looking at something like:

Straight 1 - success

Straight 2  - success

Straight 3  - success

Straight 4 <---- we are currently here

Left 3
Right 3

Left 2
Right 2

Left 1
Right 1

No matter how deep we dig into the recursive tree, Left/Right 1/2/3 will eventually be executed when we're done digging.
The problem with this is that backtracking on the recursive stack does not backtrack the robot's actual location. Therefore, the location in which Left 1 will be executed will be completely different depending on whether or not Straight 1 succeeded or not. This makes it impossible to know which location Left 1 is actually going to visit.
When you realize the chaos that this entails, you start understanding that you don't have an algorithm here, you have a brute force attack with just loads and loads of uncoordinated random movement.
If your robot had backtracked (i.e. moved backwards) whenever your Clean method returned (and bubbled up one recursive level), your logic would have been more correct. Still bugged, but the recursive approach would've at least made more sense.
The kind of recursive logic you're using here only works when each recursive level works in its own fixed location. But because you are dealing with a real robot in a room, whose location is altered by all of those recursive levels, it becomes impossible for each recursive level to ensure that its sequential operations (straight/left/right) are executed on the same fixed location regardless of what happened on lower recursive levels.


Changes all floats in a pandas DataFrame to string Below I created a function to format all the floats in a pandas DataFrame to a specific precision (6 d.p) and convert to string for output to a GUI (hence why I didn't just change the pandas display options). Is this the most efficient way to convert all floats in a pandas DataFrame to strings of a specified format?

import pandas as pd

df = pd.DataFrame({'x':[5.4525242,6.5254245,7.254542],'y':[5.4525242,6.5254245,7.254542]})

def _format_floats(df):

    df.loc[:,df.dtypes==float]=df.loc[:,df.dtypes==float].apply(lambda row: ["{:.6f}".format(num) for num in row])

_format_floats(df)

#Example output numbers changes to string with 6 decimals
df.iloc[0,0]
#'5.452524'

Since you're already calling .apply, I'd stick with that approach to iteration rather than mix that with a list comprehension.
It's generally better to avoid making data modifications in-place within a function unless explicitly asked to (via an argument, like inplace=False that you'll see in many Pandas methods) or if it's made clear by the functions name and/or docstring.
The logic is reasonably complex, so it might be clearer as a named function.
The leading _ in the function name is usually reserved for "private" functions, whereas this seems to be a general utility function. It's fine if you don't want external code to touch it, that's just not clear from this code snippet.

Here's one way you might re-write the function to follow these tips:

def format_floats(df):
    """Replaces all float columns with string columns formatted to 6 decimal places"""
    def format_column(col):
        if col.dtype != float:
            return col
        return col.apply("{:.6f}".format)

    return df.apply(format_column)

And a usage example:

In [1]: format_floats(pd.DataFrame([{'a': 1, 'b': 2.3}, {'a': 2, 'b': 3.0}]))
Out[1]:
   a         b
0  1  2.300000
1  2  3.000000


C++-safe way to use using inside headers I have this C++ 14 code in a project of mine.

Enhedron/Util/Enum.h:

#ifndef ENHEDRON_UTIL_ENUM_H_
#define ENHEDRON_UTIL_ENUM_H_

#include <stdexcept>
#include <type_traits>

namespace Enhedron { namespace Util { namespace Impl { namespace Enum {
    using std::runtime_error;
    using std::underlying_type_t;

    /**
     * Convert a value to an enum safely. The enum must have "LAST_ENUM_VALUE" as the last
     * enumerated value, and must be a simple enum starting at 0 and being densely populated. For example:
     *
     *      enum class MyEnum {
     *          VALUE_0,
     *          VALUE_1,
     *          LAST_ENUM_VALUE = VALUE_1
     *      };
     */
    template<typename Enum, typename Value>
    Enum toEnum(Value value) {
        if (value >= 0 && value <= static_cast<underlying_type_t<Enum>>(Enum::LAST_ENUM_VALUE)) {
            return static_cast<Enum>(value);
        }

        throw runtime_error("Value out of range for enum");
    }

    /**
     * Tag a type with an enumerated value to create a distinct type. Useful to differentiate values of the same type.
     * For example, to stop first and last names being interchangeable in the type system:
     *      enum class Name {
     *          FIRST,
     *          LAST
     *      };
     *
     *      using FirstName = TaggedValue<Name, Name::FIRST, string>;
     *      using LastName = TaggedValue<Name, Name::LAST, string>;
     */
    template<typename Enum, Enum tag, typename Value>
    class TaggedValue final {
        Value value;
    public:
        TaggedValue(Value value) : value(move(value)) { }

        Value& operator*() { return value; }
        const Value& operator*() const { return value; }

        Value* operator->() { return &value; }
        const Value* operator->() const { return &value; }
    };
}}}}

namespace Enhedron { namespace Util {
    using Impl::Enum::toEnum;
    using Impl::Enum::TaggedValue;
}}

#endif /* ENHEDRON_UTIL_ENUM_H_ */

Some specific things I'm unsure of:

Is there a better way to define TaggedValue so that you don't have to mention the enum type and the enum value you want to use? i.e. getting the template argument Enum inferred from the type of tag.
I'm using a namespace scheme where every header file has a namespace Enhedron::Module::Impl::FileName which is by convention private to that file. Then at the end of the file, it will export those names into the module namespace Enhedron::Module. My concern is that use of using inside the "private" namespace. Any thoughts on whether this would cause problems?

1. Auto Non-type template parameter

No, not possible, sorry, see this on SO.
I wish it could be done... but no, not in current standard.

2. Complex namespaces and using

It is not causing problems to users of the header, but the question is: who will maintain the code? You? Only you?

If the answer is yes, then it sounds too complex for me. It would be much easier to have one header (e.g. common.hpp) with the usings within one private namespace (e.g. firda::detail in my case) where any implementation will go inside this namespace (e.g firda::detail::something) automatically sharing all the usings specified in one file like a convention (common usage, something anybody needs to read before altering your code).

To be honest, I use such header importing selected features to my global namespace (firda). It is not adviced to use using namespace std and the same apply to my namespace (do not use using namespace firda, I am the only one allowed to do so in my own projects).

You can run into problems after adding usings to that common header, but it is your code and you should be able to manage it.

If the answer is no, than you need some common convention and from my perspective, it is much easier to simply forget about usings (except inside functions/methods). You can use shortcuts (namespace alias) if you really need to (e.g. namespace ns1 = some::complex::namespace::chain) placed usually on top of the header (to be easily found). Writing std::something or ns1::something is not so much typing and explicity is much better for multiple-coworker scenairo.

If you have the convention to place all such usings on top of the header, than it can be found by anybody sharing this convention. So, the idea is not that bad after all, but I would still prefer namespace aliases and std:: prefixes.


Acquiring a hash containing a key and a value I am new to Ruby and not familiar with a lot of Array features. I feel like this code can be written in a couple of fewer lines.

The idea is: I have a Definition class and a Field class. I abstracted them as Hashes to make my life easier although they should be classes.

Definition contains an Id and a Name
Field contains an Id and a Value

I want a resulting Hash that contains a Name key for each corresponding Value. However, if the name is repeated, then I need to include the Id in the Name.

Here's the code for generating mock objects:

# name and id
def mock_definitions
  d = []

  d << {
    :name => "custom def 1",
    :id   => 1
  }

  d << {
    :name => "custom def 2",
    :id   => 2
  }

  d << {
    :name => "custom def 2",
    :id   => 3
  }

  d
end

def mock_fields
  f = []

  f << {
    :id => 1,
    :value => "Value For Custom Definition 1"
  }

  f << {
    :id => 2,
    :value => "Value For Custom Definition 2"
  }

  f << {
    :id => 3,
    :value => "Value For Custom Definition 3"
  }

  f
end

def all_mocks
  return mock_definitions, mock_fields
end
############# END OF MOCK ###############

and the main logic preceded by helper functions:

def build_counter_cache(definitions = [])
  counter_cache = Hash.new

  definitions.each do |d|
    if counter_cache.has_key?(d[:name])
      counter_cache[d[:name]] +=1
    else
      counter_cache[d[:name]] = 1
    end
  end

  counter_cache
end

def build_fields_cache(fields = [])
  fields_cache = Hash.new

  fields.each do |f|
    fields_cache[f[:id]] = f[:value]
  end

  fields_cache
end

def build_search_to_replace_mapping(fields = [], definitions = [])
  counter_cache = build_counter_cache(definitions)
  custom_fields_cache = build_fields_cache(fields)

  mapping = Hash.new

  definitions.each do |d|
    search_text = ''
    replace_text = ''

    if counter_cache[d[:name]] == 1
      search_text = "{{#{d[:name]}}}"
    else
      search_text = "{{#{d[:name]}_#{d[:id]}}}"
    end

    if custom_fields_cache.has_key?(d[:id])
      replace_text = custom_fields_cache[d[:id]]
    end

    #puts "#{search_text} maps to #{replace_text}"
    mapping[search_text] = replace_text
  end

  mapping
end

### main logic begins here ###
# mocks
definitions, fields = all_mocks
mapping = build_search_to_replace_mapping(fields, definitions)

mapping.each do |key, value|
  puts "#{key} *** maps to *** #{value}"
end

This is just about your current code; existing answers have good suggestions for ways to do the entire thing differently.

As sschmeck notes, you can make good use of each_with_object here, even without radically changing your code. For instance, your helpers could be:

def build_counter_cache(definitions = [])
  counter_cache = Hash.new { 0 }
  definitions.each_with_object(counter_cache) do |d, cache|
    cache[d[:name]] += 1
  end
end

def build_fields_cache(fields = [])
  fields.each_with_object({}) do |f, cache|
    cache[f[:id]] = f[:value]
  end
end

The Hash.new { 0 } trick creates a hash that will return 0 for unknown/unset keys. So you don't need the if else in your block. (Note that the hash-with-fallback-value can bite you; it always returns the same object reference. In this case, it works, since we're using an immutable integer, but if you were to say Hash.new { [] }, you'd be getting the same array each time.)

You also have a pretty redundant method in the form of all_mocks. Really the same thing can be achieved with:

definitions, fields = mock_definitions, mock_fields

or, more readably, simply say:

definitions = mock_definitions
fields = mock_fields

All the all_mocks method does is add a layer of obfuscation to what's going on. You still need to know that it returns definitions and fields, in that order, if you want to use it. So if you have to know that, why bother calling the method in the first place?

As for alternative code, I'd do things much like sschmeck already suggested, though I'd still split the task into multiple methods rather than one big one.


Convert zip to gzip and upload to S3 bucket My code accesses an FTP server, downloads a .zip file, pushes the file contents as .gz to an AWS S3 bucket.

import boto3
import ftplib
import gzip
import io
import zipfile

def _move_to_s3(fname):
    host = 'some_host'
    user = 'some_user'
    passwd = 'some_password'

    tmp_dir = '/tmp/'
    tmp_fname = fname.split('.')[0] + '.gz'

    target_bucket = 'some_bucket'
    s3 = boto3.resource('s3')

    try:
        ftp = ftplib.FTP(host, user, passwd)

        with io.BytesIO() as data, gzip.open(tmp_dir + tmp_fname, 'wb') as gz:
            ftp.retrbinary('RETR ' + fname, data.write)
            gz.write(data.getvalue())

        s3.Object(target_bucket, tmp_fname).put(Body=open(tmp_dir + tmp_fname, 'rb'))
    except Exception as e:
        print e
    finally:
        ftp.quit()

if __name__ == '__main__':
    _move_to_s3('some_file.zip')

I welcome any comments but my main points of interest are:

Assuming the source zip contains only one text file, is it OK to
write to the gzip file from a stream containing the contents of the
downloaded zip file?
I think I should change the code to hold the gzip in memory, without generating temporary files.

Looks okay to me in general.  I'd prefer to pass in configuration
separately, parsed from a config file or command line arguments, but if
it's a one-off script it's probably okay this way.

The temporary files aren't deleted as far as I see; consider using the
tempfile module to
allocate and delete temporary files automatically.

Also, while the string to bytes conversion works as is, it's also
brittle when used with large files, e.g. larger then the amount of RAM
available, because it will store the downloaded file in memory.  I'd
suggest passing in gz.write directly instead:

with gzip.open(tmp_dir + tmp_fname, 'wb') as gz:
    ftp.retrbinary('RETR ' + fname, gz.write)

Unless I'm mistaken that should keep the bytes in order; if not you
could still pass in a function that converts only the downloaded chunk
to bytes and calls gz.write on the converted value.

Regarding your questions:

Since there's no extraction of data from the zip file, I don't see how
that matters.  If you were to extract something (a single file) from
the zip file, then yes, gzipping that will of course be okay.  Make
sure that the gzipped files are how you expect them (i.e. a single
compressed text file) and that you don't need the file name or other
attributes in the original zip archive.  At the moment you basically
upload some_file.zip.gz to S3, as in, two compressions nested.
Probably not what you want.
As argued above that's probably not advisable unless you know that the
data fits into memory.  If it does, sure, why not.

Some more remarks:

The zipfile import is unused, as mentioned above.
Prefer
os.path.join
to concatenating strings.
fname/passwd instead of filename/password seems pointless
truncation.
print should be used as a function to be forwards compatible,
i.e. print(e) instead.
The result of tmp_dir + tmp_fname should be reused instead of
writing the same expression twice.


Handling addition of two binary numbers This code is for handling addition of two binary numbers, which can be as long as 100,000 bits... and up to 500,000 queries are performed.

High-level pseudocode:

First input N: number of bits in binary number A and B.
Second input Q: number of queries to be performed.
Next N inputs: 0/1s ... bits for A
Next N inputs: 0/1s ... bits for B
Next Q inputs are one of the following:

set_a INDEX VALUE: as a result I set a[INDEX] = VALUE
set_b INDEX VALUE: as a result I set b[INDEX] = VALUE
get_c INDEX: as a result I print c[INDEX] where C = A+B (binary addition, like 100 + 101 = 1001)

#include <iostream>
#include <string>
#include <set>
#include <algorithm>
#include <utility>  // For pair
#include <iterator> // For iterator, bidirectional_iterator_tag, reverse_iterator
#include <climits>  // For CHAR_BIT, ULONG_MAX
using namespace std;

class VanEmdeBoasTree {};

int main(int argc, char * argv[])
{
  int n, q;
  char c;
  bool a[100005] = {0}, b[100005] = {0};
  VanEmdeBoasTree equals;
  cin>>n;
  cin>>q;

  for(int i=0;i<n;i++){
    cin>>c;
    if(c=='1')
      a[n-i-1] = true;
  }
  for(int i=0;i<n;i++){
    cin>>c;
    if(c=='1')
      b[n-i-1] = true;
    if(a[n-i-1] == b[n-i-1]){
      equals.insert(n-i-1);
    }
  }

  string query;
  int index, val, lastval;
  for(int i=0;i<q;i++){
    cin>>query;
    cin>>index;
    if(query[4] == 'a'){
      cin>>val;
      if(a[index] != val){
        a[index] = val;
        if(a[index] == b[index]){
          equals.insert(index);
        }else{
          equals.erase(index);
        }
      }
    }else if(query[4] == 'b'){
      cin>>val;
      if(b[index] != val){
        b[index] = val;
        if(a[index] == b[index]){
          equals.insert(index);
        }else{
          equals.erase(index);
        }
      }
    }else if(query[4] == 'c'){
      int final = 0;
      if(index == n){
        if(equals.size() > 0){
          VanEmdeBoasTree::const_iterator it = equals.predecessor(index+1);
          if(it!=equals.end() && a[*it] == 1){
            final = 1;
          }
        }
      }else{
        final = (a[index] + b[index])%2;
        if(equals.size() > 0){
          VanEmdeBoasTree::const_iterator it = equals.predecessor(index);
          int last = -1;
          if(it != equals.end())
            last = *it;
          if( last!=-1 && index!=0 && it!=equals.end() &&  a[last] == 1){
            final = 1-final;
          }
        }
      }
      cout<<final;
    }
  }

  return 0;
}

My solution is not fast enough as the Judge says "time out" after some test cases.

Profile and you will find your bottlenecks, you can do that with gprof under gcc/g++. You have to set the -g -pg on compile and then run your program like you would do normally. It will then produce a gmon.out file which you can open with gprof main gmon.out (assuming main is the name of your executable).

It will show you, in which functions your program spends most of the time.


Google Maps geocoding lookup function In an effort to keep all of the Google Maps Javascript API in it's own little world, I have created a googleMaps object that will contain all of the functions that directly make google maps webservice calls.  This object will be imported once on every page.  For now, it only has one function.

if (googleMaps === undefined || googleMaps === null)
{
  var googleMaps = {

    /**
    * Obtains the latitude and longitude of a location and then processes it with a callback function
    * @param {String} addressStr : comma separated string containing all pertinent address information
    * @param {Function} callback : handle to the function to call once the data lat/lng data has been retrieved successfully
    */
    getLatLngFromAddressString : function(addressStr, callback)
    {
      var geocoder = new google.maps.Geocoder();
      geocoder.geocode({'address': addressStr}, function(results, status) {
        if (status === google.maps.GeocoderStatus.OK)
        {
          var latitude = results[0].geometry.location.lat();
          var longitude = results[0].geometry.location.lng();
          callback({lat: latitude, lng: longitude});
        }
        else {
          // I have a custom error logging framework set up, but for simplicity i'll just console.log in code review snippet
          console.log("error");
        }
      });
    }
  }
}

In a separate file, I will call this function from verifyLocation(), shown below:

var showLocationCallback = function(loc)
{
  var latitude = loc.lat;
  var longitude = loc.lng;

  //other not important, totally unrelated processing stuff here
}

/**
 * makes a google maps geocode request for the provided address string
 */
function verifyLocation(addressStr)
{
  document.getElementById('verify-location').disabled = true;

  var location = googleMaps.getLatLngFromAddressString(addressStr, showLocationCallback);

}

I'm looking to get this bit of code reviewed.  I'm looking for any and all improvements, including:

making the code more efficient
rethinking the logic
completely restructuring the way I'm approaching this
best coding practices / naming conventions
-

You could, but don't have to, check your googleMaps variable to be falsy, instead of undefined or null.
To make this code more readable, you could seperate the function from your object. That way, if you have more functions, you have an overview of your object's implementation.

1.

old

if (googleMaps === undefined || googleMaps === null)

new

if(!googleMaps)

2.

old

var googleMaps = {
  getLatLngFromAddressString : function(addressStr, callback){
  //...
  }
}

new

var getLatLngFromAddressString = function(addressStr, callback){
//...
}

var googleMaps = {
  getLatLngFromAddressString : getLatLngFromAddressString
}


Is this correct factory method pattern? I know that there are many similar questions, but I don't understand most of those questions because I'm not sure if I know what a factory method pattern is.

So, after reading many examples over the web, I came up with the following simple classes.

Am I doing it correctly? If so...any improvements I can add?

abstract class Driveable
{
    abstract public function start();
    abstract public function stop();

}

class CoupeDriveable extends Driveable
{
    public function start()
    {
    }

    public function stop()
    {
    }
}

class MotorcycleDriveable extends Driveable
{
    public function start()
    {
    }

    public function stop()
    {
    }
}

class SedanDriveable extends Driveable
{
    public function start()
    {
    }

    public function stop()
    {
    }
}

class DriveableFactory
{
    static public function create($numberOfPeople){

        if( $numberOfPeople == 1 )
        {
            return new MotorcycleDriveable;
        }
        elseif( $numberOfPeople == 2 )
        {
            return new CoupleDriveable;
        }
        elseif( $numberOfPeople >= 3 && $numberOfPeople < 4)
        {
            return SedanDriveable;
        }
    }
}

class App
{
    static public function getDriveableMachine($numberOfPeople)
    {
        return DriveableFactory::create($numberOfPeople);
    }
}

$DriveableMachine = App::getDriveableMachine(2);
$DriveableMachine->start();

Update: according to palacsint and serghei's valueable advices, I've updated my code.

abstract class DriveableFactory
{
    static public function create($numberOfPeople);
}

class CarDriveableFactory extends DriveableFactory
{
    static public function create($numberOfPeople){

        $products = array
        (
            1=>"MotorcycleDriveable",
            2=>"CoupeDriveable",
            3=>"SedanDriveable",
            4=>"SedanDriveable"
        );

        if( isset( $products[$numberOfPeople] ) )
        {
            return new $products[$numberOfPeople];
        }
        else
        {
            throw new Exception("unable to find a suitable drivable car");
        }

    }
}

The essence of the Factory method Pattern is to "Define an interface
  for creating an object, but let the subclasses decide which class to
  instantiate. The Factory method lets a class defer instantiation to
  subclasses."

If you insist on the above GoF definition there are two issues.

First, you should create a DriveableFactory interface and rename your DriveableFactory to (for example) CarDriveableFactory.

abstract class DriveableFactory
{
    static public function create($numberOfPeople);
}

class CarDriveableFactory extends DriveableFactory
{
    static public function create($numberOfPeople) { ... }
}

But your code is fine, if you don't need (don't have a reason) the abstract DriveableFactory interface do NOT add it to the code.

The second issue is that the create method should not be static. If it's static subclasses cannot override the create method.

Finally, the App class looks unnecessary. So, I'd write something like this:

DriveableFactory factory = new CarDriveableFactory();
$DriveableMachine = factory->getDriveableMachine(2);
$DriveableMachine->start();

Some small improvements:

3.5 is an allowed value? And 3.1415? If not consider changing

else if( $numberOfPeople >= 3 && $numberOfPeople < 4)

to

else if($numberOfPeople == 3 || $numberOfPeople == 4)

In the last line of the create() method I would throw an IllegalArgumentException (or a similar one in PHP) with the message "invalid value: " . $numberOfPeople.


Flatten an array of arbitrarily nested arrays of integers Write a piece of functioning code that will flatten an array of arbitrarily nested arrays of integers into a flat array of integers. e.g. [[1,2,[3]],4] -> [1,2,3,4].

* tests use Jasmine can be found  http://jsbin.com/harofo/4/edit?html,js,output
* @input  array
* @output array
*/

function flatten(arr) {
    // this function is called in recursion mode
    let result = [];
    if (arr && arr.length > 0) {
        arr.forEach(function(value) {
            if(Array.isArray(value)) {
                result = result.concat(flatten(value));
            } else {
                result.push(value);
            }
        });
    }
    return result;

};

The check for arr.length > 0 is not needed: forEach() can be called
on an empty array (and does nothing in that case).

The check if (arr) protects against a call with an undefined argument,
but not against a call with a non-array. As an example, flatten(123)
would cause a JavaScript error. So you have to decide:

If the argument to flatten() is known to be an array then the
outer if statement is not needed at all.
If flatten() can be called with any argument than you need to check
for an array before calling forEach().

See also Flatten an array on how reduce() can be used instead of a loop.


JavaScript binary search I wrote an implementation of binary search in JavaScript earlier for kicks, but I noticed my version was significantly different than those found on Google. Here is an example of binary search I found on Google (copied here to combat link rot):

Array.prototype.binarySearch = function(find, comparator) {
    var low = 0, high = this.length - 1,
        i, comparison;

    while (low <= high) {
        i = Math.floor((low + high) / 2);
        comparison = comparator(this[i], find);
        if (comparison < 0) { low = i + 1; continue; };
        if (comparison > 0) { high = i - 1; continue; };
        return i;
    }
    return null;
};

Most of the other implementations on Google are quite similar. But in mine, I don't subtract one from the low/high and continue from there. Instead, I divide the array in half and continue dividing the appropriate side in half until I reach the proper value. Code:

function binary_search_iterative(arr, ele) {
    var beginning = 0, end = arr.length,
        target;
    while (true) {
        target = Math.floor((beginning + end) / 2);
        if ((target === end || target === beginning) && arr[target] !== ele) {
            return -1;
        }
        if (arr[target] > ele) {
            end = target;
        } else if (arr[target] < ele) {
            beginning = target;
        } else {
            return target;
        }
    }
}

I wrote this recursive-style as well, but in testing I found this iterative version was faster.

In my tests, I've found that the examples I've found on Google (though they did not include the one above, they were nearly identical) are faster on smaller arrays, but as the input size increases, mine overtakes those. Since binary searches are usually limited (?) to larger input sizes, I see this as an improvement.

Is my thinking correct? Could this be done in an even better way?

I also do not like this:

if ((target === end || target === beginning) && arr[target] !== ele) {
    return -1;
}

And I was curious if there is a good way to eliminate it or refactor the function where it's cleaner in general.

I would just like to hi-light a few points of difference in the two and how I think they are reflected in what you are seeing.

When making a comparison between arr[target] and ele, if they are not the same then you do not need to include target within the bounds of the next iteration (hence the +/- 1). This can reduce the number of comparisons though the effect is more significant on smaller datasets.
When following the above point, it becomes sufficient terminate as "not found" when beginning > end rather then check that you are looking at the beginning or end value. Also your approach may provide a false negative when looking for "B" in the following situation.

    target
    |
    v
... A B
    ^ ^
    | |
    | end
    beginning

In this situation, because target === beginning and arr[target] !== B, it returns -1.
The googled code uses parseInt, I'm not sure how this affects correctness but I suspect this has an adverse effect on the performance.

I think what you are seeing in the scalability is because the googled code does fewer comparisons but the parseInt makes the comparisons more expensive. The reduced number of comparisons is more significant for small data sets, but for larger data sets cost of the comparison becomes more significant.

I would suggest some experimentation/investigation to see if this is accurate.


Character and word count functionality I have this JS that has a lot of redundant functionality, and it works but looks unseemly.

I would love some assistance with making this more DRY (Don't Repeat Yourself):

// This provides character & word counter functionality

counter = function() {
    var body_value = $('#post_body').val();
    var title_value = $('#post_title').val();

    if (body_value.length == 0) {
        $('#wordCountBody').html(0);
        $('#totalCharsBody').html(0);
        return;
    }

    if (title_value.length == 0) {
        $('#wordCountTitle').html(0);
        return;
    }

    var regex = /\s+/gi;
    var wordCountBody = body_value.trim().replace(regex, ' ').split(' ').length;
    var totalCharsBody = body_value.length;
    var wordCountTitle = title_value.trim().replace(regex, ' ').split(' ').length;

    $('#wordCountBody').html(wordCountBody);
    $('#totalCharsBody').html(totalCharsBody);
    $('#wordCountTitle').html(wordCountTitle);
};

$(document).ready(function() {
    $('#count').click(counter);
    $('#post_body').change(counter);
    $('#post_body').keydown(counter);
    $('#post_body').keypress(counter);
    $('#post_body').keyup(counter);
    $('#post_body').blur(counter);
    $('#post_body').focus(counter);
    $('#post_title').change(counter);
    $('#post_title').keydown(counter);
    $('#post_title').keypress(counter);
    $('#post_title').keyup(counter);
    $('#post_title').blur(counter);
    $('#post_title').focus(counter);
});

Your code looks mostly fine—if you're just performing this specific functionality in one place, some repetition is okay—but there's one thing that could definitely be simplified. Your ready function could be reduced to simply this:

$(document).ready(function () {
    $('#count').click(counter);
    $('#post_body, #post_title').on('change keydown keypress keyup blur focus', counter);
});

If, however, you really wanted to reduce all repetition, you could generalize the counter function until you ended up with a fairly reusable component.

Your counter function currently performs two actions (word counting and character counting) on two separate elements (the body and the title). To make the code as DRY as possible, you should employ the single responsibility principle to break up the function into composable bits.

function countUpdater(element, transformations) {
    return function () {
        var value = element.val();
        transformations.forEach(function (transformation) {
            transformation.display.html(value.length === 0 ? 0 : transformation.counter(value));
        });
    }
}

function wordCounter(value) {
    return value.trim().replace(/\s+/g, ' ').split(' ').length;
}

function characterCounter(value) {
    return value.length;
}

Your event handler code would then become this:

$(document).ready(function () {
    var changeEvents = 'change keydown keypress keyup blur focus';

    var bodyUpdater = countUpdater($('#post_body'), [
        { display: $('#wordCountBody'), counter: wordCounter },
        { display: $('#totalCharsBody'), counter: characterCounter },
    ]);
    var titleUpdater = countUpdater($('#post_title'), [
        { display: $('#wordCountTitle'), counter: wordCounter },
    ]);

    $('#count').click(function () {
        bodyUpdater();
        titleUpdater();
    });
    $('#post_body').on(changeEvents, bodyUpdater);
    $('#post_title').on(changeEvents, titleUpdater);
});

Is that better? Worse? Well, it's certainly less clear, but it's also much more reusable. If you're going to use this type of functionality many times, it might be worth it, but otherwise, I might prefer the simpler, more explicit version.


How could I reduce repetition with properties/methods? I'm serializing instances of a lot of classes (in C# 3.0), and I've found myself writing this:

[DataContract]
public class ModelClass
{
    public static Guid GetID(ModelClass instance) { ... }

    public static T FromID<T>(Guid value) where T : ModelClass { }

    [DataMember] public Guid m_id { get; set; }
}

[DataContract]
public class ConcreteModel: ModelClass
{
    [DataMember]
    public string      m_name;

    // Stuff we don't serialize
    public Sector      m_sector;
    public Theater     m_theater;
    public Weather     m_weather;
    public TerrainType m_terrainType;

    // Link serialized ids to real objects through GetID & FromID
    [DataMember] private Guid m_sectorID
    {
        get { return GetID(m_sector); }
        set { m_sector = FromID<Sector>(value); }
    }

    [DataMember] private Guid m_theaterID
    {
        get { return GetID(m_theater); }
        set { m_theater = FromID<Theater>(value); }
    }

    [DataMember] private Guid m_weatherID
    {
        get { return GetID(m_weather); }
        set { m_weather = FromID<Weather>(value); }
    }

    [DataMember] private Guid m_terrainTypeID
    {
        get { return GetID(m_terrainType); }
        set { m_terrainType = FromID<TerrainType>(value); }
    }
}

ConcreteModel is just one of many (~100) classes that will have some objects that are going to be saved using IDs, but not the whole objects, so we'll have to write that kind of properties a lot of times.

Is there any way of avoiding this? In C++ I'd be using macros, but not in C#...

My main concern here is not writing those properties all over again like 500 times or more. Any idea on how could I avoid that?

As @Magus commented, this is as good as it gets.

...almost.

[DataContract]
public class ConcreteModel: ModelClass

[DataMember] public Guid m_id { get; set; }

[DataMember]
public string      m_name;

You're not consistent with how you're placing your DataMemberAttribute attributes; sometimes they're sitting on top of the decorated member, other times they're stuck in front, on the same line. I'd stick to consistently having them on top.

// Stuff we don't serialize
public Sector      m_sector;
public Theater     m_theater;
public Weather     m_weather;
public TerrainType m_terrainType;

Why are these fields public in the first place? You are breaking encapsulation!

[DataMember] private Guid m_sectorID
{
    get { return GetID(m_sector); }
    set { m_sector = FromID<Sector>(value); }
}

I'm not familiar with serialization using [DataMemberAttribute], but it's rather odd that you expose public fields and declare private serializable properties. Since the fields are exposed anyway, why not just do this?

private Sector m_sector;

[DataMember]
public Guid m_sectorID
{
    get { return GetID(m_sector); }
    set { m_sector = FromID<Sector>(value); }
}

The naming is, as @Magus commented already, breaking all C# naming conventions. Drop the m_ prefix, and use PascalCase for public members:

private Sector _sector; // or just "sector"

[DataMember]
public Guid SectorId
{
    get { return GetId(_sector); }
    set { _sector = FromId<Sector>(value); }
}

If ModelClass isn't intended to be instantiated and only serve as a base class, you should make it abstract - also "Class" at the end of a class name is rather useless. How about this?

public abstract class ModelBase

The public static members could made non-static, and be protected so as to only be exposed to the derived types.


Derived Class to keep track of "original strings" in std::regex and ease out their usage while coding We all know that std::regex class doesn't allow access to the original string which was used as the source expression. (In fact, GCC even keeps the original string as _M_original_str, but won't allow external access. Still, I have not used this member because it is implementation detail.)

So I derived a lightweight class to so that the std::regex will seem to behave like the string it was created from, well... not exactly, but kind of close. It lets the std::regex do its thing, but also keeps a record of the current string associated with the regex expression as its own member. And uses this member to allow new behaviors.

It allows direct retrieval of the original string. It also allows direct comparison with a std::string to check for equality, which in turn allows two regex's to be checked for equality.

But there's a catch. To check for equality, it uses the tracked string inside both objects to check for equality, which is a little flawed because two distinct strings can lead to same regex. So in that case it fails.

Also, since a constructor called with no arguments must create a regex object which matches nothing, I've accounted for that by keeping track of it in the derived class by maintaining a bool.

I want to know if it is good enough?
Have I tied up all loose ends while deriving from std::regex?
Can using it carelessly create unexpected errors?

So here it is:

#include <iostream>
#include <regex>
#include <string>

using namespace std;

class Regex: public regex
{
public:
    Regex ();
    Regex ( const string& str, flag_type flag = regex::ECMAScript );
    Regex ( const Regex& rhs );

    Regex& Assign ( const string& str, flag_type flag = regex::ECMAScript );
    Regex& Assign ( const Regex& rhs );

    string GetSourceStr () const { return m_str; }

    Regex& operator= ( const string& str ) { return this->Assign( str, flags() ); }
    Regex& operator= ( const Regex& rhs ) { return this->Assign( rhs ); }

    bool operator== ( const Regex& rhs ) const
      { return  m_str == rhs.m_str && m_match_nothing == rhs.m_match_nothing; }

    bool operator== ( const string& rhs_str ) const
      { return  m_str == rhs_str && !m_match_nothing; }

private:
    string m_str;
    bool m_match_nothing;
};

Regex::Regex ()
: regex(), m_str( "" ), m_match_nothing( true )
{}

Regex::Regex ( const string& str, flag_type flag )
: regex( str, flag ), m_str( str ), m_match_nothing( false )
{}

Regex::Regex ( const Regex& rhs )
: regex( rhs ), m_str( rhs.m_str ), m_match_nothing( rhs.m_match_nothing )
{}

Regex& Regex::Assign( const string& str, flag_type flag )
{
    m_str = str;
    m_match_nothing = false;
    regex::assign( str, flag );

    return *this;
}

Regex& Regex::Assign( const Regex& rhs )
{
    if ( this == &rhs ) return *this;

    m_str = rhs.m_str;
    m_match_nothing = rhs.m_match_nothing;
    regex::assign( rhs );

    return *this;
}

int main()
{
    string text;
    string blank_str;

    Regex pattern; // default regex object: matches nothing
    Regex pattern2(""); // a regex object which only matches a blank string

    cout << (regex_match(blank_str, pattern) ? "1 match" : "1 no_match") << "\n"
         << (regex_match(blank_str, pattern2) ? "2 match" : "2 no_match") << "\n"
         << (pattern==pattern2 ? "1==2" : "1!=2") << "\n\n";

    pattern = "a*c+";
    cout << (pattern=="a*c+" ? "yeah" : "nah") << "\n"
         << (pattern=="abc+" ? "yeah" : "nah") << "\n";

    return 0;
}

This looks like a bug:

Regex& Regex::Assign( const Regex& rhs )
{
    if ( this != &rhs ) return *this;  // <- ?

    m_str = rhs.m_str;
    m_match_nothing = rhs.m_match_nothing;
    regex::assign( rhs );

    return *this;
}

Presumably you want to abort the assignment early if:

if ( this == &rhs ) return *this;

At the moment, you only update the variables if the rhs is the same as the object receiving the call... which already has those variable values.


Naive demosaic for bayer images I have this legacy code and I don't really think this is easy to read/maintain it. Can you suggest a way to make this more readable while still saving the performance?

template<typename T>
    void static ConvertBayerToRgbImageDemosaic(T* BayerChannel, T* RgbChannel, int Width, int Height, ColorSpace ColorSpace)
    {
        //Translates index offset in Bayer image to channel offset in RGB image
        int offsets[4];
        std::vector<int> colorOffsets[4];
        //calculate offsets according to color space
        switch (ColorSpace)
        {
        case ColorSpace::BGGR:
            offsets[0] = 2;
            offsets[1] = 1;
            offsets[2] = 1;
            offsets[3] = 0;
            colorOffsets[0].push_back(0);
            colorOffsets[0].push_back(1);
            colorOffsets[0].push_back(Width);
            colorOffsets[0].push_back(Width + 1);
            colorOffsets[1].push_back(-1);
            colorOffsets[1].push_back(0);
            colorOffsets[2].push_back(0);
            colorOffsets[2].push_back(1);
            colorOffsets[3].push_back(-Width - 1);
            colorOffsets[3].push_back(-Width);
            colorOffsets[3].push_back(-1);
            colorOffsets[3].push_back(0);
            break;
        case ColorSpace::RGGB:
            offsets[0] = 0;
            offsets[1] = 1;
            offsets[2] = 1;
            offsets[3] = 2;
            colorOffsets[0].push_back(0);
            colorOffsets[0].push_back(1);
            colorOffsets[0].push_back(Width);
            colorOffsets[0].push_back(Width + 1);
            colorOffsets[1].push_back(-1);
            colorOffsets[1].push_back(0);
            colorOffsets[2].push_back(0);
            colorOffsets[2].push_back(1);
            colorOffsets[3].push_back(-Width - 1);
            colorOffsets[3].push_back(-Width);
            colorOffsets[3].push_back(-1);
            colorOffsets[3].push_back(0);
            break;
        case ColorSpace::GBRG:
            offsets[0] = 1;
            offsets[1] = 2;
            offsets[2] = 0;
            offsets[3] = 1;
            colorOffsets[0].push_back(0);
            colorOffsets[0].push_back(1);
            colorOffsets[1].push_back(-1);
            colorOffsets[1].push_back(0);
            colorOffsets[1].push_back(Width - 1);
            colorOffsets[1].push_back(Width);
            colorOffsets[2].push_back(-Width);
            colorOffsets[2].push_back(-Width + 1);
            colorOffsets[2].push_back(0);
            colorOffsets[2].push_back(1);
            colorOffsets[3].push_back(-1);
            colorOffsets[3].push_back(0);
            break;
        case ColorSpace::GRBG:
            offsets[0] = 1;
            offsets[1] = 0;
            offsets[2] = 2;
            offsets[3] = 1;
            colorOffsets[0].push_back(0);
            colorOffsets[0].push_back(1);
            colorOffsets[1].push_back(-1);
            colorOffsets[1].push_back(0);
            colorOffsets[1].push_back(Width - 1);
            colorOffsets[1].push_back(Width);
            colorOffsets[2].push_back(-Width);
            colorOffsets[2].push_back(-Width + 1);
            colorOffsets[2].push_back(0);
            colorOffsets[2].push_back(1);
            colorOffsets[3].push_back(-1);
            colorOffsets[3].push_back(0);
            break;
        default:
            throw "Unsupported colorSpace. Only bayer supported.";
        }

        #pragma omp parallel for
        for (auto row = 0; row < Height; row++)
        {
            for (auto col = 0, bayerIndex = row * Width; col < Width; col++, bayerIndex++)
            {
                auto offset = (row % 2)*2 + (col % 2); //0...3
                std::for_each(colorOffsets[offset].begin(), colorOffsets[offset].end(),
                    [&](int colorOffset)
                {
                    auto rgbIndex = (bayerIndex + colorOffset) * 3 + offsets[offset];
                    RgbChannel[rgbIndex] = BayerChannel[bayerIndex];
                });
            }
        }
    }

Proposed changes:

To shorten that huge switch statement a little bit you could extract the offsets into a few constant arrays, since those are not dependent on the function inputs.

const int offsets_BGGR[4] = { 2, 1, 1, 0 };
const int offsets_RGGB[4] = { 0, 1, 1, 2 };
const int offsets_GBRG[4] = { 1, 2, 0, 1 };
const int offsets_GRBG[4] = { 1, 0, 2, 1 };

Then just grab a pointer to the appropriate array in the switch cases.

But going further, I would also extract the switch statement into a separate helper function. Something like a GetColorSpaceConversionOffsets().

The BayerChannel parameter is only read from, so it should be marked const. This will be a valuable hint to callers of the function, since they can rest assured that the source data will remain unchanged upon function completion (unless there is a bug, of course).

Input parameter validation?

At the moment you are not doing any, which means that if a null pointer is passed as one of the parameters, the code would crash without much information about the error. Consider at the very least asserting to validate the inputs and help catching errors early.

A couple other minor points:

Avoid throwing native types, such as raw strings and integers. The C++ Standard provides a set of throwable classes in the <stdexcept> standard header, that you can use instead. Most commonly you will throw a runtime_error for generic runtime problems, but you can also define a custom exception class to make error reporting more detailed.
Use camelCase with the first letter lowercase for variables and function parameters, reserving PascalCase for types. For instance, in your function the ColorSpace type name confuses itself with the variable instance name:

void static ConvertBayerToRgbImageDemosaic(T* BayerChannel, T* RgbChannel,
             int Width, int Height, ColorSpace ColorSpace) { ...
                                    ^^^^^^^^^^ ^^^^^^^^^^

Updated code:

#include <vector>
#include <algorithm>
#include <stdexcept>

enum class ColorSpace
{
    BGGR, RGGB, GBRG, GRBG
};

// Local data and helpers "hidden" inside an anonymous namespace.
namespace
{

const int offsets_BGGR[4] = { 2, 1, 1, 0 };
const int offsets_RGGB[4] = { 0, 1, 1, 2 };
const int offsets_GBRG[4] = { 1, 2, 0, 1 };
const int offsets_GRBG[4] = { 1, 0, 2, 1 };

void GetColorSpaceConversionOffsets(ColorSpace colorSpace, int width, std::vector<int> colorOffsets[4], const int *& offsets)
{
    // Calculate offsets according to color space
    switch (colorSpace)
    {
    case ColorSpace::BGGR:
        offsets = offsets_BGGR;
        colorOffsets[0].push_back(0);
        colorOffsets[0].push_back(1);
        colorOffsets[0].push_back(width);
        colorOffsets[0].push_back(width + 1);
        colorOffsets[1].push_back(-1);
        colorOffsets[1].push_back(0);
        colorOffsets[2].push_back(0);
        colorOffsets[2].push_back(1);
        colorOffsets[3].push_back(-width - 1);
        colorOffsets[3].push_back(-width);
        colorOffsets[3].push_back(-1);
        colorOffsets[3].push_back(0);
        break;
    case ColorSpace::RGGB:
        offsets = offsets_RGGB;
        colorOffsets[0].push_back(0);
        colorOffsets[0].push_back(1);
        colorOffsets[0].push_back(width);
        colorOffsets[0].push_back(width + 1);
        colorOffsets[1].push_back(-1);
        colorOffsets[1].push_back(0);
        colorOffsets[2].push_back(0);
        colorOffsets[2].push_back(1);
        colorOffsets[3].push_back(-width - 1);
        colorOffsets[3].push_back(-width);
        colorOffsets[3].push_back(-1);
        colorOffsets[3].push_back(0);
        break;
    case ColorSpace::GBRG:
        offsets = offsets_GBRG;
        colorOffsets[0].push_back(0);
        colorOffsets[0].push_back(1);
        colorOffsets[1].push_back(-1);
        colorOffsets[1].push_back(0);
        colorOffsets[1].push_back(width - 1);
        colorOffsets[1].push_back(width);
        colorOffsets[2].push_back(-width);
        colorOffsets[2].push_back(-width + 1);
        colorOffsets[2].push_back(0);
        colorOffsets[2].push_back(1);
        colorOffsets[3].push_back(-1);
        colorOffsets[3].push_back(0);
        break;
    case ColorSpace::GRBG:
        offsets = offsets_GRBG;
        colorOffsets[0].push_back(0);
        colorOffsets[0].push_back(1);
        colorOffsets[1].push_back(-1);
        colorOffsets[1].push_back(0);
        colorOffsets[1].push_back(width - 1);
        colorOffsets[1].push_back(width);
        colorOffsets[2].push_back(-width);
        colorOffsets[2].push_back(-width + 1);
        colorOffsets[2].push_back(0);
        colorOffsets[2].push_back(1);
        colorOffsets[3].push_back(-1);
        colorOffsets[3].push_back(0);
        break;
    default:
        throw std::runtime_error("Unsupported ColorSpace. Only bayer supported.");
    }
}

} // namespace {}

template<typename T>
void ConvertBayerToRgbImageDemosaic(const T* bayerChannel, T* rgbChannel, int width, int height, ColorSpace colorSpace)
{
    // Translates index offset in Bayer image to channel offset in RGB image
    std::vector<int> colorOffsets[4];
    const int * offsets = nullptr;
    GetColorSpaceConversionOffsets(colorSpace, width, colorOffsets, offsets);

    #pragma omp parallel for
    for (auto row = 0; row < height; row++)
    {
        for (auto col = 0, bayerIndex = row * width; col < width; col++, bayerIndex++)
        {
            auto offset = (row % 2) * 2 + (col % 2); // 0...3

            std::for_each(colorOffsets[offset].begin(), colorOffsets[offset].end(),
                [&](int colorOffset)
                {
                    auto rgbIndex = (bayerIndex + colorOffset) * 3 + offsets[offset];
                    rgbChannel[rgbIndex] = bayerChannel[bayerIndex];
                });
        }
    }
}


An extension method to get the current user I think that everyone is familiar with the concept of a context (such as HttpContext in ASP.NET) where you can put some shared resources and take them back later in another module (I think the most used scenario is current/logged in user information).

Well it's obvious that usually these contexts are generic (and in a strong typed languages such as C# you should access their members using keys) and thus we have to write codes like this al the time :

var user=(User)HttpContext.Current.Items["CurrentUser"];

This code has some problems in my opinion:

Use of magic keys (we should always use constants instead of in-line strings)
Possible null reference exception (either by misspelling the key or a missing key)
Value always should cast to original value

In other words a developer should know a lot of things in order to write this line of code.

There's no doubt that we should re-factor our code so we can prevent upcoming problems.

One solution might be having a static member of a common class that every other module can call such as:

public class UserController
{
       public static User GetCurrentUser()
       {
             return (User)HttpContext.Current.Items["CurrentUser"];
       }
}

Actually we just move our repeated code to one single place as you can see therefore we can control all aspects of it (exception management and casting) in one place and now our fellow developers can write codes like this:

var user=UserController.GetCurrentUser();

Nice and easy, but is there any way to pretend that GetCurrentUser was a member of current module?

To do that we can have all our modules to be inherited from the same base class but I don't think that is a good idea because inheritance always makes a solution more complex specially in languages like C# with no multiple inheritance.

Then I thought: what if we could have an extension method that we could use in our modules? Something like this:

public static User GetCurrentUser(this IUserInfoConsumer self)
{
       return (User)HttpContext.Current.Items["CurrentUser"];
}

As you can see IUserInfoConsumer is a marker interface to mark our modules/components as a consumer of user information (it has no members) so that in every class that has this maker interface we can write a code like this:

var user=this.GetCurrentUser();

I would like to know what you think about this approach. Any other ideas?

When I encountered a similar issue I had a base controller that the rest of my controllers inherited from. Then, on that base controller there was a read only property named User that got the current user.

Personally I think inheritance can be confusing when it is abused just like everything else. As long as you keep your base controller relatively lean, and don't use it as a place to drop random methods which may not used in every controller, then I think inheriting from a base controller would be a pretty straight forward solution.


XML to Windows.Forms.Keys List It took me a lot of poking around and unit testing to get the code to look like it is right now. So I have a XML file which in part looks like this

<FunctionKeys>
    <Name>F13</Name>
    <Name>F14</Name>
    <Name>F15</Name>
    <Name>F16</Name>
</FunctionKeys>

and So I want to take that data and put it back into my class.

This is in part what I have

    public override void Load(string elementText)
    {
        var ele = XElement.Parse(elementText);
        if (ele.Element("FunctionKeys").HasElements)
        {
            var funcs = ele.Element("FunctionKeys")
                .Descendants("Name")
                .Select(x=>x.Value)
                .ToList();

            foreach (string s in funcs)
            {
                dliUnit.FunctionKeyList.Add(
                    (System.Windows.Forms.Keys)System.Enum.Parse(typeof(System.Windows.Forms.Keys),
                    s));
            }
        }
    }

Class to save to

public class DLIUnit
{
    public List<Keys> FunctionKeyList
    {
        get;set;
    }
    //Other Members
}

Somethign about the way that I parse the string back to the Enum (well the entire process really) doesn't sit well with me. I'm very bad at LINQ but have been trying hard to learn it and use it more and more when I play with XML. Is there a better/cleaner way to parse the FunctionKeys?

EDIT

I forgot to mention that I can change any portion of the code or XML file. Right now this is a new idea and can be changed to make it better.

Sticking with the approach you used how about this as an alternative.

public override void Load(string elementText)
    {
        var ele = XElement.Parse(elementText);

        var xElement = ele.Element("FunctionKeys");
        if (xElement != null && xElement.HasElements)
        {
            _dliUnit.FunctionKeyList.AddRange(xElement
                               .Descendants("Name")
                               .Select(x => EnumHelper.GetEnum<Keys>(x.Value))
                );
        }
    }

I created a little Enum Extensions class just because I like typing GetEnum:

public static class EnumHelper
{
    public static T GetEnum<T>(string name)
    {
        if (IsValidEnumFor<T>(name))
            return (T)Enum.Parse(typeof(T), name);
        else
            throw new ArgumentException(typeof(T) + "does not contain a value member = " + name);
    }

    public static T GetEnum<T>(int number)
    {
        if (Enum.IsDefined(typeof(T), number))
        {
            return (T)Enum.ToObject(typeof(T), number);
        }
        else
        {
            throw new ArgumentException(typeof(T) + "does not contain a value member = " + number.ToString());
        }
    }

    public static bool IsValidEnumFor<T>(string name)
    {
        return Enum.IsDefined(typeof(T), name);
    }
}

Alternative - XmlSerializer

One alternative I have used in the past to parse Xml into objects is the .NET XmlSerializer.  In your case you might do something like:

StringReader sr = new StringReader(xml);

// Create an XmlSerializer object to perform the deserialization
XmlSerializer xs = new XmlSerializer(typeof(DLIUnit));
return (DLIUnit)xs.Deserialize(sr);

This may not work for you but I would recommend having a look into this class as it is fairly easy to use once you get going.  Once I got my head around the basics it helped immensely.


String to binary converter I did research about binary counting, and wanted to make an application in Java that converts a string to binary code:

 /**
 * This class converts strings by looping through the characters, gets the
 * decimal code for each, then divides the number by 2 untill the quotient is 0
 *
 * @author Mr Pro Pop
 *
 */
public class Binary {

    /**
     * This function takes a string as a parameter and converts it to binary code
     *
     * @param string The string to be converted to binary code
     * @return The binary code of the input string
     */

    public String getBinaryCode(String string) {

        if (string.length() == 0)
            return null;

        char[] characters = string.toCharArray();
        StringBuilder result = new StringBuilder();
        int number = 0;

        for (int i = 0; i < characters.length; i++) {
            number = getDecimal(characters[i]);
            for (int j = 0; j < 8; j++) {
                if (number % 2 == 0) {
                    result.insert(0, 0);
                } else {
                    result.insert(0, 1);
                }
                number = (int) (number / 2);
            }
            result.append(" ");
        }
        return result.toString();
    }

    /**
     * This function gets the decimal number of a character
     *
     * @param character The character that we want the decimal value of
     * @return The decimal of the character from the ascii table
     */
    public int getDecimal(char character) {
        for (int i = 0; i <= 255; i++) {
            if (character == (char) i) {
                return i;
            }
        }
        return -1;
    }

    public static void main(String[] args) {
        String string = "a";
        Binary b = new Binary();
        System.out.println("Binary code for" + string + " is " + b.getBinaryCode(string));
    }

}

Output:

  Binary code for a is 01100001

Here it works well, however, spacing doesn't work. I do acknowledge that I could simply code this in a different easier method by Integer.toBinaryString(character[i]) but tried doing it my way to learn and especially with this method.

Any improvements I could do or anything to pay attention for?

I would change

public int getDecimal(char character)
{
    for (int i = 0; i <= 255; i++)
    {
         if (character == (char) i)
         {
              return i;
          }
     }
     return -1;
}

To just

public int getDecimal(char character)
{
     if(character<0||character>255)
          return -1;
     return character;
}

, As char is a number, too.

Also try using >> operator to get nth binary digit, which means division by 2.

number = (int) (number / 2);

To

number = number >>1;

I don't know if it would be faster in java, but it  works faster in old computer's assembly program(Division was expensive than just shifting)

If you use assembly you can access carry flags and optimize it.

For more information about it, refer here.

Provided that al holds the target number and bx holds the target string address, and cx holds 8,

L1:                  ; This is the loop
mov dl, '0'      ; Ascii character zero
shl al, 1          ; Upper bit now in carry flag
adc dl, 0        ; Adds carry flag - 0 or 1
mov [bx], dl   ; Save digit to current position
inc bx             ; Next position
loop L1          ; Counts down cx
mov [bx], 0    ; Zero terminate (might need to use register)


Making pixels of RGB barcode suitable for scanning Similar to Microsoft's HCCB before its discontinuation near 2015, Im trying to make a 2D barcode that uses RGB pixels to encode more than two possible states (Black/White).
Theoretically, at 100% data capacity, you could hold 16,777,216 different integers in a single pixel.
However, this level of storage is not feasible.

Consider the two RGB colors 47,213,71 and 47,213,72:

This unperceivable difference would have to be detected reliably even in suboptimal environments and lighting.
This is why Microsoft's HCCB only used 8 pre-determined colors, but I hypothesize modern cameras would still allow for greater detection and accuracy.

I wanted a barcode where you could change the gaps between colors, such that no two neighboring colors were less distant than the amount specified.
This way, a large gap like 128 could make each pixel represent a possible total of 8 colors - like Microsoft's original HCCB; While a small gap like 1 would give you the aforementioned 16,777,216 colors.
Now that I've finished explaining why anyone would want to do this, I can show you my C code. I'm not sure I did a good job at this, and I feel as though either my method or execution should be changed completely, so here's my approach to doing this
#include <stdio.h>
#include <stdint.h>

uint8_t* bind(uint32_t val, uint8_t bound){
  static uint8_t rgb[3];
  uint32_t buffer[3];
  uint32_t max = bound*(256/bound);

  buffer[0] = val * bound;
  buffer[1] = (buffer[0]/max)*bound;
  buffer[2] = (buffer[1]/max)*bound;

  rgb[0] = buffer[0] % max;
  rgb[1] = buffer[1] % max;
  rgb[2] = buffer[2] % max;

  return rgb;
}

uint32_t unbind(uint8_t* rgb, uint8_t bound){
  uint32_t max = 256/bound;
  return (rgb[0]/bound)+(rgb[1]/bound)*max+(rgb[2]/bound)*max*max;
}

int main(void) {
  uint32_t data = 1;
  uint8_t color_range = 128;

  uint8_t* colors = bind(data, color_range);
  uint32_t calculated = unbind(colors, color_range);

  printf("expected value: %u\nreturned value: %u\n\n", data, calculated);
  printf("rgb value: %u,%u,%u\n", colors[0], colors[1], colors[2]);
  return 0;
}

This:
static uint8_t rgb[3];
return rgb;

is a problem. Two different callers assuming to have two different copies of the return array will actually only have one, and that breaks re-entrance. Better off to return by value instead of return by reference in this case - particularly if you make a utility struct. A common pattern is to union over a three-element array and a nested struct of red, green and blue components. To ensure that this is valid, you'll need to tell your compiler to use tight packing, typically with #pragma pack. If you have misgivings on the portability of this approach and you need to support multiple exotic compilers, you'll need to ditch the union and instead write a conversion function.
bind and unbind can both be marked static since they're in the same translation unit.
The only other minor thing I see here is that two-space indentation is somewhat non-standard, and 4-space will be more legible.