Subscription handler I have so many classes like this in my solution but I've chosen this one just to show you guys what I mean by it.

In my solution I have lots of classes that just follow the same 2 classes exactly the same, except for the class name and for different purposes.

The issue is, I think I'm OP'ing it. Does something this simple, something I do with around 50 class pairs in my solution really need to take up 2 files and this much code?

It seems like it can be improved but I just can't work out a well-driven way, someone said something about structs but I didn't really look into it too much and it's been too long to remember what else they said.

Is there some sort of layout or structure I can do to reduce the amount of code or the time it takes to code this? I feel like somethings staring me right in the face and I'm missing it.

The purpose of this class pair is to cache database information, store it in a dictionary and return it in case you need it later. It saves an extra call to the database every time I need to get something, and it's just a lot easier to maintain and work with than constant database calls all over the place.

internal class SubscriptionHandler
{
    private readonly Dictionary<int, Subscription> _subscriptions;

    public SubscriptionHandler()
    {
        _subscriptions = new Dictionary<int, Subscription>();
    }

    public void Load()
    {
        using (var dbConnection = Program.Server.DatabaseHandler.Connection)
        {
            dbConnection.SetQuery("SELECT * FROM `subscriptions`;");

            using (var reader = dbConnection.ExecuteReader())
            {
                while (reader.Read())
                {
                    var subscriptionId = reader.GetInt32("id");
                    _subscriptions[subscriptionId] = new Subscription();
                }
            }
        }
    }

    public bool TryGetSubscriptionById(int subscriptionId, out Subscription subscription)
    {
        return _subscriptions.TryGetValue(subscriptionId, out subscription);
    }
}

internal class Subscription
{
    public int Id;
    public string Name;
    public string Badge;
    public int Credits;
    public int Pixels;
    public int Respects;

    public Subscription(int id, string name, string badge, int credits, int pixels, int respects)
    {
        Id = id;
        Name = name;
        Badge = badge;
        Credits = credits;
        Pixels = pixels;
        Respects = respects;
    }
}

So you're saying that you have a bunch of classes representing db rows, and need a WhateverHandler for each class?

Use generics.

https://msdn.microsoft.com/en-us/library/ms172192(v=vs.110).aspx
https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/generics/

The type is replaced whenever you need to use it, like so:

internal class Handler<T> where T : new()
{
    private readonly Dictionary<int, T> _rows;
    private readonly string _tableName;
    private Handler() {}
    public Handler(string tableName)
    {
        _rows = new Dictionary<int, T>();
        _tableName = tableName;
    }

    public void Load()
    {
        using (var dbConnection = Program.Server.DatabaseHandler.Connection)
        {
            dbConnection.SetQuery("SELECT * FROM `{0}`;".Format(tablename));

            using (var reader = dbConnection.ExecuteReader())
            {
                while (reader.Read())
                {
                    var rowId = reader.GetInt32("id");
                    _rows[rowId] = new T();
                }
            }
        }
    }

    public bool TryGetById(int rowId, out T row)
    {
        return _rows.TryGetValue(rowId, out row);
    }
}

edit: thanks @kuskmen for reminding me about the new() constraint


Ruby script to send PragPub magazine to Kindle I wrote a little ruby script to download issues of PragPub magazine and send them to my Kindle via email. I would love to get some feedback on it.

One part, specifically, where I feel like there might be a better way is where it downloads the file and writes to disk. It used to do this:

open(filepath, 'wb') do |file|
  file << open(url).read
end

But this was causing a problem when the issue was not available yet. The http request would fail and there would be an empty file left on disk. So I changed it to this:

  begin
    content = open(url).read
  rescue OpenURI::HTTPError => e
    abort "Unable to download #{filename}: #{e.message}"
  end
  open(filepath, 'wb') do |file|
    file << content
  end

This works better, but feels a little clunky.

Also, is it common/acceptable in Ruby-land to just write a procedural style script like this, or should it be organized into class(es) and methods?

pragpub2kindle.rb

#!/usr/bin/env ruby

require 'open-uri'
require 'mail'
require 'optparse'

options = {}
optparse = OptionParser.new do |opts|
  opts.banner = "Usage: pragpub2kindle.rb [options] [yyyy-mm]"
  options[:mailto] = nil
  opts.on('-t', '--mailto EMAIL', 'Send attachment to email address') do |email|
    options[:mailto] = email
  end
  options[:mailfrom] = nil
  opts.on('-f', '--mailfrom EMAIL', 'Send attachment from email address') do |email|
    options[:mailfrom] = email
  end
  options[:dir] = nil
  opts.on('-d', '--dir DIR', 'Directory to save files') do |dir|
    options[:dir] = dir
  end
  opts.on('-h', '--help', 'Display this screen') do
    puts opts
    exit
  end
end

optparse.parse!

mandatory = [:mailto, :mailfrom]
missing = mandatory.select{ |param| options[param].nil? }
if not missing.empty?
  puts "Missing options: #{missing.join(', ')}"
  puts optparse
  exit
end

dir = options[:dir] || Dir.getwd
if (!(File.directory?(dir) && File.writable?(dir)))
  puts "Directory not valid or not writable: #{dir}"
  puts optparse
  exit
end

if ARGV.length == 1
  year_month = ARGV.first
  if !year_month.match(/\d{4}-\d{2}/)
    puts "Invalid date format: #{year_month}"
    puts optparse
    exit
  end
else
  year = Time.now.strftime("%Y")
  month = Time.now.strftime("%m")
  year_month = "#{year}-#{month}"
end

filename = "pragpub-#{year_month}.mobi"
year = year_month[0..3]

url = "http://magazines.pragprog.com/#{year}/#{filename}"
filepath = File.join(dir, filename)

if File.exists?(filepath)
  puts "#{filename} already exists"
else
  puts "Downloading #{filename}..."
  begin
    content = open(url).read
  rescue OpenURI::HTTPError => e
    abort "Unable to download #{filename}: #{e.message}"
  end
  open(filepath, 'wb') do |file|
    file << content
  end
  mail_to = options[:mailto]
  mail_from =  options[:mailfrom]
  puts "Sending #{filename} to #{mail_to}..."
  mail = Mail.new
  mail.to = mail_to
  mail.from = mail_from
  mail.subject = filename
  mail.attachments[filename] = {
    :mime_type => 'application/x-mobipocket-ebook',
    :content => File.read(filepath)
  }
  mail.deliver!
end

For the first snippet there is no much room for improvement, but you can use File.write:

begin
  content = open(url).read
  File.write(filepath, content)
rescue OpenURI::HTTPError => e
  abort "Unable to download #{url}: #{e.to_s}"
end

For the options parsing part, I'd definitely recommend something better than optparse, for example https://github.com/jashmenn/trollop/


Zeller's algorithm in Python I have implemented Zeller's algorithm in Python to calculate the day of the week for some given date. I would like to get some feedback on how I can make the code shorter and more elegant:

def input_value(input_name, prompt, min_num, max_num):
    while True:
        data = raw_input(prompt)
        if data:
            try:
                input_name = int(data)
            except ValueError:
                print 'Invalid input...'
            else:
                if input_name >= min_num and input_name <= max_num:
                    return input_name
                    break
                input_name = 'Please try again: '
        else:
            print 'Goodbye!'
            break

month = 0
day = 0
year = 0
century = 0

month = input_value(month, "Please enter the month (from 1-12, where March is 1 and February is 12): ", 1, 12)
day = input_value(day, "Please enter the day (from 1-31): ", 1, 31)
year = input_value(year, "Please enter the year (from 0 - 99, eg. 88 in 1988): ", 0, 99)
century = input_value(century, "Please enter the century (from 0 - 99, eg. 19 in 1988): ", 0, 99)

A = month
B = day
C = year
D = century

W = (13*A - 1) / 5
X = C / 4
Y = D / 4
Z = W + X + Y + B + C - 2*D
R = Z % 7

birthday = "You were born on " + str(B) + "/" + str(A+2) + "/" + str(D) + str(C) + " which was a "

if R == 0:
    print birthday + 'Sunday'
elif R == 1:
    print birthday + 'Monday'
elif R == 2:
    print birthday + 'Tuesday'
elif R == 3:
    print birthday + 'Wednesday'
elif R == 4:
    print birthday + 'Thursday'
elif R == 5:
    print birthday + 'Friday'
elif R == 6:
    print birthday + 'Saturday'

Thanks in advance for taking the time to look at this.

def input_value(input_name, prompt, min_num, max_num):

input_name is always a number - why are you calling it a 'name'? Also, there's no need to take it as an argument, since you never use its initial value in your function.

    while True:
        data = raw_input(prompt)
        if data:
            try:
                input_name = int(data)
            except ValueError:
                print 'Invalid input...'

Consider putting a continue here, for clarity. It isn't necessary, though.

            else:
                if input_name >= min_num and input_name <= max_num:

Python understands if min_num <= input_name <= max_num:

                    return input_name
                    break

You don't need break here, the return makes it unreachable.

                input_name = 'Please try again: '

Assigning a string to something that's otherwise always a number is odd, and this value is never used anyway. Nix this line.

        else:
            print 'Goodbye!'
            break

This will end up falling off the end of the function, and so returning None. The rest of your script will continue, but eventually fail horribly. This probably isn't what you want.

month = 0
day = 0
year = 0
century = 0

Only reason to initialise these to 0 is to pass them into input_value, but, as I said above, it never uses the zero values. So, you don't need this.

month = input_value(month, "Please enter the month (from 1-12, where March is 1 and February is 12): ", 1, 12)

March = 1 is an odd system. Consider inputting it as January=1 and then transforming it with:

# Weirdify month counting
month = month - 2 if month < 3 else month + 10

To not surprise your users.

day = input_value(day, "Please enter the day (from 1-31): ", 1, 31)
year = input_value(year, "Please enter the year (from 0 - 99, eg. 88 in 1988): ", 0, 99)
century = input_value(century, "Please enter the century (from 0 - 99, eg. 19 in 1988): ", 0, 99)

These lines are very long. You should wrap them per PEP8:

 year = input_value("Please enter the century (from 0 - 99, eg. 19 in 1988): ",
                     0, 99)

Also, you might consider inputting year and century as a single value ('1988') and separating it in the script.

A = month
B = day
C = year
D = century

Just use the long names throughout your formulae, eg X = year / 4.

W = (13*A - 1) / 5
X = C / 4
Y = D / 4
Z = W + X + Y + B + C - 2*D
R = Z % 7

birthday = "You were born on " + str(B) + "/" + str(A+2) + "/" + str(D) + str(C) + " which was a "

Use string formatting instead of concatenation:
    birthday = "You were born on {}/{}/{}, which was a {}{}".format(day, month+2,
                                                                    century, year)

if R == 0:
    print birthday + 'Sunday'
elif R == 1:
    print birthday + 'Monday'
elif R == 2:
    print birthday + 'Tuesday'
elif R == 3:
    print birthday + 'Wednesday'
elif R == 4:
    print birthday + 'Thursday'
elif R == 5:
    print birthday + 'Friday'
elif R == 6:
    print birthday + 'Saturday'

This can go into a list:

 days = ['Sunday', 'Monday', 'Tuesday', 'Wednesday',
         'Thursday', 'Friday', 'Saturday']
 day_of_week = days[R]

And then that can go in the above string formatting instead of being concatenated on later.


jQuery nested ajax deffereds- looking for improvements Following is a working solution for a single xml ajax to retrieve category list, then individual ajax calls for each category. This was developed in response to a question on SO.

Solution creates one deffered for the category ajax call, and array of deffereds for the items ajax calls. Bottom of code nests $when(itemsCallsArray) inside $.when( catgoryCall)

Working demo

Categories should stay in order of xml  but looking for any improvements that could be used to keep the items calls in order.

/* store all ajax content in object*/
var content = {};

function parseContent(content) {
    $.each(content, function() {
        $('body').append('

' + this.a + '
' + this.b.join('
'));
    })
}
/* array to store nested ajax promises */
var deferreds_b = [];
/* A promise and ajax*/
var xml_A = $.post("/echo/xml/", {
    xml: a
}, function(returnedXML) {

    var $el_A = $(returnedXML).find("ElementA");
    var total_A = $el_A.length;
    $el_A.each(function(counter) {
        var countA = counter;
        var name_of_Attribute_of_Element_A = $(this).attr("name");
        /* create EL A object , store EL A content, create array to push B content to */
        content[countA] = {
            a: countA + ". " + name_of_Attribute_of_Element_A,
            b: []
        }

        var xml_B = $.post("/echo/xml/", {
            xml: b
        }, function(returnedXML) {

            var $el_B = $(returnedXML).find("ElementB");
            var total_B = $el_B.length;
            $el_B.each(function(idx) {
                var name_of_Attribute_of_Element_B = $(this).attr("name");
                content[countA].b.push(countA + ". " + name_of_Attribute_of_Element_B);
            });
        });
    /* push B promise to array*/
        deferreds_b.push(xml_B);

    });
});

$.when(xml_A).done(function() {
    /* when A is done , wait for B to be done*/
    $.when.apply(null, deferreds_b).done(function() {
        parseContent(content);
        $("body").append("
All done!
");
    });

})

I'm not sure what do you exactly need?
http://jsfiddle.net/tomalec/GeRkG/1/

Use promise returned by $.post. we don't need var xml_A and $.when(xml_A)
Remove deffereds_b from global scope, and use it as resolved value.

And now improvements depends on what you are trying to achive:

Just get all responses together in right order, after all is done
Do above, but performe partial operations as soon as posible, if so do you really need countA while handling B response?

1:
http://jsfiddle.net/tomalec/GeRkG/3/
 - Move B response handling outside
 - Move content to local scopes
 or even: http://jsfiddle.net/tomalec/GeRkG/4/
  - one callback, and object creation less

2:
http://jsfiddle.net/tomalec/rwq2d/2/
 - Move AB response handling outside, we can move A separately if you want to perform it sooner, than later (before B response).
 - Move content to local scopes
 - Removed closure for $.when.apply( null, requests)

function parseContent(content) {
    console.log('content',content);
    $.each(content, function() {
        $('body').append('

' + this.a + '
' + this.b.join('
'));
    })
}
function handleABResponse(returnedXML, $el_A, countA) {
    var name_of_Attribute_of_Element_A = $el_A.attr("name"),
        $el_Bs = $(returnedXML).find("ElementB"),
        results_b = [];
    $el_Bs.each(function(index, el) {
        var name_of_Attribute_of_Element_B = $(el).attr("name");
        results_b.push (countA + ". " + name_of_Attribute_of_Element_B);
    });

    return {
        a: countA + ". " + name_of_Attribute_of_Element_A,
        b: results_b
    }
}
/* Request As */
$.post("/echo/xml/", {
    xml: a
}).pipe(function(returnedXML) {
    /* process As */
    /* array to store nested ajax promises */
    var requests = [];

    var $el_As = $(returnedXML).find("ElementA");
    $el_As.each(function(countA) {
        var $el_A = $(this);
        /* push B promise to array*/
        requests.push(
            $.post("/echo/xml/", {
                xml: b
            })
            .pipe(function bSuccess (response){
                /* process A and Bs  */
                return handleABResponse(response, $el_A, countA);
            })
        );
    });
    /* return promise for all requests */
    return $.when.apply( null, requests );
}).done(function() { //content[0], content[1],..
    /* collect processed responses */
    console.warn(this,arguments);
    parseContent(arguments);
    $("body").append("
All done!
");
});


Karp-Rabin string matching in Java I wrote an implementation of the Karp-Rabin string matching algorithm in Java 7, based on the discussion in Section 32.2 of Introduction to Algorithms (CLRS). Clearly, I need more experience with numeric programming, because I spent two days wrestling with overflow issues and floating point error, but I believe it works now.

Of course, anything goes, but I'm especially looking for feedback on the following points:

Are there any cases left where it would fail?
How can I improve the design?
How can I make the code more readable?
How can I make the code more robust?
Did I miss any glaring performance traps?

Here's the class constructor and the match method, which do the bulk of the work.

public class KarpRabin {
    final long q, b;
    public static final int NONE = -1;
    Map<Character, Integer> digitMap;

    public KarpRabin(String alphabet) {
        digitMap = new HashMap<>();
        for (int i = 0; i < alphabet.length(); ++i) {
            digitMap.put(alphabet.charAt(i), i);
        }
        q = 26900927; // A prime such that b*q can reasonably fit in
                      // a word for most values of b.
        b = alphabet.length();
        if (b >= q) {
            throw new IllegalArgumentException("b cannot be larger than " + q);
        }
    }

    public int match(String pattern, String text) {
        // Returns location of first match of pattern in text.
        if (pattern.length() > text.length()) {
            throw new IllegalArgumentException("Pattern longer than text.");
        }
        int[] T = digitValue(text);
        int[] P = digitValue(pattern);
        long h, p, t;
        p = t = 0;
        h = expt(b, pattern.length()-1).mod(BigInteger.valueOf(q)).longValue(); // See note [7].

        // Calculate fingerprint of pattern and of first
        // pattern.length-length group in text.
        for (int i = 0; i < pattern.length(); ++i) {
            p = (b*p + P[i]) % q;
            t = (b*t + T[i]) % q;
        }

        for (int s = 0; s < text.length() - pattern.length(); ++s) {
            if (p == t) {
                if (pattern.equals(text.substring(s, s+pattern.length()))) {
                    return s;
                }
            }
            assert s < T.length && s + pattern.length() < T.length :
                "s is " + s + " and s+pattern.length() is " +
                (s + pattern.length());
            t = (b * (t - T[s]*h) + T[s + pattern.length()]) % q;
            // Handle Java's modulus behavior; see note[2].
            t += q;
            t %= q;
            assert t >= 0 : "t is below 0, has value " + t;
       }
       // See note [4].
       if (p == t) {
           if (pattern.equals(text.substring(
               text.length() - pattern.length(), text.length())))
               return text.length() - pattern.length();
       }
       return NONE;
}

Here's a gist with the rest of the code, including the private helper methods.

Although the current code is Java 7, I plan to switch to Java 8 in the near future, so suggestions for where I could make good use of Java 8 features is also welcome.

Clearly, I need more experience with numeric programming, because I spent two days wrestling with overflow issues and floating point error

I'd suggest to just avoid it all. Use mod \$2^{32}\$ arithmetic, ignore overflow, forget floating point and forget BigInteger.

I'm afraid, the problem is not the code, but the algorithm. I can see that the Wikipedia really describes all the strange things concerning modular arithmetic, but I'm pretty sure that you took it too literally.

For a rolling hash, all you need is something like

h = BASE_MULTIPLIER * h + inChar + OUT_MULTIPLIER * outChar;

where inChar is the incoming character, BASE_MULTIPLIER is any number co-prime to \$2^{32}\$ (i.e., odd) and OUT_MULTIPLIER is its \$n\$-th power mod \$2^{32}\$, where \$n\$ is the length of the sliding window.

If you insist on using a prime instead of \$2^{32}\$, then you can do it all without BigInteger as long as the prime fits in an int (use long and compute modulus after each operation).

If you're still unsatisfied with the hash function, then there are simple ways to improve it (just ask).

throw new IllegalArgumentException("Pattern longer than text.");

What's illegal here? It's just not there, cf. "x".contains("abc").

throw new IllegalArgumentException("b cannot be larger than " + q);

It can... IIUIC, it's just hashing, so there's no reason to whine because of collisions as long as they're rare.

  How can I make the code more robust?

Don't require to specify the alphabet and similar things. It's not necessary.

  Did I miss any glaring performance traps?

Both % and BigInteger are performance killers.

  Because of the way I implemented the algorithm, supporting general Unicode strings in UCS-2 or UTF-16 would require doing base-65,536 arithmetic.

If you implemented it my way, there'd be no problem.

  There's no hard limit on the size, but larger alphabets mean more BigInteger math.

Do no BigInteger at all.

  Although the current code is Java 7, I plan to switch to Java 8 in the near future

Just don't. If you see something where Java 8 could obviously help a lot, go for it. Otherwise, I wouldn't care. I'm not saying that Java 8 is no big improvement, it's just that sticking with Java 7 or 6 makes your program more usable.


Determine quadrant of code I'm currently in the process of coding a QuadTree.  The QuadTree seperates into four quadrants, with 1 being the top right and 4 being the bottom right.  I have this code to determine which quadrant the AABB fits into:

int getIndex(AABB aabb) {
    int index = -1;
    double vertMid = bounds.getX() + (bounds.getWidth() / 2);
    double horzMid = bounds.getY() + (bounds.getHeight() / 2);

    // Fits in top
    bool topQuadrant = aabb.getY() < horzMid && aabb.getY() + aabb.getHeight() < horzMid;
    // Fits in botom
    bool bottomQuadrant = aabb.getY() > horzMid && aabb.getY() + aabb.getHeight() < bounds.getY() + bounds.getHeight();

    // Fits in left
    if (aabb.getX() < vertMid && aabb.getX() + aabb.getWidth() < vertMid) {
        if (topQuadrant) {
            index = 1;
        } else if (bottomQuadrant) {
            index = 2;
        }
    }
    // Fits in right
    else if (aabb.getX() > vertMid && aabb.getX() + aabb.getWidth() < bounds.getX() + bounds.getWidth()) {
        if (topQuadrant) {
            index = 0;
        } else if (bottomQuadrant) {
            index = 3;
        }
    }

    return index;
}

-1 indicates it cannot be subdivided
1 indicates top right
2 indicates top left
3 indicates bottom left
4 indicates bottom right

How can I speed it up/make it cleaner? At the moment it is quite slow and I feel like there is an easier way to do it.

Note, you indicate in the text that you have quadrants:

 2 1
 3 4

But your code has the quadrants:

 1 0
 2 3

I recommend you have a function that works on just one point at a time. There are two points, the 'bottom left', and the 'top right'.

So, calculate the quadrant of each point. If they are both in the same quadrant, return the quadrant, otherwise return -1;

int getPointIndex(double horzMid, double vertMid , double x, double y) {
    int quad = 0;
    if (y < horzMid) {
        quad = x < vertMid ? 2 : 3;
    } else {
        quad = x < vertMid ? 1 : 0;
    }
    return quad;
}

Then, using this function you can:

int getIndex(AABB aabb) {

    double vertMid = bounds.getX() + (bounds.getWidth() / 2);
    double horzMid = bounds.getY() + (bounds.getHeight() / 2);

    // Quadrant of bottom-left point.
    int blQuad = getPointIndex(horzMid, vertMid, aabb.getX(), aabb.getY())
    // Quadrant of top-right point.
    int trQuad = getPointIndex(horzMid, vertMid, aabb.getX() + aabb.getWidth(), aabb.getY() + aabb.getHeight())

    if (blQuad == trQuad) {
        return blQuad;
    }

    return -1;
}


Minesweeper BombCounts I have a method which checks all of its surrounding squares and returns the number of bombs around it. But it's really long code and ugly, so can it be shorten?
final int MINE =10

for (int x = 0; x < counts.length; x++) {
    for (int y = 0; y < counts[0].length; y++) {
    if (counts[x][y] != MINE) {
    int Minesearch = 0;
    if (x > 0 && y > 0 && counts[x-1][y-1] == MINE) {//up left
        Minesearch++;
    }
    if (y > 0 && counts[x][y-1] == MINE) {//up
        Minesearch++;
    }
    if (x < counts.length - 1 && y > 0 && counts[x+1][y-1] == MINE) {//up right
        Minesearch++;
    }
    if (x > 0 && counts[x-1][y] == MINE) {//left
        Minesearch++;
    }
    if (x < counts.length - 1 && counts[x+1][y] == MINE) {//right
        Minesearch++;
    }
    if (x > 0 && y < counts[0].length - 1 && counts[x-1][y+1] == MINE) {//down left
        Minesearch++;
    }
    if (y < counts[0].length - 1 && counts[x][y+1] == MINE) {//down
        Minesearch++;
    }
    if (x < counts.length - 1 && y < counts[0].length - 1 && counts[x+1][y+1] == MINE) {//down right
        Minesearch++;
    }
    counts[x][y] = Minesearch;
    }
    }
}
}

It can be shortened and "beautified" by refactoring the bounds cyhecking and mine checking into internal method:
private boolean isWithinBounds(int x, int y) {
    return x >= 0 && y >= 0 && x < width && y < height;
}

private boolean isMine(int x, int y) {
    return field[x][y] == MINE;
}

Then the mine count becomes trivial (we can assume that the center of the 3x3 square does not have a mine, otherwise the player would have exploded and ended the game):
for (int x1 = x - 1; x1 <= x + 1; x1++) {
    for (int y1 = y - 1; y1 <= y + 1; y1++) {
        if (isWithinBounds(x1, y1) && isMine(x1, y1) {
            mineCount++;
        }
    }
}

What we have done is breaking code into methods, each of which implement a small and well defined function. Because each method does exactly one thing, they become easier to understand, maintain and test.
You should check Java naming conventions. Variable names should be in camelCase, startingWithSmallLetter.
Variable and method names should describe the reason why the code exists. E.g. mineSearch is confusing as the variable does not search mines, it just keeps count of them. Thus mineCount is a better alternative.
Counts is also confusing as it contains a value named MINE which obviously is a marker for a cell containing mine but it also contains the surrounding mine counts. I did a minesweeper clone (or actually a minesweeper solver) once and I used an array containing Cell-objects. The Cell object provided methods for querying the status of the cell (stepped on, flagged, unknown) and the number of surrounding mines if it had been stepped on.


Splitting and storing config file lines I wrote the following code to read a config file line by line and split each line into 2 parts and store them separately.  The split is done on the basis of a delimiter =:

std::ifstream configfile(path_string.c_str());
std::string line;

std::vector<std::string> linesOfConfigFile;
while (std::getline(configfile, line))
    linesOfConfigFile.push_back(line);

configfile.close();

std::string currentItem;
char delimeter = '=';

std::vector<std::string> RHS;
std::vector<long long> LHS;

bool flag = false;
for (int i =0; i<linesOfConfigFile.size(); i++)
{
    std::stringstream singleLine(linesOfConfigFile[i]);

    while(std::getline(singleLine, currentItem, delimeter))
    {
        if(flag == true)
        {
            RHS.push_back(currentItem);
            flag = false;
        }
        else if (flag == false)
        {
            long long orderbookId = 0;
            <some library class>::toNumeric(currentItem, orderbookId);
            LHS.push_back(orderbookId);
            flag = true;
        }
    }
}

for(int i=0; i<RHS.size(); i++)
    StorageMap.insert( std::pair<long long, std::string>( LHS[i], RHS[i]) );

The review comment I got was this:

  The below code can be written in a better way, Need not use a Boolean flag to differentiate RHS and LHS. The outer for loops should be sufficient.

I cannot understand what difference he wishes to have. Can someone please help me?

I wrote the following code to read a config file line by line and split each line into 2 parts and store them separately.

So there are two parts on each line, the stuff before the =, which is stored as a string, and the stuff after the =, which shall be parsed as an integer and stored as a long long.

Thus you can handle each line on its own completely, without using a flag to determine at which part of the line you currently are:

for (int i =0; i<linesOfConfigFile.size(); i++)
{
    std::stringstream singleLine(linesOfConfigFile[i]);

    // get first part of the line
    std::getline(singleLine, currentItem, delimeter);
    // store it
    RHS.push_back(currentItem);

    // Now get the next part of the line
    std::getline(singleLine, currentItem, delimeter);
    // parse it
    long long orderbookId = 0;
    <some library class>::toNumeric(currentItem, orderbookId);
    // and store the result
    LHS.push_back(orderbookId);

    // done with this line :D
}

This supposes of course that the config file is well-formed, and the format requires that each name = value is on a separate line.

If the config file is malformed, or the file format is not as simple, this version may fail where yours wouldn't (for example

name =
value

would work with yours), but neither version does robust error handling, and both would fail on most malformed inputs. Config files can generally be assumed to follow the required format, so the absence of error handling probably isn't a serious problem.


How would I make this bubble sort more pythonic? I have this code:
Sorted=False
while not Sorted:
    Sorted=True
    for x,y in enumerate(List[:-1]):
        if List[x]>List[x+1]:
            List[x],List[x+1]=List[x+1],List[x]
            Sorted=False

However the use of
Sorted=True/False

being repeated is quite ugly and it would be much nicer to write the code is something similar to:
while True:
    for x,y in enumerate(List[:-1]):
        if List[x]>List[x+1]:
            List[x],List[x+1]=List[x+1],List[x]
            break
    else:break

The only problem is that breaking from the loop this early causes the loop to be repeated many more times taking up more time overall. Are there any ways around this to make the code more pythonic or do I just need to leave it as ugly as it is?

One approach is to move most of the logic into a helper function that does the
swapping and returns the N of swaps. The outer part of the algorithm then
reduces to almost nothing.
While you're at it: (1) use conventional names for Python variables (lowercase
for variables, capitalized for classes, uppercase for constants); (2) let your
operators breathe for readability, (3) take full advantage of enumerate(),
which gives both index and value (x1), (4) use convenience variables to make
code more readable (x2 rather that repeated uses of xs[i + 1]); (5)
look for ways to pick simple variable names that help the reader understand the
algorithm (i for an index; xs for a list of values; x1 and x2 for
individual values that are being swapped) rather than purely abstract variable
names that convey nothing substantive to the reader (List, x for an
index, and y for a value); and (6) put your sort in a proper function.
def bubble_sort(xs):
    while bubble(xs):
        pass

def bubble(xs):
    n_swaps = 0
    for i, x1 in enumerate(xs[:-1]):
        x2 = xs[i + 1]
        if x1 > x2:
            xs[i] , xs[i + 1] = x2, x1
            n_swaps += 1
    return n_swaps

vals = [9, 3, 4, 2, 6, 8, 10, 1]
bubble_sort(vals)
print(vals)


Binary Search Implementation for Integer Array Purpose

Implement a binary search algorithm that takes a sorted int array and some int target value and returns the index of the
target value, if it exists.

Discussion

The way I thought about implementing a binary search algorithm was the following

Given an array of sorted int values, pick the first, last, and middle indices (round down if there is an even number of indices).
If the target value equals any of the values at the first/ last / middle index, return that index.
Else, check to see if the target value is less / greater than the middle index value.

If it's less than the middle index value, than we can assume that the target value, if it exists,
is between the first index and the middle index (since the array should be sorted). Thus, we can call our search
function on this sub-array, specifying a start index of the first index + 1 (since we already checked this)
and an end index of the middle index - 1 (again, since we already checked this).
If it's greater than the middle index value, we can assume that the target value is between the middle index and the last index.
Thus, we can call the search function on this sub-array, specifying a start index of the middle index + 1 and an
end index of the last index - 1.

We continue this process of analyzing the sub-arrays until our sub-array is empty, in which case a NoSuchElementException
is thrown.

Things that I'm unsure about

Does it make sense to have the public search method immediately call the private searchSubArray method?
The reason I did this is to obfuscate the idea of a leftIndex / rightIndex from the API user.
Does the NoSuchElementException make sense? Would returning -1 make more sense?
Any particular thoughts on my use of early returns?

Implementation

import java.util.NoSuchElementException;

public class BinarySearcher {
    /**
     * Executes a binary search for some target value given an input array of sorted values
     * @param values a sorted array
     * @param target a value to search for
     * @return the index of the target value, if it exists. If the target value does not exist, a NoSuchElementException is thrown.
     */
    public static int search(int[] values, int target) {
        return searchSubArray(values, 0, values.length - 1, target);
    }

    private static int searchSubArray(int[] values, int leftIndex, int rightIndex, int target) {
        if (values.length == 0 || leftIndex < 0 || rightIndex < 0) {
            throw new NoSuchElementException(String.format("Unable to find target: {} in values", target));
        }

        if (target == values[leftIndex]) {
            return leftIndex;
        }

        if (target == values[rightIndex]) {
            return rightIndex;
        }

        int middleIndex = Double.valueOf(Math.floor((rightIndex - leftIndex) / 2)).intValue();

        int middleValue = values[middleIndex];

        if (target < middleValue) {
            return searchSubArray(values, leftIndex + 1, middleIndex - 1, target);
        }

        else if (target > middleValue) {
            return searchSubArray(values, middleIndex + 1, rightIndex - 1, target);
        }

        else {
            return middleIndex;
        }
    }
}

Integer division does the right rounding. There is no need to bring in a double machinery.
Search and related algorithms are cleaner on the semi-open intervals (that is, the right boundary is one past the last element).
Testing first and last element looks like a premature optimization. In overwhelming number of cases it is just a waste of time.
Java does not detect and/or eliminate tail recursion. The algorithm can and should be made iterative.

Edit: tail recursion elimination.

Once you found that the recursion is really a tail one, the rest is fairly mechanical.

Rewrite the code to explicit tail form:

if (left >= right) {
    doWhateverNotFoundActionYouWant();
}

middle = left + (right - left)/2;
midValue = value[middle];

if (target < midValue) {
    right = middle;
} else if (target > midValue) {
    left = middle + 1;
} else {
    return middle;
}
return recurse(value, left, right);

Remove the recursion, and replace if with while (don't forget to invert the condition):

while (left < right) {
    middle = left + (right - left)/2;
    midValue = value[middle];

    if (target < midValue) {
        right = middle;
    else if (target > midValue) {
        left = middle + 1;
    } else {
        return middle;
    }
}


Threads with lambdas and runnable in Java I'm learning Java and I've started writing some tasks. Could you give me feedback for it?

  Write a static method andThen that takes as parameters two Runnable instances
   and returns a Runnable that runs the first, then the second. In the main method,
   pass two lambda expressions into a call to andThen , and run the returned
   instance.

public class Exercise7 {

    public static class Thread1 implements Runnable {

        @Override
        public void run() {
            System.out.println("Echo from thread1");
        }
    }

    public static class Thread2 implements Runnable {

        @Override
        public void run() {
            System.out.println("Echo from thread2");
        }
    }

    public static Runnable andThen(Runnable thread1, Runnable thread2) {

        return () -> {
            thread1.run();
            thread2.run();
        };

    }

    public static void main(String[] args) {

        Runnable thread1 = new Thread1();
        Runnable thread2 = new Thread2();

       andThen(thread1,thread2).run();
    }
}

Your code is fine, but neither the question nor your answer have anything to do with threads. In order to run two separate threads, you need to create a Thread object with your runnable, rather than just calling myRunnable.run(). The first runnable returns to the scope of your main() method before the second is executed, i.e. they are sequential and exist in a single thread.


Scrape data from a webpage and store them in a database I've written a script in python to parse different names ,bubbles and reviews of various restaurant names from tripadvisor and finally store them in a table named webdata in a database. There are two functions within my script: one for storing the scraped data in MySQL and the other for printing the same in the console. The scraper is doing its job just flawless. As this is my first ever try to store items in a database and print the already stored items in the console, I might not have been able to follow the rules of DRY.

I will vastly appreciate any modification I should pursue to make the script robust.

This is my attempt so far:

import mysql.connector
from bs4 import BeautifulSoup
import requests

URL = "https://www.tripadvisor.com.au/Restaurants-g255068-c8-Brisbane_Brisbane_Region_Queensland.html"

def store_info(link):
    mydb = mysql.connector.connect(
      host="localhost",
      user="root",
      passwd = "test123",
      database="mydatabase"
    )

    mycursor = mydb.cursor()
    mycursor.execute("DROP TABLE webdata") #kick out the table if it already exists
    mycursor.execute("CREATE TABLE webdata (name VARCHAR(255), bubble VARCHAR(255), review VARCHAR(255))")

    response = requests.get(link)
    soup = BeautifulSoup(response.text,"lxml")
    for items in soup.find_all(class_="shortSellDetails"):
        name = items.find(class_="property_title").get_text(strip=True)
        bubble = items.find(class_="ui_bubble_rating").get("alt")
        review = items.find(class_="reviewCount").get_text(strip=True)

        mycursor.execute("INSERT INTO webdata (name,bubble,review) VALUES (%s,%s,%s)",(name,bubble,review))
    mydb.commit()

def get_data():
    mydb = mysql.connector.connect(
      host="localhost",
      user="root",
      passwd = "test123",
      database="mydatabase"
    )
    mycursor = mydb.cursor()
    mycursor.execute("SELECT * FROM webdata")
    for item in mycursor.fetchall():
        print(item)
    mydb.close()

if __name__ == '__main__':
    store_info(URL)
    get_data() #retrieve the data from that table

The database setup mydb should be in it's own function, or even just
a global since this script is rather minimal.  There's no reason to
duplicate this code.
If mysql.connector doesn't, I'd look for a database connector that
supports the with ... statement for connections and transactions
(relatively sure e.g. sqlalchemy supports that).  Because you can
always forget to call commit (or rollback), but with with you
have a little bit less mental overhead.
It's a simple script ... long term I'd worry about losing data that
way.  Also why is it dropping the whole table instead of, say,
DELETE FROM webdata; and only recreating the table if it doesn't
exist?
The bs4 part looks okay and the insert too; I'd perhaps have some
checks in there that you don't overflow on the 255 limit with the
review (or the name I suppose).  Also some warnings around empty
values might be good for you to debug / notice that something's off.
AFAIK requests is a good choice, you might also want to check the
status code though.

Okay, so long-term:

To be more flexible, look at argparse or any of the command line
parsing libraries so that you never have to edit the script in case
one of the parameters changes (database connection, URL).  I'd keep
the HTML querying like it is, that's usually not wise to put into a
configuration unless it changes frequently.


Pushing data to an analytics API This is code that handles pushing data to an analytics API, and IMO it feels too much like Java.

logActivity is a method that uses Dispatch to POST the data to this API. I am worried that an exception could occur and that is why I wrapped the methods in a try/catch block. However, this feels very Javaesque.

Do I even need the try/catch block? Can logActivity method fail and cause the end point to stop working? Any better ways to do this?

  def buyLinkAnalytics(notif: Notification, reqInfo: RequestInfo): Unit = {
    try {
      logActivity(buy_link_requested, Some(LoggedInAccount(AccountId(notif.emails.get.to._id))), notif.productId.get._id.toString, None, reqInfo)
    } catch {
      case e: Exception => {
        log.error(e.toString)
      }
    }
}

  def productShareAnalytics(notif:Notification, reqInfo:RequestInfo): Unit = {
    try {
      logActivity(product_shared, Some(LoggedInAccount(AccountId(notif.emails.get.to._id))), notif.productId.get._id.toString, None, reqInfo)
    } catch {
      case e: Exception => {
        log.error(e.toString)
      }
    }
  }

  def welcomeAnalytics(notif:Notification, reqInfo:RequestInfo): Unit = {
    val uri = reqInfo.stripedUri
    try {
      logActivity(welcome, Some(LoggedInAccount(notif.accounts.get.from.get)), uri, None, reqInfo)
    } catch {
      case e: Exception => {
        log.error(e.toString)
      }
    }
  }

  def myCatalogAnalytics(notif:Notification, reqInfo:RequestInfo): Unit = {
    val uri = reqInfo.stripedUri
    try {
      logActivity(welcome, Some(LoggedInAccount(notif.accounts.get.from.get)), uri, None, reqInfo)
    } catch {
      case e: Exception => {
        log.error(e.toString)
      }
    }
  }

  def handleAnalytics(notif:Notification, reqInfo:RequestInfo) {
    notif.notification match {
      case BuyProduct => buyLinkAnalytics(notif, reqInfo)
      case ProductShare => productShareAnalytics(notif, reqInfo)
      case Welcome => welcomeAnalytics(notif, reqInfo)
      case MyCatalog => myCatalogAnalytics(notif, reqInfo)
      case _ => log.debug("Notification Type not found. Nothing put into analytics")
    }
  }

  def sendActivityLog[A] (account_id: String, activityLog: A, authParams: Map[String, String] = Map.empty[String, String], endpoint: String)(implicit fmt: Format[A]) = {
    val activityLogJson = Json.stringify (Json.toJson (activityLog))
    val scv = url(s"${config.getString("activity_log.url")}$endpoint/$account_id?key=wc_analytics") << activityLogJson <:< Map("Content-Type" -> "application/json") <:< authParams
    logger.debug(s"LOGGED: ${scv.url} << $activityLogJson")
    Http (scv OK as.String)
  }

  def logActivity (
    action      : String,
    visitor     : Option[Visitor],
    uri         : String,
    target_id   : Option[String],
    requestInfo : RequestInfo,
    context     : String = "-") = {
    import core.security.AppId.WcApp
    // In theory, no loggable business event should ever occur without an AppId.
    // But default to WC if we have to.

    val appId = requestInfo.appId.getOrElse (WcApp)._id

      val authParams = findAuthParams(requestInfo)

    if (visitor == None) {
      sendActivityLog ("None", ActivityLog (appId, action, uri, "-", context), authParams, activity_account)
    }
      visitor.foreach {
        case LoggedInAccountWithOther (account_id, other_id) =>
          sendActivityLog (
            account_id._id,
            ActivityLog (
              appId,
              action,
              uri,
              target_id.getOrElse (other_id._id),
              context
            ),
            authParams,
            activity_account
          )
        case v =>
          sendActivityLog (v.accountId._id, ActivityLog (appId, action, uri, "-", context), authParams, activity_account)
      }

  }

If you think that those try/catch blocks look too verbose, then you can create utility method like:

def logExceptions[R](body: => R): R =
  try { body }
  catch {
    case e: Exception => log.error(e.toString)
  }

Then rewrite your code like:

def buyLinkAnalytics(notif: Notification, reqInfo: RequestInfo) {
  logExceptions {
    logActivity(buy_link_requested, Some(LoggedInAccount(AccountId(notif.emails.get.to._id))), notif.productId.get._id.toString, None, reqInfo)
  }
}

def productShareAnalytics(notif:Notification, reqInfo:RequestInfo) {
  logExceptions {
    logActivity(product_shared, Some(LoggedInAccount(AccountId(notif.emails.get.to._id))), notif.productId.get._id.toString, None, reqInfo)
  }
}

...


React clock component I'm new to React.js and this code came from 30 Days of React - Day 7. However the given code example wasn't working so I rewrote it myself. My biggest issue here is how to make the component work without function getTime() using both setState and return. Any other suggestions are appreciated.

class Clock extends React.Component {
    constructor(props) {
        super(props);
        this.state = {currentTime: this.getTime()};
    }

    getTime() {
        const currentTime = new Date(),
        hours = currentTime.getHours(),
        minutes = currentTime.getMinutes(),
        seconds = currentTime.getSeconds(),
        ampm = hours >= 12 ? 'pm' : 'am';
        this.setState({currentTime: {hours, minutes, seconds, ampm}});
        return {hours, minutes, seconds, ampm};
    }

    componentDidMount() {
        const intervalId = setInterval(this.getTime.bind(this), 1000);
        this.setState({intervalId: intervalId});
    }

    componentWillDisMount() {
        clearInterval(this.state.intervalId);
    }

    render() {
        const {hours, minutes, seconds, ampm} = this.state.currentTime;
        return (

                {hours == 0 ? 12 : (hours > 12) ? hours - 12 : hours}:
                {minutes > 9 ? minutes : `0${minutes}`}:
                {seconds > 9 ? seconds : `0${seconds}`} {ampm}

        )
    }
}

ReactDOM.render(<Clock />, document.querySelector("#app"));

In think you can improve the render by following the React philosofy of writing reusable components.

If you look closer at your code:

render() {
    const {hours, minutes, seconds, ampm} = this.state.currentTime;
    return (

            {hours == 0 ? 12 : (hours > 12) ? hours - 12 : hours}:
            {minutes > 9 ? minutes : `0${minutes}`}:
            {seconds > 9 ? seconds : `0${seconds}`} {ampm}

    )
}

Here you have duplication in printing a digit. You have it with minutes and seconds, but I think hours should be printed the same way too.

You can introduce a display component as a plain function:

const PrintDigit = ({ digit }) =>
    (<React.Fragment>{digit < 9 && '0'}{digit}</React.Fragment>)

The function have props as an input parameter, that here I spread with a variable digit to document the only props expected from the component.

So your render() method could be rewritten:

render() {
    const {hours, minutes, seconds, ampm} = this.state.currentTime;
    return (

            <PrintDigit digit={hours === 0 ? 12 : hours % 12} />:
            <PrintDigit digit={minutes} />:
            <PrintDigit digit={seconds} /> {ampm}

    )
}

Is not that big improvement, but as your code is an exercise to learn React, I think is good to start thinking in that way.

The functional component could stay just in the same file of your main component or you can put it in a different file and then import where you need it.


Selective updates to immutable types There is a way in Scala to selectively change a field while making a copy of the immutable object:

case class Phone(areaCode: String, number: String) // declare type
val original = Phone("832","123-4567") // instantiate
val changed = original.copy(areaCode = "425") // get an updated copy

While in C# it looks like a real pain as usual. One need to create copy ctor and modifiers:

public class Phone
{
    Phone(Phone source)
        : this(source.AreaCode, source.Number)
    {
    }

    public Phone(string areaCode, string number)
    {
        AreaCode = areaCode;
        Number = number;
    }

    public string AreaCode { get; private set; }
    public string Number { get; private set; }

    public Phone WithAreaCode(string value) =>
        new Phone(this) { AreaCode = value };

    public Phone WithNumber(string value) =>
        new Phone(this) { Number = value };
}

We are probably supposed to charge per line…

Here is what I use to mitigate the problem. The immutable type is as easy as it could be:

public class Phone
{
    public Phone(string areaCode, string number)
    { AreaCode = areaCode; Number = number; }

    public string AreaCode { get; }
    public string Number { get; }
}

But there is an extension method to do the trick:

var original = new Phone("832", "123-4567");
var changed = original.With(p => p.AreaCode, "425");

which is defined as:

public static class Cloneable
{
    public static TSource With<TSource, TProperty>(this TSource source, Expression<Func<TSource, TProperty>> selector, TProperty value) =>
        source.With(new Binding(selector.Target(), value));

    static TSource With<TSource>(this TSource source, Binding change) =>
        (TSource)Activator.CreateInstance(typeof(TSource), source.Parameters(change));

    static object[] Parameters<TSource>(this TSource source, Binding change)
    {
        var values = source.Values().Update(change)
            .ToDictionary(b => b.Name, b => b.Value, StringComparer.OrdinalIgnoreCase);

        return typeof(TSource).GetConstructors().Single().GetParameters()
            .Select(p => values[p.Name])
            .ToArray();
    }

    static IEnumerable<Binding> Values<TSource>(this TSource source) =>
        typeof(TSource).GetProperties()
            .Select(p => new Binding(p.Name, p.GetValue(source)));

    static IEnumerable<Binding> Update(this IEnumerable<Binding> values, Binding change) =>
        values.Select(b => b.Name == change.Name ? change : b);

    static string Target<TSource, TProperty>(this Expression<Func<TSource, TProperty>> selector) =>
        selector.Body is MemberExpression member && member.Member is PropertyInfo property
            ? property.Name
            : throw new ArgumentException();

    class Binding
    {
        public Binding(string name, object value)
        { Name = name; Value = value; }

        public string Name { get; }
        public object Value { get; }
    }
}

You should be able to cut the code length and improve performance, by simplifying the algorithm. Here are some of the problems I've noticed in your implementation:

You're currently processing all of the type's properties, this is not necessary as you only need few concrete ones. There are few other problems deriving from this one.
You're creating the instance through the Activator class while you're also requesting the constructor of the class, since you already have the ConstructorInfo, it doesn't make much sense to call Activator.CreateInstance. Especially since the Activator is the slowest way to create an instance of a type. You should only stick to one of them, in this case you need information that is contained in the ConstructorInfo, I guess the choice is obvious.

A simpler version (using your Target method):

public static TSource With<TSource, TProperty>(this TSource source,
    Expression<Func<TSource, TProperty>> selector, TProperty value)
{
    var constructor = typeof(TSource).GetConstructors().Single();
    var parameters = GenerateParameters(source, constructor,
        new KeyValuePair<string, object>(selector.Target(), value)).ToArray();
    return (TSource) constructor.Invoke(parameters);
}

private static IEnumerable<object> GenerateParameters<TSource>(
    TSource source, ConstructorInfo ctor,
    KeyValuePair<string, object> parameterToModify)
{
    var type = typeof(TSource);
    foreach (var parameter in ctor.GetParameters())
    {
        if (string.Equals(parameter.Name, parameterToModify.Key, StringComparison.OrdinalIgnoreCase))
        {
            yield return parameterToModify.Value;
        }
        else
        {
            yield return type.GetProperty(parameter.Name,
                BindingFlags.IgnoreCase | BindingFlags.Public | BindingFlags.Instance)?.GetValue(source);
        }
    }
}

I'm not a fan of long ternary operators, but if you prefer that you can always switch it. I don't really like KeyValuePair<,> either, but it makes sense to keep things coupled and adding a separate class seems somewhat redundant as it will be used only in 1 occasion.

This is the cleaner approach, but if you really want to squeeze a bit more performance you can avoid the .ToArray() call by creating an object[] instead of IEnumerable<object>:

public static TSource With<TSource, TProperty>(this TSource source,
    Expression<Func<TSource, TProperty>> selector, TProperty value)
{
    var constructor = typeof(TSource).GetConstructors().Single();
    return (TSource) constructor.Invoke(GenerateParameters(source, constructor,
        new KeyValuePair<string, object>(selector.Target(), value)));
}

private static object[] GenerateParameters<TSource>(
    TSource source, ConstructorInfo ctor,
    KeyValuePair<string, object> parameterToModify)
{
    var type = typeof(TSource);
    var ctorParameters = ctor.GetParameters();
    var parameters = new object[ctorParameters.Length];
    for (int i = 0; i < parameters.Length; i++)
    {
        var ctorParameter = ctorParameters[i];
        if (string.Equals(ctorParameter.Name, parameterToModify.Key, StringComparison.OrdinalIgnoreCase))
        {
            parameters[i] = parameterToModify.Value;
        }
        else
        {
            parameters[i] = type.GetProperty(ctorParameter.Name,
                BindingFlags.IgnoreCase | BindingFlags.Public | BindingFlags.Instance)?.GetValue(source);
        }
    }
    return parameters;
}

It's a bit longer, but it's not necessarily less readable, you can pick whatever suits you better.

P.S

Perhaps some caching mechanism would be useful, to improve performance of multiple sequential calls.


Complementary multiply with carry in Rust Background and motivation

I'm a C++ developer with a passion for modern, multi-threaded and functional code. I came across Rust not so long ago and it sounded like everything I wanted my C++ code to be. Naturally, I decided to try it out.

I was working on my own C++ project that had a need for some random numbers. I thought it would be a good opportunity to use some Rust and call into it from my C++ code. After some messing about, I thought I would convert some C/C++ code to Rust from the following pages:

http://school.anhb.uwa.edu.au/personalpages/kwessen/shared/Marsaglia03.html
https://stackoverflow.com/questions/12884351/complement-multiply-with-carry-cmwc-rng-c-winmain16-compile-issue

Long story short, I ended up with the following code:

Code

#[macro_use]
extern crate lazy_static;

mod pointgen_random
{
    const CMWC_CYCLE: u32 = 4096;
    const PHI: u32 = 0x9e3779b9;

    struct ComplementaryMultiplyWithCarryGen {
        q: [u32; CMWC_CYCLE as usize],
        c: u32,
        i: usize
    }

    impl ComplementaryMultiplyWithCarryGen {

        fn new(seed: u32) -> ComplementaryMultiplyWithCarryGen {
            let mut q_init = [0; CMWC_CYCLE as usize];

            q_init[0] = seed;
            q_init[1] = seed + PHI;
            q_init[2] = seed + PHI + PHI;

            for i in 3..CMWC_CYCLE as usize{
                q_init[i] = q_init[i - 3] ^ q_init[i - 2] ^ PHI ^ seed;
            }

            return ComplementaryMultiplyWithCarryGen{q: q_init, c: 362436, i: 4095};
        }

        fn reset(&mut self, seed: u32){
            let cmwc = ComplementaryMultiplyWithCarryGen::new(seed);
            self.q = cmwc.q;
            self.i = cmwc.i;
            self.c = cmwc.c;
    }

        fn random(&mut self) -> u32{
            const A: u64 = 18782;
            const R: u32 = 0xfffffffe;

            self.i = (self.i + 1) & (CMWC_CYCLE as usize - 1);
            let t = A * self.q[self.i] as u64 + self.c as u64;

            self.c = (t >> 32) as u32;
            let mut x = (t + self.c as u64) as u32;
            if x < self.c {
                x+=1;
                self.c+=1;
            }

            self.q[self.i] = R - x;
            return self.q[self.i];
        }
    }

    use std::sync::Mutex;

    // Lazy singleton random number generator
    lazy_static!{
            static ref GLOBAL_CMWC_GEN: Mutex<ComplementaryMultiplyWithCarryGen> = Mutex::new(ComplementaryMultiplyWithCarryGen::new(0));
    }

    pub fn get_random_cmwc() -> u32
    {
        GLOBAL_CMWC_GEN.lock().unwrap().random()
    }

    pub fn set_cmwc_seed(seed: u32)
    {
        GLOBAL_CMWC_GEN.lock().unwrap().reset(seed);
    }
}

// Public exports for PointGenLib
// All prefixed with pointgen_ to avoid name conflicts
#[no_mangle]
pub extern "C" fn pointgen_set_cmwc_seed(seed: u32)
{
    pointgen_random::set_cmwc_seed(seed);
}

#[no_mangle]
pub extern "C" fn pointgen_random_cmwc() -> u32
{
    return pointgen_random::get_random_cmwc();
}

It compiles on Windows without warning and appears to give some sensible pseudo-random numbers from 0 to 2^32, but there's just a lot of things in it that I don't like or don't feel right:

In fn new(), having q_init be mut seems like a hack, is there a more functional way to do this using slices?
Is this an appropriate struct to use the Default trait? I read that it's not recommended to use that.
There's a lot of as usize around, I don't quite have the feel for casting types as I would do in C++.
fn reset() seems ridiculous, is there a better way to reassign all values at once?

Any other help/suggestions/advice you can give would be very much appreciated.

Run rustfmt.

When the preceding thing fits on a single line, curly braces go on the same line:

pub fn get_random_cmwc() -> u32 {

Binary operators have space on both sides:

self.c += 1;

Curly braces always have whitespace around them:

for i in 3..CMWC_CYCLE as usize {

Run clippy:

warning: unneeded return statement
  --> src/lib.rs:55:13
   |
55 |             return self.q[self.i];
   |             ^^^^^^^^^^^^^^^^^^^^^^ help: remove `return` as shown: `self.q[self.i]`
   |
   = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#needless_return

warning: the loop variable `i` is used to index `q_init`
  --> src/lib.rs:22:13
   |
22 | /             for i in 3..CMWC_CYCLE as usize {
23 | |                 q_init[i] = q_init[i - 3] ^ q_init[i - 2] ^ PHI ^ seed;
24 | |             }
   | |_____________^
   |
   = note: #[warn(needless_range_loop)] on by default
   = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#needless_range_loop
help: consider using an iterator
   |             for (i, <item>) in q_init.iter().enumerate().take(CMWC_CYCLE as usize).skip(3) {

You really should have some tests. Without them, you cannot tell that refactoring hasn't changed anything, or that you are producing the same value as other implementations of the algorithm. Also, running even a single test would have shown..
You need to think about integer overflow semantics! By default, Rust will panic if an integer overflows in debug mode. Setting q[2] in the initializer causes such an overflow!
The module is providing no obvious benefit. Since you already have the namespace of the crate, just use that directly.
use statements belong at the top of the file, unless they are scoped within a method.
CMWC_CYCLE is always used as a usize, so just make the constant a usize.
Many of the places that CMWC_CYCLE is used could be replaced with an equivalent call to .len().
Your struct takes over 16K of space, which seems very large for the stack. You may wish to consider switching q to a vector.
Instead of Clippy's suggestion, I might slice out a little mutable window of q. To me, this better expresses the initialization logic.
The initial value of i really looks likes CMWC_CYCLE - 1
(self.i + 1) & (CMWC_CYCLE - 1) looks to be a cutesy way of saying (self.i + 1) % CMWC_CYCLE. If so, I'd rely on the optimizer to do the right thing.
Your reset is just replacing self with the result of new, so its usefulness seems low. See mem::swap.
I really don't like global state. If your C code needs to have it, I'd only use the singleton in those two exported functions, inlining the Rust functions. If Rust code calling this needs to create a singleton, it should do so itself.
If you really want to solidly avoid name conflicts, you should add the major and potentially the minor version of your library to any extern "C" functions in addition to the library name. Otherwise you cannot link in multiple conflicting major versions.
Avoid usage of unwrap. Generally, you should favor handling the error. In terminal cases, prefer to use expect so that when the impossible case happens, you have a hint where the problem was.
In general, you often want to split the Rust code and the exposed C shims into different crates. In this case, only the C shims need lazy-static.

#[macro_use]
extern crate lazy_static;

const CMWC_CYCLE: usize = 4096;
const PHI: u32 = 0x9e3779b9;

use std::sync::Mutex;

struct ComplementaryMultiplyWithCarryGen {
    q: [u32; CMWC_CYCLE],
    c: u32,
    i: usize,
}

impl ComplementaryMultiplyWithCarryGen {
    fn new(seed: u32) -> ComplementaryMultiplyWithCarryGen {
        let mut q = [0; CMWC_CYCLE];

        q[0] = seed;
        q[1] = seed.wrapping_add(PHI);
        q[2] = seed.wrapping_add(PHI).wrapping_add(PHI);

        for i in 3..CMWC_CYCLE {
            let window = &mut q[i - 3..i + 1];
            window[3] = window[0] ^ window[1] ^ PHI ^ seed;
        }

        ComplementaryMultiplyWithCarryGen {
            q: q,
            c: 362436,
            i: 4095,
        }
    }

    fn reset(&mut self, seed: u32) {
        *self = ComplementaryMultiplyWithCarryGen::new(seed);
    }

    fn random(&mut self) -> u32 {
        const A: u64 = 18782;
        const R: u32 = 0xfffffffe;

        self.i = (self.i + 1) & (CMWC_CYCLE - 1);
        let t = A * self.q[self.i] as u64 + self.c as u64;

        self.c = (t >> 32) as u32;
        let mut x = (t + self.c as u64) as u32;
        if x < self.c {
            x += 1;
            self.c += 1;
        }

        self.q[self.i] = R - x;
        self.q[self.i]
    }
}

// Lazy singleton random number generator
lazy_static!{
    static ref GLOBAL_CMWC_GEN: Mutex<ComplementaryMultiplyWithCarryGen> =
        Mutex::new(ComplementaryMultiplyWithCarryGen::new(0));
}

// Public exports for PointGenLib
// All prefixed with pointgen_ to avoid name conflicts
#[no_mangle]
pub extern "C" fn pointgen_set_cmwc_seed(seed: u32) {
    GLOBAL_CMWC_GEN.lock()
        .expect("Cannot lock to set seed")
        .reset(seed);
}

#[no_mangle]
pub extern "C" fn pointgen_random_cmwc() -> u32 {
    GLOBAL_CMWC_GEN.lock()
        .expect("Cannot lock to produce number")
        .random()
}

#[cfg(test)]
mod test {
    use super::*;

    #[test]
    fn i_didnt_change_anything() {
        let mut rng = ComplementaryMultiplyWithCarryGen::new(0);
        let n: Vec<_> = (0..10).map(|_| rng.random()).collect();
        assert_eq!(n, [4294604858, 367747001, 735501178, 4294962861, 735512785, 3666536092, 3666528614, 4294955383, 367747001, 735501178]);
    }
}

  having q_init be mut seems like a hack

It's not, really. The trick is that an entire array has to be guaranteed to be initialized. There's no way for the compiler to tell that some later code will finish initializing the array, especially in the presence of panics. Thus you need to initialize the array with something and then go back and fill in the real values. As mentioned above, you could also switch to using a Vec. Since a Vec has a dynamic size, you can construct it from an iterator, allowing you to avoid mutability at the call site.

  Is this an appropriate struct to use the Default trait? I read that it's not recommended to use that.

Where did you read that? I'd like to check out to understand why it says so.

In general, Default is fine — so long as there's a reasonable default! Does a random number generator have a reasonable default? My gut feeling is "yes", but only if you have access to a true RNG that you can use to seed yourself. If you don't have access to a true source of entropy, then not implementing Default is a sign to the user that it's up to them.

  There's a lot of as usize around, I don't quite have the feel for casting types as I would do in C++.

Rust generally requires you to be more explicit. After the previous change to CMWC_CYCLE's type, there are a lot fewer of these. The remaining ones deal with cases where you are adding values to a u32 that will exceed the capacity and thus you temporarily need a u64. This seems like nice indication that something "non traditional" is happening there.


Calculate subtotals and totals from a form with many decimal values I've got 17 currency fields on a form and need to do some calculations with their values. I wrote this method some time ago and have come back to try and refactor it to make it more efficient and/or more readable. Could I have some pointers please?

Note: The variables appended with d are the decimals used to hold the field values, their counterparts without the d are the fields themselves. I am aware that variables could do with renaming to make more sense. Hopefully it's fairly clear what this is doing by looking at the comments.

private void getTotals() {

    //declarations of decimal variables
    decimal curPurc1d; decimal curPurc2d; decimal curPurc3d; decimal curPurc4d; //purItemxCost
    decimal curItem1Totd; decimal curItem2Totd; decimal curItem3Totd; decimal curItem4Totd; //curItemxTot
    decimal LessItem1Costd; decimal LessItem2Costd; decimal LessItem3Costd; decimal LessItem4Costd; decimal LessItem5Costd; //LessItemxCost
    decimal ditem1Cost = 0; decimal ditem2Cost = 0; decimal ditem3Cost = 0; decimal ditem4Cost = 0; //Full cost of items

    //Check if we have some valid numbers, stop if we don't
    if (   !decimal.TryParse(curPurc1.Value.ToString(), out curPurc1d)
        || !decimal.TryParse(curPurc2.Value.ToString(), out curPurc2d)
        || !decimal.TryParse(curPurc3.Value.ToString(), out curPurc3d)
        || !decimal.TryParse(curPurc4.Value.ToString(), out curPurc4d)
        || !decimal.TryParse(curItem1Tot.Value.ToString(), out curItem1Totd)
        || !decimal.TryParse(curItem2Tot.Value.ToString(), out curItem2Totd)
        || !decimal.TryParse(curItem3Tot.Value.ToString(), out curItem3Totd)
        || !decimal.TryParse(curItem4Tot.Value.ToString(), out curItem4Totd)
        || !decimal.TryParse(LessItem1Cost.Value.ToString(), out LessItem1Costd)
        || !decimal.TryParse(LessItem2Cost.Value.ToString(), out LessItem2Costd)
        || !decimal.TryParse(LessItem3Cost.Value.ToString(), out LessItem3Costd)
        || !decimal.TryParse(LessItem4Cost.Value.ToString(), out LessItem4Costd)
        || !decimal.TryParse(LessItem5Cost.Value.ToString(), out LessItem5Costd)
        )
        return;

        //For each of the 4 'items', try to get the value as a decimal, but only if the related checkbox is checked.
        if (Item1RateYN.Checked)
        {
            decimal.TryParse(curItem1Cost.Value.ToString(), out ditem1Cost);
        }
        if (Item2RateYN.Checked)
        {
            decimal.TryParse(curItem2Cost.Value.ToString(), out ditem2Cost);
        }
        if (Item3RateYN.Checked)
        {
            decimal.TryParse(curItem3Cost.Value.ToString(), out ditem3Cost);
        }
        if (Item4RateYN.Checked)
        {
            decimal.TryParse(curItem4Cost.Value.ToString(), out ditem3Cost);
        }

    //Add up some values which are always part of the subtotal and then the ditemx ones, which will be 0 or set to a numerical value depending if the checkbox is checked
    decimal subtotals = curPurc1d + curPurc2d + curPurc3d + curPurc4d + curItem1Totd + curItem2Totd + curItem3Totd + curItem4Totd
                      + ditem1Cost + ditem2Cost + ditem3Cost + ditem4Cost;
    subtotal.Value = subtotals;

    //Get total minus the cost of the property (curPurc1d)
    costPlusSubTotal.Value = subtotals - curPurc1d;

    //add up all the "less" items to know how much to reduce by
    decimal lessTotals = LessItem1Costd + LessItem2Costd + LessItem3Costd + LessItem4Costd + LessItem5Costd;
    totalLess.Value = lessTotals;

    //Total Balance due
    //subtotal minus the 'less' values total
    decimal total = (subtotals - lessTotals);
    //set the final figure into the relevant field
    balanceDueTot.Value = total;
}

Two words: loops and arrays.

There a bunch of variable names that differ only by a number. Whenever you see code like this, you can probably clean it up by creating a collection of those things and looping over them. Basically, you have a checkbox and text field repeated multiple times, and one field where you display the total of all checked fields.

Create a user control to encapsulate the checkbox and text field
Make sure this user control has a public property decimal TotalCost { get } that will:

Return the decimal-parsed values of total - cost for the fields when checked, and zero when unchecked.
Throws an exception if the decimal cannot be parsed

Expose a boolean property bool IsValid that returns true when the user as entered a valid decimals
Expose a public property bool IsChecked that returns whether or not the checkbox is checked
Create a collection of these user controls, one for each checkbox/text field combo on screen

Now loop and process:

decimal purchaseTotal = 0m;
decimal totalAmount = 0m;

foreach (var control in PurchasedItems.Where(p => p.IsChecked && p.IsValid))
{
    purchaseTotal += control.TotalCost;
}

I left some details out, but your code isn't really clear on what the UI looks like, or what the business use case is, but it really just boils down to:

Create an abstraction for what each combo of controls represents (e.g. create a new user control)
Create a collection of these controls
Loop and calculate


Invert-range operation in a circular vector I implemented a invert-range operation in a (logical) circular vector. A circular vector is a vector without a logical starting and ending object, or, equivalently, the successive element of the element at the last index of the vector is the element at index 0 (similarly for the previous element).

I'm not 100% sure it's correct. If it's correct, any suggestions to improve it (especially in terms of performance)?

#include <cmath>

...

vector<SomeObject> vec;

...

/*
 * Invert the segment starting at object at index i and ending at object at index j.
 * */
void invert(const int i, const int j) {

    // assert i and j are in the bounds

    if (vec.size() < 2 || i == j) {
        return;
    }

    int segment_size;

    if(j < i ){
        segment_size = std::ceil(((vec.size() - i) + j) / 2.0);
    } else {
        segment_size = std::ceil(std::abs(j - i) / 2.0);
    }

    for (int x = 0, k = i, w = j; x < segment_size; ++x) {
        std::swap(vec[k], vec[w]);
        k = next(k);
        w = previous(w);
    }

}

where next and previous are the following procedures:

/*
 * Returns the index to the object after the object at index i.
 * */
int next(const int i) {
    return (i + 1) % vec.size();
}

/*
 * Returns the index to the object before the object at index i.
 * */
int previous(const int i) {
    return (i - 1) % vec.size();
}

Note: these functions were actually methods, i.e. part of a class, and they actually working on a field and not on, e.g., global variable.

Bug

Your previous() function doesn't wrap around well:

int previous(const int i) {
    return (i - 1) % vec.size();
}

When i reaches 0, this function will return -1, not vec.size() - 1 as you might be expecting.  You could either go with:

int previous(const int i) {
    return (i - 1 + vec.size()) % vec.size();
}

or perhaps:

int previous(const int i) {
    if (i == 0)
        return vec.size() - 1;
    return i - 1;
}

Unnecessary use of float

The Floating Point Police™ disapprove of your unnecessary use of floating point here:

if(j < i ){
    segment_size = std::ceil(((vec.size() - i) + j) / 2.0);
} else {
    segment_size = std::ceil(std::abs(j - i) / 2.0);
}

All you are doing is rounding up, so you could just do:

if(j < i ){
    segment_size = (vec.size() - i) + j;
} else {
    segment_size = j - i;
}
segment_size = (segment_size + 1) / 2;

Technically, there could be a problem with the above if your segment size can be INT_MAX.  If this is possible, then replace that last line with this instead:

segment_size = (segment_size / 2) + (segment_size & 1);

But if your segment size could be INT_MAX, then your next() function would need to be fixed as well.


Fibonacci generator python implementation I wrote this code for a fibonacci sequence generator. Can anyone tell me whether my code is designed well? if not, why?
import logging

def fibonacci_calc(current_val, current_pos=0, max_seq_length=5, sequence_list=[]):
    try:
        if(current_pos != 0 and len(sequence_list) == 0):
            raise Exception("Please initalize {} argument: {} with 0".format(fibonacci_calc.__name__, fibonacci_calc.__code__.co_varnames[1]))
        else:
            if(len(sequence_list) == 0):
                print("Initialization, added 0 and starting number {} to sequence list".format(current_val))
                sequence_list.append(0)
                sequence_list.append(current_val)
                current_pos = 1
                print(sequence_list)
                fibonacci_calc(current_val, current_pos, max_seq_length, sequence_list)
            elif(len(sequence_list) == max_seq_length):
                print('{} values in the sequence'.format(max_seq_length))
                return
            else:
                print("Current position: {}, Total values: {}  calculating next value".format(current_pos, current_pos+1))
                next_val = sequence_list[current_pos] + sequence_list[current_pos-1]
                sequence_list.append(next_val)
                print("Current sequence list {}".format(sequence_list))
                fibonacci_calc(next_val, current_pos+1, max_seq_length, sequence_list)
    except Exception:
        logging.exception('ERROR')

fibonacci_calc(19,0, 7)

tldr; I'd say that this function is too complicated.
Recursion should be avoided when possible as an iterative method is usually more readable and could be faster. Here's some other things to consider when making a recursive function: https://gist.github.com/abinoda/5593052#when-to-use-recursion-vs-iteration.
I think that some of the naming is not as clear as it could be, although that could just be an effect of the function being recursive. Consider changing parameter max_seq_length to seq_length. I couldn't quite see any scenario where you would give less results than the value passed in to max_seq_length.
Another item that could use some thought is your use of raising an exception when validating your input:
if(current_pos != 0 and len(sequence_list) == 0):
    raise Exception("Please initalize {} argument: {} with 0".format(fibonacci_calc.__name__, fibonacci_calc.__code__.co_varnames[1]))

Here, you would likely be better off just logging a message instead of introducing more overhead by using an exception.
Here's an example of a function that addresses my concerns:
def get_fibonacci_sequence(start_num, seq_length):
    if (seq_length < 1):
        return []
    elif (seq_length == 1):
        return [start_num]
    elif (seq_length == 2):
        return [start_num, start_num]

    fibonacci_seq = [start_num, start_num]

    for i in range(2, seq_length):
        fibonacci_seq.append(fibonacci_seq[i - 1] + fibonacci_seq[i - 2])

    return fibonacci_seq

print(get_fibonacci_sequence(19, 7))

PS. I'm assuming all the print statements are just debugging aids and wouldn't be included in the final version. Those messages are likely superfluous for other purposes.


Rotate a string of integers to match index t is number of tests; each test contains two lines: n and sequence
n is number of integers
sequence is a sequence of integers from 1..n (e.g. '3 6 1 2 4 5' if n is 6)
Rotate sequence until you maximize the number of integers that match their position in the string. (e.g. '3 6 1 2 4 5' should output '1 2 4 5 3 6'). Print the answer sequence for each test.

Here is the working code. It seems to satisfy the conditions above.
Would be very grateful for any guidance to make it more efficient!
def rotate_left(seq):
    return seq[1:] + [seq[0]]

def is_in_position(seq):
    in_position = 0
    for idx, val in enumerate(seq, start=1):
        if int(val) == idx:
            in_position += 1
    return in_position

t = int(raw_input()) #number of tests
while t:
    n = int(raw_input()) #number of integers
    sequence = str(raw_input()).split() #sequence of integers from 1..n

    if not n:
        break

    max_score, answer_sequence = 0, 0

    for i in range(n):
        sequence = rotate_left(sequence)
        sequence_score = is_in_position(sequence)
        if max_score < sequence_score:
            max_score = sequence_score
            answer_sequence = sequence
            answer_sequence = ' '.join(answer_sequence)
        if max_score >= 0.5 * len(sequence):
            break
    if answer_sequence:
        print(answer_sequence)
    t -= 1

Variable name

If you were to give t a different name, you wouldn't need to comment number of tests. I suggest nb_tests.

Looping - the pythonic way

There are better ways to write :

nb_tests = int(raw_input())
while nb_tests:
    ...
    nb_tests -= 1

You could use a simple for loop.

nb_tests = int(raw_input())
for _ in range(nb_tests):

(I've used _ as a variable name because this is the convention for unused variables in Python).

Split the logic into smaller functions

At the moment, the logic related to input/output is mixed with the logic related to the actual algorithm. It would make things clearer to write proper functions to handle this properly.

def get_rotate_sequence_with_best_match(sequence):
    max_score, answer_sequence = 0, []
    n = len(sequence)
    for i in range(n):
        sequence = rotate_left(sequence)
        sequence_score = is_in_position(sequence)
        if max_score < sequence_score:
            max_score = sequence_score
            answer_sequence = sequence
        if max_score >= 0.5 * len(sequence):
            break
    return answer_sequence

nb_tests = int(raw_input())
for _ in range(nb_tests):
    n = int(raw_input())
    sequence = str(raw_input()).split()
    assert len(sequence) == n
    print(' '.join(get_rotate_sequence_with_best_match(sequence)))

See also : separation of concerns.

if __name__ == "__main__":

It is a good habit to use a if __name__ == "__main__": guard to separate your function definitions to what you actually run when you call your script. It is useful when you want to re-use your code via module import.

Tests

Now that you've re-organised a bit your code, it is much easier to write and run unit tests to ensure everything is working properly.

Based on the provided example, I have :

def run_test_on_stdin():
    nb_tests = int(raw_input())
    for _ in range(nb_tests):
        n = int(raw_input())
        sequence = str(raw_input()).split()
        assert len(sequence) == n
        print(' '.join(get_rotate_sequence_with_best_match(sequence)))

def run_unit_tests():
    seq = [3, 6, 1, 2, 4, 5]
    assert get_rotate_sequence_with_best_match(seq) == [1, 2, 4, 5, 3, 6]

if __name__ == "__main__":
    run_unit_tests()

Counting - the pythonic way

You could use sum(iterable[, start]) in is_in_position. Indeed, you could write :

def is_in_position(seq):
    return sum(int(val) == idx for idx, val in enumerate(seq, start=1))

Micro-optimisations

You don't need to always check if max_score >= 0.5 * len(sequence): : you just need to check it after updating max_score. Also, you could reuse n instead of calling len every time.

def get_rotate_sequence_with_best_match(sequence):
    max_score, answer_sequence = 0, []
    n = len(sequence)
    for i in range(n):
        sequence = rotate_left(sequence)
        sequence_score = is_in_position(sequence)
        if max_score < sequence_score:
            max_score, answer_sequence = sequence_score, sequence
            if max_score >= 0.5 * n:
                break
    return answer_sequence

Different algorithm

Generating n sequences and counting how many of the n elements are in the right position gives you an 0(n²) algorithm (at best).

A different strategy could be to look at the shift required for each number to be in the correct position. Because all numbers are in 1...n, you can compute the shift with a simple substraction and a modulus operation. Doing this for all numbers can be easily done :

print([(i - val) % n for i, val in enumerate(sequence, start=1)])

Then, you just need to see which elements occurs more often which can be done with collection.Counter.most_common.

from collections import Counter

def rotate_left(seq, shift=1):
    return seq[shift:] + seq[:shift]

def get_rotate_sequence_with_best_match(sequence):
    n = len(sequence)
    shifts = Counter([(i - val) % n for i, val in enumerate(sequence, start=1)])
    best_shift, nb_occur = shifts.most_common(1)[0]
    return rotate_left(sequence, best_shift)


Calculating the number 8 chars long which contains all possible numbers containing the numbers from 0 to n I have the following code:

    Integer cr = 3;
    String y = "\"\r\n\"";

    for (Integer i = 0; i < cr; i++)
    {
        for (Integer j = 0; j < cr; j++)
        {
            for (Integer k = 0; k < cr; k++)
            {
                for (Integer l = 0; l < cr; l++)
                {
                    for (Integer m = 0; m < cr; m++)
                    {
                        for (Integer n = 0; n < cr; n++)
                        {
                            for (Integer o = 0; o < cr; o++)
                            {
                                for (Integer p = 0; p < cr; p++)
                                {
                                    consolus.append(i.toString()+j.toString()+k.toString()+l.toString()+m.toString()+n.toString()+o.toString()+p.toString() + y);
                                }
                            }
                        }
                    }
                }
            }
        }
    }

Is there some way I can write this more efficiently? Essentially, the output is a number 8 chars long which contains all possible numbers containing the numbers from 0 to cr. This method currently works, however it doesn't seem efficient, and then writing to the TextView consolus, only occurs after all the for statements complete.

Presumably you want to avoid the 8 levels of nested loops?  (This isn't an efficiency thing, merely a code cleanliness issue.)

If so, use a length-8 array of integers, and use it to do "counting" in a while loop.

int[] cnt = new int[8];

while (1) {

    // ... Do something ...

    // Update
    for (int i = 0; i < 8; i++) {
        cnt[i]++;
        if (cnt[i] != cr) break;
        cnt[i] = 0;
    }
}

I'll leave termination of the while loop as an exercise for the reader...

Note: If you find this pattern occurring all over the place in your code, you could be cunning and put the update logic into a helper class.  And then your outer loop could become:

for (Counter c = new Counter(8,cr); c.isActive(); c.increment()) {
    ...
}


Get jQuery object from table row click Check out the fiddle here.  This JavaScript will take the row that a user clicks on and make an object with the key coming from the thead element in the table and the value coming from the data in the text in the table cell value.  I'm looking for a more professional/succint way of doing this:

JS

$(document).ready(function () {
            $('tr').click(function () {
                var tableData = $(this).children('td').map(function () {
                    return $(this).text();
                }).get();
                var props = $('thead > tr th');
                var array = [];
                props.each(function () { array.push($(this).text()) });
                //keys
                console.log(array);
                //values
                console.log(tableData);

                var obj = {};
                for (var i = 0; i < tableData.length; i++) {
                    obj[array[i]] = tableData[i];
                }
                console.log(obj);
            });

        });

HTML

                PersonId	FirstName	LastName

                1	John	Doe

                2	Jane	Doe

                3	Time	Smith

The selector $('thead > tr th') is not specific enough, if you have multiple tables in the page.  You want the headings of the same table that was clicked: $(this).closest('table').find('thead > tr th').

var array is an unsatisfying name.  I can see that it's an array — but what does it do?  How about calling it tableHeadings?

You formed tableData using Array.map(); why not do the headings the same way?

var tableHeadings = $(this).closest('table')
                           .find('thead > tr th')
                           .map(function() {
    return $(this).text();
}).get();


Selecting countries with different situations I'm trying to select two groups of countries with different situations.

SELECT cou.id,
       cou.title
FROM   continents con
       RIGHT OUTER JOIN countries cou
                     ON con.id = cou.continents_id
WHERE  con.deliver = 1
       AND con.active = 1
UNION ALL
SELECT cou2.id,
       cou2.title
FROM   countries cou2
       LEFT OUTER JOIN continents con2
                    ON con2.id = cou2.continents_id
WHERE  cou2.deliver = 1
       AND con2.deliver = 0

Continent has to be deliver = 1 and then select all countries inside it.
If Continent is deliver = 0 but there are countries with deliver = 1 inside, then select it.

What is the best way of doing this?

I have formatted the queries. I am not able to test it, so if you can give the schema for this, or create it at SQL fiddle, that will be great. Data is not required.

SELECT cou.id,
       cou.title
FROM   countries cou
       LEFT OUTER JOIN continents con
                     ON  cou.continents_id = con.id
WHERE  (con.deliver = 1 AND con.active = 1 ) OR
       (cou.deliver = 1 AND cou.deliver = 0)

Please let me know in case there are any concerns.


Class to help dealing with files, with lazy properties I have the following class to help me deal with files:

File class:

# -*- coding: utf-8 -*-

"""
file
~~~~~~~~~~~~~~~~
"""

from mimetypes    import MimeTypes
from os           import path
from lazyproperty import lazyproperty

class File(object):
    """Class to help dealing with files.
    """

    def __init__(self, path):
        """The constructor function.
        """
        self.path = path

    @lazyproperty
    def basename(self):
        """Return last portion of the file.
        """
        return path.basename(self.path)

    @lazyproperty
    def extension(self):
        """The extension name of the file, from the last occurrence of the
        . (period) character to end of string.
        """
        remaining, extension = path.splitext(self.path)
        return extension

    @lazyproperty
    def encoding(self):
        """The encoding of the file, if no encoding is found, None will be
        the default.
        """
        return self.__mime_type_and_encoding[1]

    @lazyproperty
    def abspath(self):
        """The absolute path of the file.
        """
        return path.abspath(self.path)

    @lazyproperty
    def mime_type(self):
        """The mime type associated with the file, if no mime type is found,
        None will be the default.
        """
        return self.__mime_type_and_encoding[0]

    # @private
    @lazyproperty
    def __mime_type_and_encoding(self):
        """Guess the type of a file based on its filename or URL, given by url.
        The return value is a tuple (type, encoding) where type is None if the
        type can’t be guessed (missing or unknown suffix) or a string of the
        form 'type/subtype', usable for a MIME content-type header.
        """
        return MimeTypes().guess_type(self.abspath)

lazyloading class:

# -*- coding: utf-8 -*-

"""
lazyproperty
~~~~~~~~~~~~~~~~
"""

class lazyproperty(object):
    """Class to create lazy properties.
    """

    def __init__(self, func):
        """The constructor function.
        """
        self.func = func

    def __get__(self, instance, cls):
        if instance is None: return self
        value = self.func(instance)
        setattr(instance, self.func.__name__, value)
        return value

I would like to improve the lazyproperties mime_type and encoding, this two parts:

return self.__mime_type_and_encoding[1]
return self.__mime_type_and_encoding[0]

Are making my sad, this magic numbers are very bad for maintenance. What i'm doing:

I'm using python 2.5 because i'm working in this legacy code, and the only way to get the mime type of file in this version is using the MimeTypes class:

mime_type = MimeTypes().guess_type("path/to/file")

Returns a tuple, -> ("mime_type", "encoding")

Which doesn't make any sense, since i'm asking for the  mime type, not the encoding... any way... so, i created the __mime_type_and_encoding lazyproperty to avoid calling the MimeTypes() two times, in the encoding and mime_type.

Is it possible to avoid the magic numbers, and still do what i want?

In Python it's conventional to use a single underscore to indicate that a method is only for internal use (see PEP8). The double underscore is intended to solve a particular problem, namely avoiding conflict with a method of the same name in another class, when the classes are combined via inheritance. Since you don't have to worry about that problem here, you don't need the double underscore.
In Python 2.6 or later, a simple way to avoid magic numbers would be to make a collections.namedtuple:

from collections import namedtuple

MimeTypeAndEncoding = namedtuple('MimeTypeAndEncoding', 'type encoding')

and then in _mime_type_and_encoding write:

return MimeTypeAndEncoding(*MimeTypes().guess_type(self.abspath))

and in mime_type:

return self._mime_type_and_encoding.type

But this won't work for you because you are stuck on 2.5. But you could do much the same thing (if a bit more long-windedly) using a class:

class MimeTypeAndEncoding(object):
    def __init__(self, type, encoding):
        self.type = type
        self.encoding = encoding

In Python 3.2 or later you could implement lazy_property like this, using functools.lru_cache:

from functools import lru_cache

def lazy_property(method):
    "Decorator for a property that caches the result of the first call."
    return property(lru_cache(1)(method))

(This won't work for you because you're stuck on Python 2.5, but I think it's neat.)


How to attach click to header child without reusing the code? How can I optimize my jQuery?  I remove the extra code to attach click event to the header tags.

$("#dnn_htmlPan1.htmlPan").find(":header").click(function () {
    $("#dnn_htmlPan1").find("p").slideToggle("slow");
});
$("#dnn_htmlPan2.htmlPan").find(":header").click(function () {
    $("#dnn_htmlPan2").find("p").slideToggle("slow");
});
$("#dnn_htmlPan3.htmlPan").find(":header").click(function () {
    $("#dnn_htmlPan3").find("p").slideToggle("slow");
});
$("#dnn_htmlPan4.htmlPan").find(":header").click(function () {
    $("#dnn_htmlPan4").find("p").slideToggle("slow");

});
$("#dnn_htmlPan5.htmlPan").find(":header").click(function () {
    $("#dnn_htmlPan5").find("p").slideToggle("slow");

});

Without knowing what your elements are or the html around them, i would say just combine the id's into one selector and use $(this).find in the click function. Example:

$("#id1, #id2, #id3").find(':header').click(function(){
 $(this).find('p').slideToggle('slow')l
});


Python Console App | Is there a better way to go back a page? I have a system for going back to a previously-visited page in a Python console application; it works well enough, but I feel that there could be a prettier way of doing it. Does anyone have any suggestions to improve what I have below?
def main():
    try:
        while True:
            print('Main Menu\n')

            option = input('page 1, page 2, quit\n')

            if option in ['1', 'one']:
                menu_stack.append(0)
                page_one()
            elif option in ['2', 'two']:
                menu_stack.append(0)
                page_two()
            elif option in ['q', 'quit']:
                quit()
            else:
                print('Invalid input, please try again\n')
                sleep(1)

    except KeyboardInterrupt:
        quit()

Very similar to the main menu page, below is page 1:
def page_one():
    while True:
        clear_terminal()

        print('Page One\n')

        option = input('Page 2, back\n')

        if option in ['2', 'two']:
            menu_stack.append(1)
            page_two()
        elif option in ['b', 'back']:
            menu_checker()
        else:
            print('Invalid input, please try again\n')
            sleep(1)

menu_checker() calls the other pages based on what pops from the stack:
def menu_checker():
    page = menu_stack.pop()

    if page == 1:
        page_one()
    elif page == 2:
        page_two()
    elif page == 0:
        main()
    else:
        print('Error')

Does anyone have any better ideas/solutions? Even though it doesn't cause any issues (as far as I am aware), I feel what I have is kind of clunky and could be improved.

I'm thinking there are a few different approaches you could take here.
The stack was inside you all along
It may not be the most flexible of approaches, but the way things are right now, your pages are just functions. And guess what? Your program already has a stack of functions -- you navigate it by calling functions and returning from them! You don't get a whole lot of information about where you'll end up if you return, but if that's not a problem, you might be able to just get rid of menu_checker and menu_stack altogether -- if the user wants to go back, you just return.
For example, your page_one could simply look like
def page_one():
    while True:
        clear_terminal()

        print('Page One\n')

        option = input('Page 2, back\n')

        if option in ['2', 'two']:
            page_two()
        elif option in ['b', 'back']:
            return
        else:
            print('Invalid input, please try again\n')
            sleep(1)

It's not the fanciest-looking perhaps, but it's easy to implement, easy to understand and does the job just fine.
Where do you want to go?
But maybe that isn't good enough. It might be nice if the "back" option could say something like "Back (to page 1)" depending on what the stack looks like, for example. Or maybe you'd like to also have a "forward" option in case the user accidentally presses "back" too many times. Or maybe there's another reason you don't want to use the call stack for this. That's fine too.
Another option could be to move the navigation logic from the individual menus into a single core loop. Each menu then returns which menu it thinks you should go to next.
For example, we might be looking at something like this:
def main():
    menu_stack = [main_menu]

    while menu_stack:
        current_menu = menu_stack[-1]

        next = current_menu(option)

        if next:
            menu_stack.append(next)
        else:
            menu_stack.pop()

With menus looking a lot like the previous ones, but instead of calling the next menu like page_two() they return it like return page_two. Or perhaps return None if we should go back.
Classy
Personally, though, I think there are some tasks (like asking for input) that should be the responsiblity of the app itself rather than the menus. The menu can know what it looks like and how to navigate based on input, but the core loop asks it for help and then handles those things on its own terms.
def main():
    menu_stack = [main_menu]

    while menu_stack:
        current_menu = menu_stack[-1]

        clear_terminal()

        print(current_menu.text)

        option = input(current_menu.prompt)

        try:
            next = current_menu.get_next_menu(option)

            if next:
                menu_stack.append(next)
            else:
                menu_stack.pop()
        except ValueError as e:
            print("Invalid input, please try again")
            sleep(1)

Defining a nice Menu class is left as an exercise for the reader.