Refactoring Java class with lots of constants class MyObject
{
    public static enum Type {A, B, C, D;}

    public static final int ID_MAIN = 1;
    public static final int ID_MAIN_UK = 2;
    public static final int ID_MAIN_US = 3;
    public static final int ID_SUB = 4;
    // lots more constants here

    public static final String DESCRIPTION_1 = "Desc Full Name";
    public static final String DESCRIPTION_2 = "Desc2 Full Name";
    // lots more constants here

    private int id;

    public MyObject(final int id)
    {
        this.id = id;
    }

    //simple getter
    public int getID() { return this.id;}

    // real responsibility of the class is in the following two methods
    public static String getDescription()
    {
         switch(id)
         {
              case MyObject.ID_MAIN:
              case MyObject.ID_MAIN_UK:
                  return MyObject.DESCRIPTION_1;
              case MyObject.ID_SUB:
                  return MyObject_Description_2;
              default:
                   // throw IllegalArgException
         }
    }

    public static Type getType(int id)
    {
         switch(id)
         {
             case MyObject.ID_MAIN:
             case MyObject.ID_SUB:
                 return Type.A;
             case MyObject.ID_MAIN_UK:
             case MyObject.ID_MAIN_US:
                 return Type.B;
             default:
                 return Type.Undefined;
         }
     }
 }

Basically, there is an ID that maps to both a description and a type. This ID is passed in during construction of the class and it should map to a set of constants already contained in the class. If the id is not part of the list of constants, an error is thrown when trying to get the description that maps to the id and an 'Unknown' type is return if the type is queried. The ID maps a description to a set of constants. The same ID maps to a certain Type (defined as an enum).

This code is pretty ugly because there are tons of constants defined at the top, which makes the switch statements pretty bloated. Is there a simple way to refactor this without changing the public interface? It seems trivially simple, but it seems pretty ugly no matter how you slice it. How can I simplify these mappings to make the code more concise?

I was thinking about representing the mappings in a text file and having a manager class that held simple containers in a hashmap. When the manager class is constructed, it would create the objects by reading the text file and map them to an ID. When the manager is queried with the ID, it would just call the corresponding get method, for instance:

class Manager
{
     private HashMap<int, MyObject> objectMap;

     public Manager() {} //construct the object map
     public String getDescription(int id) { return objectMap.get(id).getDescription();}
     public Type getType(int id) { return objectMap.get(id).getType();}
}

class DataContainer
{
     private String description;
     private Type type;

     public DataContainer(String desc, Type type) {//set mem vars}
     public String getDescription() //simple getter
     public Type getType() //simple getter
 }

But this solution seems too complicated. Is there a better solution, preferably one that would keep everything in one class?

This is a classic case for using an Enum....

public enum MyEnum {
    ID_MAIN("Desc Full Name", Type.A),
    ID_MAIN_UK("Desc Full Name", Type.B),
    ID_SUB("Desc1 Full Name", Type.A),
    .....

    private final String description;
    private final Type type;

    private MyEnum(String desc, Type type) {
        this.type = type;
        this.description = desc;
    }

    public int getID() {
        return ordinal() + 1;
    }

    public String getDescription() {
        return description;
    }

    public Type getType() {
        return type;
    }
}


A queue implemented using an array We've just finished learning about queues in my data structures class; I want to make sure I really understand it.  I'm still feeling a bit lost on how to optimize the data use of a queue. I've created a little program here:

#include <iostream>

class Queue{
public:
    Queue(int size = 10){
        ptr = new int[size];
        head = counter = 0;
        qSize = size;
    }

    void push(int value){
        if(counter == qSize)
            doubleSize();
        ptr[counter++] = value;
    }

    int pop(){
        if(counter == 0){
            std::cout << "The queue is empty!!! \n";
            return -1;
        }
        if(head >= counter - 1){
            int temp = ptr[head];
            std::cout << "This is the last item in the queue. \n";
            delete[] ptr;
            ptr = new int[qSize];
            head = counter = 0;
            return temp;
        }
        return ptr[head++];
    }
private:
    int *ptr;
    int head;
    int counter;
    int qSize;

    void doubleSize(){
        int prevSize = qSize;
        qSize *= 2;
        int *temp = new int[qSize];

        for(int i = 0; i < prevSize; i++){
            temp[i] = ptr[i];
        }
        delete[] ptr;
        ptr = temp;
    }
};

So, as you can test, the program runs fine; but what I'm looking for is how I can optimize my queues to be as efficient as possible. I.E. Is there a specific way that I should be programming these queues that uses the minimum amount of data, and has the smallest Order complexity?

P.S. I am aware that there is a queue library. I am more seeking to optimize how I program out queues themselves.

Rule of Zero

You should follow the rule of zero where class that deal with memory ownership should deal exclusively with memory ownership. Your class allocate raw memory but do not free it when being destroyed. It will leak memory. Also issue with copying your queue. You should use std::unique_ptr instead of raw new.

Reuse memory when queue is empty

Why do you need to free the underlying memory when the queue is empty? Can't you just set head to the beginning and reuse the memory? This will save you from having to allocate new memory.

Use STD algorithm

Use std::copy_n instead of for loop in doubleSize() to make your code more readable.


Sauna experiment - Celsius to Fahrenheit converter This is my program for a sauna. It is a Swedish sauna meant to only understand Celsius, but the user can only understand Fahrenheit, so when the user inputs Fahrenheit, it is converted to Celcius. Afterwards, the Celsius is rounded to an even value when presented to the user, to get rid of all the decimals from the conversion.

class Program
{
    static void tempCheck(double temperature)
    {
        if (temperature < 0)
        {
            Console.WriteLine("The sauna has now become a freezer, \nCelcius: {0}", Math.Round(temperature));
            Console.WriteLine();
        }
        else if (temperature < 73)
        {
            Console.WriteLine("This temperature is too low for sauna use, \nCelcius: {0}", Math.Round(temperature));
            Console.WriteLine();
        }
        else if (temperature > 100)
        {
            Console.WriteLine("You will be boiled alive if you use the sauna, \nCelcius: {0}", Math.Round(temperature));
            Console.WriteLine();
        }
        else if (temperature > 77)
        {
            Console.WriteLine("This heat will scald you, please lower the temperarture, \nCelcius: {0}", Math.Round(temperature));
            Console.WriteLine();
        }
        else if (temperature == 75)
        {
            Console.WriteLine("Perfect heat for sauna!");
            Console.WriteLine();
        }
        else
            Console.WriteLine("This temperature is adequate for sauna use! \nCelcius: {0}", Math.Round(temperature));

    }

    public static double FahrToCels(int fahr)
    {
        double fahrenheit = (5.0 / 9.0) * (fahr - 32);
        return fahrenheit;
    }

    static void Main(string[] args)
    {

        double temperature = 0;
        Console.WriteLine("Enter a desired temperature between 73 och 77 celcius");

        do
        {
            try
            {
                int fahrenheit = int.Parse(Console.ReadLine());
                Console.WriteLine(temperature);
                double celcius = FahrToCels(fahrenheit);
                Console.WriteLine(celcius);
                temperature = temperature + celcius;
                tempCheck(temperature);

            }
            catch (FormatException)
            {
                Console.WriteLine("Please only enter whole numbers, no decimals");
            }
            catch
            {
                Console.WriteLine("Error");
            }

        } while (temperature < 73 || temperature > 77);
    }
}
}

Review

Several issues have also been addressed in the comments below your question. A quick re-iteration:

You state the user only knows Fahrenheit, yet you ask them to input in Celcius: Enter a desired temperature between 73 och 77 celcius.
For each bad attempt, you add the new input to the previous input temperature = temperature + celcius;. This doesn't make any sense at all.
You should be able to read from code what the code is about. tempCheck(double temperature) doesn't tell us the unit of the input. Prefer celcius or a custom class Celcius.

tempCheck(double temperature)

This method has two very different responsibilities: (1) detect status by provided temperature (2) render status information to user. These should be two different methods to allow for reusability of the status check.

The status could be an enum:

enum TemperatureStatus
{
    Normal,   // 73°C - 77°C (but not 75°C)
    Perfect,  // 75°C
    Freezing, // < 0°C

    // and so on ..
}

Have tempCheck return the status:

static TemperatureStatus tempCheck(double celcius)
{
    if (celcius < 0) return TemperatureStatus.Freezing;

    // and so on ..

    return TemperatureStatus.Normal;
}

Rendering the status message can then go to its own method:

static string RenderStatus(TemperatureStatus status, double celcius)
{
    celcius = Math.Round(celcius);
    switch (status)
    {
        case TemperatureStatus.Freezing:
            return "The sauna has now become a freezer, \nCelcius: {celcius}";
        // and so on ..
    }
}


Specialized ConcurrentLinkedList I need a concurrent data structure with the following properties:

Low overhead for enumeration
Insert only at end.
Removal from any point.
Enumeration should be safe with concurrent writing
Enumeration does not require the list to be in a constant state
Not a problem if I enumerate over removed items

I have designed the following linked list class which should meet my requirements.

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

internal class ConcurrentLinkedList<T> : IEnumerable<Node<T>> {
    private readonly BaseNode<T> _root;
    private BaseNode<T> _end;
    public ConcurrentLinkedList() {
        _end = _root = new BaseNode<T>();
    }

    public Node<T> Insert(T value) {
        var node = new Node<T>(value);
        lock (node) {
            lock (_end) {
                node.Previous = _end;
                _end = _end.Next = node;
                return node;
            }
        }
    }

    public void Remove(Node<T> node) {
        lock (node.Previous) {
            lock (node) {
                if (node.Next == null) {
                    if (node.Previous.Next != node) return;
                    node.Previous.Next = null;
                    if (_end == node) _end = node.Previous;
                } else {
                    lock (node.Next) {
                        if (node.Previous.Next == node) node.Previous.Next = node.Next;
                        if (node.Next.Previous == node) node.Next.Previous = node.Previous;
                    }
                }
            }
        }
    }

    public IEnumerator<Node<T>> GetEnumerator() {
        for (var current = _root.Next; current != null; current = current.Next) {
            yield return current;
        }
    }

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

internal class BaseNode<T> {
    internal volatile Node<T> Next;
}

internal class Node<T> : BaseNode<T> {
    internal BaseNode<T> Previous;

    public T Value { get; }

    public Node(T value) {
        Value = value;
    }
}

I am aware the calls is vary easy to misuse but as it is only for internal use i am not concerned about this. I am considering making the all the class private subclass to ensure that there is no accidental misuse.

In the code all the writing is in a lock but there are shared volatile which are read out of lock. Apart from the possibility of enumerating deleted nodes, is there any other danger with this?

For the locking, I lock no all the node being changed. This should allow other parts of the list to be concurrency modified, the list can get long. Is this sufficient?

To avoid deadlock I always lock from the lowest node to the last.

Is the provided locking sufficient to protect the _end field? I believe that according to the logic used the current _end will always be locked when changing the value of _end. Is this sufficient?

Is this code trying to be too clever and would it better to either:

have one write lock and continue using the volatile for reading.
use a standard ReadWriteLock for the whole list.

EDIT: Possible race condition

I would not accept this code to production without proper documentation containing good proof, that it works as expected. lock (node.Previous) could get interrupted by another Remove changing node.Previous after it was passed to Monitor.Enter which smells too much (it will lock removed node instead of the one it should). Changing the order of locks may introduce dead-lock (starvation).

Let us start by Insert(1); Insert(2); Insert(3) and then call Remove from three different threads. 3rd thread will call lock (node.Previous) which contains two parts - first fetching node.Previous and then calling Monitor.Enter. Let us suspend this Remove(3) in between - fetching Node(2), but not yet calling Monitor.Enter (or suspend inside, but before it can do anything). Let us now finish Remove(2), which will change Node(3).Previous = Node(1). Let us continue 3rd thread, which will lock removed Node(2) (instead of Node(1)) and Node(3) reaching line node.Previous.Next = null. It won't crash here because lock introduces memory barrier (thus node.Previous will trully read Node(1), not the locked Node(2)). Let us suspend 3rd thread here and start 1st thread calling Remove(1), locking _root, Node(1) passing if (node.Next == null) going to else lock (node.Next). Let us now suspend this thread and continue in 3rd thread that was about to do node.Previous.Next = null. Compiler will probably cache node.Next in 1st thread, but what if it does not? lock (null). We could rewrite it (var next = node.Next; if (next == null) { ... } else lock (next)), now 1st thread will block until 3rd finishes, but is still left with node.Next = null possibly crashing in if (node.Next.Prevous == node) and even if we change that to if (next.Previous == node) next.Previous = ... it changes state of removed node which you could try to remove again... and it is all looking like not worth the effort.

Single ReaderWriteLockSlim is safe to use and the performance will still probably be good enough. Using lock only for writing (Insert, Remove) and leaving GetEnumerator without any locking may still be possible - read my original answer (3rd point).

Original answer

I will leave my original answer here as there may still be some valuable information, but I do not trust the locking now. It won't dead-lock and although I cannot proove race-condition corrupting the state (not one that could not be fixed), I myself will never use such collection, unless somebody can describe unbreakable proof that it is safe.

My reasoning why the locking is safe

All the locks are taken in same order (from root to end, forward direction), which means removal of newer nodes can block threads that are currently trying to remove older node, but there is no loop for possible starvation - the thread removing newer node will block the other temporarily, do its job and then the other can continue. There is small incosistency in Insert where you first lock the node that will become the last and then lock the last node (which is reversed order of locking), but I believe you don't even have to lock the inserted node, because it won't be visible until _end = _end.Next = node;
You always lock _end before changing it (the node _end points to). You also hold the lock of the node that will become _end after the operation (_end.Previous if removing, not necessary in Insert because nobody can see the node until you publish it by _end.Next = node and only GetEnumerator() can see it, Remove would need to lock _end first, which it cannot, until you finish inserting).
GetEnumerator does not need any lock, because removed nodes retain their Next (which is internal - some private nested interface could maybe protect it even better a bit like friend in C++, but internal appears to be sufficient for your use-case). That means that even if you remove the node that is current in GetEnumerator, you can still safely iterate. Nobody can reinsert the node or modify Next. current.Next is always node that at least existed when you started iterating (as long as pointer assignment is atomic).

Lines I deem unnecessary / redundant

lock (node) inside Insert - already described, lock (_end) is enough (and the order should be lock (_end) lock (node) to preserve the order of locking - forward).
if (node.Previous.Next != node) return; inside Remove. How could that happen? Not in normal usage, but if we consider that you could possibly call Remove multiple times for the same node. In that case, I would mark removed nodes by Previous = null and to make Remove not corrupt the state if you pass node from different list, I would change _root + _end to single doubly-linked node (add Previous to BaseNode) and make the list hybrid - null-terminated in forward direction and root-terminated in backwards direction (cyclic, starting with _root.Previous = _root). That can be implemented in such a way, that Remove could be static (not that it should, but could - does not need to access _root, all it needs is the node). That means that Remove could safely remove nodes from any list.

Style / design / naming

Node is too generic for global space (and you should use namespace). I would make it public nested (I hope you can still make the list implement IEnumerable<Node>), have Next and Previous private, add private interface to Node with Insert and Remove that will be called by the list (and Insert only as Insert => _root.Insert(value) and Remove => ((INode)node).Remove(), while having INode _root as well as Next and Previous) to protect everybody including yourself from modifying Next/Previous in unacceptable way.

P.S.: Having Next typed as Node (as you currently have) may be better for performance (needing ((INode)_root).Insert(value) which is safe cast, compiler knows it is valid). The originally proposed version (having INode-typed Next) would need casting in GetEnumerator that would have to be runtime-checked.


Performing a procedure for each form edit I am a bit new to programming. I have some edits on my form, and for each edit completed it will have to do a procedure.

Currently, I use this code that works perfectly, but I am looking for a better way to accomplish this task. If I add another edit not have to repeat all the code.

string rgxExpr = @"(\w+)\s([\w\s]*)"; // Pega primeiro nome
string rgxExpr2 = @"\b(\w{1})"; // Abreviação
string s1, s2, s3, s4,s5;

/* EDIT 1 */
Match match1 = Regex.Match(edtAutor1.Text, rgxExpr);
Match match2 = Regex.Match(match1.Groups[1].ToString(), rgxExpr2);
s1 = match1.Groups[2].ToString().ToUpper() + ", " + match2.Groups[1].ToString().ToUpper();
// Apenas um autor
s4 = match1.Groups[2].ToString().ToUpper() + ", " + match1.Groups[1].ToString().ToUpper();
/* EDIT 2 */
Match match11 = Regex.Match(edtAutor2.Text, rgxExpr);
Match match22 = Regex.Match(match11.Groups[1].ToString(), rgxExpr2);
s2 = match11.Groups[2].ToString().ToUpper() + ", " + match22.Groups[1].ToString().ToUpper();
/* EDIT 3 */
Match match111 = Regex.Match(edtAutor3.Text, rgxExpr);
Match match222 = Regex.Match(match111.Groups[1].ToString(), rgxExpr2);
s3 = match111.Groups[2].ToString().ToUpper() + ", " + match222.Groups[1].ToString().ToUpper();
/* EDIT 4 */
Match match1111 = Regex.Match(edtAutor4.Text, rgxExpr);
Match match2222 = Regex.Match(match1111.Groups[1].ToString(), rgxExpr2);
s5 = match1111.Groups[2].ToString().ToUpper() + ", " + match2222.Groups[1].ToString().ToUpper();

if (radioButton1.Checked == true)
{
    if (edtAutor1.Text != "")
    {
        /* Bibliográfia - 1 AUTOR */
        if (string.IsNullOrWhiteSpace(edtAutor2.Text) && string.IsNullOrWhiteSpace(edtAutor3.Text))
        {
            addbb.dataGridView1.Rows.Add(s4+". ", abntrTitulo.Text+"." , abntrEdicao.Text +" Ed. ", abntrLocal.Text + ": ", abntrEditor.Text + ", ",
            abntrAno.Text + ". ", abntrPaginas.Text +"p.");
        }

        /* Bibliográfia - 2 AUTORES */
        if (string.IsNullOrWhiteSpace(edtAutor3.Text))
        {
            if (edtAutor2.Text != "")
            {
                addbb.dataGridView1.Rows.Add(s1 + "; " + s2 + ". ", abntrTitulo.Text + ".", abntrEdicao.Text + " Ed. ", abntrLocal.Text + ": ", abntrEditor.Text + ", ",
            abntrAno.Text + ". ", abntrPaginas.Text + "p.");
            }
        }

        /* Bibliográfia - 3 AUTORES */
        if (string.IsNullOrWhiteSpace(edtAutor4.Text))
        {
            if (edtAutor1.Text != "" & edtAutor2.Text != "" & edtAutor3.Text != "")
            {
                addbb.dataGridView1.Rows.Add(s1 + "; " + s2 + "; " + s3 + ". ", abntrTitulo.Text + ".", abntrEdicao.Text + " Ed. ", abntrLocal.Text + ": ", abntrEditor.Text + ", ",
            abntrAno.Text + ". ", abntrPaginas.Text + "p.");
            }
        }

        /* Bibliográfia - 4 AUTORES */
        if (edtAutor1.Text != "" & edtAutor2.Text != "" & edtAutor3.Text != "" & edtAutor4.Text != "")
        {
            addbb.dataGridView1.Rows.Add(s1 + "; " + s2 + "; " + s3 + ";" + s5 + ". ", abntrTitulo.Text + ".", abntrEdicao.Text + " Ed. ", abntrLocal.Text + ": ", abntrEditor.Text + ", ",
            abntrAno.Text + ". ", abntrPaginas.Text + "p.");
        }
    }
    else
    {
        MessageBox.Show("Você precisa preencher todos os campos obrigátorios (*). Por favor, verifique se todos os campos estão preenchidos corretamente!");
    }

}

The code uses regex and separates the words of EDIT, reverses the order and abbreviates.

It looks to me that you could use a method that returns the appropriate string.  Something like the following, which follows the pattern in your code:

static string GetEdit(string txt)
{
    string rgxExpr = @"(\w+)\s([\w\s]*)"; // Pega primeiro nome
    string rgxExpr2 = @"\b(\w{1})"; // Abreviação
    Match match1 = Regex.Match(txt, rgxExpr);
    Match match2 = Regex.Match(match1.Groups[1].ToString(), rgxExpr2);
    return match1.Groups[2].ToString().ToUpper() + ", " + match2.Groups[1].ToString().ToUpper();
}

You could do something similar with the other duplicated code by using a list of strings that correspond to a list of textboxes. allowing you to add or delete as needed.


Ordering filtered results I have the following method which under code review gives me a CA1502 avoid excessive complexity error (although if I suppress the message it will build fine):

private void OrderResults()
{
    switch (_orderColumn)
    {
        case 0:
        default:
            _fitleredResults = _orderDirection.Equals("desc", StringComparison.InvariantCultureIgnoreCase) ? _fitleredResults.OrderByDescending(x => x.Street) : _fitleredResults.OrderBy(x => x.Street);
            break;

        case 1:
            _fitleredResults = _orderDirection.Equals("desc", StringComparison.InvariantCultureIgnoreCase) ? _fitleredResults.OrderByDescending(x => x.Street) : _fitleredResults.OrderBy(x => x.Street);
            break;

        case 2:
            _fitleredResults = _orderDirection.Equals("desc", StringComparison.InvariantCultureIgnoreCase) ? _fitleredResults.OrderByDescending(x => x.City) : _fitleredResults.OrderBy(x => x.City);
            break;

        case 3:
            _fitleredResults = _orderDirection.Equals("desc", StringComparison.InvariantCultureIgnoreCase) ? _fitleredResults.OrderByDescending(x => x.Country) : _fitleredResults.OrderBy(x => x.Country);
            break;

        case 4:
            _fitleredResults = _orderDirection.Equals("desc", StringComparison.InvariantCultureIgnoreCase) ? _fitleredResults.OrderByDescending(x => x.RecID) : _fitleredResults.OrderBy(x => x.RecID);
            break;
    }
}

Explanation of variables:

_filteredResults is an IEnumerable<AddressObject>;
_orderColumn is an int passed in by jQuery DataTables and doesn't match the property indexes (which is why I was mapping it using a switch)

Address object class:

public class AddressObject
{
    public string City { get; set; }
    public string Country { get; set; }
    public string CountryRegionID { get; set; }
    public string Postcode { get; set; }
    public long RecID { get; set; }
    public string Street { get; set; }
}

Is there a way to make this code less complex?

Firstly, your case 1 case is identical to your case 0,default case (order by street). Is that intentional? It looks like a mistake to me because you've cascaded 0 and default but not 1. If it isn't a mistake, and they do all have the same behaviour you should cascade them all or let them all be picked up by the default case.

I normally see default as the last option in a switch statement, I feel like that might be a convention but don't have anything to back it up.

You're repeating this code a lot: _orderDirection.Equals("desc", StringComparison.InvariantCultureIgnoreCase) put it in a variable (which will make the code more readable.

So far:

private void OrderResults()
{
    var isDescending = _orderDirection.Equals("desc", StringComparison.InvariantCultureIgnoreCase);
    switch (_orderColumn)
    {
        case 2:
            _fitleredResults = isDescending ? _fitleredResults.OrderByDescending(x => x.City) : _fitleredResults.OrderBy(x => x.City);
            break;
        case 3:
            _fitleredResults = isDescending ? _fitleredResults.OrderByDescending(x => x.Country) : _fitleredResults.OrderBy(x => x.Country);
            break;
        case 4:
            _fitleredResults = isDescending ? _fitleredResults.OrderByDescending(x => x.RecID) : _fitleredResults.OrderBy(x => x.RecID);
            break;
        default:
            _fitleredResults = isDescending ? _fitleredResults.OrderByDescending(x => x.Street) : _fitleredResults.OrderBy(x => x.Street);
            break;
    }
}

As Heslsacher says, you could further improve by using named constants for the indexes.

I did write some additional stuff about using a method to get a Func but then I realised that RectId is a long so it all went out the window.

Edit

For posterity, I'll add in what I had written:

private void OrderResults()
{
    var isDescending = _orderDirection.Equals("desc", StringComparison.InvariantCultureIgnoreCase);
    var sortProperty = GetSortProperty();
    if (isDescending)
    {
        _filteredResults = _filteredResults.OrderByDescending(sortProperty);
    }
    else
    {
        _filteredResults = _filteredResults.OrderBy(sortProperty);
    }
}

private Func<AddressObject, object> GetSortProperty()
{
    switch(_orderColumn)
    {
        case 2:
            return x => x.City;
        case 3:
            return x => x.Country;
        case 4:
            return x => x.RectId;
        default:
            return x => x.Street;
    }
}


Finding difference between adjacent integers in an array - recursive Implementation I'm trying to do a recursive implementation of the problem mentioned below.

The code works, and takes care of a corner case with only 1 element in array (in which case I should return 0 - (element[]). Could there be improvements to this solution (like say not having to pass 4 arguments or anything like similar).

Also, I don't like that count variable because its used only for 1 purpose (finding that corner case of only 1 element in original array). How can I bypass that?

(This is not a homework problem; I'm trying to learn recursion.)

  Examples

  input Array: {1, 2, 3, 4, 5} Difference output: {1, 1, 1, 1}

  input Array: {2} Difference output: {-2}

  input Array: {2, 5, 1} Difference output: {3, -4}

void differenceSequence(int array[], int n, int count, int resultArray[]) {

if (n == 0 && count == 0) {
    /* Only 1 element in array */
    resultArray[n] = -1 * array[n];
    return;
}
if(n == 0) {
    /* second base case where size will be 0, and array recursion should end */
    return;
}
resultArray[n-1] = array[n] - array[n-1];
differenceSequence(array, n-1, ++count, resultArray);
}

int main() {

int *array = (int*) malloc (sizeof(int) * ARRAY_SIZE);
int *resultArray= (int*) malloc (sizeof(int) * ARRAY_SIZE-1);
int i = 0, size = ARRAY_SIZE;

// array = (int[ARRAY_SIZE]){5, 4, 3, -1, 0};
array = (int[ARRAY_SIZE]){1};

differenceSequence(array, ARRAY_SIZE-1, 0, resultArray);

/* Print Arrays */
printf("Original Array \n");
for (i = 0; i < size; i++) {
    printf("%d \t", array[i]);
}
printf("\n");

/* Check for result array where there is only 1 element in original
 * array
 */
if(ARRAY_SIZE == 1) {
    size = ARRAY_SIZE;
} else {
    size = ARRAY_SIZE - 1;
}

printf("Difference Array \n");
for (i = 0; i < size; i++) {
    printf("%d \t", resultArray[i]);
}
printf("\n");

return 0;
}

In situations like this, it is much simpler to handle the corner case outside the recursive call:

    differenceSequence(int array[], int size, int resultArray[])
    {
        if (size == 0) {
            do_corner_case();
            return;
        }
        differenceSequenceRecursive(array, size, resultArray);
    }

and the helper differenceSequenceRecursive function only deals with regular case.


My first random password generator I'm making a simple program that generates a random password of some length with or without special characters, just for the sake of learning the C language. Finally I've got this working very well based on the outputs below:

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

char *generate_random_password(int password_lenght, int has_special_characters)
{
    const char *letters = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";
    const char *digits = "0123456789";
    const char *special_characters = "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~";

    char *random_password = malloc(sizeof(char) * (password_lenght+1));

    srandom(time(NULL));

    if(has_special_characters)
    {
        char to_be_used[95] = "\0";

        strcat(to_be_used, letters);
        strcat(to_be_used, digits);
        strcat(to_be_used, special_characters);

        for(int i = 0; i < password_lenght; i++)
        {
            const int random_index = random() % strlen(to_be_used);
            const char random_character = to_be_used[random_index];

            random_password[i] = random_character;
        }
    }
    else
    {
        char to_be_used[63] = "\0";

        strcat(to_be_used, letters);
        strcat(to_be_used, digits);

        for(int i = 0; i < password_lenght; i++)
        {
            const int random_index = random() % strlen(to_be_used);
            const char random_character = to_be_used[random_index];

            random_password[i] = random_character;
        }
    }

        return random_password;

        free(random_password);
}

int main(void)
{
    printf("%s\n", generate_random_password(17, 1));
    printf("%s\n", generate_random_password(17, 0));

    return 0;
}

The output is:

|ZzN>^5}8:i-P8197

vPrbfzBEGzmSdaPPP

It's working!

But I'm completely in doubt about these strings, pointers, char arrays, etc. I have no idea if this is written "the right way" or how it could be better. I'm concerned if I allocated the right amount for each string/char array, and if it can break or crash in some future.

PS: I'm new at C programming, that's why I don't know much about pointers and memory management yet.

If can anyone give me some feedback about it I will be very grateful!

Typo

lenght is spelled length.

Magic numbers

What does 95 signify? You'll want to put this in a named #define or a const.

Allocation failure

After calling malloc, always check that you've been given a non-null pointer. Allocation failure does happen in real life.

Indentation

You'll want to run this through an autoformatter, because your if block has wonky indentation and needs more columns to the right.

Inaccessible statement

    return random_password;

    free(random_password);

This free will never be called; delete it.

Random

The larger conceptual problem with this program is that it uses a very cryptographically weak pseudorandom number generator. This is a large and fairly complex topic, so you'll need to do some reading, but asking for random data from an entropy-controlled system source will already be better than using C rand.

That aside: you aren't calling rand, you're calling random:

  The random() function uses a nonlinear additive feedback random number generator employing a default table of size 31 long integers to return successive pseudo-random numbers in the range from 0 to RAND_MAX. The period of this random number generator is very large, approximately 16 * ((2^31) - 1).

It's probably not appropriate for cryptographic purposes. Have a read through this:

https://stackoverflow.com/questions/822323/how-to-generate-a-random-int-in-c/39475626#39475626


Combining several collections into one Is there a way to rewrite this code to make it more laconic? I'm using Java 6.

Object value = methodOutOfMyControl();
Collection<LinkedHashSet<String>> values = ((Map) value).values();
Set<String> strings = new HashSet<String>();
for (LinkedHashSet<String> set : values) {
    strings.addAll(set);
}

I feel that sometimes inlining some values makes things a little more readable. But in terms of overall compactness, this isn't that much better. Here is what I would probably write

Map value = (Map)methodOutOfMyControl();
Set<String> strings = new HashSet<String>();
for (Map.Entry<Object, LinkedHashSet<String>> set : value.entrySet()) {
    strings.addAll(set.getValue());
}


How can I make this Euler/RK4 implementation more elegant? One of the things that I'm doing to teach myself is converting some numerical methods from existing Python code (they seem to me to lend themselves to functional programming quite well).

I'd like to know how I can improve the general style of my code.
In particular, I feel like I'm overusing brackets, that I should be able to use more inbuilt functions (that I'm not aware of), and that I may be misusing the let statement.

Background

A quick refresher on the terminology I'm using (slightly different to wiki*):

Given a state vector \$\mathbf{x}\$, and a function vector \$\mathbf{f}(\mathbf{x})\$ (which is our system of first order differential equations), we want to iterate using step size \$h\$ until a given target \$t(\mathbf{x})\$ is reached.

*Different in that I don't make the useless distinction between dependent and independent variables that some texts, and wiki, use.

Euler Method

For this method, we use a very simple approximation:

$$
\mathbf{k_1} = \mathbf{f}(\mathbf{x}) \\
\mathbf{x}_{new} = \mathbf{x} + h*\mathbf{k_1}
$$

In the code, I use what is basically a 'reverse map' (ie, apply a list of functions to a single argument) that I found online:

pam :: [a -> b] -> a -> [b]
pam f x = map g f
    where g h = h x

And then we define the euler method itself:

euler :: Fractional a => [([a] -> a)] -> ([a] -> Bool) -> a -> [a] -> [a]
euler fs target h xs
    | target xs = xs
    | otherwise = let k1 = pam fs xs
                      xn = (+.) xs (map (h *) k1)
                  in euler fs target h xn
                  where (+.) = zipWith (+)

RK4 (4th order Runge-Kutta)

For this method, the approximation is a bit more complex:

$$
\mathbf{k_1} = \mathbf{f}(\mathbf{x}) \\
\mathbf{k_2} = \mathbf{f}(\mathbf{x} + \frac{h}{2}\mathbf{k_1}) \\
\mathbf{k_3} = \mathbf{f}(\mathbf{x} + \frac{h}{2}\mathbf{k_2}) \\
\mathbf{k_4} = \mathbf{f}(\mathbf{x} + h*\mathbf{k_3}) \\
\mathbf{x_{new}} = \mathbf{x} + \frac{h}{6}(\mathbf{k_1} + 2*\mathbf{k_2} + 2*\mathbf{k_3} + \mathbf{k_4})
$$

And the code I came up with:

rk4 :: Fractional a => [([a] -> a)] -> ([a] -> Bool) -> a -> [a] -> [a]
rk4 fs target h xs
    | target xs = xs
    | otherwise = let k1 = pam fs xs
                      k2 = pam fs ((+.) xs (map (h / 2 *) k1))
                      k3 = pam fs ((+.) xs (map (h / 2 *) k2))
                      k4 = pam fs ((+.) xs (map (h *) k3))
                      dx = map (h / 6 *) (foldr (+.) (repeat 0) [k1, k2, k2, k3, k3, k4])
                      xn = (+.) xs dx
                  in rk4 fs target h xn
                  where (+.) = zipWith (+)

A very simple set of functions to test this:

target :: (Fractional a, Ord a) => [a] -> Bool
target [x,y] = x >= 1.4

f1 :: Fractional a => [a] -> a
f1 [x,y] = 1

f2 :: Fractional a => [a] -> a
f2 [x,y] = x + 3 * y

Usage example:

euler [f1,f2] target 0.0001 [0,0]

I'm particularly unhappy with the need to define +. for element-wise addition (surely that exists, but Google didn't help me), and the way in which I added the various k values to create dx.

I also feel like I'm declaring too many variables (if that's what they're called?) in the let statement.

Oh, and my brackets are breeding like rabbits.

I feel ... that I should be able to use more inbuilt functions (that
  I'm not aware of)

Let me introduce you to your new best friend... Hoogle. Seriously, it's the best thing since sliced... monad analogies. Install a local copy as well.

We're producing a sequence of values by repeated application of a function, so let's search Hoogle for

(a -> a) -> a -> [a]

The first hit is iterate :: (a -> a) -> a -> [a], which looks like a good candidate. But a little further down the page, we see

  until :: (a -> Bool) -> (a -> a) -> a -> a

  until p f yields the result of applying f until p holds.

Bingo. So we know that euler is going to look like

euler :: Fractional a => [[a] -> a] -> ([a] -> Bool) -> a -> [a] -> [a]
euler fs target h = until target step
    where step x = ???

And we fill in the blanks

euler :: Fractional a => [[a] -> a] -> ([a] -> Bool) -> a -> [a] -> [a]
euler fs target h = until target step
    where step x = zipWith (+) x (map (h *) (pam fs x))

A similar transformation can be applied to rk4:

rk4step fs h x = x +. dx
    where k1 = pam fs x
          k2 = pam fs $ x +. map (h / 2 *) k1
          k3 = pam fs $ x +. map (h / 2 *) k2
          k4 = pam fs $ x +. map (h *) k3
          dx = map (h / 6 *) $ k1 +. k2 +. k2 +. k3 +. k3 +. k4
          (+.) = zipWith (+)

rk4 :: Fractional a => [[a] -> a] -> ([a] -> Bool) -> a -> [a] -> [a]
rk4 fs target h = until target (rk4step fs h)

I'm not sure if I'd actually suggest this, but you could also introduce another function, *.:

rk4step fs h x = x +. dx
    where k1 = pam fs x
          k2 = pam fs $ x +. ((h / 2) *. k1)
          k3 = pam fs $ x +. ((h / 2) *. k2)
          k4 = pam fs $ x +. (h *. k3)
          dx = (h / 6) *. (k1 +. (2 *. k2) +. (2 *. k3) +. k4)
          (+.) = zipWith (+)
          (*.) n = map (n *)

Finally, I think that pam would be more readable as a list comprehension

pam :: [a -> b] -> a -> [b]
pam fs x = [ f x | f <- fs ]


Selecting records according to an item I use Ruby 1.8.7.  I have a method which selects records according to the item that I pass to the method.

In my opinion, the code is not DRY.  Could somebody offer suggestions on refactoring it to make it shorter?

def get_record (id,item)
  case item
   when "category"
      @temp = Category.find(id)
   when "status"
      @temp = Status.find(id)
   when "industry"
     @temp = Industry.find(id)
   end
 return @temp.name if @temp
end

You want to use classify and constantize:

require 'set'
AUTHORIZED = Set[Product, Status, Industry]
def get_record(id, item)
  model = item.classify.constantize
  raise "Go away, hacker" unless AUTHORIZED.include? model
  model.find(id).name
end

Be careful about security, as without the unless this will access any class in your system. This is just an example.


Call a lua function - the C++ way I'm not that good when it comes to templates, especially when it comes to variadic templates. I want to show you some code where I use both - templates and variadic templates - to call a lua function. I'm proud that it works, and it works great, but I want to get some feedback on what I could've done better.

template<typename ... Args>
bool run_script(int id, Args && ... args) {
    const int params = sizeof...(args);
    _load(id);
    push(std::forward<Args>(args)...);
    return _run(params);
}

This is the "entry point". _load will just load the lua-function onto the stack and _run will run it with params arguments. The important part is the push function.

template<typename T>
void push(T && arg) {
    _push(std::forward<T>(arg));
}

template<typename T, typename ... Args>
void push(T && arg, Args && ... other) {
    _push(std::forward<T>(arg));
    push(std::forward<Args>(other)...);
}

void _push(int arg) {
    lua_pushinteger(L, arg);
}

void _push(const std::string& arg) {
    lua_pushstring(L, arg.c_str());
}

void _push(const char* arg) {
    lua_pushstring(L, arg);
}

I had a problem before, where I had all of them named push, but for some reason it couldn't push a std::string (std::string as argument choose the template instead of const std::string&)  but now with the templates called push and the functions which do the work _push it's working great. It's not much code, I know, but it's my the first time working with variadic templates and it took me a fair amount of time to get it working.

As your code goes everything i believe is fine. Define the template, define overloads and combine. Some things i've noticed that might be arguable:

Is it possible that your script will have zero arguments? For example a script with id = N denotes print last command or show commands history. If yes, your code wont compile since

template<typename T>
void push(T && arg) {
   _push(std::forward<T>(arg));
}

doesnt accept zero arguments. Perhaps replace it with zero arg overload?

template<typename T>
void push() {
    // cout << "All parameters passed to Lua! [Empty args case]\n";
}

Also as Args && ... other can actually be zero length there's a question of which will be called template<typename T> void push(T && arg) or template<typename T, typename ... Args> void push(T && arg, Args && ... other). Since push(std::forward<Args>(other)...) can not compile with 0 args, then the first overload will get called. I'd call that a bit misleading if somebody else has to work with your code.
The run_script func is packed with expressions which could potentially fail. So i'd probably try to encapsulate each step into its own method and move the _run() to the zero template push.

template<typename T>
void push() {
    // cout << "All parameters passed to Lua! [Empty args case]\n";
    _run(params);
}

That way if push<T>() gets called you're sure all the parameters were passed to interpreter.
Question arises here is how do you pass params to zero arg push? The topic says C++ way and that means OOP. Imo if you're asking about the C++ style then perhaps you should encapsulate your logic and provide an interface to the user. So go for a class which would hold the arg number as a private member and handle the actual processing of different types. Perhaps two classes one for command and one for processing it. That way it will be easier to scale.
And before doing that i'd think about the actual use of your application:

Will somebody else will be using it?
Will it be used in a multithreaded environment?
What guarantees for the user do you want to provide?

Your entry function bool run_script is supposed to return true or false i guess depending on how the execution of the script went. Does it really boil down to two outcomes? Are you giving a guarantee that the code would execute no matter what? Perhaps a enum class would suit better.
Your lua_pushstring(L, arg) and lua_pushinteger(L, arg) all hold some L - so seems only fair to hide it somewhere in the class as a private member as well as arg num.
Are you sure that you'll know all your types at compile time? What if somebody passes a double? There are more elegant ways to check the type being passed but this will do:

template <class A, class B>
struct eq {
   static const bool result = false;
};

template<class A> struct eq<A, A> {
    static const bool result = true;
};

and use

if (!eq<int, A>::result) {
    //...unsupported type error
}

p.s. great project btw. A while a go I've used sockets in Lua and C++ to communicate. So if your code is online I'd love to check it out.


Recursive Preorder Traversal For today, I tackled this coding challenge question:
Given a binary tree, write a method to recursively traverse the tree in the preorder manner. Mark a node as visited by adding its data to the list - Arraylist  preorderedList

ArrayList<Integer> preorderedList = new ArrayList<Integer>();
public void preorder(TreeNode root) {

   if(root == null){
       return;
   }
    preorderedList.add(root.data);
    if(root.left != null){
        preorder(root.left);
    }
     if(root.right != null){
        preorder(root.right);
    }

}

It is weird to put an object field right next to the method that uses it.  It's more common to put all the object fields together.  Of course, you don't show the rest of the class definition, so perhaps this is something you did just for this.

Consider writing instead

List<Integer> preorderedList = new ArrayList<Integer>();

It won't make much difference here, but using the interface as the type makes it easier to change implementations later.

You can simplify the null handling by changing

public void preorder(TreeNode root) {

   if(root == null){
       return;
   }
    preorderedList.add(root.data);
    if(root.left != null){
        preorder(root.left);
    }
     if(root.right != null){
        preorder(root.right);
    }

}

to

public void preorder(TreeNode root) {
    if (root == null) {
        return;
    }

    preorderedList.add(root.data);
    preorder(root.left);
    preorder(root.right);
}

This way it does the check for null after the recursive call rather than before making it.  Slightly less efficient, but easier to read.  Alternately, if that efficiency is important, note that this makes an extra check.  Consider breaking it into two methods.  This could be something like

private void _preorder(TreeNode root) {
    preorderedList.add(root.data);

    if (root.left != null) {
        _preorder(root.left);
    }

    if (root.right != null) {
        _preorder(root.right);
    }
}

public void preorder(TreeNode root) {
    if (root == null) {
        return;
    }

    _preorder(root);
}

This only checks if root is null the first time.  On the recursive calls, this isn't necessary.  It makes one extra method call (which may be inlined) rather than an extra call per leaf.  And it only checks for null once for each node rather than twice.

Benchmark, as you may find that the compiler already does this for you.


Student Class in C++ using sstream for format Based on my previous review, I'm posting another Hackerrank Class solution.

  Problem Statement

  We can store details related to a student in a class consisting of his
  age (int), first_name (string), last_name (string) and standard (int).

  You have to create a class, named Student, representing the student's
  details, as mentioned above, and store the data of a student. Create
  setter and getter functions for each element; that is, the class
  should at least have following functions:

  get_age set_age
  get_first_name, set_first_name
  get_last_name,set_last_name
  get_standard, set_standard

  Also, you have to create another method to_string() which returns the
  string consisting of the above elements, separated by a comma(,). You
  can refer to stringstream for this.
   NB: I didn't implement the to_string() as I felt I could use sstream in   the main().

  Input Format

  Input will consist of four lines.  The first line will contain an
  integer, representing the age. The second line will contain a string,
  consisting of lower-case Latin characters ('a'-'z'), representing the
  first_name of a student.  The third line will contain another string,
  consisting of lower-case Latin characters ('a'-'z'), representing the
  last_name of a student.  The fourth line will contain an integer,
  representing the standard of student.

  Note: The number of characters in first_name and last_name will not
  exceed 50.

  Output Format

  The code provided by HackerRank will use your class members to set and
  then get the elements of the Student class.

  Sample Input

15
john
carmack
10

  Sample Output

15
carmack, john
10

15,john,carmack,10

Here is my code:

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

/*
Enter code for class Student here.
Read statement for specification.
*/
class Student{

    int age;
    string first_name;
    string last_name;
    int standard;

    public:
          int get_age(){
              return age;
          }
          void set_age(int age1){
              age = age1;
          }
           string get_first_name(){
              return first_name;
          }
          void set_first_name(string first_name1){
              first_name = first_name1;
          }
          string get_last_name(){
              return last_name;
          }
          void set_last_name(string last_name1){
              last_name = last_name1;
          }
           int get_standard(){
              return standard;
          }
          void set_standard(int standard1){
              standard = standard1;
          }

};

int main() {
    int age, standard;
    string first_name, last_name;
    stringstream ss;

    cin >> age >> first_name >> last_name >> standard;

    Student st;
    st.set_age(age);
    st.set_standard(standard);
    st.set_first_name(first_name);
    st.set_last_name(last_name);

    cout << st.get_age() << "\n";
    cout << st.get_last_name() << ", " << st.get_first_name() << "\n";
    cout << st.get_standard() << "\n";
    cout << "\n";

    ss << st.get_age() <<","<<st.get_first_name() <<","<<st.get_last_name()<<","<< st.get_standard();
    cout<<ss.str()<<endl;

    return 0;
}

Final thoughts:

Have I used sstream correctly? Have I used it to its full capacity?
What could have been done better, such as style-wise?

You are writing a C++ program, so why have you used C Style comments?
If parameters aren't changing it makes it clearer to readers/maintainers if you declare them as const.  It they are const they may as well be const references.
The point of namespaces is to provide a degree of separation of the code in libraries, so they don't clash.  using namespace std; breaks this don't use it.
Defining multiple variables on a line is a maintenance nightmare, also it means you can write comments explaining what they are used for.
There are very few cases when not assigning an initial value to a variable is a good idea.  The first time you production code crashes on the customer's machine you will appreciate why initialising variables is a good idea.
Rather than calling 4 set functions why not define a constructor that takes for optional parameters?
Rather than letting main() format the output why not write a function that displays the student record in the format that the class dictates.  To make it easier, just right the data to a multi line string.
You should get into the habit of commenting the code better.
Although you don't need to explicitly declare the class members as private it is a good idea to do so, because then someone can add a public function at the top of the class without exposing all your variables.
Its a personal preference but I think the Java style of curly braces {} shouldn't be used in C++.  It makes the code clearer if every { and } is on a separate line.  IMO.
Always include all header files you need, it makes it easier to reuse code because everything you need is there.  (You missed ).
The code should be in 2 files, the class in one and main in another.

`

#include <string>
#include <iostream>
#include <sstream>

// Enter code for class Student here.
// Read statement for specification.
class Student
{
private:
    int m_age;
    string m_first_name;
    string m_last_name;
    int m_standard;

public:
    Student(const int age, const std::string& fName, const std::string& lName, const int& standard)
        : m_age(age)
        , m_first_name(fName)
        , m_last_name(lName)
        , m_standard(standard)
    {
    }

    int get_age() const
    {
        return m_age;
    }
    std::string get_first_name() const
    {
        return m_first_name;
    }
    std::string get_last_name() const
    {
        return m_last_name;
    }
    int get_standard() const
    {
        return m_standard;
    }
};

int main()
{
    int age(-1)
    int standard(-1);
    std::string first_name();
    std::string last_name();
    stringstream ss();

    cin >> age >> first_name >> last_name >> standard;

    Student st(age, first_name, last_name, standard);

    cout << st.get_age() << "\n";
    cout << st.get_last_name() << ", " << st.get_first_name() << "\n";
    cout << st.get_standard() << "\n";
    cout << "\n";

    ss << st.get_age() << "," << st.get_first_name() << "," << st.get_last_name() << "," << st.get_standard();
    cout << ss.str() << endl;

    return 0;
}

`


Refactored 2 functions into 1 - Verify UPC/EAN13 barcode I refactored 2 seperate functions that I found here into a single function:

<?
    function isValidBarcode($barcode) {

        $barcode = trim($barcode);

        //If value is not numeric, it is not valid.
        if ((!is_numeric($barcode)) && ((strlen($barcode) == 12) || (strlen($barcode) == 13))) return false;

        //set empty variables
        $even_sum = $odd_sum = 0;

        //array key value to count to, doesn't include the last 2 numbers of barcode
        $countTo = strlen($barcode) - 2;

        //loop through every digit in barcode, count even and odds based on $countTo set before.
        for ($x = 0; $x <= $countTo; $x++) {
            if ($x % 2 == 0) {
                $even_sum += $barcode[$x];
            } else {
                $odd_sum += $barcode[$x];
            }
        }

        //Multiply even_sum by 3, and then add odd_sum to it.
        $total_sum   = $even_sum * 3 + $odd_sum;

        //get highest multiple of 10 of total_sum.
        $next_ten    = ceil($total_sum / 10) * 10;

        //Subtract highest multiple of 10 from total_sum to get check_digit.
        $check_digit = $next_ten - $total_sum;

        //if check_digit is equal to the last digit in the barcode, it is valid.
        if ($check_digit == $barcode[$countTo + 1]) return true;

        //if check_digit is not equal to the last digit in the barcode, it is invalid.
        return false;
    }

?>

But, I feel as though I can make this even better. I wanted some other opinions on this, and I wanted to see what other people could come up with in terms of a solution.

My main question is, how efficient is this code, and is it possible for me to make it more efficient?

Some thoughts:

Why would you silently trim() and potentially transform an invalid bar code value into a valid one?  This would seem to give unexpected results.  If a string value needs to be trimmed, let the caller do it before calling this method.
What happens if you are not passed a string? To me this might be an exceptional event and worthy of throwing exception from this function, as this is something the developer writing code calling this function needs to be aware of.
Do you really want to use is_numeric() to validate the input format?  This can lead to false positives (for example +123.45678e9 would pass your current format check). If what you are expecting are 12 or 13 digits in this string, then precisely test for that. In addition to your questionable use of is_numeric(), you also have your length conditions set inappropriately.  You should be using OR between the digit criteria and length criteria as either cause failure.  Right now a string like foobar will actually pass your conditional because it does not satisfy both conditions.

For example:

if (!preg_match('/^[0-9]{12,13}$/', $barcode)) {
    return false;
}
// rest of code

or

if (!ctype_digit($barcode) || $bcLen < 12 || $bcLen > 13) {
    return false;
}
// rest of code

I think the logic to determine check digit can be done in a more simplified manner. Part of this simplificatoin could be to explictly convert the barcode string to an array to make it easier to work with. I have done a function rewrite below which I hope demonstrates some simplification.

Example:

function isValidBarcode($barcode)
{
    if (!is_string) {
        throw new InvalidArgumentException('String value expected.');
    }
    if (!preg_match('/^[0-9]{12,13}$/', $barcode)) {
        return false;
    }
    $barcodeArray = str_split($barcode);
    $lastDigit = (int)array_pop($barcodeArray);
    $sum = 0;
    for($i = 0; $i < count($barcodeArray); $i++) {
        $sum += (int)$barcode[$i];
        if ($i % 2 === 0) {
            $sum += (int)$barcode[$i] * 2;
        }
    }
    $checkDigit = 10 - ($sum % 10);
    return $lastDigit === $checkDigit;
}

Be careful using loose comparisons as your default means for comparing.  I would suggest you get in the habit of using exact comparisons as default unless there is good reason to do loose comparison.  I don't see any actual bug in this code (since PHP's built-in type juggling should take care of string to integer conversions in your specific use case here).  Bu,t you might not always be so lucky. PHP's loose typing can be useful, but it can also be a source of hard-to-identify bugs.  To me when reading code, I would like the code writer to explicitly demonstrate their understanding of types being applied to the variables.
Style-wise, I have a few concerns.  You are mixing camelCase and snake_case in your code.  Ideally you should pick one style and stick to it (though PHP as a language is not very good about this).  There are too many comments, most of which are not really necessary. You also have inconsistent spacing around your = assignments near the end of code. Trying to horizontally-align assignment operators can be a bit of a fool's errand if you try to do this throughout your codebase. Every time you want to make changes to variable names and such, you end up having to redo spacing.  Is it worth it?
Why are you concerned with the "efficiency" of this code?  Are you planning to execute this code thousands of time in any one script execution such that every millisecond spent in this function gets compounded?  If not, you might be prematurely optimizing this bit of code.  Sure, you want to make sure it runs with reasonable efficiency (which I think your code does since it is such a simple data structure - a string - that you are working with), but I don't think you want to necessarily squeeze out every millisecond out of this function if it means making the function hard to read or maintain.  That code quality aspect should likely be you primary concern with a function such as this which you are not likely to spend a large percentage of your overall processing time in.


How to apply multiple filters in one statement While trying to grasp the potentials of the collection features in F#, I encountered this one:
let needSpecialDocument country = (not country.IsEea) && (not country.IsInSchengen)
// could also be: not (country.IsEea || country.IsInSchengen)
let wantToMoveTo country = country.Name = "CC"

let potentialDestinations =
    countries
    |> Seq.filter needSpecialDocument2
    |> Seq.filter wantToMoveTo

Is there a way to combine the two filters into one? I managed the one below but it feels like there is a more fsharpish way to do so..
|> Seq.filter (fun c -> (needSpecialDocument c) && (wantToMoveTo c))

Test data:
type Country = {Name : string; IsEea : bool; IsInSchengen : bool}
let uk = {Name = "UK"; IsEea = false; IsInSchengen = false}
let ir = {Name = "IR"; IsEea = true; IsInSchengen = false}
let fr = {Name = "FR"; IsEea = true; IsInSchengen = true}
let ch = {Name = "CH"; IsEea = false; IsInSchengen = true}
let cc = {Name = "CC"; IsEea = false; IsInSchengen = false}
let countries = [uk;ir;fr;ch;cc]

Nope, that's pretty much the way.
If you really, really want to make this shorter and less cluttered, and if you face similar situations multiple times in your codebase, you could make yourself a special operator for combining predicates:
let (.&&.) f g c = f c && g c

Then you can use this operator in your filter:
|> Seq.filter (needSpecialDocument .&&. wantToMoveTo)

But base on my own experience I wouldn't advise this. You make the program slicker and cooler looking, but you're losing some readability. Now whoever reads your program will have to look up the meaning of .&&.. Programs are read more often than they are written.


Functional pattern matching in C# I recently read a really interesting post on adding functional pattern matching to C#. Here's a quick and dirty implementation using delegates and dictionaries.

Right now, I require that Match return an object of type TReturn. Is there any way I can easily extend my current implementation to be void (i.e. Match is not required to return anything) without significant code duplication? When I first tried this, I ended up essentially copying Match but omitting the second type parameter.

/// <summary>
/// Represents a switch on TIn.
/// </summary>
public class Match<TIn, TReturn>
{
    private Dictionary<Type, Delegate> funcs =
        new Dictionary<Type, Delegate>();

    private Dictionary<TIn, TReturn> values =
        new Dictionary<TIn, TReturn>();

    private TReturn _else;

    /// <summary>
    /// Adds a case, action pair
    /// </summary>
    public void Case<T>(Func<T, TReturn> func)
        where T : TIn
    {
        funcs[typeof(T)] = func;
    }

    /// <summary>
    /// Adds a case, value pair
    /// </summary>
    public void Case(TIn x, TReturn value)
    {
        values[x] = value;
    }

    /// <summary>
    /// Adds the default case value
    /// </summary>
    public void Else(TIn x, TReturn value)
    {
        _else = value;
    }

    /// <summary>
    /// Evaluates the match on a query
    /// Returns an object of type TReturn
    /// for first successful match
    /// </summary>
    public TReturn Eval<T>(T query)
        where T : TIn
    {
        TReturn value;
        if (values.TryGetValue(query, out value))
            return value;

        Delegate func;
        if (funcs.TryGetValue(query.GetType(), out func))
            return (TReturn)func.DynamicInvoke(query);

        return _else;
    }
}

And here's a simple test. One thing I was wondering: is it inefficient to declare a new Match inside the Calculate method?

class Program
{
    public static double Calculate(Expression exp)
    {
        var match = new Match<Expression, double>();
        match.Case<Num>((x) => x.Value);
        match.Case<Mul>((x) => Calculate(x.Left) * Calculate(x.Right));
        match.Case<Add>((x) => Calculate(x.Left) + Calculate(x.Right));
        return match.Eval(exp);
    }

    public static void Main(string[] args)
    {
        Expression tree = new Add(new Add(new Num(1), new Num(2)), new Mul(new Num(3), new Num(4)));
        double x = Calculate(tree) // = 15
    }
}

abstract class Expression
{
    public double Value;
}

abstract class BinaryExpression : Expression
{
    public Expression Left;
    public Expression Right;

    public BinaryExpression(Expression left, Expression right)
    {
        Left = left; Right = right;
    }
}

class Add : BinaryExpression
{
    public Add(Expression left, Expression right)
        : base(left, right)
    {
    }
}

class Mul : BinaryExpression
{
    public Mul(Expression left, Expression right)
        : base(left, right)
    {
    }
}

class Num : Expression
{
    public Num(double x)
    {
        Value = x;
    }
}

In your Match class, I would mark the two private dictionaries as readonly to signify intent and keep them from being assigned to accidentally anywhere else:

private readonly Dictionary<Type, Delegate> funcs =
    new Dictionary<Type, Delegate>();

private readonly Dictionary<TIn, TReturn> values =
    new Dictionary<TIn, TReturn>();

Secondly, I'd design to interfaces and make them IDictionaries as good OO practice:

private readonly IDictionary<Type, Delegate> funcs =
    new Dictionary<Type, Delegate>();

private readonly IDictionary<TIn, TReturn> values =
    new Dictionary<TIn, TReturn>();

In your example classes, you should forego public member data and use properties accessing backing fields (also marked readonly):

abstract class Expression
{
    private readonly double _Value;

    protected Expression(double value)
    {
        this._Value = value;
    }

    public double Value
    {
        get
        {
            return this._Value;
        }
    }
}

abstract class BinaryExpression : Expression
{
    private readonly Expression _Left;

    private readonly Expression _Right;

    public BinaryExpression(Expression left, Expression right) : base(default(double))
    {
        this._Left = left;
        this._Right = right;
    }

    public Expression Left
    {
        get
        {
            return this._Left;
        }
    }

    public Expression Right
    {
        get
        {
            return this._Right;
        }
    }
}

class Add : BinaryExpression
{
    public Add(Expression left, Expression right) : base(left, right)
    {
    }
}

class Mul : BinaryExpression
{
    public Mul(Expression left, Expression right) : base(left, right)
    {
    }
}

class Num : Expression
{
    public Num(double x) : base(x)
    {
    }
}


Retrieving a user's password for verification I am working on a log in form. Basically, my code searches for a user name, then retrieves the hashed password that is related to that account. It then converts that string hashed password then converts it to its real value. But my issue here is that it is messy.

 private void userVerification() {

            //Instantiate Db
            DataClasses1DataContext myDbContext = new DataClasses1DataContext(dbPath);

            //Search for uservar search

            var hasher = new Hasher();
            //by default salt size is 4
            hasher.SaltSize = 16;

            var encryptedPassword = hasher.Encrypt(txtPassword.Text);

            var searchUser = from user in myDbContext.Accounts
                             where user.accnt_User == txtUser.Text
                             select user.accnt_Pass;

            //Retrieves the first result.
            var password = searchUser.FirstOrDefault();

            //Check if there's a result or match
            if (searchUser.Count() != 0)
            {
                bool areEqual = hasher.CompareStringToHash(txtPassword.Text, password);

                if (areEqual = true)
                {
                    MessageBox.Show("CORRECT!");
                }
                else {
                    MessageBox.Show("Wrong password!");
                }
            }
            else {
                MessageBox.Show("No Such User in the database");
            }
        }

What I would correct in the code:

if anything goes wrong it's best to just display "Login incorrect", instead of specialized messages ('user not found' / 'incorrect passsword'). Giving too many details may lead to information leakage
searchUser is not a very good variable name. It in fact represents the result of a search, so I'd name it matchedUser. searchUser would be a good name for a query.
the encryptedPassword variable is assigned but never used. You could use it to compare against the password hash in the database

In the end, my code would look like this:

private void userVerification()
{
    var myDbContext = new DataClasses1DataContext(dbPath);

    var hasher = new Hasher { SaltSize = 16 };
    var encryptedPassword = hasher.Encrypt(txtPassword.Text);

    var matchedUser = myDbContext.Accounts
        .Where( user =>
                user.accnt_User == txtUser.Text &&
                user.accnt_Pass == encryptedPassword)
        .FirstOrDefault();

    if(matchedUser != null)
        MessageBox.Show("CORRECT!");
    else
        MessageBox.Show("Invalid user or password.");
}


Finding the next available filename We're having a small debate in work about the following piece of code:

private string GetNextFileName(string fileName)
{
    int index = 1;

    string nextName = $"{fileName}_{index}.png";

    while (File.Exists(nextName))
    {
        nextName = $"{fileName}_{++index}.png";
    }

    return nextName;
}

Is there anything particularly dangerous about this piece of code? It sort of feels dodgy having the condition in a while loop.

Would it be better to use a for-loop with a breakout if the number gets too high?

e.g.

private string GetNextFileName(string fileName)
{
    int i = 1;
    string nextName = $"{fileName}_{i}.png";

    for (; i < 100; ++i)
    {
        if (!File.Exists(nextName))
        {
            return nextName;
        }
        nextName = $"{fileName}_{i}.png";
    }

    throw new ApplicationException("Unable to get free filename");
}

Stylistically, the for loop is more readable.  As a general rule, if you start the loop at 1, then I would prefer the termination condition to use <=; if you start counting from 0, then < would be better.

The design of this function is conceptually flawed.  It will return an unused filename, but presumably you will eventually want to create a file with the name.  However, it is possible that someone else will have created a file with the same name, creating a conflict.  This possible race condition means that you can never be certain that the filename returned by this function is actually unused.

The way to make that guarantee is to actually create the file, such that you effectively grab a reservation on that name.  You can do that by calling File.Open(path, FileMode.CreateNew).  The caller would have to delete the file if it doesn't want it.

private static string CreateNextAvailableFile(string fileNamePrefix)
{
    string name;
    for (int i = 1; i <= 99; ++i)
    {
        try
        {
            FileStream fs = File.Open(name = $"{fileNamePrefix}_{i}.png", FileMode.CreateNew);
            fs.Close();
            return name;
        }
        catch (IOException e)
        {
            // Did File.Open() fail due to name collision or another
            // reason? Unfortunately, no specific "FileAlreadyExists"
            // exception exists.  This is a heuristic, and can be
            // fooled by a race condition where the file was deleted
            // just now by someone else.
            if (!File.Exists(name)) throw;
        }
    }

    throw new ApplicationException("Unable to get free filename");
}

As the comment notes, .NET unfortunately has no way to tell whether the IOException was due to a filename collision or something else.  The code above is therefore also vulnerable to a race condition, but it's better than your race condition, because:

This race condition is only triggered when there is an underlying I/O problem (such as insufficient permissions to create a file, or a read-only filesystem), and the file in question is deleted during the split-second between the File.Open() call and the exception handler.
This code fails in a safer way: if the race condition is triggered, it will throw an IOException.  (Arguably, such an IOException would happen anyway, later on.)  However, it's harder to tell what might be the consequences of your race condition: it might fail to detect a collision, and lead to data being overwritten.


Creating an Enum with a Custom Type in Swift 4.x I was in a code review today and someone said, "Why don't you do an enum on that?" I had started to, but conforming to a bunch of protocols seemed like a gigantic PITA and it seemed more error prone than just writing something simple, readable, and maintainable.

That said, I started to fiddle around with it this evening and I can't figure it out.

This is what I've started with:

typealias PolicyType = (filename: String, text: String)

struct Policy {
  static let first = PolicyType(filename: "firstFile.txt", text: "text in first file")
  static let second = PolicyType(filename: "secondFile.txt", text: "text in second file")
  static let third = PolicyType(filename: "thirdFile.txt", text: "text in third file")
}

let thirdPolicyText = Policy.third.text

Is there a more memory efficient, maintainable way to do this with an enum? My primary objective is maintainability.

Below is what I've come up with. It feels hacky and looks like...well, you know:

public struct PolicyType : Equatable, ExpressibleByStringLiteral {
  var filename: String
  var text: String

  //MARK:- Equatable Methods
  public static func == (lhs: PolicyType, rhs: PolicyType) -> Bool {
    return (lhs.filename == rhs.filename && lhs.text == rhs.text)
  }

  //MARK:- ExpressibleByStringLiteral Methods
  public init(stringLiteral value: String) {
    let components = value.components(separatedBy: "|")
    self.filename = components[0]
    self.text = components[1]
  }

  public init(unicodeScalarLiteral value: String) {
    self.init(stringLiteral: value)
  }
  public init(extendedGraphemeClusterLiteral value: String) {
    self.init(stringLiteral: value)
  }
}

enum Policy: PolicyType {
  case first = "testFile1.txt|This is sample text"
  case second = "testFile2.txt|This is more sample text"
  case third = "testFile3.txt|This is more sample text"
}

Policy.third.rawValue.text

The code is using Enum, but it's still using Struct.
The program may be buggy if the text value has character |. Suppose that "testFile1.txt|This is sample text" to "testFile1.txt|This is | sample text".

I have another approach with Enum in this case:

 enum Policy {
    case first
    case second
    case third

    var fileName: String {
        switch self {
        case .first: return "testFile1.txt"
        case .second: return "testFile2.txt"
        case .third: return "testFile3.txt"
        }
    }

    var text: String {
        switch self {
            case .first : return "This is sample text"
            case .second: return "This is more sample text"
            case .third: return "This is more sample text"
        }
    }
}
   print(Policy.first.text)


Efficient way to deal with maintaining Many:Many relationships in EF Code-First I've got this all working, but it seems to be quite long-winded, and I thought I'd post here and see if I'm doing it wrong...

I have a M:M relationship between an Installer and a MasterInstance. The classes (code-first) look like:

public class MasterInstance
{
    .. rest of fields here ..
    public virtual ICollection<Installer> PermittedInstallers { get; set; }
}

public class Installer
{
    .. rest of fields here ..
    public virtual ICollection<MasterInstance> PermittedMasterInstances { get; set; }
}

I want to be able to edit these in my Installer view, using a multi-list box. So, I create a ViewModel for what I need:

public class InstallerViewModel
{
    public Installer Installer { get; set; }
    public List<MasterInstance> PermittedMasterInstances { get; set; }
    public int[] SelectedMasterInstances { get; set; }
}

And then I place this in my view like so so that I can select the instances for my installer:

<div class="editor-field">
    @Html.ListBoxFor(model => model.SelectedMasterInstances,(Model.PermittedMasterInstances).Select(option => new SelectListItem {
        Text = option.Name,
        Value = option.MasterInstanceId.ToString()
}))
</div>

In the controller, for the edit action, I need to set this view model up:

var installer = context.Installers.Include(i => i.PermittedMasterInstances).Single(x => x.InstallerId == installerId);
            InstallerViewModel model = new InstallerViewModel
                                           {
                                               Installer = installer,
                                               PermittedMasterInstances = context.MasterInstances.ToList(),
                                               SelectedMasterInstances = installer.PermittedMasterInstances.Select(i => i.MasterInstanceId).ToArray()
                                           };
return View(model);

Finally, on the post of the edit, I need to delete any relationships that are no longer there and add the new ones:

// Grab the model from the viewmodel and attach to the context
var installer = installerModel.Installer;
context.Installers.Attach(installer);

// Load the related records (dont know why Lazy Loading wouldn't kick in here)
context.Entry(installer).Collection(i => i.PermittedMasterInstances).Load();
// Iterate and delete existing relationships
var instancesToDelete = installer.PermittedMasterInstances.Where(mi => !installerModel.SelectedMasterInstances.Contains(i.MasterInstanceId)).ToList();
instancesToDelete.ForEach(mi => installer.PermittedMasterInstances.Remove(mi));

// Now loop through an int[] and add those new relations, WITHOUT the pain of fetching them from the DB
foreach (var permittedMasterInstanceId in installerModel.SelectedMasterInstances)
{
    if (!installer.PermittedMasterInstances.Any(pmi => pmi.MasterInstanceId == permittedMasterInstanceId))
    {
        var masterInstance = new MasterInstance {MasterInstanceId = permittedMasterInstanceId};
        context.MasterInstances.Attach(masterInstance);
        installer.PermittedMasterInstances.Add(masterInstance);
    }
}

// We're done - save and finish.
context.Entry<Installer>(installer).State = EntityState.Modified;
context.SaveChanges();

So this works... But, it seemed like a lot of effort, is this the right/best way to achieve it?

I know this is an old question, but I think the problem is timeless. Many to many associations (i.e. without junction class) in Entity Framework are always independent associations, so you can only establish or remove them by manipulating object collections, not primitive key values. Inefficiency is inherent to the implementation.

But it is not prohibited to have a second context that only contains junction tables.

You could create a context that contains the MasterInstanceInstaller junction table and use this to update the associations in the most efficient way you can get using EF:

var installer = installerModel.Installer;

var junctions = context.MasterInstanceInstallers
                       .Where(x => x.InstallerId == installer.InstallerId)
                       .ToList();

// Delete deselected instances.
foreach(var mi in junctions
                  .Where(x => !installerModel.SelectedMasterInstances
                                             .Contains(x.MasterInstanceId)))
{
    context.MasterInstanceInstallers.Remove(mi);
}

// Add newly selected instances.
foreach(int instanceId in installerModel.SelectedMasterInstances
                                        .Except(junctions.Select(j => j.MasterInstanceId)))
}
    context.MasterInstanceInstallers.Add(new MasterInstanceInstaller
                                             {
                                                InstallerId = installer.InstallerId,
                                                MasterInstanceId = instanceId
                                             }
                                        );
}
context.SaveChanges();

Now, if necessary you can populate the updated many to many association through the main context.


Advent of code 2017 Day 2 (2) in Functional Programming (FP) I wanted to practice functional programming (fp) without using any library but using vanilla JS only. So I took a problem from Advent of Code (the 2nd part of Day 2):

https://adventofcode.com/2017/day/2

I'm doing the 2nd part of Day 2. You can only access the 2nd part once you solved the 1st part.

To access the 2nd part type in this number 50376 or check out the solution here: Advent of Code Day 2 (1) in Functional programming (FP)

  It sounds like the goal is to find the only two numbers in each row
  where one evenly divides the other - that is, where the result of the
  division operation is a whole number. They would like you to find
  those numbers on each line, divide them, and add up each line's
  result.

  For example, given the following spreadsheet:

  5 9 2 8
  9 4 7 3
  3 8 6 5

  In the first row, the only two numbers that evenly divide are 8 and 2;
  the result of this division is 4. In the second row, the two numbers
  are 9 and 3; the result is 3. In the third row, the result is 2. In
  this example, the sum of the results would be 4 + 3 + 2 = 9.

My solution in FP:

/*jshint esversion: 6*/
{
  'use strict';

  const INPUT =
    `6046   6349    208 276 4643    1085    1539    4986    7006    5374    252 4751    226 6757    7495    2923
1432    1538    1761    1658    104 826 806 109 939 886 1497    280 1412    127 1651    156
244 1048    133 232 226 1072    883 1045    1130    252 1038    1022    471 70  1222    957
87  172 93  73  67  192 249 239 155 23  189 106 55  174 181 116
5871    204 6466    6437    5716    232 1513    7079    6140    268 350 6264    6420    3904    272 5565
1093    838 90  1447    1224    744 1551    59  328 1575    1544    1360    71  1583    75  370
213 166 7601    6261    247 210 4809    6201    6690    6816    7776    2522    5618    580 2236    3598
92  168 96  132 196 157 116 94  253 128 60  167 192 156 76  148
187 111 141 143 45  132 140 402 134 227 342 276 449 148 170 348
1894    1298    1531    1354    1801    974 85  93  1712    130 1705    110 314 107 449 350
1662    1529    784 1704    1187    83  422 146 147 1869    1941    110 525 1293    158 1752
162 1135    3278    1149    3546    3686    182 149 119 1755    3656    2126    244 3347    157 865
2049    6396    4111    6702    251 669 1491    245 210 4314    6265    694 5131    228 6195    6090
458 448 324 235 69  79  94  78  515 68  380 64  440 508 503 452
198 216 5700    4212    2370    143 5140    190 4934    539 5054    3707    6121    5211    549 2790
3021    3407    218 1043    449 214 1594    3244    3097    286 114 223 1214    3102    257 3345`;

  const sum = (a, b) => a + b;
  const evenlyDiv = (diff, val) => {
    const row = val.split(/\t/);
    return diff.concat(row
      .reduce((d, v, i, line) => {
        return d + line.reduce((reduceRes, runningVar) => {
          return (runningVar % v === 0 && runningVar !== v) ?
            reduceRes + (runningVar / v) :
            reduceRes;
        }, 0);
      }, 0));
  };
  const solution = INPUT.split(/\n/)
    .reduce(evenlyDiv, [])
    .reduce(sum);

  console.log("solution ", solution);
}

Is there a better way to write it in FP with pure JavaScript, i.e. no additional FP library? Any improvement suggestions are welcomed.

While your code works, it took me a few tries to really understand how it works. I believe this is primarily caused by the deep nesting of evenlyDiv. FP is great for reusable code, which makes helper functions a really good idea. In this case, by using a few helpers you can make the code more readable.

A few pointers on your current solution:

Don't use strings as numbers. Sure, it might work right now because division can't concatenate strings like addition can, but it is a dangerous habit to get into.
There is no need to use a regular expression in this case to split the input into rows and columns. .split(/\n/) is the same as .split('\n').
(d, v, i, line) is a lot of parameters, in this case you don't need two of them. Instead of line, you can just use row, and you never used i anyways.
Don't default to reduce, the original function can be easily simplified by using map instead and just dropping the diff parameter.
Try to avoid meaningless variable names. For very simple functions, it is fine to just use a, and b, but for anything longer than a line (and even some functions of only one line) it makes the code much more difficult to scan when another programmer in 6 months (you) looks at it.

Here is how I would implement the solution (assuming a relatively small table, if the table is very large, some optimization would be a good idea).

const INPUT = `5\t9\t2\t8
    9\t4\t7\t3
    3\t8\t6\t5`;

// Contained in most FP libraries, but simple to write
const unary = fn => arg => fn(arg)

const sum = (a, b) => a + b
const isNot = a => b => a !== b
const isDivisible = dividend => divisor => dividend % divisor === 0;

/**
 * Takes an array of numbers, returns the result of the division
 * of the first two numbers which divide evenly or 0 if no two
 * numbers are divisible.
 */
const divideEvenly = numbers => {
  return numbers.reduce((carry, n) => {
    if (carry) return carry;

    const divisor = numbers
      .filter(isNot(n))
      .find(isDivisible(n));
    return divisor == null ? carry : n / divisor;
  }, 0);
}

// Without unary, parseInt would fail due to trying to parse with different bases
const solution = INPUT.split('\n')
  .map(line => line.split('\t').map(unary(parseInt)))
  .map(divideEvenly)
  .reduce(sum, 0);

console.log("solution ", solution);


Algorithm to determine if binary tree is a Binary Search Tree (BST) Continuing with algorithms I've implemented a binary search tree validator. I don't like the two boolean variables within NodeFollowsBSTContract as it feels too complicated. I feel like it should be cleaned up but don't see how, yet.
Also, before each recursive step down, to check child nodes, a new list is created. Is there's a better way to implement this check that doesn't repeatedly create new lists?
public class BinaryTreeNode
{
    public BinaryTreeNode Left { get; set; }
    public BinaryTreeNode Right { get; set; }

    public int? Value { get; }

    public BinaryTreeNode(int value)
    {
        Value = value;
    }
}

public class ValidateBST
{
    BinaryTreeNode _root;
    public ValidateBST(BinaryTreeNode root)
    {
        _root = root;
    }

    public bool IsBinarySearchTree()
    {
        if ((_root.Left?.Value ?? 0) <= (_root.Value)
            || (_root.Right?.Value ?? 0) > (_root.Value))
        {
            var listIncludingRootValue = new List<int>()
            {
                _root.Value.Value
            };

            var leftLegValid = NodeFollowsBSTContract(_root.Left, new List<int>(), new List<int>(listIncludingRootValue));

            var rightLegvalid = NodeFollowsBSTContract(_root.Right, new List<int>(listIncludingRootValue), new List<int>());

            return leftLegValid && rightLegvalid;
        }
        else
        {
            return false;
        }
    }

    private bool NodeFollowsBSTContract(BinaryTreeNode node, List<int> parentSmallerValues, List<int> parentLargerValues)
    {
        if (node == null)
        {
            return true;
        }

        bool isLessThanAllParentLargerValues = !parentLargerValues.Any()
            || parentLargerValues.Where(value => node.Value.Value <= value).Count() == parentLargerValues.Count;

        bool isGreaterThanAllParentSmallerValues = !parentSmallerValues.Any()
            || parentSmallerValues.Where(value => node.Value.Value > value).Count() == parentSmallerValues.Count;

        if (!isLessThanAllParentLargerValues || !isGreaterThanAllParentSmallerValues)
        {
            return false;
        }

        if (node.Left != null)
        {
            var updatedLargerValues = GenerateUpdatedLists(node.Value.Value, parentLargerValues);
            var updatedSmallervalues = new List<int>(parentSmallerValues);

            if (!NodeFollowsBSTContract(node.Left, updatedSmallervalues, updatedLargerValues))
            {
                return false;
            }
        }

        if (node.Right != null)
        {
            var updatedvalues = GenerateUpdatedLists(node.Value.Value, parentSmallerValues);

            if (!NodeFollowsBSTContract(node.Right, updatedvalues, parentLargerValues))
            {
                return false;
            }
        }

        return true;
    }

    private List<int> GenerateUpdatedLists(int addValue, List<int> values)
    {
        var updatedValues = new List<int>(values)
        {
            addValue
        };

        return updatedValues;
    }
}

You can tremendously reduce the memory requirements of this code by acknowledging that you can replace the lists you use by single values:
private static bool IsBinarySearchTree(Node root, int max, int min)

this allows you to successively tighten the bounds on subtrees without storing the values you traversed in a list:
public static bool IsBinarySearchTree()
{
    var valid = true;
    // could be simplified to a single expression, but this is easier to understand
    valid &= IsBinarySearchTree(_root.Left, _root.Value, Int.MIN_VALUE);
    valid &= IsBinarySearchTree(_root.Right, Int.MAX_VALUE, _root.Value);
    return valid;

}

This should already be enough to write the recursive method and I think you'll learn more if I don't spoil this for you :)
But if you want a spoiler...

 private static bool IsBinarySearchTree(Node root, int max, int min)
 {
      // if there's no node
      if (root == null)  return true;
      if (root.Value <= min || root.Value >= max)
      {
          return false;
      }
      return IsBinarySearchTree(root.Left, root.Value, min)
        && IsBinarySearchTree(root.Right, max, root.Value);
 }

This simplification is possible because you only need store the smallest larger element and the largest smaller element to be able to guarantee that relation holds for the node you're currently examining.


Top Active Answerers on a Stack Exchange site I created this query back in January to find the top active answerers on Code Review and thought that it might be a good idea to get other opinions on my SQL coding using a database that we can all access.

This will probably be the first of several queries that I post for review.

Actual SEDE Query --> Top Active Answerers on site

--@DaysSinceActivity is the amount of days that you would like to scan for activity
--@NumberOfUsers is the number of users you want to return.

DECLARE @DaysSinceActivity INT = ##DaysSinceActivity##

SELECT
    TOP ##NumberOfUsers##
    Users.Id as [User Link],
    Count(Posts.Id) AS Answers,
    CAST(AVG(CAST(Score AS float)) as numeric(6,2)) AS [Average Answer Score]
FROM
    Posts
INNER JOIN
    Users ON Users.Id = OwnerUserId
WHERE
    PostTypeId = 2 and CommunityOwnedDate is null and ClosedDate is null
    AND Users.LastAccessDate > DateAdd(Day, -31, GetDate())
GROUP BY
    Users.Id, DisplayName
HAVING
    Count(Posts.Id) > 10
ORDER BY
    [Average Answer Score] DESC

Dumping some SEDE experience in as well as a SQL review.
SEDE Specific things first
Parameters
SEDE caches the results of a query if it is run with the current data set (caches are cleared when the data is refreshed). If you run the same query twice, the second time will return the cached data. If the query has not been run, then you need to be logged in to refresh the cache. If the query has been run, then you do not need to be logged in to SEDE to see the data. As a consequence, if you share links, you should pre-populate the cache too.
Your query is parametrized, and as a result, you do not have any cached results... BUT.... SEDE parameters are of the form ##Name:Type?Default##. Use this to your advantage. By supplying a default value your query will likely be 'prepopulated'
So, set up your parameters like:

##DaysSinceActivity:int?30##
##NumberOfUsers:int?1000##

That way, people clicking through will have the default values populated, and will possibly get a better experience.
Display Name
In SEDE you can sort the data based on the columns. The [User Link] column does not sort by the name though, it sorts by the URL for the user, which, in essence, does an alphabetic sort of the user-id because that is where the URL starts to change....
Add the Display Name in as a result column as well as the [User Link] so people can sort the data alphabetically.... (because people like to know where they are...).
Ranking
Consider adding a RANK column as well, so people an sort by other columns, and still see their ranking.
SQL
you have already seen inconsistencies in the capitalization of the keywords... You have chosen upper-case, which is fine, but then these should all be upper-case:

and
null
Count
is
etc.

Bug
You supply the last-access date as a parameter, but then you ignore it:
AND Users.LastAccessDate > DateAdd(Day, -31, GetDate())

code it to 31 all the time.... hmmm.


OOCSS: Is this broken down too much? Philosophical question: How far should I break down my objects?

Inuit.css and Bootstrap have islands, islets, wells, grayboxes, and other objects that have repeating properties. I don't think it was DRY enough, so I created a generic .box object & a few skins. I am just not sure if this is overkill?

Box Object

Used to fly out content from the rest of the document.

.box
{
    margin-bottom: 24px;

    padding: 24px;
}

.box--s
{
    padding: 12px;
}

.box--l
{
    padding: 48px;
}

Round Skin

Used to round the corners of an element.

.round
{
    border-radius: 4px;
}

.round--s
{
    border-radius: 2px;
}

.round--l
{
    border-radius: 8px;
}

Gray Skin

Generic color

.gray
{
    background-color: #eeee
}

Well Skin

Box specific skin to create wells

.well
{
    box-shadow: inset 2px 2px 2px rgba(0,0,0,.5);
}

It does seem more flexible - but still not sure if I am breaking them down way too much.

This is mostly a personal issue. But, in my opinion, there are 2 issues that are going wrong in your approach.

Making your objects reusable (The main issue in the DRY principle)

In my design, I want to have say 3 designs for boxes. Then, I spend sometime designing them. (that is, creating the style in the CSS). Then, when creating the HTML, I reuse that designs.

In your system, every time that I create a box in HTML, I have to re-design the box

<div class="box round gray darkshadow redtext">

At best, you will end copy-pasting the box from the previous page.
At worst, your web won't have a consistent look.

Hardcoding your style in the HTML

What should your CSS provide you ? The power to change your gray web, that is no longer fancy, to a blue one, without modifying your HTML. But, if your elements have a class that is gray, you will end with:

.gray {
    background-color: blue;
}

That, of course, will work; but won't give you an easy time understanding what is going on