Get all monotonic sublists from a list So, i recently wrote code that can count the number of monotonic items (increasing, decreasing and constant). For an input such as x = [1,2,3,3,2,0] The provided example was:

1. Increasing: [1,2], [2,3], [1,2,3]
2. Constant: [3,3]
3. Decreasing: [3,2], [2,0], [3,2,0]

So, i broke the problem into two steps, firstly just getting all biggest monotonic sequences, and then finding all sub-lists within those sequences.
During the process it seemed to me that things started getting rather long and i was surprised by how "big" the whole thing seemed by the end of it. I was wondering if there are any tricks i missed or steps i could have done better. Also looking for tips on code readability as well. Code starts below:

x = [1,2,3,3,2,0]
prev = x[0]
curr = x[1] #keep track of two items together during iteration, previous and current
result = {"increasing": [],
          "equal": [],
          "decreasing": [],
          }

def two_item_relation(prev, curr): #compare two items in list, results in what is effectively a 3 way flag
    if prev < curr:
        return "increasing"
    elif prev == curr:
        return "equal"
    else:
        return "decreasing"

prev_state = two_item_relation(prev, curr) #keep track of previous state
result[prev_state].append([prev]) #handle first item of list

x_shifted = iter(x)
next(x_shifted) #x_shifted is now similar to x[1:]

for curr in x_shifted:
    curr_state = two_item_relation(prev, curr)
    if prev_state == curr_state: #compare if current and previous states were same.
        result[curr_state][-1].append(curr)
    else: #states were different. aka a change in trend
        result[curr_state].append([])
        result[curr_state][-1].extend([prev, curr])
    prev = curr
    prev_state = curr_state

def all_subcombinations(lst): #given a list, get all "sublists" using sliding windows
    if len(lst) < 3:
        return [lst]
    else:
        result = []
    for i in range(2, len(lst) + 1):
        for j in range(len(lst) - i + 1):
            result.extend([lst[j:j + i]])
    return result

print(" all Outputs ")
result_all_combinations = {}

for k, v in result.items():
    result_all_combinations[k] = []
    for item in v:
        result_all_combinations[k].extend(all_subcombinations(item))

print(result_all_combinations)
#Output:
{'increasing': [[1, 2], [2, 3], [1, 2, 3]],
 'equal': [[3, 3]],
 'decreasing': [[3, 2], [2, 0], [3, 2, 0]]}

A few changes:

Strings shouldn't be used here for keeping track of comparison results. Strings are prone to being typo'd, and may lead to unexpected results (like indexing result causing KeyErrors at runtime). I'd take a page from Java (and other languages) and use -1, 0 and 1 to indicate the results of a comparison. You can see it being used in an answer here. I'd make the following changes:

result = {1: [], # Increasing
          0: [], # Equal
          -1: [] # Decreasing
          }

def two_item_relation(prev, curr):
    if prev < curr:
        return 1
    elif prev == curr:
        return 0
    else:
        return -1

It's much harder to mistype -1 than it is, for example, "decreasing".

If you really wanted Strings for pretty printing purposes (like for your output at the bottom), you could maintain a dictionary mapping comparison numbers to strings:

pp_result = {1: "Increasing",
             0: "Equal",
             -1: "Decreasing"
            }

The point is that you shouldn't use easily mistyped things as keys unless necessary.

Strings also may be slower to compare, and may take more memory, but hash caching and String interning may negate those problems in some cases.

You could also write that function as something like:

def two_item_relation(prev, curr):
    return 1 if prev < curr else \
                0 if prev == curr else \
                    -1

But I'm probably going to get yelled at for even bringing that up. Conditional expressions/ternaries are nice in many cases when you want to conditionally return one or another thing, but they get a little murky as soon as you're using them to decide between three different things. It's especially bad here because this pretty much needs to be split over a few lines, which necessitates the use of line continuation characters, which are a little noisy.

I'm bringing it up in case you're unaware of conditional expressions, not because I'm necessarily suggesting their use here.

You could use enums as well:

from enum import Enum

class Compare_Result(Enum):
    INCREASING = 1
    EQUAL = 0
    DECREASING = -1

def two_item_relation(prev, curr):
    if prev < curr:
        return Compare_Result.INCREASING
    elif prev == curr:
        return Compare_Result.EQUAL
    else:
        return Compare_Result.DECREASING

This has the benefit that it makes it obvious what each result actually means. They also prints out semi-nicely, so the "pretty-printing map" may not be as necessary:

>>> str(Compare_Result.INCREASING)
'Compare_Result.INCREASING'

>>> repr(Compare_Result.INCREASING)
'<Compare_Result.INCREASING: 1>'

And, if you do typo a name (which is harder to do since IDEs can autocomplete Compare_Result.), it will fail outright with an error:

>>> Compare_Result.INCRESING

Traceback (most recent call last):
  File "<pyshell#5>", line 1, in <module>
    Compare_Result.INCRESING
  File "C:\Users\slomi\AppData\Local\Programs\Python\Python36-32\lib\enum.py", line 324, in __getattr__
    raise AttributeError(name) from None
AttributeError: INCRESING

Unfortunately though, this error does not happen immediately like it does in other languages. The faulty code needs to actually be interpreted before the error is caught. This seems to make enums less useful in Python than in languages like Java or C++, but it's still less error-prone than using Strings or Numbers.

Honestly, I'm too tired right now to comment on the algorithm, but hopefully this was helpful.


Adding a mix of strings and string arrays to an array Original question:

Function to split either a string array or string into a string array

I was asked why I didn't write multiple functions or something like that. The point of the function is to be a single one and to be able to receive an indefinite number of strings or string arrays and get the job done. Restrictions are not allowed.

The function I have written (creatively-named):

public string[] Add(string[] ogarr,object[] toadd)
{

    var results = new System.Collections.Generic.List<string>();
    results.AddRange(ogarr);

    for (int i = 0; i < toadd.Length; i++)
    {
        if (toadd[i].GetType() == typeof(string[]))
            results.AddRange((string[])toadd[i]);
        else
            results.Add((string)toadd[i]);
    }

        return results.ToArray();
}

I use this to test its functionality:

string[] temp = new string[]{"123","456","789"};

string[] thing1 = new string[]{"abc","def"};
string thing2 = "ghi";

object[] toadd = new object[] {thing1,thing2 };

temp = Add(temp,toadd);

temp is the original string array I want to add things to.
thing1 and thing2 are both things I want to add to that array, one after another, in the order of addition to the object[], obviously.

Disclaimer: I'm not trying to cut bytes; I want aesthetic code! This is my passion.

Naming Conventions

As the Naming Guidelines states:

  The goal of this chapter is to provide a consistent set of naming
  conventions that results in names that make immediate sense to
  developers.

In that sense, the creatively defined method name Add and the parameter names ogarr and toadd will not do.

The General Naming Conventions states:

  Avoid using identifiers that conflict with keywords of widely used
  programming languages.

  Do not use abbreviations or contractions as part of identifier names.

  Do favor readability over brevity.

Method name Add is a common method name used by collection types to add a new element to the collection. An Add method with source as the first parameter and the items to be added as the second parameter does not make sense.

ogarr and toadd are bad names for parameters and should be changed at least to sourceArray and itemsToAdd or similar.

And in Capitalization Conventions

  Do use camelCasing for parameter names.

I'm not suggesting to change ogarr to ogArr and toadd to toAdd of course. The parameters should have descriptive names in the first place. You can consider sourceArray and itemsToAdd or similar.

Possible Improvements

Being consistent with code formatting is important. Consider using curly brackets, always:

if (toadd[i].GetType() == typeof(string[]))
{
    results.AddRange((string[])toadd[i]);
}
else
{
    results.Add((string)toadd[i]);
}

You say

  restrictions are not allowed.

Then, why restricting the second method parameter to object[]? Let's get that as an IEnumerable. This way, your method can also operate on string[] or List<T> instances

public string[] Add(string[] sourceArray, IEnumerable itemsToAdd)

You can also change the method to be an extension method of string[]. This will clearly state the purpose of the method if declared with a good name:

public static string[] AddItemsOrRanges(this string[] sourceArray, IEnumerable itemsOrRangesToAdd)

You should always check if the parameters are passed in as null. Your code would fail with a null refererence exception at toadd.Length if toadd is passed as null.

if (toadd == null)
{
    // throw ArgumentException() ?
    return ogarr;
}

You should also check the items of the passed in collection for null references. If any of the items of the object[] toadd would be null, your code would fail with a null refererence exception at x.GetType()

if (toadd[i] == null)
{
    // throw ArgumentException() ?
    continue;
}

Finally, the implementation, with all the naming convention violations and functional confusions fixed, might look like the following:

public static string[] AddItemsOrRanges(this string[] sourceArray, IEnumerable itemsOrRangesToAdd)
{
    if (itemsOrRangesToAdd == null)
    {
        // throw ArgumentException() ?
        return sourceArray;
    }

    // It is possible with extension methods to have the self instance this as null
    // because extension methods are actually static methods and can be invoked
    // on a null reference (interesting?)
    // string[] myStringArray = null;
    // string[] finalStringArray = myStringArray.AddItemsOrRanges(new object[] { "Test" });
    // will work!
    if (sourceArray == null)
    {
        // throw ArgumentException() ?
        return sourceArray;
    }

    var results = new System.Collections.Generic.List<string>(sourceArray);

    foreach (var itemOrRangeToAdd in itemsOrRangesToAdd)
    {
        if (itemOrRangeToAdd != null)
        {
            if (itemOrRangeToAdd.GetType() == typeof(string[]))
            {
                results.AddRange(itemOrRangeToAdd as string[]);
            }
            else if (itemOrRangeToAdd.GetType() == typeof(string))
            {
                results.Add(itemOrRangeToAdd as string);
            }
            else
            {
                // What to do if the element is neither a string nor a string[]?
                // The (string) cast in the else block would fail in your source code
                // throw ArgumentException() ?
            }
        }
        else
        {
            // How would you determine if the element is to be added or not?
            // is it a null string instance, or is it a null string[] instance?
            // throw ArgumentException() ?
            continue;
        }
    }

    return results.ToArray();
}

And calling it would be like:

string[] temp = new string[] { "123", "456", "789" };

string[] thing1 = new string[] { "abc", "def" };
string thing2 = "ghi";

object[] toadd = new object[] { thing1, thing2 };

temp = temp.AddItemsOrRanges(toadd);


n-dimensional Euclidean space calculation templates I have been working with C++11 code that uses std::vector[] to store coordinates. Most often this code uses 2D or 3D but it occurred to me that it may be generally useful for n-dimensional Euclidean distance calculations.

Because these calculations are used within a simulation, they should be as fast as practical without sacrificing generality.

// euclid.h
#include <vector>
#include <algorithm>
#include <stdexcept>

template <typename T>
std::vector<T> operator+(const std::vector<T>& a, const std::vector<T>& b)
{
    if (a.size() != b.size())
        throw std::domain_error("vector addition must have equal length vectors");
    std::vector<T> result;
    result.reserve(a.size());
    std::transform(a.begin(), a.end(), b.begin(),
                   std::back_inserter(result), std::plus<T>());
    return result;
}

template <typename T>
std::vector<T> operator-(const std::vector<T>& a, const std::vector<T>& b)
{
    if (a.size() != b.size())
        throw std::domain_error("vector subtraction must have equal length vectors");
    std::vector<T> result;
    result.reserve(a.size());
    std::transform(a.begin(), a.end(), b.begin(),
                   std::back_inserter(result), std::minus<T>());
    return result;
}

template <typename T>
T squared_distance(const std::vector<T>& a, const std::vector<T>& b)
{
    if (a.size() != b.size())
        throw std::domain_error("squared_distance requires equal length vectors");
    return std::inner_product(a.begin(), a.end(), b.begin(), T(0),
        std::plus<T>(), [](T x,T y){return (y-x)*(y-x);});

}

Here is some driver code to demonstrate usage.

// points.cpp
#include <iostream>
#include <vector>
#include "euclid.h"

template <typename T>
std::ostream& operator<<(std::ostream& out, const std::vector<T> &v)
{
    out << "{ ";
    for (auto p : v)
        out << p << ' ';
    return out << "}";
}

#define say(x) std::cout << "" #x " = " << (x) << std::endl
int main()
{
    std::vector<double> origin{0, 0, 0}, a{3, 4, 5}, b{-1, -2, -3},g{0,0,0,0};
    say(origin);
    say(a);
    say(b);
    say(a+b);
    say(a-b);
    say(b-a);
    say(squared_distance(origin,a));
    say(squared_distance(origin,b));
}

The output from the program looks like this:

origin = { 0 0 0 }
a = { 3 4 5 }
b = { -1 -2 -3 }
a+b = { 2 2 2 }
a-b = { 4 6 8 }
b-a = { -4 -6 -8 }
squared_distance(origin,a) = 50
squared_distance(origin,b) = 14

I'm interested in general ideas for improvement or criticism of the existing code, but I also have some specific questions:

can the squared_distance calculation be made more efficient?
is there a nice way to reduce code duplication in the + and - operators?
I've tested with int, float, double and std::complex.  What else might be useful?
is there any point to accommodating unsigned types?
should I do anything about possible numeric overflow or underflow?
is there any point to having an #ifndef header guard in this file?

To reduce redundancy, I'd probably move the check for the inputs being the same size into a function template by itself, and just invoke that from the other three:

template <typename T>
void check_size(std::vector<T> const &a, std::vector<T> const &b) {
    if (a.size() != b.size())
        throw std::domain_error("vector addition must have equal length vectors");
}

I'd then at least test with passing one of the input parameters by value, using it as the destination for the calculation, and returning it:

template <typename T>
std::vector<T> operator+(std::vector<T> a, const std::vector<T>& b)
{
    check_size(a, b);
    std::transform(a.begin(), a.end(),
                   b.begin(),
                   a.begin(),
                   std::plus<T>());
    return a;
}

Although this doesn't always improve speed, it does often enough to be worth testing/profiling to see how well it works in this case.

Another possibility to consider would be using std::valarray instead. It's the forgotten step-child (so to speak) of the C++ standard, but it was designed specifically to support fast numeric calculations. It already defines equivalents of your + and - operators, as well as a sum and * operators, so your code comes out something like this:

std::valarray<double> origin{ 0, 0, 0 }, a{ 3, 4, 5 }, b{ -1, -2, -3 }, g{ 0, 0, 0, 0 };
say(origin);
say(a);
say(b);
say(a + b);
say(a - b);
say(b - a);
say(((a - origin) * (a - origin)).sum());
say(((b - origin) * (b - origin)).sum());

Since std::valarray defines all the operators we're using, the only other code we need is the say macro and the operator<< overload (minutely modified to take a std::valarray parameter instead of a std::vector).

Considering your more specific questions:

specific questions:

  can the squared_distance calculation be made more efficient?

std::valarray and/or std::array might help--but for a relatively small number of dimensions, I wouldn't expect to see much difference.

  is there a nice way to reduce code duplication in the + and - operators?

The code above attempts to answer this, to at least some degree.

  I've tested with int, float, double and std::complex. What else might be useful?
  is there any point to accommodating unsigned types?

I don't see a lot of point in unsigned types for this task. Yes, distances are always positive, but you can pretty easily end up with a negative number from an intermediate calculation, and you don't want that wrapping around to a large number as it would with an unsigned type.

  should I do anything about possible numeric overflow or underflow?

Harder to say. It basically depends on the use to which you're putting the code. If you need the distance, there are ways of computing it that avoid the larger magnitude of the squared distance (even as an intermediate), and are fairly fast to compute as well--but they're still slower than computing the squared distance, in which case the final result is also often the single largest value you deal with, so there's not a lot you can about overflow.

In floating point, subtraction is one of the prime culprits that can/will lead to precision loss. At least in theory, you might prefer to avoid it if possible, but for finding a squared distance I don't know of a lot of alternatives either.

  is there any point to having an #ifndef header guard in this file?

The compiler will (or should, anyway) give errors if you include it twice in the same translation unit, so it wouldn't hurt. Of course, you're unlikely to include it twice in the same translation unit directly, but its getting included via two other headers wouldn't be all that rare an occurrence.

I was going to mention std::array as well, but while I was writing this, I see @Morwenn has written one about that already.


How can I make this code (that is usually in a loop) faster and efficiently? When I have a place with limited space, where I entry a text that can be few letters or huge amount of text because it's dynamic, I'm using this code to make the text fit the specific space.

#region TextSizing
string textSizing = databaseTable.Description;
using (System.Drawing.Graphics g = System.Drawing.Graphics.FromHwnd(IntPtr.Zero))
{
    size = g.MeasureString(textSizing, new System.Drawing.Font("Arial", 14)).Width;
}

if (size < 1500)
    lblText.Text = textSizing;
else
    lblText.Text = textSizing.Length > 135 ? textSizing.Remove(135) + "..." : textSizing;
#endregion

A friend of mine suggested that I should use cache to store the Font and the Graphics, I didn't understand it very well, but keep thinking about it... How can I make this code (that is usually in a loop) faster and efficiently ?

Cache the instance of Graphics and Font objects so that you don't need to create them  each time you measure the length of string. I've created a small test to see which part takes most of the time:

private static void Main()
{
    Stopwatch stopwatch = Stopwatch.StartNew();

    stopwatch.Restart();
    for (int i = 0; i < 1000000; i++)
    {
        using (System.Drawing.Graphics g = System.Drawing.Graphics.FromHwnd(IntPtr.Zero))
        {
            var size = g.MeasureString("asdasdf", new System.Drawing.Font("Arial", 14)).Width;
        }
    }
    Console.WriteLine("Current solution - create a new Graphics per measure: {0} milliseconds", stopwatch.ElapsedMilliseconds);

    stopwatch.Restart();
    using (System.Drawing.Graphics g = System.Drawing.Graphics.FromHwnd(IntPtr.Zero))
    {
        for (int i = 0; i < 1000000; i++)
        {
            var size = g.MeasureString("asdasdf", new System.Drawing.Font("Arial", 14)).Width;
        }
    }
    Console.WriteLine("Measure the string with Graphics cached: {0} milliseconds", stopwatch.ElapsedMilliseconds);

    stopwatch.Restart();
    using (System.Drawing.Graphics g = System.Drawing.Graphics.FromHwnd(IntPtr.Zero))
    using (var font = new System.Drawing.Font("Arial", 14))
    {
        for (int i = 0; i < 1000000; i++)
        {
            var size = g.MeasureString("asdasdf", font).Width;
        }
    }
    Console.WriteLine("Measure the string with Graphics and font cached: {0} milliseconds", stopwatch.ElapsedMilliseconds);
}

Results on my computer are:

Current solution - create a new Graphics per measure: 26864 milliseconds
Measure the string with Graphics cached: 6588 milliseconds
Measure the string with Graphics and font cached: 803 milliseconds

So by caching Graphics you'll reduce the time consumption by 75%, and by caching both Font and Graphics you get a nice 33.5X performance boost :).


Library class Project I am creating a class project which involves two classes: Library and Book. I would like a general review on the code.

public class Book
{
    private String title;
    private String author;
    private String summary;

    public Book() {
        title = "";
        author = "";
        summary = "";
    }

    public Book(String title, String author, String summary) {
        this.title = title;
        this.author = author;
        this.summary = summary;
    }

    public String getTitle() {
        return title;
    }

    public String getAuthor() {
        return author;
    }

    public String getSummary() {
        return summary;
    }
}

import java.util.*;

Next is my Library class.

import java.util.Scanner;

public class Library
    {
        private String location;
        private String openingHours;
        private Book book;
        private Map<Book,Integer> bookList = new HashMap<Book,Integer>();

        public Library(String location, String openingHours) {
            this.location = location;
            this.openingHours = openingHours;
        }

        public void addBook(Book book) {
            //If the book doesn't exist add it: otherwise increase the quantity of the book
            if (bookList.containsKey(book))
                bookList.put(book, bookList.get(book) + 1);
            else
                bookList.put(book, 1);
        }

        public String printOpeningHours() {
            return openingHours;
        }

        public String printAddress() {
            return location;
        }

        public void borrowBook(Book book) {
            bookList.put(book, bookList.get(book) - 1);
        }

        public void returnBook() {
            bookList.put(book, bookList.get(book) + 1);
        }

        public String printAvailableBooks() {
            ArrayList <String> available = new ArrayList();

            for (Book books: bookList.keySet())
                available.add(books.toString());

            return available.toString();
        }

        public boolean isBorrowed() {
            int value = 0;

            for (Book bq: bookList.keySet()) {
                value = bookList.get(bq);
                if (value > 0)
                    return true;
            }
            return false;
        }

        public static void main(String[] args) {
            Scanner input = new Scanner(System.in);
            Library l1 = new Library("111 Your Street", "9am");
            l1.addBook(new Book("Test", "Author", "Test summary"));
            l1.addBook(new Book("Test2", "Author2", "Test summary2"));
            l1.addBook(new Book("Test3", "Author3", "Test summary3"));
            System.out.println(l1.printAvailableBooks());
            System.out.print("Choose book: ");
            Book book = (Book) input.next();
            borrowBook(book);
        }
    }

Empty Constructor

Is a book with no name, author, or title a book? You should not include empty, overloaded constructors unless you know valid defaults. Empty strings are not valid defaults for titles/authors/descriptions.

Data Types

You could use a Time type for opening hours and an Address type for location. This increases code readability, clarity of your api, and type safety...

private Address location;
private Time openingHours;

Method Names

Your library print functions don't print anything, but rather return values! If you have a method printAddress(), I would suspect that the address is printed to the console. Instead, it would make more sense to call this method getAddress().

These are a few quick things I noticed. I'll try to complete this answer later.


longest sequence of consecutive numbers I am trying to print longest consecutive element in sorted array  for that I have written following brute force algorithm

void Main()
{
    var arr = new[] { 1, 2, 4, 5, 6 ,7,8,10,11,12};

    int max = 0;
    int count = 1;
    int startIndex = 0;
    int start=0;
    int endIndex=0;
    for (int i = 0; i < arr.Length - 2; i++)
    {
        startIndex = i;
        while (i<arr.Length-1 && Math.Abs(arr[i] - arr[i + 1]) == 1)
        {
            i++;
            count++;
        }
        if (max < count)
        {
            max=count;
            start=startIndex;
            endIndex=i;
            count=1;

        }

    }
    for (int k = start; k <= endIndex; k++)
    {
        Console.WriteLine(arr[k]);

    }
}

I need some review comments on my solution( I don't want optimize solution)

I'm assuming in real production the values aren't hard coded and will be passed in.  Then you will want to make some guard clauses.  What is the array is empty or value of one?

You also have more variables than you need, which isn't so bad just still if you know the start and the length of the run you don't need end position.  You can also move startofrun inside the loop.  Also better naming of variable would help.

You don't need to do math.abs when dealing with ints. Just add one and see if the next one is the same value.

Also while it's legal in C# to increase the for loop counter it's not intuitive to someone coming after you.  I would recommend both of them being while loops since the control variable is changing in the loop

For a reference a method like this.  I took out the printing as that's not what this method is intended for

static IEnumerable<int> LongestStreak(params int[] values)
{
    // guard clauses
    if (values == null)
    {
        return Enumerable.Empty<int>();
    }

    // If zero or one then return it right away
    if (values.Length <= 1)
    {
        return values;
    }

    int currentPosition = 0;
    int currentLongestRun = 0;
    int startOfLongestRun = 0;
    while (currentPosition < values.Length - 2)
    {
        int startOfRun = currentPosition;
        while (currentPosition < values.Length - 1 &&
               values[currentPosition] + 1 == values[currentPosition + 1])
        {
            currentPosition++;
        }

        if (currentPosition - startOfRun > currentLongestRun)
        {
            startOfLongestRun = startOfRun;
            currentLongestRun = currentPosition - startOfRun;
        }
        // move to check next sequence
        currentPosition++;
    }

    // C# zero based array so need to add one to the streak
    int[] result = new int[currentLongestRun + 1];
    Array.Copy(values, startOfLongestRun, result, 0, currentLongestRun + 1);
    return result;

}


Computing an approximate value of Pi via Monte Carlo method in Java with streams I have this short program that attempts to compute an approximate value of \$\pi\$:

package net.coderodde.fun;

import java.awt.geom.Point2D;
import java.util.Arrays;
import java.util.Objects;
import java.util.Random;
import java.util.stream.Collectors;
import java.util.stream.IntStream;

/**
 * This class computes an approximate value of Pi.
 *
 * @author Rodion "rodde" Efremov
 * @version 1.6 (Feb 23, 2018)
 */
public class MonteCarloPiComputer {

    /**
     * The default radius of the simulated circle.
     */
    private static final double DEFAULT_RADIUS = 0.5;

    /**
     * The random number generator.
     */
    private final Random random;

    public MonteCarloPiComputer(Random random) {
        this.random =
                Objects.requireNonNull(
                        random,
                        "The input random number generator is null.");
    }

    public MonteCarloPiComputer() {
        this(new Random());
    }

    /**
     * Computes an approximate value of Pi via a Monte Carlo method. The method
     * creates {@code samples} random points, computes the percentage of all
     * points within the radius from the center of the simulated square and
     * multiplies it by {@code 4.0}.
     *
     * @param samples the number of points to create.
     * @param radius  the radius of the simulated circle.
     * @return an approximate value of Pi.
     */
    public double computeApproximateValueOfPi(int samples, double radius) {
        Point2D.Double center = new Point2D.Double(radius, radius);
        double squareSideLength = 2.0 * radius;
        long numberOfPointsWithinCircle =
            IntStream.range(0, samples)
                     .mapToObj(
                             (i) -> {
                                 return new Point2D.Double(
                                         squareSideLength * random.nextDouble(),
                                         squareSideLength * random.nextDouble());
                             })
                     .filter((point) -> {
                         return point.distance(center) < radius;
                     }).count();

        return (4.0 * numberOfPointsWithinCircle) / samples;
    }

    /**
     * Computes an approximate value of Pi via a Monte Carlo method with default
     * radius.
     *
     * @param samples the number of points to create.
     * @return an approximate value of Pi.
     */
    public double computeApproximateValueOfPi(int samples) {
        return computeApproximateValueOfPi(samples, DEFAULT_RADIUS);
    }

    public static void main(String[] args) {
        MonteCarloPiComputer computer = new MonteCarloPiComputer();

        for (int samples = 100_000; samples <= 1_000_000; samples += 100_000) {
            double approximation =
                    computer.computeApproximateValueOfPi(samples);
            double percentage = approximation / Math.PI;
            System.out.print(String.format("%7d: ", samples));
            System.out.print(String.format("%10f", approximation));
            System.out.println(
                    String.format(
                            ", percentage from exact Pi: %10f",
                            (100.0 * percentage)));
        }
    }
}

Critique request

I would like to hear any comments and improvement suggestions.

You have some superfluous imports.  I also think that you can rename computeApproximateValueOfPi() to approximatePi() without losing any clarity.

The simulation is complicated by the fact that the circle is centered at an adjustable point (radius, radius).  You could just as easily use a unit circle centered at the origin, and generate points in a square with x in the range [0, 1) and y in the range [0, 1).  The computation is mathematically equivalent, with less shifting and scaling.

Furthermore, there is no need to instantiate a Point2D for each generated point.  You can use .distance(x, y).  Better yet, avoid computing the square root by using .distanceSq(x, y).

The output routine in main() could be improved.  Your percentage variable doesn't actually store a percentage as its name suggests; the 100× happens in the formatting instead.  Splitting up the formatting into several System.out.print() calls defeats the purpose of String.format().  I'd combine them all into one System.out.format() call.  Finally, the "percentage from" wording implies that you are calculating the difference; the calculation that you actually performed is what I would call "percentage of".

import java.awt.geom.Point2D;
import java.util.Objects;
import java.util.Random;
import java.util.stream.IntStream;

public class MonteCarloPiComputer {
    /**
     * The random number generator.
     */
    private final Random random;

    public MonteCarloPiComputer(Random random) {
        this.random = Objects.requireNonNull(
            random,
            "The input random number generator is null."
        );
    }

    public MonteCarloPiComputer() {
        this(new Random());
    }

    /**
     * Computes an approximate value of Pi via a Monte Carlo method. The method
     * creates {@code samples} random points in the upper-right quadrant,
     * computes the fraction of all points within the radius from the origin
     * and multiplies it by {@code 4.0}.
     *
     * @param samples the number of points to create.
     * @return an approximate value of Pi.
     */
    public double approximatePi(int samples) {
        Random r = this.random;
        Point2D.Double origin = new Point2D.Double();
        long pointsWithinUnitArc = IntStream.range(0, samples)
             .filter(i -> origin.distanceSq(r.nextDouble(), r.nextDouble()) < 1)
             .count();
        return (4.0 * pointsWithinUnitArc) / samples;
    }

    public static void main(String[] args) {
        MonteCarloPiComputer computer = new MonteCarloPiComputer();

        for (int samples = 100_000; samples <= 1_000_000; samples += 100_000) {
            double approximation = computer.approximatePi(samples);
            double pctDiff = 100 * (approximation - Math.PI) / Math.PI;
            System.out.format("%7d: %10f, deviation from exact Pi: %+10f%%%n",
                samples,
                approximation,
                pctDiff
            );
        }
    }
}


Using if else statements correctly I have the following code, but I'm looking for a way to make the code read easier.

public void synchronizePageAndExcecuteVerifyNode() throws Exception {
    String nameTestcase = this.getClass().getSimpleName();
    WriteTo.testgoal(nameTestcase, TestgoalState.nvt, "");

    Boolean testDataGenerated = eventTestData.eventDetailMultipleDays;
    Boolean currentPage = driver.getTitle().contains(Value.TITLE_EVENT_MULTIPLE_DAYS);

    if (testDataGenerated == true) {
        if(currentPage == true){
            this.verifyEventDetailFull();
        }else{
            WriteTo.testgoal(nameTestcase, TestgoalState.NOK,
                    "Page synchronize failed");
            throw new Exception("PAGE SYNCHRONIZE FAILED");
        }
    } else {
        WriteTo.testgoal(nameTestcase, TestgoalState.NOK,
                "testdata is not generated");
        throw new Exception("NO TESTDATA GENERATED");
    }
}

if (condition == true)

Instead of

if (testDataGenerated == true) {

just write

if (testDataGenerated) {

Rewriting ifs

You can move your checks to the top of the method like this to avoid nested ifs:

    if (!testDataGenerated) {
        WriteTo.testgoal(nameTestcase, TestgoalState.NOK,
                "testdata is not generated");
        throw new Exception("NO TESTDATA GENERATED");
    }
    if(!currentPage){
        WriteTo.testgoal(nameTestcase, TestgoalState.NOK,
                "Page synchronize failed");
        throw new Exception("PAGE SYNCHRONIZE FAILED");
    }
    this.verifyEventDetailFull();


Addition plus division taking up all my CPU - Kinect depth denoising I have a method in my class that eats up something like 80% of my processor time while its running. In-fact one line within the method is responsible for the majority of this.

The idea behind this class is to stabilize the depth readings being returned by the Microsoft Kinect device by obtaining the average value of the last 4 frames plus the current one.

This is a the basic layout of the method concerned.

public event EventHandler<DenoisedDepthReadyEventArgs> DenoisedDepthReady;

private const int HistoricFrameCount = 4;

private readonly KinectSensor _KinectSensor;
private List<DepthImagePixel[]> _LastFrames;
private FilteredDepthPixel[] _FilteredDepths;
private bool _ResolutionFilteredDepthsInitialized;

//This is invoked upto 30 times a second
private void KinectSensor_AllFramesReady(object sender, AllFramesReadyEventArgs e)
{
    using (DepthImageFrame depthFrame = e.OpenDepthImageFrame())
    {
        if (depthFrame != null)
        {
            DepthImagePixel[] currentPixels = new DepthImagePixel[depthFrame.PixelDataLength]; //Note this is huge array, usually 307200
            depthFrame.CopyDepthImagePixelDataTo(currentPixels);
            this._LastFrames.Add(currentPixels);
            this.ReduceNoiseAndInvokeIfRequired(currentPixels, depthFrame.PixelDataLength, depthFrame.MaxDepth, depthFrame.MinDepth);
        }
    }
}

private void ReduceNoiseAndInvokeIfRequired(DepthImagePixel[] currentPixels, int totalPixels, int maxDepth, int minDepth)
{
    if (this._LastFrames.Count == HistoricFrameCount)
    {
        if (!this._ResolutionFilteredDepthsInitialized)
        {
            this._FilteredDepths = new FilteredDepthPixel[totalPixels];
            for (int i = 0; i < totalPixels; i++)
            {
                this._FilteredDepths[i] = new FilteredDepthPixel();
            }

            this._ResolutionFilteredDepthsInitialized = true;
        }

        for (int i = 0; i < this._LastFrames.Count; i++)
        {
            DepthImagePixel[] currentFrame = this._LastFrames[i];
            for (int p = 0; p < totalPixels; p++)
            {
                this._FilteredDepths[p].Depth = (this._FilteredDepths[p].Depth + currentFrame[p].Depth) / 2;
            }
        }

        for (int i = 0; i < currentPixels.Length; i++)
        {
            this._FilteredDepths[i].Depth = (this._FilteredDepths[i].Depth + currentPixels[i].Depth) / 2;
            this._FilteredDepths[i].IsKnownDepth = currentPixels[i].IsKnownDepth;
            this._FilteredDepths[i].PlayerIndex = currentPixels[i].PlayerIndex;
        }

        this._LastFrames.RemoveAt(this._LastFrames.Count - 1);

        RaiseOnDenoisedDepthEvent(this._FilteredDepths, maxDepth, minDepth);
    }

}

and here is what the performance profiler says about what I'm doing.

I'm trying to avoid threading and parallelization for the time being. I am also conscious that building a new collection each frame is probably silly too but not sure how to improve this given the immutability of the type I want to return.

Any input or guidance on alternative approaches would be highly appreciated.

@Jesse has already addressed the parallelization fun the way I would have addressed it. This answer merely adds a couple of coding style observations.

private readonly KinectSensor _KinectSensor;
private List<DepthImagePixel[]> _LastFrames;
private FilteredDepthPixel[] _FilteredDepths;
private bool _ResolutionFilteredDepthsInitialized;

These private fields don't follow the camelCase convention that's in order for private fields. Should be like this:

private readonly KinectSensor _kinectSensor;
private List<DepthImagePixel[]> _lastFrames;
private FilteredDepthPixel[] _filteredDepths;
private bool _resolutionFilteredDepthsInitialized;

Actually the last one, named like this, could be a delegate of some kind. I prefer using is or has as a prefix, it makes it easier to find the Boolean flags in the autocomplete dropdown:

private bool _isResolutionFilteredDepthsInitialized;

You're over-qualifying your member variables:

this._lastFrames.Add(currentPixels);
this.ReduceNoiseAndInvokeIfRequired(currentPixels, depthFrame.PixelDataLength, depthFrame.MaxDepth, depthFrame.MinDepth);

The this qualifier is redundant here, and is adding noise that's not needed. Ironic when you consider that the method invoked is called ReduceNoise ;)

_lastFrames.Add(currentPixels);
ReduceNoiseAndInvokeIfRequired(currentPixels, depthFrame.PixelDataLength, depthFrame.MaxDepth, depthFrame.MinDepth);

In terms of readability, I find it particularly rewarding with cases like this:

if (!this._ResolutionFilteredDepthsInitialized)

That turn into this:

if (!_isResolutionFilteredDepthsInitialized)

But then again, your usage of this is consistent all across, so this is basically a matter of personal preference - I prefer only qualifying with this when it's needed.

One thing though, is that if you're going to keep all the this qualifiers, then the _ underscore prefix should be dropped on the private fields.

Your event here:

public event EventHandler<DenoisedDepthReadyEventArgs> DenoisedDepthReady;

Appears to be raised by this method call:

RaiseOnDenoisedDepthEvent(this._FilteredDepths, maxDepth, minDepth);

Convention would be to raise the event in a method like this:

public void OnDenoisedDepthReady(DenoisedDepthReadyEventArgs args)
{
    if (DenoisedDepthReady == null) return;

    DenoisedDepthReady(this, args);
}

Or (and?):

public void OnDenoisedDepthReady(FilteredDepthPixel[] depths, int minDepth, int maxDepth)
{
    if (DenoisedDepthReady == null) return;

    var args = new DenoisedDepthReadyEventArgs(depths, minDepth, maxDepth); // assuming constructor
    DenoisedDepthReady(this, args); // or pass args to above overload
}


Merge Two Sorted Lists in JavaScript The task is taken from LeetCode

Merge two sorted linked lists and return it as a new list. The new list should be made by splicing together the nodes of the first two
lists.
Example 1:

Input: l1 = [1,2,4], l2 = [1,3,4]
Output: [1,1,2,3,4,4]

Example 2:
Input: l1 = [], l2 = []
Output: []

Example 3:
Input: l1 = [], l2 = [0]
Output: [0]

Constraints:

The number of nodes in both lists is in the range [0, 50].
-100 <= Node.val <= 100
Both l1 and l2 are sorted in non-decreasing order.

The following solutions are identical logically. They only differ in style:
Style 1
 /**
 * Definition for singly-linked list.
 * function ListNode(val) {
 *     this.val = val;
 *     this.next = null;
 * }
 */
/**
 * @param {ListNode} l1
 * @param {ListNode} l2
 * @return {ListNode}
 */
var mergeTwoLists = function(l1, l2) {
    if (!l1) return l2;
    if (!l2) return l1;
    if (l1.val < l2.val) {
        l1.next = mergeTwoLists(l1.next, l2);
        return l1;
    }
    l2.next = mergeTwoLists(l2.next, l1);
    return l2;
};

Style 2
 /**
 * Definition for singly-linked list.
 * function ListNode(val) {
 *     this.val = val;
 *     this.next = null;
 * }
 */
/**
 * @param {ListNode} l1
 * @param {ListNode} l2
 * @return {ListNode}
 */
var mergeTwoLists = function(l1, l2) {
    if (!l1 || !l2) return l1 || l2;
    const linkThem = (smaller, greater) => {
        smaller.next = mergeTwoLists(smaller.next, greater);
        return smaller;
    };
    return l1.val < l2.val ? linkThem(l1,l2) : linkThem(l2,l1);
};

Looks good!

I would say maybe the following styles would be just a bit more readable, easier to follow.

We can also alter l1 and l2 with more descriptive variable names.

Line Counting Fallacy:

Sometimes, Line/character countings are helpful for command line languages/scripts (awk, grep, sed, regex, etc.) or maybe Code Golfing, is not a JavaScript practice though.

Iterative using a Sentinel Node
const mergeTwoLists = function(l1, l2) {
    const sentinel = {
        val: -1,
        next: null
    }

    let head = sentinel
    while (l1 && l2) {
        if (l1.val > l2.val) {
            head.next = l2
            l2 = l2.next
        } else {
            head.next = l1
            l1 = l1.next
        }

        head = head.next
    }

    head.next = l1 || l2

    return sentinel.next
}

Recursive
const mergeTwoLists = function(l1, l2) {
    if (l1 === null) {
        return l2
    }

    if (l2 === null) {
        return l1
    }

    if (l1.val < l2.val) {
        l1.next = mergeTwoLists(l1.next, l2)
        return l1

    } else {
        l2.next = mergeTwoLists(l1, l2.next)
        return l2
    }
}

Or even better that those:
const mergeTwoLists = function(l1, l2) {
    if (l1 === null) {
        return l2;
    }

    if (l2 === null) {
        return l1;
    }

    if (l1.val < l2.val) {
        l1.next = mergeTwoLists(l1.next, l2)
        return l1;

    } else {
        l2.next = mergeTwoLists(l1, l2.next)
        return l2;
    }
}

const mergeTwoLists = function(l1, l2) {
    const sentinel = {
        val: -1,
        next: null
    };

    let head = sentinel;
    while (l1 && l2) {
        if (l1.val > l2.val) {
            head.next = l2;
            l2 = l2.next;
        } else {
            head.next = l1;
            l1 = l1.next;
        }

        head = head.next;
    }

    head.next = l1 || l2;

    return sentinel.next;
}


Custom exit with only some exceptions in Python I want to define an exit code for some exceptions and some exceptions only. I want that the only input is that dictionary with the definition.

import sys

exit_codes = { ValueError:'10', KeyError:'25' }

try:
    raise Some_Exception
except Exception as exit_err:
    if any([isinstance(exit_err, exceptions) for exceptions in exit_codes.keys()]):
        exit_code = int(exit_codes[type(exit_err)])
        sys.exit(exit_code)
    else:
        raise exit_err

except Exception is the only part that troubles me, however this seems approach seems safe.

You can use a tuple of exception types with except, so your except clause could be simplified to:

except tuple(exit_codes) as exit_err:
    exit_code = int(exit_codes[type(exit_err)])
    sys.exit(exit_code)


Partition a singly linked list using Java This is my attempted solution to exercise 2.4 in Cracking the Coding Interview.

The problem statement is:

  Write code to partition a linked list around a value x such that all nodes less than x come before all nodes greater than or equal to x.

  Example:

  Input: \$ 3 \rightarrow 5 \rightarrow 8 \rightarrow 5 \rightarrow 10 \rightarrow 2 \rightarrow 1 \$

  Output: \$ 3 \rightarrow 1 \rightarrow 2 \rightarrow 10 \rightarrow 5 \rightarrow 5 \rightarrow 8\$

I used the template code for a singly linked list from the book to write the Node class and I wrote the method partition and a JUnit test case for the method.  I would appreciate any feedback on the algorithm for partition and the code overall.

Node.java

package practice_cracking_code_interview;

public class Node {
    Node next = null;
    int data;

    public Node(int d){
        data = d;
    }

    void appendToTail(int d){
        Node end = new Node(d);
        Node n = this;
        while(n.next != null ){
            n = n.next;
        }
        n.next = end;
    }

    Node partition(Node head, int x){
        Node pRunner = head;
        Node pHead = head;
        Node pTemp = null;

        while(pRunner != null && pRunner.next != null){
            if(pRunner.next.data < x){
                pTemp = pRunner.next.next;
                pRunner.next.next = pHead;
                pHead = pRunner.next;
                pRunner.next = pTemp;
            }
            else{
                pRunner = pRunner.next;
            }
        }
        return pHead;
    }
}

TestNode.java

package practice_cracking_code_interview;

import junit.framework.TestCase;

public class TestNode extends TestCase {

    public void testAppendToTail(){
        Node head = new Node(5);
        head.appendToTail(10);
        assertEquals(head.next.data, 10);
    }

    public void testPartitionOne(){
        Node head = new Node(3);
        head.appendToTail(5);
        head.appendToTail(8);
        head.appendToTail(5);
        head.appendToTail(10);
        head.appendToTail(2);
        head.appendToTail(1);
        Node n = head.partition(head, 5);

        int[] expected = new int[]{1, 2, 3, 5, 8, 5, 10};
        int i = 0;

        while(n != null){
            assertEquals(n.data, expected[i]);
            i++;
            n = n.next;
        }
    }

    public void testPartitionTwo(){
        Node head = new Node(3);
        head.appendToTail(5);
        head.appendToTail(8);
        head.appendToTail(5);
        head.appendToTail(10);
        head.appendToTail(2);
        head.appendToTail(1);
        Node n = head.partition(head, 11);

        int[] expected = new int[]{1, 2, 10, 5, 8, 5, 3};
        int i = 0;

        while(n != null){
            assertEquals(n.data, expected[i]);
            i++;
            n = n.next;
        }
    }
}

The only suggestions I have are:

make the implementation look as "obvious,"
guarantee stability: the elements belonging in one of the sublists will preserve their relative order.

And this is what I had in mind:

public class MyList {

    private static final class MyListNode {
        int value;
        MyListNode next;

        MyListNode(int value) {
            this.value = value;
        }
    }

    private MyListNode head;
    private MyListNode tail;

    public void add(int value) {
        MyListNode newnode = new MyListNode(value);

        if (tail == null) {
            head = newnode;
        } else {
            tail.next = newnode;
        }

        tail = newnode;
    }

    public void partition(int pivot) {
        MyListNode smallListHead = null;
        MyListNode smallListTail = null;
        MyListNode largeListHead = null;
        MyListNode largeListTail = null;

        while (head != null) {
            // Remove the head node H:
            MyListNode currentNode = head;
            head = head.next;

            // Decide to which of the two lists to append the node H:
            if (currentNode.value < pivot) {
                if (smallListTail == null) {
                    smallListHead = currentNode;
                } else {
                    smallListTail.next = currentNode;
                }

                smallListTail = currentNode;
            } else {
                if (largeListTail == null) {
                    largeListHead = currentNode;
                } else {
                    largeListTail.next = currentNode;
                }

                largeListTail = currentNode;
            }
        }

        if (smallListHead == null) {
            head = largeListHead;
        } else if (largeListHead == null) {
            head = smallListHead;
        } else {
            head = smallListHead;
            smallListTail.next = largeListHead;
            tail = largeListTail;
            // Marks the end of the list, otherwise might be a cycle:
            largeListTail.next = null;
        }
    }

    public String toString() {
        StringBuilder sb = new StringBuilder().append('[');
        String separator = "";

        for (MyListNode node = head; node != null; node = node.next) {
            sb.append(separator).append(node.value);
            separator = ", ";
        }

        return sb.append(']').toString();
    }

    public static void main(final String... args) {
        MyList list = new MyList();

        list.add(3);
        list.add(5);
        list.add(8);
        list.add(5);
        list.add(10);
        list.add(2);
        list.add(1);

        System.out.println(list);
        list.partition(5);
        System.out.println(list);
        list.partition(0);
        System.out.println(list);
        list.partition(100);
        System.out.println(list);

        list = new MyList();
        list.partition(1);
        System.out.println(list);
    }
}

What comes to the complexity of my alternative, it is, at least, not worse than yours. As an additional fun, you could consider implementing quicksort for linked lists using the partition procedure you have.

Hope that helps.


Regex to get all image links I have some pretty basic Regex that scans the output of a HTML file (the whole document source) and attempts to extract all of the absolute links that look like images. Whether they are actually images or not isn't too important, as those checks would be made later.

I am just trying to see if this Regex looks okay, since I kind of hacked it together from various sources.

The Raw Regex Pattern:

\bhttps?:[^)''"]+\.(?:jpg|jpeg|gif|png)

The Regex in Practice (I am coding in Railo - an open source CFML engine):

<cfset variables.getImageURLs = reMatch('\bhttps?:[^)''"]+\.(?:jpg|jpeg|gif|png)', variables.getDocument) />

NOTE: variables.getDocument contains the raw HTML code.

It seems to work great right now and I'm not seeing any issues, but just wanting to know if you can see any potential pitfalls with this method? Is there room for improvement?

If you're truly only looking for images, why not parse any src attributes you can find? As it stands, all of the provided Regular Expressions will fail on local images, as well as on dynamically generated images, such as:

<img src="/local/images/get_profile_pic.php?id=12345" title="John Doe" />

This is perfectly valid, and none of the other expressions would capture them. Generally, for parsing HTML, you're better of traversing it with something like an XML reader, or a DOM builder, depending on how well the HTML is formed.

Ben Nadel has a pretty good blog article on working with badly formed HTML and translating into an XML document. Once you have an XML document, you can unleash the power of XPath to do some very nice searching.

If, however, you just want a quick and dirty RegEx to get the job done, I'd recommend actually using the underlying Java Regular Expression library, as it's probably more efficient when matching multiple items over a large document.

The main RegEx I'll be using is as follows:

<img\s+[^>]*?src=("|')([^"']+)\1

Which looks for the src attribute in img tags. You can vary this as you see fit.

<cfset html = variables.getDocument /> <!--- your HTML --->
<cfset pattern = CreateObject("java","java.util.regex.Pattern").compile('(?i)<img\s+[^>]*?src=("|')([^"']+)\1') />
<cfset matcher = pattern.matcher(html) />

<!--- loop through the matches --->
<cfloop condition="matcher.find()">
    <cfset src = matcher.group(2) />
</cfloop>

UPDATE :
Here's an explanation of the pattern I chose:

<img           - Literal string "<img", match the opening tag
\s+            - Match one or more whitespace characters, so <img\t is valid
[^>]*?         - Lazily match any character that is not a '>' while looking for the next literal string
src=           - Literal string "src="
("|')          - Match either a single or a double quote, both are valid in HTML
([^"']+)       - Match anything that isn't a single or double quote. Note: You *could* use [^\1] here, however this way the match will reject malformed HTML attributes that have mismatched quotes
\1             - Match the value of the first group (either a single or double quote)


Converting Array of `UINT8` (`unsigned char`) to Array of `Float32` (`float`) Using AVX2 Given input array of UINT8 (unsigned char) with numElements how could one efficiently convert it into array of Float32 (float)?

For instance, here is a vanilla code for it (Pay attention there is a scaling operation):

void ConvertFromUint8(float* mO, unsigned char* mI, int numElements, float scalingFctr)
{
    int ii;
    for (ii = 0; ii < numElements; ii++) {
        mO[ii] = (float)(mI[ii]) * scalingFctr;
    }
}

Where mO is the output array.

I need a code which utilizes up to AVX2 intrinsics.
The objective is to yield faster code than the vanilla example as in Compiler Explorer - ConvertFromUint8.

A straightforward transliterating to AVX2 intrinsics works, but I didn't like what the compilers made of it.

For example, an obvious approach is to load 8 bytes, widen them to 8 ints, etc. And that obvious way to do that, I think, is with _mm_loadl_epi64 to do the loading. Unfortunately, MSVC and even GCC refuse to merge a _mm_loadl_epi64 into the memory operand of _mm256_cvtepu8_epi32, and there is no overload of _mm256_cvtepu8_epi32 with an explicit memory operand.. Using _mm_loadu_si128 to do the loading is fine and does merge, but that merger means that the 16-byte loading intrinsic is used but only 8 bytes of memory are actually loaded. It's strange, but it works, though it may make some people nervous to use this near the end of the data as it looks like it would read past the end.

Anyway, my first concern was the stores. The GCC auto-vectorized version split the 256bit store into two 128bit stores, perhaps to avoid unaligned 256bit stores. But it's not so hard to align the destination, assuming mO is at least 8-aligned, so I'd say that's a better approach. The ICC auto-vectorized version doesn't try to avoid the big potentially-unaligned stores, perhaps it hopes for the best or thinks it shouldn't matter much. It is my understanding though that we should avoid wide unaligned stores (in the sense of the address actually being unaligned) as long as the cost for doing so is reasonable. The ICC versions also avoids small loads, preferring this construct:

vmovdqu ymm3, YMMWORD PTR [r9+rsi] #15.26
vextracti128 xmm7, ymm3, 1 #15.26
vpsrldq xmm4, xmm3, 8 #15.26
vpsrldq xmm8, xmm7, 8 #15.26

I don't like it, this trades 4 loads (in the form of memory operands of vpmovzxbd) for a large load and some shuffle-type operations. That raises the total to 7 shuffle-type operations per iteration, they all need to go to p5 on current Intel µarchs, so that's a likely bottleneck. LLVM-MCA agrees with that and calculates that the loop takes just over 7 cycles per iteration on average, due to p5 contention. Plus, such a larger load increases to ratio of "slow loads" (eg 4K crossings and cache misses) to "fast loads", and makes more work dependent on that slow load, making it less likely that OoOE can hide the slowness.

On the other hand with 4 separate loads, the loop is like this (code below, compiled with ICC):

..B2.8: # Preds ..B2.47 ..B2.6 ..B2.8
  vpmovzxbd ymm2, QWORD PTR [rax+rsi] #34.42
  vpmovzxbd ymm5, QWORD PTR [8+rax+rsi] #36.42
  vpmovzxbd ymm8, QWORD PTR [16+rax+rsi] #38.42
  vpmovzxbd ymm11, QWORD PTR [24+rax+rsi] #40.42
  vcvtdq2ps ymm3, ymm2 #34.23
  vcvtdq2ps ymm6, ymm5 #36.23
  vcvtdq2ps ymm9, ymm8 #38.23
  vcvtdq2ps ymm12, ymm11 #40.23
  vmulps ymm4, ymm0, ymm3 #35.42
  vmulps ymm7, ymm0, ymm6 #37.46
  vmulps ymm10, ymm0, ymm9 #39.47
  vmulps ymm13, ymm0, ymm12 #41.47
  vmovups YMMWORD PTR [rdi+rax*4], ymm4 #35.33
  vmovups YMMWORD PTR [32+rdi+rax*4], ymm7 #37.33
  vmovups YMMWORD PTR [64+rdi+rax*4], ymm10 #39.33
  vmovups YMMWORD PTR [96+rdi+rax*4], ymm13 #41.33
  add rax, 32 #33.43
  cmp rax, rcx #33.39
  jb ..B2.8 # Prob 82% #33.39

Which LLVM-MCA thinks is just under 5 cycles per iteration, which seems good to me. This could be improved slightly by unrolling even more, because the scalar arithmetic does "get in the way" a bit.

By the way I changed some int to size_t to avoid some sign-extension, it wasn't really a big deal though.

void ConvertFromUint8_AVX2(float* mO, unsigned char* mI, size_t numElements, float scalingFctr)
{
    size_t ii;
    __m256 vscalingFctr, tmp;

    vscalingFctr = _mm256_set1_ps(scalingFctr);

    // prologue, do scalar iterations until the output address is 32-aligned
    for (ii = 0; ii < numElements && ((uintptr_t)(mO + ii) & 31); ii++) {
        mO[ii] = (float)(mI[ii]) * scalingFctr;
    }
    // main loop
    if (numElements >= 32) {
        for (; ii < numElements - 31; ii += 32) {
            tmp = _mm256_cvtepi32_ps(_mm256_cvtepu8_epi32(_mm_loadu_si128((__m128i*)(mI + ii))));
            _mm256_store_ps(mO + ii, _mm256_mul_ps(tmp, vscalingFctr));
            tmp = _mm256_cvtepi32_ps(_mm256_cvtepu8_epi32(_mm_loadu_si128((__m128i*)(mI + ii + 8))));
            _mm256_store_ps(mO + ii + 8, _mm256_mul_ps(tmp, vscalingFctr));
            tmp = _mm256_cvtepi32_ps(_mm256_cvtepu8_epi32(_mm_loadu_si128((__m128i*)(mI + ii + 16))));
            _mm256_store_ps(mO + ii + 16, _mm256_mul_ps(tmp, vscalingFctr));
            tmp = _mm256_cvtepi32_ps(_mm256_cvtepu8_epi32(_mm_loadu_si128((__m128i*)(mI + ii + 24))));
            _mm256_store_ps(mO + ii + 24, _mm256_mul_ps(tmp, vscalingFctr));
        }
    }
    // epilogue
    for (; ii < numElements; ii++) {
        mO[ii] = (float)(mI[ii]) * scalingFctr;
    }
}


Penney Game - mapping macros to strings The SPOJ problem:

  Given a 40 character string representing outcomes of 40 coin tosses,
  find the frequency destribution of all the possible outcome triplets.
  So, for string like : HHHH....40 Hs ,the result is HHH : 38 and the
  rest 0. The exact input output format is given in the link.

My approach, as is clear from the code, is to iterate through the string and match all consecutive sub-strings of length 3, while increasing the frequency count.

My solution got accepted, but I am not really happy with what I have done here. As you can see, there is a lot of redundant code I could not remove.

For instance, if string "TTT" could be directly mapped to the macro TTT, somehow, and so on, a large number of lines of code could have been reduced. I know technically strings and macros are two different things, but is there some trick that I am missing?

#include<stdio.h>
#include<stdlib.h>
#include<string.h>
#include<limits.h>
#define TTT 0
#define TTH 1
#define THT 2
#define THH 3
#define HTT 4
#define HTH 5
#define HHT 6
#define HHH 7
void getCount(char str[] , int count[]){
    int i;
    for(i=0;i<8;++i)
        count[i]=0;

    int l = strlen(str);
    for(i=2;i<l;++i){
        if(strncmp(str+i-2 , "TTT" , 3)==0)
            ++count[TTT];
        if(strncmp(str+i-2 , "TTH" , 3)==0)
            ++count[TTH];
        if(strncmp(str+i-2 , "THT" , 3)==0)
            ++count[THT];
        if(strncmp(str+i-2 , "THH" , 3)==0)
            ++count[THH];
        if(strncmp(str+i-2 , "HTT" , 3)==0)
            ++count[HTT];
        if(strncmp(str+i-2 , "HTH" , 3)==0)
            ++count[HTH];
        if(strncmp(str+i-2 , "HHT" , 3)==0)
            ++count[HHT];
        if(strncmp(str+i-2 , "HHH" , 3)==0)
            ++count[HHH];
    }
}
int main()
{
    char str[41];
    int t,n,count[8],i;
    scanf("%d",&t);
    while(t--){
        scanf("%d",&n);
        printf("%d ",n);

        scanf("%s",str);
        getCount(str,count);

        for(i=0;i<8;++i){
            printf("%d ",count[i]);
        }
        printf("\n");
    }
    return 0;
}

Associating a set of constants with corresponding strings is difficult in C.  One approach is to define a structure, such as

struct patterns {
    const char combination[4];
    int count;
};

Here we have space for a string and an integer to count occurrences.
Creating an array of these and initializing it gives us this:

struct patterns patterns[N_PATTERNS] = {
    {"HHH", 0}, {"HHT", 0}, {"HTH", 0}, {"HTT", 0},
    {"THH", 0}, {"THT", 0}, {"TTH", 0}, {"TTT", 0},
};

Then you can loop through the strings by accessing each array member and
incrementing the counts.  The count and string are kept together so that
indexing errors cannot happen.

Also, your main is too complicated.  Just reading the string from the
command line is easier.

Putting it together, gives me this:

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

#define N_PATTERNS 8
struct patterns {
    const char combination[4];
    int count;
};

static int increment_occurance(struct patterns *p, const char *s)
{
    for (int i = 0; i < N_PATTERNS; ++i) {
        if (!memcmp(s, p[i].combination, 3)) {
            p[i].count += 1;
            return 1;
        }
    }
    return 0;
}

static void count_occurances(struct patterns *p, const char *s)
{
    const char *end = s + strlen(s) - 2;

    for (; s < end; ++s) {
        increment_occurance(p, s);
    }
}

int main(int argc, char **argv)
{
    struct patterns patterns[N_PATTERNS] = {
        {"HHH", 0}, {"HHT", 0}, {"HTH", 0}, {"HTT", 0},
        {"THH", 0}, {"THT", 0}, {"TTH", 0}, {"TTT", 0},
    };
    if (argc < 2) {
        fprintf(stderr, "Usage: %s string\n", argv[0]);
        return EXIT_FAILURE;
    }
    count_occurances(patterns, argv[1]);

    for (int i = 0; i < N_PATTERNS; ++i) {
        printf("%s %d\n", patterns[i].combination, patterns[i].count);
    }
    return EXIT_SUCCESS;
}

EDIT: note that we can use the more efficient memcmp instead of strncmp in increment_occurance.


Random array of numbers without repeating - oojs practice I'd like to practice oojs so I've written code:

function Randoms(c, l, h) {
    var count = c,
        min = l,
        max = h,
        nums = Array(),
        self = this;

    //returns random number from given max and min
    var getRand = function () {
        return Math.floor(Math.random() * (max - min) + min);

    };

    //checks if given number is unique
    var check = function (a) {
        for (var i = nums.length - 1; i >= 0; i--) {
            if (nums[i] == a) {
                return true;
            }
        }
        return false;
    };

    //makes array with random, unique numbers
    this.build = function () {

        var r = getRand();

        if (nums.length === 0) {
            nums.push(r);
        } else {
            do {
                r = getRand();
            } while (check(r));

            nums.push(r);
        }

        if (nums.length == count) {
            return true;
        } else {
            this.build();
        }
    };

    //returns array
    this.get = function () {
        return nums;
    };

    //simulating constructor
    this.build();

}

var a = new Randoms(1, 1, 18);
console.log(a.get());

The purpose is to get array with random, unique numbers.

What do you think? Is it a good code or does it require modyfications?

Your check method is inefficient for large datasets. I'd recommend using a hash instead. This will eliminate the need of iterating over the array every time you want to check if a number is there.
It is possible for your code to enter an infinite loop if count is greater than max - min. You need to test for this.
The code can be simplified a bit as well.
The recursion slows down the generation dramatically.

I've take then liberty of performing some optimizations: http://jsfiddle.net/FMSpG/2/

function Randoms(c, l, h) {
    var count = c,
        min = l,
        max = h,
        nums = {},
        out = [],
        r, len = 0;

    if (max - min < count) count = max - min;

    var getRand = function() {
        return Math.floor(Math.random() * (max - min) + min);
    },  check = function(a) {
        return nums[a];
    },  add = function(a) {
        nums[a] = 1;
        out.push(a);
        len++;
    };

    (function init() {
        while (len < count){
            if (len == 0) r = getRand();
            else while (check(r = getRand()));
            add(r);
        }
    })();

    this.get = function() {
        return out;
    };
}

And here's a jsPerf showing the huge difference in performance for large datasets:

http://jsperf.com/unique-randomization


Reverse the word in a string with the same order in javascript I want reverse the string with the same order. We should not use the Array functions like split(), .reverse() and some other array functions also except array.length.

Here i am attached my code. How to achieve that thing more efficiently.

var str = "i am javascript";
// result "i ma tpircsavaj"
function reverseString(myStr){
    var strlen = myStr.length, result = "", reverseStr = "", reverseStrArr = [];
  for(var i = strlen-1; i >= 0; i--){
    reverseStr +=  myStr[i];
  }

    for(var j = 0; j < strlen; j++){
    if(reverseStr[j] == " "){
      reverseStrArr.push(result);
      result = "";
    }else{
      result += reverseStr[j];
      if(j + 1 == strlen){
        reverseStrArr.push(result);
        result = "";
      }
    }
  }

  for(var k=reverseStrArr.length - 1; k >= 0; k--){
    result += reverseStrArr[k] + " "
  }
  console.log(result);
}
reverseString(str);

I would split this into two function, simply because they do different things:

First a simple string reverser:

function reverseString(input)
{
    var output = "";
    for(var i = input.length - 1; i >= 0; i--) {
        output +=  input[i];
    }
    return output;
}

This is an easy function that everybody can understand. But we need to reverse words, so we make another function for that.

function reverseWords(input)
{
    var word = "", output = "";
    for(var i = 0; i < input.length; i++) {
        if (input[i] == " ") {
            output += reverseString(word) + " ";
            word = "";
        }
        else {
            word += input[i];
        }
    }
    return output + reverseString(word);
}

And that's it. Now this might not be the niftiest, or shortest, solution, but it is one that reasonably easy to understand, and you get two functions for the price of one.

I had to correct this code after a comment from Blindman67 saying it didn't work for some cases.

A shorter version is also possible:

function reverseWordsShorter(input)
{
    var word = "", output = "";
    for(var i = input.length - 1; i >= 0; i--) {
        if (input[i] == " ") {
            output = " " + word + output;
            word = "";
        }
        else {
            word += input[i];
        }
    }
    return word + output;
}

Also notice that there's no erroneous space at the end anymore.


Standard Test methods for checking values in Lists, Maps etc I needed to check the values in various Lists and Maps. In my case, the various types of Objects that needed checking were of four types:

String
List<String>
Map<String, String>
Map<String, List<String>>

So, I came up with the following types test methods:

@Test
public void testString() {
    Assert.assertEquals("String Mismatch;", providedString, receivedString);
}

@Test
public void testList() {
    Assert.assertNotNull("Provided List is null;", providedList);
    Assert.assertNotNull("Received List is null;", receivedList);
    Assert.assertEquals("Size mismatch for lists;", providedList.size(), receivedList.size());
    Assert.assertTrue("Missing values in received list; Missing: " + CollectionUtils.subtract(providedList, receivedList) + "; ", receivedList.containsAll(providedList));
}

@Test
public void testMap() {
    Assert.assertNotNull("Provided Map is null;", providedMap);
    Assert.assertNotNull("Received Map is null;", receivedMap);
    Assert.assertEquals("Size mismatch for maps;", providedMap.size(), receivedMap.size());
    Assert.assertTrue("Missing keys in received map;", receivedMap.keySet().containsAll(providedMap.keySet()));

    providedMap.keySet().stream().forEach((key) -> {
        Assert.assertEquals("Value mismatch for key '" + key + "';", providedMap.get(key), receivedMap.get(key));
    });
}

@Test
public void testMapList() {
    Assert.assertNotNull("Provided Map is null;", providedMapList);
    Assert.assertNotNull("Received Map is null;", receivedMapList);
    Assert.assertEquals("Size mismatch for maps;", providedMapList.size(), receivedMapList.size());
    Assert.assertTrue("Missing keys in received map;", receivedMapList.keySet().containsAll(providedMapList.keySet()));

    providedMapList.keySet().stream().forEach((key) -> {
        List<String> pList = providedMapList.get(key);
        List<String> rList = receivedMapList.get(key);

        Assert.assertNotNull("Provided List is null for key '" + key + "';", pList);
        Assert.assertNotNull("Received List is null for key '" + key + "';", rList);
        Assert.assertEquals("Size mismatch for lists for key '" + key + "';", pList.size(), rList.size());
        Assert.assertTrue("Missing values in received list for key '" + key + "';", rList.containsAll(pList));
    });
}

The test methods in my project follow the above standard methods depending upon the object that I'm testing for. They are working fine but I'm not completely confident about the coverage.

Also, in the above snippets I've used provided and received as placeholders. In the actual test cases, certain methods are being called to populate the corresponding objects for provided and received. So, they are not necessarily being populated/initialized in @BeforeClass or @Before.

Should I be creating different Test methods for each assert (excluding assertNotNull() methods) that I'm doing? Am I missing any case? Can this be made shorter with the coverage being the same?

Update:

Full Code for testing: Standard Test

Create a TestRunner class to run it and get the results using the following:

Result result = JUnitCore.runClasses(StandardTest.class);

Map equals has same coverage and it is not attached to implementation: See map.equals javadoc.

So "better" (at least, more compact code) would be:

  assertEquals("something", providedMap, receivedMap);

Well, same for list. That means, you can use the same command as above.

  assertEquals("something", providedList, receivedList);

And even more...

  assertEquals("something", providedMapList, receivedMapList);

Sure, you will not see which key is missing.

So, you can create new static matchers, as assertEquals that could provide more information. But, assertEquals already prints the content of each container (executing toString of expected and received object), so you can figure out what is missing anyway.

Example:

java.lang.AssertionError:
Expected :[1, 2, 3, 4, 6]
Actual   :[1, 2, 3, 4, 5]

Edited

If the output is too big, you can use your method. There are some options to follow:

Create a class with static methods (similar with org.junit.Assert)

public class ContainerAssert {

public static <U, V >void assertEquals(Map<U, V> providedMap, Map<U, V> receivedMap) {
    Assert.assertNotNull("Provided Map is null;", providedMap);
    Assert.assertNotNull("Received Map is null;", receivedMap);
    Assert.assertEquals("Size mismatch for maps;", providedMap.size(), receivedMap.size());
    Assert.assertTrue("Missing keys in received map;", receivedMap.keySet().containsAll(providedMap.keySet()));

    providedMap.keySet().stream().forEach((key) -> {
        Assert.assertEquals("Value mismatch for key '" + key + "';", providedMap.get(key), receivedMap.get(key));
    });
  }
  // ...
}

Use Hamcrest

 // with strict order
 List<String> collection = Lists.newArrayList("ab", "cd", "ef");
 assertThat(collection, contains("ab", "cd", "ef"));

Use Hamcrest and create a specialized matcher


Parsing item page for an online shop In this function I'm parsing the page of the item in the online shop. Some items lack the picture, some lack price etc, so there are few if-else checks.

My questions are:

How to get rid of these ugly if-else checks?
How I can "pythonize" my code further?
Is it correct? I.e when I manipulate the variable value using previously assigned value:

name = soup.find('span',{"class":"project_name"})
if name is not None:
    name = re.sub(r'[«,»]','', name.text)

Below is my full code:

def grab_properties(url,html):
    soup = BeautifulSoup(html)
    link = soup.find('iframe', {"src":"e.smth.com"})

    if link is not None:
        link = link.get("src")
        id = re.match(".*\[(\w+)\].*", link).group(1)
        if id is not None:
            if id not in ids:
                name = soup.find('span',{"class":"project_name"})
                if name is not None:
                    name = re.sub(r'[«,»]','', name.text)
                    if (soup.find('img',{"id":"item0image"}) is not None):
                        img_url = soup.find('img',{"id":"item0image"}).get('src')
                        price = soup.find('span',{"class":"project_price"})
                        if price is not None:
                            price = price.text
                            price = re.sub(u'[руб,\., ]','', price)
                        else:
                            price =u"Call us!"
                        return [id.encode('utf8'), name.encode('utf8'), url.encode('utf8'), img_url.encode('utf8'), price.encode('utf8')]

You cannot get rid of those if-else checks, but you can get rid of the nesting. For example:

if foo:
    if bar:
        return foo + bar

could be flattened to

if not foo:
    return None
if not bar:
    return None
return foo + bar

While this is is in fact longer, such code tends to be easier to read.

If we apply that to your code (and fix some style issues like putting a space after each comma), then we end up with:

def grab_properties(url, html):
    soup = BeautifulSoup(html)

    link = soup.find('iframe', {"src": "e.smth.com"})
    if link is None:
        return None
    link = link.get("src")

    id = re.match(".*\[(\w+)\].*", link).group(1)
    if (id is None) or (id in ids):
        return None

    name = soup.find('span', {"class": "project_name"})
    if name is None:
        return None
    name = re.sub(r'[«,»]', '', name.text)

    image_element = soup.find('img', {"id": "item0image"})
    if image_element is None:
        return None
    img_url = image_element.get('src')

    price = soup.find('span', {"class": "project_price"})
    if price is None:
        price = u"Call us!"
    else
        price = re.sub(u'[руб,\., ]', '', price.text)

    return [attr.encode('utf8') for atr in (id, name, url, img_url, price)]

Notice that I created the image_element variable to avoid searching for the same element multiple times, that I used a list comprehension to encode various strings to UTF-8, and that I used empty lines to separate the code for unrelated attributes, thus making the code easier to understand. I also swapped the if price is not None condition around to avoid a confusing negation.

We should now talk about regular expressions (see also the re library documentation). The […] is a character class. It matches any of the contained characters, so [abc] will match either a or b or c, but not the whole string abc. The comma is not special inside a character class, so [«,»] will match left or right guillemets, or a comma. To match just the angled quotation marks, use [«»]. Similarly, [руб,\., ] will match either р or у or б or a comma or a period or a space. Note that inside a character class, the period is not a metacharacter and does not have to be escaped. So that charclass would be equivalent to [р,у б.] (the order of characters does not matter). If you want to match alternative patterns, use the | regex operator: руб|[. ], which matches either the substring руб or a period or a space.

You could also consider precompiling your regular expressions, which might improve performance if you call your function very often:

RE_GUILLEMETS = re.compile('[«»]')

def ...:
    ...
    name = RE_GUILLEMETS.sub('', name.text)


Parallelization of number factors using OpenMP For a simple try at parallelization on my own outside of school, I've created a number factors calculator.  I hope to eventually come up with something more creative.

Since I don't have access to parallel computers at this time, I'm using OpenMP provided by my compiler (gcc 4.8.1) and running it on my laptop (Intel Core i3-2330M).  I'm using a maximum of four threads, which was determined from a call to omp_get_max_threads().

I've conducted four runs, each with four billion values and from one to four threads:

#include <cstdint>
#include <cstdlib>
#include <ctime>
#include <iomanip>
#include <iostream>
#include <map>
#include <omp.h>

void displayCompTime(std::clock_t start, std::clock_t end, std::int64_t integer, int threads)
{
    double elapsed = static_cast<double>(end - start) / CLOCKS_PER_SEC;

    std::cout << integer << " values and " << threads << " thread(s): "
              << std::setprecision(4) << std::fixed << elapsed << "s\n";
}

void calcFactors(std::int64_t integer, int threads)
{
    std::map<std::int64_t, std::int64_t> factors;
    std::int64_t i;

    #pragma omp parallel for num_threads(threads) default(none) \
        shared(factors, integer), private(i)
    for (i = 2; i <= integer; i++)
    {
        if (integer % i == 0)
        {
            factors[i] = integer / i;
        }
    }
}

int main()
{
    const std::int64_t integer = 4000000000;
    const int runs = 4;

    for (int i = 0; i < runs; i++)
    {
        std::clock_t start = std::clock();
        int threads = i + 1;
        calcFactors(integer, threads);
        std::clock_t end = std::clock();
        displayCompTime(start, end, integer, threads);
    }
}

Output:

4000000000 values and 1 thread(s): 67.7330s
4000000000 values and 2 thread(s): 40.7640s
4000000000 values and 3 thread(s): 32.5630s
4000000000 values and 4 thread(s): 29.7640s

Based on these results, this code doesn't appear to scale very well.  I don't know if using a non-default static schedule would give faster times, and anything else would just incur additional overhead.  Fortunately, I didn't need to include atomic or critical.

Would avoiding a lot of division help?  I didn't try for anything else yet as this is only a start.  I also wanted to see how well my laptop could handle parallelization.

Other than performance, I'm okay with any general OpenMP advice.  I was sure to use some good practices for that, such as default(none) for explicitly listing the variables.

I see that OpenMP's rules will make things a little difficult here.  For instance, I won't be able to concisely set i to either 2 or 3, depending on integer's parity.  This is because OpenMP requires the loop counter to be set within the loop, though it can still be declared beforehand.  I would otherwise have to put a ternary within the loop statement, which would look ugly.  It could save one iteration, which may not make a huge difference.

As such, I might as well initialize i within the loop statement and then remove the private part from the preprocessor directive:

    shared(factors, integer)
for (std::int64_t i = 2; i <= integer; i++)

Performance-wise, I was able to get a small boost by initializing factors outside of the timed section and passing it to calcFactors().  Regardless of the time it usually takes to initialize an std::map, my runtime will always be limited by that as it's part of the serial code.

With the same division and modulus operations, I would likely not get any significant performance boost.  Regardless of the thread count, the number of modulus and division operations still vary, which may explain the poor scalability over time.  I also cannot test with higher thread counts due to my machine's limited number of available threads.


Sample repository class How can I make it shorter and cleaner?

using Queries.Core.Repositories;
using System;
using System.Collections.Generic;
using System.Data.Entity;
using System.Linq;
using System.Linq.Expressions;

namespace Queries.Persistence.Repositories
{
public class Repository<TEntity> : IRepository<TEntity> where TEntity : class
{
    protected readonly DbContext Context;

    public Repository(DbContext context)
    {
        Context = context;
    }

    public TEntity Get(int id)
    {
        return Context.Set<TEntity>().Find(id);
    }

    public IEnumerable<TEntity> GetAll()
    {
        return Context.Set<TEntity>().ToList();
    }

    public IEnumerable<TEntity> Find(Expression<Func<TEntity, bool>> predicate)
    {
        return Context.Set<TEntity>().Where(predicate);
    }

    public TEntity SingleOrDefault(Expression<Func<TEntity, bool>> predicate)
    {
        return Context.Set<TEntity>().SingleOrDefault(predicate);
    }

    public void Add(TEntity entity)
    {
        Context.Set<TEntity>().Add(entity);
    }

    public void AddRange(IEnumerable<TEntity> entities)
    {
        Context.Set<TEntity>().AddRange(entities);
    }

    public void Remove(TEntity entity)
    {
        Context.Set<TEntity>().Remove(entity);
    }

    public void RemoveRange(IEnumerable<TEntity> entities)
    {
        Context.Set<TEntity>().RemoveRange(entities);
    }
}
}

If you're worried about repeating Context.Set<TEntity>() about the place, you can assign a field in your constructor instead:

public class Repository<TEntity> : IRepository<TEntity> where TEntity : class
{
    protected readonly DbContext Context;
    protected readonly IDbSet<TEntity> Set;

    public Repository(DbContext context)
    {
        Context = context;
        Set = context.Set<TEntity>();
    }

    public TEntity Get(int id)
    {
        return Set.Find(id);
    }
}

I would say that I don't generally like generic repositories but I won't get in to that now. A lot of people use the pattern and I don't know of any canonical 'this is bad' sources so am happy to disagree over it.

One thing you do need to consider though: multiple column primary keys. That's why Find takes an array (params). You're making a method that will throw if the object has a multiple column primary key (Get).


Managing a SQLite Database Connection When the application connects to the database it keeps the connection open until the user closes the file using File->Close.

Is this bad practice?

Should I be opening and closing the connection every time I need to use it?

using System.Data;
using System.Data.SQLite;
using System.IO;
using System.Windows.Forms;

namespace timelord
{
    /// <summary>
    /// Manages the tasks database
    /// </summary>
    class Timesheet
    {
        private string filePath;
        private SQLiteConnection sqlite;
        private SQLiteDataAdapter adapter;
        private SQLiteCommandBuilder builder;
        private DataTable datatable;

        /// <summary>
        /// Creates a timesheet object that determines if the filepath exists.
        /// If the filepath exists then it opens a database.
        /// If it does not exist it creates a database.
        /// </summary>
        /// <param name="filePath">The path to open or create a database file at.</param>
        public Timesheet(string filePath)
        {
            this.filePath = filePath;

            if (File.Exists(filePath))
            {
                openDatabase();

            }else
            {
                createDatabase();

                createSchema();
            }

            prepareQueries();

            this.datatable = new DataTable();

            this.adapter.Fill(this.datatable);
        }

        /// <summary>
        /// Calls the methods that create the database file, connection, and schema
        /// </summary>
        private void createDatabase()
        {
            SQLiteConnection.CreateFile(filePath);

            openDatabase();
        }

        /// <summary>
        /// Creates the database connection
        /// </summary>
        private void openDatabase()
        {
            sqlite = new SQLiteConnection("Data Source=" + this.filePath + ";Version=3;");
            try
            {
                sqlite.Open();
            }
            catch(SQLiteException e)
            {
                MessageBox.Show(e.Message);
            }
        }

        /// <summary>
        /// Creates the schema for all tables nessicary
        /// </summary>
        private void createSchema()
        {
            string query = "CREATE TABLE task (id INTEGER PRIMARY KEY AUTOINCREMENT, description TEXT, begindate TEXT, enddate TEXT, status INTEGER default 0)";

            SQLiteCommand cmd = new SQLiteCommand(query, sqlite);

            cmd.ExecuteNonQuery();

        }

        /// <summary>
        /// Prepares the queries used to manipulate the timesheet
        /// </summary>
        private void prepareQueries()
        {
            adapter = new SQLiteDataAdapter("select id,description,begindate,enddate,status from task", sqlite);

            builder = new SQLiteCommandBuilder(adapter);

            adapter.UpdateCommand = builder.GetUpdateCommand();
            adapter.DeleteCommand = builder.GetDeleteCommand();
            adapter.InsertCommand = builder.GetInsertCommand();
        }

        /// <summary>
        /// Return the task table
        /// </summary>
        /// <returns></returns>
        public DataTable Tasks()
        {
            return this.datatable;
        }

        public void Update()
        {
            adapter.Update(datatable);

            datatable.Clear();

            adapter.Fill(datatable);
        }

        /// <summary>
        /// Closes the database connection
        /// </summary>
        public void close()
        {
            sqlite.Close();
        }
    }
}

You don't need to create the database file if it doesn't exist, the SQLite driver will take care of it; to ensure your table does not exist on creation, you can just add IF NOT EXISTS to the query.

I would also close the connection every time, this has already been covered by other answers. In addition I would make the class stateless as it doesn't need a state if we open/close the connection every time (Being stateless brings a lot of other advantages).

I would also remove any logic from the constructor, to do this you will need a state or you will have to check on every command unfortunately so I left it to you.

Here is my 0.02:

internal class Timesheet
{
    private const string SelectTaskQuery = "SELECT id, description, begindate, enddate, status FROM task";

    private readonly string _filePath;

    public Timesheet(string filePath)
    {
        _filePath = filePath;

        InitializeDb();
    }

    private DataTable Tasks() => ExecuteWithConnection(Tasks);

    private void InitializeDb() => ExecuteWithConnection(connection =>
    {
        using (var command = connection.CreateCommand())
            CreateSchemaIfNotExists(command);
    });

    private SQLiteConnection GetConnection() => new SQLiteConnection("Data Source=" + _filePath + ";Version=3;");

    private void ExecuteWithConnection(Action<SQLiteConnection> action)
    {
        using (var connection = GetConnection())
        {
            connection.Open();

            action(connection);
        }
    }

    private T ExecuteWithConnection<T>(Func<SQLiteConnection, T> action)
    {
        using (var connection = GetConnection())
        {
            connection.Open();

            return action(connection);
        }
    }

    private static DataTable Tasks(SQLiteConnection connection)
    {
        var adapter = new SQLiteDataAdapter(SelectTaskQuery, connection);

        var datatable = new DataTable();

        adapter.Fill(datatable);

        return datatable;
    }

    public void Update(DataTable dataTable)
    {
        ExecuteWithConnection(connection =>
        {
            var adapter = new SQLiteDataAdapter(SelectTaskQuery, connection);

            var builder = new SQLiteCommandBuilder(adapter);

            adapter.Update(dataTable);
        });
    }

    private static void CreateSchemaIfNotExists(IDbCommand command)
    {
        const string query = "CREATE TABLE IF NOT EXISTS task (id INTEGER PRIMARY KEY AUTOINCREMENT, description TEXT, begindate TEXT, enddate TEXT, status INTEGER default 0)";

        command.CommandText = query;

        command.ExecuteNonQuery();
    }
}

I would also add a small ORM like Dapper and add statically typed entities for insert/update.


Optimizing simple C++ grading program I have started working on the projects listed here.  I want to practice my C++ coding skills.  I would love any constructive criticism I can get!

This is for the very first problem, #1 "Grading Program"

#include <iostream>
using std::cout;
using std::cin;

int main()
{
    int UserNumber;
    char Grade;

    cout << "Enter the grade you got on that test: ";
    cin >> UserNumber;
    cout << "\n\n\n";

    if (UserNumber > 89 && UserNumber < 101)
    {
        Grade = 'A';
    }

    else if (UserNumber > 79 && UserNumber < 91)
    {
        Grade = 'B';
    }

    else if (UserNumber > 69 && UserNumber < 81)
    {
        Grade = 'C';
    }

    else if (UserNumber > 59 && UserNumber < 71)
    {
        Grade = 'D';
    }

    else if (UserNumber > -1 && UserNumber < 61)
    {
        Grade = 'F';
    }
    else
    {
        Grade = 'Z';
    }

    switch (Grade)
    {
    case 'A':
        cout << "Perfect! You got a: " << Grade;
        break;
    case 'B':
        cout << "Could've been worse. You got a: " << Grade;
        break;
    case 'C':
        cout << "It's okay I guess, you got a: " << Grade;
        break;
    case 'D':
        cout << "Uggh.. You got a: " << Grade;
        break;
    case 'F':
        cout << "Better luck next time! You got a: " << Grade;
        break;
    case 'Z':
        cout << "Undefined value! Make sure it's a value from 0-100!";
        break;

    }
    cout << "\n\n\n\t\t\t";
    return 0;
}

There's probably much that can be done, but I'll give it a quick review:

Are only integer grades allowed?  They can be decimals in real life, and that would make for a more robust grading system.
Consider renaming UserNumber to something like numericalGrade.  The former sounds like it's for an ID number, not a grade.
Do not "list" variables at the top of a function in C++, unless it's absolutely necessary.  Here, you can declare UserNumber between the std::cout and std::cin.
There's no need for a Z case here.  Use a default case instead, which will be reached if no other case is reached.
You don't need an explicit return 0 at the end.  The compiler will do this return for you at the end of main() as successful termination is always implied here.


Creating a pure Ruby object (PORO) to email files in a Rails application I wrote a service object that emails a list of files to a list of email addresses.

I am unsure about my emails method. It splits a string into an array of emails. It then loops over the array and rejects any elements that do not match the email regex. The return value is an array of valid emails stripped of whitespace.

Another thing I'm getting a bad feeling about is the caching of the bucket, @bucket ||= ..., and the url = get_urls temp variable in the send_email method.

class FileSharingService
  def initialize(files, params)
    @files = files
    @params = params
    @client = AWS::S3.new
  end

  def send_email
    urls = get_urls
    emails.each do |email|
      FileSharingMailer.email_files(email, params[:message], urls)
    end
  end

  def emails
    @params[:emails].split(',').reject do |email|
      email.strip!
      email !~ /\A[\w+\-.]+@[a-z\d\-.]+\.[a-z]+\z/i
    end
  end

  def get_urls
    @files.map do |file|
      bucket.objects[file.current_version.path].url_for(:get, expires: 1.week.from_now)
    end
  end

  def bucket
    @bucket ||= @client.buckets[ENV['S3_BUCKET']]
  end
end

Good job on

Tightly focused methods that do what their name says
Non-surprising formatting
Testable design

This is pretty good code

Environment variable

There's a few things that bother me about this class's use of the S3_BUCKET environment variable.

It makes unit test awkward, as the test must either set the environment variable and then restore it, or it must stub ENV#[], either of which is a minor pain.
It hard-codes the environment variable name

My first thought was to have the class take the bucket name as an argument with a default:

def initialize(files, params, bucket_name = ENV['S3_BUCKET'])

But I think that is a half measure.  Instead, let's just pass in the bucket:

def initialize(files, params, bucket)

bucket is an instance of this class, which does a lazy fetch of the AWS::S3 bucket:

require 'forwardable'
require 'memoist'

class Bucket

  extend Forwardable

  def initialize(bucket_name = ENV['S3_BUCKET'])
    @client = AWS::S3.new
    @bucket_name = bucket_name
  end

  def_delegator :bucket, :objects

  private

  def bucket
    @client.buckets[@bucket_name]
  end
  memoize :bucket

end

The memoist gem Is a fancy version of "@bucket ||= ...".
Forwardable#def_delegator delegates the #object method to the result of the #bucket method.

The use of the Bucket class has these advantages:

FileSharingService:

no longer has to know about the environment variable.
does not have to concern itself with lazy-initialization and caching.
can now be easily tested in isolation--just give it a test double for its bucket instance.

Bucket can be easily tested in isolation.

Checking email addresses

I would consider taking the regular expression in this expression:

email !~ /\A[\w+\-.]+@[a-z\d\-.]+\.[a-z]+\z/i

And, at a minimum, give it a constant.  The goal is to make the code more self-documenting:

WELL_FORMED_EMAIL_ADDRESS = /\A[\w+\-.]+@[a-z\d\-.]+\.[a-z]+\z/i
...
email !~ WELL_FORMED_EMAIL_ADDRESS

I might go one step farther and move the entire check into another class.  Make it someone else's responsibility to know what's a good email address and what's not:

EmailAddress.well_formed?(email)

params

The only use the class makes of params is of @params[:emails].  If that is unlikely to change in the future, I would consider just passing in the email addresses, and not concerning this class with params at all.

def initialize(files, email_addresses, ...)


Project Euler Attack #2 This is my implementation of Project Euler #2 in C#.  It takes 0 milliseconds to run, and I believe it is correct - I have seen nothing to indicate otherwise!

  By considering the terms in the Fibonacci sequence whose values do not exceed four million, find the sum of the even-valued terms.

static void GetFibos(ref List<int> nums, int maxVal)
{
    nums.Add(1);
    int lastNum = 2;

    while (lastNum <= maxVal)
    {
        nums.Add(lastNum);
        lastNum = nums[nums.Count - 2] + nums[nums.Count - 1];
    }
}

static void Main(string[] args)
{
    Stopwatch s = new Stopwatch();
    s.Start();

    List<int> fiboNums = new List<int>();
    GetFibos(ref fiboNums, 4000000);

    int sumEvenFibos = 0;

    foreach (int i in fiboNums) { if (i % 2 == 0) sumEvenFibos += i; }

    s.Stop();

    Console.WriteLine(sumEvenFibos);
    Console.WriteLine(s.ElapsedMilliseconds);
}

Is there anything I should be doing different?

Why do you need the actual list of Fibonacci numbers? That just requires space, and time.

Believe it or not, you have over-thought this problem. Additionally, your code style is off.

C# Code conventions always use braces for single-statement blocks, and the brace should open at the start of a new line. Code like:

foreach (int i in fiboNums) { if (i % 2 == 0) sumEvenFibos += i; }

should be:

foreach (int i in fiboNums)
{
    if (i % 2 == 0)
    {
        sumEvenFibos += i;
    }
}

As for the list, you don't need it:

int current = 1;
int previous = 0;
int sum = 0;
while (current <= 4000000)
{
    if ((current % 2) == 0)
    {
        sum += current;
    }
    int tmp = current + previous;
    previous = current;
    current = tmp;
}

Now, that whole thing should be put in its own function, and your main method with a parameter for the upper limit (4,000,000) and it becomes:

static void Main(string[] args)
{
    Stopwatch s = new Stopwatch();
    s.Start();

    int sumEvenFibos = GetFiboSum(4000000);

    s.Stop();

    Console.WriteLine(sumEvenFibos);
    Console.WriteLine(s.ElapsedMilliseconds);
}