Make Project Euler 27 solution idiomatic Ruby I learned to program in Java and C# and in my free time I am using Ruby because it is fun. Unfortunately I have the feeling that I write Ruby code like I am making Java or C# code. I have learned to use regex instead of strings for comparison, using each instead of for-loops, keeping method names lowercase, and how to use code blocks (which are quite similar to lambda expressions in C#).

I would love to improve the Rubiness of my code and I hope you would be willing to give me one or more pointers on a piece of code I made. It answers Project Euler problem 27.

class Integer
  def prime?
    return false if self < 1
    2.upto(Math.sqrt(self)) do |i|
      return false if self % i == 0
    end
    true
  end
end

def get_amount_of_primes_from_quadratic_formula(a,b)
  primes = []
  still_all_primes = true
  n = 0
  while still_all_primes
    result = n**2 + a*n + b
    if result.prime? then
      primes << result
    else
      still_all_primes = false
    end
    n += 1
  end
  primes.size
end

def get_product_of_coefficients_that_produce_maximum_number_of_primes_for_consecutive_values()
  max_product = 0
  max_primes = 0
  -999.upto(1000) do |a|
    -999.upto(1000) do |b|
      primes = get_amount_of_primes_from_quadratic_formula(a,b)
      if primes > max_primes then
        max_primes = primes
        max_product = a*b
      end
    end
  end
  max_product
end

start = Time.now
answer = get_product_of_coefficients_that_produce_maximum_number_of_primes_for_consecutive_values()

puts "The answer is #{answer} and it took #{Time.now-start} seconds."

I think I can improve on the if-then statement and write it more concise, and also the two "upto-loops" where I first declare the variables max_primes and max_product can be written in a more Ruby-way I am sure.

I would be very grateful if you could let me know how to write more like Ruby!

Links that ask similar questions which I am reading in the meantime:

Advice on making ruby code more ruby-like
Ruby method return values - how to be more Ruby-like?
How to make my script nicer / more Ruby?

Succinctness
As rubyists, we love being succinct, and we love playing with enumerations.

You will see very few literal false and true in ruby code, as well as very few explicit return calls.

For example:

Instead of writing return false if self < 1 we will prefer to compound the condition to self >= 1 && ... which will do the same thing, but we "save" return false.

The power of Enumeration
Ruby has a very powerful Enumerable, and is used widely, often more than once in a line (using method chaining).

For example:

2.upto(Math.sqrt(self)) do |i|
  return false if self % i == 0
end

Here you check if any of the numbers in the range are a divisor for self, and break if there is any. A more ruby way of doing it will be:

return false if 2.upto(Math.sqrt(self)).any? { |i| self % i == 0 }

We'll also prefer to more succinct range syntax (2..Math.sqrt(self)), which is simply shorter...

So now, our def prime? method could be reduced to a one-liner:

class Integer
  def prime?
    self > 1 && !(2..Math.sqrt(self)).any? { |i| self % i == 0 }
  end
end

Mapping
Anywhere in the code I see the following pattern:

result = []
some_loop do
  result << something
end

A red flag is raised, and I look for a way to use map to do the same thing:

result = some_loop.map { something }

Your code goes over all the non-negative integers, and takes counts how many of them result in a prime, until the first non-prime.

"All the non-negative integers" can be expressed in ruby as (0..Float::INFINITY), so we can write:

(0..Float::INFINITY).map { |n| n**2 + a*n + b }.take_while { |result| result.prime? }.count

This code takes each integer, maps it into the result of n**2 + a*n + b, takes all the results until they are no longer prime, and counts how many are there.

Cool! Right? The only problem with the code above, is that it will take infinity to complete it, as it takes all the numbers and maps them, and then checks for how many to take.

To solve this problem ruby now has...

Lazy Enumerables
As of ruby 2.0, lazy enumerables allows you to calculate values in an infinite stream only as needed.

To solve the problem above, all we need to do now is to add the lazy operator on the range:

(0..Float::INFINITY).lazy.map { |n| n**2 + a*n + b }.take_while(&:prime?).count

And we have another one-liner!

Everything is an enumerable
So you want to save on your "upto-loops"? Let's do it!

You want to enumerate over each pair of numbers from -999 to 1000, so what you actually want is to have a long matrix of those pairs:

[[-999, -999], [-999, -998],...,[1000, 1000]].do_something_smart

To do that, you can use product:

(-999..1000).to_a.product((-999..1000).to_a)

But since both a and b have the same range, we can even DRY this up, and use repeated_permutation:

(-999..1000).to_a.repeated_permutation(2)

Both of these solutions will give you the needed matrix, so we can move on the see what we should do with it...

We want to get the coeffiecients that produce the number of primes, so let's do just that:

a, b = (-999..1000).to_a.repeated_permutation(2).max_by { |a, b| get_amount_of_primes_from_quadratic_formula(a,b) }

Now all we need to do is multiply them with each other!

Method naming
Your names are very verbose, which is a good thing, but ruby idiom frowns upon get_ prefixes. Also, prefer using verbs already in the language (count) over those which are not in the language (amount_of)

So now the code will look like:

class Integer
  def prime?
    self > 1 && !(2..Math.sqrt(self)).any? { |i| self % i == 0 }
  end
end

def count_quadratic_formula_primes(a,b)
  (0..Float::INFINITY).lazy.map { |n| n**2 + a*n + b }.take_while(&:prime?).count
end

def product_of_coefficients_that_produce_maximum_number_of_primes_for_consecutive_values()
  a, b = (-999..1000).to_a.repeated_permutation(2).max_by { |a, b| count_quadratic_formula_primes(a,b) }
  a * b
end

start = Time.now
answer = product_of_coefficients_that_produce_maximum_number_of_primes_for_consecutive_values

puts "The answer is #{answer} and it took #{Time.now-start} seconds."

15 lines of hard-core ruby-style code!

Enjoy!

Update
It seems that lazy adds considerable overhead to the performance of the code. So it is not advisable to use it.

Fortunately this works:

(0..Float::INFINITY).take_while { |n| (n**2 + a*n + b).prime? }.count

My code still runs ~2 times slower than the original (ends in 18 seconds), but it is more reasonable than with lazy...


Python 3 function to rotate an image 90° A coding challenge that rotates an image 90 degrees counterclockwise. The image is represented as an array matrix. I believe the time complexity is O(n2), but I'd like to know for sure, as well as any other feedback.

def rotate_image(img):
    rotated_image = [[] for x in range(len(img))]
    for i in range(len(img)):
        for j in range(len(img[i])):
          rotated_image[len(img) - j - 1].append(img[i][j])
    return rotated_image

Example usage:

image = [
  [1, 1, 5, 9, 9],
  [2, 2, 6, 0, 0],
  [3, 3, 7, 1, 1],
  [4, 4, 8, 2, 2],
  [5, 5, 9, 3, 3]
]

rotated_img = rotate_image(image)
for i in rotated_img:
    print(i)

Outputs:

[9, 0, 1, 2, 3]
[9, 0, 1, 2, 3]
[5, 6, 7, 8, 9]
[1, 2, 3, 4, 5]
[1, 2, 3, 4, 5]

You can use NumPy module that's good with arrays and matrices. It has a built-in for exactly that purpose -

import numpy as np

np.rot90(image).tolist()

With array manipulations, that's essentially same as performing matrix/array transpose and then flipping the rows -

np.asarray(image).T[::-1].tolist()

If the input is already an array, we can skip the array-conversion. Also, if the output as an array is okay, it would be simply a view into the input and as such the entire operation would be virtually-free.

Thus, with image_arr as the input array, it would be -

np.rot90(image_arr)

With transpose and flipping rows -

image_arr.T[::-1]

Let's take the provided sample and check out outputs on an IPython console -

In [48]: image
Out[48]:
[[1, 1, 5, 9, 9],
 [2, 2, 6, 0, 0],
 [3, 3, 7, 1, 1],
 [4, 4, 8, 2, 2],
 [5, 5, 9, 3, 3]]

In [50]: np.asarray(image).T[::-1].tolist()
Out[50]:
[[9, 0, 1, 2, 3],
 [9, 0, 1, 2, 3],
 [5, 6, 7, 8, 9],
 [1, 2, 3, 4, 5],
 [1, 2, 3, 4, 5]]

Timings on a large 5000 x 5000 sized image

1) Image as a list :

In [53]: image = np.random.randint(0,256,(5000,5000)).tolist()

# @Dima Tisnek's soln
In [54]: %timeit list(reversed(list(zip(*image))))
1 loop, best of 3: 1.09 s per loop

In [55]: %timeit np.array(image).T[::-1].tolist()
1 loop, best of 3: 1.06 s per loop

Time-complexity

There's no time-complexity involved here (not on computation anyway) and the entire play is about array and list conversion, as shown below when we break down the steps -

In [72]: image_arr = np.array(image)

In [71]: %timeit np.array(image) # convert to array
1 loop, best of 3: 771 ms per loop

In [73]: %timeit image_arr.T[::-1] # perform 90deg rotation
1000000 loops, best of 3: 372 ns per loop

In [74]: %timeit image_arr.T[::-1].tolist() # convert back to list
1 loop, best of 3: 296 ms per loop

2) Image and output as arrays :

In [56]: image = np.random.randint(0,256,(5000,5000))

# @Dima Tisnek's soln
In [57]: %timeit list(reversed(list(zip(*image))))
1 loop, best of 3: 1.34 s per loop

In [58]: %timeit image.T[::-1]
1000000 loops, best of 3: 363 ns per loop

In [59]: %timeit np.rot90(image)
100000 loops, best of 3: 9.05 µs per loop

The last two NumPy based ones are virtually free as discussed earlier.  This is because internally image.T[::-1] is same as input image, but with different stride pattern representation. Let's verify that they are same by checking their memory occupancy -

In [60]: np.shares_memory(image, image.T[::-1])
Out[60]: True

Conversion to list on own-data for slight perf. boost

Closer inspection on list conversion reveals that converting to list when the strided pattern isn't regular (row-order) might not be the most optimal scenario. So, one way would be create a copy of array data once we have the rotated one and then convert. This seems to give around 10% improvement -

In [2]: image = np.random.randint(0,256,(5000,5000)).tolist()

In [8]: %timeit list(reversed(list(zip(*image))))
1 loop, best of 3: 1.12 s per loop

In [9]: %timeit np.asarray(image).T[::-1].tolist()
1 loop, best of 3: 1.11 s per loop

# Have own-data (copy) and then convert to list
In [10]: %timeit np.asarray(image).T[::-1].copy().tolist()
1 loop, best of 3: 1.01 s per loop


Remove last odd number from number list in Python The simplified question is:

  Given a list of numbers, write a Python script that will remove the last odd number from the list.

One of the challenges is if the last odd number appears multiple times, some approaches will remove the first occurrence of that number, rather than the last.

One solution that I have is to find the max index for the odd numbers and then pop that number from the list:

def remove_last_odd(numbers):
    count = 0
    for num in numbers:
        if num%2 == 1:
            max = count
        count += 1
    print max
    numbers.pop(max)

def run():
    numbers = [1, 7, 2, 34, 8, 7, 2, 5, 14, 22, 93, 48, 76, 15, 7]
    print numbers
    remove_last_odd(numbers)
    print numbers

run()

This works, but I feel like there's got to be a better way. I've thought about reversing the list, removing the first odd number and then re-reversing:

# use this instead of remove_last_odd function
def remove_backwards(numbers):
    rev_num = numbers[::-1]
    for num in rev_num:
        if num % 2 == 1:
            rev_num.remove(num)
            numbers = rev_num[::-1]
            return numbers

This also works, and feels cleaner, but if anyone can provide advice on preferred methods/more Pythonic approaches, it'd be much appreciated!

By my interpretation, if the list contains no odd numbers, then the function should make no change.  Your first function, since it unconditionally ends with numbers.pop(max), will crash with an UnboundLocalError: local variable 'max' referenced before assignment.

count += 1 is awkward.  Counting loops are normally written using some kind of range() or enumerate().

The first implementation is inefficient, since it examines the entire list from the beginning.

The second function, as you noted, is inefficient in that it makes two copies of the list.  Furthermore, rev_num.remove(num) is fully O(n): it performs another linear search, and then it copies the rest of the elements to fill the hole.

I would write a solution this way.  This function mutates the list, and also indicates which index was removed, if any.

def remove_last_odd(numbers):
    for i in range(len(numbers) - 1, -1, -1):
        if numbers[i] % 2:
            numbers.pop(i)
            return i
    return None

Here is another way to do it, using enumerate() and reversed(), which iterates backwards without copying the list.  Note that it returns a negative index.

def remove_last_odd(numbers):
    for i, num in enumerate(reversed(numbers), 1):
        if num % 2:
            numbers.pop(-i)
            return -i
    return None


Using LINQ to perform a LEFT OUTER JOIN in 2 DataTables (Multiples criteria) I know that exists a lot of solutions about how to create an OUTER JOIN between two DataTables.

I created the following code in C#:

DataTable vDT1 = new DataTable();
vDT1.Columns.Add("Key");
vDT1.Columns.Add("Key2");
vDT1.Columns.Add("Data1");
vDT1.Columns.Add("Data2");

vDT1.Rows.Add(new object[] { "01", "ZZ", "DATA1_AAAA", "DATA2_AAAA" });
vDT1.Rows.Add(new object[] { "02", "ZZ", "DATA1_BBBB", "DATA2_BBBB" });

DataTable vDT2 = new DataTable();
vDT2.Columns.Add("Key");
vDT2.Columns.Add("Key2");
vDT2.Columns.Add("Data3");
vDT2.Columns.Add("Data4");

vDT2.Rows.Add(new object[] { "01", "ZZ", "DATA3_AAAA", "DATA4_AAAA" });
vDT2.Rows.Add(new object[] { "01", "ZZ", "DATA3_BBBB", "DATA4_BBBB" });
vDT2.Rows.Add(new object[] { "01", "ZZ", "DATA3_CCCC", "DATA4_CCCC" });
vDT2.Rows.Add(new object[] { "01", "ZZ", "DATA3_DDDD", "DATA4_DDDD" });

DataTable vDT3 = new DataTable();
vDT3.Columns.Add("Key");
vDT3.Columns.Add("Key2");
vDT3.Columns.Add("Data1");
vDT3.Columns.Add("Data2");
vDT3.Columns.Add("KeyTemp1");
vDT3.Columns.Add("KeyTemp2");
vDT3.Columns.Add("Data3");
vDT3.Columns.Add("Data4");

DataRow vDRnull = vDT2.Rows.Add();

var vLINQ = vDT1.AsEnumerable()
    .GroupJoin(vDT2.AsEnumerable(),
        dr1 => new { key1 = dr1["Key"], key2 = dr1["Key2"] },
        dr2 => new { key1 = dr2["Key"], key2 = dr2["Key2"] },
        (dr1, result) => dr1.ItemArray.Koncat(
            ((result.FirstOrDefault<DataRow>() == null)
                ? vDRnull
                : result.FirstOrDefault<DataRow>()).ItemArray));

foreach (var aw in vLINQ)
{
    vDT3.Rows.Add(aw);
}

I implemented this extension:

public static T[] Koncat<T>(this T[] x, T[] y)
{
    if (x == null) throw new ArgumentNullException("x");
    if (y == null) throw new ArgumentNullException("y");
    int oldLen = x.Length;
    Array.Resize<T>(ref x, x.Length + y.Length);
    Array.Copy(y, 0, x, oldLen, y.Length);
    return x;
}

This easy and clean (from my point of view), but I wanted to ask the experts for recommendations, improvements, or if this method have a performance flaw.

Normally I need manage huge amount of data (More than 100K items).

DataTables are quite powefull and offer lots of the real database functionality. Also as far as joins are concerned a few things are possible and I'm of the opinion that if someone uses DataTables he also should use the functionality they offer ;-)

It this case using DataTable joins your example could look like this:

DataSet ds = new DataSet();

DataTable dt1 = new DataTable();
dt1.Columns.Add("Key");
dt1.Columns.Add("Key2");
dt1.Columns.Add("Data1");
dt1.Columns.Add("Data2");

dt1.Rows.Add(new object[] { "01", "ZZ", "DATA1_AAAA", "DATA2_AAAA" });
dt1.Rows.Add(new object[] { "02", "ZZ", "DATA1_BBBB", "DATA2_BBBB" });

DataTable dt2 = new DataTable();
dt2.Columns.Add("Key");
dt2.Columns.Add("Key2");
dt2.Columns.Add("Data3");
dt2.Columns.Add("Data4");

dt2.Rows.Add(new object[] { "01", "ZZ", "DATA3_AAAA", "DATA4_AAAA" });
dt2.Rows.Add(new object[] { "01", "ZZ", "DATA3_BBBB", "DATA4_BBBB" });
dt2.Rows.Add(new object[] { "01", "ZZ", "DATA3_CCCC", "DATA4_CCCC" });
dt2.Rows.Add(new object[] { "01", "ZZ", "DATA3_DDDD", "DATA4_DDDD" });
//dt2.Rows.Add(new object[] { "02", "ZZ", "DATA5_DDDD", "DATA4_DDDD" });

ds.Tables.Add(dt1);
ds.Tables.Add(dt2);

// specify the relations between the data tables
DataRelation drel = new DataRelation(
    "MyJoin",
    new DataColumn[] { dt1.Columns["Key"], dt1.Columns["Key2"] },
    new DataColumn[] { dt2.Columns["Key"], dt2.Columns["Key2"]});

ds.Relations.Add(drel);

DataTable jt = new DataTable("JoinedTable");
jt.Columns.Add("Key");
jt.Columns.Add("Key2");
jt.Columns.Add("Data1");
jt.Columns.Add("Data2");
jt.Columns.Add("Data3");
jt.Columns.Add("Data4");
ds.Tables.Add(jt);

// create the result table
foreach (DataRow row in dt1.Rows)
{
    var childRows = row.GetChildRows("MyJoin");
    // mimics left join
    var hasChildRows = childRows.Length > 0;
    if (!hasChildRows)
    {
        jt.Rows.Add(row["Key"], row["Key2"], row["Data1"], row["Data2"], null, null);
        continue;
    }

    foreach (var child in childRows)
    {
        jt.Rows.Add(row["Key"], row["Key2"], row["Data1"], row["Data2"], child["Data3"], child["Data4"]);
    }
}

jt.Rows.Dump(); // LINQPad dump

As far as your code is concerned I'm not happy with the Koncat method because it modifies the ItemArray that belongs to the DataRow instead of creating a new result.

You actually don't need it because LINQ already has a such a method that you could use like this:

dr1.ItemArray
.Concat(result.Any() ? result.First().ItemArray: Enumerable.Empty<object>())
.ToArray());

It's not necessary to call the FirstOrDefault method two times. It's better to just check if the result has Any rows and then get the First one and its ItemArray or otherwise an empty IEnumerable, finally you turn it into an array and you're done:

var vLINQ = vDT1.AsEnumerable()
    .GroupJoin(
        vDT2.AsEnumerable(),
        dr1 => new { key1 = dr1["Key"], key2 = dr1["Key2"] },
        dr2 => new { key1 = dr2["Key"], key2 = dr2["Key2"] },
        (dr1, result) =>
            dr1.ItemArray
            .Concat(result.Any() ? result.First().ItemArray: Enumerable.Empty<object>())
            .ToArray());

or if you can use C# 6 even shorter with the ?. and ?? operators

dr1.ItemArray
.Concat(result.FirstOrDefault()?.ItemArray ?? Enumerable.Empty<object>())
.ToArray());


Karplus Strong pluck generation I want to use a simple implementation of the Karplus-Strong algorithm to generate pluck sounds and chords.

My existing code seems correct but is slow when more than a few plucks are being generated. Is there a way to improve the performance of this. In particular is there a better way to use numpy here, or would something like a collections.deque be faster.

import numpy as np

sample_rate = 44100
damping = 0.999

def generate(f, vol, nsamples):
    """Generate a Karplus-Strong pluck.

    Arguments:
    f -- the frequency, as an integer
    vol -- volume, a float in [0.0, 1.0]
    nsamples -- the number of samples to generate. To generate a pluck t
    seconds long, one needs t * sample_rate samples. nsamples is an int.

    Return value:
    A numpy array of floats with length nsamples.
    """

    N = sample_rate // f
    buf = np.random.rand(N) * 2 - 1
    samples = np.empty(nsamples, dtype=float)

    for i in range(nsamples):
        samples[i] = buf[i % N]
        avg = damping * 0.5 * (buf[i % N] + buf[(1 + i) % N])
        buf[i % N] = avg

    return samples * vol

def generate_chord(f, nsamples):
    """Generate a chord of several plucks

    Arguments:
    f -- a (usually short) list of integer frequencies
    nsamples -- the length of the chord, a chord of length t seconds needs
    t * sample_rate, an integer.

    Return value:
    A numpy array
    """
    samples = np.zeros(nsamples, dtype=float)
    for note in f:
        samples += generate(note, 0.5, nsamples)
    return samples

if __name__ == "__main__":
    import matplotlib.pyplot as plt
    from scipy.io.wavfile import write

    strum = generate_chord(  # A Major
        [220, 220 * 5 // 4, 220 * 6 // 4, 220 * 2], sample_rate * 5)
    plt.plot(strum)
    plt.show()

    write("pluck.wav", sample_rate, strum)

This produces a sound file in pluck.wav with a waveform like this:

Congratulations

You mostly follows standards which is good. You also have good documentation which is great. All in all the code reads well and is easily understandable.

Style-wise, I just have some nitpicks:

from PEP8, you should name your constants using all caps (SAMPLE_RATE, DAMPING);
you should not abreviate that much your variable names frequency and volume are to be prefered to f and vol;
you may want to name things to avoid comments: A_major = [220, 220 * 5 // 4, 220 * 6 // 4, 220 * 2].

Summation

In pure Python world the following code:

value = 0
for element in array:
    value += compute(element)
return value

is prefered written as:

return sum(compute(element) for element in array)

As it is both cleaner and faster. In numpy world, it is pretty much the same. You can write:

def generate_chord(frequencies, samples):
    plucks = (generate(note, 0.5, samples) for note in frequencies)
    return np.sum(plucks, axis=0)

and get the same result.

Interface

When reading your docstrings, I wondered why you would ask your users (or yourself) to pass in a number of samples if what is more natural to them is to use a duration in seconds. Do not expose implementation details such as your sample rate to the users and let them use what is more natural to them.

I also wonder why you hardcode the volume in generate_chord and let it variable in generate. If the aim is to not have to choose it for each test, you can still use a parameter with default value instead of hardcoding it. It will make things more obvious.

Lastly, I don't really understand the need for integral frequencies as the computations can be performed just as well with floating point values. Just make sure to truncate the computation of N.

I would use the following code as a base for improvements:

import numpy as np

SAMPLE_RATE = 44100
DAMPING = 0.999

def generate(frequency, volume, duration):
    """Generate a Karplus-Strong pluck.

    Arguments:
    frequency -- the frequency, as a float
    volume -- volume, a float in [0.0, 1.0]
    duration -- the length of the generated pluck, in seconds.

    Return value:
    A numpy array of floats with length nsamples.
    """

    samples_count = duration * SAMPLE_RATE
    N = int(SAMPLE_RATE / frequency)
    buf = np.random.rand(N) * 2 - 1
    samples = np.empty(samples_count, dtype=float)

    for i in range(samples_count):
        samples[i] = buf[i % N]
        avg = DAMPING * 0.5 * (buf[i % N] + buf[(1 + i) % N])
        buf[i % N] = avg

    return samples * volume

def generate_chord(frequencies, duration, volume=0.5):
    """Generate a chord of several plucks

    Arguments:
    frequencies -- a (usually short) list of frequencies
    describing a chord.
    duration -- the length of the generated pluck, in seconds.

    Return value:
    A numpy array
    """

    plucks = (generate(note, volume, duration) for note in frequencies)
    return np.sum(plucks, axis=0)

if __name__ == "__main__":
    import scipy.io.wavfile
    import matplotlib.pyplot as plt

    A_major = [220, 220 * 5 // 4, 220 * 6 // 4, 220 * 2]
    strum = generate_chord(A_major, duration=5)

    plt.plot(strum)
    plt.show()

    scipy.io.wavfile.write(filename, SAMPLE_RATE, strum)

Beyond frequencies

Now, I’m not musician. And even if I were, I would need to look up frequencies of required note as I don't think your average user would know them by heart. It would be best to provide named constants to remember that for us.

Reading at https://en.wikipedia.org/wiki/Musical_note one can easily define something like:

C = 2 ** (-9/12) * 440
C_sharp = 2 ** (-8/12) * 440
D_flat = 2 ** (-8/12) * 440
D = 2 ** (-7/12) * 440
D_sharp = 2 ** (-6/12) * 440
E_flat = 2 ** (-6/12) * 440
E = 2 ** (-5/12) * 440
F = 2 ** (-4/12) * 440
F_sharp = 2 ** (-3/12) * 440
G_flat = 2 ** (-3/12) * 440
G = 2 ** (-2/12) * 440
G_sharp = 2 ** (-1/12) * 440
A_flat = 2 ** (-1/12) * 440
A = 2 ** (0/12) * 440
A_sharp = 2 ** (1/12) * 440
B_flat = 2 ** (1/12) * 440
B = 2 ** (2/12) * 440

But this does not account for every octave. Let's fix that and simplify the reading using an enum:

import enum

def octave(order):
    frequency_of_A = 440 * 2 ** (order - 4)

    class Octave(enum.Enum):
        C = -9
        C_sharp = -8
        D_flat = -8
        D = -7
        D_sharp = -6
        E_flat = -6
        E = -5
        F = -4
        F_sharp = -3
        G_flat = -3
        G = -2
        G_sharp = -1
        A_flat = -1
        A = 0
        A_sharp = 1
        B_flat = 1
        B = 2

        def __float__(self):
            return 2 ** (self.value/12) * frequency_of_A

    return Octave

SubSubContra = octave(-1)
SubContra = octave(0)
Contra = octave(1)
Great = octave(2)
Small = octave(3)
OneLined = octave(4)
TwoLined = octave(5)
ThreeLined = octave(6)
FourLined = octave(7)
FiveLined = octave(8)
SixLined = octave(9)

Now I can easily define A Major using:

A_major = [Small.A, OneLined.C_sharp, OneLined.E, OneLined.A]

Proposed improvements

import enum
import numpy as np

SAMPLE_RATE = 44100
DAMPING = 0.999

def octave(order):
    """Create an enum of all the notes in the octave
    of the given order.
    """
    frequency_of_A = 440 * 2 ** (order - 4)

    class Octave(enum.Enum):
        C = -9
        C_sharp = -8
        D_flat = -8
        D = -7
        D_sharp = -6
        E_flat = -6
        E = -5
        F = -4
        F_sharp = -3
        G_flat = -3
        G = -2
        G_sharp = -1
        A_flat = -1
        A = 0
        A_sharp = 1
        B_flat = 1
        B = 2

        def __float__(self):
            return 2 ** (self.value/12) * frequency_of_A

    return Octave

SubSubContra = octave(-1)
SubContra = octave(0)
Contra = octave(1)
Great = octave(2)
Small = octave(3)
OneLined = octave(4)
TwoLined = octave(5)
ThreeLined = octave(6)
FourLined = octave(7)
FiveLined = octave(8)
SixLined = octave(9)

def generate(frequency, volume, duration):
    """Generate a Karplus-Strong pluck.

    Arguments:
    frequency -- the frequency, as a float
    volume -- volume, a float in [0.0, 1.0]
    duration -- the length of the generated pluck, in seconds.

    Return value:
    A numpy array of floats with length nsamples.
    """

    samples_count = duration * SAMPLE_RATE
    N = int(SAMPLE_RATE / frequency)
    buf = np.random.rand(N) * 2 - 1
    samples = np.empty(samples_count, dtype=float)

    for i in range(samples_count):
        samples[i] = buf[i % N]
        avg = DAMPING * 0.5 * (buf[i % N] + buf[(1 + i) % N])
        buf[i % N] = avg

    return samples * volume

def generate_chord(frequencies, duration, volume=0.5):
    """Generate a chord of several plucks

    Arguments:
    frequencies -- a (usually short) list of frequencies
    describing a chord.
    duration -- the length of the generated pluck, in seconds.

    Return value:
    A numpy array
    """

    notes = map(float, frequencies)
    plucks = (generate(note, volume, duration) for note in notes)
    return np.sum(plucks, axis=0)

if __name__ == "__main__":
    import scipy.io.wavfile
    import matplotlib.pyplot as plt

    A_major = [Small.A, OneLined.C_sharp, OneLined.E, OneLined.A]
    strum = generate_chord(A_major, duration=5)

    plt.plot(strum)
    plt.show()

    scipy.io.wavfile.write(filename, SAMPLE_RATE, strum)


E-mail crawler for yellowpages I've written a script in python to scrape e-mail addresses from different pizza shops located in los-angeles available in yellowpage traversing multiple pages. It is able to go one-layer deep and dig out email addresses. I believe, this crawler has got the ability to parse all the emails from any link no matter how many pages it has spread across. Just needed to adjust the last page number in the crawler.

Here is what I've written:

import requests
from lxml import html

link = "https://www.yellowpages.com"
for page_num in range(1,10):
  for item in html.fromstring(requests.get("https://www.yellowpages.com/search?search_terms=pizza&geo_location_terms=Los%20Angeles%2C%20CA&page={0}".format(page_num)).text).cssselect('div.info h2.n a:not([itemprop="name"]).business-name'):
    for data in html.fromstring(requests.get(link + item.attrib['href']).text).cssselect('a.email-business'):
      print(data.attrib['href'].replace("mailto:",""))

Knowing that you had a ton of scrappers questions, this doesn't look nice at all. It looks like you were in a hurry and didn't much care about how the code looks.

Styling

Use 4 spaces per indentation level
Constants should be LOWERCASED
You should have a space after ,
Your lines are way too long. Stick to 72 characters or 120 (at most)

Code
First, you don't have to hardcode your search items. Instead, you might as well define a function which returns the data you need (the entire URL):
def build_url(item, location, page):
    params = urllib.parse.urlencode({
        'search_terms': item,
        'geo_location_terms': location,
        'page': page
    })

    return '{}/search?{}'.format(URL, params)

With that in mind, your code might be restructured nicer, like this:
import requests
import urllib

from lxml import html

URL = "https://www.yellowpages.com"
LOWER_LIMIT = 1
UPPER_LIMIT = 10

def build_url(item, location, page):
    params = urllib.parse.urlencode({
        'search_terms': item,
        'geo_location_terms': location,
        'page': page
    })

    return '{}/search?{}'.format(URL, params)

def scrape():
    for page in range(LOWER_LIMIT, UPPER_LIMIT):
        url = build_url('pizza', 'Los Angeles CA', page)
        html_ = requests.get(url).text
        for item in html.fromstring(html_).cssselect('div.info h2.n a:not([itemprop="name"]).business-name'):
            for data in html.fromstring(requests.get(URL + item.attrib['href']).text).cssselect('a.email-business'):
                print(data.attrib['href'].replace("mailto:", ""))

if __name__ == '__main__':
    scrape()

Other changes that I did above:

Move the logic into separate functions
Make the range arguments constants so that they can be easily modified
Add the guard check if __name__ == '__main__'

As is, your scrapper is pretty slow because it has to search in each page the entire html for that specific a info. You might consider using Scrapy.


Working with Arrays in Swift It's an exercise from a Swift-course I'm currently taking.

  You are given an array of integers.  Make an additional array named
  computedNumbers. computedNumbers should multiply each element in the
  numbers array by the next following element. The last element shall be
  multiplied by the first element.

  Example: If numbers was equal to ... [3, 1, 4, 2] ... then the
  computedNumbers should computed [3 x 1, 1 x 4, 4 x 2, 2 x 3], so that
  they become: [3, 4, 8, 6]

Here my solution:

var numbers = [45, 73, 195, 53]

var computedNumbers = [Int]()
let count = numbers.count
var i = 0

while i < count - 1 {
    computedNumbers.append(numbers[i] * numbers[i + 1])
    i = i + 1
}

computedNumbers.append(numbers[count - 1] * numbers[0])

print(computedNumbers)

It has passed the automated tests, but it isn't very elegant.

It there a better way to accomplish the task?

What should I have done different and why?

Looking forward to reading your comments and answers.

Handle edge cases

If the given numbers array is empty then the first loop will do nothing, but

computedNumbers.append(numbers[count - 1] * numbers[0])

aborts with a runtime exception. The correct result in this case would be to set computedNumbers to an empty array as well.

Better loops

This

var i = 0
while i < count - 1 {
    // ...
    i = i + 1
}

is shorter and better done as

for i in 0 ..< count - 1 {
    // ...
}

The advantages are

The scope of i  is limited to the loop.
i is a constant.
The range of i is clearly seen right at the start of the loop.

In this particular case we iterate of adjacent array elements, which is conveniently by by zip()ping the array with a shifted view of itself:

var computedNumbers = zip(numbers, numbers.dropFirst()).map(*)
computedNumbers.append(numbers[count - 1] * numbers[0])

If we append the first element to the shifted view then the complete operation reduces to

let computedNumbers = zip(numbers, [numbers.dropFirst(), numbers.prefix(1)].joined()).map(*)

Note that this handles also the case of an empty array gracefully.

Use constants if possible

The numbers array is never mutated, therefore it should be declared as a constant with let:

let numbers = [3, 1, 4, 2]

Using a constant

makes it clear to the reader that this value is never mutated,
helps to avoid unintentional mutation,
possibly allows better compiler optimization.

Make it a function

If we put the functionality in a separate function then

it becomes reusable,
test cases can be added more easily,
we can give it a meaningful name,
we can add documentation.

In our case the function would simply be

/// Compute an array where each element in the numbers array is multipled by
/// the next following element. The last element is multiplied by the first element.
/// - Parameter numbers: An array of integers.
func circularAdjacentProducts(of numbers: [Int]) -> [Int] {
    return zip(numbers, [numbers.dropFirst(), numbers.prefix(1)].joined()).map(*)
}

and would then be used as

let numbers = [3, 1, 4, 2]
let computedNumbers = circularAdjacentProducts(of: numbers)
print(computedNumbers)

Make it generic on the element type

The same operation could be done with an array of floating point values (Float or Double), or with other integer types (e.g. Int16) as long as the multiplication does not overflow. Therefore we can generalize it to array of type [T] where T conforms to a protocol which required a multiplication operation.

A possible choice is the Numeric protocol which is adopted by all floating point and integer types:

func circularAdjacentProducts<T: Numeric>(of numbers: [T]) -> [T] {
    return zip(numbers, [numbers.dropFirst(), numbers.prefix(1)].joined()).map(*)
}

Example:

let floats = [3.3, 1.1, 4, 2.2]
let computedFloats = circularAdjacentProducts(of: floats)
print(computedFloats) // [3.63, 4.4, 8.8, 7.26]

Make it generic on the collection type

The next possible generalization is to apply this operation not only to arrays, but to arbitrary collections:

extension Collection where Element: Numeric {
    func circularAdjacentProducts() -> [Element] {
        return zip(self, [dropFirst(), prefix(1)].joined()).map(*)
    }
}

Example with an ArraySlice:

let numbers = Array(1...10).dropFirst(3)
print(numbers) // [4, 5, 6, 7, 8, 9, 10]
let computedFloats = numbers.circularAdjacentProducts()
print(computedFloats) // [20, 30, 42, 56, 72, 90, 40]


Binary Conversion in Java Before implementing a GUI I tried performing it at CLI first and I also tried to implement it by using calling methods to another class.

BinaryConversion class:

import java.io.IOException;
import static java.lang.System.in;
import java.util.InputMismatchException;
import java.util.Scanner;

public class BinaryConversion{

    public static void main(String[] args) throws IOException {

        try (Scanner in = new Scanner(System.in)) {

            System.out.print("Enter given: ");
                int givenNum = in.nextInt();
            System.out.println("Binary: " + convert.getBinary(givenNum));
            System.out.println("Octal:  " + convert.getOctal(givenNum));
            System.out.println("Hex:    " + convert.getHex(givenNum));

        } catch(InputMismatchException e) {

            System.out.println("Looks like you entered a non integer value.");

        } finally {

            in.close();

        }
    }
}

Convert class:

public final class convert{

    private convert() {
        // removes the default constructor
    }

    public static String getBinary(int given) {
        char binNumbers[] = {'0','1'};
        String str = "";
        int rem;
            while (given > 0) {
                rem = given % 2;
                str = binNumbers[rem] + str;
                given /= 2;
            }
        return str;
    }

    public static String getOctal(int given) {
       char octalNumbers[] = {'0','1','2','3','4','5','6','7'};
       String str = "";
       int rem;
           while (given > 0) {
               rem = given % 8;
               str = octalNumbers[rem] + str;
               given /= 8;
            }
       return str;
    }

    public static String getHex(int given) {
        char hexNumbers[] = {'0','1','2','3','4','5','6','7','8','9','A','B','C','D','E','F'};
        String str = "";
        int rem;
            while(given > 0){
                rem = given % 16;
                str = hexNumbers[rem] + str;
                given /= 16;
            }
        return str;
    }
}

My questions are:

Did I missed something?
Is there a more efficient implementation of this program?

First: Please format your code more consistently.

The indention of the whileloop is very unusual. Better (and more common):

public static String getBinary(int given) {
    char binNumbers[] = {'0','1'};
    String str = "";
    int rem;
    while (given > 0) {
        rem = given % 2;
        str = binNumbers[rem] + str;
        given /= 2;
    }
    return str;
}

Next: Please use the default cases for identifiers. Classes should start upper case (Convert instead of convert).

Also use StringBuilder instead of String-concatenation:

public static String getBinary(int given) {
    char binNumbers[] = {'0','1'};
    StringBuilder sb = new StringBuilder();
    int rem;
    while (given > 0) {
        rem = given % 2;
        sb.append(binNumbers[rem]);
        given /= 2;
    }
    return sb.toString();
}

Ths will be a large performace gain. Your code basically translates to:

public static String getBinary(int given) {
    char binNumbers[] = {'0','1'};
    String str = "";
    int rem;
    while (given > 0) {
        rem = given % 2;
        //str = binNumbers[rem] + str;
        // Code generated by compiler for the above line of code
        StringBuffer sb = new StringBuffer();
        sb.append(binNumbers[rem]);
        sb.append(str);
        str = sb.toString();
        // End of generated code
        given /= 2;
    }
    return str;
}

As you can see the code generated many objects which have to be cleaned up afterwards by the Garbage Collector.

Hint:

You can use Integer.toString(given, 2) instead of your Convert.getBinary(given) for the same result.


Writing a date checker in C I'm currently working on writing a date checker function in C and wondering how it can be improved upon.

  Q: Write a function that is passed a month, day, and year and will
  determine if that date is valid. You can assume each parameter is passed in as an
  integer.  Remember to check for leap year.

I currently have this and want to see if there are any beginner mistakes, if pointers could improve efficiency or redundancies in my code.

#include <stdio.h>
#include <stdbool.h>
bool is_valid_date(int month,int day ,int year) {
    //Validate year
    int days_on_month[]={31,28,31,30,31,30,31,31,30,31,30};
    if(year<0||year>2017) {
        //year not valid , stop validation , date is invalid
        return false;
    }
    bool isLeapYear;
    //Check for leap year , every 4 years there is a leap year
    if(year%4==0) {
        //if year is evenly divisble by 100 it must also be by 400
        if(year%100==0) {
            if(year%400==0) {
                isLeapYear=true;
            }else{
                isLeapYear=false;
            }
        }else{
            isLeapYear=true;
        }
    }
    //Validate months;
    if(month<1||month>12) {
        // not valid , stop validation , date is invalid
        return false;
    }
    //validate day
    if(month!=2) {
        //Valid Date
        if(day>=1&&day<=days_on_month[month-1]){return true;}
    }
    //February (changes no of days on leap year)
    else
    {
        if(isLeapYear) {
            if(day>=1&&day<=29){return true;}
        }else{
        if(day>=1&&day<=28){return false;}
        }
    }
    return false;
}

// test if function works
int main(void) {
    int day = 28;
    int month = 12;
    int year = 2012;
    printf("Is it true? Yes = 1 No = 0! = %d\n", is_valid_date(day, month, year));
    return 0;
}

Some quick formatting tips:

You use whitespace sparingly. In the example code, there is a lot of room for more whitespace that makes the code more readable. Something like:

if (year % 400 == 0) {
    isLeapYear = true;
}

It makes the code more easier to read.

Also, and this is not a recommendation but just an option you may want to consider, but you can move the if-else to a new line. I've always found that easier to read, and I've noticed that some (not all) teachers of mine tend to find that good formatting.

So for example:

if(year%400==0) {
    isLeapYear=true;
}else{
    isLeapYear=false;
}

This code can be rewritten as:

if(year%400==0) {
    isLeapYear=true;
}
else{
    isLeapYear=false;
}

**As an aside, upon receiving further advice from co-users on CodeReview in the comments, it is better to use curly braces, even if they are solely for one command in an if-statement, as it will help in detecting the scope of the if-statement with more ease.


Generating a unique reference number for logging purposes in a custom exception I've come up with a base class for exceptions for this SMS-based service I am working on. The basic idea behind the service is that a user sends a text message to a number, the message requests a particular action to be carried out by an agent that logs into a different website, from which some account information is scraped and spat back out to the user. Small take on QoL. Obviously, things don't always work as intended for different reasons.

When something goes wrong and the returned text message doesn't contain the requested information, I want the user to have a reference number to that error, should the issue persist. To accommodate for that I will keep logs, where I will quickly be able to look up what happened using the reference number. The logs will contain full error information.

Intended use of the custom exceptions:

try:
    service.login(credentials)
except AuthenticationError as e:
    sms_client.send(phone_number, f'{Something went wrong. If this keeps on happening, please contact us with this error ref: {e.reference}')

...

import logging
import uuid

logging.basicConfig(filename='errors.log', level=logging.ERROR)

class ServiceException(Exception):
    def __init__(self, message, url):
        self.reference = str(uuid.uuid4()).split('-')[0]
        error_message = f'{message}: \'{url}\' | error ref: {self.reference}'
        logging.error(error_message)
        super(Exception, self).__init__(error_message)

class AuthenticationError(ServiceException):
    """Ambiguous error prevented successful authentication."""

concerned if cutting the UUID short will keep it unique

Well, you're outputting 32 bits. So you can do ballpark 64k transactions (2^16) before running into Big Trouble, according to the birthday paradox.

If you realistically expect someone to report issues within a day, then the identifier is effectively (day, reference), which looks more attractive, as you can do some tens of thousands of transactions each day rather than over the lifetime of the service. More generally, resolve ties with "last one wins", and then you're comparing "typical distance between collisions" to "typical time for user to report an issue".

You're putting 4 bits of entropy into each hex nybble, which likely will be copy-n-pasted (or forwarded) by the user. If you're working within an 8-character constraint, consider using a scheme like base36 or base64 so each character sends more bits. Without pasting there will be usability limitations, such as eyeballing an 'l' and sending the digit '1'.

If you're not worried about users making up identifiers, consider using (userid, serial_num), or better: (userid, timestamp).


HackerRank Lower Bound-STL This is still my first week of learning C++, I solved the lower bound STL challenge in HackerRank Lower Bound STL but I'm having time exceeded issue for long input. I'm open to constructive criticism, ways to improve in terms of syntax, style and performance.

You are given \$N\$ integers in the sorted order. Then you are given
\$Q\$ queries. In each query, you will be given an integer and you have
to tell whether that integer is present in the array if so you have
to tell at which index it is present and if it is not present you have
to tell the index at which the smallest integer that is just greater
than the given number is present. Lower bound is a function that can
be used with a sorted vector.
Input Format
The first line of the input contains the number of integers \$N\$.
The next line contains \$N\$ integers in sorted order. The next line
contains \$Q\$, the number of queries. Then \$Q\$ lines follow each
containing a single integer \$Y\$.
If the same number is present multiple times, you have to print the first index at which it occurs. The input is such that you always have
an answer for each query.
Constraints

\$1 \leq N \leq 10^5\$
\$1 \leq X_i \leq 10^9\$, where \$X_i\$ is \$i^{th}\$ element in the    array
\$1 \leq Q \leq 10^5\$
\$1 \leq Y \leq 10^9\$

Output Format
For each query you have to print "Yes"(without the quotes) if the
number is present and at which index it is present separated by a
space. If the number is not present you have to print "No"(without the
quotes) followed by the index of the next smallest number just greater
than that number. You have to output each query in a new line.
Sample Input
 8
 1 1 2 2 6 9 9 15
 4
 1
 4
 9
 15

Sample Output
 Yes 1
 No 5
 Yes 6
 Yes 8

Here is my code
#include <cmath>
#include <cstdio>
#include <vector>
#include <iostream>
#include <algorithm>

int main() {
    /* Enter your code here. Read input from STDIN. Print output to STDOUT */
    int N;
    int query;
    int queryLength;
    {

    std::vector<int>::iterator low;
    std::vector<int>::iterator  foundValue;
    std::cin >> N;

    std::vector<int> v(N);
    //populate the vector
     for(int i =0; i<N; i++){
         std::cin >> v[i];
     }

    std::cin >> queryLength;

    for(int j = 0; j < queryLength; j++){
         std::cin >> query;
         foundValue= find(v.begin(), v.end(), query);
         if (foundValue != v.end())
         {
             std::cout << "Yes "<< (foundValue-v.begin()+ 1) << std::endl;
         }
          else{
            low = std::lower_bound (v.begin(), v.end(),query);
             std::cout << "No "<< find(v.begin(), v.end(), *low)-v.begin()+ 1 << std::endl;
          }
    }
}
    return 0;
}

Although you've probably already solved this problem, I'll still post my review for completion's sake.
1. Performance
Just as Loki said, you should use std::lower_bound() and std::distance() to solve this task, because std::find() has higher complexity.
2. General tips

Do not include unused libraries (in this case, <cmath> and <cstdio>).

Only declare variables when you need them. Declaring all the variables at the beginning of the program is old C style practice, which is discouraged in C++.

Prefer prefix to postfix operator++. The postfix version creates a copy, so if you do not need that copy, you should use the prefix version.

If you have access to C++11, you should consider using auto more. It'll make your code easier to read and save you some typing.

You can safely omit return 0 from main();

[C++11: 3.6.1/5]: A return statement in main has the effect of leaving the main function (destroying any objects with automatic storage duration) and calling std::exit with the return value as the argument. If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;.

Give your variables better names. Although other variable names in your code are good, a vector named v is not very descriptive.

Use const_iterators for functions that do not alter the original range.

3. Final code
#include <vector>
#include <iostream>
#include <algorithm>

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

    std::vector<int> numbers(N);
    //populate the vector
     for(int i = 0; i < N; ++i) {
         std::cin >> numbers[i];
     }

    int queryLength;
    std::cin >> queryLength;

    for(int j = 0; j < queryLength; ++j) {
        int query;
        std::cin >> query;
        auto lowerBoundIt = std::lower_bound(numbers.cbegin(), numbers.cend(), query);
        if(*lowerBoundIt == query) {
            std::cout << "Yes ";
        }
        else {
            std::cout << "No ";
        }
        std::cout << std::distance(numbers.cbegin(), lowerBoundIt) + 1 << '\n';
    }
}


Does using a lambda make this code more DRY? I started out with a function having code duplication like this:

    private static void GetAndSaveResources(string url)
    {
            ...

            if (url.Contains("[" + "access_id" + "]"))
            {
                RestCall.UriVariables.Add("access_id", 1);
            }

            if (url.Contains("[" + "affiliate_status" + "]"))
            {
                RestCall.UriVariables.Add("affiliate_status", "approved");
            }

            ...
    }

Then, because I had copy/pasted to make that code, I started trying to eliminate that duplication.

First I extracted a method:

    private static void ConfigureUriVariable(string url, string variable, object value)
    {
        if (url.Contains("[" + variable + "]"))
        {
            RestCall.UriVariables.Add(variable, value);
        }
    }

However, while that eliminated the duplication it also created a duplication of string url accross two functions so I futher reduced it to:

        Action<string,object> setUriVariable = (variable, value) =>
        {
            if (url.Contains("[" + variable + "]"))
            {
                RestCall.UriVariables.Add(variable, value);
            }
        };
        setUriVariable("access_id", 1);
        setUriVariable("affiliate_status", "approved");

However, I would have been confused by that code before I learned about lamdas so I wonder, was the last step a step in the wrong direction?

First, yes, I do think your solution with a lambda is a good choice. But it's worth to also consider other options:

Translate the closure into an object. That would mean creating a class, with a constructor that takes url (and possibly RestCall, if it's an instance property) and stores it into a filed. It also has a method that actually does the work. Basically, this is the same as you lambda, only much more code. I think this is probably not a good solution in this case.
Go the other way around: create a Dictionary<string, object> (or Tuple<string, object>[] if order is important for you; or an array of anonymous type) with your parameters and then iterate over that:

var items = new Dictionary<string, object>
{
    { "access_id", 1 },
    { "affiliate_status", "approved" }
};

foreach (var item in items)
{
    if (url.Contains("[" + item.Key + "]"))
    {
        RestCall.UriVariables.Add(item.Key, item Value);
    }
};


Binary Search tree deletion optimization I have just started implementing Binary Search tree on my own without using any tutorials online.
Please have a look and suggest how can I make code less cluttered and more faster. Right now I am using lots of if-else conditions and want to remove them as much as I can.

boolean deleteNode(int data, TreeNode node )
{
    if( data < node.key) deleteNode(data, node.left);
    else if( data > node.key) deleteNode(data, node.right);

    else {
        System.out.println("Value of node = " + node.key);
        //case 1 - Left and Right is null - Leaves
        if(node.left == null && node.right == null)
        {
            if(node == node.parent.left)node.parent.left =  null;
            else node.parent.right = null;
            System.out.println("Inside first if");
        }

        //case 2 - One child is null

        else if(node.left !=null && node.right ==null)

        {
            if(node == node.parent.left)node.parent.left = node.left;
            else node.parent.right = node.left;

        }
        else if(node.right !=null && node.left ==null)
        {
            if(node == node.parent.right)node.parent.right = node.right;
            else node.parent.left = node.right;
        }

        //case 3 - Delete node is an internal node

        else if(node.left !=null && node.right !=null)
        {
            TreeNode minNode = treeMin(node.right);

            if(minNode.parent == node)
            {
                if(node.parent == null)
                    {
                        node.key = minNode.key;
                        node.right = minNode.right;
                    }
                else{
                if(node == node.parent.left)
                    {
                        node.parent.left.key = minNode.key;
                        node.parent.right.right = minNode.right;

                    }
                if(node == node.parent.right)
                    {
                        node.parent.right.key = minNode.key;
                        node.parent.right.right = minNode.right;
                    }
                }
            }

            else
            {
                if(node == node.parent.left)
                {
                    node.parent.left.key = minNode.key;
                    node.left.right = minNode.right;

                }
                else if(node == node.parent.right)
                {

                    node.parent.right.key = minNode.key;
                    node.right.left = minNode.right;

                }
            }
        }

    }
    return true;

}

You are always returning true so I guess you don't really need current return value. In that case you might want to try a bit different approach.
You want to do something like:

 if( data < node.key) {
    node.left = deleteNode(data, node.left);
 } else if( data > node.key) {
    node.right = deleteNode(data, node.right);
 } else {
    //here return resulting node after deletion, null if node had no children
 }

That way you don't need to know anything about parent


Compute spherical distance matrix from list of geographical coordinates I've got a list of geographic coordinates ([lat, long]) and want to compute the corresponding matrix of distances. Distance shall be spherical distance in km.

def distance_on_unit_sphere(coord1, coord2):
    lat1 = coord1[0]
    long1 = coord1[1]
    lat2 = coord2[0]
    long2 = coord2[1]
    # Convert latitude and longitude to spherical coordinates in radians.
    degrees_to_radians = math.pi/180.0
    phi1 = (90.0 - lat1)*degrees_to_radians
    phi2 = (90.0 - lat2)*degrees_to_radians
    theta1 = long1*degrees_to_radians
    theta2 = long2*degrees_to_radians
    # Compute spherical distance from spherical coordinates.
    cos = (math.sin(phi1)*math.sin(phi2)*math.cos(theta1 - theta2) +
           math.cos(phi1)*math.cos(phi2))
    arc = math.acos( cos )
    # Multiply with radius of the earth in km (GRS 80-Ellipsoid)
    distance = arc * 6371.007176
    return distance

import math
from collections import OrderedDict

    coordinates_dict = {u'YALE UNIVERSITY': [41.3111, -72.9267],
                        u'YONSEI UNIVERSITY': [37.5664, 126.939],
                        u'YORK UNIVERSITY': [43.7731, -79.5036],
                        u'YUAN ZE UNIVERSITY': [24.9697, 121.267],
                        u'ZHEJIANG UNIVERSITY': [30.2636, 120.121],
                        u'ZHONGNAN UNIVERSITY OF ECONOMICS AND LAW': [30.4752, 114.394]}

    distance_dict = {}

    for university in coordinates_dict.keys():
        distance_dict[university] = OrderedDict()
        coord1 = coordinates_dict[university]
        for other_university in coordinates_dict.keys():
            coord2 = coordinates_dict[other_university]
            distance = distance_on_unit_sphere(coord1, coord2)
            distance_dict[university][other_university] = distance

While distance_on_unit_sphere() is certainly already optimal (I got it from johndcook.com), I think the process to compute the matrix could be sped up. Right now I use a nested loop, I believe that's quite inefficient, right?

With numpy and its meshgrid functions, you don't have to do any looping at all, although under the hood looping similar to yours is going on (but in C, and thus much faster).

Here's a numpy version of your function:

import numpy as np

def distance_on_sphere_numpy(coordinate_array):
    """
    Compute a distance matrix of the coordinates using a spherical metric.
    :param coordinate_array: numpy.ndarray with shape (n,2); latitude is in 1st col, longitude in 2nd.
    :returns distance_mat: numpy.ndarray with shape (n, n) containing distance in km between coords.
    """
    # Radius of the earth in km (GRS 80-Ellipsoid)
    EARTH_RADIUS = 6371.007176

    # Unpacking coordinates
    latitudes = coordinate_array[:, 0]
    longitudes = coordinate_array[:, 1]

    # Convert latitude and longitude to spherical coordinates in radians.
    degrees_to_radians = np.pi/180.0
    phi_values = (90.0 - latitudes)*degrees_to_radians
    theta_values = longitudes*degrees_to_radians

    # Expand phi_values and theta_values into grids
    theta_1, theta_2 = np.meshgrid(theta_values, theta_values)
    theta_diff_mat = theta_1 - theta_2

    phi_1, phi_2 = np.meshgrid(phi_values, phi_values)

    # Compute spherical distance from spherical coordinates
    angle = (np.sin(phi_1) * np.sin(phi_2) * np.cos(theta_diff_mat) +
           np.cos(phi_1) * np.cos(phi_2))
    arc = np.arccos(angle)

    # Multiply by earth's radius to obtain distance in km
    return arc * EARTH_RADIUS

In addition to changing things to be compatible with numpy, there were several other stylistic and efficiency notes:

I renamed the function to eliminate the unit word from its name, which was misleading: you are multiplying the Earth's radius, so the result is not distances on a unit sphere but rather on an Earth-sized sphere.
I added docstrings to explain the function.
Rather than have the "magic number" 6371.007176 in the bottom of the code, I moved it to the top, defined it as a variable with an all-caps name.  This make it more apparent that the function relies on this value and is consistent with recommended Python PEP8 style best practices.  The units (km) in particular may not be apparent to users of the code.
It is obviously inefficient to compute every distance twice, as my version currently does.  However, your version also double-counted, which would be difficult to avoid without storing a copy of the keys in your dictionary in a list or tuple, so you could enumerate() them and then make the second loop go only from the index of the outer loop on up.
For easy array-based trigonometry, I switched the calls to math.cos() and math.acos() to np.cos() and np.arccos().  After that the only import I needed from math was pi, but as Jaime pointed out that is available from numpy, so I just used that version and eliminated all imports of anything from math.

This function is usable like this:

coordinates_dict = {u'YALE UNIVERSITY': [41.3111, -72.9267],
                    u'YONSEI UNIVERSITY': [37.5664, 126.939],
                    u'YORK UNIVERSITY': [43.7731, -79.5036],
                    u'YUAN ZE UNIVERSITY': [24.9697, 121.267],
                    u'ZHEJIANG UNIVERSITY': [30.2636, 120.121],
                    u'ZHONGNAN UNIVERSITY OF ECONOMICS AND LAW': [30.4752, 114.394]}

coordinates_array = np.array([(val[0], val[1]) for key, val in coordinates_dict.iteritems()])

distance_on_sphere_numpy(coordinates_array)
Out[2]:
array([[  1.34258937e-04,   1.20828584e+04,   1.16387852e+04,
          1.15437577e+04,   1.05861981e+04,   6.04128344e+02],
       [  1.20828584e+04,   0.00000000e+00,   9.11965962e+02,
          5.99372703e+02,   1.50000489e+03,   1.25010601e+04],
       [  1.16387852e+04,   9.11965962e+02,   0.00000000e+00,
          5.49877343e+02,   1.39741343e+03,   1.19974669e+04],
       [  1.15437577e+04,   5.99372703e+02,   5.49877343e+02,
          0.00000000e+00,   1.02655483e+03,   1.19442021e+04],
       [  1.05861981e+04,   1.50000489e+03,   1.39741343e+03,
          1.02655483e+03,   0.00000000e+00,   1.10150349e+04],
       [  6.04128344e+02,   1.25010601e+04,   1.19974669e+04,
          1.19442021e+04,   1.10150349e+04,   0.00000000e+00]])

By adding your code into its own function, I was able to compare timing between the numpy version and your version:

import math
from collections import OrderedDict
import numpy as np

def distance_on_unit_sphere(coord1, coord2):
    lat1 = coord1[0]
    long1 = coord1[1]
    lat2 = coord2[0]
    long2 = coord2[1]
    # Convert latitude and longitude to spherical coordinates in radians.
    degrees_to_radians = math.pi/180.0
    phi1 = (90.0 - lat1)*degrees_to_radians
    phi2 = (90.0 - lat2)*degrees_to_radians
    theta1 = long1*degrees_to_radians
    theta2 = long2*degrees_to_radians
    # Compute spherical distance from spherical coordinates.
    cos = (math.sin(phi1)*math.sin(phi2)*math.cos(theta1 - theta2) +
           math.cos(phi1)*math.cos(phi2))
    arc = math.acos( cos )
    # Multiply with radius of the earth in km (GRS 80-Ellipsoid)
    distance = arc * 6371.007176
    return distance

def compute_distance_mat(coordinates_dict):
    distance_dict = {}
    for university in coordinates_dict.keys():
        distance_dict[university] = OrderedDict()
        coord1 = coordinates_dict[university]
        for other_university in coordinates_dict.keys():
            coord2 = coordinates_dict[other_university]
            distance = distance_on_unit_sphere(coord1, coord2)
            distance_dict[university][other_university] = distance
    return distance_dict

coordinates_dict = {u'YALE UNIVERSITY': [41.3111, -72.9267],
                    u'YONSEI UNIVERSITY': [37.5664, 126.939],
                    u'YORK UNIVERSITY': [43.7731, -79.5036],
                    u'YUAN ZE UNIVERSITY': [24.9697, 121.267],
                    u'ZHEJIANG UNIVERSITY': [30.2636, 120.121],
                    u'ZHONGNAN UNIVERSITY OF ECONOMICS AND LAW': [30.4752, 114.394]}

import timeit
%timeit compute_distance_mat(coordinates_dict)

%timeit distance_on_sphere_numpy(coordinates_array)
10000 loops, best of 3: 125 µs per loop
10000 loops, best of 3: 40.8 µs per loop

So numpy is already three times faster for this small test case.


Simple calculator in C# It is a basic calculator where the user inputs two numbers, and an operation and the program makes it into an equation and gets the answer.  For example, if the user enters number 2 number 3 and tells it to multiply, it will display the answer as 6.  I just want to know how this could be improved.

Could I have used loops or more advanced procedures to get the same output?

 namespace Test
 {
    class Program
    {
            static void Main(string[] args)
            {
                int firstNum;
                int secondNum;                   //Variables for equation
                string operation;
                int answer;

                Console.WriteLine("Hello, welcome to Alex's basic calculator!");
                Console.ReadLine();

                Console.Write("Enter the first number in your basic equation: ");
                firstNum = Convert.ToInt32(Console.ReadLine());

                //User input for equation
                Console.Write("Now enter your second number in the basic equation: ");
                secondNum = Convert.ToInt32(Console.ReadLine());
                Console.Write("Ok now enter your operation ( x , / , +, -) ");
                operation = Console.ReadLine();

                if (operation == "x")
                {
                   answer = firstNum * secondNum;
                   Console.WriteLine(firstNum + " x " + secondNum + " = " + answer);
                   Console.ReadLine();
                }
                else if (operation == "/")
                {
                   answer = firstNum / secondNum;
                   Console.WriteLine(firstNum + " / " + secondNum + " = " + answer);
                   Console.ReadLine();
                }
                //Getting answers
                else if (operation == "+")
                {
                   answer = firstNum + secondNum;
                   Console.WriteLine(firstNum + " + " + secondNum + " = " + answer);
                   Console.ReadLine();
                }
                else if (operation == "-")
                {
                   answer = firstNum - secondNum;
                   Console.WriteLine(firstNum + " - " + secondNum + " = " + answer);
                   Console.ReadLine();
                }
                else
                {
                   Console.WriteLine("Sorry that is not correct format! Please restart!");     //Catch
                   Console.ReadLine();
                }
           }
     }
}

I'm going to go over a few things in this answer, hopefully it makes them simple enough. Then, at the end, I'm going to go way overboard and significantly over-engineer this programme while still making it shorter.

First, let's talk about your input handling (or lack thereof). As programmers, we should strive to be able to gracefully handle any/all input, and do something appropriate as a result. For invalid input, usually we reprompt (sometimes we just gracefully fail), for valid input, we go to the next step.

You assume all input will be valid, which is a huge potential for problematic behavior. What happens if I enter one for either number? Bad things, my friend, bad things.

So, let's handle all that gracefully. I wrote a specific portion of a library I have written specifically for input handling, and gracefully doing so.

Basically, you need all three files from this GitHub folder. You will also need both the bottom files, from this other GitHub folder. You'll want to modify the namespaces to suit your structure. (You may also just download the entire repo, and build Evbpc.Framework, which I recommend. All of this is beyond the scope of this answer.)

Next, you'll want to create a ConsolePrompt object:

var prompter = new ConsolePrompt(null);

Once you've done that, you can simply use:

var validOperations = new[] { "x", "/", "+", "-" };
var firstNum prompter.Prompt<int>("Enter the first number in your basic equation", PromptOptions.Required);
var secondNum = prompter.Prompt<int>("Now enter your second number in the basic equation", PromptOptions.Required);
var operation = prompter.Prompt<string>("Ok now enter your operation (" + string.Join(", ", validOperations) + ")", PromptOptions.Required, validationMethod: x => validOperations.Contains(x));

Now we've done three things here:

We validate all the numbers and reprompt the user if the number is invalid.
We make sure the user only entered a basic operation for the intended operation.
We make a list of valid operations at the beginning, which means if we add one we can do so easily.

Next, we'll talk about your conditions. You use an if statement for all your operations, but that's unnecessary and creates extra processing in the case that the operation is the last condition. (Each previous if is evaluated before it continues to the next.)

Instead, we'll go to a switch statement:

switch (operation)
{
    case "x":
        answer = firstNum * secondNum;
        break;
    case "/":
        answer = firstNum / secondNum;
        break;
    case "+":
        answer = firstNum + secondNum;
        break;
    case "-":
        answer = firstNum - secondNum;
        break:
}

By using a switch we've made things substantially more clear: our intention is to only evaluate what operation is and do something based on it.

Next, we'll talk about your output. You repeat certain statements excessively, let's find a better way to do that.

We have firstNum and secondNum, as well as operation and answer. So how can we rewrite our Console.WriteLine methods to be more dynamic? Well, with the switch it's easy, right after the end brace for the switch we just write what each value was:

Console.WriteLine(firstNum + " " + operation + " " + secondNum + " = " + answer);

Boom. We don't need the Console.WriteLine in each switch (or if) block. We just need one at the end.

But there's a better way to write this (with C#6.0, that is):

Console.WriteLine($"{firstNum} {operation} {secondNum} = {answer}");

Using string interpolation we make it much easier to read what's going on.

You also had the idea for a while loop in your head to repeat the calculations, so let's do that:

do
{
    // All our calculator code
} while (prompter.Prompt("Enter exit to quit, anything else to continue", PromptOptions.Optional, "", parseResultMethod: x => x.ToLowerInvariant()) != "exit");

And now it will continue until the user types exit at that prompt.

So, we've improved everything you've got going, but there's still one more improvement we can make: convert all our operations to a dictionary.

What do I mean? Well I'll show you.

var operations = new Dictionary<string, Func<int, int, int>>
{
    {"x", (x, y) => x * y },
    {"/", (x, y) => x / y },
    {"+", (x, y) => x + y },
    {"-", (x, y) => x - y }
};
var operation = prompter.Prompt<string>("Ok now enter your operation (" + string.Join(", ", operations.Keys) + ")", PromptOptions.Required, null, null, x => operations.Keys.Contains(x));
var firstNum = prompter.Prompt<int>("Enter the first number in your basic equation", PromptOptions.Required);
var secondNum = prompter.Prompt<int>("Enter the second number in your basic equation", PromptOptions.Required);
var answer = operations[operation](firstNum, secondNum);

What did we just do? We just replaced the entire switch/if blocks, all the manually typing x, /, +, -, etc. with a dictionary of string -> function. Where does this benefit us? Simple. I want to add exponential calculation.

Simply add:

{"^", (x, y) => (int)Math.Pow(x, y)}

to our operations dictionary. Now all our prompts and whatnot can support the power function.

This brings up a new issue: you have no support for decimal/double/float numbers. This is really easy to modify, simply replace all our int types with double.

Lastly, as for a bonus to this answer, you can also add alias operations. (What do I mean? Well what if I naturally use * instead of x for multiplication?)

This is super easy:

// Let's add our alias operations, */x
operations.Add("*", (x, y) => operations["x"](x, y));

Simply add them after the var operations statement.

var operations = new Dictionary<string, Func<double, double, double>>
{
    {"x", (x, y) => x * y },
    {"/", (x, y) => x / y },
    {"+", (x, y) => x + y },
    {"-", (x, y) => x - y },
    {"^", (x, y) => Math.Pow(x, y) },
    {"%", (x, y) => x % y }
};

// Let's add our alias operations, */x
operations.Add("*", (x, y) => operations["x"](x, y));

Boom, now you can use * instead of x for multiplication. (But you can still use x as well.)

Our entire programme is now down to 29 lines with appropriate whitespace, and supports much more than your original did. (Your original was 44 lines.)

var prompter = new ConsolePrompt(null);

Console.WriteLine("Hello, welcome to Alex's basic calculator!");
Console.ReadLine();

var operations = new Dictionary<string, Func<double, double, double>>
{
    {"x", (x, y) => x * y },
    {"/", (x, y) => x / y },
    {"+", (x, y) => x + y },
    {"-", (x, y) => x - y },
    {"^", (x, y) => Math.Pow(x, y) },
    {"%", (x, y) => x % y }
};

// Let's add our alias operations, */x
operations.Add("*", (x, y) => operations["x"](x, y));

do
{
    var firstNum = prompter.Prompt<double>("Enter the first number in your basic equation", PromptOptions.Required);
    var secondNum = prompter.Prompt<double>("Enter the second number in your basic equation", PromptOptions.Required);
    var operation = prompter.Prompt<string>("Ok now enter your operation (" + string.Join(", ", operations.Keys) + ")", PromptOptions.Required, null, null, x => operations.Keys.Contains(x));

    var answer = operations[operation](firstNum, secondNum);

    Console.WriteLine($"{firstNum} {operation} {secondNum} = {answer}");
    Console.ReadLine();
} while (prompter.Prompt("Enter exit to quit, anything else to continue", PromptOptions.Optional, "", parseResultMethod: x => x.ToLowerInvariant()) != "exit");

Pretty cool, eh?

All-in-all, very good start. Hopefully you learned a lot from this answer and can apply some of it to future applications. :)


Automatically remove O365 license from AD user when using powershell As you can see I have been using multiple conditions such as if, elseif, else, but it takes too much time to complete the process.

How can I improve the speed of the script?

Import-Module ActiveDirectory
Import-Module MSOnline

$password = ConvertTo-SecureString 'PASSWORD' -AsPlainText -Force
$LiveCred = New-Object System.Management.Automation.PSCredential ("username@domain.com", $password)
New-PSSession -ConfigurationName Microsoft.Exchange -ConnectionUri https://ps.outlook.com/powershell/ -Credential $LiveCred -Authentication Basic -AllowRedirection

Connect-MsolService -Credential $Livecred

$userID = Import-Csv "c:\export\list.csv"
$LogFile = "C:\export\logs.txt"
foreach ($user in $userID)
{

    $ADuser = Get-ADUser -Filter "EmployeeId -eq $($user.EmployeeID)" -Properties whenCreated, Enabled, SAMAccountName
    $O365User = Get-MsolUser -UserPrincipalName $ADuser.UserPrincipalName

    if (($ADuser.Enabled -eq $true) -and ($O365User.isLicensed -eq $true))
    {
        Get-MsolUSer -UserPrincipalName $ADuser.UserPrincipalName
        Set-MsolUserLicense -UserPrincipalName $ADuser.UserPrincipalName -RemoveLicenses "company:ENTERPRISEPACK"
        #move user OU, change description, disable account, remove SG members list
        #$Date = Get-Date -Format MM-dd-yyyy
        #Set-ADUser -Identity $ADuser.SAMAccountName -Replace @{info="User disabled at $Date"}
        # $User has been disabled and remove office 365 licence from user.
        #"$ADuser.SAMAccountName has been disabled and remove office 365 licence from user." | Out-File $LogFile -Append -Force

    }
    elseif (($ADuser.Enabled -eq $true) -and ($O365User.isLicensed -eq $false))
    {
        #move user OU, change description, disable account, remove SG members list
        #Set-ADUser username -Replace @{info='New info for the notes field'}
        #"$ADuser.SAMAccountName has been disabled and already without office 365 licence." | Out-File $LogFile -Append -Force

    }
    elseif (($ADuser.Enabled -eq $false) -and ($O365User.isLicensed -eq $false))
    {
        #do nothing
        #"$ADuser.SAMAccountName has been already disabled and already without office 365 licence." | Out-File $LogFile -Append -Force

    }
    elseif (($ADuser.Enabled -eq $false) -and ($O365User.isLicensed -eq $true))
    {
        Get-MsolUSer -UserPrincipalName $ADuser.UserPrincipalName
        Set-MsolUserLicense -UserPrincipalName $ADuser.UserPrincipalName -RemoveLicenses "company:ENTERPRISEPACK"
        #"$ADuser.SAMAccountName has been already disabled and but revoke office 365 licence." | Out-File $LogFile -Append -Force

    }
    else
    {
        #User does not exist in AD
        #"$user.EmployeeID does not exist in Active Directory." | Out-File $LogFile -Append -Force

    }

}

Not entirely sure if the code you have posted is exactly what you want reviewed but I am going to write this based on what I see.

Clear text passwords

Have to cover this one first to get it out of the way. I see that you have hardcoded a password. This is a bad practice of security though obscurity. Anyone opening the script via text will be able to see your credential set. While this is not always ideal I would suggest that you have your script prompt the user for credentials using Get-Credential.

Empty if clauses

  As you can see I have been using multiple conditions such as if, elseif, else

Yes, I can see that. However most of these blocks are empty with comments stating "Do nothing".

elseif (($ADuser.Enabled -eq $false) -and ($O365User.isLicensed -eq $false))
{
    #do nothing
    #"$ADuser.SAMAccountName has been already disabled and already without office 365 licence." | Out-File $LogFile -Append -Force

}

Why bother wasting CPU cycles on something that does not matter? I assume at one point you had code in there but now you don't so just remove those empty clauses.

Repeating code

At the start of your loop pass you have a call to Get-MsolUser.

$O365User = Get-MsolUser -UserPrincipalName $ADuser.UserPrincipalName

Then inside your loops you make that call again but it just go to standard output. Why bother getting the same information again? Nothing would have changed in that time. I would just removed those lines in your if clauses. If you wanted to see it on screen you could just write the line $O365User and that would save calling the cmdlet again.

Reducing if complexity

So of the remaining if clauses that acutally do something they are both doing the same thing! Removing a license via -RemoveLicenses "company:ENTERPRISEPACK". You seem to be doing this regardless of the user being enabled or not.

This is another part of the code that make me wonder if this is what you're actually doing or if you scrubbed it too much for this question. Either way I am taking if for face value here.

Instead of checking whether the user is enabled or not simply filter for $O365User.isLicensed and process those.

$userID = Import-Csv "c:\export\list.csv"
$LogFile = "C:\export\logs.txt"
foreach ($user in $userID)
{
    $ADuser = Get-ADUser -Filter "EmployeeId -eq $($user.EmployeeID)" -Properties whenCreated, Enabled, SAMAccountName
    $O365User = Get-MsolUser -UserPrincipalName $ADuser.UserPrincipalName
    if ($O365User.isLicensed){Set-MsolUserLicense -UserPrincipalName $ADuser.UserPrincipalName -RemoveLicenses "company:ENTERPRISEPACK"}
}

Error prevention

In your code and my snippet there is not error prevention built in. If there was a bad entry in the CSV or something along those lines the script would fail if it did not find a user. Consider using -ErrorAction SilentlyContinue and then checking if the variables contain data.

$ADuser = Get-ADUser -Filter "EmployeeId -eq $($user.EmployeeID)" -ErrorAction SilentlyContinue
if($ADUser){
    # Process as normal
} else {
    Write-Warning "No matching user for $($user.EmployeeID) was located."
}

You will notice that I dropped the properties parameter from your Get-ADUser call. Enabled and samaccountname are returned by default. While whenCreated is not, you don't use it either. True, the return in performance is likely not measurable - but if you don't use it just remove it.

Jobs and workflows

This can be a large subject so I will touch on it here. If you have 1000's of these entries to process you might want to have a look at jobs or parallel processing:

https://stackoverflow.com/questions/4016451/can-powershell-run-commands-in-parallel
https://stackoverflow.com/questions/8781666/run-n-parallel-jobs-in-powershell
about_foreach_parallel


How to make Project Euler 32 in Python faster? We shall say that an n-digit number is pandigital if it makes use of
  all the digits 1 to n exactly once; for example, the 5-digit number,
  15234, is 1 through 5 pandigital.

  The product 7254 is unusual, as the identity, 39 × 186 = 7254,
  containing multiplicand, multiplier, and product is 1 through 9
  pandigital.

  Find the sum of all products whose multiplicand/multiplier/product
  identity can be written as a 1 through 9 pandigital.

  HINT: Some products can be obtained in more than one way so be sure to
  only include it once in your sum.

I need help on making this code faster. Right now I'm using double nested for-loops; is there a way I can change that?

from timeit import default_timer as timer

start = timer()
products = set()
for a in range(12, 100):
    for b in range(102, 1000): # to get 9-digit: 2-digit * 3-digit
        product = a * b
        digits = list(str(a) + str(b) + str(product))
        digits = [int(x) for x in digits]
        if sorted(digits, key = int) == range(1, 10):
            products.add(product)

for a in range(1, 10):
    for b in range(1023, 10000): # other option is 1-digit * 4-digit
        product = a * b
        digits = list(str(a) + str(b) + str(product))
        digits = [int(x) for x in digits]
        if sorted(digits, key = int) == range(1, 10):
            products.add(product)

ans = sum(list(products))
elapsed_time = (timer() - start) * 1000 # s --> ms

print "Found %d in %r ms." % (ans, elapsed_time)

Since you know that all of the non-zero digits must be present in the answer, you can skip most of the numbers in the range.  That is, it's pointless to do a test multiplication of 22*3333 because those both repeat digits.

So what is needed is an efficient way to run through all of the permutations of digits quickly, and fortunately, Python's itertools provides exactly that.

import itertools
start = timer()
products = set()
def makeint(n):
    return int(''.join(map(str,n)))

target = range(1,10)
d = itertools.permutations(target, 5)
for a in d:
    product = makeint(a[:2])*makeint(a[2:])
    if sorted(list(a)+map(int,str(product))) == target:
        products.add(product)
    product = makeint(a[:1])*makeint(a[1:])
    if sorted(list(a)+map(int,str(product))) == target:
        products.add(product)

ans = sum(list(products))

The way this works is to pick off permutations 5 digits at a time and then group as 2 and 3 or as 1, 4 as you have done.  There are other refinements one can make, such as skipping every number which ends in a 5 (because 5*n will always end in either 0, which is not in the range, or 5 which would be a duplicate digit), but I'll leave such refinements to you.

As it stands, this code runs almost 8x faster than the original.


Creating a list of list after traversing binary search tree in level order Here is my code.

/**
 * Definition for a binary tree node.
 * public class TreeNode {
 *     int val;
 *     TreeNode left;
 *     TreeNode right;
 *     TreeNode(int x) { val = x; }
 * }
 */
public class Solution {
 public List<List<Integer>> levelOrder(TreeNode root) {
        List<List<Integer>> list = new LinkedList<List<Integer>>();
        if (root == null) {
            return list;
        } else {
            Queue<TreeNode> q = new LinkedList<>();
            q.add(root);
            while (!q.isEmpty()) {
                int size = q.size();
                LinkedList<Integer> layers = new LinkedList<>();
                while (size != 0) {
                    TreeNode current = q.poll();
                    layers.add(current.val);
                    if (current.left != null) {
                        q.add(current.left);
                    }
                    if (current.right != null) {
                        q.add(current.right);
                    }
                    size--;
                }

                list.add(layers);

            }
        }
        return list;

    }

}

Please let me know if any improvement/correction.

Nicely done. I especially like how you used the size of the queue to know how many items are there on the level. I have only minor improvement ideas.

You can simplify this declaration:

List<List<Integer>> list = new LinkedList<List<Integer>>();

Using the diamond operator <>, like you already did at other places:

List<List<Integer>> list = new LinkedList<>();

The code is deeply indented. You could reduce the indent level by eliminating the else branch of the outermost condition:

    if (root == null) {
        return list;
    }
    Queue<TreeNode> q = new LinkedList<>();
    q.add(root);
    // ...

The while loop could be written as a for loop. The benefit is that it will be harder to forget to decrement size.

Lastly, some of the names could be better.

Instead of "layers", I propose singular "layer"
Instead of "size", I propose "count"


Two java classes with different annotation values only Consider two classes:

First

@Path("/first/{param}") // 1st line
public class First {

    @GET
    @Path("/{data}/difference") // 5th line
    public Response doStuff(
            @Context HttpServletRequest request,
            @Context UriInfo uri,
            @Context HttpHeaders httpHeaders,
            @PathParam("param") String param,
            @PathParam("data") String data
    ) {

        // whatever
        return new Response();

    }

}

Second

@Path("/second/{param}") // 1st line
public class Second extends First {

    @GET
    @Path("/{data}") // 5th line
    public Response doStuff(
            @Context HttpServletRequest request,
            @Context UriInfo uri,
            @Context HttpHeaders httpHeaders,
            @PathParam("param") String param,
            @PathParam("data") String data
    ) {

        return super.doStuff(
            request,
            uri,
            httpHeaders,
            param,
            data
        );

    }

}

The only differences between this two block of code are:

@Path() annotation value in the 1st line of both of them
@Path() annotation value in the 5th line of both of them

How can I remove all the redundant code from it?

You can parameterize the @path annotation with regular expressions to specify one class/method that accepts multiple paths

@Path("/{firstpath: first|second}/{param}") // will match /first/{param} and /second/{param}
public class First {

    @GET
    @Path("/{data}{p:/?}{difference:(\w*)}") // {p:/?} makes "/" optional, {difference:(\w*)} allows for optional text
    public Response doStuff(
            @Context HttpServletRequest request,
            @Context UriInfo uri,
            @Context HttpHeaders httpHeaders,
            @PathParam("param") String param,
            @PathParam("data") String data
    ) {

        // whatever
        return new Response();

    }

}


static_new and static_delete Here's a trick for preallocating some memory for some type T. Do there exist similar tricks? Improvements?

#include <cassert>

#include <cstdlib>

#include <atomic>

#include <limits>

#include <new>

#include <thread>

#include <utility>

namespace
{
  template <typename T, typename A = unsigned>
  struct static_store
  {
    static constexpr auto const max_instances =
      ::std::numeric_limits<unsigned char>::digits * sizeof(A);

    static void cleanup() { delete [] store_; }

#ifdef __GNUC__
    template <typename U>
    static int ffz(U const v)
    {
      return __builtin_ctzll(~v);
    }
#elif _MSC_VER && !__INTEL_COMPILER
    template <typename U>
    static int ffz(U const v)
    {
      return ::std::numeric_limits<unsigned char>::digits * sizeof(v) -
        __lzcnt64(v & -v);
    }
#elif __INTEL_COMPILER
    template <typename U>
    static int ffz(U const v)
    {
      return _bit_scan_forward(~v);
    }
#else
    template <typename U>
    static int ffz(U v)
    {
      decltype(ffz()) b{};

      for (; (v & 1); ++b)
      {
        v >>= 1;
      }

      return b;
    }
#endif

    static ::std::atomic_flag lock_;

    static A memory_map_;

    static typename ::std::aligned_storage<sizeof(T),
      alignof(T)>::type* store_;
  };

  template <typename T, typename A>
  ::std::atomic_flag static_store<T, A>::lock_;

  template <typename T, typename A>
  A static_store<T, A>::memory_map_;

  template <typename T, typename A>
  typename ::std::aligned_storage<sizeof(T), alignof(T)>::type*
    static_store<T, A>::store_{(::std::atexit(static_store<T, A>::cleanup),
      new typename ::std::aligned_storage<sizeof(T),
        alignof(T)>::type[static_store<T, A>::max_instances])};

  template <typename T, typename ...A>
  inline T* static_new(A&& ...args)
  {
    using static_store = static_store<T>;

    while (static_store::lock_.test_and_set(::std::memory_order_acquire))
    {
      ::std::this_thread::yield();
    }

    auto const i(static_store::ffz(static_store::memory_map_));

    auto p(new (&static_store::store_[i]) T(::std::forward<A>(args)...));

    static_store::memory_map_ |= 1ull << i;

    static_store::lock_.clear(::std::memory_order_release);

    return p;
  }

  template <typename T>
  inline void static_delete(T* const p)
  {
    using static_store = static_store<T>;

    auto const i(p - static_cast<T*>(static_cast<void*>(
      static_store::store_)));
    //assert(!as_const(static_store::memory_map_)[i]);

    while (static_store::lock_.test_and_set(::std::memory_order_acquire))
    {
      ::std::this_thread::yield();
    }

    static_store::memory_map_ &= ~(1ull << i);

    static_cast<T*>(static_cast<void*>(&static_store::store_[i]))->~T();

    static_store::lock_.clear(::std::memory_order_release);
  }
}

Usage:

auto const p(static_new<int>());
*p = 10;
static_delete(p);

The code uses an integral type (A), as a bit mask, where each cleared bit means, that a corresponding slot in the store_ is not occupied, and a set bit means it is occupied. This is done to minimize the storage requirements for the allocation mechanism. To find a free slot, the ffz function (find first zero) is used: it is compiled into a single instruction on many platforms. The same thing could have been done with an array of bools, but with greatly increased storage requirements.

I still have trouble understanding how the whole works, but there are still some small things that could be improved:

In your comments, you say that A is an integer type used as a bitmask. The safest bitmask types are unsigned types or std::bitset. You should make this condition clear in your code, write

  template <typename T, typename BitmaskType=unsigned>
  struct static_store { /* ... */ };

You could even make it an error to use a nonconforming type and create a is_bitmask trait, which would allow you to create a meaningful error message:

static_assert(is_bitmask<BitmaskType>::value,
              "the BitmaskType requirements are not satisfied");

I got confused with these lines:

template <typename T, typename ...A>
inline T* static_new(A&& ...args)

It is once again a naming problem: I had in mind the template parameter A from static_store. Since it represents arguments to be forwarding, you should just use the common convetion and name it Args instead. I don't know whether there are written guidelines to name variadic parameter packs, but Args is probably the name that I saw the most often.
You could probably slightly hide details and reduce the visual burden by using a typedef for the type of store_:

using store_type = typename ::std::aligned_storage<sizeof(T), alignof(T)>::type;


Initialize bunch of variables in constructor by extracting value from dictionary I have a headers dictionary value being passed to one of my constructors and based on that dictionary value I am extracting individual headers I need and setting all my variables.

public Tester(IDictionary<string, StringValues> headers)
 {
     int textId = 0;

     if (headers == null)
     {
         ProcId = GenerateProcId();
         DbId = GenerateDbId();
         ThetaId = GenerateThetaId();
         return;
     }

     ProcIdHeader = GetHeaderValue(headers, PROC_HEADER);
     if (string.IsNullOrWhiteSpace(ProcIdHeader))
     {
         ProcId = GenerateProcId();
     }
     else
     {
         ProcId = ProcIdHeader;
     }

     DbIdHeader = GetHeaderValue(headers, DB_HEADER);
     if (string.IsNullOrWhiteSpace(DbIdHeader))
     {
         DbId = GenerateDbId();
     }
     else
     {
         DbId = DbIdHeader;
     }

     ThetaId = GenerateThetaId();

     var textIdStr = GetHeaderValue(headers, TEXT_HEADER);
     int.TryParse(textIdStr, out textId);
     TextIdValue = textId;
     TextId = textId;

     AlphaHeader = GetHeaderValue(headers, ALPHA_HEADER);
     if (!string.IsNullOrWhiteSpace(AlphaHeader))
     {
         AlphaContent = AlphaHeader;
     }

     TimeoutIdHeader = GetHeaderValue(headers, TIMEOUT_HEADER);
     if (!string.IsNullOrWhiteSpace(TimeoutIdHeader))
     {
         TimeoutId = TimeoutIdHeader;
     }

     FilterHeader = GetHeaderValue(headers, FILTER_HEADER);
     if (!string.IsNullOrWhiteSpace(FilterHeader))
     {
         FilterId = FilterHeader;
     }

     PreviewHeader = GetHeaderValue(headers, PREVIEW_HEADER);
     if (!string.IsNullOrWhiteSpace(PreviewHeader))
     {
         PreviewId = PreviewHeader;
     }

     AntHeader = GetHeaderValue(headers, ANT_HEADER);
     if (!string.IsNullOrWhiteSpace(AntHeader))
     {
         AntId = AntHeader;
     }
     LocalHeader = GetHeaderValue(headers, LOCAL_HEADER);
     AppHeader = GetHeaderValue(headers, APP_HEADER);
     if (!string.IsNullOrWhiteSpace(AppHeader))
     {
         AppId = AppHeader;
     }

     ContextHeader = GetHeaderValue(headers, CONTEXT_HEADER);
     if (ContextHeader != null)
     {
         try
         {
             ContextStr = JsonSerializer.Deserialize<ContextStr>(ContextHeader);
         }
         catch (Exception e)
         {
             ContextStr = new ContextStr();
         }
     }
     if (LocalHeader != null)
     {
         try
         {
             LocalStr = JsonSerializer.Deserialize<LocalStr>(LocalHeader);
         }
         catch (Exception e)
         {
             LocalStr = new LocalStr();
         }
     }
     PageHeader = GetHeaderValue(headers, PAGE_HEADER);
     if (!string.IsNullOrWhiteSpace(PageHeader))
     {
         PageStr = PageHeader;
     }
     ChannelId = GetHeaderValue(headers, CHANNEL_HEADER);
 }

 private string GetHeaderValue(IDictionary<string, StringValues> headers, string headerKey)
 {
     headers.TryGetValue(headerKey, out var headerValues);
     if (headerValues.Count > 0) return headerValues[0];
     return null;
 }

I wanted to see if there is any better way to rewrite the constructor logic that I have. As of now it looks very bulky with bunch of if/else block. Anything that can be improve in my code?

Note: It's a legacy code base system so changing whole design might be difficult but little refactoring by splitting it into various methods is possible.

You can simplify that portion of code. First, you'll need to create an array that will holds the header keys such as PROC_HEADER ..etc. then you can go from there.

Here is an example (NOTE, keys are just placeholders in this example, so you know which const needs to be replaced).

public Tester(IDictionary<string, StringValues> headers)
 {
    // these will be overwritten if their customer global are not null.
    DbId = GenerateDbId();
    ProcId = GenerateProcId();
    ThetaId = GenerateThetaId();

    if (headers == null) { return; }

    var headerKeys = new[]{
        "PROC_HEADER",
        "DB_HEADER",
        "TEXT_HEADER",
        "ALPHA_HEADER",
        "TIMEOUT_HEADER",
        "FILTER_HEADER",
        "PREVIEW_HEADER",
        "ANT_HEADER",
        "APP_HEADER",
        "PAGE_HEADER",
        "LOCAL_HEADER",
        "CONTEXT_HEADER",
        "CHANNEL_HEADER"
    };

    foreach(var key in headerKeys)
    {
        headers.TryGetValue(key, out var headerValue);

        if (headerValue.Count > 0)
        {
            switch(key)
            {
                case "PROC_HEADER":
                    SetHeaderValue(headerValue[0], "ProcIdHeader", "ProcId");
                    break;
                case "DB_HEADER":
                    SetHeaderValue(headerValue[0], "DbIdHeader",  "DbId");
                    break;
                case "TEXT_HEADER":
                     int.TryParse(headerValue[0],  var textId);
                     TextIdValue = textId;
                     TextId = textId;
                    break;
                case "ALPHA_HEADER":
                    SetHeaderValue(headerValue[0],  "AlphaHeader",  "AlphaContent");
                    break;
                case "TIMEOUT_HEADER":
                    SetHeaderValue(headerValue[0],   "TimeIdHeader",  "TimeId");
                    break;
                case "FILTER_HEADER":
                    SetHeaderValue(headerValue[0],   "FilterHeader",  "FilterId");
                    break;
                case "PREVIEW_HEADER":
                    SetHeaderValue(headerValue[0],   "PreviewHeader",  "PreviewId");
                    break;
                case "ANT_HEADER":
                    SetHeaderValue(headerValue[0],   "AntHeader",  "AntId");
                    break;
                case "APP_HEADER":
                    SetHeaderValue(headerValue[0],   "AppHeader",  "AppId");
                    break;
                case "PAGE_HEADER":
                    SetHeaderValue(headerValue[0],   "PageHeader",  "PageStr");
                    break;
                case "CHANNEL_HEADER":
                    ChannelId = headerValue[0];
                    break;
                case "LOCAL_HEADER":
                    LocalHeader = headerValue[0];
                    LocalStr = DeserializeHeader<LocalStr>(LocalHeader);
                    break;
                case "CONTEXT_HEADER":
                    ContextHeader = headerValue[0];
                    ContextStr = DeserializeHeader<ContextStr>(ContextHeader);
                    break;
            }
        }
    }
}

private T DeserializeHeader<T>(string str) where T : new()
{
    if(!string.IsNullOrEmpty(str))
    {
        try
        {
            return JsonSerializer.Deserialize<T>(str);
        }
        catch (Exception e)
        {
            return new T();
        }
    }

    return default;
}

public void SetHeaderValue(string headerValue, string customerPropertyName, string serverPropertyName)
{
    var custProperty = this.GetType().GetProperty(customerPropertyName);

    if(custProperty != null) { custProperty.SetValue(this, headerValue); }

    if(!string.IsNullOrWhiteSpace(headerValue))
    {
        var serverProperty = this.GetType().GetProperty(serverPropertyName);

        if(serverProperty != null) { serverProperty.SetValue(this, headerValue); }
    }

}

I've decided to use switch to group them up, as it would be more readable, and also it has an a performance advantage over if/else blocks as it would be compiled into jump table which would be faster than if/else blocks.