Sequence generators not only for potatoes but also for apples and oranges The last question about sequence generators Growing potatoes in delayed sequences was only about potatoes. I thougt why not make it work with apples and oranges too so I made it generic.

The base class got a new name and a generic argument and now looks like this:

public abstract class GeneratedSequence<T> : IEnumerable<T>
{
    protected GeneratedSequence(int count) { Count = count; }

    public int Count { get; }

    public IEnumerator<T> GetEnumerator() => Generate().Take(Count).GetEnumerator();

    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();

    protected abstract IEnumerable<T> Generate();
}

The RegularSequence has been upgraded and is now generic too:

public class RegularSequence<T> : GeneratedSequence<T>
{
    private readonly T _value;
    public RegularSequence(T value, int count) : base(count) { _value = value; }
    protected override IEnumerable<T> Generate()
    {
        while (true) yield return _value;
    }
}

The biggest change undergone however the FibonnaciSequence. Generics don't support arithmetic operations so I needed to add a lambda for the sum. Now it looks like this:

public class FibonacciSequence<T> : GeneratedSequence<T>
{
    private T _preview;
    private T _current;
    private readonly Func<T, T, T> _sum;

    public FibonacciSequence(T firstTwo, T firstStep, int count, Func<T, T, T> sum) : base(count)
    {
        _sum = sum;
        _preview = firstTwo;
        _current = _sum(_preview, firstStep);
    }

    protected override IEnumerable<T> Generate()
    {
        yield return _preview;
        yield return _preview;
        yield return _current;

        while (true)
        {
            var newCurrent = _sum(_preview, _current);
            yield return newCurrent;
            _preview = _current;
            _current = newCurrent;
        }
    }
}

To simplify the creation process I also added a new FibonacciSequenceFactory:

public class FibonacciSequenceFactory
{
    public static FibonacciSequence<TimeSpan> Create(TimeSpan firstTwo,TimeSpan firstStep, int count)
    {
        return new FibonacciSequence<TimeSpan>(firstTwo, firstStep, count, (x, y) => x + y);
    }

    public static FibonacciSequence<int> Create(int firstTwo, int firstStep, int count)
    {
        return new FibonacciSequence<int>(firstTwo, firstStep, count, (x, y) => x + y);
    }
}

I've applied the same pattern to the GeometricSequence but as it already had a lambda for the incrementation I just changed the argument to T:

public class GeometricSequence<T> : GeneratedSequence<T>
{
    private T _current;
    private readonly Func<T, T> _increment;
    public GeometricSequence(T first, Func<T, T> increment, int count) : base(count)
    {
        _current = first;
        _increment = increment;
    }
    protected override IEnumerable<T> Generate()
    {
        yield return _current;

        while (true)
        {
            yield return (_current = _increment(_current));
        };
    }
}

public class GeometricSequenceFactory
{
    public static GeometricSequence<TimeSpan> Double(TimeSpan first, int count)
    {
        return new GeometricSequence<TimeSpan>(first, x => TimeSpan.FromTicks(x.Ticks * 2), count);
    }

    public static GeometricSequence<TimeSpan> Triple(TimeSpan first, int count)
    {
        return new GeometricSequence<TimeSpan>(first, x => TimeSpan.FromTicks(x.Ticks * 3), count);
    }

    public static GeometricSequence<TimeSpan> Halve(TimeSpan first, int count)
    {
        return new GeometricSequence<TimeSpan>(first, x => TimeSpan.FromTicks(x.Ticks / 2), count);
    }
}

Usage example:

var fs = FibonacciSequenceFactory.Create(2, 4, 10);

A slightly different approach is to avoid the sublclassing and instead feed the generic DelaySequence class with a generator function. It makes it more flexible.

using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;

namespace CR146856
{
  public class MyDelaySequence<T> : IEnumerable<T>
  {
    Func<IEnumerable<T>> m_generator;
    int m_count = 0;

    /// <summary>
    /// A contructor for a plain sequence of regular delays
    /// </summary>
    /// <param name="count">Number of retries.</param>
    /// <param name="delay">The delay after each retry.</param>
    public MyDelaySequence(int count, T delay)
    {
      m_count = count;
      m_generator = () =>
      {
        return Enumerable.Range(1, count).Select(i => delay);
      };
    }

    /// <summary>
    /// Constructor for a custom delay generator.
    /// </summary>
    /// <param name="count">Number of retries.</param>
    /// <param name="generator">A custom generator of delays between retries.</param>
    public MyDelaySequence(int count, Func<IEnumerable<T>> generator)
    {
      m_count = count;
      m_generator = generator;
    }

    public IEnumerator<T> GetEnumerator()
    {
      return m_generator().Take(m_count).GetEnumerator();
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
      return GetEnumerator();
    }
  }

  class Program
  {
    static IEnumerable<int> GeometricDelayGenerator()
    {
      int delay = 1000;
      while (true)
      {
        yield return delay;
        delay += delay;
      }
    }

    static IEnumerable<int> FibonacciIntDelayGenerator()
    {
      int first = 1000;

      yield return first;
      yield return first;

      int second = first + 3000;
      yield return second;

      while (true)
      {
        yield return first + second;
        int tmp = first;
        first = second;
        second = first + tmp;
      }
    }

    static IEnumerable<TimeSpan> FibonacciTimeSpanDelayGenerator()
    {
      TimeSpan first = TimeSpan.FromSeconds(1);

      yield return first;
      yield return first;

      TimeSpan second = first + TimeSpan.FromSeconds(3);
      yield return second;

      while (true)
      {
        yield return first + second;
        TimeSpan tmp = first;
        first = second;
        second = first + tmp;
      }
    }

    static void Main(string[] args)
    {
      try
      {
        TimeSpan seqDelay = TimeSpan.FromSeconds(1);
        int count = 7;

        foreach (var ts in new MyDelaySequence<TimeSpan>(count, FibonacciTimeSpanDelayGenerator))
        {
          Console.WriteLine(ts);
        }
      }
      catch (Exception ex)
      {
        Console.WriteLine($"ERROR: {ex.Message}");
      }

      Console.WriteLine("END");
      Console.ReadLine();
    }
  }
}

That said I tend to think you overdo a rather simple construct:

    foreach (var delay in FibonacciIntDelayGenerator().Take(count))
    {
      Console.WriteLine($"If service not responding wait in {delay} ms and retry.");
    }

EDIT:

In case of a reusable library of functions I would do it the static way:

  public static class MyDelayGenerators
  {
    public static IEnumerable<T> DelayGenerator<T>(int retries, T initialDelay, Func<T, T> offsetFunc)
    {
      T delay = initialDelay;
      for (int i = 0; i < retries; i++)
      {
        yield return delay;
        delay = offsetFunc(delay);
      }
    }

    public static IEnumerable<T> GeometricDelayGenerator<T>(int retries, T initialDelay, Func<T, T> sum)
    {
      return DelayGenerator(retries, initialDelay, sum);
    }

    public static IEnumerable<int> GeometricDelayGenerator(int retries, int initialDelay)
    {
      return DelayGenerator(retries, initialDelay, (d) => d + d);
    }

    public static IEnumerable<TimeSpan> GeometricDelayGenerator(int retries, TimeSpan initialDelay)
    {
      return DelayGenerator(retries, initialDelay, (d) => d + d);
    }

    public static IEnumerable<int> RegularDelayGenerator(int retries, int delay)
    {
      for (int i = 0; i < retries; i++)
      {
        yield return delay;
      }
    }

    public static IEnumerable<TimeSpan> RegularDelayGenerator(int retries, TimeSpan delay)
    {
      for (int i = 0; i < retries; i++)
      {
        yield return delay;
      }
    }

    public static IEnumerable<T> FibonacciDelayGenerator<T>(int retries, T first, T firstOffset, Func<T, T, T> sum)
    {
      yield return first;
      yield return first;

      T second = sum(first, firstOffset);
      yield return second;

      retries -= 3;
      for (int i = 0; i < retries; i++)
      {
        T tmp = sum(first, second);
        yield return tmp;
        first = second;
        second = tmp;
      }
    }

    public static IEnumerable<int> FibonacciDelayGenerator(int retries, int first = 1000, int firstOffset = 1000)
    {
      return FibonacciDelayGenerator(retries, first, firstOffset, (f, s) =>
      {
        int tmp = f;
        f = s;
        s = f + tmp;
        return s;
      });
    }

    public static IEnumerable<TimeSpan> FibonacciDelayGenerator(int retries, TimeSpan first, TimeSpan firstOffset)
    {
      return FibonacciDelayGenerator(retries, first, firstOffset, (f, s) =>
      {
        TimeSpan tmp = f;
        f = s;
        s = f + tmp;
        return s;
      });
    }
  }

It has a number of predefined functions and a set of generic as well to be use on the fly.


Environment variable check with default value in csharp Looking for more simple one liner for environment variable check with default value in csharp, any help is appreciated.
public static string GetEnvironmentVariable1(string envName, string defaultValue)
{
   var env = Environment.GetEnvironmentVariable(envName);
   return string.IsNullOrEmpty(env) ? defaultValue : env;
}

Tried this code, but getting compiler error that possible null reference could be returned
public static string GetEnvironmentVariable2(string envName, string defaultValue)
{
  return string.IsNullOrEmpty(Environment.GetEnvironmentVariable(envName)) ? defaultValue : Environment.GetEnvironmentVariable(envName);
}

Use ??. Null-coalescing operator.
Null check only
public static string GetEnvironmentVariable(string name, string defaultValue)
    => Environment.GetEnvironmentVariable(name) ?? defaultValue;

Null and empty string check
public static string GetEnvironmentVariable(string name, string defaultValue)
    => Environment.GetEnvironmentVariable(name) is string v && v.Length > 0 ? v : defaultValue;

Pattern matching.


CLRS Exercise 2.3-4: Recursive Insertion Sort I'm going through CLRS 3e and executing exercises to obtain a better understanding of CS fundamentals.

Below is my solution to the following exercise from chapter 2:

  We can express insertion sort as a recursive procedure as follows. In order to sort \$A[*1..n*]\$, we recursively sort \$A[*1..n-1*]\$ and then insert \$A[*n*]\$ into the sorted array \$A[*1..n-1*]\$. Write a recurrence for the running time of this recursive version of insertion sort.

I was able to come up with a recurrence of:

T(N) = O(1) if N <= 1; T(N-1) + O(N) if N > 1

NOTE: The \$O\$ is supposed to represent theta notation.

import random

def recursive_insertion_sort(A, n):

    if n < 1:
        return A

    recursive_insertion_sort(A, n - 1)

    key = A[n]

    while n > 0 and key < A[n - 1]:
        A[n] = A[n - 1]
        n -= 1

    A[n] = key
    return A

A = [3, 1, 0, 2, 3]
B = random.sample(range(1, 11), 10)
C = random.sample(range(1, 11), 10)
D = random.sample(range(1, 11), 10)
E = [1]
F = []

assert recursive_insertion_sort(A, len(A) - 1) == sorted(A)
assert recursive_insertion_sort(B, len(B) - 1) == sorted(B)
assert recursive_insertion_sort(C, len(C) - 1) == sorted(C)
assert recursive_insertion_sort(D, len(D) - 1) == sorted(D)
assert recursive_insertion_sort(E, len(E) - 1) == sorted(E)
assert recursive_insertion_sort(F, len(F) - 1) == sorted(F)

I struggle with translating recursive algorithms to code. By my understanding this code is a correct recursive insertion sort due to the invariant that the array at \$A[0.. n - 1]\$ is a sorted array of the first \$n - 1\$ elements of the entire array \$A[0...n]\$.

I do not have a formal CS background so I'm unsure if this invariant holds up to more rigorous analysis. Furthermore if anyone has suggestions to how I can make this code cleaner that would be appreciated. As an aside I am trying to right this code w/o the use of built in functions aside from very primitive (ex. len()) functions.

The implementation looks fine to me, but writing the recurrence with a big theta isn't right. The problem is that the time taken to insert item \$n\$ into the right place among items 1 to \$n-1\$ depends on the values of the items — if item \$n\$ is bigger than all of items 1 to \$n-1\$ then it takes constant time to "insert" it. What we can say is that in the worst case it takes time proportional to \$n\$ to insert the item.

Let's go back and try again from first principles. If you look at the analysis of the iterative version of the insertion sort algorithm (on pages 25–6 of CLRS) you'll see that the approach the authors take is to annotate each line of code with a cost and number of times it is executed. So let's do that for the code in the post:

                                        COST    TIMES
--------------------------------------  ------  ------
def recursive_insertion_sort(A, n):
    if n < 1:                           c1      1
        return A
    recursive_insertion_sort(A, n - 1)  T(n-1)  1
    key = A[n]                          c2      1
    while n > 0 and key < A[n - 1]:     c3      t(n) + 1
        A[n] = A[n - 1]                 c4      t(n)
        n -= 1                          c5      t(n)
    A[n] = key                          c6      1
    return A                            c7      1

Here \$t(n)\$ is the number of times that the while loop needs to be iterated when the function is called with the value \$n\$. This depends on the input A, so we might need to make separate worst-case and best-case analyses (as you can see the authors do for the iterative version on page 26).

So we have $$ T(n) = T(n-1) + c_1 + c_2 + c_3 + c_6 + c_7 + (c_3 + c_4 + c_5)t(n) $$ Now, in the best case, the input is already sorted, and so the while loop is never executed, and so \$t(n) = 0\$, hence $$ T(n) \ge T(n-1) + c_1 + c_2 + c_3 + c_6 + c_7 $$ and following similar analysis to page 26, we find $$ T(n) \ge (c_1 + c_2 + c_3 + c_6 + c_7)n $$ and so \$T(n) = \Omega(n)\$.

In the worst case, the input is sorted in reverse order, and so \$t(n) = n-1\$, hence  $$ T(n) \le T(n-1) + c_1 + c_2 + c_3 + c_6 + c_7 + (n-1)(c_3 + c_4 + c_5) $$ and again following similar analysis to page 27, we find $$ \eqalign{ T(n) &\le (c_1 + c_2 + c_3 + c_6 + c_7)n + {c_3 + c_4 + c_5\over2}n(n-1) \\ &= {c_3 + c_4 + c_5\over2}n^2 + \left(c_1 + c_2 + c_3 + c_6 + c_7 - {c_3\over2} - {c_4\over2} - {c_5\over2}\right)n} $$ and so \$T(n) = O(n^2)\$.

So this is an algorithm for which we can't find a big-Theta expression for the runtime — it's bounded below by \$ \Omega(n) \$ and above by \$O(n^2)\$ and these do not meet in the middle.

(There are various ways to take shortcuts in this kind of analysis, especially if you are only interested in the asymptotic result, but when you are starting out, it's good to go through all the details so that you have confidence you're not missing anything.)


Minimum number of coins - (dynamic programming solution - topdown approach) This is a problem from topcoder tutorials

Given a list of N coins, their values (V1, V2, ... , VN), and the total sum S. Find the minimum number of coins the sum of which is S (we can use as many coins of one type as we want), or report that it's not possible to select coins in such a way that they sum up to S.

This is my first attempt at a DP solution. Did I do it right? How to make this code better, faster and less ugly? Is there a better solution than what I did?

#include <iostream>
#include <vector>
#include <set>

int coins_td(std::vector<int>& v, int n, std::set<int>& s);

int coins_td(int n, std::set<int>& s) {
    std::vector<int> coins(n+1, -1);
    coins[0] = 0;
    return coins_td(coins, n, s);
}

int coins_td(std::vector<int>& v, int n, std::set<int>& s) {
    static const int coin = 1;
    if (n < 0) {
        return -2;
    }
    if (n == 0) {
        return 0;
    }
    if (v[n] != -1) {
        return v[n];
    }
    int tcoins = coin+coins_td(v, n-*s.begin(), s);
    for (auto i = s.rbegin(); i != s.rend(); ++i) {
        if (n >= *i) {
            tcoins = std::min(tcoins, coin+coins_td(v, n-*i, s));
            break;
        }
    }
    v[n] = tcoins;
    return tcoins;
}

int main() {
    int value;
    std::cin >> value;
    if (value <= 0) {
        std::cout << value << '\n';
    } else {
        std::set<int> values;
        int v;
        while (std::cin >> v) {
            values.insert(v);
        }
        int result = coins_td(value, values);
        if (result == 0) {
            std::cout << "it's not possible to select coins in such a way that they sum up to " << value << "." << '\n';
        } else {
            std::cout << result << '\n';
        }
    }
}

main

    if (value <= 0) {
        std::cout << value << '\n';

This seems to be an error condition.  You might consider using std::cerr instead:

        std::cerr << "Value must be positive.  You provided:  " << value << '\n';
        return EXIT_FAILURE;

This also adds an error message so that we don't have to read the code to try to figure out why it is printing a value to the screen.

The explicit return allows us to return something different for the error condition than for a successful run.  Once we have it, we can get rid of the else clause and just proceed with the rest of the block.

        if (result == 0) {

I'd tend to make this <= 0 instead.  Only positive values are correct.  In fact, I might switch the logic around the other way:

    if ( 0 < result ) {
        std::cout << "The minimum number of coins that sum to " << value << " is " << result << ".\n";
    } else {
        std::cout << "It's not possible to select coins in such a way that they sum up to " << value << ".\n";
    }

I find it easier to follow to make the failure case the else (except for an early return as used prior).  I also added more of a message in the case of success.

Naming

int coins_td(std::vector<int>& v, int n, std::set<int>& s);

The input is described as a desired sum, S, and a list of coin values, V1..VN.  So I would expect a vector named v to refer to that input value.  However, in this case, v holds an intermediate result and s holds the coin values.  The desired value is in n.  It would be reasonable to use the same names as in the problem statement.  Normally if you change them, you'd change them to be more descriptive, e.g. desired_sum and coin_values.

What is a coins_td?  I've been looking at this problem for some time now, and I still have no idea.  It's possible that I'm just being slow, but this suggests to me that it is a non-descriptive name.  Writing it out rather than abbreviating might help.

    static const int coin = 1;

While it's good that you're trying to avoid magic numbers in your code, this is a bit confusing.  What is coin and why does it equal 1?  I actually think that using the value 1 is more obvious about what is happening in this case.

Correctness

for (auto i = s.rbegin(); i != s.rend(); ++i) {
    if (n >= *i) {
        tcoins = std::min(tcoins, coin+coins_td(v, n-*i, s));
        break;
    }
}

This is not correct because it stops at the first one rather than the smallest one.  Consider a desired value of 7 with coin values of 1, 3, 4, and 5.  This code would return an answer of 3 (for 1+1+5), but the correct answer is 2 (4+3).  You should remove the break statement.

    int tcoins = coin+coins_td(v, n-*s.begin(), s);

This is incorrect because it is possible that there may be no way to reach the value n-*s.begin() in which case it will return -1, which will be smaller than any of the correct values.  Instead, set it to a number larger than the maximum possible value:

int tcoins = n + 1;

That will make the loop logic work consistently.

    if (n < 0) {
        return -2;
    }

Same thing here.  You should return a large number, not a small one.  That will also fix the problem of this being a magic number based on the fact that 1 + -2 = -1 where -1 is your magic value for an unset entry in v.

    if (n < 0) {
        return std::numeric_limits<int>::max();
    }

More info on std::numeric_limits.  Or leave this off entirely, as in the current code, this should never happen.  It only happens in the original code because of initializing tcoins without checking that its possible.

std::vector<int> coins(n+1, -1);

And again here.

    if (n == 0) {
        return 0;
    }
    if (v[n] != -1) {
        return v[n];
    }

These are redundant.  Since v[0] = 0, the second if will have the same effect as the first when n == 0.

    if ( v[n] <= n ) {
        return v[n];
    }

Or

    if ( v[n] < std::numeric_limits<int>::max() ) {
        return v[n];
    }

Either should work fine.  Note that a valid v[n] can't be greater than n because the coin values are positive integers (or at least I think that they should be).


Recursive check for locked files I'll have to move a large network directory with many sub-directories and thousands of files. A number of users work on these files on a daily basis. OS is windows.

The move itself (copy and delete) I'll do using robocopy.

Before starting the batch process of moving, I would like to check for locked files. In case some files or folders are locked, I won't bother starting the process.

robocopy has a dry run switch, but that doesn't check for locked files which would lead to an unsuccessful run.

I quickly came up with something like the below, in Python:

import os

is_locked=list()

for root, dirnames, filenames in os.walk('path/to/directory'):
    for filename in filenames:
        full_filename = os.path.join(root, filename)
        try:
            pf = open(full_filename, 'a')
            pf.close()
        except PermissionError:
            is_locked.append(full_filename)

Your opinion whether this is a good or bad way of doing it? Any general comment?
Performance, etc?

Is it harmless to be opening these files?

--- edit ---

Summary of the most relevant comments:

Break the loop when first locked file is found.

Bugs:

After building your list, you throw it away without printing anything (and if not, please supply all your code)

Minor:

pathlib >>> os. It leads to much better readable code, and less boilerplate.
is_locked=list() should be: is_locked = [] Note spaces and the literal.
pf.close() isn't a reliable statement. Instead, consider using something like with open(full_filename) as pf: pass
"path/to/directory" is a magic variable. Make it a global constant instead.
Use a if __name__ == "__main__": guard. Always.

Are you going to manually unlock all the files?

If not, you can just break after the first error and inform the user.


Converting numbers to words - JavaScript I recently worked on this problem that converts numbers to words. I was wondering if there is a better way to do it than mine, And I will really appreciate any reviews to my version of the solution and on how to make it better

/**
 * Converts a given number into words
 * @param {integer} , num, the number to be converted into words
 * @return {string} , the coverted number in words
 */
function numToEng(num){
  let numInWords = '';

  // will be using this as a number that will be reduced by 1000
  let tempNum = num;

  const wordsForConstruction = {
    0: 'zero',
    1: 'one',
    2: 'two',
    3: 'three',
    4: 'four',
    5: 'five',
    6: 'six',
    7: 'seven',
    8: 'eight',
    9: 'nine',
    10: 'ten',
    11: 'elleven',
    12: 'twelve',
    13: 'thirteen',
    14: 'fourteen',
    15: 'fifteen',
    16: 'sixteen',
    17: 'seventeen',
    18: 'eighteen',
    19: 'nineteen',
    20: 'twenty',
    30: 'thirty',
    40: 'fourty',
    50: 'fifty',
    60: 'sixty',
    70: 'seventy',
    80: 'eighty',
    90: 'ninety',
  }

  const wordsToAdd = [
    'thousand',
    'million',
    'trillion',
  ]

  if(num >= 0 && num <= 19){
    return `${wordsForConstruction[num]}`;
  }

  let hundreds;
  let tens;
  let ones;

  for(let i = 0; tempNum; i++){

    hundreds = tempNum % 1000;
    tempNum = (tempNum-hundreds)/1000;

    // construct words for hundred only if it is not a zero
    if(hundreds){

      // If the loop runs the second time, this will add the words
      // thousand, million, trillion, and so on, depending on the num
      // of words in the wordsToAdd[] Array
      if(i){
        numInWords = wordsToAdd[i-1] + " " + numInWords;
      }

      tens = hundreds % 100;
      hundreds = (hundreds - tens)/100;

      // construct words for tens only if it is not a zero
      if(tens){

        // is tens is a property of the wordsforconstruction then
        // just pull the word from the object
        if(wordsForConstruction.hasOwnProperty(tens)){
          numInWords = wordsForConstruction[tens] + " " + numInWords;
        }else{
          ones = tens%10;
          tens = tens-ones;
          if(ones){
            numInWords = wordsForConstruction[ones] + " " + numInWords;
          }
          numInWords = wordsForConstruction[tens] + " " + numInWords;
        }
      }

      if(hundreds){
        numInWords = wordsForConstruction[hundreds] + " hundred " + numInWords;
      }
    }
  }

  return numInWords;
}

/**
 * Initiates the program and runs the test cases
 */
function main(){
  const testCases = [
    0,
    101,
    100234,
    909,
    1000000,
    1000,
    9999999,
    90909090,
  ]

  testCases.map(tCase => {
    console.log();
    console.log(`numToEng(${tCase}) ➞ ${numToEng(tCase)}`);
  })
}

main();

Also if you find any bugs or issues with the solution please feel free to reply to this thanks in advance.

I like how you've reduced all of the words to lists, that's the right start.

Let's get into it:

  for(let i = 0; tempNum; i++){

I have no idea what you've doing here, I assume when tempNum===0 then the loop stops?

Don't do this, it's confusing as hell.

I'm going to suggest that you rewrite the entire thing as a recursive function or a reducing function, but if you insist on using a loop, then just make it a while(true) loop and add a return or a break statement to it to exit it.

You have a lot of branching logic:

  for(let i = 0; tempNum; i++){
    if(hundreds){
      if(i){
      }
      if(tens){
        if(wordsForConstruction.hasOwnProperty(tens)){

        }else{
          if(ones){
          }
        }
      }

      if(hundreds){
      }
    }
  }

The term to think about here is cyclomatic complexity keeping track of which branch the code is currently in involves holding a lot of information in your head, and then you have to run through it again for the next number etc. If you code is looking like a lot of these nested if statements, that's a code smell, and you should be doing something else.

The real issue you have, is that you are manually writing the code to do the 'detect how many hundreds, thousands, millions', instead you you could abstract this to a map.

What I would do is:

const powerMap = {
  "million": 1000000,
  "thousand": 1000,
  "hundred" : 100,
  "rest": 1,
}; //And ofcourse you can extend this to billions, trillions, etc.

And then you can create a general abstraction to to count how many of each there are:

const powerMap = {
  "million": 1000000,
  "thousand": 1000,
  "hundred" : 100,
  "rest": 1,
}; //And ofcourse you can extend this to billions, trillions, etc.

function getUnitCounts(value) {
    const data = Object.entries(powerMap).reduce(({value, result}, [unitName, power]) => {
        const nCount = Math.floor(value/power);
        const nRemainder = value%power;

        return {
            value: nRemainder,
            result: {
                ...result,
                [unitName]: nCount
            }
        }

    }, {
         value,
         result: {}
    });

    return data.result;

}

console.log(getUnitCounts(20020000));
console.log(getUnitCounts(1100));
console.log(getUnitCounts(201110));

Now - if you're not familar with spread syntax and Array.reduce this might seem confusing. But notice how I don't use a single if statement - all I'm doing is looping over a list and returning an accumulator. Once you've familiar with the odd syntax, this is easier to keep in your head.

From the answer I've given here, it would now be a matter of converting the count of each unit into the words, and then also applying the words for 0-100 like you have.


C# beginner's exercise in calling methods This is one of my first experiences with C#. I'm trying to learn and as can be seen, I need some obvious help. I know it's only an example but I'd like to start with best practices from the beginning. This was my first experience with public and private methods.

What can I do to improve my code and why is the change better? What about it makes it preferred or a best practice?

using System;

namespace CallMethodsWithinSameClass
{
    class Program
    {
        static void Main()
        {
            bool I_Has_Energy = true;
            OtherClass TestClass = new OtherClass();
            I_Has_Energy = TestClass.NoIDont;
            Console.WriteLine("Do I have energy? " + I_Has_Energy);
            Console.WriteLine("Getting a public method " + TestClass.GiveMeAString());
            Console.WriteLine("Grabbing the privates " + TestClass.GrabThemPrivates());
            Console.ReadKey();
        }
    }

    class OtherClass
    {
        public bool YesIDo = true;
        public bool NoIDont = false;
        public string SimpleString = "This is only a test.";
        public string GiveMeAString()
        {
            return "Here's your sign, I mean string.";
        }
        private string KeepItToYourself()
        {
            return "Make sure you'll be gentle with them... *Wink*";
        }
        public string GrabThemPrivates()
        {
            return KeepItToYourself();
        }
    }
}

I have a few tips for your code:

Keep variable names consistent. Doing this makes it easier to remember (or guess) what you named them in the future. One good way of doing this is capitalizing all variable names the same way. Most programmers use what's known as camel case.

Example:

int camelCaseRules;

This will help you A LOT in the future when coming up with coherent variable names, and since most programmers use camel case, it will help you in team projects.
Another thing about variables is to make sure that you can understand it. E.g. don't name a variable NID (Standing for noIDont). When naming variables, you should make sure that you can get a basic understanding of what they do/are used for without digging in the code to figure it out.
Comment, comment, and comment. Keeping up good commenting habits early on in programming will help you tremendously in the future. This will heavily increase your code readability and make it so that when you come back to the code in a week, you can understand why you did something the way you did it. Just putting in those two forward slashes makes a huge difference. Something to keep in mind when commenting your code however (which was brought to my attention by Hosch250), is to not comment on what the code does, but what it is used for. Basically, well written code should explain what it does without comments, and comments should explain what it is used for in the program.

// Comment your code!

Finally, only make this public if they have to be (e.g. your YesIDo variable in the OtherClass class. I would explain why, but I think this article does a much better job of doing so.

Also, a good YouTube video I would check out is here.


Showing popup according to the header nav link I am showing templates in popups according to the user navigation links. It works for me, but, is this the correct way to do it?

var app = angular.module('plunker', []);

app.service('modalService', function( $rootScope ) {

  this.hide = function () {

    this.show = false;

  }

  this.showIt = function () {

    this.show = true;

  }

  this.setCategory = function ( category ) {

     return this.showPath = category+'.html'

  }

  this.showCategory = function (category) {

    this.setCategory( category )
    $rootScope.$apply();
  }

})

app.controller('header', function($scope) {

  $scope.view = "home view";

});

app.controller('home', function($scope, modalService) {

  $scope.name = 'World';
  $scope.service = modalService;

});

//header directive

app.directive('headerDir', function( modalService) {

  return {
      restrict : "E",
      replace:true,
      templateUrl:'header.html',
      scope:{},
      link : function (scope, element, attrs) {

         element.on('click', '.edit', function () {
            modalService.showIt();
            modalService.showCategory('edit');
         });

         element.on('click', '.service', function () {
           modalService.showIt();
           modalService.showCategory('service');

         })

      }
  }

});

app.directive('popUpDir', function () {

  return {
    replace:true,
    restrict:"E",
    templateUrl : "popup.html"
  }

})

Click on the top link to show the popup, and click on the screen to hide the popup.

Live Demo

this.hide = function () {

    this.show = false;

  }

  this.showIt = function () {

    this.show = true;

  }

You have extraneous lines everywhere, and extraneous space everywhere, trim it down.

this.hide   = function(){ this.show = false; }
this.showIt = function(){ this.show = true;  }

On the other hand, here, you're missing space (but also have extra space):

  this.setCategory = function ( category ) {

     return this.showPath = category+'.html'
                                   ^^^
  }

should be:

this.setCategory = function(category){ this.showPath = category + '.html'; }

The return is redundant, as the value goes nowhere in showCategory.

     element.on('click', '.edit', function () {
        modalService.showIt();
        modalService.showCategory('edit');
     });

     element.on('click', '.service', function () {
       modalService.showIt();
       modalService.showCategory('service');

This is redundant, pass the .service / .edit in as a variable.


Sieve of Eratosthenes - Standard and Optimized implementation I am a Java developer who is taking Python for the first time.

I'm sure this is not at all elegant since I am thinking more in C syntax.

"""
This module contains two implementations of the algorithm Sieve of
Eratosthenes.
"""

# import ############################################################### import #
import math
import numpy
#
 # fun1 ################################################################# fun1 #
def SieveBasic(n):
 """
 This function runs the basic sieve of eratosthenis algorithm (non-optimized)
 and returns a list of prime numbers. The algorithm is implemented as described @:
 http://en.wikipedia.org/wiki/Sieve_of_Eratosthenes#Example

 """

 l = list(range(2, n+1))
 isPrime = [True] * (n-1)

 for m,n in enumerate(l):
     currentCheck = n
     for i,x in enumerate(l[m+1:]): #take each number and compare with later numbers
       if x % currentCheck == 0:
            isPrime[i+m+1] = False

 primes = [0,1]
 for i,x in enumerate(isPrime):
     if x==True:
         primes.append(2+i)

 return primes

 """
 This function generates a list of Js required for optimized sieve of eratosthenis algorithm.

 """
def generateJs(i,n):
    j=i**2
    if j<n:
        yield j

    while j+i<=n:
        j+=i
        yield j

def SieveOptimized(n):
 """
 This function runs the optimized sieve of eratosthenis algorithm
 and returns a list of prime numbers. The algorithm is implemented as described @:
 http://en.wikipedia.org/wiki/Sieve_of_Eratosthenes#Implementation

 """
 l = list(range(2, n+1))
 isPrime = [True] * (n+1)
 maxChk = math.sqrt(n)

 for q,r in enumerate(l):
    if r>maxChk:
        break

    for z in generateJs(r,n):
       isPrime[z] = False

 return numpy.where(isPrime)[0] # how cool is this

Style

You code does not follow PEP 8, the usual Python coding convention. You'll find tools such as pep8 (or its online version), pylint, pyflakes or pychecker to check for this and other points that might make your code cleaner, more idiomatic or more correct.

I can't be bothered to break the too long lines nor to fix typos in the comments but I did perform the other changes so make pep8 happy.

The result is :

"""
This module contains two implementations of the algorithm Sieve of
Eratosthenes.
"""

import math
import numpy

def SieveBasic(n):
    """
    This function runs the basic sieve of eratosthenis algorithm (non-optimized)
    and returns a list of prime numbers. The algorithm is implemented as described @:
    http://en.wikipedia.org/wiki/Sieve_of_Eratosthenes#Example
    """
    l = list(range(2, n+1))
    isPrime = [True] * (n-1)

    for m, n in enumerate(l):
        currentCheck = n
        # take each number and compare with later numbers
        for i, x in enumerate(l[m+1:]):
            if x % currentCheck == 0:
                isPrime[i+m+1] = False

    primes = [0, 1]
    for i, prime in enumerate(isPrime):
        if prime:
            primes.append(2+i)

    return primes

def generateJs(i, n):
    """
    This function generates a list of Js required for optimized sieve of eratosthenis algorithm.
    """
    j = i**2
    if j < n:
        yield j

    while j+i <= n:
        j += i
        yield j

def SieveOptimized(n):
    """
        This function runs the optimized sieve of eratosthenis algorithm
        and returns a list of prime numbers. The algorithm is implemented as described @:
        http://en.wikipedia.org/wiki/Sieve_of_Eratosthenes#Implementation

    """
    l = list(range(2, n+1))
    isPrime = [True] * (n+1)
    maxChk = math.sqrt(n)

    for q, r in enumerate(l):
        if r > maxChk:
            break

        for z in generateJs(r, n):
            isPrime[z] = False

    return numpy.where(isPrime)[0]

def main():
    """Main function"""
    for n in range(1, 200):
        s1 = SieveBasic(n)
        s2 = SieveOptimized(n).tolist()
        if s1 != s2:
            print n, s1, s2

if __name__ == "__main__":
    main()

Also, the name of generateJs is not so great as it doesn't tell us much.

Correctness

When you write two functions to perform the same thing, it can be a good idea to check that you get the same result on a big set of inputs. Here's what I wrote :

def main():
    """Main function"""
    for n in range(1, 200):
        s1 = SieveBasic(n)
        s2 = SieveOptimized(n).tolist()
        if s1 != s2:
            print n, s1, s2

This shows that the result is different when n is 4, 9, 25, 49, 121, 169. The optimised algorithm consider these values as prime even though they shouldn't (as they are perfect squares - of prime numbers which might have its importance when looking for the fix). Once this is fixed, you can use assert to ensure the results of the two functions are the same.

In your case, the fix is simple : changing if j < n for if j <= n in generateJs :

def generateJs(i, n):
    """
    This function generates a list of Js required for optimized sieve of eratosthenis algorithm.
    """
    j = i**2
    if j <= n:
        yield j

    while j+i <= n:
        j += i
        yield

0 and 1 shouldn't be considered as primes. Again, fixing this is simple : initialising primes as being primes = [] in the basic function and setting isPrime[0] = isPrime[1] = False in the optimised function.

Improving the code for the optimised version

In generateJs, you can factorise code by writing :

def generateJs(i, n):
    """
    This function generates a list of Js required for optimized sieve of eratosthenis algorithm.
    """
    j = i**2
    while j <= n:
        yield j
        j += i

Interestingly, this now looks a lot like range (or xrange depending on the version of Python you are using). We can get rid of it and write :

Also, you don't need to use enumerate:

def SieveOptimized(n):
    """
        This function runs the optimized sieve of eratosthenis algorithm
        and returns a list of prime numbers. The algorithm is implemented as described @:
        http://en.wikipedia.org/wiki/Sieve_of_Eratosthenes#Implementation

    """
    l = list(range(2, n+1))
    isPrime = [True] * (n+1)
    isPrime[0] = isPrime[1] = False
    maxChk = math.sqrt(n)

    for r in l:
        if r > maxChk:
            break

        for z in xrange(r*r, n+1, r):
            isPrime[z] = False

    return numpy.where(isPrime)[0]

Now, if you think about it, the list l is not that useful. The point is just to perform a re-indexing : what we are really doing is that we loop starting as 2 and we stop at n (included). We can write this directly : for r in xrange(2, n+1):. The same kind of comment also applies to the "basic" version of the code.

def SieveOptimized(n):
    """
        This function runs the optimized sieve of eratosthenis algorithm
        and returns a list of prime numbers. The algorithm is implemented as described @:
        http://en.wikipedia.org/wiki/Sieve_of_Eratosthenes#Implementation

    """
    isPrime = [True] * (n+1)
    isPrime[0] = isPrime[1] = False
    maxChk = math.sqrt(n)

    for r in xrange(2, n+1):
        if r > maxChk:
            break

        for z in xrange(r*r, n+1, r):
            isPrime[z] = False

    return numpy.where(isPrime)[0]

Because sqrt(n) + 1 <= n + 1 for any n >= 1, we can use maxChk in the call to range :

def SieveOptimized(n):
    """
        This function runs the optimized sieve of eratosthenis algorithm
        and returns a list of prime numbers. The algorithm is implemented as described @:
        http://en.wikipedia.org/wiki/Sieve_of_Eratosthenes#Implementation

    """
    isPrime = [True] * (n+1)
    isPrime[0] = isPrime[1] = False

    for r in xrange(2, int(math.sqrt(n))+1):
        for z in xrange(r*r, n+1, r):
            isPrime[z] = False

    return numpy.where(isPrime)[0]

Improving the code for the basic version

Here again, the list l is not that useful. The point is just to perform a re-indexing : what we are really doing is that we loop starting as 2 and we stop at n (included).

You can put a few asserts in your code to double check that we don't really need the values from l as we can compute them.

for m, n in enumerate(l):
    assert m == n-2
    assert n == m+2
    currentCheck = m+2
    # take each number and compare with later numbers
    for i, x in enumerate(l[m+1:]):
        assert x - m - i == 3
        assert x == 3 + m + i
        if x % currentCheck == 0:
            isPrime[i+m+1] = False

Messing a bit with indices, you get :

def SieveBasic(n):
    """
    This function runs the basic sieve of eratosthenis algorithm (non-optimized)
    and returns a list of prime numbers. The algorithm is implemented as described @:
    http://en.wikipedia.org/wiki/Sieve_of_Eratosthenes#Example
    """
    isPrime = [True] * (n-1)

    for x in range(2, n+1):
        # take each number and compare with later numbers
        for j in range(x+1, n+1):
            if j % x == 0:
                isPrime[j - 2] = False

    primes = []
    for i, prime in enumerate(isPrime):
        if prime:
            primes.append(2+i)

    return primes

And the end of the function can be re-written with a list comprehension :

return [2+i for i, prime in enumerate(isPrime) if prime]

This code is still not optimal but because it corresponds to the "basic" version, I guess there is no need to try to make it too much better.

Additional note

In my code, I've been using range and xrange as I am used to switch between Python 2 and Python 3. Depending on the version you are using, I suggest you have a look online to see more details about this. Just keep in mind that you should try to avoid creating a list when an iterator is enough for you.


Combinations of size k from a list in OCaml I wanted to write a small but non-trivial function in OCaml to test my understanding. I have only a basic understanding of the libraries, and no idea at all of what is considered good style. The function computes all combinations of size k from a list, where 0 <= k <= length lst.

Would someone mind commenting on

Whether the function is generally well written (have I covered all cases, are there opportunities for tail recursion that I've missed?)
Have I made good use of libraries (e.g. I defined is_empty and tails because I couldn't find them in the List module, but maybe they are somewhere else?)
Is the style okay, particularly the use of let statements and indentation?

The code is:

let rec tails = function
  | []          -> []
  | _ :: t as l -> l :: tails t

let is_empty = function
  | [] -> true
  | _  -> false

let rec combnk k lst =
  if k = 0 then [[]]
  else let f = function
    | []      -> [] (* I think this is unnecessary, but I get a pattern match warning o/w *)
    | x :: xs -> List.map (fun z -> x :: z) (combnk (k-1) xs)
  in if is_empty lst then []
     else List.concat (List.map f (tails lst))

Based on the excellent comments by amon (see below) I have written to what I think is the most readable version of this function, which is the one that inlines the definition of tails and gets rid of is_empty completely, but doesn't go all the way to removing the use of List.concat and List.map, because I believe in using library functions to simplify the code wherever possible.

In particular, the layout guidelines make the structure of the function much clearer, and I think that in this version it is obvious what algorithm is being used, whereas it is somewhat obfuscated in the original. Thanks, amos!

let rec combnk k lst =
  if k = 0 then
    [[]]
  else
    let rec inner = function
      | []      -> []
      | x :: xs -> List.map (fun z -> x :: z) (combnk (k - 1) xs) :: inner xs in
    List.concat (inner lst)

Your tails is very beatiful, your is_empty useless, and combnk a mess.

In combnk, your indentation obfuscates the actual structure of the code. Here is a better indentation:

let rec combnk k lst =
  if k = 0 then
    [[]]
  else
    let f = function
      | []      -> []
      | x :: xs -> List.map (fun z -> x :: z) (combnk (k - 1) xs)
    in
      if is_empty lst then
        []
      else
        List.concat (List.map f (tails lst))

Now there are some interesting observations to be made here: The branch if is_empty lst then [] does not access f, so we could move this test outside of the let:

let rec combnk k lst =
  if k = 0 then
    [[]]
  else if is_empty lst then
    []
  else
    let f = function
      | []      -> []
      | x :: xs -> List.map (fun z -> x :: z) (combnk (k - 1) xs)
    in
      List.concat (List.map f (tails lst))

But is the is_empty test actually necessary? tails [] produces an empty list, List.map f [] produces an empty list for any function f, and List.concat [] also produces an empty list. We now have:

let rec combnk k lst =
  if k = 0 then
    [[]]
  else
    let f = function
      | []      -> []
      | x :: xs -> List.map (fun z -> x :: z) (combnk (k - 1) xs)
    in
      List.concat (List.map f (tails lst))

How can this be improved? We can move the tails definitions inside the else-branch let so that it's restricted to the only scope where it is used:

let rec combnk k lst =
  if k = 0 then
    [[]]
  else
    let rec tails = function
      | []          -> []
      | _ :: t as l -> l :: tails t
    and f = function
      | []      -> []
      | x :: xs -> List.map (fun z -> x :: z) (combnk (k - 1) xs)
    in
      List.concat (List.map f (tails lst))

Regarding the question whether the case [] -> [] in f is necessary except for the type system: The answer is no, as the list produced by tails cannot contain another empty list – l :: [] is l again. This would change when you swap [] -> [] in tails for [] -> [[]], which would be arguably more correct.

Now that f and tails are so close together you may notice some similarities.  Indeed, we can combine the two directly, thus getting rid of one map:

let rec combnk k lst =
  if k = 0 then
    [[]]
  else
    let rec inner = function
      | []      -> []
      | x :: xs -> (List.map (fun z -> x :: z) (combnk (k - 1) xs)) :: inner xs
    in
      List.concat (inner lst)

Of course, inner could be made partially tail recursive (but this reverses the order of combinations):

let rec combnk k lst =
  if k = 0 then
    [[]]
  else
    let rec inner acc = function
      | []      -> acc
      | x :: xs ->
        let this_length = List.map (fun z -> x :: z) (combnk (k - 1) xs)
        in
          inner (this_length :: acc) xs
    in
      List.concat (inner [] lst)

There is still an indirect recursion through combnk, more obvious if we rewrite it like this:

let rec combnk k lst =
  let rec inner acc k lst =
    match k with
    | 0 -> [[]]
    | _ ->
      match lst with
      | []      -> List.flatten acc
      | x :: xs ->
        let this_length = List.map (fun z -> x :: z) (inner [] (k - 1) xs)
        in
          inner (this_length :: acc) k xs
  in
    inner [] k lst

Now all that is left to do is to write a map that takes an external accumulator, thus also removing the need for flatten or concat:

let rec combnk k lst =
  let rec inner acc k lst =
    match k with
    | 0 -> [[]]
    | _ ->
      match lst with
      | []      -> acc
      | x :: xs ->
        let rec accmap acc f = function
          | []      -> acc
          | x :: xs -> accmap ((f x) :: acc) f xs
        in
          let newacc = accmap acc (fun z -> x :: z) (inner [] (k - 1) xs)
          in
            inner newacc k xs
    in
      inner [] k lst


Creating a TreeNode hierarchy in C# The following function accepts a list of Topic entities, retrieved from a database using LINQ-to-Entities. Each Topic has an Id,  Title and ParentId.

I want to populate an ASP.NET TreeView control, and so the function is creating a hierarchy of the Topics based on their ParentId. If a Topic has no parent, its ParentId is null and I put it under the 'root'.

public TreeNode GenerateTopicsTree(List<Topic> topics) {
  var s = new Stack<TreeNodeAndId>();
  var root = new TreeNode("Topics", "0");
  s.Push(new TreeNodeAndId { Id = null, TreeNode = root });
  while (s.Count > 0) {
    var node = s.Peek();
    var current = topics.FirstOrDefault(o => o.ParentId == node.Id);
    if (current == null) {
      s.Pop();
      continue;
    }
    var child = new TreeNode(current.Title, current.Id.ToString());
    node.TreeNode.ChildNodes.Add(child);
    s.Push(new TreeNodeAndId { Id = current.Id, TreeNode = child });
    topics.Remove(current);
  }
  return root;
}

struct TreeNodeAndId
{
  public TreeNode TreeNode;
  public int? Id;
}

Any improvements?

Just a word of advice, creating a mutable struct which is always a bad idea.  It must be a class otherwise you're just leaving yourself open to problems and confusion.

You could improve it much more if you don't restrict your methods to use lists of topics.  You could then easily apply recursion here and write it in a more functional style with simpler logic.

static TreeNode GenerateTopicsTree(IEnumerable<Topic> topics)
{
    // shouldn't the root value be null?
    var root = new TreeNode("Topics", null);
    return GenerateTopicSubTree(root, topics);
}

static TreeNode GenerateTopicSubTree(TreeNode root, IEnumerable<Topic> topics)
{
    // partition the topics to child and non-child topics
    var rootId = GetId(root);
    var childTopics = topics.ToLookup(topic => topic.ParentId == rootId);

    // create and add subtrees to the current node
    var childNodes = childTopics[true].Select(GenerateNode);
    foreach (var childNode in childNodes)
    {
        root.ChildNodes.Add(GenerateTopicSubTree(childNode, childTopics[false]));
    }
    return root;
}

static int? GetId(TreeNode node)
{
    int id;
    if (Int32.TryParse(node.Value, out id))
        return id;
    return null;
}

static TreeNode GenerateNode(Topic topic)
{
    return new TreeNode(topic.Title, Convert.ToString(topic.Id));
}

Consider using data binding to create your tree instead.  I don't know how it works with ASP.NET so I can't really give you tips on how to do it.  But doing so should make this step unnecessary as the framework will generate the tree for you.  You'll probably have to create a class to represent the topics organized in a hierarchy but you could use the above code to create that hierarchy.

After thinking about this again, I think it would be better to just group them all at once in the beginning instead of partitioning at every step.  Here's an alternate implementation:

static TreeNode GenerateTopicsTreeAlt(IEnumerable<Topic> topics)
{
    var root = new TreeNode("Topics", null);

    // group all children together now so we don't need to regroup them again later
    var childTopics = topics.ToLookup(topic => topic.ParentId);
    return GenerateTopicSubTreeAlt(root, childTopics);
}

static TreeNode GenerateTopicSubTreeAlt(TreeNode root, ILookup<int?, Topic> childTopics)
{
    // create and add subtrees to the current node
    var rootId = GetId(root);
    var childNodes = childTopics[rootId].Select(GenerateNode);
    foreach (var childNode in childNodes)
    {
        root.ChildNodes.Add(GenerateTopicSubTreeAlt(childNode, childTopics));
    }
    return root;
}


Determine if N is Even I was playing with JS and wanted to create an isEven(n) function that returns true if it is even and false otherwise.

Here is what I did:

function isEven(n){
  return n % 2 ? false : true;
}

print(isEven(6));
//-> true
print(isEven(-3));
//-> false

My question is:

Is this good code? How is JS interpreting this code?

I'm assuming JS returns true if n % 2 equals to 1 and false if 0.

Is this a good solution or would you prefer writting something else like:

function isEven(n){
   return (n % 2 == 0) ? true : false;
}
print(isEven(6));
//-> true
print(isEven(-3));
//-> false

First, a number X is even if X mod 2 is 0.

I think is more understandable something like this:

function isEven(n){
  return (n % 2 == 0);
}

The function will return true if the number n is even and false if it's odd.


Parsing financials from a YAML file. Is this normal/idiomatic in Rust? I wrote this Rust code to parse my financials from a YAML file and my main concern is the large match branches (although general code review is welcome; still a Rust beginner):
extern crate yaml_rust;
use yaml_rust::{Yaml, YamlLoader};

fn main() -> Result<(), std::io::Error> {
    let doc = std::fs::read_to_string("finances.yaml")?;
    let data = YamlLoader::load_from_str(&doc).unwrap();
    let doc = &data[0];

    let map = doc.as_hash().unwrap();

    for (k, v) in map.iter() {
        println!("{}: ", k.as_str().unwrap());
        let sum = unwrap_value(v);
        println!("== {}", sum);
        println!();
    }

    Ok(())
}

fn unwrap_value(v: &Yaml) -> f32 {
    match v {
        Yaml::Hash(v) => {
            let mut sum = 0.;
            for (k, vv) in v.iter() {
                match vv {
                    Yaml::String(_vv) => {
                        print!("\t\t {}: ", k.as_str().unwrap());
                    }
                    Yaml::Integer(_vv) => {
                        print!("\t\t {}: ", k.as_str().unwrap());
                    }
                    _ => {
                        println!("\t* {}:", k.as_str().unwrap());
                    }
                }
                sum += unwrap_value(vv);
            }
            sum
        }
        Yaml::Array(v) => {
            let mut sub_sum = 0.;
            for h in v.iter() {
                sub_sum += unwrap_value(h);
            }
            println!("\t= {}", sub_sum);
            sub_sum
        }
        Yaml::String(v) => {
            let tot: f32 = v
                .split("+")
                .fold(0., |sum, s| sum + s.trim().parse::<f32>().unwrap());
            println!("{}", tot);
            tot
        }
        Yaml::Integer(v) => {
            println!("{}", v);
            *v as f32
        }
        _ => 0.,
    }
}

Here's a sample YAML of the file I'm parsing:
---
'2021-01-01':
  apartment:
    - rent: 2750
  transportation:
    - uber: 87.69 + 55.36 + 26 + 42 + 42 + 34.92 + 25.76 + 42 + 42
    - bus: 12
'2021-02-01':
  apartment:
    - rent: 2750
  bills:
    - elctricity: 27
  transportation:
    - uber: 87.69 + 55.36 + 26 + 42 + 42 + 34.92 + 25.76 + 42 + 42
    - bus: 12

and this is the output for the example above:
2021-01-01:
        * apartment:
                 rent: 2750
        = 2750
        * transportation:
                 uber: 397.73
                 bus: 12
        = 409.73
== 3159.73

2021-02-01:
        * apartment:
                 rent: 2750
        = 2750
        * bills:
                 elctricity: 27
        = 27
        * transportation:
                 uber: 397.73
                 bus: 12
        = 409.73
== 3186.73

Structure your data
The reason you need nested matches is that even after parsing the YAML document into a Yaml object, it's still essentially unstructured data. Not every YAML object will have the right shape, so you have to validate it piece by piece as you traverse it. Worse, you'll have to validate it all over again next time you try to traverse it. This is inefficient and ugly.
Define some data structures that will hold the information currently held in a YAML document. How you define these depends on how rigid the data format is and what you plan to do with it. Here's one way to do it. You'll need to add chrono to your dependencies.
use chrono::NaiveDate as Date;

struct Document {
    by_date: HashMap<Date, Expenses>,
}

struct Expenses {
    by_category: HashMap<String, Vec<Entry>>,
}

struct Entry {
    payee: String,
    payments: Vec<f32>,
}

Define what it means to total up the whole Document by deferring to its contents:
impl Document {
    fn total(&self) -> f32 {
        // the total of a Document is the sum of the total expenses on each date
        self.by_date.values().map(|expenses| expenses.total()).sum()
    }
}

impl Expenses {
    fn total(&self) -> f32 {
        // the total is the sum of the totals of each Entry in each category
        self.by_category.values().flat_map(|entries| entries.iter().map(Entry::total)).sum()
    }
}

impl Entry {
    fn total(&self) -> f32 {
        // the total is the sum of all the individual payments
        self.payments.iter().sum()
    }
}

Note 1: I am completely ignoring the printing part of unwrap_value. You could add that to the above code relatively easily, but it would be noisy and not super instructive, so I'm just going to focus on the totaling.
Note 2: It might sometimes be useful to write a Total trait and implement it for Entry, Document and Expenses, instead of having three unrelated functions. But it doesn't seem useful here, so I didn't.

Deserialize with Serde
Now, the only problem that remains is how to turn a YAML document into a Document. You could write code to convert from a Yaml object, but the bulk of the work has been done for you already by the Serde library. Serde is an extremely versatile library for serialization and deserialization of just about any Rust data structure from just about any data format. Any time you're considering serialization, unless you know you need something specific, it's a good idea to start with Serde. You'll need to add serde, serde-yaml and serde-derive to your dependencies.
When the data format is flexible, you might just add #[derive(Deserialize)] on all your structs to get the default behavior. In this case, since there's a particular YAML format we're trying to match, we need to add some annotations to explain to Serde just how to put things together. I used chrono::NaiveDate in part because it already implements Deserialize, which makes this task easier by half.
#[derive(Deserialize)]
#[serde(transparent)]
struct Document {
    by_date: HashMap<Date, Expenses>,
}

#[derive(Deserialize)]
#[serde(transparent)]
struct Expenses {
    by_category: HashMap<String, Vec<Entry>>,
}

#[derive(Deserialize)]
#[serde(try_from = "HashMap<String, String>")]
struct Entry {
    payee: String,
    payments: Vec<f32>,
}

impl TryFrom<HashMap<String, String>> for Entry {
    type Error = String;
    fn try_from(value: HashMap<String, String>) -> Result<Self, Self::Error> {
        for (payee, etc) in value {
            let payments = etc.split('+')
                .map(|s| s.trim().parse::<f32>())
                .collect::<Result<_, _>>()
                .map_err(|e| e.to_string())?;
            return Ok(Entry { payee, payments });
        }
        Err("Empty map".to_owned())
    }
}

I didn't have to actually write any code for Document and Expenses; I just used the serde(transparent) attribute to tell Serde to treat those structs exactly the same as their contained types. For Entry, since Serde's YAML deserializer can already parse a string like rent: 2750 into a HashMap<String, String>, I just implemented TryFrom<HashMap<String, String>> and told Serde to use that. This handy trick requires less code and is easier to wrap your head around than a full Deserialize implementation, but may be slightly slower.
Now all we have to do is use it:
let doc: Document = serde_yaml::from_str(&doc).unwrap();
println!("total: {}", doc.total());

For a small initial investment in code, your data is now made of meaningful, Rusty types like Date and HashMap instead of all different Yaml values. You can add a little bit more code to also support serialization, and because Serde is a general purpose (de)serialization library and not a particular data format, switching to another format like JSON or bincode (should you want to) is just a couple lines of code.
And another thing
Floating-point numbers are bad for money because they have variable precision. Use a decimal library, or fixed-point arithmetic (i.e. store an integer number of the smallest possible amount of money – cents instead of dollars.) If you absolutely must use floating-point numbers, at least use f64, and don't say I didn't warn you not to.


Simple PHP Validation Routine I have created a simple validation routine in php and i'm wanting to make sure I am doing things efficiently....

function reg_err_validation($reg_errors) {
        global $woocommerce;
        extract($_POST);

        if($firstname == '' )
        {
            $reg_errors->add( 'registration-error', __( 'Please enter First Name.', 'woocommerce' ));
        }
        if($lastname == '' )
        {
            $reg_errors->add( 'registration-error', __( 'Please enter Last Name.', 'woocommerce' ));
        }
        if($phone == '' )
        {
            $reg_errors->add( 'registration-error', __( 'Please enter Phone.', 'woocommerce' ));
        }
        if($address1 == '' )
        {
            $reg_errors->add( 'registration-error', __( 'Please enter address.', 'woocommerce' ));
        }
        if($city == '' )
        {
            $reg_errors->add( 'registration-error', __( 'Please enter City.', 'woocommerce' ));
        }
        if($postcode == '' )
        {
            $reg_errors->add( 'registration-error', __( 'Please enter Postcode.', 'woocommerce' ));
        }
        return $reg_errors;
}

It all works correctly but would like to try and simplify it if possible.

There is one huge security oversight here, and that's the use of extract().

What you're doing is extracting $_POST data straight into $variables.

Suppose someone was to edit your form, or the POST request, and add a field of "woocommerce", $_POST['woocommerce'] would then become $woocommerce.

This would overwrite whatever your existing $woocommerce variable is storing, and as it'a global I'm guessing it's pretty important.

You can avoid overwritting existing variables by using EXTR_OVERWRITE and EXTR_SKIP.
See the PHP documentation for more information.

I advise you drop the extract() function, especially for $_POST/$_GET/$_FILES, $_REQUESTS.
In your case, it's much safer to rely on isset() or empty(). This will also prevent any E_NOTICES occurring when referencing a variable which may not exist, when using extract().

extract($_POST);

if($firstname == '' )
{
    $reg_errors->add( 'registration-error', __( 'Please enter First Name.', 'woocommerce' ));
}

Should become,

if(empty($_POST['firstname']))
{
    $reg_errors->add( 'registration-error', __( 'Please enter First Name.', 'woocommerce' ));
}


K&R C book - exercise 3.3 I have an exercise in my ANSI C book:

  3.3. Write a function expand(s1,s2) that expands shorthand notations like a-z in the string s1 into the equivalent complete list abc…xyz in
  s2. Allow for letters of either case and digits, and be prepared to
  handle cases like a-b-c and a-z0-9 and -a-z. Arrange that a leading or
  trailing - is taken literally.

I have attempted to solve it and here's my code:

#include <stdio.h>
#include <string.h>
#define MAXLEN 100

void expand(char * s1, char * s2);

int main(void) {
    char s1[] = "a-b-c";
    char s2[MAXLEN];
    expand(s1, s2);
    printf("%s\n", s2);
    return 0;
}

void expand(char * s1, char * s2) {
    char prev = '\0';
    char mid = '\0';
    char next = '\0';
    if (s1[0] == '-') {
        *s2 = *s1;
        s1++, s2++;
    }
    while (*s1 != '\0') {
        if (next == '\0' && mid == '-')
            *s2++ = mid;
        else if (mid == '-') {
            if (prev < next)
                for (char i = prev; i <= next; i++, s2++)
                    *s2 = i;
            else
                for (char i = prev; i >= next; i--, s2++)
                    *s2 = i;
        }
        prev = *s1;
        s1++;
        mid = *s1;
        if (*s1 != '\0')
            next = *(s1 + 1);
    }
    *s2 = *s1;
}

What do you think about this solution? Does it solve the exercise? Is it a good way to solve it?

#include <string.h>

We don't use any of the definitions of this header, so it can safely be omitted.

#define MAXLEN 100

Why 100?  More importantly, what happens when we reach this maximum length?  We probably need to add some code to stop producing output when it's reached.  The provided interface expand(s1,s2) doesn't allow us to tell the function how much space is available to write into s2, so that's probably out of scope for the exercise, but consider implementing expand(s1,s2,max_len) as a further exercise (and a good one, that will be useful in your future C coding).

What should the output be for a-b-c?  Your program produces abbc, but I don't know if that's expected.  At least add a comment to document your assumptions.  Other possible interpretations include abc and ab-c, so explain why you've chosen abbc for this input.

Another problematic test case: --5.

Use const for the input string.  If we also return the start of the output, we can use the function more naturally:
char *expand(const char *s1, char *s2) {
    char *const s2_start = s2;

    /*...*/

    return s2_start;
}

int main(void) {
    char s2[MAXLEN];
    puts(expand("--5", s2));
}

Note that I've used puts() as a simpler alternative to sprintf("%s\n", ).

Your indexing is odd and inconsistent.  I see s1[0] mixed in with *s, and *(s1 + 1) in place of the simpler s1[1].  You can also make use of the value of increment expressions, like this:
if (*s1 == '-') {
    *s2++ = *s1++;
}

            char i = prev;
            if (prev < next)
                while (i <= next)
                    *s2++ = i++;
            else
                while (i >= next)
                    *s2++ = i--;

Improved code
I've completely re-written, observing the above changes:
#include <stdio.h>

char *expand(const char *s1, char *s2, int max_len) {
    char *const s2_start = s2;
    char *const s2_end = s2_start + max_len - 1;

    while (*s1 && s2 < s2_end) {
        if (*s1 == '-' && s2 != s2_start && s1[1]) {
            /* spotted hyphen not at beginning or end */
            char first = s1[-1];
            char last = *++s1;
            /* produce a range first..last,
               exclusive (we've already written first) */
            if (first == last) {
                ++s1;
            } else if (first < last) {
                /* ascending */
                for (char i = ++first;  i < last && s2 < s2_end;  ++i)
                    *s2++ = i;
            } else {
                /* descending */
                for (char i = --first;  i > last && s2 < s2_end;  --i)
                    *s2++ = i;
            }
        } else {
            /* copy to output */
            *s2++ = *s1++;
        }
    }
    *s2 = '\0';

    return s2_start;
}

#define MAXLEN 100
int main(void) {
    char s2[MAXLEN];
    puts(expand("a-z-a-z-a", s2, MAXLEN));
}


Creating SQL Indexes on Large Tables I currently have a table with about 29 million records. The table currently has no indexes.  However, I would like to build a clustered composite index built on three columns.

When I attempt to add the NON NULL constraint, my logs fill up. So my approach is to create three new columns with NON NULL, fill them up with data from the original three columns, and create my index on the three new columns.

In order to do this, I first loop through the table in batches to populate the column. This is because I was running out of transaction log space. This was based on the lock escalation page on Microsoft's site. The threshold is 5000 locks. If rows are being locked, I wanted to come in under that limit. So 4000 rows at a time.

I didn't create a new table, insert into it, then drop & rename because I thought it would be more efficient to just change some data than move everything - I might be wrong about this. When I did try it, I ran out of log space again.

This is the SQL I am using to update my new columns looks like this:

DECLARE @Rows INT,
        @BatchSize INT;      -- keep this below 5k

SET @BatchSize = 4000;

SET @Rows = @BatchSize;

BEGIN TRY
    WHILE (@Rows = @BatchSize)
    BEGIN
        UPDATE Top(@BatchSize) Master.VOUCHER_LINE
        SET tempBusinessUnit = BUSINESS_UNIT
        FROM Master.VOUCHER_LINE
        WHERE tempBusinessUnit = 'xxxxx'; /* This is a default value for the column.
        It identifies columns that have not been updated. */

        SET @Rows = @@ROWCOUNT;

        /* This prints the time that each loop is completed.
        I use it to estimate how much time this script will take. */
        DECLARE @Time nvarchar(100)
        SET @Time = CAST(CURRENT_TIMESTAMP as nvarchar)
        RAISERROR(@Time, 0, 1) WITH NOWAIT

    END
END TRY

BEGIN CATCH
    DECLARE @ErrorMessage NVARCHAR(4000);
    DECLARE @ErrorSeverity INT;
    DECLARE @ErrorState INT;

    SELECT @ErrorMessage = ERROR_MESSAGE(), @ErrorSeverity = ERROR_SEVERITY(), @ErrorState = ERROR_STATE();

    RAISERROR(@ErrorMessage, @ErrorSeverity, @ErrorState);
END CATCH;

With 29 million records (and about 100 columns), I expect this operation to take a while. After running for the first hour, the performance is 4,000 records about every 2 minutes.  At that rate this single column will be done in approximately 10 days.

Is there something I can do to improve performance?

Community wiki because the root of the answer (use an MLT to copy the data) was already in the comments. Shoutout to dnoeth and Der Kommissar for already explaining most of this in the comments.

Kendra Little has a great article on this topic. In general, the ALTER TABLE statement can't be assumed to be minimally logged, in particular if it is a size-of-data operation, meaning that

each record in the table must be updated

When something is a size-of-data operation is a little outside the scope of this question, and you can find good resources for it online, but in general, if changing the column metadata also requires updating the table itself (which making a column NOT NULL is) then you're going to hit this bucket. As Kendra mentions, this can be really bad:

Once you get to the point where you can alter the data, you run into a different problem:

Altering the column uses a huge amount of transaction log space
It’s reaaaalllll slow

This is because changing from INT to BIGINT is a “size-of-data” operation.
SQL Server has to go through and update Every. Single. Row.
And log it.
And SQL Server has to reserve extra space in the log in case it needs to roll back (because that will require additional logging).
If you get this far without thinking about how much log file you’re going to need and you run out, then things get extra slow, because most of that rollback’s going to be done by just one thread.

The solution is, not surprisingly, also called out in that article. You're likely going to need to take a downtime, and you're likely going to need to create a new table, copy all the data, and then swizzle everything to be pointing in the right direction.
The most important feature that this relies on is Minimal Logging (required reading). If/when you achieve Minimal Logging, you're likely going to be able to eliminate your batching (or at least get rid of a lot of it). Additionally, you can likely do it without too much of a downtime if you use Snapshot Isolation (also required reading). In that scenario, you just start dumping data into a copy of the table to get most of it in, then take a downtime, clean up any discrepancies that happened during that time, swap the tables, fix any foreign keys, etc etc, then come away from downtime.
If you find yourself still in need of batching, you're going to be better off first getting a list of keys to process, and looping over that instead of using TOP or other techniques (likely gets you better table access and will avoid any sorts).
You'll also want to keep your recovery model in mind; if you have a FULL recovery model then you will need a manual full-backup, then switch to SIMPLE, do the work, switch back, and backup again. This is because minimal logging requires SIMPLE recovery mode.


Discord bot with Psycopg I've made a Discord Bot in Python that uses the Psycopg module to make calls to a PostgreSQL database in order to display information.
https://github.com/Chombler/cardbot
Specifically, I call this function when a user calls the bot:
def pullCardRecord(recordName):
    success = True
    try:
        print("Trying")
        connection = psycopg2.connect(user = db_credentials[0],
                                        password = db_credentials[1],
                                        host = db_credentials[2],
                                        port = db_credentials[3],
                                        database = db_credentials[4])
        print("connected")
        cursor = connection.cursor()

        join_table_query = '''
        SELECT id
        FROM card
        WHERE card.name = ('%s') ''' % (recordName)

        cursor.execute(join_table_query)
        results = cursor.fetchall()
        print(results)
        if len(results) < 1:
            success = False
            raise ValueError('The name that was given to cardbot didn\'t exist in the card table.')

        join_table_query = '''
        SELECT  name,
                cardclass.cardclass,
                tribe.tribe, cardtype.cardtype,
                cost, side.side, strength, trait.strengthmodifier, health, trait.healthmodifier,
                trait.trait,
                ability,
                flavor,
                cardset.cardset,
                rarity.rarity
        FROM card
        LEFT JOIN cardtoclass ON card.id = cardtoclass.cardid
        LEFT JOIN cardclass ON cardtoclass.classid = cardclass.id
        LEFT JOIN cardtotrait ON cardtotrait.cardid = card.id
        LEFT JOIN trait ON cardtotrait.traitid = trait.id
        LEFT JOIN cardtotribe ON card.id = cardtotribe.cardid
        LEFT JOIN tribe ON cardtotribe.tribeid = tribe.id
        LEFT JOIN cardtype ON cardtype.id = card.typeid
        LEFT JOIN cardset ON cardset.id = card.setid
        LEFT JOIN rarity ON card.rarityid = rarity.id
        LEFT JOIN side ON card.sideid = side.id
        WHERE card.name = ('%s') ''' % (recordName)

        cursor.execute(join_table_query)
        results = cursor.fetchall()

        print("Printing Results")
        for row in results:
            for col in row:
                print(col)
            print()

        cardInstance = cardObject(results)
        print(cardInstance.information())

        # Print PostgreSQL version
        cursor.execute("SELECT version();")
        record = cursor.fetchone()
        print("You are connected to - ", record,"\n")

    except (Exception, psycopg2.Error) as error :
        print ("Error retrieving card information using PostgreSQL,", error)
    finally:
        #closing database connection.
        if(connection):
            cursor.close()
            connection.close()
            print("PostgreSQL connection is closed")
        return(cardInstance.information() if success else "I'm sorry, I couldn't find a card with that name.")

I want to make sure that the bot is protected from any direct sql injections before I deploy it. Is it currently fairly safe or do I need to add anything to protect its integrity?

Unpacking
Try this rather than hard-coded indexing:
    user, password, host, port, database = db_credentials
    connection = psycopg2.connect(user, ...)

Or, if you're able to rearrange those parameters such that they match those of the connect signature:

dbname – the database name (database is a deprecated alias)
user – user name used to authenticate
password – password used to authenticate
host – database host address (defaults to UNIX socket if not provided)
port – connection port number (defaults to 5432 if not provided)

then you can simply do
connection = psycopg2.connect(*db_credentials)

Logs
Consider replacing
    print("Trying")
    print("connected")

with calls to the actual logging module, which is more configurable and maintainable.
Quote escapes
'The name that was given to cardbot didn\'t exist in the card table.'

can be
"The name that was given to cardbot didn't exist in the card table."

In-application queries
Your join_table_query is long. There are several approaches to improve this - either save a view in the database (my favourite), or a stored procedure (common but I think it's overkill in this case).
Injection

the bot is protected from any direct sql injections before I deploy it

This is directly vulnerable:
'''... WHERE card.name = ('%s') ''' % (recordName)

Never (ever) use string formatting to insert parameters to a query. All DB connection libraries have anticipated this concern and most approach it using "prepared statements".
This article is old but relevant. The actual reference shows that you should be passing vars as a sequence or a mapping, which will prevent injection.


Need reviews for impacts of dirty bug fix I was having a hard time with bug logstash#665. Hope they will fix it some day, but sparc/solaris support is probably not on top of their priority list. For further reference, original error is:

NotImplementedError: stat.st_dev unsupported or native support failed to load dev_major at org/jruby/RubyFileStat.java:190
_discover_file at /tmp/aaaa/logstash-1.4.1.dev/vendor/bundle/jruby/1.9/gems/filewatch-0.5.1/lib/filewatch/watch.rb:140

After digging a lot I found out that the problem here is the usage of a native library which is not correclty retrieved on Solaris. So when ruby fails, it is trying a fallback to java, which fails as well. Now I have retrieved the original watch.rb source file and isolated the problematic function:

private
def _discover_file(path, initial=false)
  globbed_dirs = Dir.glob(path)
  @logger.debug("_discover_file_glob: #{path}: glob is: #{globbed_dirs}")
  if globbed_dirs.empty? && File.file?(path)
    globbed_dirs = [path]
    @logger.debug("_discover_file_glob: #{path}: glob is: #{globbed_dirs} because glob did not work")
  end
  globbed_dirs.each do |file|
    next if @files.member?(file)
    next unless File.file?(file)

    @logger.debug("_discover_file: #{path}: new: #{file} (exclude is #{@exclude.inspect})")

    skip = false
    @exclude.each do |pattern|
      if File.fnmatch?(pattern, File.basename(file))
        @logger.debug("_discover_file: #{file}: skipping because it " +
                      "matches exclude #{pattern}")
        skip = true
        break
      end
    end
    next if skip

    stat = File::Stat.new(file)
    @files[file] = {
      :size => 0,
      :inode => [stat.ino, stat.dev_major, stat.dev_minor],
      :create_sent => false,
    }
    if initial
      @files[file][:initial] = true
    end
  end
end # def _discover_file

More precisely, the bug was happening there:

    stat = File::Stat.new(file)
    @files[file] = {
      :size => 0,
      :inode => [stat.ino, stat.dev_major, stat.dev_minor],
      :create_sent => false,
    }

So I a tried to understand what those two lines do and how to work them around to somehow have something which works. For reference, documentation says:

  dev_major() public

  Returns the major part of File_Stat#dev or nil.

After some experimentation, I concluded that since this function can return nil, I could just replace the call of the function by nil like this:

stat = File::Stat.new(file)
@files[file] = {
  :size => 0,
  :inode => [stat.ino, nil, nil],
  :create_sent => false,
}

Just replaced _stat.dev_major_ and _stat.dev_minor_ by nil, launched the program, and it worked. Now I did not test a lot but everything seems to work fine.

So I need your reviews for that last snippet of code which is of my own: The program seems to be bearing with it, but is it really safe to do this?

In Unix filesystems, an inode is a number that uniquely identifies a file within the filesystem.  (More strictly speaking, an inode is a data structure that holds metadata about a file within the filesystem; each inode is assigned an inode number, and the number is all you care about unless you're a kernel developer.)

However, the inode number only uniquely identifies a file within its filesystem.  If /tmp and /usr are separate filesystems, inode 3461 of the /tmp filesystem might be /tmp/.ICE-unix while inode 3461 on /usr might be /usr/share/dict/words.  Therefore, to uniquely identify a file on the whole computer, a tuple of (inode number, device major number, device minor number) is needed.

By ignoring the device number, you might end up acting on events on a file with a coincidentally equal inode number on a different filesystem — though the chance is miniscule.

If you decide to keep this workaround, then I think you should make a corresponding change on line 71 as well.


Calculations to position a text box next to another object I am implementing a program to draw a box of text next to some other drawn object (that can have any position and dimensions) and the text box should always be visible, even if the drawn object's width is of the entire screen and vice versa.
They should simply not overlap.

This is a code to position the text on some relatively good coordinates.

if(x > (box_w + 20)) box_x = x - (box_w + 20);
else                            box_x = w + 20;
if(y > (box_h + 20)) box_y = y - (box_h + 20);
else                            box_y = h + 20;
if(w > width - (box_h + 20) || x + w + box_w + 20 > width)
{
    box_x = 20;

    if(y < box_h + 20)
    {
        box_y = h + 20;
    }
    else box_y = y - (box_h + 20);
}

x, y, w, h are the coords and dimensions for the object; box_x, box_y, box_w and box_y are for the text box and width/height is the dimension of the area.

Here is schematically how this code positions text (darker blue) near the object (lighter blue)

It is quite obvious that it could have been written much better.
To target performance, but more importantly - code space.

Can anyone suggest a better way to find good coordinates for the text box?

This is a definite candidate for using rect / point structures, but I don't know what the rest of your code-base looks like, so just talking about the algorithm alone, one approach would be:

void FindNonOverlappingPosition(
    uint canvasWidth, uint canvasHeight,
    uint x1, uint y1, uint width1, uint height1,
    uint width2, uint height2, uint spacing,
    uint* outX, uint* outY)
{
    // Position above if possible, below otherwise
    if (y1 >= height2 + spacing)
        *outY = y1 - (height2 + spacing);
    else
        *outY = y1 + height1 + spacing;

    // Position left-aligned if content is closer to the left of the canvas, right-aligned otherwise
    if (x1 < canvasWidth - width1)
        *outX = x1;
    else
        *outX = (x1 + width1) - width2;

    // Ensure that the box being positioned fits on the canvas
    if (*outX < 0)
        *outX = 0;
    else if (*outX + width2 > canvasWidth)
        *outX = canvasWidth - width2;
}

This fails in cases where an alignment according to those rules is impossible, such as the content taking up the entire height of the screen, or the text box being wider than the screen.  You could check for those cases and return an error code if desired.


Determine whether to push or update object in array based on unique ID I am trying to write a basic function that will decide whether the object already exists in array of objects based on unique id that is inside this object as a property. Upon searching I came up with this code, and it seems to be doing the job fine, but I can imagine there is a better and faster way to determine whether to push or update the object. If you could share your best answer for this please...

var myArray = [
  {
    id: 1,
    car: "Ford"
  },
  {
    id: 2,
    car: "Honda"
  }
];

var objToOverwrite = {
  id: 1,
  car: "Bentley" // to overwrite car: "Ford"
};

var objToPush = {
  id: 3,
  car: "Toyota"
};

function pushToArray ( arr, obj ) {

  var existingIds = arr.map((obj) => obj.id);

    if (! existingIds.includes(obj.id)) {
      arr.push(obj);
    } else {
      arr.forEach((element, index) => {
        if (element.id === obj.id) {
          arr[index] = obj;
        };
      });
    };
};

pushToArray ( myArray, objToPush );

console.log(myArray);

Your pushToArray function is less efficient than it could be because it it looping over the entire array multiple times. We could instead use find()/findIndex() to loop over the array just once:

function pushToArray(arr, obj) {
    const index = arr.findIndex((e) => e.id === obj.id);

    if (index === -1) {
        arr.push(obj);
    } else {
        arr[index] = obj;
    }
}

This is still O(n) performance, but does less repeated work and does not require altering the data structure you are using.

If you need better performance than that, you can keep the array sorted by ID and implement a simple binary search, or you can change your data structure to one more optimized for fast lookups.


Add/Remove items thread-safely in List Recently I had to lock collections (of type List<T>) when items were added or removed. Because several collections were used in given code instead of creating helper methods for each collection, I made an extension methods:

public static class MyExtension
{
    public static void AddThreadSafely<T>(this ICollection<T> collection, T item)
    {
        if (collection == null)
            throw new ArgumentNullException("collection");

        bool locked = false;
        try
        {
             System.Threading.Monitor.Enter(collection, ref locked);
             collection.Add(item);
        }
        finally
        {
             if (locked)
                 System.Threading.Monitor.Exit(collection);
        }
    }

    //The same with remove
}

and then simply replaced Add with AddThreadSafely.

I like the simplicity of this solution but... What are the drawbacks of using this method? Will it be good idea to use this method of locking collections in future projects?

I would advise against this approach, you are correct in the fact that it is simple, however it's not foolproof. There is nothing actually ensuring that someone doesn't just call .Add() on the collection bypassing your extension method which allows for difficult to find bugs/race conditions.

Don't forget that all Monitor.Enter does is ensure that if multiple threads call Monitor.Enter that only 1 can enter a code block at a time. It doesn't prevent a caller on another thread mutating the collection if they don't call Monitor.Enter on the same obejct.

You would be better off either creating a custom collection which inherits from CollectionBase and encapsulating the lock logic in there so that any caller calling .Add() goes through the lock and is unable to bypass it.

Or if you are using .NET 4.0+, look at one of the collections in System.Collections.Concurrent as they are designed for this sort of activity in the first place.


Elegantly exclude part of an operation for the last iteration in a loop I'm afraid just about everybody who read this question has misunderstood it. I'm not trying to find the best way of printing a vector. I'm trying to find the best way of excluding part of an operation from the last iteration in a loop. I'm seeking advice about a pattern that is repeated is many different contexts. For this reason, I cannot provide a concrete example.

This question is off-topic

A well-formed comma-separated list has commas after all but the last element. This means that when dealing with these lists, the comma has to be dealt with on all but the last iteration. This is how I do things currently:

for (auto e = vec.cbegin(); e != vec.cend(); ++e) {
  // code that has to be executed for every element
  std::cout << *e;
  if (e != vec.cend() - 1) {
    // code that must not be executed for the last element
    std::cout << ", ";
  }
}

There are five instances of the above pattern in the project that I'm working on. I feel like there might be a more elegant way of doing this. An alternative that I've come up with is this:

// infinite loop if vec is empty
if (!vec.empty()) {
  for (auto e = vec.cbegin(); e != vec.cend() - 1; ++e) {
    // code that has to be executed for every element
    std::cout << *e;
    // code that must not be executed for the last element
    std::cout << ", ";
  }
  // code that has to be executed for every element
  std::cout << vec.back();
}

This alternative is slightly faster because there isn't an if in the body but that's not a big deal. The if is true for all but the last iteration so the branch predictor will probably only miss on the last iteration. I also have to repeat code that has to be executed for every element which means that I need to hoist that into a function.

The first snippet is the best that I can come up with but I feel like this could be done better. Note that writing to std::cout is just an example.

If you are just doing a single-shot printing of a container, just print the first element and then print delim-value pairs.  This reduces the iterator requirements to being equality comparable, dereferenceable, and incrementable.

if (!vec.empty()) {  // invert to return immediately if this is a function
    auto first = vec.cbegin();
    std::cout << *first;

    while (vec.cend() != ++first) {
        std::cout << ", " << *first;
    }
}

There is also a stateful approach that reduces the operation strength from branching on whether to print the delimiter to a pointer assignment.

auto actual_delim = ", ";
auto delim = "";

for (const auto& elem : vec) {
    std::cout << delim << elem;
    delim = actual_delim;
}

See infix_iterator where this stateful approach is used.


Checking for missing values in several columns of a spreadsheet Basically I had 3 columns of Data

Adjudicated Cases
Not Adjudicated Cases
All Cases

and I wanted to get a list of all the cases that were missing from Column A and Column B, with a little help from @RubberDucky and his really cool Enumerable.cls (Found on GitHub in His VBX Repository) I used the Merge function to first merge Column A and Column B together in an Enumerable and then using the Contains function I checked to see if the Values in the Enumerable were contained in Column C, if they weren't there I added them to a second Enumerable and then printed that one out into another column.

How can I further hone my VBA skills?

Sub FindUnAssigned()
    With Sheets("FY13")
        Dim e As New Enumerable
        Dim lastARow As Integer
        lastARow = .Range("A" & .Rows.Count).End(xlUp).Row
        Dim lastBRow As Integer
        lastBRow = .Range("B" & .Rows.Count).End(xlUp).Row

        Set e.Collection = Range("A3:A" & lastARow)
        e.Merge (Range("B3:B" & lastBRow))

        Dim cell As Range
        Dim columnD As New Enumerable
        Dim lastCRow As Integer
        lastCRow = .Range("C" & .Rows.Count).End(xlUp).Row

        For Each cell In Range("C3:C" & lastCRow)
            If Not (e.Contains(cell.value, True)) Then
                columnD.Add (cell.value)
            End If
        Next cell

        Dim i As Integer
        For i = 1 To columnD.Count
            Dim j As Integer
            j = i + 2
            Range(("D" & j), ("D" & columnD.Count + 2)) = columnD.item(i)
        Next i
    End With
End Sub

I discourage the use of single letter variable names. So, I recommend changing e to something else. I'm drawing a blank though because you can't use Cells in Excel's flavor of vba. That's the object that refers to the sheet's collection of all cells. Maybe cases would work given the context.
I like your use of the With statement here. I'm tossing that out there because I know others will disagree with me. With can be abused, but this is not the case with your code.
When declaring an iterator variable for a ForEach statement, I like to declare it just outside the loop. You should move Dim cell As Range.
j is a useless variable. I would just add two to i instead.
Speaking of 2, it's a magic number. I would consider declaring an offset variable.

    Dim i As Integer
    Const offset As Integer = 2

    For i = 1 To columnD.Count
        Range(("D" & i + offset), ("D" & columnD.Count + offset)) = columnD.item(i)
    Next i

I take back what I said about the With statement. It would be a good use, but you're using Range wrong. For example, this line doesn't do what you think it does.

Range(("D" & i + offset), ("D" & columnD.Count + offset))

You're using the global Range object, which is the same as saying ActiveSheet.Range(). That's not what you want. This can lead to some hard to track down heisenbugs. You want to work on your named sheet. You need to add the dot in order to be working on the named sheet.

.Range(("D" & i + offset), ("D" & columnD.Count + offset))

From here on out I'm at a bit of an unfair advantage because I wrote the Enumerable class that you're using.

You don't need the .value when using the useDefaultProperty option. Value is the default property of any Range object.

If Not (e.Contains(cell.value, True)) Then

Can be changed to

If Not (e.Contains(cell, True)) Then

The same thing applies to the Add method.

columnD.Add (cell.value)

Enumerable uses a magic attribute that gives it a default property as well. So, just like the built in Collection, there's no reason to use the verbose method of getting an item. Item is the default property.

columnD.item(i)

Becomes

columnD(i)