Generic java configuration class with type safety In the first code snippet I am creating a generic class to hold configuration details.  I am currently using it as a way to pass run time configuration options to plugins in a generic manner.  The second snippet is a helper class that utilizes the class to provide the public methods for use which helps guarantee type safety.

Questions

How is my use of generics?  I feel like there is a better way to handle this.
Is there a better design pattern to be used here?

DefaultConfiguration

import java.util.HashMap;
import java.util.Map;

public class DefaultConfiguration implements Configuration {
    private final Map<String, Setting> options;

    public DefaultConfiguration() {
        this.options = new HashMap<String, Setting>();
    }

    @Override
    public <T> T get(String option, Class<T> type) {
        Setting setting = options.get(option);
        if (setting != null) {
            return type.cast(setting.getSetting(type));
        }
        return null;
    }

    @Override
    public <T> void set(String option, T setting, Class<T> type) {
        options.put(option, new Setting<T>(setting, type));
    }

    @Override
    public void set(String option, Object setting) {
        options.put(option, new Setting(setting, setting.getClass()));
    }

    class Setting<W> {
        private final Class<? super W> type;
        private final W setting;

        public Setting(W setting, Class<? super W> type) {
            this.type = type;
            this.setting = setting;
        }

        public W getSetting(Class<? super W> type) {
            if (type.isAssignableFrom(this.type)) {
                return setting;
            }
            return null;
        }
    }
}

MyConfiguration

public class MyConfiguration extends DefaultConfiguration {
    public String getSettingA() {
        return super.get("settingA", String.class);
    }

    public void setSettingA(String settingA) {
        super.set("settingA", settingA, String.class);
    }

    public Integer getSettingB() {
        return super.get("settingB", Integer.class);
    }

    public void setSettingB() {
        super.set("settingB", settingB, Integer.class);
    }
}

Updated Classes

import java.util.HashMap;

public class Configuration
    extends HashMap<String, Object>
{
    /**
     *
     */
    private static final long serialVersionUID = 1L;

    @Override
    public <T> T get(String option, Class<T> type)
    {
        Object obj = get(option);
        if (obj == null) {
            return null;
        }
        try {
            return type.cast(obj);
        } catch (ClassCastException ex) {
            throw new ConfigurationException(ex);
        }
    }
}

public class MyConfiguration
    extends Configuration
{
    /**
     *
     */
    private static final long serialVersionUID = 1L;

    private static final String SETTING_A = "settingA";
    private static final String SETTING_B = "settingB";

    public String getSettingA()
    {
        return get(SETTING_A, String.class);
    }

    public void setSettingA(String settingA)
    {
        put(SETTING_A, settingA);
    }

    public Integer getSettingB()
    {
        return get(SETTING_B, Integer.class);
    }

    public void setSettingB()
    {
        put(SETTING_B, settingB);
    }
}

Generics can only go so far to help you, and, this is one of those times where it just does not help all that much.

In essence, you have the following:

a configuration system where you can get values based on names (a Map).
when you set a value it is for a given type (which is either specified when you set the value, or it is not, in which case it is inferred).
when you get a value, it is matched against the type the getter expects, and, if it matches, it is returned.

Since the getter/setter is the only place where you actually know the type of the value, there is actually zero benefit for your entire Setting subclass... it is redundant. You can get the same behaviour with:

private final Map<String, Object> options;

public DefaultConfiguration() {
    this.options = new HashMap<String, Object>();
}

@Override
public <T> T get(String option, Class<T> type) {
    Object value = options.get(option);
    if (value != null) {
        return type.cast(value);
    }
    return null;
}

@Override
public <T> void set(String option, T setting, Class<T> type) {
    // no point in even checking the type, the generic method will enforce that.
    // this method is essentially redundant, and could be
    //   public void set(String option, Object value);
    options.put(option, setting);
}

@Override
public void set(String option, Object setting) {
    // this essentially a duplicate option.
    options.put(option, setting);
}

here you have the exact same functionality as you did before.... if the user wants to, they can compile-time type-check their values when they set them (by ensusuring the Class<T> parameter matches the T setting value

When they retrieve the value, they can cast the result to the specified type, or return null if it is not castable.

The functionality in MyConfiguration is unchanged, but, I recommend you use constants for the keys:

private static final String SETTINGA = "settingA";
....

Finally, you may want to consider your DefaultConfiguration extending something like HashMap directly.... like:

public class DefaultConfiguration
          extends HashMap<String,Object>
          implements Configuration {

then, remove the options Map variable, and simply have methods like (note the direct get(option) instead of options.get(option):

@Override
public <T> T get(String option, Class<T> type) {
    Setting setting = get(option);
    if (setting != null) {
        return type.cast(setting.getSetting(type));
    }
    return null;
}

Now, when people use your class, they have access directly to the configuration too (with, and without your 'helper' methods).

The bottom line though, is that your implementation does not enforce type-safety between the set and the get. The only thing it does is return null instead of throwing a ClassCastException when people use your code wrong.


AES encryption in PHP PHP doesn't have a built-in functions for AES (specifically, AES-128) encoding/decoding, so I've had to implement my own, and this is what I have come up to (of course, taken from different many sources, mostly not coded by me).

My question is, are these correct? Not the code itself, but what they do. The correctness of the algorithm. I have tested them, and they apparently do work. But there might be a subtle error that I'm overlooking...

Encoding algorithm:

function aes128_encode($data, $mode)
{
        switch ($mode) {
        case "ECB":
        case "CBC":
            if ($mode === "ECB") {
                $cipher = mcrypt_module_open(MCRYPT_RIJNDAEL_128, '', MCRYPT_MODE_ECB, '');
            } else {
                $cipher = mcrypt_module_open(MCRYPT_RIJNDAEL_128, '', MCRYPT_MODE_CBC, '');
            }

            $iv = mcrypt_create_iv(mcrypt_enc_get_iv_size($cipher), MCRYPT_RAND);
            $key = substr(CIPHERKEY, 0, mcrypt_enc_get_key_size($cipher));

            if (mcrypt_generic_init($cipher, $key, $iv) != 1) {
                $cipherData = mcrypt_generic($cipher, $data);

                mcrypt_generic_deinit($cipher);
                mcrypt_module_close($cipher);

                if ($mode === "ECB") {
                    $sanitizedCipherData = trim(base64_encode($cipherData));
                } else {
                    $sanitizedCipherData = trim(base64_encode($iv)."_".base64_encode($cipherData));
                }

                return $sanitizedCipherData;
            } else {
                return false;
            }
            break;

        default:
            return false;
            break;
        }
    }

Decoding algorithm:

function aes128_decode($data, $mode)
{
    switch ($mode) {
    case "ECB":
        $cipher = mcrypt_module_open(MCRYPT_RIJNDAEL_128, '', MCRYPT_MODE_ECB, '');
        $key = substr(CIPHERKEY, 0, mcrypt_enc_get_key_size($cipher));

        // Fake iv to call mcrypt_generic_init
        $iv = mcrypt_create_iv(mcrypt_enc_get_iv_size($cipher), MCRYPT_RAND);
        $cipherData = base64_decode($data);

        if (mcrypt_generic_init($cipher, $key, $iv) != -1) {
            $originalData = mdecrypt_generic($cipher, $cipherData);
            mcrypt_generic_deinit($cipher);
            mcrypt_module_close($cipher);

            return $originalData;
        } else {
            return false;
        }
        break;

    case "CBC":
        $cipher = mcrypt_module_open(MCRYPT_RIJNDAEL_128, '', MCRYPT_MODE_CBC, '');
        $key = substr(CIPHERKEY, 0, mcrypt_enc_get_key_size($cipher));
        $parts = explode("_", $data);
        $iv = base64_decode($parts[0]);
        $cipherData = base64_decode($parts[1]);

        if (mcrypt_generic_init($cipher, $key, $iv) != -1) {
            $originalData = mdecrypt_generic($cipher, $cipherData);
            mcrypt_generic_deinit($cipher);
            mcrypt_module_close($cipher);

            return $originalData;
        } else {
            return false;
        }
        break;

    default:
        return false;
        break;
    }
}

NOTE: The $mode variable indicates which mode of operation will be used (ECB or CBC). The CIPHERKEY is a constant with a randomized value that should be only known by the admin of the website.

Implementing AES alone is very risky, mainly because it is very easy to make a mistake. I strongly recommend you use phpseclib for this.

Also you should not use ECB mode unless you are encrypting only 1 block.


jQuery redirect form I'm using jQuery to create a (hidden) form that redirects people to another page using JavaScript.

// redirect the user to another url
function redirect(options) {
  // create the form
  $("<form>").attr({
    "hidden": true, "action": options.url, "target": (options.new_tab === true) ? "_blank" : "_self"
  // create the query inputs and append them to the form
  }).append($.map(options.queries || {}, function(name, value) {
    return $("<input>").attr({"name": name, "value": value, "hidden": true})[0];
  // append the form to the body and submit it
  })).appendTo("body").submit();
}

To simplify this question the form looks like this when broken down:

// redirect the user to another url
function redirect(options) {
  // create the form
  var form = $("<form>").attr({
    "hidden": true,
    "action": options.url,
    "target": (options.new_tab === true) ? "_blank" : "_self"
  });

  // create the query inputs and append them to the form
  form.append($.map(options.queries || {}, function(name, value) {
    return $("<input>").attr({
      "name": name,
      "value": value,
      "hidden": true
    })[0];
  }));

  // append the form to the body and submit it
  form.appendTo("body").submit();
}

How can I simplify the second part of this code?

// create the query inputs and append them to the form
form.append($.map(options.queries || {}, function(name, value) {
  return $("<input>").attr({
    "name": name,
    "value": value,
    "hidden": true
  })[0];
}));

You shouldn't need to post (or even have) two versions of the same source code. You should only have one readable version, and use a compressor to create a production version.

And IMO it would be fine to use method chaining in the second (readable) version.

Concerning your question: I think the whole code is fine as it is. I'll list some points that one may differently in the second block, but I don't believe they are really needed:

You could avoid the || {} by validating the options first, or before calling this method, or simply not allow the queries property to be undefined.

You could use an arrow function (if your target systems support it, or by using a ES6 compiler):

form.append($.map(options.queries, (name, value) =>
  $("<input>").attr({
    "name": name,
    "value": value,
    "hidden": true
  })[0]
));

And you could use the native JavaScript and more light weight Array.map() method:

form.append(Object.keys(options.queries).map((key, index) =>
  $("<input>").attr({
    "name": key,
    "value": options.queries[key],
    "hidden": true
  })[0]
));

Expanding on the last point: Now-a-days it's worth considering, if you still need jQuery at all. It was once a big help to bridge incompatibilities between browsers, but current browsers almost compatible to each other.

EDIT: Here's a version without jQuery:

function redirect(options) {
    let form = document.createElement("form");
    form.setAttribute("hidden", "");
    form.setAttribute("action", options.url);
    form.setAttribute("target", (options.new_tab === true) ? "_blank" : "_self");

    Object.entries(options.queries || {}).forEach(([name, value]) => {
        let input = document.createElement("input");
        input.setAttribute("hidden", "");
        input.setAttribute("name", name);
        input.setAttribute("value", value);
        form.appendChild(input);
    });

    document.body.appendChild(form);
    form.submit();
}


Finding duplicate strings within a file This code basically finds duplicate strings within a file.

  An example: DUPLICATE_LENGTH set to 6, file contains:

process hash michael honeycomb interrupt system call deadlock scheduling michael

  The output will be michael, as its a duplicate with a length of 6 or
  higher

The following code shows my approach to solving this issue.  If there are completely different ideas how to solve it, I'm open too, but primarily I'd like to get some feedback of the code I wrote.

I'm also searching performance optimizations as it gets kinda slow on large files.

import codecs

# Constants
DUPLICATE_LENGTH = 30;
FILE_NAME = "data.txt"
SHOW_RESULTS_WHILE_PROCESSING = True

def showDuplicate(duplicate):
    print("Duplicate found:{0}".format(duplicate))

fileHandle = codecs.open(FILE_NAME, "r", "utf-8-sig")
fileContent = fileHandle.read()
fileHandle.close()

substringList = []  #contains all possible duplicates.
duplicatesList = [] #contains all duplicates

for i in range(0, len(fileContent) - DUPLICATE_LENGTH):
    end = i + DUPLICATE_LENGTH
    duplicate = fileContent[i:end]
    if duplicate in substringList and '|' not in duplicate:
        duplicatesList.append(duplicate)
    else:
        substringList.append(duplicate)

resultList = []
currentMatch = duplicatesList[0]
currentMatchPos = 1
for i in range(1, len(duplicatesList)):
    if currentMatch[currentMatchPos:] in duplicatesList[i]:
        currentMatch += duplicatesList[i][-1]
        currentMatchPos += 1
    else:
        if SHOW_RESULTS_WHILE_PROCESSING:
            showDuplicate(currentMatch)

        resultList.append(currentMatch) # this match is closed, add it!
        currentMatch = duplicatesList[i]
        currentMatchPos = 1

if not SHOW_RESULTS_WHILE_PROCESSING:
    for duplicate in resultList:
        showDuplicate(duplicate)

if not resultList:
    print "Awesome, no duplicates were found!"

Firstly, your code doesn't actually work. Run it against your example data and DUPLICATE_LENGTH. It has no results. So I'm going to look at your code stylistically and ignore the algorithm for now:

import codecs

# Constants
DUPLICATE_LENGTH = 30;

No need for that semicolon

FILE_NAME = "data.txt"
SHOW_RESULTS_WHILE_PROCESSING = True

def showDuplicate(duplicate):

python style guide recommends seperation_by_underscores for function names

    print("Duplicate found:{0}".format(duplicate))

fileHandle = codecs.open(FILE_NAME, "r", "utf-8-sig")
fileContent = fileHandle.read()
fileHandle.close()

Python style guide recommends words_seperated_by_underscores for variable names.

substringList = []  #contains all possible duplicates.
duplicatesList = [] #contains all duplicates

for i in range(0, len(fileContent) - DUPLICATE_LENGTH):

No need for the 0, range(x) == range(0, x)

    end = i + DUPLICATE_LENGTH
    duplicate = fileContent[i:end]

I'd combine those two lines. I'd also not call it a duplicate as its not a duplicate, just a substring.

    if duplicate in substringList and '|' not in duplicate:

I not clear what significance the |  has

        duplicatesList.append(duplicate)
    else:
        substringList.append(duplicate)

resultList = []
currentMatch = duplicatesList[0]
currentMatchPos = 1

Any sort of complex logic should really be in  a function, not at the module level.

for i in range(1, len(duplicatesList)):

Do you really need the indexes? Maybe you should use for duplicate in duplicatesList[1:]

    if currentMatch[currentMatchPos:] in duplicatesList[i]:
        currentMatch += duplicatesList[i][-1]
        currentMatchPos += 1
    else:
        if SHOW_RESULTS_WHILE_PROCESSING:
            showDuplicate(currentMatch)

        resultList.append(currentMatch) # this match is closed, add it!
        currentMatch = duplicatesList[i]
        currentMatchPos = 1

I dislike the structure of this code. You are manipulating several variables across loop iterations which makes the code hard to follow. But since the algorithm is simply incorrect for what you are doing I can't really tell you how to fix it.

if not SHOW_RESULTS_WHILE_PROCESSING:
    for duplicate in resultList:
        showDuplicate(duplicate)

if not resultList:
    print "Awesome, no duplicates were found!"

Ok, now for the actual algorithm. I think wilberforce has misinterpreted what you want. His algorithm requires the duplication to be aligned, which I don't think you want.

The simple brute force strategy is as follows:

# look at every position where the duplicates could start
for x_idx in range(len(file_content)):
    for y_idx in range(x_idx):
        # count the length of the duplicate
        for length, (x_letter, y_letter) in enumerate(zip(file_content[x_idx:], file_content[y_idx:])):
            if x_letter != y_letter:
                break
        # if its good enough, print it
        if length > DUPLICATE_LENGTH:
            showDuplicate(file_content[x_idx:x_idx + length])

A more efficient and clever algorithm is to use dynamic programming:

# we create 2D list (list of lists) where lengths[x][y] will be length
# of the duplicate which ends on letter x-1 and y-1 in the file content
# so we will calculate the length of duplicate for every combination of two positions
file_size = len(file_content) + 1
lengths = [ [None] * file_size for x in range(file_size) ]

# If we are before the characters, the length of the duplicate string is
# obviously zero
for x in range(file_size):
    lengths[x][0] = 0
    lengths[0][x] = 0

for x in range(1, file_size):
    # we don't check cases where x == y or y > x
    # x == y is trivial and y > x is just
    # the transpose
    for y in range(1, x):
        if file_content[x-1] == file_content[y-1]:
            # if two letters match, the duplicate length
            # is 1 plus whatever we had before
            duplicate_length = lengths[x][y] = lengths[x-1][y-1] + 1
            if duplicate_length > DUPLICATE_LENGTH:
                showDuplicate(file_content[x-duplicate_length:x])
        else:
            # if the files don't match the duplicate ends here
            lengths[x][y] = 0

My two programs produce slightly different results. The first says:

Duplicate found: michael
Duplicate found:michael

The second says:

Duplicate found: michae
Duplicate found: michael

The difference is because the actual match is 7 characters long. As a result, it matches twice. I don't know what results you actually do want, so I haven't looked into it.


CommandLine StreamFormatTester Using Chris Adamson's book Learning Core Audio I'm getting familiar with Core Audio for Mac/iOS. The code examples are 100% Objective-C. I'm trying to follow along using Swift 3.

This small piece of software is used to find usable AudioStreamBasicDescriptions for a given file type and format.

import Foundation
import AudioToolbox

// Create ID for specific fileType and fileFormat
var fileTypeAndFormat = AudioFileTypeAndFormatID()
fileTypeAndFormat.mFileType = kAudioFileCAFType
fileTypeAndFormat.mFormatID = kAudioFormatLinearPCM

// Target: Gather possible AudioStreamBasicDescriptions for given fileType and fileFormat
// Get size of array containing all available AudioStreamBasicDescriptions
var possibleError = noErr
var infoSize: UInt32 = 0

possibleError = AudioFileGetGlobalInfoSize(kAudioFileGlobalInfo_AvailableStreamDescriptionsForFormat,
                                           UInt32(MemoryLayout<AudioFileTypeAndFormatID>.stride),
                                           &fileTypeAndFormat,
                                           &infoSize)
assert(possibleError == noErr)

// Get number of AudioStreamBasicDescription stored in array
let asbdCount = Int(infoSize) / MemoryLayout<AudioStreamBasicDescription>.stride

// Allocate memory for array to store AudioStreamBasicDescriptions in
var asbds = [AudioStreamBasicDescription](repeating: AudioStreamBasicDescription(), count: asbdCount)

// Fill array with all available AudioStreamBasicDescriptions
possibleError = AudioFileGetGlobalInfo(kAudioFileGlobalInfo_AvailableStreamDescriptionsForFormat,
                                       UInt32(MemoryLayout<AudioFileTypeAndFormatID>.stride),
                                       &fileTypeAndFormat,
                                       &infoSize,
                                       &asbds)
assert(possibleError == noErr)

// Print each AudioStreamBasicDescription of array
for n in 0..<asbdCount {
    var reformattedID = asbds[n].mFormatID.bigEndian

    // TODO: This seems too ugly to be true
    withUnsafePointer(to: &reformattedID) { stringIDPtr in
        print(String(format: "%d: mFormatID: %s, mFormatFlags: %d, mBitsPerChannel: %d", n, stringIDPtr,
                     asbds[n].mFormatFlags, asbds[n].mBitsPerChannel))
    }
}

In an production environment the asserts should be replaced with better error handling, but correct error handling was not the purpose of this script.

Specific parts I'd like to get a review on:

The memory allocation to store available StreamDescriptions for format in. Is the allocation via array Swift-style-conform?
Getting the mFormatID of an AudioStreamBasicDescription via withUnsafePointer(to: &reformattedID) seems utterly unpleasant. Can this be realized in a more elegant way?

The "file and format ID" can be created in one step as

var fileTypeAndFormat = AudioFileTypeAndFormatID(mFileType: kAudioFileCAFType,
                                                 mFormatID: kAudioFormatLinearPCM)

Using a Swift Array as storage is fine, and has the advantage that
the memory is managed automatically (contrary  to memory from malloc
or UnsafeMutablePointer.alloc()). The array creation can be written
a tiny be shorter as

var audioDescriptions = Array(repeating: AudioStreamBasicDescription(), count: asbdCount)

because the element type is inferred from the first argument. I have
also chosen a different name, asbds is very short and cryptic.

For-loops are often better replaced by an enumeration:

for audioDesc in audioDescriptions {
    // ...
}

or, if you need the element index as well:

for (idx, audioDesc) in audioDescriptions.enumerated() {
    // ...
}

This saves array lookups inside the loop.

Treating the "four-character code" in mFormatID as a string is
dangerous and causes undefined behaviour, because the %s format
expects a zero-terminated sequence of chars.
You can get additional unwanted output, or the program might crash.

You could use the %4.4s format instead, which limits the output
to four characters:

for (idx, audioDesc) in audioDescriptions.enumerated() {
    var reformattedID = audioDesc.mFormatID.bigEndian

    withUnsafePointer(to: &reformattedID) { stringIDPtr in
        print(String(format: "%d: mFormatID: %4.4s, mFormatFlags: %d, mBitsPerChannel: %d",
                     idx, stringIDPtr,
                     audioDesc.mFormatFlags, audioDesc.mBitsPerChannel))
    }
}

A better solution is to convert the integer to a four-character
string, e.g. using one of the methods in
How to convert memory of Int32 as four characters, for example

func fourCCToString(_ value: FourCharCode) -> String {
    let utf16 = [
        UInt16((value >> 24) & 0xFF),
        UInt16((value >> 16) & 0xFF),
        UInt16((value >> 8) & 0xFF),
        UInt16((value & 0xFF)) ]
    return String(utf16CodeUnits: utf16, count: 4)
}

which is then used as

for (idx, audioDesc) in audioDescriptions.enumerated() {
    let reformattedID = fourCCToString(audioDesc.mFormatID)

    print(String(format: "%d: mFormatID: %@, mFormatFlags: %d, mBitsPerChannel: %d",
                 idx, reformattedID,
                 audioDesc.mFormatFlags, audioDesc.mBitsPerChannel))
}

Note that a Swift string is printed with the %@ format.

Or simply use string interpolation:

for (idx, audioDesc) in audioDescriptions.enumerated() {
    let reformattedID = fourCCToString(audioDesc.mFormatID)

    print("\(idx): mFormatID: \(reformattedID), mFormatFlags: \(audioDesc.mFormatFlags), mBitsPerChannel: \(audioDesc.mBitsPerChannel)")
}


find in which sublist of a list are any value of a list in python I want to return a NumPy array of Element IDs with at least one Node being in the interesting Nodes list.
I have:

A list of interesting Nodes, Interesting_Nodes.
The size is ~300,000 items.

A list of sublists which represent Node forming an Element, Nodes.
The size is ~1,000,000 lists of ~20 values.
Nodes[i] contain the 20 Nodes creating Elements[i]. Which is an integer ID.

A list of Elements, Elements.
The size is ~1,000,000 items.

Here is an example of these lists.
import numpy as np
Interesting_Nodes=[1,2,10,40,400,1000]
Elements=[1,2,3]
Nodes=[[1,20,25],[30,400,35],[500,501,502]]

In this case the function will return [1,2] because Elements[0] have the Node 1 in the Interesting_Nodes list and Elements[1] contains the Node 400 which is also in the Interesting_Nodes list
I wrote this it seems to work but is very slow. Is there a way to perform this faster?
def recuperation_liste_element_dans_domaine_interet(Interesting_Nodes,Elements,Nodes):
    Liste_elements=list([])
    for n in Interesting_Nodes:
        id=np.where(Nodes==n)
        Liste_elements=Liste_elements+list(Elements[id[0]])
    nbrListe = list(set(Liste_elements)) # remove all the element duplication
    return np.array(nbrListe)

Another way to perform that I think could be: (be still too slow)
def recuperation_liste_element_dans_domaine_interet(Interesting_Nodes,Elements,Nodes):
    Liste_elements=[0]*len(Elements)
    compteur=0
    j=0
    for n in Nodes:
        if any(i in Interesting_Nodes for i in n):
            Liste_elements[compteur]=Elements[j]
            compteur=compteur+1
        j=j+1
    Liste_elements=Liste_elements[:compteur]
    return np.array(Liste_elements)

I'm assuming Elements is always a list of continuous indices from zero to the length of Nodes minus one and thus strictly not needed.
As a general guideline, explicit loops tend to be slow and also hard to read (and non-Pythonic).
To solve your problem efficiently, use data structures (namely sets):
import numpy as np

# Suppose you start with these lists
u = [1,2,10,40,400,1000]
n = [[1,20,25],[30,400,35],[500,501,502]]

# Convert them into sets
u = set(u)
n = [set(x) for x in n]

#  An individual bit is true if the corresponding list is "interesting"
idx = [bool(a.intersection(u)) for a in n]

# Perhaps convert to integer indices if you wish
np.where(idx)


Selecting data from a table based on value in another table (SQL server) I am using the northwind example database  to brush up my SQL server skills.

I want to select all the products ordered by the customer with the customer id 'Folko'.

My idea is to join the Orders table with the OrderDetails table on OrderID and then join the Products table with the OrderDetails table on ProductID.
Then insert the WHERE clause (i.e. where Orders.CustomerID = 'FOLKO')

Here is the query:

select Products.ProductName from [Order Details]
inner join Orders on [Order Details].OrderID = Orders.OrderID
inner join Products on [Order Details].ProductID = Products.ProductID
where Orders.CustomerID = 'FOLKO'

I have also managed to get the same information with the following query:

select Products.ProductName from [Order Details], Orders, Products
where Products.ProductID = [Order Details].ProductID
and Orders.OrderID = [Order Details].OrderID
and Orders.CustomerID = 'FOLKO'

I am getting the result that I want but I wonder if these are the preferred ways (or if one or the other is better) or if it should be done in a different way altogether?

The first query, i.e. using explicit JOINs, is generally the preferred way, and has benefits like:

verbosity
easier to read and maintain
less likely to have accidental cross joins

For more precise and detailed answers, see the answers to this question on SO (bearing in mind it is tagged mysql, it still is relevant for sql-server). Also read this related blog post by Baron Schwartz.


Object-oriented Tamagotchi-like program with a GUI I am currently working on a small python project for an undergraduate course in python. The task is to create a Tamagotchi-like program with a GUI. My question is whether my code is Object oriented or not? Here are my classes:

Tamagotchi class

class Tamagotchi(Canvas):

    def __init__(self, radie, x,y):
        super().__init__()
        self.radie = radie
        self.x=x
        self.y=y

        self.createCircle(x, y, radie, fill="black")

    def createCircle(self, x, y, radie, **kwargs):
        circle = self.create_oval(x-radie,y-radie,x+radie,y+radie,**kwargs)
        return circle

    def updateCircle(self,radie, **kwargs):
        self.delete("all")
        self.radie=radie
        self.createCircle(self.x,self.y,self.radie, fill="black")

GUI class

class GUI(Frame):
    def __init__(self):
        super().__init__()
        tamagotchi = Tamagotchi(60,140,125)
        tamagotchi.pack(side=TOP)

        activityList = []

        Style().configure("TButton", padding=(20, 20, 20, 20), font='serif 10')

        sizeLbl = Label(self, text="Storlek: ")
        sizeLbl.grid(row=0, column=3, sticky=N)

        size = Label(self, text = tamagotchi.radie, foreground="red", font=(None, 15))
        size.grid(row=0,column=3, sticky = S)

        sovaButton = Button(self, text="Sova", command = lambda: self.tamagotchiLogic("sova", activityList, tamagotchi, size))
        sovaButton.grid(row=0, column=0)

        tentaButton = Button(self, text="Tenta", command = lambda: self.tamagotchiLogic("tenta", activityList, tamagotchi, size))
        tentaButton.grid(row=0, column=1)

        festaButton = Button(self, text="Festa", command = lambda: self.tamagotchiLogic("festa", activityList, tamagotchi, size))
        festaButton.grid(row=1, column=0)

        pluggaButton = Button(self, text="Plugga", command = lambda: self.tamagotchiLogic("plugga", activityList, tamagotchi, size))
        pluggaButton.grid(row=1, column=1)

        self.centerWindow()

    def centerWindow(self):
        w = 300
        h = 393

        sw = self.master.winfo_screenwidth()
        sh = self.master.winfo_screenheight()

        x = (sw - w) / 2
        y = (sh - h) / 2
        self.master.geometry('%dx%d+%d+%d' % (w, h, x, y))

    def tamagotchiLogic(self, activity, activitylist, tamagotchi, sizeLabel):
        activitylist.append(activity)

        end = len(activitylist)
        actString = ''.join(activitylist[end-3:end])

        print(actString)

        growthCombinations = ["pluggasovatenta", "sovatentafesta"]
        declineCombinations = ["festasovatenta","tentapluggaplugga"]
        boredCombinations = ["festafestafetsa","pluggapluggaplugga","sovasovasova","tentatentatenta"]

        if actString in growthCombinations:
            tamagotchi.updateCircle(tamagotchi.radie*1.05, fill="black")

        elif actString in declineCombinations:
            tamagotchi.updateCircle(tamagotchi.radie*0.95, fill="black")

        elif actString in boredCombinations:
            tamagotchi.updateCircle(tamagotchi.radie*(1/sqrt(2)), fill="black")

        sizeLabel.config(text=round(tamagotchi.radie))

Main

def main():
    root = Tk()

    gui = GUI()
    gui.pack()

    root.mainloop()

if __name__ == '__main__':
    main()

Your code is built by using classes and consequently by creating objects(instances) of those classes. In addition you are also utilizing the concept of inheritance to associate multiple classes to each other.
So yes, your code is OOP.


Another DataGrid (ObservableCollection) Filtering Method I have re-written my DataGrid filtering method to try to improve performance and I feel that although this is the fastest yet, there is still room for improvement on some of the slower machines my program runs on.

The DataGrid is bound to an ObservableCollection, as the user enters text into my searchBox each CompanyModel that matches the filter is added into a filter ListBox. The ListBox is limited to the first 25 CompanyModels so as to enhance performance. Here is the method that is called as the user types;

private void FilterCompanies()
{
    FilteredCompanies = new ObservableCollection<CompanyModel>();
    FilteredCompanies.Clear();

    string searchText = CleanseText(searchBox.Text.ToLower());

    foreach (CompanyModel company in AllCompanies)
    {
        if (!string.IsNullOrEmpty(searchText))
        {
            if (FilteredCompanies.Count <= 25)
            {
                if (CompanyMatchesFilters(company))
                {
                    if (CleanseText(company.Name.ToLower()).Contains(searchText) ||
                        CleanseText(company.Town.ToLower()).Contains(searchText) ||
                        CleanseText(company.Postcode.ToLower()).Contains(searchText))
                    {
                        FilteredCompanies.Add(company);
                    }
                }
            }
        }
    }

    if (FilteredCompanies.Count > 0)
    {
        companiesListBox.ItemsSource = FilteredCompanies;
        companiesListBox.Visibility = Visibility.Visible;
    }
    else
    {
        companiesListBox.ItemsSource = null;
        companiesListBox.Visibility = Visibility.Collapsed;
    }
}

From the code you can see there are two more methods involved in the filtering;

CompanyMatchesFilters

private bool CompanyMatchesFilters(CompanyModel company)
{
    foreach (FilterItem item in firstListBoxItems)
    {
        if (item.ID == 1 && company.CurrentStatus != 1)
        {
            return false;
        }
        if (item.ID == 2 && company.Subcontractor != 1)
        {
            return false;
        }
        if (item.ID == 3 && company.Supplier != 1)
        {
            return false;
        }
        if (item.ID == 4 && company.Planthire != 1)
        {
            return false;
        }
        if (item.ID == 5 && company.Architect != 1)
        {
            return false;
        }
        if (item.ID == 6 && company.QS != 1)
        {
            return false;
        }
        if (item.ID == 7 && company.ProjectManager != 1)
        {
            return false;
        }
        if (item.ID == 8 && company.StructEng != 1)
        {
            return false;
        }
        if (item.ID == 9 && company.ServiceEng != 1)
        {
            return false;
        }
    }
    return true;
}

CleanseText

private static Regex badChars = new Regex("[^A-Za-z0-9']");

private string CleanseText(string text)
{
    return badChars.Replace(text, "");
}

I would be very grateful if I could have some advice on;

A: How to improve my code in general
B(maybe more importantly): How    to further improve the performance
of my methods if at all possible

You can refactor this into LINQ query for (arguably) better readability.

AllCompanies.Where(c => CompanyMatchesFilters(c))
            .Where(c => CleanseText(company.Name).Contains(searchText)
                     || CleanseText(company.Town).Contains(searchText)
                     || CleanseText(company.Postcode).Contains(searchText))
            .Take(25);

As for performance - nothing in the code you've shown strikes me as potential performance bottleneck. You will probably have to run a profiler to pinpoint it. One thing in particular you might want to look at is what triggers the search and how often it is executed. For example, if I enter 10 chars in search box, do I search 1 or 10 times? While the filtering logic looks fairly lightweight, the performance implications of recreating 25 item templates in UI on every keystroke can be severe (depending on how complex the DataTemplate is). Rx has some nice extensions that might help in throttling UI events.

P.S. Whats the point of removing all meaningful characters in CleanseText? What characters are you interested in then? This is extremely unclear, you should document that portion of your code.


Implementing RadixSort using JavaScript Can someone review my code? I am trying to implement radix sort in JavaScript.

var numbers = [90, 46, 7, 12, 100, 68];
var length = numbers.length;
var buckets = new Array(10);

Clear();
function Clear() {
    for (var i = 0; i < 10; i++)
        buckets[i] = [];
}

RadixSort();
function RadixSort() {
    var maxElement = Math.max.apply(null , numbers);
    var maxDigits = GetDigitsCount(maxElement);
    var count = 0;
    while (count < maxDigits) {
        CopyToArray();
        for (var index = 0; index < length; index++) {
            var tenIndex = Math.pow(10, count);
            var remNumber = parseInt(numbers[index] / tenIndex, 10);
            var bucketIndex = remNumber % 10;
            buckets[bucketIndex].push(numbers[index]);
        }
        count++;
    }
    for (var i = 0; i < 9; i++) {
        var arr = buckets[i];
        var arrLength = arr.length;
        for (var j = 0; j < arrLength; j++)
            console.log(arr[j]);
    }
}

function GetDigitsCount(number) {
    var count = 0;
    while (number > 0) {
        number = parseInt(number / 10);
        count++;
    }
    return count;
}

function CopyToArray() {
    var k = 0;
    for (var i = 0; i < 10; i++) {
        var arr = buckets[i];
        var arrLength = arr.length;
        for (var j = 0; j < arrLength; j++) {
            numbers[k++] = arr[j];
        }
        buckets[i] = [];
    }
}

Your code is not finished.

I could not say this is a finished bit of code as it simply does not work. A simple test, a random set of integers and comparing it to the result of the standard sort showed that more often than not the function just failed.

Read this Gramma and types

3 rules of coding:

There is no code without testing.
There is no code without testing.
There is no code without testing.

When creating a function such as this it is important that you have a reliable means of testing the code over all possible inputs. One test case just does not cut it. Having a full test at the start makes writing the code a lot easier. You can make a change and instantly test all possible inputs, it also helps you optimise the code once you have the basics worked out.

Example test function

This is the function that I used to test your code. It creates a random  list of numbers of different lengths, different numbers of digits, and different ranges of digits. If it fails any test then it stops displaying the failed array and the correct array.

function test() {
    function log (data) {console.log(data); }
    function createRandomArray (maxItems, maxDigits, digitRange) {
        var numSize, num;
        var numbers = [];
        var count = Math.random() * maxItems;
        while (count-- > 0) {
            numSize = Math.random() * maxDigits;
            num = 0;
            while (numSize-- > 0) {
                num *= 10;
                num += Math.floor(Math.random() * digitRange);
            }
            numbers.push(num);
        }
        return numbers;
    }
    var items, digits, range, array;
    for (items = 0; items <= 20; items += 1) {
        for (digits = 1; digits <= 9; digits += 1) {
            for (range = 1; range <= 9; range += 1) {
                array = createRandomArray(items, digits, range);
                length = array.length;
                numbers = [...array];
                RadixSort();
                array.sort((a, b) => a - b);
                if (array.join(",") !== numbers.join(",")) {
                    log("==============================");
                    log("Test failed.");
                    log("Items = "+length)
                    log("Max digits = "+digits)
                    log("Digit range = "+range)
                    log("- Result --------------------")
                    numbers.forEach(n => log(n))
                    log("- Expected ------------------")
                    array.forEach(n => log(n) )
                    return
                }
            }
        }
    }
    log("All passed")
}
test();

Style and syntax.

Only capitalise variables if they are functions that are intended to be instantiated via the new token. Note the syntax highlighting.

function MyObj(){}   // good
var obj = new MyObj();

function myObj(){}   // bad
var obj = new myObj();

function doSomething(){} // good

function DoSomething(){} // bad

var myData = []; // good

var MyData = []; // bad.

Don't take shortcuts when it comes to syntax, especial concerning curly braces when defining blocks, it is a major source of syntax related bugs that are very hard to find when you are staring at thousands of lines of code.

for(i = 0; i < 10; i+= 1)
    doSomethif();            // bad

for(i = 0; i < 10; i+= 1) doSomethif();  // better but still bad

for(i = 0; i < 10; i+= 1) {  doSomethif(); }  // OK

for(i = 0; i < 10; i+= 1) {    // Good
    doSomethif();
}

Same for if, while, do statement blocks.
Layout. Don't spread out initialization code throughout the source. You are making calls to clear and radixSort intermixed within the source. Ok maybe for a short bit of code but when your code gets over a few pages it can be very hard to see what is going on.

The general layout is Function declarations first, then variables, and then functional source. I personally use bottom up layout style. The deepest level function first, top of the page, then down the page to the highest level function. So in most cases you know that to locate a function being called from within another is to look above the current functions location.

You can also use top down, it does not matter which direction, just as long as you are consistent.
Naming. Use clear descriptive words or phrases for the functions. You have Clear. Clear what, maybe resetBuckets would be a better name for the function.
Use ES6. There is no reason not to use Javascript latest and best incarnation. Using ES5 because it has more browsers that support it is only shooting your self in the foot. Time moves along and ES6 will soon be ES7. As a programmer you can not afford to not learn the latest and best, by the time you know it back to front, it will be obsolete.
Use const for any variables the will not change. (not a must, but it makes you look more professional) The general accepted reasoning between the use of constants const, block scope let, and function scope var variables is mostly about reducing bugs by protecting state. You cant mess up the state of the code if it is a protected 'const' or inaccessible (out of scope), which makes sense, well kind of, if you feel that you need the training wheels down that is.

Understand what the differences are and use them correctly. Really it is a joke that const was added to ES6, as it is borrowed from the compiled language world, where consts are there to improve speed and access, not to protect state. Don't get me started on let. End rant.

I use const because I hope one day the browsers will give a performance boost for const (sadly not yet) and as it does not decrease performance it is a bit of future optimization. I never use let because it is just wrong in Javascript.

Code

Keep the global name-space (scope) clean. You have polluted the global name-space with some nasty variable names that are going to clash, length, numbers, clear are very common variable names and if you are on a page that shares Javascript and any of that code is slack your code will stop working. Finding out why your code suddenly stops working is next to impossible, so expose only what you must and hide the rest behind your functions scope.

Example of clean namespace code:

// pass to the function data
function radixSort(numArray){
    // define required functions within the function
    function Clear() {
        for (var i = 0; i < 10; i++)
            buckets[i] = [];
    }
    // declare inside the function
    var length = numArray.length;
    var buckets = new Array(10);

    // return the processed information
    return sorted

    // Or leave in place but still return the array
    return numArray
 }

Don't use new Array() and new Object() when creating objects and arrays, they are not quite the same as using literals [] for array and {} for object. The same applies to strings (if you're ever tempted) don't use = new String("my string"); use = "my string";

I know that there is no easy way to create an array of predetermined size easily when using literal declaration, but the little bit of extra code is usually needed anyway.
Don't delete or dereference if it can be avoided. Javascript runs in a managed environment, if you don't need something you just throw it way and it will be cleaned up for you. When you need memory it is there (well within limits). But all that cleaning up the managed code does for you does not come for free. If you are a messy coder there is a lot more work for the GC (garbage collector) to do. If you want some new memory, and it is a mess, then you don't get it until it is been cleaned up. This can become very noticeable as GC is blocking.

So always keep that in mind when coding in any managed environment.

Example:

// bad
function clear() {
    for (var i = 0; i < 10; i++) {
        buckets[i] = []; // dereferences the array held in [i] and creates a brand new one
    }
}

// good
function clear() {
    for (var i = 0; i < 10; i++) {
        buckets[i].length = 0; // reduces the array size but does not create a new one
    }
}

// great by hard work
// first array item is the current number of usable items in the array
function clear() {
    for (var i = 0; i < 11; i++) {
        buckets[i][0] = 0; // totally clean nothing created, nothing deleted, and very fast
                       // but you can no longer use bucket[i].length.
    }
}

Don't repeat code. You have the function copyToArray and at the bottom of radixSort the very same code, done twice. If you want to change something you have to do it twice, and if it is a long bit of code, you may forget one.
Don't repeat calculations. You have:

// Bad
for (var index = 0; index < length; index++) {
    var tenIndex = Math.pow(10, count); // recalculating the same number for each array item. Bad
    //... blah blah
}
count++

// Good
var tenIndex = Math.pow(10, count); // only once
for (var index = 0; index < length; index++) {
    //... blah blah
}
count++

Use the correct function appropriately. parseInt is for parsing integer values into Javascript's floating point numbers. It IS NOT how you remove the fractional part of a number. Use Math.floor, Math.ceil or Math.round. Some may suggest using bitwise operators to trim of the fraction var num = myNum | 0; Don't unless you understand the difference between a double and a signed int 32 and why that will make the the test runs fail.

The radixSort

Part of its beauty is that it is a very quick sort as you only need to iterate the numbers array once for each digit in the biggest number. But there is also another optimisation you missed. Numbers that are smaller than the current digit are already sorted and thus do not need to be iterated over again and can be removed from the array of values to be sorted. This give a huge performance boost.

Note that I expect that the numbers are correctly formatted as unsigned integers and no exponent notation (e.g. 127e10).

Thus the rewrite:

// One  function name so no global scope pollution.
// lower-case as this is a function rather than an object
// Pass the array of numbers rather than use a global.
// Returns a new array of numbers sorted.
function radixSort (numbers) {
    function emptyBuckets () {          // empties buckets and adds contents back to workArray
        workArray.length = 0;
        for (i = 0; i < 10; i += 1) {   // could have used buckets forEach but this is quicker on average
            if(buckets[i].length > 0){
                workArray.push(...buckets[i]);
                buckets[i].length = 0;
            }
        }
    }
    var i;                  // hoist declarations
    const results = [];     // array that holds the finnal sorted numbers
    const buckets = [[], [], [], [], [], [], [], [], [], []];  // buckets
    const workArray = [...numbers]; // copy the numbers
    var power = 0;                  // current digit as a power of ten
    var tenPow = 1;                 // ten to the power of power
    if(numbers.length <= 1){        // if one or no items then dont sort
        return workArray;           // dont sort if there is no need.
    }
    // as numbers are sorted and moved to the result array the numbers
    while (workArray.length > 0) {
        for (i = 0; i < workArray.length; i += 1) { // for all numbers still being sorted
            if (workArray[i] < tenPow) {            // is the number samller than the current digit
                results.push(workArray[i]);         //Yes it is sorted then remove a put o nthe result array
            } else {
                // add to bucket. Use Math.floor and save complexity doing it in one statement line
                buckets[Math.floor(workArray[i] / tenPow) % 10].push(workArray[i]);
            }
        }
        power += 1;
        tenPow = Math.pow(10, power);
        emptyBuckets();
    }
    return results;
}


Parts Inventory System I have been working on some code for a basic inventory management system and I have all the features that I believe are need like adding parts displaying parts list and the such as you can see. I believe I have worked out all the bugs I can find. I'm looking to see if I got all the bugs out and maybe some optimizations that I am not seeing. The code is fully working and I'm just making sure I am not overlooking anything before I start using this code.

dict_member = {}
parts = dict_member
class Parts():
    def __init__(self, part, part_number, quantity):
        self.part = part
        self.part_number = part_number
        self.quantity = quantity
    def display (self):
        for part_number, parts in dict_member.items():
            print('Part Name:', parts.part + ', ''Part Number:' + parts.part_number + ', ''Quantity:' + parts.quantity)
            if parts == 0:
                print('No Parts Found in List')
    def add(self):
            part = input("Enter Name of Part:\n ")
            part_number = input("Enter Part Number:\n ")
            quantity = input("Enter Quantity:\n ")
            dict_member[part] = Parts(part, part_number, quantity)
    def remove(self):
            part = str(input("Enter Part Name to Remove\n"))
            if part in parts:
                del parts[part]
            else:
                print("Part Not Found.")
    def edit(self, part):
            if part in parts:
                par = input("Enter New Part\n")
                num = input("Enter New Part Number\n ")
                quan = input("Enter New Quantity\n ")
                del dict_member[part]
                dict_member[part] = Parts(par, num, quan)
            else:
                print("No such Part exists")

    def saveData(self):
        filename = input("Filename to save: ")
        out_file = open(filename, "wt")
        for part_number, parts in dict_member.items():
            out_file.write(parts.part + "," + parts.part_number + "," + parts.quantity + "\n")
        out_file.close()
        print("File Saved")

    def loadData(self):
        try:
            filename = input("Filename to load: ")
            in_file = open(filename, "rt")
            while True:
                in_line = in_file.readline()
                if not in_line:
                    break
                in_line = in_line[:-1]
                part, part_number, quantity = in_line.split(",")
                dict_member[part] = Parts(part, part_number, quantity)
            in_file.close()
        except FileNotFoundError:
            print("File Not Found.")
        return parts

def display_menu():
    try:
        print("")
        print("1. Parts ")
        print("2. Add Part")
        print("3. Remove Part ")
        print("4. Edit Part")
        print("5. Save Part List")
        print("6. Load Part List")
        print("9. Exit ")
        print("")
        return int(input("Selection> "))
    except ValueError:
        print("Selection Not Valid.")
    return parts

print("Welcome Sticks&Stones Inventory System")
part_instance = Parts(None, None, None)
menu_item = display_menu()
while menu_item != 9:
    if menu_item == 1:
        part_instance.display()
    elif menu_item == 2:
        part_instance.add()
    elif menu_item == 3:
        part_instance.remove()
    elif menu_item == 4:
        m = input("Enter Part Name to Edit\n")
        part_instance.edit(m)
    elif menu_item == 5:
        part_instance.saveData()
    elif menu_item == 6:
        part_instance.loadData()
    menu_item = display_menu()
print("Exiting Program...")

You are mixing two very different objects and tasks in your Parts class. On the one side there is the object of a Part and on the other hand there is a dictionary of parts which is your inventory system. You should separate the two:

import sys
from collections import namedtuple

class Part(namedtuple("Part", "name part_number quantity")):
    def __str__(self):
        return ", ".join(self)

class Parts(dict):
    def display (self):
        if not self:
            print('No Parts Found in List')
            return
        print()
        print('Name, Part Number, Quantity')
        for part in self.values():
            print(part)
        print()

    def add(self, *args):
        try:
            name, part_number, quantity = args
        except ValueError:
            name = input("Enter Name of Part:\n ")
            part_number = input("Enter Part Number:\n ")
            quantity = input("Enter Quantity:\n ")
        self[name] = Part(name, part_number, quantity)

    def remove(self, part=""):
        if not part:
            part = input("Enter Part Name to Remove\n")
        try:
            del self[part]
        except Keyerror:
            print("Part {} not found.".format(part))

    def edit(self, part=""):
        if not part:
            part = input("Enter Part Name to Edit\n")
        try:
            new_name = input("Enter new part name\n")
            number = input("Enter new part number\n ")
            quantity = input("Enter new quantity\n ")
            self[part] = Part(new_name, number, quantity)
        except KeyError:
            print("No such Part exists: {}".format(part))

    def save(self, filename=""):
        if not filename:
            filename = input("Filename to save: ")
        with open(filename, "wt") as out_file:
            for part in self.values():
                out_file.write("{}\n".format(part))
        print("File saved")

    def load(self, filename=""):
        if not filename:
            filename = input("Filename to load: ")
        try:
            with open(filename, "rt") as in_file:
                for line in in_file:
                    if not line:
                        break
                    part, part_number, quantity = line.strip().split(",")
                    self.add(part, part_number, quantity)
        except FileNotFoundError:
            print("File Not Found.")

def menu(inventory):
    menu_list = [("Parts", inventory.display),
                 ("Add Part", inventory.add),
                 ("Remove Part", inventory.remove),
                 ("Edit Part", inventory.edit),
                 ("Save Part List", inventory.save),
                 ("Load Part List", inventory.load),
                 ("Exit", sys.exit)]
    while True:
        for i, (name, _) in enumerate(menu_list, 1):
            print("{}. {}".format(i, name))
        try:
            user = int(input("Selection> "))
            menu_list[user-1][1]()
        except (ValueError, IndexError):
            print("Selection Not Valid.")

def main():
    print("Welcome to Sticks&Stones Inventory System")
    inventory = Parts()
    while True:
        menu(inventory)

if __name__ == '__main__':
    try:
        main()
    except (KeyboardInterrupt, SystemExit):
        print("Exiting Program...")

Here I made two classes. I also made the inventory a sub-class of dict. This way you don't have to keep an external dictionary (and could even have two different inventories at the same time, maybe for different things?).

All methods of the inventory take parameters if you want to, and ask for them if they are not supplied.

A dict has dict.values which directly gives you a list (or iterable) of all values.

The Part is just a data container, so I used collections.namedtuple here, and just added nicer printing by overriding the magic __str__ method, which is invoked when calling e.g. print(part).


Unsubscribing Facebook admins and regular users from notifications What is the best way to DRY this code up?

class NotificationsController < ApplicationController
  before_filter :load_user

  def unsubscribe
    if @facebook_admin && @facebook_admin.auth_key == params[:auth_key]
      @facebook_admin.update_notification_settings(params[:type] => false)
    elsif @user && @user.auth_key == params[:auth_key]
      @user.update_notification_settings(params[:type] => false)
    end
  end

  private

  def load_user
    if params[:type].present?
      if params[:facebook_admin_id].present?
        @facebook_admin = FacebookAdmin.find_by_id(params[:facebook_admin_id])
      elsif params[:user_id].present?
        @user = User.find_by_id(params[:user_id])
      end
    end
  end
end

Verbosity of a controller is typically a signal of dubious design. As I see it, you have two sensible options:

If User and FacebookAdmin are two very different user entities: they should be treated in two separate controllers.
If User and FacebookAdmin are very similar (i.e. FacebookAdmin inherits from User): they can share the same controller but use always @user as variable. This way the code will be simplified dramatically.

What you should not have is a controller full of conditional branches, that will be an unmaintainable mess (and awful to test).


A better way to set flags I need to use some flags in my code. To make it a bit clearer, I am looking to set the flags in some structure, and I need to match the following needs:

The flag representation in the structureis hidden.
The following methods are provided (or the same idea at least): setFlag(structWithFlags *, ACTION_FLAGS ),
isFlagSet(structWithFlags *, ACTION_FLAGS) and removeFlag(structWithFlags *, ACTION_FLAGS)
The structure should not contain a "magic number" that defines the size of the flag bytes, since the number of flags may vary, when compiling for different projects.

In general, this is serving as part of a general module, used by many different modules, in many different combinations.

Currently, my method to achieve that is as follows:

projectSpecificNeeds.h:

typedef enum ACTION_FLAGS {
    ACTION_FLAG_NO_1,
    ACTION_FLAG_NO_2,
    //.. etc.
    FLAG_LAST
}ACTION_FLAGS ;

genericModuleThatServesEveryBody.c:

#include <limits.h>
#include "projectSpecificNeeds.h"
typedef struct structWithFlags {
    // Some irrelevant fields
    // each flag is represented by one bit in the flagByte array
    char flagBytes[FLAG_LAST / CHAR_BIT + 1];
}structWithFlags ;

structWithFlags * getStructWithFlags() {
    return calloc(sizeof(structWithFlags));
}

// Bitwise OR between the relevant flag byte and the flag
void setFlag(structWithFlags * st, ACTION_FLAGS flag) {
    st->flagBytes[flag / CHAR_BIT] |= 1 << (flag % CHAR_BIT);
}

// return the AND between the relevant flag byte and the flag
char isFlagSet(structWithFlags * st, ACTION_FLAGS flag) {
    return st->flagBytes[flag / CHAR_BIT] & ( 1 << (flag % CHAR_BIT));
}

// Bitwise AND between the relevant flag byte all bits except of the flag
void removeFlag(structWithFlags * st, ACTION_FLAGS flag) {
    st->flagBytes[flag / CHAR_BIT] &= ~(1 << (flag % CHAR_BIT));
}

It works well, and I met no problem with it until now. But I wonder whether there's some more elegant / readable yet memory saving method to implement this functionality.

How many flags do you need? If you can settle with only 32 or 64 different flags instead of a generic-length array of flags, then it would probably be better to allocate the flags in a simple unsigned long, to save your program from all the runtime calculations.

The main efficiency problem in your code is that the compiler can't know the value of the "flag" parameter, so it must calculate division and modulo in runtime to know where to set the flag.

Your code is also limited to one flag set at a time. A common way to write code like this from the caller's side is setFlag (&the_struct, FLAG_THIS | FLAG_THAT);. The whole Windows API uses flags like that, with bitwise OR. If you stored the flags in an unsigned long that wouldn't be a problem, as you wouldn't need to index the flag array. Then the caller wouldn't need to call your function over and over to set several flags at the same time.

Surely it must be ok to group flags into 32/64 bit variables, related to a specific code module?


Database access class using VB.NET I've recently been asked to make some emergency changes to a legacy application written in VB.NET.  I come from a C# background so am not overly familiar with this language, though there are enough similarities for me to get by.

The current code has no separation of concerns (e.g. DAL code is written in the codebehind of each page rather than separated out).  To make it usable without introducing too much risk I'm avoiding libraries but extracting the DAL code to separate classes.  Since my DB code's going to be used from a lot of places in the app, I wanted to check that this all makes sense, that it is reasonably efficient and that I haven't made some schoolboy error in creating this.

Imports Microsoft.VisualBasic
Imports System.Data.SqlClient
Imports System.Collections.Generic

Public Class DB

    Private _connectionString As String

    Public Sub New(connectionString As String)
        _connectionString = connectionString
    End Sub

    Public Sub ExecuteNonQuery(cmdTxt As String, params As Dictionary(Of String, Object))
        Using cmd As SqlCommand = BuildCommand(cmdTxt, params)
            cmd.ExecuteNonQuery()
        End Using
    End Sub

    Public Function ExecuteReader(cmdTxt As String, params As Dictionary(Of String, Object)) As SqlDataReader
        Using cmd As SqlCommand = BuildCommand(cmdTxt, params)
            Return cmd.ExecuteReader()
        End Using
    End Function

    Public Function ExecuteScalar(cmdTxt As String, params As Dictionary(Of String, Object)) As Object
        Using cmd As SqlCommand = BuildCommand(cmdTxt, params)
            Return cmd.ExecuteScalar()
        End Using
    End Function

    Private Function BuildCommand(cmdTxt As String, params As Dictionary(Of String, Object)) As SqlCommand
        Using con As New SqlConnection(_connectionString)
            Using cmd As SqlCommand = con.CreateCommand()
                cmd.CommandType = CommandType.StoredProcedure
                cmd.CommandText = cmdTxt
                AddParameters(cmd, params)
                con.Open() 'working on the assumption this command will be run as soon as it's retuned; so this open is left as late as possible but here to avoid duplicate code
                Return cmd
            End Using
        End Using
    End Function

    Private Sub AddParameters(ByRef cmd As SqlCommand, params As Dictionary(Of String, Object))
        If Not params Is Nothing Then
            For Each kvp As KeyValuePair(Of String, Object) In params
                cmd.Parameters.AddWithValue(kvp.Key, kvp.Value)
            Next
        End If
    End Sub

End Class

Now that's some coincidence! No later than this week I wrote an ADODB "wrapper" for some vb6 legacy code (here). This looks very much similar, only for ADO.NET.

I don't like the name. DB is not respecting the PascalCase naming convention for VB.NET classes (same in C#) - I realize it's a short handy name, but it looks like a variable's name. I'd rename the class to, say, AdoCommandBuilder; the name of the object should say what the object's role is. In this case, building an ADO.NET command.

I might be completely wrong here, but it seems to me that a thread entering BuildCommand from ExecuteReader would open up a connection, and then when the function returns, the connection would be disposed before the client code can start iterating the reader. Thus I wouldn't wrap an IDisposable in a Using block if I'm going to be returning it.

Anyway your command is being disposed twice. Once in BuildCommand:

        Using cmd As SqlCommand = con.CreateCommand()
            cmd.CommandType = CommandType.StoredProcedure
            cmd.CommandText = cmdTxt
            AddParameters(cmd, params)
            con.Open()
            Return cmd
        End Using '<<<<<< here

...and once more in the ExecuteXxxxx methods:

    Using cmd As SqlCommand = BuildCommand(cmdTxt, params)
        Return cmd.ExecuteReader()
    End Using ' <<<<<< here too!

I'd remove the Using cmd As SqlCommand from the CreateCommand method.

I agree with @Malachi, that BuildCommand shouldn't be opening the connection - its concern is building a command, so it should be taking a connection as a parameter; by doing that you enable extending your class with methods that take a connection, which enables running your commands in a transaction.

Hence, I think I would drop the parameterized constructor, add a Connect(connectionString As String) method instead, encapsulate the connection (, implement IDisposable) and give each ExecuteXxxxxx method an overload that takes an IDbConnection parameter; if you're not going to be taking in a connection, then you should own the whole process and return the data, not the reader itself - the idea is that you generally want the object that creates an IDisposable to be the one calling Dispose on it, so I'd only return a SqlDataReader when the client code owns the connection.


Print a christmas tree class ChristmasTree
{
    private static void byHand()
    {
        System.out.println( "Chritsmas tree, yey." );
        System.out.println( "     *" );
        System.out.println( "    ***" );
        System.out.println( "   *****" );
        System.out.println( "  *******" );
        System.out.println( " ********" );
        System.out.println( "**********" );
        System.out.println( "    |" );
        System.out.println( "    _" );
    }

    private static void withLoops()
    {
        int rows = 6;
        StringBuilder sb = new StringBuilder();
        for( int row = 0; row < rows; ++row )
        {
            for( int spaces = rows - row; spaces > 0; --spaces )
            {
                sb.append( " " );
            }

            for( int stars = 0; stars < 2 * row + 1; ++stars )
            {
                sb.append( "*" );
            }
            sb.append( "\n" );

        }

        for( int spaces = 0; spaces < rows; ++spaces )
        {
            sb.append( " " );
        }
        sb.append( "|" );
        sb.append( "\n" );
        for( int spaces = 0; spaces < rows; ++spaces )
        {
            sb.append( " " );
        }
        sb.append( "_" );
        System.out.println( sb.toString() );
    }

    private static String spaces( int num )
    {
        StringBuilder sb = new StringBuilder();
        for( int spaces = 0; spaces < num; ++spaces )
        {
            sb.append( " " );
        }
        return sb.toString();
    }

    private static String properly( int rows )
    {
        StringBuilder sb = new StringBuilder();
        for( int row = 0; row < rows; ++row )
        {
            for( int spaces = rows - row; spaces > 0; --spaces )
            {
                sb.append( " " );
            }

            for( int stars = 0; stars < 2 * row + 1; ++stars )
            {
                sb.append( "*" );
            }
            sb.append( "\n" );

        }
        sb.append( spaces( rows ) );
        sb.append( "|" );
        sb.append( "\n" );
        sb.append( spaces( rows ) );
        sb.append( "_" );

        return sb.toString();
    }

    public static void main(String[] args)
    {
        byHand();

        withLoops();

        // What if the tree needs to be 100 lines high?
        // We arrive at the concept of `scalability`.
         System.out.println( properly( 16 ) );
    }
}

Topics I have considered but decided against:

pass the tree height on the command line via java.util.Scanner;
make a copy of spaces() called stars()
explain the purpose of the class at the very beginning a la python docstring
split the methods so the don't exceed 20 LOC

Also there's this.

I have some suggestions for your code.

Most of your loops can be replaced with java.lang.String#repeat (Java 11+)

Example
"*".repeat(10) // Will give you "**********"

I suggest that you create a class constant for each of the tree's parts (*, \n, |) to allow the code to be easier to refactor / make changes in the future, if you need to (i.e., change the star by a plus sign, etc.).

For the tree's parts, I suggest that you use character instead of string, since the character will take less memory than the string (very small amount in this case, but still).
" " -> ' '


Makefile and object file directory Ok this is the first makefile I've ever written. I it works and that's about it. Things that I was wondering if if I can specify where the object files go. It looks really bad with all the .o files cluttering the working folder. I was thinking a src folder and maybe a obj folder? Is there a better naming convention for Compiler_flags and c_compiler_flags? Also is there other ways of optimizing the makefile? Thanks all help is extremely appreciated.

OBJS = close.o init.o main.o texture.o load_media.o
CC = g++
COMPILER_FLAGS = -w
C_COMPILER_FLAGS = -c -w
LINKER_FLAGS = -lSDL2 -lSDL2_image -lSDL2_mixer -lSDL2_ttf
OBJ_NAME = run

all : exe

debug : COMPILER_FLAGS += -g
debug : C_COMPILER_FLAGS += -g
debug : exe

exe : $(OBJS)
    $(CC) $(OBJS) $(COMPILER_FLAGS) $(LINKER_FLAGS) -o $(OBJ_NAME)

close.o : isolation.h close.cpp
    $(CC) $(C_COMPILER_FLAGS) $(LINKER_FLAGS) close.cpp

init.o : isolation.h init.cpp
    $(CC) $(C_COMPILER_FLAGS) $(LINKER_FLAGS) init.cpp

main.o : isolation.h main.cpp
    $(CC) $(C_COMPILER_FLAGS) $(LINKER_FLAGS) main.cpp

texture.o : isolation.h texture.h texture.cpp
    $(CC) $(C_COMPILER_FLAGS) $(LINKER_FLAGS) texture.cpp

load_media.o : isolation.h load_media.cpp
    $(CC) $(C_COMPILER_FLAGS) $(LINKER_FLAGS) load_media.cpp

clean:
        \rm *.o *~ run

There is no need to pass linker flags to the compile stage. You only them when you link the final executable.
Traditionally (aligned with make defaults) the flags variables are CFLAGS for C compiler, CXXFLAGS for C++ compiler, and LDFAGS for the link stage specifics. It is not recommended however to have -c as a part of CFLAGS: the CFLAGS can be reused for non-compile action (e.g. dependency generation as described below).
exe is a phony target (and better be specified as such); a file named exe is never created, so it is never up to date, and the makefile forces linking even if nothing has been changed. I recommend to change the link recipe to

$(OBJ_NAME): $(OBJS)
    ....

Stem rules let you DRY:

%.o: %.cpp
    $(CC) $(CXXFLAGS) $<

For a separate build directory you may look at VPATH; I prefer to to point it explicitly in the stem rule:

$(BUILD)/%.o: %.cpp
    $(CC) -o $@ $(CXXFLAGS) $<

As a side note, I'd go one step further, and separate debug and release builds.
Dependency generation. The stem rules above do not specify #include dependencies. With gcc it is easy to automate. -M family of flags:

$(BUILD)/%.d: %.cpp
    $(CC) -o $@ -MM -MT $(CXXFLAGS) $<

create a makefile fragment which you'd include in your makefile.

Since the dep and obj file names mirror the source file names (only the extension differs) it is recommended to generate them using the patsubst function.
Never use -w. Warnings are your friends. I highly recommend passing at least -Wall -Wextra.
Putting it (almost) all together,

SRC = close.cpp init.cpp main.cpp texture.cpp load_media.cpp
OBJ = $(patsubst %.cpp,$(BUILD)/%.o,$(SRC))
DEP = $(patsubst %.cpp,$(BUILD)/%.o,%(SRC))

TARGET = run

$(BUILD)/%.o: %.cpp
    $(CXX) -c      -o $@ $(CXXFLAGS) $<

$(BUILD)/%.d: %.cpp
    $(CXX) -MM -MT -o $@ $(CXXFLAGS) $<

-include $(DEP)

$(BUILD)/$(TARGET): $(OBJ)
    $(CXX) -o $@ $(CXXFLAGS) $(LDFLAGS) $(OBJ)


React component to animate planets on a canvas I wrote some JavaScript to generate some graphics on a canvas, on a regular HTML page.

I now want the same code to run in a component that is part of a React app. I've done this:

  componentDidMount() {
    const canvas = this.refs.firstCanvas
    const ctx = canvas.getContext('2d')
    const bgCanvas = this.refs.firstbgCanvas
    const bgCtx = bgCanvas.getContext('2d')

    function generateStarfield() {
      bgCtx.clearRect(0, 0, bgCanvas.width, bgCanvas.height)
      for (let i = 0; i < 2000; i++) {
        bgCtx.beginPath()
        const x = Math.random()*bgCanvas.width
        const y = Math.random()*bgCanvas.height
        bgCtx.arc(x, y, 0.35, 0, 2*Math.PI, 'anticlockwise')
        bgCtx.closePath()
        bgCtx.fillStyle = 'white'
        bgCtx.fill()
      }
    }
    generateStarfield()

    const origin_x = 1000
    const origin_y = 1000
    const scale = 1000

    class planet {
      constructor(orbital_velocity, offset_theta, orbital_radius, radius, colour) {
        this.orbital_velocity = orbital_velocity
        this.orbital_radius = orbital_radius
        this.colour = colour
        this.radius = radius
        this.offset_theta = offset_theta
        this.draw()
      }

      draw() {
      const theta = this.offset_theta + (t*this.orbital_velocity)
      const x = origin_x + (this.orbital_radius*Math.cos(theta))
      const y = origin_y + (this.orbital_radius*Math.sin(theta))
      ctx.beginPath()
      ctx.arc(x, y, this.radius, 0, 2*Math.PI, 'anticlockwise')
      ctx.closePath()
      ctx.fillStyle = this.colour
      ctx.fill()
      }
    }

    let t = 0

    const a = new planet((0.2+0.4*Math.random())*Math.PI, 2*Math.random()*Math.PI, scale*0.1, 10, 'white')
    const b = new planet((0.2+0.4*Math.random())*Math.PI, 2*Math.random()*Math.PI, scale*0.2, 16, 'white')
    const c = new planet((0.2+0.4*Math.random())*Math.PI, 2*Math.random()*Math.PI, scale*0.3, 18, 'white')
    const d = new planet((0.2+0.4*Math.random())*Math.PI, 2*Math.random()*Math.PI, scale*0.4, 14, 'white')
    const e = new planet((0.2+0.4*Math.random())*Math.PI, 2*Math.random()*Math.PI, scale*0.5, 12, 'white')
    const f = new planet((0.2+0.4*Math.random())*Math.PI, 2*Math.random()*Math.PI, scale*0.6, 28, 'white')
    const g = new planet((0.2+0.4*Math.random())*Math.PI, 2*Math.random()*Math.PI, scale*0.7, 22, 'white')
    const h = new planet((0.2+0.4*Math.random())*Math.PI, 2*Math.random()*Math.PI, scale*0.8, 20, 'white')

    setInterval(function() {
      ctx.clearRect(0, 0, canvas.width, canvas.height)
      ctx.beginPath()
      a.draw()
      b.draw()
      c.draw()
      d.draw()
      e.draw()
      f.draw()
      g.draw()
      h.draw()
      t += 0.05
    }, 40)
  }

Now, this works (!), but I've just lifted it directly from the original JavaScript, and modified the document.getElementByID calls to use refs. Is it bad practice to stick all of this stuff into componentDidMount?

Note - I'm aware that the code itself could be tidied up, I'm mainly asking about putting all of this (especially the definitions for planet and generateStarField) into componentDidMount.

You can do this, if it works... but it's not really using the React architecture or patterns.

Most of this code that is drawing should be in the render method.
You'll want to move some of the state (canvas, ctx, bgCanvas, bgCtx) into the class's state, or just as instance variables.
You can probably do most of the creation in the constructor, although doing it in componentDidMount will work.
You can integrate the animation with React,
setInterval(() => this.forceUpdate(() => this.t += 0.05)). This makes sense in componentDidMount. You'll also want to turn this off in the unmount method.

EDIT/ADDED LATER

...maybe you just want to leave it the way it is, although it's quite non-React... it's working fine, though, and there aren't really problems...

To make it "React" you're probably going to have to re-write it completely-- because just moving pieces around like I suggested may make you end up fighting the framework more than leveraging. Looking more, you'd probably make separate components for the different canvases and perhaps pass in t as a property, so there's a container component that functions as the "clock". Doing something like that would allow the regular React rendering mechanisms work.


Getting all the dates in a month that do not coincide with a project's dates I'm creating an array of dates that won't be selectable in a calendar. The following works, although the way I've gone about it is quite long-winded.

I was wondering if there is a more elegant way of getting all the dates in a month, where dates don't fall on various days a project does run on. It's probably more clear from the example...

$project_days_this_month = [];
$month_array = [];

$month = date('n');
$year = date('Y');

$project_days = [1, 6]; // days of week the project runs on
$days_in_month = cal_days_in_month(CAL_GREGORIAN, $month, $year);

// create an array of days in month
for ($day = 1; $day <= $days_in_month; $day++) {
  $date = $year.'-'.$month.'-'.$day; //format date
  $month_array[] = $date;
}

// get project days in month
foreach ($project_days as $pd) {
  //loop through all days
  for ($day = 1; $day <= $days_in_month; $day++) {

    $date = $year.'-'.$month.'-'.$day; //format date
    $day_number_of_week = date('N', strtotime($date));

    // add project days to array
    if($day_number_of_week == $pd){
        $project_days_this_month[] = $date;
    }

  }
}

// get the difference - these will be non project days
$non_project_days = array_diff($month_array, $project_days_this_month);
print_r($non_project_days);

Are there any PHP functions that can be used here that would might make this code more concise?

My attempt to make this code as clean as possible.
Note that i have not changed almost any of the logic you provided.

<?php

class ProjectDays {
    private $projectDays;

    public function __construct(array $projectDays, $month, $year) {
        $this->projectDays = $projectDays;
        $this->month = $month;
        $this->year = $year;
    }

    public function getNonProjectDays() {
        $monthDays = array();
        $projectDays = array();
        $daysInMonth = $this->getDaysInMonth();
        foreach ($this->projectDays as $projectDay) {
            for ($day = 1; $day <= $daysInMonth; $day++) {
                $date = $this->formatDate($day);
                $monthDays[$day] = $date;
                $weekDay = $this->getWeekDay($date);
                if($weekDay == $projectDay){
                    $projectDays[] = $date;
                }
            }
        }
        return array_diff($monthDays, $projectDays);
    }

    private function getWeekDay($date) {
        return date('N', strtotime($date));
    }

    private function getDaysInMonth() {
        return cal_days_in_month(CAL_GREGORIAN, $this->month, $this->year);
    }

    private function formatDate($day) {
        return "{$this->year}-{$this->month}-{$day}";
    }
}

$excludedDays = new ProjectDays([1, 6], date('n'), date('Y'));

print_r($excludedDays->getNonProjectDays());


Command-line program for applying several operations on a PDF file I am building a command-line .exe that can apply several operations on a PDF file (add text, images, resize, crop, etc).

Currently, my Program.cs looks a bit like this (it uses CommandLineParser):

switch (invokedVerb)
{
    case "barcode":
        Operations.BarcodeOperations.AddBarCode(options.AddBarcodeVerb);
        break;

    case "addblankpage":
        Operations.PageOperations.AddBlankPage(options.AddBlankPageVerb);
        break;
}

As you can see, I have split the operations into several XXXOperations classes, each of them having very similar instructions:

public static void AddStuff(StuffOptions options)
{
    Logging.Log("here is a bunch of logging");

    // here sometimes there is some action-specific code but not often

    using (DocWrapper doc = new DocWrapper(options.File)) // this is in all actions
    {
        foreach (int page in doc.GetPagesToModify(options.Pages)) // this is in most actions
        {
            // call some stuff on the doc instance
        }

        doc.Save(options.OutputFile); // this is in all actions
    }
}

So, all actions create a new instance of DocWrapper, most of them loop on its pages (but I could modify the actions so that all of them do), and all of them save, but each of them do a different set of actions inside it.

How could I refactor this code so that the DocWrapper instantiation, the pages loop and the save are in a single place, but I can specify custom code inside the loop?

I'm thinking of using delegates to define my actions, but I have no idea where to start, since I'm not very familiar with them. I'm also considering inheriting from an abstract class that implements the loop and abstracts a DoWork method, called inside the loop and implemented in the XXXOperations class, but since I don't know delegates, I'm not sure which is more adapted to the situation.

Yep. I suppose your idea is generaly fine. This is being called Command Pattern. You might like to read more about it on Google. Used together with other well known pattern Strategy they both might be used here. Plus another one - Template Method. Let me just suggest some basic approach:

public abstract class BaseCommand
{
    protected BaseCommand()
    {

    }

    public void Process(StuffOptions options)
    {
        Logging.Log("here is a bunch of logging");

        // here sometimes there is some action-specific code but not often
        ExtraStuff();

        using (DocWrapper doc = new DocWrapper(options.File)) // this is in all actions
        {
            foreach (int page in doc.GetPagesToModify(options.Pages)) // this is in most actions
            {
                // call some stuff on the doc instance
                DoRealStuff(doc);
            }

            doc.Save(options.OutputFile); // this is in all actions
        }
    }

    protected abstract void DoRealStuff(DocWrapper doc);

    protected virtual void ExtraStuff()
    {

    }
}

So, now you can go and implement your commands. You have to ALWAYS implement abstract method DoRealStuff() but only override ExtraStuff() when needed.

class BaseCommandA : BaseCommand
{
    protected override void DoRealStuff(DocWrapper doc)
    {
        // Doing my stuff here!
    }
}

class BaseCommandB : BaseCommand
{
    protected override void ExtraStuff()
    {
        base.ExtraStuff();

        // I have to set some extra paramaetrs here...
    }

    protected override void DoRealStuff(DocWrapper doc)
    {
        // Doing my other stuff here!
    }
}

Then there is matter of implementing last part - strategy. You can keep your static switch-get approach if you like, bu t better is to hide your classes details and return commands only by their base type:

public static class CommandFactory
{
    private static BaseCommandA commandAInstance = new BaseCommandA();
    private static BaseCommandB commandBInstance = new BaseCommandB();

    public static BaseCommand GetCommand(invokedVerb)
    {
        switch (invokedVerb)
        {
            case "barcode": return commandAInstance;
            case "addblankpage": return commandBInstance;

            // ...
        }
    }
}

You might like to go into some lazy initialization inside if you like...

And then - grande finale - using all this stuff:

    public void Processing(cmd)
    {
        CommandFactory.GetCommand(cmd.Verb).Process(cmd.Options);
        // ...
    }

Ofcourse all types, and arguments are being simplified. Good luck and have nice coding.

EDIT: And yes - there ofcourse are some precautions - when commands are done like that (static) all processing shall be stateless (no properties or fields inside command classes). If this is required - for example to pass some data from DoExtraStuff() - you just remove static fields and create new instance of command every time the GetCommand() is being called. Good thing is - the main Processing function remains the same - power of encapsulation.
Delegate approach might also be fine, but I think my classic solution is much easier to expand, extend and maintain.

EDIT 2: I was asked by OP to explain more situation where there is some data to pass between preprocessing function and main function. There are at least two possible solutions.
First is to implement factory other way, so this is no longer using static instances of commands but creates a new one for each call:

public static class CommandFactory2
{
    public static BaseCommand GetCommand(string invokedVerb)
    {
        switch (invokedVerb)
        {
            case "barcode": return new CommandA();
            case "addblankpage": return new CommandB();

            // ...
        }

        return null;
    }
}

The only remaining thing is to add some fields / properties to BaseCommand or specific Command and just share values between methods of the class, like:

class CommandC : BaseCommand
{
    private string _extraValue = null;

    protected override void ExtraStuff()
    {
        base.ExtraStuff();
        _extraValue = ProcessExtraValue();

        // I have to set some extra parameters here...
    }

    protected override void DoRealStuff(DocWrapper doc)
    {
        // Doing my other stuff here!
        switch (_extraValue)
        {
           // ...
        }
    }

    private string ProcessExtraValue()
    {
        //...
        return "";
    }
}

The second approach would be suitable if you always produce same intermediate data, so then you do not alter Factory but rather BaseCommand interface and children, like:

public abstract class BaseCommand
{
    protected BaseCommand()
    {

    }

    public void Process(StuffOptions options)
    {
        Logging.Log("here is a bunch of logging");

        // here sometimes there is some action-specific code but not often
        ExtraData data = new ExtraData();
        ExtraStuff(data);

        using (DocWrapper doc = new DocWrapper(options.File)) // this is in all actions
        {
            foreach (int page in doc.GetPagesToModify(options.Pages)) // this is in most actions
            {
                // call some stuff on the doc instance
                DoRealStuff(doc, data);
            }

            doc.Save(options.OutputFile); // this is in all actions
        }
    }

    protected abstract void DoRealStuff(DocWrapper doc, ExtraData data);

    protected virtual void ExtraStuff(ExtraData data)
    {

    }
}

And then you just fill this ExtraData class with whathever proeprties you wan't to share.

I would personally prefer first one, as it is more elastic and introduces extra stuff only when this is required (ie. only inside these commands that need this). However drawback is if you are planning on intense processing - you will be creating new instance of command class every time you are going to use it...

Eventually, this can be solved on factory level - implement each command creation the best way that suits it better - even create deeper command inheritance to distinguish different behavior. But now I went too far (and broke SOLID a bit), so let allow me to finish at this point :P


ExpressJS easily serve static files I have a NodeJS package, staticize, which takes an object of HTTP routes -> file locations, then serves them dynamically on each request.

Could I get some feedback on the latest rewrite of the main file? (Also available on GitHub here)

var fs = require('fs');
var mime = require('mime');

module.exports = function(app, routes) {
    // loop through routes
    for (var index in routes) {
        // closure: route is the HTTP request route, path is the file path
        (function (route, path) {
            app.get(route, function(req, res) {
                // create a readable stream
                var stream = fs.createReadStream(path, {encoding: 'utf8'});

                stream
                    .once('error', function() {
                        res.status(500).end('Server error.');
                        console.error('ERROR (staticize): Could not open readable stream to ' + path);
                    })
                    .once('readable', function() {
                        // create headers
                        var headers = {
                            'Content-Type': mime.lookup(path)
                        };

                        // write head then pipe response
                        res.writeHead(200, headers);
                        stream.pipe(res);
                    });
            });
        })(index, routes[index]);
    }
};

It is called simply like this:

staticize(app, {
    '/foo': '../test/foo/bar.txt',
    '/foo/more': '../test/foo-extra/bar.txt'
});

I've tried to perfect it as much as possible, but I may have overlooked some things.

It looks pretty good and definitely works, but I would approach this a little bit differently and try to avoid that closure - it's a little bit difficult to read and it's also unnecessary. You can use Object.keys if you want to be able to use an object for your routes parameter.

module.exports = function(app, routes) {
    // loop through routes
    var len = routes.length,
        keys = Object.keys(routes),
        // Define path and stream outside of the loop, at the top
        // of the module as private vars and reuse (they are
        // hoisted by JavaScript anyway.
        url,
        path,
        stream;

  // Use a while loop here because it doesn't matter which
  // order you setup these routes in. This is the least verbose.
  while(len--){
    url = keys[len];
    app.get(url, function(req, res) {
        path = routes[url];
        // create a readable stream
        stream = fs.createReadStream(path, {encoding: 'utf8'});

        stream
        .once('error', function() {
            res.status(500).end('Server error.');
            console.error('ERROR (staticize): Could not open readable stream to ' + path);
        })
        .once('readable', function() {
            // create headers
            var headers = {
                'Content-Type': mime.lookup(path)
            };

            // write head then pipe response
            res.writeHead(200, headers);
            stream.pipe(res);
        });
    });
  }
};

Personally, I would use an array with named properties instead of an Object for my routes, but key/value is OK. Something like this would allow you to just do a routes.forEach() and have direct access without having to use Object.keys:

staticize(app, [
    {
        url: '/foo',
        path: '../test/foo/bar.txt'
    }, {
        url: '/foo/more',
        path: '../test/foo-extra/bar.txt'
    }
]);

module.exports = function(app, routes) {
  // Use a forEach here instead of a loop
  routes.forEach(function(route){
    app.get(route.url, function(req, res) {
        // Save the route since we use it three times in here
        var path = route.path;

        // create a readable stream
        var stream = fs.createReadStream(path, {encoding: 'utf8'});

        stream
        .once('error', function() {
            res.status(500).end('Server error.');
            console.error('ERROR (staticize): Could not open readable stream to ' + path);
        })
        .once('readable', function() {
            // create headers
            var headers = {
                'Content-Type': mime.lookup(path)
            };

            // write head then pipe response
            res.writeHead(200, headers);
            stream.pipe(res);
        });
    });
  });
};


Hackerrank challenge - Apple and Orange The problem I am trying to solve is this.  Apples fall at various offsets relative to an apple tree at position a, and oranges fall at various given offsets relative to an orange tree at position b.  How many of each kind of fruit lie between positions s and t?

I pass 8 from the 11 test cases, I fail 3 tests due to timeout error.

I spent the last hour or so trying to refactor my code to make it faster but I am out of ideas now.
Can someone give an advice what should I do in order to make it faster?

function countApplesAndOranges(s, t, a, b, apples, oranges) {
    let house = [];
    let applesCount = 0;
    let orangesCount = 0;
    for(let i = s; i <= t; i++){
        house.push(i);
    };
    const applesPositions = apples.map(val=> val + a);
    const orangesPositions = oranges.map(val=> val + b);

    for(let j = 0; j < house.length; j++){
        for(let k = 0; k < applesPositions.length; k++){
            if(house[j] === applesPositions[k]) applesCount++;
        };
        for(let m = 0; m < orangesPositions.length; m++){
            if(house[j] === orangesPositions[m]) orangesCount++;
        };
    };
    console.log(`${applesCount}${'\n'}${orangesCount}`);

};

Just a few if statements will do.
Your answer to the problem is rather strange.
To find out if an apple or orange is between s and t is just a simple if statement.
if(apple[index] + appleTreePos >= s && apple[index] + appleTreePos <= t){
    // Apple is on the house.

// Same for the orange.

So you only need to loop over the apples and oranges once. Best is to have a single loop that does both the apples and oranges at the same time, then a second loop to do the remaining (if any) fruit.
There is no need to create any arrays. Your solutions is exponentially lots more work than is needed. For speed aim to reduce the number of times your code loops. Note that Array.map is the same as doing a for loop over each item, but much slower in this case.
NOTE I struck out exponentially and added "lots" as some have complained, I used it in the common vernacular (increasing rate of change) and not as in CS time complexity.
Your code loops over the length of the house, then the number of apples and oranges, and then you loop the length of the house times the number of apples plus oranges.
If the house is 5 units wide and there are 5 of each fruit you iterate over them a total of (house pos to array) 5 + (map each fruit array) 5 + 5, (counting) 5 * (5 + 5) for a total of 65 iterations. This is 60 times too many.
Example
I have not run this code so there could be a few typos, but the general structure is as described above.
This is faster than your solution, If it is fast enough to pass the test I do not know.
function countApplesAndOranges(houseLeft, houseRight, appleTreePos, orangeTreePos, apples, oranges) {
    var appleCount = 0;
    var orangeCount = 0;
    var i = 0;

    // find smallest group
    const len = Math.min(apples.length, oranges.length);

    // do apples and oranges
    while (i < len) {
        let pos = apples[i] + appleTreePos;
        appleCount += pos >= houseLeft && pos <= houseRight ? 1 : 0;
        pos = oranges[i++] + orangeTreePos;
        orangeCount += pos >= houseLeft && pos <= houseRight ? 1 : 0;
    }

    // if same number of fallen then return result
    if (apples.length === oranges.length) {
        console.log(`${appleCount}${'\n'}${orangeCount}`);
        return;
    }

    // if more apples have fallen the do remaining apples
    if (apples.length > len) {
        while (i < apples.length) {
            const pos = apples[i++] + appleTreePos;
            appleCount += pos >= houseLeft && pos <= houseRight ? 1 : 0;
        }
        // all done return result
        console.log(`${appleCount}${'\n'}${orangeCount}`);
        return;
    }

    // must be more oranges so do the rest of them and return result
    while (i < oranges.length) {
        const pos = oranges[i++] + orangeTreePos;
        orangeCount += pos >= houseLeft && pos <= houseRight ? 1 : 0;
    }
    console.log(`${appleCount}${'\n'}${orangeCount}`);
 }


Retrieve index of HTML element within parent I have a function I've been using for years that retrieves the index of an HTML element within its parent element, or, if a second argument is passed to it, the index of an HTML element within the collection of elements of the same name in that parent:

var childindex=function(node){
    var index=0;
    while(node.previousSibling){
        if(arguments.length==childindex.length||node.previousSibling.nodeName.toLowerCase()==arguments[1])index++;
        node=node.previousSibling;
    }
    return index;
}

However, in a recent overhaul of my JavaScript library, I went over all these old functions to try to improve upon them and this is what I came up with to replace the above:

let childindex=(node,...selectors)=>
        Array.from(selectors.length?node.parentNode.querySelectorAll(`:scope>${selectors.join(",:scope>")}`):node.parentNode.children).indexOf(node);

I'm pretty happy with the result, especially as it gives me the added ability to get the index of the element within any collection of selectors by passing each selector as a new argument and also ignores text nodes even when no additional arguments are passed to it.

I'm wondering, though, if anyone can see anywhere I can improve on it further, either in efficiency or conciseness of code.

Here's a working example to show it in action:

(()=>{
    let item=document.getElementById("getthis"),
        childindex=(node,...selectors)=>
            Array.from(selectors.length?node.parentNode.querySelectorAll(`:scope>${selectors.join(",:scope>")}`):node.parentNode.children).indexOf(node);

    //Gets index of `item` within all child elements of

    console.log(childindex(item)); // 5
    //Gets index of `item` within all child
 elements of

    console.log(childindex(item,"article")); // 3
    //Gets index of `item` within all child elements of
 with .class
    console.log(childindex(item,".class")); // 2
    //Gets index of `item` within all child
 &  elements of

    console.log(childindex(item,"header","article")); // 4

})();

I only use ternary operator with simple expressions.

Try to put them in independent methods. Otherwise, use the simple and boring (but expressive) if statement.