Counting Sequential Booleans This function takes a list/array of booleans and converts them to an array that counts the number of either True/False values found next to each other.
I'd like to see this optimized for performance. It's not too slow, but I do use multiple loops with embedded if-else statements, I'm wondering if they're absolutely necessary.
import numpy as np

x = np.random.uniform(1,100,100)
b = x > x.mean()

#function start, input is b
endarray = []
count = 0
instance = True
while True:
    subarray = 0
    while True:
        if count >= len(b):
            endarray.append(subarray)
            break
        if b[count] == instance:
            subarray += 1
            count += 1
        else:
            endarray.append(subarray)
            instance = not instance
            break
    if count >= len(b):
        break

if len(endarray) % 2 != 0:
    endarray = np.append(endarray, 0)
else:
    endarray = np.asarray(endarray)

endarray = endarray.reshape(-1,2)

The output is a Nx2 array, where the left-hand values are always a count of Trues, and the right-hand values are always a count of Falses.
After a sequence of False values are no longer continuous(a True value pops up), the next count of True values begin, and vice versa.
Example input
b
Out[31]:
array([ True,  True,  True, False,  True,  True,  True,  True, False,
       False,  True, False, False,  True, False, False, False, False,
        True, False, False, False,  True,  True,  True,  True, False,
       False,  True, False, False, False, False, False, False,  True,
        True, False,  True,  True, False, False,  True, False, False,
        True, False, False,  True, False,  True, False,  True, False,
        True,  True,  True, False,  True, False,  True,  True,  True,
        True, False, False,  True, False,  True,  True,  True,  True,
        True,  True, False,  True,  True, False,  True,  True, False,
       False,  True, False,  True, False, False,  True,  True,  True,
        True, False, False, False, False, False,  True,  True,  True,
        True])

Example output
endarray
Out[32]:
array([[3, 1],
       [4, 2],
       [1, 2],
       [1, 4],
       [1, 3],
       [4, 2],
       [1, 6],
       [2, 1],
       [2, 2],
       [1, 2],
       [1, 2],
       [1, 1],
       [1, 1],
       [1, 1],
       [3, 1],
       [1, 1],
       [4, 2],
       [1, 1],
       [6, 1],
       [2, 1],
       [2, 2],
       [1, 1],
       [1, 2],
       [4, 5],
       [4, 0]])

Edit:
I wanted to add an updated version of this code, the one in the answer below is not technically correct in all regards. But this was entirely derived from it:
m = np.append(b[0], np.diff(b))
_, c = np.unique(m.cumsum(), return_index=True)
out = np.diff(np.append(c, len(b)))
if b[0] == False:
    out = np.append(0, out)
if len(out) % 2:
    out = np.append(out, 0)
out = out.reshape(-1, 2)

Using itertools.groupby
What you are looking for is itertools.groupby. When there is an odd number of groups then we use try-except block here.
from itertools import groupby

get_grp_len = lambda grp: len([*grp])

def transform(b):
    if len(b) == 0:  # if not b wouldn't work since your `b` is ndarray
        return []
    it = groupby(b)
    out = []
    for _, grp in it:
        try:
            t_size = get_grp_len(grp)
            f_size = get_grp_len(next(it)[1])
            out.append([t_size, f_size])
        except StopIteration:
            out.append([t_size, 0])
    return out

print(transform(b)) # `b` taken from the question itself.
# same output as expected output posted in question.

NumPy has a lot of vectorized operations
NumPy has many vectorized operations, you have to find the correct ones. Not an expert but the below approach should do good.
The idea here is to find the index of the first value from each group and then take the difference.

We check if ith element is not equal to i+1th element.
Now use np.ndarray.cumsum to give each group a unique sequential number.
Then use np.unique to get first index of value from each group.
Find the difference between i+1th value and ith to get size of each grp. We can use np.diff

m = b != (np.r_[np.nan, b[:-1]])
_, c = np.unique(m.cumsum(), return_index=True)
#print(c)
# array([ 0,  3,  4,  8, 10, 11, 13, 14, 18, 19, 22, 26, 28, 29, 35, 37, 38,
#        40, 42, 43, 45, 46, 48, 49, 50, 51, 52, 53, 54, 57, 58, 59, 60, 64,
#        66, 67, 68, 74, 75, 77, 78, 80, 82, 83, 84, 85, 87, 91, 96])

# np.unique gives the index of the first occurrence, our `b` len is 100
# and `c` stopped at 96. we need to add the last index + 1 to it.

out = np.diff(np.r_[c, len(b)])

# Now, reshape your array, if it's of odd length add a 0 at end.
if len(out) % 2:
    out = np.r_[out, 0].reshape(-1, 2)
out = out.reshape(-1, 2)
print(out)
# same output mentioned in the question.

Using Pandas's GroupBy
A lot of times pandas and NumPy are used together so might as well post pandas code too.
import pandas as pd
s = pd.Series(b)
g = s.ne(s.shift()).cumsum()
out = s.groupby(g).size().to_numpy()
# repeat same step as we did in above solution add a 0 if len is odd.

if len(out)%2:
    out = np.r_[out, 0].reshape(-1, 2)
out = out.reshape(-1, 2)
print(out)

Code Review
Everything's good as far as I can see, well-named variables, proper indentation but too many if :p


Querying a phone directory until end of input is reached I need to create a phone directory with given inputs, then I need to
  query the dictionary for unknown number of given keys, if not found,
  print Not found. Link of Original HackerRank Problem here

I have written this code in Python3, and it's working fine. Please review, and suggest other ways of taking unknown number of inputs. Should it be done without using sys?

import sys
phone={}
n = int(input())
for _ in range(n):
    line = input().split()
    phone[line[0]] = line[1]
for line in sys.stdin:
    key=line.strip('\n')
    try:
        print(key+'='+phone[key])
    except KeyError:
            print('Not found')

Take care to adhere to PEP 8 code formatting guidelines.  In particular, there should be a space before and after each = and +.  The KeyError handler should have just one level of indentation.

The phonebook could be constructed using a one-liner.  I consider it good practice to specify maxsplit=1, though it makes no practical difference when you are assured that the input will be well formed.

Getting the rest of the input using for line in sys.stdin is not a bad approach, I think.

import sys

n = int(input())
phonebook = dict(input().split(maxsplit=1) for _ in range(n))
for line in sys.stdin:
    key = line.strip('\n')
    try:
        print('{0}={1}'.format(key, phonebook[key]))
    except KeyError:
        print('Not found')


Sort Stack using just an additional stack The problem is from the cracking coding Interview programming book.

The problem is that:

  Sort Stack: Write a program to sort a stack such that the smallest
  items are on the top. You can use an additional temporary stack, but
  you may not copy the elements into any other data structure (such as
  an array).

Below is my code in scala to sort Stack s, however, I think
it is not the optimal solution in scala, may I know any other solutions can be written in functional programming language scala.

import scala.collection.mutable.Stack
    def sorted(s: Stack[Int]): Unit={
        def sortedwithtemp(original: Stack[Int], temp:Stack[Int], current: Int): Unit=(current, temp.top)match{
          case (a,b) if a>=b =>temp.push(a);
          if(original.size==0) original.pushAll(temp)
          else {
            val curr= original.pop()
          sortedwithtemp(original, temp, curr)
          }

          case (a,b) => original.push(temp.pop());
           if (temp.isEmpty) {temp.push(current); val curr= original.pop();sortedwithtemp(original, temp, curr)}
           else sortedwithtemp(original, temp, current)
        }

        val tempstack=new Stack[Int]
        if (s.size>=2){
          tempstack.push(s.pop())
          val current=s.pop()
          sortedwithtemp(s, tempstack, current)
        }
   }

It's easier if you define your own Stack type, as the Scala Stack is a collection type which makes it a bit heavyweight.

A stack is either an Empty stack or an element (top) pushed onto a stack (pop):

// Define a simple immutable stack datatype.
trait Stack[+T]
object Empty extends Stack[Nothing]
case class Push[+T](top : T, pop : Stack[T]) extends Stack[T]

We can sort a stack by inserting each element onto a sorted stack. The insertion is done by popping the stack until we find the right position, and then rebuilding the stack when the call-stack returns.

object StackSort {
    // Sort a stack using insertion-sort.
    def sort[T](s : Stack[T])(implicit ord : Ordering[T]) : Stack[T] =
        s match {
            case Empty => s
            case Push(top, pop) => insert(top, sort(pop))
        }

    // Insert an element into a sorted stack.
    def insert[T](x : T, s : Stack[T])(implicit ord : Ordering[T]) : Stack[T] =
        s match {
            case Push(top, pop) if (ord.lt(x, top)) => Push(top, insert(x, pop))
            case _ => Push(x, s)
        }
}

A simple test:

object Test {
    import StackSort._

    def main(args : Array[String]) : Unit = {
        val test = Push(3, Push(1, Push(2, Push(5, Empty))))
        println(test)

        val sorted = sort(test)
        println(sorted)
    }
}

which outputs:

Push(3,Push(1,Push(2,Push(5,Empty$@45c8e616))))
Push(5,Push(3,Push(2,Push(1,Empty$@45c8e616))))


C++ Calculator for complex numbers I've written a little calculator in C++ for complex numbers:

#include <iostream>
using namespace std;

class ComplexNumber {

    public:
        double real;
        double imaginary;

        void add(ComplexNumber a, ComplexNumber b) {

            //Just add real- and imaginary-parts
            double real = a.real + b.real;
            double imaginary = a.imaginary + b.imaginary;
            ComplexNumber c = ComplexNumber(real, imaginary);
            cout << "a + b = " << c.real << " + (" << c.imaginary << ") * i" << endl;
        }

        void sub(ComplexNumber a, ComplexNumber b) {

            //Just subtract real- and imaginary-parts
            double real = a.real - b.real;
            double imaginary = a.imaginary - b.imaginary;
            ComplexNumber c = ComplexNumber(real, imaginary);
            cout << "a - b = " << c.real << " + (" << c.imaginary << ") * i" << endl;
        }

        void multiply(ComplexNumber a, ComplexNumber b) {

            //Use binomial theorem to find formula to multiply complex numbers
            double real = a.real * b.real - a.imaginary * b.imaginary;
            double imaginary = a.imaginary * b.real + a.real * b.imaginary;
            ComplexNumber c = ComplexNumber(real, imaginary);
            cout << "a * b = " << c.real << " + (" << c.imaginary << ") * i" << endl;
        }

        void divide(ComplexNumber a, ComplexNumber b) {

            //Again binomial theorem
            double real = (a.real * b.real + a.imaginary * b.imaginary) / (b.real * b.real + b.imaginary * b.imaginary);
            double imaginary = (a.imaginary * b.real - a.real * b.imaginary) / (b.real * b.real + b.imaginary * b.imaginary);
            ComplexNumber c = ComplexNumber(real, imaginary);
            cout << "a : b = " << c.real << " + (" << c.imaginary << ") * i" << endl;
        }

    /*
     * Constructor to create complex numbers
     */
    ComplexNumber(double real, double imaginary) {
        this->real = real;
        this->imaginary = imaginary;
    }
};

int main() {

    /*
     * Variables for the real- and imaginary-parts of
     * two complex numbers
     */
    double realA;
    double imaginaryA;
    double realB;
    double imaginaryB;

    /*
     * User input
     */
    cout << "enter real(A), imag(A), real(B) and imag(B) >> ";
    cin >> realA >> imaginaryA >> realB >> imaginaryB;
    cout << endl;

    /*
     * Creation of two objects of the type "ComplexNumber"
     */
    ComplexNumber a = ComplexNumber(realA, imaginaryA);
    ComplexNumber b = ComplexNumber(realB, imaginaryB);

    /*
     * Calling the functions to add, subtract, multiply and
     * divide the two complex numbers.
     */
    a.add(a, b);
    a.sub(a, b);
    a.multiply(a, b);
    a.divide(a, b);
    return 0;
}

I would appreciate any suggestions to improve the code.

You can find my follow-up question here.

In short programs it can be OK, but in general avoid  writing using namespace std. You'll find plenty of material here and elsewhere on why this is so.
To promote proper encapsulation of data, both real and imaginary should be declared under private, i.e., not be visible to the outside.
All of the four member functions that perform arithmetic take on too much responsibility and as a result, are very inconvenient for the user. That is, remember: one function, one responsibility. If you add, then you don't also print. For example, as a user, I just want to use your class for complex arithmetic - I don't want to print every time I do so!
Your four member functions don't modify the state of the object. This makes the whole class and its functionality quite rigid and strange. As it is, the functionality appears as it should be a collection of four free functions not inside any class (indeed, perhaps your background is in Java where I can imagine this is more common). A more intuitive interface for let's say the addition would be void add(const ComplexNumber& other) { ... }, where the implementation actually adds to real and imaginary of *this. Same for the other three operations.
If you wanted to get fancy, you could use operator overloading to allow for a natural way to express complex arithmetic for the user.
It would be useful to add a void print() const { ... } method in case the user wants to print.
Use an initializer list if you need to write explicit constructors, i.e., write ComplexNumber(double r, double i) : real(r), imaginary(i) { } instead. If you don't, the compiler will call default constructors on the members first which in your case is unnecessary.
In modern C++, we have the option of using in-class constructors for default values. This is quite handy, i.e., you could have double real {0.0}; (similarly for imaginary) if you wanted to support the creation of complex numbers without an explicit constructor call.
By the way, you don't have to write ComplexNumber a = ComplexNumber(realA, imaginaryA); when it's much cleaner to write ComplexNumber a(realA, imaginaryA);.
Perhaps you know this, but std::complex<T> does exist if you wanted to do complex arithmetic in a more serious setting.


C# Multi level HTML TagBuilder object - Design pattern Review needed I find the TagBuilder written in C# little poor without the ability to "host" inside it sub tags (as objects not as strings).

So I wrote a simple class that inherits TagBuilder and allow to save sub tags.
I can navigate inside it and add things to it while I move across the application.
And when I call the Top Tag Object .ToString, it will render the entire html

public class MultiLevelHtmlTag : TagBuilder
{
    public MultiLevelHtmlTag(string tagName) : base(tagName) { }

    public List<MultiLevelHtmlTag> InnerTags = new List<MultiLevelHtmlTag>();

    public override string ToString()
    {
        if (InnerTags.Count > 0)
        {
            foreach (MultiLevelHtmlTag tag in InnerTags)
            {
                this.InnerHtml += tag.ToString();
            }
        }
        return base.ToString();
    }
}

this is how I use it:

MultiLevelHtmlTag top = new MultiLevelHtmlTag("div");
top.InnerTags.Add(new MultiLevelHtmlTag("span"));
log.Debug(top.ToString());

output:

What do you think about it?
is that a goot pattern?

I would make sure the list is not exposed publicly - the caller can then do whatever they want with that list, even assign null to it.  So I've refactored it a little bit and implemented Add() (you may also want to implement Remove(), etc.):

/// <summary>
/// Simple class that inherits TagBuilder and allow to save sub tags.
/// </summary>
public class MultiLevelHtmlTag : TagBuilder
{
    /// <summary>
    /// List of inner tags.
    /// </summary>
    private readonly IList<MultiLevelHtmlTag> innerTags = new List<MultiLevelHtmlTag>();

    /// <summary>
    /// Initializes a new instance of the <see cref="MultiLevelHtmlTag"/> class.
    /// </summary>
    /// <param name="tagName">The name of the tag.</param>
    public MultiLevelHtmlTag(string tagName) : base(tagName)
    {
    }

    /// <summary>
    /// Gets the inner tag list.
    /// </summary>
    /// <value>The inner tag list.</value>
    public IEnumerable<MultiLevelHtmlTag> InnerTags
    {
        get
        {
            return new ReadOnlyCollection<MultiLevelHtmlTag>(this.innerTags);
        }
    }

    /// <summary>
    /// Adds the specified tag to the inner tag list.
    /// </summary>
    /// <param name="tag">The tag to add.</param>
    public void Add(MultiLevelHtmlTag tag)
    {
        if (tag == null)
        {
            throw new ArgumentNullException("tag");
        }

        this.innerTags.Add(tag);
    }

    /// <summary>
    /// Returns a <see cref="System.String"/> that represents this instance.
    /// </summary>
    /// <returns>
    /// A <see cref="System.String"/> that represents this instance.
    /// </returns>
    public override string ToString()
    {
        var sb = new StringBuilder();

        foreach (var tag in this.innerTags)
        {
            sb.Append(tag.ToString());
        }

        this.InnerHtml = sb.ToString();
        return base.ToString();
    }
}


Drawing on a pixel in blocks and diagonally I've been working on this mini project to teach myself how to better use the graphical functions and nested loops in Java. I have achieved my basic goal with this project to be able to draw on a screen pixel by pixel in blocks and diagonally.

The basic idea is that it draws on the screen pixel by pixel in two different ways, one is diagonally and one is block by block.

I'm looking for any way at all to improve my program without implementing code very much more advanced than the code used in the program so far. I'm really looking for any improvements at all. A way to make it more compact, faster running, basically anything at all to make it 'better'.

    /**
     * @author Samuel
     */
    package main;
import java.awt.Color;
import java.awt.Dimension;
import java.awt.Graphics;
import java.util.Random;
import javax.swing.JFrame;
@SuppressWarnings("serial")
public class Draw extends JFrame{
    public static int width = 1280;
    public static int height = (width/16)*9;
    int x = 3;
    int y = 25;
    Color randomColor = Color.BLACK;
    public Draw() {

        this.setPreferredSize(new Dimension(width,height));
        this.setResizable(false);
        this.setLocationRelativeTo(null);
        this.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        this.pack();
        this.setVisible(true);
    }
    @Override
    public void paint(Graphics g) {
        super.paint(g);
        while(true) {
            nextPixel2(); //Change to nextPixel() for block, nextPixel2 for diagonal
            g.setColor(randomColor);
            g.drawLine(x, y, x, y);
            pixels++;
        }
    }
    //250000~ timer value = 1 Second/Pixel
    int timer = 1;
    int pixels = 0;
    int xLock = x;
    int xStart = x;
    int yLock = y;
    int yStart = y;
    int blockSize = 50;
    int changeColor = 5; //Diagonal change color
    int i = 0;
    public void nextPixel2(){ //Diagonal
        x--;
        y++;
        for(int i = 0;i < timer;i++){
            i++;
            System.out.println("Pixels drawn: "+pixels);
        }
        while(x < xStart || y > height){
            xLock++;
            x=xLock;
            y=yLock;
            while(x > width) {
                xLock--;
                x=xLock;
                yLock++;
                y=yLock;
            }
            if(y == height && x == width){
                xLock = xStart;
                x = xLock;
                yLock = yStart;
                y = yLock;
            }
            if(i < changeColor){
                i++;
            }else{
            generateColor();
            i=0;
            }
        }
    }
    public void nextPixel(){ //Blocks
        x++;
        pixels++;
            for(int i = 0;i < timer;i++){
                i++;
                System.out.println("Pixels drawn: "+pixels);
            }
            while(x-xLock >= blockSize){
                x=xLock;
                y++;
                while(y-yLock >= blockSize){
                    y=yLock;
                    xLock+= blockSize;
                    x=xLock;
                    while(xLock >= width){
                        yLock+= blockSize;
                        y=yLock-1;
                        xLock=xStart;
                        if(yLock >= height){
                            y = yStart;
                            yLock = yStart;
                            x = xStart;
                            xLock = xStart;
                        }
                    }
                generateColor();
                }
            }
        }
    public void generateColor(){
        Random rand = new Random();
        float red = rand.nextFloat();
        float green = rand.nextFloat();
        float blue = rand.nextFloat();
        randomColor = new Color(red, green, blue);
    }
    public static void main(String[] args){
        Draw draw = new Draw();
    }
}

Naming

Names of methods should describe the purpose they have. If a comment is needed to describe what a method does //Change to nextPixel() for block, nextPixel2 for diagonal, one should think about another more meaningful name.
What would Mr.Maintainer think at reading this code ?

@Override
public void paint(Graphics g) {
    super.paint(g);
    while(true) {
        nextPixel2(); //Change to nextPixel() for block, nextPixel2 for diagonal
        g.setColor(randomColor);
        g.drawLine(x, y, x, y);
        pixels++;
    }
}

He would get the conclusions

I need to dig into the netxPixel2() method
Inside this method the value of randomColor and a location (defined by x and y) will maybe be changed.
What does the nextPixel2() do and why does it change 2 things
There is another method nextPixel() involved, and I need to dig into this too
This is a never ending while loop

Decoupling
As the pixel manipulation code only relies on a Graphics, there is no need for this code to be inside a JFrame extending class. You should better use composition over inheritence.

Style

The indention of the code can be improved a lot. Maybe some of this came from pasting here but for sure not this

if(i < changeColor){
    i++;
}else{
generateColor();
i=0;
}

Ok, let us dig into nextPixel2()

What do we see ?

Neither xStart nor yStart will be changed ever (after a quick glance into nextPixel()), so we should make them final and also give them a new name like e.g xBorderPosition

for(int i = 0;i < timer;i++){
   i++;
   System.out.println("Pixels drawn: "+pixels);
 }

This is just boilerplate code, as pixels isn't increased anywhere in the method, it stays at the initial value of 0, this should be removed, also because the responsibility of this method does not involve output of any data, but manipulating some values.

Algorithm

The x value will be decremented and the y value will be incremented. This will happen until

x < xStart or in other words until x == 2. At this point xLock will be incremented, x = xLock and y = 25 = yLock = yStart.

or

y > height which will be true after xLock == 699 so this will be the case we need to dig into

If the first condition is true and xLock will be incremented to xLock == 699 we start again with y = 25 = yLock = yStart and x = xLock = 699. After some iterations of the while(true) of the paint() method we will reach at the entrance to nextPixel() with the values x == 4 and y == 720 and after incrementing and decrementing we have x == 3 and  y == 721.  Se let us see what the following striped code will do then

while(x < xStart || y > height){
    xLock++; // now 700
    x=xLock; // now 700
    y=yLock; // now 25
    while(x > width) { // still false
        xLock--;
        x=xLock;
        yLock++;
        y=yLock;
    }
}

So the xLock == 699 border doesn't change anything as x is still <= width. But what we see is, if xLock would reach the value 1281 then x > width will be true and something will happen.

yLock will be incremented by one and xLock will become 1280 and the x > width won't be true again.

So as there is always only one iteration, we can replace this while loop with an if statement. So if anytime x > width we need to increment yLock by one and set xLock == width. And as we set x = xLock we can check if xLock > width and assign then the xLock and yLock values to x and y

Refactoring

while(x < xStart || y > height){
    xLock++;
    if (xLock > width) {
        yLock++;
        xLock = width;
    }
    x = xLock;
    y = yLock;
    if(y == height && x == width){
        xLock = xStart;
        x = xLock;
        yLock = yStart;
        y = yLock;
    }
    if(i < changeColor){
        i++;
    }else{
        generateColor();
        i=0;
    }
}

As we have seen x == width will only be true, if xLock > width, we can check y == height inside this if statement.

Refactoring again

while(x < xStart || y > height){
    xLock++;
    if (xLock > width) {
        yLock++;
        xLock = width;
        if(yLock == height){
            xLock = xStart;
            yLock = yStart;
        }
    }
    x = xLock;
    y = yLock;
    if(i < changeColor){
        i++;
    }else{
        generateColor();
        i=0;
    }
}

If we look at the remaining while conditions we see, that after this condtions will be true the first time, neither x < xStart will be true nor y > height will be true. So there is no need for a while loop. To save an additional indention, we also revert the condition to be a guard condition. Additional we extract the check for generating a random color to a separate method isColorChangeNeeded(), rename the class variable i to colorCheckCounter and the changeColor variable to colorCheckLimit which should be final also. And finally we will rename the method to assignDiagonalPixelLocation

public void assignDiagonalPixelLocation(){
    x--;
    y++;
    if (x >= xStart && y <=height) { return; }

    xLock++;
    if (xLock > width) {
        yLock++;
        xLock = width;
        if(yLock == height){
            xLock = xStart;
            yLock = yStart;
        }
    }
    x = xLock;
    y = yLock;
    if(isColorChangeNeeded()){
        generateColor();
    }
}

private boolean isColorChangeNeeded() {
    colorCheckCounter++;
    if (colorCheckCounter <= colorCheckLimit){ return false; }

    colorCheckCounter=0;
    return true;
}


Find common items between two collections, and set values in one collection when matched I want to find common items between two collection, and set values from one collection to another collection. I am aware of similar posts on the web, but they are different from this post. I want to know if there is a better algorithm or method than my current code below

class Job
{
    public Job(int id)
    {
        this.Id = id;
    }
    public int Id { get; private set; }

    public DateTime DiscountDate;

    //more properties
}

class JobManager
{
    public List<Job> GetJobs()
    {

        //Step 1: initialize the first collection
        List<Job> jobs = new List<Job>()
        {
            new Job(1),  //more other properties initialization
            new Job(2),
            new Job(3),
            new Job(4)
        };

        //Step 2: get second collection (id, and DiscountDate) by ids. The ids might be part or whole of collection in step 1
        List<int> idsForDiscountDate = new List<int> {2, 4}; //all ids must be part or whole of jobs list above, ie 1-4
        DataView dataView = GetDisccountDatesFromdatabaseBy(idsForDiscountDate);  //all rows must be part or whole of jobs list above, ie 1-4

        //Step 3: Find match, and set the value from dataView to jobs when matched.
        foreach (var job in jobs)
        {
            foreach (DataRowView dataRowView in dataView)
            {
                if (int.Parse(dataRowView["ID"].ToString()) == job.Id
                    //&& other logic here
                 )
                {

                   //once matched, it sets the value. This is what I need
                    job.DiscountDate = (DateTime) dataRowView["DiscountDate"];
                    break;
                }
            }
        }

        return jobs;
    }

    //get discount dates from database
    private DataView GetDisccountDatesFromdatabaseBy(List<int> idsForDisccountDate)
    {
        throw new NotImplementedException();

    }
}

It would be better to create a Dictionary<int, Job> of jobs and use it in the foreach loop. Otherwise, you have to parse dataRowView["ID"] field as many times as jobs are available (at worst). If number of jobs is less than 10, it is more performant to iterate the list of jobs instead of using a Dictionary<int, Job>:

//Step 3: Find match, and set the value from dataView to jobs when matched.
    Dictionary<int, Job> jobDict = jobs.ToDictionary(j => j.Id);

    foreach (DataRowView dataRowView in dataView)
    {
        var id = int.Parse(dataRowView["ID"].ToString());
        Job job;
        if (jobDict.TryGet(id, out job)
            //&& other logic here
         )
        {
            //once matched, it sets the value. This is what I need
            job.DiscountDate = (DateTime) dataRowView["DiscountDate"];
            break;
        }
    }


Sieve of Eratosthenes in C++11 Please point out every problem you see with this code. Any fundamental flaws, naming conventions, design decisions, you name it.

#include <iostream>
#include <algorithm>
#include <vector>

class SieveHelper {
public:
    SieveHelper() : cur(2) {}

    bool operator()(unsigned int i) {
        return i != cur && i % cur == 0;
    }

    unsigned int cur;
};

void sieve(std::vector<unsigned int>& arr) {
    SieveHelper helper;
    auto it = arr.begin();

    while (it != arr.end()) {
        helper.cur = *it;

        auto last = std::remove_if(it, arr.end(), helper);
        arr.erase(last, arr.end());

        it++;
    }
}

int main() {
    int n;
    std::cin >> n;

    std::vector<unsigned int> arr(n);
    for (int i = 2; i <= n; i++)
        arr[i-2] = i;

    sieve(arr);

    std::for_each(arr.begin(), arr.end(), [](unsigned int i) {
        std::cout << i << " ";
    });
    return 0;
}

I sort includes alphabetically, making it easier to keep track of which headers are included in long lists.

I would write:

#include <algorithm>
#include <iostream>
#include <vector>

In a long, unsorted list of includes it's hard to see if a header is included twice:

#include <vector>
#include <list>
#include <algorithm>
#include <new>
#include <fstream>
#include <set>
#include <stdexcept>
#include <list>
#include <cstdlib>

Imagine this problem increasing if you have a couple dozen includes!

arr is very undescriptive, and in this case even misleading. This isn't an array, it's a std::vector. vec would be slightly better, but try to describe what the data structure contains or does rather than what kind of data structure it is. Even something as simple as numbers is better than vec (or arr), and you can probably improve that even further. Naming is very important to ensure readability.
Consider using <cstdint> to control the size of numbers. This makes porting a whole lot easier.
In C++, it's a good habit to prefer prefix increment- and decrement-operators, because they are usually more efficient for non-built-in types. It doesn't really matter for built-in types, but consistency is nice.
Consider using using-declarations, i.e. using std::vector; and so on, to make the code more readable.
I'd usually make SieveHelper::cur private and control access to it, but I presume you know that and did it this way on purpose. It's fine for a small program, but remember that programs grow.
Use std::vector::at(), which is bounds-checked, whenever array indexing is not in the speed-critical path. The exception is when you're 110 % sure you can never, ever go out of bounds - but better safe than sorry!
Consider making sieve() take and return an argument, rather than just manipulate an output argument. This is definitely a trade-off, so use your own judgement, but output parameters are generally less readable.

On the plus side, this is mostly good. Extra points for using built-in algorithms, lambda and other juicy C++11 stuff :)


Find and remove duplicate files in one or more directories I needed this to clean up a couple of similar music libraries, which also contained some empty files and duplicate files with different names.

# dupFinder.py
import os, sys, shutil, datetime
import hashlib

def findDup(parentFolder):
    # Dups in format {hash:[names]}
    dups = {}
    for dirName, subdirs, fileList in os.walk(parentFolder):
        print('Scanning %s...' % dirName)
        for filename in fileList:
            # Get the path to the file
            path = os.path.join(dirName, filename)
            print(path)
            # Calculate hash
            file_hash = hashfile(path)
            # Add or append the file path
            if file_hash in dups:
                dups[file_hash].append(path)
            else:
                dups[file_hash] = [path]
    return dups

# Joins two dictionaries
def joinDicts(dict1, dict2):
    for key in dict2.keys():
        if key in dict1:
            dict1[key] = dict1[key] + dict2[key]
        else:
            dict1[key] = dict2[key]

def hashfile(path, blocksize = 65536):
    print(path)
    afile = open(path, 'rb')
    hasher = hashlib.md5()
    buf = afile.read(blocksize)
    while len(buf) > 0:
        hasher.update(buf)
        buf = afile.read(blocksize)
    afile.close()
    return hasher.hexdigest()

def handleResults(dict1):
    results = list(filter(lambda x: len(x) > 1, dict1.values()))
    if not 'testrun' in globals():
        backupdir = 'removed_' + str(datetime.datetime.now())
        if not os.path.exists(backupdir):
            os.makedirs(backupdir)
    else:
        backupdir = 'backup_dir_date_now'
    if len(results) > 0:
        print('Duplicates Found:')
        print('The following files are identical. The name could differ, but the content is identical')
        print('^^^^^^^^^^^^^^^^^')
        count = 0
        for result in results:
            for subresult in result:
                if count == 0:
                    print('KEEPING ONE COPY: %s.' % subresult)
                    count += 1
            elif 'testrun' in globals():
                print('\t\t %s WILL BE REMOVED TO: %s' % (subresult, backupdir))
            else:
                print('\t\tREMOVING %s TO %s' % (subresult, backupdir))
                try:
                    shutil.move(subresult, backupdir)
                except shutil.Error:
                    print ('\t\t EXISTS. DELETING.')
                    os.remove(subresult)
                print('___________________')

    else:
        print('No duplicate files found.')

if __name__ == '__main__':
    if len(sys.argv) > 1:
        dups = {}
        if sys.argv[1] == '-t':
            testrun = 1
            folders = sys.argv[2:]
        else: folders = sys.argv[1:]
        for i in folders:
            # Iterate the folders given
            if os.path.exists(i):
                # Find the duplicated files and append them to the dups
                joinDicts(dups, findDup(i))
            else:
                print('%s is not a valid path, please verify' % i)
                sys.exit()
        handleResults(dups)
    else:
        print('Usage: python dupFinder.py [-t] folder or python [-t] dupFinder.py folder1 folder2 folder3')

It can be run in test mode with -t flag.

Wow - awesome advice here sure transformed the code: GitHub Repo

If I had to pick a least-favourite line in the program, it would be this one:

if 'testrun' in globals():

… because

Global variables are bad.
You didn't even declare testrun as a global variable using global testrun, but rather you used introspection.
You didn't test the flag to see whether it was True or False; you just check for its existence.
The whole problem could have been avoided by calling handleResults() with a parameter.

print('%s is not a valid path, please verify' % i)
sys.exit()

When you exit the program due to an error, exit with a non-zero status.
i is an unconventional iteration variable, since it has the connotation of being an integer.  The code would be more readable if you had written for folder in folders: ….

In hashfile(), you should call open() using a with block.
Since hashfile() seems like it could be a basic reusable operation, I would avoid having it print anything.  The findDup() function, which calls hashfile(), prints status updates, and in fact it prints the same path just before calling hashfile().


URL Conditional in JavaScript I'd appreciate any help just refining this working code. It has a bit of a weird context, but basically it's for a gallery style site that has hundreds of pages, but needs new navigation buttons. Unfortunately there's no way to add these buttons without adding them for over 300+ pages manually, or using a javascript like this.

Let's say we have a URL like the following (this coincides with the site's URL structure):

http:/test.com/10-funny-pictures/

The script links next button that links to http:/test.com/10-funny-pictures/2/, and hides the last button.

If the last page in the gallery is http:/test.com/10-funny-pictures/10/, then it sets the last button to link to http:/test.com/10-funny-pictures/9/. But on the last page of a gallery, there is a javascript inserted on that last page that holds the URL of a new gallery to send the user to when the click next, that URL is passed to the main JavaScript and then the next button links to that rather than http:/test.com/10-funny-pictures/11/ (which doesn't exist).

It's messy, but the JSFiddle is right here, any help making the code more efficient would be appreciated. I just kind of made it but I'm not good at doing things the best way when I code.

Last

Next

    #lastbutton {
display: inline-block;
width: 80px;
height: 20px;
background-color: #0a72c2;
color: #ffffff;
text-decoration: none;
text-align: center;
padding: 8px;
font-size: 16px;
}

#nextbutton {
display: inline-block;
width: 80px;
height: 20px;
background-color: #0a72c2;
color: #ffffff;
text-decoration: none;
text-align: center;
padding: 8px;
font-size: 16px;
}

#leftcontainer {
margin-top: 30px;
float: left;
text-align: center;
width: 50%;
}

#rightcontainer {
margin-top: 30px;
text-align: center;
float: right;
width: 50%;
}

From a once over:

oneRoot and twoRoot are only needed in their respective if block, you should move the var statements to those if blocks.
This:

if(twodigit == 10) {
    lastURL = twoRoot + "9/";
    nextURL = twoRoot + "11/";
} else {
    lastURL = twoRoot + (twodigit - 1) + "/";
    nextURL = twoRoot + (twodigit + 1) + "/";
}

could simply be

    lastURL = twoRoot + (twodigit - 1) + "/";
    nextURL = twoRoot + (twodigit + 1) + "/";

I am not sure why you needed to call out the (twodigit == 10 case
urlData <- Not sure what this does at all
var oneslash = currentURL.substr(currentURL.length - 2); <- You are not using this
onedigit = oneslash.substr(0,1)*1; <- You already captured this in lastChar, you could simply do onedigit = +lastChar;
You have 0 comments..
You did not declare onedigit with var
You have some indentation trouble towards the end of your code.

I have something like this: ( also on http://jsfiddle.net/konijn_gmail_com/unQQ7/ )

function linkSet() {
    var currentURL = location.href;
    var lastURL = "";
    var nextURL = "";
    var lastChar = currentURL.charAt(currentURL.length - 2);
    var secondLastChar = currentURL.charAt(currentURL.length - 3);

    if(isNaN(lastChar) === true) {
        //Hide left button, hard code right button to go to ../2
        var leftContain = document.getElementById('leftcontainer');
        var rightContain = document.getElementById('rightcontainer');
        leftContain.style.display = "none";
        rightContain.style.width = "100%";
        rightContain.style.float = "none";
        lastURL = currentURL;
        nextURL = currentURL + "2/";
    } else if(isNaN(secondLastChar) === false) {
        //The page number is a double digit, simply derive last/next
        var twoRoot = currentURL.slice(0, -3);
        var twoSlash = currentURL.substr(currentURL.length - 3);
        var twoDigit = twoSlash.substr(0, 2) * 1;
        lastURL = twoRoot + (twoDigit - 1) + "/";
        nextURL = twoRoot + (twoDigit + 1) + "/";
   } else {
       //The page number is single digit, with a special case for page 2
        var oneRoot = currentURL.slice(0, -2);
        var oneDigit = +lastChar;
        if(onedigit == 2) {
            lastURL = oneRoot;
            nextURL = oneRoot + "3/";
        } else {
            lastURL = oneRoot + (oneDigit - 1) + "/";
            nextURL = oneRoot + (oneDigit + 1) + "/";
        }
    }
    document.getElementById("lastlink").href = lastURL;
    document.getElementById("nextlink").href = nextURL;
}

If I had to write the code for a personal project, I would not distinguish between 1 char and 2 chars ( what if there are over a thousands pages ? ), I would just get the number from the URL and build the buttons from that. The code in that case would be easier to follow as well (to me at least). ( http://jsfiddle.net/konijn_gmail_com/xNQ7m/ )

function hideLeftButton() {
    var leftContain = document.getElementById('leftcontainer');
    var rightContain = document.getElementById('rightcontainer');
    leftContain.style.display = 'none';
    rightContain.style.width = '100%';
    rightContain.style.float = 'none';
}

function linkSet() {
    var currentURL = location.href,
        parts = currentURL.split('/'),
        lastURL, nextURL;

    //Get rid of "" ( because URL ends with / )
    parts.pop();
    //Next part of the URL is the number ( or not )
    number = parts.pop();
    //Build up the root to append numbers to
    var root = parts.join('/') + '/';

    if (isNaN(number) === true) {
        hideLeftButton()
        lastURL = currentURL;
        nextURL = currentURL + '2/';
    } else if (number == 2) {
        lastURL = root;
        nextURL = root + '3/';
    } else {
        lastURL = root + (+number - 1) + '/';
        nextURL = root + (+number + 1) + '/';
    }
    document.getElementById('lastlink').href = lastURL;
    document.getElementById('nextlink').href = nextURL;
}


Early Binding with Generics I have a class which takes the method address and arguments, and executes it later when told to do so.

' need to turn option strict off due to Execute method executing late-bound code
Option Strict Off

Public Class WorkItem
    Private Action As Object
    Private Args() As Object

    Public Overloads Sub [Set](action As Action)
        SetArgs(action)
    End Sub

    Public Overloads Sub [Set](Of T)(action As Action(Of T), arg As T)
        SetArgs(action, arg)
    End Sub

    Public Overloads Sub [Set](Of T1, T2)(action As Action(Of T1, T2), arg1 As T1, arg2 As T2)
        SetArgs(action, arg1, arg2)
    End Sub

    '*** more overloads of [Set] method go here...

    Private Sub SetArgs(ByVal action As Object, ParamArray args() As Object)
        Me.Action = action
        Me.Args = args
    End Sub

    Public Sub Execute()
        '-- early binding doesn't work
        'DirectCast(Me.Action, Action(Of T)).Invoke(Args(0))

        '-- this works, but forces me to to keep option strict off
        Select Case Args.Length
            Case 0 : Me.Action.Invoke()
            Case 1 : Me.Action.Invoke(Args(0))
            Case 2 : Me.Action.Invoke(Args(0), Args(1))
            Case 3 : Me.Action.Invoke(Args(0), Args(1), Args(2))
            Case 4 : Me.Action.Invoke(Args(0), Args(1), Args(2), Args(3))
        End Select
    End Sub
End Class

Here is some tester code:

Public Class Form1
    Dim TheTask As WorkItem

    Private Sub Button1_Click(sender As Object, e As EventArgs) Handles Button1.Click
        TheTask = New WorkItem
        TheTask.Set(AddressOf DummyProc, TextBox1)
    End Sub

    Private Sub Button2_Click(sender As Object, e As EventArgs) Handles Button2.Click
        TheTask.Execute()
    End Sub

    Private Sub DummyProc(arg As TextBox)
        Threading.Thread.Sleep(1000)
        Debug.Print("work completed")
    End Sub
End Class

All this works with OPTION STRICT OFF

The WorkItem class obviously doesn't work with OPTION STRICT ON, due to the late-bound call in Execute method.

Is there any way I can convert the late-bound call to early binding?

You're working with delegates so the Action variable should be declared as Delegate. Please note that you need to enclose the type in square brackets as Delegate is a reserved word.
Don't use Dim in class-scoped variables, instead apply the access level.
Declare your arrays in a .net fashion. The type should be Object().
All non-public fields should be in lowerCamelCase.
There's no need to apply the Overloads modifier as you're not redeclaring any existing methods.

Apply all the fixes allows you to perform a dynamic invoke.

Public Class WorkItem

    Private action As [Delegate]
    Private args As Object()

    Public Sub [Set](action As Action)
        Me.SetArgs(action)
    End Sub

    Public Sub [Set](Of T)(action As Action(Of T), arg As T)
        Me.SetArgs(action, arg)
    End Sub

    Public Sub [Set](Of T1, T2)(action As Action(Of T1, T2), arg1 As T1, arg2 As T2)
        Me.SetArgs(action, arg1, arg2)
    End Sub

    Private Sub SetArgs(ByVal action As [Delegate], ParamArray args As Object())
        Me.action = action
        Me.args = args
    End Sub

    Public Sub Execute()
        Me.action.DynamicInvoke(Me.args)
    End Sub

End Class

Alternative solution

Another approach would be to wrap the delegate in a parameterless action.

Public Class WorkItem

    Private method As Action

    Public Sub [Set](Of T)(action As Action(Of T), arg As T)
        Me.method = New Action(Sub() action.Invoke(arg))
    End Sub

    Public Sub [Set](Of T1, T2)(action As Action(Of T1, T2), arg1 As T1, arg2 As T2)
        Me.method = New Action(Sub() action.Invoke(arg1, arg2))
    End Sub

    Public Sub [Set](Of T1, T2, T3)(action As Action(Of T1, T2, T3), arg1 As T1, arg2 As T2, arg3 As T3)
        Me.method = New Action(Sub() action.Invoke(arg1, arg2, arg3))
    End Sub

    Public Sub Execute()
        Me.method.Invoke()
    End Sub

End Class

For old compilers

Public Delegate Sub Action()
Public Delegate Sub Action(Of T)(ByVal obj As T)
Public Delegate Sub Action(Of T1, T2)(ByVal arg1 As T1, ByVal arg2 As T2)
Public Delegate Sub Action(Of T1, T2, T3)(ByVal arg1 As T1, ByVal arg2 As T2, ByVal arg3 As T3)

Public Class WorkItem

    Private Class Closure(Of T)

        Public action As Action(Of T)
        Public arg As T

        Public Sub Invoke()
            Me.action.Invoke(Me.arg)
        End Sub

    End Class

    Private Class Closure(Of T1, T2)

        Public action As Action(Of T1, T2)
        Public arg1 As T1
        Public arg2 As T2

        Public Sub Invoke()
            Me.action.Invoke(Me.arg1, Me.arg2)
        End Sub

    End Class

    Private Class Closure(Of T1, T2, T3)

        Public action As Action(Of T1, T2, T3)
        Public arg1 As T1
        Public arg2 As T2
        Public arg3 As T3

        Public Sub Invoke()
            Me.action.Invoke(Me.arg1, Me.arg2, Me.arg3)
        End Sub

    End Class

    Private method As Action

    Public Sub [Set](Of T)(ByVal action As Action(Of T), ByVal arg As T)
        Dim closure As New Closure(Of T)
        closure.action = action
        closure.arg = arg
        Me.method = New Action(AddressOf closure.Invoke)
    End Sub

    Public Sub [Set](Of T1, T2)(ByVal action As Action(Of T1, T2), ByVal arg1 As T1, ByVal arg2 As T2)
        Dim closure As New Closure(Of T1, T2)
        closure.action = action
        closure.arg1 = arg1
        closure.arg2 = arg2
        Me.method = New Action(AddressOf closure.Invoke)
    End Sub

    Public Sub [Set](Of T1, T2, T3)(ByVal action As Action(Of T1, T2, T3), ByVal arg1 As T1, ByVal arg2 As T2, ByVal arg3 As T3)
        Dim closure As New Closure(Of T1, T2, T3)
        closure.action = action
        closure.arg1 = arg1
        closure.arg2 = arg2
        closure.arg3 = arg3
        Me.method = New Action(AddressOf closure.Invoke)
    End Sub

    Public Sub Execute()
        Me.method.Invoke()
    End Sub

End Class


Preventing Division by Zero numerator: Value being divided.

denominator: Divisor value.

method so far:

def calc_percentage(numerator, denominator)
  ((numerator/ (denominator.to_f.nonzero? || 1 )) * 100)
end

What are the bad practices you see in the code above?  How can I improve it or write it in a better way ?

Well to me the bad thing is that the function produces an unexpected behavior.

You try to divide by 0, it divides by 1. Huh?

What I would do is throw an Argument Error Exception (or what ever other exception that you think is appropriate).

def calc_percentage(numerator, denominator)
  if denominator.to_f.nonzero
      then (numerator/ denominator.to_f) * 100
      else raise ArgumentError, "Denominator can not be 0.", caller
  end
end

This way it produces something that is logic and consistent.


Wavefront 3D Obj parser/viewer For learning Rust I decided to make a 3D .obj viewer. I wrote a simplified OBJ parser without materials that only supports triangles.

Considering I'm completely new to Rust and don't really know anyone who's good at idiomatic Rust I figured I'd ask here :)

use std::fs;

use crate::geometry::*;
use crate::object::Object;

fn str_to_vec3(s: &str) -> Option<Vec3> {
    let bits: Vec<&str> = s.split(" ").collect();
    if bits.len() != 3 {
        return None;
    }

    Some(Vec3 {
        x: bits[0].parse().unwrap(),
        y: bits[1].parse().unwrap(),
        z: bits[2].parse().unwrap(),
    })
}

fn str_to_vec2(s: &str) -> Option<Vec2> {
    let bits: Vec<&str> = s.split(" ").collect();
    if bits.len() != 2 {
        return None;
    }

    Some(Vec2 {
        x: bits[0].parse().unwrap(),
        y: bits[1].parse().unwrap(),
    })
}

type VertexDescription = (usize, usize, usize);
type TriangleDescription = (VertexDescription, VertexDescription, VertexDescription);

fn str_to_vertex_description(s: &str) -> Option<VertexDescription> {
    let bits: Vec<&str> = s.split("/").collect();
    if bits.len() != 3 {
        return None;
    }

    Some((
        bits[0].parse::<usize>().unwrap() - 1,
        bits[1].parse::<usize>().unwrap() - 1,
        bits[2].parse::<usize>().unwrap() - 1,
    ))
}

fn str_to_triangle_description(s: &str) -> Option<TriangleDescription> {
    let bits: Vec<&str> = s.split(" ").collect();
    if bits.len() != 3 {
        return None;
    }

    Some((
        str_to_vertex_description(bits[0]).expect("Failed to parse vertex"),
        str_to_vertex_description(bits[1]).expect("Failed to parse vertex"),
        str_to_vertex_description(bits[2]).expect("Failed to parse vertex"),
    ))
}

pub fn parse(path: String) -> Object {
    let object_string = fs::read_to_string(path).expect("Cannot read .obj file!");
    let lines = object_string.lines();
    let mut positions: Vec<Vec3> = vec![];
    let mut normals: Vec<Vec3> = vec![];
    let mut uvs: Vec<Vec2> = vec![];
    let mut triangle_descriptions: Vec<TriangleDescription> = vec![];

    for line in lines {
        match &line[0..2] {
            "v " => match str_to_vec3(&line[2..]) {
                Some(vec3) => positions.push(vec3),
                None => panic!("Malformed 'v' value!"),
            },
            "vn" => match str_to_vec3(&line[3..]) {
                Some(vec3) => normals.push(vec3),
                None => panic!("Malformed 'vn' value!"),
            },
            "vt" => match str_to_vec2(&line[3..]) {
                Some(vec2) => uvs.push(vec2),
                None => panic!("Malformed 'vt' value!"),
            },
            "f " => match str_to_triangle_description(&line[2..]) {
                Some(td) => triangle_descriptions.push(td),
                None => panic!("Malformed 'f' value!"),
            },
            _ => {}
        }
    }

    let triangles: Vec<Triangle3> = triangle_descriptions
        .iter()
        .map(|&td| {
            let v1: Vertex3 = Vertex3 {
                pos: positions[(td.0).0],
                uv: uvs[(td.0).1],
                normal: normals[(td.0).2],
            };
            let v2: Vertex3 = Vertex3 {
                pos: positions[(td.1).0],
                uv: uvs[(td.1).1],
                normal: normals[(td.1).2],
            };
            let v3: Vertex3 = Vertex3 {
                pos: positions[(td.2).0],
                uv: uvs[(td.2).1],
                normal: normals[(td.2).2],
            };
            (v1, v2, v3)
        })
        .collect();

    let mut obj = Object::new();
    obj.triangles = triangles;
    obj
}

Is this a good approach? What can I improve on?

Any feedback is welcome!

fn str_to_vec3(s: &str) -> Option<Vec3> {
    let bits: Vec<&str> = s.split(" ").collect();
    if bits.len() != 3 {
        return None;
    }

    Some(Vec3 {
        x: bits[0].parse().unwrap(),
        y: bits[1].parse().unwrap(),
        z: bits[2].parse().unwrap(),
    })

}

For some errors, you panic (if parsing fails) and for other errors you return None. You really should pick one strategy and stick with it. If you want to avoid panicing it is more idiomatic to return Result than Option when it is an error.

Much of your code repeats the same basic action: split/parse/return. You can combine some of the code with a function like this:

fn split_parse<T: std::str::FromStr>(text: &str, split: &str) -> Result<Vec<T>, T::Err> {
    text.split(split).map(str::parse).collect()
}


K&R C Exercise 1-14: Histogram of the frequency of types of characters This is for K&R C Exercise 1-14: basically asking to create a program that prints a histogram of the frequency of types of characters. This is done with very basic C functions/knowledge as covered in the first chapter.

I feel like this code as it stands is pretty clunky with all the for loops to print (though it does function!) Is there a simple way to reduce the number of loops (and also the loop variables) in a more concise way?

#include <stdio.h>

int main()
{
    int c, i, j, k, l, h, nwhite, nother;
    int ndigits[10];

    nwhite = nother = 0;

    for (i = 0; i < 10; ++i)
        ndigits[i] = 0;

    while ((c = getchar()) != EOF)
    {
        if (c >= '0' && c <= '9')
            ++ndigits[c-'0'];
        else if (c == '\n' || c == '\t' || c == ' ')
            ++nwhite;
        else
            ++nother;
    }
    for (j = 0; j <= nwhite; ++j)
    {
        if (nwhite - j > 0)
            putchar('*');
    }
    putchar('\n');
    for (k = 0; k <= nother; ++k)
    {
        if (nother - k > 0)
            putchar('*');
    }
    putchar('\n');
    for (i = 0; i < 10; ++i)
    {
        for (h = 0; h < ndigits[i]; ++h)
        {
            if (ndigits[i] - h > 0)
                putchar('*');
        }
        putchar('\n');
    }
}

Nice job on your code. I only have a few things to say along with some extra nit-picks at the end.

Refactor

I see this construct a lot in your code:

for (j = 0; j <= nwhite; ++j)
{
    if (nwhite - j > 0)
        putchar('*');
}
putchar("\n");

Of course, different variables are used every time, but the concept is the same. You could actually simplify your code if you extracted this logic to a separate function. Here is what I mean:

void print_stars(limit) {
    for(int i = 0; i < limit; i++) {
        if(limit - i > 0) {
            putchar("*");
        }
    }
    putchar("\n");
}

Now, you can just use this function every time you come across this construct:

print_stars(nwhite);
print_stars(nother);
for (i = 0; i < 10; ++i)
{
    print_stars(ndigits[i]);
}

Nitpicks

That refactoring was really all I had to say about your code. These are just extra (yet important) things.

Always use braces. I notice that you've omitted braces on some if statements, and that's probably because K&R does that a lot. In actual code, this is bad practice because it can lead to hard-to-find yet easy-to-get bugs.
You can simplify initializing arrays with 0s with this:

int ndigits[10] = {0};

There is no need for an extra loop to do this.


Fetching specific foreign exchange rates from fixer using curl and jsconcpp in C++ I am trying to create my own algorithmic trading system using C++. I have searched the web for a nice tutorial for such systems and I didnt find any. Then I started to learn about curl and jsoncpp libraries and even documentation of those libraries are not made for a novice programmer like me. I mostly reused most of the codes for creating this scrapper especially WriteCallback function which I directly copy pasted from another c++ code without understanding it. I am sharing this whole code because some one else dosent need to reinvent the wheel and can use this code as a template.Any way this code is working properly (Ubuntu 14.04, CodeBlocks IDE, API: fixer.io) and any kind of suggestion or feedback is welcomed.

#include <string.h>
#include <iostream>
#include <stdio.h>
#include <jsoncpp/json/json.h>
#include <curl/curl.h>

//writing call back function for storing fetched values in memory
static size_t WriteCallback(void *contents, size_t size, size_t nmemb, void *userp)
{
    ((std::string*)userp)->append((char*)contents, size * nmemb);
    return size * nmemb;
}

int main(void)
{
  std::string readBuffer;

 //global initiliation of curl before calling a function
 curl_global_init( CURL_GLOBAL_ALL );

 //creating session handle
 CURL * myHandle;

 // We’ll store the result of CURL’s webpage retrieval, for simple error checking.
 CURLcode result;

 // notice the lack of major error-checking, for brevity
 myHandle = curl_easy_init ( ) ;

 //after creating handle we ill start transfering webpage

 //curl_easy_setopt is used to tell libcurl how to behave.
 //By setting the appropriate options, the application can change libcurl's behavior.

 //CURLOPT_URL provide the URL to use in the request. Pass in a pointer to the URL to work with.
 //sample json output >> {"base":"EUR","date":"2016-07-22","rates":{"GBP":0.84108,"USD":1.1014}}
 curl_easy_setopt(myHandle, CURLOPT_URL, "http://api.fixer.io/latest?symbols=USD,GBP");

   /* send all data to this function  */
  curl_easy_setopt(myHandle, CURLOPT_WRITEFUNCTION, WriteCallback);

   /* we pass our 'chunk' struct to the callback function */
  curl_easy_setopt(myHandle, CURLOPT_WRITEDATA, &readBuffer);

 //perform a blocking file transfer
 result = curl_easy_perform( myHandle );

 /* check for errors */
  if(result != CURLE_OK) {
    fprintf(stderr, "curl_easy_perform() failed: %s\n",
            curl_easy_strerror(result));
  }
  else {
    /*
     * Now, our chunk.memory points to a memory block that is chunk.size
     * bytes big and contains the remote file.
     *
     * Do something nice with it!
     */

    //std::cout << readBuffer << std::endl;

    //json parsing
Json::Value root;   // will contains the root value after parsing.
Json::Reader reader;
bool parsingSuccessful = reader.parse( readBuffer, root );
if(not parsingSuccessful)
 {
   // Report failures and their locations
   // in the document.
   std::cout<<"Failed to parse JSON"<<std::endl
       <<reader.getFormatedErrorMessages()
       <<std::endl;
   return 1;
 }else{
 std::cout<<"\nSucess parsing json\n"<<std::endl;
 std::cout << root<< std::endl;
 std::cout <<"Base = "<< root["base"].asString() << std::endl;
 std::cout <<"Date = "<< root["date"].asString() << std::endl;
 std::cout <<"Rate GBP ="<< root["rates"]["GBP"].asFloat() << std::endl;
 std::cout <<"Rate USD ="<< root["rates"]["USD"].asFloat() << std::endl;

 }

  }

 //End a libcurl easy handle.This function must be the last function to call for an easy session
 curl_easy_cleanup( myHandle );

 return 0;
}

These are C headers

#include <string.h>
#include <stdio.h>

You should probably use the C++ version (I hope you are not using C-String anywhere).

#include <strings>
#include <cstdio>

Header include paths

#include <jsoncpp/json/json.h>

This should probably be json/json.h and you should specify the location where jsoncpp is installed with compiler flags.

Curl is a C library.

Therefore any callbacks should be C functions (not C++ functions). There is no guarantee in the standard thay they use the same ABI.

static size_t WriteCallback(void *contents, size_t size, size_t nmemb, void *userp)

Should be:

extern "C" size_t WriteCallback(void *contents, size_t size, size_t nmemb, void *userp)

Never use C-Casts

C casts are hard to spot (in general). But also using C++ casts you can mark dangerious casts by uisng the appropriate cast.

    ((std::string*)userp)->append((char*)contents, size * nmemb);

I would use reinterpret_cast<> here. So that it is easy to spot and you intuitively know that this is a dangerous operation.

reinterpret_cast<std::string*>(userp)->append(static_cast<char*>(contents), size * nmemb);

Always check error codes.

All the following return an error code. You should validate these functions worked by checking this error code.

 curl_global_init( CURL_GLOBAL_ALL );
 curl_easy_setopt(myHandle, CURLOPT_URL, "http://api.fixer.io/latest?symbols=USD,GBP");
 curl_easy_setopt(myHandle, CURLOPT_WRITEFUNCTION, WriteCallback);
 curl_easy_setopt(myHandle, CURLOPT_WRITEDATA, &readBuffer);

Prefer "\n" over std::endl

 std::cout<<"\nSucess parsing json\n"<<std::endl;
 std::cout << root<< std::endl;
 std::cout <<"Base = "<< root["base"].asString() << std::endl;
 std::cout <<"Date = "<< root["date"].asString() << std::endl;
 std::cout <<"Rate GBP ="<< root["rates"]["GBP"].asFloat() << std::endl;
 std::cout <<"Rate USD ="<< root["rates"]["USD"].asFloat() << std::endl;

The difference is that std::endl also calls std::flush on the stream. There is no need to flush the stream manually. The stream is designed to flush when it needs too. When you do it manually you will make your code much more eneficent.

RAII

 myHandle = curl_easy_init ( ) ;
 // STUFF
 curl_easy_cleanup( myHandle );

When you see this pattern. You should be thinking of using RAII. This will make sure your resources are cleaned up correctly in all situations (including when exceptions are thrown) are the function returns early (aka return 1;).


Changing a menu tab's color on click I've got a tab menu and functions that change the tab's color on click, giving the clicked tab a different color from the others. The thing is, when I click one, I give it another background-color easily but I'm also setting all the other tabs background-color.

In my largest menu there are 6 tabs:

document.getElementById('tab1').onclick = changeColor1;
document.getElementById('tab2').onclick = changeColor2;
document.getElementById('tab3').onclick = changeColor3;
document.getElementById('tab4').onclick = changeColor4;
document.getElementById('tab5').onclick = changeColor5;
document.getElementById('tab6').onclick = changeColor6;

function changeColor1() {
    document.getElementById('tab1').style.backgroundColor = "white";
    document.getElementById('tab2').style.backgroundColor = "silver";
    document.getElementById('tab3').style.backgroundColor = "silver";
    document.getElementById('tab4').style.backgroundColor = "silver";
    document.getElementById('tab5').style.backgroundColor = "silver";
    document.getElementById('tab6').style.backgroundColor = "silver";
    return false;
}

function changeColor2() {
    document.getElementById('tab1').style.backgroundColor = "silver";
    document.getElementById('tab2').style.backgroundColor = "white";
    document.getElementById('tab3').style.backgroundColor = "silver";
    document.getElementById('tab4').style.backgroundColor = "silver";
    document.getElementById('tab5').style.backgroundColor = "silver";
    document.getElementById('tab6').style.backgroundColor = "silver";
    return false;
}

...
...

... and it goes on for the 6 tabs. I'm sure this can be reduced, I just don't know how.

I would suggest changing your html and giving all the tags some class like 'tag', then you'll be able to do something like:

<button id="tab1" class="tab" onClick="changeColor(this.id)">B1</button>
<button id="tab2" class="tab" onClick="changeColor(this.id)">B2</button>
<button id="tab3" class="tab" onClick="changeColor(this.id)">B3</button>
<script type="text/javascript">
    function changeColor(id) {
        var tabs = document.getElementsByClassName('tab')
        for (var i = 0; i < tabs.length; ++i) {
            var item = tabs[i];
            item.style.backgroundColor = (item.id == id) ? "white" : "silver";
        }
    }
</script>

see fiddle - http://fiddle.jshell.net/vP7ku/

must shorter, Hua?


Finding pair of sum in sorted array In a sorted array, I am trying to find pairs that sum up to a certain value. I was wondering if anyone could help me improve my code in performance or memory. I believe my code is O(n). If there are other method for finding pairs the sum up to a certain value that is much more efficient, I am open to these methods!

static int[] intArray = {0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20};

public static ArrayList<String> pairSum(int[] j, int sum) {
    int len = j.length;
    int pointer1;
    int pointer2;
    ArrayList<String> storage = new ArrayList<String>();

    for (int i = 0; i < len; i++) {
        pointer1 = j[i];
        for (int k = len - 1; k >= 0; k--) {
            pointer2 = j[k];
            if (pointer1 + pointer2 == sum && pointer1 != pointer2) {
                i++;
                String pair = Integer.toString(pointer1) + " and " + Integer.toString(pointer2) ;
                storage.add(pair);
            }
        }
    }
    return storage;
}

If I ran this code, say pairSum(intArray,16), I would get:

[0 and 16, 2 and 14, 4 and 12, 6 and 10, 9 and 7, 11 and 5, 13 and 3, 15 and 1]

If I remove the i++, the output would be:

[0 and 16, 1 and 15, 2 and 14, 3 and 13, 4 and 12, 5 and 11, 6 and 10, 7 and 9, 9 and 7, 10 and 6, 11 and 5, 12 and 4, 13 and 3, 14 and 2, 15 and 1, 16 and 0]

EDIT: Please see update at end because there is an improved solution that is inspired by Mike Chamberlain's answer...

There are two solutions which come to mind for your problem, but first, let's look through your code:

you have the variables pointer1 and pointer2. These are very descriptive names, which is great, but unfortunately they are not pointers. They are the values at the i and k positions in the array. In other words, i and k are the actual pointers. I would call pointer1 and pointer2 something like val1 and val2. But, in fact, I would drop the pointers entirely, and just use an index in to the data (why copy the data when you don't need to....? )
In most languages the variable name i is 'reserved' for indexed looping through data, just like you have for (int i = 0; i < len; i++) {...}. It is standard to use j and k as the variable names for any inner/nested loops you need. i is the top level, j is the first nested level, k is the subsequent level, and, if you need more than that, you should be calling a function/method (Never use l as a variable name, it is too easy to confuse with 1). You are breaking this convention by using j as an array, and going immediately to k inside your loop. My suggestion is to rename j to be data, and to rename k to be j.
as a general rule, when you have a loop constrained by an index, like you have for (int i = 0; i < len; i++) { ..., you should never modify the index inside the loop. I cannot think of a reason why it should ever be necessary, and, certainly, it should never be conditionally modified (inside an if-statement inside another loop). This will lead to bugs. In fact, the simple fact that you have that inner i++ tells me you do not understand your own code's behaviour.
Your method should return the interface type List<String> instead of the concrete class ArrayList<String> since there is no reason for outsiders to know the physical implementation.
storage is not a great name for a variable.... everything could be called storage since all variables store things... I would choose something like: results.
Your inner if condition makes sure that the sums are a match, but also checks pointer1 != pointer2. This second check is a problem, because it should not be testing the values, but the indexes.... For example what if the data is [4,4] and the target sum is 8?
To get your result pair as a String you do: String pair = Integer.toString(pointer1) + " and " + Integer.toString(pointer2).... this is serious overkill, try this instead: String pair = pointer1 + " and " + pointer2. In Java the + String operator will implicitly convert the integers to a String... no need to do it yourself.

So, you have some style problems, and some concerns about your inner workings.

Now, let's talk about your algorithm..... It loops through each value. For each value, it then compares it against every other value (except itself). So, you loop n times, and, for each n you do m checks. Your complexity is O(n x m), but, in reality, n and m are the same, so your complexity is O(n2)

Now, about that i++ inside the k loop.... the reason you need it is because your k loop is starting from the wrong place. your algorithm is wrong. Let me explain.

If you have the data [ 0, 1, 2, 3, 4, 5 ] , you are using your i index to loop through all the data. Your first data will be 0. You sum this with 5 (Note, you have summed 0 + 5 now), and check the result, then with 4, and so on. When you are done, you move i to the value 1, and you keep moving i until you get to the value 5 for i. You then compare **this value 5 to all the other values, including 0. This is **the second time you sum the items 0 + 5  (but it is now 5 + 0) **.

You need to change the algorithm to only scan those values that have not yet been scanned. The easiest way to do this is to modify your inner/nested loop. Consider your code:

   for (int i = 0; i < len; i++) {
       ...
       for (int k = len - 1; k >= 0; k--) {
           ...

Now, we change the inner loop as follows:

for (int i = 0; i < len; i++) {
    ....
    for (int j = i + 1; j < len; j++) {

(note, I have renamed k to be j). We start our j index at i + 1, because we don't need to sum values before i since that sum was done already when i was smaller.....

Now, our loops only calculate the sum value once for each pair of values.

Unfortunately, our complexity is still O(n2) because even though our inner loop is only testing half (on average) the values, we have not actually changed the way the algorithm scales (if you double the input data, you with quadruple the execution time).

Still, even with this algorithm change, and some variable renaming, and some bug fixing, your code will look like:

public static List<String> pairSum(int[] data, int sum) {
    int len = data.length;
    int val1;
    int val2;
    List<String> results = new ArrayList<String>();

    for (int i = 0; i < len; i++) {
        val1 = data[i];
        for (int j = i + 1; j < len; j++) {
            val2 = data[j];
            if (val1 + val2 == sum) {
                String pair = val1 + " and " + val2;
                results.add(pair);
            }
        }
    }
    return results;
}

This is OK, but my preference would be to strip all the val1 variables entirely, and you can simplify some other things too. Consider the following more 'compact' code:

public static List<String> pairSum(int[] data, int sum) {
    List<String> results = new ArrayList<String>();

    for (int i = 0; i < data.length; i++) {
        for (int j = i + 1; j < data.length; j++) {
            if (data[i] + data[j] == sum) {
                results.add(data[i] + " and " + data[j]);
            }
        }
    }

    return results;
}

That is what I would recommend for a simple, easy-to-read option that is O(n2). For small data sets (say less than 50 or so), this solution will be great.

But, if you have more data, a better solution would be more complicated, but faster.

In your example code, you have an input test array that is all positive, sorted, and the values are unique. Let's assume that all the data will be this way (we can modify this assumption later).

Consider the algorithm that does:

take all values that are less than half the target `sum`.
find a value that is the difference

It would look like:

public static List<String> pairSumFast(int[] data, int sum) {
    List<String> results = new ArrayList<String>();

    int limit = sum / 2;
    for (int i = 0; i < data.length && data[i] <= limit; i++) {
        int match = Arrays.binarySearch(data, i + 1, data.length, sum - data[i]);
        if (match >= 0) {
            results.add(data[i] + " and " + data[match]);
        }
    }

    return results;
}

This has the complexity of O( nlog(n) ) for the loop, and the binary-search respectively.

If your data set is large, and the values are unique, but the data is unsorted, then I would recommend the change:

public static List<String> pairSumFast(int[] indata, int sum) {
    List<String> results = new ArrayList<String>();

    int[] data = Arrays.copyOf(indata, indata.length);
    Arrays.sort(data);

    int limit = sum / 2;
    for (int i = 0; i < data.length && data[i] <= limit; i++) {
        int match = Arrays.binarySearch(data, i + 1, data.length, sum - data[i]);
        if (match >= 0) {
            results.add(data[i] + " and " + data[match]);
        }
    }

    return results;
}

This is O(n log(n) ) for the complexity still... (it scales this way, even through it does more computation....)

Finally, if the data is not unique, then you will get duplicate output values. To accommodate that, I would add an inner loop to the match....:

public static List<String> pairSumFast(int[] data, int sum) {
    List<String> results = new ArrayList<String>();

    int limit = sum / 2;
    for (int i = 0; i < data.length && data[i] <= limit; i++) {
        int match = Arrays.binarySearch(data, i + 1, data.length, sum - data[i]);
        if (match >= 0) {
            String pair = data[i] + " and " + data[match];
            results.add(pair);
            // binary search will return the index of one match, look around to find others.
            int more = match - 1;
            while (more >=0 && data[more] == data[match]) {
                results.add(pair);
                more--;
            }
            more = match + 1;
            while (more < data.length && data[more] == data[match]) {
                results.add(pair);
                more++;
            }
        }
    }

    return results;
}

EDIT: A better solution...

Mike Chamberlain suggests using a Map to preserve the values you have seen in order to match the pairs in a single O(n) iteration of the list.

This single-iteration made me realize that there is a much simpler solution to this problem... iterate from both sides of the data.... consider the following:

public static List<String> pairSumFastest(int[] data, int sum) {
    ArrayList<String> results = new ArrayList<String>();

    int low = 0;
    int high = data.length - 1;
    while (low < high) {
        while (low < high && data[low] + data[high] < sum) {
            low++;
        }
        while (low < high && data[low] + data[high] > sum) {
            high--;
        }
        if (low < high) {
            results.add( data[low] + " and " + data[high]);
            low++;
            high--;
        }
    }

    return results;
}

This has true O(n) complexity, and has no memory overhead either.


Force params to lowercase I think there must be a more pythonic way to achieve the lowercase-forcing that I need to do here.  The keys in my lookup (self.adjustments) are all lc, params sent in to function can be any case (data coming from third party).  Also, is it objectionable to have self.adjustments used here but not passed in?

def getRevenue(self, metric, productParam, platformParam):
    product = productParam.lower()  # better way to do this?
    platform = platformParam.lower()    # better way to do this?
    # get adjustment for product.platform, multiply by metric
    if product not in self.adjustments:
        raise Exception('Unknown adsense product: {}'.format(product))

If these parameters may not always be lowercase,
then I don't think there's a better way.

However, if you have many callers that are guaranteed to use lowercase parameters,
and it's a pity to call .lower() for those,
then you could split this into 2 functions:

One for callers that are guaranteed to use cleaned parameters (lowercased parameters)
One for untrusted callers

For example:

def _get_revenue(self, metric, product, platform):
    # get adjustment for product.platform, multiply by metric
    if product not in self.adjustments:
        raise Exception('Unknown adsense product: {}'.format(product))

def get_revenue(self, metric, product, platform):
    self._get_revenue(metric, product.lower(), platform.lower())

In the examples I used snake_case naming for method names,
as recommended by PEP8.

By the way it seems a bit odd that the function name starts with "get" but it doesn't return anything.

Without seeing the full code,
I don't see a reason to object against the use of self.adjustments.
Whether it's objectionable or not really depends on the rest of the code.

Something to object against though is the platform param which is not used by this method.
Ideally it shouldn't be there at all.


Identify possible pair of factors from a product This is my first C++ program (not counting "hello world") and I'd like to know if there are some semantic mistakes or if I could rewrite the code in a more simple way.

It performs brute-force factorisation of the number provided on standard input.

Here is the code:

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

int main() {
    int f1 = 0;
    int f2 = 0;
    int p;
    int increment = 1;
    cin >> p;
    if ((f1 && f2) == p) {
        cout << f1 << "x" << f2 << "=" << p;
    }
    else if (p < 00) {
        cout << "Insert a positive number";
    }

    while (
        ( (f1 && f2) != p) && (f1 < p) && (f2 <= p) ) {

        f1++;
        if (f1 * f2 == p) {
            cout << f1 << "x" << f2 << "=" << p << "\n";
        }
        f2 = increment;
        if (f1 == p) {
            increment++;
                f1 = 0;
        }
    }
    system("pause");
        return 0;
    }

Thanks for your feedback!

Don't using namespace std; - this namespace isn't designed for wholesale import like that.

Import <cstdlib> to declare std::system() - though note that almost any use of this function makes your code highly platform-specific (that just gives a sh: pause: not found error here, for example).

Never ignore the return value from std::system or from streaming with >>.

Error messages should go to std::cerr and end with a newline (and we should return early in the error case, rather than continuing to the while loop).

if ((f1 && f2) == p) is a highly unconventional way of writing if (p == 0).  Don't make it unnecessarily hard to read.

In the while condition, (f1 && f2) != p) can only be false when p is 1, so just treat that case separately, before the loop.

Prefer pre-increment (++x) to post-increment (x++) in C++ code when not using the result.  Here, with integers, the compiler can optimize to the same code, but with more complex types, that's not always possible.


Find sum of 3 that total a target from a List A related Java question got me curios.

All unique combinations (not permutations) of 3 values that sum to a target from a list of integers.

Values can duplicate in the list but are only used once.

By sorting the input the evaluation is able to take shortcuts.

Feedback on both code and speed please.

Assume all input and sum >= 0.

public static List<List<int>> FindThreeSum(List<int> input, int sum = 24)
{
    //cannot have default on a list to my knowledge
    if(input.Count < 3)
    {
        input = new List<int> { 8, 12, 6, 18, 4, 3, 8, 1, 6, 3, 8, 9, 0 };
        //in real life throw an error
    }
    List<int> sortedInput = input.OrderBy(x => x).ToList();
    Debug.WriteLine(string.Join(", ", sortedInput));
    int sortedCount = sortedInput.Count;
    int maxInput = sortedInput[sortedCount - 1];
    List<List<int>> findSum = new List<List<int>>();
    if (3 * (long)maxInput < (long)sum || sortedInput[0] < 0 || sum < 0 || 3 * sortedInput[0] > sum)
    {
        return findSum;
    }
    int sumSoFar;
    int sumI = int.MaxValue;
    int sumJ = 0;
    int sumK = 0;
    for (int i = 0; i < sortedCount - 2; i++)
    {
        if(sortedInput[i] == sumI)
        {
            continue;
        }
        sumI = sortedInput[i];
        if(3 * sumI > sum)
        {
            break;  //sumSoFar is only going to get bigger
        }
        for (int j = i + 1; j < sortedCount - 1; j++)
        {
            if (sortedInput[j] == sumJ)
            {
                continue;
            }
            sumJ = sortedInput[j];
            sumSoFar = sumI + sumJ;
            if (sumSoFar + sumJ > sum)
            {
                break;
            }
            else if(sumSoFar + maxInput < sum)
            {
                continue;
            }
            for (int k = j + 1; k < sortedCount; k++)
            {
                if (sortedInput[k] == sumK)
                {
                    continue;
                }
                sumK = sortedInput[k];
                sumSoFar = sumI + sumJ + sumK;
                if (sumSoFar > sum)
                {
                    break;
                }
                else if(sumSoFar == sum)
                {
                    findSum.Add(new List<int> { sumI, sumJ, sumK });
                    Debug.WriteLine($"{sumI} {sumJ} {sumK}");
                }
            }
        }
    }
    Debug.WriteLine("daDone");
    return findSum;
}

In general it looks OK to me, but you could maybe consider the following:

1) Return a IEnumerable<int[]> instead of List<List<int>> and then yield the positive results when found:

            ...
            else if (sumSoFar == sum)
            {
              yield return new int[] { iValue, jValue, kValue };
            }...

2) The names sumI, sumJ, sumK are somewhat misleading because they aren't sums. Better names would be valueI, -J, -K

  int valueI;
  int valueJ;
  int valueK;

3) For the fun of it, you could consider: Because you basically do the same same loop nested three times, it could be a candidate for a recursive function iterating over each addend and yielding all the positive sums. In that way you could generalize the algorithm to handle any number of addends...


Find number repeated odd times in array I completed the following exercise:

  You are given an array of repeating numbers. All numbers repeat in an even way, except for one. Find that odd occurring number.

function findOdd(arr){
  obj = {};

  for (var el in arr) {
    if (!obj[arr[el]]) {
      obj[arr[el]] = 1;
    } else {
      obj[arr[el]]++;
    }
  }

  for (var key in obj) {
    if(obj[key] % 2 === 1){
      obj = {'odd': key}
    }
  }
  return parseInt(obj['odd']);
}

findOdd([1,1,2,4,4,4]);

I am trying to figure out if there are better ways to implement this. A few things come to mind:

I believe I can nest the for loops together. I'm not sure if this saves any space or is preferable styling wise.
I re-assigned the created object in the function so it just holds the result. I figured it's a bit more readable. Should I have just returned it in the second for loop?
The code seems 'dirty' to me. What might be another possible solution?

Yes dirty, buggy, and bad.

There are several problems with your code. You have undeclared variables, do not protect against iterating over unintended enumerable array properties, redundant code, inappropriate object types, and unneeded iteration.

Undeclared variable.

You use the variable obj without declaring it. It is thus created in global scope and available to any function. This can result in difficult to spot bugs and unexpected behaviour.

Always on the first use of a variable declare it with one of JavaScript's variable types var, let, or const. In this case I will not say which type to use as the variable obj is badly used and inappropriate for the situation.

Use strict

To save you from ever make this mistake again you should add to the first line of any javascript file or script tag the line

"use strict";

this will enforce some stricter parsing standards and help you avoid making mistakes such as "forgetting to declare". It also has the benefit of running your code quicker.

Which way to iterate.

There are many ways in javascript to iterate over a set of items in arrays, objects, iterable objects, and array like objects. Using built in statements for, for...in, for...of, while, do...while, and more

The one you use is about the worst for the situation. The problem with for...in is that it will iterate over all the arr/object enumerable properties, including properties that are inherited via the object's prototype.

This means you can get almost anything for each iteration, and thus need to check every item to see if it is what you intend to be handling, and if it belongs to the object or has it be inherited from another. This is just a pain, so much so I never use it.

For your code you should have used

for(const el of arr){

Which does what you intended without the risk of getting unknown properties.

Dont burn cycles

Good code is efficient code, with every instruction the computer executes in your code, you burn power, sucked from batteries, the grid, You chew the clients time and you extract a little more from the environment we share as a comunity.

Yes your code is trivial in the scheme of things, immeasurable, but combined millions of programmes, or with fortune on your side an app you write  may end up on billions of devices, and particular parts of your code may execute trillions of times a minute. Then it matters.

Always (I digress, no rant... :P) code for efficiency, even the little bits combined matter.

Your code could have exited early as the problem states that there is only one set of odd numbers in the array. So as soon as you find the odd set, the job is over and you should exit. On average you will do a quarter as much processing overall. On average you could do up to a quarter less processing overall.

Use appropriate Objects.

You are using obj as a map, for each unique key (number) you create an entry and use the associated value to count the number of keys. Though technically not wrong javascript provides a object Map that is better suited (see rewrite)

The rewrite

If the array of numbers was known to be sorted or numbers were grouped then a different solution could be used that is more efficient, But as there is no mention of the array being sorted or numbers grouped apart from the sample data the best is to play safe and assume unsorted data.

Some may opt to sort the array to take advantage of the better solution, but the sort is expensive and would offset any benefits the more efficient method would give.

UPDATE I miss read the question and thought that counts of one were to be excluded. I have changed the code via commenting out the incorrect behaviour.

function findOdd( arr ) {
    const counts = new Map();
    // find each unique number and count the entries
    for (const num of arr) {
        const item = counts.get(num); // get entry with key = num
        if (item !== undefined) {     // does it exist
            counts.set(num, item + 1);          // yes count one more
        } else {
            counts.set(num, 1);       // no so add and set count at 1
        }
    }
    // all items in the array have been checked so find the first with odd count
    for (const [num, count] of counts) {
        if (/*count > 2 &&*/ count % 2) { // See update in answer re comment
            return num; // we have found what we are looking for
        }
    }
    // the problem suggests that there is always an odd set so this
    // line will never be reached.
}

Sorted version

Just for the exercise I have included the more efficient code that assumes the array of numbers is sorted or that numbers are in groups.

function findOdd (arr) {
    var i = arr.length - 1;
    var count = 1;
    var prev = arr[i];
    while (i--) {
        const current = arr[i];
        if (current !== prev) {
            if (count % 2) { return prev }  // Update remove test for count > 2
            count = 1;
        } else { count += 1 }
        prev = current;
    }
}

Which on average executes in 1/3rd the time of the other method


Simple Python script to sort and make json data unique I'm retrieving e-mails from a collection of a few hundred *.json files using the following:

Python Script

import os, json

ordersDirectory = "C:\dev\orderArchive"
eMails = []
numUniqueEmails = 0
numEmails = 0

# json objects stored in multiple files, itterate over all of them
for filename in os.listdir(ordersDirectory):
    with open(ordersDirectory + '\\' + filename) as data_file:
        data = json.load(data_file)
        eMails.append(data["details"]["email"])
        numEmails +=1

# create and sort unique list of e-mails
uniqueEmails = sorted(list(set(eMails)))

print("Comma dilimited e-mails: \n")
for email in uniqueEmails:
    # print e-mails to console (comma delimited)
    numUniqueEmails += 1
    print(email + ",", end= " ")

print("\n\nNumber of unique e-mails is: " + str(numUniqueEmails) + " (originally " + str(numEmails) + ")")

Example JSON:

{
   "details":{
      "firstName":"Tester",
      "surname":"Testerman",
      "email":"test@test.com"
      },
   "orders":[
      {
         "name":"A test order",
         "quantity":20
      },
      {
         "name":"Testable data",
         "quantity":20
      }]
}

Example output

Assuming that 30 files were processed and there were 3 unique e-mail addresses among them.

  Comma dilimited e-mails:

  test@test.com, test2@test.com, emailhereagain@test.com,

  Number of unique e-mails is: 3 (originally 30)

I know it's not very functional however I'm a C# developer primarily - I can get the job done in Python (and had to quickly for this task) but I'd love to know how to think more "pythonic". Any feedback or suggestions about style or performance are also appreciated.

I'd recommend making use of a set, which stores only unique elements. That way you can just count insertions to get the total (with duplicates) and the unique ones are easy to retrieve.

Instead of iterating through unique emails and printing them with a comma and a space as a terminator (which gives you the odd comma and space at the end of the emails), make use of ", ".join().

Lastly, eMails should be emails in terms of punctuation, constants should be all caps, and camelCase is discouraged. For clarity, I changed the variable names to be more expressive.

Here is my complete solution:

import os, json

ORDERS_DIRECTORY = "C:\dev\orderArchive" # Reformatted constant.
unique_emails = set() # Holds only unique emails by design.
total_emails = 0 # Boring running total

# json objects stored in multiple files, itterate over all of them
for filename in os.listdir(ORDERS_DIRECTORY):
    with open(os.path.join(ORDERS_DIRECTORY, filename)) as data_file:
        data = json.load(data_file)
        unique_emails.add(data["details"]["email"]) # Make use of set here.
        total_emails += 1 # Corrected spacing here.

print("Comma dilimited e-mails: \n")
print(", ".join(unique_emails)) # Make use of join here.
print("\n\nNumber of unique e-mails is: {} (originally {})".format(len(unique_emails), total_emails))

On a side note, you may want to consider having some more error handling for the email field not being there, file not containing json etc. I'd also consider making the directory a program argument.

Update: fixed ORDERS_DIRECTORY consistency, use of os.path.join(), and str.format() per comments.


QuickSort C# Implementation It has been a while since I've implemented a QuickSort and I wanted to write it down like I remembered.

int MyPartition(List<int> list, int left, int right)
{
    int pivot = list[left];

    while(true)
    {
        while(list[left] < pivot)
            left++;

        while(list[right] > pivot)
            right--;

        if(list[right] == pivot && list[left] == pivot)
            left++;

        if(left < right)
        {
            int temp = list[left];
            list[left] = list[right];
            list[right] = temp;
        }
        else
        {
            return right;
        }
    }
}

void MyQuickSort(List<int> list, int left, int right)
{
    if(list == null || list.Count <= 1)
        return;

    if(left < right)
    {
        int pivotIdx = MyPartition(list, left, right);

        if(pivotIdx > 1)
            MyQuickSort(list, left, pivotIdx - 1);

        if(pivotIdx + 1 < right)
            MyQuickSort(list, pivotIdx + 1, right);
    }
}

Sample usage:

var list = RandomList(1000);
MyQuickSort(list, 0, list.Count - 1);

With QuickSort, the complexity always comes down to two things:

how you deal with equal numbers (duplicates)
which index you use to return from the partition (left or right)

You need to decide up-front whether the pivot value is going to be in the left partition, or the right partition. If the pivot is going to be in the left, then you need to return left index, and the left partition is values <= the pivot.

In my 'education', all the examples I looked at, and all the implementations I have done since then, have always returned the left index.... and they have always included the pivot in the left side.

There are a number of things that stand out to me in your code:

you do not start with the left at left + 1 (you know that list[left] == pivot for the first check...)
It is common practice for the 'right' index to start at the length (i.e. to be 1 after the last member).
you have odd handling for the duplicate value case
you are returning right instead of left
you are not swapping the pivot to where it belongs.... The point of the partitioning is that you are supposed to end up with the pivot value where it belongs.
you are not checking for left >= right on the increment side of the partition loops

The odd handling of duplicates is because the 'left' loop should be a <= loop, not a < loop:

        while(list[left] <= pivot)
            left++;

and you should get rid of the condition:

        if(list[right] == pivot && list[left] == pivot)
            left++;

You should return left.

Then, in the Recursive call, you have the constant value 1 which you use to test the left-condition.... This is a big bug, because the pivot will only return 0 for the left-most partition. The 1 constant should be left ....

I ended up ideoning your code to play with it. IU shuffled around a lot of the logic.

Have a look...

This is the code that does the sort closer to the way that I expect:

using System;
using System.Collections.Generic;

public class Test
{
    static List<int> RandomList(int size) {
        List<int> ret = new List<int>(size);
        Random rand = new Random(1);
        for (int i = 0; i < size; i++) {
            ret.Add(rand.Next(size));
        }
        return ret;
    }

    static int MyPartition(List<int> list, int left, int right)
    {
        int start = left;
        int pivot = list[start];
        left++;
        right--;

        while(true)
        {
            while(left <= right && list[left] <= pivot)
                left++;

            while(left <= right && list[right] > pivot)
                right--;

            if(left > right)
            {
                list[start] = list[left - 1];
                list[left - 1] = pivot;

                return left;
            }

            int temp = list[left];
            list[left] = list[right];
            list[right] = temp;

        }
    }

    static void MyQuickSort(List<int> list, int left, int right)
    {
        if(list == null || list.Count <= 1)
            return;

        if(left < right)
        {
            int pivotIdx = MyPartition(list, left, right);
            //Console.WriteLine("MQS " + left + " " + right);
            //DumpList(list);
            MyQuickSort(list, left, pivotIdx - 1);
            MyQuickSort(list, pivotIdx, right);
        }
    }

    static void DumpList(List<int> list) {
        list.ForEach(delegate(int val)
        {
            Console.Write(val);
            Console.Write(", ");
        });
        Console.WriteLine();
    }

    public static void Main()
    {
        List<int> list = RandomList(100);
        DumpList(list);
        MyQuickSort(list, 0, list.Count);
        DumpList(list);
    }
}