Search and filter in MVC3 Razor using Linq This code works fine, but I just don't like all the if else.  If I keep adding field to filter, it's going to get messy.

I am not using EF and cannot on this.

Controller code:

var books = bookRepository.GetDogs();
if (!String.IsNullOrEmpty(searchString))
{

    if (!String.IsNullOrWhiteSpace(searchString) && searchGender != String.Empty && searchHand != String.Empty)
    {
        books = books.Where(b => b.Name.ToUpper().Contains(searchString.ToUpper())
                                 && b.Gender == searchGender && b.Handedness == searchHand);
    }
    else if (!String.IsNullOrWhiteSpace(searchGender) && searchString == String.Empty && searchHand == String.Empty)
    {
        books = books.Where(b => b.Gender == searchGender);
    }
    else if (String.IsNullOrWhiteSpace(searchGender) && searchString != String.Empty && searchHand == String.Empty)
    {
        books = books.Where(b => b.Name.ToUpper().Contains(searchString.ToUpper()));
    }
}

return View(books);

My view:

<p>Find by Name : @Html.TextBox("searchString", ViewBag.CurrentFilter as string)

@Html.DropDownList("searchGender", new SelectList(ViewBag.BookNames), "-- Select All --")
@Html.DropDownList("searchHand", new SelectList(ViewBag.HandNames), "---Select All--")

Just put if, not else if so you can have a cumulative multi criteria search, and one condition in every if.

So every condition is easy to read, and if more than one criteria is filled, they will cumulate fine.

if (!String.IsNullOrWhiteSpace(searchString))
   books = books.Where(b => b.Name.ToUpper().Contains(searchString.ToUpper());

if (!String.IsNullOrWhiteSpace(searchGender))
   books = books.Where(b => b.Gender == searchGender);

if (String.IsNullOrWhiteSpace(searchHand))
   books = books.Where(b => b.Handedness == searchHand);


Faster tests with dependency analysis I have a performance issue causing test->code->test cycle to be really slow. The slowness is hard to avoid, since I am doing some heavy image processing, and I am trying to accelerate things a bit by not running functions when it is not needed. For ex: compute some numbers from big images -> serialize results in text files -> resume computations from text files.

Currently, I commit the text files containing the results to share them with the team, and let other people run the tests quickly. But I have to make sure that whenever one dependency is updated (src images, preprocessing functions, etc), those value are recomputed. I looked for a tool which would allow me to describe dependencies and production rules to automate the process and avoid committing data in the codebase. I thought about makefile, but I read a lot of negative advice against it. I found stuff like Scons and other build automation tools, but those are for building software, and they do not seem adapted for my task.

I decided to write a small lib to tackle this issue (my environment is mainly Python and some C). The goal was to have objects knowing the production rule for their own output, and aware of the other objects they depend upon. An object knows if its output is up-to-date by comparing the current MD5 checksum of the file against the last one (stored somewhere in a small temp file).

Am I reinventing a wheel here? Is there a tool out there I should use instead of a custom lib? If not, is this a good pattern for taking care of this problem?

import os
import path
from md5 import md5 as stdMd5

# path.root is the root of our repo
hashDir = os.path.join(path.root, '_hashes')

def md5(toHash):
    return stdMd5(toHash).hexdigest()

class BaseRule(object):
    '''base object, managing work like checking if results are up to date, and
    calling production rules if they are not'''

    outPath = None

    def __init__(self, func):
        self.func = func

    @property
    def hashPath(self):
        return os.path.join(hashDir, md5(self.outPath))

    def getOutHash(self):
        with open(self.outPath) as f:
            fileHash = md5(f.read())
        return fileHash

    def isUpToDate(self):
        if not os.path.exists(self.outPath):
            return False
        if not os.path.exists(self.hashPath):
            return False
        with open(self.outPath) as f:
            fileHash = self.getOutHash()
        with open(self.hashPath) as f:
            storedHash = f.read().strip()
        return storedHash == fileHash

    def storeHash(self):
        if not os.path.exists(hashDir):
            os.makedirs(hashDir)
        with open(self.hashPath, 'w') as f:
            f.write(self.getOutHash())

    def get(self):
        inputPathes = dict([(key, inp.get()) for key, inp in self.inputs.items()])
        if not self.isUpToDate():
            self.func(outPath=self.outPath, **inputPathes)
            self.storeHash()
        return self.outPath

class StableSrc(BaseRule):
    'source file that never change'

    inputs = {}

    def __init__(self, path):
        self.outPath = path

    def isUpToDate(self):
        return True

def rule(_inputs, _outPath):
    'decorator used to declare dependencies'

    class Rule(BaseRule):
        inputs = _inputs
        outPath = _outPath

    return Rule

def copyTest():
    'test function'

    import shutil

    @rule({'inp' : StableSrc('test.txt')}, 'test2.txt')
    def newTest(inp, outPath):
        print 'copy'
        shutil.copy(inp, outPath)

    @rule({'inp' : newTest}, 'test3.txt')
    def newNewTest(inp, outPath):
        print 'copy2'
        shutil.copy(inp, outPath)

    return newNewTest.get()

if __name__ == '__main__':
    copyTest() # will copy test.txt to test2.txt and test3.txt. If ran a second time, won't copy anything

# path.root is the root of our repo
hashDir = os.path.join(path.root, '_hashes')

Python style guide recommends that global constants are ALL_CAPS

class BaseRule(object):
    '''base object, managing work like checking if results are up to date, and
    calling production rules if they are not'''

    outPath = None

Python style guide recommends that class level constants are ALL_CAPS

    def isUpToDate(self):
        if not os.path.exists(self.outPath):
            return False
        if not os.path.exists(self.hashPath):
            return False
        with open(self.outPath) as f:
            fileHash = self.getOutHas()

You don't use the file you open here, instead you open the file inside getOutHash

        with open(self.hashPath) as f:
            storedHash = f.read().strip()
        return storedHash == fileHash

    def get(self):
        inputPathes = dict([(key, inp.get()) for key, inp in self.inputs.items()])

No need for the square brackets. They make it so that it produces a list. But since you are creating a dictionary there is no need to make a list first.

        if not self.isUpToDate():
            self.func(outPath=self.outPath, **inputPathes)
            self.storeHash()
        return self.outPath

class StableSrc(BaseRule):
    'source file that never change'

    inputs = {}

I know you'll never modify this, but inputs is an object level attribute above, and its clearest if you keep it that way.

    def __init__(self, path):
        self.outPath = path

    def isUpToDate(self):
        return True

def copyTest():
    'test function'

    import shutil

    @rule({'inp' : StableSrc('test.txt')}, 'test2.txt')
    def newTest(inp, outPath):
        print 'copy'
        shutil.copy(inp, outPath)

Usually you've got a small number of possible actions with different inputs/outputs for each use. Your decorator method assumes that each action will be different.

You aren't tracking dependencies amongst the rules. So you can't have some like .swig produces .cpp produces .o which is linked to produce .exe
Creating a class for every rule is odd. It would make more sense to create an object for each rule.
The only thing you've got over makefiles is your use of a hash rather then a timestamp


REST API PHP find by id Description: The web server retrieves data from file database (pdf, doc, 3d file, zip and rar) and financial program such as inventory, price, indexes, order status and so on.

I'm doing REST API PHP for the first time,  I've never used this API with PHP, I am an absolute beginner to REST.

Code returns data to cooperate with a www site.

First, I will test for a product that exists.

Output code 200:get_id2.php?id=39

id  "39"
caption "product01"
filename    "http://localhost/photo_gallery/public/files/images/image01.jpg"
description "testing Api"

Next, I will test for a product that does not exist.

Output code 404:get_id2.php?id=99999

message "No product."

I need your opinions.

Are there any code smells (can it be done better)?
Better to use prepared statement or not?
Use htmlspecialchars and urlencode for output?
What is good practice for REST API (PHP)?

Thank you in advance for your opinion.

File: get_id2.php

header("Access-Control-Allow-Origin: *");
header("Access-Control-Allow-Headers: access");
header("Access-Control-Allow-Methods: GET");
header("Access-Control-Allow-Credentials: true");
header("Content-Type: application/json; charset=UTF-8");

    include_once('config_setup.php');

    //$id = $_GET['id'];
    $id = isset($_GET['id']) ? $_GET['id'] : die();

    $sql = "SELECT * ";
    $sql .= "FROM photographs ";
    $sql .= "WHERE id='" . db_escape($db, $id) . "'";

    $result = mysqli_query($db, $sql);
    confirm_db_connect($result);

    $url = 'http://localhost/photo_gallery/public/files/images/';

    $allowed_tags = '

';

    $photo = mysqli_fetch_assoc($result);

            $id             = $photo['id'];
            $caption        = $photo['caption'];
            $filename       = $photo['filename'];
            $description2   = $photo['description2'];

    if ($photo['caption'] != null) {

        // create array
        $product_arr = array(
            "id"            => u(h($id)),
            "caption"       => h($caption),
            "filename"      => $url . h($filename),
            "description2"  => strip_tags($description2, $allowed_tags)
        );

        // set response code - 200 OK
        http_response_code(200);
        echo json_encode( $product_arr );

    } else {
        // set response code - 404 Not found
        http_response_code(404);
        // tell the user product does not exist
        echo json_encode(array("message" => "No product."));
    }

    mysqli_free_result($result);

    db_disconnect($db);

Functions

function h($string="") {
  return htmlspecialchars($string);
}
function u($string="") {
  return urlencode($string);
}
function confirm_result_set($result_set) {
    if (!$result_set) {
        exit("Database query failed.");
    }
  }
function db_escape($connection, $string) {
    return mysqli_real_escape_string($connection, $string);
  }
function db_disconnect($connection) {
    if(isset($connection)) {
      mysqli_close($connection);
    }
  }

get_id2.php is expected to produce a json string, so make sure that it always does that.
Don't use die() because then you are not providing an informative json response.
Don't be too informative when the expected id is missing or invald -- be clear about the problem, but also vague.
Validate the incoming id using isset() then ctype_digit(), if it passes those checks, then your script may advance to the querying step.
I prefer object-oriented mysqli syntax since it is less verbose, but you can keep using procedural if you like.
Calling confirm_db_connect($result); after executing the query doesn't make any sense.  If you are going to bother checking the connection, you should be doing that before trying to execute the query.
You only need four columns in your result set, so I advise that you list those columns explicitly in your SELECT clause so that you don't ask for more than you need.
I don't see any value in declaring single use variables from the result set.

$id             = $photo['id'];
$caption        = $photo['caption'];
$filename       = $photo['filename'];
$description2   = $photo['description2'];

Just use the $photo[...] variables directly when sanitizing/preparing the data.
Those single character sanitizing/preparing functions are a bad idea.  Developers that need to read your script in the future (including yourself) will have no chances of instantly understanding what these calls do.  This will require manually jumping to another file to look up what action is being executed.  Use more meaningful function names and don't try to save characters at the cost of losing comprehensibility.  In fact, it only hurts your code to write a custom function that replaces a single native function call -- just use the native call and your script and developers will be happier.
As of PHP7.4, strip_tags() allows an alternative declaration of allowed tags.  So your second parameter could look like this:

['div', 'img', 'h1', 'h2', 'p', 'br', 'strong', 'em', 'ul', 'li', 'table', 'td', 'tr', 'th', 'tbody']

Admittedly, when using proper spacing between values, the expression ends up being longer; but it can be sensibly broken into multiple lines to reduce overall width.
I prefer to write the negative response before the positive responses in my projects for consistency.  This just means moving the No product. outcome earlier in the code.
It is not necessary to manually free the result nor close the db connection when this script resolves; php will do this clean up for you automatically.
write json_encode() just once and pass a variable to it -- for the sake of DRY-ness.

My untested suggestions:

header("Access-Control-Allow-Origin: *");
header("Access-Control-Allow-Headers: access");
header("Access-Control-Allow-Methods: GET");
header("Access-Control-Allow-Credentials: true");
header("Content-Type: application/json; charset=UTF-8");

if (!isset($_GET['id']) || !ctype_digit($_GET['id'])) {
    $response = ['message' => 'Missing/Invalid identifier provided'];
} else {
    include_once('config_setup.php');
    $sql = "SELECT id, caption, filename, description2 FROM photographs WHERE id = " . $_GET['id'];
    $result = mysqli_query($db, $sql);
    if (!$result) {
        $response = ['message' => 'Please contact the dev team.'];
    } else {
        $photo = mysqli_fetch_assoc($result);
        if (!$photo) {
            $response = ['message' => 'No product.'];
        } else {
            $url = 'http://localhost/photo_gallery/public/files/images/';
            $allowed = '

';
            $response = [
                'id'           => urlencode(htmlspecialchars($photo['id'])),
                'caption'      => htmlspecialchars($photo['caption']),
                'filename'     => $url . htmlspecialchars($photo['filename']),
                'description2' => strip_tags($photo['description2'], $allowed),
            ];
        }
    }
}

http_response_code(isset($response['message']) ? 404 : 200);
echo json_encode($response);

Granted, some developers will not wish to write three nested condition blocks and might prefer to write early json exits.  I am choosing not to do this because my script is not excessively wide and it makes only one call of http_response_code() and json_encode() at the end.  "To each their own."  Restructure the snippet however you like.

An alternative structure with multiple returns to avoid arrowhead code:

function getResponse() {
    if (!isset($_GET['id']) || !ctype_digit($_GET['id'])) {
        return ['message' => 'Missing/Invalid identifier provided'];
    }
    include_once('config_setup.php');
    $sql = "SELECT id, caption, filename, description2 FROM photographs WHERE id = " . $_GET['id'];
    $result = mysqli_query($db, $sql);
    if (!$result) {
        return ['message' => 'Please contact the dev team.'];
    }
    $photo = mysqli_fetch_assoc($result);
    if (!$photo) {
        return ['message' => 'No product.'];
    }
    $url = 'http://localhost/photo_gallery/public/files/images/';
    $allowed = '

';
    return [
        'id'           => urlencode(htmlspecialchars($photo['id'])),
        'caption'      => htmlspecialchars($photo['caption']),
        'filename'     => $url . htmlspecialchars($photo['filename']),
        'description2' => strip_tags($photo['description2'], $allowed),
    ];
}

$response = getResponse();
http_response_code(isset($response['message']) ? 404 : 200);
echo json_encode($response);


Solving Knight's Travails Problem without using vector for position I'm trying to implement a solution to the The Odin Project - Project 2: Knight’s Travails in Clojure (Functional Programming) based on the solution posted by benjdelt.

I would like to know your opinion about what should I change to make the code clearer and more functional (as in Functional Programming) and how should I document it.

I don't know if this code is considered functional. I am relying heavily on recursions, is this ok? If not, how can I reduce (or use any other method) to make it more functional?

Note: Instead of using a vector with position x and y "[x, y]", I chose to represent the chess board by the square index, e.g. the first square is at index 0, the second square is at index 1 ... the last square is at index 63.

(def board
 "- Not used anywhere yet, just shows how the board looks like. -"
  [(vec (range 0 8))
   (vec (range 8 16))
   (vec (range 16 24))
   (vec (range 24 32))
   (vec (range 32 40))
   (vec (range 40 48))
   (vec (range 48 56))
   (vec (range 56 64))])

(defn knight-moves
  "Get all the allowed moves of a knight at `node` position.
  Excludes all the moves that go out of the board."
  ([node]
  (let [a (- node 17)
        b (- node 15)
        c (- node 10)
        d (- node 6)
        e (+ node 6)
        f (+ node 10)
        g (+ node 15)
        h (+ node 17)
        all-neighbours [a b c d e f g h]
        possible-neighbours (vec (remove #(or (neg? %) (> % 63)) all-neighbours))]
    {:neighbours possible-neighbours :current-node node :parent-node nil}))
  ([node parent]
   (let [a (- node 17)
         b (- node 15)
         c (- node 10)
         d (- node 6)
         e (+ node 6)
         f (+ node 10)
         g (+ node 15)
         h (+ node 17)
         all-neighbours [a b c d e f g h]
         possible-neighbours (vec (remove #(or (neg? %) (> % 63)) all-neighbours))]
     {:neighbours possible-neighbours :current-node node :parent-node parent})))

;; (knight-moves 35)

(defn nodes
  "Get all the neighbours and subsequent neighbours of `starting-node`.
  Given a `node`, return a vector of maps with all the possible neighbours and
  subsequent neighbours.
  Given a `node` and a `parent`, return a vector of maps with all the possible
  nodes and subsequent neighbours and save its parent location."
  ([starting-node]
  (let [movements (knight-moves starting-node)]
    ;; For each neighbours of `starting-node`, calculate its subsequent neighbours.
    (vec (for [node (:neighbours movements)]
           (knight-moves node (knight-moves starting-node))))))
  ([starting-node parent]
   (let [movements (knight-moves starting-node)]
     (vec (for [node (:neighbours movements)]
            (knight-moves node (knight-moves starting-node parent)))))))

;; (nodes 35)

(defn backtracking
  "Return the parent location of each subsequent node in `path`."
  ([final-node] (backtracking final-node []))
  ([final-node path]
   (if (:parent-node final-node)
     (recur (:parent-node final-node) (merge path (:current-node final-node)))
     (reverse (distinct path)))))

(defn find-path
  "Given a `starting-node`, calculate the path to the `goal-node`"
  ([starting-node goal-node]
   (find-path (knight-moves starting-node) goal-node []))
  ([starting-node goal-node path]
   ;; If not yet arrived at goal
   (if-not (= (:current-node starting-node) goal-node)
     ;; Create a new node with the current-node as parent and add it to path.
     ;; Iterate through all nodes to find the path.
     (let [new-node (nodes (:current-node starting-node) starting-node)
           path (concat path new-node)]
       (recur (first path) goal-node (vec (concat (rest path) new-node))))
     ;; If path found, return the path
     (backtracking starting-node))))

;; (find-path 27 28)

(defn -main
  "I don't do a whole lot ... yet."
  [& args]
  (find-path 27 28))

(-main)
; => (27 12 18 28)

I'll be commenting just on the code itself, not on improvements to the algorithm. I'll also be removing your docs for brevity here.

First, I see a ton of repetitious code here. board and knight-moves contain a lot of duplicate code that can be generalized and cleaned up.

First, board:

(defn board [side-length]
  (->> (range (* side-length side-length)) ; Create all the numbered cells
       (partition side-length) ; Split them into rows
       (mapv vec))) ; Turn the board into a 2D vector

I turned it into a function that accepts the side length, and made it so it can handle whatever valid values you throw at it.

The different arity versions of knight-moves are almost identical. The only difference is that parent defaults to nil. This can be easily reduced:

(defn knight-moves
  ([node]
   (knight-moves node nil)) ; Default parent to nil

  ([node parent]
   (let [a (- node 17)
         b (- node 15)
         c (- node 10)
         d (- node 6)
         e (+ node 6)
         f (+ node 10)
         g (+ node 15)
         h (+ node 17)
         all-neighbours [a b c d e f g h]
         possible-neighbours (vec (remove #(or (neg? %) (> % 63)) all-neighbours))]
     {:neighbours possible-neighbours :current-node node :parent-node parent})))

That's still super bulky though. I bet there's a way to handle this without hardcoding all the numbers, but I can't think of it right now. A significantly more succinct version however could be:

(defn knight-moves
  ([node]
   (knight-moves node nil)) ; Default parent to nil

  ([node parent]
   (let [offsets [6 10 15 17]
         pos (map #(+ node %) offsets) ; Just map over the offsets so you aren't writing them twice.
         neg (map #(- node %) offsets)

         ; I'm reversing neg here so it matches your previous output. It seems valid without,
         ;  it just gives different answers.
         all-neighbours (vec (concat (reverse neg) pos)) ; Then stick the two together.
         possible-neighbours (vec (remove #(or (neg? %) (> % 63)) all-neighbours))]
     {:neighbours possible-neighbours :current-node node :parent-node parent})))

Note how I'm reversing neg. If I don't, the -main will return different results. The results without the call to reverse should be valid, they're just different from what you were getting before.

In nodes, I think you're trying too hard to use both arities of knight-moves, which is leading to duplicate code. Why not just pass nil? I'd also use mapv here instead of for. for is really only beneficial when you need to make use of it's ability to generate combinations, or make use of :when/:while:

(defn nodes
  ([starting-node]
   (nodes starting-node nil)) ; Default to nil, just like before

  ([starting-node parent]
   (let [movements (knight-moves starting-node)]
     ; You could use a full fn here instead of relying on the function macro
     ;  if you want a better identifier than %
     (mapv #(knight-moves % (knight-moves starting-node parent))
           (:neighbours movements)))))

I honestly couldn't find anything glaringly wrong with the rest of the code, so I left it as-is. This is the final result that I ended up with:

(ns irrelevant.knights.corrected)

(defn board
  [side-length]
  (->> (range (* side-length side-length)) ; Create all the numbered cells
       (partition side-length) ; Split them into rows
       (mapv vec))) ; Turn the board into a 2D vector

(defn knight-moves
  ([node]
   (knight-moves node nil)) ; Default parent to nil

  ([node parent]
   (let [offsets [6 10 15 17]
         pos (map #(+ node %) offsets) ; Just map over the offsets so you aren't writing them twice.
         neg (map #(- node %) offsets)

         ; I'm reversing neg here so it matches your previous output. It seems valid without,
         ;  it just gives different answers.
         all-neighbours (vec (concat (reverse neg) pos)) ; Then stick the two together.
         possible-neighbours (vec (remove #(or (neg? %) (> % 63)) all-neighbours))]
     {:neighbours possible-neighbours :current-node node :parent-node parent})))

(defn nodes
  ([starting-node]
   (nodes starting-node nil)) ; Default to nil, just like before

  ([starting-node parent]
   (let [movements (knight-moves starting-node)]
     ; You could use a full fn here instead of relying on the function macro
     ;  if you want a better identifier than %
     (mapv #(knight-moves % (knight-moves starting-node parent))
           (:neighbours movements)))))

(defn backtracking
  ([final-node] (backtracking final-node []))
  ([final-node path]
   (if (:parent-node final-node)
     (recur (:parent-node final-node) (merge path (:current-node final-node)))
     (reverse (distinct path)))))

(defn find-path
  ([starting-node goal-node]
   (find-path (knight-moves starting-node) goal-node []))
  ([starting-node goal-node path]
    ;; If not yet arrived at goal
   (if-not (= (:current-node starting-node) goal-node)
     ;; Create a new node with the current-node as parent and add it to path.
     ;; Iterate through all nodes to find the path.
     (let [new-node (nodes (:current-node starting-node) starting-node)
           path (concat path new-node)]
       (recur (first path) goal-node (vec (concat (rest path) new-node))))
     ;; If path found, return the path
     (backtracking starting-node))))

(defn -main
  "I don't do a whole lot ... yet."
  [& args]
  (find-path 27 28))

(-main)
; => (27 12 18 28)


Exclusive and inclusive filtering of attributes Here is a simple filter function I have:

function filter (attributes, data, include) {
    if (include) {
        let res = {}
        if (typeof attributes === 'string') {
            res[attributes] = data[attributes]
            return res
        }
        if (attributes.length) {
            attributes.forEach(attr => res[attr] = data[attr])
        }
        return res
    } else {
        if (typeof attributes === 'string') {
            delete data[attributes]
            return data
        }
        if (attributes.length) {
            attributes.forEach(attr => delete data[attr])
        }
        return data
    }
}

As you can see, these parts are same:

if (typeof attributes === 'string') {
    // some code
    return // some variable
}
if (attributes.length) {
    attributes.forEach(attr => /* some code */)
}
return // some variable

What is the best way to remove this duplicated code?

To @Tushar's point, those 2 code blocks are not similar. In fact, I would advise you to turn that function in to 2 functions with a better name. Especially since your function optionally depending on include modifies data which is not a good design. It should either always generate a new object, or always modify data. Probably it should always generate a new object.

Furthemore for greater flexibility, I would not check for attributes.length but for attributes.forEach, this way you know for sure the function/method is defined.

In Lodash the first function would be called pick, and the second function would be omit.

Thinking about this, if you are always going to build a new object, you could remove repetitive coding:

function filter(attributes, data, include) {
    'use strict';

    let keys = Object.keys(data),
        res = {};
    //Turn attributes into an array if needed
    attributes = (typeof attributes === 'string') ? [attributes] : attributes;
    keys.forEach(key => {
        //If its an attribute we include, get it
        //If its an attribute we dont exclude, get it
        if ((include && attributes.indexOf(key) != -1) ||
            (!include && attributes.indexOf(key) == -1)) {
            res[key] = data[key];
        }
    });
    return res;
}


Enumerating smart card terminals asynchronously in ViewModel constructor I started a project the other day and started going down the path of using async and await as I have never used it before and it seems like a good match. The app that I'm making is a WPF solution using MVVM. It is going to read from a smart card and show the information that was saved to the card. It is part of a demo/concept. As it stands right now the View has next to nothing in it so I'm not going to show it. The test and the ViewModel are of the interest. So let me begin with the test as it is where I first asked this question in my head. I'm using NUnit 3.2 and Moq 4.2

MainWindowViewModelTests.cs

[TestFixture]
public class MainWindowViewModelTests
{
    private Mock<ITerminalFactory> _factory;

    [SetUp]
    public void Setup()
    {
        _factory = new Mock<ITerminalFactory>();
    }

    [Test]
    public void WhenNoTerminalIsConnected_ErrorMessageIndicatesNoTerminal()
    {
        _factory.Setup(f => f.EnumerateTerminals()).Returns(async ()=>
        {
            await Task.Yield();
            return new ITerminal[0];
        });

        var target = GetClassUnderTest();

        Assert.That(target.Error, Is.EqualTo("No Terminals"));
    }

    [Test]
    public void WhenAtleastOneTerminalIsConnected_NoErrorMessages()
    {
        _factory.Setup(f => f.EnumerateTerminals()).Returns(async () =>
        {
            await Task.Yield();
            return new ITerminal[] { Mock.Of<ITerminal>()};
        });

        var target = GetClassUnderTest();

        Assert.That(target.Error, Is.Null.Or.Empty);
        Assert.That(target.Terminals.Count, Is.EqualTo(1));
    }

    private MainWindowViewModel GetClassUnderTest()
    {
        var target = new MainWindowViewModel(_factory.Object);
        while (!target.IsLoaded)
        {
            System.Threading.Thread.Sleep(1);
        }
        return target;
    }
}

MainWindowViewModel.cs

public class MainWindowViewModel : HandleErrorsViewModel
{
    public ICollection<ITerminal> Terminals { get; }
    public bool IsLoaded { get; private set; }

    public MainWindowViewModel(ITerminalFactory terminalFactory)
    {
        Terminals = new ObservableCollection<ITerminal>();
        PopulateTerminals(terminalFactory);
    }

    private async void PopulateTerminals(ITerminalFactory terminalFactory)
    {
        IEnumerable<ITerminal> terminals = await terminalFactory.EnumerateTerminals();
        terminals.ToList().ForEach(Terminals.Add);

        if (Terminals.Count == 0)
            Error = "No Terminals";

        IsLoaded = true;
    }
}

HandleErrorsViewModel.cs and BaseViewModel

omitted as it is just a wrapper around IDataErrorInfo and a BaseViewModel which is just a wrapper around INotifyPropertyChanged

ITerminal/ITerminalFactory

public interface ITerminal : System.IDisposable
{
    string Name { get; }

    IChippedCard Connect();
    void Disconnect();
}
public interface ITerminalFactory
{
    Task<IEnumerable<ITerminal>> EnumerateTerminals();
}

And that is as far as I am right now. I've neve used async and await before. I've never used a Task as a return method before, and I always desire to use the best practices so that is why I am here. So please fire away and let me know thoughts/concerns/tips/ideas.

I saw that you use in your test async/await, so you can do the GetClassUnderTest in the same way. Return a Task and wait for it asynchronously.

Also you can change the inner while and Thread.Sleep to something else.
I'd change the PopulateTerminals to public (it doesn't change any inner state except for the Terminals collection and you can do a check before the population that verify if is already populated) and then from your test call it.

I think make it public is the best way, but if you prefer not to do it, consider to use some signal to indicate about IsLoading and not do a useless while.

I don't like that EnumerateTerminals returns IEnumerable<ITerminal>. I don't know what is the logic of the enumerating, but I guess you return List from there so change also the return value to Task<List<ITerminal>>. If you indeed return IEnumerable, this can be a problem because the laziness.

In the VM why is your collection ICollection and not ObservableCollection? If you do it purposely for some reason (sorting) it's OK but if not change it to concrete type (its clearer and avoid conversions).

The ITerminalFactory can be injected to VM as property for example and then you don't need to pass it to populate the collection.


K-nearest neighbours in C# for large number of dimensions I'm implementing the K-nearest neighbours classification algorithm in C# for a training and testing set of about 20,000 samples and 25 dimensions.

There are only two classes, represented by '0' and '1' in my implementation. For now, I have the following simple implementation:

// testSamples and trainSamples consists of about 20k vectors each with 25 dimensions
// trainClasses contains 0 or 1 signifying the corresponding class for each sample in trainSamples
static int[] TestKnnCase(IList<double[]> trainSamples, IList<double[]> testSamples, IList<int[]> trainClasses, int K)
{
    Console.WriteLine("Performing KNN with K = "+K);

    var testResults = new int[testSamples.Count()];

    var testNumber = testSamples.Count();
    var trainNumber = trainSamples.Count();
    // Declaring these here so that I don't have to 'new' them over and over again in the main loop,
    // just to save some overhead
    var distances = new double[trainNumber][];
    for (var i = 0; i < trainNumber; i++)
    {
       distances[i] = new double[2]; // Will store both distance and index in here
    }

    // Performing KNN ...
    for (var tst = 0; tst < testNumber; tst++)
    {
        // For every test sample, calculate distance from every training sample
        Parallel.For(0, trainNumber, trn =>
        {
            var dist = GetDistance(testSamples[tst], trainSamples[trn]);
            // Storing distance as well as index
            distances[trn][0] = dist;
            distances[trn][1] = trn;
        });

        // Sort distances and take top K (?What happens in case of multiple points at the same distance?)
        var votingDistances = distances.AsParallel().OrderBy(t => t[0]).Take(K);

        // Do a 'majority vote' to classify test sample
        var yea = 0.0;
        var nay = 0.0;

        foreach (var voter in votingDistances)
        {
            if (trainClasses[(int)voter[1]] == 1)
               yea++;
            else
               nay++;
        }
        if (yea > nay)
            testResults[tst] = 1;
        else
            testResults[tst] = 0;

    }

    return testResults;
}

// Calculates and returns square of Euclidean distance between two vectors
static double GetDistance(IList<double> sample1, IList<double> sample2)
{
    var distance = 0.0;
    // assume sample1 and sample2 are valid i.e. same length

    for (var i = 0; i < sample1.Count; i++)
    {
        var temp = sample1[i] - sample2[i];
        distance += temp * temp;
    }
    return distance;
}

This takes quite a bit of time to execute. On my system it takes about 80 seconds to complete. How can I optimize this, while ensuring that it would also scale to larger number of data samples? As you can see, I've tried using PLINQ and parallel for loops, which did help (without these, it was taking about 120 seconds). What else can I do?

I've read about KD-trees being efficient for KNN in general, but every source I read stated that they're not efficient for higher dimensions.

I also found this Stack Overflow discussion about this, but it seems like this is 3 years old, and I was hoping that someone would know about better solutions to this problem by now.

I've looked at machine learning libraries in C#, but for various reasons I don't want to call R or C code from my C# program, and some other libraries I saw were no more efficient than the code I've written. Now I'm just trying to figure out how I could write the most optimized code for this myself.

I cannot reduce the number of dimensions using PCA or something. For this particular model, 25 dimensions are required.

Also, I did track the execution time using a profiler, and it seems that more than 60% of the runtime is spent in the GetDistance() function, which is why I was wondering whether there exists a different algorithm using a different data structure that does this more optimally.

var distances = new double[trainNumber][];
for (var i = 0; i < trainNumber; i++)
{
   distances[i] = new double[2]; // Will store both distance and index in here
}

This is a code smell. You shouldn't use a jagged double array to store an array of distances and indexes. Despite the comment, what you're doing is unclear, and it's very confusing to have a variable named distances that stores both distances and indexes. The only justification for this would be if you actually had hard profiling evidence that it caused a significant speedup.

Make a separate class (or struct, if you're worried about overhead) with members double distance; int index; and then trainInfo (the former distances) should just be a trainNumber-sized array of that type.

Also, since you only need the top K elements, you don't need to sort the whole list (n log n time). You ought to be able to do it with a partial sort (actual code sample) which is almost linear-speed. There are also parallel algorithms for this; you could probably get a speedup using PLINQ with a custom aggregate.

On to the next refactoring.

foreach (var voter in votingDistances)
{
    if (trainClasses[(int)voter[1]] == 1)
       yea++;
    else
       nay++;
}

This code is also crying out for LINQ. How about

yea = votingDistances.AsParallel().Count(voter=>trainClasses[voter.index] == 1);
nay = votingDistances.Count - yea;

And I'd refactor this:

for (var i = 0; i < sample1.Count; i++)
{
    var temp = sample1[i] - sample2[i];
    distance += temp * temp;
}

to this:

var differences = sample1.AsParallel().Zip(sample2,(s1,s2)=>s1-s2);
distance = differences.Sum(x=>x*x);

though you could get improved performance from a custom aggregate.


Python script audibly represents itself I was just messing around with python making wav files; when I came up with this (what I think is) pretty neat script. It takes each character and represents it with a tone.

import wave
import struct
from math import sin
import os

RATE = 44100
maxVol = 2 ** 15 - 1.0  # maximum amplitude

noise_output = wave.open('noise2.wav', 'w')
noise_output.setparams((2, 2, 44100, 0, 'NONE', 'not compressed'))

a = lambda i, f: sin(f * i / RATE)
f = open(os.path.basename(__file__)).read()
values = [[a, (ord(l))] for l in f]
result = []
count = 0

for i, j in values:
    count_end = count + RATE / 8
    while count < count_end:
        packed_value = struct.pack('h', maxVol * i(count, j * 20 + 1000))
        result.append(packed_value)
        result.append(packed_value)
        count += 1

result_str = ''.join(result)
noise_output.writeframes(result_str)

noise_output.close()

I found the original here. The original just makes a constant tone. Runs fine with Python 3. I would love to see a decoder for this; but I don't know enough about calculating frequencies.

You should not use single letter variables.
Rather than a lambda you can create an actual function and pass its name.
There are still several magic numbers. What is 8, 20, etc.?
maxVol should also be uppercase, and should probably be named MAXIMUM_AMPLITUDE to avoid the need for the comment.
Rather than duplicating result.append you can use a separate function with a counter for how many times you want to repeat it.
Encapsulate functionality in functions or objects. Initialising noise_output (or the output as an object), the for loop and outputting/closing the stream are candidates.
Check whether it's possible to use with wave.open('noise2.wav', 'w') as noise_output: or similar.
Rather than reading the whole file into memory you can read a chunk at a time to speed up the program.
Some variable names are difficult to understand. values is almost meaningless (all it tells me is that it's an array-like structure), results likewise (how about something like audio_bytes?).


Listing links for categories I've developed some PHP that grabs all of a business listing's categories and then displays just the children categories. This is built on Wordpress. I'm still in the learning process and would appreciate some assistance here. Any optimizations you can suggest I make to my code would be really helpful. Please note, if it's really technical, there's a good chance it'll go over my head at this point, so examples are appreciated.

<?php

$permalink = get_permalink( $id );

$seo = get_the_title()." : ";

$Category_links = ' Found in the ';

$term_list_category = wp_get_post_terms(get_the_ID(), 'listings_categories', array("fields" => "ids"));

//THIS CODE REMOVES PARENTS FROM BEING DISPLAYED IN THE LISTING CATEGORIES
foreach ($term_list_category as $k=>$term) {
    $children = get_term_children( $term, 'listings_categories');
if ($children)
    unset($term_list_category[$k]);
 }

$i = 0;

$count = count($term_list_category);

if ( $count > 0 ){

    foreach ( $term_list_category as $term_category ) {

        $thisCat = get_term_by( 'id', $term_category, 'listings_categories');

        $url = '{'slug'}.'" class="listing-links-cat" href="#" title="'.$thisCat->{'name'}.'" >'.$thisCat->{'name'}.'';

        $i ++;

        $seo .= " " . $thisCat->{'name'} . "";

        $Category_links .= " " . $url . "";

        if($count-1 == $i){
            $Category_links .= " and ";  $seo .= ", ";
        }elseif($count > 1 && $count !== $i){
            $Category_links .= ", ";  $seo .= ", ";
        }

    }
    $Category_links .= " Categories";
?>

<? echo $Category_links;  ?>

One thing that is quite important to really understand. It's something that is said all too often, but it's very true:

  Premature optimzation is the root of all evil

By your own admission, you're using wordpress, and post some 30-odd lines of PHP code, asking us what you could do to improve performance. Well, I can tell you that some parts of your code are perhaps not as optimal as they could be, but really: you won't notice any difference even if your code were written as efficiently as possible. The real overhead still is the fact that you're using wordpress, which means there's a ton of code being executed you've probably not even looked at.

Is that a bad thing? No, of course not. What does that mean for you? Well, that you shouldn't be focussing on writing the optimal code just yet. First, you should define "optimized code".

Code can be optimized for:

Readability. Very important if you're collaborating, and if ever you have to revisit a piece of code half a year after writing it
Debugging. Code that is optimized for debugging is well documented, well structured, and has a lot of functions that all do a distinct task, and do it well, without an awful lot of complexity.
Speed. This is, I think what you're after, but at this stage, you shouldn't be. You really shouldn't be thinking about this yet, because code written for performance tends to be error prone, harder to read, and difficult to debug. You can optimize code once you've thoroughly tested every single expression of every statement several times over.
Though we seldom see this kind of optimization today, not too long ago, code could also be optimized for storage. Back in the day where a 10Mb drive was worth the equivalent of a years' salary. This point is kind of irrelevant, but just for completeness' sake, I thought I'd mention is.

Having said that, I really do feel as though you should be focussing on writing clean code. Code that is easy to debug, easy to understand and is more self-documenting. I'll go through your code bit by bit, and give a couple of tips to optimize both in terms of readability and execution-time performance (though, again, in terms of speed, there won't be much between the two versions).

$permalink = get_permalink( $id );
$seo = get_the_title()." : "; //more on this var later
//camelCase the vars, it's more common and for many, more readable
$categoryLinks = ' Found in the ';
//spread out function calls that get too long
$termListCategory = wp_get_post_terms(
    get_the_ID(),//this way, you can comment on each param you're passing
    'listings_categories',//like so: this is some string
    array(
        'fields' => 'ids'//assoc array argument specifying something...
    )
);
/** only display children, if term has children, remove it from the array
 * Just like it might be a good idea to use multi-line comments
 * and don't use SHOUT CASE in documentation, why would you?
 */
foreach ($termListCategory as $k => $term)
{//whitespace is good, use it more
    //this assignment is a bit redundant
    $children = get_term_children(
        $term,
        'listings_categories'
    );
    //this is technically valid, but it's not safe
    //think back to apple's infamous goto fail; bug, use brackets
    if ($children)
    {
        unset($termListCategory[$k]);
    }
}

an alternative way to write this loop would be this:

foreach ($termListCategory as $k => $term)
{
    if (get_term_children($term, 'listings_categories'))
    {
        unset($termListCategory[$k]);
    }
}

That, to me at least looks a lot cleaner. This is assuming you can't do anything about the return value of wp_get_post_terms like: structuring the data differently, using different query methods and all that, so that you don't have to fetch data that you're just going to filter from the array anyway.

All that asside, let's continue:

$i = 0;//This doesn't belong. $i is a loop var (99% of the time), why declare + initialize it here?

$count = count($term_list_category);//this is a pointless count call, just write this:
if ($termListCategory) {
//instead of: if ( $count > 0 ){
//an empty array is falsy
    foreach ( $termListCategory as $termCategory )
    {
        $thisCat = get_term_by(
            'id',
            $termCategory,
            'listings_categories'
        );

        //Separate this out into a function!!
        $url = '{'slug'} . '" class="listing-links-cat" href="#" title="' . $thisCat->{'name'} . '" >' . $thisCat->{'name'} . '';

I've always gotten the hebejeebies when I see string concatenation like this. Please, separate it out into a function, with clear type-hints (if possible), and don't access properties using string constants the way you do. I'd simply write this:

function getUrlFromObject(stdClass $obj, $id)
{
    $url = 'slug . '" class="listing-links-cat" '
        . 'href="#" title="' . $obj->name . '" >' . $obj->name . '';
    return $url;
}
//call like so, inside the foreach
$url = getUrlFromObject($thisCat, $termCategory);

Well, since you've asked us not to go too technical, I've kept the string concatenation business like you're using, but just in case you want to look into something new, I'd actually use sprintf  here, I think, or an actual DOM parser/builder (like DOMDocument or SimpleXMLElement). Anyway, here's the sprintf version:

function getUrlFromObject(stdClass $obj, $id)
{
    return sprintf(
        '%3$s',
        $id,
        $obj->slug,
        $obj->name
    );
}

To my eye at least, that looks a lot cleaner than what you have now, but that might be a matter of personal preference, just like your adding spaces between a variable and the postfix-increment operator:

        $i ++;

Whereas I'd write either ++$i; or $i++;. Some people might claim that ++$i; is faster, but honestly: that's a myth that comes from the C-world where there can be a difference between ++i and i++ if the compiler optimization is switched off. Either way, in PHP it doesn't matter, but it's more common to see $i++; anyway (without the spaces).

        $seo .= " " . $thisCat->{'name'} . "";

        $Category_links .= " " . $url . "";

        if($count-1 == $i){
            $Category_links .= " and ";  $seo .= ", ";
        }elseif($count > 1 && $count !== $i){
            $Category_links .= ", ";  $seo .= ", ";
        }

    }
    $Category_links .= " Categories";
?>

I cannot, for the life of me, understand why you're closing the PHP tags, only to open them again and echo here:

<? echo $Category_links;  ?>

Lastly, what you're doing with these $seo and $Category_links vars is just very error-prone, and clunky-looking. I'd suggest you turn them into arrays, and simply add new values to them. An array of strings can be imploded using any delimiter you like so replace this:

$seo = get_the_title() . ' : ';
//...more stuff
foreach ($termListCategory as $termCategory)
{
}

With something more like:

$seo = array(
    get_the_title() . ' :',
);
$categoryLinks = array();//create empty array
//...more stuff
foreach ($termListCategory as $termCategory)
{
    $seo[] = $thisCat->name;//add this to the seo ARRAY
    $categoryLinks[] = getUrlFromObject($thisCat, $termCategory);
}
$seoString = implode(', ', $seo);//turns array into comma-separated string
$categoryLinkString = implode(', ', $categoryLinks) . ' and Categories';
//or simply directly output the string:
echo implode(', ', $categoryLinks) . ' and Categories';

Is this code faster? Well, probably not. Sure, I'm not calling count for no obvious reason, and I don't use string concatenation that often, but I am manipulating two arrays, which comes at a cost of its own. Using a custom function, and calling that isn't free either, but let's be honest: that's not going to be your major bottleneck. Besides, the upshot is: code that is a lot easier on the eyes:

/**
 * String object into link, pass object, then term (id)
 *
 * @param stdClass $obj
 * @param string $id
 * @return string
 */
function getUrlFromObject(stdClass $obj, $id)
{
    return sprintf(
        '%3$s',
        $id,
        $obj->slug,
        $obj->name
    );
}

$permalink = get_permalink( $id );
$seo = array(
    get_the_title() . ' :',
);
//this var was a string, but we've moved it to where we echo the imploded links array (before the imploded array, see below)
$categoryLinks = array();
$termListCategory = wp_get_post_terms(
    get_the_ID(),
    'listings_categories',
    array(
        'fields' => 'ids'
    )
);
foreach ($termListCategory as $k => $term)
{
    //if term does NOT have children, process it
    if (!get_term_children($term, 'listings_categories'))
    {
        $seo[] = $thisCat->name;//add this to the seo ARRAY
        $categoryLinks[] = getUrlFromObject($thisCat, $termCategory);
    }
    //if you want, you can add
    // an else { unset($termListCategory[$k]); } here
    // I just don't see the point of it here
}
$seoString = implode(', ', $seo);//turns array into comma-separated string
//Add the "found in the" bit here
echo 'Found in the ', implode(', ', $categoryLinks) . ' and Categories';

So after putting it all together I have:

Removed a lot of concatenation
Removed some function calls (count, for example)
Added other function calls
Added a function definition
Removed the secondary loop
Removed a fairly pointless if around the second loop
IMO simplified the code quite a lot, and made it easier to debug
Added some documentation


Simplify value selection I have two different data inputs for the same value, I want to use the non empty one if one of them is, to return the empty string if both are empty, and to flag an error if both are not empty but differ and return one of them.

Empty for my criteria matches the meaning of string.IsNullOrEmpty().

And it's okay that it's written as a property (don't worry about that part.)

This works but feels overly verbose. Can you think of any alternatives?

    public string Callkey
    {
        get
        {
            if (string.IsNullOrEmpty(AS_CALL_KEY) && !string.IsNullOrEmpty(CALLKEY))
            {
                return CALLKEY.Trim();
            }
            if (!string.IsNullOrEmpty(AS_CALL_KEY) && string.IsNullOrEmpty(CALLKEY))
            {
                return AS_CALL_KEY.Trim();
            }
            if (!string.IsNullOrEmpty(AS_CALL_KEY) && !string.IsNullOrEmpty(CALLKEY))
            {
                if (!AS_CALL_KEY.Trim().Equals(CALLKEY.Trim()))
                {
                    _error = true;
                }
                return AS_CALL_KEY.Trim();
            }
            if (string.IsNullOrEmpty(AS_CALL_KEY) && string.IsNullOrEmpty(CALLKEY))
            {
                return "";
            }
            _error = true;
            return "THISNEVERHAPPENS";
        }
    }

You have four cases and each of them behaves differently, so you actually need to have four different branches. But you can simplify your code by extracting the checks into variables and using nested ifs:

public string CallKey
{
    get
    {
        bool callKeyEmpty = string.IsNullOrEmpty(CALLKEY);
        bool asCallKeyEmpty = string.IsNullOrEmpty(AS_CALL_KEY);

        if (callKeyEmpty)
        {
            if (asCallKeyEmpty)
                return "";
            else
                return AS_CALL_KEY.Trim();
        }
        else
        {
            if (asCallKeyEmpty)
                return CALLKEY.Trim();
            else
            {
                if (AS_CALL_KEY.Trim() != CALLKEY.Trim())
                    _error = true;
                return AS_CALL_KEY.Trim();
            }
        }
    }
}

As an added benefit, you can now remove the “this never happens” part.


Make all additional threads return from a method using Interlocked.Increment I need to ensure a method is executed only by one thread at a time. Contrary to what most of the synchronization primitives do, I don't need other threads to wait, I want them to return from the method immediately if it is already executed by another thread. Additional complexity is that the method uses async/await calls.

I implemented it using Interlocked.Increment. I tested in a simple test scenario and it seems to be working. However, I'm not sure, is it a correct implementation and will it work in all cases? Or maybe there is a simpler way to achieve the same goal?

private int _threadsCount = 0;

public async Task Method()
{
    if (Interlocked.Increment(ref _threadsCount) > 1)
    {
        Interlocked.Decrement(ref _threadsCount);
        return;
    }

    try
    {
        await DoWork();
    }
    finally
    {
        Interlocked.Decrement(ref _threadsCount);
    }
}

Yes, your code should work. Though the first part can be simplified to:

if (Interlocked.CompareExchange(ref _threadsCount, 1, 0) != 0)
{
    return;
}


Redirecting arriving customers to the nearest empty queue I have a task to make a checkout line controller. First it receives, as input, an amount of checkouts and commands. Then commands that are C for coming and L for leaving, and after those the number of the checkout they are coming/leaving. Also, if there is someone at the checkout, they should be sent to the nearest empty one. If 2 are equally near, the one with the lowest number is chosen.

Input could be:

3 5
C 1
C 2
L 1
C 1
C 1

As output it just tells the checkout where each arriving customer is served. So output would be:

1
2
1
3

Code:

#include <iostream>
#include <vector>
using namespace std;

int main () {
    int n, m, checkout;
    char action;
    vector <int> output (0);

    cin >> n >> m;

    vector <bool> state (n,false); //

    for (int i = 0; i < m; i++) {
        cin >> action >> checkout;
        checkout--; //Aligns checkout number with vectors number

        if (action == 'C') { //If someone comes
            if (state[checkout] == false) { //If the checkout he comes to is empty
                state[checkout] = true; // If it is, it's not anymore
                output.push_back(checkout + 1);
            } else { // If it isn't, check which is the closest empty one
                for (int i = 1; i < n; i++) { //Loops through below and above checkouts
                    if (checkout - i >= 0) {
                        if (state[checkout - i] == false) {
                            state[checkout - i] = true;
                            output.push_back(checkout - i + 1);
                            break;
                        }
                    }
                    if (checkout + i < n) {
                        if (state[checkout + i] == false) {
                            state[checkout + i] = true;
                            output.push_back(checkout + i + 1);
                            break;
                        }
                    }
                }
            }
        }
        if (action == 'L') {
            state[checkout] = false;
        }
    }
    for ( int j = 0; j < output.size(); j++) {
        cout << output[j] << endl; //Outputs after all input is over (somone said I should do this)
    }
}

My program runs too slowly. I use codejudge.net to check if it is working properly. I have already improved it a lot, from what I started with, but it still doesn't cut it.

This question was originally posted on Stack Overflow.

I see some things that may help you improve your code.

Don't abuse using namespace std

Putting using namespace std within your program is generally a bad habit that you'd do well to avoid.

Check for errors

It may not be part of your assignment, but you should know that real programs should check for errors and handle them appropriately.

Use a function

Right now, everything is in main but we could make things a little more clear by creating a function.  Specifically, as each customer arrives at a register, they need to find the nearest open register.  This suggests a function: findNearestOpenRegister which could take a proposed register as an argument.  However, this function would have to get a number of other things passed to it which suggests a further refinement, described in the next suggestion.

Use an object

We can make the main code cleaner by using an object to represent the array of cashiers at a store.  I'd probably write it like this:

// represents a linear array of checkout registers at a store
class Checkout {
public:
    Checkout(int registerCount)
    : unoccupied(registerCount, true)
    {}
    int arrive(int r) {
        int lo, hi;
        for (lo = hi = r; lo >= 0 || hi < unoccupied.size(); --lo, ++hi) {
            if (lo >= 0 && unoccupied[lo]) {
                unoccupied[lo] = false;
                return lo;
            } else if (hi < unoccupied.size() && unoccupied[hi]) {
                unoccupied[hi] = false;
                return hi;
            }
        }
        return ERROR;   // this is an error condition!
    }
    int leave(int r) {
        if (unoccupied[r])
            return ERROR;
        unoccupied[r] = true;
        return r;
    }
    static constexpr int ERROR = -1;
private:
    std::vector <bool> unoccupied;
};

Now the main function can use the object:

int main () {
    int n;
    int m;

    std::cin >> n >> m;
    Checkout reg(n);

    for (int i = 0; i < m; ++i) {
        int checkout;
        char action;
        std::cin >> action >> checkout;
        --checkout; // checkout is 1-based in input, zero-based internally
        switch(action) {
            case 'C':  // someone comes to the register
                checkout = reg.arrive(checkout);
                if (checkout == Checkout::ERROR) {
                    std::cout << "Error: no free registers in line " << i+2 << ".\n";
                    return 1;
                }
                std::cout << ++checkout << '\n';
                break;
            case 'L':  // someone leaves the register
                checkout = reg.leave(checkout);
                if (checkout == Checkout::ERROR) {
                    std::cout << "Error: attempt to leave free register in line " << i+2 << ".\n";
                    return 1;
                }
                break;
            default:
                std::cout << "Error: unknown action '" << action << "' in line " << i+2 << ".\n";
                return 1;
        }
    }
}

Note that error checking has been added, as per the earlier suggestion.  Also note that rather than storing the output, this version simply emits it as it's generated, avoiding the additional data structure that is otherwise needed.

Profile the program

If you find that the program is not performing as fast as you need, the first step should be to try to find out where the program is spending its time.  This can be done by several different methods.  I tend to use gprof.  Among the possibilities are using a different data structure.  See this question for an example.  Another possibility is to execute std::ios_base::sync_with_stdio(false);, but whether that speeds thing up or not is uncertain until you measure.


Mathematical boundaries (audio meter) I've just been putting together an audio meter by stacking bootstrap progress bars, and thought there was probably a better way, mathematically / logically, of achieving this.

levels = (vumeter) ->
  green = if vumeter % 50 < vumeter then 50 else vumeter
  yellow = if vumeter % 85 < vumeter then 35 else vumeter - 50
  red = if vumeter % 100 < vumeter then 15 else vumeter - 85

  'green': green
  'yellow': yellow
  'red': red

vumeter % 50 < vumeter doesn't make much sense to me. For non-negative numbers, this is equivalent to vumeter > 50, so you can just use that instead. For negative numbers, this will always return false, although it seems like this would be an illegal argument to your function.

@mleyfman provided a great solution. In practice, the code provided achieves something called clamping, where you restrict a value to a specified range. You can roll your own clamp function like so:

clamp = (value, min, max) -> Math.min(Math.max(min, value), max)

levels = (vumeter) ->
  'green'  : clamp(vumeter,  0,  50)
  'yellow' : clamp(vumeter, 50,  85) - 50
  'red'    : clamp(vumeter, 85, 100) - 85
  // or, if you find it more meaningful to provide maxima as parameters rather than ranges:
  'green': clamp(vumeter, 0, 50)
  'yellow': clamp(vumeter - 50, 0, 35)
  'red': clamp(vumeter - 85, 0, 15)


Printing an Alternating Pattern to the Console For this assignment, you will create the pattern of a chess board that
  is 8 x 8.  Use X and O to represent the squares.

  Create the appropriate nested looping structure to output the characters in an 8 x 8 grid on the screen using Console.Write() or
  Console.WriteLine() as appropriate.
  Include a decision structure to ensure that alternate rows start with opposite characters as a real chess board alternates the colors
  among rows.

Simple enough, but my first version stuffed everything inside of Main, which I don't care for. So, I wrote a second version that nicely separated the concerns. The problem with that, in my eyes at least, is that it now does exactly twice the amount of work as my initial "naive" version. Does the benefit of separating concerns outweigh the performance hit on an already exponential algorithm?

Also, I don't like this snippet (that exists in both versions of the code).

if (row % 2 == 0)
{
  grid[row, col] = (col % 2 == 0) ? dark : light;
}
else
{
  grid[row, col] = (col % 2 == 0) ? light : dark;
}

This differs only in the order of the characters, but I'm too close to it at this point to see a better way to clean up the control logic. Advice on it would be appreciated.

Version 1:

public static void Main()
{
    const int gridSize = 8;
    const string dark = "x";
    const string light = "o";

    for (var row = 0; row < gridSize; row++)
    {
        for (var col = 0; col < gridSize; col++)
        {
            string output;
            if (row % 2 == 0)
            {
                output = (col % 2 == 0) ? dark : light;
            }
            else
            {
                output = (col % 2 == 0) ? light : dark;
            }

            Console.Write(output);
        }
        Console.WriteLine();
    }
}

Version 2: (ideone here)

public static void Main()
{
    var grid = CreateGrid(8, 'o', 'x');
    PrintGrid(grid);
}

private static void PrintGrid(char[,] grid)
{
    for (var row = 0; row < grid.GetLength(0); row++)
    {
        for (var col = 0; col < grid.GetLength(0); col++)
        {
            Console.Write(grid[row, col]);
        }
        Console.WriteLine();
    }
}

private static char[,] CreateGrid(int gridSize, char light, char dark)
{
    var grid = new char[gridSize, gridSize];

    for (var row = 0; row < gridSize; row++)
    {
        for (var col = 0; col < gridSize; col++)
        {
            if (row % 2 == 0)
            {
                grid[row, col] = (col % 2 == 0) ? dark : light;
            }
            else
            {
                grid[row, col] = (col % 2 == 0) ? light : dark;
            }
        }
    }

    return grid;
}

I like that you have separated the CreateGrid method out in to a reusable method. This is good.

Your logic inside the method is even quite good, despite your concerns about the duplication. There is a trick, though.... use a boolean:

    for (var row = 0; row < gridSize; row++)
    {
        bool toggle = row % 2 == 0;
        for (var col = 0; col < gridSize; col++)
        {
            grid[row, col] = toggle ? dark : light;
            toggle = !toggle;
        }
    }

Your print-grid method would likely benefit from some Linq, but what you have is OK.... rather, it is very readable. Linq (or using a StringBuilder) will improve performance by reducing the calls to Write.


Simple program to print from a buffer until a newline is found For practice, I wrote a small program that goes through a string in the data section (although, it could be easily adapted to take user input), and print out the characters in the string until it finds a newline, then stops.
This is by far the most complicated assembly program I've ever written. It's the first time I've ever used a jump and cmp. It's simple, but there's likely improvements I can make, so I thought I'd try to get feedback.
I decided to write it in 32-bit because the system calls are a little easier to wrap my head around. I'll transition to x86-64 once I know what exactly my school is going to be using; because I've seen both so far in slides.
firstjmp.asm
global _start

section .data
    input: db `1234567890\n`

section .text
    _start:
        mov esi, input  ; Current head of string

        .loop:
            mov eax, 4
            mov ebx, 1
            mov ecx, esi
            mov edx, 1
            int 0x80

            inc esi

            cmp BYTE [esi], 10
            jne .loop  ; Stop looping once we've found the newline

        mov eax, 1
        mov ebx, 0
        int 0x80

nasm firstjmp.asm -g -f elf32 -Wall -o firstjmp.o
ld firstjmp.o -m elf_i386 -o firstjmp

┌─[brendon@parrot]─[~/Desktop/Classes/CompArc/Lab4/asm]
└──╼ $./firstjmp
1234567890┌─[brendon@parrot]─[~/Desktop/Classes/CompArc/Lab4/asm]

I haven't used nasm, but superficially, it looks like you are calling out to the kernel for each character in the string.  This is likely to be a relatively expensive operation.
Looking at this, it would appear that edx can be used to specify the number of characters to output.  With this in mind, it would probably be more efficient to find the string length first and then output the entire string in a single system call.
It's also worth considering breaking up your logic into functions.  This doesn't make a lot of difference with such a small program, however it'll be important as you start trying to do more complex logic.  I'm not sure what the standard calling conventions are, however you could end up with something 'like' this...
global _start

section .data
    input: db `1234567890\n`

section .text
    _exit:
        mov eax, 1
        mov ebx, 0
        int 0x80

    ; Input esi points to string
    ; Output edx contains length
    ; Modifies eax
    _linelen:
        mov eax, esi
        .loop:
            inc eax

            cmp BYTE [eax], 10
            jne .loop
        mov edx, eax
        sub edx, esi
        ret

    ; Input esi points to string
    ;       edx contains string length
    ; Modifies eax, ebx
    _print:
        mov eax, 4
        mov ebx, 1
        int 0x80
        ret

    _start:
        mov esi, input  ; Current head of string

        call _linelen

        mov ecx, esi
        call _print

        call _exit


Combining values corresponding to the same key What the following code does is:

The key field is column0 (where sometimes there could be single key, and sometimes keys separated by comma).

The rest of the columns are also either single or comma separated.  But based on the single key value from column0, the goal is to collect all the values as a set in the rest of the columns.

import sys
import csv

dict={}

def getVal(k):
  try:
    v = dict[k]
  except:
    v= None
  return v

# First read as a line and first transformation separates first column
# and stores into a table with key,value (where value is the remaining)
for line in sys.stdin:
  line = line.strip()
  row = line.split('\t')
  l = len(row)
  keyvalues = row[0]
  items = keyvalues.split(",")
  for item in items:
    key = item
    value=[]
    for i in range(1, l):
       value.append(row[i].split(","))
    if getVal(key) == None:
      dict[key] = value
    else:
      prevval = dict[key]
      cols = len(prevval)
      newvalue=[]
      newlist = []
      for j in range(0,cols):
        newlist = prevval[j]+value[j]
        newset = set(newlist)
        newvalue.append(list(newset))
      dict[key] = newvalue

for k,v in dict.items():
   rowstr = k+"\t"
   ncols = len(v)
   for items in v:
     cols=""
     for item in items:
       cols +=item+","
     cols = cols[0:len(cols)-1]
     rowstr += cols+"\t"
   print rowstr

Sample input

3,15,75 1635,1762   878 3425    121,122,123
15  1762    871 3475    121,125,126
3   1585,1590,5192  882,832,841 3200,3211   120,121,122,123,124

I'm getting the results correctly as expected, but I would like to know any improvements to make on the code.

Indentation matters a lot in Python.  Please follow the PEP 8 standard, which is 4 spaces.

This code feels very procedural.  Python can be much more expressive than that, if you take advantage of features like list comprehensions.

You've used a lot of variables: line, row, l, keyvalues, items, item, key, value, i, prevval, cols, newvalue, newlist, j, newset — too many for me to keep track of what each represents.  You could reduce the mental load by eliminating variables for ephemeral expressions.  For example, you could eliminate items by writing for item in keyvalues.split(",").

I see that you considered using the csv module, but didn't.  If you did, you could have simplified

for line in sys.stdin:
  line = line.strip()
  row = line.split('\t')

… to for row in csv.reader(fileinput.input(), delimiter='\t').  I recommend fileinput instead of sys.stdin so that the user has the option of passing a filename on the command line.

The getVal(k) function can just be replaced by dict.get(k, None).  It would be better to avoid choosing dict as the name of a variable, since it shadows the dict() constructor.

Suggested solution

import csv
import fileinput

data = {}

for row in csv.reader(fileinput.input(), delimiter='\t'):
    values = [set(col.split(',')) for col in row]
    for key in values.pop(0):   # Numbers in column 0
        data[key] = [
            new_col.union(old_col)
            for new_col, old_col in zip(values, data.get(key, values))
        ]

for key, values in data.items():
    print '\t'.join([key] + [','.join(col) for col in values])


Typesafe scanf-like function with variadic templates I'm hoping to get some feedback on a function with variadic templates that parses a format string and fills in some parameters whose order and types are based on the characters in the format string. Kind of like scanf().

(For the curious, I am trying to make a typesafe version of this function, documentation here.)

I'm pretty experienced with C but the last time I did anything serious with C++ was at university 15 years ago and it seems like a completely different language now.

This toy code works for me and fails in the ways I'd expect it to when I give it bad input. However, I'm not too familiar with the standard library so I have a strong feeling that I may be missing something. Also the code seems overly verbose for what it does.

So here's what I'm hoping to find out:

Is this code idiomatic modern C++?
Am I overlooking useful features in the standard library that would make things easier?
Should I be using pointers in this way, or am I thinking too much in C?
Should I consider another approach entirely, like tscanf(format, paramname1, paramname2) >> param1 >> param2?

I am constrained to use C++11, the code currently does not use Boost.

#include <cstring>
#include <iostream>

struct SomeStruct {
    int val;
};

template<typename T>
bool
assign(const char c, T& ref) {
    return false;
}

template<>
bool
assign(const char c, bool*& ref) {
    if (c != 'b')
        return false;
    *ref = true;
    return true;
}

template<>
bool
assign(const char c, int*& ref) {
    if (c != 'i')
        return false;
    *ref = 42;
    return true;
}

template<>
bool
assign(const char c, SomeStruct& ref) {
    if (c != 'h')
        return false;
    ref.val = 81;
    return true;
}

template<typename T>
bool
tscanf(const char *format, const char *name, T& ref)
{
    if (std::strlen(format) != 1) {
        std::cerr << "Wrong number of arguments" << std::endl;
        return false;
    }

    if (!assign(format[0], ref)) {
        std::cerr << "Wrong type for argument " << name << std::endl;
        return false;
    }

    return true;
}

template<typename T, typename... Args>
bool
tscanf(const char *format, const char *name, T& ref, Args... args)
{
    if (!assign(format[0], ref)) {
        std::cerr << "Wrong type for argument " << name << std::endl;
        return false;
    }

    return tscanf(format + 1, args...);
}

int
main(void)
{
    bool b;
    int i;
    SomeStruct h;
    if (!tscanf("hbi",
        "handle", h,
        "switch", &b,
        "num", &i))
        return 1;
    std::cout << h.val << ' ' << b  << ' ' << i << std::endl;
    return 0;
}

So here's what I'm hoping to find out:
  Is this code idiomatic modern C++?

Pretty much.
You use recursion to handle the multiple arguments. But a better technique may be to use the compiler to build objects to handle each argument. Unfortunately the way you have to defined the interface makes this a bit hard.
If we re-consider the interface to make it like C-scanf.

template<typename T>
int parser(char const*& format, char const*& value, T& value)
{
     // Parse format and value.
     // convert the first % in format into value and save in value.
     // return 1 if value was correctly parsed otherwise 0
}

template<typename... Args)
int tscanf(char const* format, char const* value, Args&... args)
{
    // This will call parser() for each argument.
    // Each of the parameters inside {} are guaranteed to be constructed
    // left to right.
    // Because the compiler knows the number of arguments you
    // get a fixed size array that can be used to count the
    // number of parameters that were actually found.
    auto count[] = {parser(format, value, args)...};

    // Probably need a check for unmatched format parts.

    // The return the number of format parameters matched.
    return std::accumulate(std::begin(count), std::end(count), 0);
}

  Am I overlooking useful features in the standard library that would make things easier?

I don't think there is anything you absolutely need.

  Should I be using pointers in this way, or am I thinking too much in C?

NO. Absolutely do not use pointers like that.

Prefer to use references. This way you don't need to check for null. You also express your intent much more clearly so there is no confusion on the ownership of the pointer.

  Should I consider another approach entirely, like tscanf(format, paramname1, paramname2) >> param1 >> param2?

There have been a couple of attempts like this. boost::format springs to mind. They used % rather than >>. But personally I don't see a need for this. BUT big advancements have come from trying the non obvious (things I would not think of). So please experiment.


Semantic HTML5 and proper use of tags I'm trying to develop a base for a blog using some of the new tags introduced in HTML5 and I want to not only make sure I'm using them correctly, but my code is also semantic.

Here is just the 'sample' document.

<!DOCTYPE html>

<meta http-equiv="content-type" content="text/html;charset=utf-8"/>
<title>page title</title>

        <hgroup>

Blog title

Blog tagline goes here

        </hgroup>

Top Link

Top Link

Top Link

sidebar link

sidebar link

sidebar link

Article Header

Section header

Article section content

Section header

Article section content

Section header

Article section content

            posted by user on <time datetime="2012-01-01T00:00+00:00">January 1st, 2012. 12:00pm</time>

Article Header

Section header

Article section content

            posted by user on <time datetime="2012-01-01T00:00+00:00">January 1st, 2012. 12:00pm</time>

Article Header

Section header

Article section content

            posted by user on <time datetime="2012-01-01T00:00+00:00">January 1st, 2012. 12:00pm</time>

    Page footer.

Can anyone see any misuse of the new tags or any better - cleaner way to write this?

Quick review:

Remove this damn
, styling is for CSS.
In each
, you better have an
 :-)
You can add rel="author" to your  link on the author. Even better, you can use The author's name (Google will recognize this as a "rich snippet" and show your face in google results. Take a look at http://schema.org for more information). Rich snippets are a whole other topic, so I'll stop talking about it :)

Edit after your comment:

Each article should contain an
 for its article sections.
Each article should have its title in an
. Yes, even if there are multiple
 in the main page. If there is just the titles on the main page (no article's text), feel free to use
.
The
 on the frontpage is tolerated, not on the blog pages. Feel free to use it or not on the frontpage, but make sure that each blog page has the title of the post as an
, not the site's name.

Related picture:


Share data between 2 React components using React Hooks In my app I have 3 components:

MovieSearchComponent to "house" the other 2 components
MovieSearch to search an API and retrieve the data
MovieResultList to display the data in the browser

Both components are wrapped in a parent container MovieSearchComponent.
My MovieSearchComponent looks like this:
interface Movie {
  original_title: string;
  id: string;
}

const MovieSearchComponent = () => {
  const [movieList, setMovies] = useState<Movie[]>([]);

  const addMovie = ((movies: Movie[]) => {
    setMovies([...movies]);
  });

  return (
    <React.Fragment>
      <MovieSearch addMovie={addMovie} />
      <MovieResultList movieList={movieList}/>
    </React.Fragment>
  )
}

In here I have a empty MovieList array that uses the setMovies function to fill the MovieList array. There's also a addMovie function that gets called from the MovieSearch component and takes a array as a parameter. Then I pass the MovieList array to the MovieResultList component.
The MovieSearch component:
const Search = styled.input`
  color: green;
`

const MovieSearch = ( {addMovie }) => {
    const apikey = 'api_key=***************dad4';
    const baseurl = 'https://api.themoviedb.org/3/search/movie?'

    const searchTMDBapi = (e) => {
        e.persist()
        setMovieSearchResults(e);
    }

    const setMovieSearchResults = debounce((e) => {
        const query = e.target.value;
        fetch(baseurl + apikey + '&language=en-US&query=' + query + '&page=1&include_adult=false')
          .then(response => response.json())
          .then(data => addMovie(data.results))
  }, 500);

  return <Search placeholder="Search" onChange={searchTMDBapi}/>
}

In this function component I render a input field by using styled components. The input field calls the searchTMDBapi function when something is typed. In that method I call the setMovieSearchResults method which calls the api and sets the api data in the Hook by using .then(data => addMovie(data.results))
The addMovie Hook updates the movieList array in MovieSearchComponent and the <MovieResultList movieList={movieList}/>  syntax passes the movieList array to the MovieResultList which renders it:
const MovieResultList = (props) => {
    return (

                {props.movieList.map(movie => {
                    return (
{movie.original_title}
)
                })}

    )
}

My goal was to create 2 components. 1 to retrieve data from a external API and the other to display that data in a different component. I succeeded in that but I was wondering if there are some aspects on which I could improve this code and my React knowledge.

MovieSearchComponent

With the useState the common pattern is to name the state update function matching with the state value, i.e. movieList and setMovieList. This makes it clear what state the function will update.
I had to read and reread your description of addMovie and compare it against the actual implementation. It isn't clear if the function is to add a movie, add an array of movies, or, based on the implementation, replace existing movie array list. The name of the function and the result of the function executing should match. In your description you say addMovie updates the movielist array, so for the sake of code review I'll assume that to mean the third option with state replacement. In this case the callback function can simply be setMovieList. Update the prop name as well.

Code suggestions
import React, { Fragment } from 'react';

...

const MovieSearchComponent = () => {
  const [movieList, setMovieList] = useState<Movie[]>([]);

  return (
    <Fragment>
      <MovieSearch setMovieList={setMovieList} />
      <MovieResultList movieList={movieList}/>
    </Fragment>
  )
};

MovieSearch

Update prop addMovie to setMovieList.
Values like apiKey and baseurl that never change don't need to be in the functional component body and continually redefined. Declare them externally in the file.
It may be better to extract the synthetic event data and allow the event object to be returned to the event pool as quickly as possible. Extract and pass the event's target's value to setMovieSearchResults.
Handle the fetch request's unhappy paths, i.e. a not-successful responses or errors handling successful responses. Checking that the fetch was successful.
You should always standardize and sanitize user input.

Code suggestions
const apikey = 'api_key=***************dad4';
const baseurl = 'https://api.themoviedb.org/3/search/movie?'

const Search = styled.input`
  color: green;
`;

const MovieSearch = ({ setMovieList }) => {
    const searchTMDBapi = e => {
        const { value } = e.target;

        // Simple example: URI encode and lowercase
        // ... there's more you could do
        const query = encodeURI(value.toLowerCase());

        setMovieSearchResults(query);
    }

    const setMovieSearchResults = debounce(query => {
        fetch(baseurl + apikey + '&language=en-US&query=' + query + '&page=1&include_adult=false')
          .then(response => {
            if (!response.ok) {
              throw new Error('Network response was not ok');
            }
            return response.json();
          })
          .then(data => setMovieList(data.results))
          .catch(console.error);
  }, 500);

  return <Search placeholder="Search" onChange={searchTMDBapi}/>
}

MovieResultList

Pretty simple & clean component. Only suggestion here might be to tighten up variables by using object destructuring.

Code suggestions
const MovieResultList = ({ movieList }) => (

      {movieList.map(({ id, original_title }) =>
{original_title}
)}

);


Functions between certain times of the day I'm working on something for my school's laptops that will disable videos/games between 8:00 AM and 3:15 PM. Due to the giant time gap, I will not be waiting to see if the code functions properly or not. What I want to know is if my code will function properly, as I'm not good enough at JavaScript to know myself.

function timer() {
    var date = new Date();
    var hours = date.getHours();
    var minutes = date.getMinutes();

    if (minutes < 10) {
        var minutes = "0" + minutes;
    } else {}

    $(".time").html(hours + ":" + minutes),
        setInterval(timer, 1000);

    // Disable stuff
    if (hours > 8 & hours < 15) {
        $("video").hide();
        $("video").parent().html("Can you do us all a favor and not eat up all the bandwidth with your video watching?" + "
" + "Thank you!" + "
" + "
");
        $("embed").hide();
        $("embed").parent().html("This is no time for childish games." + "
" + "Shouldn't you be studying or doing something related to school?" + "
" + "
");
    } else if(hours > 15 & hours < 8) {
        $("video").show();
        $("embed").show();
    }
}

I will comment on this part...

if (hours > 8 & hours < 15) {
    $("video").hide();
    $("video").parent().html("Can you do us all a favor and not eat up all the bandwidth with your video watching?" + "
" + "Thank you!" + "
" + "
");
    $("embed").hide();
    $("embed").parent().html("This is no time for childish games." + "
" + "Shouldn't you be studying or doing something related to school?" + "
" + "
");
} else if(hours > 15 & hours < 8) {
    $("video").show();
    $("embed").show();
}

As Hosch said, you should be using the conditional operator &&.

Also, the else if clause wouldn't work because (hours > 15 && hours < 8) is only true if hours > 15 AND hours < 8, so it's never true. What you want is the conditional "or" operator: if (hours > 15 || hours < 8). But you don't even need to specify that if what you want is just to cover all the other cases: "if it's between 8 AM and 3:15 PM do this, otherwise do that".

So...

else { ... }

looks better.

Finally, hours > 8 && hours < 15 would only be true between 9 AM and 2 PM (instead of between 8 AM and 3 PM) as you're using the "greater than" (>) and "lesser than" (<) operators instead of "greater or equal" (>=) and "lesser or equal" (<=).

In order to cover the whole range you specified, you should write something like:

if ((hours >= 8 && hours < 15) || (hours == 15 && minutes <= 15)) { ... }


Character permutations - Revisited Several hours ago I posted this question about Generating character permutations in Python.

User rolfl said that my main problem was not using the right tool for the job, as Python is interpreted it will be much slower than a compiled solution. So I tried to write this in C.

Objective

Generate a file containing all fixed length l permutations of a given set of chars, in this case upper-case alphabet.

i.e.: With l = 5 the output file should look like:

AAAAA
AAAAB
AAAAC
...
ZZZZZ

My solution

I used a recursive for loop. It's my first program in C (after a hello world) so I don't know what it's worth. It runs in about 2.621s on my 2.9 GHz Intel Core i7

#include <stdio.h>

FILE *fp;

char alphabet[26] = { 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z'};
int len = 5;

void recloop(char *pwd, int repeat) {
  int i;
  for (i = 0; i < 26; i++) {
    pwd[len-repeat] = alphabet[i];

    if (repeat == 1) {
      fprintf(fp, "%s\n", pwd);
    }
    else {
      recloop(pwd, repeat - 1);
    }
  }
}

int main(void) {
  char pwd[len];
  fp = fopen("output", "w");
  recloop(pwd, len);
}

A few notes:

You aren't declaring alphabet in a very readable and maintainable form.  Here's how I would do it:

static const char alphabet[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";

As you can see, I also made this variable static and const.  static renders variable local to the file, and const makes it so that we can't screw with the array somewhere along the line (and also makes things a tidbit faster).  This leads me to my next point...

Also, you don't have to specify the number of elements in the array when you declare it like this, since the compiler can determine that itself (because it is constant).
fp and len are global variables when they shouldn't be.  Arguably alphabet shouldn't be one either, but I think it is find as long as you have the static in front of it.  Anyways, they should be declared as local variables and then passed to the functions that need them.
I'm not sure your algorithm is the most optimal.  I used a method known as "backtracking" myself, which seems to be the most efficient that I could find in practice.
fprintf() isn't going to be as efficient compared to fwrite().  I would consider rewriting your code if you still want to generate a permutation table.

Time test (without compiler optimizations)

mycode.c:

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

static const char alphabet[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
static const int alphabetSize = sizeof(alphabet) - 1;

static void bruteImpl(char* str, size_t index, size_t maxDepth)
{
    for (size_t i = 0; i < alphabetSize; ++i)
    {
        str[index] = alphabet[i];

        if (index == maxDepth - 1); //printf("%s\n", str);
        else bruteImpl(str, index + 1, maxDepth);
    }
}

void bruteSequential(size_t maxLen)
{
    char* buf = calloc(maxLen + 1, sizeof(char));

    for (size_t len = 1; len <= maxLen; ++len)
    {
        bruteImpl(buf, 0, len);
    }

    free(buf);
}

int main(void)
{
    bruteSequential(5);
}

$ gcc opCode.c # I took out your fprintf call so that it wouldn't be a factor
$ time ./a.out

real  0m0.170s
user  0m0.139s
sys   0m0.009s

$ gcc mycode.c
$ time ./a.out

real  0m0.043s
user  0m0.039s
sys   0m0.002s


Can this promise be chained more cleanly? The below code uses Promises/A+ (specifically rsvp.js) to generate a token.  It does so by either taking an existing token or downloading and parsing a token from a web page.  Is it possible to chain this to LoadURIPromise more cleanly? Usually one can chain together promises without causing as much indentation drift.

new RSVP.Promise(function (resolve) {
    if (TokenData) {
        resolve(TokenData);
    } else {
        LoadURIPromise(TokenURL).then(function (xmlDoc) {
            TokenData = xmlDoc.evaluate(
                '/root/theToken',
                xmlDoc.documentElement,
                null,
                XPathResult.FIRST_ORDERED_NODE_TYPE,
                null
            ).singleNodeValue.textContent;
            resolve(TokenData);
        });
    }
}).then(function (token) {
    //Use Token here
}

I have never used RSVP, I am more familiar with Q, but I believe following should do what you need:
var evaluate = function (xmlDoc) {
    TokenData = xmlDoc.evaluate(
        '/root/theToken',
    xmlDoc.documentElement,
    null,
    XPathResult.FIRST_ORDERED_NODE_TYPE,
    null).singleNodeValue.textContent;
    return TokenData;
}

RSVP.resolve(TokenData || LoadURIPromise(TokenURL).then(evaluate))
    .then(function (token) {
    //Use Token here
    console.info(token);
});

Here is working jsFiddle
BTW, do you really need TokenData to be global?


Generating a 3D Point Cloud I'm trying to produce a 3D point cloud from a depth image and some camera intrinsics. The image is 640x480, and is a NumPy array of bytes. The output is a (rows * columns) x 3 array of points.

I've gotten the function to work perfectly, but it's way too slow! (takes like 2 seconds per image to process). I'm wondering if there are any optimizations I can make before giving up and writing a C module.

def create_point_cloud(self, depth_image):
    shape = depth_image.shape;
    rows = shape[0];
    cols = shape[1];

    points = np.zeros((rows * cols, 3), np.float32);

    bytes_to_units = (1.0 / 256.0);

    # Linear iterator for convenience
    i = 0
    # For each pixel in the image...
    for r in xrange(0, rows):
        for c in xrange(0, cols):
            # Get the depth in bytes
            depth = depth_image[r, c, 0];

            # If the depth is 0x0 or 0xFF, its invalid.
            # By convention it should be replaced by a NaN depth.
            if(depth > 0 and depth < 255):
                # The true depth of the pixel in units
                z = depth * bytes_to_units;

                # Get the x, y, z coordinates in units of the pixel
                points[i, 0] = (c - self.cx) / self.fx * z;
                points[i, 1] = (r - self.cy) / self.fy * z;
                points[i, 2] = z
            else:
                # Invalid points have a NaN depth
                points[i, 2] = np.nan;
            i = i + 1
    return points

1. Review

No docstring. What does this function do? What parameters does it take? What shape must the depth_image parameter be? What does it return?
In Python, there is no need for a semi-colon at the end of a statement (unless another statement follows on the same line) and it is best to omit it.
It seems that depth_image is required to have three dimensions, but only the coordinate 0 is used on the third dimension. It would be simpler if the function took a two-dimensional depth image. A caller with a three-dimensional image could pass depth_image[...,0].
The result is returned as a linear array rather than in the natural shape as a cols × rows × 3 array. This loses information about the shape of the image that might be needed by some callers. There's a comment that says this is "for convenience" but if so, the caller can easily call numpy.reshape.

2. Vectorize

As always with Numpy performance problems, an important step is to scrutinize all the loops that run in the Python interpreter (the for and while loops) to see if they can be vectorized. A loop running inside Numpy is usually hundreds of times faster than the same loop running in native Python.

In this case it should be straightforward: instead of looping over pixels and doing some operations on each pixel, apply those operations to the whole image and let Numpy worry about looping over the pixels.

We only need the 0 plane on the third axis, as discussed in §1.3 above:

depth = depth_image[..., 0]

Let's make a mask array of valid pixels:

valid = (depth > 0) & (depth < 255)

The z-coordinates of the result can be computed by using numpy.where to distinguish between valid and invalid pixels:

z = np.where(valid, depth / 256.0, np.nan)

For the other coordinates of the result, we need row and column coordinates for each pixel, which can be generated using numpy.meshgrid:

rows, cols = depth_image.shape
c, r = np.meshgrid(np.arange(cols), np.arange(rows), sparse=True)

Then we have:

x = np.where(valid, z * (c - self.cx) / self.fx, 0)
y = np.where(valid, z * (r - self.cy) / self.fy, 0)

The coordinate arrays can be stacked using numpy.dstack to produce the result:

return np.dstack((x, y, z))

If the caller really needs a linear array, they can flatten it using numpy.reshape.

3. Revised code

Putting this together:

def point_cloud(self, depth):
    """Transform a depth image into a point cloud with one point for each
    pixel in the image, using the camera transform for a camera
    centred at cx, cy with field of view fx, fy.

    depth is a 2-D ndarray with shape (rows, cols) containing
    depths from 1 to 254 inclusive. The result is a 3-D array with
    shape (rows, cols, 3). Pixels with invalid depth in the input have
    NaN for the z-coordinate in the result.

    """
    rows, cols = depth.shape
    c, r = np.meshgrid(np.arange(cols), np.arange(rows), sparse=True)
    valid = (depth > 0) & (depth < 255)
    z = np.where(valid, depth / 256.0, np.nan)
    x = np.where(valid, z * (c - self.cx) / self.fx, 0)
    y = np.where(valid, z * (r - self.cy) / self.fy, 0)
    return np.dstack((x, y, z))

I find that this is about fifty times as fast as the original code.