Matrix of heights test I was writing a program for my math class because my Professor asked me to. The problem is as follows: 300 people are arranged in 30 rows each of 10 people. The tallest of each row is chosen, and the shortest of each column. The problem is to prove whether the shortest of the taller people is taller, or the tallest of the shorter people. My program is as follows, but 90% of my coding standards are from PPCG, so my code is likely awful.

import random

multiple = int(input("Would you like multiple runs or one (enter 1 or 0)?\n").rstrip())

def weighted_random():
    # between 3 ft and 6 ft
    temp = random.random()
    t = 0
    if temp > 0.95:
        t = 6
    elif temp < 0.1:
        t = 3
    elif temp < 0.5:
        t = 4
    else:
        t = 5
    return t + random.random()

def output_array(array):
    for i in array:
        for j in i:
            print("%.3f" % j, end=' ')
        print()

def _zip(array):
    _x = len(array[1])
    _temp = []
    for i in range(_x):
        _temp.append([j[i] for j in array])
    return _temp

heights = [[weighted_random() for j in range(10)] for i in range(30)]

if not multiple:
    print("Men")

    output_array(heights)

    tall = [max(i) for i in heights]

    tall_man = min(tall)

    short = [min(i) for i in _zip(heights)]

    short_man = max(short)

    print("Tallest of short: ", end='')

    print(short_man)

    print("Shortest of tall: ", end='')

    print(tall_man)

    print("Answer is: ", end='')

    print(["tallest of short", "shortest of tall"][short_man < tall_man])

else:
    evalf = []

    times = int(input("Number of evaluations?\n").rstrip()) or 10000

    zeroes = 0

    for _i in range(times):
        heights = [[weighted_random() for j in range(10)] for i in range(30)]

        tall = [max(i) for i in heights]

        tall_man = min(tall)

        short = [min(i) for i in _zip(heights)]

        short_man = max(short)

        if short_man >= tall_man:

            zeroes += 1

    print("Out of %d evaluations, we got %d results as shortest of tall taller than tallest of short, and %d times they were equal." % (times, times - zeroes, zeroes))

An old question, bet let's leave no question unanswered... I will go through your code (mostly) in order in which it is presented.

Asking for multiple is, in my opinion, in the wrong position, as it is separated by functions definitions from the rest of the main code. I'd move it after the functions definitions.

Closely related to that, asking the user to answer with C-like logic of 1 for "yes" or 0 for "no" is not pleasant, especially since Python doesn't share C's woes with strings. Btw, int(...) has no problem with extra spaces, so (r)stripping is not needed.

And, since we're becoming user-friendly here, let's allow users to make typos without crashes and other unexpected behaviour by asking them again if they give a nonsense answer. All together:

while True:
    answer = input("Would you like answer runs (y/n)? ").strip().lower()
    if answer in {"y", "n", "yes", "no"}:
        break
multiple = (answer in {"y", "yes"})

Creating weighted randoms can be done in a more concise and configurable way. I have chosen this:

weights = [0, 0, 0, 0.1, 0.5, 0.95, 1]
def weighted_random():
    # between 3 ft and 6 ft
    major = random.random()
    major = next(idx for idx,limit in enumerate(weights) if limit > major)
    return major + random.random()

The weights list (could've been a tuple as well) contains limits. The major value (the integer part) of the height is the index of the first value bigger than the random that was picked. So, for example, 0.05 is between 0 and 0.1, meaning that we pick major = 3 because 3 is the index of 0.1.

Apart from keeping the code short, this makes it trivial to change the weights, including adding more weight categories, as long as we keep to the premise that these define the integer part. Otherwise, we could've used a dictionary or some similar approach.

We create heights twice in the program, so let's not copy/paste this non-trivial and important code. A function is better:

def make_heights(rows=30, cols=30):
    return [[weighted_random() for j in range(cols)] for i in range(rows)]

Printing is also best done using generators and joining the elements with the space character:

def output_heights(data):
    print("Men:")
    for row in data:
        print(" ".join("%.3f" % el for el in row))

Your _zip function confused me at first. There is a zip() in Python, but it does something far more reasonable for a function with such a name. Consider renaming this to "transpose".

Either way, I think it's best to handle "the shortest of the tall ones" and "the tallest of the short ones" the same way, with a single function for each of them:

def shortest_of_talls(data):
    return min(max(row) for row in data)

def tallest_of_shorts(data):
    return max(min(row[j] for row in data) for j,_ in enumerate(data[0]))

Here, I use enumerate to get (index, value) tuples, and I use a throwaway variable _ for values because I don't need them. There are a few situations when _ is not a good choice, but that's beyond the scope of this review.

Btw, notice that I use data. It's more descriptive than array (albeit, heights or people might have been a better choice).

Your main if looks like this: if not multiple: ... else: .... Does it not make more sense to do if multiple: ... else: ...? I changed it thus, so let's first review the "multiple" code.

I prefer evalf = list() over evalf = [], as I find it more readable. You're more likely to notice a bug if you put set() instead of list() than if you put {} instead of [].

We decided above to be more user friendly, so let's keep that up when asking about the number of runs. You wanted to ask the user to give you an integer, and default to 10000 if they give 0. Here is the code that takes an empty(ish) string (we allow spaces) which is treated as the default, or a positive integer. Any other response will repeat the question:

    while True:
        times = input("Number of evaluations (leave empty for 10000)? ")
        if times.strip() == "":
            times = 10000
            break
        try:
            times = int(times)
            if times > 0:
                break
        except ValueError:
            pass

The next part of your code is biased toward what you know (expect?) is the right answer. If I got it right, this is a research question, so let's research the results properly, by counting how many "tallest of shorts", "shortest of talls", and "equals" are there:

    cnt_tos = cnt_sot = cnt_eq = 0

Since your default is to run 10000 tests, and that takes time (roughly a minute on my laptop), I decided to add progress information here. It's a neat little trick: print a message, but end with "\r" instead of "\n" to place the cursor at the beginning of the same line, instead of the next one:

    cnt_tos = cnt_sot = cnt_eq = 0
    done = -1
    for run in range(times):
        new_done = 100*run // times
        if new_done > done:
            done = new_done
            print("Done: %d%%" % done, end="\r")
        heights = make_heights()

        tall_man = shortest_of_talls(heights)
        short_man = tallest_of_shorts(heights)

        if tall_man > short_man:
            cnt_sot += 1
        elif tall_man < short_man:
            cnt_tos += 1
        else:
            cnt_eq += 1

    print("Out of %d evaluations, we got:" % times)
    print("  * {times} times ({percent:.3f}%) the shortest of tall was taller,".format(times=cnt_sot, percent=100*cnt_sot/times))
    print("  * {times} times ({percent:.3f}%) the tallest or short was taller,".format(times=cnt_tos, percent=100*cnt_tos/times))
    print("  * {times} times ({percent:.3f}%) they were equal.".format(times=cnt_eq, percent=100*cnt_eq/times))

Notice that I have removed many blank lines that you have. The Style Guide for Python, which is one of the core documents for Python programmers, suggests that those should be used sparingly. I am probably too generous with them, but I prefer separating parts of the code that do semantically different parts of the job.

I also replaced C-style formats for the newer format() function, to make the code more readable (and, in theory, easier to change/translate, as these arguments are no longer positional). I will rarely use C-style formatting if there is more than one variable involved.

I did similar changes to your "single run" branch:

    heights = make_heights()
    output_heights(heights)

    tall_man = shortest_of_talls(heights)
    short_man = tallest_of_shorts(heights)

    print("Tallest of short:", short_man)
    print("Shortest of tall:", tall_man)
    print(
        "The answer is: ",
        "tallest of short" if short_man > tall_man else "shortest of tall"
    )

As you can see, Python has ternary operator, and this is far more readable than the original print(["tallest of short", "shortest of tall"][short_man < tall_man]).

Here is my variant of your code as a whole:

#!/usr/bin/env python3

import random

# Between 3 ft and 6 ft
weights = [0, 0, 0, 0.1, 0.5, 0.95, 1]

def weighted_random():
    major = random.random()
    major = next(idx for idx,limit in enumerate(weights) if limit > major)
    return major + random.random()

def make_heights(rows=30, cols=30):
    return [[weighted_random() for j in range(cols)] for i in range(rows)]

def output_heights(data):
    print("Men:")
    for row in data:
        print(" ".join("%.3f" % el for el in row))

def shortest_of_talls(data):
    return min(max(row) for row in data)

def tallest_of_shorts(data):
    return max(min(row[j] for row in data) for j,_ in enumerate(data[0]))

while True:
    answer = input("Would you like answer runs (y/n)? ").strip().lower()
    if answer in {"y", "n", "yes", "no"}:
        break
multiple = (answer in {"y", "yes"})

if multiple:
    evalf = list()
    while True:
        times = input("Number of evaluations (leave empty for 10000)? ")
        if times.strip() == "":
            times = 10000
            break
        try:
            times = int(times)
            if times > 0:
                break
        except ValueError:
            pass

    cnt_tos = cnt_sot = cnt_eq = 0
    done = -1
    for run in range(times):
        new_done = 100*run // times
        if new_done > done:
            done = new_done
            print("Done: %d%%" % done, end="\r")
        heights = make_heights()

        tall_man = shortest_of_talls(heights)
        short_man = tallest_of_shorts(heights)

        if tall_man > short_man:
            cnt_sot += 1
        elif tall_man < short_man:
            cnt_tos += 1
        else:
            cnt_eq += 1

    print("Out of %d evaluations, we got:" % times)
    print("  * {times} times ({percent:.3f}%) the shortest of tall was taller,".format(times=cnt_sot, percent=100*cnt_sot/times))
    print("  * {times} times ({percent:.3f}%) the tallest or short was taller,".format(times=cnt_tos, percent=100*cnt_tos/times))
    print("  * {times} times ({percent:.3f}%) they were equal.".format(times=cnt_eq, percent=100*cnt_eq/times))
else:
    heights = make_heights()

    output_heights(heights)

    tall_man = shortest_of_talls(heights)
    short_man = tallest_of_shorts(heights)

    print("Tallest of short:", short_man)
    print("Shortest of tall:", tall_man)
    print(
        "The answer is: ",
        "tallest of short" if short_man > tall_man else "shortest of tall"
    )

Apart from these, I have three more comments:

Use sensible names. Apart from zip and array, there was also zeroes, which was probably meant as "those where the condition is zero (it's actually False in Python)", but this is really not informative to anyone reading the code.
Consider including docstrings in all of your functions. It's a good habit to have, lest you end up banging your head on the desk when reading trying to understand your own code in a few months.
Since you're working with matrices, using NumPy's array could be a good choice, especially because it provides a simple and fast transpose and weighted sampling.


Dynamically generating a HTML option list using Angular.js I used step 4 from the Angular tutorial to do some tinkering of my own. In that particular step of the tutorial, a option list is created that determines how a list is sorted (Alphabetically, by age). I tweaked the code to dynamically generate these sorting options from the data set.

Essentially, it takes the first element of the JSON data and creates a option for each of the keys:

<select ng-model="orderProp">
   <option value="{{opt}}" ng-repeat="(opt,value) in phones[0]">{{opt}}</option>
</select>

Any remarks or suggestions?

This is the full index.html file. The example is reproducible by installing the Angular tutorial.

<!doctype html>

  <meta charset="utf-8">
  <title>Google Phone Gallery</title>
  <link rel="stylesheet" href="css/app.css">
  <link rel="stylesheet" href="css/bootstrap.css">

        Search: <input ng-model="query">
        Sort by:
        <select ng-model="orderProp">
            <option value="{{opt}}" ng-repeat="(opt,value) in phones[0]">{{opt}}</option>
        </select>

 Now ordered by {{orderProp}}

            {{phone.name}}

{{phone.snippet}}

and the JSON data:

  $scope.phones = [
    {'name': 'Nexus S',
     'snippet': 'Fast just got faster with Nexus S.',
     'age': 1},
    {'name': 'Motorola XOOM™ with Wi-Fi',
     'snippet': 'The Next, Next Generation tablet.',
     'age': 2},
    {'name': 'MOTOROLA XOOM™',
     'snippet': 'The Next, Next Generation tablet.',
     'age': 3}
  ];

I think it's a bad idea for several reasons:

It is possible to have an empty data set in real-world app but it should not affect your sorting options in any way.
It is possible to have some optional fields or fields that aren't suit for sorting - images, additional description (original step-4 actually has such field) etc
You should have a full control over what you're displaying and being able to change it with ease.
Pretty titles instead of field names 'as is' look much better.
In case of a huge list of options you may need to group them using 'disabled' options as delimiters.

The main idea is to keep the balance between decoupling and ease of reusability and extensibility of your functionality. One of solutions is to create an object that describes the way particular data can be sorted. You can implement it as following, for example: http://jsfiddle.net/ocLrt7vp/

Adding sorting options to phones

$scope.phones =
{
    sorting: [
        {title:"---Name---", disabled:true},
        {sortQuery: "name", title: "By name: A-Z"},
        {sortQuery: "-name", title: "By name: Z-A"},
        {title:"---AGE---", disabled:true},
        {sortQuery: "-age", title: "New first"},
        {sortQuery: "age", title: "Old first"}
    ],
    data: [
        {'name': 'Nexus S',
            'snippet': 'Fast just got faster with Nexus S.',
            'age': 2010,
            androidVersion:"Unknown"},
        {'name': 'Motorola XOOM™ with Wi-Fi',
            'snippet': 'The Next, Next Generation tablet.',
            'age': 2012,
            weight: 708},
        {'name': 'MOTOROLA XOOM™',
            'snippet': 'The Next, Next Generation tablet.',
            'age': 2011}
    ]
};

And changing index.html as following

    Search: <input ng-model="query">
    Sort by:
      <select ng-model="orderProp">
          <option value="{{sorting.sortQuery}}" ng-repeat="sorting in phones.sorting"
                  ng-selected="sorting.sortQuery == orderProp"
                  ng-disabled="sorting.disabled">{{sorting.title}}</option>
      </select>

        {{phone.name}}

{{phone.snippet}}

Weight: {{phone.weight}}g

Android Version: {{phone.androidVersion}}g

On the other hand detailed configuration may appear to be excessive for a regular real-world site. As well as passing data that changes once or two in a lifetime with each request should be considered as an example of a bad practice. You can achieve this behaviour in a number of different ways, better ways, but the goal of this comment is to share my vision with you, hope it helps :)


Storing, fetching, and deleting battle simulation results my connection code:

    <?php
try {
    $pdo = new PDO("mysql:host=localhost;dbname=game;
    charset=utf8", "root");
}
catch(PDOException $e){
    echo $e->getMessage();
}
?>

my class code:

    <?php
include './database/config.php';

class DB {
    public function dataWarrior($battle,$name,$type){
        global $pdo;
        try {
            $stmt = $pdo->prepare("SELECT battle_id,warrior_name,warrior_type,health,attack,defense,speed,evade
            FROM battle WHERE battle_id = :battle AND warrior_name = :name AND warrior_type = :type LIMIT 1");

            $stmt->execute(array(":battle"=>$battle, ":name"=>$name, ":type"=>$type));

            return $stmt;
        }
        catch(PDOException $e){
            echo $e->getMessage();
        }
    }
    public function checkBattleID($battle){
        global $pdo;
        try {
            $stmt = $pdo->prepare("SELECT battle_id FROM battle WHERE battle_id=:battle LIMIT 1");

            $stmt->execute(array(":battle"=>$battle));

            if($stmt->rowCount()>0){
                return false;
            }
            else {
                return true;
            }
        }
        catch(PDOException $e){
            echo $e->getMessage();
        }
    }
    public function createBattle($warrior){
        global $pdo;
        try {
            $stmt = $pdo->prepare('INSERT INTO battle(battle_id,warrior_name,warrior_type,
            health,attack,defense,speed,evade) VALUES(:battle,:name,:type,:health,:attack,:defense,:speed,:evade)');

            $stmt->execute(array(':battle'=>$warrior['battle'], ':name'=>$warrior['name'],
            ':type'=>$warrior['type'], ':health'=>$warrior['health'], ':attack'=>$warrior['attack'],
            ':defense'=>$warrior['defense'], ':speed'=>$warrior['speed'], ':evade'=>$warrior['evade']));
        }
        catch(PDOException $e){
            echo $e->getMessage();
        }
    }
    public function deleteBattle($battle){
        global $pdo;
        try {
            $stmt = $pdo->prepare('DELETE FROM battle WHERE battle_id = :battle');
            $stmt->execute(array(':battle'=>$battle));
        }
        catch(PDOException $e){
            echo $e->getMessage();
        }
    }
}
?>

Stay away from globals. You will never need them. Never. Especially don't use globals in a class where they are completely unnecessary. They are an older technology that is full of security holes. Because $pdo is in the global scope, if I were of malicious intent I could hack into your database simply by using the already authenticated variable you gave me. This also makes it difficult to figure out where your variables are coming from. Without any kind of context it could become very easy to lose track of it. Instead of separating your connection logic from your class, just put it in the class constructor and use class properties to simulate a "global" state. Unlike Peter's example, this keeps the connection logic associated with the class instead of injecting it. The problem with dependency injection, in this case, is that it makes no sense unless you are reusing that same connection in another class at the same time. With the limited knowledge of this system I cannot know one way or the other and so DI seems a bit abstract.

private $pdo;//declare class property first

public function __construct() {
    try {
        $this->pdo = new PDO(//define class property
            'mysql:host=localhost;
            dbname=game;
            charset=utf8',

            'root'
        );
    } catch( PDOException $e ) {
        echo $e->getMessage();
    }
}

//to use class property
$this->pdo->prepare(/*etc...*/);

The reason we are using private in the above scenario is the same reason we aren't using globals. If the property were public it could be accessed outside of the class, which is not something we want to provide access to.

You have three more variables that could be made into properties. Properties are an essential part of OOP, they encapsulate the data needed to make the class work. They do this by allowing different methods to communicate with each other by storing and accessing properties within their class instead of returning and parsing parameters. Not to say methods can't have parameters, only that most become unnecessary. Without properties, you might as well be using regular functions. So, how do we apply this? The parameters you pass to your dataWarrior() method seem to be reused at different points in the class. The $battle parameter is used to determine the battle_id in every method, the $name parameter is used to get the warrior_name in two of the methods, and the $type parameter is used to get the type in two methods. So, instead of passing these values again and again, just assign them to a property and use that property whenever you would have used the passed parameter. This is most effective if done in the first method in which these parameters are all used. I would assume this is in createBattle()?

public function createBattle( $warrior ) {
    $this->battle = $warrior[ 'battle' ];
    $this->name = $warrior[ 'name' ];
    $this->type = $warrior[ 'type' ];

    //etc...
}

Don't forget to declare those properties at the top of your class. Declaring a property is equally as important as declaring your methods. If you don't declare a property or method it defaults to the public scope, but this wont always be the case. I have heard rumors of this being deprecated soon, which means no more default values and whole lot more warnings. So be prepared and code accordingly.

So, to answer your question: Not really, but a decent enough start. Just keep in mind the following principles: "Don't Repeat Yourself" (DRY), Single Responsibility, and, as Peter pointed out, SOLID, which is kind of the first two plus a few others all rolled into one. If your code follows these principles and utilizes the fundamental aspects of the OOP (Encapsulation, Inheritance, and Polymorphism), then you should be fine.

BTW: What tutorials are you using that is teaching you to use globals? I've become curious because this seems to be a growing trend recently, and if this is all coming from the same source I want to see what I can do about stopping it.


How to (and should I) avoid repetition while assigning values from different sources? In a Mako template partial I accept an argument (step) that can be an instance of Step class, a dict or None. Can I in some way avoid repetition or do the check in other more 'pythonic' way? Or how could I gracefully avoid checking types at all?

<%include file="_step.mako" args="step=step" />

and then in _step.mako:

<%page args="step"/>
<%! from package.models.recipe import Step %>
<%
if isinstance(step, Step):
    number = step.number
    time_value = step.time_value
    text = step.text
elif type(step) is dict:
    number = step['number']
    time_value = step['time_value']
    text = step['text']
else:
    number = 1
    time_value = ''
    text = ''
%>

1.I assume if type(step) is dict: should be elif type(step) is dict:.

2.isinstance is preferable than type because isinstance caters for inheritance, while checking for equality of type does not (it demands identity of types and rejects instances of subtypes).

3.You also can apply duck-typing.

try:
    number = step.number
except AttributeError:
    try:
        number = step['number']
    except KeyError:
        number = 1

And these ways are quite pythonic.

4.I suggest you can inherit your Step class from dict then you don't need to check types. Yo will just do

number = step.get('number', 1)
time_value = step.get('time_value', '')


Transform DataReader to List using reflections I have implemented the code to convert datareader to list of objects. My intention was to not to use column name string literals while reading the datareader.

public static IEnumerable<T> GetListFromDataReader<T>(IDataReader reader) where T : new()
{
    var properties = typeof(T).GetProperties();

    var modelProperties = new List<string>();
    var columnList = (reader.GetSchemaTable().Select()).Select(r => r.ItemArray[0].ToString());
    while (reader.Read())
    {
        var element = Activator.CreateInstance<T>();
        Dictionary<string, string> dbMappings = DBColumn(element);
        string columnName;
        foreach (var f in properties)
        {

            if (!columnList.Contains(f.Name) && !dbMappings.ContainsKey(f.Name))
                continue;
            columnName = dbMappings.ContainsKey(f.Name) ? dbMappings[f.Name] : f.Name;
            var o = (object)reader[columnName];

            if (o.GetType() != typeof(DBNull)) f.SetValue(element, ChangeType(o, f.PropertyType), null);
        }
        yield return element;
    }

}

public static object ChangeType(object value, Type conversion)
{
    var t = conversion;

    if (t.IsGenericType && t.GetGenericTypeDefinition().Equals(typeof(Nullable<>)))
    {
        if (value == null)
        {
            return null;
        }

        t = Nullable.GetUnderlyingType(t); ;
    }

    return Convert.ChangeType(value, t);
}

public static Dictionary<string,string> DBColumn<T>(T item) where T:new()
{

    Dictionary<string, string> dbMappings = new Dictionary<string, string>();
    var type = item.GetType();
    var properties = type.GetProperties();
    foreach (var property in properties)
    {
        var attributes = property.GetCustomAttributes(false);
        var columnMapping = attributes
    .FirstOrDefault(a => a.GetType() == typeof(DbColumnAttribute));
        if (columnMapping != null)
        {
            dbMappings.Add(property.Name, ((DbColumnAttribute)columnMapping).Name);
        }
    }
    return dbMappings;
}

Can somebody help me by providing a review?  I am doubtful of too much usage of reflections in loops would downgrade the performance. Or should I compromise and better to go with string literals itself while reading?

I can suggest a number of improvements you can make to the code:

You're calling GetSchemaTable to get column names. To simply fetch column names you don't need, there is a faster alternative:

var columns = Enumerable.Range(0, reader.FieldCount).Select(reader.GetName).ToList();

You're not computing the result of your select query to get column names. Select extension method is lazily evaluated which means it is executed every time you call it. Instead you should materialize the query by calling ToList() or something like that (as shown above).
You should make use of expression trees to generate compile code at run-time. You can make use of such code in many ways:

Db operations are expensive. Add reflection to it, it gets worse. Expression trees help you get the benefit of reflection at native speed once you compile some logic. You should replace all reflection logic with expression trees. One example
Activator.CreateInstance<T>() uses reflection behind the scenes which is slower. Expression trees can make that case faster too. See
reader["columnName"] will be marginally slower than reader[columnIndex] though it will ultimately depend on the ADO.NET connector implementation. It is safe to assume the latter is faster. You could spit out code at run-time to cache the ordinal values of fields and use it to query the reader like shown here.

Your columnCollection.Contains(string columnName) is case-sensitive. Make it case-insensitive since property naming convention can differ from db column naming conventions.
You don't need the casting here:

(object)reader[columnName];

What you get returned from reader is already object. Just code readability matters.
You're not assigning the column value to property when reader returns DBNull. This can give rise to oddities at times. Imagine you have an unfortunately designed class like this:

class User { public User() { Property = "42"; } }

Assume your db value for Property column is DBNull. If you don't assign anything in your mapping code then the returned User object will have a default value of "42" for Property where as in db the corresponding field will be DBNull which indicates null.
As Jesse has pointed in his answer, you're using reflection code to map inside the loop. I would take it outside. But unlike him I wouldn't recommend caching in a static dictionary permanently. I would like to optimize for memory too.
You're doing double dictionary lookup in this line:

columnName = dbMappings.ContainsKey(f.Name) ? dbMappings[f.Name] : f.Name;

Use TryGetValue instead.
No need to check twice for column name like this:

if (!columnList.Contains(f.Name) && !dbMappings.ContainsKey(f.Name))
    continue;
columnName = dbMappings.ContainsKey(f.Name) ? dbMappings[f.Name] : f.Name;

In fact, no need to check for presence columns in column collection at all; just loop column collection straight away instead of property collection. Or even better, take expression route, no need of any looping, you could assign to variables statement by statement.
You're doing GetProperties twice. Instead call it once, and cache it in dictionary. So your dictionary must look like Dictionary<string, PropertInfo> rather than Dictionary<string, string>. Or make use of ordinals, so your dictionary looks like Dictionary<int, PropertyInfo>.
Since you're constraining the T to new() you can just call new T() rather than Activator.CreateInstance<T>(). This is simpler syntax.
The columnName value you use here:

(object)reader[columnName];

is obtained from property names. If there is a case difference between property name and the actual field name in the reader the lookup will be slower, depending on the implementation of ADO.NET connector. It's better to use the column name you get from the reader itself. Or even better, just use ordinals to query (i.e. indices).
Not all properties are settable. It is better to check it beforehand.
There's a deeper problem here with IDisposables. You have to use the using statement to properly dispose your connection, command and reader objects. When you leave the method which has using keyword used, your objects (especially the reader) will be disposed and hence you wont be able to operate on reader outside your method. Your alternative is to not use using at all and dispose off everything manually after you're done. This wont look good. I would rather iterate the reader in the method which initializes the reader rather than in the mapper class, and return using the yield keyword which blocks disposing off reader object until enumeration.

The gist of it (except expression trees which is too much plumbing, but I have given a reference link so that you can work on) would look like:

public class DataReaderMapper<T> where T : new()
{
    Dictionary<int, PropertyInfo> mappings;

    public DataReaderMapper(IDataReader reader)
    {
        this.mappings = Mappings(reader);
    }

    // int part is column indices (ordinals)
    static Dictionary<int, PropertyInfo> Mappings(IDataReader reader)
    {
        var columns = Enumerable.Range(0, reader.FieldCount);
        var properties = typeof(T)
                        .GetProperties()
                        .Select(prop => new
                        {
                            prop,
                            attr = prop.GetCustomAttributes(typeof(DbColumnAttribute)).FirstOrDefault()
                        })
                        .Select(x => new
                        {
                            name = x.attr == null ? x.prop.Name : ((DbColumnAttribute)x.attr).Name,
                            x.prop
                        });
        return columns
              .Join(properties, reader.GetName, x => x.name, (index, x) => new
              {
                  index,
                  prop = !x.prop.CanWrite ? null : x.prop
              }, StringComparer.InvariantCultureIgnoreCase)
             .Where(x => x.prop != null) // only settable properties accounted for
             .ToDictionary(x => x.index, x => x.prop);
    }

    public T MapFrom(IDataRecord record)
    {
        var element = new T();
        foreach (var map in mappings)
            map.Value.SetValue(element, ChangeType(record[map.Key], map.Value.PropertyType));

        return element;
    }

    static object ChangeType(object value, Type targetType)
    {
        if (value == null || value == DBNull.Value)
            return null;

        return Convert.ChangeType(value, Nullable.GetUnderlyingType(targetType) ?? targetType);
    }
}

You can call it like:

using (var reader = cmd.ExecuteReader())
{
    var mapper = new DataReaderMapper<T>(reader);
    while (reader.Read())
        yield return mapper.MapFrom(reader);
}

I believe this code is overall shorter, but personally I think you should go for expressions instead of reflection, or even better try a tried and tested ORM.

I'm passing reader object to the constructor of mapper class as well as in the MapFrom method for a reason. Designing a clean API is little difficult here considering reader form both IDataReader (the enumerable) as well as IDataRecord (the result of enumeration). The reason I pass it separately is performance. I dont cache anything, so memory management is left to client. If you don't want the mapping to happen every time you can just cache the mapper instance and dispose it off when you don't want anymore. For e.g. you could do this:

 DataReaderMapper<T> mapper;
 using (var reader = cmd.ExecuteReader())
 {
     mapper = new DataReaderMapper<T>(reader); // initialized once
     while (reader.Read())
         yield return mapper.MapFrom(reader);
 }

 using (var reader = cmd.ExecuteReader())
 {
     while (reader.Read())
         yield return mapper.MapFrom(reader); // no need of further initialization
 }

To use it at a later stage you can cache mapper in some dictionary detailing which out will be too much for now.


Jsoup connection to URL I have simple class that I want to ask if is there any possible to improve it? I mean, for me it looks poor. Is there any way to use here try-with-resources, stream or optional?

package bookstore.scraper;

import lombok.extern.slf4j.Slf4j;
import org.jsoup.Jsoup;
import org.jsoup.nodes.Document;

import java.io.IOException;

@Slf4j
public class JSoupConnector {

    public static Document prepareConnectionToURL(String URL) {
        Document document = null;
        try {
            document = Jsoup.connect(URL).get();
        } catch (IOException e) {
            log.warn("Cannot connect to URL!");
        }
        return document;
    }
}

This error handling looks pretty useless to me:

The log message doesn't record which URL was inaccessible.
If an error occurs, then the code will continue to execute. That means that whoever calls this function will have to handle the possibility of a null result. If that isn't done, then it will cause a NullPointerException.

A better idea would be to simply propagate the exception, so that the caller will stop trying to process a Document that isn't there.

Furthermore, what does it mean to "prepare a connection"? It looks like this code goes way beyond preparing a connection — it actually makes the connection and fetches the content as a Document!

So, the class should look more like this:

public class JSoupConnector {
    public static Document connect(String url) throws IOException {
        try {
            return Jsoup.connect(url).get();
        } catch (IOException e) {
            log.warn("Cannot connect to URL " + url);
            throw e;
        }
    }
}

At that point, this wrapper is hardly doing anything at all, and you might consider eliminating this helper functional altogether.


CSS Optimization? How can I clean my code My CSS feels dirty and I have no idea how to improve it.

Suggestions are greatly appreciated!

html, body { border:0; margin:0; padding:0; }
body { font: normal 12px helvetica,sans-serif; }
.header {
    background-color:#242424;
    height:40px; }
.header h1, h2 { text-transform:uppercase; }
.header h1 {
    color:#FFF;
    display:inline;
    font-size:12px; }
.header h2 {
    color:#B4B4B4;
    display:inline;
    font-size:12px; }
.header #title, .sub #nav, #content {
    margin:0 auto;
    width:797px; }
.header #title {
    padding-top:12px; }
.header #version {
    color:#FFF;
    float:right;
    font-size:10px;
    margin:-13px 10px 0 0; }
.sub {
    background-color:#E4F2FD;
    border-bottom:#D1E5EE solid 1px;
    height:30px; }
.sub ul, li { margin:0; padding:0; list-style:none; }
.sub li {
    float:left;
    padding-left:10px;
    margin:8px 20px 0 -18px; }
.sub li a {
    color:#21759B;
    padding:8px;
    text-decoration:none; }
.sub li a:hover { background-color:#FFF; }
#content { margin-top:15px; }
h1 { font-size:13px; }
hr {
    border:0;
    height:1px;
    background-color:#E9E9E9; }
.quick-add {
    background-color:#E9E9E9;
    color:#575757;
    font-size:12px;
    margin-top:10px;
    padding:2px; }
.quick-add td { padding-left:8px; }
#data { margin-top:15px; }
#member { width:100.5%; margin-left:-1px;}
#even {
    background-color:#EFF8FF;
    color:#21759B; }
#odd {
    background-color:#FFF; }
#data td { padding:3px; }
#a { width:5%; text-align:center;}
#b { width:40%; }
#c { width:10%; }
#d { width:20%; }
#e { width:15%; }

Also, I have two different tables on two different pages (one for members and another for information) what's the best way to independently style them?

Thanks!

Use classes instead of ids

#even
#odd
#a, #b, #c, #d, #e

These are clearly repeating patterns that might happen several times in the same page.
An id is supposed to be unique on the page.

Use better names

The id or class of an HTML element, like a class or function name when programming, is supposed to describe it.
#a, #b, etc, are bad names. If your HTML and CSS are small, it's not complicated to figure out what they do. But if they grow, or if someone else picks up your code, it might start to get tricky to figure out what each class/id is about.

Clear nomenclature is better.

Use relative sizes

Your body defines font-size as 12px. Your h3, 13px.
If you change the body's font-size, you will have to hunt down all other definitions and update them - e.g. the h3.

If you define the first font-size absolutely (body: 12px) and then define all others relatively (h3: 110%), when you update the body's font-size the others adjust automatically.
This has the added advantage of being friendlier to people using user styles, e.g. people that use larger font-sizes to be able to read better.

Misc

On the hr, why not style the border instead of clearing the border and using background-colour?
I see too many negative margins. It feels somehow wrong.

"Two different tables"

I would first style them exactly the same, and then if necessary just add extra styles to differentiate them - perhaps change the colour of the borders or the background of the headers.
I'm not sure what you are asking there.


Sum of divisors in Haskell I decided to write a function divisorSum that sums the divisors of number. For instance 1, 2, 3 and 6 divide 6 evenly so:

$$ \sigma(6) = 1 + 2 + 3 + 6= 12 $$

I decided to use Euler's recurrence relation to calculate the sum of divisors:

$$\sigma(n) = \sigma(n-1) + \sigma(n-2) - \sigma(n-5) - \sigma(n-7) + \sigma(n-12) +\sigma(n-15) + \ldots$$

i.e.

$$\sigma(n) = \sum_{i\in \mathbb Z_0} (-1)^{i+1}\left( \sigma\left(n - \tfrac{3i^2-i}{2}\right) + \delta\left(n,\tfrac{3i^2-i}{2}\right)n \right)$$

(See here for the details). As such, I decided to export some other useful functions like nthPentagonal which returns the nth (generalized) pentagonal number. I created a new project with stack new and modified these two files:

src/Lib.hs

module Lib
    ( nthPentagonal,
      pentagonals,
      divisorSum,
    ) where

-- | Creates a [generalized pentagonal integer]
-- | (https://en.wikipedia.org/wiki/Pentagonal_number_theorem) integer.
nthPentagonal :: Integer -> Integer
nthPentagonal n = n * (3 * n - 1) `div` 2

-- | Creates a lazy list of all the pentagonal numbers.
pentagonals :: [Integer]
pentagonals = map nthPentagonal integerStream

-- | Provides a stream for representing a bijection from naturals to integers
-- | i.e. [1, -1, 2, -2, ... ].
integerStream :: [Integer]
integerStream = map integerOrdering [1 .. ]
    where
    integerOrdering :: Integer -> Integer
    integerOrdering n
        | n `rem` 2 == 0 = (n `div` 2) * (-1)
        | otherwise = (n `div` 2) + 1

-- | Using Euler's formula for the divisor function, we see that each summand
-- | alternates between two positive and two negative. This provides a stream
-- | of 1 1 -1 -1 1 1 ... to utilze in assiting this property.
additiveStream :: [Integer]
additiveStream = map summandSign [0 .. ]
    where
    summandSign :: Integer -> Integer
    summandSign n
        | n `rem` 4 >= 2 = -1
        | otherwise = 1

-- | Kronkecker delta, return 0 if the integers are not the same, otherwise,
-- | return the value of the integer.
delta :: Integer -> Integer -> Integer
delta n i
    | n == i = n
    | otherwise = 0

-- | Calculate the sum of the divisors.
-- | Utilizes Euler's recurrence formula:
-- | $\sigma(n) = \sigma(n - 1) + \sigma(n - 2) - \sigma(n - 5) \ldots $
-- | See [here](https://math.stackexchange.com/a/22744/15140) for more informa-
-- | tion.
divisorSum :: Integer -> Integer
divisorSum n
    | n <= 0 = 0
    | otherwise = sum $ takeWhile (/= 0)
                                  (zipWith (+)
                                           (divisorStream n)
                                           (markPentagonal n))
    where
    pentDual :: Integer -> [Integer]
    pentDual n = [ n - x | x <- pentagonals]
    divisorStream :: Integer -> [Integer]
    divisorStream n = zipWith (*)
                              (map divisorSum (pentDual n))
                              additiveStream
    markPentagonal :: Integer -> [Integer]
    markPentagonal n = zipWith (*)
                               (zipWith (delta)
                                        pentagonals
                                        (repeat n))
                               additiveStream

app/Main.hs (mostly just to "test" it.)

module Main where

import Lib

main :: IO ()
main = putStrLn $ show $ divisorSum 8

TL;DR:

use memoization to speed up calculations
if you drive for performance use quot instead of div
if possible, try to define your sequences without complicated functions
use even n instead of n `rem` 2 == 0 or odd n instead of n `rem` 2 == 1

Type annotations and documentation

You use both type annotations as well as documentation. Great. There are only two small drawbacks:

The documentation strings sometimes miss highlighting, e.g. [1, -1, 2, -2,...] in pentagonals. Cross-references would also be great, e.g. pentagonals could point to nthPentagonal in its documentation.
Type annotations for workers (the local bindings) only contribute noise, as their type will be inferred by its surrounding function's types.

For example

integerStream :: [Integer]
integerStream = map integerOrdering [1 .. ]
    where
    integerOrdering :: Integer -> Integer
    integerOrdering n
        | n `rem` 2 == 0 = (n `div` 2) * (-1)
        | otherwise = (n `div` 2) + 1

doesn't need the second type annotation:

integerStream :: [Integer]
integerStream = map integerOrdering [1 .. ]
    where
    integerOrdering n
        | n `rem` 2 == 0 = (n `div` 2) * (-1)
        | otherwise = (n `div` 2) + 1

Convey ideas in code as direct as possible

Above, we use n `rem` 2 == 0 to check whether a number is even. However, there is already a function for that: even. It immediately tells us the purpose of the expression, so let's use that:

integerStream :: [Integer]
integerStream = map integerOrdering [1 .. ]
    where
    integerOrdering n
        | even n    = (n `div` 2) * (-1)
        | otherwise = (n `div` 2) + 1

Use quotRem if you need both the result of rem and quot

integerStream is a special case, though: as we need both the reminder as well as div's result, we can use divMod or quotRem, e.g.

integerStream :: [Integer]
integerStream = map integerOrdering [1 .. ]
    where
    integerOrdering n
        | r         = -q
        | otherwise = q + 1
       where
        (q, r) = n `quotRem` 2

Use simpler code where applicable

We stay at integerStream. As the documentation tells us, we want to have a bijection from the sequence of natural numbers to a sequence of all integers.

While the canonical mapping is indeed

$$
f(n) =
\begin{cases}
\frac{n-1}{2}+1& \text{if } n \text{ is odd}\\
-\frac{n}{2}& \text{otherwise}
\end{cases}
$$
we don't need to use that definition in our code. Instead, we can use

$$
n_1,-n_1,n_2,-n_2,\ldots.
$$

integerStream :: [Integer]
integerStream = go 1
    where
      go n = n : -n : go (n + 1)

Or, if you don't want to write it explicitly

integerStream :: [Integer]
integerStream = concatMap mirror [1..]
    where
      mirror n = [n, -n]

Both variants have the charm that no division is necessary in the computation of your list.

Use cycle for repeating lists

While the methods above arguably break down to personal preference, additiveStream can benefit from cycle:

additiveStream :: [Integer]
additiveStream = cycle [1, 1, -1, -1]

Generalize functions where applicable

delta can be written for any  type that is an instance of Num an Eq, so lets generalize:

delta :: (Eq n, Num n) => n -> n -> n
delta n i
    | n == i    = n
    | otherwise = 0


Replace tabs in input and variable vs symbolic parameter I am learning C and this is an exercise from The C Programmnig Languange by Brian and Ritchie.

  Write a program "detab" that replaces tabs in the input with the proper number of blanks to space to the next tab stop. Assume a fixed set of tab stops, say every \$n\$ columns. Should \$n\$ be a variable or a symbolic parameter?

Here's my try, and I've made \$n\$ a parameter. I would appreciate any feedback and explanation on symbolic parameter.

#include <stdio.h>

#define TABLENGTH 8

void detab(int n);

int main () {

  int c, noOfTabs;
  noOfTabs = 0;

  while ((c = getchar()) != EOF) {

    if (c == '\t') {
      ++noOfTabs;
    } else if (noOfTabs > 0) {
      detab(noOfTabs);
      noOfTabs = 0;
      putchar(c);
    }

  }

  return 0;
}

void detab (int n) {

  while (n > 0) {
    --n;
    int i;
    for (i = 0; i < TABLENGTH; ++i) {
      putchar(' ');
    }
  }

}

The code is not ready for review, because it does not solve the exercise. It blindly replaces each tab with TABLENGTH spaces. This is not detabbing; the amount of blanks must give you a next tab stop. For example, for

a\tb
aa\tb
aaa\tb

the output should be

a       b
aa      b
aaa     b

(number of blanks is 7, 6, and 5 respectively, to align b at the tab stop, the 8th column). Your code produces

a        b
aa        b
aaa        b

instead.


Variadic function to find the first matching (string) argument in a larger array Playing around with parsing a data buffer I wrote a small search function to find the first complete match out of a set of (string) arguments.

The (simplified) buffer class with the find function:

// buffer_simple.hpp
#pragma once

#include <cstddef>
#include <utility>

template <typename T, size_t S>
class buffer {
public:
    constexpr buffer() {}
    template <size_t Si>
    constexpr buffer(const T (&arr)[Si]) {
        for (size_t i = 0, l = (S < Si ? S : Si); i < l; ++i) {
            values[i] = arr[i];
        }
    }

    /**
     * Find the first matching array in a larger array and return the argument index and buffer end index of the match.
     * @param   str One or many strings to search for
     * @returns std::pair<argument index, end of match index in buffer>
     */
    template <size_t... SS>
    constexpr std::pair<int, size_t> find_first(const T (&...str)[SS]) const {
        size_t matching[] { (SS - 1)... }; // Amount of remaining 'characters to match'
        for (size_t i = 0; i < S; ++i) { // Loop over the dataset
            int mi = 0;
            ((matching[mi++] = (str[(SS - 1) - matching[mi]] == values[i]) ? (matching[mi] - 1) : ((str[0] == values[i]) ? (SS - 2) : (SS - 1) )), ...); // Nasty pseudo loop for width first search
            for (mi = 0; mi < sizeof...(SS); ++mi) { // See if we have any full matches
                if (matching[mi] == 0) return { mi, i };
            }
        }
        return { -1, 0 };
    }

    constexpr T& operator[](const size_t i) { return values[i]; }
    constexpr const T& operator[](const size_t i) const { return values[i]; }
private:
    T values[S] {0};
};

An example use case:

#include <iostream>
#include "buffer_simple.hpp"

buffer<char, 256> b { "find mefind me:gehoehgioewghowieanother string:" };

int main () {
    const auto result = b.find_first("find me:", "bla", "another string:");
    // switch (result.first) {
    // case 0:
    //     break;
    // case 1:
    //     break;
    // case 2:
    //     break;
    // }
    std::cout << "Result index: " << result.first << ", offset: " << result.second << std::endl;
}

This will return:

Result index: 0, offset: 14

Despite the extremely nasty fold expression this actually works. During runtime that is. As a test I tried declaring the buffer and search result constexpr and things blew up with GCC reporting an Array out of bounds error. I'm curious to see which other pitfalls might be lurking.

Good stuff!

I only see one big issue (which is actually what's preventing you from using this as a constexpr).

Errors:

UB: Unsequenced modification

matching[mi++] = (str[(SS - 1) - matching[mi]]
         ^^                               ^^

Which value does mi take for that second indexing? The C++ standard says: either the pre or post increment value is legal. You have undefined behavior here.

You can solve this by separating out the mi incrementation as a separate statement. Using the evil comma operator would allow you to do it as a fairly small modification of your existing code:

(
  ((matching[mi] = (str[(SS - 1) - matching[mi]] == values[i])
     ? (matching[mi] - 1)
     : ((str[0] == values[i]) ? (SS - 2) : (SS - 1))
  ), ++mi),
...);

This actually makes the constexpr version work!

Style

I think find_first should be a free-floating function, since you already have the [] accessors.
I would definitely try to format that fold expression in a more readable manner. See my version of it above for an example.

Improvements

Add a template deduction guide:

Something like this:

template<typename T, size_t LEN>
buffer(const T(&)[LEN]) -> buffer<T, LEN>;

So then your declare buffers in a cleaner manner, and have the buffer be perfectly sized:

buffer<char, 256> b { "find mefind me:gehoehgioewghowieanother string:" };
//becomes
buffer b { "find mefind me:gehoehgioewghowieanother string:" };


Create monthly calendar with number of days and offset (day of week) as user input I am taking a beginning programming class and have been given the assignment to create a calendar as described in the title.  Per the assignment the user entering offset of 0 will begin the calendar on a monday and 6 will start on sunday.  There are to be 4 spaces per column and the request for input should prompt the user again if the given input isn't a valid option (ie more than 6 days or a month with less than 28 or more than 31 days).

The program works as written, what I am looking for is suggestions/critiques on ways that I might have done it better given what has been taught in the class so far.  It is early in the class, so far it has been limited to pretty much if/else and loops.

#include <iostream>

int getInputDays()
{
  int inputDays = 0;
  bool validInput = false; // used to verify input is valid
  while (validInput == false)
    {
      std::cout << "Number of Days: ";
      std::cin >> inputDays;
      // will call input valid if between 28 and 31 days
      if (inputDays >= 28 && inputDays <= 31)
    validInput = true;
    }
  return inputDays;
}

int getInputOffset()
{
  int inputOffset = 0;
  bool validInput = false; // used to verify input is valid
  while (validInput == false)
    {
      std::cout << "Offset: ";
      std::cin >> inputOffset;
      // will call input valid if between 0 and 6
      if (inputOffset >= 0 && inputOffset <= 6)
    validInput = true;
    }
  // convert offset from 0 being monday to 0 being sunday
  if (inputOffset == 6)
    inputOffset = 0;
  else
    inputOffset += 1;
  return inputOffset;
}

void displayOffset(int offset)
// used to create offset on the first line of calendar
{
  if (offset == 1)
    std::cout << "       1";
  else if (offset == 2)
    std::cout << "           1";
  else if (offset == 3)
    std::cout << "              1";
  else if (offset == 4)
    std::cout << "                   1";
  else if (offset == 5)
    std::cout << "                       1";
  else
    std::cout << "                           1";
}

void drawCalendar (int days, int offset)
{
  int date = 1;
  int spaces = 0; // used to keep track of location on each line
  std::cout << "  Su  Mo  Tu  We  Th  Fr  Sa"  << std::endl;  // calendar header
  while (date <= days)
    {
      if (offset > 0)  // for months starting on any date other than sunday
    {
      displayOffset(offset);
      date = 2;
      spaces = (offset * 4) + 4;
      offset = 0;
    }
      while (spaces < 28) // 28 spaces per line per assignment
    {
      if (date >= 10) // 2 spaces before double digit numbers
        {
          if (date > days)
        break;
          std::cout << "  " << date;
        }
      else            // 3 spaces before single digit numbers
        std::cout << "   " << date;
      date += 1;
      spaces += 4;
    }
      std::cout << std::endl;
      spaces = 0; // reset spaces to 0 at the end of each line
    }
}

int main()
{
  // prompt user to input days and offset per assignment
  int days = getInputDays();
  int offset = getInputOffset();
  // use input information to create calendar display
  drawCalendar(days, offset);
}

First Welcome to Code Review, and nice first question for a beginner. You did
a good job with variable names, function names, comments and indentation,
although validInput = true; is incorrectly indented in both functions. The
function drawCalendar() is very inconsistent in the indentation.

Consider the User
You might want to add an error message such as
"Invalid input please enter a number between X and Y" with X being the lower
limit and Y being the upper limit.

DRY Principle
There is a software engineering principle call the Don't Repeat Yourself
principle that may apply to the two input functions. Well written functions
can be used in multiple places in code and decrease the amount of code
necessary to implement a program or library. This code count contain a
function called getInput() that implements most of getInputDays() and
a lot of getInputOffset()

int getInput(string inputPrompt, int lowerBound, int upperBound)
{
  int inputValue = 0;
  bool validInput = false; // used to verify input is valid
  while (validInput == false)
  {
    std::cout << inputPrompt << " : ";
    std::cin >> InputValue;
    if (inputValue >= lowerBound && inputValue <= upperBound)
    {
      validInput = true;
    }
    else
    {
      std::cout << "Invalid input, please enter a number between"
          << lowerBound << " and " << upperBound << "\n";
    }
  }
  return InputValue;
}

Reducing the amount of code is a benefit because it makes it easier to
write the code in the first place, modify the code later and debug the
code.

You did well following another software engineering called the Single
Responsibility Principle.
The single responsibility principle states that every module or class should
have responsibility over a single part of the functionality provided by the
software, and that responsibility should be entirely encapsulated by the
class. All its services should be narrowly aligned with that responsibility.

When raw numbers are included in code they can often be confusing. Also if
the same number is used multiple times in the code for the same reason,
such as for array bounds it is easier to understand the code and modify
the code using symbolic constants. Some examples that might apply to
this program

//Symbolic constants should be capitalized, multiple words separated
// by underscore.
constexpr int SHORT_MONTH = 28;
constexpr int LONG_MONTH = 31;
constexpr int SUNDAY = 6;
constexpr int MONDAY = 0;
constexpr int TUESDAY = 1;
constexpr int SPACES_IN_COLUMN = 4;

Switch Statement Versus if then else if
The function displayOffset() could be implemented in the following manner:

void displayOffset(int offset)
// used to create offset on the first line of calendar
{
  switch (offset == 1)
    case TUESDAY :
      std::cout << "       1";
      break;
    case WEDNESDAY :
      std::cout << "           1";
      break;
   ...
  }
}

This is somewhat more readable and maintainable that multiple if then else if
statements. It is much easier to add a new case than it is to add a new
else if statement.

Another alternative would be:

void displayOffset(int offset)
// used to create offset on the first line of calendar
{
  for (int i = 0; i < offset * SPACES_IN_COLUMN; i++)
  {
    std::count << " ";
  }
}

Complex Functions
It might be easier to understand, debug and modify the drawCalendar()
function if it was broken up into sub-functions following the Single
Responsibility Principle mentioned above.


Update Map in Haskell I wrote a function that inserts or updates a Map key value depending on whether the key is already in the map. If it is, then the value gets added to the already existing map's value.

import Data.Map as Map

updateMap :: (Ord k, Num v) => k -> v -> Map k v -> Map k v
updateMap k v map = if member k map then Map.adjust (+ v) k map
                    else Map.insert k v map

Tests

*Main> updateMap 1 1 $ Map.singleton 1 100
fromList [(1,101)]
*Main> updateMap 2 1 $ Map.singleton 1 100
fromList [(1,100),(2,1)]
*Main> updateMap 1 33 $ Map.singleton 1 100
fromList [(1,133)]

Please critique.

updateMapPF :: (Ord k, Num a) => k -> a -> Map k a -> Map k a
updateMapPF = Map.insertWith (+)

λ> updateMapPF 1 1 $ Map.singleton 1 100
fromList [(1,101)]
λ> updateMapPF 2 1 $ Map.singleton 1 100
fromList [(1,100),(2,1)]
λ> updateMapPF 1 33 $ Map.singleton 1 100
fromList [(1,133)]

Process:

Notice the use of Map.insert

Notice that you are mapping (+) over the possibility of having or not having a value

Notice that Data.Map has a function for applying functions to values, called Data.Map.insertWith. It's extremely common for collections to have a helper function for "insert data with a helper function / default value"

Notice the shared structure of the types of your function and insertWith

insertWith :: Ord k => (a -> a -> a) -> k -> a -> Map k a -> Map k a

updateMap  :: (Ord k, Num v) =>         k -> v -> Map k v -> Map k v

Realize that applying (+) to insertWith will make the types identical

λ> :t insertWith

insertWith
  :: Ord k => (a -> a -> a) -> k -> a -> Map k a -> Map k a

λ> :t insertWith (+)

insertWith (+) :: (Ord k, Num a) => k -> a -> Map k a -> Map k a

Validate assumptions with specific examples, more rigor is available with the use of QuickCheck.

λ> updateMapPF 1 1 $ Map.singleton 1 100
fromList [(1,101)]

λ> updateMapPF 2 1 $ Map.singleton 1 100
fromList [(1,100),(2,1)]

λ> updateMapPF 1 33 $ Map.singleton 1 100
fromList [(1,133)]

This is a practically golden opportunity to demonstrate how eta reduction can simplify code and kill off redundant logic.


Reading elements for a vector until EOL I'm a novice to programming completely. I'm trying to teach myself C++. I want to write code to create two vectors and read their elements from input.

vector<int> v1, v2;
/*Reading vector values*/
cout << "Enter the elements of 1st vector" << endl;
int ele;
while (cin >> ele)
    v1.push_back(ele);
cout << "enter the elements of 2nd vector" << endl;
while (cin >> ele)
    v2.push_back(ele);

Though it basically does what I coded it for, I want to have some improvements.

When I input elements into a vector, I would like Enter key to mean that I'm done entering the vector, while a space key says I'm entering the next element.
How can I write a loop for vector 1 and vector 2, as the code for entering elements is the same for both the vectors? (I want to write a loop for reading the two vectors.)

Perhaps the simplest solution is to read the input line and parse it. You are also correct it is a good idea to write a function for reading a vector.

Consider the following:

#include <iostream>
#include <string>
#include <sstream>
#include <vector>
#include <iterator>

std::vector<int> read_vector()
{
    std::string line = "";
    std::getline(std::cin, line);
    std::istringstream iss(line);

    std::vector<int> v;

    int x = 0;
    while (iss >> x)
        v.push_back(x);

    return v;
}

int main()
{
    std::cout << "Enter the elements of 1st vector\n";
    std::vector<int> v1 = read_vector();

    std::cout << "Enter the elements of 2nd vector\n";
    std::vector<int> v2 = read_vector();

    std::cout << "v1 = { ";
    for (auto e : v1)
        std::cout << e << " ";

    std::cout << "}\nv2 = { ";
    for (auto e : v2)
        std::cout << e << " ";
    std::cout << "}";
}

The functionality of reading a vector is now found from read_vector; quite obviously, it reads the input string (until the delimiter, which is by default an end of the line), and reads space-separated integers, pushes them to the vector, and returns it.


Handling errors with minimal repetition when using Find function I have a function that essentially searches out a value in a table by looking through the column headers and row headers to locate a data point. The function takes the sheet name, the column header name, and row header name as arguments, and I have an error handler set up if any one of the methods performed with those variables fails. However, each handler is very similar, so there is much repetition. Is there a more clear way to have all errors handled uniquely but not to have to re-write Exit Function and other such things?

Function getDataPoint(rowItem As String, searchCol As Integer, colItem As String, searchRow As Integer, shtName As String) As Variant

    Dim rowNum As Integer, colNum As Integer
    Dim rowNumRng As Range, colNumRng As Range

    'Find worksheet to search
    On Error GoTo shtNotFound:
    With ThisWorkbook.Sheets(shtName)

        'Find rowNum where rowItem appears in searchCol
        On Error GoTo rowNotFound:
        Set rowNumRng = .Columns(searchCol).Find(What:=rowItem, _
            After:=.Cells(1, searchCol), _
            LookIn:=xlValues, _
            LookAt:=xlWhole, _
            SearchOrder:=xlByColumns, _
            SearchDirection:=xlNext, _
            MatchCase:=True)

        rowNum = rowNumRng.Row

        'Find colNum where colItem appears in searchRow
        On Error GoTo colNotFound:
        Set colNumRng = .Rows(searchRow).Find(What:=colItem, _
            After:=.Cells(searchRow, 1), _
            LookIn:=xlValues, _
            LookAt:=xlWhole, _
            SearchOrder:=xlByRows, _
            SearchDirection:=xlNext, _
            MatchCase:=True)

        colNum = colNumRng.Column

        getDataPoint = .Cells(rowNum, colNum).Value

    End With
    On Error GoTo 0
    Exit Function

 shtNotFound:
    Debug.Print "Sheet not found"
    getDataPoint = "NOT FOUND"
    Exit Function
 rowNotFound:
    Debug.Print "Row item not found"
    getDataPoint = "NOT FOUND"
    Exit Function
 colNotFound:
    Debug.Print "Column item not found"
    getDataPoint = "NOT FOUND"
    Exit Function

End Function

If you absolutely need to use your Find approach, or if you'd just like to see a centralized error handler, as per you question, then you can use a variable to store the custom error message, and then refer to that variable from the error handler:

Also, in you On Error statement, you don't need the trailing ":" as that indicates that the line will have another statement following the ":"

Function getDataPoint(rowItem As String, searchCol As Integer, colItem As String, searchRow As Integer, shtName As String) As Variant

  Dim rowNum As Integer, colNum As Integer
  Dim rowNumRng As Range, colNumRng As Range
  Dim sError As String

  'Find worksheet to search
  sError = "Sheet not found"
  On Error GoTo itmNotFound
  With ThisWorkbook.Sheets(shtName)
    'Find rowNum where rowItem appears in searchCol
    sError = "Row not found"
    Set rowNumRng = .Columns(searchCol).Find(What:=rowItem, _ After:=.Cells(1, searchCol), _ LookIn:=xlValues, _ LookAt:=xlWhole, _ SearchOrder:=xlByColumns, _ SearchDirection:=xlNext, _ MatchCase:=True)
    rowNum = rowNumRng.Row
    'Find colNum where colItem appears in searchRow
    sError = "Column not found"
    Set colNumRng = .Rows(searchRow).Find(What:=colItem, _ After:=.Cells(searchRow, 1), _ LookIn:=xlValues, _ LookAt:=xlWhole, _ SearchOrder:=xlByRows, _ SearchDirection:=xlNext, _ MatchCase:=True)
    colNum = colNumRng.Column
    getDataPoint = .Cells(rowNum, colNum).Value
  End With
  On Error GoTo 0
  Exit Function

itmNotFound:
  Debug.Print sError
  getDataPoint = "NOT FOUND"

End Function


Simple jQuery Slider Plugin I'm writing a very simple fading slider plugin for my own use. I would like to know what I could do to improve upon what I've already done.

I think I've done well, but I am certain there are things that I can do to improve.

(function($){
    $.fn.slider = function(options){
        var settings = $.extend({
            selector:this,
            speed:3000
        }, options);

        var $children = settings.selector.children();//Check the children from the element calling this function
        var $currentChildIndex = 0;//Set the current Index to 0 since we've not started yet.
        var $current = $($children[$currentChildIndex]);//Get the current child (Redundant?)

        setInterval(function(){
            $current = $($children[$currentChildIndex]);//Assign current from list of children using currentChildIndex

            $current.addClass('active');//Assign active to current slider

            $children.each(function(key,obj){
                if(!$current.is($(obj))){//Remove all active elements that aren't the current slider
                    $(obj).removeClass('active');
                }
            });

            if($currentChildIndex==($children.length-1)){//Check if Index has reached the end, if so reset to start
                $currentChildIndex=0;
            }else{
                $currentChildIndex++;
            }
        },settings.speed);//Wait every X seconds before changing slide
    }
}(jQuery));

I think you can remove all of the comments you have here, as I think you are already using mostly meaningful variable names (a few minor comments on this below) and the code is clear in intent.  The comments here mostly seem like clutter. This is not to mention that comments at the end of a line of code are generally hard to read.  Comments should typically be before the line/section of code to which they apply on their own line(s).
Typically jQuery plug-ins are applied like $([selector]).slider(). Such that inside the plugin code this refers to a jQuery collection.  You are not treating this as a collection but rather as an individual element.  Think of this as a collection.  Also, I don't think selector setting is relevent for this reason.  Typically element selection has already been applied by the point you are in the plug-in code.  Keeping this philosophy of potentially working with a collection of elements, would also allow you to invoke multiple sliders with one pass.

For example:

// every element with slider class will be turned into a slider
$('.slider').slider();

Typically jQuery plug-ins are designed to maintain chaining so a return statement should be applied to allow the collection that the plug-in is acting against (or some filtered subset of it depending on the logic of the plug-in) to be returned for further method chaining.
Should $children be named $slides to indicate the selected children represent the slides to be displayed?
Regarding var $currentChildIndex, why not just currentChildIndex?  The styling approach of prepending $ to variables is typically to help identify jQuery collections.  This is not a jQuery collection. This variable name is probably also unnecessarily long.  Perhaps just currentIndex which still conveys clear meaning.
I don't think you need $current at all.  You can directly reference members of the $children collection directly using $children.eq(currentIndex)
No need to iterate $children to remove class from inactive children.  You can simplify this.

For example:

$children.removeClass('active');
$children.eq(currentIndex).addClass('active');

You can simplify your if-else (to avoid unnecessary else code paths).

For example:

currentChildIndex++;
if(currentChildIndex === $children.length) {
    currentChildIndex = 0;
}

I would recommend using strict comparisons (===, !==) as your default means to compare rather than loose comparisons.  Loose comparisons tend to introduce bugs around truthy/falsey behavior (though in this case there should not be concern as you are clearly only comparing integer values).  IMO, this is just a good habit to get into with one's coding.  Only use loose comparisons where there is a specific reason to do so.

Putting it all together you might have something like:

(function($){
    $.fn.slider = function(options){
        var settings = $.extend({
            speed:3000
        }, options);

        return this.each(function() {
            var $slides = $(this).children();
            var currentIndex = 0;

            setInterval(function(){
                $slides.removeClass('active');
                $slides.eq(currentIndex).addClass('active');
                currentIndex++;
                if (currentIndex === $slides.length) {
                    currentIndex = 0;
               }
            },settings.speed);
        });
    }
}(jQuery));


Using objects to make state with AJAX functions as properties I'm using bootstrap 3.0 to display a tooltip (this example isn't specific to boot-strap) where the html inside the tooltip is fed from an AJAX call.  In the future I want to be able to use the same element to fire different AJAX calls (something like fire a different AJAX call if the shift+click was used).  To make that easy, I decided to use a tooltipManager object.  This object currently has two properties:

title: in bootstrap, this is the html represented when an element is hovered over
placement: the values for this are left,right,top,bottom

EDIT: I just got the program working, but I need to know WHY it's working because I tried something like this for hours and had nothing but bad luck.  Why is setPlacement able to return the correct value?

 $(document).ready(function () {
        //in the future there will be n amount of these
        //each will have the ability to use different urls to make different AJAX calls (not implemented yet)
        var tooltipManager = {
            title: function () {
                //ajax code to get title from database
                $.ajax({
                    type: "POST",
                    contentType: "application/json",
                    url: "Service.asmx/GetDrugs",
                    dataType: "json",
                    success: function (data) {
                        //bootstrap uses the title attribute to set the html inside the tooltip
                        //here it's set to the results of the AJAX
                        $('#click').attr('title', data.d)
                                   .tooltip({
                                       trigger: 'click',
                                       html: false,
                                       //using the sizer function the tooltip is placed where it needs to be.
                                       placement: tooltipManager.placement.setPlacement(data.d)
                                       //it seems to me that it would be better design to call the tooltipManager
                                       //setPlacement function, but since it's an async request, it fails

                                   }).click();
                    },
                    error: function (xhr) {
                        console.log('failed: ' + xhr.status);
                    }
                });

            },
            placement: {
                left: 'left',
                top: 'top',
                right: 'right',
                bottom: 'bottom',
                setPlacement: function (data) {
                    if (data.length > 4) {
                        return this.bottom;
                    }
                    else {
                        return this.right;
                    }
                }
            }
        }
        $('#click').one('click', function () {
            //how can I pass the entire state of the object here, instead of invoking these methods one by one.
            tooltipManager.title();
        });
        //not sure if this is good design to have this not a property of the tooltipManager object
        //this works currently for placing the tooltip in the correct position
        function sizer(data) {
            if (data.length > 4) {
                return tooltipManager.placement.bottom
            }
            else {
                return tooltipManager.placement.right
            }
        }

    });

Your setPlacement method returns the "correct" data because whatever d is in your data object you receive from Service.asmx/GetDrugs is compared to be greater than 4. If it is then it will set the tooltip placement of the $('#click') element to bottom, otherwise it will set it to right. You can check it out by sticking this code at the bottom of the page:

alert('this is 3 characters: ' + tooltipManager.placement.setPlacement('asd'));
alert('this is 5 characters: ' + tooltipManager.placement.setPlacement('asdfg'));

All that placement does is it returns the words "left","top","right", or "bottom" depending on what it's set to in the placement object. You can change that object to this and it will still work:

placement: {
    l: 'left',
    t: 'top',
    r: 'right',
    b: 'bottom',
    setPlacement: function (data) {
        if (data.length > 4) {
            return this.b;
        }
        else {
            return this.r;
        }
    }
}

The setPlacement method doesn't actually set anything. What does is your jQuery's tooltip method.

You can change up all of line 48 like this (you also had a typo):

$('#click').on('click', tooltipManager.title);

You can delete the sizer function as I don't see a use for it. Something like that should be a method of the tooltipManager object anyways. On the same subject, since setPlacement doesn't actually set anything, you should probably change it to getPlacement or calcPlacement. It's more semantic and will help other developers that come behind you.

I would personally redo the entire tooltipManager.placement object to show the actual data that would be unknown. As an example, why on Earth would you need a static object to tell you what the number 2 is? Same for the string 'bottom'. tooltipManager.placement.bottom is the same thing as 'bottom'. Here's how I would structure that:

var tooltipManager = {
    setTitle: function () {
        $.ajax({
            type: "POST",
            contentType: "application/json",
            url: "Service.asmx/GetDrugs",
            dataType: "json",
            success: function (data) {
                $('#click').attr('title', data.d)
                .tooltip({
                    trigger: 'click',
                    html: false,
                    placement: tooltipManager.setPlacement(data.d)
                }).click();
            },
            error: function (xhr) {
                console.error('Failed tooltip request: ' + xhr.status);
            }
        });
    },
    setPlacement: function (data) {
        if (data.length > 4) {
            tooltipManager.placement = 'bottom';
            return 'bottom';
        } else {
            tooltipManager.placement = 'right';
            return 'right';
        }
    }
}

$('#click').on('click', tooltipManager.setTitle);

Note: I actually kept the setPlacement method, I just changed it so that it actually sets data. You can access the placement of the tooltip in javascript with tooltipManager.placement if you want. If you intend on having multiple tooltips you can just convert this all into a class. Also, I changed the tooltipManager.title method into tooltipManager.setTitle because it actually sets data.


Javascript Object Placement / String Parsing Method This JS function is intended to retrieve or place a value into an object with the nested keys as a string.

For example

var obj = {a: {b: [4]}};
parse_obj_key(obj, "a.b.0") should equal 4.
parse_obj_key(obj, "a.c", 2) should add another element to "a" named "c" with value 2.

The method works, but I'd like to clean it up if possible (or a more polished implementation which is publicly available).  I'd also love to know of any edge-case failures which can be found.

function parse_obj_key(obj, loc, val){
    var _o = obj;
    while (true){
        var pos = loc.indexOf('.');

        if (!_o || typeof _o != 'object'){
            $.log("Invalid obj path: " + loc + "\n" + JSON.stringify(obj));
            return null;
        }

        if (pos === -1){
            if (val){
                _o[loc] = val;
                return obj;
            } else {
                if (!isNaN(parseInt(loc)))
                    loc = parseInt(loc);

                return _o[loc];
            }
        }
        var part = loc.substring(0, pos);
        var loc = loc.substring(pos + 1);

        if (!isNaN(parseInt(part)))
            part = parseInt(part);

        if (!(part in _o)){
            if (val)
                _o[part] = new object;
            else
                return null;
        }
        _o = _o[part];
    }
}

Here is what's wrong with your code:

Do not use variable names like _o. Get an editor with good auto-completion.
typeof _o != 'object' does not do what you think it does: typeof([1,2]) // "object".
In general, doing those kinds of checks is a code smell.
if (!isNaN(parseInt(loc))) loc = parseInt(loc);. Confusing and not needed.
JavaScript: ['a', 'b']["1"] // 'b'. Same goes for the other isNaN
in. Do not do that check. null is a value, but what you want to return is the lack of value. It is undefined in JavaScript, and it is what will be returned if there is no value.
Consider using split instead of indexOf and substring. It is much faster and makes the code more readable.

So, here is a neat version for you:

function chained(obj, chain, value){
    var assigning = (value !== undefined);
    // split chain on array and property accessors
    chain = chain.split(/[.\[\]]+/);
    // remove trailing ']' from split
    if (!chain[chain.length - 1]) chain.pop();
    // traverse 1 level less when assigning
    var n = chain.length - assigning;
    for (var i = 0, data = obj; i < n; i++) {
        data = data[chain[i]];
        // if (data === undefined) return; // uncomment to handle bad chain keys
    }

    if (assigning) {
        data[chain[n]] = value;
        return obj;
    } else {
        return data;
    }
}

Blogged: http://glebm.blogspot.com/2011/01/javascript-chained-nested-assignment.html

Please come up with further improvements :)


Find allele frequencies at each site for each iteration for each population from FASTA file The script takes a FASTA format file in input and outputs the frequencies of each amino acid (A, C, T and G) at each site per population per iteration. The name of each sequence looks like >it0pop1ind0locus0. In this example, this indicates that the sequence that follows is the DNA sequence for the zeroth iteration (it), the first population (pop), the zeroth individual (ind) at the zeroth locus (locus).

My goal (just to restate it) is to get the frequencies of each amino acid (A, C, T and G) at each site per population per iteration.

from __future__ import division
import os
from Bio import SeqIO
import itertools
import re
import sys
os.chdir("/path/to/directory/")

#### ReFormat the data ####

maxit = 1
maxpop = 1
maxind = 1
maxlocus = 1
for pos, i in enumerate(SeqIO.parse("0.0.1.fasta", "fasta")):
    if pos == 0:
        maxsite = len(i.seq)
    v1 = re.findall(r'\d+', i.id)
    v = []
    for val in v1:
        v.append(int(val))
    maxit = max(v[0]+1, maxit)
    maxpop = max(v[1]+1, maxpop)
    maxind = max(v[2]+1, maxind)
    maxlocus = max(v[3]+1, maxlocus)

all = [[[[ "x" * maxsite for locus in xrange(maxlocus)] for ind in xrange(maxind)] for pop in xrange(maxpop)] for it in xrange(maxit)]

for i in SeqIO.parse("0.0.1.fasta", "fasta"):
    v1 = re.findall(r'\d+', i.id)
    v = []
    for val in v1:
        v.append(int(val))
    it = v[0]
    pop = v[1]
    ind = v[2]
    locus = v[3]
    all[it][pop][ind][locus] = str(i.seq)

#### Calculate the frequencies ####

pA = [[[2 for it in xrange(maxsite)] for pop in xrange(maxpop)] for site in xrange(maxit)]
pC = [[[2 for it in xrange(maxsite)] for pop in xrange(maxpop)] for site in xrange(maxit)]
pG = [[[2 for it in xrange(maxsite)] for pop in xrange(maxpop)] for site in xrange(maxit)]
pT = [[[2 for it in xrange(maxsite)] for pop in xrange(maxpop)] for site in xrange(maxit)]

for it in xrange(maxit):
    for pop in xrange(maxpop):
        for locus in xrange(maxlocus):
            for site in xrange(maxsite):
                x = []
                for ind in xrange(maxind):
                    x.append(all[it][pop][ind][locus][site])
                pA[it][pop][site] = x.count("A")/maxind
                pT[it][pop][site] = x.count("T")/maxind
                pC[it][pop][site] = x.count("C")/maxind
                pG[it][pop][site] = x.count("G")/maxind

Here are the values I get:

>>> pA
[[[0.16666666666666666, 0.0, 0.0, 0.0, 1.0, 0.0, 1.0, 0.6666666666666666, 0.0], [0.0, 0.0, 0.0, 0.0, 1.0, 0.0, 1.0, 1.0, 0.0]]]
>>> pC
[[[0.8333333333333334, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 0.0, 1.0], [1.0, 0.0, 0.0, 0.6666666666666666, 0.0, 0.0, 0.0, 0.0, 1.0]]]
>>> pG
[[[0.0, 1.0, 1.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0], [0.0, 1.0, 1.0, 0.3333333333333333, 0.0, 1.0, 0.0, 0.0, 0.0]]]
>>> pT
[[[0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.3333333333333333, 0.0], [0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0]]]

Here is the data for the example (FASTA). In this example, we have only 1 iteration (it), 2 populations (pop), 5 individuals (ind), 1 locus (locus) and 9 sites (length of the sequence).

>it0pop0ind0locus0
CGGCAGAAC
>it0pop0ind1locus0
CGGCAGATC
>it0pop0ind2locus0
CGGCAGAAC
>it0pop0ind3locus0
CGGCAGAAC
>it0pop0ind4locus0
CGGCAGATC
>it0pop0ind5locus0
AGGCAGAAC
>it0pop1ind0locus0
CGGCAGAAC
>it0pop1ind1locus0
CGGCAGAAC
>it0pop1ind2locus0
CGGGAGAAC
>it0pop1ind3locus0
CGGCAGAAC
>it0pop1ind4locus0
CGGGAGAAC
>it0pop1ind5locus0
CGGCAGAAC

let me choose

First things first; your code is chdiring to /path/to/directory/ and opening 0.0.1.fasta. This is woefully inflexible.

The simplest fix is to accept an argument on the command line, as sys.argv[1]. (Don't use chdir.)

numpy

What you're doing looks a lil' bit crazy. To deal with highly-dimensional data, Numpy is often a good candidate.

A first-pass would look something like

from __future__ import division

import os
from Bio import SeqIO
import itertools
import re
import sys
import numpy

#### ReFormat the data ####

filename = sys.argv[1]

maxit = 1
maxpop = 1
maxind = 1
maxlocus = 1
for pos, i in enumerate(SeqIO.parse(filename, "fasta")):
    print(i.seq)

    if pos == 0:
        maxsite = len(i.seq)

    it, pop, ind, locus = [int(d) for d in re.findall(r'\d+', i.id)]

    maxit = max(it+1, maxit)
    maxpop = max(pop+1, maxpop)
    maxind = max(ind+1, maxind)
    maxlocus = max(locus+1, maxlocus)

all = numpy.full((maxit, maxpop, maxind, maxlocus, maxsite), '\0', dtype="S")

for i in SeqIO.parse(filename, "fasta"):
    it, pop, ind, locus = [int(d) for d in re.findall(r'\d+', i.id)]
    all[it, pop, ind, locus] = i.seq

print(all)
print("---")
#### Calculate the frequencies ####

pA = numpy.full((maxit, maxpop, maxsite), 2)
pC = numpy.full((maxit, maxpop, maxsite), 2)
pG = numpy.full((maxit, maxpop, maxsite), 2)
pT = numpy.full((maxit, maxpop, maxsite), 2)

for it in xrange(maxit):
    for pop in xrange(maxpop):
        for locus in xrange(maxlocus):
            for site in xrange(maxsite):
                x = []
                for ind in xrange(maxind):
                    x.append(all[it, pop, ind, locus, site])
                pA[it, pop, site] = x.count("A") / maxind
                pT[it, pop, site] = x.count("T") / maxind
                pC[it, pop, site] = x.count("C") / maxind
                pG[it, pop, site] = x.count("G") / maxind

print(pA)
print(pC)
print(pG)
print(pT)

Then one can use Numpy tools like fancy slicing

from __future__ import division

import os
import re
import sys

import numpy

from Bio import SeqIO

def parse_id(string_id):
    return tuple(map(int, re.findall(r'\d+', string_id)))

def main():
    filename = sys.argv[1]

    data = list(SeqIO.parse(filename, "fasta"))
    header_keys = numpy.array([parse_id(datum.id) for datum in data])

    maxit, maxpop, maxind, maxlocus = header_keys.max(axis=0) + 1
    maxsite = len(data[0].seq)

    all = numpy.full((maxit, maxpop, maxind, maxlocus, maxsite), '\0', dtype="S")

    for header_key, sequence in zip(header_keys, data):
        all[tuple(header_key)] = sequence

    #### Calculate the frequencies ####
    pA = numpy.full((maxit, maxpop, maxsite), 2)
    pC = numpy.full((maxit, maxpop, maxsite), 2)
    pG = numpy.full((maxit, maxpop, maxsite), 2)
    pT = numpy.full((maxit, maxpop, maxsite), 2)

    for locus in xrange(maxlocus):
        for site in xrange(maxsite):
            x = all[..., locus, :]
            pA[...] = (x == "A").mean(axis=-2)
            pT[...] = (x == "T").mean(axis=-2)
            pC[...] = (x == "C").mean(axis=-2)
            pG[...] = (x == "G").mean(axis=-2)

    print(pA)
    print(pC)
    print(pG)
    print(pT)

main()

Note that

for site in xrange(maxsite):

is pointless as you don't use site and the effect is idempotent. Note then that

for locus in xrange(maxlocus):
    x = all[..., locus, :]
    pA[...] = (x == "A").mean(axis=-2)
    pT[...] = (x == "T").mean(axis=-2)
    pC[...] = (x == "C").mean(axis=-2)
    pG[...] = (x == "G").mean(axis=-2)

is just

x = all[..., -1, :]
pA[...] = (x == "A").mean(axis=-2)
pT[...] = (x == "T").mean(axis=-2)
pC[...] = (x == "C").mean(axis=-2)
pG[...] = (x == "G").mean(axis=-2)

Then one can skip the pre-initialization (pA = numpy.full(...)) and just do

x = all[..., -1, :]
pA = (x == "A").mean(axis=-2)
pT = (x == "T").mean(axis=-2)
pC = (x == "C").mean(axis=-2)
pG = (x == "G").mean(axis=-2)

This allows removing even more of the code.

intermission

Don't use all as a variable name; it clashes with the built-in (same for id).

finishing up

I feel this is a bit awkward still, but it's nicer than before.

from __future__ import division

import os
import re
import sys

import numpy

from Bio import SeqIO

def parse_id(string_id):
    return tuple(map(int, re.findall(r'\d+', string_id)))[:3]

def main():
    filename = sys.argv[1]

    data = list(SeqIO.parse(filename, "fasta"))
    header_keys = numpy.array([parse_id(datum.id) for datum in data])

    maxit, maxpop, maxind = header_keys.max(axis=0) + 1
    maxsite = len(data[0].seq)

    sequences = numpy.full((maxit, maxpop, maxind, maxsite), '\0', dtype="S")
    for header_key, sequence in zip(header_keys, data):
        sequences[tuple(header_key)] = sequence

    # Calculate frequencies
    pA = (sequences == "A").mean(axis=-2)
    pT = (sequences == "T").mean(axis=-2)
    pC = (sequences == "C").mean(axis=-2)
    pG = (sequences == "G").mean(axis=-2)

    print(pA)
    print(pC)
    print(pG)
    print(pT)

main()


Lua/Love2D Resource Loader (Queue) I made this resource queue in Love2D.

function GameState:queue(file, arg)
    self.res.loaded = self.res.loaded + 1;
    self.res.queue[self.res.loaded] = {f = file, t = game.getResourceType(file), id = self.res.loaded, s = arg};
    game.log("Resource Queued: "..file);
    return {f = self.res.loaded, t = game.getResourceType(file), id = self.res.loaded, s = arg};
end

function GameState:loadQueue(loader, post)
    self.post = post or function() self.state = self.prevState end;

    game.log("Loading Resource Queue...");

    queueThread = love.thread.newThread("queue.lua");
    queueChannel = love.thread.getChannel("queue");

    local fonts = {};

    for k, v in pairs(self.res.queue) do
        if (not v or not v.f or not v.t) then goto continue end;
        if (v.t ~= self.RESOURCE_FONT) then
            queueChannel:push(v);
        else
            fonts[k] = v;
        end
        ::continue::
    end

    for k, v in pairs(fonts) do
        self.res.fnt[k] = love.graphics.newFont(v.f, v.s);
        print("Loaded Resource: "..v.f);
    end

    queueChannel:push(table.size(self.res.queue)-table.size(fonts));

    if (table.size(self.res.queue)-table.size(fonts) > 0) then
        queueThread:start();
    else
        queueChannel = nil;
        queueThread = nil;
        return;
    end

    self.state = self.STATE_LOADING
end

The thread:

require "love.image"
require "love.audio"
require "love.sound"
require "love.timer"

queue = {};
img = {};
snd = {};
local queueChannel = love.thread.getChannel("queue");

local pop = queueChannel:pop();

while (type(pop) ~= "number" and pop) do
    queue[pop.id] = pop;
    pop = queueChannel:pop();
end

for k, v in pairs(queue) do
    if (not v or not v.f or not v.t) then print("Resource Load Failed!"); goto continue; end;

    if (v.t == 0) then -- image
        img[k] = love.image.newImageData(v.f);
        queueChannel:supply({data = img[k], t = 0, id = k, s = v.s});
    end
    if (v.t == 1) then -- sound
        snd[k] = love.audio.newSource(v.f, "static");
        queueChannel:supply({data = snd[k], t = 1, id = k, s = v.s});
    end
    ::continue::
    queue[k] = nil;
    print("Loaded Resource: "..v.f);
    love.timer.sleep(0.01);
end

queueChannel:clear();
queueChannel:push(true);

return;

Handling while loading resources:

if (self.state == self.STATE_LOADING) then
    local e = queueThread:getError();
    if (e ~= nil) then
        print("Thread: "..e)
    end

    local r = queueChannel:peek();
    if (type(r) == "table" and r and r.data and r.t) then
        if (r.t == self.RESOURCE_IMAGE) then
            if (r.s) then
                love.graphics.setDefaultFilter(r.s, r.s);
            end
            self.res.img[r.id] = love.graphics.newImage(r.data);
        end
        if (r.t == self.RESOURCE_SOUND) then
            self.res.snd[r.id] = r.data;
        end
        queueChannel:pop();
    end

    if (queueChannel:peek() == true and type(queueChannel:peek()) == "boolean") then
        self.post();
        game.log("Finished Loading Resources.");
    end
end

If there's any bad habits, or anything that could be improved, please let me know.

In Lua, it's not necessary to separate statements by a semicolon (it's not even necessary to do it when writting several statements in the same line). However it's possible to do it.

In this snippet:

for k, v in pairs(self.res.queue) do
    if (not v or not v.f or not v.t) then goto continue end;
    if (v.t ~= self.RESOURCE_FONT) then
        queueChannel:push(v);
    else
        fonts[k] = v;
    end
    ::continue::
end

You're using pairs in several parts of the code to iterate tables which are actually arrays. Variable self.res.queue is an array since:

function GameState:queue(file, arg)
    self.res.loaded = self.res.loaded + 1;
    self.res.queue[self.res.loaded] = <object>;
end

In the cases a table is actually an array is preferred to use ipairs because is way more faster than pairs. In addition to that, it tells other people reading the code the table is actually an array (which is a valuable hint in a dynamic language such as Lua).

At the bottom of GameState:loadQueue:

if (table.size(self.res.queue)-table.size(fonts) > 0) then
    queueThread:start(); else
    queueChannel = nil;
    queueThread = nil;
    return;
end

self.state = self.STATE_LOADING

The return statement in the else branch can be removed if you move self.state up to the then branch. The semantics hold as either queueThread:start() or not.

if (table.size(self.res.queue)-table.size(fonts) > 0) then
    self.state = self.STATE_LOADING;
    queueThread:start();
else
    queueChannel = nil;
    queueThread = nil;
    return;
end


Better way to count the number of occurrences of a maximum number in the array in Ruby I am trying to write a program to get the count of the maximum number in an array of integers. The below code is what I have written. Is there any better way to write this code in Ruby?
def max_number_count(my_array)
    max_num, max_num_count = my_array.inject([0, 0]) {
                               |arr, age| age > arr[0] ?
                               [arr[0] = age, arr[1] = 1] : age == arr[0] ?
                               arr[1] += 1 : arr[1] = arr[1];
                               arr
                             }
    max_num_count
end

max_number_count([1,2,3,3,3])
#3
```

Your method shouldn't call the given array my_array. It's not the method's array, it's the caller's array. Just call it array, which is also shorter, so less clutter.
And you're making it very complicated. The array already offers all you need:
def max_number_count(array)
  array.count(array.max)
end

That's also much faster:
            user     system      total        real
yours   1.696091   0.034977   1.731068 (  3.476805)
mine    0.074057   0.000000   0.074057 (  0.131310)

Benchmark code:
require 'benchmark'

def max_number_count1(my_array)
    max_num, max_num_count = my_array.inject([0, 0]) {
                               |arr, age| age > arr[0] ?
                               [arr[0] = age, arr[1] = 1] : age == arr[0] ?
                               arr[1] += 1 : arr[1] = arr[1];
                               arr
                             }
    max_num_count
end

def max_number_count2(array)
  array.count(array.max)
end

n = 5
array = (1..1000).to_a * 1000
Benchmark.bmbm do |x|
  x.report("yours") { n.times { max_number_count1(array) } }
  x.report("mine")  { n.times { max_number_count2(array) } }
end


Project Euler #4 - Finding the largest palindrome that is the product of two three-digit numbers. This is my attempt at the 4th problem on Project Euler:

#include "stdafx.h"
#include <string>
#include <vector>
#include <iostream>

using namespace std;

int palindromeCheck(int pp) {
    int temp = pp;
    string palindrome = std::to_string(pp);

    auto it1 = palindrome.begin();
    auto it2 = palindrome.end() - 1;

    while (it2 - it1 >= 1) {
        if (*it1 == *it2) {
            ++it1;
            --it2;
        }
        else {
            return 0;
        }
    }
    return 1;
}

int main()
{
    int highestPalindrome = 0;
    for (int x = 100; x <= 999; x++) {
        for (int y = 100; y <= 999; y++) {
            int newDigit = x * y;
            int paliCheck = palindromeCheck(newDigit);
            if (paliCheck == 1) {
                if (newDigit > highestPalindrome) {
                    highestPalindrome = newDigit;
                }
            }
        }
    }
    std::cout << highestPalindrome;
}

I'm trying to get better at both C++ and problem-solving so any help/tips are appreciated.

using namespace std; is a bad practice.  You should should spell out either what you are using, or spell out explicitly the std:: when you are using something.  You are doing the latter already with std::to_string and std::cout, just not with string.

Your palindromeCheck function should return a bool, instead of an int with specific integers having certain meanings.

temp is not used.

You are checking both 100 * 101 and 101 * 100.   Multiplication is commutative, so you can cut your work in half by adjusting your inner y loop to only use number from a range which hasn't been checked yet (from x to 999, inclusive).

newDigit is an odd variable name for the product of x * y.

palindromeCheck(newDigit) is an expensive operation.  newDigit > highestPalindrome is a relatively cheap test.  Consider reversing the order of these tests to avoid calling an expensive function when the result will never be used.


Determine if a String can be parsed to LocalDateTime or Int I have the following kotlin function that takes in a string "value", then attempts to parse "value" as both a LocalDateTime and an Int. It returns a Pair of the parsed value of "value" and an enum that specifies which of the three types it was determined to be.

enum class RangeValueType{
    Int,
    Date,
    String
}

fun parseValue(value: String): Pair<RangeValueType, Any> {
    val rangeDate: LocalDateTime
    val rangeInt: Int
    //try date first
    try {
        //ISO_LOCAL_DATE_TIME e.g. 2011-12-03T10:15:30 for Dec. 3rd, 2011 & 10:15 30 sec.
        //can't use basic date b/c e.g. 20111203 is indistinguishable from an int.
        rangeDate = LocalDateTime.parse(value)
        //it parsed to a date, so return the date
        return Pair(RangeValueType.Date, rangeDate)
    }catch (dtpe: DateTimeParseException) {
        //not a date
    }
    //try number next
    try{
        rangeInt = value.toInt()
        //it parsed to an int, so return the int
        return Pair(RangeValueType.Int, rangeInt)
    }catch (nfe: NumberFormatException)
    {
        //not an int
    }
    //not date or int, must be a string
    return Pair(RangeValueType.String, value)
}

My principal concern is that I am using caught exceptions as a means of flow control, which I understand to generally be a Bad Idea. I know C# has a "TryParse" method for exactly this kind of case, but I wasn't able to find anything for Kotlin / Java.

Is there a better way to parse the values into their proper types?

(Disclamer: I only know Kotlin theoretically)

You can avoid the second try/catch by using String.toIntOrNull instead of String.toInt.

And by extracting the LocalDateTime conversion into a separate function, that also returns null when it fails (which also would help fulfill the single responsibility principle), you could then use a functional approach with the safe call operator (?.), the Elvis operator (?:) and the run extension method:

enum class RangeValueType {
    INT,
    DATE,
    STRING
}

fun parseLocalDateTime(value: String): LocalDateTime? {
    try {
        return LocalDateTime.parse(value)
    } catch (dtpe: DateTimeParseException) {
        return null;
    }
}

fun parseValue(value: String): Pair<RangeValueType, Any> {
    return parseLocalDateTime(value)?.run { Pair(RangeValueType.DATE, this) } ?:
             value.toIntOrNull()?.run { Pair(RangeValueType.INT, this) } ?:
             value.run { Pair(RangeValueType.STRING, this) }
}

Runable version

(NB: enum constants should be written all capitalized so they are not confused with types.)