Input stream for console menu I've written a simple numeric menu which displays in the console. When the user types in '1', something happens.  When the user hits ENTER, I clear the whole output except for the menu itself. When the user types in '5', the program ends.

However, the code got ugly. It's full of cin.get()s, couts, and other stuff.

int main()
{
    using namespace std;
    node *root = NULL;
    char ch = '0';

    while(ch != '5')
    {
        cout << "1. Add new elements\n"
            "2. Display info about the tree\n"
            "3. Remove nodes\n"
            "4. Export to file\n"
            "5. Exit\n";
        cin >> ch; cin.get();

        switch(ch)
        {
            case '1': case '3':
            {
                vector<int> numbers;
                string line;
                cout << "\nProvide numbers divided with spaces: ";
                getline(cin, line);
                istringstream in(line, istringstream::in);
                int current;
                while (in >> current) numbers.push_back(current);

                (ch == '1') ? insert(root, numbers) : remove(root, numbers);
                cout << "\nClick to return to menu";
                cin.get();
                break;
            }
            case '2':
            {
                displayTreeInfo(root, cout);
                cout << "\n\nClick to return to menu";
                cin.get();
                break;
            }
            case '4':
            {
                string name;
                cout << "Provide filename: ";
                cin >> name;

                buildTree(root, loadNumbersFromFile(name));
                exportTreeToFile(root, name);
                std::cout << "\nData has been exported.\n\n";
                cin.get(); cin.get();
                break;
            }
        }
        system("cls");
    }

    exit(EXIT_SUCCESS);
}

How could I improve this code?

Using namespace std: I've already mentioned this in the comments.  Although it's not a huge deal for small programs such as this, it's still not a good habit to develop.
While loop: Put std::cin << ch both before the loop and after the switch.  You could also change the loop condition to

(ch == '1' || ch == '2' || ch == '3' || ch == '4')

if you really want to make sure that the program will only proceed with a valid input.    Other than that, your original condition is okay.
std::cin: You don't need std::cin.get() alongside std::cin >>.  Just use std::cin >> for char inputs.  However, you'll need a std::cin.ignore() before each getline since you're doing a char input (menu prompt) before a std::string input in your cases.  For case 4, keep a consistent getline for the filename input and put an ignore before it.

Also, you don't need any cins after your cases since your while loop will always ask for an input for ch.  I do see what you're doing with your outputs in your cases, but you don't need to do that.  Just let your cases do their things and go straight to the menu and/or prompt again.
Filename input: You should provide validation for the filename input.  If the user enters in invalid filename, what should happen?  Consider implementing a loop asking the user for a valid filename as long as such file cannot be opened.  With the provided code here, it's a bit tough for me to give a piece of example code for that.
Functions: You could nicely organize your switch statements by putting the body code into functions.  Just name them as they appear in your menu.  Call each one in your respective cases, along with the breaks.  Also, it might be better to have separate procedures for add/remove.
Exit: You don't really need exit(EXIT_SUCCESS) here if the program will always terminate successfully.  You also don't need system("cls"), so just leave it out.  If you're doing that just to prevent the menu from appearing before each prompt, just pt it before the while loop.


Returns the year for each month by period - nested case statements def return_year_by_month_and_period(month)
pw = self.property_water.first().billing_increment
case pw
  when "12"
    self.property_water.where(period: month).first().year
  when "6"
    case month
      when 1, 2
        self.property_water.where(period: 1).first().year
      when 3, 4
        self.property_water.where(period: 2).first().year
      when 5, 6
        self.property_water.where(period: 3).first().year
      when 7, 8
        self.property_water.where(period: 4).first().year
      when 9, 10
        self.property_water.where(period: 5).first().year
      else
        self.property_water.where(period: 6).first().year
    end
  when "4"
    case month
      when 1, 2, 3
        self.property_water.where(period: 1).first().year
      when 4, 5, 6
        self.property_water.where(period: 2).first().year
      when 7, 8, 9
        self.property_water.where(period: 3).first().year
      else
        self.property_water.where(period: 4).first().year
    end
  when "2"
    case month
      when 1, 2, 3, 4, 5, 6
        self.property_water.where(period: 1).first().year
      else
        self.property_water.where(period: 2).first().year
    end
  else
    self.property_water.first().year
end
end

"Period" in the text above is one of months, bi-monthly, quarterly, semi-annually or annually.

For each of the 12 months in the calendar year, we want to associate a year (e.g. 2015) with the month depending on what year is associated with the period.

First let me say - I think the database modeling for this particular thing is horribly FUBAR.  I inherited the project - and re-making the database is on the roadmap, but not the immediate need.

So in the example above, if I had a period of 2 (bi-monthly) - I would only have 6 records in the property_waters table.  But I still have 12 months, so I need to say "the 1st period is Jan and Feb" and this logic has to carry.  The property_waters table does NOT have a 'month' column, meaning my two attributes are period and year.  From the period, I can determine which months belong in which group, and return the year of the period.

The code works - but even after I wrote it I knew I could do better.  I just can't see it, so please help me learn a better way to do this.

EDIT:
Billing increment for every record is always the same integer as the period. So if we have 6 records for billing period of bi-monthly, all 6 records have billing_increment = 6.

Ridiculous I know. We can just count the records to return this same thing...

Assuming that billing_increment is always an integer that is a factor of 12, the code should be entirely formulaic.

def return_year_by_month_and_period(month)
  period_len = property_water.first.billing_increment.to_i
  if 12 % period_len != 0
    # TODO: decide what the appropriate behavior should be
    self.property_water.first.year
  else
    nth_period = (month - 1) / period_len + 1
    property_water.where(period: nth_period).first.year
  end
end

I've incorporated additional suggestions from @tokland:

In Ruby it's not idiomatic to write self., nor parens on calls without arguments.
Write the second case in an else. Full conditional branches are easier to read.


API for looking up Japanese postal code Most of my experience is in Python, Java and embedded C, and I need to learn Scala and Play for a new job. As an exercise, I decided to create a very simple API for looking up area by postal code, backed by Redis. It's just a single controller with a couple of routes.

I'm hoping for feedback from experienced Scala or Play users. Please suggest any improvements to make the code more readable, maintainable, idiomatic, performant, secure etc. As a starting point, I enabled several static analysis tools in my build and fixed the issues reported.

controllers/JPI.scala

package controllers

// Scala imports
import util.control.Breaks._

// Play imports
import play.api.mvc._
import play.api.Play.current
import play.api.cache.Cache
import play.api.libs.json._
import com.typesafe.plugin.RedisPlugin

// Java imports
import java.io.{BufferedReader, InputStreamReader}
import java.net.URL
import java.util.zip.ZipInputStream

object JPI extends Controller {

  final val JapaneseURI = "http://www.post.japanpost.jp/zipcode/dl/kogaki/zip/ken_all.zip"
  final val TestURI = "http://localhost:9000/assets/ken_all.zip"
  final val CSVEncoding = "SJIS"

  final val PostalCodeMin = 0
  final val PostalCodeMax = 9999999
  final val PostalCodeFmt = "%07d"

  final val CSVFields = List(
    ("prefecture_kana", 3),
    ("municipality_kana", 4),
    ("neighbourhood_kana", 5),
    ("prefecture_kanji", 6),
    ("municipality_kanji", 7),
    ("neighbourhood_kanji", 8)
  )

  /**
   Lookup a numeric postal code in the Redis cache
  */
  def lookup(postalCode: Integer) = Action {
    if (postalCode < PostalCodeMin || postalCode > PostalCodeMax) {
      NotFound(
        "Postal code outside valid range (%s to %s)".format(
          fmt(PostalCodeMin), fmt(PostalCodeMax)
        )
      )
    } else {
      val json: Option[String] = Cache.getAs[String](postalCode.toString)
      json match {
        case None => NotFound("Postal code not found")
        case Some(j) => Ok(j).as("application/json; charset=utf-8")
      }
    }
  }

  /**
    Imports postal codes from a zipped CSV reference as follows:
    - Download the complete code directory from the specified URL
    - Unzips the first file and parses the CSV rows
    - Serializes CSV rows into JSON
    - Inserts JSON objects into Redis hash
   */
  def buildCache() = Action {
    val inStream = new ZipInputStream(new URL(TestURI).openStream)
    val csvFile = Option(inStream.getNextEntry)
    csvFile match {
      case Some(f) => {
        val reader = new BufferedReader(new InputStreamReader(inStream, CSVEncoding))
        for (line <- Iterator.continually(Option(reader.readLine)).takeWhile(!_.isEmpty)) {
          breakable {
            val elements = line.getOrElse(break).split(",")
            val code = stripQuotes(elements(2))
            Cache.set(code, Json.stringify(parseCSVRow(elements)))
          }
        }
        Ok("Cache rebuilt")
      }
      case None => InternalServerError("Cache rebuild failed")
    }
  }

  /**
    Format a numeric postal code as a String
  */
  private def fmt(postalCode: Integer) : String = {
    PostalCodeFmt.format(postalCode)
  }

  /**
    Strip trailing and leading quotes from a String
  */
  private def stripQuotes(s : String) : String = {
    s.stripPrefix("\"").stripSuffix("\"")
  }

  /**
    Parse and convert a CSV row into a JSON object according to mapping in CSVFields
  */
  private def parseCSVRow(row : Array[String]) : JsObject = {
    val strippedRow = row map { stripQuotes(_) }
    val fields = CSVFields map {
      f => f._1 -> Json.toJsFieldJsValueWrapper(strippedRow(f._2))
    }
    Json.obj( fields: _* )
  }

}

Building and running this code is as simple as:

git clone https://github.com/bsutherland/jpi.git
cd jpi
sbt compile
sbt run

I'm skeptical of accepting postal codes as Integers.  Japanese postal codes are of the format NNN-NNNN.  Accepting a String, which may contain an optional hyphen and leading zeros, would be more appropriate.

Furthermore, an API that is designed to take an integer could never be extended to countries like the UK and Canada, where postal codes are alphanumeric.  It would also be problematic for US ZIP+4 codes, which, despite being mostly numeric like Japanese codes, may overflow an Integer.  (It's odd that you take an Integer instead of an Int, but that's a moot point.)


Find the nth term of a sequence that consists of Fibonacci and prime numbers interleaved I have given a series problem 1,2,1,3,2,5,3,7,5.... in which the odd terms forms the Fibonacci series and even terms form the prime number series. I have to write a code in which I have to find the nth term. Following is the code for finding the nth term, I want to know whether it can be written more efficiently or not and how?

 def fib(n):
     t1=0
     t2=1
     for i in range(1,n+1):
         add = t1+t2
         t1 = t2
         t2 = add
     print(t1)

 def prime(n):
     count =1
     for i in range(2,100):
         flag =0
         for j in range(2,i):
             if (i%j==0):
                flag=1
                break
         if (flag==1):
             continue
         else:
             if (count==n):
                 print(i)
                 break
             else:
                 count = count+1

 n= int(input('enter the positive number: ' ))

 if (n%2==0):
     prime(int(n/2))
 else:
     fib(int(n/2)+1)

You could use a few tricks to implement the two sequences more efficiently, but the short version of my answer is that most significant performance improvements you could make involve some relatively advanced math, and the smaller improvements do more to improve your code's readability than its performance.

Useful improvements to prime

If you keep a list of the primes you find, you only need to check if those divide the new numbers you are checking, rather than checking every number up to the number you are looking at.

You could also skip over even numbers in the outer loop (use range(3, max, 2)), thus avoiding checking even numbers that you can be sure aren't prime (you would need to add a special case for 2).

The inner loop (j) can stop at i/2, because no number can be evenly divided by a number more than half its size. Similarly, you can stop the loop at when you pass the square root of n, but you would have to implement that by squaring the factors because sqrt is limited by the inaccuracy of floating-point numbers.

Using all of these suggestions, the code might look a little like this:

def nth_prime(n):
    if n == 1:
        return 2
    primes = [2]
    for candidate_prime in range(3, MAX, 2):
        for p in primes:
            if (p ** 2 > candidate_prime):
                break # candidate is prime
            if (candidate_prime % p == 0)
                break # p divides candidate; candidate is not prime
        if no primes divided candidate_prime:
            primes.append(candidate_prime)
            if len(primes) == n:
                return candidate_prime

Additional optimizations for determining whether a number is prime are discussed on the Wikipedia page on the subject.

These improvements will only start to have noticeable effects when you start looking at very large primes, so you might also want to use itertools.count to look at all numbers instead of stopping at 100.

(If you really want to stop at 100, you could also just make a list of the prime numbers up to 100 and index that for maximum efficiency.)

Links to mathematical solutions

To really improve efficiency, this answer suggests the solution in this blog post, but this is probably overkill unless you really want to be able to calculate very large fibonacci numbers very fast (I can't tell that there's a delay in your function until somewhere far above n=10000).

This question goes into depth about how to find the nth prime number, but the final point is important to note:

  tl;dr: Finding the nth prime can be efficiently done, but the more efficient you want it, the more mathematics is involved.

Other suggestions

The following suggestions aren't really about efficiency (if they change the efficiency of your code, the difference will be immeasurably small), but they might make your code a little cleaner.

In the loop you're using for the Fibonacci sequence, you can just write t1, t2 = t2, t1 + t2 to update both values in one line.
When creating the nth Fibonacci number, you can just loop over range(n); there's no need to adjust the ends of the loop. And if you're not using the index i in that loop, Python convention is call it _, so you end up with for _ in range(n).
Using Python's less-than-well-known for/else feature might let you eliminate the flag variable. If you put an else block after a loop in Python, it will run only if you do not break out of the loop, which allows you to avoid flag variables.

for i in some_list:
    if condition(i):
        break
else:
   do_this_if_condition_was_never_true()


Sort a stack in ascending order Sort a stack in ascending order. You may only use one additional stack to hold items.

Any comments on my solution?
Worst case complexities:
Memory complexity: O(n)
Runtime complexity: O(n^2)

Also, I implemented the Stack class as having Object items and then cast these into Integers to be more general. Is this a good way of doing this?

import org.junit.Test;

public class Solution {
  // Sort a stack in ascending order
  // May only use one additional stack to hold items
  // Stack supports push, pop, peek, and isEmpty
  private Stack stack;

  private Stack auxiliaryStack;

  @Test
  void testStackIncreasingOrder() {
    stack = new Stack();
    stack.push(3);
    stack.push(2);
    stack.push(1);
    System.out.println("original stack");
    printStack();
    sortStack();
    System.out.println("Sorted stack");
    printStack();
  }

  @Test
  void testEmptyStack(){
    stack = new Stack();
    System.out.println("original stack");
    printStack();
    sortStack();
    System.out.println("Sorted stack");
    printStack();
  }

  @Test
  void testStackRandomOrder() {
    stack = new Stack();
    stack.push(3);
    stack.push(2);
    stack.push(1);
    stack.push(5);
    stack.push(1);
    stack.push(1);
    stack.push(12);
    stack.push(0);
    System.out.println("original stack");
    printStack();
    sortStack();
    System.out.println("Sorted stack");
    printStack();
  }

  public static void main(String[] args) {
    Solution e = new Solution();
    System.out.println("Test reverse sorted stack");
    e.testStackIncreasingOrder();
    System.out.println("Test empty stack");
    e.testEmptyStack();
    System.out.println("Test randomly ordered stack");
    e.testStackRandomOrder();
  }

  public void sortStack() {
    auxiliaryStack = new Stack();
    if (stack.isEmpty()) {
      return;
    }

    while (!stack.isEmpty()) {
      Object topOfStack = stack.pop();
      if (auxiliaryStack.top != null) {
        Object topOfAuxiliary = auxiliaryStack.peek();
        if ((Integer) topOfStack > (Integer) topOfAuxiliary) {
          emptyAuxiliary();
        }
      }
      auxiliaryStack.push(topOfStack);
    }
//    System.out.println("AuxiliaryStack");
//    printAuxiliaryStack();
    emptyAuxiliary();
  }

  public void emptyAuxiliary() {
    // Put all the elements of the auxiliary stack back in
    // the original one
    while (!auxiliaryStack.isEmpty()) {
      Object element = auxiliaryStack.pop();
      stack.push(element);
    }
  }

  public Stack getStack() {
    return stack;
  }

  public void printStack() {
    Node current = stack.top;
    while (current != null) {
      System.out.println(current.data);
      current = current.next;
    }
  }

  public void printAuxiliaryStack() {
    Node current = auxiliaryStack.top;
    while (current != null) {
      System.out.println(current.data);
      current = current.next;
    }
  }

}

class Stack {
  Node top;

  Object pop() {
    if (top != null) {
      Object item = top.data;
      top = top.next;
      return item;
    }
    return null;
  }

  void push(Object item) {
    Node t = new Node(item);
    t.next = top;
    top = t;
  }

  Object peek() {
    return top.data;
  }

  boolean isEmpty() {
    if (top == null) {
      return true;
    }
    return false;
  }
}

class Node {
  Object data;

  Node next;

  public Node(Object item) {
    data = item;
  }

  public Node(Object item, Node n) {
    data = item;
    next = n;
  }
}

Generics

Generics... you want them. In your solution, they would be relatively easy to apply too. Consider your Node class:

class Node {
  Object data;

  Node next;

  public Node(Object item) {
    data = item;
  }

  public Node(Object item, Node n) {
    data = item;
    next = n;
  }
}

Now, that Node only works on Object values, but you want it to be an Integer. Using generics, this is a treat (while we are at it, we will make data and next private and final, and add getters for them...):

class Node<T> {

  private final T data;
  private final Node<T> next;

  public Node(T item) {
    data = item;
  }

  public Node(T item, Node<T> n) {
    data = item;
    next = n;
  }

  public T getData() {
    return data;
  }

  public Node<T> getNext() {
    return next;
  }

}

There, now Node is self contained, immutable, and generic. It can contain any object type, just specify the type you want when you construct it, for example:

Node<Integer> node = new Node<>(10);

Applying the same logic to your Stack class, we get:

class Stack<T> {
  Node<T> top;

  T pop() {
    if (top != null) {
      T item = top.getData();
      top = top.getNext();
      return item;
    }
    return null;
  }

  void push(T item) {
    Node t = new Node(item, top);
    // commented out, make Node immutable, pass in as constructor
    // t.next = top;
    top = t;
  }

  T peek() {
    return top.getData();
  }

  boolean isEmpty() {
    if (top == null) {
      return true;
    }
    return false;
  }
}

Right, that makes it generic, there are still a bunch of problems... but, it can be easy to use with:

Stack<Integer> stack = new Stack<>();

With the above stack, you should not need any casts from Object to Integer

Simplifications

Now, looking at some other parts of your code, let's simplify...

Chained constructors: whenever possible, have just one constructor that "does stuff" for your class, and have your other constructors call the main one. Your constructors look like:

  public Node(Object item) {
    data = item;
  }

  public Node(Object item, Node n) {
    data = item;
    next = n;
  }

Note that they both access the data = item item directly. It would be better to change your first constructor to be:

  public Node(T item) {
      this(item, null);
  }

The above constructor calls the second one with a null value.

Boolean expressions are boolean. If statements test a boolean condition. Why do both?

  boolean isEmpty() {
    if (top == null) {
      return true;
    }
    return false;
  }

This is a good one, it can be simply:

boolean isEmpty() {
  return top == null;
}

The push can be simplified too:

  void push(T item) {
    Node t = new Node(item, top);
    // commented out, make Node immutable, pass in as constructor
    // t.next = top;
    top = t;
  }

That can be simply:

  void push(T item) {
    top = new Node(item, top);
  }

Note that there is no need for the one-argument constructor now in the Node class.

Code duplication is always a red flag. Consider the following methods:

  public void printStack() {
    Node current = stack.top;
    while (current != null) {
      System.out.println(current.data);
      current = current.next;
    }
  }

  public void printAuxiliaryStack() {
    Node current = auxiliaryStack.top;
    while (current != null) {
      System.out.println(current.data);
      current = current.next;
    }
  }

Why have the method twice, when instead you can have just:

  public void printStack(Node<T> current) {
    while (current != null) {
      System.out.println(current.getData());
      current = current.getNext();
    }
  }

Now, you can call that method with either the stack or the auxilliaryStack top nodes... but, that method really belongs on the stack itself, not outside the stack. Move it to the stack, and it becomes:

  public void printStack() {
    Node<T> current = top;
    while (current != null) {
      System.out.println(current.getData());
      current = current.getNext();
    }
  }

Now you can simply do:

stack.printStack();
auxilliaryStack.printStack();

Bugs

The following is a bug:

  Object peek() {
    return top.data;
  }

What if the stack is empty? You will get a NullPointerException!

Instead, do:

  T peek() {
      return top == null ? null : top.getData();
  }

Sorting

Your algorithm looks sane, but the interface is ... cumbersome. You have a stack, and auxilliaryStack declared as members of the Solution class. Now, the rules of object orientation indicate that the data should be encapsulated. There are two logical ways to implement the sort that I can see. The one is to have a static method that takes a Stack as input, and sorts it in place (creating a temporary, and method-private auxStack to sort with). The other solution adds the sort method to the Stack class itself (and also has an internal auxStack). The second route is the better one.

public void sort() {
    if (isEmpty()) {
        return;
    }
    Stack<T> auxStack = new Stack<>();
    .....
}

I recommend you make the changes and see how it works out for you. You will find that the logic and flow of the structure is neater, and more manageable.


Solving Fizz Buzz using LINQ in C# The Fizz Buzz challenge is well-known and should not need any explanations, right?
I recently got a bit bored and came up with a solution for Fizz Buzz using LINQ and even asynchronous code and a lot of extension methods. Actually, it's only Extension methods!
So, I thought, let's share it and have a review of my code!

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

namespace FizzBuzzApp
{
    using IntEnum = IEnumerable<int>;
    using Pair = KeyValuePair<int, string>;
    using PairEnum = IEnumerable<KeyValuePair<int, string>>;
    using StringEnum = IEnumerable<string>;

    public static class FizzBuzzClass
    {
        public static T DoThis<T>(this T data, Action<T> action) { action(data); return data; }
        public static T DoThis<T>(this T data, Action action) { action(); return data; }
        public static IEnumerable<T> DoThis<T>(this IEnumerable<T> data, Action<T> action) { foreach (var item in data) { action(item); yield return item; } }

        public static T IfThis<T>(this T data, Func<T, bool> condition, Func<T, T> action) => condition(data) ? action(data) : data;
        public static T IfThis<T>(this T data, Func<T, bool> condition, Func<T, T> onTrue, Func<T, T> onFalse) => condition(data) ? onTrue(data) : onFalse(data);

        public static IntEnum MakeIntArray(this int count, int start = 0) => Enumerable.Range(start, count);

        public static Pair NewPair(this int key) => new Pair(key, string.Empty);
        public static Pair NewPair(this Pair pair, string data) => new Pair(pair.Key, data.Trim());
        public static PairEnum MakePairs(this IntEnum data, bool asynchronous) => asynchronous ? data.AsParallel().Select(NewPair) : data.Select(NewPair);
        public static IOrderedEnumerable<Pair> SortPairs(this IEnumerable<Pair> data) => data.OrderBy(p => p.Key);

        public static bool IsFizz(this Pair data) => data.Key % 3 == 0;
        public static Pair DoFizz(this Pair data) => data.NewPair("Fizz " + data.Value);
        public static Pair Fizz(this Pair data) => data.IfThis(IsFizz, DoFizz);
        public static PairEnum Fizz(this PairEnum data, bool asynchronous) => asynchronous ? data.AsParallel().Select(Fizz) : data.Select(Fizz);

        public static bool IsBuzz(this Pair data) => data.Key % 5 == 0;
        public static Pair DoBuzz(this Pair data) => data.NewPair(data.Value + " Buzz");
        public static Pair Buzz(this Pair data) => data.IfThis(IsBuzz, DoBuzz);
        public static PairEnum Buzz(this PairEnum data, bool asynchronous) => asynchronous ? data.AsParallel().Select(Buzz) : data.Select(Buzz);

        public static bool IsNumber(this Pair data) => string.IsNullOrEmpty(data.Value);
        public static Pair DoNumber(this Pair data) => data.NewPair(data.Key.ToString());
        public static Pair Number(this Pair data) => data.IfThis(IsNumber, DoNumber);
        public static PairEnum Number(this PairEnum data, bool asynchronous) => asynchronous ? data.AsParallel().Select(Number) : data.Select(Number);

        public static StringEnum Values(this PairEnum data) => data.Select(p => p.Value);
        public static string Combine(this StringEnum data) => string.Join(", ", data);

        public static string FizzBuzz(this int count, bool asynchronous = false) =>
            count.MakeIntArray()
                 .MakePairs(asynchronous)
                 .Fizz(asynchronous)
                 .Buzz(asynchronous)
                 .Number(asynchronous)
                 .SortPairs()
                 .Values()
                 .Combine();
}

To use it, just use 100.FizzBuzz(true); for the asynchronous version.
Now, the thing is that it's full one-liner extension methods. It's definitely not something a beginner would write. But is it well-written for experts?

Parallel computing and asynchonous computing are two separate things. While they might be similar in some cases, the two terms have two very distinct meanings in C#-land:

Asynchronous computing is used for the async/await and Task<T> idioms.
Parallel computing is what you are doing here, using plinq: processing streams of data in different threads in parallel.

With that said, you're not using the features of Plinq to the fullest. Instead of passing the (incorrectly named) asynchronous to each of the methods, count.MakeIntArray().AsParallel() would have sufficed to make the entire thing parallel.

I think you went a bit overboard with the amount of extension methods you created, while gaining little for it in return. What if a new request would come to say something new on every number modulo 10? Now you have to make changes in 4 methods (or more accurately, create 4 more methods) to account for these changes!

Instead you could have made the Fizz and Buzz methods two versions of a more generic Substitute method:

public string Substitute(int number, Func<int, bool> when, string with);


A service that returns paged Products with optional filtering In order to better understand what the code does, I've captured this screencast.

Basically, I have a products page and a user can filter products.

CODE

Here is the Page action method on my ProductController:

public ActionResult Page(SearchViewModel search, int page = 1)
{
  var viewModel = _productService.GetPagedProducts(search, page);
  PopulateDropDownSelectListsForSearchVm(viewModel.Search);
  return View(viewModel);
}

That PopulateDropDownSelectListsForSearchVm is a helper method that sits within the controller.

Here is the implementation of the GetPagedProducts method on the IProductService interface:

public ProductPageViewModel GetPagedProducts(SearchViewModel search, int page = 1)
{
  int totalNumberOfProducts;
  if (HaveSearchTermsChanged(search)) page = 1;
  var products = _context.Products.Where(p => p.IsDeleted == search.ShowDeleted).AsQueryable();
  if (search.CategoryId != 0) products = products.Where(p => p.CategoryId == search.CategoryId);
  if (search.BrandId != 0) products = products.Where(p => p.BrandId == search.BrandId);
  if (search.QualityId != 0) products = products.Where(p => p.QualityId == search.QualityId);

  if (!string.IsNullOrEmpty(search.SearchTerm))
  {
    products = products
                .Where(p => p.Name == search.SearchTerm.Trim())
                .OrderBy(p => p.Name);
    totalNumberOfProducts = products.Count();
    products = products.Skip(_recordsPerPage * (page - 1)).Take(_recordsPerPage);
  }
  else
  {
    products = products.OrderBy(p => p.Name);
    totalNumberOfProducts = products.Count();
    products = products.Skip(_recordsPerPage * (page - 1)).Take(_recordsPerPage);
  }

  var productPageVm = new ProductPageViewModel
  {
    Products = ProductViewModelFactory.BuildListOfProductViewModels(products.ToList()),
    Pagination = new PaginationViewModel
    {
      CurrentPage = page,
      RecordsPerPage = _recordsPerPage,
      TotalRecords = totalNumberOfProducts
    },
    Search = search
  };

  TrackCurrentSearchTerm(productPageVm);
  return productPageVm;
}

Readability is really important to me, and right now it just doesn't feel readable. What can I do to improve the overall code, and have it be very readable?

I would suggest you to move the pagination settings to your SearchModel

Then you can have an utility/extension method or whatever you whish with this signature

public IQueryable<Product> ApplyFilter(IQueryable<Product> productQuery, SearchModel model)

and you write there ll the filtering business, so you can re use it in more methods and simplify the readability of your main method.

Finally, I would not recommend to rewrite your if statements like this

if (search.CategoryId != 0)
{
    products = products.Where(p => p.CategoryId == search.CategoryId);
}


Python program for converting scientific notation to fixed point I had to replace all instances of scientific notation with fixed point, and wrote a Python script to help out.  Here it is:

#!/usr/bin/python2

# Originally written by Anton Golov on 29 Jan 2012.
# Feedback can be sent to <email> .

"""Provide functions for converting scientific notation to fixed point.

When imported, reads lines from stdin and prints them back to stdout with
scientific notation removed.

WARNING:  The resulting fixed point values may be of a different precision
than the original values.  This will usually be higher, but may be lower for
large numbers.  Please don't rely on the precision of the values generated by
this script for error analysis.

"""

from re import compile

exp_regex = compile(r"(\d+(\.\d+)?)[Ee](\+|-)(\d+)")

def to_fixed_point(match):
    """Return the fixed point form of the matched number.

    Parameters:
        match is a MatchObject that matches exp_regex or similar.

    If you wish to make match using your own regex, keep the following in mind:
        group 1 should be the coefficient
        group 3 should be the sign
        group 4 should be the exponent

    """
    sign = -1 if match.group(3) == "-" else 1
    coefficient = float(match.group(1))
    exponent = sign * float(match.group(4))
    return "%.16f" % (coefficient * 10**exponent)

def main():
    """Read lines from stdin and print them with scientific notation removed.

    """
    try:
        while True:
            line = raw_input('')
            print exp_regex.sub(to_fixed_point, line)
    except EOFError:
        pass

if __name__ == "__main__":
    main()

I am primarily interested about the way the regex is used (can it match something it can't replace?), but comments on code clarity and features that may be simple to add are also welcome, as well as any further nitpicking .

from re import compile

exp_regex = compile(r"(\d+(\.\d+)?)[Ee](\+|-)(\d+)")

Python style guide recommends that global constants be in ALL_CAPS. And what about numbers like "1e99"? They won't match this regex.

def to_fixed_point(match):
    """Return the fixed point form of the matched number.

    Parameters:
        match is a MatchObject that matches exp_regex or similar.

    If you wish to make match using your own regex, keep the following in mind:
        group 1 should be the coefficient
        group 3 should be the sign
        group 4 should be the exponent

    """

I'd suggest breaking the groups out of the match into local variables

coefficient, decimals, sign, exponent = match.groups()

And then use those names instead of match.group(1). That way your code will be easier to follow.

    sign = -1 if match.group(3) == "-" else 1
    coefficient = float(match.group(1))
    exponent = sign * float(match.group(4))
    return "%.16f" % (coefficient * 10**exponent)

Here's the thing. Python already support scientific notation, so you should just ask python to do conversion for you:

 value = float(match.group(0))

Less bugs, more efficient, better all around.

def main():
    """Read lines from stdin and print them with scientific notation removed.

    """
    try:
        while True:
            line = raw_input('')
            print exp_regex.sub(to_fixed_point, line)
    except EOFError:
        pass

I'd suggest using for line in sys.stdin:. It'll automatically get you the lines, stop when the file ends. That'll make this piece of code simpler.

if __name__ == "__main__":
    main()


Function measuring time of another function I wanted to have a function called timeit which can measure the time of execution of n iterations of any other function. I still learning C++ but I come up with that : Any thought ?
#include<chrono>

using namespace std;

template<typename output, typename ...ArgsT, typename ...Input>
double timeit(output (*function)(Input...), int n, ArgsT ...args)
{
   auto start = chrono::high_resolution_clock::now();
   for(int k = 0; k < n; ++k)
   {
      function(args...);
   }
   auto end = chrono::high_resolution_clock::now();
   chrono::duration<double> elapsed = (end-start);
   return elapsed.count();
}

The big thing you're missing here is perfect forwarding. When you receive the argument pack, it may contain one or more rvalue references. But when you pass args... to the function you're timing, you've given a name to the pack, so even though what you received were rvalue references, what you end up passing to the timed function won't be an rvalue reference any more.
To fix this, you'd use std::forward:
function(std::forward<ArgsT>(args)...);

The other obvious addition I'd consider would be gathering and returning the return values from the calls you make. One possibility would be to return something like a pair<double, std::vector<output>>, to hold the time and the return values.


Algebraic notation with RegEx I've written an algebraic notation to use for moves in a game and I'm currently writing code to parse the information from the notation.  I'm doing this using regExes.  This is coded in HaXe.

For example, two functions in the parser would look like this:

public function getMoveType(gameText:String):String
{
   var moveTypeRegEx:EReg = ~/MatchThis/;
   moveTypeRegEx.match(gameText);
   var moveType:String = moveTypeRegEx.matched(0); //this returns entire string that was matched.
   return moveType;
}

public function getPlayerName(gameText:String):String
{
    var playerNameRegEx:EReg = ~/MatchThis/;
    playerNameRegEx.match(gameText);
    var playerName:String = playerNameRegEx.matched(0);
    return playerName;
}

How could this be written better to make it more readable or more organized?
Would it be better to have a class file GameRegExes, which contains all the regexes I need with getter functions to those regexes?

Please let me know if I'm being too vague.

The use of regular expressions is an implementation detail.

I think you should continue to structure your API as functions that take a higher level input, such as a game text, and destructure it into the end product that your API's clients want.

That said, you can define a private helper that takes a regular expression and does the work of matching the text and extracting group 0.  Then each of your public functions would delegate most of the work to that just providing it with the appropriate regular expression and maybe the index of the group to return.

private function gameTextExtractor(gameText:String, regex:EReg):String
{
    regex.match(gameText);
    return regex.matched(0);
}


Multiple if then return with conditions Is there a more elegant way of doing this? I really think this looks ugly.

private static string InsertMethodNameHere( bool hasCondition1, bool hasCondition, bool hasCondition3)
        {
            if (!hasCondition1 && !hasCondition2 && !hasCondition3)
                return "0";

            if (hasCondition1 && !hasCondition2 && !hasCondition3)
                return "1";

            if (!hasCondition1 && hasCondition2 && !hasCondition3)
                return "2";

            if (hasCondition1 && hasCondition2 && !hasCondition3)
                return "3";

            if (!hasCondition1 && !hasCondition2 && hasCondition3)
                return "4";

            if (hasCondition1 && !hasCondition2 && hasCondition3)
                return "5";

            if (!hasCondition1 && hasCondition2 && hasCondition3)
                return "6";

            if (hasCondition1 && hasCondition2 && hasCondition3)
                return "7";

            throw new Exception("Unable to determine.");
}

How about this?

private static string InsertMethodNameHere(bool hasCondition1, bool hasCondition2, bool hasCondition3)
{
    return ((hasCondition1 ? 1 : 0) +
            (hasCondition2 ? 2 : 0) +
            (hasCondition3 ? 4 : 0)).ToString(CultureInfo.InvariantCulture);
}

If you want a more general solution, you could write an extension method that converts a list of bools to an int:

public static int ToInt(this IEnumerable<bool> bools)
{
    return bools.Select((t, i) => (t ? 1 : 0) << i).Sum();
}


Python qwerty keyboard checker I am creating a code which checks a user inputted password and gives points based on it. One of the points is: If it has 3 letters adjacent/consecutive on the QWERTY UK keyboard then it will be -5 points. e.g "qwerty" will be -20 points for "qwe" "wer "ert" "rty". I have got a solution to this but I was wondering if there is any way of improving it.

input = input("What is your password?")
qwerty = 'qwertyuiopasdfghjklzxcvbnm'
lower = input.lower()
for idx in range(0, len(lower) - 2):
    test_seq = lower[idx:idx + 3]
    if test_seq in qwerty:
        points -= 5
print(points)

According to my interpretation, your solution does not actually meet the specification:

  If it has 3 letters adjacent/consecutive on the QWERTY UK keyboard then it will be -5 points

For example, pas does not have three adjacent keys on a QWERTY keyboard, yet it gets penalized by your code.  On the other hand, your code fails to detect backwards adjacent sequences such as ewq.


Generating and printing chess board in Rust This is the beginning of a chess program I'm writing in Rust.  I'm new to Rust, so while this program works, I'm sure there are many improvements that could be made.  For instance, I wanted to declare the squares variable as a fixed sized array of 64 square structs, rather than using a vector, but I couldn't get that to work, I think because I'm using a trait object.

First is the code within lib.rs.

mod piece;

use std::fmt;
use piece::*;

struct Square {
    piece: Option<Box<Piece>>
}

impl Square {
    pub fn new() -> Square {
        Square { piece: None }
    }

    fn symbol(&self) -> &str {
        match self.piece {
            Some(ref p) => p.symbol(),
            None => "."
        }
    }
}

impl fmt::Display for Square {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "{}", self.symbol())
    }
}

pub struct Board {
    squares: Vec<Square>
}

impl Board {
    pub fn new() -> Board {
        let mut squares: Vec<Square> = Vec::new();
        let p = vec![(0, 1), (7, 6)];

        for _ in 0..64 { squares.push(Square::new()); }

        for pos in p {
            squares[pos.0 * 8 + 0].piece = Some(Box::new(Rook {}));
            squares[pos.0 * 8 + 1].piece = Some(Box::new(Knight {}));
            squares[pos.0 * 8 + 2].piece = Some(Box::new(Bishop {}));
            squares[pos.0 * 8 + 3].piece = Some(Box::new(Queen {}));
            squares[pos.0 * 8 + 4].piece = Some(Box::new(King {}));
            squares[pos.0 * 8 + 5].piece = Some(Box::new(Bishop {}));
            squares[pos.0 * 8 + 6].piece = Some(Box::new(Knight {}));
            squares[pos.0 * 8 + 7].piece = Some(Box::new(Rook {}));

            for i in (pos.1 * 8)..(pos.1 * 8 + 8) {
                squares[i].piece = Some(Box::new(Pawn { en_passant: false }))
            }
        }

        Board {
            squares: squares
        }
    }

    pub fn valid_moves() {

    }
}

impl fmt::Display for Board {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        let mut res = String::new();
        for i in 0..8 {
            for j in 0..8 {
                res.push_str(self.squares[(i * 8) + j].symbol());
            }
            res.push_str("\n");
        }
        write!(f, "{}", res)
    }
}

Here is the code that defines the pieces (not much to this yet).

pub trait Piece {
    fn symbol(&self) -> &str;
    fn valid_moves(&self);
}

pub struct King;
impl Piece for King {
    fn symbol(&self) -> &str {
        "K"
    }

    fn valid_moves(&self) {

    }
}

pub struct Queen;
impl Piece for Queen {
    fn symbol(&self) -> &str {
        "Q"
    }

    fn valid_moves(&self) {

    }
}

pub struct Rook;
impl Piece for Rook {
    fn symbol(&self) -> &str {
        "R"
    }

    fn valid_moves(&self) {

    }
}

pub struct Bishop;
impl Piece for Bishop {
    fn symbol(&self) -> &str {
        "B"
    }

    fn valid_moves(&self) {

    }
}

pub struct Knight;
impl Piece for Knight {
    fn symbol(&self) -> &str {
        "k"
    }

    fn valid_moves(&self) {

    }
}

pub struct Pawn { pub en_passant: bool }
impl Piece for Pawn {
    fn symbol(&self) -> &str {
        "p"
    }

    fn valid_moves(&self) {

    }
}

I'd love to know if there are ways I could improve on this.

You can define your pieces as an enum, then you don't need to box them in your squares:

#[derive(Clone, Copy)]
pub enum Piece {
    King,
    Queen,
    Rook,
    Bishop,
    Knight,
    Pawn
}

Notice that we derive Clone and Copy; this is so that we can put them in our board array later.  You now define your symbols here. Notice how they're all in one nice place now?

#[derive(Clone, Copy)]
struct Square {
    piece: Option<Piece>
}

impl Square {
    pub fn new() -> Square {
        Square { piece: None }
    }

    fn symbol(&self) -> &str {
        match self.piece {
            Some(Piece::King)   => "K",
            Some(Piece::Queen)  => "Q",
            Some(Piece::Rook)   => "R",
            Some(Piece::Bishop) => "B",
            Some(Piece::Knight) => "k",
            Some(Piece::Pawn)   => "p",
            None                => "."
        }
    }
}

impl fmt::Display for Square {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "{}", self.symbol())
    }
}

You have also eliminated the need to have any Boxes, which could have some annoying unwrapping later.  It also eliminated some runtime errors and helps the compiler make sure your code is correct.

You can define your board using a 2D array like so:

pub struct Board {
    squares: [[Square; 8]; 8]
}

impl Board {
    pub fn new() -> Board {
        let mut squares = [[Square { piece: None }; 8]; 8];
        // ...
    }
}

Without seeing what you tried, I can't know why it didn't work before, it could be because you didn't derive Clone and Copy for your squares and pieces.  I have opted to go for a 2D array, as I feel it is easier to reason about than a single 1D array.  It also means you don't need to have lookups like myArray[x * width + y] everywhere which is just messy, I find myArray[x][y] is preferable.  I won't show it, but I updated your fmt::Display for Board code to match.

Finally your board creation.  Look upon the niceness of the enum, and rejoice!  Also see how nice the 2D array is, and pattern matching.  I really love Rust.

pub fn new() -> Board {
    let mut squares = [[Square { piece: None }; 8]; 8];

    for x in 0..8 {
        squares[x][1].piece = Some(Piece::Pawn);
        squares[x][6].piece = Some(Piece::Pawn);
        let piece = match x {
            0 | 7 => Some(Piece::Rook),
            1 | 6 => Some(Piece::Knight),
            2 | 5 => Some(Piece::Bishop),
            3 => Some(Piece::King),
            4 => Some(Piece::Queen),
            _ => unreachable!()
        };
        squares[x][0].piece = piece;
        squares[x][7].piece = piece;
    }

    Board {
        squares: squares
    }
}

It might feel as if I've really torn your code apart here, but it's not surprising given how different Rust actually is compared to what most people would call its closest competitor, C++.  Implementing Clone and Copy is very easy to miss and very important if you don't want to have a nightmare of boxed instances later.  Using the enum style I have here does mean that functionality for things like finding valid moves will be the job of the board, rather than the pieces, but you already started down that track.  Don't forget as well that Rusts "thick" enums mean that you can still attach a moveset to your enumerated type, if that ends up being the easiest way to implement it.


Refactoring my windowsFormApp code I have around 150 lines of code that I managed to optimize.

But, because my two methods and the six if-statements almost are identical, could there be room for improvement?  Maybe it could be done with only one button and one listbox?

As I'm out of ideas and could learn more myself, I'd like to know what could be done to improve this code.

public Form1()
    {
      InitializeComponent();
      timer1.Interval = 100;
      timer1.Start();
    }

    private void timer1_Tick(object sender, EventArgs e)
    {
      lblClock.Text = DateTime.Now.ToString("HH:mm:ss");
    }
        // ADD values BUTTON
    private void button8_Click(object sender, EventArgs e)
    {
        AddValues();
    }

        // REMOVE values BUTTON
    private void button7_Click(object sender, EventArgs e)
    {
        RemoveValues();
    }

    private void AddValues()
    {
            // ADD hours
        if (!box_Hour.Items.Contains(DateTime.Now.Hour))
        {
            box_Hour.Items.Add(DateTime.Now.Hour);
        }
            // ADD minutes
        if (!box_Minute.Items.Contains(DateTime.Now.Minute))
        {
            box_Minute.Items.Add(DateTime.Now.Minute);
        }
            // ADD seconds
        if (!box_Second.Items.Contains(DateTime.Now.Second))
        {
            box_Second.Items.Add(DateTime.Now.Second);
        }

    }

    private void RemoveValues()
    {
            // REMOVE hours
        if (box_Hour.Items.Contains(DateTime.Now.Hour))
        {
            box_Hour.Items.Remove(DateTime.Now.Hour);
        }
            // REMOVE minutes
        if (box_Minute.Items.Contains(DateTime.Now.Minute))
        {
            box_Minute.Items.Remove(DateTime.Now.Minute);
        }
            // REMOVE seconds
        if (box_Second.Items.Contains(DateTime.Now.Second))
        {
            box_Second.Items.Remove(DateTime.Now.Second);
        }
    }

Yeah, you can simplify your code. To have a single method for both of them, you would need to specify whether the check was negated and what action to perform (that could be done using a delegate).

To simplify it even more, use a loop over the three boxes (possibly using a Dictionary to connect each box to the date part).

Also, when working with DateTime.Now, you should make sure to access that property only once, then use its value. If you don't, you could get inconsistent results if the time changes while your method is executing.

private void AddValues()
{
    ProcessValues(true, (items, value) => items.Add(value));
}

private void RemoveValues()
{
    ProcessValues(false, (items, value) => items.Remove(value));
}

private void ProcessValues(
    bool negateContains, Action<ListBox.ObjectCollection, int> successAction)
{
    var now = DateTime.Now;

    var pairs = new Dictionary<ListBox, int>
    {
        { box_Hour, now.Hour },
        { box_Minute, now.Minute },
        { box_Second, now.Second }
    };

    foreach (var pair in pairs)
    {
        bool condition = pair.Key.Items.Contains(pair.Value);

        if (negateContains)
            condition = !condition;

        if (condition)
            successAction(pair.Key.Items, pair.Value);
    }
}

This makes your code less repetitive, but also more complicated. It's up to you to decide if that's worth it.


Returning a list of divisors for a number This function takes in a number and returns all divisors for that number.  list_to_number() is a function used to retrieve a list of prime numbers up to a limit, but I am not concerned over the code of that function right now, only this divisor code.  I am planning on reusing it when solving various Project Euler problems.

def list_divisors(num):
    ''' Creates a list of all divisors of num
    '''
    orig_num = num
    prime_list = list_to_number(int(num / 2) + 1)
    divisors = [1, num]
    for i in prime_list:
        num = orig_num
        while not num % i:
            divisors.append(i)
            num = int(num / i)
    for i in range(len(divisors) - 2):
            for j in range(i + 1, len(divisors) - 1):
                if i and j and j != num and not orig_num % (i * j):
                    divisors.append(i * j)
    divisors = list(set(divisors))
    divisors.sort()
    return divisors

Don't reset num to orig_num (to fasten division/modulo)
Think functionnaly :

From your prime factors, if you generate a new collection with all possible combinations, no need to check your products.
It makes harder to introduce bugs. Your code is indeed broken and won't output 20 or 25 (...) as divisors of 100.
You can re-use existing (iter)tools.

Avoid unnecessary convertions (sorted can take any iterable)
Compute primes as you need them (eg, for 10**6, only 2 and 5 will suffice). Ie, use a generator.

This leads to :

from prime_sieve import gen_primes
from itertools import combinations,  chain
import operator

def prod(l):
   return reduce(operator.mul, l, 1)

def powerset(lst):
    "powerset([1,2,3]) --> () (1,) (2,) (3,) (1,2) (1,3) (2,3) (1,2,3)"
    return chain.from_iterable(combinations(lst, r) for r in range(len(lst)+1))

def list_divisors(num):
    ''' Creates a list of all divisors of num
    '''
    primes = gen_primes()
    prime_divisors = []
    while num>1:
       p = primes.next()
       while not num % p:
          prime_divisors.append(p)
          num = int(num / p)

    return sorted(set(prod(fs) for fs in powerset(prime_divisors)))

Now, the "factor & multiplicity" approach like in suggested link is really more efficient.
Here is my take on it :

from prime_sieve import gen_primes
from itertools import product
from collections import Counter
import operator

def prime_factors(num):
   """Get prime divisors with multiplicity"""

   pf = Counter()
   primes = gen_primes()
   while num>1:
      p = primes.next()
      m = 0
      while m == 0 :
         d,m = divmod(num,p)
         if m == 0 :
            pf[p] += 1
            num = d
   return pf

def prod(l):
   return reduce(operator.mul, l, 1)

def powered(factors, powers):
   return prod(f**p for (f,p) in zip(factors, powers))

def divisors(num) :

   pf = prime_factors(num)
   primes = pf.keys()
   #For each prime, possible exponents
   exponents = [range(i+1) for i in pf.values()]
   return sorted([powered(primes,es) for es in product(*exponents)])


Node2D and Node3D classes implemented using a shared decorator EDIT: I have made another go at this using metaclasses here. I think it's a much better approach.

I have the following classes. validator is a decorator that receives a class which defines validation criteria for a decorated function. ValidateKeys is the validation criteria for this example (other criteria I plan to include later are maximum, minimum, and rounding/decimal places). Node is a parent class with a decorated setter, and Node2D and Node3D are child classes differing only in the validation criteria each uses.

I'm wondering if this is a good solution, if there are problems with doing it this way (i.e., inheritance) that I'm not seeing, etc etc. Any criticism is welcome.

One thing that I don't like, and doesn't seem to make sense, is that I had to move the list of valid keys from the validator to the parent class itself (see old code version below). This doesn't really make sense to me because I'd like to separate the valid list from the implementation of the Node class [the Node class is just and example; I will transform it into something more general, for making Node (valid members: x,y), Element (valid members: i,j,k,l), Boundary (valid member: b) etc etc].

class validator(object):
    def __init__(self, TheValidator, *args, **kwargs):
        print("New validator Object")
        self.TheValidator = TheValidator(*args,**kwargs)
    def __call__(self,f):
        def wrapped_f(instance, *args,**kwargs):
            self.TheValidator(instance, *args, **kwargs)
            return f(instance,*args,**kwargs)
        return wrapped_f

class ValidateKeys(object):
    def __init__(self,*keysIterable):
        print("New ValidateKeys Object")
    def __call__(self, instance, **kwargs):
        for a in kwargs:
            if not a in instance.valid_keys:
                raise Exception()
        instance.__dict__.update(kwargs)

class Node(object):
    def __new__(klass, *valid_keys):
        Node.valid_keys = valid_keys
        return super(Node, klass).__new__(klass)
    @property
    def coords(self):
        return self.__dict__
    @coords.setter
    def coords(self,Coords):
        self.set_coords(**Coords)
    @validator(ValidateKeys)
    def set_coords(self,**Coords):
        pass

class Node3D(Node):
    def __new__(klass):
        return super(Node3D, klass).__new__(klass, 'x','y','z')

class Node2D(Node):
    def __new__(klass):
        return super(Node2D, klass).__new__(klass, 'x','y')

In case it helps make clear what I'm trying to accomplish, an older version of my solution is below prior to creating the Node parent class. I added the parent class to cut down on duplicate code, but to do that I had to refactor a lot, including moving valid_keys from the ValidateKeys class to the Node class.

class validator(object):
    def __init__(self, TheValidator, *args, **kwargs):
        self.TheValidator = TheValidator(*args,**kwargs)
    def __call__(self,f):
        def wrapped_f(instance, *args,**kwargs):
            self.TheValidator(instance, *args, **kwargs)
            return f(instance,*args,**kwargs)
        return wrapped_f

class ValidateKeys(object):
    def __init__(self,*keysIterable):
        self.valid_keys = keysIterable
    def __call__(self, instance, **kwargs):
        for a in kwargs:
            if not a in self.valid_keys:
                raise Exception()
        instance.__dict__.update(kwargs)

class Node3D(object):
    @property
    def coords(self):
        return self.__dict__
    @coords.setter
    def coords(self,Coords):
        self.set_coords(**Coords)
    @Validator(ValidateKeys, 'x','y','z')
    def set_coords(self,**Coords):
        pass

class Node2D(object):
    @property
    def coords(self):
        return self.__dict__
    @coords.setter
    def coords(self,Coords):
        self.set_coords(**Coords)
    @Validator(ValidateKeys, 'x','y')
    def set_coords(self,**Coords):
        pass

One thing that I don't like, and doesn't seem to make sense, is that I had to move the list of valid keys from the validator to the parent class itself

That's not only something to dislike -- it is actually broken. If you do this...

n3 = Node3D()
n2 = Node2D()
n3.coords = {'x':1,'y':2,'z':3}

...you will get Exception because the second line has caused Node.valid_keys to be overwritten. To fix that, you could eliminate all three __new__ methods and instead define the subclasses like this:

class Node3D(Node):
    valid_keys ='x','y','z'

class Node2D(Node):
    valid_keys ='x','y'

However, the whole approach is not very good because one can still do for example this:

>>> node = Node2D()
>>> node.coords = {'y': 2, 'x': 1}
>>> node.z = 3
>>> node.coords
{'y': 2, 'x': 1, 'z': 3}

Using a namedtuple as discussed in comments is a better idea.


Traversing a Binary Search Tree in Swift I'm implementing the Binary Search Tree data structure in Swift. It looks like this:

class BinarySearchTree<Key: Comparable, Value> {

    let key: Key
    var value: Value
    var left, right: BinarySearchTree<Key, Value>?

    init(key: Key, value: Value) {
        self.key = key
        self.value = value
    }

    // irrelevant methods for constructing a tree

}

To be able to traverse through it using for (key, value) in myTree { }, BinarySearchTree has to implement SequenceType. This is my first attempt:

extension BinarySearchTree: SequenceType {

    func generate() -> AnyGenerator<(Key, Value)> {
        let leftGenerator = left?.generate()
        let rightGenerator = right?.generate()

        let (key, value) = (self.key, self.value)
        var generatedSelf = false

        return AnyGenerator {
            if generatedSelf { return rightGenerator?.next() }
            else if let next = leftGenerator?.next() { return next }
            else {
                generatedSelf = true
                return (key, value)
            }
        }
    }

}

It works fine, but when I run benchmarks, it's just not as fast as I'd like. Can my traversal algorithm be improved, or is this the fastest way to do it?

Your generate() method works correctly, as far as I can see. But it
creates an AnyGenerator for every node in the tree, i.e. it uses
more memory than necessary. Each call to the next() method of the generator
then traverses down the next() methods along the left or right subtrees,
which makes it slow.

An iterative approach using a stack instead of "nested generators"
seems to be faster. Here is a possible implementation of
the iterativeInorder algorithm described in Wikipedia: Tree traversal:

func generate() -> AnyGenerator<(Key, Value)> {

    var node : BinarySearchTree? = self
    var stack : [BinarySearchTree] = []

    return AnyGenerator {
        // Traverse from current node to the far left, pushing all
        // nodes onto the stack:
        while let node1 = node {
            stack.append(node1)
            node = node1.left
        }
        if !stack.isEmpty {
            // Pop node from stack and return its (key, value),
            // then continue with right subtree.
            let current = stack.removeLast()
            defer { node = current.right }
            return (current.key, current.value)
        } else {
            return nil
        }
    }
}

Note that the size of the stack array is limited by the tree height.

In my test with a tree of 1,000,000 nodes, the traversal is done
in about 0.2 seconds, compared to 3.5 seconds with your original
method (test done in Release mode on a MacBook).


Target src/main.rs::main()? I'm basically wondering what the optimal method for returning to the top of src/main.rs::main() is?

Maybe something like this:

mod main;

pub fn main() {
    crate::main();
}

Or maybe there's a keyword built in?

I want to handle certain kinds of errors by 'restarting the program'.

There is a lot of way, I advice the simple one, use a loop:

enum Error {
    A,
}

fn run() -> Result<(), Error> {
    if rand::random() {
        Ok(())
    } else {
        Err(Error::A)
    }
}

pub fn main() {
    while let Err(_) = run() {
        println!("Hello");
    }
}

However, be sure the program end maybe add a maximum fail counter.


Generating N Prime Numbers I'm trying to generate N prime numbers where my N = 200,000.

The method that I think I wrote is -

  Start with first prime of 2
  Increment possible prime number by 1
  See if possible prime is divisible by any found prime (if yes exit to 2)
  Stop testing when next found prime is more than half of the possible prime (exit to 2)
  Add it to the list

It works, but it is taking 1 minute 8 seconds which seems really long. Is my algorithm not doing what I think or is it just implemented incorrectly? Or should I not be using these arrays?

Option Explicit On
Option Strict On
Option Infer On
Option Compare Text
Module PrimeNumberGenerator
     Sub Main()
        Static start_time As DateTime
        Static end_time As DateTime
        Dim elapsed_time As TimeSpan
        start_time = DateTime.Now
        Dim targetPrimes(199999) As Integer
        targetPrimes = GetPrimes(targetPrimes.Length)
        end_time = DateTime.Now
        elapsed_time = end_time - start_time
        Debug.Print(elapsed_time.ToString)
        Debug.Print((targetPrimes(500)).ToString)
    End Sub

    Private Function GetPrimes(ByVal countOfPrimes As Integer) As Integer()
        Dim myPrimes(countOfPrimes - 1) As Integer
        Dim primeIndex As Integer = 0
        Dim testIndex As Integer = 0
        Dim possiblePrime As Integer = 2
        myPrimes(primeIndex) = possiblePrime
        Do Until primeIndex = countOfPrimes - 1
            possiblePrime += 1
            For testIndex = 0 To primeIndex
                If possiblePrime Mod myPrimes(testIndex) = 0 Then
                    Continue Do
                ElseIf myPrimes(testIndex + 1) * 2 > possiblePrime Then
                    Exit For
                End If
            Next
            primeIndex += 1
            myPrimes(primeIndex) = possiblePrime
        Loop
        Return myPrimes
    End Function
End Module

One speed tweak is to search to only the square root of the 'possiblePrime'.  This will reduce computation by a significant order of magnitude.

Another tweak is to add 2, instead of 1 to 'possiblePrime' as you know that all even numbers > 2 are not prime.


One hash with params populate two objects I have a JSON API where a user can update a post and related details to that post. To simplify the process the user just fills in one hash and we don't expose our architecture. What I did here works but I would like to know if their is a better way. This smells to me, but I don't know why...

def update
    params['blog_post'] = params.delete 'post'
    @blog_post.update_attributes blog_post_params
    update_blog_post_groups if params['blog_post']['group_ids']
    params['blog_post_details'] = params.delete 'blog_post'
    @blog_post.blog_post_details.update_attributes blog_post_details_params
    render :blog_post, status: 200
end

blog_post_params and blog_post_details_params are strong_parameter methods.

It seems that I should use first : accepts_nested_attributes_for

This could be refactored into a service object that does the manipulations. Your controller would still manipulate the Hash, but it would provide the service object the hash values it needs:

class BlogPostUpdateService
  def initialize(blog_post_params, blog_post_details_params)
    @blog_post_params = blog_post_params
    @blog_post_details_params = blog_post_details_params
  end

  def execute(blog_post)
    blog_post.update_attributes @blog_post_params
    blog_post.blog_post_details.update_attributes @blog_post_details_params
    update_blog_post_groups blog_post if @blog_post_params['group_ids']
    blog_post
  end

private
  def update_blog_post_groups(blog_post)
    # ...
  end
end

All of the logic for updating the blog post would be contained within the service object. Abstracting your internal architecture should be handled by the controller, which then delegates to the service for the actual modifications:

def update
  # ... abstract away your internal schema
  update_service = BlogPostUpdateService.new(blog_post_params, blog_post_details_params)
  update_service.execute @blog_post
  render :blog_post, status: 200
end

I'm also not clear why you are deleting 'post' and saving at as 'blog_post', but then later deleting 'blog_post' and saving it as 'blog_post_details'. This seems like you are jumping through a lot of hoops and I'm not sure what benefit it has, other than being really confusing.


Initialize an object with a key at runtime Often I want to construct an object with a runtime-generated key.

Here's how I have been doing it:

function make_obj(key, func, val){
  let foo={};
  foo[key] = func(val);
  return foo;
}

Usage:

make_obj('food', x=>`I like ${x}`, 'soup'); // {food: 'I like soup'}

Is there a shorter way to do this?

ECMAScript 2015 adds a new feature called computed property names.
It allows you to create a property in object literal without specifying the name for the key -- it lets you use a value of a variable.

const keyName = 'foo'
const object = { [keyName]: 21 } // { foo: 21 }

You can read about a bunch of similar things on 2ality.

In your example, that would be:

function make_obj (key, func, val) {
  return { [key]: func(val) };
}


Logger module in child processes from os.fork() I have had a hard time understanding all the documentation on the logger module and forking processes.

My goal is to have each child process that's worked to write to their own separate log file.

This seems to work but I just want to try and verify that this is the correct way.

import os
import logging

def generate_primes(number):
    logger = logging.getLogger(str(os.getpid()) + ':logger')

    for num in range(number + 1):
       # prime numbers are greater than 1
       if num > 1:
           for i in range(2,num):
               if (num % i) == 0:
                   break
           else:
               logger.info('child:' + str(os.getpid()) + ':' + str(num))

def start_workers():
    logger = logging.getLogger('parent:prime')
    logger.info('parent forking')

    for child in range(10):
        pid = os.fork()

        if not pid:

            logger = logging.getLogger('child:' + str(os.getpid()))
            fh = logging.FileHandler('logs/child_primes_' + str(os.getpid()) + '.log')
            logger.addHandler(fh)

            generate_primes(10000)

            os._exit(0)
        else:
            os.waitpid(pid, 0)

if __name__ == "__main__":

    logging.basicConfig(filename='logs/parent_primes.log', level=logging.INFO)
    logger = logging.getLogger('parent:prime')

    logger.info('Fork parent/child logging example.')
    logger.info('Starting children')

    start_workers()

Thomas, your solutions looks good. I have few remarks regarding the code style.

The following code looks a bit strange to me:

for num in range(number + 1):
    # prime numbers are greater than 1
    if num > 1:
        for i in range(2,num):
        ...

You can easily replace it with:

for num in range(2, number + 1):
    for i in range(2, num):
    ...

Do not use string concatenations, they are making your code less readable. Better use the .format() method of the string:

'child:' + str(os.getpid()) + ':' + str(num)

# Better:
'child:{pid}:{num}'.format(pid=os.getpid(), num=num)

In order to not put the meta information to every logging request, you can use logging formatters. Please check that doc for more details: https://docs.python.org/2/howto/logging-cookbook.html#using-logging-in-multiple-modules.
To simplify logging creation and make a code a bit more readable, you can create a function get_logger:

def get_logger(pid):
    file_name = 'logs/child_primes_{}.log'.format(pid)
    file_handler = logging.FileHandler(file_name)
    name = 'child:{}'.format(pid)
    logger = logging.getLogger(name)
    logger.addHandler(file_handler)
    # TODO: Add a formater here
    return logger


Conversion from Roman to decimal numbers My program makes a conversion between Roman and decimal numbers. Can you give me some advice on how to make it look more readable or to improve in general?

Would code extraction of similar-looking logic from switch cases into helper methods do the job? Or can we take some other approach and use recursion, enums or regular expressions to solve the conversion task?

import java.util.Scanner;

public class Romans_to_Decimals
{
    public static void main(String[] args)
    {
    System.out.println("Enter roman number from I to MMMCMXCIX (1 to 3999), q to quit: ");
    Scanner in = new Scanner(System.in);

    boolean done = false;
    while (!done && in.hasNext())
    {
        String number = in.next();
        if (number.equals("q"))
        {
            done = true;
        }
        else
        {
            int result = 0;
            char[] array = number.toCharArray();

            for (int i = number.length() - 1; i >= 0; i--)
            {
                switch (array[i])
                {
                case 'I':
                        result += 1;
                        break;
                case 'X':
                        if (i != 0 && array[i-1] == 'I')    //IX = 9
                        {
                            result += 9;
                            i--;
                        }
                        else
                        {
                            result += 10;
                        }
                        break;
                case 'C':
                        if (i != 0 && array[i-1] == 'X')    //XC = 90
                        {
                            result += 90;
                            i--;
                        }
                        else
                        {
                            result += 100;
                        }
                        break;
                case 'M':
                        if (i != 0 && array[i-1] == 'C')    //CM = 900
                        {
                            result += 900;
                            i--;
                        }
                        else
                        {
                            result += 1000;
                        }
                        break;
                case 'V':
                        if (i != 0 && array[i-1] == 'I')    //IV = 4
                        {
                            result += 4;
                            i--;
                        }
                        else
                        {
                            result += 5;
                        }
                        break;
                case 'L':
                        if (i != 0 && array[i-1] == 'X')    //XL = 40
                        {
                            result += 40;
                            i--;
                        }
                        else
                        {
                            result += 50;
                        }
                        break;
                case 'D':
                        if (i != 0 && array[i-1] == 'C')    //CD = 400
                        {
                            result += 400;
                            i--;
                        }
                        else
                        {
                            result += 500;
                        }
                        break;
                default:
                        System.out.println("Not correct roman number");
                        break;
                }
            }
            System.out.println("Corresponding decimal number is: " + result);
            }
        }
        in.close();
    }
}

public class Romans_to_Decimals

"when in Rome, do as the Romans do!"
keep to Java naming conventions when programming java.

for (int i = number.length() - 1; i >= 0; i--)

use the fore each notation if possible. In your case this would require a different approach to the problem tough...

switch (array[i])
 //..
    if (i != 0 && array[i-1] == 'I')

use variables with explicit names.

 case 'X':
                    if (i != 0 && array[i-1] == 'I')    //IX = 9
                    {
                        result += 9;
                        i--;
                    }
                    else
                    {
                        result += 10;
                    }
                    break;
            case 'C':
                    if (i != 0 && array[i-1] == 'X')    //XC = 90
                    {
                        result += 90;
                        i--;
                    }
                    else
                    {
                        result += 100;
                    }
                    break;
 for (char currentDigit:  number.toCharArray())

identify the variable elements whithing this code duplication and make them actula variables, then extract the logic to a method the getst the variables as parameters. Uning an enum to represent individual roman digits helps a lot:

    public class RomanNumbers {
        enum DigitValue {
            DIGIT_INVALID('-', 0),
            DIGIT_I('I', 1),
            DIGIT_V('V', 5),
            DIGIT_X('X', 10),
            DIGIT_L('L', 50),
            DIGIT_C('C', 100),
            DIGIT_D('D', 500),
            DIGIT_M('M', 1000),;
            private final char digit;
            private final int value;

            DigitValue(char digit, int value) {
                this.digit = digit;
                this.value = value;
            }

            static DigitValue getFor(char digit) {
                DigitValue[] values = values();
                for (DigitValue digitValue : values) {
                    if (digitValue.digit == digit) {
                        return digitValue;
                    }
                }
                throw new IllegalArgumentException(digit
                        + " is not valid part of a roman number");
            }

        }

        // member variable neede because we cannot return more than one value
        private static int result;  // static because you refused to create an actual object.

        public static void main(String[] args) {
            System.out.println("Enter roman number from I to MMMCMXCIX (1 to 3999), q to quit: ");
            Scanner in = new Scanner(System.in);

            boolean done = false;
            while (!done
                    && in.hasNext()) {
                String number = in.next();
                if (number.equals("q")) {
                    done = true;
                } else {
                    result = 0;
                    char[] array = number.toCharArray();

                    for (int i = number.length()
                            - 1; i >= 0; i--) {
                        DigitValue currentDigit = DigitValue.getFor(array[i]);
                        DigitValue digitBefore = i > 0 ? DigitValue.getFor(array[i- 1]) : DigitValue.INVALID);

                        switch (currentDigit) {
                            case DIGIT_I:
                                result += 1;
                                break;
                            case DIGIT_X:
                                i = caculateFor(currentDigit, digitBefore, DigitValue.DIGIT_I, i);
                                break;
                            case DIGIT_C:
                                i = caculateFor(currentDigit, digitBefore, DigitValue.DIGIT_X, i);
                                break;
                            case DIGIT_M:
                                i = caculateFor(currentDigit, digitBefore, DigitValue.DIGIT_C, i);
                                break;
                            case DIGIT_V:
                                i = caculateFor(currentDigit, digitBefore, DigitValue.DIGIT_I, i);
                                break;
                            case DIGIT_L:
                                i = caculateFor(currentDigit, digitBefore, DigitValue.DIGIT_X, i);
                                break;
                            case DIGIT_D:
                                i = caculateFor(currentDigit, digitBefore, DigitValue.DIGIT_C, i);
                                break;
                            default:
                                System.out.println("Not correct roman number");
                                break;
                        }
                    }
                    System.out.println("Corresponding decimal number is: "
                            + result);
                }
            }
            in.close();
        }

        private static int caculateFor(DigitValue currentDigitValue,
                DigitValue digitBefore,
                DigitValue reducingprevousDigit,
                int loopIndex) {
            result += currentDigitValue.value;
            if (digitBefore == reducingprevousDigit) {
                result -= reducingprevousDigit.value;
                loopIndex--;
            }
            return loopIndex;
        }
    }


Converting url data to base 64 I've a piece of code down below that I'm really unsure about. Basically an user can type the url (urlStr param of the method) he wants. From this url I need to :

get the data as Base64 so I can display it in an html img src tag.
Being able to crop the image afterward and save the image as a file

What I'm a bit unsure about is the way I deal with the extension and the fact that I use a BufferedImage.

public Object getAsObject(FacesContext arg0, UIComponent arg1, String urlStr) {
    //extension of the image
    String extension = urlStr.substring(urlStr.lastIndexOf(".")+1);
    //object containing some infos I need
    UrlImage urlImage = new UrlImage();
    if(("jpg").equals(extension)||("jpeg").equals(extension)||("png").equals(extension)){
        urlImage.setStringUrl(urlStr);
        ByteArrayOutputStream os = null;
        try {
                final URL url = new URL(urlStr);
                final HttpURLConnection connection = (HttpURLConnection) url
                        .openConnection();
                connection.setRequestProperty("User-Agent","Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_5) AppleWebKit/537.31 (KHTML, like Gecko) Chrome/26.0.1410.65 Safari/537.31");

                // I'm not sure I should be doing that here
                BufferedImage image = ImageIO.read(connection.getInputStream());

                urlImage.setBuffImg(image);

                connection.disconnect();
                String str;

                os = new ByteArrayOutputStream();
                ImageIO.write(image, extension, Base64.getEncoder().wrap(os));
                str =  os.toString(StandardCharsets.ISO_8859_1.name());
                urlImage.setBase64image("data:image/"+extension+";base64,"+str);
                System.out.println("data:image/"+extension+";base64,"+str);
        } catch (MalformedURLException e) {
        } catch (IOException e) {
        }finally {
            IOUtils.closeQuietly(os);
        }
    }
        return urlImage;

}

UrlImage is just:

public class UrlImage{
    private String stringUrl;
    private String base64image;
    private BufferedImage buffImg; // this will be used to crop the image.
}

Basically there is an input field on the website. When user types an URL in the input field, an ajax request is made and the method above is called with the URL the user typed (He could change 10 times the URL which means it will go 10 times through that method). When I've the data as base 64 I send it back to the browser of the user which displays the image. Then the user can crop the image as he pleases and when he is done he presses OK. Afterward I crop the image on server side with the BufferedImage (that I already have because of the previous ajax req) and save the file.

I would have done the process of converting an URL to base 64 on client side if I could but after some tries it seems like it is impossible because of cross-origin issues.

Whole code works though.

I'm assuming you have some way to handle other image formats, like .bmp or .gif; if not to actually handle them, at least a way to tell the user "this service does not support that image type" or something of the sort.

public Object getAsObject(FacesContext arg0, UIComponent arg1, String urlStr) {

Looks like you've got some overhead in there, you don't use the arg0 or arg1, why are they in here? Why is your method name getAsObject when instead you're doing something like downloadImageFromUrlToBase64?

If your framework is troubling you like this, consider putting the actual work in a separate class or function and having a simple method that only passes what you need. That way you're less attached to the framework which you are not using.

String extension = urlStr.substring(urlStr.lastIndexOf(".")+1);

Here you have a bug; if I pass "jpeg" in here, you say 'the last index of "." is non-existent, so the return value will be -1, add one, is 0'. Meaning we substring from the start of the string, returning the full string.

You'll silently swallow the resulting MalformedURLException, so if there is going to be any feedback to the user, it's going to be along the lines of "something went wrong".

            final URL url = new URL(urlStr);
            final HttpURLConnection connection = (HttpURLConnection) url
                    .openConnection();
            connection.setRequestProperty("User-Agent","Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_5) AppleWebKit/537.31 (KHTML, like Gecko) Chrome/26.0.1410.65 Safari/537.31");

            // I'm not sure I should be doing that here
            BufferedImage image = ImageIO.read(connection.getInputStream());

            urlImage.setBuffImg(image);

            connection.disconnect();

Here you indeed have the option of moving this to a separate method. I think it would help.

private BufferedImage downloadImageFromUrl(String url){ ... }

String str;

os = new ByteArrayOutputStream();
ImageIO.write(image, extension, Base64.getEncoder().wrap(os));
str =  os.toString(StandardCharsets.ISO_8859_1.name());

This looks like it has seen some heavy editing. There's a variable str declared, but oh wait, gotta do something else first, namely filling the buffer, and only then can we put it in the string. There's also a double space between the = and os.toString, which means (to me) that you should probably run your autoformatter more often or configure it to fix double spaces like that.

Anyway, there's no need to create a local variable before you're going to use it in this case, so just reorder the statements:

os = new ByteArrayOutputStream();
ImageIO.write(image, extension, Base64.getEncoder().wrap(os));
String str = os.toString(StandardCharsets.ISO_8859_1.name());

And give the local a better name too, I almost thought you were appending 'ISO_8859_1' to your urlImage.

os = new ByteArrayOutputStream();
ImageIO.write(image, extension, Base64.getEncoder().wrap(os));
String imageInBase64 = os.toString(StandardCharsets.ISO_8859_1.name());

Lastly, I don't know what you're doing with that user-agent string, but I don't think you're 4 browsers at once and it probably needs a comment.


CSV File Parser in Ruby The following is my Ruby attempt at a (very) basic CSV file parser class, inspired by an exercise from the book Seven Languages in Seven Weeks. I'm a Ruby novice and will be grateful for any suggestions for improvement.

#!/usr/local/bin/ruby -w

# CSV file parser.
#
# USAGE:
# CsvFile.new('data.csv').each { |row| puts row.firstname }
#
class CsvFile

  # Parses the given CSV file into a collection of rows.
  #
  def initialize file
    @rows = []
    headers = file.gets.chomp.split(", ")
    file.each do |line|
      values = {}
      headers.zip(line.chomp.split(", ")).each do |key, value|
        values[key] = value
      end
      @rows << CsvRow.new(values)
    end
  end

  # Iterates over all rows.
  #
  def each
    @rows.each { |row| yield(row) }
  end

  # Returns the index-th row, or null if no such row exists.
  #
  def [] index
    @rows[index]
  end
end

# CSV row.
#
class CsvRow

  # Creates a new CSV row with the given values.
  #
  # * *Args*    :
  #   - +values+ -> a hash containing the column -> value mapping
  #
  def initialize values
    @values = values
  end

  # Returns the value in the column given as method name, or null if
  # no such value exists.
  #
  def method_missing name, *args
    @values[name.to_s]
  end
end

# TEST CASES
#

class AssertionError < RuntimeError
end

def assert &block
    raise AssertionError unless yield
end

require 'stringio'

file = StringIO.new(
"firstname, lastname, age, sex
Andrej, Beles, 25, male
Delia, Marin, 20, female
Henry, Prahanth, 33, male"
)

csvFile = CsvFile.new(file)

row = csvFile[0]
assert { row.firstname == "Andrej" }
assert { row.lastname == "Beles" }
assert { row.age == "25" }
assert { row.sex == "male" }

row = csvFile[1]
assert { row.firstname == "Delia" }
assert { row.lastname == "Marin" }
assert { row.age == "20" }
assert { row.sex == "female" }

row = csvFile[2]
assert { row.firstname == "Henry" }
assert { row.lastname == "Prahanth" }
assert { row.age == "33" }
assert { row.sex == "male" }

puts "DONE."

You've got a bug here.

headers = file.gets.chomp.split(", ")

There is no requirement in a *.csv file that there be a space after the comma between items. In fact, if there is a space, that space should be considered part of the item, not part of the delimiter.

The next thing I notice is that comma separated files are not strictly separated by commas. Other delimiters are possible, and likely to be come across. Things such as pipes | are common. I would consider supporting other delimiters. For example, Excel supports Tabs, Semicolons, Commas, and Spaces, along with an option for a custom defined delimiter. You might not want to fuss with supporting user defined delimiters, but certainly your class becomes more useful if you support the ones I've mentioned.

The process of implementing this should clean up the string literal duplication you have here.

  def initialize file
    @rows = []
    headers = file.gets.chomp.split(", ")
    file.each do |line|
      values = {}
      headers.zip(line.chomp.split(", ")).each do |key, value|
        values[key] = value
      end
      @rows << CsvRow.new(values)
    end
  end

But minimally, you should replace ", " with a constant value so you never accidentally change the delimiter in one place, but not the other.


Conditional jump or move depends on uninitialised value Please review the following code:

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

int main(void)
{
    char *some_text = "THIS IS SOME ARBITRARY TEXT FOR TESTING THE STRLEN AND COPY FUNCTIONS";

    char *test = malloc(strlen(some_text) + 1);
    strncpy(test, some_text, strlen(some_text));
    strcat(test, "\0");

    printf("%s\n", test);
    if (test != NULL) free (test);
}

create a char* some_text with constant string;
create a char*test with and allocate memory the size of some_text
copy the content of some_text to test
concatenate the termination character to test
print out the test
free the test pointer

If I run this with valgrind, it says:

==31176== Conditional jump or move depends on uninitialised value(s)
==31176==    at 0x4008667: __GI_strlen (mc_replace_strmem.c:404)
==31176==    by 0x861DD4: puts (in /lib/libc-2.12.so)
==31176==    by 0x80484DF: main (strlen_and_copy.c:13)

The line mentioned in valgrind output strlen_and_copy.c:13 is below:

printf("%s\n", test);

Is it avoidable or should it be ignored? What is wrong with my code above?  Someone please explain.

Some general remarks to your code:

You should store the string length as a variable rather than calling it multiple times.
If you do strncpy(test, some_text, strlen(some_text) + 1) you can get rid of the strcat as strncpy will padd the remainder with \0 if src is less than n characters.
You don't need to test for NULL before free

a) because you'd get a segfault anyway before that line of code if it were NULL
b) because the standard guarantees that it is safe to call free(NULL)


Eliminate copy/paste and Factor logic out of foreach loop I have two conditions that need to traverse the same data but inside the traversal the logic changes slightly for each respective condition and in different places. If it was the same place that would be easy to factor out, but what's the best approach if it's not all in the same place for every condition? I seem to run into this situation often when parsing data.

switch(batch.DocumentType.ImportedFileDisposition)
{
    case ImportedFileDispositionActions.Delete:
        foreach (string documentMetadataFile in documentMetadataPaths)
        {
            string documentPath = Path.GetDirectoryName(documentMetadataFile);
            XDocument metadata = XDocument.Load(documentMetadataFile);
            foreach (var pageElement in metadata.Root.Element("Pages").Elements("Page"))
            {
                string pageName = null;
                XAttribute attribute = pageElement.Attribute("FileName");
                if (attribute != null)
                    pageName = attribute.Value;

                string pagePath = null;
                string originalPagePath = null;
                if (!string.IsNullOrWhiteSpace(pageName))
                {
                    pagePath = IOUtility.PathCombine(documentPath, pageName);
                    File.Delete(pagePath);
                }
            }
            File.Delete(documentMetadataFile);
            Directory.Delete(documentPath, true);
        }
        break;
    case ImportedFileDispositionActions.Archive:
        foreach (string documentMetadataFile in documentMetadataPaths)
        {
            string documentPath = Path.GetDirectoryName(documentMetadataFile);
            string baseImportDirectory = documentPath;
            string archiveDirectory = GenerateArchiveDirectory(batch, documentPath);
            XDocument metadata = XDocument.Load(documentMetadataFile);
            foreach (var pageElement in metadata.Root.Element("Pages").Elements("Page"))
            {
                string pageName = null;
                XAttribute attribute = pageElement.Attribute("FileName");
                if (attribute != null)
                    pageName = attribute.Value;

                string pagePath = null;
                string originalPagePath = null;
                if (!string.IsNullOrWhiteSpace(pageName))
                {
                    pagePath = IOUtility.PathCombine(documentPath, pageName);
                    string newArchivePath = IOUtility.PathCombine(archiveDirectory, pageName);
                    try
                    {
                        IOUtility.MoveFile(pagePath, newArchivePath);
                    }
                    catch
                    {
                        try
                        {
                            IOUtility.CopyFile(pagePath,newArchivePath, true);
                        }
                        catch (Exception ex)
                        {
                            throw new IOException("Failed to move or copy file: " + pagePath + " to archive: " + newArchivePath, ex);
                        }
                    }
                    File.Delete(pagePath);
                }
            }
            File.Delete(documentMetadataFile);
            Directory.Delete(documentPath, true);
        }
        break;
}

I'd start by writing another method, containing what's common in both blocks; then I'd extract another method for processing a single file, and in that method I'd decide if I want to archive or just delete the file... and then I'd get annoyed with having a method that does two things and I'd extract some MoveOrCopyToArchive method, and try to delete the file in all cases.

But that's just easy moving stuff around and reducing redundancies.

This is more worrying:

try
{
    IOUtility.MoveFile(pagePath, newArchivePath);
}
catch
{
    try
    {
        IOUtility.CopyFile(pagePath,newArchivePath, true);
    }
    catch (Exception ex)
    {
        throw new IOException("Failed to move or copy file: " + pagePath + " to archive: " + newArchivePath, ex);
    }
}
File.Delete(pagePath);

Nested try/catch blocks have a funky smell, but the worrying line is the last one: what makes you think pagePath can be safely deleted? If something went wrong moving or copying it, my bet is on File.Delete throwing an exception as well - and that one will bubble up your stack, meaning Directory.Delete would never get called... and the entire application might just die a horrible death.

Now back to that nested try/catch - first you attempt to move the file, and if that fails, you try to copy it. There are several reasons for failing to move a file, several of which will also fail to copy it. I think that's a good case for cherry-picking which exceptions will warrant trying to copy, and which ones aren't worth it:

ArgumentNullException is almost certainly ruled out, you're checking if pageName is null already. But I don't know what IOUtility.PathCombine does, and I have no garantee that GenerateArchiveDirectory returns a valid path either. Better safe than sorry; catching this exception means attempting to copy the same file/path is 100% certain to fail as well.
ArgumentException or NotSupportedException would be thrown for a path containing characters invalid in a path, or for a path in an invalid format. No need to try copying that file, it's garanteed to fail anyway.
DirectoryNotFoundException and PathTooLongException would also fail copying.
IOException would be thrown if the destination file already exists, or if the source filename wouldn't be found. Clearly not worth trying to copy.
UnauthorizedAccessException - ok, moving can't work, but maybe the file can be copied.

https://msdn.microsoft.com/library/system.io.file.move(v=vs.110).aspx

The catch block that doesn't specify any exception not only enters on any exception, without a handle on the exception object you have lost the original exception that tells you how the first operation - moving the file - failed.

Catching System.Exception would be over-the-top here; as shown above, you'd only really want to try a copy when an UnauthorizedAccessException is thrown - every other exception you can possibly catch there, would be caused by something that would also make the copy fail.

But then if any of these exceptions is caught, my bet is on File.Delete throwing the same exception.

I'll add that throwing your own IOException is confusing here, because the System.IO methods you're using can throw that same exception type, with a different meaning. If you really want to wrap the exception with one of your own, then you have a case for a custom exception type here - a FileArchiveException or something similar.

Why is there no try/catch block in the ImportedFileDispositionActions.Delete case?

I'd do it something like this:

if (fileDisposition == ImportedFileDispositionActions.Archive)
{
    SendToArchive(pagePath, documentMetadataFile, batch);
}

if (File.Exists(pagePath))
{
    File.Delete(pagePath);
}

//Assert.IsFalse(File.Exists(pagePath));

Understanding at a glance what's going on is easier if each idea reads like a sentence.

The nested try/catch can be avoided by extracting a method out of the try block:

private readonly ILogger _logger; // do something about exceptions!

private void SendToArchive(string path, string metadata, FooBar batch)
{
    try
    {
        var archivePath = GenerateArchiveDirectory(path, metadata, batch);
        MoveOrCopyToArchive(archivePath);
    }
    catch(IOException exception)
    {
        _logger.Info(exception);
    }
    catch(Exception exception)
    {
        _logger.Debug(exception);
    }
}

private void MoveOrCopyToArchive(string path)
{
    try
    {
        File.Move(path, ArchivePath);
    }
    catch(UnauthorizedAccessException exception)
    {
        _logger.Warn(exception);
        File.Copy(path, ArchivePath, true);
    }
}

The stack trace will contain enough information to determine if the exception was thrown moving, copy, or deleting the file; there's not enough information in your post to recommend whether exceptions should be handled, rethrown or simply bubbled up - I've made a few assumptions here.