Simple SqlHelper which wraps ADO.NET methods I am creating a simple SqlHelper which will simplify the ADO.NET method usage. Can someone please review this class for any issue or missing something?

public static class SqlHelper
{
    public static async Task<int> ExecuteNonQueryAsync(string connectionString, CommandType cmdType, string cmdText,
        params SqlParameter[] commandParameters)
    {
        using (var connection = new SqlConnection(connectionString))
        {
            using (var command = new SqlCommand(cmdText, connection))
            {
                try
                {
                    command.CommandType = cmdType;
                    command.Parameters.AddRange(commandParameters);
                    connection.Open();
                    return await command.ExecuteNonQueryAsync();
                }
                finally
                {
                    connection.Close();
                }
            }
        }

    }

    public static async Task<SqlDataReader> ExecuteReaderAsync(string connectionString, CommandType cmdType,
        string cmdText, params SqlParameter[] commandParameters)
    {
        using (var connection = new SqlConnection(connectionString))
        {
            using (var command = new SqlCommand(cmdText, connection))
            {
                try
                {
                    command.CommandType = cmdType;
                    command.Parameters.AddRange(commandParameters);
                    connection.Open();
                    return await command.ExecuteReaderAsync();
                }
                finally
                {
                    connection.Close();
                }
            }
        }
    }

    public static async Task<object> ExecuteScalarAsync(string connectionString, CommandType cmdType, string cmdText,
        params SqlParameter[] commandParameters)
    {
        using (var connection = new SqlConnection(connectionString))
        {
            using (var command = new SqlCommand(cmdText, connection))
            {
                try
                {
                    command.CommandType = cmdType;
                    command.Parameters.AddRange(commandParameters);
                    connection.Open();
                    return await command.ExecuteScalarAsync();
                }
                finally
                {
                    connection.Close();
                }
            }
        }
    }

}

Nesting.
You can reduce nesting by stacking the using blocks, like this:
    using (var connection = new SqlConnection(connectionString))
    using (var command = new SqlCommand(cmdText, connection))
    {
        try
        {
            command.CommandType = cmdType;
            command.Parameters.AddRange(commandParameters);
            connection.Open();
            return await command.ExecuteNonQueryAsync();
        }
        finally
        {
            connection.Close();
        }
    }

I like your usage of var, and the fact that you've wrapped the IDisposable stuff in using blocks. However this is overkill:

    finally
    {
        connection.Close();
    }

Because the connection is wrapped in a using block, the connection will be cleanly closed and disposed, whatever the reason is for the code to reach the end of the using scope. In other words, you can also ditch the try block, and further reduce nesting:
    using (var connection = new SqlConnection(connectionString))
    using (var command = new SqlCommand(cmdText, connection))
    {
        command.CommandType = cmdType;
        command.Parameters.AddRange(commandParameters);
        connection.Open();
        return await command.ExecuteNonQueryAsync();
    }

That's because a using block is a language shortcut for... a try...finally block.

The XxxxxHelper Syndrome
The problem with a static class called SqlHelper, is that over time, it becomes a dumping ground for anything remotely related to SQL; it grows hair and tentacles, and becomes a huge mess before you even notice: "Helper" is simply too vague of a name to result in anything cleanly focused.
I would suggest renaming it to something like SqlCommandWrapper - then it's far less likely to become bloated with things that have nothing to do with executing a SqlCommand, but might be remotely related to SqlWhatever.


Mapping elements of nested list over values in a Python dict Suppose there's a 3-level nested list, for example,

[[['<s>', '<s>', 'eu', 'rejects', 'german'], ['<s>', 'eu', 'rejects', 'german', 'call']],\
 [['eu', 'rejects', 'german', 'call', 'to'], ['rejects', 'german', 'call', 'to', 'boycott']]]

level 1: sentence level
level 2: context window level
level 3: word/token level

Task: I'd like to map each token to its corresponding index with a pre-defined Python dictionary (e.g, word2index['xx']=4).

Output example:

[[[2522, 2522, 475, 1620, 397], [2522, 475, 1620, 397, 439]],\
 [[475, 1620, 397, 439, 4], [1620, 397, 439, 4, 1443]]]

The following is what I've done,

def word2index(input, model):
    """ Map words to index
    """
    input2index = []
    for sent in input:
        sent2index = []
        for window in sent:
            window2index = []
            for elem in window:
                try:
                    index = model.vocab[elem].index
                    window2index.append(index)
                except KeyError:
                    unk = model.vocab['unk'].index
                    window2index.append(unk)
            sent2index.append(window2index)
        input2index.append(sent2index)
    return input2index

which involves lots of loops and creations of new list as intermediate containers. It'd be very easy to lose the track forgetting which container to append current data, especially when the nested list gets deeper.

Question: Would there be a more cleaner way to code such task?

You can use the dict.get(key, default) method to provide a default value instead of catching an Exception. This makes your computation a single expression, which is good for further inlining:

for elem in window:
    index = model.vocab.get(elem, unk).index
    window2index.append(index)

Which can be rewritten as:

window2index = [model.vocab.get(elem, unk).index for elem in window]

Which is faster in Cpython, and fairly straightforward.

Now, of course, you have a similar pattern for sent, and a similar pattern for input:

input2index = [EXPR-USING-sent for sent in input]

sent2index = [EXPR-USING-window for window in sent]

window2index = ... see above ...

So you can stack them all together as:

unk = model.vocab['unk']
input2index = [EXPR-USING-sent for sent in input]
return input2index

which becomes:

unk = model.vocab['unk']
input2index = [[EXPR-USING-window for window in sent] for sent in input]
return input2index

which becomes:

unk = model.vocab['unk']
input2index = [[[model.vocab.get(elem, unk).index
                for elem in window]
                for window in sent]
                for sent in input]
return input2index


C++ generic double-dispatcher/visitor I'm trying to reduce the boilerplate code for the visitor/double dispatcher pattern. First I've created a way to enforce the dispatcher to declare the dispatch(T) function for every type supported and also a dispatch_gen(T) for the generic case which I don't need to implement. This is not too hard:

/*
A generic way to get the type of a base class using a tag function
*/
template <typename T, typename U>
T base_of(U T::*);

/*
Generic dispatcher
*/
template <typename... T>
struct double_dispatcher_impl;

template <typename Head>
struct double_dispatcher_impl<Head> {
    virtual void dispatch(Head*) = 0;
};

template <typename Head, typename... Tail>
struct double_dispatcher_impl<Head, Tail...> : public double_dispatcher_impl<Tail...> {
    using double_dispatcher_impl<Tail...>::dispatch;

    virtual void dispatch(Head*) = 0;
};

template <typename... T>
struct double_dispatcher;

template <typename Head, typename... Tail>
struct double_dispatcher<Head, Tail...> : public double_dispatcher_impl<Head, Tail...> {
    using Base = decltype(base_of(&Head::dispatch_id));
    using double_dispatcher_impl<Head, Tail...>::dispatch;

    virtual void dispatch_gen(Base* b) { b->dispatch(this); }
};

The dispatch_id() is just a hacky way to get the base class, it server as a tag. Now the base class for a visitable/dispatchable:

template <typename D>
struct double_dispatchable {
    virtual void dispatch(D* d) { }
    void dispatch_id() { }
};

This gives me a relatively "nice" usage. Here's a simple example:

struct Circle;
struct Rect;
struct Triangle;
typedef double_dispatcher<Circle, Rect, Triangle> shape_disp;

struct Shape : public double_dispatchable<shape_disp> { };
struct Circle : public Shape    { void dispatch(shape_disp* disp) { disp->dispatch(this); } };
struct Rect : public Shape      { void dispatch(shape_disp* disp) { disp->dispatch(this); } };
struct Triangle : public Shape  { void dispatch(shape_disp* disp) { disp->dispatch(this); } };

struct my_dispatcher : public shape_disp {
    void dispatch(Circle* c)    { std::cout << "Circle!" << std::endl; }
    void dispatch(Rect* c)      { std::cout << "Rect!" << std::endl; }
    void dispatch(Triangle* c)  { std::cout << "Triangle!" << std::endl; }
};

Now there's still a lot of repetitive code in there (the callbacks for the subclasses). How could I improve the pattern? Can I remove the repetitive code with metaprogramming and generics, without using macros?

I have implemented something like that in one of my projects, the trick is to use CRTP to pass the Child type to the parent.

This is a simplified code :

#include "collisions.h"
#define SHAPES Box, Sphere, Mesh
class Box;
class Sphere;
class Mesh;

class Geometry;

// Collidable defines all the pure virtual functions.
template <typename... Shapes>
struct Collidable;

template <>
struct Collidable<> {
    virtual bool collide(const Geometry* obj) const = 0 ;
};

template <typename Shape, typename... Others>
struct Collidable<Shape, Others...> : Collidable<Others...> {
    using Collidable<Others...>::collide;
    virtual bool collide(const Shape* obj) const = 0;
};

// Geometry inherits from Collidable and is our common base class.
class Geometry : public Collidable<SHAPES> {
    public:
        Geometry();
        virtual ~Geometry();
        glm::vec3 getPosition() const;

    private:
        glm::vec3 m_pos;
};

// Dispatcher inherits from Geometry and implements one dispatch function
// for all the given type and the Geometry one.
template <class T, typename... Shapes>
struct Dispatcher;

template <class T>
struct Dispatcher<T> : public Geometry {
    bool collide(const Geometry* obj) const final override {
        return obj->collide(static_cast<const T*>(this));
    }
};

template <class T, typename Shape, typename... Others>
struct Dispatcher<T, Shape, Others...> : public Dispatcher<T, Others...> {
    using Dispatcher<T, Others...>::collide;
    bool collide(const Shape* obj) const final override {
        return Collisions::collide(obj, static_cast<const T*>(this));
    }
};

// Shape<T> is our generic dispatch class.
template <class T>
class Shape : public Dispatcher<T, SHAPES> {};

With all shapes defined like:

class Sphere : public Shape<Sphere> {...};

Of course, all the corresponding Collisions::collide(*,*) must be declared.
I am not very familiar with template meta-programming, so I couldn't find a clean solution to replace the #define SHAPES Box, Sphere, Mesh and the classes declarations at the beginning of the file.

Furthermore, I don't know if a good compiler can optimize virtual calls using de-virtualization (that's why I tried to add final keyword).


Assigning database credentials I have a config.ini that contains my database credentials. I retrieve them with a parser class like this:

public function getField($section, $field) {
    return $this->iniArray[$section][$field];
}

In my DbConnect class I do this, which seems silly:

public function __construct() {
    $ini = new Configuration('configs/config.ini');
    $this->host = $ini->getField('Database', 'host');
    $this->user = $ini->getField('Database', 'user');
    $this->pass = $ini->getField('Database', 'password');
    $this->dbname = $ini->getField('Database', 'name');

I'm sure there is a better way to set these values, but I'm not sure how.

Perhaps instead of an ini file, a JSON would suit the purpose better. I've seen this multiple times, even in large, well-known PHP frameworks.

You can find a lot about decoding JSON from the docs. Here's a little blog post about handling decoding. And here's a StackOverflow question regarding the external file.

Also, if you keep the same class, I suggest passing Configuration through the constructor to practice dependency injection.

public function __construct(Configuration $ini) {
    $this->host = $ini->getField('Database', 'host');
    $this->user = $ini->getField('Database', 'user');
    $this->pass = $ini->getField('Database', 'password');
    $this->dbname = $ini->getField('Database', 'name');
}


Event handlers & helper methods for a Kendo Grid Below are a few of my handlers and helper methods. This works perfectly fine, but I'm pretty sure I'm breaking about 1001 modern conventions and possibly even optimizations.
Could you provide some insights as to how I can improve the structure and design of this code?

function getSelectedId() {
    var grid = $("MainGrid").data("kendoGrid");
    var selected = grid.select();
    var data = grid.dataItem(selected);

    return data.Id;
}

function changeGrid(e) {
    var id = getSelectedId();

    if (id == 0) {
        hideSubGrid();
    } else {
        refreshSubGrid();
        showSubGrid();
        setTitle(id);
    }
}

function hideSubGrid() {
    var subGrid = $("SubGrid").data("kendoGrid");

    subGrid.addClass("hidden");
}

function showSubGrid() {
    var subGrid = $("SubGrid").data("kendoGrid");

    subGrid.removeClass("hidden");
}

function refreshSubGrid(e) {
    var subGrid = $("SubGrid").data("kendoGrid");

    subGrid.dataSource.read();
    subGrid.refresh();
}

function setTitle(id) {
    $("#Title").text(id);
}

Some quick searching on the subject, suggests a style similar to what I've tried in the below code. However, I don't think this looks a whole lot better. I guess it's a step in the right direction, where I won't have to repeat myself as much, though.

var gridHandler = {
    mainGrid: $("MainGrid").data("kendoGrid"),
    subGrid: $("SubGrid").data("kendoGrid"),
    title: $("#Title"),

    getSelectedId: function () {
        var selected = this.mainGrid.select();
        var data = this.mainGrid.dataItem(selected);

        return data.Id;
    }

    changeGrid: function (e) {
        var id = this.getSelectedId();

        if (id == 0) {
            this.hideSubGrid();
        } else {
            this.refreshSubGrid();
            this.showSubGrid();
            this.setTitle(id);
        }
    }

    hideSubGrid: function () {
        this.subGrid.addClass("hidden");
    }

    showSubGrid: function () {
        this.subGrid.removeClass("hidden");
    }

    refreshSubGrid: function (e) {
        this.subGrid.dataSource.read();
        this.subGrid.refresh();
    }

    setTitle: function (id) {
        this.title.text(id);
    }
}

The second solution has two main improvements:

Fewer lookups. Every time you call your original getSelectedId method,
the lookup for $("MainGrid").data("kendoGrid"); is performed again.
The 2nd version caches this in gridHandler.mainGrid -- only one lookup.
The same goes for subGrid.
More compact. For example the subGrid local variable here was not particularly useful:

function hideSubGrid() {
    var subGrid = $("SubGrid").data("kendoGrid");

    subGrid.addClass("hidden");
}

It would have been simpler and more natural this way:

function hideSubGrid() {
    $("SubGrid").data("kendoGrid").addClass("hidden");
}

Reduced namespace clutter. By putting the methods in a gridHandler, you don't have many methods lying around in the global namespace, they are all neatly inside gridHandler.


Reading and merging multiple data files I've recently started teaching myself Python but have no prior programming experience or formal training. I've successfully created a program that outputs what I wanted it to output, but I'm looking for guidance on design/efficiency. Is there an overall design that would have been more efficient? Any pieces of code that should be re-written to be more stylistically correct? I purposely wrote this program without utilizing a database, but my next goal is to re-write the program to use a database instead of dictionaries.

The goal of this program is to read in 3 files that have a common key (custID). A customer can have multiple orders, but only one email/customer record. File layouts:

names.txt: custID|firstName|lastName|address1|address2|city|state|zip
emails.txt: custID|email
orders.txt: orderID|orderDate|channel|orderAmt|custID

Then, output a file with each order appearing on a new line:

output.txt: custID|firstName|lastName|city|state|email|orderDate|orderAmount

Program:

import re
import os

cust_info = {}
cust_emails = {}
cust_orders = {}
cust_join = {}

#Read Customer file with layout:
#custID|firstName|lastName|address1|address2|city|state|zip
def read_cust_file():
    with open('names.txt', 'r') as f:
        for line in f:
            split_line = re.sub("\s\s+", '|', line).strip().split('|')
            cust_info[int(split_line[0])] = "|".join(split_line[1:])
    return cust_info

#Read Email file with layout: custID|email
def read_email_file():
    with open('emails.txt', 'r') as g:
        for line in g:
            split_line = re.sub("\s\s+", '|', line).strip().split('|')
            cust_emails[int(split_line[0])] = "|".join(split_line[1:])
    return cust_emails

#Read Order file with layout: orderID|orderDate|channel|orderAmt|custID
#There can be multiple orders per custID
def read_order_file():
    with open('orders.txt', 'r') as f:
        for line in f:
            split_line = re.sub("\s\s+", '|', line).strip().split('|')
            try:
                cust_orders.setdefault(int(split_line[4]),[]).append("|".join(
                    split_line[0:4]))
            except KeyError:
                pass
    return cust_orders

#Join names, emails, and orders together on custID (dict key)
def join_orders():

    for key in set(list(cust_info) + list(cust_emails) + list(cust_orders)):
        try:
            cust_join.setdefault(key,[]).append(cust_info[key])
        except KeyError:
            pass

        try:
            cust_join.setdefault(key,[]).append(cust_emails[key])
        except KeyError:
            pass

        try:
            cust_join.setdefault(key,[]).append(cust_orders[key])
        except KeyError:
            pass

    return cust_join

#Format output layout:
#custID|firstName|lastName|city|state|email|orderDate|orderAmount
#Each order will be on a separate line, even if a cust made more than 1 order.
def write_output():
    output_file = open('output.txt', 'w')
    for k, v in cust_join.items():
        if len(v) == 3:
            for item in v[2]:
                split_cust = v[0].split('|')
                split_item = item.split('|')
                output_file.write("%s|%s|%s|%s|%s|%s|%s|%s\n" % (
                    k, split_cust[0], split_cust[1], split_cust[4],
                    split_cust[5], v[1], split_item[1], split_item[3]))
    output_file.truncate(output_file.tell() - len(os.linesep))
    output_file.close()

def main():

    read_cust_file()
    read_email_file()
    read_order_file()
    join_orders()
    write_output()

if __name__ == "__main__":
    main()

I really appreciate any advice you all can give!

Looks not too bad, it's readable enough.  The use of with is good (except for writeOutput, where it's not used for some reason) and the code is split into separate functions by functionality.  That said:

Stylistic

You should read and follow PEP8 (naming of globals, functions,
variables, whitespace, no need for semicolons).
You don't need those globals, just return values from your functions.
Maybe use docstrings instead of comments if you're describing the
functionality of a function.
Create more functions for common code.  If you copy and paste
something is wrong (usually).  That applies to all the reader functions, as they share the line splitting and accumulation code.
The try/except blocks in joinOrders aren't good.  Exceptions
aren't for control flow (usually) and you're better off using a
different approach for better understanding (where could the exception
be raised from) and maintainability (if you move the block or change
things, does the exception still only apply to the part you originally
wanted too, etc.)

Design

You might be better off using the csv module with custom separators
to read the files.  Even as a beginner it's a good idea to use
libraries to make your life easier.
The double splitting in the read functions looks unnecessary, but I'm
not quite sure.
Why the weird truncate?  It's normal for text files to end with a
newline.
Unless you're not concerned about speed the list concatenation in
joinOrders isn't the best idea.  Creating the set from custInfo,
then calling update on the set would be better to avoid unnecessary
allocations for the list.
Use iter* on dictionaries, lists, etc. for efficiency if you only iterate over the elements.  Here that would be iteritems instead of items.

Overall the split into input and output makes sense and I wouldn't worry too much about efficiency at this point (unless you're processing huge amounts of data that is).


A Python script to plot data and save to PDF I have a very simple Python script. All it does is open two data files from a given directory, read the data, make a series of plots and save as PDF. It works, but it is very slow. It takes almost 20 seconds for data files that have 50-100 lines and <30 variables.

import numpy as np
import matplotlib.pyplot as plt
from matplotlib.backends.backend_pdf import PdfPages

with open('file1.out') as f:
    var1 = f.readline().split()
with open('file2.out') as f:
    var1 = f.readline().split()

df1 = np.loadtxt('file1.out', skiprows=1, unpack=True)
df2 = np.loadtxt('file2.out', skiprows=1, unpack=True)

nc1 = df1.shape[0]
nc2 = df2.shape[0]

with PdfPages('file_output.pdf') as pdf:

    ## file1.out
    fig = plt.figure(figsize=(11,7))
    j = 1
    for i in range(1,nc1):
        ax = fig.add_subplot(3,2,j)
        ax.plot(df1[0], df1[i], linestyle='-', color='black')
        ax.set(title=var1[i], xlabel='seconds', ylabel='')
        if j == 6:
            pdf.savefig(fig)
            fig = plt.figure(figsize=(11,7))
            j = 1
        else:
            j = j + 1
    pdf.savefig(fig)

    ## file2.out
    fig = plt.figure(figsize=(11,7))
    j = 1
    for i in range(1,nc2):
    ... # and it continues like the block of code above

My questions are:

Do I need all those imports and are they slowing down the execution?
Is there a better way to read the data files then opening them twice (once to get the file header and once to get data)?
Am I using the matplotlib commands correctly/efficiently (I am not very familiar with matplotlib, and this is basically my first attempt to use it)?

Please keep in mind that ideally this script should have as few dependencies as possible, because it is meant to be used on different systems by different users.

The data files have the following format:

              t             X1             X2             X3             X4             X5             X6             X7             X8            X11            X12            X13            X14            X15            X16
  6.000000E+001  4.309764E-007  2.059219E-004  9.055840E-007  2.257223E-003  1.148868E-002  7.605114E-002  4.517820E-004  3.228596E-008  2.678874E-006  7.095441E-006  1.581115E-007  1.010346E-006  1.617892E-006  9.706194E-007
  1.200000E+002  4.309764E-007  2.059219E-004  9.055840E-007  2.257223E-003  1.148868E-002  7.605114E-002  4.517820E-004  3.228596E-008  2.678874E-006  7.095441E-006  1.581115E-007  1.010346E-006  1.617892E-006  9.706194E-007
  1.800000E+002  3.936234E-007  2.027775E-004  8.644279E-007  2.180931E-003  1.131226E-002  7.476778E-002  4.353550E-004  3.037527E-008  2.534515E-006  6.778434E-006  1.470889E-007  9.488175E-007  1.531702E-006  9.189112E-007

coding style

Your code is almost pep-8 compliant. There are a few spaces missing after comma's, but all in all this is not too bad. I myself use black to take care of this formatting for me.

some of the variables names can be clearer. What does nc1 mean for example

magic numbers

The number 3, 2 and 6 are the number of rows and columns on the grid. Better would be to make them real variables, and replace 6 with rows * columns. If you ever decide you want 4 columns, you don't have to chase down all those magic numbers

looping

You are looping over the indexes of var and df. Better here would be to use zip to iterate over both tables together. If you want to group them per 6, you can use the grouper itertools recipe. and enumerate to get the index of the different subplots.

rows, columns = 3, 2

for group in grouper(zip(var1[1:], df1[1:]), rows * columns):
    fig = plt.figure(figsize=(11, 7))
    for i, (label, row) in enumerate(filter(None, group)):
        ax = fig.add_subplot(rows, columns, i + 1)
        ax.plot(df1[0], row, linestyle="-", color="black")
        ax.set(title=label, xlabel="seconds", ylabel="")

The filter(None,...) is to eliminate the items that get the fillvalue in the grouper

Is a lot clearer than the juggling with nc1 and j

functions

This would be a lot easier to test an handle if you would separate the different parts of the script into functions

reading the file
making 1 page plot
appending the different pages

This will also allow each of those parts to be tested separately

reading the file

Instead of loading the file twice  and using numpy, using pandas, which supports data with names and indices will simplify this part a lot

df = pd.read_csv(<filename>, sep="\s+", index_col=0)

this is a labelled DataFrame, so no more need to use var1  for the column names

making the individual plot:

group the columns per 6

def column_grouper(df, n):
    for i in range(0, df.shape[1], n):
        yield df.iloc[:, i:i+n]

this simple helper generator can group the data per 6 columns

make the plot

def generate_plots(df, rows=3, columns=2):
    for group in column_grouper(df, rows * columns):
        fig = plt.figure(figsize=(11, 7))
        for i, (label, column) in enumerate(group.items()):
            ax = fig.add_subplot(rows, columns,i + 1)
            ax.plot(column, linestyle='-', color='black')
            ax.set(title=label, xlabel='seconds', ylabel='')
        yield fig

saving the pdf

Here a simple method that accepts an iterable of figures and a filename will do the trick

def save_plots(figures, output_file):
    with PdfPages(output_file) as pdf:
        for fig in figures:
            pdf.savefig(fig)

pulling it together

def parse_file(input_file, output_file, rows=3, columns=2):
    df = pd.read_csv(input_file, sep="\s+", index_col=0)
    figures = generate_plots(df, rows, columns)
    save_plots(figures, output_file)

and then calling this behind a main guard

if __name__ == "__main__":
    input_files = ['file1.out', 'file2.out']
    output_file = 'file_output.pdf'

    for input_file in input_files:
        parse_file(input_file, output_file)

If this still is too slow, at least now the different parts of the program are split, and you can start looking what part of the program is slowing everything down


Mars Rover challenge in Python - general feedback I have given the Mars Rover challenge a go in Python as DSA practice.

Here is the challenge:

  A rover’s position and location is represented by a combination of x and y co-ordinates and a letter representing one of the four cardinal compass points. The plateau is divided up into a grid to simplify navigation. An example position might be 0, 0, N, which means the rover is in the bottom left corner and facing North.

  In order to control a rover , NASA sends a simple string of letters. The possible letters are ‘L’, ‘R’ and ‘M’. ‘L’ and ‘R’ makes the rover spin 90 degrees left or right respectively, without moving from its current spot. ‘M’ means move forward one grid point, and maintain the same heading._

  Test Input:

  5 5

  1 2 N

  LMLMLMLMM

  3 3 E

  MMRMMRMRRM

  Expected Output:

  1 3 N

  5 1 E

I am still relatively new to Python so know this is a bit basic - but I wondered if I could get some general feedback on my code for best coding practice?

RIGHT_ROTATE = {
    'N':'E',
    'E':'S',
    'S':'W',
    'W':'N'
}

LEFT_ROTATE = {
    'N':'W',
    'W':'S',
    'S':'E',
    'E':'N'
}

class MarsRover():

    def __init__(self, X, Y, direction):

        self.X = X
        self.Y = Y
        self.direction = direction

    def rotate_right(self):

        self.direction = RIGHT_ROTATE[self.direction]

    def rotate_left(self):

        self.direction = LEFT_ROTATE[self.direction]

    def move(self):

        if self.direction == 'N':
            self.Y += 1
        elif self.direction == 'E':
            self.X += 1
        elif self.direction == 'S':
            self.Y -= 1
        elif self.direction == 'W':
            self.X -= 1

    def __str__(self):

        return str(self.X) + " " + str(self.Y) + " " + self.direction

    @classmethod
    def get_rover_position(self):

        position = input("Position:")
        X = int(position[0])
        Y = int(position[2])
        direction = position[4]
        return self(X, Y, direction)

class Plateau():

    def __init__(self, height, width):

        self.height = height
        self.width = width

    @classmethod
    def get_plateau_size(self):

        plateau_size = input("Plateau size:")
        height = int(plateau_size[0])
        width = int(plateau_size[2])
        return self(height, width)

def main():

    plateau = Plateau.get_plateau_size()

    while True:

        rover = MarsRover.get_rover_position()

        current_command = 0
        command = input("Please input directions for rover.")
        command_length = len(command)

        while current_command <= command_length - 1:

            if command[current_command] == 'L':
                rover.rotate_left()
                current_command += 1
            elif command[current_command] == 'R':
                rover.rotate_right()
                current_command += 1
            elif command[current_command] == 'M':
                rover.move()
                current_command += 1

        result = str(rover)
        print(result)

if __name__ == '__main__':

    main()

The Plateau class seems to be pretty unused:

It doesn't have actual behaviour, it only hosts the get_plateau_size() method.
That method should have a first parameter named cls by convention. self is only used for instances, but here you get the class itself, not an instance.
By that, it also returns a Plateau, because calling a class creates an instance. This is kind-of confusing, because I'd have expected some kind of size.
Further, the user interaction (input()) was unexpected here. Don't hide such usage details in classes that make sense on their own.
The call contains redundancy as well, Plateau.get_plateau_size() features the term "plateau" twice. Once would have been enough.
Anyhow, the result isn't even used, so this whole class could be removed!

LEFT_ROTATE and RIGHT_ROTATE are both only used in a single function, no need to make them globals.
Take a look at the dataclass decorator ( https://docs.python.org/3/library/dataclasses.html), it could help you structure your classes a bit better. In particular, it would generate __str__ for you.


Switching tabs, with previous/next arrows, using jQuery You can find a JSFiddle demo of my tabs.

The jQuery tabs work well as intended. However I was asked to include arrows alongside the tabs so readers could navigate through tabs using them instead of the tab titles alone.

So I used:

$("li.tab").click( function() {
  $('#div1').removeClass( 'hide' );
  $('#div2').removeClass( 'hide' );
  $('#div3').removeClass( 'hide' );
  $('#div1').removeClass( 'show' );
  $('#div2').removeClass( 'show' );
  $('#div3').removeClass( 'show' );
} );
$("a.rightarrow1").click( function() {
  $('#div1').addClass( 'hide' );
  $('#div2').addClass( 'show' );
  $('#div2').removeClass( 'hide' );
  $('#div3').addClass( 'hide' );
  $('li.tab2').addClass( 'current' );
  $('li.tab1').removeClass( 'current' );
  $('li.tab3').removeClass( 'current' );
} );
$("a.leftarrow1").click( function() {
  $('#div1').addClass( 'hide' );
  $('#div2').addClass( 'hide' );
  $('#div3').addClass( 'show' );
  $('#div3').removeClass( 'hide' );
  $('li.tab3').addClass( 'current' );
  $('li.tab2').removeClass( 'current' );
  $('li.tab1').removeClass( 'current' );
} );
$("a.rightarrow2").click( function() {
  $('#div1').addClass( 'hide' );
  $('#div2').addClass( 'hide' );
  $('#div3').addClass( 'show' );
  $('#div3').removeClass( 'hide' );
  $('li.tab3').addClass( 'current' );
  $('li.tab2').removeClass( 'current' );
  $('li.tab1').removeClass( 'current' );
} );
$("a.leftarrow2").click( function() {
  $('#div1').addClass( 'show' );
  $('#div1').removeClass( 'hide' );
  $('#div2').addClass( 'hide' );
  $('#div3').addClass( 'hide' );
  $('li.tab1').addClass( 'current' );
  $('li.tab2').removeClass( 'current' );
  $('li.tab3').removeClass( 'current' );
} );
$("a.rightarrow3").click( function() {
  $('#div1').addClass( 'show' );
  $('#div1').removeClass( 'hide' );
  $('#div2').addClass( 'hide' );
  $('#div3').addClass( 'hide' );
  $('li.tab1').addClass( 'current' );
  $('li.tab2').removeClass( 'current' );
  $('li.tab3').removeClass( 'current' );
} );
$("a.leftarrow3").click( function() {
  $('#div1').addClass( 'hide' );
  $('#div2').addClass( 'show' );
  $('#div2').removeClass( 'hide' );
  $('#div3').addClass( 'hide' );
  $('li.tab2').addClass( 'current' );
  $('li.tab3').removeClass( 'current' );
  $('li.tab1').removeClass( 'current' );
} );

It's long, but it works. But can it be made more elegant?

Firstly, the obvious:

Both .addClass and .removeClass can deal with multiple classes at once, so wherever you have something like

  $('#div1').removeClass( 'hide' );
  ...
  $('#div1').removeClass( 'show' );
  ...

you can instead write

  $('#div1').removeClass(['hide', 'show']);

You have a few places where you're just removing the same set of classes from a series of elements. You might consider crafting your selector differently to select them all at once, or at the very least define an intermediate function so that you can write something like

stripClasses(["#div1", "#div2", ...], ['hide', 'show']);

All of this actually seems to be pointless, because if the idea is just to navigate through tabs with arrow buttons, you can use the built-in selected option, both to find out which is currently active and to change it.

The fundamental reason you've got so much complexity in that code block is that you seem to have decided to manually manage all the class changes that go along with moving from one tab to another. You don't actually need to do that.

Something like

$("#tabs").tabs();

$(".left-arrow").click(function () { changeTab("#tabs", -1)});
$(".right-arrow").click(function () { changeTab("#tabs", +1)});

function changeTab(elem, by) {
    var sel = $(elem).tabs("option", "selected");
    var newTab = bounded(0, sel+by, 3);
    $(elem).tabs("option", "selected", newTab);
}

function bounded(lower, num, upper) {
    return Math.min(upper, Math.max(lower, num));
}

should work fine (change the newTab expression to get different behavior, like wrapping, out of it). Just be mindful of your local namespaces and styles.


Python code to weave rows I want to write a cleaner version of the code shown below. It's about two different rows consisting of numbers that have to be weaved.

It's for an assignment for a course i'm currently following. This is the assignment:

Traveler, if riches are to be achieved, the solution only has to be weaved.
5,4 4,5 8,7
Weave 6,3 3,2 9,6 4,3
Weave 7,6
Weave 9,8
Weave 5,5 7,8 6,5 6,4

I have to start with the coordinates on the first row, weave the coordinates of the second row through the first row, then weave the coordinates of the third row through the resulting row etc. Note, if two rows of unequal length have to be woven, the two rows are woven as far as possible. After the elements of the shortest row have been exhausted, the row is completed by adding the remaining elements of the longest row.

The input is this:

5,4 4,5 8,7=6,3 3,2 9,6 4,3=7,6=9,8=5,5 7,8 6,5 6,4

The output has to be like this:

6,4
6,5
10,8
8,8
8,6
7,5
7,3
7,4
5,5
4,2
9,7
10,6
5,3

My code is in three files.

Coordinaat.py

class Coordinaat:
    def __init__(self, x_coordinaat, y_coordinaat):
        self.x_coordinaat = x_coordinaat
        self.y_coordinaat = y_coordinaat

    def bereken_nieuw_coordinaat(self):
        self.x_coordinaat += 1
        self.y_coordinaat += 0
        return self.x_coordinaat, self.y_coordinaat

CoordinaatRij.py

class CoordinaatRij:
    def __init__(self, rij):
        self.rij = rij

    def voeg_toe(self,coordinaat):
        self.rij += [coordinaat]

    def breid_uit(self, rij):
        for coordinaat1 in rij:
            self.rij += [coordinaat1]

    def weave(self, weef_rij):
        resultaat = CoordinaatRij([])

        for i in range(len(self.rij)):
            if i < len(self.rij):
                resultaat.voeg_toe(self.rij[i])
            if i < len(weef_rij.rij):
                resultaat.voeg_toe(weef_rij.rij[i])
        if len(weef_rij.rij) > len(self.rij):
            resultaat.breid_uit(weef_rij.rij[len(self.rij):])
        return resultaat

Excerpt of the main code

file_input = file.read()
strip_file = file_input.strip()
invoer = strip_file.split("=")

from Coordinaat import Coordinaat
from CoordinaatRij import CoordinaatRij

def nieuwe_rijen(invoer):
    rijen = []
    for i in range(len(invoer)):
        rijen += [CoordinaatRij(invoer[i].split())]
    vorige_rij = rijen[0]

    for rij in rijen[1:]:
        vorige_rij = vorige_rij.weave(rij)
    return vorige_rij

def nieuwe_coordinaten(geweefde_rijen):
    for coordinaat1 in geweefde_rijen.rij:
        huidige_coordinaten = Coordinaat(int(coordinaat1[0]), int(coordinaat1[2]))
        nieuwe_coordinaten = huidige_coordinaten.bereken_nieuw_coordinaat()
        print '%d, %d' %(nieuwe_coordinaten[0], nieuwe_coordinaten[1])

geweefde_rijen = nieuwe_rijen(invoer)
nieuwe_coordinaten(geweefde_rijen)

English code reads better, since the Python standard library is English.  If you use Dutch, though, please do so consistently.  Don't mix file_input and invoer, or def weave(self, weef_rij).

It's hard to understand what the task to be performed is, and what this code does.  Docstrings and doctests would be a good idea.

It is awkward how you do parts of the parsing all over the place:

strip_file = file_input.strip() and strip_file.split('=') in the main code.  It's especially bad that this snippet is placed before the imports, and before the next line of code (geweefde_rijen = nieuwe_rijen(invoer)).
invoer[i].split() within nieuwe_rijen().
int(coordinaat1[0]), int(coordinaat1[2]) within nieuwe_coordinaten().

It would be better to get the parsing done all at once, into a list of pairs of integers.  Not only would the code be easier to understand, it would also let the program fail earlier if the input is invalid.

I think you have overdone the OOP here.  There is not much point in defining a Coordinaat class, which is basically just a tuple, or CoordinaatRij, which is basically just a list.  All you need for this job is a few functions.

Specifically, it would be useful to take advantage of itertools.izip_longest(), which is the core of what you are trying to do.  Then, to transform the result repeatedly for subsequent rows, use the reduce() function.

from itertools import chain, izip_longest

def coordinaatrijen_parsen(tekenreeks, rij_scheidingsteken='\n'):
    """
    Yield each row of coordinates as a list of (int, int).

    >>> list(coordinaatrijen_parsen('1,2 3,4=  5,6 ', '='))
    [[(1, 2), (3, 4)], [(5, 6)]]
    """
    for rij in tekenreeks.split(rij_scheidingsteken):
        yield [
            tuple(int(c) for c in coord.split(','))
            for coord in rij.split()
        ]

def weven(*rijen):
    """
    Interleave elements from the parameters.

    >>> weven('Geeogn', 'odmre')
    ['G', 'o', 'e', 'd', 'e', 'm', 'o', 'r', 'g', 'e', 'n']
    >>> weven('Gee', 'odmorgen')
    ['G', 'o', 'e', 'd', 'e', 'm', 'o', 'r', 'g', 'e', 'n']
    >>> weven('Gd', 'o', 'eemorgen')
    ['G', 'o', 'e', 'd', 'e', 'm', 'o', 'r', 'g', 'e', 'n']
    """
    return filter(None, chain(*izip_longest(*rijen)))

if __name__ == '__main__':
    with open(...) as f:
        rijen = coordinaatrijen_parsen(f.read(), '=')
    resultaat = [(x + 1, y) for x, y in reduce(weven, rijen, [])]
    for coord in resultaat:
        print '%d, %d' % coord


Length of the longest consecutive run of 1s in its binary representation The task

  Given an integer n, return the length of the longest consecutive run
  of 1s in its binary representation.

  For example, given 156, you should return 3

My function solution 1

Style 1

function findMaxNumberOfConsecOnes(dec){
  const countConsecOnes = (acc, x) => [acc[0] < acc[1]
                                       ? acc[1]
                                       : acc[0],
                                       x === "1"
                                       ? acc[1] + 1
                                       : 0 ];

  return [...(dec >>> 0).toString(2)]
      .reduce(countConsecOnes, [0,0]);
}

Style 2

function findMaxNumberOfConsecOnes(dec){
  const countConsecOnes = (acc, x) => [acc[0] < acc[1] ? acc[1] : acc[0],
                                       x === "1" ? acc[1] + 1 : 0 ];

  return [...(dec >>> 0).toString(2)]
      .reduce(countConsecOnes, [0,0]);

Style 1 and Style 2 have the exact same logic, but different style. My first idea was to write it in Style 1. But I then thought what was said to me in this thread about indentation. Therefore I opt for Style 2. But still not sure, whether this is the right way.

Alternatively:

My functional Solution 2

function findMaxNumberOfConsecOnes(dec){
  return [...(dec >>> 0).toString(2)]
    .reduce((acc, x) => {
      const current = x === "1" ? acc[1] + 1 : 0;
      const max = Math.max(acc[0], current);
      return [max, current];
  }, [0,0]);
}

My imperative Solution

function findMaxNumberOfConsecOnes(dec){
  const binStr = (dec >>> 0).toString(2);
  let max = 0, current = 0;
  for (const s of binStr) {
    current = s === "1" ? current + 1 : 0;
    max = Math.max(max, current);
  }
  return max;
}

Code alignment.
I still think this style of indentation is poor. It is particularly ill suited to large code bases where you have literally hundreds of thousands of lines of code to maintain.
Its about reading the code and our inherently lazy brain. You scroll through the code looking for a problem scanning the lines and then you have code way to the right. Its not good.
I would write it as
function findMaxNumberOfConsecOnes(dec) {
    const countConsec = (ac, x) => [
        ac[0] < ac[1] ? ac[1] : ac[0], // or Math.max(...ac)
        x === "1" ? ac[1] + 1 : 0
    ];

    return [...(dec >>> 0).toString(2)]
        .reduce(countConsec, [0, 0])[0]; // Returning number rather than array
}

Note that I added  () around the comma separated group and then the second ternary. Not because it is needed, but when you are checking code the IDE will highlight matching braces (I have mine set to bright bold red) saving time and effort when looking for typos in compound expressions..
Bug?
The functional solutions return differently (an array) from the imperative solution (a number). My guess is you forgot to extract the max from the array.
Number V String
Numbers always win.
I assume by the >>> 0 you are expecting negative values and that the MSB (most significant bit) the sign bit, is counted.
Strings are always the slowest way to deal with numbers. You do get an automatic reduction in the number of bits to count as toString(2) drops the leading zeros so it has one handy benefit.
Comparing your solutions
In terms of performance the imperative solution is the quickest at around 20% faster than the other two (tested on a random set of signed int32). This is almost a given as functional sacrifices performance in hope of less bugs.
Using numbers only
By avoiding the use of strings you avoid the memory allocation overhead and the slow string compare tests.
The following example is up to an order of magnitude faster than your imperative solution and can also avoid stepping over the leading bits of negative values.

size = Math.log2(Math.abs(num)) + 1 | 0 get the number of bits to count
const leading = num < 0 ? 32 - size : 0 If negative gets the number of leading bits
return Math.max(count + leading, max) add any leading bits to the count and check for max

The rest is straight forward.
function countBitSeq(num) {
    var max = 0, count = 0, size;
    size = Math.log2(Math.abs(num)) + 1 | 0;
    const leading = num < 0 ? 32 - size : 0;
    while (size --) {
        max = Math.max((num & 1 ? ++count : count = 0), max);
        num >>= 1;
    }
    return Math.max(count + leading, max);
}


Adding 1 to Date while binding using KnockoutJS I am using KnockoutJS to bind some values. I have a date value TimeByDay that I bind like this:

But I would also like to transform the date on a different location and simply add a day to the TimeByDay value.

I can do it like this:

Is there a prettier way to do this? Maybe some feature in KnockoutJS that I am missing?

(The reason I am encapsulating both values in a new Date() construct is that I also apply formatting to the date.)

ViewModel:

$(document).ready(function () {

    var serverData = $.getJSON("../../_api/");
    serverData.done(function (data) {
        ko.applyBindings(new EngagementsList(data.value));
    });

});

function EngagementsList(engagementList) {
    var self = this;
    self.entries = engagementList;
}

HTML:

One way would be to move the date display logic to your view model.

function vm(){
    var self= this;

    self.TimeByDay = ko.observable(new Date("December 17, 1995 03:24:00"));

    // fn extention of ko.observable
    ko.observable.fn.AddOneDay = function(){
        return new Date(this().setDate(this().getDate()+1))
    };

    self.AddOneDay = function(TimeByDay){
        return new Date(TimeByDay().setDate(TimeByDay().getDate()+1))
    };
}

And then in your view you can have

Or

Use the extention fn method if your application requires 1 day added to date frequently. Read this for more.

Jsfiddle: https://jsfiddle.net/newuserjs/oLh6u67z/4/


"String Enum" implementation - comments? I have a need to represent a finite set of discrete string values:

When these strings are used in code, they must belong to the valid set, or serious errors (or worse) can occur.
As "magic strings" are a terrible code smell, I want to encapsulate the strings within a non-string type, so that I can require the use of my type instead of just a string and thus enforce at compile-time that a value is given that I know will work.
No constants; if I change the string behind the scenes I should not have to recompile all usages of my type.
The identifiers representing the valid values should not have to have any formulaic similarity to the actual string (no Enum.ToString or similar).
Lastly, while I want to enforce the use of my type so I can restrict values to the ones I know are valid, the type must be easily convertible to a string so that the piece of code that really needs the string doesn't have to undergo a complicated or esoteric conversion.

What I came up with is a variation of a "multiton"; a class that cannot be instantiated in code, but has a finite number of static instances each representing a valid value of the type. They are implicitly convertible to strings and so the static members just drop in wherever a string is needed.

//abstract base; derive to create your specific set of values
public abstract class StringEnum
{
    protected StringEnum(string name)
    {
        this.name = name;
    }

    private readonly string name;

    public static implicit operator string(StringEnum sp)
    {
        return sp.name;
    }
}

//one possible derivation
public class CommandNames:StringEnum
{
    //unfortunately each derivation has to call back to the base constructor;
    //it would be nice not to have to do this, but meh
    protected CommandNames(string commName) : base(commName) { }

    //obviously the instance names don't have to match the real string value exactly
    public static readonly CommandNames DoSomething = new CommandNames("system638_Command_doSomething");
}

//example usage
public void ExecuteCommand(CommandNames name)
{
    //assume ExecuteByName expects a string
    CommandExecutor.ExecuteByName(name); //the StringEnum just drops in
}

...

//Only one of the static instances, with string values I control,
//can be passed into this function, thus safely encapsulating the inner
//objects' use of "magic strings".
ExecuteCommand(CommandNames.DoSomething);

Thoughts?

A few things don't add up for me regarding your usage.  You want compile time absolution, but dynamic values while stating the critical nature of its use.

One option:
     You can just take care of this by having objects with inheritance structures and loading your "dynamic" values from an application configuration or db.  Just have 3 layers of inheritance 1) StringEnum 2) Enum Declaration (like System638) and 3) actual values.  Just wire up the inheritance.  Mostly likely though, interfaces would be more appropriate for restrictions at the 1 & 2 layers.

Recommended (assuming the distributed smell of the use case):
     Build your functional class and bind it to a single DB table that ensures the integrity you are looking for.  Just cache the values in memory for performance needs.

Recommended (given the critical nature of its use)
     Find a different way to design the system so that you can fully embed what you need in code.  For example, a web service.  Even if you need a local presence you can utilize a web service to control the pieces you need.


Checking to see if value is in datalist or not I have a form that is used for inputting data, one of the fields is a datalist with around 6000 options in it, I needed to check that the value entered in to the datalist is actually in the list, here is what I came up with,

$('#submit').on('click', function (e) {
    var datalistval = $('#ItemSelect').val().split('|');
    datalistval = $.trim(datalistval[0]);

    var IsinList;
    $("#jobs option").each(function ()
    var arraylist = $(this).val().split('|');
    arraylist = $.trim(arraylist[0]);
    if ($(this).val() != datalistval) {
        console.log($(this).val());
        console.log("Not there ");
    } else {
        console.log("there");
        IsinList = true;
        return false;
    }
    });

if (IsinList !== true) {
    alert('Please check the product code as it is not in the list, make sure you select an option in the list ');
    e.preventDefault();
}
});

Is this the most optimum way of doing this? Is there anything I can do to improve it?

Your code is a bit hard to read, but I suggest that

array.indexOf(element)

or

array.some(callback)

is what you are looking for.

Documentation:

indexOf

some


Utility functions for a linked list I have written code to add a node to the head of the list. Criticisms and feedback are welcome.

typedef struct sList
{
    void *data;
    struct sList *nextNode;
}sList;

sList *addNode2Head(sList *psList,void *data)
{
    if(psList == NULL)
        psList = newNode();
    else
    {
        psList = addHeadNode(psList);
        psList->data = data;
    }
    return psList;
}

sList *newNode(void)
{
    sList *psList =(sList *) malloc(sizeof(sList));
    //sList *psList = malloc(sizeof *sList); //Is it correct way ? if so why ?
    psList->nextNode = NULL;
    return psList;
}

sList *addHeadNode(sList *psList)
{
   sList *psListTemp = newNode();
   psListTemp->nextNode = psList;
   return psListTemp;
   //Another way ,
   //psList = psListTemp; //No return needed
}

The name addNode2Head will be better as addNodeToHead. Using the number 2 for the word to is rather poor choice in a function name.
The function addNodeToHead can be simplified to:

sList *addNodeToHead(sList *psList, void *data)
{
   sList* node = newNode();
   node->data = data;

   // It doesn't matter what psList is. The new
   // node is now the head of the list.
   node->nextNode = psList;

   return node;
}

Given the simplified implementation of addNodeToHead, the function addHeadNode might be unnecessary.
Don't explicitly cast the return value of malloc. See Do I cast the result of malloc?

Instead of

sList *psList =(sList *) malloc(sizeof(sList));

use

sList *psList = malloc(sizeof(sList));

Always check the return value of malloc before proceeding to use it.

sList *newNode(void)
{
    sList *psList = malloc(sizeof(sList));
    if ( psList == NULL )
    {
       return NULL;
    }
    psList->nextNode = NULL;
    return psList;
}

Make sure that you check the value returned by the above function.

   sList* node = newNode();
   if ( node == NULL )
   {
      // Decide how you want to handle the error.
      // return NULL???
   }

   node->data = data;


ZigZag array implementation I have implemented a zigzag array which will sort smaller greater smaller and goes on...
For example :
3 7 4 8 2 6 1 is a valid ordering.
The relationship between numbers is
3 < 7 > 4 < 8 > 2 < 6 > 1
3 smaller than 7
7 greater than 4
4 smaller than 8
8 greater than and so on

the ordering always has to be "smaller than" then "greater than" then "smaller than"...
Are there any other suggestions for improving the solution?
Thanks.
package main.algorithms;

public class ZigZag {

    public static void main(String[] args) {
        int[] array = {4,3,7,8,6,2,1};
        zigZag(array);
        for (int i: array){
            System.out.println(i);
        }
    }

    // Time O(n)
    // Space O(1)
    private static int[] zigZag(int[] array) {

        int status = 0;
        int counter = 0;
        while(counter < array.length-1){
            if(status ==0) {
                if (array[counter] > array[counter + 1]) {
                    int temp = array[counter + 1];
                    array[counter + 1] = array[counter];
                    array[counter] = temp;
                }
                status = 1;
            } else {
                if (array[counter] < array[counter + 1]) {
                    int temp = array[counter + 1];
                    array[counter + 1] = array[counter];
                    array[counter] = temp;
                }
                status = 0;
            }
            counter++;
        }
        return array;
    }
}

Instead of that while-loop, I'd use a for-loop and use the common loop/index variable i.
Instead of a separate status variable, I'd simply use i % 2.
I'd deduplicate the two cases.
Putting the current two array elements into variables avoids duplicating the longer array[i] and array[i + 1] and simplifies the swap.
As Joop Eggen points out, it's probably better to not modify the array in-place and return it. (Unless you have a good reason to, which it seems you don't, as your own calling code ignores the returned value.)

Code:
    private static void zigZag(int[] array) {
        for (int i = 0; i < array.length - 1; i++) {
            int a = array[i], b = array[i + 1];
            if (i % 2 == 0 ? a > b : a < b) {
                array[i] = b;
                array[i + 1] = a;
            }
        }
    }


Selection sort algorithm in Python I'm learning basic algorithms and implementing them in Python. Critiques needed/welcome.

import pudb; pu.db
import random
def selection_sort(list_):
    """Implement selection_sort algorithm: iterate L to R in list, grab value
    if greatest and inserting into correct pos. After each loop, decrement
    length of iterated list by 1"""
    val_list, greatest = list(range(len(list_))), 0
    for num in range(len(list_)):
        greatest = val_list[-1]
        for val in val_list:
            if list_[greatest] < list_[val]:
                greatest = val
        if list_[greatest] > list_[val_list[-1]]:
            list_.insert(val, list_.pop(greatest))
            val_list.remove(val)
    return list_

if __name__ == '__main__':
    unsorted = list(range(9))
    random.shuffle(unsorted)
    assert selection_sort(unsorted) == list(range(9))

Most of my comments on your previous question apply here too, so I'll just summarize them quickly:

Debugging code that you forgot to remove.
Docstring written from the implementer's point of view.
Variable name respelled to avoid shadowing a built-in.
Test case not organized into a unit test.
Test case not very stringent (just 9 items).

In addition:

Given an array of length n, your algorithm makes n passes over the array, and in pass i it finds the ith largest item in the array (which at that point in the algorithm is the largest item in the first n − i positions) and swaps it into the position n − i − 1. This ought to be simple, but it's implemented in a very complex way, via the list val_list of indexes.

Here's a much simpler implementation:

def selection_sort(seq):
    """Sort the mutable sequence seq in place and return it."""
    for i in reversed(range(len(seq))):
        # Find the index of greatest item in seq[:i+1].
        greatest = 0
        for j in range(1, i + 1):
            if seq[j] > seq[greatest]:
                greatest = j
        seq[i], seq[greatest] = seq[greatest], seq[i]
    return seq

Note that I've avoided testing seq[greatest] > seq[i] before doing the swap, because I know that the only way this condition can fail is if greatest == i, and then the swap has no effect. So it's simplest to skip the test.

I could simplify this still further using Python's built-in function max:

def selection_sort(seq):
    """Sort the mutable sequence seq in place and return it."""
    for i in reversed(range(len(seq))):
        greatest = max(range(i + 1), key=seq.__getitem__)
        seq[i], seq[greatest] = seq[greatest], seq[i]
    return seq

But this seems like it's avoiding the spirit of the exercise: I mean, if I can use max, then why not sorted?


Linked List design and implementation - C++ Seeking recommendations on how to improve this code, LL always seem to be a struggle.

A basic linked list that does include next and previous pointers. I used a struct inside of a class, which I'm not sure if that's frowned upon. The program includes 5 basic member functions, one to add to the head of the list, one to add to the tail of the list, one to delete the head, one to delete any index, one that checks the list for duplicate data (based on the data variable in the struct),  and a print. Everything works as it should, but I'm just not sure if I was efficient, in fact I'm pretty sure I wasn't. Sorry If im forgetting something, but I'm new here!

1.

class list
{
private:
    struct node
    {
        int data;
        node* next;
        node* prev;
    };

    typedef node* nodePtr;

    nodePtr head;
    nodePtr temp;
    nodePtr prev;

public:
    list();
    void add_to_head(int);
    void add_to_tail(int);
    void delete_index(int);
    void check_duplicates(int);
    void print();
};

2.

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

list::list(void)
{
    head = NULL;
    temp = NULL;
    prev = NULL;
}

void list:: add_to_head(int num)
{
    nodePtr n = new node;

    n->data = num;

    if(head == NULL)
    {
        n->prev = NULL;
        n->next = NULL;
        head = n;

    }
    else
    {
        n->next = head;
        n->prev = NULL;
        head = n;
    }

    return;
}

void list::add_to_tail(int num)
{
    nodePtr n = new node;

    n->data = num;

    if(head == NULL)
    {
        n->prev = NULL;
        n->next = NULL;
        head = n;
    }
    else
    {
        temp = head;

        while(temp->next != NULL)
        {
            temp = temp->next;
        }

        temp->next = n;
        n->prev = temp;
        n->next = NULL;
    }

    return;
}

void list::delete_index(int index)
{
    if(index == 0)
    {
        temp = head->next;
        temp ->prev = NULL;

        prev = head;
        delete prev;

        head = temp;
    }
    else
    {
        nodePtr del = new node;

        int count =0;
        temp = head;

        while(count != index)
        {
            prev = temp;
            temp = temp->next;
            count++;
        }

        del = temp;

        if(temp->next == NULL)
        {
            prev->next = NULL;
        }
        else
        {
            prev->next = temp->next;
            temp = temp->next;
            temp->prev = prev;
        }

        delete del;
    }

    return;
}

void list::check_duplicates(int num)
{
    int count = -1;

    temp = head;

    while(temp != NULL)
    {
        if(temp->data == num)
        {
            count++;
        }

        temp = temp->next;
    }

    if(count > 0)
    {
        cout<<endl<<endl<<"The number "<<num<<" was found "<<count + 1<<" times."<<endl;
    }

    else if(count == -1)
    {
        if(head == NULL)
        {
            cout<<"The list is empty."<<endl;
        }
        else
        {
            cout<<"The number was not found within the list."<<endl;
        }
    }

    return;
}

void list::print()
{
    temp = head;

    cout<<endl<<"The list:"<<endl;

    while(temp != NULL)
    {
        cout<<temp->data<<" ";
        temp = temp->next;
    }

    return;
}

You can simplify your code a lot.

    if(head == NULL)
    {
        n->prev = NULL;
        n->next = NULL;
        head = n;
    }
    else
    {
        n->next = head;
        n->prev = NULL;
        head = n;
    }

Both sides of this else look identical.
I think you can shorten your code with:

void list:: add_to_head(int num)
{
    nodePtr oldHead = head;
    head = new node{num, oldHead, nullptr);
    // Also you forget to do this bit.
    if (oldHead) {
        oldHead->prev = head;
    }
}

@pm100 covered the temporary variable.

nodePtr temp;
nodePtr prev;

You don't need these as a class member. It should be local to the member functions where they are used.

If I was you I would add another member though.

nodePtr tail;

Then you don't need to search for the tail every time you add a new element.

Const correctness. In a lot of code we pass variables by reference and const reference. If you pass an object by const reference you can only call const members on it.

So you should take care to mark functions that don't mutate the object as const. This will allow you to call the methods from a const reference. Also it enables some compiler optimizations when it knows you are not mutating the object.

So the following functions should probably be const.

void check_duplicates(int) const;
void print() const;

Your print() is fine. But I would make it more generic. Rather than print to std::cout you should print to a generic stream (you can always default to std:::cout.

print(std::ostream& str = std::cout) const;

Also in C++ printing is usually done via operator<<. So I would add this function. All it needs to do is call the print function passing the stream.

std::ostream& operator<<(std::ostream& str, List const& data)
{
     data.print(str);
     return str;
}


Streamlining button creation and Handlers The code below works but I can not help thinking it's to 'clunky' and could be so much tidier! It's the first time I've really used AddHandler so go gentle...

Create button(s) and handler(s)

The code below works but I can not help thinking it's to 'clunky' and could be so much tidier! It's the first time I've really used AddHandler so go gentle...

Create button(s) and handler(s)

 Public Sub AddChartRow(ByVal button As String, ByVal id As String, ByVal high As Integer, ByVal medium As Integer, ByVal low As Integer, ByVal na As Integer)

    ChartArea.Controls.Add(New LiteralControl("
"))

    Dim btnButton As New Button() With {.Text = button, .ID = id, .CssClass = vRiskActive}

            If id = "btn1" Then
                AddHandler btnButton.Click, AddressOf btnItem_1 ' here Buttons is your Handler
            End If
            If id = "btn2" Then
                AddHandler btnButton.Click, AddressOf btnItem_2 ' here Buttons is your Handler
            End If
            If id = "btn3" Then
                AddHandler btnButton.Click, AddressOf btnItem_3 ' here Buttons is your Handler
            End If
If id = "btn4" Then
                AddHandler btnButton.Click, AddressOf btnItem_4 ' here Buttons is your Handler
            End If
If id = "btn5" Then
                AddHandler btnButton.Click, AddressOf btnItem_5 ' here Buttons is your Handler
            End If
If id = "btn6" Then
                AddHandler btnButton.Click, AddressOf btnItem_6 ' here Buttons is your Handler
            End If
If id = "btn7" Then
                AddHandler btnButton.Click, AddressOf btnItem_7 ' here Buttons is your Handler
            End If
If id = "btn8" Then
                AddHandler btnButton.Click, AddressOf btnItem_8 ' here Buttons is your Handler
            End If

If id = "btn9" Then
                AddHandler btnButton.Click, AddressOf btnItem_9 ' here Buttons is your Handler
            End If
If id = "btn10" Then
                AddHandler btnButton.Click, AddressOf btnItem_10 ' here Buttons is your Handler
            End If

            If (oGetPercentages.SumTotal) <> "0" Then
                ChartArea.Controls.Add(btnButton)
                ChartArea.Controls.Add(New LiteralControl("
" & "
" & "
" & high & "
" & "
" & medium & "
" & "
" & low & "
" & "
" & na & "
" & "
" & "
" & "
"))
            Else
                ChartAreaGrey.Controls.Add(btnButton)
                ChartAreaGrey.Controls.Add(New LiteralControl("
" & "
" & "
" & "
" & "
"))
            End If

        End Sub

 Public Sub btnItem_1()
            Call PrcDash2("1")
        End Sub

        Public Sub btnItem_2()
            Call PrcDash2("2")
        End Sub

        Public Sub btnItem_3()
            Call PrcDash2("3")
        End Sub

 Public Sub btnItem_4()
            Call PrcDash2("4")
        End Sub

 Public Sub btnItem_5()
            Call PrcDash2("5")
        End Sub

 Public Sub btnItem_6()
            Call PrcDash2("6")
        End Sub

 Public Sub btnItem_7()
            Call PrcDash2("7")
        End Sub

 Public Sub btnItem_8()
            Call PrcDash2("8")
        End Sub

 Public Sub btnItem_9()
            Call PrcDash2("9")
        End Sub

 Public Sub btnItem_10()
            Call PrcDash2("10")
        End Sub

The Button control/class has a Tag property of type Object. You could simply pass to that property your strings "1", "2" etc. In this way you would only need one eventhandler where you the read the value of the Tag property back, call the ToString() method and then pass this string as argument to the PrcDash2() method.


Rotating lists in linear time and constant space (Java) - follow-up (The previous and the initial iteration.)

After some critique, I found the way to keep the rotation operation also for java.util.Lists that do not provide random access iterators:

package net.coderodde.util;

import java.util.Arrays;
import java.util.Collections;
import java.util.LinkedList;
import java.util.List;
import java.util.Objects;
import java.util.Scanner;

/**
 * This class contains a static method for rotating lists in linear time and
 * constant space.
 *
 * @author Rodion "rodde" Efremov
 * @version 1.6 (Oct 6, 2017)
 */
public final class ListRotation {

    private ListRotation() {}

    /**
     * Performs the list rotation.
     *
     * @param <T> the list element type.
     * @param list the list whose content to rotate.
     * @param rotationCount the number of steps to rotate to the right. If
     * negative, rotates to the left.
     */
    public static <T> void rotate(List<T> list, int rotationCount) {
        Objects.requireNonNull(list, "The input list is null.");
        rotationCount %= list.size();

        if (rotationCount < 0) {
            rotationCount += list.size();
        }

        Collections.<T>reverse(list);
        Collections.<T>reverse(list.subList(0, rotationCount));
        Collections.<T>reverse(list.subList(rotationCount, list.size()));
    }

    public static void main(String[] args) {
        Scanner scanner = new Scanner(System.in);
        List<Integer> list =
                new LinkedList<>(Arrays.asList(1, 2, 3, 4, 5, 6, 7));

        System.out.println(list);

        while (true) {
            int rotationCount = scanner.nextInt();
            rotate(list, rotationCount);
            System.out.println(list);
        }
    }
}

So how does that look now?

Input validation

If the list is empty, there will be division by zero.
Since you check for null input list,
it would make sense to do likewise for empty list as well.

Type parameter

The <T> is not needed in the Collections.<T>reverse statements,
this works just fine:

Collections.reverse(list);
Collections.reverse(list.subList(0, rotationCount));
Collections.reverse(list.subList(rotationCount, list.size()));


Evaluating a dotted quad IP address has clumsy use of inject/fold I wrote this to consume valid IPv4 addresses in dotted-quad decimal form:

def dotted_ip_to_num ip
  strings = ip.split('.').reverse
  strings.each_with_index.inject(0) do |sum, pair|
    str, i = pair
    sum + str.to_i * (2 ** (i * 8))
  end
end

But my chain of each_with_index and inject (aka left-fold or reduce) looks clumsy to me.

Firstly, Ruby won't let me match (aka desctructure) the pair yielded by each_with_index like this |sum, str, i| or this |sum, [str, i]|. Did I miss something? So, I do this on a line of its own (line 4.)

Secondly, although the message chain is only 2 messages, is it so complex in this case that I should split it up to do the summation as an extra line, like this?

octets = strings.each_with_index.collect do |str, i|
  str.to_i * (2 ** (i * 8))
end
octets.inject(0, :+)

Please set aside the improvement of multiplying the accumulator by 256 at each inject/reduce step, as that eliminates each_with_index entirely, and means I won't get the answer to my two main questions.

I also welcome reviews from users of non-Ruby collections libraries. (FYI collect is map in some other languages.)

The correct Ruby-way (also Perl and others) is to use pack/unpack, since low-level bytes job is what they are implemented for.

def dotted_ip_to_num ip
    ip.split(".").map(&:to_i).pack("C*").unpack("N")[0]
end


Making as many unique strings as possible by removing two characters I'm attempting a programming challenge type task in C#, where the goal is to determine how many unique strings can be obtained by removing two characters. The prompt for the task implied that I should create the set of all possible strings with 2 chars removed, then return the number of items in the set. I was initially suspicious of this, as I've often found that the only way to complete these kinds of tasks is to abstract away from actually storing results or enumerating possibilities wherever possible - but due to the requirement that only unique strings should be counted, I don't know how to avoid storing information about every result so far. Supposedly I have to be able to handle strings of up to a million characters in length - and I can't think of any way right now to avoid the hideous iteration count and massive result set that a million character string would require.
Here is my code so far. It works, but its way too slow, and I think large inputs might be generating incorrect results, but I'm not actually sure:
 private static void Main(String[] args)
{
    var input = Console.ReadLine();
    Console.WriteLine(FindBeautifulStrings(input).Count);
}

// B will always be larger than A because of the way we're iterating so we have to remove it first.
private static string RemoveTwo(string input, int indexA, int indexB)
{
    return input.Remove(indexB, 1).Remove(indexA, 1);
}

private static HashSet<int> FindBeautifulStrings(string input)
{
    // Iterate over every character in the string, then for each character, iterate over every
    // other character, removing the two selected characters; return set of all possible results.
    int inputLength = input.Length;
    HashSet<int> results = new HashSet<int>();
    for (int i = 0; i < inputLength; ++i)
    {
        for (int j = i + 1; j < inputLength; ++j)
        {
            results.Add(RemoveTwo(input, i, j).GetHashCode());
        }
    }
    return results;
}

Storing hashes of strings instead of strings themselves is the only idea I've come up with in terms of more efficiently detecting a string identical to one I've already seen. Since the non-duplicate requirement means that the resulting combinations themselves are significant, I haven't been able to avoid working with strings entirely and solve it mathematically instead (if duplicates were permissable, I feel like this could be solved using the equation n!/(n - (n - 2)! * (n - 2)!. Is there any way to determine the non-duplicate possibilities mathematically without iterating over or storing the strings themselves? If not, is there any way to optimise what I have so far?
Edit:
I thought I should clarify - although the question intuitively feels like it's about permutations, a mistake made both by myself initially and by a few others so far, the only operation performed on the original string is removal of characters.
It works, I think, like this:
input: apple

i=0, j=1: ple
i=0, j=2: ple **Doesn't count, duplicate**
i=0, j=3: ppe
i=0, j=4: ppl
i=1, j=2: ale
i=1, j=3: ape
i=1, j=4: apl
i=2, j=3: ape **Doesn't count, duplicate**
i=2, j=4: apl **Doesn't count, duplicate**
i=3, j=4: app

Unique strings: 7

Instead of using string.Remove (and creating a 2 string for each index combination), you could create an array of chars / ints and work on that array.
You dont have to iterate from i = 0 to i = str.Length and j = 0 to j.str.Length because that results in duplicated indexes (e.g. 1,2 and 2,1).
The idea with the hashes is good, but as mentioned in comment, you have to check for all "uncertain duplicates" if they are actually duplicates.

The following code shows a simple implementaion that considers the points above:

public class Variation
{
    public Variation(int hash, int index1, int index2)
    {
        Hash = hash;
        Index1 = index1;
        Index2 = index2;
    }
    public int Index1 { get; }
    public int Index2 { get; }
    public int Hash { get; }
}

public int CountVariantions(string input)
{
    int[] inputArray = input.ToCharArray().Select(c => (int)c).ToArray();
    var variations = new List<Variation>();
    for (int i = 0; i < input.Length; i++)
        for (int j = i + 1; j < input.Length; j++)
            variations.Add(GetVariation(inputArray, i, j));

    var groups = variations.GroupBy(v => v.Hash).ToArray();

    var uncertainDublicates = groups.Where(g => g.Skip(1).Any()).ToArray();

    var dublicatesRealCount = GetRealCount(inputArray, uncertainDublicates);

    return groups.Length - uncertainDublicates.Length + dublicatesRealCount;
}

private int GetRealCount(int[] inputArray, IEnumerable<IGrouping<int, Variation>> duplicates)
{
    // todo: check if the duplicates are actually identically
    return duplicates.Count();
}

private static Variation GetVariation(int[] inputArray, int index1, int index2)
{
    var hashValue = Enumerable
        .Range(0, inputArray.Length)
        .Where(i => i != index1 && i != index2)
        .Select(i => inputArray[i])
        .Aggregate((hash, val) => hash ^ val);

    return new Variation(hashValue, index1, index2);
}


Registering multiple keypresses simultaneously, very slow I'm working on a Pong game and right now I'm working on being able to register multiple KeyDown presses simulatenously. I used the Win32 native method GetKeyBoardState first to loop through all of the virtual keys and see if any of the keys matched the game input keys. Now this worked just fine but in the interest of optimization I figured that it would be quicker to use GetKeyState since there are only 4 valid input keys and then simply check if any of those keys are pressed down.

Surprisingly it is incredibly slow and choppy, a lot worse than GetKeyBoardState so I'm wondering if there's something in my code that's slowing it down. Here's the complete implementation

NativeMethods.cs

internal static class NativeMethods
    {
        [DllImport("user32.dll")]
        [return: MarshalAs(UnmanagedType.Bool)]
        internal static extern bool GetKeyboardState(byte[] lpKeyState);

        [DllImport("user32.dll")]
        internal static extern short GetKeyState(int nVirtKey);
    }

Keyboard.cs

static class Keyboard
{
    public static bool GetKeyState(Keys vKey)
    {
        short results = NativeMethods.GetKeyState((int)vKey);
        // We're only going to use this to look for arrow keys so
        // No need to differntiate results based on low or high order bits
        switch (results)
        {
            case 0: //KEY STATE: NOT PRESSED
                return false;
            case -127: //KEY STATE: PRESSED
                return true;
        }
        return false;
    }

GameForm.cs

 private void GameForm_KeyDown(object sender, KeyEventArgs e)
        {
            var movementData = new Tuple<int, Move>(-1, Pong.Move.Down);

            //Check how many keys that are pressed down (for example both players might be moving their paddles simulatenously)
            var pressedKeys = inputKeys.Where(key => Keyboard.GetKeyState(key));

            //Check if pressed key equals any of the player assigned keys.. If so call MovePaddle()

            foreach (Keys key in pressedKeys)
            {
                switch (key)
                {
                    case Keys.Up:
                        movementData = Tuple.Create(0, Pong.Move.Up);
                        break;
                    case Keys.Down:
                        movementData = Tuple.Create(0, Pong.Move.Down);
                        break;
                    case Keys.W:
                        movementData = Tuple.Create(1, Pong.Move.Up);
                        break;
                    case Keys.S:
                        movementData = Tuple.Create(1, Pong.Move.Down);
                        break;
                }
                gameController.MovePaddle(movementData.Item1, movementData.Item2);
            }
        }

What is impacting you performance so heavily is the creation of Tuples everytime you recieve a keypressed event.

creating objects is slow. What you should do instead is, directly apply the changes to the moving objects (your two pong-thingies).

private void GameForm_KeyDown(object sender, KeyEventArgs e)
{
   var pressedKeys = inputKeys.Where(key => Keyboard.GetKeyState(key));
   foreach(Keys key in pressedKeys)
      switch(key){
         case Keys.Up:
             leftPong.Positon += FIXEDVALUE;
             break;
         case Keys.Down:
             leftPong.Position -= FIXEDVALUE;
             break;
         //continue...


Elegant way to "construct" kind-of enum values in go I'm quite new to Go and would like to improve my code. I have a representation for access levels in Go like this:

type AccessLevelValue int

const (
    GuestPermissions     AccessLevelValue = 10
    ReporterPermissions  AccessLevelValue = 20
    DeveloperPermissions AccessLevelValue = 30
    MasterPermissions    AccessLevelValue = 40
    OwnerPermission      AccessLevelValue = 50
)

What I want is a "constructor" for AccessLevelValue to which I can pass a numeric value from {10,20,30,40,50} and that returns me the according AccessLevelValue. This is my current implementation, but it seems not so nice to me:

func int2AccessLevel(accessLevel int) *AccessLevelValue {
    if accessLevel == 10 { return AccessLevel(GuestPermissions)}
    if accessLevel == 20 { return AccessLevel(ReporterPermissions)}
    if accessLevel == 30 { return AccessLevel(DeveloperPermissions)}
    if accessLevel == 40 { return AccessLevel(MasterPermissions)}
    if accessLevel == 50 { return AccessLevel(OwnerPermission)}
    return nil
}

func AccessLevel(v AccessLevelValue) *AccessLevelValue {
    p := new(AccessLevelValue)
    *p = v
    return p
}

Is there any cleaner implementation that doesn't need the if statements and returns the same result?

Note that unlike C enums, your const declarations define some values of AccessLevelValue, but in Go they are not considered to exhaustively define all valid values of AccessLevelValue.

Also, the values are integer constants - why are you passing around pointers to them?

You might consider a switch statement:

function int2AccessLevel(level_int int) *AccessLevel {
    switch level_int {
    case 10: return GuestPermissions;
    case 20: return ReportedPermissions;
    // ...
    default: panic("Unrecognized value for AccessLevel");
    }
}


Is this a "pythonic" method of executing functions based on the values of a tuple? I have a situation where I have six possible situations which can relate to four different results. Instead of using an extended if/else statement, I was wondering if it would be more pythonic to use a dictionary to call the functions that I would call inside the if/else as a replacement for a "switch" statement, like one might use in C# or php.

My switch statement depends on two values which I'm using to build a tuple, which I'll in turn use as the key to the dictionary that will function as my "switch". I will be getting the values for the tuple from two other functions (database calls), which is why I have the example one() and zero() functions.

This is the code pattern I'm thinking of using which I stumbled on with playing around in the python shell:

def one():
    #Simulated database value
    return 1

def zero():
    return 0

def run():
    #Shows the correct function ran
    print "RUN"
    return 1

def walk():
    print "WALK"
    return 1

def main():
    switch_dictionary = {}

    #These are the values that I will want to use to decide
    #which functions to use
    switch_dictionary[(0,0)] = run
    switch_dictionary[(1,1)] = walk

    #These are the tuples that I will build from the database
    zero_tuple = (zero(), zero())
    one_tuple = (one(), one())

    #These actually run the functions. In practice I will simply
    #have the one tuple which is dependent on the database information
    #to run the function that I defined before
    switch_dictionary[zero_tuple]()
    switch_dictionary[one_tuple]()

I don't have the actual code written or I would post it here, as I would like to know if this method is considered a python best practice. I'm still a python learner in university, and if this is a method that's a bad habit, then I would like to kick it now before I get out into the real world.

Note, the result of executing the code above is as expected, simply "RUN" and "WALK".

In short, a dictionary of functions IS the Python equivalent of the switch statement.

However, your implementation is awful. Start by dropping the "switch" in your variable names. It is not a switch, it is a dictionary. And there is no point in using "dictionary" in the name either. Make a real name like "act_on_msg" but even that is weak in my opinion.

And don't even explain that you are using a dictionary as a switch in your comments. At most, mention that  actOnMsg[combomsg]() runs the action method just to help those who are not used to seeing parentheses in that context.


Timer class in PHP OOP I have done a lot of research on OOP and I keep seeming to read different opinions on several items, especially the scope resolution operator (::) and handling class containing private functions.

When calling a private function from within the class it seems that a majority of programmers recommend self::method(), is this accurate? I have also seen and tested Class::method().

Also, I am looking for more information, or recommendations on appropriate ways to instantiate class variables, whether it be protected, private, var, etc...

Take this Timer class I wrote for example:

class Timer {
    protected $_start, $_pause, $_stop, $_elapsed;
    protected $_laps = array();
    protected $_count = 1;
    protected $_lapTotalTime = 0;

    public function __construct($start = '') {
        $start = strtolower($start);
        ($start === 'start') ? self::start() : NULL;
    }

    public function start( ) {
        $this->_start = Timer::getMicroTime();
    }

    public function stop( ) {
        $this->_stop = Timer::getMicroTime();
    }

    public function pause( ) {
        $this->_pause = Timer::getMicroTime();
        $this->_elapsed += ($this->_pause - $this->_start);
    }

    public function resume( ) {
        $this->_start = Timer::getMicroTime();
    }

    // Used to build an array of times for multiple timers
    public function lap($key = '') {
        ($key === '') ? $key = 'Lap' : $key = $key;
        if (isset($this->_start)) {
            self::stop();
            $this->_lapTotalTime += ($this->_stop - $this->_start);
            $this->_laps[$key.' '.$this->_count] = self::getLapTime();
            self::start();
            $this->_count++;
        }
    }

    public function getTime() {
        if (!isset($this->_stop)) {
            $this->_stop = Timer::getMicroTime();
        }
        if (!empty($this->_laps)) {
            $this->_laps['Total'] = self::timeToString($this->_lapTotalTime);
            return $this->_laps;
        }
        return self::timeToString();
    }

    // PRIVATE CLASS FUNCTIONS
    private function getLapTime() {
        return self::timeToString();
    }

    private function getMicroTime( ) {
        list($usec, $sec) = explode(' ', microtime());
        return ((float) $usec + (float) $sec);
    }

    private function timeToString($seconds = '') {
        if ($seconds === '') {
            $seconds = ($this->_stop - $this->_start) + $this->_elapsed;
        }
        $seconds = Timer::roundMicroTime($seconds);
        $hours = floor($seconds / (60 * 60));
        $divisor_for_minutes = $seconds % (60 * 60);
        $minutes = floor($divisor_for_minutes / 60);
        return $hours . "h:" . $minutes . "m:" . $seconds . "s";
    }

    private function roundMicroTime($microTime) {
        return round($microTime, 4, PHP_ROUND_HALF_UP);
    }
}

All of my instantiated variables are declared as protected. My reasoning, if I am understanding correctly is because although this Class doesn't extend another class, the other class may extend Timer. Am I on the right track?

Example of Timer use with laps:

$t = new Timer('start');
usleep(3215789);

// Declare a key for the lap array
$t->lap('Sleep');
usleep(4445666);

// No key declaration
$t->lap();
usleep(1000000);

echo "
";
if (is_array($t->getTime())) {
    print_r($t->getTime());
} else {
    echo $t->getTime();
}
echo "
";

Output:

Array
(
    [Sleep 1] => 0h:0m:3.2154s
    [Lap 2] => 0h:0m:4.4462s
    [Total] => 0h:0m:7.6616s
)

Without laps:

$t = new Timer('start');
usleep(3215789);
usleep(1000000);

echo "
";
if (is_array($t->getTime())) {
    print_r($t->getTime());
} else {
    echo $t->getTime();
}
echo "
";

Output:

0h:0m:4.2158s

Never ever use var. That's a deprecated form of public;
Avoid using public on your properties, you want to have controll about the value of every property in your class;
If you use protected or private doesn't make much sense. Protected means it can't be accessed from the outer scope, but it can be accessed from another class which extends this class. Private means it can only be accessed from the class where it is defined.

I like to have getters/setters for every property and never use $this->property directly inside my class, some other don't like that. See also this article by Fabien Potencier: "Pragmatism over Theory: Protected vs Private";
The :: operator, or T_PAAMAYIM_NEKUDOTAYIM, should only be used by accessing class constants and static vars/methods;
Avoid using static vars/methods;
Use $this->method() or $this->property to access properties from the inside of a class. It works just like you use it outside the class, except that you use the $this variable (which contains a reference to the current instance of the class).


Security concerns with locking files I'm writing a perl script which reads logs from a file and looks for matches from another file before sending the matches off by email. The problem is, I'm dealing with files which could be changed (a log written to them by syslog) at any time. I discovered that files can be locked in perl, however I'm not sure if it is 100% secure yet. Here is the pertinent snippet:

    # Check for valid email
    unless ($mailto =~ EMAIL_REGEX) {
        die "Please supply a valid email"
    }

    # Build a rule from a key and an email
    # Write it to LOGMAN_CONF
    my $key_and_email = $keyword.":".$mailto."\n";
    open (RULES, ">>", LOGMAN_CONF)
        or die "Cannot open logman configuration file ".LOGMAN_CONF;
    flock(RULES, LOCK_EX) or die "Can't lock ".LOGMAN_CONF;
    print RULES $key_and_email;
    close RULES;

    # Check if CRONTAB_LOC needs a send entry
    open (CRON, "<", CRONTAB_LOC)
        or die "Cannot open crontab file ".CRONTAB_LOC;
    flock(CRON, LOCK_SH) or die "Can't lock ".CRONTAB_LOC;
    my @cronLines = <CRON>;
    close CRON;

    my $match = 0;
    foreach my $line (@cronLines) {
        if ($line eq CRON_ENTRY) {
            $match = 1;
        }
    }
    unless ($match == 1) {
        open (CRON, ">>", CRONTAB_LOC)
            or die "Cannot open crontab file ".CRONTAB_LOC;
        flock(CRON, LOCK_EX) or die "Can't lock ".CRONTAB_LOC;
        print CRON CRON_ENTRY;
        close CRON;
    }

Feel free to critique my style or structure, as this is my first contact with Perl.

NB: There are a couple of constants used above which I haven't shown where they are defined. I could add them if anyone needs, but they are for the most part location to files.

Locking is advisory which mean unless syslog is also doing flock() before write (which is doubtful) your locking scheme will have no effect. Some operating systems like AIX do have manditory/enforced locking mechanisms but I don't believe they're standard at the filesystem level across any UNIX system.


Performance optimization in function for datastructure mapping I want to optimize a Perl function which is frequently used in my application. The function creates a special datastructure from the results of DBI::fetchall_arrayref which looks like:

$columns = ['COLNAME_1','COLNAME_2','COLNAME_3']
$rows    = [ ['row_1_col_1', 'row_1_col_2', 'row_1_col_3'],
             ['row_2_col_1', 'row_2_col_2', 'row_2_col_3'],
             ['row_3_col_1', 'row_3_col_2', 'row_3_col_3']
];

The new datastructure must contain the data in the following form (all row-values for every column in a single arrayref)

$retval = {
    row_count => 3,
    col_count => 3,
    COLNAME_1 => ['row_1_col_1', 'row_2_col_1', 'row_3_col_1' ],
    COLNAME_2 => ['row_1_col_2', 'row_2_col_2', 'row_3_col_2' ],
    COLNAME_3 => ['row_1_col_3', 'row_2_col_3', 'row_3_col_3' ]
}

The new datastructure is a Hash of Arrays and is used in the whole application. I cannot change the format (its too frequently used). I wrote a function for this conversion. I've already done some some performance optimization after profiling my application. But it's not enough. Now the function looks like:

sub reorganize($$) {
    my ($self,$columns,$rows) = @_;
    my $col_count = scalar(@$columns);
    my $row_count = scalar(@$rows);
    my $col_index = 0;
    my $row_index = 0;
    my $retval = {  # new datastructure
        row_count   => $row_count,
        col_count   => $col_count
    };

    # iterate through all columns
    for($col_index=0; $col_index<$col_count; $col_index++) {
        # create a arrayref for all row-values of the current column
        # set it to the correct size and assign all values to this arrayref
        my $tmp = [];
        $#{$tmp} = $row_count-1;    # set size of array to the number of rows

        # iterate through all rows
        for($row_index=0; $row_index<$row_count; $row_index++) {
            # assign values to arrayref (which has the correct size) instead of a "slow" push
            $tmp->[$row_index] = $rows->[$row_index][$col_index];
        }
        # Assign the arrayref to the hash. The hash-key is the name of the column
        $retval->{$columns->[$col_index]} = $tmp;
    }
    return $retval;
}

My Question:

Is there a way to further optimize this function (maybe using $[...])?  I found some hints here at page 18 and 19, but I don't have any experience in using $ in different contexts.

I have to say that the function listed above is the best I can do. There may be other ways to do some optimization which I have never heard of.

The following code is about 35% faster (measured with Benchmark). The tricks:

no anonymous array created for $tmp.
explicit return removed.
variables created in place where their value is needed.

Some of the tricks added just a 3%, the first one seemed the most important. YMMV.

I experimented with $_ and maps, too, but it seems the plain old C-style loop is the fastest.

sub faster {
    my ($self, $columns, $rows) = @_;
    my $retval = {
        row_count   => my $row_count = @$rows,
        col_count   => my $col_count = @$columns,
    };
    for (my $col_index = 0 ; $col_index < $col_count ; $col_index++) {
        my $tmp;
        for (my $row_index = 0 ; $row_index < $row_count ; $row_index++) {
            $tmp->[$row_index] = $rows->[$row_index][$col_index];
        }
        $retval->{$columns->[$col_index]} = $tmp;
    }
    $retval
}