Validating Mongo collections I figure there is a better way that I should be doing this:

if !db.collection_names.include?("reports".to_s)
  db.create_collection("reports")
end

if !db.collection_names.include?("days".to_s)
  db.create_collection("days")
end

if !db.collection_names.include?("users".to_s)
  db.create_collection("users")
end

Well, firstly: "string".to_s is entirely redundant. That's already a String, so converting it again is just wasting your (processor's) time.

Secondly, writing if blocks with just one line and no else wastes a good deal of vertical real estate. I'd recommend that instead of

if condition
  statement
end

you should write

statement if condition

Ruby has unless as the opposite to if, and when you find yourself writing if !..., it's better to use unless ... instead. This is mostly because of convention -- Ruby programmers use unless a lot more, so it's quicker to parse.

As for shortening the method calls themselves, you could just pass strict: false in with the name, so each individual call becomes more like this:

db.create_collection('name', strict: false)

Then you don't need the condition at all.

Then, since you have three objects on which you want to perform the same action, you can put them in an array:

['reports', 'days', 'users']

(or use the fancier %w{ reports days users }, like I do below, but you'll want to read up on its gotchas first)

Then we can loop through that array:

%w{ reports days users }.each do |name|
  #...
end

And in place of #... we put the code to create the table:

db.create_collection(name, strict: false)

Or, if you don't want to rely on swallowing errors:

db.create_collection(name) unless db.collection_names.include?(name)

If we put that all together, it becomes this:

%w{ reports days users }.each do |name|
  db.create_collection(name) unless db.collection_names.include?(name)
end

Now, to add a collection, all you have to do is add it to the list. For example, if you wanted to add one called "years", add years in to the %w:

%w{ reports days users years }.each do |name|
  db.create_collection(name) unless db.collection_names.include?(name)
end


Finding the nearest agents to a customer using IComparable I got a C# class:

public class Agent
{
    public string AgentId { set; get; }
    public double Longitude { set; get; }
    public double Latitude { set; get; }
}

There are several agents in several different locations. During the program's lifetime, they can receive a call to another (latitude, longitude) point which is unknown during runtime. I got an API to calculate the distance in meters between their location and the given point. What I need eventually is to find the 5 closest agents to the given point.

What I did in order to achieve this is adding another class:

public class AgentDistance : Agent, IComparable<AgentDistance>
{

    public AgentDistance(Agent agent)
    {
        if(agent == null)
        {
            throw new Exception("Can't initalize agent with distance since agent is null");
        }
        this.AgentId = agent.AgentId;
        this.Latitude = agent.Latitude;
        this.Longitude = agent.Longitude;
    }

    public double Distance { set; get; }

    public int CompareTo(AgentDistance other)
    {
        if(other == null)
        {
            return 1;
        }
        return Distance.CompareTo(other.Distance);
    }
}

And the usage:

var agents = db.GetAgents();
if(agents == null || agents.Count == 0)
{
    Console.WriteLine("No Results");
}
List<AgentDistance> agentsWithDistance = new List<AgentDistance>();
foreach(Agent agent in agents)
{
    double res = ws.GetDistanceMeters(agent.Latitude, agent.Longitude, customerLatitude, customerLongitude);
    agentsWithDistance.Add(new AgentDistance(agent) { Distance = res });
}
agentsWithDistance.Sort();
for(int i = 0; i < 5; i++)
{
    Console.WriteLine(string.Format("agent_id:  {0}  distance: {1} m", agentsWithDistance[i].AgentId, agentsWithDistance[i].Distance));
}

It works, but is there a more elegant way to do it? I'm not sure if adding another class might be a bit redundant since all it does is just adding a property for sorting, but adding the distance property to the Agent class, doesn't make so much sense.

var agents = db.GetAgents();
if(agents == null || agents.Count == 0)
{
    Console.WriteLine("No Results");
}

You should exit your application/code block when you detect an invalid state. Here even if "No Results" is printed, the app will still run into a NullReferenceException or IndexOutOfRangeException in the next few steps.  Also, talking of IndexOutOfRangeException, there is no check against if there is at least 5 agents in your db.

Instead of creating an IComparable, you can just use linq to sort directly. But, since in your case, you also need to print out the distance (the value used for sorting), we will need to create an anonymous class to hold it:

var agents = db.GetAgents();
if(agents == null || agents.Count == 0)
{
    Console.WriteLine("No Results");
    return;
}

var nearestAgents = agents
    .Select(x => new
    {
        x.AgentId,
        DistanceToCustomer = ws.GetDistanceMeters(x.Latitude, x.Longitude, customerLatitude, customerLongitude)
    })
    .OrderBy(x => x.DistanceToCustomer);
foreach (var agent in nearestAgents.Take(5))
{
    Console.WriteLine($"agent_id:  {agent.AgentId}  distance: {agent.DistanceToCustomer} m");
}

The .Take(5) ensures that only 5 agent will be printed out, or less.


Discriminated-unions in C# So I really want to have something similar to discriminated unions in C#.

One way to do it is to use a visitor pattern, but it takes half a life to write all broilerplate code by hands.
There is another way that would allow me writing a bit less code, but it is based on delegates.

Consider the analogue of Options from F#:

public abstract class Optional<TValue>
{
    public abstract TResult Resolve<TResult>(Func<TValue, TResult> someHandler, Func<String, TResult> noneHandler);
}

public class Some<TValue> : Optional<TValue>
{
    public Some(TValue value)
    {
        this.Value = value;
    }
    public TValue Value { get; private set; }
    public override TResult Resolve<TResult>(Func<TValue, TResult> someHandler, Func<String, TResult> noneHandler)
    {
        return someHandler(this.Value);
    }
}

public class None<TValue> : Optional<TValue>
{
    public None(String message)
    {
        this.Message = message;
    }
    public String Message { get; private set; }
    public override TResult Resolve<TResult>(Func<TValue, TResult> someHandler, Func<String, TResult> noneHandler)
    {
        return noneHandler(this.Message);
    }
}

public class Demo {
    public Optional<Uri> ParseAbsoluteUri(String whateverUrl)
    {
        if (Uri.IsWellFormedUriString(whateverUrl, UriKind.Absolute))
        {
            var uri = new Uri(whateverUrl, UriKind.Absolute);
            return new Some<Uri>(uri);
        }
        else
        {
            return new None<Uri>("Given URL is not absolute.");
        }
    }
    public String ProcessUrl(String whateverUrl)
    {
        var uriOpt = this.ParseAbsoluteUri(whateverUrl);
        var result = uriOpt.Resolve(uri => "Hey, your URL \"" + uri + "\" looks nice!", message => "Sorry, the URI that you gave looks bad: " + message);
        return result;
    }
    public void Test()
    {
        Trace.WriteLine(this.ProcessUrl("http://google.com"));
        Trace.WriteLine(this.ProcessUrl("Not url at all"));

        /* output:
            Hey, your URL "http://google.com/" looks nice!
            Sorry, the URI that you gave looks bad: Given URL is not absolute.
         */
    }
}

Each delegate is essentially a class that the C# compiler makes to wrap the pointer to a function. So with the excessive use of such approach will I hit any sort of problems? (Bloating the size of the assembly for example?)
Delegates aren't the fastest things in the galaxy, especially multicasts ones, this approach uses them heavily.
Making a call to a virtual method lessens (if not eliminates at all) a chance for JIT inlining.

So all in all I am a bit scared to go all the way this way due to performance concerns. I would like to know what you think of this.

This looks very well crafted. I don't know f# and I'm not really into functional programming, but I can enumerate everything I like about this code:

Type parameter names are descriptive: TValue and TResult are perfect.
The interface for Optional<TValue> is segregated; it's very focused and specialized, which makes it highly reusable... which is a nice feature for an abstract class.
Usage of this qualifier is consistent.

Well as I enumerated these points I did find a few minor nitpicks:

Usage of this qualifier is redundant.
Usage of String could be replaced with C# language alias string (but then again usage of String is beautifully consistent).

And as I enumerated these minor nitpicks I did find one tiny little thing that could be improved: the Message property in the derived types is only assigned through the constructor, using the private setter. I'd make it readonly and change the auto-property for a get-only property:

public None(String message)
{
    _message = message;
}

private readonly String _message;
public String Message { get { return _message; } }

...and this is where I'm clashing with this - I prefer prefixing private fields with an underscore and avoid this altogether, you might have it like this instead:

public None(String message)
{
    this.message = message;
}

private readonly String message;
public String Message { get { return this.message; } }


removeAll(closure) in Swift From this library I wrote, I have created this function in Swift, but I'm not happy with the implementation. Can anyone suggest a better way to do this? I really don't like changing counters inside for-loops...

extension Array {

    /// Removes all items that conform to the given closure
    mutating func removeAll(closure: (T) -> Bool) {
        for var counter = 0; counter < count; counter += 1 {
            if closure(self[counter]) {
                removeAtIndex(counter)
                counter -= 1
            }
        }
    }
}

Some general remarks:

The method name removeAll is misleading, as it does not remove
all elements from the array. I would call it for example removeMatching.
The method parameter is a closure, but acts as a predicate, so
predicate might be a better name.
The counter variable in the method is actually the array index
so you could call it index.
The method documentation can be improved (details at the end of
this answer).

Your concrete problem about modifying the loop counter inside the
loop can easily be solved by traversing the array indices in
reverse order:

for var index = count - 1; index >= 0; index-- { ... }

Using stride() allows you to have a constant loop variable:

for index in stride(from: count - 1, through: 0, by: -1) { ... }

Putting it all together:

extension Array {

    /// Removes all items that satisfy the predicate:
    mutating func removeMatching(predicate: (T) -> Bool) {
        for index in stride(from: count - 1, through: 0, by: -1) {
            if predicate(self[index]) {
                removeAtIndex(index)
            }
        }
    }
}

An alternative implementation would be

extension Array {

    /// Removes all items that satisfy the predicate:
    mutating func removeMatching(predicate: (T) -> Bool) {
        self = self.filter { !predicate($0) }
    }
}

The first method removes the matching elements, while the second method
creates a new array and then replaces self. Which one is more
efficient depends how many elements are actually removed in relation
to the array size.

Notice that in Swift 2, Element should be used in place of T.

As mentioned above, the inline method documentation can be improved
(I took this information from the SO thread Swift Documentation Comments). Example:

extension Array {
    /// Removes all items that satisfy the `predicate`
    ///
    /// :param: predicate A boolean predicate.
    mutating func removeMatching(predicate: (T) -> Bool) {
        self = self.filter { !predicate($0) }
    }
}

Now "Option-Click" will show you

Addendum: A removeAll(where:) was added to the Swift Standard library in Swift 4.2, so there is no need to define your own method for that purpose anymore.

As explained in SE-0197 this method is supposed to be both safe and efficient.


Populate list with Excel range and keep formatting I have written a Python programme that takes data from Excel sheets and adds this data to new .DAT files.
I use win32com.client because the Excel sheets will already be opened and other modules need to open a file before they can process it.

As the win32com.client outputs the range as a Component Object Model, it changes the content in some minor but destructive ways. Numbers, such as 2, are outputted as 2.0. Moreover, empty cells are outputted as None.
For this reason I cannot immediately put the cells' values into a file, and instead I use a loop to fill a list during which I edit the data.

This loop, however, is relatively very slow. I'd like a faster method, but I am unsure how to achieve this.

from win32com.client import GetObject

def convertdata(r, l):
    newr = []
    for c in r:
        # Some codes start with leading zeroes and need to be left as-is
        # The try...except clause would convert this code into integers, stripping
        # the leading zeroes. str(c) == '0.0' makes sure single 0 values do get
        # processed.
        if str(c) == '0.0' or not str(c)[:1] == '0':
            try:
                float(str(c))
                newr.append(str(int(c)))
            except ValueError:
                if str(c) == 'None':
                    newr.append('')
                else:
                    newr.append(str(c))
        else:
            newr.append(str(c))
        if c.Column == l and not c.Row - 1 == r.Rows.Count:
            newr.append('\n')
        else:
            newr.append('\t')
    return newr

# ... Code omitted
exl = GetObject(None, "Excel.Application")
exl_wbs = exl.Workbooks
sht = exlwbs[0].Worksheets(2)
rng = sht.Range(sht.Cells(2, 1), sht.Cells(lrow, lcol))
newrng = convertdata(rng, lcol)
dataf = ''.join(newrng)

with open(fpath, "w") as newfile:
    newfile.write(dataf)

lrow and lcol are integers. fpath is a string to the new file. sht is a Worksheet object.

Input example

Output example (not the same data as the input, but you get the gist of it)

I   08.5070     Plate assembly  Plate assembly  5   5070    VIRTUAL 1   1   1               0
I   0070_01.01      Plate D10 (SA)  Plate D10 (SA)  08.58070    101 VIRTUAL 1   1   1               0
I   001170      Support Support 6   1170    VIRTUAL 1   1   1               0
I   0010.1170_01.01     conveyor (SA)   conveyor (SA)   0090.1170   101 VIRTUAL

variable names

1-letter variable names are difficult to understand. Correct names for variables, functions and classes goes already a long way to comment the intent of the code.
r is a data range, c is a cell, l is the last row in the range, then call em thus

functions

I would extract the code that is needed to parse 1 cell:

def convert_cell(data):
    """converts a single cell"""
    data_string = str(data)
    if data_string == "0.0":
        return "0"
    if data_string[0] == 0:
        return data_string
    if data_string == "None":
        return ""
    try:
        float(data_string)
        return str(int(data))
    except ValueError:
        return data_string

The advantage is that you cal skip some levels of nesting, and it is really clear what each type of input get transformed to. I also refactored the multiple calls to str(c) to a variable

generator

Instead of starting a list, and appending to it, you can better yield the results.

def convertdata(data_range, last_column):
    for cell in data_range:
        yield convert_cell(cell)
        is_last_column = cell.Column == last_column
        is_last_row = cell.Row - 1 == data_range.Rows.Count
        yield "\n" if is_last_column and not is_last_row else "\t"

data_range = sheet.Range(sheet.Cells(2, 1), sheet.Cells(lrow, lcol))
dataf = ''.join(convertdata(data_range, lcol))


MultiDictionary implementation C# Because C# .NET does not come with a MultiDictionary, I implemented one based on this StackOverflow answer.

The MultiDictionary should be able to hold multiple values for one key and remove the key, if no value is assigned to the key anymore.

I wanted to implement nearly all methods, which the standard Dictionary provides:

void Add(TKey key, TValue value);
bool Remove(TKey key);
void Clear();
bool ContainsKey(TKey key);
bool TryGetValue(TKey key, out TValue value);

To use the MultiDictionary properly some signatures needed to be changed. This is my implementation:

public class MultiDictionary<TKey, TValue>
{
    private readonly Dictionary<TKey, List<TValue>> _Data = new Dictionary<TKey, List<TValue>>();

    public Dictionary<TKey, List<TValue>>.ValueCollection Values => _Data.Values;
    public Dictionary<TKey, List<TValue>>.KeyCollection Keys => _Data.Keys;
    public void Add(TKey key, TValue value) { this[key].Add(value); }
    public bool Remove(TKey key, TValue value) { return this[key].Remove(value); }
    public void Clear() { _Data.Clear(); }
    public bool ContainsKey(TKey key) { return _Data.ContainsKey(key); }
    public bool TryGetValue(TKey key, out List<TValue> value) { return _Data.TryGetValue(key, out value); }

    private struct Entry : IEnumerable<TValue>
    {
        private readonly MultiDictionary<TKey, TValue> _Dictionary;
        private TKey Key { get; }

        internal void Add(TValue value)
        {
            if (!_Dictionary._Data.TryGetValue(Key, out var list))
                list = new List<TValue>();
            list.Add(value);
            _Dictionary._Data[Key] = list;
        }

        internal bool Remove(TValue value)
        {
            if (!_Dictionary._Data.TryGetValue(Key, out var list))
                return false;
            var result = list.Remove(value);
            if (list.Count == 0)
                _Dictionary._Data.Remove(Key);
            return result;
        }

        internal Entry(MultiDictionary<TKey, TValue> dictionary, TKey key)
        {
            _Dictionary = dictionary;
            Key        = key;
        }

        public IEnumerator<TValue> GetEnumerator()
        {
            return !_Dictionary._Data.TryGetValue(Key, out var list) ? Enumerable.Empty<TValue>().GetEnumerator() : list.GetEnumerator();
        }
        IEnumerator IEnumerable.GetEnumerator() { return GetEnumerator(); }
    }

    private Entry this[TKey key] => new Entry(this, key);
}

I wanted to know, if this implementation is valid (working as I expect) and if you have improvements regarding coding style, safety and so on.

Thanks in Advance!

couple of points from my side:

If you add same TKey instance to your List it will be added only
once. But if you add other instance of TKey but with the same
'content' the end result of Add method is dictionary with two keys with same content and two
lists. Is that part of requirement?

Example:

public class Foo
{
    public string X;
    public int Y;

    public Foo(string x, int y)
    {
        X = x;
        Y = y;
    }
}

var multiDict = new MultiDict<Foo, string>();

var firstFoo = new Foo("abc", 1);
multiDict.Add(firstFoo, "someValue1");
multiDict.Add(firstFoo, "someValue2");

var secondFoo = new Foo("abc", 1);
multiDict.Add(secondFoo, "someValue3");

Similar problem is for TValue, but it's related with removing elements. Solution for both cases is implementing IEqualityComparer for both TKey and TValue and force class user to inject it via constructor and use [THIS] dictonary constructor. That will guarantee uniqueness for your key. Please keep in mind that you should also use RemoveAll method on list to be sure that all elements that matches predicate are removed.
You're incosistent with indentations of methods, curly brackets,
whitespaces between methods. Please clean that up.
I don't see the point of Entry struct. You can easily get rid of it
and put all methods in MultiDictionary class itself.
I would consider to change interface to more 'detailed' interface.
What I mean by that? Let's say that Remove returns count of elements
actually removed. Point only to consideration.
I will change implementation of Add method to something different,
because you always (when key exists in dictionary or not) seeking
through dictionary twice: first time is on TryGetValue method and
second while attaching list to key. Attaching list to key is
necessary only when you're creating new one.


Color substitution in a BufferedImage The setPixelColor function below changes the color of pixels.
I need some suggestions to optimize this function.

Example:

public static void main(String[] args) throws IOException {
    BufferedImage pic1 = ImageIO.read(new File("Images/Input-1.bmp"));
    setPixelColor(pic1, 34, 177, 76, 127, 127, 127);
}

This changes the left image into the right image.

 private static void setPixelColor(BufferedImage imgBuf, int red, int green, int blue, int newRed, int newGreen, int newBlue) throws IOException {
    int[] RGBarray;
    int w;
    int h;

    //Declare color arrays
    int[][] alphaPixels;
    int[][] redPixels;
    int[][] greenPixels;
    int[][] bluePixels;

    w = imgBuf.getWidth();
    h = imgBuf.getHeight();

    alphaPixels = new int[h][w];
    redPixels = new int[h][w];
    greenPixels = new int[h][w];
    bluePixels = new int[h][w];

    RGBarray = imgBuf.getRGB(0, 0, w, h, null, 0, w);

    //Bit shift values into arrays
    int i = 0;
    for (int row = 0; row < h; row++) {
        for (int col = 0; col < w; col++) {
            alphaPixels[row][col] = ((RGBarray[i] >> 24) & 0xff);
            redPixels[row][col] = ((RGBarray[i] >> 16) & 0xff);
            greenPixels[row][col] = ((RGBarray[i] >> 8) & 0xff);
            bluePixels[row][col] = (RGBarray[i] & 0xff);
            i++;
        }
    }

    //Set the values back to integers using re-bit shifting
    for (int row = 0; row < h; row++) {
        for (int col = 0; col < w; col++) {
            if (redPixels[row][col] == red && greenPixels[row][col] == green && bluePixels[row][col] == blue) {
                int rgb = (alphaPixels[row][col] & 0xff) << 24 |
                        (redPixels[row][col] & newRed) << 16 |
                        (greenPixels[row][col] & newGreen) << 8 |
                        (bluePixels[row][col] & newBlue);
                imgBuf.setRGB(col, row, rgb);
            }
        }
    }

    //Write back image
    ImageIO.write(imgBuf, "bmp", new File("Images/Output2.bmp"));
}

I think that changeColor or substColor would be a more appropriate name for this function.

I don't see why the function should write out its result to a file.  That's a violation of the Single Responsibility Principle.  What if I want to perform multiple color substitutions before writing out the result?  What if I want a different output filename, or a format other than BMP?

Avoid empty declarations, when you can declare and initialize values at the same time.  It's more readable and less error-prone.

You don't care about the x-y coordinates of each pixel — only the color matters.  So, there is no need to construct the 2-D arrays representing the red, green, and blue channels.

/**
 * Changes all pixels of an old color into a new color, preserving the
 * alpha channel.
 */
private static void changeColor(
        BufferedImage imgBuf,
        int oldRed, int oldGreen, int oldBlue,
        int newRed, int newGreen, int newBlue) {

    int RGB_MASK = 0x00ffffff;
    int ALPHA_MASK = 0xff000000;

    int oldRGB = oldRed << 16 | oldGreen << 8 | oldBlue;
    int toggleRGB = oldRGB ^ (newRed << 16 | newGreen << 8 | newBlue);

    int w = imgBuf.getWidth();
    int h = imgBuf.getHeight();

    int[] rgb = imgBuf.getRGB(0, 0, w, h, null, 0, w);
    for (int i = 0; i < rgb.length; i++) {
        if ((rgb[i] & RGB_MASK) == oldRGB) {
            rgb[i] ^= toggleRGB;
        }
    }
    imgBuf.setRGB(0, 0, w, h, rgb, 0, w);
}

public static void main(String[] args) throws IOException {
    BufferedImage pic1 = ImageIO.read(new File(…));
    changeColor(pic1, 34, 177, 76, 127, 127, 127);
    ImageIO.write(pic1, "bmp", new File(…));
}


A program to represent a coin amount using the smallest number of coins Recently, I was learning about greedy algorithms. One example that was given was an algorithm for exchanging a given money amount using the smallest number of coins. After the lecture, I wrote a C program implementing this:

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

static int compareDenominations(const void *a, const void *b) {
    return (*((int*) b) - *((int*) a));
}

int main(int argc, char *argv[]) {
    long amount;
    int count;
    long *denominations;
    if (argc > 2) {
        errno = 0;
        amount = strtol(argv[1], NULL, 10);
        if (errno || amount <= 0) {
            puts("The coin amount is either negative, zero, or too big.");
            return EXIT_FAILURE;
        }
        count = argc - 2;
        if (count == 0) {
            puts("Please input at least one coin denomination.");
            return EXIT_FAILURE;
        }
        denominations = malloc(sizeof(*denominations) * count);
        if (denominations == NULL) {
            puts("Insufficient memory.");
            return EXIT_FAILURE;
        }
        int i;
        for (i = 0; i < count; i++) {
            errno = 0;
            denominations[i] = strtol(argv[i + 2], NULL, 10);
            if (errno || denominations[i] <= 0) {
                puts("A denomination amount is either negative, zero, or too big.");
                return EXIT_FAILURE;
            } else {
                int j;
                for (j = 0; j < i; j++) {
                    if (denominations[j] == denominations[i]) {
                        fprintf(stderr, "%ld is a duplicate coin denomination.\n", denominations[j]);
                        return EXIT_FAILURE;
                    }
                }
            }
        }
    } else {
        puts("Please enter a coin amount followed by a non-zero number of unique denominations, all separated by spaces.");
        return EXIT_FAILURE;
    }
    heapsort(denominations, count, sizeof(*denominations), compareDenominations);
    printf("Amount: %ld | Denominations: [ ", amount);
    int i;
    for (i = 0; i < count; i++) {
        printf("%ld ", denominations[i]);
    }
    puts("]");
    int denominationCount = 0;
    for (i = 0; i < count; i++) {
        denominationCount = 0;
        while (amount >= denominations[i]) {
            amount -= denominations[i];
            denominationCount++;
        }
        denominations[i] = denominationCount;
    }
    if (amount) {
        puts("The coin amount could not be represented using the given denominations.");
        return EXIT_FAILURE;
    }
    printf("Counts: [ ");
    for (i = 0; i < count; i++) {
        printf("%ld ", denominations[i]);
    }
    puts("]");
    return EXIT_SUCCESS;
}

Because my program can accept arbitrary inputs, this greedy algorithm may not always output the most optimal solution, but I am still interested in the quality of my implementation.

Don't invent error messages. If strtol sets errno, use perror() to print a precise reason for a failure.
Flat is better than nested. I recommend to handle argc < 2 at the beginning, to remove one level of indentations of the main algorithm. Along the same line, there is no need to else once if branch does return.
Testing for duplicated denomination is easier after they are sorted.
while (amount >= denominations[i]) loop is a long way to say

denominationCount = amount / denomination[i];
amount %= denomination[i];

You may want to break the for (i = 0; i < count; i++) early, as soon as amount reaches 0.
Qudos for sizeof(*denominations).
The algorithm may fail to find a solution.


Strategy pattern instances based on enums I have a view which takes a DataPresenterStrategy instance to retrieve the proper text out of some data:

public interface DataPresenterStrategy {

    String getDisplayString(Data data);

    String getUnit();
}

public class DistanceStrategy implements DataPresenterStrategy {

    @Override
    public String getDisplayString(Data data) {
        return data.getDistance();
    }

    @Override
    public String getUnit() {
         return "m";
    }
}

public class PowerStrategy implements DataPresenterStrategy {

    @Override
    public String getDisplayString(Data data) {
        return data.getPower();
    }

    @Override
    public String getUnit() {
         return "W";
    }
}

Based on some enum, I know which DataPresenterStrategy to create:

enum DataPresenterStrategyType { DISTANCE, POWER }

DataPresenterStrategy toStrategy(StrategyType type) {
    switch(type) {
        case DISTANCE:
            return new DistanceStrategy();
        case POWER:
            return new PowerStrategy();
    }
}

Obviously, this is simplified. In my actual code I have 25 of those Strategy classes.
Is there a better way to do this DataPresenterStrategy instance creation?

Usage in views:

public interface DataPresenter {
    void setDataPresenterStrategy(DataPresenterStrategy strategy);
}

public class DataView implements DataPresenter {

    private DataPresenterStrategy mDataPresenterStrategy;

    @Override
    public void setDataPresenterStrategy(DataPresenterStrategy strategy) {
        mDataPresenterStrategy = strategy;
    }

}

public class MyApplication {

    private DataPresenter[] mDataPresenters;

    public void setDataPresenterStrategy(int dataPresenterIndex, DataPresenterStrategyType type) {
        switch (type)
          /* (...) */
    }

}

setDataPresenterStrategy(int, DataPresenterStrategyType) is called when a user selects an item from a spinner:

class MyOnItemSelectedListener implements OnItemSelectedListener {

    private int mPresenterIndex;

    public OnItemSelectedListener(int presenterIndex) {
        mPresenterIndex = presenterIndex;
    }

    void onItemSelected(int which) {
         setDataPresenterStrategy(mPresenterIndex, DataPresenterStrategyType.values()[which];
    }

}

Another way to do the instance creation would be to give each type a constructor reference for the strategy it corresponds to.

enum DataPresenterStrategyType {
    DISTANCE(DistanceStrategy::new), POWER(PowerStrategy::new);
    private final Supplier<DataPresenterStrategy> constructor;

    DataPresenterStrategyType(Supplier<DataPresenterStrategy> constructor){
        this.constructor = constructor;
    }

    DataPresenterStrategy newStrategy() {
        return constructor.get();
    }
}

The main advantages I see for this way are the compile time safety that each type has a Supplier and that you don't need to look for all the switch statements in your project to add cases if you add a new strategy type.

If not all of the constructors of your strategies take the same arguments you can replace the constructor reference with a lambda.

If you don't have access to Java 8, you can accomplish the same thing more verbosely with an anonymous class instead of the constructor reference, or you could make newStrategy() an abstract method and implement it in each type.


Design guideline for saving big byte stream in c# I have an application where I am receiving big byte array very fast around per 50 miliseconds.

The byte array contains some information like file name etc. The data (byte array ) may come from several sources.

Each time I receive the data, I have to find the file name and save the data to that file name.

I need some guide lines to how should I design it so that it works efficient.

Following is my code...

public class DataSaver
{
    private Dictionary<string, FileStream> _dictFileStream;

    public void SaveData(byte[] byteArray)
    {
        string fileName = GetFileNameFromArray(byteArray);
        FileStream fs = GetFileStream(fileName);
        fs.Write(byteArray, 0, byteArray.Length);
    }

    private FileStream GetFileStream(string fileName)
    {
        FileStream fs;
        bool hasStream = _dictFileStream.TryGetValue(fileName, out fs);
        if (!hasStream)
        {
            fs = new FileStream(fileName, FileMode.Append);
            _dictFileStream.Add(fileName, fs);
        }
        return fs;
    }

    public void CloseSaver()
    {
        foreach (var key in _dictFileStream.Keys)
        {
            _dictFileStream[key].Close();
        }
    }
}

How can I improve this code ? I need to create a thread maybe to do the saving.

You need to ask yourself 4 questions to start with:

How "big" is "big byte array"?
How fast is "very fast" ? Can we take 20 Hz as a design assumption?
Should data be appended in-order?
What is the expected overall size for a given file?

Just for the taste of it, appending 4MB to a file, on my 10 YO machine, takes less than 10 ms. A modern SSD and CPU will outdo this by an order of magnitude.

Anyway, knowing nothing, I would start with the most simple imaginable scheme, which is completely synchronous, and let it fail:

public static class FileSaver
{
    public static bool TrySaveData(byte[] data)
    {
        int dataOffset = 0;
        var file = GetFileName(data, out dataOffset);
        return TryAppend(file, data, dataOffset);
    }

    private static string GetFileName(byte[] data, out int offset)
    {
        string res = ...   // get the file name
        offset = ...       // depends on your format
        return res;
    }

    private static bool TryAppend(string file, byte[] data, int offset = 0)
    {
        try
        {
            using (var stream = new FileStream(file, FileMode.Append))
            {
                stream.Write(data, offset, data.Length - offset);
            }
            return true;
        }
        catch (Exception ex) { /* log some error */ }
        return false;
    }
}

Why is this good? Because by the time it fails, you'll be smart enough to answer 1-4, and then we would design something else based on real understanding of the problem.


Improving performance while sorting a JavaScript Object I am sorting an array (filtered) containing JS-Objects by their attribute sortdist:

filtered.sort(function (a, b) {
  if (!a.sortdist) { return 1;}
  else if (!b.sortdist) { return -1;}
  else if (a.sortdist > b.sortdist) { return 1;}
  else { return -1; }
});

However, I am sorting 9000 datasets, which takes a really long time. Is there any way to improve the performance of my sorting algorithm?

Disclaimer: I don't know anything about javascript

If you have a construct like

if (condition){
   return someValue;
} else {
   return someOtherValue;
}

the else can be omited, because if the first condition is true the else is never reached.

So your code could be refactored to

filtered.sort(function (a, b) {
  if (!b.sortdist) { return -1; }
  if (!a.sortdist || a.sortdist > b.sortdist) { return 1; }
  return -1;
});

which doesn't improve the performance, but I don't see anything to make your code faster.


Split CSV by Repeated cells python Any time that a row ID (oddly placed in column 8, i.e. row[7]) is repeated after the first instance, I want to write those rows into a second file.  The code I'm using is extremely slow -- it's a 40-column CSV with about a million rows.  This is what I have:

def in_out_gorbsplit(inf, outf1, outf2):
    outf1 = csv.writer(open(outf1, 'wb'), delimiter=',', lineterminator='\n')
    outf2 = csv.writer(open(outf2, 'wb'), delimiter=',', lineterminator='\n')
    inf1 = csv.reader(open(inf, 'rbU'), delimiter=',')
    inf1.next()
    checklist = []
    for row in inf1:
        id_num = str(row[7])
        if id_num not in checklist:
            outf1.writerow(row)
            checklist.append(id_num)
        else:
            outf2.writerow(row)

Since checklist is a list, a "not in" operation has to iterate over all elements to give the correct answer. In other words, it has a complexity of \$O(n)\$. Use a set() instead, to lower the complexity of the operation to \$O(1)\$, making it much faster.

Also don't forget to close open file handles.


Check for idle status in meteor by using 'setInterval' I'm trying to check in my meteor app for a defined timeout (idle user) to show a modal. But the way I did it seems a bit too complicated to me.

I'm doing a setInterval for 1 minute. Then a counter - which is stored in a Session var (maybe a local reactive var would be smarter?) - will be incremented by one. If there is any event (mousemove, keypress and so on), the counter will be reset.

Template.example.onRendered(function() {
    Session.set('idleTime', 0);
    //Increment the idle time counter every minute.
    var idleInterval = setInterval(timerIncrement, 60000);

    function timerIncrement() {
        idleTime = Session.get('idleTime');
        idleTime = idleTime + 1;
        Session.set('idleTime', Session.get('idleTime'));

        if (idleTime > 14) {
            // show modal after 15 minutes
        }
    }
});

Template.example.events({
    'mousemove, mousedown, touchstart, keypress': function() {
        Session.set('idleTime', 0);
    }
});

I think interval is overkill why not something like this:

var timer;
Template.example.onRendered(function() {
  timer = setTimeout(showModal, 60000* 15);
});

Template.example.events({
    'mousemove, mousedown, touchstart, keypress': function() {
       clearTimeout(timer);
       timer = setTimeout(showModal, 60000* 15);
    }
});
showModal = function(){};

Edit:

You just want to wait for 15 minutes so set a time out for that time and eventually reset timer. Create a timer variable that will hold the timer id so you can reset (delete&recreate) when a user do something.


Setting Sass environment variables in Grunt I am into building functional prototypes, but also for outputting CSS and JS, which means we have 2 times we build out our code, once as a demo with HTML files (we are using assemble.io) and another, where we are building out the files to be uploaded to production (an Enterprise CMS).

Most things about the build are the same, but the main difference is certain paths within the CSS need to change. I have set up a sass variable, and when building, I change it and then change it back when I'm done (I do it in place, because other tasks are moving the sass files around.)

The problem is this method just feels really wonky, and not at all extensible.

We are using a gruntfile that is mostly the same as the one provided with the yeoman webapp generator, with some of our specific tasks added for good measure.

Here are excerpts of the relevant sections of my Gruntfile:

  grunt.initConfig({
    //...

    // Copies remaining files to places other tasks can use
    copy: {
      dist: {
        files: [{
          expand: true,
          dot: true,
          cwd: '<%= config.app %>',
          dest: '<%= config.dist %>',
          src: [
            '*.{ico,png,txt}',
            'images/{,*/}*.webp',
            '{,*/}*.html',
            'fonts/{,*/}*.*',
            'styles/fonts/{,*/}*.*'
          ]
        }, {
          src: 'node_modules/apache-server-configs/dist/.htaccess',
          dest: '<%= config.dist %>/.htaccess'
        }, {
          expand: true,
          dot: true,
          cwd: '<%= config.app %>/components/bootstrap-sass-official/assets/fonts/bootstrap',
          src: '*',
          dest: '<%= config.dist %>/fonts'
        }, {
          expand: true,
          dot: true,
          cwd: '<%= config.app %>/components/font-awesome/fonts/',
          src: '*',
          dest: '<%= config.dist %>/fonts'
        }]
      },
      styles: {
        expand: true,
        dot: true,
        cwd: '<%= config.app %>/styles',
        dest: '.tmp/styles/',
        src: '{,*/}*.css'
      }
    },

    // Run some tasks in parallel to speed up build process
    concurrent: {
      server: [
        'sass:server',
        'assemble'
      ],
      test: [
        'copy:styles'
      ],
      dist: [
        'sass',
        'copy:styles',
        'assemble',
        'imagemin',
        'svgmin'
      ]
    },
    replace: {
      prodSass: {
        src: ['<%= config.app %>/styles/utils/_variables.scss'],
        overwrite: true,
        replacements: [
          {
            from: '$site-root: $build-root',
            to: '$site-root: $prod-root'
          }
        ]
      },
      prodSassRevert: {
        src: ['<%= config.app %>/styles/utils/_variables.scss'],
        overwrite: true,
        replacements: [
          {
            from: '$site-root: $prod-root',
            to: '$site-root: $build-root'
          }
        ]
      }
    }
    //..
  });

  grunt.registerTask(
    'build',
    'Build out the codez! takes an optional `env` parameter so we can build in different ways',
    function(env){
      grunt.task.run(
        'clean:dist',
        'handlebars',
        'assemble',
        'wiredep'
      );

      // if 'prod' environment we need to do some variable modification
      // before the sass is compiled (which happens in 'concurrent')
      if (env === 'prod'){
        grunt.task.run('replace:prodSass');
      }

      grunt.task.run(
        'concurrent:dist',
        'preprocess:html',
        'useminPrepare',
        'autoprefixer',
        'concat',
        'cssmin',
        'pixrem:dist',
        'uglify',
        'copy:dist',
        'modernizr',
        'usemin'
      );

      if (env === 'prod'){
        grunt.task.run('replace:prodSassRevert');
      }
    }
  );

Here is a trick I used a few weeks back when I ran into the same issue as you are (were?) having. Although I've used grunt-contrib-sass I think the trick should translate quite well to grunt-sass. What I basically did was to define two environment files with the same name and placed them in different folders. Next I configured Compass through Grunt to use different include paths (pointing to the two different folders) based on the build target. My scss file setup was pretty much as follows :

sass/
  dev/
    _env.scss
  dist/
    _env.scss

  component/
    _stuff.scss

  main.scss

and my main.scss file opened with the line

@import "env";

// use env variables from here on

Next, in my case, I defined two compass tasks in my Gruntfile which, based on environment, would either add sass/dev or sass/dist to the include path, e.g. :

compass : {
  options : {
    sassDir : '<%= project.app %>/sass',
    // ... and other options
  },
  dev : {
    options : {
      importPath : ['<%=project.app%>/sass/dev']
    }
  },
  dist : {
    options : {
      importPath : ['<%=project.app%>/sass/dist']
    }
  }
}

Now by executing either compass:dist or compass:dev tasks the include paths differ which in turn result in the inclusion of the desired set of environment variables. For grunt-sass the same thing should be possible by setting the includePaths property (rather than importPath in grunt-contrib-sass). Hope that helps!


Speed up counting sort algorithm in Ruby I'm attempting this simple HackerRank counting sort problem but testcase #3, which has 100,000 test cases, is timing out.

Is there something particularly inefficient about this code?

input = []
$stdin.each do |line|
  input << line
end
input.shift

arr = input.map { |line| line.split(' ')[0].to_i }

answer = (0..99).map do |i|
  counter = 0
  arr.uniq.each do |j|
    counter += arr.count(j) if j <= i
  end
  counter
end

puts answer.join ' '

arr.count(j) scans the array, so it takes linear time. Doing it once for each element of arr.uniq takes quadratic time. Doing it 100 times doesn't help either. :)

The arr.uniq.each loop is unnecessary, though (as is the call to uniq). The problem is to count the entries less than i, and you can express that directly:

counter = arr.count {|n| n <= i}

This takes linear time, so it should be much faster.

For another speedup, you can solve the whole problem with only one pass over the input, not 100. As the problem description hints, you'll need an auxiliary array to store the counts.

By the way, the first four lines can be replaced by one, using one of Ruby's convenient utilities:

input = $stdin.readlines

The whole program can be simplified to two lines.


Creating a RegExp in JavaScript from user input My goal is to accept user input regex in the following forms

/regex|here/i
/regex|here/
regex|here

From there I want to be able to pass that to a function and return a RexExp object for use later with string.match()

I came up with the below to achieve that, and not break the caller which is expecting the RegExp object returned.  Is there any part of this that I'm doing wrong or exceedingly sub-optimally?  (I'm not very familiar with Javascript).

function regGen(uInput)
{
    var inReg = "";
    var inMod = "";
    var outReg = /(?!)/

    if(uInput[0] == "/" && uInput[uInput.length-1] == "/")
    {
        inReg = uInput.slice(1,uInput.length-1);
    }
    else if(uInput[0] == "/")
    {
        var temp = uInput.split("/");
        inMod = temp[temp.length-1];
        inReg = temp;
            inReg.pop();
            inReg.shift();
        inReg = inReg.join("/");
    }
    else
    {
        inReg = uInput;
    }
    try {
        outReg = new RegExp(inReg,inMod);
    }
    catch(err)
    {
        alert("Malformed Regex");
    }
    return outReg;
}

The else if block was pretty hard to follow.  I would say that your function has rather weird behaviour (treating the text as flags) if the input has a leading slash but no trailing slash.

Anyway, even if you are not very familiar with JavaScript, surely you are familiar with regular expressions?  Why not just use a regular expression to parse your regular expression?

function regGen(uInput) {
    var regExpParser = new RegExp("^/(.*)/(.*)|(.*)");
    var match = uInput.match(regExpParser);
    return new RegExp(match[1] || match[3], match[2]);
}

Strictly speaking, we should both be more careful about interpreting possible backslashes in the input, but that is rather tricky to do correctly.

I've taken out the alert(); a malformed expression would probably be better handled by the caller.


ES6 block-scope variable usage I am experimenting with ES6 using BabelJS in a codepen project you can find here. I am confused as to why my code breaks when I decide to use let instead of var in the following function:

function getUnits(country) {
const imperialCountries = ['US', 'BS', 'BZ', 'KY', 'PW'];

if (imperialCountries.indexOf(country) === -1) {
  var units = 'metric';
} else {
  units = 'imperial';
}
console.log(country, units);
return units;
}

Thank you!

let and const allows us to declare block scoped variable. In your code

if (imperialCountries.indexOf(country) === -1) {
  var units = 'metric';
} else {
  units = 'imperial';
}

when you replace var with let the variable units becomes a block scoped variable and is recognized only inside the if block. You can make the following change in your code to use let:

let units;
if (imperialCountries.indexOf(country) === -1) {
  units = 'metric';
} else {
  units = 'imperial';
}


Removing checks that are only necessary on first pass I've got a function that's executed in a game loop. There are a couple of checks that are really only necessary during the first pass of the game loop. How can I remove these? There is more code like this, but this is the shortest example.

public void Update(Frame rawBodyData)
{
    //Catch the first pass, where RawBody and FilteredBody are null
    //I'd like to remove this check
    if (RawBody.IsNull())
    {
        RawBody = rawBodyData;
    }
    //And this check
    if (FilteredBody.IsNull())
    {
        FilteredBody = rawBodyData;
    }

    _partiallyFilteredBody = rawBodyData;

    foreach (var filter in Filters)
    {
        //Check is a certain flag is set
        if ((_filterMethods & filter.Key) != 0)
        {
            _partiallyFilteredBody = filter.Value.Filter(_partiallyFilteredBody, FilteredBody);
        }
    }

    FilteredBody = _partiallyFilteredBody;
    RawBody = rawBodyData;
}

Other comments are certainly welcome. This is supposed to be high performance code, so I'd like to avoid things like LINQ (which ReSharper is already suggesting).

You can remove the check for RawBody.IsNull() because you don't read the value of RawBody inside this method. If at the other places where some similiar code is called the rawBody is read, I would suggest to extract this check to a separate method and setting a flag which indicates that the check is already done. Like

private bool _bodyValueIsAssigned = false;
private void AssignValueIfNotAssigned(Frame frame)
{
    if (_bodyValueIsAssigned) { return; }

    if (RawBody.IsNull())
    {
        RawBody = frame;
    }

    if (FilteredBody.IsNull())
    {
        FilteredBody = frame;
    }

    _bodyValueIsAssigned  = true;
}

This can be called for the point where you call the Update() method. Without seeing more about how this is called (from an event or something similiar) there can't be said more.

If Filters is a List<T> or an array you should use performance wise a for loop instead of a foreach. See: https://stackoverflow.com/a/365658


Counting the number of lines in a file I'm easing back into C after several months of using Python and Matlab. Can anyone give me some pointers on how to improve my code's efficiency and readability?

#include <stdio.h>

#define MAX_ARG 2

int count( char FNAME[] );

int main( int argc, char *argv[]) {
    if( argc > MAX_ARG ) {
        printf("Too many arguments ( %d ) supplied.\n", argc);
        return 0;
    }
    int FILE_LENGTH = 0;
    FILE_LENGTH = count( argv[1] );
    printf("The length of %s is: %d lines.\n", argv[1], FILE_LENGTH);
    return 0;
}

int count( char FNAME[] ) {
    FILE *fp;
    int CHR;
    int LINE_COUNT = 0;
    fp = fopen( FNAME,"r" );
    while( ( CHR = fgetc( fp ) ) != EOF ) {
        if( CHR == '\n' ) {
             LINE_COUNT++;
        }
    }
    fclose( fp );
    return LINE_COUNT;
}

The code starts off very well, with a check to make sure there are not too many arguments, but it doesn't do enough error checking. There could be too few arguments as well (no file name on the command line). Errors in the execution of the file should return a non-zero number and zero for success. Try using the constants EXIT_SUCCESS and EXIT_FAILURE that are defined in stdlib.h. There probably should be an error check to see if fopen() returns a valid file pointer or not.

Generally in C variables are named in lower case, constants and macros are named in all upper case. The constant MAX_ARG follows this standard, but the variables LINE_COUNT, FNAME, FILE_LENGTH and CHR are all opposite to the standard.

The function fgets() would be very useful to the program, and possibly enhance performance. fgets() retrieves a line of text from a file if the buffer supplied is big enough.

You could also input a sector at a time as @Maybe_Factor suggests, although it would probably be better to start testing with 4K, then 8K and up to 64K based on the operating systems blocking factor.


Restaurant Menu System I wrote a basic menu application for my intro to python. I'm fairly new, and python does look promising.

the_burger = 16.99;
french_fries = 5.99;
currie_sauce = 19.99;
napkins_with_chocolates = 10.50;
juice_box = 89.01;
takeout = 18.99;
total = 0.0;
DONE = False
print("""
+-------------------------------------------+
| The Restaurant at the End of the Universe |
+---------------------------------+---------+
| A\tThe "Big Boy" Burger      | $""" + str(the_burger) + """  |
+---------------------------------+---------+
| B\tFrench Fries              | $""" + str(french_fries) + """   |
+---------------------------------+---------+
| C\tCurrie sauce              | $""" + str(currie_sauce) + """  |
+---------------------------------+---------+
| D\tNapkins with Chocolates   | $""" + str(napkins_with_chocolates) + str(0) + """  |
+---------------------------------+---------+
| E\tJuice Box                 | $""" + str(juice_box) + """  |
+---------------------------------+---------+
| F\tTakeout                   | $""" + str(takeout) + """  |
+---------------------------------+---------+
""");
while(not DONE):
  print("Total:", total);
  Item = input("Select a letter or 'done': ");
  if Item is "A":
    total += the_burger;
  elif Item is "B":
    total += french_fries;
  elif Item is "C":
    total += currie_sauce;
  elif Item is "D":
    total += napkins_with_chocolates;
  elif Item is "E":
    total += juice_box;
  elif Item is "F":
    total += takeout;
  elif Item is "done":
    print("Final total:", total);
    DONE = True

Flag variables suck, and should be avoided.  Moreover, variables should not be named in ALL_CAPS to look like constants.  All you need to get out of the loop is a break.

You've hard-coded the parts of the menu in three places:

the prices
the ASCII table
the loop

All of the menu information should be defined in one place.  You can programmatically generate the ASCII table using the astropy.io.ascii package, but I've put together a quick-and-dirty implementation below.

The if statements in the loop should be replaced by a dictionary lookup.  Furthermore, is is the wrong operator to use; string comparison should be done using ==.  In fact, entering "done" doesn't correctly end the loop, because of that.

You used + str(0) as a hack to get a price ending in "0" to display properly.  To represent fixed-point numbers, you should use a Decimal instead.

This program is long enough that it would be a good idea to make a main() function.

Statements should generally not be terminated with semicolons in Python.  Also, PEP 8, the official style guide, specifies that indentation should be four spaces.  This is an important convention in Python, where indentation matters a lot.

Suggested solution

from collections import OrderedDict, namedtuple
from decimal import Decimal
from string import ascii_uppercase

def tabular(table, widths):
    def sandwich(delim, contents):
        return delim + delim.join(contents) + delim
    def cell(value, width):
        return ' ' + str(value).ljust(width - 2)
    def cells(row):
        return sandwich('|', (cell(col, w) for col, w in zip(row, widths))) + '\n'
    horiz_rule = sandwich('+', ('-' * (w - 1) for w in widths)) + '\n'
    return sandwich(horiz_rule, (cells(row) for row in table))

# In Python 3.7, this should be a @dataclass instead:
class Item(namedtuple('Item', 'name price')):
    def __new__(cls, name, price):
        return super().__new__(cls, name, Decimal(price))

def main():
    menu_items = OrderedDict(zip(ascii_uppercase, [
        Item('The "Big Boy" Burger', '16.99'),
        Item('French Fries', '5.99'),
        Item('Currie sauce', '19.99'),
        Item('Napkins with Chokolates', '10.50'),
        Item('Juice Box', '89.01'),
        Item('Takeout', '18.99'),
    ]))

    print(
        tabular([['The Restaurant at the End of the Universe']], [36 + 9]) +
        tabular(
            (('{0} {1.name}'.format(*stuff), '${1.price}'.format(*stuff))
              for stuff in menu_items.items()),
            [36, 9]
        )
    )

    total = Decimal('0.00')
    while True:
        print('Total: ${0}'.format(total))
        selection = input("Select a letter or 'done': ")
        if selection == 'done':
            break
        total += menu_items[selection].price
    print('Final total: ${0}'.format(total))

if __name__ == '__main__':
    main()


KMeans in the shortest and most readable format I'm learning Python (coming from Java) so I decided to write KMeans as a practice for the language. However I want to see how could one improve the code and making it shorter and yet readable.  I still find the code rather long. Also if you have comments regarding conventions or proper practices, I would really appreciate it.

import numpy as np
from numpy import linalg as LA

def main():
    #data = np.arange(20).reshape( (4,5) )
    file_name = "/Users/x/Desktop/tst.csv"
    with open(file_name) as f:
        ncols = len(f.readline().split(","))
    data = np.loadtxt(file_name, delimiter=",", usecols=range(0,ncols-1))
    kmeans = KMeans()
    kmeans.cluster(data, 3, 200)
    print kmeans.means

class KMeans:

    def __init__(self):
        self.means = None
        self.data_assignments = None

    #data is a 2D Numpy array
    def cluster(self, data, numClusters, iterations):
        if numClusters < 1:
            raise Exception("The number of clusters should be larger than 0.")
        if numClusters > data.shape[0]:
            raise Exception("The number of clusters is beyond the number of rows.")

        #Pick random means
        randomMeansIndices = np.random.choice( range( data.shape[0] ),
                                               size=numClusters, replace=False )

        for i in randomMeansIndices:
            if self.means is not None:
                self.means = np.vstack( ( self.means, data[ i ] ) )
            else:
                self.means = data[ i ]

        for iteration in xrange(iterations):
            #Data assignment
            self.data_assignments = {}
            for row in data:
                distances = {}
                meanID = 0
                for mean in self.means:
                    distances[meanID] = LA.norm(mean - row)
                    meanID += 1
                nearestMean = min( distances, key=lambda x: distances[x] )
                if nearestMean in self.data_assignments:
                    self.data_assignments[nearestMean] = np.vstack( (self.data_assignments[nearestMean], row) )
                else:
                    self.data_assignments[nearestMean] = row

            #Update the means
            self.means = None
            for mean_data_matrix in self.data_assignments.values():
                if mean_data_matrix.ndim == 1:
                    mean_data_matrix = mean_data_matrix.reshape((1,-1)) #reshape to make it 2D
                if self.means is None:
                    self.means = np.mean( mean_data_matrix, axis=0 ) #the first updated mean vector
                else:
                    newMean = np.mean( mean_data_matrix, axis=0 ) #updated mean vector
                    self.means = np.vstack( (self.means, newMean) )#add an updated mean

main()

You use vstack a lot to build arrays row by row, with the additional complication of initializing the array variable to None, which needs a special case in your loops then. This complication could be avoided by intializing to zero-height 2D array instead, but in any case repeated vstack use is inefficient because it copies the array every time. Better options are:

Collect rows in a list and vstack the list in the end. Good approach when you don't know the height of the array in advance.
Initialize array to right size and assign into rows.
Best of all, create array at once by a vectorized operation.

For example this

self.means = None
for i in randomMeansIndices:
    if self.means is not None:
        self.means = np.vstack( ( self.means, data[ i ] ) )
    else:
        self.means = data[ i ]

could be done in one line, as you can index arrays with arrays:

self.means = data[randomMeansIndices]

This code could avoid the loop

distances = {}
meanID = 0
for mean in self.means:
    distances[meanID] = LA.norm(mean - row)
    meanID += 1
nearestMean = min( distances, key=lambda x: distances[x] )

by computing all the norms at once (NumPy 1.8 required). argmin gives the index of the minimum.

distances = LA.norm(self.means - row, axis=1)
nearestMean = np.argmin(distances)

The KMeans class has no purpose other than holding the result of clustering. The cluster method could be a stand-alone function instead and just return means, data_assignments. If you prefer to access the two items by name, use a collections.namedtuple as the return value.


Simple cache mechanism? In my system I frequently require data by Ajax requests. For example, different lists, content of modal dialogs, etc. This data can't be changed after request, so I wrote cache mechanism which stores Ajax result and then before Ajax request checks is there stored data.
Here is example:

YFeX.cache = { }; // YFex is my own namespace

$('#account_modal_button').click(function () {
    if (YFeX.cache.account_modal !== undefined) {
        YFeX.modal(YFeX.cache.account_modal.title, YFeX.cache.account_modal.body);
    } else {
        $.ajax({
            type:'GET',
            url:'/?/login',
            dataType:'json',
            context:$(this),
            success:function (data) {
                YFeX.cache.account_modal = data.modal;
                // YFeX.modal will be called automatically
            }
        });
    }
    return false;
});

Are there any 'bad' parts in my solution?

From a once over;

Unless YFeX is also a constructor, it should be yFex
lowerCamelCase is good for you: account_modal -> accountModal
I would store YFeX.cache.account_modal into a var cachedAccountModal so that you do not have to repeat YFeX.cache.account_modal all the time
Your code is very optimistic, you should also deal with fail
Consider using the shorthand $.get()

Other than that, the code looks fine to me.


Simple RPG - Alice fights Bob This is designed to be a simple example of a RPG battle, for a talk I hope to give. The rules:

Alice and Bob both have 5 hit points and inflict 2 damage.
Each turn, Alice attacks Bob, and Bob attacks Alice.
3/4 of attacks hit their target.
1/2 of the attacks that hit their target are blocked, inflicting half damage.
When one character is dead, the game is over.

Important ideas:

Sending the attack and receiving the attack are handled separately.
It's a re-usable API for characters, so game.rb is a single use case. There could be others.

character.rb

class Character

  attr_accessor :name, :hit_points

  def initialize(name, hit_points, hit_chance, block_chance, damage)
    @name = name
    @hit_points = hit_points
    @hit_chance = hit_chance
    @block_chance = block_chance
    @damage = damage
  end

  def attack(other_character)
    puts "#{name} attacks #{other_character.name}"
    if roll(@hit_chance)
      puts "  The attack hits"
      other_character.be_attacked(@damage)
    else
      puts "  The attack missed"
    end
  end

  def be_attacked(damage)
    if roll(@block_chance)
      puts "  It was blocked, half damage done."
      take_damage(damage / 2)
    else
      take_damage(damage)
    end
  end

  def take_damage(damage)
    @hit_points -= damage
    puts "  -#{damage} hit points"
    puts "  #{name} has been defeated" if dead?
  end

  def dead?
    @hit_points <= 0
  end

  private

  def roll(chance)
    rand(100) < chance
  end

end

game.rb

class Game

  def initialize
    @alice = Character.new('Alice', 5, 75, 50, 2)
    @bob = Character.new('Bob', 5, 75, 50, 2)
  end

  def turn
    @alice.attack(@bob)
    @bob.attack(@alice)
    report_status
    sleep 1
  end

  def over?
    (@alice.dead? or @bob.dead?)
  end

  def report_status
    puts ''
    if over?
      puts "The game is over!"
    else
      puts "Alice: #{@alice.hit_points} - Bob: #{@bob.hit_points}"
      puts ''
    end
  end

end

script.rb (executable file)

require_relative 'character.rb'
require_relative 'game.rb'

game = Game.new
game.turn until game.over?

Example output:

Alice attacks Bob
  The attack missed
Bob attacks Alice
  The attack hits
  -2 hit points

Alice: 3 - Bob: 5

Alice attacks Bob
  The attack hits
  -2 hit points
Bob attacks Alice
  The attack missed

Alice: 3 - Bob: 3

Alice attacks Bob
  The attack hits
  -2 hit points
Bob attacks Alice
  The attack hits
  -2 hit points

Alice: 1 - Bob: 1

Alice attacks Bob
  The attack hits
  It was blocked, half damage done.
  -1 hit points
  Bob has been defeated
Bob attacks Alice
  The attack hits
  -2 hit points
  Alice has been defeated

The game is over!

When one character is dead, the game is over.

No, it isn't, because in your own example output it says:

  Alice attacks Bob [...] Bob has been defeated

  Bob attacks Alice [...] Alice has been defeated

  The game is over!

Bob is apparently dangerous, even in death.

Anyway, code review:

I'd prefer that the characters weren't responsible for printing anything. Their responsibility should only be to be number generators; let the game handle the encounter.

I'd also advise against using attr_accessor :hit_points, since that means you can set the hit points from anywhere. Using attr_reader would be more fitting, I think.

Lastly, naming you instance variables @alice and @bob is rather too specific; they're both just characters. Besides, you might want more at some point.

Here's how I might do the character class:

class Character
  attr_reader :name, :hit_points, :damage

  def initialize(name, hit_points, damage, hit_chance, block_chance)
    @name = name
    @hit_points = hit_points
    @damage = damage
    @hit_chance = hit_chance
    @block_chance = block_chance
  end

  def dead?
    @hit_points <= 0
  end

  def hits?
    roll(@hit_chance)
  end

  def blocks?
    roll(@block_chance)
  end

  def take_damage(amount)
    @hit_points -= amount
  end

  def block_damage
    @block_damage ||= (damage / 2).round
  end

  private

  def roll(threshold)
    rand(100) > threshold
  end
end

As you can see, it just a data model; it doesn't manipulate anything outside itself.

For the game class, here's a simple implementation that has 3 characters in the mix (everyone gets an attack at each of the others, which isn't too RPG'ish, but it's just for illustration).

class Game
  def initialize
    @characters = []
    @characters << Character.new("Alice", 5, 2, 75, 50)
    @characters << Character.new("Bob", 5, 2, 75, 50)
    @characters << Character.new("Carol", 5, 2, 75, 50)
  end

  def play
    while play_turn
      puts "----------------------------"
      print_status
      puts "----------------------------"
      sleep 1
    end
  end

  def play_turn
    living_characters.permutation(2) do |a, b|
      puts "#{a.name} attacks #{b.name}"

      if a.hits?
        blocked = b.blocks?
        damage = blocked ? a.block_damage : a.damage
        puts "\t#{b.name} blocks" if blocked
        puts "\t#{a.name} inflicts #{damage} points of damage"
        b.take_damage(damage)
        puts "\t#{b.name} has been defeated" if b.dead?
      else
        puts "\t#{a.name} misses"
      end

      if living_characters.size == 1
        puts "Game over, #{a.name} wins"
        return false
      end
    end
    true
  end

  def print_status
    living_characters.each do |player|
      puts "#{player.name}: #{player.hit_points} hit points"
    end
  end

  private

  def living_characters
    @characters.reject(&:dead?)
  end
end

The game class is responsible for printing too, which isn't necessary the nicest way to do things. It conflates the view and controller, even though the model (the character) is now nicely encapsulated. However, I don't know the exact ruleset and UI you're going for, so I'll leave it as-is.

This class also does things a little differently than yours: The game ends the moment there's only one character left alive; recently dead characters do not get a final swing.


Tenis Common opponents R I'm follow this article about "common opponent in tennis", my goal is to script it in the most efficient way. Below you can find my code but is so slow. To calculate the result of 1 match my laptop spent 120seconds more or less, and I have a dataset of 150k of rows to calculate.

The article: (PDF)

I need your help to clean and improve my code. Links to the data used:

tableA

tableB

data_tennis_co

data_tennis_co = read.table("test_co.csv", header=FALSE, sep=",", fill = TRUE)
A = read.table("tableA.csv", header=FALSE, sep=",", fill = TRUE)
B = read.table("tableB.csv", header=FALSE, sep=",", fill = TRUE)

#BASIC FUNCTIONS
G<-function(p){res<- p^4*(15-4*p-((10*p^2)/(1-2*p*(1-p))))}
d<- function(p,q) {res<- p*q*(1-(p*(1-q)+(1-p)*q))^-1}
TB <- function(p,q) {res <- foreach(i = seq_along(1:28), .combine = sum) %dopar% {tb<-A[i,1]*p^A[i,2]*(1-p)^A[i,3]*q^A[i,4]*(1-q)^A[i,5]*d(p,q)^A[i,6]}}
S <- function(p,q) {res <- foreach(i = seq_along(1:21), .combine = rbind) %dopar% {s<-B[i,1]*G(p)^B[i,2]*(1-G(p))^B[i,3]*G(q)^B[i,4]*(1-G(q))^B[i,5]*(G(p)*G(q)+(G(p)*(1-G(q))+(1-G(p))*G(q))*TB(p,q))^B[i,6]} sum(res)}
M3 <- function(p,q) {res <- S(p,q)^2*(1+2*(1-S(p,q)))}
DELTA_AB <- function(spwAC,rpwAC,spwBC,rpwBC) {res <- (spwAC-(1-rpwAC))-(spwBC-(1-rpwBC))}
PR<- function(spwAC,rpwAC,spwBC,rpwBC) {res <- (M3(0.6+DELTA_AB(spwAC,rpwAC,spwBC,rpwBC),(1-0.6))+M3(0.6,(1-(0.6-DELTA_AB(spwAC,rpwAC,spwBC,rpwBC)))))/2}

#COMMON OPPONENTS
MAL<-function(id1,id2){
prova<- subset(data_tennis_co, V3 == 1 & V4==2)
  previous<-subset(data_tennis_co, V2 < prova$V2)
  s1 <- subset(previous, V3 == 1 | V4==1)
  s2 <- subset(previous, V3 ==2 | V4==2)
  s1$opp <- ifelse(s1$V3==1, s1$V4, s1$V3)
  s2$opp <- ifelse(s2$V3==2, s2$V4, s2$V3)
  inn<- intersect(s1$opp,s2$opp)
  common1<-s1[s1$opp %in% inn,]
  common2<-s2[s2$opp %in% inn,]
  # fare media se id non unico
  COM <- merge(common1, common2,by=c("opp"))
  COM$OMALLEY <- unlist(mapply(PR, COM$V5.x, COM$V6.x, COM$V7.y, COM$V8.y))
  COM$OMALLEY[is.nan(COM$OMALLEY)] <- 0.5
  return(tryCatch(sum(COM$OMALLEY)/nrow(COM), error=function(e) NaN))
}

RESA<-MAL(1,2)

You could help us by providing the output of the profiler:

Rprof(tmp <- tempfile())
RESA <- MAL(1, 2)
Rprof()
summaryRprof(tmp)
unlink(tmp)

, but I suspect the main bottleneck in your code is the use of parallel loops in TB and S, for operations that can be done faster using vectorized R functions. Please give this a try and let us know if it is faster for you:

G  <- function(p)    p^4*(15-4*p-((10*p^2)/(1-2*p*(1-p))))
d  <- function(p, q) p*q*(1-(p*(1-q)+(1-p)*q))^-1
TB <- function(p, q) sum(A[,1] * p^A[,2] * (1-p)^A[,3] *
                         q^A[,4] * (1-q)^A[,5] * d(p,q)^A[,6])
S  <- function(p, q) {
   Gp <- G(p)
   Gq <- G(q)
   sum(B[,1] * Gp^B[,2] * (1-Gp)^B[,3] * Gq^B[,4] * (1-Gq)^B[,5] *
      (Gp*Gq+(Gp*(1-Gq)+(1-Gp)*Gq)*TB(p,q))^B[,6])
}
M3 <- function(p, q) {
   s <- S(p,q)
   s^2*(1+2*(1-s))
}
DELTA_AB <- function(spwAC,rpwAC,spwBC,rpwBC) (spwAC-(1-rpwAC)) -
                                              (spwBC-(1-rpwBC))
PR <- function(spwAC,rpwAC,spwBC,rpwBC) {
   D <- DELTA_AB(spwAC, rpwAC, spwBC, rpwBC)
  (M3(p = 0.6 + D, q = (1 - 0.6)) +
   M3(p = 0.6, q = 1 - (0.6 - D))) / 2
}


Full Adder in Verilog I'll be teaching Computer Architecture at the undergraduate level this fall and want to make sure that my example Verilog code follows best practices. I welcome any suggestions, however minor, for improving this code, which runs at EDA Playground.
Our textbook is Computer Organization and Design by Hennessy and Patterson. It doesn't say much about Verilog, and I will only be taking a small piece of code from an appendix, so there is no style to be consistent with.
Testbench
module test;
  reg   a;
  reg   b;
  reg   c_in;
  wire  sum;
  wire  c_out;

  ADDER adder(a, b, c_in, sum, c_out);

  initial begin
    // Dump waves
    $dumpfile("dump.vcd");
    $dumpvars(1);

    for (int i = 0; i < 8; i++) begin
      {a, b, c_in} = i;
      display;
    end
  end

  task display;
    #1;
    $display("%b + %b + %b = %b%b", a, b, c_in, c_out, sum);
  endtask

endmodule

Design
module ADDER (a, b, c_in, sum, c_out);
  input     a;
  input     b;
  input     c_in;
  output    sum;
  output    c_out;

  assign c_out = (a & b) | (a & c_in) | (b & c_in);
  assign sum = a ^ b ^ c_in;
endmodule

In the design module, use ANSI-style port declarations to reduce redundant port lists (refer to IEEE-Std 1800-2017, section 23.2.1 Module header definition):
module ADDER (
  input     a,
  input     b,
  input     c_in,
  output    sum,
  output    c_out
);

In the testbench, use connections-by-name instead of connections-by-order:
ADDER adder (
    .a     (a),
    .b     (b),
    .c_in  (c_in),
    .sum   (sum),
    .c_out (c_out)
);

This involves more typing, but it avoids common connection errors, and it makes the code easier to understand (more self-documenting).  Refer to Std section
23.3.2.2 Connecting module instance ports by name.
I usually find it helpful for debugging to also display the time:
$display($time, " %b + %b + %b = %b%b", a, b, c_in, c_out, sum);


Selecting all posts with a certain tag The task is to select all posts tagged with a certain tag, along with other tags the post may have, given as text rather than ID.

Table posts

CREATE TABLE `posts` (
 `id` int(10) unsigned NOT NULL AUTO_INCREMENT,
 `title` text NOT NULL,
 `text` text NOT NULL,
 `date` datetime NOT NULL,
 PRIMARY KEY (`id`)
) ENGINE=MyISAM AUTO_INCREMENT=10 DEFAULT CHARSET=utf8

Table tags

CREATE TABLE `tags` (
 `id` int(10) unsigned NOT NULL AUTO_INCREMENT,
 `tag` varchar(20) NOT NULL,
 PRIMARY KEY (`id`),
 UNIQUE KEY `tag` (`tag`)
) ENGINE=MyISAM AUTO_INCREMENT=5 DEFAULT CHARSET=utf8

Table posts_tags

CREATE TABLE `posts_tags` (
 `post` int(10) unsigned NOT NULL,
 `tag` int(11) NOT NULL,
 PRIMARY KEY (`post`,`tag`),
 KEY `tag` (`tag`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8

And here is the SQL I've come up with:

SELECT posts.id,
       title,
       posts.text,
       date,
       t.tag
FROM   tags
       LEFT JOIN posts_tags
              ON tags.id = posts_tags.tag
       LEFT JOIN posts
              ON posts.id = posts_tags.post
       LEFT JOIN posts_tags pt
              ON posts.id = pt.post
       LEFT JOIN tags t
              ON t.id = pt.tag
WHERE  tags.tag = ?
ORDER  BY posts.id DESC

Are there any optimizations that can be done to this code?

Looks good.
There is not a whole lot to say that could improve performance. It's a pretty minimal example, so take that for what it is.

Table naming
Your DDL naming convention is hard to follow in your post_tags table:

CREATE TABLE `posts_tags` (
 `post` int(10) unsigned NOT NULL,
 `tag` int(11) NOT NULL,
 PRIMARY KEY (`post`,`tag`),
 KEY `tag` (`tag`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8

Would be more logical if you specified it was an Id in the name:
CREATE TABLE `posts_tags` (
 `post_id` int(10) unsigned NOT NULL,
 `tag_id` int(11) NOT NULL,
 PRIMARY KEY (`post`,`tag`),
 KEY `tag` (`tag`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8

Foreign keys
It looks like you don't have any FOREIGN KEY constraints, which may be a problem, or at least something you would want to integrate, so as to be able to raise key violations. For example, using the above-referenced table:
CREATE TABLE `posts_tags` (
 `post_id` int(10) unsigned NOT NULL,
 `tag_id` int(11) NOT NULL,
 PRIMARY KEY (`post`,`tag`),
 KEY `tag` (`tag`)
 FOREIGN KEY (post_id) REFERENCES posts(id),
 FOREIGN KEY (tag_id) REFERENCES tags(id),
) ENGINE=InnoDB DEFAULT CHARSET=utf8

One last thing. The back ticks in MySQL are only needed for names that use reserved characters/delimiters, for example names that have a space between words. The way you are naming your tables and columns, none of those would be needed.


Modular arithmetic brute-force congruence finder My full code is running too slow. I do profile.run to my project and found out this function consumes a lot of time.

The main problem is just to return an array containing all \$x\$ satisfying both of these equations with some large primes like \$23857201\$.

$$(x^3+v) \mod P = 0$$

$$(3xx + 3hx + h^2) \mod P = 0$$

     def afind(h,v):
        p=[]
        for x in range(23857201):
           if (x**3+v) % 23857201 == 0 and (3*x*x+3*h*x+h**2)*h%23857201==0:
              p.append(x)
     return p

It took 21.69 sec for afind(1,940). I tried Cython, but it didn't help. How can I speed this up?

In case it is you who figured out an \$h\$ factor in the code snippet, add \$x^3\$ to both sides and rewrite the second equation as \$(x + h)^3 \equiv x^3 (\mod P)\$, so that with the first one in mind, the system comes down to

\$x^3 + v \equiv 0 (\mod P)\$

\$(x + h)^3 + v \equiv 0 (\mod P)\$

The programmatic approach is obvious:

find all solutions to \$x^3 + v \equiv 0 (\mod P)\$, and
select those which are exactly \$h\$ apart.

Of course the first step is a key (the second is trivial). I am not that good in a theory of Diophantine cubics to suggest any more.

PS: Math.SE is indeed a right place to ask.


Centre all columns of an array I have this working code tested over lots of np.arrays. It is a for-loop that walks through all the values of a np.array (x,y).

For each y-row, it finds the first x-column for which the value is different to zero. Afterward, it finds the last x-column for which value is different to 0.

Then all the columns between first x-column and last x-column are centred.

This is then repeated for all y-rows.
Example:

#Input :
array([[ 0.0,  0.149,  0.064, 0.736,  0.0],
       [ 0.0,  0.0,  0.258,   0.979,  0.618 ],
       [ 0.0,  0.0,  0.0,     0.786,  0.666],
       [ 0.0,  0.0,  0.0,     0.782,  0.954],

 #Output :
array([[ 0.0,  0.149, 0.064, 0.736, 0.0],
       [ 0.0,  0.258, 0.979, 0.618, 0.0],
       [ 0.0,  0.786, 0.666, 0.0,   0.0],
       [ 0.0,  0.782, 0.954, 0.0,   0.0],

Also:

Not all the values between first and last columns are different than zero.

for y in range(len(array)):

    begin = False
    inside = False
    end = False

    for x in range(len(array[0])):

        if (array[y][x] == 0) & (begin == True) & (end == False):
            boundary_two = ( x - 1 )
            inside = False
            end = True

        elif (array[y][x] != 0) & (inside == False):
            boundary_one = x
            begin = True
            inside = True

    y_position.append(y)
    m = np.split(array[y],[boundary_one,boundary_two])
    zeros = len(array[0])-len(m[1])
    array[y] = np.concatenate((np.zeros(zeros//2),m[1],np.zeros(int(np.ceil(zeros/2)))))

Furthermore, I added a variable(count) inside the function (which I erase for the upper code example, to simplify lecture) which count how many empty rows since last non empty rows. When count ==10, we break out of the loop. This is to save time. Once we have +/- 10 empty rows after the non-empty ones, it is sure all other y-rows will be empty as well.

Finally, I must save the value for the last y-row non-empty.

This is my script most time demanding calculation, so I was wondering if there is a way of to improve it, either by making it clearer and/or faster.

Thanks a lot, hope it is clear!

review

comparison

no need to compare against True or False. (begin == True) can be easier expressed as begin and (end == False) as not end. and comparison chaining is done with and, not &(bitwise and)

iteration

In python, it is very seldomly needed to iterate over the indices. Instead of for y in range(len(array)): and for x in range(len(array[0])), you can do

for row in array:
    ...
    for x, element in enumerate(row):
        if element == 0 and begin and not end:

alternative solution

try to vectorise as much as possible. The easiest thing to do is count the zeroes in front and at the back:

def count_zeroes(array):
    return (array.cumsum(axis=1) == 0).sum(axis=1)

zeroes_front = count_zeroes(array)

and then the same for the reverse:

zeroes_back = count_zeroes(test_data[:,::-1])

The amount each row needs to roll is:

roll = (zeroes_front + zeroes_back) //2 - zeroes_front

array([ 0, -1, -2, -2])

and then you apply this roll over each row:

np.array([np.roll(row, r) for row, r in zip(test_data, roll)])

array([[0.   , 0.149, 0.064, 0.736, 0.   ],
       [0.   , 0.258, 0.979, 0.618, 0.   ],
       [0.   , 0.786, 0.666, 0.   , 0.   ],
       [0.   , 0.782, 0.954, 0.   , 0.   ]])

in total:

def centre(array):
    zeroes_front = count_zeroes(array)
    zeroes_back = count_zeroes(array[:,::-1])
    roll = (zeroes_front + zeroes_back) //2 - zeroes_front
    return np.array([np.roll(row, r) for row, r in zip(array, roll)])


How can I better my URL checker program? I made a LinkChecker program which checks URLs from a text file and outputs status into another file like this:

http://valid-url.com;OK
http://invalid-url.com;The remote server returned an error: (404) Not Found.

I have about 1400 URLs in the input file and it takes about 12 minutes to run.

I made it work. In what ways can I achieve rightness and fastness?

public class LinkChecker
{
    static void Main()
    {
        string line = string.Empty;
        string inputFile = @"D:\tmp\links.txt";
        StreamReader file = new StreamReader(inputFile);
        string outputFile = @"D:\tmp\out.txt";

        ClearOutputFile(outputFile);

        while(!string.IsNullOrEmpty(line = file.ReadLine()))
        {
            string currentUrlAndStatus = CheckURL(line);
            if(!string.IsNullOrEmpty(currentUrlAndStatus))
            {
                WriteToFile(currentUrlStatus, outputFile);
            }
        }
    }

    private static string CheckURL(string url)
    {
        string status = string.Empty;

        HttpWebRequest request = (HttpWebRequest)WebRequest.Create(url);
        request.Method = "HEAD";
        request.Proxy = null;

        try
        {
            using(HttpWebResponse response = (HttpWebResponse)request.GetResponse())
            {
                status = response.StatusCode.ToString();
            }
        }
        catch (WebException ex)
        {
            status = ex.Status;
        }

        return url + ";" + status;
    }

    private static void ClearOutputFile(string file)
    {
        using (Filestream fileStream = File.Open(file, FileMode.Open)
        {
            fileStream.SetLength(0);
        }
    }

    private static void WriteToFile(string message, string filename)
    {
        using(var writer = new StreamWriter(filename, true))
        {
            writer.WriteLine(message);
        }
    }
}

You forgot using using in your StreamReader

    using(StreamReader file = new StreamReader(inputFile)){
    string outputFile = @"D:\tmp\out.txt";

    ClearOutputFile(outputFile);

    while(!string.IsNullOrEmpty(line = file.ReadLine()))
    {
        string currentUrlAndStatus = CheckURL(line);
        if(!string.IsNullOrEmpty(currentUrlAndStatus))
        {
            WriteToFile(currentUrlStatus, outputFile);
        }
    }
  }

Use the var keyword for local variables, let the compiler figure out the type for you. Type inference helps in refactoring.

 var line = string.Empty;
 var inputFile = @"D:\tmp\links.txt";
 using(var file = new StreamReader(inputFile)){
 //...
 }
 var outputFile = @"D:\tmp\out.txt";
 // etc..


Trying to improve my javascript code in this simple challenge from coderbyte Here is a slightly modified challenge from Coderbyte:

Determine if a given string is an acceptable. The str parameter will be composed of + and = symbols with several letters between them (ie. ++d+===+c++==a) and for the string to be true each letter must be surrounded by a + symbol. So the string to the left would be false. The string will not be empty and will have at least one letter.

For whatever reason, I have struggled mightily with this challenge. I tried to use a regular expression in my Booleans, I tried cascading if statements, I tried a while statement surrounding if statements, I tried to structure my code as a function. After hours of failure, I finally resorted to the following:

var str = prompt("Please enter a test string: ").split("");
answer = "true";

if (((str[0] != "+") && (str[0] != "=")) || ((str[str.length-1] != "+") && (str[str.length-1] != "="))){
  answer = "false";
}

for (var i = 1; i < str.length-1; i++){
  if (((str[i] != "+") && (str[i] != "=")) && (str [i-1] != "+" || str [i+1] != "+")){
      answer = "false";
  }
}

console.log(answer);

I think it works, but of all the possible solutions to this problem, I am guessing that it is toward the bottom in terms of efficiency and elegance. I'd really appreciate some guidance on how this could be improved.

Below is one of my "solutions" that did not work. Among other things, the Booleans are not evaluating correctly. I must be using the regular expressions incorrectly.

var str = prompt("Please enter a test string: ").split("");
answer = "true";

if ((str[0] === /[a-z]/gi) || (str[str.length-1] === /[a-z]/gi)){
    answer = "false";
}
for (var i = 1; i < str.length-1; i++){
  if (str[i] === /[a-z]/gi && (str [i-1] != "+" || str [i+1] != "+")){
      answer = "false";
  }

}

console.log(answer);

Your version is actually not that bad but could be simplified slightly and wrapped in a function. You could use charAt to get the character from a string instead of splitting the string. You don't need the initial check on the first and last characters; you can just do these as part of the main loop. (This works because str.charAt(-1) and str.charAt(str.length) both just return empty strings. Similarly if you were using an array, arr[-1] returns undefined.)

Wrapping this in a function and returning false when a non-matching character is found makes the code more efficient, as it does not have to run through the remaining loops when it already knows the answer (this would be the same as inserting break into your code.)

The below code also checks that the characters are either +, =, or a letter, which your original code does not do (so it would accept "===+!+==+3+", for example).

var str = "++d+===+c++=a";
console.log(checkString(str));

function checkString(str) {
    for (var i = 0; i < str.length; i++) {
        if (str.charAt(i) != '+' && str.charAt(i) != '=' &&
            !(str.charAt(i) >= 'a' && str.charAt(i) <= 'z' &&
              str.charAt(i - 1) == '+' && str.charAt(i + 1) == '+')) {
            return false;
        }
    }
    return true;
}

To use regular expressions you need to write pattern.test(str), e.g. /[a-z]/gi.test('a') returns true. However the point of using regex is that you can test a whole string in one go, rather than character-by-character. I think the following will work:

/^((\+[a-z])*\+|=)+$/gi.test('++d+===+c++=a')


Composite Design Pattern in Scala The following is a personal attempt at implementing the Composite design pattern in Scala. Observation is abstract...

class CompositeObservation(obss: Observation*) extends Observation {
  val elements: MutableList[Observation] = new MutableList[Observation]()

  elements ++ obss

  def hasElement(o: Observation): Boolean = elements.contains(o);
}

hasElement fails to return if an element is contained in the composite. Questions:

Am I misinterpreting the ++ operator? The Observation*?
What is the most ideomatic way to implement this pattern in Scala?

++ returns a new collection, you need ++= here
Your attempt looks fine to me. Maybe you should additionally implement the Traversable trait or so, and delegate the calls to elements in order to make things a little bit more convenient.

[Edit]

class CompositeObservation(obss: Observation*)
     extends Observation with Traversable[Observation] {

  val elements = new MutableList[Observation]()

  elements ++= obss

  def hasElement(o: Observation): Boolean = elements.contains(o);

  def foreach[U](f: (Observation) => U): Unit = elements.foreach(f)
}


Synchronous and asynchronous behavior for client I am working on a project in which I am supposed to make synchronous and asynchronous behavior of my client.

The customer will call our client with a userId and we will construct a URL from that userId and make an HTTP call to that URL and we will get a JSON string back after hitting the URL. And after we get the response back as a JSON string, we will send that JSON string back to our customer.

In this case, I need to have synchronous and asynchronous methods.  Some customers will call the executeSynchronous method to get the same feature and some customers will call our executeAsynchronous method to get the data back.

Interface:

public interface Client {

    // for synchronous
    public String executeSynchronous(final String userId);

    // for asynchronous
    public Future<String> executeAsynchronous(final String userId);
}

SmartClient which implements the Client interface:

public class SmartClient implements Client {

    ExecutorService executor = Executors.newFixedThreadPool(5);

    // This is for synchronous call
    @Override
    public String executeSynchronous(String userId) {

        String response = null;
        Future<String> handle = getHandle(userId);

        try {
            response = handle.get(500, TimeUnit.MILLISECONDS);
        } catch (TimeoutException e) {

        }

        return response;
    }

    // This is for asynchronous call
    @Override
    public Future<String> executeAsynchronous(String userId) {

        return getHandle(userId);

    }

    private Future<String> getHandle(String userId) {
        Future<String> future = null;

        Task task = new Task(userId);
        future = executor.submit(task);

        return future;

    }
}

Simple class which will perform the actual task:

class Task implements Callable<String> {

    private final String userId;

    public Task(String userId) {
        this.userId = userId;
    }

    public String call() throws Exception {

        String url = createURL(userId);

        // make a HTTP call to the URL
        RestTemplate restTemplate = new RestTemplate();
        String jsonResponse = restTemplate.getForObject(url, String.class);

        return jsonResponse;
    }

    // create a URL
    private String createURL(String userId) {
        String generateURL = somecode;

        return generateURL;
    }
}

Is this the correct and efficient way of doing this problem?
How about the exception handling?
Do I need any other catch blocks at any places?

Late answer to this question.....

There are a few things which should be considered when implementing a solution like this, and best-practice comes in to play here.

  One nit-pick ... before we go further, your interface the methods
  executeAsynchronous and executeSynchronous, whereas your
  implementation has executeAsync and execute. This is a carelessly
  posted question?

What you have, at the moment, works, but is it the best way? I don't think so.

You code has a few features that are important:

executeSynchronous() - waits until you have a result, returns the result.
executeAsynchronous() - returns a Future immediately which can be processed after other things are done, if needed.
it has a private getHandle() method which does the hard work of creating a Callable, and returning a future.

Now, I have tried to list why there are problems with this, but, it's easier to actually show you a different approach, and explain why it is better.... so, here's a different approach, which reverses the logic.

First thing, we are going to make the interface a generic interface... why does it always have to return a String value for each URL? Also, why is it a Client? It is not really a client, but it is a RemoteCall. This is not the full Client, but a part of what it does when it talks to the server.... so, let's rename the Client interface, and make it generic.... Also, while we are at it, you need to declare that you throw some form of exception... my preference is for a new checked exception type, but maybe you can borrow an existing exception. What you have is not best-practice. Here's a decent exception to throw:

class RemoteCallException extends Exception {

    private static final long serialVersionUID = 1L;

    public RemoteCallException(String message, Throwable cause) {
        super(message, cause);
    }

    public RemoteCallException(String message) {
        super(message);
    }

}

Then, with that exception, we can complete the interface:

public interface RemoteCall<T> {

    // for synchronous
    public T executeSynchronous(final String action) throws RemoteCallException;

    // for asynchronous
    public Future<T> executeAsynchronous(final String action);
}

Right, this interface is now a better representation of the work it is supposed to do.

Now, here's a key feature I want you to consider.... at the moment, you may only have one 'action' you need to execute on the server, the part of translating a userid in to a JSOM response. But, I expect there is, or will be, more. Having to build a new implementation for your Client interface for every type of operation is a real PITA, especially when it requires building a new Callable / Task instance.... so, what we do is we create a convenient abstract class, which will do all the synchronous/asynchronous work for us....

Note, it does not implement executeSynchronous(), and it is still fully generic. It uses an executor service that is passed in.

One of the really huge advantages of reversing the logic (there is no Callable involved in the synchronous call) is that the work is done on the calling thread. In your implementation, you have two threads fully occupied, the calling thread waits for work to be done, and a thread in the ExecutorService is actually doing the work. By making the executeSynchronous() call the way I suggest, only one thread is occupied, and the thread that is busy is the same thread that calls executeSynchronous()

public abstract class AbstractRemoteCall<T> implements RemoteCall<T> {

    private final ExecutorService executor;

    public AbstractRemoteCall(ExecutorService executor) {
        this.executor = executor;
    }

    // note, final so it cannot be overridden in a sub class.
    // note, action is final so it can be passed to the callable.
    public final Future<T> executeAsynchronous(final String action) {

        Callable<T> task = new Callable<T>() {

            @Override
            public T call() throws RemoteCallException {
                return executeSynchronous(action);
            }

        };

        return executor.submit(task);
    }
}

OK, so, the above class deals with any asynchronous method calls, and delegates the synchronous calls to the actual implementations.

Now we get to your actual implementation you want to do, the userid -> JSON transformation. It is now as easy as:

public class SmartClient extends AbstractRemoteCall<String> {

    public SmartClient() {
        super(Executors.newFixedThreadPool(5));
    }

    @Override
    public String executeSynchronous(String userId) throws RemoteCallException {

        String url = createURL(userId);

        // make a HTTP call to the URL
        RestTemplate restTemplate = new RestTemplate();

        return restTemplate.getForObject(url, String.class);

    }

    // create a URL
    private String createURL(String userId) {
        String generateURL = somecode;

        return generateURL;
    }
}

The advantages of all these changes are:

the abstract class is the only place you need to worry about threading
in a synchronous call, the work is done on the calling thread
there is a better mechanism for error handling with the new RemoteCallException.
you can now have many different implementations of the abstract class, each implementation does their own type of restful work, and you don't need to worry about the threading in any of them.
the actual working classes (like SmartClient) do one thing, and one thing only.

There are other advantages.... but, what you have now, though it works, is not nearly as maintainable, extensible, and reliable as what it could be.

EDIT: Some use-cases

How would the code I recommend be used? Two situations.... Synchronous

// set up some system, need the JSON data
SmartClient sc = new SmartClient();
String json = sc.executeSynchronous(userid);

Collecting data asynchronous:

// Set up a number of things, timing can be in parallel.
try {
    SmartClient sc = new SmartClient();
    Future<String> jsonfut = sc.executeAsynchronous(userid);
    // set up some other things.
    // perhaps some thing that gets other data....
    //   this is a different class just like the SmartClient that can be asynchronous
    DataCLient dc = new DataClient();
    Future<String> defautsfut = dc.executeAsynchronous("getdefaults");
    Future<String> messagesfut = dc.executeAsyncrhonous("messages");
    String json = jsonfut.get(500, TimeUnit.MILLISECONDS);
    String defaults = defaultsfut.get(); // no timeout
    String messages = messagesfut.get(); // no timeout
} catch (TimeoutException te) {
    // handle appropriately
}