Leetcode - find if BST is valid or not Given a binary tree, determine if it is a valid binary search tree
  (BST).

  Assume a BST is defined as follows:

  The left subtree of a node contains only nodes with keys less than the
  node's key. The right subtree of a node contains only nodes with keys
  greater than the node's key. Both the left and right subtrees must
  also be binary search trees.

Example 1:

  2
   / \
  1   3

Input: [2,1,3]
Output: true

Example 2:

 5
   / \
  1   4
     / \
    3   6

Input: [5,1,4,null,null,3,6]
Output: false
Explanation: The root node's value is 5 but its right child's value is 4.

My solution

Do inorder traversal and keep it in stack.
Iterate through stack to see if any of the value on top is less than equal to second top.

Definition for a binary tree node.

 public class TreeNode {
       int val;
       TreeNode left;
       TreeNode right;
       TreeNode(int x) { val = x; }
  }

class Solution {

    public void doInOrderTraversal(TreeNode root, Stack s) {
        if(root == null) {
            return;
        }
        doInOrderTraversal(root.left, s);
        s.push(root.val);
        doInOrderTraversal(root.right, s);

    }

    public boolean isValidBST(TreeNode root) {

        if(root == null) {
            return true;
        }

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

        doInOrderTraversal(root, stack);

        while(!stack.isEmpty()) {
            int top = stack.pop();
            if(stack.isEmpty()) {
                return true;
            }
            int secondTop = stack.peek();
            if(top <= secondTop) {
                return false;
            }
        }
        return true;
    }
}

I was thinking to not use stack, but two values current and previous. And keep check if current is less than previous then only break. I am not sure, how to do this. Please suggest.

Your solution:

seems legit :)
One thing I'd suggest here is to use ArrayDeque instead of Stack

Another possible solution:

Basically, for every subtree we have constrain, that every node in it should be in range (X, Y).

For root this range will be (-inf; +inf) - in other words, there could be any value in root.

For root's left subtree range will be (-inf, value-in-root), for right - (value-in-root, +inf).

Last thing - on each iteration we should check, that value in node is within this range, like so:

public boolean doInOrderTraversal(TreeNode root, int min, int max) {
    if (root == null) {
        return true;
    }
    if (root.val <= min || root.val >= max) {
        return false;
    }

    return doInOrderTraversal(root.left, min, root.val) && doInOrderTraversal(root.right, root.val, max);
}


Function to find longest word in a string Write a function called longest which will take a string of space-separated words and will return the longest one.

My code:

function longest(str) {
  var words = arguments.split(" ");
  var words_count = words.length;

  var longest_word_length = 0;

 for(var i = 0; i < words_count; i++){
   if(longest_word_length < words[i].length){
       longest_word_length = words[i].length;
   }
}
return longest_word_length;
}

Running it with this test:

describe("test for longest function functionality", function() {
  it("should return correct output for normal strings", function() {
  expect(longest("A")).toEqual("A");
  expect(longest("I love Avatar")).toEqual("Avatar");
  expect(longest("The stupidities of youth")).toEqual("stupidities");
  });

  it("should return correct output for gibberish", function() {
  expect(longest("hgdydrxtfEq Rradsc tstsa taeWwwecfgdd")).toEqual("hgdydrxtfEq");
  });

 it("should work for sentences with numbers", function() {
 expect(longest("This is a sentence with a number 7685838788")).toEqual("7685838788");
 });
});

Good on you for writing real tests.

Overall, the code seems fine as-is (edit: Actually, no. See David Harkness' comment on the question; arguments.split should be str.split) though I do have a few comments:

The JavaScript convention is to use camelCase for variable names - not snake_case.
Splitting by " " works, but a more comprehensive solution would be to split by /\s+/. That is, split by one or more whitespace characters. That way you avoid zero-length strings in the array if there are two spaces in a row, and it'll also split on newlines and other whitespace. And if you trim() the string first, you'll also avoid empty strings at the start/end if the input string has leading or trailing whitespace.

Also the whole thing can be made more functional (and use some ES6 syntax):

function longest(string) {
  return string
    .trim()
    .split(/\s+/)
    .reduce((longest, word) => word.length > longest.length ? word : longest);
}

reduce (aka "fold" in other languages), when given no 2nd argument, will use the first element - the first word - in the array as its initial memo. The callback then either reuses that word, or replaces it when it finds one that's longer. Whatever the memo is at the end is what gets returned.

Another approach, much less functional, could be to use String.replace as a "scan" function:

function longest(string) {
  var length = 0;
  var longestWord = "";
  string.replace(/\S+/g, (word) => {
    if(word.length > length) {
      length = word.length;
      longestWord = word;
    }
    return ""; // for good measure; otherwise all words will be replaced with "undefined" in-memory
  });
  return longestWord;
}

The regex finds any run of non-whitespace character of length 1 or more. It's not terribly pretty, since replace isn't being used "as intended", really, and there are some closure side-effects. It doesn't have the memory consumption of the previous solution however (no array used to hold the split string).


First Occurrence of Number in Binary Search? I am trying to write a function, where given a sorted array, and a number, it returns the FIRST index of that number.

Is this correct? Code is in Ruby, but even if you don't know Ruby, you can pretty much understand the syntax.

def first(array, n)
        if(array.size == 0)
           return -1
        end

        l = 0
        u = array.size - 1

        #LOOP INVARIANT:
        # l <= startIndex <= u

        while( l != u)
         #LOOP TERMINATES when l = u.
         #this occurs if we reach the far right or far left without finding the #
         #or if we found the start index of the number

            m = (l+u)/2
            if(array[m] > n)
                u = m -1    # l <= startIndex <= u

            elsif(array[m] < n)
                l = m + 1   # l <= startIndex <= u

            elsif(array[m] == n)
                u = m       # l <= startIndex <= u

            end

            #LOOP always terminates because we're narrowing the range by changing l or u.
            #if there's 1 element, finding the midpoint won't narrow the range,
            #but the while condition ends the loop anyway.
        end

        if(array[u] == n)
           return u
        else
           return -1
        end

    end

Not exactly a code review, but did you know that ruby 2 introduces binary search for arrays ?

See : Array#bsearch.

your problem could be solved like this:

myarray.each_with_index.to_a.bsearch{|(x,index)| x == n}
# => if found, returns an array containing the number and the index


dir="auto" JavaScript shim for IE Reason for script:

dir="auto" is an attribute value from the HTML 5 spec with current poor support in IE and Opera browsers.  The project I am working on only cares about IE, Firefox and Safari browsers and handles text in both left to right LTR and right to left RTL languages.  For cases where the directionality of the text coming from the data source is unkown, I would like to use dir="auto" to handle most cases of directionality. For IE, I would like to simulate this functionality using JavaScript.

Goal of review:

I am hopeful that a review of my work will identify areas where I can improve the code to be more in line with its stated purpose as well as aligning the code to best practices and identifying any bugs that may exist.

Implementation:

An isLTR function was used as I figured most use cases would be against LTR text as opposed to RTL.
In my research, I wandered across a script by Nicholas Zakas which I thought would be useful and have integrated it into my solution.
The project uses Dojo 1.8.1.

XHTML:

Script call in head tag after call to Dojo library:

<!--[if IE]><script type="text/javascript" src="./PatternsTools/PatternsBidi.js"></script><![endif]-->

JavaScript:

window.Patterns = window.Patterns || {};
window.Patterns.Bidi = window.Patterns.Bidi || {};
window.Patterns.Bidi.Strings = window.Patterns.Bidi.Strings || {};
window.Patterns.Bidi.Strings.LTRcharacters = "\u0041-\u005A\u0061-\u007A\u00C0-\u00D6\u00D8-\u00F6\u00F8-\u02B8\u0300-\u0590\u0800-\u1FFF\u200E\u202A\u202D\u2C00-\uFB1C\uFDFE-\uFE6F\uFEFD-\uFFFF";
window.Patterns.Bidi.Strings.RTLcharacters = "\u0591-\u07FF\u200F\u202B\u202E\uFB1D-\uFDFD\uFE70-\uFEFC";
window.Patterns.Bidi.Strings.NeutralWeakCharacters = "\u0000-\u0040\u005B-\u0060\u007B-\u00BF\u00D7\u00F7\u02B9-\u02FF\u2000-\u2BFF\u2010-\u2029\u202C\u202F-\u2BFF";

(function(/*Class Object*/ Bidi){
    function getDirection(/*HTML Element*/ element) {
    /*Copyright 2010 Nicholas C. Zakas. All rights reserved.*/
    /*MIT Licensed*/
        var result = null;
        if (element){
            if (window.getComputedStyle){
                result = window.getComputedStyle(element,null).direction;
            } else if (element.currentStyle){
                result = element.currentStyle.direction;
            }
        }

        return result;
    }

    function prependOverride(/*HTML Element*/ element, /*Boolean*/ isLTR) {
        var direction = getDirection(element) === "ltr";
        if (isLTR !== direction) {
            element.innerHTML = (isLTR ? "&#8237;" : "&#8238;") + element.innerHTML + "&#8236;";
        }
    }

    function isLTR(/*String*/ s) {
        var ltrDirCheck = new RegExp("^[" + Bidi.Strings.NeutralWeakCharacters + "]*?[" + Bidi.Strings.LTRcharacters + "]");
        return ltrDirCheck.test(s);
    };

    (function init() {
        require(["dojo/ready", "dojo/_base/array"], function(/*Function*/ ready, /*Object*/ array) {
            ready(function() {
                array.forEach(dojo.query("[dir=auto]"), function(/*HTML Element*/ element, /*Integer*/ i) {
                    prependOverride(element, isLTR(element.innerHTML));
                });
            });
        });
    })();
})(window.Patterns.Bidi);

My 2 cents:

Code looks really good overall
In prependOverride, the naming is unfortunate

The parameter isLTR is really what you request, not what it is
The var direction really is a boolean, not the direction of the element
If you are going to /** type **/ variables, you might as well go Spartan and use e instead of element

prependOverride has 4 magical constants
isLTR should cache the regex, not rebuild it in every call

for prependOverride the following might be cleaner:

function prependOverride(/*HTML Element*/ e, /*Boolean*/ requiresLTR) {
    var hasLTR = getDirection(e) === "ltr";
    if (hasLTR !== requiresLTR) {
        e.innerHTML = (requiresLTR? "&#8237;" : "&#8238;") + e.innerHTML + "&#8236;";
    }
}

or, even you could pass the direction you require

function prependOverride(/*HTML Element*/ e, /*String*/ requiredLTR) {
    var actualLTR = getDirection(e);
    if (actualLTR !== requiredLTR) {
        e.innerHTML = (requiredLTR=="ltr"? "&#8237;" : "&#8238;") + e.innerHTML + "&#8236;";
    }
}

I will leave the naming of the magical constants to you.


Taking objects from literal with many objects I have data returned from an API coming back in a response: response.data

[
    {"amount":"45342",
    "comment":"random text",
    "currency":"USD"},
    {"city":"LA",
    "amount":"54366",
    "lane_id":{"$oid":"5645999965a5b0"}}
]

I need to take only the amount and create a new set of objects like this:
[{amount: "45342"}, {amount: "54366"}]

The obvious way would be to push it through a loop like so:

var newArray = [];
var newObject;
var i, len;
for (len = response.data.length, i=0; i<len; ++i){
  newObject = response.data[i];
  newArray.push({amount: newObject.amount});
};

Is there a better way? I am running on angular 1.5 and jquery

you can use the map function provided for all arrays. You would need to make sure that response.data always returns an array, even in the case where nothing was returned from the api otherwise you can get an error.

var newArray = response.data.map(function(data) {
  return {amount: data.amount}
});

would get you what you are looking for.


Count word frequencies, and print them most-frequent first I'm writing a comparison of a simple word-counting program in various languages. The task is to count the frequencies of unique, space-separated words in the input, and output the results most frequent first. Case should be normalized to lowercase (ASCII is okay).
But it's been a long time since I've written C++ (before the C++11 days). Is this idiomatic modern C++? Any improvements I could make to make this more idiomatic or simpler (I'm not looking for efficiency improvements here). I want to stick to the standard library.
#include <algorithm>
#include <iostream>
#include <string>
#include <unordered_map>
#include <vector>

using namespace std;

int main() {
    string word;
    unordered_map<string, int> counts;
    while (cin >> word) {
        transform(word.begin(), word.end(), word.begin(),
            [](unsigned char c){ return tolower(c); });
        counts[word]++;
    }

    vector<pair<string, int>> ordered(counts.begin(), counts.end());
    sort(ordered.begin(), ordered.end(), [](auto &a, auto &b) {
        return a.second > b.second;
    });

    for (auto count : ordered) {
        cout << count.first << " " << count.second << "\n";
    }
    return 0;
}

Mainly this looks fine.

using namespace std; is a bad habit to get into due to potential name collisions. Instead prefer to type std:: where necessary.

counts[word]++;. We don't need an un-incremented copy of the count, so we should use pre-increment here instead: ++counts[word];

sort(ordered.begin(), ordered.end(), [](auto &a, auto &b) we don't alter a or b in the lambda, so these should be const&.

for (auto count : ordered) copies the pairs. Again, we should be using auto const& here.

return 0; we don't need this, as the compiler will add it for us automatically. We might also use a named constant instead of 0, specifically EXIT_SUCCESS from <cstdlib>.


Listing employees whose employee number is divisible by some given number Here I want to improve the code in addListofEmployees and mainMethod. How can I do so?

import java.util.LinkedList;
import java.util.List;
import java.util.Scanner;
import java.util.stream.Collectors;

public class AddEmployees extends Employee {

    public static List<Employee> addListOfEmployees(long upperLimit) {

        List<Employee> list = new LinkedList<>();
        for (long i = 0; i < upperLimit; i++) {
            list.add(new Employee(i));
        }
        return list;
    }

    public static List<Employee> getAllNumbersDividedByProvidedNumber(long upperLimit, int divideBy) {
        return AddEmployees.addListOfEmployees(upperLimit).parallelStream()
                .filter(Employee -> Employee.getNumber() % divideBy == 0).collect(Collectors.toList());

    }

    public static void mainMethod() {
        Scanner sc = null;
        try {
            sc = new Scanner(System.in);
            System.out.println("enter upper limit as lower is zero");
            long upperLimit = sc.nextInt();
            System.out.println("enter the number you want to divide with");
            int divideBy = sc.nextInt();

            for (Employee e : AddEmployees.getAllNumbersDividedByProvidedNumber(upperLimit, divideBy)) {
                System.out.println(e.getNumber());
            }
        } catch (Exception ex) {
            ex.printStackTrace(System.out);
        } finally {
            if (sc != null)
                sc.close();
        }

    }

    public static void main(String[] args) {
        mainMethod();
    }
}

Employee Class

package com.lambada.filter_predicatemethod;

public class Employee {

    private long number;

    public Employee() {
    }

    public Employee(long number) {
        this.number = number;
    }

    public long getNumber() {
        return number;
    }

}

Edit

I thinks this can achieving same result but not sure that chaining is a good part

public static List<Employee> getEmployeeDividedBy(long upperLimit, long dividedby) {
        return LongStream.range(0, upperLimit).mapToObj(Employee::new).collect(Collectors.toList()).stream()
                .filter(Employee -> Employee.getNumber() % dividedby == 0).collect(Collectors.toList());
    }

Yes, there is.

AddEmployee.java:

private AddEmployees() {}

public static List<Employee> createListOfEmployees(long upperLimit) {
    return LongStream.range(0, upperLimit)
                     .mapToObj(Employee::new)
                     .collect(Collectors.toList());
}

public static List<Employee> getAllNumbersDividedByProvidedNumber(long upperLimit, int divideBy) {
    return createListOfEmployees(upperLimit).parallelStream()
            .filter(Employee -> Employee.getNumber() % divideBy == 0).collect(Collectors.toList());

}

Note that the constructor is declared private since it is useless in this context. Also, addListOfEmployees is not the best possible name; I used createListOfEmployees instead.

Also, since you are printing employees, it would be very nice if they have overriden the toString method.

All in all, I had this in mind:

Employee.java:

public class Employee {

    private final long number;

    public Employee() {
        this(0L);
    }

    public Employee(long number) {
        this.number = number;
    }

    public long getNumber() {
        return number;
    }

    public String toString() {
        return "[Employee ID: " + number + "]";
    }
}

AddEmployees.java:

import java.util.List;
import java.util.Scanner;
import java.util.stream.Collectors;
import java.util.stream.LongStream;

public class AddEmployees extends Employee {

    private AddEmployees() {}

    public static List<Employee> createListOfEmployees(long upperLimit) {
        return LongStream.range(0, upperLimit)
                         .mapToObj(Employee::new)
                         .collect(Collectors.toList());
    }

    public static List<Employee> getAllNumbersDividedByProvidedNumber(long upperLimit, int divideBy) {
        return createListOfEmployees(upperLimit).parallelStream()
                .filter(Employee -> Employee.getNumber() % divideBy == 0).collect(Collectors.toList());

    }

    public static void main(String[] args) {
        Scanner sc = new Scanner(System.in);
        System.out.println("Enter the number of employees: ");
        int numberOfEmployees = sc.nextInt();
        System.out.print("Enter the divisor: ");
        int divisor = sc.nextInt();

        getAllNumbersDividedByProvidedNumber(numberOfEmployees,
                                             divisor)
                .forEach(System.out::println);
    }
}

Additional advice

You can shorten your getEmployeeDividedBy a bit:

public static List<Employee> getEmployeeDividedBy(long upperLimit, long dividedby) {
    return LongStream.range(0, upperLimit)
                     .filter(l -> l % dividedby == 0)
                     .mapToObj(Employee::new)
                     .collect(Collectors.toList());
}

Note that I removed .collect(Collectors.toList()).stream() since it is not required. Also, I first filter, and map only after that since this prunes away some pointless computation.

Hope that helps.


Check digital input signals that is sent as an integer using bitwise checking I have a program that works with a hardware via an ActiveX API. The API have events that is triggered by the hardware.
In the hardware there is an IO card. There is 16 in and 16 out. Changes in input signal triggers the event handler below.
The event uses an int to show which signal is set/unset. This by using binary addresses for each signal

Signal          Mask
INPUT_0         0x01;
INPUT_1         0x02;
INPUT_2         0x04;
INPUT_3         0x08;
...
INPUT_11        0x800

(there are some signals that are used for something else and can't be used as Input)

The signals can be set/unset in any order, but the event will trigger every time a change is done!
The value, p_nBits, will include all signals state. This means that if INPUT_0 is set and then INPUT_1 is set
then p_nBits will be 3, i.e. both signals! But INPUT_0 has already been handled and should not be handled again until it is unset.
I have an integer currentBits that is the current state of the input signals.

Any error in logic?
What can be improved?
Is it possible to make it faster?

// Current state of Input signals
int currentBits = 0;

public void IoPort_sigInputChange(int p_nBits)
{
    lock (lockObj)
    {

        if ((p_nBits & INPUT_0) == INPUT_0)
        {
            if ((currentBits & INPUT_0) != INPUT_0)
            {
                currentBits |= INPUT_0;
                RaiseRunModeEvent(true);
            }
        }
        else
        {
            if ((currentBits & INPUT_0) == INPUT_0)
            {
                currentBits &= ~INPUT_0;
                RaiseRunModeEvent(false);
            }
        }

        if ((p_nBits & INPUT_1) == INPUT_1)
        {
            if ((currentBits & INPUT_1) != INPUT_1)
            {
                currentBits |= INPUT_1;
                RaiseAllPositionsSetEvent(true);
            }
        }
        else
        {
            if ((currentBits & INPUT_1) == INPUT_1)
            {
                currentBits &= ~INPUT_1;
                RaiseAllPositionsSetEvent(false);
            }
        }

        if ((p_nBits & INPUT_2) == INPUT_2)
        {
            if ((currentBits & INPUT_2) != INPUT_2)
            {
                currentBits |= INPUT_2;
                RaiseOrderEndedEvent(true);
            }
        }
        else
        {
            if ((currentBits & INPUT_2) == INPUT_2)
            {
                currentBits &= ~INPUT_2;
                RaiseOrderEndedEvent(false);
            }
        }

        // more checks may occur
    }
}

Firstly, you can simplify the outer else statements:

...
    else
    {
      if ((currentBits & INPUT_0) == INPUT_0)
      {
        ...
      }
    }
...

to

...
else if ((currentBits & INPUT_0) == INPUT_0)
{
} ...

Secondly, if the check of all masks follows the same pattern as showed, you can create a mask checker method like this one (in C# 7.0):

private void HandleMask(int bitPattern, (int mask, Action<bool> action) maskItem)
{
  if ((bitPattern & maskItem.mask) == maskItem.mask)
  {
    if ((currentBits & maskItem.mask) != maskItem.mask)
    {
      currentBits |= maskItem.mask;
      maskItem.action(true);
    }
  }
  else if ((currentBits & maskItem.mask) == maskItem.mask)
  {
    currentBits &= ~maskItem.mask;
    maskItem.action(false);
  }
}

public void IoPort_sigInputChange(int p_nBits)
{
  lock (lockObj)
  {
    var maskItems = new(int mask, Action<bool> action)[]
    {
      (mask: INPUT_0, action: RaiseRunModeEvent),
      (mask: INPUT_1, action: RaiseAllPositionsSetEvent),
      (mask: INPUT_2, action: RaiseOrderEndedEvent),
    };

    foreach (var maskItem in maskItems)
    {
      HandleMask(p_nBits, maskItem);
    }
  }
}

Update

In .net 3.5 you should/could do it in a way like this:

private void HandleMask(int bitPattern, int mask, Action<bool> action)
{
  if ((bitPattern & mask) == mask)
  {
    if ((currentBits & mask) != mask)
    {
      currentBits |= mask;
      action(true);
    }
  }
  else if ((currentBits & mask) == mask)
  {
    currentBits &= ~mask;
    action(false);
  }
}

public void IoPort_sigInputChange(int p_nBits)
{
  lock (lockObj)
  {
    var maskItems = new[]
    {
      new { mask = INPUT_0, action = (Action<bool>)RaiseRunModeEvent },
      new { mask = INPUT_1, action = (Action<bool>)RaiseAllPositionsSetEvent },
      new { mask = INPUT_2, action = (Action<bool>)RaiseOrderEndedEvent },
    };

    foreach (var maskItem in maskItems)
    {
      HandleMask(p_nBits, maskItem.mask, maskItem.action);
    }
  }
}


Renames file-name references inside files in a directory The filenames.txt contains all the names of the files that you want to change in a given directory.
It is generated by:

import os

walk = os.walk("./app/")

for dirpath, dirs, files in walk:
    for filename in files:
        fpath = os.path.join(dirpath, filename)
        with open("filenames.txt", "a") as f:
            f.write(filename + "\n")

The Test directory is the directory whose files you are targeting.

"""Rename files.
Rename file references containing `-` with `_`
"""
import os

# Directory to traverse
walk = os.walk('./Test/')

# A file containing filenames that you want to change.
# Already generated
with open("filenames.txt") as filenames_to_change:
    names = filenames_to_change.read().splitlines()

for dirpath, dirs, files in walk:
    for filename in files:
        fpath = os.path.join(dirpath, filename)

        for name in names:
            old_name = name
            new_name = name.replace('-', '_')
            print("{} >>> {}".format(old_name, new_name))

            with open(fpath) as f:
                s = f.read()
            s = s.replace(old_name, new_name)
            with open(fpath, "w") as f:
                f.write(s)

For each filename you want to replace in the content, you open each file, mutate the content and write it back. Meaning that if you want to modify 10 filenames, you open each file 10 times and write it back 10 times.

This is clearly inefficient, you should open each file once, perform the 10 modifications you need and then write it back.

You also recompute new_name for each file which is not necessary as you could store this information somehow.

Last but not least, you could define some function to organize the code and give meaning to your actions:

"""
Rename references to filenames inside content of files in a directory.

Rename file references containing `-` with `_`
"""

import os

def update_references(conversion_table, directory):
    for root, dirs, files in os.walk(directory):
        for filename in files:
            path = os.path.join(root, filename)

            # Read initial content from file
            with open(path) as f:
                content = f.read()

            # Update content with new references
            for old_name, new_name in conversion_table.items():
                content = content.replace(old_name, new_name)

            # Update file with new content
            with open(path, "w") as f:
                f.write(content)

if __name__ == '__main__':
    # A file containing filenames that you want to change.
    # Already generated
    with open("filenames.txt") as filenames_to_change:
        names = filenames_to_change.read().splitlines()

    renamed = {name: name.replace('-', '_') for name in names}
    update_references(renamed, './Test/')


Retrieving all institutions in a specified country I'm checking each variable to prevent null exception, but still wondering can this be improve in much simpler and cleaner if you will be writing the same code? Please see the below snippet:

[AjaxOnly]
public ActionResult GetInstitutions(string parent)
{
    if (string.IsNullOrEmpty(parent))
        return Json(string.Empty, "application/json", Encoding.UTF8, JsonRequestBehavior.AllowGet);

    var country = _countryRespository.GetByCountryCode(parent);

    if (country == null)
        return Json(string.Empty, "application/json", Encoding.UTF8, JsonRequestBehavior.AllowGet);

    if (country.Institutions == null || country.Institutions.Count <= 0)
        return Json(string.Empty, "application/json", Encoding.UTF8, JsonRequestBehavior.AllowGet);

    var institutions = country.Institutions
        .OrderBy(i => i.Name)
        .Where(i => !i.Name.Contains("institutiontest1"))
        .Where(i => !i.Name.Contains("institutiontest2"))
        .Select(i => new SelectListItem { Text = i.Name, Value = i.Id.ToString() });

    return Json(institutions, "application/json", Encoding.UTF8, JsonRequestBehavior.AllowGet);
}

CountryRepository.GetByCountryCode method implementation

public Country GetByCountryCode(string countryCode)
{
    return Session.Query<Country>()
                    .Where(c => c.Iso == countryCode)
                    .FetchMany(c => c.Institutions)
                    .SingleOrDefault();
}

Country.Institutions should never return a null collection, and if it ever does, I'd prefer having a NullReferenceException thrown to tell me something is broken with my API. The null-check is therefore redundant here:

if (country.Institutions == null || country.Institutions.Count <= 0)
    return Json(string.Empty, "application/json", Encoding.UTF8, JsonRequestBehavior.AllowGet);

As for country.Institutions.Count <= 0, it makes me raise an eyebrow. How would .Count ever return -12? Clearly the intent here is to say "if there aren't any institutions in that country", which is better conveyed like this:

if (!country.Institutions.Any())
    return Json(string.Empty, ...);

Now, you have two separate conditions that both return the same "empty value" - combine them - and because a scope is always better when it's explicit, add those curly braces, too:

if (country == null || !country.Institutions.Any())
{
    return Json(string.Empty, "application/json", Encoding.UTF8, JsonRequestBehavior.AllowGet);
}

This leaves another "empty value" returned when parent is null... which is annoying.

Why can't GetByCountryCode handle null input? (Iso can't legally be empty, right?)

public Country GetByCountryCode(string countryCode)
{
    if (string.IsNullOrEmpty(countryCode)) { return null; }

    return Session.Query<Country>()
                  .Where(country => country.Iso == countryCode)
                  .FetchMany(country => country.Institutions)
                  .SingleOrDefault();
}

That way country will be null given a null input, and you don't need to repeat yourself anymore:

var country = _countryRespository.GetByCountryCode(parent);
if (country == null || !country.Institutions.Any())
{
    return Json(string.Empty, "application/json", Encoding.UTF8, JsonRequestBehavior.AllowGet);
}

And then select your data:

var institutions = country.Institutions
    .OrderBy(i => i.Name)
    .Where(i => !i.Name.Contains("institutiontest1"))
    .Where(i => !i.Name.Contains("institutiontest2"))
    .Select(i => new SelectListItem { Text = i.Name, Value = i.Id.ToString() });

hmm.. what's with these .Where statements? Filtering out test data? So if you go and add some institutiontest3 you'll need to add yet another .Where call?

Don't do that. Make yourself a separate, dev/test database, and have another database for your production data - and use proper configuration to determine which connection string to use to initialize your Session.

TL;DR:

[AjaxOnly]
public ActionResult GetInstitutions(string parent)
{
    var country = _countryRespository.GetByCountryCode(parent);
    if (country == null || !country.Institutions.Any())
    {
        return Json(string.Empty, "application/json", Encoding.UTF8, JsonRequestBehavior.AllowGet);
    }

    var institutions = country.Institutions
                              .OrderBy(institution => institution.Name)
                              .Select(institution => new SelectListItem
                              {
                                  Text = institution.Name,
                                  Value = institution.Id.ToString()
                              });
    return Json(institutions, "application/json", Encoding.UTF8, JsonRequestBehavior.AllowGet);
}

If the filtered-out values are meant to be specific values, consider making them explicit:

var ourInstitutions = new[]
{
    "foo",
    "bar"
};

And then you should be able to do this:

.Where(institution => !ourInstitutions.Contains(institution.Name))

Which makes the intent much clearer. If your ORM complains (looks like you're using NHibernate, which I'm not very familiar with), then make that filtering handled by LINQ-to-Objects instead:

.ToList()
.Where(institution => !ourInstitutions.Contains(institution.Name))

If it's just a few values to iterate, there shouldn't be much of a penalty there, and the readability+maintainability gain is considerable, assuming you're looking for specific values. Otherwise, this would be equivalent:

.Where(institution => !ourInstitutions.Any(ours => institution.Name.Contains(ours.Name))


Use of mediator design pattern with mailing list implementation I have implemented the following scenario for the Mediator pattern sample:

MailingListMediator: Manages the subscription and send the mail back and forth.

JavaMailingList
SQLMailingList

(Colleagues)MailUsers: They can login, subscribe to the mailing list, send mails to mailing list.

GmailUsers
YahooUsers

My implementation looks like this:

MailingListMediator

public abstract class MailingListMediator {

    protected List<MailUser> mailingList;

    public MailingListMediator() {
        this.mailingList = new ArrayList<MailUser>();
    }

    public void sendMail(String message){
        for(MailUser mUser:mailingList){
            System.out.println("Mail message: '"+message+"' sent to "+ mUser.getMailId());
        }
    }
    public abstract String subscribe(MailUser mailUser);
}

MailUser

public abstract class MailUser {
    public abstract void login(String mailID);
    public abstract void sendMailToMailingList(MailingListMediator mailingList, String message);
    public abstract String getMailId();
}

GmailUser

public class GmailUser extends MailUser {
    String mailID;

    @Override
    public void login(String mailID) {
        this.mailID = mailID;
    }

    @Override
    public void sendMailToMailingList(MailingListMediator mailingList, String message)     {
        mailingList.sendMail(message);
    }

    @Override
    public String getMailId() {
        if(mailID != null){
            return mailID;
        }else{
            return "Login First";
        }
    }
}

YahooUser

public class YahooUser extends MailUser {
    String mailID;

    @Override
    public void login(String mailID) {
        this.mailID = mailID;
    }

    @Override
    public void sendMailToMailingList(MailingListMediator mailingList, String message) {
        mailingList.sendMail(message);
    }

    @Override
    public String getMailId() {
        if(mailID != null){
            return mailID;
        }else{
            return "Login First";
        }
    }
}

JavaMailingList

public class JavaMailingList extends MailingListMediator {
    private final String mailingListId = "java";

    @Override
    public String subscribe(MailUser mailUser) {
        mailingList.add(mailUser);
        return mailingListId;
    }
}

SQLMailingList

public class SQLMailingList extends MailingListMediator {
    private final String mailingListId = "sql";

    @Override
    public String subscribe(MailUser mailUser) {
        mailingList.add(mailUser);
        return mailingListId;
    }
}

ForumUsers

public class ForumUsers{
    public static void main(String[] args) {
        MailingListMediator javaMailingList = new JavaMailingList();
        //User 1
        MailUser gmail = new GmailUser();
        gmail.login("iamgmail@gmail.com");
        javaMailingList.subscribe(gmail); // He subscribes

        //User 2
        MailUser yahoo = new YahooUser();
        yahoo.login("iamyahoo@yahoo.com");
        javaMailingList.subscribe(yahoo); // He subscribes

        //gmail user sends a question to the Java Mailing List
        gmail.sendMailToMailingList(javaMailingList, "How to excel in Java??!!");
    }
}

Does this implementation adhere to the Mediator Design? What improvements can be made?

I am not familiar with the Mediator pattern, but I think your code is not complex enough to make good use of the pattern. Take your JavaMailingList and SQLMailingList, what is the difference between those classes ? The value of mailingListId. The implementation of subscribe is exactly the same, you add the same variable to the class. If your MailingListMediator would look like :

public class MailingListMediator {
    private final String mailingListId;
    protected List<MailUser> mailingList;

    public MailingListMediator(String mailingListId) {
        this.mailingList = new ArrayList<MailUser>();
        this.mailingListId = mailingListId;
    }

    public void sendMail(String message){
        for(MailUser mUser:mailingList){
            System.out.println("Mail message: '"+message+"' sent to "+ mUser.getMailId());
        }
    }
    public String subscribe(MailUser mailUser) {
        mailingList.add(mailUser);
        return mailingListId;
    }
}

There is suddenly no need for  JavaMailingList and SQLMailingList.

The same principle apply to YahooUser and GmailUser. Those classes are almost a copy-paste of each other. Same implementation for all the methods and adding the same variable to the class. Why is mailID an attribute in the children classes but not in MailUser? I could repeat the same process that I used for MailingListMediator and I could remove YahooUser and GmailUser.

My conclusion is your code looks like trying to be complicated when in fact it is not. You want to try the Mediator pattern ? Well wait when you will need it, do not force your code into a pattern. Pattern will be useful when you need it, otherwise it will only make your code less readable and harder to maintain.

Since you wanted to implement the Mediator pattern, I will expand more on that part. You do have a mediator pattern implemented. Your "colleagues" do interact between each other through the mediator that is the MailingList. There isn't a MailUser that interact directly with each other, that is the main point of the mediator pattern. Do know that your example is a little weak. I would suggest you, if you really want to test the mediator pattern, try a to code a chat application. This could be a good exercise since you will have a lot of users that will need to communicate, but you don't want the class to be tightly coupled.


Ordering a .txt file numerically by size and alphabetically I am doing a controlled assessment and one of the tasks is that we have to sort the saved pupils scores (numerically), and their names (alphabetically) in a .txt file. Any other improvements to the code would also be greatly appreciated.

import random
classcheck = False
while classcheck == False:
    classnumber = input("what class are you in")
    if classnumber == "1":
        print()
        classcheck = True
    elif classnumber == "2":
        print()
        classcheck = True
    elif classnumber == "3":
        print()
        classcheck = True
    else:
        print("that is not a valid class number")

student1name = input("please enter your name")
student1score = 0

for i in range(10):
    question1no1 = random.randint(1,20)
    question1no2 = random.randint(1,20)

    operators = ['+','-','*']
    op_number  = random.randint(0,2)
    op_sym = operators[op_number]
    op = op_number

    if op == 0:
        ans = question1no1 + question1no2
    elif op == 1:
        ans = question1no1 - question1no2
    elif op == 2:
        ans = question1no1 * question1no2

    print(str(question1no1), str(op_sym) , str(question1no2))

    question = "what is "+str(question1no1) + str(op_sym) +                                             str(question1no2)+"?"

    student1answer1 = input(question)

    print (student1answer1)

    if str(student1answer1) == str(ans):
        print("congrats you got the answer right")
        student1score = student1score + 1

    else:
        print("sorry you got the answer wrong")

print("your score was " + str(student1score) + " out of 10")

if classnumber == "1":
    class1score = open("class1score.txt", "a")
    class1score.write("\n" + student1name + (" ") + str(student1score))
    class1score.close()

if classnumber == "2":
    class2score = open("class2score.txt", "a")
    class2score.write("\n" + student1name + (" ") + str(student1score))
    class2score.close()

if classnumber == "3":

    class3score = open("class3score.txt", "a")
    class3score.write("\n" + student1name + (" ") + str(student1score))
    class3score.close()

My first suggestion would be to use some extra functions. However, let's look at it in several parts.

First, the getting of the class number. You have

classcheck = False
while classcheck == False:
    classnumber = input("what class are you in")
    if classnumber == "1":
        print()
        classcheck = True
    elif classnumber == "2":
        print()
        classcheck = True
    elif classnumber == "3":
        print()
        classcheck = True
    else:
        print("that is not a valid class number")

The if statements are quite the same. By using set-membership, we can reduce the duplicate code.

classcheck = False
while classcheck == False:
    classnumber = input("what class are you in")
    if classnumber in {"1", "2", "3"}:
        classcheck = True
    else:
        print("that is not a valid class number")

Furthermore, writing while foo == False: is not really idiomatic Python. Better would be while foo:. But, in this case you're trying to emulate a do {...} while (...) loop. I'd suggest writing it as follows:

while True:
    classnumber = input("what class are you in")
    if classnumber in {"1", "2", "3"}:
        break
    print("that is not a valid class number")

The asking of the name is quite obvious, let's move to the part where the questions get asked:

for i in range(10):

The variable i does not actually get used. It's a minor nitpick, but convention has it that you should write

for _ in range(10):

instead.

    question1no1 = random.randint(1,20)
    question1no2 = random.randint(1,20)

What is question1no1 referring to? This will also be executed for question 2 to 10. Maybe operand_left and operand_right would be better names? Leaving them be for now, but it's something you can ponder about.

Next, the creation of the 'puzzle'/question.

    operators = ['+','-','*']
    op_number  = random.randint(0,2)
    op_sym = operators[op_number]
    op = op_number

    if op == 0:
        ans = question1no1 + question1no2
    elif op == 1:
        ans = question1no1 - question1no2
    elif op == 2:
        ans = question1no1 * question1no2

There are a few lines between the definition of the operators, and the calculation of the desired result. First suggestion: use random.choice(operators) instead of random.randint(0, 2)

    operators = ['+','-','*']
    op_sym = random.choice(operators)

    if op_sym == '+':
        ans = question1no1 + question1no2
    elif op_sym == '-':
        ans = question1no1 - question1no2
    elif op_sym == '*':
        ans = question1no1 * question1no2

Already a lot clearer, no? Still, I've typed the + sign 3 times here. By using the operator module, I could write:

import operator
...
...

    operators = [
        ('+', operator.add),
        ('-', operator.sub),
        ('*', operator.mul),
    ]
    op_sym, op_func = random.choice(operators)
    ans = op_func(question1no1, question1no2)

Adding a new operator would be a simple method of adding another line in the list above.

I'll assume the explicit print statements are a bit of debugging work, and ignore those. Ideally you'd remove them.

Look at how you write the question.

    question = "what is "+str(question1no1) + str(op_sym) + str(question1no2)+"?"

There are so many things going on, it is a bit worry-some. Also, you might want to add a space around the operator, causing the need for yet some more work. By using string formatting (https://docs.python.org/3.5/library/stdtypes.html#str.format), you can make it a bit simpler:

    question = "what is {} {} {}?".format(question1no1, op_sym, question1no2)

Now, we get to student1answer1. Why not just answer? (Or given_answer, and rename ans to expected_answer).

As for checking the results:

    if str(student1answer1) == str(ans):
        print("congrats you got the answer right")
        student1score = student1score + 1

    else:
        print("sorry you got the answer wrong")

You can remove the empty line between the if and the else block. But more importantly, instead of

        student1score = student1score + 1

you can write

        student1score += 1

with the same effect.

At the end, we also see duplicated code regarding to the class numbers:

if classnumber == "1":
    class1score = open("class1score.txt", "a")
    class1score.write("\n" + student1name + (" ") + str(student1score))
    class1score.close()

if classnumber == "2":
    class2score = open("class2score.txt", "a")
    class2score.write("\n" + student1name + (" ") + str(student1score))
    class2score.close()

if classnumber == "3":

    class3score = open("class3score.txt", "a")
    class3score.write("\n" + student1name + (" ") + str(student1score))
    class3score.close()

It should be quite obvious that the only difference in these statements is the filename. "Easy" fix:

if classnumber == "1":
    filename = "class1score.txt"
if classnumber == "2":
    filename = "class2score.txt"
if classnumber == "3":
    filename = "class3score.txt"

class_score = open(filename, "a")
class_score.write("\n" + student1name + " " + str(student1score))
class_score.close()

But the if is now still very suspicious. Let's replace those 3 if-statements with 1 statement:

filename = "class" + classnumber + "score.txt"

or

filename = "class{}score.txt".format(classnumber)

(my preference is the second).

Finally: please use capitalization when writing strings for the end-user. So, instead of "what class are you in", write "What class are you in? ". (The final space is to make the question look even better when entering the data, but that's merely convention.)

After making the changes, I ended up with

import operator
import random

while True:
    classnumber = input("What class are you in? ")
    if classnumber in {"1", "2", "3"}:
        break
    print("That is not a valid class number.")

student1name = input("Please enter your name: ")
student1score = 0

for _ in range(10):
    question1no1 = random.randint(1,20)
    question1no2 = random.randint(1,20)

    operators = [
        ('+', operator.add),
        ('-', operator.sub),
        ('*', operator.mul),
    ]
    op_sym, op_func = random.choice(operators)
    ans = op_func(question1no1, question1no2)

    question = "What is {} {} {}? ".format(question1no1, op_sym, question1no2)

    student1answer1 = input(question)

    if str(student1answer1) == str(ans):
        print("Congrats, you got the answer right!")
        student1score += 1
    else:
        print("Sorry, you got the answer wrong.")

print("Your score was " + str(student1score) + " out of 10.")

filename = "class{}score.txt".format(classnumber)
class_score = open(filename, "a")
class_score.write("\n" + student1name + " " + str(student1score))
class_score.close()

Also, I'd like to change the last write to have the "\n" at the end instead of at the beginning, as it's convention to end a line with a "\n" instead of starting one with it. But that's up to you, as it would change the semantics of the script.


Vigenère cipher in Ruby What I'm trying to do: implement the Vigenère cipher in Ruby. I already have a working version, but I want to make sure it is efficient and well-designed.

module Crypto
  # Vigenère cipher encryption and decryption abstraction
  module Vigenere
    LETTERS = ('a'..'z').to_a.freeze
    private_constant :LETTERS

    module_function

    # Encrypts a string
    #
    # @param string [String] the string that will be encrypted
    # @param key [String] the key that will be used to encrypt the string
    #
    # @return [String] the encrypted string
    def encrypt(string:, key:)
      key = make_key(length: string.length, key: key)

      string.length.times.map { |i|
        p = LETTERS.find_index(string[i])
        k = LETTERS.find_index(key[i])

        LETTERS[(p + k) % 26]
      }.join
    end

    # Decrypts an encrypted string
    #
    # @param string [String] the encrypted string that will be decrypted
    # @param key [String] the key that will be used to decrypt the string
    #
    # @return [String] the decrypted string
    def decrypt(string:, key:)
      key = make_key(length: string.length, key: key)

      string.length.times.map { |i|
        c = LETTERS.find_index(string[i])
        k = LETTERS.find_index(key[i])

        LETTERS[(c - k + 26) % 26]
      }.join
    end

    # Repeats a word until it matches a certain length
    #
    # @param length [Integer] the length of the word being encrypted/decrypted
    # @param key [String] the word that will be repeated
    #
    # @return [String] the word in its new form
    def make_key(length:, key:)
      i = 0
      length.times do
        i = 0 if i == key.length
        break if key.length == length

        key << key[i]
        i += 1
      end

      key
    end

    private_class_method :make_key
  end
end

I do have some specific questions:

1. private_class_method

This is the best way I found to define a private method in a module, but it feels weird to me. Isn't there a better way to do that? My first implementation was this:

module Crypto
  class Vigenere
    class << self
      def encrypt # ...
      def decrypt # ...

      private

      def make_key # ...
    end
  end
end

which was fine for me. But then I read this rule on the Ruby Style Guide repository. So I switched to using module, but it doesn't feel right to use private methods in this structure. Am I wrong?

2. reseting a counter (index)

Take a look at this snippet of code:

def make_key(length:, key:)
  i = 0
  length.times do
    i = 0 if i == key.length
    break if key.length == length

    key << key[i]
    i += 1
  end

  key
end

Defining a counter (i) and manually incrementing it... looks awkward, doesn't it (at least in the Ruby world)? Is there a better way to do this?

3. valid multi-line block with curly braces?

Now take a look at this snippet:

string.length.times.map { |i|
  p = LETTERS.find_index(string[i])
  k = LETTERS.find_index(key[i])

  LETTERS[(p + k) % 26]
}.join

I know most Ruby developers tend to use curly braces only for one-line blocks and do-end for multi-line blocks, but this time it seems okay using curly braces with a multi-line block, since I'm chaining #join right after. What would you do:

1. use do-end, store it in a variable and invoke #join after that

new_letters = string.length.times.map do |i|
  p = LETTERS.find_index(string[i])
  k = LETTERS.find_index(key[i])

  LETTERS[(p + k) % 26]
end

new_letters.join

2. what I did (use curly braces even with multi-line block and chain #join)

And of course, if you have any other observations, please share.

The code is rather clear, so consider the following to be nitpicks.

I'd say that you are generating a new key stream from the key. I'd certainly not reuse the key variable.

The first i = 0 before the loop seems spurious.

Using i as a counter is well understood, and I'd not worry overly much on the style of it. You are probably the only one who cares if it is really Ruby-esk; developers down the line will understand it.

What I wonder though is that you run your loop length times, but there is a break that seems to trigger before that. That's not all too clear to me.

I wonder what happens if you supply it an "empty" key string. Some guard statements may be in order.

Same for the curly braces. It's clear as it is, choose whatever you want. Personally I slightly favor the braces.

You could consider creating a mod function, however since % is already the modulus, which will never return a negative value if the right value is positive, it seems to me that removing the + 26 is probably the only thing you need to change (during decryption).

Instead of using 26 as unexplained magic value, you should get the size of the LETTERS range instead. That way you can also expand your ciphertext later.

I've got no opinion on the private_class_method as I'm not a Ruby developer (I'm specialized in knowing many languages / constructs and of course applied crypto).


Monitoring directories for files that are generated by an engineering program I have a service that monitors failure and success directories for files that are generated by an engineering program. I want to check the change was in the success or failure folder, and if it was a new file creation. When those criteria are met I need to check if the file type matches one of the keys for a dictionary, and if it does I want to copy that file to a destination as declared in the config file.

I have an if statement that says:

if (NormalizePath(Path.GetDirectoryName(e.FullPath)) == NormalizePath(this.successFolder)
        && e.ChangeType == WatcherChangeTypes.Created)
{
    string fileExtension = Path.GetExtension(e.FullPath);
    try
    {
        //if the file type is desired copy to destinations.
        if( this.successDictionary.ContainsKey(fileExtension) )
        {
            CopyToDestination(this.successDictionary[fileExtension], e);
        }
    }
    catch (IOException ioe)
    {
        activeLog.WriteLineToLog("FolderMonitor IOException: " + ioe.Message);
    }
}
//if a file was added to the fail folder
else if (NormalizePath(Path.GetDirectoryName(e.FullPath)) == NormalizePath(this.failFolder)
            && e.ChangeType == WatcherChangeTypes.Created)
{
    activeLog.WriteLineToLog(e.FullPath + " has been added to the fail folder.");
    string fileExtension = Path.GetExtension(e.FullPath);
    try
    {
        //if the file type is desired copy to destinations.
        if (this.failDictionary.ContainsKey(fileExtension))
        {
            CopyToDestination(this.failDictionary[fileExtension], e);
        }
    }
    catch (IOException ioe)
    {
        activeLog.WriteLineToLog("FolderMonitor IOException: " + ioe.Message);
    }
}

Is it better to do it the first way or the second way?

string fileExtension = Path.GetExtension(e.FullPath);
if (NormalizePath(Path.GetDirectoryName(e.FullPath)) == NormalizePath(this.successFolder)
        && e.ChangeType == WatcherChangeTypes.Created)
{
    try
    {
        //if the file type is desired copy to destinations.
        if( this.successDictionary.ContainsKey(fileExtension) )
        {
            CopyToDestination(this.successDictionary[fileExtension], e);
        }
    }
    catch (IOException ioe)
    {
        activeLog.WriteLineToLog("FolderMonitor IOException: " + ioe.Message);
    }
}
//if a file was added to the fail folder
else if (NormalizePath(Path.GetDirectoryName(e.FullPath)) == NormalizePath(this.failFolder)
            && e.ChangeType == WatcherChangeTypes.Created)
{
    activeLog.WriteLineToLog(e.FullPath + " has been added to the fail folder.");
    try
    {
        //if the file type is desired copy to destinations.
        if (this.failDictionary.ContainsKey(fileExtension))
        {
            CopyToDestination(this.failDictionary[fileExtension], e);
        }
    }
    catch (IOException ioe)
    {
        activeLog.WriteLineToLog("FolderMonitor IOException: " + ioe.Message);
    }
}

It's really just a question of if it is better to sometimes waste a bit of memory, or if it is better to have it more optimized. I understand that in the grand scheme of things it's rather minimal how much memory it uses, but the voices in my head can't decide which one is better.

In this case, it would likely not make a difference. It is one call into the standard library that does a string operation that produces a short lived variable. If you want to know the correct answer, you should profile the code with real data to see what performs better.

If you don't care enough to profile it, do whatever lets you sleep better at night.

An optimization that you can do to speed up the code is change the if clauses.

In both cases, you execute NormalizePath(Path.GetDirectoryName(e.FullPath)).

This can be done once, then use the result in the string comparison of the if clause.
Perform the check of e.ChangeType first.

If the type doesn't match, you don't need do any path manipulation. Simply moving that check to the left side of the && instead of the right means the boolean expression can short circuit after a cheap operation before doing the expensive one. Also, since both cases are checking for the same type, you can pull the whole check out to an outer if block.
Do this.failFolder or this.successFolder change?

If not, you can store the normalized path as a instance variable and not perform the operation each time.

The IOException is being handled in the same way and (it appears that) CopyToDestination() is the only thing that would cause it. You reduce this down to one try catch instead of two.

In general, you shouldn't be commenting your if clauses. If the boolean expression is complicated enough that it needs to be described, it should be extracted into a function with a descriptive name. In theses cases, the check being performed is straight forward.

Throw that all together and you get something like this:

if (e.ChangeType == WatcherChangeTypes.Created)
{
    var normalizedPath = NormalizePath(Path.GetDirectoryName(e.FullPath));
    try
    {
        if (normalizedPath == this.normalizeSuccessFolder)
        {
            string fileExtension = Path.GetExtension(e.FullPath);
            if(this.successDictionary.ContainsKey(fileExtension))
            {
                CopyToDestination(this.successDictionary[fileExtension], e);
            }
        }
        else if (normalizedPath == this.normalizeFailFolder)
        {
            activeLog.WriteLineToLog(e.FullPath + " has been added to the fail folder.");
            string fileExtension = Path.GetExtension(e.FullPath);
            if (this.failDictionary.ContainsKey(fileExtension))
            {
                CopyToDestination(this.failDictionary[fileExtension], e);
            }
        }
    }
    catch (IOException ioe)
    {
        activeLog.WriteLineToLog("FolderMonitor IOException: " + ioe.Message);
    }
}

Edit:

Extracting common code into a method as pointed out by ChrisWue. There is still a repeated pattern there, but with just two cases, it isn't needed to abstract it more.

Had to make an assumption about the dictionary type and make up a name for the type of e. Even though this code doesn't care if the copy is attempted, I added a boolean return to indicate if it did. I also switched to using TryGetValue() instead of indexing into the dictionary twice.

private bool AttemptCopyToDestination(IDictionary<string,string> knownExtensions, SomeTypeName e)
{
    string fileExtension = Path.GetExtension(e.FullPath);
    string destination;
    if (knownExtensions.TryGetValue(fileExtension, out destination)
    {
        CopyToDestination(destination, e);
        return true;
    }
    return false;
}

// ...

if (e.ChangeType == WatcherChangeTypes.Created)
{
    var normalizedPath = NormalizePath(Path.GetDirectoryName(e.FullPath));
    try
    {
        if (normalizedPath == this.normalizeSuccessFolder)
        {
            AttemptCopyToDestination(this.successDictionary, e);
        }
        else if (normalizedPath == this.normalizeFailFolder)
        {
            activeLog.WriteLineToLog(e.FullPath + " has been added to the fail folder.");
            AttemptCopyToDestination(this.failDictionary, e);
        }
    }
    catch (IOException ioe)
    {
        activeLog.WriteLineToLog("FolderMonitor IOException: " + ioe.Message);
    }
}


Find common substring that starts a set of strings How would you in a pythonic manner write a function that returns the common substring found at the beginning of a set of strings? This is my ugly attempt:

def common_start(s):
   l = len(s)
   if l == 0: return None
   elif l == 1: return next(iter(s))
   else:
     i = iter(s)
     start = next(i)
     while start != "":
       if all(ii.startswith(start) for ii in i):
           return start
       else:
         i = iter(s)
         start = start[:-1]
         next(i) # were comparing againt the first item, so we don't need it
     else:
       return ""

s = set(("abc1","abcd","abc","abc abc"))
common_start(s) # "abc"

s = set(("ab","abcd","abc","abc abc"))
common_start(s) # "ab"

s = set(("ab",))
common_start(s) # "ab"

s = set(("","abcd","abc","abc abc"))
common_start(s) # ""

s = set()
common_start(s) # None

You should read PEP 8 and get a linter; pycodestyle, pylint, flake8, prospector, coala. It doesn't matter which one, just get one and use it.

Indenting with 3 spaces is awful, no-one can easily interact with your code.
I've never in my life seen 3 spaces for indentation, it makes me think your post is a joke.
Variables like s, l and  ii are useless.
It's really bad to put statements on the same line as other statements.

  if l == 0: return None

The amount of times I've just not seen the return None with lines like this isn't funny. It's nearly 100%.

Change your if, elif, else to guard statements and you can dedent your code.
There's no need for while else, you've not broken from the loop.
Returning None is just odd at best, change it to an empty string.
Taking a set as input is really strange, it causes you to have to do next(iter(s)). And you're not even exploiting the benefits of it.
Using iterators is clever, but not smart. You can just make a new list to contain all but the first value.

Additionally it doesn't really matter if you check against the first value because it's always going to be a subset of itself. Just seems like adding complexity for the sake of complexity.
You can remove the if l == 1 check.

def common_start(values):
    if len(values) != 0:
        start = values[0]
        while start:
            if all(value.startswith(start) for value in values):
                return start
            start = start[:-1]
    return ""

However this isn't the best. If there are \$s\$ values, and each have a length \$l\$ then your code runs in \$O(sl^2)\$ time.

Instead I'd suggest checking the first value against everything once - running in \$s\$ time. Since you do this \$l\$ times it runs in \$O(sl)\$ time.

To do so you can use zip to iterate through all the first characters, then second characters at the same time. From this you can check if they are the same and append them to a temporary list. Finally you return the contents of the list converted to one single string. In the end you would end up with something like salparadise created.

Again I would still make an empty list or set as input return an empty string.

def common(values):
    start = []
    for characters in zip(*values):
        if len(set(characters)) != 1:
            break
        start.append(characters[0])
    return ''.join(start)


C++ Interrupted Bubble Sort I've written an interrupted bubble sort, which prints the intermediate result after some specified number of iterations of bubble sort. But I seem to have an speed issue. Could anyone help me spot some places that need to be optimized or are just plain wrong?

#include <iostream>
#include <fstream>
#include <string>
#include <vector>
#include <sstream>

using std::cout;
using std::endl;
using std::string;
using std::vector;
using std::ifstream;
using std::stoi;
using std::stringstream;

template<typename T>
void printVector(vector<T> vect) {
    for(int i = 0; i < vect.size(); i++) {
        if(i == 0) {
            cout << vect[i];
        } else {
            cout << " " << vect[i];
        }
    }
    cout << endl;
}

template<typename T>
void swap(T &elementOne, T &elementTwo) {
    T temp = elementOne;
    elementOne = elementTwo;
    elementTwo = temp;
}

void bubbleSort(vector<int> numbers, long int iterations) {
    for(int i = 0; i < iterations; i++) {
        for(int j = 0; j < numbers.size(); j++) {
            if(numbers[j] > numbers[j + 1]) {
                swap(numbers[j], numbers[j + 1]);
            }
        }
    }
    printVector(numbers);
}

void parse(string line) {
    size_t delimiterPos = line.find("|");
    long int iterations = stol(line.substr(delimiterPos + 1));
    string numbersString = line.substr(0, delimiterPos - 1);

    stringstream numbersSS(numbersString);
    vector<int> numbers;
    int tempNum;
    while(numbersSS >> tempNum) {
        numbers.push_back(tempNum);
    }
    bubbleSort(numbers, iterations);
}

int main(int argc, const char * argv[]) {
    ifstream file(argv[1]);
    string line;
    while(getline(file, line)) {
        parse(line);
    }
    return 0;
}

Single Responsibility

As written, parse parses data and sorts them. This is plain wrong. Same goes for bubbleSort which sorts and prints. I recommend to change parse to

long int parse(string line, vector<int>& numbers);

and use it as in

vector<int> data;
while (getline(file, line)) {
    long int iterations = parse(line, data);
    bubbleSort(data, iterations);
    printVector(data);
}

Kudos for not using namespace std!
The code seems to implement interrupted bubble sort correctly. I don't really understand the expected outcome of the assignment so no comments on the speed.


Extract multiple attributes values in XML tags I have a XML file (test.xml) that can be sum up as follow (I filtered it so it will be more readable):

<coverage complexity="0" line-rate="0.66" lines-covered="66" lines-valid="100">
    <packages>
        <package complexity="0" line-rate="0.66" name=".">
            <classes>
                <class complexity="0" name="file_a.py" line-rate="0.7674">
                <class complexity="0" name="file_b.py" line-rate="0.2727">
                <class complexity="0" name="file_c.py" line-rate="1">
            </classes>
        </package>
    </packages>
</coverage>

For each line, I want to extract both name and line-rate info, for example output could be:

. 0.66
file_a.py 0.7674
file_b.py 0.2727
file_c.py 1

Note that I'll like to skip the 1st line since it as no name field.

Right now I managed to get that output with the following bash script:

#!/bin/bash

# Extract info in lines containing either "<package " or "<class "
linerates=`grep '<package \|<class ' test.xml | awk -F "line-rate=" '{print $2}' | awk -F '"' '{$
names=`grep '<package \|<class ' test.xml | awk -F "name=" '{print $2}' | awk -F '"' '{print $2}$

# Transform to array
linerates=(${linerates// / })
names=(${names// / })

# Print table
for i in "${!names[@]}"
do
        echo ${names[$i]} ${linerates[i]}
done

Since the code is quite ugly, I wonder if there is a way to extract those two informations in a more elegant way, let say in one command line / without the need to use a for loop

Edit

I switch to python and got this:

from bs4 import BeautifulSoup as bs

with open('test.xml', 'r') as file:
    content = file.readlines()
    content = "".join(content)
    bs_content = bs(content, 'lxml')

list_ = list(bs_content.find('classes').children)
list_ = list(filter(lambda a: a != '\n', list_))

for c in list_:
    print(c.get('name'), c.get('line-rate'))

The output is a bit reduced (but I'm OK with it)

file_a.py 0.7674
file_b.py 0.2727
file_c.py 1

I am still looking to do it using a single command line but for now I will go with the python version

Edit (following greybeard's comment)

I filtered my XML file to remove all unnecessary lines (none of them have attributes name nor line-rate). E.g of removed lines:

<lines>
     <line hits="1" number="1"/>
</lines>

Not much complications my file is generated so the attributes should always be in the same order. Coverage, package and class have more attributes. E.g. for "coverage" also has a timestamp and a version attributes ; "class" has a filename attribute which is the same as name

Feel free to ask if I forgot some other information

There are many issues with your posted example that prevent it from being sanely parsed by XML, including lack of closing tags and lack of a starting xml tag. You say that the content is generated: if you generated it, you should try to fix this. Anyway.

import re

pat = re.compile('name="(.*?)".*'
                 'line-rate="([0-9.]*)"')

with open('test.xml') as f:
    for line in f:
        match = pat.search(line)
        if match:
            print(match.expand(r'\1 \2'))

This makes many assumptions:

The attributes actually are in the same order every time (in your example, they aren't, despite you saying that they should be)
The file is guaranteed to have at most one line-rate/name pair per line
Those two attributes are always on the same line as each other

If all of those conditions are satisfied, the above works.

Actual XML

If (as it seems you suggested in a now-rolled-back edit) your input can actually be valid XML, then a method that will perform more validation is

from xml.sax import parse, ContentHandler

class Handler(ContentHandler):
    def startElement(self, name, attrs):
        name, rate = attrs.get('name'), attrs.get('line-rate')
        if name and rate:
            print(f'{name} {rate}')

parse('test.xml', Handler())

I do not know if it is faster, but it's probably a better idea to do this and halt-and-catch-fire if the XML is malformed, which SAX will do for you.


Draw crosshairs on the screen - ZedGraph Here's some part of my code is use to draw objects on the screen :

Class members:

private double cursorX = 0;
private double cursorY = 0;
private LineObj cursorOldY = new LineObj();
private LineObj cursorOldX = new LineObj();
private int index1 = 0;

private double cursor2X = 0;
private double cursor2Y = 0;
private LineObj cursor2OldY = new LineObj();
private LineObj cursor2OldX = new LineObj();
private int index2 = -1;

ZedGraph.CurveItem curve = null;

Drawing function :

private void drawCursors(bool first, bool second, bool fromKeyboard = false, Point mousePoint = new Point())
{
    GraphPane p = lineGraphControl1.GraphPane;
    if (p.CurveList.Count == 0 || curve == null)
        return;

    ZedGraph.Axis axis = curve.IsY2Axis ? (Axis)p.Y2AxisList[curve.YAxisIndex] : (Axis)p.YAxisList[curve.YAxisIndex];
    ZedGraph.PointPair point = new PointPair(0.0, 0.0);
    if (first)
    {
        p.GraphObjList.Remove(cursorOldY);
        p.GraphObjList.Remove(cursorOldX);
        if (fromKeyboard)
           point = curve.Points[index1];
        else if (lineGraphControl1.Graph.XAxis.Mode == XAxisMode.Parameter)
        {
            ZedGraph.CurveItem outputCurve;
            p.FindNearestPoint(mousePoint, curve, out outputCurve, out index1);
            if (index1 >= 0)
               point = curve.Points[index1];
        }
        else
        {
           if (!mousePoint.IsEmpty)
               p.ReverseTransform(mousePoint, out cursorX, out cursorY);
           point = findPoint(curve, cursorX);
        }
        cursorX = point.X;
        cursorY = transform(point.Y, axis);
        cursorOldY = new LineObj(cursorX, p.YAxis.Scale.Min, cursorX, p.YAxis.Scale.Max);
        cursorOldX = new LineObj(p.XAxis.Scale.Max, cursorY, p.XAxis.Scale.Min, cursorY);

       if (cursorX > p.XAxis.Scale.Min && cursorX < p.XAxis.Scale.Max)
           p.GraphObjList.Add(cursorOldY);

       if (cursorY > p.YAxis.Scale.Min && cursorY < p.YAxis.Scale.Max)
           p.GraphObjList.Add(cursorOldX);
   }
   if (second)
   {
       p.GraphObjList.Remove(cursor2OldY);
       p.GraphObjList.Remove(cursor2OldX);
       if (fromKeyboard)
           point = curve.Points[index2];
       else if (lineGraphControl1.Graph.XAxis.Mode == XAxisMode.Parameter)
       {
           ZedGraph.CurveItem outputCurve;
           p.FindNearestPoint(mousePoint, curve, out outputCurve, out index2);
           if (index2 >= 0)
               point = curve.Points[index2];
       }
       else
       {
           if (!mousePoint.IsEmpty)
               p.ReverseTransform(mousePoint, out cursor2X, out cursor2Y);
           point = findPoint(curve, cursor2X);
           if (index2 == -1)
           {
               index2 = (int) Math.Round(curve.NPts * 0.75, 0);
               point = curve.Points[index2];
           }
       }

       cursor2X = point.X;
       cursor2Y = transform(point.Y, axis);
       cursor2OldY = new LineObj(cursor2X, p.YAxis.Scale.Min, cursor2X, p.YAxis.Scale.Max);
       cursor2OldX = new LineObj(p.XAxis.Scale.Max, cursor2Y, p.XAxis.Scale.Min, cursor2Y);

       if (cursor2X > p.XAxis.Scale.Min && cursor2X < p.XAxis.Scale.Max)
           p.GraphObjList.Add(cursor2OldY);
       if (cursor2Y > p.YAxis.Scale.Min && cursor2Y < p.YAxis.Scale.Max)
           p.GraphObjList.Add(cursor2OldX);
    }
}

Is (structurally speaking) there some optimizations I missed ?

Mis-Matching curly braces are not a good thing, you do it all over the place, it makes your code hard to read

   if (fromKeyboard)
       point = curve.Points[index1];
    else if (lineGraphControl1.Graph.XAxis.Mode == XAxisMode.Parameter)
    {
        ZedGraph.CurveItem outputCurve;
        p.FindNearestPoint(mousePoint, curve, out outputCurve, out index1);
        if (index1 >= 0)
           point = curve.Points[index1];
    }
    else
    {

it should look like this

   if (fromKeyboard)
   {
       point = curve.Points[index1];
   }
   else if (lineGraphControl1.Graph.XAxis.Mode == XAxisMode.Parameter)
   {
        ZedGraph.CurveItem outputCurve;
        p.FindNearestPoint(mousePoint, curve, out outputCurve, out index1);
        if (index1 >= 0)
           point = curve.Points[index1];
   }
   else
   {

Indentation only on an if statement all by itself is fine, I personally don't like it, but when an else statement is added to the if statement, it starts to muddy the waters around your if statement and make it look sloppy.

The single, one-line if statements should also have a buffer zone after them as well.


Check Is Anagram Scala HashMap based implementation Here is the HashMap based implementation of the function checking whether two strings are anagrams:

Example:

isAnagram("SAVE", "VASE") // returns true
isAnagram("SAV",  "VASE") // returns false
isAnagram("SAVE", "VAS")  // returns false
isAnagram("SAVE", "VAEE") // returns false

My implementation:

def isAnagram(value1: String, value2: String): Boolean = {
  value1.length == value2.length && {
    val index = new mutable.HashMap[Char, Int]()

    def addToIndex(c: Char) = index(c) = index.getOrElse(c, 0) + 1

    value1.foreach(c => addToIndex(c))

    def decrementAndCheckInIndex(c: Char): Boolean = index.get(c) match {
      case None => false
      case Some(count) => index(c) = count - 1; true
    }

    value2.forall(decrementAndCheckInIndex) && index.forall { case (key, value) => value == 0 }
  }
}

Could you please provide feedback on this code. I am interested in the mutable data structure use here, is it ok? And in general, what can I make better?

It seems to me that groupBy() will do something very similar to your addToIndex(). The result is an immutable.Map[Char,String] instead of a mutable.HashMap[Char, Int], so the question is: which is better/easier for comparison purposes.

It turns out that the simple == evaluation is sufficiently deep to handle this.

def isAnagram(value1: String, value2: String): Boolean =
  value1.groupBy(identity) == value2.groupBy(identity)

Another, rather obvious, anagram test is simply:

value1.sorted == value2.sorted

I think, for long strings, grouping/indexing might be more efficient than sorting since each string is traversed only once. On the other hand, comparison for equality might be more complex for Maps than it would be for Strings.


Orange Tree simulator I'm practicing SOLID principles and the concept of making a clear distinction between an object's private and public interfaces.

I made the conscious choice to extract complex logic into private methods to hide the implementation details from the public interface. I did this because I heard that it's best for public methods to clearly indicate what they're doing so that others have an easier time figuring out what the program is doing.

I watched a talk by Ben Orenstein, during which he revealed that he does not test private methods very often. I don't know if this is a good idea or not, but I trust his practices. How do you all feel about this? For those of you who agree with him, under what circumstances DO you test your private methods? How do you decide it's important enough to test?

The specs:

require_relative '../lib/oranges'
require 'spec_helper'

describe OrangeTree do
  let(:orange_tree) { OrangeTree.new }

  it 'starts with an age of 0' do
    expect(orange_tree.age).to eq 0
  end

  it 'starts with an height of 0' do
    expect(orange_tree.height).to eq 0
  end

  it 'increases age by one when we call age!' do
    orange_tree.age!

    expect(orange_tree.age).to eq 1
  end

  it 'can be aged twice' do
    age_by_years(2)

    expect(orange_tree.age).to eq 2
  end

  it 'grows 2 feet each year' do
    orange_tree.age!

    expect(orange_tree.height).to eq 2
  end

  it 'dies after it reaches its lifespan' do
    age_by_years(15)

    expect(orange_tree.send(:dead?)).to eq true
  end

  it 'only has oranges' do
    expect(orange_tree.oranges).to eq []
  end

  it 'only grows oranges after reaches 3 years old' do
    assert_emptiness_when_young
    orange_tree.age!

    all_oranges?(orange_tree)
  end

  it 'can be picked' do
    age_by_years(3)
    orange_tree.oranges.pick!

    expect(orange_tree.oranges.count).to eq 2
  end

  private

  def all_oranges?(orange_tree)
    orange_tree.oranges.each do |orange|
      expect(orange).to be_an_instance_of(Orange)
    end
  end

  def assert_emptiness_when_young
    orange_tree.age!

    expect(orange_tree.oranges).to eq []

    orange_tree.age!

    expect(orange_tree.oranges).to eq []
  end

  def age_by_years(number_of_years)
    number_of_years.times { orange_tree.age! }
  end
end

The main program:

class Orange
end

class OrangeTree
  attr_reader :age, :height, :oranges

  LIFESPAN = 15
  FRUIT_BEARING_AGE = 3

  def initialize
    @age = 0
    @height = 0
    @oranges = []
  end

  def age!
    increase_age_by_one_year
    increase_height_by_two_feet
    add_three_oranges_to_tree if reached_age?(FRUIT_BEARING_AGE)
  end

  def dead?
    reached_age?(LIFESPAN)
  end

  private

  def reached_age?(requirement)
    age >= requirement
  end

  def increase_age_by_one_year
    @age += 1
  end

  def increase_height_by_two_feet
    @height += 2
  end

  def add_three_oranges_to_tree
    3.times { oranges << Orange.new }
  end
end

class Array
  def pick!
    pop
  end
end

Any suggestions?

In some sense, the question of whether or not to test private methods might be a better fit for Programmers.SE. I'll try to answer, but first a few notes on your code (since this is Code Review):

It seems the tree keeps growing and keeps adding oranges after its death, which doesn't seem quite right (though I'm sure the State of Florida would find it interesting!). A failing test for this could be:

it "stops growing once it's reached its lifespan" do
  age_by_years(15)

  expect {
    orange_tree.age!
  }.to_not change { orange_tree.height }
end

You should probably add a similar one to check that the number of oranges stops increasing too.
You might also ask, if you never pick any oranges, do they just stick around from year to year? I mean, there's ripe and then there's withered on the branch :)
Incidentally, the expect {...}.to change {...} test could be useful in a few places. If you want to check how much a value changed, you can add a by at the end, e.g.

it 'increases age by one when we call age!' do
  expect {
    orange_tree.age!
  }.to change { orange_tree.age }.by 1
end

I'm not a fan of monkey patching Array in this case. While it seems neat to call orange_tree.oranges.pick!, extending a core class to do so is going too far (especially as it's not even new functionality, just an alias). The semantics only make sense as longs as we're talking about an array of oranges, but an array is a generic object that can be used for anything.
You might have an array of OrangeTree objects - does it make sense that you can call orange_trees.pick!? Maybe, yeah, if you got a bunch of oranges back. But this is just a cutesy alias for pop.
I might just add a pick method to OrangeTree instead. Maybe give it an argument to say how many to pick.
The same way you have #dead? as a shortcut for calling #reached_age? with a certain value, I'd add a #fruit_bearing? method that works the same way. I don't know if I'd bother with the #reached_age?method, though, as it's so simple. It's actually shorter to write age >= ...
Your #increase_age_by_one_year, #increase_height_by_two_feet, and #add_three_oranges_to_tree methods are too specific in their naming. Sure, the names are accurate descriptions, but it's like having magic numbers in your method names. What if you decide that it should grow 3 feet per year? Or that the number of oranges to add is derived from the tree's age? Then the method names are no longer appropriate. I'd stick with just #increase_age, #increase_height, and #add_oranges.
As long as the increments are constant, though, it'd probably be good to use, well, constants, rather than hardcoded numbers.
The #add_three_oranges_to_tree method is also sort of redundantly named: What would it add oranges to, if not the tree?
Speaking of, while you can pick oranges, and thus decrease their number, the height can't decrease the same way. So there's no need to keep track of it in an instance variable. Instead, you could just do:

def height
  age * 2 # or, better yet, multiply by a named constant
end

Of course, there's still the question of what happens to the age and height, after the tree's death. You might say that a 100-year-old tree is indeed 100 years old in terms of age - it's just been dead for 85 years. But of course it shouldn't then be 200 feet tall.
Really minor thing, but if you intend for Orange to just be an empty class, you can make the extra clear by writing it in 1 line: class Orange; end
As for your tests: What is #assert_emptiness_when_young doing as a method? It's a test like all the others, not a helper. And you only use it in one place, so I don't see why it's separate. It's also potentially misleading: You create a tree, and call age! once. But what if the fruit-bearing age was 5? Ageing it once would no longer be a good test. I'd simply write:

it "has no oranges when it's young" do
  age_by_years(2)
  expect(orange_tree.oranges).to be_empty
end

Incidentally, it'd be nice to change that #age_by_years method. Age what by years? It'd be simpler to just have a method called tree_aged or tree_with_age that simply returns a tree with the given age.
That would let you simplify many of your tests, like the one above, to just:

  expect(tree_aged(2).oranges).to be_empty

Anyway, back to your questions:

Usually, no, I wouldn't test private methods. Tests are just code, and much like any other code that'll use your class, it shouldn't have to call private methods. That's why they're private. What's important is that the public interface works as expected. And if it does, that indicates that whatever the class is doing in private must work as expected too.

Same as how you don't need to poke around in a car's engine as long as it starts and gets you to your destination just fine.

However, if your tests of the public interface are failing, and you want to go debugging, you might add a test of a private method to help you fix it. Again, the car analogy: If the car doesn't work, your mechanic will want to look around in the engine.

Another case in which you might want to test private methods, is if your public interface is very simple but also very flexible and accepts all sorts of arguments (say, a great big hash of options). Checking every combination of arguments would take forever, and you still might miss some edge cases. But if the private methods each handle just one of those arguments, you can perhaps test those by themselves, and thus be reasonably satisfied that things will work with any combination of arguments. It's not a common situation though, since you'll generally want to avoid such complex "black box" constructions anyway.

So again, there isn't a great need to test private methods. You can mess around with the class' private methods, and re-run the public method tests, and it'll tell you if things work as expect. Because again, if the public interface (which is what other code will be using) works, who cares about how the private methods work? Even if you've used a private method test to help you fix a problem, the real payoff should still be that the public method tests now act as intended. So if anything, you should add a few more of those, and scrap the private one.

Take the proposed test above that verifies that the tree doesn't keep growing after its death. The test is still only using the public #height method/accessor. Since it's failing, you'll want to do something. You can add whatever logic you want to the public or private methods; when the tests pass, you know you've fixed that particular issue. No need to specifically test the private methods.

There's also just basic decoupling. Since private methods aren't meant for consumption, and should be able to change radically, as long as the public interface stays intact. For instance, since you're not testing private methods right now, you could rewrite the class as below, and your current tests would still pass just fine.

class OrangeTree
  LIFESPAN = 15
  FRUIT_BEARING_AGE = 3
  ORANGES_PER_YEAR = 3

  attr_reader :age, :height, :oranges

  def initialize
    @age = 0
    @oranges = []
  end

  def age!
    @age += 1
    add_oranges if fruit_bearing?
  end

  def dead?
    age >= LIFESPAN
  end

  def fruit_bearing?
    age >= FRUIT_BEARING_AGE
  end

  def height
    age * 2
  end

  private

  def add_oranges
    ORANGES_PER_YEAR.times { oranges << Orange.new }
  end
end

(note that this implementation still has the same issues as yours: It keeps growing and adding oranges even if it's dead.)


Ugly code unit testing with PHPUnit and Doctrine2 (an ORM) I am trying to unit test a small class that I consider part of the "business-logic" layer of my project.  It mostly just interact's with Doctrine's entity manager and a couple of entities with a many-to-many association.  I'm finding that unit testing it while mocking out its dependencies is just ludicrous.  Just the code to set up mocks is going to be at least twice as long as the class itself.  What am I doing wrong, or should I just not bother doing the mocks and let it interact with a real test database?

Here is the class I am trying to test:

class UserPermissions{

    protected $em;

    public function __construct($entityManager){
        $this->em = $entityManager;
    }

    /**
     * returns user's permissions for all products or specific product
     *
     **/
    public function getFromUUID($uuid, $productId=NULL){

        $user = $this->em->find('\MyProject\Entity\User',$uuid);
        $product = $productId ? $this->em->find('\MyProject\Entity\Product',$productId) : NULL;

        if ($user===null) {
            return null;
        } else {
            $userProducts = $user->getUserProducts();
            $result = [];

            if ($productId) {
                $criteria = Criteria::create()
                    ->where(Criteria::expr()->eq("product",$product))
                    ;
                $userProducts = $userProducts->matching($criteria);
            }

            for ($i = 0; $i < count($userProducts); $i++) {
                $productI = $userProducts[$i];
                array_push($result, array(
                    "product" => $productI->getProduct()->getId(),
                    "permissions" => $productI->getPermissions(),
                ));
            }

            return $result;
        }
    }
}

And here is what my test class is starting to look like:

class UserPermissionsTest extends PHPUnit_Framework_TestCase
{
    protected $testService;
    protected $mockProduct;
    protected $mockUser;
    protected $mockUserProduct;

    protected function setUp(){

        $em = $this->getMockBuilder('\Doctrine\ORM\EntityManager')
                   ->disableOriginalConstructor()
                             ->getMock();

        $emMap = array(
            array('\MyProject\Entity\User','a',$this->mockUser),
            array('\MyProject\Entity\Product','b',$this->mockProduct),
        );

        $em->expects($this->any())
           ->method('find')
             ->will($this->returnValueMap($emMap));

//      $userMockBuilder = $this->getMockBuilder('\MyProject\Entity\User')
//                              ->disableOriginalConstructor()

        $user = $this->getMockBuilder('\MyProject\Entity\User')
                     ->disableOriginalConstructor()
                                 ->getMock();

        $user->expects($this->any())
             ->method('getUserProducts')
                 ->will($this->returnValue($this->mockUserProduct);

        $this->mockUser=$user;

        $userProduct = $this->getMockBuilder('\MyProject\Entity\UserProduct')
                       ->disableOriginalConstructor()
                                   ->getMock();

        $userProduct->expects($this->any())
                    ->method('matching')
                        ->will($this->returnValue($this->mockProduct);

        $this->mockUserProduct = $userProduct;

        $product =$this->getMockBuilder('\MyProject\Entity\Product')
                     ->disableOriginalConstructor()
                                 ->getMock();

        $product->expects($this->any())
                    ->method('getProduct')
                        ->will($this->returnValue($this->mockProduct);

.... and so on.  It's obviously not done yet.

I decided that there was too much database access code in my UserPermissions class and split it into two classes.  The first class is now much easier to mock and the second class is just a wrapper for a doctrine query, which is probably almost pointless to mock.

class UserPermissions{

    protected $dbalUserProducts;

    public function __construct($dbalUserProducts){
        $this->dbalUserProducts= $dbalUserProducts;
    }

    /**
     * returns user's permissions for all products or specific product
     **/
    public function get($userId, $productId=NULL){

        $userProducts = $this->dbalUserProducts->get($userId, $productId);

        $result = [];

        for ($i = 0; $i < count($userProducts); $i++) {
            $up = $userProducts[$i];
            array_push($result, array(
                "product" => $up->getProduct()->getId(),
                "permissions" => $up->getPermissions(),
            ));
        }

        return $result;
    }
}

class UserProductQuery
{
    protected $em;

    public function __construct($entityManager)
    {
        $this->em = $entityManager;
    }

    public function get($userId=null, $productId=null){
        $qb = $this->em->createQueryBuilder();

        $qb->select('u')
           ->from('\MyProject\Entity\UserProduct', 'u');

        if ($userId) {
            $qb->andWhere($qb->expr()->eq('u.user', ':userId'));
            $qb->setParameter('userId',$userId);
        }
        if ($productId) {
            $qb->andWhere($qb->expr()->eq('u.product', ':productId'));
            $qb->setParameter('productId',$productId);
        }

        return($qb->getQuery()->getResult());
    }
}


Concurrent blocking Tasks in a BackgroundWorker I'm using a StaTaskScheduler (TPL extension) to open some web browser windows simultaneously and navigate to different sites (using WaitN, but it might as well just be a WebBrowser).

In my example below I'm running 5 concurrent browsers. Works fine, but it locked the UI thread, so I wrapped my tasks in a BackgroundWorker. Seems like it does the job, but is it the correct way of handling the issue? Do I need to dispose the worker once I'm done? And finally, is lock (ret) the correct way of keeping ret thread safe?

StaTaskScheduler sta = new StaTaskScheduler(5);
BackgroundWorker bw = new BackgroundWorker();

bw.DoWork += (s, ea) =>
{
    List<string> ret = new List<string>();

    lock (ret)

    Task.WaitAll(Util.GetList().Select(row => Task.Factory.StartNew(() =>
    {

        IEnumerable<string> result = DoUiLockingWork(row.Item1, row.Item2);
        ret.AddRange(result);

    }, CancellationToken.None, TaskCreationOptions.None, sta)).ToArray());

    ea.Result = ret;
};

bw.RunWorkerCompleted += (s, ea) =>
{
    List<string> result = ea.Result as List<string>;

    if (result != null)
        Debug.WriteLine(result.Count);
};

bw.RunWorkerAsync();

I would go with Task-based method (if you're using .Net 4.5) instead of the BackgroundWorker. That will sort out most of your problems.

Something in the vicinities of

async Task<List<T>> yourJobAsync(...)
{
    var results = await Task.Run ( () =>
    {
        //your job.
        return jobResults
    });
    return  yourJobAsyncResults
}


N-Layer using MEF dependency injection on MVC project I'm trying to create a "perfect" generic solution structure for MVC (5.2.3.0) projects using MEF.

Currently my solution looks like this.

Core layer contains generic classes that could be used in any project
Data layer contains a ApplicationContext (: DbContext) and models
Repositories layer contains Repository classes to access data
Service layer contains business rules
TreeOfMana is the presentation (MVC)
Dependencies is where I'm adding all interfaces (IUser, IRepository, IApplicationContext...)

They don't know each other

The layers "export" their DLLs on the Post Build event to the presentation bin folder.

copy "$(TargetFileName)" "$(SolutionDir)$(SolutionName)\bin"

This allows me to load assemblies and add to MEF

public static class MefConfig
{
    public static CompositionContainer Container { get; private set; }

    public static void Register()
    {
        var files = Directory.GetFiles(AppDomain.CurrentDomain.BaseDirectory, "TreeOfMana*.dll", SearchOption.AllDirectories)
            .Where(o => !o.Replace(AppDomain.CurrentDomain.BaseDirectory, "").Contains("obj\\"));

        var catalogAggregator = new AggregateCatalog();

        foreach (var file in files)
        {
            catalogAggregator.Catalogs.Add(new AssemblyCatalog(Assembly.LoadFrom(file)));
        }

        Container = new CompositionContainer(catalogAggregator);
    }
}

To wire-up with MVC I'm using a custom controller factory. For that first I need to export the controller:

public class ControllerExportAttribute : ExportAttribute
{
    public ControllerExportAttribute(Type concreteType)
        : base(concreteType.FullName, typeof(IController))
    {

    }
}

[ControllerExportAttribute(typeof(HomeController))]
[PartCreationPolicy(System.ComponentModel.Composition.CreationPolicy.NonShared)]
public class HomeController : Controller { }

By exporting as IController and having the contract name I can find it on the exports list.

My ControllerFactory is:

public class MefControllerFactory : DefaultControllerFactory
{
    private readonly CompositionContainer compositionContainer;

    public MefControllerFactory(CompositionContainer compositionContainer)
    {
        this.compositionContainer = compositionContainer;
    }

    protected override IController GetControllerInstance(System.Web.Routing.RequestContext requestContext, Type controllerType)
    {
        IController result = null;

        var export = compositionContainer.GetExports(typeof(IController), null, controllerType.FullName).SingleOrDefault();

        if (null != export)
            result = export.Value as IController;

        if (null != result)
            compositionContainer.SatisfyImportsOnce(result);

        return result;
    }
}

The controller factory finds the exported controller where the contract name ends with the controller name requested.

Question

I'm looking for advice on how to separate the layers, how to properly use MEF on an architecture like this.

Does this structure I created look decent?
Does it make sense?
Should I change something?
Am I going mad?

Security

MEF is a lightweight framework for discovering plugins at runtime. Be wary that plugins are loaded using the same security and evidence of the container code. Anyone deploying malicious TreeOfMana*.ddl in your output folder has the same access rights to the system as your application.

var files = Directory.GetFiles(
    AppDomain.CurrentDomain.BaseDirectory, "TreeOfMana*.dll",
        SearchOption.AllDirectories)
            .Where(o => !o.Replace(AppDomain.CurrentDomain.BaseDirectory,
                 "").Contains("obj\\"));

MAF comes with built-in functionality to provide separate and limited permissions for addins. But I'm not sure whether or to which degree this framework is still supported. If you keep using MEF, make sure no malicious code could get access to the system. Some considerations:

Assembly Security
Create a dedicated Application Domain for running plugins
You could create custom authentication with DLL license keys