Moving List View items from one list to another I'm writing a form that contains 2 ListViews with a set of 4 buttons. 2 move all the items from one to the other in each direction, the other move only selected items in each direction.

Nothing fancy or complicated.

However I'm feeling like I could write the MoveItem method in a more elegant way and was wondering if any would mind taking a look and advising me.

private void MoveItem(ListView source, ListView dest, bool all)
{
    if (all)
    {
        foreach (ListViewItem item in source.Items)
        {
            source.Items.Remove(item);
            dest.Items.Add(item);
        }
    }
    else
    {
        foreach (ListViewItem item in source.SelectedItems)
        {
            source.Items.Remove(item);
            dest.Items.Add(item);
        }
    }
}

Any suggestions would be thankfully recived

Before thinking about how to write this in a more elegant way, you should first ask yourself: is this code actually correct? The “problem” with that code is that you're iterating the collection you're modifying. Quite often (for example in the case of List<T>), that's not allowed and your foreach would throw an exception. Fortunately for you, the enumerator of ListViewItemCollection iterates over a snapshot of the collection of the items, so your code will actually work fine (although I didn't find this behavior documented anywhere).

Now, if you want to refactor code like this, just have a look at what's the same and what's different. You can notice that the only different thing in the two branches is the source of the foreach. So, just create a local variable for it, set it correctly and then have common code that iterates it:

private static void MoveItem(ListView source, ListView dest, bool all)
{
    var items = all ? source.Items : source.SelectedItems;

    foreach (ListViewItem item in items)
    {
        source.Items.Remove(item);
        dest.Items.Add(item);
    }
}


Check if the jQuery library is present locally Q.

Basically this piece of code appears in my class constructor. It's working, but it seems very confusing.

It basically checks if the jQuery library is present locally and if it's not it will try to download it.

Is there any way I can improve this? Namely avoiding re-setting vars all the time?

CODE:

//First we check if JQuery version is correct
if ( isset($versionList->$version) ) {
    $filePathFull = __DIR__ . '/versions/jquery-' . $version . '.js';
    $filePathMin  = __DIR__ . '/versions/jquery-' . $version . '.min.js';
    $this->versionLoaded = $version;

    //Now we check if the JQuery version is installed
    if ( !file_exists($filePathFull) || !file_exists($filePathMin)) {

        // They are not installed, so we will try to download and install it
        if (!self::installVersion($version)) {
            // Download failed, so we try the defaults
            $filePathFull = __DIR__ . '/versions/jquery-' . JQuery::DEFAULT_VERSION . '.js';
            $filePathMin = __DIR__ . '/versions/jquery-' . JQuery::DEFAULT_VERSION . '.min.js';
            $this->versionLoaded = JQuery::DEFAULT_VERSION;
        }
    }
} else {
    //Version is incorrect we go with the defaults
    $filePathFull = __DIR__ . '/versions/jquery-' . JQuery::DEFAULT_VERSION . '.js';
    $filePathMin = __DIR__ . '/versions/jquery-' . JQuery::DEFAULT_VERSION . '.min.js';
    $this->versionLoaded = JQuery::DEFAULT_VERSION;
}

//This is a redundant check to see if the defaults are installed
if ( !file_exists($filePathFull) || !file_exists($filePathMin) ) {

    // They are not installed, so we will try to download and install it
    if (!self::installVersion(JQuery::DEFAULT_VERSION)) {
        //Defaults are not installed so we throw an exception
        throw new Exception("Nor the version $version or the default version " . JQuery::DEFAULT_VERSION . " could be found or downloaded");
    }
}

Edit:

MORE DETAILS

A more detailed explanation...

As you correctly assumed, this is a "plugin". The plugable system requires wrapped JavaScript libraries in PHP classes that are used for a JS Dependency Manager. The JS Dependency Manager extends Composer which constrains how we handle this a bit. We have, however, freedom to chose how we wrap the JS Library.

Goals:

Avoidance of library repetition (like having 2 versions of jQuery running which causes weird JS bugs)
Automatic update of 3rd party JavaScript (directly from the repository instead of downloading and including them directly)
Caching, Minimizing and removal of dead code from JS Scripts through Google Closure Compiler
Libraries and scripts dependency management (specially regarding specific library versions such as jquery)

Well, you could set this up to use additional methods. You don't have to throw everything into the constructor. This would be a big step in the right direction, at least in as far as OOP and best practice is concerned. This will help you to understand what is going on much easier while also helping you to avoid violating basic OOP principles. For instance:

Single Responsibility

The first principle you are violating, or so it seems, is the Single Responsibility Principle. As the name implies, your classes and methods should be responsible for just one thing, to the exclusion of all others. They can request information from others if necessary, but they should not be aware of how those others work. This principle is typically demonstrated with a function/method, but should also be applied to the classes themselves.

It's hard to tell in this context, because all you provided were the guts, but, judging by the static properties, I would assume you are handling these loading failures for another class. If you are not able to modify the class in question, such as if this were a library, then this functionality should be added to an intermediary, such as a controller, otherwise this really belongs in the JQuery class. Let's continue under the assumption that this is being constructed in said intermediary.

Don't Repeat Yourself

The "Don't Repeat Yourself" (DRY) Principle is another of the principles you are violating. As the name implies, your code should not repeat. Typically this is done through functions/methods, but it can also be done with variables/properties and loops, though it is not just confined to this. Keeping your code DRY takes a lot of foresight, but really helps in the long run. So let's take a look at how your code is repeating itself and try to fix this.

This should be fairly obvious. There are two files that are being verifyed and the same things are being done to both files. To paraphrase someone important to DRY (can't remember who, only that it was similar to the Rule of Three): we can either do something once, or do something many times; There is no such thing as doing something only twice. To follow this pearl of wisdom, we can create a method, and then use that method each time we need it. This also follows the Single Responsibility principle because it abstracts the task away from the constructor, whose only job is to instantiate initial values.

public function verifyVersion( $file, $version ) {
    //generalized verification code
}

As the comment says, this method should be generalized for a single file, which can either have a boolean return value or a boolean property it manipulates to show whether verification was a success. It seems like you already have such a property in $versionLoaded so we can use that. Its not exactly a boolean, but it can be used as one so long as its default value is FALSE or NULL.

if( ! $this->versionLoaded ) {
    throw new Exception(
        "Neither the version $version nor the default version "
        . JQuery::DEFAULT_VERSION
        . " could be found or downloaded"
    );
}

BTW: I slightly edited your exception because the grammar was bothering me. "Nor" is the negative connotation of "or" and "neither" prefixes a list of negatives. Though I'm not an English major, so you can take that with a grain of salt.

How do we generalize our check? And equally important, how do we do it just once? In your code you were checking if these files exist not once, but twice. If we check it once, and assume that if the version is loaded then the file exists, then we wont need to check it a second time.

if( file_exists( $file ) || self::installVersion( $version ) ) {
    $this->versionLoaded = $version;
}

That should leave us with one final task. How to check each file. Well, I mentioned that loops were sometimes used in DRY. Here's a pretty good time to use one.

$header  = __DIR__ . '/versions/jquery-';
$default = JQuery::DEFAULT_VERSION;

$files = array(
    $header . $version . '.js',
    $header . $version . '.min.js',
    $header . $default . '.js',
    $header . $default . '.min.js'
);

foreach( $files AS $file ) {
    $this->verifyVersion( $file, $version );
    if( $this->versionLoaded ) {
        return;
    }
}

throw new Exception(
    "Neither the version $version nor the default version "
    . JQuery::DEFAULT_VERSION
    . " could be found or downloaded"
);

Of course, this is still violating the Single Responsibility Principle. This constructor is still doing too much, but I'll leave that up to you to refactor. If you use everything I posted above it should be pretty simple. I'll give you a couple of hints though:

Those files are not violating DRY. You might be thinking that using $header and '.js' repeatedly is a violation, but that is to ensure that our method is as reusable as possible. The only way they might be is by their similar structure, which I can't see refactoring without confusing the issue. Though you might want to try that for yourself.
I think a pretty good name for this new method might be loadJQuery().

I hope this helps!

Edit

I forgot about the isset( $versionList->$version ); bit, so the above code will need to be refactored for that as well, but I think the solution should be obvious once that last method is implemented. If you need clarification, let me know.


"B1ff filter" reads and returns message This is a programming exercise from chapter 8 of a C learning book by K N King. The program should prompt a user for a message, and return it after translating it to B1ff speak. e.g A->4, b->8 etc

The program works as asked in the exercise, however how can I create a more efficient declaration of the message[] array? Currently it is initialized to size N which is defined as 100 (large enough for most messages), instead of the size of the message from the user.

#include <stdbool.h>
#include <stdlib.h>
#include <time.h>
#include <stdio.h>
#include <ctype.h>

#define N 100

int main(int argc, const char * argv[]) {

    //Initialize
    char message[N] = {' '};
    char ch;
    int i = 0, j = 0, k= 0;

    //Prompt user for message
    printf("Enter a message: ");
    //Get message and put each character into an array
    while((ch = getchar()) != '\n'){
        ch = toupper(ch);
        message[i] = ch;
        i++;
    }
  //Print the B1ff speak converting characters
    printf("In B1FF-speak: ");
    for(k = 0; k<i; k++){
        switch(message[k]){
            case 's':
                printf("5");
                break;
            case 'O':
                printf("0");
                break;
            case 'I':
                printf("1");
                break;
            case 'E':
                printf("3");
                break;
            case 'B':
                printf("8");
                break;
            case 'A':
                printf("4");
                break;
            default:
                printf("%c", message[k]);
                break;
        }
    }

    //Print 10 or so exclamation marks and be a citizen
    printf("!!!!!!!!!!!!!!\n");
    return 0;
}

Unused: we don't use j, and we don't need to include <stdbool.h>, <stdlib.h> or <time.h>.
We have two loops - one that reads input into message[] and one that converts and outputs the content of message[].  We can avoid the storage by combining the two loops into one:
while ((ch = getchar()) != '\n') {
    ch = toupper(ch);

    switch (ch) {
        /* ... */
    }

There's a couple of fixes needed above.  Firstly, we need to check that ch is not EOF, or we'll loop indefinitely if the input stream is closed before the first newline.  ch needs to be an int value here, as the distinction between a character and end-of-file is lost when we narrow to char.
Secondly, ch holds values of a (possibly signed) char, but toupper() (like all the <ctype.h> functions) works on an int representation of unsigned char value.  Fixing those gives us:
int ch;
while ((ch = getchar()) != '\n' && ch != EOF) {
    ch = toupper((unsigned char)ch);

Instead of the switch, we might consider a table-driven approach.  That would work something like this:
char conversions[UCHAR_MAX];
/* identity mapping for most chars */
for (unsigned i = 0;  i < sizeof conversions;  ++i) {
    conversions[i] = (char)i;
}
/* exceptions */
conversions['S'] = '5';
conversions['O'] = '0';
conversions['I'] = '1';
conversions['E'] = '3';
conversions['B'] = '8';
conversions['A'] = '4';

int ch;
while ((ch = getchar()) != '\n' && ch != EOF) {
    putchar(conversions[toupper(ch)]);
}

Finally, puts() is simpler than printf() for simple strings ending in newline, and we don't need to return 0 from main().

Modified code
#include <ctype.h>
#include <limits.h>
#include <stdio.h>

int main(void)
{
    char conversions[UCHAR_MAX];
    /* identity mapping for most chars */
    for (unsigned i = 0;  i < sizeof conversions;  ++i) {
        conversions[i] = (char)i;
    }
    /* exceptions */
    conversions['S'] = '5';
    conversions['O'] = '0';
    conversions['I'] = '1';
    conversions['E'] = '3';
    conversions['B'] = '8';
    conversions['A'] = '4';

    printf("Enter a message: ");

    int ch;
    while ((ch = getchar()) != '\n' && ch != EOF) {
        putchar(conversions[toupper(ch)]);
    }

    // Finish the sentence
    puts("!!!!!!!!!!!!!!");
}


Building a summary string from a flags enum I'm trying to replicate the attributes text that appears in File Explorer, for files and directories.

I can't just use the enum names, as the summary text uses a single character for each set flag, and that character isn't always the first character of the enum member. I'm also not using the reserved FileAttribute.Device attribute. So, I'm using a string.join operation to concatenate the flag characters.

    public string AttributeSummary
    {
        get
        {
            FileAttributes fatt = (FileAttributes)_attribs;

            return string.Join(string.Empty,
                (fatt.HasFlag(FileAttributes.ReadOnly) ?          "R" : string.Empty),
                (fatt.HasFlag(FileAttributes.Hidden) ?            "H" : string.Empty),
                (fatt.HasFlag(FileAttributes.System) ?            "S" : string.Empty),
                (fatt.HasFlag(FileAttributes.Directory) ?         "D" : string.Empty),
                (fatt.HasFlag(FileAttributes.Archive) ?           "A" : string.Empty),
                (fatt.HasFlag(FileAttributes.Normal) ?            "N" : string.Empty),
                (fatt.HasFlag(FileAttributes.Temporary) ?         "T" : string.Empty),
                (fatt.HasFlag(FileAttributes.SparseFile) ?        "P" : string.Empty),
                (fatt.HasFlag(FileAttributes.ReparsePoint) ?      "L" : string.Empty),
                (fatt.HasFlag(FileAttributes.Compressed) ?        "C" : string.Empty),
                (fatt.HasFlag(FileAttributes.Offline) ?           "O" : string.Empty),
                (fatt.HasFlag(FileAttributes.NotContentIndexed) ? "I" : string.Empty),
                (fatt.HasFlag(FileAttributes.Encrypted) ?         "E" : string.Empty),
                (fatt.HasFlag(FileAttributes.IntegrityStream) ?   "V" : string.Empty),
                (fatt.HasFlag(FileAttributes.NoScrubData) ?       "X" : string.Empty));
        }
    }

Am I going about this in the most efficient way?

Am I going about this in the most efficient way?

If you're worried about efficiency, then Enum.HasFlag is known to be fairly slow.

Instead, I would use a very simple helper method:

private static bool HasFlag(FileAttributes value, FileAttributes flag)
    => value & flag != 0;

Though if you're doing that, you might as well expand the helper function to give you the flag character:

private static string GetAttributeCharacter(
    FileAttributes value, FileAttributes flag, string character)
    => (value & flag != 0) ? character : string.Empty;

I'm on the fence whether the helper method should access _attribs directly or whether it should get its value in a parameter. On one hand, reading the field over and over is likely going to be slower. On the other hand, that's the kind of microoptimization that does not matter in the vast majority of cases and accessing it directly would remove quite a lot of duplicated code.

Using string.Concat(…) instead of string.Join(string.Empty, …) is simpler and should be faster.

If you're on C# 6.0, you can avoid some code repetition by using using static System.IO.FileAttributes;.

If it's clear what type a variable is, like your fatt, most code styles accept using var for its type. Also, I think that abbreviations like fatt are generally not worth it: they save you some type, but make understanding the code later harder.

Also, I don't understand why is the cast to FileAttributes necessary, but I'm going to assume there is a good reason for it.

Put together:

private string GetAttributeCharacter(FileAttributes flag, string character)
    => ((FileAttributes)_attribs & flag != 0) ? character : string.Empty;

public string AttributeSummary
{
    get
    {
        return string.Concat(
            GetAttributeCharacter(ReadOnly,          "R"),
            GetAttributeCharacter(Hidden,            "H"),
            GetAttributeCharacter(System,            "S"),
            GetAttributeCharacter(Directory,         "D"),
            GetAttributeCharacter(Archive,           "A"),
            GetAttributeCharacter(Normal,            "N"),
            GetAttributeCharacter(Temporary,         "T"),
            GetAttributeCharacter(SparseFile,        "P"),
            GetAttributeCharacter(ReparsePoint,      "L"),
            GetAttributeCharacter(Compressed,        "C"),
            GetAttributeCharacter(Offline,           "O"),
            GetAttributeCharacter(NotContentIndexed, "I"),
            GetAttributeCharacter(Encrypted,         "E"),
            GetAttributeCharacter(IntegrityStream,   "V"),
            GetAttributeCharacter(NoScrubData,       "X"));
    }
}


Sum of digits in Clojure I wrote a simple program to sum the digits of a number in Clojure for learning purpose. Not very complex (I hope) so not adding any explanation. I am a beginner in Clojure so please don't mind pointing out even the most obvious mistakes.

NOTE: It recursively calculates the sum of digits in the result until a single digit result is obtained. For example, given "4312", 1 is returned (4312 -> 10 -> 1).

(defn sum-once [x]
  (reduce +
          (map
            #(Integer/parseInt (str %))
            (seq (char-array x)))))

(defn sum-digits [x]
  (let [y (sum-once x)]
    (if (< y 10) y
                 (sum-digits (str y)))))

I was confused by your code until reading the comments. You want to change the name of sum-once to sum-digits or digit-sum (pick this one!) or anything like it, since that's what it really does.

Your current sum-digits should become digital-root (that's the term for what you're trying to achieve).

  The digital root (also repeated digital sum) of a non-negative integer
  is the (single digit) value obtained by an iterative process of
  summing digits, on each iteration using the result from the previous
  iteration to compute a digit sum. The process continues until a
  single-digit number is reached.


LaTeX table generator - file in argument list My code generates a LaTeX table from a plain text file which uses whitespace as delimiters. It asks you the title of the columns one-by-one (mind the formatting!). Then it outputs a latex.txt file with a nicely formatted table inside.

How can I improve it?

Here's the code:

#include <iostream>
#include <fstream>
#include <sstream>
#include <vector>

int main(int argc, char* argv[])
{
    if(argc!=2){
        std::cerr << "Error loading file." << std::endl;
        return 1;
    }
    int col = 0, row = 0;
    // First of all. Provide an input file split by whitespaces.
    std::ifstream fin(argv[1],std::ios::in);
    std::vector< std::vector<double> > matrix;
    while(!fin.eof()){
        std::vector<double> v;
        std::string line;
        getline(fin, line);
        std::istringstream nums(line);
        double x;
        while(nums >> x){
            v.push_back(x);
        }
        matrix.push_back(v);
        row++;
    }
    // Basic.
    row--;
    col = matrix[0].size();
    std::ofstream fout("latex.txt",std::ios::out);
    fout << "\\begin{center}\n\\begin{tabular}{";
    for(int i=0;i<col;i++){
        fout << "|c";
    }
    fout << "|}\n";
    fout << "\\hline\n";
    // Ask for col titles:
    for(int i=0;i<col;i++){
        std::cout << "What is the title of the " << i+1 <<  ". column?(Please mind the formatting!)" << std::endl;
        std::string title;
        getline(std::cin,title);
        fout << title;
        if(i!=col-1){
            fout << " & ";
        }else{
            fout << " \\\\\n\\hline\n";
        }
    }
    for(int i=0;i<row;i++){
        for(int j=0;j<col;j++){
            fout << matrix[i][j];
            if(j!=col-1){
                fout << " & ";
            }else{
                fout << " \\\\\n\\hline\n";
            }
        }
    }
    fout << "\\end{tabular}\n";
    fout << "\\end{center}";
    return 0;
}

Argument checking
You test that the user has specified exactly one input file, but if not, then print the misleading message

Error loading file.

You would be better with a usage message, such as
std::cerr << "Usage: " << argv[0] << " filename" << std::endl;

Well done for correctly using cerr for such messages.
File checking
Here, you use the supplied filename, but never check whether fin was successfully opened.  You probably want:
std::ifstream fin(argv[1],std::ios::in);
if (!fin) {
    std::cerr << argv[1] << ": not readable" << std::endl;
    return 1;
}

Similarly for fout:
std::ofstream fout("latex.txt", std::ios::out);
if (!fin) {
    std::cerr << "latex.txt" << ": not writeable" << std::endl;
    return 1;
}

The reading loop also doesn't check for errors, and it has the while (!eof()) antipattern.  This should be something more like (untested):
std::string line;
while (getline(fin, line)) {
    std::vector<double> v;
    std::istringstream nums(line);
    double x;
    while (nums >> x) {
        v.push_back(x);
    }
    if (!nums.eof()) {
        std::cerr << argv[1] << ": parse error" << std::endl;
    }
    matrix.push_back(v);
    row++;
}
if (!fin) {
    std::cerr << argv[1] << ": reading failed" << std::endl;
    return 1;
}

At the very end of the program, check the fail bit of fout:
fout << "\\end{tabular}\n";
fout << "\\end{center}";
return !fout;

Input values checking
If there were no values, you might want to fail before starting to write the output:
const auto rows = matrix.size();
if (rows == 0) {
    std::cerr << argv[1] << ": empty" << std::endl;
    return 1;
}

I'm not sure whether the input needs to be rectangular (i.e. all rows have the same number of columns).  If so, test it:
const auto cols = matrix[0].size();
for (auto i = cols/cols;  i < matrix.size();  ++i) {
    if (matrix[i].size() != cols) {
        std::cerr << argv[1] << ":" << (i+1)
                  << ": line has " << matrix[i].size() << " columns, expected " << cols << std::endl;
        return 1;
    }
}

That cols/cols is just a shorter way to write (std::vector<double>::size_type)1.
Overall approach
You probably don't want to be typing column headings each time you update the data file, so you're most likely to redirect stdin from a file (and you may well want to automate with Make or similar).  I'd suggest that you accept two filename arguments - column headings first, one per line; numeric data second.  That way, you can use the number of lines of the first file as the expected column count when reading the data file (and you could error early when reading a non-conforming line).
Reading the column names first gives you another potential advantage: you could transform the input to output line by line, rather than having to store all lines to the end.  This makes the program a much better pipeline player, and is a good practice when the inputs might be very large.
You might consider simply writing the output to standard out, rather than opening latex.txt; that gives you more flexibility in how you use it.  You'd have to not prompt for the headings then, though!
My version
I've only compiled this, not run it.  In particular, I may well have bad logic in reading the double values from the line string, as I don't normally read whole lines and then parse them.  I've made a couple of other changes, such as reducing scope of temporary variables, closing fin as soon as we've finished reading, and sprinkling const fairy-dust where I can.
#include <iostream>
#include <fstream>
#include <sstream>
#include <vector>

int main(int argc, char* argv[])
{
    if (argc!=2) {
        std::cerr << "Usage: " << argv[0] << " filename" << std::endl;
        return 1;
    }

    std::ifstream fin(argv[1], std::ios::in);
    if (!fin) {
        std::cerr << argv[1] << ": not readable" << std::endl;
        return 1;
    }

    std::vector< std::vector<double> > matrix;

    // Read input values into matrix
    {
        std::string line;
        while (getline(fin, line)) {
            std::vector<double> v;
            std::istringstream nums(line);
            double x;
            while (nums >> x) {
                v.push_back(x);
            }
            if (!nums) {
                std::cerr << argv[1] << ": parse error" << std::endl;
            }
            matrix.push_back(v);
        }
    }
    if (!fin) {
        std::cerr << argv[1] << ": reading failed" << std::endl;
        return 1;
    }
    fin.close();

    // Test that input isn't empty
    const auto rows = matrix.size();
    if (rows == 0) {
        std::cerr << argv[1] << ": empty" << std::endl;
        return 1;
    }
    // Test that input is rectangular
    const auto cols = matrix[0].size();
    for (auto i = cols/cols;  i < matrix.size();  ++i) {
        if (matrix[i].size() != cols) {
            std::cerr << argv[1] << ":" << (i+1)
                      << ": line has " << matrix[i].size() << " columns; expected " << cols << std::endl;
            return 1;
        }
    }

    auto const& outfile = "latex.txt";
    std::ofstream fout(outfile, std::ios::out);
    if (!fin) {
        std::cerr << outfile << ": not writeable" << std::endl;
        return 1;
    }

    static auto const col_sep = " & ";
    static auto const row_sep = " \\\\\n\\hline\n";

    fout << "\\begin{center}\n\\begin{tabular}{";
    for (int i=0;  i<col;  ++i) {
        fout << "|c";
    }
    fout << "|}\n"
         << "\\hline\n";

    // Ask for col titles:
    for (int i=0;  i<cols;  ++i) {
        std::cout << "Enter the title of column " << i+1 << ", as TeX source:" << std::endl;
        std::string title;
        getline(std::cin,title);
        fout << title
             << (i==cols-1 ? row_sep : col_sep);
    }

    // Write out the values
    for (const auto& v: matrix) {
        for (auto i=0u;  i<cols;  ++i) {
            fout << v[i]
                 << (i==cols-1 ? row_sep : col_sep);
        }
    }

    fout << "\\end{tabular}\n"
         << "\\end{center}";
    fout.close();

    if (!fout) {
        std::cerr << "Failed to write " << outfile << std::endl;
        return 1;
    }

    return 0;
}


HMAC with SHA256/512 in Rust I've tried as best I can, to implement HMAC as specified in the RFC 2104. This is also my first Rust project, so the chances that rookie mistakes have been made are high. Since I'm fairly new to the language, I'm looking for feedback in general.

use functions;
use ring::{digest, test};

// Set blocksizes
pub const BLOCKSIZE_256: usize = 64;
pub const BLOCKSIZE_512: usize = 128;

/// Return either a SHA256 or SHA512 digest of byte vector
fn hash(variant: i32, mut data: Vec<u8>) -> Vec<u8> {
    if variant == 256 {
        data = (digest::digest(&digest::SHA256, &data).as_ref()).to_vec();
    } else if variant == 512 {
        data = (digest::digest(&digest::SHA512, &data).as_ref()).to_vec();
    } else {
        panic!("Invalid variant. Valid variants are 256 and 512.");
    }
    return data;
}

/// Return a key k that has been padded to fit the selected blocksize
fn key_deriv(variant: i32, mut k: Vec<u8>) -> Vec<u8> {
    if variant == 256 {
        // If key k is bigger than blocksize, it should be hashed and then padded with zeroes
        // to fit blocksize
        if k.len() > BLOCKSIZE_256 {
            k = hash(variant, k);
        }
        while k.len() < BLOCKSIZE_256 {
            k.push(0x00);
        }
        return k;
    } else if variant == 512 {
        if k.len() > BLOCKSIZE_512 {
            k = hash(variant, k);
        }
        while k.len() < BLOCKSIZE_512 {
            k.push(0x00);
        }
        return k;
    } else {
        panic!("Invalid variant. Valid variants are 256 and 512.");
    }
}

/// Returns an HMAC from message m and key k
pub fn hmac(variant: i32, mut k: Vec<u8>, mut m: Vec<u8>) -> Vec<u8> {
    // Initialize vectors that will hold the ipad and opad
    let mut ipad = vec![];
    let mut opad = vec![];
    // Pad the key
    k = key_deriv(variant, k);

    for count in 0..k.len() {
        ipad.push(k[count] ^ 0x36);
        opad.push(k[count] ^ 0x5C);
    }

    ipad.append(&mut m);
    ipad = hash(variant, ipad);
    opad.append(&mut ipad);
    opad = hash(variant, opad);

    return opad;
}

A possible improvement I'm considering is to change the zero padding of k in key_deriv(). Right now it operates on a while loop, but I noticed that an approach like below, was slightly faster. Though I suspected it might be a little more memory intensive because of the vector allocation and I'm not sure which of the two to choose.

let len = k.len();
if len < BLOCKSIZE_256 {
    k.append(&mut vec![0x00; (BLOCKSIZE_256-len)]);
}

A comment stating that you are declaring values isn't useful.
Don't use return at the end of blocks; just let the block evaluate to the last expression.
This extends to conditionals like if/else: just let the condition evaluate, there's no reason to set a variable. Note that this allows you to avoid unneeded mutability.
Any time you find yourself writing parallel conditions in multiple places, that's an opportunity to add abstraction. In this case, an enum would be sufficient. Note that this removes the panics; we've statically removed an entire failure case!
You don't need to add parenthesis for chained functions.
Pay attention to your duplication between your conditionals. You can extract most of the duplicated code to DRY up the logic. Note how you even have comments on only one of your two conditional branches even though it should apply to both.
This even makes it so that we no longer need constants because the match prevents duplication.
Instead of looping and pushing values, use Vec:resize
Instead of calling the method key_deriv and having comments explaining that it pads it, just rename the method.
Don't needlessly make variable bindings mutable; it's completely fine to re-bind the same name with let. A good example of this is the key in hmac.
Take &[u8] if you aren't making use of the memory allocation.
Use a Cow to avoid allocating anything if the key is already correctly sized.
Creating a Vec and then manually pushing bytes is probably the slowest path. Instead, either allocate a Vec with enough space or use collect which knows how many elements there are. You could also duplicate the key twice and then mutate it in place.
Avoid indexing into a slice when feasible. Iterators tend to have less overhead.
Don't use variable names like k. Instead, type out very long words like "key".
There's no real reason to use Vec::append as you don't use the source vector until it's destroyed a few lines later. Might as well use the boring extend_from_slice.
Because you've forced hash to return a Vec, you are forcing one extra allocation (that of hash(inner_pad)) You can avoid that by returning the result of the digest and only converting to a Vec when needed.

extern crate ring;

use ring::digest;
use std::borrow::Cow;

enum Variant {
    Small,
    Big,
}

impl Variant {
    /// Return either a SHA256 or SHA512 digest of byte vector
    fn hash(&self, data: &[u8]) -> Vec<u8> {
        let method = match *self {
            Variant::Small => &digest::SHA256,
            Variant::Big => &digest::SHA512,
        };
        digest::digest(method, data).as_ref().to_vec()
    }

    fn blocksize(&self) -> usize {
        match *self {
            Variant::Small => 64,
            Variant::Big => 128,
        }
    }

    fn pad_key<'a>(&self, k: &'a [u8]) -> Cow<'a, [u8]> {
        let mut k = Cow::from(k);

        // If key k is bigger than blocksize, it should be hashed...
        if k.len() > self.blocksize() {
            k = self.hash(&k).into();
        }

        // ... and then padded with zeroes to fit the blocksize
        if k.len() < self.blocksize() {
            let mut resized_key = k.into_owned();
            resized_key.resize(self.blocksize(), 0x00);
            k = resized_key.into();
        }

        k
    }

    /// Returns an HMAC from message m and key k
    pub fn hmac(&self, key: &[u8], message: &[u8]) -> Vec<u8> {
        let key = self.pad_key(key);

        let make_padded_key = |byte: u8| {
            let mut pad = key.to_vec();
            for i in &mut pad { *i ^= byte };
            pad
        };

        let mut inner_pad = make_padded_key(0x36);
        let mut outer_pad = make_padded_key(0x5C);

        inner_pad.extend_from_slice(message);
        let inner_pad = self.hash(&inner_pad);

        outer_pad.extend_from_slice(&inner_pad);
        self.hash(&outer_pad)
    }
}

fn main() {}


Working with images in multiple formats I've been doing some research around how to best make use of the WebP images (with fallback) in React. Most solutions point to something similar to what I've written below. That means if I have a lot of images I have to export the same image twice in assets.js (both PNG and WebP), import twice and provide two Image props for the same image.
This feels very inefficient. Is this an uncommon way of working with images? Is there a better way for working with images that live locally?
assets.js - where I collect all my assets and export them as strings. Imagine this list being really lengthy.
export { default as image1 } from "./assets/image-1.png";
export { default as image1Webp } from "./assets/image-1.webp";

In the file I import the assets I need for that page
import {
    image1,
    image1Webp
} from "./assets";

The component + props
<Image
  src={image1Webp}
  fallback={image1}
  alt="Lorem"
/>

Image.js component
const Image = ({
  src,
  fallback,
  type = 'image/webp',
  ...delegated
}) => {
  return (
    <picture>
      <source srcSet={src} type={type} />
      <img src={fallback} {...delegated} />
    </picture>
  );
};

Some things I've tried:
Generally the recommended way is to add images to a React project is to use require(img/path). So I tried things like appending .png or .webp to the base URL, inside the Image component. so I only need to define one string and then the component handles the splitting.
e.g. pseudocode:
export const toWEBP = (src) => {
  const location = src.split(".")
  return `${location}.webp`
}

It would be great if the code could be simplified using dynamic imports.  I haven't tried this myself but if the image names could be put into an array then something like the following might work, where the exported images are set as properties on an object that can be exported. When this SO answer to Create a loop to import files dynamically in React was written only Chrome and Safari supported that feature but since then it appears that Edge and Firefox now offer support as well.
const images = {};
const nums = [1, 2];
for (const format of  ['png', 'webp']) {
  for (const num of nums) {
    const key = `image${num}` + format === 'webp' ? 'webp' : '';
    images[key] =  await import( `./assets/image-${num}.${format}`);
  }
}

But according to the Hacks blog post by Jason Orendorff ES6 In Depth: Modules

For a dynamic language, JavaScript has gotten itself a surprisingly static module system.

All flavors of import and export are allowed only at toplevel in a module. There are no conditional imports or exports, and you can’t use import in function scope.
All exported identifiers must be explicitly exported by name in the source code. You can’t programmatically loop through an array and export a bunch of names in a data-driven way.

So the function code above could be exported in an async function:
export async function getImages() {
    const images = {};
    //...set keys on images
    return images;
}

Then that function can be imported:
import { getImages } from './assets.js';

But to use that function it would need to be run in an async function:
(async () => {
  const images = await getImages();
  //use images
})();

Other approaches are listed in answers to this similar post on freecodecamp.org - e.g. from this answer by Dan Couper:

One pretty simple solution:

// images.js
const images = [
  { id: 1, src: './assets/image01.jpg', title: 'foo', description: 'bar' },
  { id: 2, src: './assets/image02.jpg', title: 'foo', description: 'bar' },
  { id: 3, src: './assets/image03.jpg', title: 'foo', description: 'bar' },
  { id: 4, src: './assets/image04.jpg', title: 'foo', description: 'bar' },
  { id: 5, src: './assets/image05.jpg', title: 'foo', description: 'bar' },
  ...etc
];
export default images;

// MyComponent.js
import images from './images'
//...snip
{ images.map(({id, src, title, description}) => <img key={id} src={src} title={title} alt={description} />)

You’re basically writing a database of images that match the ones in assets.
Then you can be clever in images.js to pull the correct prefix based on the environment if you want. There will be a WebPack plugin that kinda does this as well if you have a google around (you want it to take a directory of files and generate an iterable set of paths to them).
The import as described is more for importing one-off images (logos etc) rather than importing lots of images from a directory.

Regarding the pseudocode at the end of the post:

export const toWEBP = (src) => {
  const location = src.split(".")
  return `${location}.webp`
}

if src contained an extension (e.g. png) then webp would be appended to that existing extension. One option would be to use pop to remove the existing extension and also push webp onto the array before using it in the return value.


Using UnitOfWork/Repository pattern in static class/methods I know using a UnitOfWork/Repository pattern in a normal class should work fine without any issues but how about using them in a static class/methods (I mean using a private static IUnitOfWork instance, refer CustomerService class). Will this work fine in multiple user scenario without overlapping with other request (I am using asp.net)?

Can anyone please tell me whether the below code looks fine or I will get any issues (overlapping db issues) going forward because of the static IUnitOfWork instance? Also, please advice me if there is any other better way to do this.

public interface IDataRepository<T> where T : class
{
    IQueryable<T> Find(Expression<Func<T, bool>> predicate);
    IQueryable<T> GetAll();
    T GetById(Guid id);
    T GetById(int id);
    void Add(T entity);
    void Update(T entity);
    void Remove(T entity);
}

public class DataRepository<T> : IDataRepository<T> where T : class
{

}

public interface IUnitOfWork
{
    IDataRepository<Customer> CustomerRepository { get; }
    IDataRepository<Product> ProductRepository { get; }

    void SaveChanges();
}

public class UnitOfWork : IUnitOfWork
{
    private readonly SomeContext _context;

    private IDataRepository<Customer> _customerRepository { get; set; }
    private IDataRepository<Product> _productRepository { get; set; }

    public UnitOfWork()
    {
        _context = new SomeContext();
    }

    public IDataRepository<Customer> CustomerRepository
    {
        get
        {
            if (_customerRepository == null)
            {
                _customerRepository = new DataRepository<Customer>(_context);
            }
            return _customerRepository;
        }
    }

    public IDataRepository<Product> CustomerRepository
    {
        get
        {
            if (_productRepository  == null)
            {
                _productRepository = new DataRepository<Product>(_context);
            }
            return _productRepository;
        }
    }
}

public class CustomerService
{
    private static IUnitOfWork _unitOfWork = new UnitOfWork();

    public static List<Customer> GetCustomers()
    {
        _unitOfWork.CustomerRepository.Find(t => t.ID == 123).ToList();
    }

    public static bool IsCustomerExist()
    {
        return (_unitOfWork.CustomerRepository.Find(t => t.ID == 123).Count() > 0);
    }

    public static bool UpdateCustomer(Customer customer)
    {
        .....
        _unitOfWork.SaveChanges();
    }
}

It entirely depends on your implementation of the unit of works members. If the IDataRepository is using something like NHibernate and keeping open persistent connections (which, being part of a unit of work, I presume it might) you will absolutely run into weird database connectivity issues when using a private static member.

The reason for this, is that the static property is shared between all instances of the CustomerService, in your example, so you'll have one of those, and it'll be constructed with "whatever it gets at the time" - for example, the first database connection or session your application opens.  If you have some sort of session per request handling that subsequently disposes of that session, the static class will still hold a reference to it, and you'll have a bad time.

To deal with this problem in some things that "must be" static, there's a cheap little trick you can use - add a static func to some form of resolver or container that you wire up at bootstrapping time:

public class ContainerRegistrationCode
{
    public void RegisterTypesInContainer()
    {
        Kernel.Bind<IUnitOfWork>().To<UnitOfWork>().InRequestScope();
        CustomerService.GetCurrentUnitOfWork = () => Kernel.GetService<IUnitOfWork>();
    }
}

public class CustomerService
{
    public static Func<IUnitOfWork> GetCurrentUnitOfWork { get; set; }

    public static List<Customer> GetCustomers()
    {
        var currentUnitOfWork = GetCurrentUnitOfWork ();
        currentUnitOfWork.CustomerRepository.Find(t => t.ID == 123).ToList();
    }
}

This way, you delegate out to a container or something that can manage the lifecycle of the unit of work, and every time you use it in a static context, you call the func which gets whatever the current one is.

It's a bit of a hack, don't use statics, use a good container instead that supports scoping (like Ninject) and you just shouldn't have this problem.


HackerRank Equal with Scala For studying Scala, I wrote some code to solve the Equal problem on HackerRank. But I am stuck on test case #15 due to timeout.

  Christy is interning at HackerRank. One day she has to distribute some
  chocolates to her colleagues. She is biased towards her friends and
  may have distributed the chocolates unequally. One of the program
  managers gets to know this and orders Christy to make sure everyone
  gets equal number of chocolates.

  But to make things difficult for the intern, she is ordered to
  equalize the number of chocolates for every colleague in the following
  manner,

  For every operation, she can choose one of her colleagues and can do
  one of the three things.

  She can give one chocolate to every colleague other than chosen one.
  She can give two chocolates to every colleague other than chosen one.
  She can give five chocolates to every colleague other than chosen one.

  Calculate minimum number of such operations needed to ensure that
  every colleague has the same number of chocolates.

My code is like below I will appreciate your advice to improve performance.

object EqualsTest {
  def main(args: Array[String]): Unit = {
    val numOfTestCases = readInt();
    Range(0, numOfTestCases).foreach(i => {
      val numberCount = readInt();
      var numbers = Array.ofDim[Int](numberCount)
      val inputNumbers = scala.io.StdIn.readLine().split("""\s+""")
      0.until(numberCount).foreach(i => {
        numbers(i) = inputNumbers(i).trim.toInt
      })
      // val numbers = scala.io.StdIn.readLine().split(" ").map(s => s.toInt);
      var baseLine = numbers.min
      println(calOpCount(numbers, baseLine))
    })

    def f(b: Int)(s: Int): Int = {
      val i = s - b
      if (i==0) 0
      else if (i == 1 || i == 2 || i == 5) 1
      else (i/5 + (i%5)/2 + ((i%5)%2))
    }

    def calOpCount(source: Array[Int], baseLine: Int): Int = {
       val baseLines = 0.to(baseLine)
       val result = for {i <- baseLines; f1 = f(i)(_)} yield (source.map(s => f1(s.toInt)).sum)
       result.min
    }
  }
}

The performance issue and the bug

The program essentially looks for the minimum operations to reduce the numbers to some base line.
There are two problems with the choice of base lines.

The performance issue is that you try base lines from 0 until the minimum value.
Consider this input:

999

The program will check base lines from 0 to 999, in vain.
You need up check only the 3 baselines: min, min - 1 and min - 2.

The second problem is a bug, and it is a related issue.
Consider this input:

0 4 4

The program will check only one base line, 0.
And that will give the incorrect result 4.
Checking -1 would give the correct result 3.

Working with user input

The readInt method is deprecated in favor of StdIn.readInt.

The way you parsed the input is unnecessarily complicated.
This line is commented out,
even though it was really on the right track:

val numbers = scala.io.StdIn.readLine().split(" ").map(s => s.toInt);

Scala style

Some of the var could have been val or unnecessary altogether,
and semicolons at end of lines are not recommended.
Also, you used Range(0, numOfTestCases) at one point,
even though you used the more idiomatic x.until(y) syntax elsewhere.

Alternative implementation

Putting the above tips together:

import scala.io.StdIn

object Solution {

  def main(args: Array[String]): Unit = {
    val numOfTestCases = StdIn.readInt()
    0.until(numOfTestCases).foreach(_ => {
      StdIn.readInt()
      val numbers = StdIn.readLine().split(" ").map(s => s.toInt)
      println(calOpCount(numbers))
    })
  }

  def steps(i: Int): Int = {
    if (i == 0) 0
    else if (i == 1 || i == 2 || i == 5) 1
    else i / 5 + (i % 5) / 2 + ((i % 5) % 2)
  }

  def calOpCount(source: Array[Int]): Int = {
    val min = source.min
    (for {base <- 0.to(2)} yield source.map(s => steps(s - min + base)).sum).min
  }
}


Key generating program in Python I am about two months into programming with Python (no previous experience) and I tried to write a program which would generate a key and assign it to a student.

import random
import string

code_list = []
students = ["Aljoša", "Blažka", "Nana", "Kaja", "Alja", "Tjaša", "Ana",
           "Gal", "Danijela", "Alma", "Neja", "Žiga K.", "Patricija", "Aja",
           "Kristjan",  "Urban",  "Janja", "Lea", "Žana", "Aljaž", "Tilen",
           "Matic", "Marija", "Žiga T."]

#It generates the code and puts it in a list
size = 6
code = string.ascii_uppercase + string.digits
for i in range(24):
    code_list.append(str(''.join(random.choice(code) for _ in range(size))))

#Combines the two lists (students and codes) into one
students_plus_codes = [x + str(" - " + y) for y in students for x in
                      code_list]

#It prints only every 25th element from the combined list
new = [x for i, x in enumerate(students_plus_codes) if i % 25 == 0]

for i in new:
    print(i)`

My problem is that this code is not very elegant and just want to ask how I could write it more efficiently (especially the last part where I have to manually print out every 25th element of a list because every key gets assigned to every student).

The reason that you are getting so many more entries is because of this list comprehension:

 students_plus_codes = [x + str(" - " + y) for y in students for x in code_list]

It's a nested for loop. If we expand it we can see what is happening:

students_plus_codes = []
for y in students:  # students has 24 items
    for x in code_list:  # code_list has 24 items
        students_plus_codes.append(x + str(" - " + y))

So for every iteration of students you do 24 iterations of code_list.

Fortunately, python has a builtin function called zip that will merge lists for us. So we can replace the nested list comprehension with this line:

students_plus_codes = list(zip(students, code_list))

Which results in a list of tuples:

[('Aljoša', '256D2B'), ('Blažka', 'OEGJL9'), ('Nana', 'GB1PJL'), ('Kaja', 'F0P0F2'),
 ('Alja', '62KU94'), ... ('Matic', 'E7CJIP'), ('Marija', '1D2UCL'), ('Žiga T.', '6X1DD5')]

Generating the codes:

First, I would replace for i in range(24): with for i in range(len(students)): as this allows you to change the number of students without having to change other aspects of your code. In this instance 24 is a magic number that could cause issues down the line.

You could even create a function that generates each code, then you can use an easy to read list comprehension.

# <=python3.5
def generate_code(n, characters):
    return ''.join([random.choice(characters) for _ in range(n)])

# >=python3.6
def create_code(n, characters):
    return ''.join(random.choices(characters, k=n))

Printing the codes:

It isn't ideal to join the names and codes as a single string as this makes it hard to use them separately later. For this reason, I used a list of tuples to match them as pairs. To print them the for loop has two variables, student and code that takes advantage of unpacking and then prints them using the format function.

for student, code in students_plus_codes:
    print('{} - {}'.format(code, student))

Altogether:

import random
import string

def create_code(n, characters):
    return ''.join(random.choices(characters, k=n))

students = ["Aljoša", "Blažka", "Nana", "Kaja", "Alja", "Tjaša", "Ana",
           "Gal", "Danijela", "Alma", "Neja", "Žiga K.", "Patricija", "Aja",
           "Kristjan",  "Urban",  "Janja", "Lea", "Žana", "Aljaž", "Tilen",
           "Matic", "Marija", "Žiga T."]

# It generates the code and puts it in a list
key_size = 6
chars = string.ascii_uppercase + string.digits
code_list = [create_code(key_size, chars) for _ in range(len(students))]

# Combines the two lists (students and codes) into a list of tuples
students_plus_codes = list(zip(students, code_list))

for student, code in students_plus_codes:
    print('{} - {}'.format(code, student))


Find best candidates in efficient way, Django I'm beginner in django and python. I have models :
 class Employee(models.Model):
    full_name = models.CharField(max_length = 64)
    title = models.CharField(max_length = 64)

    def __str__(self):
        return f"{self.full_name} ( {self.title} )"

class Skill(models.Model):
    name = models.CharField(max_length = 64)

    def __str__(self):
        return f"{self.name}"

class Candidate(models.Model):
    employee = models.ForeignKey(Employee, on_delete=models.CASCADE, related_name="employee")
    skill = models.ForeignKey(Skill, on_delete=models.CASCADE, related_name="skill")

    def __str__(self):
        return f"{self.id}: {self.employee} knows - {self.skill}"

class Job(models.Model):
    title = models.CharField(max_length = 64)
    skills = models.ManyToManyField(Skill, blank=True, related_name="Jobs")

    def __str__(self):
        return f"{self.title}"

In views.py, i have 'finder' function :
def finder(job_id):
    job = Job.objects.get(id=job_id) # get the specific job
    relevant_candidates = [] # all the relevant candidates of this kob
    common = [] # number of common skills between the employee_skill and the
    relevant_employees_by_title = Employee.objects.filter(title = job.title) # first filter the candidates by the job title
    job_skills = []
    for skill in job.skills.all():
        print(skill.id)
        job_skills.append(skill.id)

    for employee in relevant_employees_by_title:
        employee_skills =[]
        candidateCorrect = Candidate.objects.filter(employee__id = employee.id).values_list('skill', flat=True)
        for skill in candidateCorrect:
            employee_skills.append(skill)

        common_skills = list(set(job_skills) & set(employee_skills))

        if (len(common_skills)>0): #if there are common skills
            relevant_candidates.append(employee)
            common.append(len(common_skills))

    candidates = zip(relevant_candidates,common)
    candidates = sorted(candidates,key = lambda t: t[1], reverse = True) # sort the candidates by the number of common skiils , descending order
    candidates = candidates[:50] # Select the best 50 candidates

    return candidates

This function get the job_id and need to find the best candidates for this job : first by matching between the job title to the employee title (for ex' : software developer), and then matching between candidate's skills to job's required skiils .
I think that my function is inefficient. Someone has any idea how to write it in efficient way?

You could use comprehensions to greater effect (here: directly build job_skills using a set comprehension) and also let got of having to put everything into a list.
You could also store your suitable candidates in a collections.Counter, to get the most_common(n) method for free.
from collections import Counter

def finder(job_id, n=50):
    job_skills = {skill.id for skill in Job.objects.get(id=job_id).skills.all()}

    candidates = Counter()
    for employee in Employee.objects.filter(title=job.title):
        employee_skills = Candidate.objects\
                                   .filter(employee__id=employee.id)\
                                   .values_list('skill', flat=True)
        common_skills = len(job_skills.intersection(employee_skills))
        if common_skills:
            candidates[employee] = common_skills
    return candidates.most_common(n)

I also followed Python's official style-guide, PEP8, which recommends not surrounding = with spaces when using it for keyword arguments, used the fact that the number 0 is falsey, while all other numbers are truthy and made the number of candidates to return configurable (with 50 the default value).

Note that this task would have been slightly easier, if the Employee object already had the skill attached, or at least a link to the Candidate object.


C code displaying random datasets I have much more experience with PAWN scripting language, but I'm digging into K&R book (I think it is it), and trying some stuff on my own.

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

#define ITEMS_COUNT 16

unsigned int randr(unsigned int min, unsigned int max);
inline short randomType();
inline char* randomCompanyName();
inline char* randomPersonName();
inline char* randomPersonSurname();
void populateItems();
void dumpItems();

typedef unsigned short int DBID;

typedef struct {
    char name[64];
    char surname[64];
} Person;

typedef struct {
    char name[128];
} Company;

typedef struct {
    unsigned short int type;
    DBID dbID;
    union {
        Person PersonItem;
        Company CompanyItem;
    } chosenItem;
} Item;

enum E_TYPE {
    TYPE_PERSON,
    TYPE_COMPANY
};

Item
    itemList[ITEMS_COUNT];

int main(int argc, char* argv[]) {
    /* Init stuff */
    srand(time(NULL));

    populateItems();
    dumpItems();
    return 0;
}

void populateItems() {
    Item
        singleItem;
    size_t
        i = 0;

    for(; i != ITEMS_COUNT; ++i) {
        singleItem.dbID =   i + 1;
        singleItem.type = randomType();

        if(TYPE_PERSON == singleItem.type) {
            strncpy(singleItem.chosenItem.PersonItem.name, randomPersonName(), sizeof(singleItem.chosenItem.PersonItem.name));
            strncpy(singleItem.chosenItem.PersonItem.surname, randomPersonSurname(), sizeof(singleItem.chosenItem.PersonItem.surname));
        } else {
            strncpy(singleItem.chosenItem.CompanyItem.name, randomCompanyName(), sizeof(singleItem.chosenItem.CompanyItem.name));
        }
        memcpy(&itemList[i], &singleItem, sizeof(singleItem));
    }
}

void dumpItems() {
    Item
        *currentItem;
    size_t
        i = 0;
    char
        str[256];

    for(; i != ITEMS_COUNT; ++i) {
        currentItem = &itemList[i];

        //Not safe sprintf!
        sprintf(str, "Row %d: { dbID: %d,", i + 1, currentItem->dbID);

        if(TYPE_PERSON == currentItem->type) {
            sprintf(str, "%s name: %s, surname: %s }", str, currentItem->chosenItem.PersonItem.name, currentItem->chosenItem.PersonItem.surname);
        } else {
            sprintf(str, "%s company name: %s }", str, currentItem->chosenItem.CompanyItem.name);
        }

        printf("%s\n", str);
    }
}

inline short randomType() {
    return randr(0, 1);
}

inline char* randomCompanyName() {
    static char*
        nameList[32] = {
        "Microsoft",
        "Valve",
        "Vamonos pest",
        "Hello folks!",
        "Just a program inc"
    };
    return nameList[randr(0, 4)];
}

inline char* randomPersonName() {
    static char*
        nameList[32] = {
        "John",
        "Mike",
        "Emily",
        "Jessica",
        "Steven"
    };
    return nameList[randr(0, 4)];
}

inline char* randomPersonSurname() {
    static char*
        nameList[32]  = {
        "Ross",
        "Litt",
        "Hellyeah"
    };
    return nameList[randr(0, 2)];
}

unsigned int randr(unsigned int min, unsigned int max)
{
    double
        scaled = (double) rand() / RAND_MAX;

    return (max - min + 1) * scaled + min;
}

I'd like you to tell me if I've made any major mistakes, and how's my coding style. Additionally I have a question about unions. I thought that it simply pointed to regions, and that you didn't have to care later which one you are using, like:

//instead of
singleItem.chosenItem.PersonItem.name
//use
singleItem.chosenItem.name

which would point at either PersonItem.name, or CompanyItem.name. How to achieve somthing similar?

My little bit of input:

inline is a C++ keyword, and it's only a hint to the compiler. The only reason to explicitly use inline is when you define function's body inside a header file, which is included in more than one source file.
Avoid basic data types like int, use types from stdint.h instead
Use comments to explain why are you doing something, and what are function arguments for
No reason to break a line after declaring a type.
No reason to give nameList[32] a size - the compiler will guess it on its own. Just do it as

static char* nameList[]  = {
    "Ross",
    "Litt",
    "Hellyeah"
};

Use sizeof(nameList) - 1 instead of a magic number to indicate last array index - there will be less changes if you ever add more names.

Regarding your union questions:

union is kinda like a struct; the only difference is how it's handled in memory.
Since your union has two fields (PersonItem and CompanyItem), you must say which one is to be used. Every object created with your struct will contain both fields. What you're trying to do is called polymorphism, and you have it with classes in C++. Original C does not have such option.


Structure of website columns and navigation I have made this HTML5 structure for a new website I am working on. I'd like to know if there is a more elegant approach to doing it.

Is the wrapper for the two column considered bad practice?
Should the secondary navigation on the left be an article or a div?

header

H1 title

Lorem ipsum dolor sit amen lorem ipsum dolor sit amen

- main navigation

- main navigation

- main navigation

button call to action

Title h2 secondary menu

navigation

navigation

navigation

Title h2 secondary menu

navigation

navigation

navigation

slideshow widget

review widget

product grid title

Text text text lorem ipsum dolor sit amen

product 1

product 2

product 3

<

product grid title

Text text text lorem ipsum dolor sit amen

product 1

product 2

product 3

/div>

footer

Don't worry about wrapping your content too much if the style cannot be achieved without it. If it can, the extra wrap may be overkill.
Your markup relies a lot on the <article> where it may not be necessary. For instance, you put one <article> tag into another, which is odd. You also put and <article> tag into the <header>, which is also odd.

If you want simple containers, I would recommend you stick with <div>s with semantic class attributes. They have been around for a while and used for wrapping other tags. They are the tried and true method and their lack of semantic meaning will not confuse the reader or crawler.


Determine if a string of characters are unique using quicksort This will determine if a string is unique or not using quicksort, so that it should take \$O(nlogn)\$ time and not \$O(n^2)\$ time.

I will call the test function code from main.

This runs successfully, but when I try to write a randomized partition, it will fail. I have written that code separately.

FindUniqueCharInString.h

#define MAX 15
class FindUniqueCharInString
{
public:
    FindUniqueCharInString(void);
    ~FindUniqueCharInString(void);
    void test();

private:
    void swap(int index1, int index2);
    void quickSort(int start, int end);
    int partition(int start, int end);

    char arr[15];
};

FindUniqueCharInString.cpp

#include "FindUniqueCharInString.h"
#include <iostream>

using std::cout;
using std::endl;

FindUniqueCharInString::FindUniqueCharInString(void)
{
    char *tempArr = "SOMESTRING";

    strcpy(arr,tempArr);
}

FindUniqueCharInString::~FindUniqueCharInString(void)
{
}

void FindUniqueCharInString::test()
{
    quickSort(0,strlen(arr));

    for(int i = 0;i<strlen(arr);i++)
    {
        cout<<arr[i] << " ";
    }

    cout<<endl;

    bool isUnique = true;
    for(int i = 0;i<strlen(arr) - 1;i++)
    {
        if(arr[i] == arr[i+1])
            isUnique = false;
    }

    if(isUnique)
        cout<<"Unique" <<endl;
    else
        cout<<"Not unique"<<endl;

}

void FindUniqueCharInString::swap(int index1, int index2)
{
    if(arr[index1] != arr[index2])
    {
        char temp = arr[index1];
        arr[index1] = arr[index2];
        arr[index2] = temp;
    }
}

void FindUniqueCharInString::quickSort(int start, int end)
{
    if(start >= end)
        return;

    int p = partition(start,end);
    quickSort(start, p - 1);
    quickSort(p + 1,end);
}

int FindUniqueCharInString::partition(int start, int end)
{
    int pivot = arr[end - 1];
    int t = start;

    for(int i = start; i< end-1; i++)
    {
        if(arr[i] <= pivot)
        {
            swap(i,t);
            t++;
        }
    }

    swap(t,end - 1); //swap index and pivot pos

    return t;
}

Randomize partition function:

int FindUniqueCharInString::partition(int start, int end)
{
    int m = start + (end - start)/2;
    int pivot = arr[m];
    int t = start;

    swap(m,end -1); //putting pivot element to end

    for(int i = start; i< end-1; i++)
    {
        if(arr[i] <= pivot)
        {
            swap(i,t);
            t++;
        }
    }

    swap(t,end - 1); //swap index and pivot pos

    return t;
}

Complexity

If the intent was to meet an \$O(N log N)\$ requirement, you've technically failed--quicksort has an expected complexity proportional to N log N, but big-O deals strictly with the worst-case complexity--and worst-case complexity of quicksort is \$O(N^2)\$.

Class Design

One of the old basic ideas of how to do object-oriented design is to start by writing down a one-page (or less) description of what the program is supposed to do. You then pick out the nouns that form the major themes in that description. Those are your classes.

The point of that is to note that most classes should normally represent nouns1. Things that exist. Objects. In a typical case they should not represent verbs.

In this case, however, you have one primary class--and its name is a verb, not a noun. This leads to a lot of question over whether it makes sense for this class to be a class, or whether we have kind of a conglomeration of "stuff" thrown together into something that says it's a class, but isn't doesn't form a particularly coherent...thing.

Standard Containers

Good uses for C style arrays in C++ are quite rare. Your arr doesn't strike me as one of them. If you really need a fixed-size array, you should probably use std::array instead. Otherwise, you're probably best off using std::vector.

Standard Algorithms

The standard library already has algorithms to handle the job at hand. Most code should just use st::sort followed by std::unique, rather than attempting to craft the same on your own.

Self-defeating design

Right now, you've limited the collection size to only 15 elements. For such a small size, a simpler sorting algorithm (e.g., insertion sort) will almost always be at least a little faster than a quicksort. In short, combining the elements you have in the way you have just doesn't make sense.

Use of strlen

If you insist on using strlen at all, you shouldn't call it in the condition of a for loop, like:

for (int i=0; i<strlen(foo); i++)

A few compilers know how to optimize this by hoisting the call to strlen out of the loop, but most don't. With those compilers, this loop will have \$O(N^2)\$ complexity instead of the O(N) you probably want/expect.

Note that if you used std::vector or std::string for your storage, this wouldn't be a problem. Unlike strlen, both these guarantee that you can find the size with \$O(1)\$ complexity instead of \$O(N)\$.

Side Effect-ridden code

Most of your functions implicitly modify FindUniqueCharInString::arr. It's generally much better to avoid this when possible--and it's pretty trivial in most of these cases. Just for a trivial example, right now your swap is passed to integers, i and j, and it swaps arr[i] with arr[j]. It would be much better to pass (for example) pointers or references to the contents, so the modifications are explicit:

swap(i, j); // ambiguous, implicit side effects on `arr`

swap(arr[i], arr[j]); // clear, effect on `arr` is obvious.

Summary

At least in my opinion, the other issues pale compared to just making good use of the standard library.

std::string arr {"SOMESTRING"};
std::sort(arr.begin(), arr.end());
auto end = std::unique(arr.begin(), arr.end());

// show the unique characters
std::copy(arr.begin(), end, std::ostream_iterator<char>(std::cout, "\n"));

1. The primary exception to this is function objects--i.e., classes that act mostly like a function, including overloading operator() to do whatever it is they do


Adding a replica of some DOM elements on click I have the following code which inserts a new element of the same type over and over again to the parent element. I am new to JavaScript so was wondering if there was a better way of doing this. I apologise for the variable names, I'll get around to changing those later, just allows me to see the flow better. I'm trying to write this in pure js.

//Add a new lobby element
document.getElementById("addLobby").addEventListener("click", function(e) {
  var layer1 = document.createElement("div");
  layer1.setAttribute("class","col-wd-12 col-md-6 col-sm-4");
  var layer2 = document.createElement("div");
  layer2.setAttribute("class","col lobby");
  var layer3 = document.createElement("div");
  layer3.setAttribute("class","col lobby-infoPanel");
  var layer3v2 = document.createElement("div");
  layer3v2.setAttribute("class","overlay lobby-overlay");
  var layer3v2v1 = document.createElement("a");
  layer3v2v1.setAttribute("href","#");
  var l3v2v1v1 = document.createElement("span");
  l3v2v1v1.setAttribute("class", "overlay-options lobby-overlay-options");
  var layer4 = document.createElement("div");
  layer4.setAttribute("class","col lobby-info-count");
  var layer4v2 = document.createElement("div");
  layer4v2.setAttribute("class","col lobby-info-count lobby-info-users");

  var l3v2v1v1Text = document.createTextNode("+");
  var l4text = document.createTextNode("5/5");
  var l4v2text = document.createTextNode("@User1");

  layer4v2.appendChild(l4v2text);
  layer4.appendChild(l4text);
  layer3.appendChild(layer4);
  layer3.appendChild(layer4v2);
  l3v2v1v1.appendChild(l3v2v1v1Text);
  layer3v2v1.appendChild(l3v2v1v1);
  layer3v2.appendChild(layer3v2v1);
  layer2.appendChild(layer3);
  layer2.appendChild(layer3v2);
  layer1.appendChild(layer2);

  var lastNode = document.getElementById("addLobby").parentElement;
  lastNode.parentElement.insertBefore(layer1, lastNode);
});

First suggestion

One first refactoring idea is to create a function that creates an element, sets its attributes and adds it to a parent element.

function createElement(elementType, classes, parent) {
    var e = document.createElement(e);
    e.setAttribute('class', classes);
    parent.appendChild(e);
    return e;
}

You could also separate your structure from your DOM manipulation.
I suggest you create your structure as a JSON, and then create the nodes recursively.

For example, you could define your structure as:

{
  element: 'div',
  class: 'col-wd-12 col-md-6 col-sm-4',
  children: [{
    element: 'div',
    class: 'col loby',
    children: [
      //...
    ]
  },
  // ...
}

and then:

function createDOMElements(data) {
  // ...

  // create the element
  var elem = document.createElement(data.element);

  // add its attributes
  for (var attribute in data) {
    if (data.hasOwnProperty(attribute) && attribute !== 'element' && attribute !== 'children') {
      var attributeValue = data[attribute];
      elem.setAttribute(attribute, attributeValue);
    }
  }
  // ...

  // add the children (forEach is from ES5 on)
  data.children.forEach(function(child) {
    // recursive call to create child elements
    var childElement = createDOMElements(child);
    elem.appendChild(childElement);
  });

  return elem;
}

Here is a jsfiddle with the whole working example.

Second suggestion (less JS)

You could create a hidden HTML 'template' and display it when needed.

You can set:

visibility to hidden if you still want to affect the layout (take the space needed)
or display to none

in css (more info here) directly in the style attribute or through a class. On the click event you make this element visible.

The HTML:

  <div id="template" class="col-wd-12 col-md-6 col-sm-4" style="display:none;">
      <!-- all the children -->
  </div>

And then in the JS:

document.getElementById("addLobby").addEventListener("click", function(e) {
  document.getElementById("template").style.display = "initial";
});

Here is the second fiddle.

If you need to add the template multiple times, or if you have multiple templates on your page, you should define them at the end on the page.
When needed, clone the template and append it to the desired parent.

Conclusion

If you don't have a particular need to create the elements in the JS, I think the second solution it's more elegant (and less code ;) )


WCF general response validation for multiple services Background information

I am building a web application that consumes 3 different WCF services that are hosted internally.

Each response object from the service inherits from a base class ResponseBase. This class contains properties int ErrorCode and string ErrorMessage.

When I am consuming these services in my web application I want to implement some general error handling that verifies if the response object contains an errorcode < 0.

What I've come up with is a static class ServiceHelper with several overloads of the following method:

public static void VerifyResponse(dynamic response)
{
    if (response == null) throw new ArgumentNullException("response");

    if (response.ErrorCode < 0)
        throw new InvalidServiceResponseException(response.ErrorCode, response.ErrorMessage);
}

Now as you can see the type of the parameter is dynamic. The reason for this, and this is basically why I'm asking this question, is that I don't have a general base object to define all response objects. Because each WCF service will creat its own contracts there can't be a shared base object between services. Well... it can be done but we prefer to keep it simple for now.

Question

Do you of some magic way to validate 3 different types of response objects but with a properly typed parameter and without creating a different method for each service. Because each service has a different ResponseBase type even though it is actually the same.

Or do you think it is permitted to use a dynamic parameter in this context?

Peter - a couple of observations:

Since you say "Each response object from the service inherits from a base class ResponseBase" the gut reaction is to just use ResponseBase and let the inheritance model in C# handle everything. As you go on to explain, however, ResponseBase is not really the same base class across services, as they vary from service to service with generated code. In this case, you know exactly what the shape of the object is, but don't know its type. This is an example of 'duck typing' - if it looks like a duck, walks like a duck, and quacks like a duck, then we might as well treat it like a duck. This is exactly what dynamic typing is good for and seems to me an excellent use of this feature of C# 4.0.
The two exception conditions in your code are not created equal. The first seems to flag an incorrect use of the method, while the second is the method doing its job. So consider replacing the first one with a Code Contract if you are on .NET 4. (See Dino Esposito's articles in MSDN Magazine - 3 months in a row - for a good overview, starting here. Note especially that Code Contracts can be turned off for runtime, and they may be off already in your project. See Dino's articles for an explanation.)

Here is pseudocode:

public static void VerifyResponse(dynamic response)
{
   Contract.Requires(response != null);
   // or perhaps: Contract.Requires<ArgumentNullException>(response != null, "response");
   // or: if (response == null) throw new ArgumentNullException("response");

   if (response.ErrorCode < 0)
      throw new InvalidServiceResponseException(response.ErrorCode, response.ErrorMessage);
}


Follow up Implementation on Monty Hall Simulation I have taken your inputs on my original question and formed a new implementation of the Monty Hall Simulation:

Again keep an eye on CleanCode and OOP

The Price to be won:

enum Price {
    GOAT, CAR
}

The Door keeping that price in secret

class Door {

    private final Price price;

    Door(Price price){
        this.price = price;
    }

    Price getPrice() {
        return price;
    }
}

the Player who participates in an Monty Hall Game

class Player {

    private final boolean preferresChange;
    private int winCounter;

    Player(boolean preferresChange){
        this.preferresChange = preferresChange;
    }

    boolean preferresChange(){
        return preferresChange;
    }

    void increaseWinCounter() {
        winCounter = winCounter + 1;
    }

    int getWinCounter(){
        return winCounter;
    }
}

the MontyHall in which the player takes action

class MontyHall {

    private final List<Door> doors;
    private Door selected;
    private Door openOne;
    private final Random random;
    private final int amountOfDoors;

    MontyHall(int seed, int amount) {
        random = new Random(seed);
        amountOfDoors = amount;
        doors = IntStream.range(0, amountOfDoors).
            mapToObj(e -> new Door(Price.GOAT)).
            collect(Collectors.toList());
        doors.set(random.nextInt(amountOfDoors), new Door(Price.CAR));
    }

    void chooseDoor(){
        selected = doors.get(random.nextInt(amountOfDoors));
    }

    void openDoor(){
        openOne = doors.stream().
            filter(d -> !d.equals(selected) && Price.GOAT == d.getPrice()).
            collect(Collectors.toList()).
            get(0);
    }

    void changeDoor(){
        selected = doors.stream().
            filter(d -> !d.equals(openOne) && !d.equals(selected)).
            collect(Collectors.toList()).
            get(0);
    }

    Price getPrice(){
        return selected.getPrice();
    }

}

and the simulator who hosts the games

class Simulator {

    public static void main(String[] args) {
        Simulator simulator = new Simulator();
        simulator.simulate(10000, 3);
    }

    private void simulate(int iterations, int amountDoors) {
        Player changer = new Player(true);
        Player stayer = new Player(false);

        Random random = new Random();
        for (int i = 0; i < iterations; i ++){
            int seed = random.nextInt();
            playOneRound(changer, seed, amountDoors);
            playOneRound(stayer, seed, amountDoors);
        }
        System.out.println("changer: "+changer.getWinCounter());
        System.out.println("stayer : "+stayer.getWinCounter());
    }

    private void playOneRound(Player player, int seed, int amountDoors) {
        playOneRound(player, new MontyHall(seed, amountDoors));
    }

    private void playOneRound(Player player, MontyHall montyHall) {
        montyHall.chooseDoor();
        montyHall.openDoor();
        if (player.preferresChange()){
            montyHall.changeDoor();
        }
        if (Price.CAR == montyHall.getPrice()){
            player.increaseWinCounter();
        }
    }

}

Positive Feedback

I liked how you follow the rules of Clean Code, that you write small methods and small classes and give all readable names!

You've introduced your own data types that make the code more readable. The MontyHall-Algorithm can now be read like a poem:

montyHall.chooseDoor();
montyHall.openDoor();
if (player.preferresChange()){
   montyHall.changeDoor();
}
if (Price.CAR == montyHall.getPrice()){
   player.increaseWinCounter();
}

And all your methods have only one intention level what makes them so easy to understand.

Some Criticism

Class Names Refer to Unexpected Behavior

I read on Wikipedia:

  You [the player] pick a door, say No. 1, and the host, who knows what's behind the doors, opens another door, say No. 3

After I read your code I was a little bit confused at some places. But not because of the quality of your code, which is for me very high, but because of the quote above.

In your game the Player can't pick a Door. Instead the Player gets picked a door by MontyHall: (montyHall.chooseDoor()).

Additional to that the Player has a winCounter, but actually the Simulation should track how often a Player wins the MontyHall.

No Outcome

What would you expect your main to do without any knowledge about our code base?

public static void main(String[] args) {
    Simulator simulator = new Simulator();
    simulator.simulate(10000, 3);
}

It could run something like a game loop or start a GUI to interact with but instead it writes data to the console.

Much cleaner would be:

public static void main(String[] args) {
    Simulator simulator = new Simulator();
    simulator.printSimulations(10000, 3);
}

But now imagine you want to compare a "Changer" and a "Stayer". You can't do it because you have no outcome! You through your outcome away by printing it to the console instead of storing and returning it.

public static void main(String[] args) {
    Simulator simulator = new Simulator();
    SimulationResult result = simulator.simulate(10000, 3);
    int average = result.calculateAverageOfStayer();
    System.out.println(average);
}

Feature Envy

  A method accesses the data of another object more than its own data.

The method playOneRound in playOneRound is in my eyes a feature envy. All the tasks should be done by MontyHall. So the method playOneRound should look like:

private void playOneRound(Player player, MontyHall montyHall) {
    montyHall.playGameWith(player);
}

Use Methods instead of Getters and Operations

In your code I can find code structure like x.getValue == something, but much better to read would be x.isSomething()

Some inspiration for you:

Price.GOAT == d.getPrice() means door.isHidingGoat()

Price.CAR == montyHall.getPrice() means montyHall.isWon()

!d.equals(selected) means door.notEquals(selected)

In your older post @Simon Forsberg mentioned:

  I have a feeling that using a class for StraightPlayer, ChangePlayer and ShowMaster will be a bit overkill.

I know you still have the flag argument in Player. But that's fine. In general, refactoring can be an overkill at all. And you can see that for this little problem you can divide the logic into so many different classes and methods. When you write code, you have to find a balance between all OOP/clean code and your task.

Thanks for this cool problem and that you let me review it. I learned a lot while read your code :]


Function to streamline PDO statements This function arguably has little use as it's so simple, but I'm writing a web application with a lot of SQL statements littered all over the place and I thought this function might reduce code duplication and improve readability. I'm wondering if what I'm doing is a good idea, what the "best practice" is here, and any suggestions to make the function more robust and secure. It seems like attempts to further facilitate querying with PDO often trades efficiency or security for ease of use, which I'm trying to avoid.

<?php
// Configs
define("HOST", "localhost");
define("USER", "username");
define("PASS", "password");
define("NAME", "database");

// Connect to database
try {
  $db = new PDO("mysql:host=".HOST.";dbname=".NAME.";charset=utf8", "".USER."", "".PASS."");
  $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
  // PDO fetch docs: http://php.net/manual/en/pdostatement.fetch.php
  $db->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_ASSOC);
} catch (PDOException $e) {
  // User friendly page to show on connection error
  include 'includes/sql-error.php';
  die();
}

// Simple function to handle PDO prepared statements
function sql($db, $q, $params, $return) {
  $stmt = $db->prepare($q);
  $stmt->execute($params);
  if ($return == "rows") {
    return $stmt->fetchAll();
  }
  elseif ($return == "count") {
    return $stmt->rowCount();
  }
}

?>

The usage of this function would look something like this

<?php
// With SELECT
// Call function
$rows = sql($db, "SELECT * FROM table WHERE id = ?", array($id), "rows");
// Get results
foreach($rows as $row) {
    echo $row['field1'].' '.$row['field2']; //etc...
}

// With INSERT
// Call function
sql($db, "INSERT INTO table (field1, field2, field3) VALUES (?, ?, ?)", array($id, $name, $pass));

// With UPDATE
// Call function
sql($db, "UPDATE table SET name = ? WHERE id = ?", array($name, $id));

// With DELETE
// Call function
sql($db, "DELETE FROM table WHERE id = ?", array($id));
?>

I think that you should consider an important drawback of your solution: every query execution will read in main memory all the records that satisfy the query, before processing them. This is due to the fact that the sql function performs a fetchAll.

For instance, the php manual says:

  Using this method to fetch large result sets will result in a heavy demand on system and possibly network resources.

So, if your application only does queries that retrieve small quantities of data, I think your solution is ok, since it improves the readability of the program, but if the program sometimes need large quantities of data, you should avoid the use of fetchAll.


Recursive bubble sort in OCaml I'm new in OCaml and just want to be sure that I write code in "ocaml way". My other first programs in OCaml was imperative and looks like python-code. So I decided to write simple code and share it for review.

let print lst =
    List.map string_of_int lst
        |> String.concat " "
        |> print_endline
;;

let rec sort lst =
    let sorted = match lst with
    | hd1 :: hd2 :: tl ->
        if hd1 > hd2 then
            hd2 :: sort (hd1 :: tl)
        else
            hd1 :: sort (hd2 :: tl)
    | tl -> tl
    in
    if lst = sorted then
        lst
    else
        sort sorted
;;

let () =
    let lst1 = [3; 4; 2; 1] in
    print lst1;
    sort lst1 |> print;

    let lst2 = [5; 6; 7; 0; 1; 4; 2; 9; 3; 8] in
    print lst2;
    sort lst2 |> print;
;;

Result:

3 4 2 1
1 2 3 4
5 6 7 0 1 4 2 9 3 8
0 1 2 3 4 5 6 7 8 9

Your code is easy to understand, so I can only suggest minor changes. The first thing is just a preference of mine: if a function body is nothing but a match ... with expression, I use the function keyword, instead. The second, more important recommendation is to use three clauses in the match body so you don't build a new, temporary list (hd2 :: tl) as you recurse. With both of these minor changes, sort looks like this:

let rec sort lst =
  let sorted = function
  | hd1 :: hd2 :: tl when hd1 > hd2 ->
        hd2 :: sort (hd1 :: tl)
  | hd1 :: tl ->
        hd1 :: sort tl
  | tl -> tl
in
if lst = sorted then
    lst
else
    sort sorted

As the list gets more and more sorted, the second pattern gets matched more often and simply travels down the list. Your version continues to build a new list -- even when the list is sorted.

One other thing: OCaml has an equality operator that tests for physical equality (in addition to structural equality.) So we can avoid building a new list in the second case, if it's not necessary:

| (hd1 :: tl) as lst ->
    let tl' = sort tl in
    if tl' == tl then
        lst
    else
        hd1 :: tl'

Both these changes will prevent a lot of temporary objects from being created. Unfortunately, it's still a bubble sort, so it's never going to be a speed-demon. :D


PHP error handling: log everything, and redirect critical errors to an "oops" page I'm VERY new to error/exception handling, previously just echoing everything to screen, and am trying to really clean up my code.  My goal was to log everything but only show the user a friendly "oops" page if something was seriously broken.  How does what I have below look?

Any best practice advice is appreciated.

<?php
error_reporting(~0);
ini_set('display_errors', 0);
ini_set('log_errors', 1);

/* Set the error handler. */
set_error_handler(function ($errno, $errstr, $errfile, $errline) {
    /* Ignore @-suppressed errors */
    if (!($errno & error_reporting())) return;

    $e = array('type'=>$errno,
               'message'=>$errstr,
               'file'=>$errfile,
               'line'=>$errline);
    redirect($e);
});

/* Set the exception handler. */
set_exception_handler(function ($e) {
    $e = array('type'=>$e->getCode(),
               'message'=>$e->getMessage(),
               'file'=>$e->getFile(),
               'line'=>$e->getLine());
    redirect($e);
});

/* Check if there were any errors on shutdown. */
register_shutdown_function(function () {
    if (!is_null($e = error_get_last())) {
        redirect($e);
    }
});

function redirect($e) {
    $now = date('d-M-Y H:i:s');
    $type = format_error_type($e['type']);
    $message = "[$now] $type: {$e['message']} in {$e['file']} on line {$e['line']}\n";
    $error_log_name = ini_get('error_log');
    error_log($message, 3, $error_log_name);

    switch ($e['type']) {
        /* We'll ignore these errors.  They're only here for reference. */
        case E_WARNING:
        case E_NOTICE:
        case E_CORE_WARNING:
        case E_COMPILE_WARNING:
        case E_USER_WARNING:
        case E_USER_NOTICE:
        case E_STRICT:
        case E_RECOVERABLE_ERROR:
        case E_DEPRECATED:
        case E_USER_DEPRECATED:
        case E_ALL:
            break;
        /* Redirect to "oops" page on the following errors. */
        case 0: /* Exceptions return zero for type */
        case E_ERROR:
        case E_PARSE:
        case E_CORE_ERROR:
        case E_COMPILE_ERROR:
        case E_USER_ERROR:
            return header('Location: /oops_page.htm');
    }
}

function format_error_type($type) {
    switch($type) {
        case 0:
            return 'Uncaught exception';
        case E_ERROR: /* 1 */
            return 'E_ERROR';
        case E_WARNING: /* 2 */
            return 'E_WARNING';
        case E_PARSE: /* 4 */
            return 'E_PARSE';
        case E_NOTICE: /* 8 */
            return 'E_NOTICE';
        case E_CORE_ERROR: /* 16 */
            return 'E_CORE_ERROR';
        case E_CORE_WARNING: /* 32 */
            return 'E_CORE_WARNING';
        case E_CORE_ERROR: /* 64 */
            return 'E_COMPILE_ERROR';
        case E_CORE_WARNING: /* 128 */
            return 'E_COMPILE_WARNING';
        case E_USER_ERROR: /* 256 */
            return 'E_USER_ERROR';
        case E_USER_WARNING: /* 512 */
            return 'E_USER_WARNING';
        case E_USER_NOTICE: /* 1024 */
            return 'E_USER_NOTICE';
        case E_STRICT: /* 2048 */
            return 'E_STRICT';
        case E_RECOVERABLE_ERROR: /* 4096 */
            return 'E_RECOVERABLE_ERROR';
        case E_DEPRECATED: /* 8192 */
            return 'E_DEPRECATED';
        case E_USER_DEPRECATED: /* 16384 */
            return 'E_USER_DEPRECATED';
    }
    return $type;
}
?>

A few things jump out at me:

The redirect of $e['type'] == 0:
For exceptions the default code is 0. Another code could be used and if the exception is not catched, it will throw a fatal error. Your handler does watch for fatal errors, so this may be a non-issue. The exception condition will just be logged twice.
For the actual header redirect:
Some browsers are slow on the header redirect (I'm not sure why). If you allow the rest of your code to continue executing, it could cause an ugly screen flicker. Again, since you are only redirecting fatal errors, probably a non-issue.
The logging functionality:
You may want to consider moving this out into another function. Some day you may decide to log the errors to a database, send emails, or even a text message. You could add these to that redirect method, but that would start to get pretty messy. If each piece were a separate method that was called from a generic log method, that would be easier to read and change as needed.


Fastest / most efficient random bit indexing in Python Using Python/Cython, I am trying to construct an efficient way to index boolean values. I have been trying to use the bitarray and/or bitstring packages to construct a fast, non memory intensive way to hold a lot of booleans. My goal was for a list like object that could be sliced as such, and actually managed the bool values in real bits of data:
>>> from bitarray import bitarray
>>> array = bitarray(1000000) # creates 1000000 allocated bits
>>> array.setall(False)
>>> array[4] # holds boolean values
False
>>> from timeit import timeit
>>> timeit(setup = 'from __main__ import array',
           stmt = 'array[500000]')
0.21002184844218164
# one million iterations, making each iteration 210 ns

>>> from bitstring import BitArray
>>> array = BitArray(1000000) # just as we did in the first example
>>> array[6] # all are False by default
False
>>> from timeit import timeit
>>> timeit(setup = 'from __main__ import array',
           stmt = 'array[500000]')
2.6748261753875795
# oh no, this one is very slow, each iteration takes 2674 ns

As you can see, there are already packages that construct this binary data and allow you to index it at random. Would it be possible to get to an even lower level to the RAM myself? 210ns is fast, but I would like to get indexing below 40ns on a 1000000 bit array. How can this be done?
Edit:
Individual item assignment to the boolean array after it is created, would be nice, but it is not required.

What exactly do you want to do with your array of booleans? If you used numpy, you could instantiate your boolean array as an array of uint8 one eighth the size of the boolean array, e.g.

In [38]: a = np.random.randint(256, size=(1000000//8,)).astype(np.uint8)

To extract items, you could do something like:

In [39]: def get_bools(a, i):
   ....:     b = i // 8
   ....:     o = i % 8
   ....:     mask = 1 << o
   ....:     return (a[b] & mask) != 0
   ....:

If you run this to extract a single item, the speed is appalling:

In [40]: idx = 500000

In [41]: %timeit get_bools(a, idx)
100000 loops, best of 3: 4.53 us per loop

But if you do it for 1000 items at once, things look (much) better:

In [42]: idx = np.random.randint(1000000, size=(1000,))

In [43]: %timeit get_bools(a, idx)
10000 loops, best of 3: 31.1 us per loop

Note that the last timing means ~30 ns per item retrieved, right where you wanted to be.

If you need to extract the boolean values one at a time, you are going to have to deal with the painful Python overhead. Notice that simply getting a single item from an array is terribly slow:

In [44]: %timeit a[62500]
10000000 loops, best of 3: 135 ns per loop

But if you can vectorize your code with numpy, or use a C array directly from cython, then you should have no problem in achieving your speed goal.


TextField Validation for Teleport Feature in Game I've implemented a teleport feature for my game and I would like some feedback on the way that I validate the user input. I am using the libGDX library.

When the player clicks the Teleport Menu button, a menu will pop up that has two TextFields pre-populated with zeros.  The user can click the TextField to begin input, and then can input any string that they want to.  Once the player clicks Teleport, the current Strings of the TextFields are retrieved and handed off to a method that turns them into valid integers.

Here is a screenshot of the Teleport Menu:

Here is the TeleportMenu class, it is not too large:

public class TeleportMenu extends BZMenuTable {

    private final NinePatch menuBackground;
    private final MainGame game;
    private final float stageWidth;
    private final float stageHeight;

    private TextField longitudeTextField;
    private TextField latitudeTextField;

    public TeleportMenu(Skin skin, Stage hudStage, NinePatch menuBackground, MainGame game, float stageWidth, float stageHeight) {
        super(skin, hudStage);
        this.menuBackground = menuBackground;
        this.game = game;
        this.stageWidth = stageWidth;
        this.stageHeight = stageHeight;
    }

    @Override
    public void build() {

        TextButton teleportButton = new TextButton("Teleport", skin);
        teleportButton.addListener(new ClickListener() {
            @Override
            public void clicked(InputEvent event, float x, float y) {
                TeleportMenu.this.activateTeleport();
                TeleportMenu.this.close();
            }
        });

        this.menuTable.setBackground(new NinePatchDrawable(this.menuBackground));
        this.menuTable.setPosition(this.stageWidth/10, this.stageHeight/8);
        this.menuTable.setWidth((float)(this.stageWidth * 0.8));
        this.menuTable.setHeight((float)(this.stageHeight * 0.6));

        this.menuTable.add("Teleport");
        this.menuTable.row();

        this.menuTable.add("Latitude 0 - 50");
        this.menuTable.row();
        this.latitudeTextField = new TextField("0", skin);
        this.menuTable.add(latitudeTextField);
        this.menuTable.row();

        this.menuTable.add("Longitude 0 - 50");
        this.menuTable.row();
        this.longitudeTextField = new TextField("0", skin);
        this.menuTable.add(longitudeTextField);
        this.menuTable.row();

        this.menuTable.add(teleportButton).pad(10).width(this.stageWidth/5).height(this.stageHeight/12).fill();
    }

    private void activateTeleport() {
        int xPos = this.processTextField(this.latitudeTextField.getText());
        int yPos = this.processTextField(this.longitudeTextField.getText());
        this.game.playerActivatedTeleport(xPos, yPos);
    }

    private int processTextField(String string) {
        Integer position;
        try {
            position = Integer.parseInt(string);
        } catch (Exception e) {
            System.out.println("Invalid input");
            position = 0;
        }
        if (position < 0) {
            position = 0;
        }
        if (position >= World.WORLD_WIDTH) {
            position = World.WORLD_WIDTH - 1; //because it is the index of an array
        }
        return position;
    }
}

Specifically I am curious about the processTextField method, but I am always interested in any kind of feedback.

Table organization

this.menuTable.setPosition(this.stageWidth/10, this.stageHeight/8);
this.menuTable.setWidth((float)(this.stageWidth * 0.8));
this.menuTable.setHeight((float)(this.stageHeight * 0.6));

I have mentioned before that I do not like this. This is not the "correct" way to setup a table on your screen. When you solve this issue, you will no longer need to pass around stageWidth and stageHeight.

Guess what the magic number is today?

this.menuTable.add("Latitude 0 - 50");
this.menuTable.add("Longitude 0 - 50");

Extract 50 to a static final int constant and use string concatenation to set the string:

this.menuTable.add("Latitude 0 - " + MAX);

processTextField

The first thing that comes to my mind is: If a user enters invalid input, should they then really be teleported?

If I enter the coordinate -42, 1not2a3number4, I would expect an error message, not being teleported to (0, 0). Also remember that users will not be able to see System.out.println output.

That being said, I'd also make some other changes:

Use int instead of Integer for position variable
Instead of if (position < 0) do position = Math.max(0, position);
Instead of if (position >= MAX) do position = Math.min(MAX, position);

The both Math.max and Math.min uses can be put together into:

position = Math.min(MAX, Math.max(0, position));

However, there is a LibGDX method in MathUtils that already does this:

position = MathUtils.clamp(position, 0, MAX);