Stock process program I have the following program and it's working as I had hoped but now I would like to tidy up the code, putting bits into new methods, so I'm not left with loads of code in the main method. I'm new to Java, so don't know how to go about this.

I have the following methods:

    public String getInventoryCode()
{
    return inventoryCode;
}

  public int getQuantityInStock()
{
    return quantityInStock;
}

public int getDailyDemand()
{
    return dailyDemand;
}

  public int getReOrder()
{
    return reOrder;
}

public int getLeadTime()
{
    return leadTime;
}

*/ Would like to use these */

*/ Would like to use instead of quantityIn -= item_1.getDailyDemand() in main method */

public void removeDailyDemand(){

    quantityInStock -= dailyDemand;
}

*/ Would like to use instead of quantityIn < reOrderThreshold  */

public void newDeliveryIn(){
    ;
}

*/ Would like to use instead of quantityIn += item_1.getReOrder() in main method */

public void isReOrderPoint(){
    ;
}

The bottom three methods I have made as I want to use them but I'm not exactly sure how, so they aren't currently doing anything, I have got all the rest to function as I had hoped.

Here is the main code:

/* Create new item - Inventory Code, Starting Quantity In Stock, Daily Demand, Re-order Amount, Time in Transit (Lead-time) */

    StockItem item_1 = new StockItem("A654Y", 1000, 50, 500, 5);

    /* Column titles */

            System.out.println("Day" + "\t" + "Stock" + "\t" + "Order Status");
            System.out.println("");

            /*Variables */

    int reOrderThreshold = 600;
    int orderTravelTime = 0;
    boolean orderInTravel = false;
    int quantityIn = item_1.getQuantityInStock();

    /* For loop to count 50 days */

    for (int n = 1; n <= 50; n++) {
        String out = "";

        /* Remove daily demand from current stock */
        quantityIn -= item_1.getDailyDemand();

        out += n;
        out += " " + "\t" + quantityIn + "\t";

        /* If order has been ordered and has not arrived */

        if (orderInTravel && orderTravelTime < item_1.getLeadTime()) {

            orderTravelTime++;

                            out += "In Transit -" + "\t";

        /* If ordered and has arrived */

        } else if (orderInTravel && orderTravelTime >= item_1.getLeadTime()) {
            orderTravelTime = 0;
            orderInTravel = false;

                            quantityIn += item_1.getReOrder();
                            out += "New Stock Delivered (" + item_1.getReOrder()")   and Will be Added to Quantity in Stock the Next Day";

        }
                    /* If stock gets too low, order */

        if (quantityIn < reOrderThreshold) {
            orderInTravel = true;

            out += "Ordered";
        }

                    /* Print out results */

        System.out.println(out);
    }

Here is part of the output:

Day   Stock   Order Status

1     950
2     900
3     850
4     800
5     750
6     700
7     650
8     600
9     550 Ordered
10    500 In Transit -    Ordered
11    450 In Transit -    Ordered
12    400 In Transit -    Ordered
13    350 In Transit -    Ordered
14    300 In Transit -    Ordered
15    250 New Stock Delivered (500) and Will be Added to Quantity in Stock the Next Day
16    700
17    650
18    600
19    550 Ordered
20    500 In Transit -    Ordered
21    450 In Transit -    Ordered
...

The first thing I should say is that your code looks very messy and requires "cleaning up". I don't know for sure if this is because of formatting problems when transferring from IDE to Code Review, but if it is not, then formatting needs work.
After formatting, it looks like this:
public class Test {

    public static void main(String[] args) {
        StockItem item_1 = new StockItem("A654Y", 1000, 50, 500, 5);
        System.out.println("Day" + "\t" + "Stock" + "\t" + "Order Status");
        System.out.println();
        int reOrderThreshold = 600;
        int orderTravelTime = 0;
        boolean orderInTravel = false;
        int quantityIn = item_1.getQuantityInStock();
        for (int n = 1; n <= 50; n++) {
            String out = "";
            quantityIn -= item_1.getDailyDemand();
            out += n;
            out += " " + "\t" + quantityIn + "\t";
            if (orderInTravel && orderTravelTime < item_1.getLeadTime()) {
                orderTravelTime++;
                out += "In Transit -" + "\t";
            } else if (orderInTravel && orderTravelTime >= item_1.getLeadTime()) {
                orderTravelTime = 0;
                orderInTravel = false;
                quantityIn += item_1.getReOrder();
                out += "New Stock Delivered ("
                        + item_1.getReOrder()
                        + ")   and Will be Added to Quantity in Stock the Next Day";
            }
            if (quantityIn < reOrderThreshold) {
                orderInTravel = true;
                out += "Ordered";
            }
            System.out.println(out);
        }
    }
}

class StockItem {

    String inventoryCode;
    int quantityInStock;
    int dailyDemand;
    int reOrder;
    int leadTime;

    public StockItem(String inventoryCode, int quantityInStock,
            int dailyDemand, int reOrder, int leadTime) {
        this.inventoryCode = inventoryCode;
        this.quantityInStock = quantityInStock;
        this.dailyDemand = dailyDemand;
        this.reOrder = reOrder;
        this.leadTime = leadTime;
    }

    public String getInventoryCode() {
        return inventoryCode;
    }

    public int getQuantityInStock() {
        return quantityInStock;
    }

    public int getDailyDemand() {
        return dailyDemand;
    }

    public int getReOrder() {
        return reOrder;
    }

    public int getLeadTime() {
        return leadTime;
    }

}

Note that this is not the final result. There are a lot more to add before it's good.
Now to the actual code...
First of all...
In the main method, there is this:
System.out.println("Day" + "\t" + "Stock" + "\t" + "Order Status");
System.out.println();

This could be easily changed to:
System.out.println("Day\tStock\tOrder Status\n");

This improves efficiency by reducing the time required to concatenate the Strings, as well as removing the requirement of the empty System.out.println(). In fact, you split the Strings where no split is necessary. " " + "\t" can just simply be " \t".
String out = "";

Remember that Strings are immutable. Since out is concatenated multiple times, you should use StringBuilder, and add Strings with out.append(...).
All the code in the for loop can be at least broken down into two methods. But before we do that, the variables should be instantiated outside of main, with static.
static StockItem item_1 = new StockItem("A654Y", 1000, 50, 500, 5);
static int reOrderThreshold = 600;
static int orderTravelTime = 0;
static boolean orderInTravel = false;
static int quantityIn = item_1.getQuantityInStock();

Then, let's break it away from the main method:
public static void main(String[] args) {
    System.out.println("Day\tStock\tOrder Status\n");
    for (int n = 1; n <= 50; n++) {
        doDay(n);
    }
}

private static void doDay(int day) {
    StringBuilder out = new StringBuilder();
    quantityIn -= item_1.getDailyDemand();
    out.append(day).append(" \t").append(quantityIn).append("\t");
    if (orderInTravel && orderTravelTime < item_1.getLeadTime()) {
        orderTravelTime++;
        out.append("In Transit -\t");
    } else if (orderInTravel && orderTravelTime >= item_1.getLeadTime()) {
        orderTravelTime = 0;
        orderInTravel = false;
        quantityIn += item_1.getReOrder();
        out.append("New Stock Delivered (").append(item_1.getReOrder()).append(")   and Will be Added to Quantity in Stock the Next Day");
    }
    if (quantityIn < reOrderThreshold) {
        orderInTravel = true;
        out.append("Ordered");
    }
    System.out.println(out);
}

And then draw out the if statements:
private static void doDay(int day) {
    StringBuilder out = new StringBuilder();
    quantityIn -= item_1.getDailyDemand();
    out.append(day).append(" \t").append(quantityIn).append("\t");
    checkOrder(out);
    checkStock(out);
    System.out.println(out);
}

private static void checkOrder(StringBuilder output) {
    if (orderInTravel && orderTravelTime < item_1.getLeadTime()) {
        orderTravelTime++;
        output.append("In Transit -\t");
    } else if (orderInTravel && orderTravelTime >= item_1.getLeadTime()) {
        orderTravelTime = 0;
        orderInTravel = false;
        quantityIn += item_1.getReOrder();
        output.append("New Stock Delivered (").append(item_1.getReOrder()).append(")   and Will be Added to Quantity in Stock the Next Day");
    }
}

private static void checkStock(StringBuilder output) {
    if (quantityIn < reOrderThreshold) {
        orderInTravel = true;
        output.append("Ordered");
    }
}

And there you go, small methods!
As for the extra methods, I think they are unnecessary.
Final code:
public class Main {

    static StockItem item_1 = new StockItem("A654Y", 1000, 50, 500, 5);
    static int reOrderThreshold = 600;
    static int orderTravelTime = 0;
    static boolean orderInTravel = false;
    static int quantityIn = item_1.getQuantityInStock();

    public static void main(String[] args) {
        System.out.println("Day\tStock\tOrder Status\n");
        for (int n = 1; n <= 50; n++) {
            doDay(n);
        }
    }

    private static void doDay(int day) {
        StringBuilder out = new StringBuilder();
        quantityIn -= item_1.getDailyDemand();
        out.append(day).append(" \t").append(quantityIn).append("\t");
        checkOrder(out);
        checkStock(out);
        System.out.println(out);
    }

    private static void checkOrder(StringBuilder output) {
        if (orderInTravel && orderTravelTime < item_1.getLeadTime()) {
            orderTravelTime++;
            output.append("In Transit -\t");
        } else if (orderInTravel && orderTravelTime >= item_1.getLeadTime()) {
            orderTravelTime = 0;
            orderInTravel = false;
            quantityIn += item_1.getReOrder();
            output.append("New Stock Delivered (")
                    .append(item_1.getReOrder())
                    .append(")   and Will be Added to Quantity in Stock the Next Day");
        }
    }

    private static void checkStock(StringBuilder output) {
        if (quantityIn < reOrderThreshold) {
            orderInTravel = true;
            output.append("Ordered");
        }
    }

}

class StockItem {

    String inventoryCode;
    int quantityInStock;
    int dailyDemand;
    int reOrder;
    int leadTime;

    public StockItem(String inventoryCode, int quantityInStock,
            int dailyDemand, int reOrder, int leadTime) {
        this.inventoryCode = inventoryCode;
        this.quantityInStock = quantityInStock;
        this.dailyDemand = dailyDemand;
        this.reOrder = reOrder;
        this.leadTime = leadTime;
    }

    public String getInventoryCode() {
        return inventoryCode;
    }

    public int getQuantityInStock() {
        return quantityInStock;
    }

    public int getDailyDemand() {
        return dailyDemand;
    }

    public int getReOrder() {
        return reOrder;
    }

    public int getLeadTime() {
        return leadTime;
    }

}


Searching purchase records, with or without a vendor criterion I'm trying to figure out a better way to have one query here. I want to be able to send something to last where statement a wildcard so I can select all vendors. Right now if I don't include that line it doesn't filter by vendor so I essentially get all the purchase requests.

Any thoughts of a cleaner way to do these queries?

 if @vendor == "0" #checks for vendor
    @purchase_requests = PurchaseRequest.includes(:purchase_order)
                          .where(:created_at => @date_start..@date_end)
                          .where(:total_cost => @cost_beginning..@cost_end)
  else
    @purchase_requests = PurchaseRequest.includes(:purchase_order)
                          .where(:created_at => @date_start..@date_end)
                          .where(:total_cost => @cost_beginning..@cost_end)
                          .where("purchaseorder.VendorRef_ListID = ?", @vendor)
  end

The simplest refactor would be:

# this is the base query
requests = PurchaseRequest.includes(:purchase_order)
             .where(:created_at => @date_start..@date_end)
             .where(:total_cost => @cost_beginning..@cost_end)

# then we add additional conditions if required
requests =
  unless @vendor == "0" # if vendor, get specific purchase request
    request.where("purchaseorder.VendorRef_ListID = ?", @vendor)
  end

If you find that you do certain queries often, the it's probably a chance to use scopes.  You may already know about scopes, but if not, check out Mastering ActiveRecord and Arel.

class PurchaseRequest < ActiveRecord::Base
  has_one :purchase_order

  # requests within a certain data
  scope :date_between, ->(date_range) {
    where(:created_at => date_range)
  }

  # requests with a certain cost range
  scope :cost_between, ->(date_range) {
    where(:total_cost => date_range)
  }

  # find request for a specific vendor
  scope :for_vendor, ->(vendor) {
    # this scope does an inner join to purchase_order and adds
    # a condition that the PurchaseOrder must belong to a specific
    # vendor.  That is, only requests matching the vendor are returned
    joins(:purchase_order).merge( PurchaseOrder.for_vendor(vendor) )
  }
end

class PurchaseOrder < ActiveRecord::Base
  # purchase orders belonging to a specific vendor
  scope :for_vendor, ->(vendor) {
    where(:VendorRef_ListID => vendor)
  }
end

So with scopes added, you can now refactor your code like so:

# this is the base query
requests = PurchaseRequest.includes(:purchase_order)
             .date_between(@date_start..@date_end)
             .cost_between(@cost_beginning..@cost_end)

# if vendor, get specific request
requests = request.for_vendor(@vendor) unless @vendor == "0"


Singly Linked List Implementation in C I'm preparing for an interview soon, and I've written the following linked list implementation from scratch.

This is all self-taught, and I'm still working on my C. I feel that the implementation is correct (if someone catches a corner case though, that would be great!), and I'm looking for feedback on best C practices.

For example: Is the spacing excessive? Are 1-line control flow statements okay? Is the constructor implementation idiomatic?

Header file:

#ifndef Interview_Prep_SLL_h
#define Interview_Prep_SLL_h

typedef struct SLLNode
{
    int val;
    struct SLLNode* next;
} SLLNode;

SLLNode* new_SLLNode(int val);
SLLNode* SLLappend(SLLNode* head, int val);
SLLNode* SLLinsert(SLLNode* head, int val, int ind);
SLLNode* SLLdelete(SLLNode* head, int val);
SLLNode* SLLreverse(SLLNode* head);
void SLLprint(SLLNode* head);

#endif

Source file:

// constructor
SLLNode* new_SLLNode(int val)
{
    SLLNode* head = malloc(sizeof(SLLNode));
    head->val = val;
    head->next = NULL;
    return head;
}

// add node to end
SLLNode* SLLappend(SLLNode* head, int val)
{
    if (head == NULL) return new_SLLNode(val);

    SLLNode* curr = head;

    while (curr->next != NULL) curr = curr->next;

    curr->next = new_SLLNode(val);

    return head;
}

// insert node at index (position) ind
SLLNode* SLLinsert(SLLNode* head, int val, int ind)
{
    if (head == NULL && ind > 0) return head;
    if (head == NULL && ind == 0) return new_SLLNode(val);

    SLLNode* curr = head;
    int count = 0;

    while (curr != NULL && count < ind - 1)
    {
        curr = curr->next;
        count++;
    }

    if (curr == NULL) return head;

    SLLNode* temp = curr->next;
    SLLNode* new = new_SLLNode(val);

    curr->next = new;
    new->next = temp;

    return head;
}

SLLNode* SLLdelete(SLLNode* head, int val)
{
    if (head == NULL) return head;

    if (head->val == val) return head->next;

    SLLNode* curr = head;

    while (curr->next != NULL && curr->next->val != val) curr = curr->next;

    if (curr->next == NULL) return head;

    SLLNode* temp = curr->next;
    curr->next = curr->next->next;
    free(temp);

    return head;
}

SLLNode* SLLreverse(SLLNode* head)
{
    if (head == NULL) return head;
    if (head->next == NULL) return head;

    SLLNode* curr = head;
    SLLNode* next = curr->next;
    SLLNode* nextnext = next->next;

    while (true)
    {
        if (curr == head) curr->next = NULL;

        next->next = curr;
        curr = next;
        next = nextnext;

        if (nextnext == NULL) return curr;

        nextnext = nextnext->next;
    }
}

// print values of nodes separated by spaces
void SLLprint(SLLNode* head)
{
    while (head != NULL)
    {
        printf("%d ", head->val);
        head = head->next;
    }
    printf("\n");
}

Overall this seems fine: a classing linked list implementation in C.
A couple of things can be simplified or improved here and there.

Instead of testing head == NULL twice:

if (head == NULL && ind > 0) return head;
if (head == NULL && ind == 0) return new_SLLNode(val);

It would be better to eliminate this duplication and do it only once:

if (head == NULL) {
    if (ind > 0) return NULL;
    if (ind == 0) return new_SLLNode(val);
}

Notice that I also changed return head to return NULL.
I know that head is NULL there,
but it's more explicit this way.

I have a feeling that after the while loop that iterates until ind to insert an element, when you do this:

if (curr == NULL) return head;

Possibly you really meant this instead:

if (curr == NULL) return curr;
// or
if (curr == NULL) return NULL;

I could be wrong, but perhaps not using explicit NULLs somehow led to mistakes like this. (I assume it was a mistake to return head when trying to insert beyond end + 1 of the linked list, which indicates a likely bug in the caller.)

  Is the spacing excessive?

No, I think it's just right.

  Are 1-line control flow statements okay?

They are ok. But keep in mind that if/for/while statements without a { ... } block can lead to nasty bugs, like in this famous example.
Some static analysis tools flag a warning for such statements.


Custom Sum implementation There is a custom summary function needed as to receive an integer input (e.g. 32456) and returns 3+2+4+5+6. Can you think of any other (or better) solution besides this?

public class CustomSum {
    public int sum(int input) {
        int sum = 0;

        while ( input >= 1 ) {
            sum += input % 10;
            input = input / 10;
        }

        return sum;
    }

    public static void main(String[] args) {
        int input = Integer.parseInt(args[0]); //assuming args[0] is integer

        CustomSum c = new CustomSum();
        int sum = c.sum(input);

        System.out.println("sum: " + sum);
    }
}

In every iteration I intended to extract the right digit by input % 10, and omit the extracted right digit for next iteration by input / 10. Now consider when we are in last iteration and have a 1 digit number. The input / 10 would be greater than 0 and less than 1 (if actually it was float or double not int). So I didn't actually changed that specific condition, although with int it will always be 0.

Your sum(...) function is fine, given the specification, but there is jut an issue of input validation... what about negative values?

Your current loop condition is input >= 1. This would be better written as input > 0. When you see the >= 1 it makes you wonder if there's a condition that's weird. In this case, there's not.

Further, your code excludes all negative values. What about the sum of -123?

That makes me think that the code should all be looped on the condition input != 0, but that would sum -123 as -6, and not 6.

I was initially tempted to suggest that you have the code:

    input = Math.abs(input);

    int sum = 0;

    while ( input != 0 ) {
        sum += input % 10;
        input = input / 10;
    }

    return sum;

Unfortunately, that has a flaw with Integer.MIN_VALUE (the abs of Integer.MIN_VALUE is.... Integer.MIN_VALUE - a negative number). The net result, is that I considered doing an abs on each digit value inside the loop, but then realized, that all the digits will be negative, so the 'sum' will just be a large negative value, and we can take the absolute of the sum safely, because the sum of the digits will never be large enough to be a problem...

Thus, the solution I would recommend is:

public static int sumSumAbs(int input) {

    int sum = 0;

    while ( input != 0 ) {
        sum += input % 10;
        input = input / 10;
    }

    return Math.abs(sum);
}

That code (adjusted to fit in to my Eclipse IDE with a static method call, and different name), will work for any input value, of any sign, and ignore the sign in the result.


Find maximum value of Point represented by an Array of values for each coordinate Suppose there is an array A = {3,2,5}, then points for this array correspond to indexes (i,j) where i <= j = {(0,0), (0,1), (0,2), (1,1), (1,2), (2,2)}. The program returns max(sum) such that sum = A[i] + A[j] + (j - i). For the above array :

point (0,0) will have sum = (3 + 3) + (0 - 0) = 6
point (0,1) will have sum = (3 + 2) + (1 - 0) = 6
point (0,2) will have sum = (3 + 5) + (2 - 0) = 10
point (1,1) will have sum = (2 + 2) + (0 - 0) = 4
point (1,2) will have sum = (2 + 5) + (2 - 1) = 8
point (2,2) will have sum = (5 + 5) + (2 - 2) = 10

Therefore program should return max(sum) = 10
I have solved this problem, but I'm struggling to reduce the complexity of the solution to O(n). Is there a more elegant, efficient way to this?

static int solution(int[] arr) {
    //when array is empty
    if(arr.length == 0) {
        return 0;
    }
    //when array is of length 1
    if(arr.length == 1) {
        return 2*arr[0];
    }
    List<Integer> list1 = new ArrayList<Integer>();
    for(int i=0;i<arr.length;i++) {
        list1.add(arr[i]);
    }
    //copy old list to new one and sort the new one
    List<Integer> list2 = new ArrayList<Integer>(list1);
    Collections.sort(list2);
    //replace values to indexes using 1st list
    for(int i=0;i<list2.size();i++) {
        list2.add(i,list1.indexOf(list2.remove(i)));
    }
    //sort old list. Now my 1st list contains sorted values, and 2nd one       contains corresponding old indexes
    Collections.sort(list1);
    int i = list1.size()-1;
    //set max to point (n-1,n-1)
    int max = 2*list1.get(i);
    while(i > 0) {
        int j = 1;
        while(j < i) {
            int sum = (list1.get(i) + list1.get(i-j)) + (list2.get(i) -list2.get(i-j));
            if(sum > max) {
                max = sum;
            }
            j++;
        }
        i--;
    }
    return max;
}

I'm not a Lisp guy, but parenthesis are powerful. Change this:

sum = A[i] + A[j] + (j - i)

to that:

sum = (A[i] - i) + (A[j] + j)

Both things are equivalent, but the second one looks quite different.
Its a sum of two numbers and each one only depends on its own index, there's a term based on i and one on j (and the array A of course).

A sum (of independent summands) becomes max, if each summand is max.

The thing is that being independent means that you can calculate both maximums "parallel" to each other while iterating through the array.
I called them iMax, which is  value - index and jMax which is value + index

Implementation is straight forward:

public class ElementIndexSum
{
    public static void main (String[] args)
    {
        int[] A = {2,3,5};

        int iMax = Integer.MIN_VALUE, jMax = Integer.MIN_VALUE; // minimum possible value serves as neutral starting point to find maximum

        for (int index = 0; index < A.length; ++index)
        {
            // find maximum value from values in array
            iMax = Math.max(iMax, A[index] - index); // with subtraction of its index
            jMax = Math.max(jMax, A[index] + index); // with addition of its index
        }

        System.out.println("maximum( A[i] + A[j] + i - j ) = " + (iMax + jMax));
    }
}

Result in console:

$ java ElementIndexSum
maximum( A[i] + A[j] + i - j ) = 10

I have no clue about this bick Oh stuff, but if your goal was to iterate only once and not in a nested way, I think the above is what you are looking for.


Change random methods without losing too much performance I have a class in C# that uses a Random object to get a list of numbers randomized from an array of 1-25. Now what I need is improve this method to use RNGCryptoServiceProvider because it is not random enough for the application.

I know that RNGCryptoServiceProvider is slower than Random, but I need to change it and I don't know anything about this class.  How big should the Buffer storage be so as to not slow down the client PC too much?

What I'm passing to the method is this array:

string[] num25 = { "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12", "13", "14", "15", "16", "17", "18", "19", "20", "21", "22", "23", "24", "25" };

This is the actual code:

public static class RandomStringArrayTool
{
    /// <summary>
    /// Stores the current random number
    /// </summary>
    static Random _random = new Random();

    /// <summary>
    /// Return randomized version of the string array
    /// </summary>
    public static string[] RandomizeStrings(string[] arr)
    {
        List<KeyValuePair<int, string>> list = new List<KeyValuePair<int, string>>();
        // Add all strings from array
        // Add new random int each time
        foreach (string s in arr)
        {
            list.Add(new KeyValuePair<int, string>(_random.Next(), s));
        }
        // Sort the list by the random number
        var sorted = from item in list
                     orderby item.Key
                     select item;
        // Allocate new string array
        string[] result = new string[arr.Length];
        // Copy values to array
        int index = 0;
        foreach (KeyValuePair<int, string> pair in sorted)
        {
            result[index] = pair.Value;
            index++;
        }
        // Return copied array
        return result;
    }
}

First of all, you should shuffle the array instead of sorting it on a random number. There can be duplicates in the random numbers, which means that lower numbers would end up earlier in the array slightly more often. This is naturally a small difference as the random numbers are large, but as you want to avoid any predictable pattern, it is still important.

Look at the Fisher-Yates algorithm for shuffling the values in an array.

To use the crypto random generator, you need a way to transform the bytes that it returns into a number in a specific range. You can use a method like the following to get a number in a range with the same probability for all numbers in the range (limited only by the randomness of the generator). It calculates the largest range of numbers that can be used to create a number in the desired range, then loops until it has picked a number in that range.

public static int GetInt(RNGCryptoServiceProvider rnd, int max) {
  byte[] r = new byte[4];
  int value;
  do {
    rnd.GetBytes(r);
    value = BitConverter.ToInt32(r, 0) & Int32.MaxValue;
  } while (value >= max * (Int32.MaxValue / max));
  return value % max;
}

This is based on the example on documentation page for the GetBytes method, but adapted for larger numbers.

On my computer the function will generate a million random numbers in 450 ms. The Random class does it in 20 ms, so it's slower, but not very slow. Shuffling on the other hand is faster than sorting, so you will get some performance back there.


Matching any certain selection of inputs I have the following RegEx which matches any of a selection of inputs (see the list below).

While it works fine, I'm wondering if there is a better way of writing it, as it looks a little dirty.  I've been using various RegEx builders but couldn't come up with a shorter version, so any tips would be appreciated.

-[0-9]+px$|^[0-9]+px$|^[0-9]+em$|^-[0-9]+em$|^[0-9]+\%$|^auto$|^0$

Here is the list of valid entries (where 10 can be any integer):

10px
-10px
10em
-10em
10%
auto
0

You can combine px and em into a single group
You can pull in the negation sign with a ?
[0-9] is equivalent (usually) to \d
^ and $ can be pulled out to the end (or use the api than only matches the entire string.

this results in:

^(-?\d+(px|em)|\d+%|auto|0)$

If you can have a negative % values then it's even simpler: ^(-?\d+(px|em|%)|auto|0)$


Logger class for logging connections I have this class whose code I pasted in full. The CredentialsManager class is not shown here as all it does is return the DB connection variables.

Can be improved or is this is OK as I've written it? Namely, I'm interested if it's OK to have a private variable mysqli populate inside the function openConnection() or if I should have done that in the constructor.

I'm pretty confident it would be a bad thing to have the same code for opening a connection in each function of the class, which is why I've put the connection opening code in a function itself.

<?php
class Logger
{
    // private variables
    private $_host;
    private $_database;
    private $_username;
    private $_password;
    private $_mysqli;

    // public functions
    public function __construct()
    {
        $credientials = CredentialsManager::GetCredentials();
        $this->_host = $credientials['host'];
        $this->_database = $credientials['database'];
        $this->_username = $credientials['user'];
        $this->_password = $credientials['pass'];
    }

    public function RecordLog($log){
        if ($this->openConnection()){
            if (!($stmt = $this->_mysqli->prepare("INSERT INTO logz (log) VALUES (?)"))) {
                echo "Prepare failed: (" . $this->_mysqli->errno . ") " . $this->_mysqli->error;
            }

            if (! $stmt->bind_param("s", $log)){
                echo "Binding failed: (". $stmt->errno .") " . $stmt->error;
            }

            if (! $stmt->execute() ){
                echo "Execute failed: " . $stmt->error;
            }

            if ($stmt->affected_rows > 0)
                return true;
            else
                return false;
        }
        else
            return false;
    }

    // private functions
    private function openConnection()
    {
        $this->_mysqli = new mysqli($this->_host, $this->_username, $this->_password, $this->_database);
        if ($this->_mysqli->connect_errno) {
            echo "Failed to connect to MySQL: " . $this->_mysqli->connect_error;
            return false;
        }

        return true;
    }
}
?>

The first suggestion I would like to make is for you to become familiar with Inversion of Control (IoC) and Dependency Injection (DI). These two principles state that dependencies should not be hardcoded into your classes, but should be "injected" into them. This ensures that your class does not become dependent upon a single entity and can thus be extended. This is most commonly combined with type hinting.

Type hinting ensures that a parameter is of the proper type, otherwise PHP will terminate execution of the script with an error. You can use the class you are expecting, or you can instead use a parent class or interface so that any similar class can be used, thus making it more extensible. I will demonstrate using the former as I don't know if there is a parent class or interface to use.

public function __construct( CredentialsManager $credentials ) {
    //etc...
}

//usage
$log = new Logger( CredentialsManager::GetCredentials() );

You should also become aware of the Arrow Anti-Pattern. Through it you learn that unnecessarily indented code is bad. To illustrate this they usually show code that comes to peaks to form an arrow. The easiest way to refactor your code to avoid this anti-pattern is to return early. For example, if we reverse your first if statement in your RecordLog() method we can return early thus making the else statement unnecessary and removing an entire level of indentation from that method.

if( ! $this->openConnection() ) {
    return FALSE;
}

//etc...

Speaking of that else statement. PHP inherently requires braces, otherwise you would not be forced to add them after your statements reached more than one line. Adding them helps ensure no mistakes are made and keeps your code consistent. I would strongly recommend adding braces, even if you think they are unnecessary.

else {
    return FALSE;
}

Always avoid assigning variables in statements. There are quite a few reasons for this:

They are extremely difficult to debug. I actually missed this the first time through and thought you were using globals.
They decrease legibility. Usually they increase line length and make statements more complex.
There is usually no way for the reader, or your IDE, to determine if this was done accidentally. Therefore it is fairly easy to accidentally assign a value to a variable when you actually meant to compare it.

There are exceptions to this rule, such as when setting up an iterator, but typically this should be avoided.

//common
for( $i = 0; $i < $count; $i++ ) {
//not as common, but fine
while( ( $buffer = fgets( $handle, 4096 ) ) !== false ) {
//as it should be
$stmt = $this->_mysqli->prepare( 'INSERT INTO logz (log) VALUES (?)' );
if( ! $stmt ) {

When you want a boolean return value that reflects a boolean state of what you are comparing, you can simply return the comparison and skip the unnecessary if/else structure.

return $stmt->affected_rows > 0;

Your RecordLog() method violates the Single Responsibility Principle. This principle states that your methods should do one thing and delegate everything else. Instead you have this method doing everything. A better way to rewrite this is to call the logger whenever an error is encountered and pass that error to it. Instead you are calling the logger to perform the functionality that might produce errors in order to catch them. This is backwards.

if( ! $this->performAction() ) {
    $this->RecordLog( $error );
}

Expanding upon this concept, and those mentioned previously, our classes should be equally singular. It may not be as obvious, but your logger should really only be concerned with logging the errors, not with the how of it. If you instead inject the database into this class, then that database can change without needing to modify the logger. So you should be able to go from using a MySQLi database to PDO or any other database without needing to modify this class. Though this is more advanced and I wouldn't suggest trying to tackle this until you are more comfortable with the rest.

$log = new Logger( $mysqli );
$log = new Logger( $pdo );

The way you are currently connecting to your database restarts the connection every time RecordLog() is called. This is bad. You only need one connection per instance of the class. Creating it in the constructor ensures this. In fact, since you are not reusing any of those connection properties, you could probably get away with openConnection() BEING the constructor. If you just directly apply the credentials then declaring them as properties will become unnecesary.

$this->_mysqli = new mysqli(
    $credientials[ 'host' ],
    $credientials[ 'user' ],
    $credientials[ 'pass' ],
    $credientials[ 'database' ]
);

If you take my advice about that RecordLog() method, then you can "log" the connection error from the constructor as well, thus making your code more uniform.

if( $this->_mysqli->connect_errno ) {
    $this->RecordLog(
        "Failed to connect to MySQL: "
        . $this->_mysqli->connect_error
    );
}

So, something you should begin to notice from all of these suggestions is that your class now doesn't do much. You have a constructor and a logger. The constructor is a wrapper for the MySQLi class' constructor and as such can be abstracted out. This leaves just the logger method, and if we look at what this is doing, then we can also determine that this is unnecessary. Assuming you are planning on modifying this functionality to do more than just echo the error, then that leaves just the one method. This is not enough to justify a class. A function yes, but not a class. I would say forgo the class and just use PHP's MySQLi class directly. Extending, or in this case, pseudo extending, a class is unnecessary unless you are planning on doing something DRASTIC with that extension. Just creating a connection is not enough. However, it is a good start to trying to understand OOP concepts.


Two methods for finding a sprite in sprite sheet C++, which is better? I have two snippets of code that I would like you to look at. I can't figure out which of the two is most efficient (or better practice, if applicable). I'm currently finding the row and column of a sprite in a sprite sheet in order to render it to the screen; which works find and dandy. However, I now have two algorithms:

const double Testvalue = 155.0f; //This values changes, could be any natural number.
const double imageWidth = 614.0f;
const double SpriteAverageSize = 34.0f; //floor(value / spriteAverageSize) gives us max rows (which is this case is 18).

double numRows = floor(imageWidth / SpriteAverageSize);
double RowValue = ceil(Testvalue / numRows);
double ColumnValue = numRows - ((numRows  * RowValue) - Testvalue);

//This will do the following operation:
//numRows = floor(614.0/ 34.0) -> floor(18.05..) -> 18.0.
//RowValue = ceil(155.0/ 18.0) -> ceil(8.611..) -> 9.0.
//ColumnValue = 18.0 - ((18.0 * 9.0) - 155.0) -> 18.0 - (162.0 - 155.0) -> 18.0 - 7.0 -> 11.0.

//Row: 9 ; Column: 11.

or

const double Testvalue = 155.0f;
const double imageWidth = 614.0f;
const double SpriteAverageSize = 34.0f;

double RowValue = ceil(Testvalue / floor(imageWidth / SpriteAverageSize));
double ColumnValue = floor(imageWidth / SpriteAverageSize) - ((floor(imageWidth / SpriteAverageSize) * RowValue) - Testvalue);

//This will do the following operation:
//RowValue = ceil(155.0 / floor(614.0 / 34.0)) -> ceil(155.0 / floor(18.05..)) -> ceil(155.0 / 18.0) -> ceil(8,61..) -> 9.0.
//ColumnValue = floor(614.0 / 34.0) - ((floor(614.0 / 34.0) * 9.0) - 155.0) -> floor(18.05..) - ((floor(18.05..) * 9.0) - 155.0) -> 18.0 - ((18.0 * 9.0) - 155.0) -> 18.0 - 7.0 -> 11.0.

//Row: 9 ; Column: 11

To clarify, these two work perfectly (I have also implemented it as a method in my class to do the operation as I get the data from an XML file), I get the right row and column value as I've tested several times and implementing it works too. I would just like some tips on which of the two algorithms you'd prefer in a block of code and which of the two is more efficient?

I prefer the first one, because having the named variable numRows:

Certainly makes the code more "self-documenting": it's easier for me (the programmer) to understand what the algorithm is doing
May make the code faster: because you only do the floor(imageWidth / SpriteAverageSize) calculation once instead of three times (although it's possible that an optimizing compiler might be able to make that optimization itself, even without your 'hint' that it's a reused sub-expression)


Database abstraction class in C# Here is the link to the revised version of this code:
Database abstraction class in C# (version 2)

I am new to C# and have coded a database abstraction class that I would like to get comments on.  It uses prepared SQL statements.  I would like this to not get distracted with prepared statements versus non-prepared statements discussion; please just review the code assuming prepared statements are appropriate for this class.

class DatabaseAccess
{
    private SqlCommand alreadyDownloadedCommand;
    private SqlCommand markedDownloadedCommand;

    public DatabaseAccess()
    {
        SqlConnection myConnection = new SqlConnection("user id=username;" +
                                   "password=password;server=serverurl;" +
                                   "Trusted_Connection=yes;" +
                                   "database=database; " +
                                   "connection timeout=30");

        myConnection.Open();
        alreadyDownloadedCommand = getAlreadyDownloadedCommand(myConnection);
        markedDownloadedCommand = getMarkedDownloadedCommand(myConnection);
    }

    private SqlCommand getMarkedDownloadedCommand(SqlConnection connection)
    {
        const string markDownloadedInsert =
        @"insert into dbo.InspectionsDownloaded(FileName)
          values (@FileName)
        ";
        SqlCommand command = new SqlCommand(null, connection);
        command.CommandText = markDownloadedInsert;
        command.Parameters.Add(new SqlParameter("@FileName", System.Data.SqlDbType.Text, 80));

        return command;
    }
    private SqlCommand getAlreadyDownloadedCommand(SqlConnection connection)
    {
        const string alreadyDownloadedQuery =
        @"select count(*) as numDownloaded
            from dbo.InspectionsDownloaded id
           where id.FileName = @FileName
        ";
        SqlCommand command = new SqlCommand(null, connection);
        command.CommandText = alreadyDownloadedQuery;
        command.Parameters.Add(new SqlParameter("@FileName", System.Data.SqlDbType.Text, 80));

        return command;
    }
    public Boolean AlreadyDownloaded(String filename)
    {
        Int32 rowCount;
        alreadyDownloadedCommand.Parameters[0].Value = filename;
        rowCount = (Int32)alreadyDownloadedCommand.ExecuteScalar();
        return (rowCount != 0);
    }
    public void MarkDownloaded(String filename)
    {
        markedDownloadedCommand.Parameters[0].Value = filename;
        markedDownloadedCommand.ExecuteNonQuery();
    }
}

I am new to C# and have coded a database abstraction class that I
  would like to get comments on.  It uses prepared SQL statements.

No, it doesn't. If you want it to use prepared statements then you have to call Prepare() on the SqlCommand instances.

Whether you really want actual prepared statements or whether you just want to leave escaping to the library is, as you say, up to you.

class DatabaseAccess
{
    private SqlCommand alreadyDownloadedCommand;
    private SqlCommand markedDownloadedCommand;

Both SqlCommand and SqlConnection are IDisposable. Best practices would require you to make DatabaseAccess implement IDisposable by disposing of all three.

        SqlConnection myConnection = new SqlConnection("user id=username;" +
                                   "password=password;server=serverurl;" +
                                   "Trusted_Connection=yes;" +
                                   "database=database; " +
                                   "connection timeout=30");

If this is for a throwaway project, hard-coding the connection string like that is acceptable. If it's for a "serious" project then you should really get the connection string from App.config with System.Configuration.ConfigurationManager.ConnectionStrings. That has the benefit of allowing you to have dev and prod configurations, as well as to handle server migrations, password cycling, etc. without recompiling.

        SqlCommand command = new SqlCommand(null, connection);
        command.CommandText = markDownloadedInsert;

Why not this?

     SqlCommand command = new SqlCommand(markDownloadedInsert, connection);

        command.Parameters.Add(new SqlParameter("@FileName", System.Data.SqlDbType.Text, 80));

...

    public void MarkDownloaded(String filename)
    {
        markedDownloadedCommand.Parameters[0].Value = filename;

If filename is longer than 80 characters, it will be silently truncated. I would think it advisable to check the length and throw an ArgumentOutOfRangeException if it's too long. Similarly in the other method.

    public Boolean AlreadyDownloaded(String filename)
    {
        Int32 rowCount;

Finally, some debatable points of style. I think it's quite unusual to use Boolean, String, Int32 instead of the aliases bool, string, int. And it's fairly conventional that a method (or property) which tests a condition and returns a bool should have a name beginning Is: here IsAlreadyDownloaded.


Application of higher-order function in Python If \$f\$ is a numerical function and \$n\$ is a positive integer, then
  we can form the \$n\$th repeated application of \$f\$, which is
  defined to be the function whose value at \$x\$ is
  \$f(f(...(f(x))...))\$. For example, if \$f\$ adds 1 to its argument,
  then the \$n\$th repeated application of \$f\$ adds \$n\$. Write a
  function that takes as inputs a function \$f\$ and a positive integer
  \$n\$ and returns the function that computes the \$n\$th repeated
  application of \$f\$:

def repeated(f, n):
    """Return the function that computes the nth application of f.

    f -- a function that takes one argument
    n -- a positive integer

    >>> repeated(square, 2)(5)
    625
    >>> repeated(square, 4)(5)
    152587890625
    """
    "*** YOUR CODE HERE ***"

Below is the solution:

from operator import mul

def repeated(f, n):
    """Return the function that computes the nth application of f.

    f -- a function that takes one argument
    n -- a positve integer

    >>> repeated(square, 2)(5)
    625
    >>> repeated(square, 4)(5)
    152587890625
    """
    def g(x):
        i = 1
        while i <= n:
            x, i = f(x), i + 1
        return x
    return g

def square(x):
    return mul(x, x)

print(repeated(square,4)(2))

I've tested it and it looks fine.

Can I optimise this code better? Do you think I can use better names instead of i & g?

Nice docstring.

Your loop is too complicated, and not idiomatic Python.  Use range(n) to repeat n times:

def repeated(f, n):
    """Docstring here"""
    def g(x):
        for _ in range(n):
            x = f(x)
        return x
    return g

Your repeated() function works fine for functions that accept a single argument, like square(x) (which could just be written x * x, by the way).  However, it fails for higher-arity functions, such as

def fib_iter(a, b):
    return b, a + b

To handle multi-argument functions…

def repeated(f, n):
    """Docstring here"""
    def g(*x):
        for _ in range(n):
            x = f(*x) if isinstance(x, tuple) else f(x)
        return x
    return g

However, there is a bug in that: repeated(square, 0)(2) would return a tuple (2,) rather than an int 2.  To work around that special case…

def repeated(f, n):
    """Docstring here"""
    def g(*x):
        for _ in range(n):
            x = f(*x) if isinstance(x, tuple) else f(x)
        return x

    def unpack(*x):
        result = g(*x)
        if isinstance(result, tuple) and len(result) == 1:
            return result[0]
        else:
            return result

    return unpack


Implementation of callback in database request I am a complete beginner in JavaScript / Node.js / Express, but have some experience with php.

I am trying to pass data from a SQLite database to the console. In my previous attempts, console.log() was executed before the database got queried. Now I got it to work in the correct order with what I think is a callback, and I'm getting the data in the console.

Questions:

-Is this an actual callback?

-Is there a better (shorter / easier) way to "do this after that"?

-If I want to execute multiple functions in a certain order, would I want to end up with something like doThis(doThat(thenDoThis(finallyDoThat)));?

-If there's anything else about the code, feel free to review and comment.

// Some test code to get data from database

function getAllChampions(callback){
    db.all("SELECT * FROM champions", function(error, rows){
        if (error) console.log(error);
        return callback(error, rows);
    });
};

function showAllChampions(error, champions){
    if (error) console.log(error);
    else console.log('Champions: ' + JSON.stringify(champions));
};

getAllChampions(showAllChampions);

If you run multiple async functions in a certain orderi it will look like something like this :

getData(function(a){
    getMoreData(a, function(b){
        getMoreData(b, function(c){
            getMoreData(c, function(d){
                getMoreData(d, function(e){
                    ...
                });
            });
        });
    });
});

but it does not look nice and hard to control. Instead use promises, I recommend bluebirdjs library If you use nodejs, take it easy.


Searching posts/comments by keywords I've put together another simple query that allows you to search post/comments with specific keywords. At the moment, only post bodies are checked.

DECLARE @search_keywords NVARCHAR(MAX) = ##SearchKeywords:string##;
DECLARE @search_type_id NVARCHAR(MAX) = ##SearchTypeId:string##;

IF (LOWER(@search_type_id) = 'post')
BEGIN
  SELECT
      Id AS [Post Link]
    , OwnerUserId AS [User Link]
    , Score
    , Tags
  FROM Posts WHERE
    LOWER(Body) LIKE LOWER(
      CONCAT('% ', REPLACE(@search_keywords, ' ', '%%'), ' %')
    )
  ORDER BY Score DESC;
END

ELSE IF (LOWER(@search_type_id) = 'comment')
BEGIN
  SELECT
      Id AS [Comment Link]
    , UserId AS [User Link]
    , Score
    , Text
  FROM Comments WHERE
    LOWER(Text) LIKE LOWER(
      CONCAT('% ', REPLACE(@search_keywords, ' ', '%%'), ' %')
    )
  ORDER BY Score DESC;
END

ELSE
BEGIN
  PRINT 'SearchTypeId must either be "comment" or "post".';
END

Here's some sample input. Single quotes, '', are not needed for the input.

SearchKeywords: :)
SearchTypeId:   comment

Let's start with your input parameters, since that is SEDE-specific. Did you know you can add parameter hints to make the experience easier for users? See the SEDE Help Page. So we could do something like this:

-- SearchKeywords: Search for word(s) "No need for quote marks"
DECLARE @search_keywords NVARCHAR(MAX) = ##SearchKeywords:string##;
-- SearchTypeId: post / comment "No need for quote marks"
DECLARE @search_type_id NVARCHAR(MAX) = ##SearchTypeId:string##;##;

That way it looks like this instead, including the little tool tip:

That said, there are some things we could improve. As it stands, when you search for a post there are a few deficiencies in the result set:

No indication whether it is a Question, Answer, or Other type of post.

This is easy to improve. Let's declare two new variables. We will reuse them for your comments query, so let's declare them at the top.

DECLARE @question INT = (SELECT Id FROM PostTypes WHERE Name = 'Question');
DECLARE @answer INT = (SELECT Id FROM PostTypes WHERE Name = 'Answer');

Then we can simply use a CASE statement to add in the type of post, like this:

, CASE
    WHEN PostTypeId = @Question THEN 'Question'
    WHEN PostTypeId = @Answer THEN 'Answer'
    ELSE 'Other'
    END AS [Post Type]

This will show you another deficiency:

Non-Question posts are missing tags!

The reason, as I'm sure you know, is that only Question posts have Tags. What we can do then is use a self-referencing join to get the Tags from the parent questions, again using a CASE statement, like this:

, CASE
    WHEN PostTypeId = @Question THEN Tags
    WHEN PostTypeId = @Answer THEN (SELECT parent.Tags FROM Posts AS parent INNER JOIN Posts AS child ON child.ParentId = Parent.Id WHERE child.Id = Posts.Id)
    ELSE NULL END AS [Tags]

You could break down that one-liner subquery with line breaks, if it makes it easier to read for you.

So we're in pretty good shape for the post search now! Let's look at the comment query.

I think it would be good information to know what kind of post the comment was made on, so why don't we add a join to the Posts table, and get that information from there, using again a CASE statement:

SELECT TOP 10
    Comments.Id AS [Comment Link]
  , Comments.UserId AS [User Link]
  , CASE
      WHEN Posts.PostTypeId = @Question THEN 'Question'
      WHEN Posts.PostTypeId = @Answer THEN 'Answer'
      ELSE 'Other'
      END AS [Post Type]
  , Comments.Score
  , Comments.Text

FROM Comments
INNER JOIN Posts ON Posts.Id = Comments.PostId

Note that I had to reference the tables for each column otherwise it would be an ambiguous join and give you an error.

If you wanted to you could also use the same method as above to get the post tags, although that may not be desired with comments. Also, in both cases, perhaps consider adding the CreationDate field which is useful for sorting.

With everything put together, I came up with this query:

-- SearchKeywords: Search for word(s) "No need for quote marks"
DECLARE @search_keywords NVARCHAR(MAX) = ##SearchKeywords:string##;
-- SearchTypeId: post / comment "No need for quote marks"
DECLARE @search_type_id NVARCHAR(MAX) = ##SearchTypeId:string##;

DECLARE @question INT = (SELECT Id FROM PostTypes WHERE Name = 'Question');
DECLARE @answer INT = (SELECT Id FROM PostTypes WHERE Name = 'Answer');

IF (LOWER(@search_type_id) = 'post')
BEGIN

  SELECT
      Id AS [Post Link]
    , OwnerUserId AS [User Link]
    , Score
    , CASE
        WHEN PostTypeId = @Question THEN 'Question'
        WHEN PostTypeId = @Answer THEN 'Answer'
        ELSE 'Other'
        END AS [Post Type]
    , CASE
        WHEN PostTypeId = @Question THEN Tags
        WHEN PostTypeId = @Answer THEN (SELECT parent.Tags FROM Posts AS parent INNER JOIN Posts AS child ON child.ParentId = Parent.Id WHERE child.Id = Posts.Id)
        ELSE NULL END AS [Tags]
    , CreationDate
  FROM Posts
  WHERE
    LOWER(Body) LIKE LOWER(
      CONCAT('% ', REPLACE(@search_keywords, ' ', '%%'), ' %')
    )
  ORDER BY Score DESC;
END

ELSE IF (LOWER(@search_type_id) = 'comment')
BEGIN
  SELECT
      Comments.Id AS [Comment Link]
    , Comments.UserId AS [User Link]
    , CASE
        WHEN Posts.PostTypeId = @Question THEN 'Question'
        WHEN Posts.PostTypeId = @Answer THEN 'Answer'
        ELSE 'Other'
        END AS [Post Type]
    , Comments.Score
    , Comments.Text
    , Comments.CreationDate AS [Comment Creation Date]
    , Posts.CreationDate AS [Post Creation Date]
  FROM Comments
    INNER JOIN Posts ON Posts.Id = Comments.PostId
  WHERE
    LOWER(Text) LIKE LOWER(
      CONCAT('% ', REPLACE(@search_keywords, ' ', '%%'), ' %')
    )
  ORDER BY Score DESC;
END

ELSE
BEGIN
  PRINT 'SearchTypeId must either be "comment" or "post".';
END


A recursive function that performs a binary search I created a recursive function that uses binary search to just return true if it finds the value and false if it does not.

I'm new to recursion and binary search so please let me know where I can improve upon.

/**
* Uses binary search O(log n). Returns true if the values is in the value array false if it's not. Recursive
*/
bool binarySearch( int value, int values[], int left, int right ) {

    //keep track of when the array will be empty and return false
    if ( right < left ) {
        return false;
    }

    //Find the middle of the array
    int mid = ( left + right ) / 2;

    //compare the value to the middle of the array. If it's a match return true, else move the left position and right position accordingly
    if( value == values[ mid ] ) {
        return true;
    } else if ( value < values[ mid ] ) {
        right = mid - 1;
    } else {
        left = mid + 1;
    }

    //return the function
    return binarySearch( value, values, left, right );
}

You return a boolean to indicate whether the value was found or not.  The function would be more useful if you returned an index at which the value was found (or -1 if not found).  It's better information for the same amount of work.

It appears that left is the leftmost index to consider, and right is the rightmost index to consider.  It would be more idiomatic to follow the convention of inclusive-exclusive bounds, with left being the leftmost index and right being just beyond the last element.  That way, right - left indicates the number of elements in the array.

You've implemented the search using recursion, but it could also be done using just a loop.  You would avoid the overhead of function calls and the possibility of stack overflow.

int mid = ( left + right ) / 2 is vulnerable to integer overflow if both indices are large.  int mid = left + (right - left) / 2 would not overflow.


Refactoring my controller specs In my project, I have this kind of code to test my controllers:

#encoding=utf-8
require 'spec_helper'

describe ProjectsController do
  let!(:project) { build_stubbed(:project, user: user) }
  let(:user) { build_stubbed(:user) }

  before(:each) { sign_in user }

  describe "GET 'index'" do
    it "render index template" do
      get 'index'
      should render_template('index')
    end
  end

  describe "GET 'show'" do
    let!(:comment) { build_stubbed(:comment) }

    before(:each) { Project.stubs(:find).returns(project) }

    it "render show template" do
      get 'show', id: project.id
      should render_template('show')
    end

    it "create a new comment" do
      Comment.expects(:new).returns(comment)
      get 'show', id: project.id
    end
  end

  describe "GET 'new'" do
    it "render new template" do
      get 'new'
      should render_template('new')
    end
  end

  describe "POST 'create'" do
    before(:each) do
      Project.stubs(:new).returns(project)
    end

    context "with valid data" do
      before(:each) { project.stubs(:save).returns(true) }

      it "redirect to project's path" do
        post 'create'
        should redirect_to(project)
      end

      it "save the project" do
        lambda do
          post 'create'
        end.should change(user.projects, :size).by(1)
      end

      it "set a flash message" do
        post 'create'
        should set_the_flash[:notice].to("Votre projet a été ajouté")
      end
    end

    context "with invalid data" do
      before(:each) { project.stubs(:save).returns(false) }

      it "render new template" do
        post 'create'
        should render_template('new')
      end
    end
  end

  describe "GET 'edit'" do
    before(:each) do
      Project.stubs(:find).returns(project)
    end

    it "render edit template" do
      get 'edit', id: project.id
      should render_template('edit')
    end
  end

  describe "POST 'update'" do
    before(:each) do
      Project.stubs(:find).returns(project)
    end

    context "when valid" do
      before(:each) { project.stubs(:update_attributes).returns(true) }

      it "redirect to project's path" do
        post 'update', id: project.id
        should redirect_to(project)
      end

      it "update the project" do
        project.expects(:update_attributes)
        post 'update', id: project.id
      end

      it "set a flash message" do
        post 'update', id: project.id
        should set_the_flash[:notice].to("Votre projet a été modifié")
      end
    end

    context "when invalid" do
      before(:each) { project.stubs(:update_attributes).returns(false) }

      it "render edit template" do
        post 'update', id: project.id
        should render_template('edit')
      end
    end
  end

  describe "DELETE 'destroy'" do
    before(:each) do
      Project.stubs(:find).returns(project)
      project.stubs(:destroy)
    end

    it "redirect to project's path" do
      delete 'destroy', id: project.id
      should redirect_to(root_path)
    end

    it "delete the project" do
      project.expects(:destroy)
      delete 'destroy', id: project.id
    end

    it "set a flash message" do
      delete 'destroy', id: project.id
      should set_the_flash[:notice].to("Votre projet a été supprimé")
    end
  end
end

I do this for:

Isolate my tests: I don't want to retest the model and I don't want to hit the database. It's faster.
Test all actions that should be do by the controller

And I have these questions:

Should I test the assignments or is to long for nothing?
How should I to stub my models? Is it better to place all stubs in a global before(:each) like this?

...
before(:each) do
  sign_in user
  Project.stubs(:find).returns(project)
  Project.stubs(:new).returns(project)
  project.stubs(:destroy)
end
...

It's DRYer and cleaner but less efficient.
Should I really isolate my tests? I tested it without stubs and I lose 10 seconds. What do you think about that?

I found this answer helpful; I also like using the shoulda matchers to clean things up.

Yes, I think you should test your assignments. If you use the shoulda matchers, this becomes much less painful, for example:

describe "POST 'update'" do

  context "when invalid" do
    before(:each) do
      project.stubs(:update_attributes).returns(false)
      post 'update', id: project.id
    end

    it { should render_template('edit') }
    it { should assign_to(:project).with(project) }
  end
end

I usually stub where it's relevant rather than at a global level, but I don't have a good argument as to why besides that it's easier for me to understand when I'm reading it.

It's fine to test in isolation as long as you're testing your full stack somewhere like with cucumber or integration tests.


Bash script, "flash cards" for Spanish vocabulary I use this bash script to review over vocabulary with my dental Spanish class. The script works great for me. However, I am wondering what other ways the same thing could be done. How could it be more efficient? Perhaps the script could me made more readable? Maybe other areas could be made more proper or elegant? If I have done something well, then that feedback too is appreciated. I am not particularly interested in adding in checks/tests of the formatting of my vocabulary sheet or checking to make sure the file exists. The comments have my English translations of the Spanish in the script.

This script reads each line of the vocabulary file from the top to the bottom and tests whether the user knows how to say the equivalent in Spanish. It is formatted like this, English: SpanishOpt1, SpanishOpt2, SpanishOpt3. Any of the options between commas could count as correct.

This is a sample of what a vocab file looks like:

floor of the mouth: piso de la boca
cheek: mejilla, cachete
throat: garganta
change in color: cambio en color, cambio de color
taste buds: bulbos gustativos
growth: crecimiento
bruise: moretón, cardenal, magulladura, moradura, contusión

Here is the script:

#!/bin/bash

read -r -p "1 - evaluación oral
2 - radiografía
Elige uno: " opt
# Choose one.

clear

if ! [[ "$opt" =~ ^[1-2]$ ]]; then
    # That is not an option. Choose 1 or 2.
    echo "Eso no es una opción. Elige 1 o 2."
    exit 1
fi

fname=("" "evaluación-oral-vocabulario.txt" "radiografía-vocabulario.txt")
file="/home/brock/2-Work/Dental/${fname[$opt]}"
vocab_items=$(wc -l < "$file")
line_num=1
num_right=0

until (("$line_num" > "$vocab_items")); do
    vocab_item=$(head -n"$line_num" "$file" | tail -n1)
    english=$(echo "$vocab_item" | cut -d: -f1)
    spanish=$(echo "$vocab_item" | cut -d: -f2 | sed 's/ //')
    plural_options=$(echo "$spanish" | grep ,)
    plural_vocab_item=$(echo "$spanish" | cut -d, -f1 | wc -w)
    if [ -n "$plural_options" ] || (("$plural_vocab_item" > "1")); then
        # The "s" is to make the subject below "Las palabras" plural if needed. The "n" is for the plural verb conjugation.
        s="s"; n="n"
    else
        s=""; n=""
    fi
    # How do you say ... in Spanish?
    read -r -p "¿Cómo se dice \"$english\" en español? " r
    answer_check=$(echo "_${spanish}_" | sed 's/, /_/g' | grep "_${r}_")
    # The "y" means "and" which is added before the last item in a list with no serial comma.
    spanish_feedback=$(echo "$spanish" | sed 's/\(.*\),/\1 y/')
    if [ -n "$answer_check" ]; then
        # You are correct. The word(s) ... mean ...
        echo -e "\tTienes razón. La$s palabra$s \"$spanish_feedback\" quiere$n decir \"$english\".\n"
        ((num_right++))
    else
        # You are not correct. The word(s) ... mean ...
        echo -e "\tNo tienes razón. La$s palabra$s \"$spanish_feedback\" quiere$n decir \"$english\".\n"
        pair_wrong[$line_num]="$vocab_item\n"
    fi
    ((line_num++))
done

# You know ... out of ... terms overall.
echo "Tú sabes $num_right de $vocab_items términos en total."
if [ -z "${pair_wrong[*]}" ]; then
    # Incredible! One hundred percent!
    echo "¡Increíble! ¡Cien por ciento!"
else
    # You have to work more on:
    echo -e "Tienes que trabajar más en:\n ${pair_wrong[*]}"
fi

I haven't used or reviewed bash script for ever, so I'm just going to give a few comments:

Your script reads nicely, and even though I don't know spanish, there are some comments explaining the vital parts.
I think the script could benefit from a little more spacing, so that the various sections stood out a little more. The top half is better than the bottom half.
The variable names are good and helpful
However there are two code smells that I see:

Each time you set vocab_item you're re-reading the file. So at end of script you've read the file \$N\$ times, where \$N\$ is the number of lines
Each time you set english and spanish to trigger a new call to cut.

Reading the main file once

After a little searching I found this thread on SO:Looping through the content of a file in bash. The accepted answers uses the following construct:

while read p; do
  echo $p
done <peptides.txt

This would allow for reading the file just once, but you'll need to change a little of your logic related to counting the correct number of answers.

Splitting a line into an array

Another search, and another SO thread: Split string into an array in bash. This one to avoid the calls to cut.

IFS=', ' read -r -a parts <<< "$vocab_item"
english="${parts[0]}"
spanish="${parts[1]}"

Hope this helps to some extent.


Reading WLAN packets The script reads WLAN packets, looks for probe requests and then extracts SSID, MAC address and then stores them in a list if the MAC address/SSID combo is unique.

if p.haslayer(Dot11ProbeReq):
    if pc == 0:
      print O+'[+]'+W+' Got first packet! Many more to go...\n'
        if p.haslayer(Dot11Elt):
          pc +=1
          vali = 0
            if p.ID == 0:
              ssid = p.info
                if ssid != "":
                  for (i,j) in obs:
                    if (i,j) != (p.addr2,ssid):
                        vali += 1
                    else:
                        break
                    if vali == len(obs):
                        obs.append((p.addr2,ssid))
                        checkmac(p.addr2)
                        checknet(ssid)
                        count +=1
                        get_oui(p.addr2)
                        print str(count)+'>',p.addr2+' ('+G+macf+W+') <--Probing--> '+O+ssid+W
                        wr_log(p.addr2,ssid,macf)
                    else:
                            pass

What modifications can I make to make the code faster, specially the for block and the validation process? Any other optimizations or practices that I should be following?

This script is reliant on the scapy library. Here p is a packet that scapy intercepts. It could be any type of packet - management, data or control etc. and can be of any subtype - deauthentication, probe response, authentication, probe request etc. The function that I have mentioned above reads each packet p and processes it the way I desire. The code begins with intercepting any packet p from the air and checking if the packet is a probe request. A probe request is emitted by all wifi enabled devices when they are looking to connect to a wifi network if they can't find one or even if they are connected, then to find a backup network in case the currently connected network fails. These probe requests are automatic and the users don't get to choose anything about them. The devices will emit these requests broadcasting a list of known wireless networks to check if there is any of them available nearby. After the packet is confirmed to be a probe request, the packet counter (pc) checks if it is the first packet, if it is then it displays a message confirming that we are able to receive requests and the network is up. Then the function checks whether the packet has a Dot11Elt layer, which is the information element that contains all the info about the device and previously connected wireless networks. If it does have an information layer then the value in the first information layer, which happens to be the ssid of a previously connected network, is stored in a variable called ssid. the variable p.addr2 is the MAC address of the device that the packet came from. Each MAC/SSID pair found is stored in an orderly fashion inside the list obs. Each packet intercepted, as many can be duplicates, are checked against the ones already stored in obs and if unique against each pair already stored, then the value of the vali (validation counter) is incremented. Needless to say, if the value of vali is the same as the length of obs then the packet is unique and did not match any other (MAC/SSID) pair stored in obs and is hence, printed. Before printing, the unique (MAC/SSID) pair is appended to the list obs and the functions checkmac() and checknet() check whether the MAC address or the SSID is unique, if they are then a counter is incremented and that helps us to identify unique devices and unique networks after the script stops running. The function get_oui() gets the manufacturer name though the use of netaddr library for each and every MAC address we find and wr_log() writes the MAC address, SSID name and manufacturer info in a log file.

Your variable names are quite hard to make sense of. I had to read your explanatory text to figure out what vali was supposed to do; I still have no idea what obs means. Judging by the line print O+'[+]'+W+' Got first packet! Many more to go...\n', it also looks like you might have a variable called "O" (capital letter oh), which most style guides advise against because it's hard to distinguish it from 0 (the number zero). I would use longer, more descriptive names. Write out validation or valid_pairs instead of using vali. Write packet and packet_counter instead of p and pc. I could have written my first sentence as "ur vrbl nms r q hd 2 mk sns of", and you might have been able to understand it, but ovssly ts mch mr dfclt 2 ndrstnd if i wrt lk ths. Apply the same principle to your code.

Spaces between your string concatenation operators also make code easier to read: print O + '[+]' + W + ' Got first packet! Many more to go...\n' is easier to read than the version with everything all scrunched up. In this case, though, I would prefer format strings to using concatenation: print "{} [+] {} Got first packet!".format(O, W). (Or the percent-sign version, if you're stuck on a version of Python before 2.6.)

Your code is rather deeply nested, which makes it harder to read and understand overall. The Zen of Python says flat is better than nested, and while I don't always agree with the Zen, in this case I think it's right. To relieve this problem, I would make some functions. Pick out some nested blocks and drop them into their own functions, passing in whatever arguments they need, so your code is flatter and easier to read and understand. At the very least, your entire block under if p.haslayer(Dot11Elt) could be its own function.

There are improvements both big and small that could be made to your for loop. It looks like you could do the same thing you currently do in a lot less code, like this:

if (p.addr2, ssid) not in obs:
    obs.append((p.addr2,ssid))
    checkmac(p.addr2)
    checknet(ssid)
    count +=1
    get_oui(p.addr2)
    print str(count)+'>',p.addr2+' ('+G+macf+W+') <--Probing--> '+O+ssid+W
    wr_log(p.addr2,ssid,macf)

This does a linear search of obs for the tuple (p.addr2, ssid); it checks every element in obs to see if it's equal to (p.addr2, ssid) as your current code does.

But there are faster ways to do this. I would probably rethink the way obs is stored. Right now, you're storing the valid pairs of MAC addresses and SSIDs as a list, so in order to find duplicates, you have to do a list search. You could speed this up by sorting the list and using a binary search, but you can speed it up even more by bringing a set into the equation.

It wasn't clear to me from your explanation whether you need obs to be in order or not. If you don't need it in order, you can just switch from a list to a set:

if (p.addr2, ssid) not in obs_set:
    obs_set.add((p.addr2, ssid))
    # other stuff

If you do need it in order, you can still use a set to speed this particular operation up, but unfortunately, there's no ordered set in Python. The Stack Overflow question Does Python have an ordered set? gives several ways you can mimic one. Another option: you could just keep your current list, but also add everything to a set. Maybe even wrap it in a class so you don't have to remember to do both. It's sort of hacky, and it takes more memory, but it's simple, quick to implement, avoids adding more dependencies, and is much easier than implementing your own red-black tree. (I consider an OrderedDict with all the values as True to be kind of hacky and a waste of memory too.)


Cleaning up conditional statements regarding new values Please suggest ways of cleaning up this code.

Hashtable newValues = e.Values;

string NewPosition = null;
string NewFirst = null;
string NewLast = null;
string NewEmail = null;
int NewAppt = 0;

if (newValues["Position"] == null)
{
    NewPosition = "";
}
else
{
    NewPosition = newValues["Position"].ToString();
}

if (newValues["First Name"] == null)
{
    NewFirst = "";
}
else
{
    NewFirst = newValues["First Name"].ToString();
}

if (newValues["Last Name"] == null)
{
    NewLast = "";
}
else
{
    NewLast = newValues["Last Name"].ToString();
}

if (newValues["Email"] == null)
{
    NewEmail = "";
}
else
{
    NewEmail = newValues["Email"].ToString();
}

if (newValues["Appts"] == null)
{
    NewAppt = 0;
}
else
{
    NewAppt = 1;
}

You can use the null-coalescing operator ?? like this:

NewPosition = (newValues["Position"] ?? "").ToString();

The core of this approach is this expression:

newValues["Position"] ?? ""

The ?? operator evaluates the left expression first (i.e. newValues["Position"]), and uses it as the result if it's not null. If the first expression is null, the second expression is evaluated, and its result is returned as the overall result of the expression.

For the last one, use ?:

NewAppt = (newValues["Appts"] != null) ? 1 : 0;


Rails authentication checks I have this authenticate! method where I am trying to retrieve their account based on the subdomain, then find the user. if user is found & their password is matched, we return success, else fail in all other cases. here is the code:

def authenticate!
    account = Account.find_by(subdomain: subdomain)
    if account
      u = account.users.find_by email: params["user"]["email"]
      if u.nil?
        fail!
      else
        u.authenticate(params["user"]["password"]) ? success!(u) : fail!
      end
    else
      fail!
    end
end

Now, I could simply this further by moving subdomain into a helper method,

def subdomain
  ActionDispatch::Http::URL.extract_subdomains(request.host, 1)
end

def authenticate!
      account = Account.find_by(subdomain: subdomain)
      if account
        u = account.users.find_by email: params["user"]["email"]
        if u.nil?
          fail!
        else
          u.authenticate(params["user"]["password"]) ? success!(u) : fail!
        end
      else
        fail!
      end
end

But how to I simply these nested conditionals?

Another view on @tokland solution:

def authenticate!
  return fail! unless account = Account.find_by(subdomain: subdomain)
  return fail! unless user = account.users.find_by(email: params["user"]["email"])
  return fail! unless user.authenticate(params["user"]["password"])
  success! user
end

This intention is to be as close to "original task explained in common English" as possible. And return keyword allows me to do that.


Inline conversion/cast from Expression> to LambdaExpression I have the following code (I need the LambdaExpression as not all Func<>'s are T, int):

var orderDelegates = new Dictionary<string, LambdaExpression>();

Expression<Func<Image, int>> id = i => i.Id;
orderDelegates.Add(ContentItem.ORDER_BY_ID, id);

Is it possible to make this shorter?

Best something like:

var orderDelegates = new Dictionary<string, LambdaExpression>()
{
    { ContentItem.ORDER_BY_ID, ????? },
    ...
};

I already tried the following, which did not work:

orderDelegates.Add(ContentItem.ORDER_BY_ID, (Expression<Func<Image, int>>)i => i.id);
//or
orderDelegates.Add(ContentItem.ORDER_BY_ID, i => i.id as Expression<Func<Image, int>>);

I "improved" it a little to the following already, but "full inlining" would be the desired result:

public static Dictionary<string, LambdaExpression> GetOrderDelegates<T>() where T : ContentItem
{
    Expression<Func<T, int>> id = i => i.Id;
    Expression<Func<T, bool>> enabled = i => i.IsEnabled;
    Expression<Func<T, IComparable>> author = i => i.Creator.UserName;
    //...

    return new Dictionary<string, LambdaExpression>()
        {
            { ContentItem.ORDER_BY_ID, id },
            { ContentItem.ORDER_BY_ENABLED, enabled },
            { ContentItem.ORDER_BY_AUTHOR, author },
            //...
        };
}

You can do this by creating a custom class that supports collection initializers. Its Add() method would be generic, so that it fits any allowed expression:

class ExpressionDictionary<T> : IEnumerable<KeyValuePair<string, LambdaExpression>>
{
    private readonly Dictionary<string, LambdaExpression> m_dictionary
        = new Dictionary<string, LambdaExpression>();

    public void Add<TResult>(string key, Expression<Func<T, TResult>> expression)
    {
        m_dictionary.Add(key, expression);
    }

    public IEnumerator<KeyValuePair<string, LambdaExpression>> GetEnumerator()
    {
        return m_dictionary.GetEnumerator();
    }

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

Usage:

new ExpressionDictionary<T>
{
    { ContentItem.ORDER_BY_ID, i => i.Id },
    { ContentItem.ORDER_BY_ENABLED, i => i.IsEnabled },
    //the cast is necessary if the result has to be Func<T, IComparable>
    // and not Func<T, string>
    { ContentItem.ORDER_BY_AUTHOR, i => (IComparable)i.Creator.UserName },
    //...
};

You will need to add other methods to ExpressionDictionary. Either implement the whole IDictionary<TKey, TValue> interface and make GetOrderDelegates() return that. Or just add a getter for the private Dictionary and return that.

But even better option might be not to use Dictionary at all. Instead create OrderByDeletegates class (though that's probably not the best name, since it would contain expressions, not delegates) that would have properties for all expressions:

class OrderByDeletegates<T> where T : ContentItem
{
    public Expression<Func<T, int>> OrderById { get; set; }
    public Expression<Func<T, bool>> OrderByEnabled { get; set; }
    public Expression<Func<T, IComparable>> OrderByAuthor { get; set; }
    // …
}

Usage:

new OrderByDeletegates<T>
{
    OrderById = i => i.Id,
    OrderByEnabled = i => i.IsEnabled,
    OrderByAuthor = i => i.Creator.UserName,
    // …
};

The advantage of this is that it's type-safe. You can't for example accidentally do something like OrderByEnabled = i => i.Id.


Using dependency injection for Node payment system I would like some feedback on my code. I am currently building a payments solution in a serverless lambda project. This what I came up with (very basic code at the moment):

// "interface implementation"
export default class Stripe {
  constructor() {
    this.stripe = new Stripe()
  }

  async charge(data) {
    try {
      const result = await this.stripe.chargeCustomerByToken(data);
      return result;
    } catch (error) {
      throw error;
    }
  }
}

export default class PaymentRepository {
  constructor(PaymentInterface) {
    this.ipayment = PaymentInterface;
  }

  async charge(data) {
    try {
      const result = await this.ipayment.charge(data);
      return result;
    } catch (error) {
      throw error;
    }
  }
}

export const charge = async (repository, data) => {
  if (!repository || !data) {
    throw new InvalidParameterError('Missing dependencies');
  }

  try {
    const result = await repository.charge(data);
    return result;
  } catch (error) {
    throw error;
  }
};

I would then use it like this in my main handler: charge(new PaymentRepository(new Stripe())).

This is my current structure. It feels super repetitive but still flexible and is easy to test, and also easy to change payment API (maybe to PayPal or something). But I'm not really sure if I'm just over-complexing the whole project, or maybe this isn't even the Node/JS way to code.

It looks good to me, very SOLID.

Here are a couple of suggestions:

I'm not really seeing the value in the PaymentRepository layer. If the Stripe wrapper is following a strict interface then just import it directly as your payment gateway i.e.

import PaymentGateway from './stripe';
...
charge(new PaymentGateway());

This still gives you the flexibility & testability but with one less layer. If you introduced a new payment gateway all that changes is the import e.g.

import PaymentGateway from './worldpay.js'

Also, you may have gone a bit overboard with your use of async / await. This code:

async x() {
  try {
    const result = await promise();
    return result;
  } catch (e) {
    throw e;
  }
}

Is effectively the same as:

x() {
  return promise();
}

Because you don't actually use the result (or error) until further up the callstack. Although having said that, you might want to consider deserializing your Stripe API responses to avoid leaking internal data through the layers.


Generating a game board with squares in a rectangle I am trying to generate a game board (similar to a Monopoly board). I am new to Scala and am thinking that there may be a better way to do this. But improvements to the code are also welcome.

class BoardGenerator(squaresPerPlayer: Int, numberOfPlayers: Int) {
  val directionTuples = Array((1, 0), (0, 1), (-1, 0), (0, -1))

  def generate: List[Square] = {
    val squaresPerSide = squaresPerPlayer * numberOfPlayers / 4

    def generateBoard(squaresLeft: Int, direction: Int, x: Int, y: Int): List[Square] = {
      if (direction == 4)
        Nil
      else if (squaresLeft > 1)
        Square(x, y) :: generateBoard(squaresLeft - 1, direction,
          x + directionTuples(direction)._1, y + directionTuples(direction)._2)
      else
        Square(x, y) :: generateBoard(squaresPerSide, direction + 1,
          x + directionTuples(direction)._1, y + directionTuples(direction)._2)
    }
    generateBoard(squaresPerSide, 0, 0, 0)
  }
}

directions as a variable name instead of directionTuples.
Instead of having a bunch of ifs in generateBoard, use pattern matching.
Instead of passing x: Int, y: Int in generateBoard, just pass a tuple which you could call currentPos.  You could define a function to add the direction tuple with the currentPos tuple instead of typing x + directionTuples(direction)._1, y + directionTuples(direction)._2 twice.  (There might actually be a very easy way to add (Int, Int) + (Int, Int) in Scala, but I can't think of it right now.)


Better Fibonacci sequence calculation I attempted to make better Fibonacci sequence calculation algorithm in C++. This was the best I could:

constexpr unsigned fibonacci(unsigned n) {
    unsigned result[2] = {1, 0};
    for (unsigned i = 1; i < n; ++i)
        result[i%2] = result[0] + result[1];
    return result[1 - n%2];
}

...which runs at O(n) time complexity and O(1) space complexity.
Is there any better?

(Uh-oh: better - better define a quality measure!)

Your code doesn't tell what it's about.
constexpr looks good - "my" environment complains with C++11.
"The array&index manipulation" where "simultaneous assignment" is wanted is hard to read (could be worse: the elements of result[] could be non-interchangeable).
Alas, what I found for "modern C++" is ghastly compared to python's a, b = b, a + b. I appreciate the attempt to avoid avoidable assignments; I'm mildly curious if it makes any difference in the code generated by an optimising compiler.
Is there any better? Well, with output size limited by a constant, there's a tighter limit:
the runtime of your code is in O(1), just as any other.
In a comment, you express concern about the complexity of multiplication. If you accept ("bit-wise") "shift" as a (very) cheap operation, you can take three steps in the Fibonacci sequence at once without an increase in "logic gate complexity":

#include <cstdlib>
#include <tuple>
#include <iostream>

/// Iterates an a,b = b,a+b sequence in steps of three.
//constexpr
static unsigned long tri(int previous, int current, const unsigned int n) {
    if (n < 2)
        return n ? current : previous;
    std::div_t split = std::div(n-2, 3);
    while (0 <= --split.rem)
        std::tie(previous, current)
            = std::make_tuple(current, current+previous);
    unsigned long
        a = current - previous,
        b = current + previous;
    while (0 <= --split.quot)
        std::tie(a, b) = std::make_tuple(b, (b<<2)+a);

    return b;
}
/// Iterates the Fibonacci sequence in steps of three.
unsigned long fibonacci(const unsigned int n) {
    return tri(0, 1, n);
}
/// Iterates the Lucas numbers in steps of three.
unsigned long lucas(const unsigned int n) {
    return tri(2, 1, n);
}

(For variants using arrays of precomputed elements in stead of "the setup-loop" (and a main()), consult the edit history.)
(b*Phi³ coincidentally can be computed with just two summands (and no other power up to 2³² can).)


Radio group labelling and semantics I can't seem to get fieldset's to react properly when using display:table and display:table-cell even if I reset the default values, it just reacts oddly in a variety of ways in different browsers/devices, so as a result I have actually removed all fieldset's and replaced them with div's.

Is this a semantically accurate method to label a radio group? or is there a better more semantic method aside from using fieldset/legend?

<form method="get" novalidate>

      Pet Type

          <input id="r3" name="same2" type="radio" />
          Cat

          <input id="r4" name="same2" type="radio" />
          Dog

</form>

I've always approached it in this manner, but to avoid using flexbox and floats I used the above instead.

<form method="get" novalidate>
<fieldset class="table">

      <legend for="r1-group">Pet Type</legend>

          <input id="r3" name="same2" type="radio" />
          Cat

          <input id="r4" name="same2" type="radio" />
          Dog

</fieldset>
</form>

Should I be bolstering this with Schema's microdata and aria to improve semantics instead?

You can’t use the label element like that. Its for attribute may only refer to "labelable elements" (and div isn’t labelable).

Semantic markup

(Assuming that you can’t use fieldset.)

You can ignore all the div elements (except their "meaningful" attributes, if any), because they are meaningless. So your markup is essentially this:

<form method="get" novalidate>

  Pet Type

  <input id="r3" name="same2" type="radio" />
  Cat

  <input id="r4" name="same2" type="radio" />
  Dog

</form>

Having a label without associated form control is not wrong, but maybe uncommon. It "represents a caption in a user interface", so I guess you could keep it like that.

If you want more "structure", you could use the dl element. However, I think it might be overkill for such a simple case.

Accessibility

WAI-ARIA recommends "that elements with role radio are explicitly grouped in order to indicate which ones affect the same value".

(In HTML5, input elements in the radio button state have the radio role by default.)

How to group them explicitly?

  This is achieved by enclosing the radio elements in an element with role radiogroup.

So you should use the radiogroup role, e.g.:

<form method="get" novalidate>

    Pet Type

    <input id="r3" name="same2" type="radio" />
    Cat

    <input id="r4" name="same2" type="radio" />
    Dog

</form>

(HTML5 recommends not to set role="radio" for the radio buttons explicitly, but you may do so if you want to.)


Replace match inside tags The problem I want to solve is to replace a given string inside tags.

For example, if I'm given:

  Some text abc [tag]some text abc, more text abc[/tag] still some more text

I want to replace abc for def but only inside the tags, so the output would be:

  Some text abc [tag]some text def, more text def[/tag] still some more text

We may assume that the tag nesting is well formed. My solution is the following:

def replace_inside(text):
    i = text.find('[tag]')
    while i >= 0:
        j = text.find('[/tag]', i+1)
        snippet = text[i:j].replace('abc', 'def')
        text = text[:i] + snippet + text[j:]
        i = text.find('[tag]', j)
    return text

I was wondering if it can be solved in a more elegant way, for example, using regex.

Your code is incorrect. See the following case:

print replace_inside("[tag]abc[/tag]abc[tag]abc[/tag]")

You can indeed use regular expressions

pattern = re.compile(r"\[tag\].*?\[/tag\]")
return pattern.sub(lambda match: match.group(0).replace('abc','def') ,text)


Handling of open database connection and calling shared function from WebAPI I'm working on an ASP.NET project using VB.NET that uses Dapper and the code as implemented so far runs fine with just me testing. In the example below, Dapper calls a stored proc.

But I am wondering primarily right now if the method of generating an open connection (see dbConnFactory below) implemented in the Domain project is not good practice.

Also, can there be pitfalls to calling a Shared function from .NET's WebAPI, whether it the function is for GET, PUT, POST, or Delete?

AutomoblieDomain project

Imports System.Data.Common
Imports Dapper
Namespace DAL

    Public Class DomainSettings
         Public Shared Property CustomConnectionString As String
    End Class

    Public Class dbConnFactory
        Public Shared Function GetOpenConnection() As DbConnection
            Dim connection = New SqlClient.SqlConnection(CustomConnectionString)
            connection.Open()
            Return connection
        End Function
    End Class

    Public Class CarTypes
        Public Property CarTypeID As Integer
        Public Property CarTypeText As String

        Public Shared Function GetList() As IEnumerable(Of CarTypes)
            Using conn = dbConnFactory.GetOpenConnection()
                Dim _list = conn.Query(Of CarTypes)("dbo.CarTypes_GetList", CommandType.StoredProcedure).ToList()
                Return _list
            End Using
        End Function
    End Class

End Namespace

Web UI Project

Imports System.Net
Imports System.Web.Http
Imports AutomobileDomain

Public Class CarTypesController
    Inherits ApiController

    <HttpGet()>
    Public Function GetList() As IEnumerable(Of DAL.CarTypes)
        Return DAL.CarTypes.GetList()
    End Function

End Class

According to MSDN:

  "We recommend that you always close the connection when you are finished using it in order for the connection to be returned to the pool. You can do this using either the Close or Dispose methods of the Connection object, or by opening all connections inside of a using statement in C#, or a Using statement in Visual Basic."
   http://msdn.microsoft.com/en-us/library/8xx3tyca(v=VS.80).aspx - Section: "Adding Connections"

The way your code is written, (connection being delivered via a factory method) does look a little risky. However, the way MSDN describes it, at the end of the Using block, the connection object will be automatically closed and disposed.


Printing a huge document I have a web page containing n pages of paged data, showing 20 records at a time. The front end has a link to another page that opens the same data, but without the paging and calls window.print on document.ready.

On my paged content view I have a link:

Print all items

In printallthisstuff.php I have:

$(document).ready(function(){
    window.print();
});

printaallthisstuff.php can be a very large document, taking several seconds to load. What would be a better way of doing this?

I'd suggest that you do:

Print all items

where print_frame.html is a simple page that displays an animated loading gif, and contains js that makes an ajax call to printallthisstuff.php.  The success callback for that ajax call will then replace the body content (which currently displays loading.gif) with the html returned from printallthisstuff.php, and call print, as you are doing now.


Configurable synchronization approach in Java I am interested in the community opinion about the following approach for the synchronization.
Generally it is based on the idea to have a configurable way to apply locking on some logic parts.

Any comments and review is appreciated :) .

lock.properties keeps configuration:

foo=true
bar=false

SynFactory.java produces locks:

import java.io.IOException;
import java.util.HashMap;
import java.util.Map;
import java.util.Properties;

public class SyncFactory {
  private static HashMap<String, Object> locks = null;

  public static synchronized Object getLock(String key) {
    init();
    return locks.containsKey(key) ? locks.get(key) : new Object();
  }

  private static void init() {
    if (locks == null) {
      try {
        locks = new HashMap<String, Object>();

        Properties properties = new Properties();
        properties.load(SyncFactory.class.getClassLoader().getResourceAsStream("lock.properties"));

        for (Map.Entry<Object, Object> entry : properties.entrySet()) {
          // key=true  => locking with ine object
          // key=false => lock object should always be new
          if (Boolean.parseBoolean(String.valueOf(entry.getValue()))) {
            locks.put((String) entry.getKey(), new Object());
          }
        }

      } catch (IOException e) {
        throw new RuntimeException(e);
      }
    }
  }

}

Service.java actual usage:

public class Service {

  public void foo() {
    synchronized (SyncFactory.getLock("foo")) {
      // do some logic
    }
  }

  public void bar() {
    synchronized (SyncFactory.getLock("bar")) {
      // do some logic
    }
  }
}

I just don't see a point in this factory. It has nothing in common with locking except class name, getLock method and "lock.properties" string inside. It's just a global hash map.
You always can (should) use synchronized with this or even better with private ivar. Any need too use your factory makes me think there is a bad design.


REST API wrapper testing I'm writing wrapper for REST API of a service. In my wrapper I have quite simple method:

public List<string> GetAvailableSources()
{
    var request = new RestRequest($"session/{Id}");
    var sessionProto = _client.Execute<SessionPrototype>(request);
    return sessionProto.Data.InstalledSources;
}

It's really simple and just sends request to service and returns result. However, unit test for that method is huge:

[Test]
public void GetAvailableSources_MustReturnItemsFromResponse()
{
    const long sessionId = 123;

    var clientMock = new Mock<IRestClient>();

    clientMock.Setup(x => x.Execute<SessionPrototype>(It.IsAny<IRestRequest>()))
        .Returns(new RestResponse<SessionPrototype>()
        {
            Data = new SessionPrototype
            {
                Id = sessionId,
                InstalledSources = new List<string>
                {
                    "Scanner1",
                    "Scanner2"
                }
            },
            StatusCode = HttpStatusCode.OK
        }).Verifiable();

    var session = new Session(sessionId, clientMock.Object);
    var sources = session.GetAvailableSources();
    Assert.That(sources, Has.Count.EqualTo(2));
    Assert.That(sources[0], Is.EqualTo("Scanner1"));
    Assert.That(sources[1], Is.EqualTo("Scanner2"));
    clientMock.Verify(
        m => m.Execute<SessionPrototype>(
            It.Is<IRestRequest>(
                r => r.Resource == $"session/{sessionId}" && r.Method == Method.GET)));
}

Is there a way for simplifying tests for REST API?

You could simplify entire test class by extracting part responsible for general configuration. Then you can easili split your test into two, first verifying that your IRestClient is properly called during the process and second that assures tested method returns result provided by this client. For tests that require specific configuration, like throwing exceptions and so on, I found it a nice way to extract this configuration to separate, private methods and calling them when necessary instead of configuring all the scenarios in the Setup method.

[TestFixture]
public class ClientTest
{
    private const long SessionId = 123;
    private Mock<IRestClient> _clientMock;
    private List<string> _expectedSources;
    private Session _session;

    [SetUp]
    public void Setup()
    {
        _expectedSources = new List<string>
                {
                    "Scanner1",
                    "Scanner2"
                };
        _clientMock = new Mock<IRestClient>();
        _clientMock.Setup(x => x.Execute<SessionPrototype>(It.IsAny<IRestRequest>()))
                   .Returns(new RestResponse<SessionPrototype>()
        {
            Data = new SessionPrototype
            {
                Id = SessionId,
                InstalledSources = _expectedSources
            },
            StatusCode = HttpStatusCode.OK
        }).Verifiable();

        _session = new Session(SessionId, _clientMock.Object);
    }

    [Test]
    public void GetAvailableSources_MustCallTheClientUsingProperRequest()
    {
        _clientMock.Verify(
            m => m.Execute<SessionPrototype>(
                It.Is<IRestRequest>(
                r => r.Resource == $"session/{SessionId}" &&
                     r.Method == Method.GET)));
    }

    [Test]
    public void GetAvailableSources_ReturnsSourcesProvidedByClient()
    {
        var sources = _session.GetAvailableSources();

        CollectionAssert.AreEquivalent(_expectedSources, sources);
    }
}


Finding the first non-repeated character in a string Given a string, find the first non-repeated character in it.

E.g. "yellow" should return "y"

There are several solutions for this in other languages but I haven't seen one written in Perl.

My main question is: can this be done in a more Perl-ish way or generally in an even shorter way?

use strict;
use warnings "all";

while (<DATA>)
{
    while (m,(.),g)
    {
        my $c = $1;
        if (s,$1,,g < 2)
        {
            print "$c\n";
            last;
        }
    }
}

__DATA__
yellow
tooth

Don't reinvent the wheel, there is a function (ie. singleton) in the package MoreUtils that does the job:

#!/usr/bin/perl
use Modern::Perl;
use List::MoreUtils qw(singleton);

while (<DATA>) {
    chomp;    # don't forget it, it removes the linebreak.
    # split explodes the string in character
    # singleton keeps characters that appear only once
    # ($first) contains the first character that appears only once.
    my ($first) = singleton split//, $_;
    say $first;
}

__DATA__
yellow
tooth

Output:

y
h