Concatenating strings into a fixed-size char array in C Using the standard C library, I want to repeatedly concatenate onto a fixed size string. Despite its promising name, the semantics of strncat do not appear suited to doing this, and the return value provides no new information. strncpy also requires a strlen to update the position. The reason I used snprintf (C99) in the code below is that it returns the number of characters written, which I can subtract from future calls. I've written a helper struct:

#include <stdlib.h> /* EXIT_SUCCESS */
#include <stdio.h>  /* *printf */
#include <string.h> /* strlen */

struct Supercat {
    char *print, *cursor;
    size_t left;
    int is_truncated;
};

/** Initialises {cat} to hold the string {print}, size {print_size}. It stores
 the empty string in print. */
static void supercat_init(struct Supercat *const cat, char *const print,
    const size_t print_size) {
    cat->print        = cat->cursor = print;
    cat->left         = print_size;
    cat->is_truncated = 0;
    print[0] = '\0';
}

/** Adds {append} to the string specified when \see{supercat_init} was
 called. If {append} is too big for the size, it truncates the string and sets
 {cat.is_truncated}. */
static void supercat(struct Supercat *const cat, const char *const append) {
    size_t size_took;
    int took;

    if(cat->is_truncated) return;
    took = snprintf(cat->cursor, cat->left, "%s", append);
    if(took < 0)  { cat->is_truncated = -1; return; }
    if(took == 0) { return; }
    if((size_took = took) >= cat->left) {
        cat->is_truncated = -1, size_took = cat->left - 1;
    }
    cat->cursor += size_took, cat->left -= size_took;
}

static const char *const start_str     = "[ ";
static const char *const end_str       = " ]";
static const char *const alter_end_str = "...]";
static const char *const sep_str       = ", ";
static const char *const null_str      = "Null";

int main(int argc, char **argv) {
    static char buffer[80];
    struct Supercat cat;
    int i;

    /* want to terminate before the end; always have space for alter_end_str */
    supercat_init(&cat, buffer, sizeof buffer - strlen(alter_end_str));

    if(!argc) {
        supercat(&cat, null_str);
    } else {
        supercat(&cat, start_str);
        for(i = 0; i < argc; i++) {
            if(i) supercat(&cat, sep_str);
            supercat(&cat, argv[i]);
            if(cat.is_truncated) break;
        }
        /* we are guaranteed to have enough room */
        sprintf(cat.cursor, "%s", cat.is_truncated ? alter_end_str : end_str);
    }

    printf("Arguments: %s.\n", cat.print);

    return EXIT_SUCCESS;
}

The string now will go up to 79 characters and refuse to write any more:

Thor:Supercat neil$ bin/Supercat
Arguments: [ bin/Supercat ].
Thor:Supercat neil$ bin/Supercat saves the day!
Arguments: [ bin/Supercat, saves, the, day! ].
Thor:Supercat neil$ bin/Supercat where are you? we need you to rescue us from the burning building.
Arguments: [ bin/Supercat, where, are, you?, we, need, you, to, rescue, us, from, the,...].
Thor:Supercat neil$ bin/Supercat where are you? we need you to rescue us from this burning building.
Arguments: [ bin/Supercat, where, are, you?, we, need, you, to, rescue, us, from, this...].
Thor:Supercat neil$ bin/Supercat where are you? we need you to rescue us from danger.
Arguments: [ bin/Supercat, where, are, you?, we, need, you, to, rescue, us, from, dang...].

Using a temporary structure is a pain, but it seems to me like writing something like if(strlen(strncat(cat, argv[i], sizeof cat - strlen(cat))) >= sizeof buffer) break; would be inefficient on large strings because it traverses the string 3 times (I count) to write one value. On the other hand, snprintf is overkill with a constant format string.

Back in the day, even before the official C89 standard was released, people were asking "why the hell does strcat not return the end pointer?"

I believe that the C standards committee, in a fit of pique, "doubled down" on their mistake and made a willful effort not to remedy it, simply because it would be admitting that they made a boneheaded error in the first place. It's been a sore spot ever since, and now they've switched to strcat_s which doesn't return a pointer at all, which leaves the problem unresolved, and leaves strcat as an unloved and underused function.

Also pretty much from the get-go, there were libraries of string functions that provided an alternative version of strcat that did the right thing and returned a pointer to the end of the string. Often, those functions were named strecat or strcate, with the e meaning "return a pointer to the end." There were also strecpy and various strne___ functions. (IIRC, Borland provided them with their Turbo C++ compiler.)

Considering that you know the size of the buffer, I'd suggest you simply write (or search for) a version of strnecpy and strnecat and maintain your own count of characters remaining.

I suppose you could maintain both pointer and count in static variables, and accept a NULL destination to mean "use the previous values". You should read up on strtok first, though, to understand the implications - in general that sort of code isn't thread-safe, interrupt-safe, etc. (strtok has nothing to do with strcat but it's defined to have similar "use the last string" behavior, and usually comes with a bunch of caveats in the documentation.)

register char *p = buffer;
register char *pe = buffer + sizeof(buffer);

p = strnecpy(p, start_str, pe - p);

for (i = 0; i < argc; ++i) {
    if (i) p = strnecat(p, sep_str, pe - p);
    p = strnecat(p, argv[i], pe - p);

    if (p >= pe - 1) break;
}

Or, if a NULL destination means "re-use the last pointer and size info" you might have:

strnecpy(buffer, start_str, sizeof(buffer));

for (i = 0; i < argc; ++i) {
    if (i) strnecat(NULL, sep_str);
    if (strnecat(NULL, argv[i]) >= buffer + sizeof(buffer) - 1) break;
}

FWIW, I actually think this last bit would be a bad idea, simply because it relies so very much on hidden behavior. It's not at all clear, just reading the code, that there's a bunch of secret-squirrel things happening in the background. With a C++ string class, that's okay - with C strings, secret accounting is surprising.


Building equal number of records list object I am trying to get equal number of records in both columns, even if that means that first column doesn't fill page to bottom on the second page. The only way I was able to do this was by using a maxrow count and breaking when it is equal to the max row count.

 public List<DataForExcelExport> GetData()
        {

            var list = new List<DataForExcelExport>();
            int ROWS_PER_PAGE = 41;
            int MaxRow = 0;

            SqlDataAdapter dataAdapter = new SqlDataAdapter(selectCommandText, connectionString);
            DataTable dataTable = new DataTable();
            dataAdapter.Fill(dataTable);
            bool flgLastPage = false;
            for (int i = 0; i < dataTable.Rows.Count; i++)
            {
                DataRow dr = dataTable.Rows[i];
                var item = new DataForExcelExport();
                SetItemData(dr, item, true);
                if (i + ROWS_PER_PAGE < dataTable.Rows.Count)
                {
                    SetItemData(dataTable.Rows[i + ROWS_PER_PAGE], item, false);
                }
                list.Add(item);
                if (flgLastPage == false)
                {
                    if ((i + 1) % ROWS_PER_PAGE == 0)
                    {
                        i += ROWS_PER_PAGE;

                        int remaining = dataTable.Rows.Count - (i + 1);
                        if (remaining < 2 * ROWS_PER_PAGE)
                        {
                            ROWS_PER_PAGE = remaining / 2 + remaining % 2;
                            flgLastPage = true;

                        }
                    }
                }
                int maxrow = i + ROWS_PER_PAGE;
                if (maxrow == dataTable.Rows.Count)
                {
                    break;

                }
            }
            return list;

        }

expected output: when there are a total of 118 rows

- page 1
_________
  1 | 41
... |...
 38 | 78
 39 | 79
 40 | 80
_________

- page 2
_________
 81 | 99
 82 | 100
... | ...
98  |118
_________

Try following some simple rules: Avoid setting flags in loops. Try calculating the data you need before taking action. Only increment counters inside loop declaration. This way even if something gets broken - you will not have to wonder "how did we end up here?". Instead you will be able to tell "this index / limit is not calculated correctly".

Also when saying

  even if that means that first column doesn't fill page to bottom on the second page

Did you mean:

  even if that means both columns don't fill the last page to the bottom

This is how I understood it.

int ROWS_PER_PAGE = 41;
int totalDataCount = dataTable.Rows.Count;
int pagesToProcess = (int)Math.Ceiling(totalDataCount / 2.0 / ROWS_PER_PAGE);
int itemsPerPage = ROWS_PER_PAGE * 2;

for(int pageIdx = 0; pageIdx < pagesToProcess; pageIdx++)
{
    int itemsOnThisPage = (pageIdx + 1) * itemsPerPage >= totalDataCount ? totalDataCount - pageIdx * itemsPerPage : itemsPerPage;
    int rowsOnThisPage = (int)Math.Ceiling( itemsOnThisPage / 2.0 );
    for( int rowIdx = 0; rowIdx < rowsOnThisPage; rowIdx++)
    {
        var item = new DataForExcelExport();
        int itemIdx1 = pageIdx * itemsPerPage + rowIdx;
        int itemIdx2 = itemIdx1 + rowsOnThisPage;
        SetItemData(dataTable.Rows[itemIdx1], item, true);
        if(totalDataCount > itemIdx2)
            SetItemData(dataTable.Rows[itemIdx2], item, false);
    }
}

Ah yes, and try to name variables in a meaningful way. Names like i or maxrow do not tell what the variable is really used for.


Is it fine to typecast from one implementation type to another while using an object? I've a simple code base with few 'weapon' concrete classes which implements different contracts in order to be used by its clients.

My contracts:

public interface IWeaponPrimaryAttack
{
    void DoAttack();
}
public interface IWeaponSecondaryAttack
{
    void DoSecondaryAttack();
}
public interface IReloadable
{
    void Reload();
}

Concrete implementation or the actual weapons:

public class Katana : IWeaponPrimaryAttack, IWeaponSecondaryAttack
{
    public void DoAttack(){Console.WriteLine ("Swing");}
    public void DoSecondaryAttack() {Console.WriteLine ("Stab");}
}
public class ShotGun : IWeaponPrimaryAttack, IReloadable
{
    public void DoAttack(){Console.WriteLine ("Swing");}
    public void Reload() {//reload it}
}

Clients that uses these concrete classes:

public class PrimaryAttack
{
    private IWeaponPrimaryAttack _attack;
    public PrimaryAttack(IWeaponPrimaryAttack attack)
    {
        _attack = attack;
    }
    public void DoAttack()
    {
        _attack.DoAttack();
    }
}
public class SecondaryAttack
{
    private IWeaponSecondaryAttack _attack;
    public SecondaryAttack(IWeaponSecondaryAttack attack)
    {
        _attack = attack;
    }
    public void DoSecondaryAttack()
    {
        _attack.DoSecondaryAttack();
    }
}
public class WeaponReload
{
    private IReloadable _reloader;
    public WeaponReload(IReloadable reloader)
    {
        _reloader = reloader;
    }
    public void Reload()
    {
        _reloader.Reload();
    }
}

New of course the instantiation of concrete class only be known when the user selects one among many weapons(one among ShotGun, Katana, etc).

Let say the use has selected ShotGun as the weapon, based on the selection it might go like this:

IWeaponPrimaryAttack weapon = new ShotGun(); // get it from a factory
PrimaryAttack primaryAttack = new PrimaryAttack(weapon);
primaryAttack.DoAttack();

Now for the WeaponReload have to do a typecast here in order to use it.

WeaponReload reloader = new WeaponReload ((IReloadable)weapon);
reloader.Reload();

I have questions around it,

Should I really have to end up typecasting eventually?
Or there is a better way to handle this object creation part.
Or there is entirely a better design that I can come up with which
does not necessarily ends up in casting.
Or casting it like this is totally fine.

Composition over inheritance and Strategy Pattern

I would, instead of having differents weapons implementing differents interfaces defining their behaviour, create classes that holds the logic of the behaviour

Let's consider a weapon can have a primary attack, a secondary one and reload.

The class could be defined as such :

abstract class Weapon
{
    public IWeaponPrimaryAttack WeaponPrimaryAttack { get; set; }
    public IWeaponSecondaryAttack WeaponSecondaryAttack { get; set; }
    public IReloadable Reloadable { get; set; }
}

Now, we can define classes that holds differents behaviours of the interfaces.

In example :

public class SlashAttack : IWeaponPrimaryAttack
{
    public void DoAttack()
    {
        Console.WriteLine("Performing a slash attack");
    }
}

public class StabAttack : IWeaponSecondaryAttack
{
    public void DoSecondaryAttack()
    {
        Console.WriteLine("Performing a stab attack");
    }
}

public class Shooting12Caliber : IWeaponPrimaryAttack
{
    public void DoAttack()
    {
        Console.WriteLine("Bam !");
    }
}

public class Caliber12Reloader : IReloadable
{
    public void Reload()
    {
        Console.WriteLine("Reloading caliber 12 !");
    }
}

public class ShootingWith556 : IWeaponPrimaryAttack
{
    public void DoAttack()
    {
        Console.WriteLine("Bam (version 5.56) !");
    }
}

public class Caliber556Reloader : IReloadable
{
    public void Reload()
    {
        Console.WriteLine("Reloading caliber 5.56 !");
    }
}

And now, you can use theses class as properties in differents weapon classes.

You'll implement once the methods and can use the same for differents weapons :

public class Katana : Weapon
{
    public Katana()
    {
         WeaponPrimaryAttack = new SlashAttack();
         WeaponSecondaryAttack = new StabAttack();
    }
}

public class Machete : Weapon
{
    public Machete()
    {
         WeaponPrimaryAttack = new SlashAttack();
    }
}

public class Shotgun : Weapon
{
    public Shotgun()
    {
         WeaponPrimaryAttack = new Shooting12Caliber();
         Reloadable = new Caliber12Reloader();
    }
}

public class AssaultRifleWithBayonet : Weapon
{
    public AssaultRifleWithBayonet()
    {
         WeaponPrimaryAttack = new ShootingWith556();
         WeaponSecondaryAttack = new StabAttack();
         Reloadable = new Caliber556Reloader();
    }
}

Now, your methods implementations are refactored within classes and are reusable. You don't need to typecast to check if this or that interface is implemented.

if (weapon.Reloadable != null)
{
    weapon.Reloadable.Reload();
}
else
{
    Console.WriteLine("This weapon isn't meant to be reloaded");
}

/*
This works too if you want to omit the Reloadable != null

public class CannotReload : IReloadable
{
    public void Reload()
    {
        Console.WriteLine("This weapon isn't meant to be reloaded");
    }
}
*/


Need review on concatenation I am trying to concatenate the echo statements but I am failing. Also any other tips are appreciated in your review.

<?php
// load all 'category' terms for the post
$terms = get_the_terms($post->ID, 'category');

// we will use the first term to load ACF data from
if( !empty($terms) )
{
    $term = array_pop($terms);

    $custom_field = get_field('category_image', 'category_' . $term->term_id );

    // do something with $custom_field
    echo "";
    echo    "<img src="<?php echo $image; ?>" alt="<?php echo get_cat_name($cat->term_id); ?>" class="item-image">";
    echo "";
}
?>

The places where I have worked, as well as a number of articles I have read, have taught me that echoing HTML is bad practice, and really if you think about it from a maintenance perspective you want that clear separation of PHP and HTML. An example of your code with that separation:

<?php
    $terms = get_the_terms($post->ID, 'category');

    if(!empty($terms)){
        $term = array_pop($terms);
        $custom_field = get_field('category_image', 'category_' . $term->term_id );
?>
    <a href="<?php echo get_field('category_link', $custom_field); ?>">
        <img src="<?php echo $image; ?>" alt="<?php echo get_cat_name($cat->term_id); ?>" class="item-image" />

<?php } ?>

Or if you wanted to simplify it a bit for future readers:

<?php
    $terms = get_the_terms($post->ID, 'category');

    if(!empty($terms)){
        $term = array_pop($terms);
        $custom_field = get_field('category_image', 'category_' . $term->term_id );
        $href = get_field('category_link', $custom_field);
        $cat_name = get_cat_name($cat->term_id);
?>
    <a href="<?php echo $href; ?>">
        <img src="<?php echo $image; ?>" alt="<?php echo $cat_name; ?>" class="item-image" />

<?php } ?>

Or you could build an array out of the values if you wanted to "encapsulate" the link or something instead. Either way, a setup like this is much clearer and more maintainable than if you were to concatenate everything into one giant string and echo it.

Hope this helps.


Reading from and writing to a file without passing around filename I was looking for a way to write to and then read from a file in different parts of my code, without passing the filename around and without consumers needing to know it was coming from a file.

I came up with this FilePipe based on io.Pipe:

package pipe

import (
    "io"
    "os"
)

func FilePipe(name string) (*io.PipeReader, *io.PipeWriter) {
    inr, inw := io.Pipe()
    outr, outw := io.Pipe()

    go func() {
        // Open the file for writing
        f, err := os.Create(name)
        if err != nil {
            outw.CloseWithError(err)
            return
        }

        // Copy the input to the file
        _, err = io.Copy(f, inr)
        f.Close()
        if err != nil {
            outw.CloseWithError(err)
            return
        }

        // Open the file for reading
        f, err = os.Open(name)
        if err != nil {
            outw.CloseWithError(err)
            return
        }

        // Copy the file to the output
        _, err = io.Copy(outw, f)
        f.Close()
        if err != nil {
            outw.CloseWithError(err)
            return
        }
        outw.Close()
    }()

    return outr, inw
}

What do you think? Is there a better way to do this?

you could use a teereader:

inr, inw := io.Pipe()
f, _ := os.Create(name)
tr := io.TeeReader(inr, f)
return tr, inw

use this code in go routine and handle errors


Inversion count via divide and conquer I'm happy to hear thoughts and ideas on structure/performance/testing/whatever and multi-threading, which I haven't gotten into yet with Python.

Full code here. Assignment file and a test file included in the folder.

Excerpt:

import unittest
import sys

def Merge_and_CountSplitInv(A,start_index,len_1st,len_2nd):
    """
    Merges two similar length pre-sorted sub-slices of array A while counting
    inversions.
    Input:
    array A, start_index (location of first sub-slice)
    len_1st,len_2nd (subslice_lengths).
    Output: inversions (values where i<j, but A[i]>A[j])
    Side Effect: two consecutive A subsections are sorted.
    """
    inversions=0
    temp_array=[]
    index_1st=start_index
    index_2nd=start_index+len_1st
    #while both indices are in range
    while ((index_1st < start_index+len_1st) and
          (index_2nd < start_index+len_1st+len_2nd)):
        #place smaller value in temp_array, increase inversions
        if A[index_1st]<=A[index_2nd]:
            temp_array.append(A[index_1st])
            index_1st += 1
        else:
            temp_array.append(A[index_2nd])
            index_2nd += 1
            inversions+=(start_index+len_1st-index_1st)
    #one index is out of range:
    if index_2nd == start_index+len_1st+len_2nd: # 2nd index complete
        #add leftover 1st half values to temp_array
        #before destroying them by the write process.
        temp_array.extend(A[index_1st:start_index+len_1st])
    else: # 1st index complete
        pass #no need to write over leftovers in 2nd sub-array
    #write temp_array over A[start_index:start_index+len(temp_array)]
    write_index=start_index
    for value in temp_array:
        A[write_index]=value
        write_index +=1
    return inversions

def Sort_and_Count(A, n, start_index=0):
    """
    Input: array A, length n, start_index(default=0)
    Output: inversions (values where i<j, but A[i]>A[j])
    Side Effect: A is sorted.
    """
    if n==1:
        return 0    # base case, array of length 1
    else:
        # Input: 1st and 2nd halves of current sub-array
        len_1st=n//2
        len_2nd=n//2+n%2
        x=Sort_and_Count(A, len_1st, start_index)
        y=Sort_and_Count(A, len_2nd, start_index+len_1st)
        # merge the (newly sorted) half-sized sub-arrays
        z=Merge_and_CountSplitInv(A,start_index,len_1st,len_2nd)
    return x+y+z

class TestSort_and_Count(unittest.TestCase):
    """
    Basic test class
    """

    def test_Sort_and_Count(self):
        A=[1]
        res1 = Sort_and_Count(A,len(A))  # single element
        self.assertEqual(res1, 0)
        B=[1,3,5,2,4,6]
        res2 = Sort_and_Count(B,len(B))  # even length
        self.assertEqual(res2, 3)
        C=[1,3,5,2,4,6,3]
        res3 = Sort_and_Count(C,len(C))  # odd length, duplicate value
        self.assertEqual(res3, 6)

def main(file_name):
    #TODO: take values from file and run sort_and_count
    with open(file_name) as fh:
        if file_name[:4] == 'test':
            print(fh.readline())  # get rid of first answer line from debug file
        A = list(map(int, [line.strip() for line in fh]))
        print(Sort_and_Count(A,len(A)))

if __name__ == '__main__':

    #TODO: working with argv to accept file input
    if len(sys.argv) > 2:
        sys.exit("Usage: inv_count <file_name> (leave empty for testing)")
    if len(sys.argv) == 1:
        print("No filename input, testing...")
        unittest.main()
    # else: argv == 2
    main(sys.argv[1])

1. Bug

If you pass the empty list to Sort_and_Count, it recurses until it runs out of stack:

>>> Sort_and_Count([], 0)
Traceback (most recent call last):
  File "cr107928.py", line 67, in test_Sort_and_Count
    Sort_and_Count([],0)
  ... many lines omitted ...
  File "cr107928.py", line 49, in Sort_and_Count
    if n==1:
RecursionError: maximum recursion depth exceeded in comparison

2. Review

The code omits spaces around operators. This makes it hard to read. It is better to follow the Python style guide (PEP8):

  Always surround these binary operators with a single space on either side: assignment (=), augmented assignment (+=, -= etc.), comparisons (==, <, >, !=, <>, <=, >=, in, not in, is, is not), Booleans (and, or, not). [...] If operators with different priorities are used, consider adding whitespace around the operators with the lowest priority(ies).

The names do not follow the Python style guide. This recommends:

  Class names should normally use the CapWords convention.

so TestSortAndCount would be better than TestSort_and_Count.

  Function names should be lowercase, with words separated by underscores as necessary to improve readability.

so merge_and_count_inversions would be better than Merge_and_CountSplitInv.

It's not compulsory to follow the Python style guide, but it makes it easier for other Python programmers to read your code.
Python's documentation uses the term "sequence" to refer to indexable collections, not "array". There's less risk of confusion if you stick to the standard term.
The docstring says, "Merges two similar length pre-sorted sub-slices", but in fact the function works fine even if the subsequences are not of similar length. I know that when called from Sort_and_Count the subsequences will be equal in length or differ by one, but it's better to document the actual behaviour of the function.
Instead of index_1st and index_2nd, I would use i and j. These variables are used many times, so using short names reduces the amount of code and make it easier to read.
The arguments len_1st and len_2nd are only used within the expressions start_index+len_1st and start_index+len_1st+len_2nd. This suggests that the arguments to the function should be start, middle and end instead.
Instead of updating A one element at a time:

write_index=start_index
for value in temp_array:
    A[write_index]=value
    write_index +=1

update all elements at once using a slice assignment:

A[start_index:start_index + len(temp_array)] = temp_array

This is efficient (in CPython, anyway) because the replacement is the same length as the slice being assigned.
In Sort_and_Count, the caller is expected to pass in the length of the sequence. It would be simpler if Sort_and_Count called len instead. This could easily be done using a helper function; see updated code below.
The test case doesn't try the empty list (see "Bug" above).
The test cases only test that the inversion count is correct, but don't actually test that the sequence is sorted! You need something like:

self.assertEqual(A, sorted(A))

The test cases have hard-coded counts of inversions. But it is easy to count the inversions in a sequence, using itertools.combinations:

sum(j < i for i, j in combinations(seq, 2))

This is inefficient (it's \$ Θ(n^2) \$), but clearly correct, and so a good comparison for the efficient, but not clearly correct, counts produced by the function under test.
The test cases are very repetitive. This could be avoided with a helper method, something like this:

def check(self, seq):
    expected = sum(j < i for i, j in combinations(seq, 2))
    found = sort_and_count_inversions(seq)
    self.assertEqual(expected, found)
    self.assertEqual(seq, sorted(seq))

The test cases are not very thorough. You could easily test many more cases using random test generation:

for _ in range(100):
    self.check([randrange(100) for _ in range(randrange(100))])

3. Revised code

from itertools import combinations
from random import randrange
from unittest import TestCase

def merge_and_count_inversions(seq, start, middle, end):
    """Merge two adjacent sorted sub-sequences of seq and count
    inversions.

    Arguments:
    seq -- sequence to sort
    start -- index of beginning of first sorted subsequence
    middle -- end of first, and beginning of second, sorted subsequence
    end -- end of second sorted subsequence

    Result: number of inversions (cases where i < j, but seq[i] > seq[j]).

    Side effect: seq is sorted between start and end.

    """
    assert 0 <= start < middle < end <= len(seq)
    inversions = 0
    temp = []
    i = start
    j = middle
    while i < middle and j < end:
        if seq[i] <= seq[j]:
            temp.append(seq[i])
            i += 1
        else:
            temp.append(seq[j])
            j += 1
            inversions += middle - i
    if j == end:
        # Second subsequence is complete: process remainder of first.
        temp.extend(seq[i:middle])
    else:
        # First subsequence is complete: no need to process
        # remaininder of second, since it does not move.
        pass
    # Insert sorted results into original sequence.
    seq[start:start+len(temp)] = temp
    return inversions

def sort_and_count_inversions(seq):
    """Sort a sequence and count inversions.

    Argument: seq -- a sequence

    Result: number of inversions (cases where i < j, but seq[i] > seq[j]).

    Side effect: seq is sorted.

    """
    def sort_and_count(seq, start, end):
        if end - start < 2:
            return 0
        middle = (start + end) // 2
        return (sort_and_count(seq, start, middle)
                + sort_and_count(seq, middle, end)
                + merge_and_count_inversions(seq, start, middle, end))
    return sort_and_count(seq, 0, len(seq))

class TestSortAndCount(TestCase):
    def check(self, seq):
        expected = sum(j < i for i, j in combinations(seq, 2))
        found = sort_and_count_inversions(seq)
        self.assertEqual(expected, found)
        self.assertEqual(seq, sorted(seq))

    def runTest(self):
        self.check([])                    # empty sequence
        self.check([1])                   # single element
        self.check([1, 3, 5, 2, 4, 6])    # even length
        self.check([1, 3, 5, 2, 4, 6, 3]) # odd length, duplicate value
        for _ in range(100):              # random test cases
            self.check([randrange(100) for _ in range(randrange(100))])


Replace template based on placeholder values I have wrote a PHP template code to replace placeholders based on the value provided within the placeholder. For example the content may have placeholders like
{ITEM-100} {ITEM-110} and I want to replace them with a text made by the details fetched from db with item_id=100 or item_id=110.

The code below id working fine, and I would like to get suggestions to improve the performance of the code. Thanks in advance.

$content = 'Here is the item list {ITEM-100}  {ITEM-110}  {ITEM-200}';

if(preg_match_all('/{ITEM-.*?}/', $content, $matches))
{
    foreach($matches[0] as $v)
    {
        $value = trim($v, '{}');
        $val_chunk = explode('-', $value);
        if(isset($val_chunk[1])){
            $item_card = get_item_card($val_chunk[1]);
            $content = str_replace($v, $item_card, $content);
        }
    }
}

echo $content;

The best way to improve the performance of this code is probably by using the preg_replace_callback()-function. This function allows you to search for a regex in a string, and call a function for every match. This function then returns the replacement. This avoids running multiple str_replace() operations over the string, which can be costly for large strings.

Secondly, we can use a capture group in the regex to avoid the use of explode(). I.e. by changing the regex from /{ITEM-.*?}/ to /{ITEM-(.*?)}/ we can ensure that we get the matched item identifier directly.

Applying those two modifications, we would end up with something like:

$content = 'Here is the item list {ITEM-100}  {ITEM-110}  {ITEM-200}';
function get_replacement($matches) {
    $value = $matches[1]; // Index 1 is the first capture group, which contains our item number.
    return get_item_card($value);
}
$content = preg_replace_callback('/{ITEM-(.*?)}/', 'get_replacement', $content);
echo $content;


Self Join Exercise. Have I over-complicated it? This my 3rd question on the same exercise, but by no means a duplicate.  The two previous questions were posted on StackOverflow here and here.

Now I'm posting my Oracle solution (below) that works. I wonder if the same could've been simpler and/or much more efficient. Not in terms of CTE or analytic expressions but with basic simple logic.

Data:

CREATE TABLE Readings (
  user_id varchar(10),
  reading_time int ,
  x decimal(10,2),
  y decimal(10,2)
);

INSERT ALL
    INTO Readings (user_id, reading_time, x, y) VALUES ('u1', 60, 345, 400)
    INTO Readings (user_id, reading_time, x, y) VALUES ('u1', 100, 560, 300)
    INTO Readings (user_id, reading_time, x, y) VALUES ('u2', 35, 1024, 250)
    INTO Readings (user_id, reading_time, x, y) VALUES ('u1', 90, 450, 450)
    INTO Readings (user_id, reading_time, x, y) VALUES ('u3', 150, 600, 100)
    INTO Readings (user_id, reading_time, x, y) VALUES ('u3', 100, 500, 125)
SELECT * FROM dual;

Intermediate script:

SELECT r.user_id, rm.reading_time start_time, r.reading_time end_time,
       (r.reading_time-rm.reading_time) time_spent,
       (TO_CHAR(rm.x)||' ; '||TO_CHAR(rm.y)) start_point,
       (TO_CHAR(r.x)||' ; '||TO_CHAR(r.y)) end_point,
       SQRT(POWER(r.x-rm.x, 2)+POWER(r.x-rm.y, 2)) distance
FROM Readings r
JOIN Readings rm ON (r.user_id = rm.user_id and
                     rm.reading_time = (SELECT MAX(r2.reading_time) FROM Readings r2 WHERE r2.reading_time < r.reading_time))
ORDER BY 1,2;

Final script:

SELECT rr.user_id, SUM(rr.distance) "Total Distance",
       SUM(rr.time_spent) "Total Time", SUM(rr.distance)/SUM(rr.time_spent) "Average Speed"
FROM
(SELECT r.user_id, (r.reading_time-rm.reading_time) time_spent,
        SQRT(POWER(r.x-rm.x, 2)+POWER(r.x-rm.y, 2)) distance
FROM Readings r
JOIN Readings rm ON (r.user_id = rm.user_id and
                     rm.reading_time = (SELECT MAX(r2.reading_time) FROM Readings r2 WHERE r2.reading_time < r.reading_time))) rr
GROUP BY rr.user_id
ORDER BY 1;

Exercise Description

Multiple users roam a plain and at irregular time intervals report their coordinates (x, y).  This information (user id, time-stamp and the coordinates) populate table Readings.

For each user that reported more than one set of coordinates we need to find total distance traveled, total time spent, and their average speed.

For the sake of simplicity coordinates are Cartesian and time-stamps are integers.

You have a bug. Change the row ('u3', 100, 500, 125) to ('u3', 99, 500, 125), and try again. You will see that u3 row disappears.

You can fix it with the following minimal change:

rm.reading_time = (SELECT MAX(r2.reading_time)
  FROM Readings r2
  WHERE  r2.user_id=r.user_id -- you have to join on ids first
  and r2.reading_time < r.reading_time)

There is another bug here:
r.x-rm.y should have been r.y-rm.y.
user_id, reading_time is a unique key by problem definition. Having it declared will enable us to detect some invalid inputs. What is the average speed of an object that can be at more than one place?
I would go along with something like this for the intermediate script:

SELECT t.*, t2.*
FROM (SELECT r.user_id,r.reading_time,
         max(r.x) AS x1, max(r.y) AS y1,
         max(rm.reading_time) AS prev
FROM Readings r
JOIN Readings rm ON (r.user_id = rm.user_id AND rm.reading_time < r.reading_time)
GROUP BY r.user_id,r.reading_time) t
INNER JOIN Readings t2
ON t.user_id = t2.user_id AND t2.reading_time = t.prev;


Generic enum to text lookup in C Can my code from this question be improved? I am thiking of validating inputs, portability issues, etc

Please note that we must declare our strings at compile time. We code embedded systems and are not allowed to malloc().

[Update]

I forgot to state that our enums are non-contiguous and have a wide range, which can make a difference.

Here's some test data:

// +=+=+=+=+=+=+=+=+=+=+=+=+=+=
typedef enum
{
    north,
    south,
    east,
    west
} E_directions;

struct direction_datum
{
    E_directions direction;
    char         direction_name[6];
};

struct direction_datum direction_data[] =
{
    {north, "north"},
    {south, "south"},
    {east,  "east"},
    {west,  "west"},
};

// +=+=+=+=+=+=+=+=+=+=+=+=+=+=
typedef enum
{
    hearts,
    spades,
    diamonds,
    clubs,
} E_suits;

struct suit_datum
{
    E_suits suit;
    char    suit_name[9];
};

struct suit_datum suit_data[] =
{
    {hearts,   "hearts"},
    {spades,   "spades"},
    {diamonds, "diamonds",},
    {clubs,    "clubs"},
};

and, here's the code, which should  get text from an enum, as defined by a struct of enum/string, where different structs can have different length arrays.:

#define ARRAY_LENGTH(A) sizeof(A) / sizeof(*A)

char *Get_text_from_enum(int enum_value,
                         void *array,
                         unsigned int array_length,
                         unsigned int size_of_array_entry)
{
    unsigned int i;
    unsigned int offset;

    for (i = 0; i < array_length; i++)
    {
        offset = i * size_of_array_entry;

        if ((int) * ((int *) (array+ offset)) == enum_value)
            return (char *)  (array + offset + sizeof(int));
    }

    return NULL;
}

printf("Expect south,    got %s\n",
          Get_text_from_enum(south,
                             direction_data,
                             ARRAY_LENGTH(direction_data),
                             sizeof(direction_data[0])));

printf("Expect diamonds, got %s\n",
          Get_text_from_enum(diamonds,
                             suit_data,
                             ARRAY_LENGTH(suit_data),
                             sizeof(suit_data[0])));

Illegal void pointer arithmetic
This code is illegal in C:
void *array;
... (array + offset)

You are not allowed to do pointer arithmetic on a void * pointer.  You should use char * instead of void *.
Simplifications
I would suggest defining a generic structure like this:
typedef struct EnumName {
    int         value;
    const char *name;
} EnumName;

to avoid all of the struct length shenanigans.  You can also make it a rule that each EnumName array must be terminated with a NULL name.  This rule allows you to avoid passing in an array length argument.  Then your code would look like this:
const EnumName direction_data[] =
{
    {north, "north"},
    {south, "south"},
    {east,  "east" },
    {west,  "west" },
    {0,      NULL  },
};

const char *Get_text_from_enum(int enum_value, const EnumName *names)
{
    while (names->value != enum_value && names->name != NULL)
        names++;
    return names->name;
}

int main(void)
{
    printf("Expect south,    got %s\n",
            Get_text_from_enum(south, direction_data));
}


Leap Year game in Java I created just for practice this leapyear script.

What do you think about it?
How can I make it more efficient?

package com.company;

import java.util.Scanner;

public class Main {

public static void main(String[] args) {

    Scanner input = new Scanner(System.in);
    System.out.printf("Plese enter year: ");
    int chosedYear = input.nextInt();

    leapYearMethod(chosedYear);

}

public static void leapYearMethod (int leapyear) {
    if ( leapyear % 4 == 0 ){
        System.out.println("Yeeees!");
    } else {
        int years = leapyear % 4;
        if(years == 1){
            System.out.println("3 years later there is a leapyear.");
        } else if( years == 2) {
            System.out.println("2 years later there is a leapyear.");
        } else if( years == 3) {
            System.out.println("1 years later there is a leapyear.");
        }
    }
    }
}

Regardless of your algorithm, you can simplify it much more with a simple if or switch.

int years = leapyear%4;
switch( years ) {
  case 0:
    println("Yeeees!");
    break;
  case 1:
    println("1 year later there is a leapyear.");
    break;
  default:
    println((4-years)+" years later there is a leapyear.");
    break;
}

But I doubt that this is a good practising exercise:

Do you have an unit test for it ? If you had to create one, how could you test the result since it is printed to the console.

How could you improve this code to make it testable and extendable. What could you do if I want to print another message ?


Scraping after login using Scrapy I just finished a scraper in python using scrapy. The scraper logs in to a certain page and then scrapes a list of other pages using the authenticated session.
It retrieves the title of these pages and puts it in a list of items together with the page name.

Because there are little examples to find on continuing scraping after log-in I would like to receive some feedback on if this is the best way to continue scraping.

(Also I have a weird workaround to get the pagename into the itemlist, since I already have the pagename in the pagelist, but in the action method I retrieve it from the url again. Tips on doing this in an easier way are welcome too.)

p.s. The code has been slightly edited for sharing purposes.

class ShareSpider(scrapy.Spider):
    name = "sharespider"
    start_urls = ['http://www.example.com/public/login.aspx']

    def parse(self, response):
        yield scrapy.FormRequest.from_response(
            response,
            formxpath='//form[@id="login"]',
            formdata={
                'UserName': 'UserNameHere',
                'Password': 'PasswordHere',
                'Action':'1',
            },
            callback=self.after_login
        )

    def after_login(self, response):
        baseurl = 'http://www.example.com/public/'
        #Specify pages to crawl here:
        pagelist = ['page1.aspx', 'page2.aspx', 'page3.aspx', 'page4.aspx']
        for page in pagelist:
            yield Request(url= baseurl + page + "?id=1",
            callback=self.action)

    def action(self, response):
        pageurl = str(response.url)
        page = re.search('public/(.*)id=1', pageurl)
        if page:
            pagename = page.group(1)
        #Get page <title> element and strip whitespace
        title = str(response.selector.xpath('//title/text()').extract_first())
        res = title.strip()

        item = PageItem()
        item['pagename'] = pagename
        item['description'] = res
        yield item

I'll start off by saying this is actually really good.
There are a few minor PEP8 errors:

In after_login you call Request. the argument url shouldn't have a space on the right side of the equals.
Another thing in Request. The argument callback needs at least one more indent.
However you should either aligned with opening delimiter or use hanging indents.
And so you should change this to either of:

yield Request(url=baseurl + page + "?id=1",
              callback=self.action)

yield Request(
    url=baseurl + page + "?id=1",
    callback=self.action)

Variables and functions should be named in snake_case.
pagelist should be page_list, pageurl page_url, etc.

Whilst not being a PEP8 error you may want to change page_list to say pages.
This is as multiple pages are probably a list, and sounds more fluid.

I say the above as consistency is the best way to allow others to easily read your work.
But they are quite minor.

Personally I'd move the baseurl out of the class and make it a constant.
This allows you to easily change the url, say the company you're scraping change their company/domain name or their top-level domain.
I'd also move other system settings, 'UserNameHere', 'PasswordHere', 'http://www.example.com'.
I may even say that pagelist should be moved out too.

Now for comments on your business logic, as parse and after_login are very short 3 line functions,
there's honestly not much to say about them.
action however has a few problems:

response.url is a string and so you shouldn't cast it to one.
This confused me as I it made me think response.url would be something that isn't a string.
You seem to be getting the response as an instance of TextResponse, which contains selector.
This also contains the shortcut xpath which is equivalent to selector.xpath.
Which allows you to reduce the size of that large command.
Unfortunately scrapy doesn't document extract_first, but looking at the source they use Parsel.
The documentation on extract_first was also kinda lacking, so I read the source code again, and it seems like they always return strings or None.
So you probably don't need the str around extract_first if you set default to an empty string.
You don't define pagename unless page is not None.
But you go on to use it regardless.
This is wrong, and can lead to errors.
There are two ways to come at this, either silently fail.
Or raise an exception, where NameError: name 'pagename' is not defined is not a good error.
Finally it's strange to see yield rather than return, you're only going to ever return one thing.
And so unless it's a requirement imposed by scrapy I'd change it to return.

And so if I were to improve it, leaving yield and failing silently, I'd end with:

BASE_URL = 'http://www.example.com'
USER_NAME = 'UserNameHere'
PASSWORD = 'PasswordHere'
PAGES = ['page1.aspx', 'page2.aspx', 'page3.aspx', 'page4.aspx']

class ShareSpider(scrapy.Spider):
    name = "sharespider"
    start_urls = [BASE_URL + '/public/login.aspx']

    def parse(self, response):
        yield scrapy.FormRequest.from_response(
            response,
            formxpath='//form[@id="login"]',
            formdata={
                'UserName': USER_NAME,
                'Password': PASSWORD,
                'Action':'1',
            },
            callback=self.after_login)

    def after_login(self, response):
        base_url = BASE_URL + '/public/'
        for page in PAGES:
            yield Request(
                url=base_url + page + "?id=1",
                callback=self.action)

    def action(self, response):
        page = re.search('public/(.*)id=1', response.url)
        if page:
            page_name = page.group(1)
            title = response.xpath('//title/text()').extract_first('').strip()
            item = PageItem()
            item['pagename'] = page_name
            item['description'] = title
            yield item


Return one particular element first, when iterating a lua table I wish to iterate through a Lua table, but want to get one particular element first always (its key is known beforehand).

Since the pairs function does not guarantee the order of a table's elements, I wrote a custom iterator function (I can insert every element into a indexed table and use the table.sort function, but I wanted to try writing a custom iterator).

local function
pairsWithPriority(t,priorityKey)

    local i = 0

    local function
    closure(_,lastReturnedKey)

        local k,v
        if i == 0 then  --the first element should always be t[prioritykey]
            k,v = priorityKey,t[priorityKey]
        elseif i == 1 then --since we have returned the first element we reset the next pointer
            k,v = next(t, nil)
        else
            k,v = next(t,lastReturnedKey)
        end

        if i > 0 then
            if k == priorityKey then --the first element is encountered AFTER it has been manually returned, so discard
                k,v = next(t, k)
            end
        end

        i = i + 1
        return k,v
    end

    return closure
 end

When I use this iterator function:

local t = {a=1,b=2,c=6,d=4,e=4}
for i,v in pairsWithPriority(t,"c") do
    print(i,v)
end

t[c] is always returned first.

This iterator works, but I'd like to know if there is a better way of doing this in a cleaner and more efficient way

You are performing the check i==0 and i==1 every time after i has incremented beyond those values. You can reduce this check by creating a lookup table.

lookup = {
    [0] = function()
        return priorityKey, t[priorityKey]
    end,
    [1] = function()
        return next(t, nil)
    end
}

The same can be done for you later checking i == priorityKey. I modified the lookup functions instead to deal with that case:

local function pairsWithPriority (t,priorityKey)
    local i, lookup = 0, {
        [0] = function()
            return priorityKey, t[priorityKey], true
        end,
        [1] = function()
            return next( t, nil )
        end,
    }
    local function closure( _, lastReturnedKey )
        local k, v, b
        if i > 1 then
            k, v = next( t, lastReturnedKey )
        else
            k, v, b = lookup[i]()
            if b then i = i + 1 return k, v end
        end
        if k == priorityKey then --the first element is encountered AFTER it has been manually returned, so discard
            k, v = next( t, k )
        end
        i = i + 1
        return k, v
    end
    return closure
end


Random Plinko path using functional programming style In order to learn both scala and functional programming, I've been rewriting the projects from my introductory CS class in scala. One of the projects was a plinko game, where the user picks a slot to drop a token into and then the path that the token took and the money won from the end slot is printed to the console. The number of slots is constrained and so as the token falls row by row it can never be less than 0 or greater than 8.

I opted to use a stream to calculate the path, but I feel like the way that I am determining whether or not the token is within the constraints isn't really representative of functional programming because I know that generally vals are preferred to vars. What is a better way to write this in a functional style? In general, is there a way I should think about keeping track of things like this that is more in keeping with a functional programming style?

def getPath(prevSlot: Double): Stream[Double] = {
  var left = Random.nextBoolean
  if (prevSlot <= 0.0) {
    left = false
  }
  else if (prevSlot >= 8.0) {
    left = true
  }

  if (left) {
    prevSlot #:: getPath(prevSlot - 0.5)
  }
  else {
    prevSlot #:: getPath(prevSlot + 0.5)
  }
}

In Scala the if-else pattern is an expression, which means we can use it to return a value (similar to Java's or C++'s ternary operator). You don't have to change much in order to take advantage of this:

def getPath(prevSlot: Double): Stream[Double] = {
  val left =
    if (prevSlot <= 0.0) false
    else if (prevSlot >= 8.0) true
    else Random.nextBoolean

  if (left) prevSlot #:: getPath(prevSlot - 0.5)
  else prevSlot #:: getPath(prevSlot + 0.5)
}

And then of course you could get a readout of your path with the following:

getPath(4.0).take(13).toList.foreach(println)


Refactoring multiple `else if` code I think I can refactor this:

if (clicked_id == "contain-word") {
  $("#specie_filter li.checked").removeClass("checked");
  $("#specie_filter li:first").addClass("checked")
  $("#specie_filter ul").scrollTop(0)

  $("#cell_filter li.checked").removeClass("checked")
  $("#cell_filter li:first").addClass("checked")
  $("#cell_filter ul").scrollTop(0)

  $("#factor_filter li.checked").removeClass("checked")
  $("#factor_filter li:first").addClass("checked")
  $("#factor_filter ul").scrollTop(0)
} else if (clicked_id == "specie_filter") {
  $("#cell_filter li.checked").removeClass("checked")
  $("#cell_filter li:first").addClass("checked")
  $("#cell_filter ul").scrollTop(0)

  $("#factor_filter li.checked").removeClass("checked")
  $("#factor_filter li:first").addClass("checked")
  $("#factor_filter ul").scrollTop(0)

} else if (clicked_id == 'cell_filter') {
  $("#factor_filter li.checked").removeClass("checked")
  $("#factor_filter li:first").addClass("checked")
  $("#factor_filter ul").scrollTop(0)
}

it into something like

if (clicked_id == "contain-word" | clicked_id == "specie_filter"`)

but that will make the code more complex and the duplicity still exists in the conditional statement.  Does anyone have ideas about refactoring this?

I know I'm late to the game, but each filter function contains essentially the same lines of code. Only the container element differs:

function processFilter($container) {
    $container.find("li.checked").removeClass("checked");
    $container.find("li:first").addClass("checked");
    $container.find("ul").scrollTop(0);
}

switch (clicked_id) {
    case "contain-word":
        processFilter($("#specie_filter"))
        // fall-through...
    case "specie_filter":
        processFilter($("#cell_filter"));
        // fall-through...
    case "cell_filter":
        processFilter($("#factor_filter"));
        // fall-through...
}

EDIT: @toto2, thanks for the reminder. Yeah, comments are useful in this case since omitting the breaks in a switch statement is not normally encountered.

Also, if you want to get a little crazy with the jQuery API, this could also work:

function processFilter(selector) {
    $(selector)
        .find("ul").scrollTop(0)
        .find("li.checked").removeClass("checked")
        .parent()
        .find("li:first").addClass("checked");
}

switch (clicked_id) {
    case "contain-word":
        processFilter("#specie_filter");
        // fall-through...
    case "specie_filter":
        processFilter("#cell_filter");
        // fall-through...
    case "cell_filter":
        processFilter("#factor_filter");
        // fall-through...
}


Splitting canvas app into separate functions This is all in one methods but I would like to see it in individual separate methods. The code works fine but all of the code is in the canvasApp() methods and I would like to see it all in individual methods so that I could call them as below.

Question: How to make all of the code into separate function?

canvasApp();
gameloop();

Code:

function canvasApp() {

  if (!canvasSupport()) {
             return;
        }

    function  drawScreen () {
        context.fillStyle = '#EEEEEE';
        context.fillRect(0, 0, theCanvas.width, theCanvas.height);
        //Box
        context.strokeStyle = '#000000';
        context.strokeRect(1,  1, theCanvas.width-2, theCanvas.height-2);
        // Create ball
        y += speed;
        context.fillStyle = "#000000";
        context.beginPath();
        context.arc(x,y,15,0,Math.PI*2,true);
        context.closePath();
        context.fill();
    }

    theCanvas = document.getElementById('canvasOne');
    context = theCanvas.getContext('2d');

    var speed = 5;
    var y = 10;
    var x = 250;

    function gameLoop() {
        window.setTimeout(gameLoop, 20);
        drawScreen();
    }

    gameLoop();
}

From a Code Review perspective, what you are asking for, is worse.

Currently, you only need to call canvasApp() and that function does all the work. Inside you have drawScreen which is periodically called, starting in gameLoop.

Still,

I would put all the var statements on top for readability
I would put var in front of context and theCanvas (!)
I would assign your colors ('#EEEEEE') to meaningfully named variables
I would assign Math.PI*2 to a var doublePi = Math.PI*2 for speed
I would consider dropping the curly braces from the guard clause
//Box is not really a meaningful naming, //border and //background would make more sense`

That would give this:

function canvasApp() {

    if (!canvasSupport())
      return;

    var canvas = document.getElementById('canvasOne'),
        context = canvas.getContext('2d'),
        borderColor = '#EEEEEE',
        backgroundColor = '#000000',
        ballColor = '#000000',
        speed = 5,
        y = 10,
        x = 250,
        doublePi = Math.PI*2,
        ballSize = 15,
        refreshInterval = 20; //In milliseconds

    function  drawScreen () {
        //Background + Border
        context.fillStyle = borderColor;
        context.fillRect(0, 0, canvas.width, canvas.height);
        //Background
        context.strokeStyle = backgroundColor;
        context.strokeRect(1,  1, canvas.width-2, canvas.height-2);
        // Create ball
        y += speed;
        context.fillStyle = ballColor;
        context.beginPath();
        context.arc(x,y,ballSize,0,doublePi,true);
        context.closePath();
        context.fill();
    }

    function gameLoop() {
        window.setTimeout(gameLoop, refreshInterval);
        drawScreen();
    }

    gameLoop();
}

I am still not too excited by redrawing every 20 milliseconds 2 large boxes, but I will leave the research on further optimization to you.

Update: after playing the code and reading it more closely, the code only redraws a square once, and then the border, so that should be fine.

A working jsbin version is here: the animation is fast enough, though disappointing as there is no bouncing..
http://jsbin.com/lidil/2


Is this Agent/Actor implementation issue free? I implemented this Agent class for a project recently and was wondering if I could get some other eyes to look at it -- I'm currently the only developer where I work so I can't exactly ask someone here to do it.

I'm pretty sure it's correct, but then I'm too close to it.

public abstract class Agent<M> : IDisposable
{
    private Thread thread;
    private Queue<M> messageQueue;
    private bool quit;

    public Agent()
    {
        this.thread = new Thread(new ThreadStart(AgentThreadBody));
        this.messageQueue = new Queue<M>();
        this.quit = false;

        this.thread.Start();
    }

    /// <summary>
    /// Stops the message-handling thread.
    ///
    /// Do not call this method from within the message-
    /// handling method, or it will result in a deadlock
    /// (because this method waits for the message-handling
    /// thread to stop).
    /// </summary>
    public virtual void Dispose()
    {
        this.quit = true;
        this.thread.Interrupt();
        this.thread.Join();
        this.thread = null;
        // clear messageQueue before nulling?
        // (would do this to dispose queued items)
        this.messageQueue = null;
    }

    public void QueueMessage(M message)
    {
        lock (this.messageQueue)
        {
            this.messageQueue.Enqueue(message);
            this.thread.Interrupt();
        }
    }

    private void AgentThreadBody()
    {
        while (!this.quit)
        {
            M message = default(M);
            bool messageAvailable = false;

            lock (messageQueue)
            {
                if (messageQueue.Count > 0)
                {
                    message = messageQueue.Dequeue();
                    messageAvailable = true;
                }
            }

            try
            {
                if (!messageAvailable)
                {
                    // if the Interrupt() method was
                    // called before we sleep or is
                    // called while we're sleeping,
                    // this will throw:
                    // ThreadInterruptedException
                    Thread.Sleep(Timeout.Infinite);
                }
            }
            catch (ThreadInterruptedException)
            {
                // we have a new message to handle,
                // so get it, or we've been told
                // to quit.
                continue;
            }

            ProcessMessage(message);
        }
    }

    protected abstract void ProcessMessage(M message);
}

Also, are there any special considerations you can think of that should be made by a class inheriting from this base class? (I can't think of any.)

I could add start/stop methods, but at the moment they're not needed.

Btw, I have to use .NET 3.5.

Few minor points:

Generic type parameter should be called <T> as per convention.
public constructors in abstract classes make no sense - make it
protected.
Make thread and messageQueue readonly and don't
set them to null in the Dispose() method.
Better yet, implement the Disposable Pattern correctly. Though note, I do disagree with their setting of the IDisposable members to null. It's really not necessary at all; but calling Dispose() is. Their _disposed class member conveys enough information necessary.
You should also lock on a dedicated locking class member when accessing your quit class member since it is accessed via multiple threads.
Possibly wrap your call to ProcessMessage(message) in a try..catch block with appropriate error handling (or not, as the case could be). Unless, of course, you trust any subclasses from doing nasty things in their override of it.

Just my initial thoughts. Hope they help.


Console user main menu This is a simple console menu that just holds a main menu and a few menu "buttons" (numbers and titles which the user has to enter for the button to do its action). It does not need submenus or more complicated input handling.

I went with an object-oriented approach and defined three classes: Menu, Button, and Controller. The Menu initialises with a name and a list of menu buttons.

class Menu(object):

    """Base class for the menu"""

    def __init__(self, name, buttons):
        # Initialize values
        self.name = name
        self.buttons = buttons

    def display(self):

        """Displaying the menu alongside the navigation elements"""

        # Display menu name
        print self.name

        # Display menu buttons
        for button in self.buttons:
            print "   ", button.nav, button.name

        # Wait for user input
        self.userInput()

    def userInput(self):

        """Method to check and act upon user's input"""

        # This holds the amount of errors for the
        # navigation element to input comparison.
        errSel = 0
        inputSel = raw_input("Enter selection> ")

        for button in self.buttons:
            # If input equals to button's navigation element
            if inputSel == str(button.nav):
                # Do the button's function
                button.do()
            # If input != navigation element
            else:
                # Increase "error on selection" by one, for
                # counting the errors and checking their
                # amount against the total number of
                # buttons. If greater to or equal that means
                # no elements were selected.
                # In that case show error and try again
                errSel += 1

        # No usable input, try again
        if errSel >= len(self.buttons):
            print "Error on selection; try again."

The Button class is accepting a name, a function and a navigation element (the number you have to press for it to do the button's action):

class Button(object):

    """Base class for menu buttons"""

    def __init__(self, name, func, nav):
        # Initialize values
        self.name = name
        # Function associated with button
        self.func = func
        # Navigation element; number which user has to enter to do button action
        self.nav = nav

    def do(self):
        # Do the button's function
        self.func()

Finally, the Controller. Did I do it the right way? I mean it works but having that while loop cycling with only the raw_input() function stopping it from spamming infinite messages - is that correct or should I write something that prevents the while loop while a menu was already displayed and is awaiting user input?

class Controller(object):
    def __init__(self, menu):
        # Initialize values
        self.menu = menu
        # Start menu displaying / cycling
        self.cycle()

    def cycle(self):

        """Method for displaying / cycling through the menus"""

        while True:
            # Display menu and redisplay after button's function completes
            # Is this right? Will this loop correctly and never interfere
            # with raw_input()?
            self.menu.display()

Finally, the code that puts it all together:

import sys

def main():
    mainMenuButtonName = Button("Show name", showName, 1)
    mainMenuButtonVersion = Button("Show version", showVersion, 2)
    mainMenuButtonAbout = Button("Show about", showAbout, 3)
    mainMenuButtonQuit = Button("Quit", quit, 0)

    mainMenuButtons = [mainMenuButtonName, mainMenuButtonVersion, mainMenuButtonAbout, mainMenuButtonQuit]

    mainMenu = Menu("Main menu", mainMenuButtons)
    controller = Controller(mainMenu)

    controller.cycle()

def showName():
    print "My name is..."

def showVersion():
    print "My version is 0.1"

def showAbout():
    print "I am a demo app for testing menus"

def quit():
    sys.exit(0)

if __name__ == "__main__":
    main()

I think display() and userInput() would be clearer if you did it like this:

def display(self):
    """
    Display the menu alongside the navigation elements
    """
    response = None
    while response is None:
        # Display menu buttons
        for button in self.menu:
            print "   ", button.nav, button.name

        # Wait for user input
        response = self.userInput()

def userInput(self):
    """
    Method to check and act upon user's input
    """
    inputSel = raw_input("Enter selection> ")
    for button in self.menu:
        # If input equals to button's navigation element
        if inputSel == str(button.nav):
            button.do()
            return inputSel

    return None

So display() keeps showing the menu as long as userInput() returns None. userInput() returns None if the user's input doesn't match a button, or the inputSel value if it does match.

I think the way the Controller loops is fine. The program will always stop when it gets to raw_input() and read stdin until the user hits ENTER or RETURN. That's what raw_input() does.

You're managing your buttons as a list, which means you have to scan the whole list until you find the navigation element the user selected. If you put them in a dict, you can simply use the user's input to index directly into the structure and select the correct button (or determine that the user's selection is invalid). However, you still want them as a list for display because you can't control the order in which entries are pulled from a dictionary and I think you want your menu to display in the same order every time. That needs a list.

I would suggest the following for the constructor for the Display class. We're going to pass in a list so it's easy to define, and we'll keep the list, but we'll also have the class internally turn that list into a dict so it can more easily find the user's input on each selection.

class Display
    def __init__(self, menu):
        self.menu_list = menu
        self.menu_dict = {}
        # Initialise values
        for button in menu:
            self.menu_dict[button.nav] = button

With the simplification in userInput() and display() above, the shouldCycle member goes away. Now, with the menu defined internally as a dictionary (self.menu_dict) and as a list (self.menu_list), display() and userInput() will look like this:

def display(self):
    """
    Display the menu alongside the navigation elements
    """
    response = None
    while response is None:
        # Display menu buttons -- use the list so we get the same
        # order every time.
        for button in self.menu_list:
            print "   ", button.nav, button.name

        # Wait for user input
        response = self.userInput()

def userInput(self):
    """
    Method to check and act upon user's input
    """
    inputSel = raw_input("Enter selection> ")
    try:
        # Here we use the dictionary for ease of lookup
        button = self.menu_dict[int(inputSel)]
    except KeyError:
        # The user's selection didn't match any of the button.nav
        # values, so we got a KeyError exception on the dictionary
        return None

    button.do()
    return inputSel


Including a page specified in query string This is how I include a page depending by name.

Example URL:

  http://www.page.com/index.php?page=admin

where admin.inc.php lays inside /modules/ directory.

The files are located in a directory I specified. The included files should not render when connected to them directly.

Are there ANY issues with the code?
Are there any security flaws, such as remote file inclusion?
What could I improve in this approach?
Is this the right approach to modularize a page?
Are included pages secure enough to not be rendered by themselves?

<?php
    class cms {
        private $config_module_name;
        private $config_module_extention        = '.inc.php'; // ex: .inc.php
        private $config_module_directory        = 'modules'; // ex: modules
        private $config_module_list;

        function __construct() {
            $this->SetModuleList();
        }

        public function GetModuleExtention() {
            return $this->config_module_extention;
        }

        private function GetModuleDirectory() {
            return $this->config_module_directory;
        }

        public function GetModuleList() {
            $moduleList = array();
            foreach($this->config_module_list as $module) {
                if($this->CheckModule($module) == 1) { // 1 for pure module result
                    $moduleNoExt = str_replace($this->GetModuleExtention(), '', $module);
                    $moduleList[] = str_replace($this->GetModuleExtention(), '', $moduleNoExt);
                }
            }
            return $moduleList;
        }

        private function SetModuleList() {
            $this->config_module_list = array_diff(scandir($this->GetModuleDirectory(), 1), array('..', '.'));
        }

        // 0 - invalid; 1 - correct module; 2 - folder
        private function CheckModule($module) {
            $moduleNameArray = explode(".", $module);
            $countParts = count($moduleNameArray);
            if($countParts == 1) { // folder return 2
                return 2;
            }
            else if($countParts == 3) { // module must count 3
                $moduleNameExtention = '.' .  $moduleNameArray[1] . '.' . $moduleNameArray[2];
                // module must match set module extention
                if($this->GetModuleExtention() == $moduleNameExtention) {
                    return 1;
                }
            }
            return 0; // invalid module - if reached
        }

        public function IncludeModule($name) {
            if(in_array($name, $this->GetModuleList())) {
                include($this->GetModuleDirectory() . '/' . $name . $this->GetModuleExtention());
            }
            else { // if module doesn't exist - throw 404
                header('HTTP/1.0 404 Not Found');
                $this->ConcatContent('
Error 404

This page doesn\'t exist.');
            }
        }
    }
?>

Index page where all pages get included:

<?php
    define('CMS_TOKEN', '');
    include('config/config.inc.php');
    $cms = new cms()

    if(($_SERVER['REQUEST_METHOD'] === 'GET' || $_SERVER['REQUEST_METHOD'] === 'POST') && !empty($_GET['page'])) {
        if($_GET['page'] == '404') {
            header('HTTP/1.0 404 Not Found');
            $cms->ConcatContent('
Error 404

This page doesn\'t exist.');
        }
        else {
            print_r($_GET);
            $cms->IncludeModule($_GET['page']);
        }
    }
?>

This is how included module looks like. The file must be included by index, or else it should redirect.

<?php
    $module_name        = 'Register';
    if(!defined('CMS_TOKEN')) {
        // page cannot be rendered
        header('Location: ../');
    }
    else {
        // render page
    }
?>

Common practice is to check all GET/POST/SERVER/ENV parameter values against a whitelist or regex before using it for any purpose. You are checking it with a list derived from scandir, but that makes the assumption that the modules directory hasn't been compromised.
Running scandir on every request is a likely performance bottleneck. One possible solution is to cache the results of the scandir and subsequent filtering to a file above the document root and only re-run the scan if the mod time of the modules directory is more recent than the mod time of the cache file (check out the filemtime() docs).
You have some duplicate code for handling 404s. This could be combined into one method if you want to adhere to DRY principles.
A more widely accepted way to prevent a file from being executed is to compare the __FILE__ constant to the SCRIPT_FILENAME which has been set by the server (if available in the $_SERVER global). It's not fool proof, but more portable than your solution.


PHP & MySQLi Login Script - Is it safe from Injection? I've been geeking up on MySQLi prepared statements to avoid potential SQL injection and I have managed to modify my legacy code (previously just using mysqli_real_escape_string).

I am looking for some advice regarding if this code template is safe enough to use as I will be looking to update the rest of my code across my site.  I received some help from Fred-ii so thanks for that.

One issue I know is present is the use of $_SERVER['HTTP_REFERER'] but I will be creating a workaround via an array of acceptable URLs.

<?php
// Starting Session

session_start();
include '../connection.php';

// Previous page variable

$previous = $_SERVER['HTTP_REFERER'];

//Acquire POST values Please note: Sanity checks will be added for these =)

$myusername = $_POST["username"];
$mypassword = $_POST["password"];

// Preparing the statement

$stmt = $connection-> prepare("SELECT username, password FROM $tbl_name WHERE username= ? ");
$stmt -> bind_param("s", $myusername);

// Sanity checks on the statement and binding

if($stmt ->execute() == true) {
    $stmt -> bind_result($user, $pass);
    $stmt -> fetch();

    //Checking hashed password

    if (password_verify($mypassword, $pass)) {

        $_SESSION['login_user']= $myusername;
        header("location: ../member"); // Redirecting To Other Page

    } else {

        $_SESSION["login_error"] = "

Incorrect username or password, please try again.

";

        header('Location:'.$previous);

    }

    }

/* Close statement */
$connection-> close();

Looking for any feedback to improve this and any other recommendations.

Thanks. Stan.

There's no need for the $user variable (it's already in $myusername) so don't query for it either.

Please, deal with the $previous = $_SERVER['HTTP_REFERER']; now. You don't want to allow a malicious user to edit the HTTP headers you send.

What do you plan to do if any of the database calls fail? You don't seem to be checking for nor handling that. Also, it's best to have the exact same behavior on "invalid username" as well as "password supplied does not match". I'm not sure if your code does that or not.

I didn't think people usually put spaces around the -> method call operator, but at least be consistent with your spacing.


Inefficient DNA heterodimerization script I have stuck with this script it would be great if you could help me with your inputs. My problem is that I think the script is not that efficient - it takes a lot of time to end running.
I have a fasta file with around 9000 sequence lines (example below) and What my script does is:

reads the first line (ignores lines start with >) and makes 6mers (6 character blocks)
adds these 6mers to a list
makes reverse-complement of previous 6mers (list2)
saves the line if non of the reverse-complement 6mers are in the line.
Then goes to the next line in the file, and check if it contains any of the reverse-complement 6mers (in list2). If it does, it discards it. If it does not, it saves that line, and reads all reverse complement 6-mers of the new one into the list2 - in addition to the reverse-complement 6-mers that were already there.

my file:
>seq1
TCAGATGTGTATAAGAGACAGTTATTAGCCGGTTCCAGGTATGCAGTATGAGAA
>seq2
TCAGATGTGTATAAGAGACAGCGCCTTAATGTTGTCAGATGTCGAAGGTTAGAA
>seq3
TCAGATGTGTATAAGAGACAGTGTTACAGCGAGTGTTATTCCCAAGTTGAGGAA
>seq4
TCAGATGTGTATAAGAGACAGTTACCTGGCTGCAATATGGTTTTAGAGGACGAA

and this is my code:
import sys
from Bio import SeqIO
from Bio.Seq import Seq

def hetero_dimerization():
    script = sys.argv[0]
    file1 = sys.argv[1]
    list = []
    list2 = []
    with open(file1, 'r') as file:
        for record in SeqIO.parse(file, 'fasta'):
            for i in range(len(record.seq)):
                kmer = str(record.seq[i:i + 6])
                if len(kmer) == 6:
                    list.append(kmer)
            #print(record.seq)
            #print(list)

            for kmers in list:
                C_kmer = Seq(kmers).complement()
                list2.append(C_kmer[::-1])
            #print(list2)

            cnt=0
            if any(items in record.seq for items in list2):
                cnt +=1

            if cnt == 0:
                print('>'+record.id)
                print(record.seq)

if __name__ == '__main__':
    hetero_dimerization()

Here is your original code with comments on which lines should be revised for clarity
import sys
from Bio import SeqIO
from Bio.Seq import Seq

def hetero_dimerization():
    script = sys.argv[0]  # script is unused; the line should be removed
    file1 = sys.argv[1]
    list = []  # list is a python built-in python; overwrite it at your own peril. I'd suggest renaming this variable
    list2 = []
    with open(file1, 'r') as file:
        for record in SeqIO.parse(file, 'fasta'):
            for i in range(len(record.seq)):
                kmer = str(record.seq[i:i + 6])
                if len(kmer) == 6:  # this check is redundant. kmer is guaranteed to be of length 6
                    list.append(kmer)
            #print(record.seq)  # if you submit code for review, avoid commented out source code. it is confusing
            #print(list)  # same here, avoid commented out code

            # this is a bug. You add every kmer to the list even if you discard the sequence
            # you also add redundant ones, which causes considerable slowdown
            for kmers in list:
                C_kmer = Seq(kmers).complement()
                list2.append(C_kmer[::-1])
            #print(list2)  # avoid commented out code

            # adding a counter for a simple if is not very clean
            # instead consider `if not any(...):`
            cnt=0
            if any(items in record.seq for items in list2):
                cnt +=1

            if cnt == 0:
                print('>'+record.id)
                print(record.seq)

if __name__ == '__main__':
    hetero_dimerization()

Your code contains a bug (@danzel already pointed that out), as you append the list if inverse complements for every 6mer you encounter, irrespective of it being from a sequence you wish to discard and irrespective of it being already contained in the list. You've fixed the bug in your updated code and in the process also changed the algorithm to avoid a lot of unnecessary work.
You can slightly (~15%) improve this further by (a) switching the complement list to a set and searching the set for each 6mer in the sequence (instead of the other way around) and by stopping the search early (breaking) if you encounter a 6mer that is already in the list. I also took the liberty to python-ify your code a bit in the process.
def kmer_generator(record, k=6):
    sequence = record.seq
    for idx in range(len(sequence) - (k-1)):
        yield sequence[idx:(idx+6)]

def hetero_dimerization_optimized():
    taboo_list = set()
    resulting_sequences = list()
    with open("fasta-file", 'r') as file:
        for record in SeqIO.parse(file, 'fasta'):
            new_taboo_items = set()
            for kmer in kmer_generator(record):
                if str(kmer) in taboo_list:
                    break
                complement = str(kmer.reverse_complement())
                new_taboo_items.add(complement)
            else:
                # this is entered only if no break occurs
                # i.e. if no kmer was in the taboo_list :)
                taboo_list = taboo_list.union(new_taboo_items)
                resulting_sequences.append(record.seq)
    return resulting_sequences

From here, your most noticable speedups would come from (a) writing your own reverse_complement function that reduces constant overhead (~50% of the runtime is spend on this line), (b) switching to numpy and using chararray to get access to np.view and avoid further copies of your data; this is in addition to (a) and could save you an additional 20%+ runtime.
After this, you could look into multiprocessing to use additional cores to pre-filter sequences, but tbh. this will likely be over-engieered for a mere 9000 sequences.


Refactoring with many duplicates using Bash I have a curl based working bash script, but I have many duplicates on the "CURL" body.

How do I improve it efficiently to eliminate duplications?

if ! npm test
    then
      # Tell the human about the failure, if the npm output wasn't enough
      echo >&2 "Testing failed"
      curl -X POST -H 'Content-type: application/json' --data '{"text":"Tests failed for Platform: '"$PLATFORM"'\nEnvironment: '"$ENVIRONMENT"'"}' https://hooks.slack.com/services/rr/qq/pp

      # Exit and tell computers about the failure (0 = success, 1+ = failure)
      echo "A test Failed"
      exit 1

    else

      echo "Tests passed"
      curl -X POST -H 'Content-type: application/json' --data '{"text":"Tests passed for Platform: '"$PLATFORM"'\nEnvironment: '"$ENVIRONMENT"'"}' https://hooks.slack.com/services/rr/qq/pp

    fi

Use a function with a parameter for the variable part to eliminate duplication:

post_report() {
  local result=$1
  curl -X POST -H 'Content-type: application/json' \
      --data '{"text":"Tests '"$result"' for Platform: '"$PLATFORM"'\nEnvironment: '"$ENVIRONMENT"'"}' \
      https://hooks.slack.com/services/rr/qq/pp
}

if ! npm test
then
  echo >&2 "Testing failed"
  post_report failed
  exit 1
else
  echo "Tests passed"
  post_report passed
fi


Go program to find duplicated files in a directory (recursively) This is my first Go program. I'm learning the language but it's a bit difficult to understand all the concepts so in order to practice I wrote this. It's a simple program which recursively check for duplicated files in a directory.

It uses a SHA256 hash on files in order to identify if two files are the same or not. I spawn multiple workers to handle this hashing.

Here is how it works:

n workers (goroutine) are spawned, each of them waiting for file paths to process on the same channel, named input in my code.
1 goroutine is spawned to recursively search for files in the direvtory, and populate the input channel with file names.
The main goroutine process the results as soon as they are available and add them to a map of sha256->[file, file, ...].

Finally we just display the duplicates.

Please feel to comment on anything, I really want to progress in Go, and especially "idiomatic" Go.

EDIT: Improved my initial code with flags and error management.

package main

import (
    "crypto/sha256"
    "encoding/hex"
    "fmt"
    "os"
    "path/filepath"
    "sync"
    "flag"
    "runtime"
    "io"
)

var dir string
var workers int

type Result struct {
    file   string
    sha256 [32]byte
}

func worker(input chan string, results chan<- *Result, wg *sync.WaitGroup) {
    for file := range input {
        var h = sha256.New()
        var sum [32]byte
        f, err := os.Open(file)
        if err != nil {
            fmt.Fprintln(os.Stderr, err)
            continue
        }
        if _, err = io.Copy(h, f); err != nil {
            fmt.Fprintln(os.Stderr, err)
            f.Close()
            continue
        }
        f.Close()
        copy(sum[:], h.Sum(nil))
        results <- &Result{
            file:   file,
            sha256: sum,
        }
    }
    wg.Done()
}

func search(input chan string) {
    filepath.Walk(dir, func(path string, info os.FileInfo, err error) error {
        if err != nil {
            fmt.Fprintln(os.Stderr, err)
        } else if info.Mode().IsRegular() {
            input <- path
        }
        return nil
    })
    close(input)
}

func main() {

    flag.StringVar(&dir, "dir", ".", "directory to search")
    flag.IntVar(&workers, "workers", runtime.NumCPU(), "number of workers")
    flag.Parse()

    fmt.Printf("Searching in %s using %d workers...\n", dir, workers)

    input := make(chan string)
    results := make(chan *Result)

    wg := sync.WaitGroup{}
    wg.Add(workers)

    for i := 0; i < workers; i++ {
        go worker(input, results, &wg)
    }

    go search(input)
    go func() {
        wg.Wait()
        close(results)
    }()

    counter := make(map[[32]byte][]string)
    for result := range results {
        counter[result.sha256] = append(counter[result.sha256], result.file)
    }

    for sha, files := range counter {
        if len(files) > 1 {
            fmt.Printf("Found %d duplicates for %s: \n", len(files), hex.EncodeToString(sha[:]))
            for _, f := range files {
                fmt.Println("-> ", f)
            }
        }
    }

}

1. declare all 'var' at once

instead of

var dir string
var workers int

you can do

var (
    dir string
    workers int
)

or even better, use local var instead of global var irectly in your main() function

dir := flag.String("dir", ".", "directory to search")
workers := flag.Int("workers", runtime.NumCPU(), "number of workers")

2. Make sure that arguments are valid

if worker is <= 0, the program will panic. A little check after flag.Parse() could prevent this:

if workers <= 0 {
    fmt.Printf("workers has to be > 0, was %d", workers)
}

3. Improve hash computing:

First, each worker only need a single instance of hash.Hash, as you can call Reset() on it after each file:

h := sha256.New()
for file := range input {
    ...
    results <- &Result{...}
    h.Reset()
}

Also, the hash of each file could be stored as a string instead of a [32]byte to avoid some operations:

results <- &Result{
    file:   file,
    sha256: fmt.Sprintf("%x", h.Sum(nil)),
}

4. Always specify the channel direction when you can

From the golang specifications:

  A channel provides a mechanism for concurrently executing functions
  to communicate by sending and receiving values of a specified element
  type. The value of an uninitialized channel is nil.

  ChannelType = ( "chan" | "chan" "<-" | "<-" "chan" ) ElementType .

  The optional <- operator specifies the channel direction, send or
  receive. If no direction is given, the channel is bidirectional. A
  channel may be constrained only to send or only to receive by
  conversion or assignment.

Specify the channel direction helps understand what a method is doing

Here is the new version of the code:

package main

import (
    "crypto/sha256"
    "flag"
    "fmt"
    "io"
    "os"
    "path/filepath"
    "runtime"
    "sync"
)

type Result struct {
    file   string
    sha256 string
}

func search(dir string, input chan<- string) {
    filepath.Walk(dir, func(path string, info os.FileInfo, err error) error {
        if err != nil {
            fmt.Fprintln(os.Stderr, err)
        } else if info.Mode().IsRegular() {
            input <- path
        }
        return nil
    })
    close(input)
}

func startWorker(input <-chan string, results chan<- *Result, wg *sync.WaitGroup) {
    h := sha256.New()
    for file := range input {
        f, err := os.Open("file.txt")
        if err != nil {
            fmt.Fprintln(os.Stderr, err)
            continue
        }
        if _, err := io.Copy(h, f); err != nil {
            fmt.Fprintln(os.Stderr, err)
            f.Close()
            continue
        }
        f.Close()
        results <- &Result{
            file:   file,
            sha256: fmt.Sprintf("%x", h.Sum(nil)),
        }
        h.Reset()
    }
    wg.Done()
}

func run(dir string, workers int) (map[string][]string, error) {

    input := make(chan string)
    go search(dir, input)

    counter := make(map[string][]string)
    results := make(chan *Result)
    go func() {
        for r := range results {
            counter[r.sha256] = append(counter[r.sha256], r.file)
        }
    }()

    var wg sync.WaitGroup
    wg.Add(workers)
    for i := 0; i < workers; i++ {
        go startWorker(input, results, &wg)
    }
    wg.Wait()
    close(results)

    return counter, nil
}

func main() {

    dir := flag.String("dir", ".", "directory to search")
    workers := flag.Int("workers", runtime.NumCPU(), "number of workers")
    flag.Parse()

    if *workers <= 0 {
        fmt.Printf("workers has to be > 0, was %d \n", workers)
        os.Exit(1)
    }
    fmt.Printf("Searching in %s using %d workers...\n", *dir, *workers)

    counter, err := run(*dir, *workers)
    if err != nil {
        fmt.Printf("failed! %v\n", err)
        os.Exit(1)
    }

    for sha, files := range counter {
        if len(files) > 1 {
            fmt.Printf("Found %d duplicates for %v: \n", len(files), sha)
            for _, f := range files {
                fmt.Println("-> ", f)
            }
        }
    }
}

Possible improvements:

Currently, if an error is thrown somewhere in the code, the program does not stop but just write the error to os.Stderr. It may be better to return this error and then call os.Exit(1)


Generating unique names for things like filenames There are systems that when you duplicate an asset, the new assets are created with an index, so that the name do not collide. For instance, if I have an asset called House, and I press "Duplicate", a new asset will be created called House (1), and if I press again, it will be called House (2)
I wanted to make a system to duplicate that behavior, but I'm not sure if I am making it too complicated, or faulty. I'm also scared of infinite loops.
How would you improve this?
    public class NameGenerator
    {
        private readonly string baseName;

        private readonly Func<string, bool> isValid;

        public NameGenerator(string baseName, Func<string, bool> isValid)
        {
            this.baseName = baseName;
            this.isValid = isValid;
        }

        public string Generate()
        {
            if (isValid(baseName)) return baseName;

            for (var i = 0; ; i++)
            {
                var candidate = $"{baseName} ({i})";
                if (isValid(candidate)) return candidate;
            }
        }
    }
```

Potential infinite loop
If the isValid method is faulty, i.e always returns false, we'd have an infinite loop on our hands. How do we fix it? I'd fix it in coding in some (configurable but not necessary) upper limit, say 255, so if we tried 255 times, and it still fails, just throw an exception. This could either be done using a public property with a default value, or an optional constructor parameter, then all you'd need to do is check whether i is less than that upper bound.

Naming, specifically underscores
Technically, there's nothing wrong with your naming right now, as the Microsoft Naming Guidelines is outdated (from 2008) and not complete at all. And this Stack Overflow Q&A states that you shouldn't use underscores, and instead use this to differentiate between members and parameters (as you are right now). But even that is from 2012, and looking at some of Microsoft's own code, like this line in the ASP.NET Core repository shows that even they use an underscore prefix.

Resulting code:
public class NameGenerator
{
    private readonly string _baseName;

    private readonly Func<string, bool> _isValid;

    public int MaxAttempts {get; set;} = 255;

    public NameGenerator(string baseName, Func<string, bool> isValid)
    {
        _baseName = baseName;
        _isValid = isValid;
    }

    public string Generate()
    {
        // This is just a personal preference, keep it as one line if you want
        if (_isValid(_baseName))
            return _baseName;

        for (var i = 0; i < MaxAttempts; i++)
        {
            var candidate = $"{_baseName} ({i})";
            if (_isValid(candidate))
                return candidate;
        }
    }
}

Instead of a property to set the max attempts, you could also use an optional constructor parameter, like this:
public class NameGenerator
{
    // ...
    private readonly int _maxAttempts;

    public NameGenerator(string baseName, Func<string, bool> isValid, int maxAttempts = 255)
    {
        // ...
        _maxAttempts = maxAttempts;
    }

    // ...
}


CodedUi method which checks all cells in certain column I have the following extension method which checks all cells for values in a table column with the specified header name:

public static bool AllCellsInColumnHaveValue(this HtmlTable table, string columnName)
{
    // Find the columnindex of the specified column name
    var cell = new HtmlHeaderCell(table);
    cell.SearchProperties.Add(new PropertyExpression
        (UITestControl.PropertyNames.FriendlyName, columnName, PropertyExpressionOperator.Contains));
    var columnIndex = cell.ColumnIndex;

    // Check all cells in the column if they have a value
    foreach (var row in table.Rows)
    {
        var tableCells = row.GetChildren();

        if (string.IsNullOrWhiteSpace(tableCells[columnIndex].FriendlyName))
        {
            return false;
        }
    }

    // If we get here all rows in the column contain a value
    return true;
}

Is there a way to improve this method?

Disclaimer: Never done this UITestControl stuff

Edge case

If the searched columnName isn't contained in the header of the table I would expect the returned columnIndex to be -1 and therefor the check for IsNullOrWhiteSpace() will throw an exception.

Instead of using row.GetChildren() and then access the needed cell by index, you should better use the right tool for the job and call the GetCell(int) method.

Because an extension method can be also called directly like AllCellsInColumnHaveValue(aHtmlTable, aColumnName); you should ensure that the passed in HtmlTable tableis not null.


Is this a correct use of using protocols and delegate pattern in Swift? I am trying to grasp the concept of delegates and protocols in Swift. So I have implemented my own PlayableMedia protocol with two concrete classes BlueRayMedia and DVDMedia like so:

protocol PlayableMedia {
    func play()
    func stop()
}

class BlueRayMedia:PlayableMedia {

    func play() {
        println("BlueRayMedia is playing")
    }

    func stop() {
        println("BlueRayMedia has stopped playing")
    }

}

class DVDMedia:PlayableMedia {

    func play() {
        println("DVD is playing")
    }

    func stop() {
        println("DVD has stopped playing")
    }

}

and then in my controller I use it like this:

class DVDPlayer {

    var media:PlayableMedia // delegate property

    init(media:PlayableMedia){
        self.media = media
    }

    func didStartPlaying() {
       media.play()
    }

    func didStopPlaying() {
        media.stop()
    }

}

Then I try to use it like this:

var dvdPlayer:DVDPlayer = DVDPlayer(media: BlueRayMedia())

dvdPlayer.didStartPlaying()

Is this use of delegate pattern correct? Can this be improved?

While this code is technically fine and will work, as Flambino has pointed out, it's rather pointless and with the fact that play() is called in didStartPlaying() it's a bit confusing.  I know that's an implementation detail and doesn't have anything to do with the protocol necessarily, however, it does hint at a lack of understanding of protocols/delegates.

There's not a lot out there in the way of Swift code.  However, there is a lot out there in the way of Objective-C code.  And in Objective-C, there's heavy use of the protocol-delegate pattern as well.

The same Foundation protocol/delegate patterns exist exactly as they are in Objective-C also in Swift, it's just a different syntax.  But the method names are all basically the same, and you can definitely pick up on the very distinct naming pattern and get the idea of how protocol/delegate patterns work.

The most common protocol/delegate pattern in iOS development is that of the UITableViewDelegate and UITableViewDataSource (something similar probably exists for NSTableView in OSX).  Almost everyone who has done much of an iOS programming has at some point worked with a UITableView.

What actually makes a lot more sense in this case is for PlayableMedia to be a superclass.  BlueRayMedia and DVDMedia should be subclasses of PlayableMedia... and these objects should be delegated.

What would make a lot of sense would be something like this:

protocol PlayableMediaDelegate {
    func shouldBeginPlaying() -> Bool
    func willBeginPlaying()
    func didBeginPlaying()

    func shouldStopPlaying() -> Bool
    func willStopPlaying()
    func didStopPlaying()
}

Now the media has a delegate property, which is simply an AnyObject that conforms to the PlayableMediaDelegate protocol.

Any object can call play() on the PlayableMedia object, and then the play method would actually look something like this:

func play() {
    if delegate.shouldBeginPlaying() {
        delegate.willBeginPlaying()
        println("PlayableMedia is now playing...")
        delegate.didBeginPlaying()
    }
}

And stop() would look something like this:

func stop() {
    if delegate.shouldStopPlaying() {
        delegate.willStopPlaying()
        println("PlayableMedia has stopped playing...")
        delegate.didStopPlaying()
    }
}

Where in both cases the println() code is replaced with code that would actually start/stop the playable media from playing.

The point here is with these delegate methods, we let ANY OBJECT delegate our playable media.  The shouldBeginPlaying() and shouldStopPlaying() methods give our delegate a chance to prevent/allow that action from happening.  If the delegate returns false from this method, the action isn't taken.

The other methods just let our delegate know about the life cycle of the media starting/stopping.

Some examples of things that you might want to do in these delegate methods...

In shouldBeginPlaying(), you may check the media's age restrictions and decide not to play the media.  You may also check the device battery life, etc.  Any condition in which you might not want to play the media, your delegate can just return false from this method to prevent the action from happening.

In willStopPlaying(), the delegate may want to save the current position in the media so that it can be resumed later.  The PlayableMedia class probably shouldn't necessarily do keep track of where it left of last time.

In willBeginPlaying(), you might want to call some methods on the PlayableMedia object to finish setting it up for getting played.  Perhaps moving to the point in which was last left off.

The main idea here is that we're writing a class that we want to allow delegation by any sort of unknown object.  What you shouldn't do when designing a protocol is start by writing the class you intend to use as the delegate, then write a protocol to match that class, then write the class that needs to be delegated by the first class--that's backwards.


Outputting a logo from the Wordpress Customiser I managed to solve a little problem I had myself by reading and modifying.
My code works, but as a novice I would like to ask if you could advise me of a cleaner way to achieve the same thing.

Things look a bit messy to me, so I'm sure there is probably a better way to do this.

This code simply outputs a logo from the Wordpress Customiser.

Any advice or direction will be greatly appreciated.

<?php
             $custom_logo_id = get_theme_mod('custom_logo');
             $logo = wp_get_attachment_image_src($custom_logo_id, 'full');
             if (has_custom_logo()) {
                 echo '';
             } else { ?>
                 <img src='<?php echo get_template_directory_uri(); ?>/img/logo.png'>
<?php }
 ?>

Here are my  suggestions:

There is no point of "fetching" the image url if has_custom_logo returns false.

Instead, I would define $logo with a default value, then, if has_custom_logo is true, I would just rewrite it's value.

$logo = get_template_directory_uri() . '/img/logo.png';

if(has_custom_logo()){
  $logos = wp_get_attachment_image_src(get_theme_mod('custom_logo'), 'full');
  $logo  = reset($logos);
}

echo sprintf('<img src="%s" />', esc_url($logo));

I'm using reset method, it

  rewinds array's internal pointer to the first element and returns the
  value of the first array element.

You can convert this to a method and use it in multiple places.