All the knight moves in all the right places Challenge

Find all possible moves for a knight on an empty chessboard.

Specifications

The first argument is a path to a file.
The file contains multiple lines.
Each line is a test case representing the position of the knight in CN form.

C is a letter from “a” to “h” and denotes a column.
N is a number from 1 to 8 and denotes a row.

For each test case, print all positions for the next move of the knight ordered alphabetically.

Sample Input

  g2
  a1
  d6
  e5
  b1

Sample Output

  e1 e3 f4 h4
  b3 c2
  b5 b7 c4 c8 e4 e8 f5 f7
  c4 c6 d3 d7 f3 f7 g4 g6
  a3 c3 d2

Source

My Solution

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

#define LINE_LENGTH 5
#define BOARD_LENGTH 8
#define MOVE_BUFFER 24

int rows[] = {1, 2, 3, 4, 5, 6, 7, 8};
char cols[] = {'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h'};
char moves[MOVE_BUFFER];
int move_iter = 0;

char num_convert(int *n) {
    for (int i = 0; i < BOARD_LENGTH; i++) {
        if (*n == rows[i]) {
            return cols[i];
        }
    }
}

int alpha_convert(char *c) {
    for (int i = 0; i < BOARD_LENGTH; i++) {
        if (*c == cols[i]) {
            return rows[i];
        }
    }
}

void add_moves(int c, int n1, int n2) {
    if (c >= 1 && c <= 8) {
        if (n1 >= 1) {
            moves[move_iter++] = num_convert(&c);
            moves[move_iter++] = n1 + '0';
            moves[move_iter++] = ' ';
        }
        if (n2 <= 8) {
            moves[move_iter++] = num_convert(&c);
            moves[move_iter++] = n2 + '0';
            moves[move_iter++] = ' ';
        }
    }
}

char* valid_moves(char position[]) {
    int C = alpha_convert(&position[0]);
    int N = atoi(&position[1]);

    add_moves(C - 2, N - 1, N + 1);
    add_moves(C - 1, N - 2, N + 2);
    add_moves(C + 1, N - 2, N + 2);
    add_moves(C + 2, N - 1, N + 1);

    moves[move_iter - 1] = '\0';
    move_iter = 0;
    return moves;
 }

int main(int argc, char *args[]) {
    if (argc < 2) {
        fprintf(stderr, "File path not provided. Exiting...\n");
        return 1;
    }

    if (argc > 2) {
        puts("Excessive arguments, only the first will be considered.");
    }

    FILE *file = fopen(args[1], "r");
    if (file == NULL) {
        perror("Error");
        return 1;
    }

    char position[LINE_LENGTH];
    while (fgets(position, LINE_LENGTH, file)) {
        puts(valid_moves(position));
    }

    fclose(file);
}

Joshua: What's the difference?

Computers sometimes have a hard time to differentiate between things that are apples and oranges to us, some examples are:

if this is a game or if this is real
thermonuclear war and a game of chess
char and int

You have two functions converting from char to int and back:

char num_convert(int *n) {
    for (int i = 0; i < BOARD_LENGTH; i++) {
        if (*n == rows[i]) {
            return cols[i];
        }
    }
}

int alpha_convert(char *c) {
    for (int i = 0; i < BOARD_LENGTH; i++) {
        if (*c == cols[i]) {
            return rows[i];
        }
    }
}

This is not necessary in C, because char and int are kind of the same thing. You can do regular calculations with characters just like you do with integers. Once in character, stay in character.

So when it comes to playing the game of converting between both types, I'd have to say the only winning move is not to play.

the mysterious void add_moves()

I have no idea what this function does.

void add_moves(int c, int n1, int n2) {
    if (c >= 1 && c <= 8) {
        if (n1 >= 1) {
            moves[move_iter++] = num_convert(&c);
            moves[move_iter++] = n1 + '0';
            moves[move_iter++] = ' ';
        }
        if (n2 <= 8) {
            moves[move_iter++] = num_convert(&c);
            moves[move_iter++] = n2 + '0';
            moves[move_iter++] = ' ';
        }
    }
}

Why are there two n but only a single c? What do those values represent?
They are apparently boundary checked, too. Then there's this code block that's pretty much duplicated for both n, which is bad.

        if (n1 >= 1) {
            moves[move_iter++] = num_convert(&c);
            moves[move_iter++] = n1 + '0';
            moves[move_iter++] = ' ';
        }

After all, something is somehow added to moves, which justifies the name add_moves(). But it's not immediately clear to me what's going on in that function and I feel like too much is going on.

a hint in char* valid_moves()

This function takes away a bit of the mystery from what add_moves() is doing. As it calls it with different possible moves of the knight.

add_moves(C - 2, N - 1, N + 1);
add_moves(C - 1, N - 2, N + 2);
add_moves(C + 1, N - 2, N + 2);
add_moves(C + 2, N - 1, N + 1);

This is not very intuitive. Why do I only see 4 lines here? The knight can do 8 moves in general. Now it is clear why add_moves() takes two n values as parameters, but only one c.

This distribution of logic doesn't appear to be plausible. It's like some part of the movement is calculated in one place while the other happens later. If the concerns of the functions were more clearly separated, it would be easier to understand them.

I gave it a try myself. Here's how I did it:

Position type

This is all about positions, so let's create a type for that.

typedef struct
{
    signed char column;
    int row;
}
Position;

We're not in oop land, but we still group data together if it belongs together. This helps a lot when passing it around.

  disclaimer: I often see type names like position_t and I absolutely hate that, which is why I'm not using it. Your naming convention may vary.

relative knight moves as positions

Is 5 meters an absolute position or a difference between two positions? It can very well be both! In the same idea, let's create an array of Position that represent the relative movements a knight can perform.

Position allKnightMovesInAlphabeticalOrder[] =
{
    {-2, -1},
    {-2, +1},
    {-1, -2},
    {-1, +2},
    {+1, -2},
    {+1, +2},
    {+2, -1},
    {+2, +1}
};

#define numberOfKnightMoves 8

As you can see, -2 is a perfectly fine signed char value. 1

If you are uncomfortable reusing the Position type which from its name might suggest to be an absolute position, you can always typedef it to a Move type, which makes the intention more clear.

position check

A handy helper function could solve the sub-problem of whether a certain position is on the board or not.

bool positionIsInChessboard(Position *position)
{
    return
        position->column >= 'a' &&
        position->column <= 'h' &&
        position->row >= 1 &&
        position->row <= 8;
}

You need to #include <stdbool.h> for bool. If you don't want that, you should be able to use _Bool as a return type.

making a move

Another helper function that simply adds two positions together.

void addTwoPositions(Position *a, Position *b, Position *result)
{
    result->column = a->column + b->column;
    result->row = a->row + b->row;
}

Again, as a Position might represent a relative movement, this starts to make sense I hope. If not, recall how a 2D vector in math might represent a fixed point or the difference between two points. This is the same idea.

putting it all together

#include <stdio.h>
#include <stdbool.h>

typedef struct
{
    signed char column;
    int row;
}
Position;

Position allKnightMovesInAlphabeticalOrder[] =
{
    {-2, -1},
    {-2, +1},
    {-1, -2},
    {-1, +2},
    {+1, -2},
    {+1, +2},
    {+2, -1},
    {+2, +1}
};

#define numberOfKnightMoves 8

bool positionIsInChessboard(Position *position)
{
    return
        position->column >= 'a' &&
        position->column <= 'h' &&
        position->row >= 1 &&
        position->row <= 8;
}

void addTwoPositions(Position *a, Position *b, Position *result)
{
    result->column = a->column + b->column;
    result->row = a->row + b->row;
}

int main ()
{
    Position testPositions[] =
    {
        {'g', 2},
        {'a', 1},
        {'d', 6},
        {'e', 5},
        {'b', 1}
    };

    int numberOfTestPositions = sizeof(testPositions)/sizeof(Position);

    Position result;

    for(int testPositionIndex = 0; testPositionIndex < numberOfTestPositions; testPositionIndex++)
    {
        printf("%c%d:\t", testPositions[testPositionIndex].column, testPositions[testPositionIndex].row);

        for(int moveIndex = 0; moveIndex < numberOfKnightMoves; moveIndex++)
        {
            addTwoPositions(&testPositions[testPositionIndex], &allKnightMovesInAlphabeticalOrder[moveIndex], &result);

            if(positionIsInChessboard(&result))
            {
                printf("%c%d ", result.column, result.row);
            }
        }

        printf("\r\n");
    }
}

I hard coded the test positions into main() for brevity. the basic idea for every test case is to iterate over all the possible knight moves, perform them via addTwoPositions() and then check the validity of the result with positionIsInChessboard().

I do not build up a buffer as you do in your code simply because that isn't necessary.

Here's what I get as a result in terminal:

$ ./chess
g2: e1 e3 f4 h4
a1: b3 c2
d6: b5 b7 c4 c8 e4 e8 f5 f7
e5: c4 c6 d3 d7 f3 f7 g4 g6
b1: a3 c3 d2

1 thanks @Daniel Jour for pointing out that plain char has an unspecified signedness, which may vary across platforms and compilers.

For me, I get the same output for any of the three versions char, signed char or unsigned char. It's still good practice to use signed char as it makes it more obvious that a Position might be a relative position.


Iterating over global keymap, removing all bindings to super keys Background

Emacs on OS X by default binds several keys with the super modifier. I do not want these bindings as I switch between OS X and Windows frequently, and want a clean slate on both platforms to bind super keys for my own purposes.

Code

global-unset-all-super-key.el

(defun global-unset-all-super-key ()
  "Will unset any single key in global keymap that has the super
modifier."
  (let ((km (current-global-map)))
    (while km
      (let ((maybe-event (and (listp (car km))
                              (caar km))))
        (if (and (eventp maybe-event) ; Also filters out maybe-event
                                      ; when nil because (car km) was not a list.
                 (memq 'super (event-modifiers maybe-event)))
            (global-unset-key (vector maybe-event))))
      (setq km (cdr km)))))

(provide 'global-unset-all-super-key)

Lines added to init.el.

;; Remove default super bindings on Mac systems.
;; Do this early, before any mappings are added.
(when (string-equal system-type "darwin")
  (require 'global-unset-all-super-key)
  (global-unset-all-super-key))

Questions

Is this reasonably idiomatic lisp?
Are there elisp constructs that would simplify the implementation?
Are there elisp constructs that would make the implementation clearer?
And finally, is (while <list> <body> (setq <list> (car <list>))) the best that can be done to loop over a list taking each element in turn and doing something for side effects? map doesn't seem like a reasonable alternative because I'm not trying to build up a list to return. Something like Clojure's doseq.

Apart from using dolist as mentioned by @brfennpocock it looks mostly
fine.

system-type is a symbol though, so you can compare it with eq
instead of string-equal (unless I missed a reason to use that here).

Also the single if can also be when for consistency.

(defun global-unset-all-super-key ()
"Will unset any single key in global keymap that has the super modifier."
(dolist (km (current-global-map))
    (let ((maybe-event (and (listp (car km)) (caar km))))
    (when (and (eventp maybe-event)
                (memq 'super (event-modifiers maybe-event)))
        (global-unset-key (vector maybe-event))))))

For init.el if you have more functions also consider the
autoload
facilities, but that's not necessary for just this bit of code (actually
I'd probably put it all in init.el to be honest even though a separate
file like this is somewhat cleaner).


Null-delimited to printf %b converter I wrote a D implementation of the nul2pfb utility from here, as the link to the source code was broken and I wanted to try to learn D. I noticed that it was rather slow (could barely keep up with the find -print0 that was passing it data, when it should be far faster as it need not do anywhere near as many system calls).

This implementation works correctly (tested with zsh and bash printf built-ins, as well as /usr/bin/printf).

I am a newbie to D and do not understand.

How can I improve this code?

import std.stdio;
import std.conv;

void main()
{
  foreach (ubyte[] mybuff; chunks(stdin, 4096)) {
    encodebuff (mybuff);
  }
}

@safe void encodebuff (ubyte[] mybuff) {
  foreach (ubyte i; mybuff) {
    char b = to!char(i);
    switch (i) {
      case 'a': .. case 'z':
      case 'A': .. case 'Z':
      case '0': .. case '9':
      case '/':
      case '.':
      case '_':
      case ':': writeChar(b); break;
      default: writeOctal(b); break;
      case 0: writeChar ('\n'); break;
      case '\\': writeString(`\\`); break;
      case '\t': writeString(`\t`); break;
      case '\n': writeString(`\n`); break;
      case '\r': writeString(`\r`); break;
      case '\f': writeString(`\f`); break;
      case '\v': writeString(`\v`); break;
      case '\a': writeString(`\a`); break;
      case '\b': writeString(`\b`); break;
    }
  }
}

@trusted void writeString (string a)
{
  write (a);
}

@trusted void writeOctal (int a)
{
  writef ("\\%.4o", a); // leading 0 needed for for zsh printf '%b'
}

@trusted void writeChar (char a)
{
  write (a);
}

Tests: (note that filelist is a NUL-delimited list of files generated by
    find -print0 on my home directory, and filelist2.txt is generated from filelist by
    filelist.txt sed -e 's/\x0/\n/g' > filelist2.txt
 and is thus the corresponding list of newline-delimited filenames).

# the sed script escapes the backslashes so xargs does not clobber them
diff filelist2.txt <(<filelist.txt char2code2 | sed -e 's/\\/\\\\/g' | xargs /usr/bin/printf "%b\n")
# from within zsh
bash -c 'diff filelist2.txt <(for i in "$(<filelist.txt char2code)"; do printf "%b\n" "$i"; done)'
# from within zsh and bash
diff filelist.txt <(for i in $(char2code <filelist.txt); do printf '%b\0' "$i"; done)
# from within zsh, bash, and dash
for i in $(char2code <filelist.txt); do printf '%b\0' "$i"; done | diff - filelist.txt

Do write and writef automatically buffer writes? If not I bet that's your problem.

One suggestion I do have that you could try is using a final switch statement. It would require an enum containing all the possible values of a ubyte, but in return your switch would essentially be a jump table and a bit faster.
If you're worried about this making your switch look huge you could just mixin the boring portion, generating a string within a loop and adding that string to compilation.

You could also try using a char[128] to store all conversions from ubyte to char, skipping the to! call which might be a bit slower. Making the conversion an array lookup instead of a method call.

This code doesn't use the buffered writes I mentioned, I'll leave that to someone else since I'm unsure about it.

import std.stdio;
import std.conv;

//This enum is so you can use final switch, use a proper ascii order
enum MyEnum {
    J0, J1, J2, ... //Using J because you can't start identifiers with a number, so I just picked something
    Ja, Jb, Jc, ...
    JA, ...
    ...
}

//a lookup table to make conversion possible in this direction.
static MyEnum[128] reverseMyEnum = {
    J0, J1, J2, ... //in proper ascii order, I know this is wrong
}

//replaces your use of to!
static char[128] ascii = {
    //just put the ascii chars here
}

void main()
{
  foreach (ubyte[] mybuff; chunks(stdin, 4096)) {
    encodebuff (mybuff);
  }
}

@pure string generateBoring() {
    string code;
    for (char value; a .. z, A .. Z, 0 .. 9)
        code ~= "case MyEnum.J'"~value~"': writeChar(b); break;\n";
    for (char value; any other uncaught cases)
        code ~= "case MyEnum.J'"~value~"': writeOctal(b); break;\n";
    return code;
}

@safe void encodebuff (ubyte[] mybuff) {
  foreach (ubyte i; mybuff) {
    char b = ascii[i]; //integer promotion lets you use a ubyte where int is expected
    final switch (reverseJB[i]) {
        mixin(generateBoring());
        case 0: writeChar ('\n'); break;
        case MyEnum.JBackBack: writeString(`\\`); break;
        case MyEnum.JBackT: writeString(`\t`); break;
        case MyEnum.JBackN: writeString(`\n`); break;
        case MyEnum.JBackR: writeString(`\r`); break;
        case MyEnum.JBackF: writeString(`\f`); break;
        case MyEnum.JBackV: writeString(`\v`); break;
        case MyEnum.JBackA: writeString(`\a`); break;
        case MyEnum.JBackB: writeString(`\b`); break;
    }
  }
}

@trusted void writeString (string a)
{
  write (a);
}

@trusted void writeOctal (int a)
{
  writef ("\\%.4o", a); // leading 0 needed for for zsh printf '%b'
}

@trusted void writeChar (char a)
{
  write (a);
}


Implementation of portscanner for a host I have tried to check whether the ports of a host from port 0 to port 1023 is open or not. Could someone please review this code and provide feedback.

#include<stdio.h>
#include<stdlib.h>
#include<string.h>
#include<sys/socket.h>
#include<sys/types.h>
#include<netdb.h>
#include<error.h>
#include<errno.h>
char *convert_int_to_string(int num);

char *convert_int_to_string(int num)
{
    int tmp;
    int i = 0;
    int j = 0;
    static char a[5] = {'0'};

    while (num > 0) {
        tmp = num % 10;
        a[i++] = tmp + '0';
        num = num / 10;
    }
    a[i] = '\0';
    for (j = 0; j < i / 2; j++) {
        tmp = a[j];
        a[j] = a[i - j - 1];
        a[i - j - 1] = tmp;
    }
    return a;
}

int main(int argc, char **argv)
{
    int status;
    char *node;
    char *port_no;
    int sock_fd;
    int i = 0;
    struct  addrinfo hints, *serviceinfo;

    if (argc != 2)
        error(1, errno, "Too many or few arguments\n");
    node = argv[1];
    memset(&hints, 0, sizeof hints);
    hints.ai_family = AF_INET;
    hints.ai_socktype = SOCK_STREAM;
    for (i = 0; i < 1024; i++) {
        port_no = convert_int_to_string(i);
        status = getaddrinfo(node, port_no, &hints, &serviceinfo);
        if (status != 0) {
            error(1, errno, "error in getaddrinfo() function call\n");
        }
        sock_fd = socket(serviceinfo->ai_family, serviceinfo->ai_socktype, serviceinfo->ai_protocol);
        if (sock_fd == -1)
            error(1, errno, "error in socket() function call\n");
        status = connect(sock_fd, serviceinfo->ai_addr, serviceinfo->ai_addrlen);
        if (status != -1)
            printf("Port : %s is open\n", port_no);
        else
            printf("Port : %s is closed\n", port_no);
    }
}

Use getaddrinfo only once, to resolve the host.  Do the port number iteration directly yourself.  While it might be more complicated, it also doesn't call for repeated hostname resolutions.  This also allows you to eliminate convert_int_to_string()
If you don't want IPv4 and IPv6 (which seems to be indicated by specifying AF_INET, you might be better off using the older functions.  This eliminates the complexity noted before.
As the last step in the loop, you MUST close the socket.  Otherwise, you will wind up with 1027 open descriptors.  Since current linux systems have a limit of 1024 open descriptors, your program will terminate early without this.  Other OSes will have similar limits.  Since you only ever need 4 open descriptors (or maybe 5 depending on getaddrinfo()), you should do the cleanup.
I suggest localizing variables where possible.
In convert_int_to_string(), if you start at the end of the buffer and work backwards, you can return the point in the buffer that you have reached instead of reversing the text.  Alternatively, snprintf(a, sizeof(a), "%d", num); works well.
Passing errno to error() is a mistake if errno isn't set.  This includes the wrong number of arguments error (no number) and the call to getaddrinfo() (which returns an error number and has its own function to make it a string).


Rust function to read the first line of a file, strip leading hashes and whitespace I’m writing a Rust function for getting a title based on the first line of a file.

The files are written in Markdown, and the first line should be a heading that starts with one or more hashes, followed by some text. Examples:

# This is a top-level heading
## This is a second-level heading
#### Let's jump straight to the fourth-level heading

I want to throw away the leading hashes, discard any leading/trailing whitespace, and return the remaining string. Example outputs:

"This is a top-level heading"
"This is a second-level heading"
"Let's jump straight to the fourth-level heading"

Assume that, for now, I’m not worried about edge cases like a first line that’s only whitespace and hashes, or a file whose first line is pathologically long.

This is the program I’ve written to do it:

use std::fs;
use std::io::{BufRead, BufReader};
use std::path::PathBuf;

/// Get the title of a Markdown file.
///
/// Reads the first line of a Markdown file, strips any hashes and
/// leading/trailing whitespace, and returns the title.
fn title_string(path: PathBuf) -> String {

    // Read the first line of the file into `title`.
    let file = match fs::File::open(&path) {
        Ok(file) => file,
        Err(_) => panic!("Unable to read title from {:?}", &path),
    };
    let mut buffer = BufReader::new(file);
    let mut first_line = String::new();
    let _ = buffer.read_line(&mut first_line);

    // Where do the leading hashes stop?
    let mut last_hash = 0;
    for (idx, c) in first_line.chars().enumerate() {
        if c != '#' {
            last_hash = idx;
            break
        }
    }

    // Trim the leading hashes and any whitespace
    let first_line: String = first_line.drain(last_hash..).collect();
    let first_line = String::from(first_line.trim());

    first_line
}

fn main() {
    let title = title_string(PathBuf::from("./example.md"));
    println!("The title is '{}'", title);
}

I’m fairly new to Rust, and I’m sure I’m doing stuff that isn’t as optimal or idiomatic as it could be.  Particular questions:

Is this idiomatic Rust?
Is there a better way to strip leading characters from a string? I looked in the documentation for std::string::String and couldn’t see anything. The String::from() feels a bit inefficient.
Is there anything unsafe that could easily crash (the panic! aside)?

Clippy returns a helpful suggestion:

warning: returning the result of a let binding from a block.
         Consider returning the expression directly.
         #[warn(let_and_return)] on by default
   |>
   |>     first_line
   |>     ^^^^^^^^^^
note: this expression can be directly returned
   |>
   |>     let first_line = String::from(first_line.trim());
   |>                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Don't ignore Results by using let _ =! You should always return them or use expect or unwrap.
The loop could be simplified by using take_while:

let last_hash = first_line.chars().take_while(|&c| c == '#').count();

However, treating one character as one byte is a bad idea because strings are UTF-8 encoded. UTF-8 is a variable-length encoding. You can use char_indices instead.
It's slightly more efficient to take a slice of the string, instead of drain and collect here. It avoids one extra allocation.
Change your function to accept any type that implements BufRead; this will allow you to write easier unit tests.
Actually add those unit tests!
There's no need to create a PathBuf, you aren't pushing path components on. You could just make a &Path, but most functions accept any type that can be converted to a Path (AsRef<Path>). &str implements that.
There's no need to take a reference to something being passed to println! or panic!. These macros automatically take a reference.

use std::fs;
use std::io::BufReader;
use std::io::prelude::*;

/// Get the title of a Markdown file.
///
/// Reads the first line of a Markdown file, strips any hashes and
/// leading/trailing whitespace, and returns the title.
fn title_string<R>(mut rdr: R) -> String
    where R: BufRead,
{
    let mut first_line = String::new();

    rdr.read_line(&mut first_line).expect("Unable to read line");

    // Where do the leading hashes stop?
    let last_hash = first_line
        .char_indices()
        .skip_while(|&(_, c)| c == '#')
        .next()
        .map_or(0, |(idx, _)| idx);

    // Trim the leading hashes and any whitespace
    first_line[last_hash..].trim().into()
}

/// Read the first line of the file into `title`.
fn main() {
    let path = "./example.md";

    let file = match fs::File::open(path) {
        Ok(file) => file,
        Err(_) => panic!("Unable to read title from {}", path),
    };
    let buffer = BufReader::new(file);

    let title = title_string(buffer);

    println!("The title is '{}'", title);
}

#[cfg(test)]
mod test {
    use super::title_string;

    #[test]
    fn top_level_heading() {
        assert_eq!(title_string(b"# This is a top-level heading".as_ref()),
                   "This is a top-level heading")
    }

    #[test]
    fn second_level_heading() {
        assert_eq!(title_string(b"## This is a second-level heading".as_ref()),
                   "This is a second-level heading");
    }

    #[test]
    fn fourth_level_heading() {
        assert_eq!(title_string(b"#### Let's jump straight to the fourth-level heading".as_ref()),
                   "Let's jump straight to the fourth-level heading");
    }
}

You should also investigate using a real Markdown parser to avoid nasty pitfalls.


Clojure core.async web crawler I'm currently a beginner with clojure and I thought I'd try building a web crawler with core.async.

What I have works, but I am looking for feedback on the following points:

How can I avoid using massive buffers when I don't want to lose values?
Am I using go blocks efficiently? Are there places where a thread would be more appropriate?
How can I better determine when I can finish crawling? Currently I have a timeout of 3 seconds on taking from the urls-chan and if the timeout wins, I assume we're done. This doesn't seem very efficient.

Here is the main part of the code:

(def visited-urls (atom #{}))
(def site-map (atom {}))

;; I've given massive buffers my two channels here because I don't want to drop
;; values. I'm not quite sure why they need to be so big, but anything smaller gives me:
;; Exception in thread "async-dispatch-1626" java.lang.AssertionError:
;;   Assert failed: No more than 1024 pending puts are allowed on a single channel. Consider using a windowed buffer.
;;   (< (.size puts) impl/MAX-QUEUE-SIZE)
(def urls-chan (chan 102400))
(def log-chan (chan 102400))

(def exit-chan (chan 1))

(defn get-doc
  "Fetches a parsed html page from the given url and places onto a channel"
  [url]
  (go (let [{:keys [error body opts headers]} (<! (async-get url))
            content-type (:content-type headers)]
        (if (or error (not (.startsWith content-type "text/html")))
          (do (log "error fetching" url)
              false)
          (Jsoup/parse body (base-url (:url opts)))))))

;; Main event loop
(defn start-consumers
  "Spins up n go blocks to take a url from urls-chan, store its assets and then
  puts its links onto urls-chan, repeating until there are no more urls to take"
  [n domain]
  (dotimes [_ n]
    (go-loop [url (<! urls-chan)]
             (when-not (@visited-urls url)
               (log "crawling" url)
               (swap! visited-urls conj url)
               (when-let [doc (<! (get-doc url))]
                 (swap! site-map assoc url (get-assets doc))
                 (doseq [url (get-links doc domain)]
                   (go (>! urls-chan url)))))
             ;; Take the next url off the q, if 3 secs go by assume no more are coming
             (let [[value channel] (alts! [urls-chan (timeout 3000)])]
               (if (= channel urls-chan)
                 (recur value)
                 (>! exit-chan true))))))

(defn -main
  "Crawls [domain] for links to assets"
  [domain]
  (let [start-time (System/currentTimeMillis)]
    (start-logger)
    (log "Begining crawl of" domain)
    (start-consumers 40 domain)
    ;; Kick off with the first url
    (>!! urls-chan domain)
    (<!! exit-chan)
    (println (json/write-str @site-map))
    (<!! (log "Completed after" (seconds-since start-time) "seconds"))))

(when-not (@visited-urls url)more than 1 consumers may be looking at the same unvisited url at this point. They will be crawling the same url which is unexpected but it doesn't seem to break anything.

I don't see any better way with clojure atom. Actually atom doesn't buy you anything here because all it does is to mutate a global state. I think java.util.concurrent.ConcurrentHashMap works better. visited-urls can be a map of URL to boolean representing if the URL has been visited. the condition should be .putIfAbsent(url, true) == null


Can this simple percentage calculator be further optimized? public double getValueWithPercentage(double number, double percentage)
{
    return number + ((percentage / 100.0) * number);
}

Are there any tricks to optimize this simple code?

Since floating-point divide is sometimes a little slower than multiply (and never faster, AFAIK), I'd write your function like this:

public double getValueWithPercentage(double number, double percentage)
{
    return number * (1 + percentage * 0.01);
}


Generic container assignment For setters that set a member collection, I write template functions so as to accept any collection type as input:

class HasItems
{
public:
    template <typename C>
    void items(const C& items) {
        // Copy-and-swap omitted
        m_items.assign(std::begin(items), std::end(items));
    }

private:
    std::vector<Item> m_items;
}

But suppose Item is big, and I want to move them around instead of copy if possible.  And furthermore, suppose someone passed me an xvalue std::vector<Item>; I could just move that whole container into place and avoid touching the elements at all!  That looks like this:

class HasItems
{
public:
    template <typename C>
    void items(C&& items) {
        // Helper function implemented below
        assign(m_items, std::forward<C>(items));
    }

private:
    std::vector<Item> m_items;
}

// Assigns the contents of src to dest, moving as much as possible
template <typename C1, typename C2>
void assign(C1& dest, C2&& src) {
    assign(dest, std::forward<C2>(src), std::is_assignable<C1&, C2&&>{});
}

// Called if we can use operator=
template <typename C1, typename C2>
void assign(C1& dest, C2&& src, std::true_type use_equals) {
    dest = std::forward<C2>(src);
}

// Called if we must use dest.assign()
template <typename C1, typename C2>
void assign(C1& dest, C2&& src, std::false_type use_equals) {
    assign_elements(dest, std::forward<C2>(src), std::is_rvalue_reference<C2&&>{});
}

// Called if we can move the elements
template <typename C1, typename C2>
void assign_elements(C1& dest, C2&& src, std::true_type move) {
    dest.assign(std::make_move_iterator(std::begin(src)),
                std::make_move_iterator(std::end(src)));
}

// Called if we must copy the elements
template <typename C1, typename C2>
void assign_elements(C1& dest, C2&& src, std::false_type move) {
    dest.assign(std::begin(src), std::end(src));
}

Is this a good approach?  In particular, is there a nice way to avoid the tag dispatch on std::is_rvalue_reference<C2&&> in the assign_elements() case?  (It was avoided in the operator= case by using dest = std::forward<C2>(src) instead of using std::move() conditionally.)  It seems to pass my simple set of unit tests.

I've implemented this idiom and discovered a major caveat after a few weeks: it doesn't play nicely with boost::iterator_range or other collection views.  An example:

std::vector<std::string> dest;
std::list<std::string> src{"foo", "bar"};
// Moves elements out of src, expected
assign(dest, std::move(src));

src = {"foo", "bar"};
// Also moves elements out of src!!!
assign(dest, boost::make_iterator_range(src));

This happens because boost::make_iterator_range(src) is an xvalue.  I don't see a good way to distinguish between "container" (meaning safe to move from xvalues) and "container view" (meaning not safe to move from).


Converting a list of integers into a two-dimensional array in Java I want to convert a list of integers into a two-dimensional array for use with TestNG DataProvider.  Here is the code I've written:

static List<Integer> studentList = new ArrayList<Integer>();

@Test()
public void testMethod(){
    studentList.add(477915);
    studentList.add(477916);
    studentList.add(477917);
    System.err.println(studentList);
}

@DataProvider(name = "persons")
public static Object[][] primeNumbers() {
    Object [][] objArray = new Object[studentList.size()][];

    for(int i=0;i< studentList.size();i++){
        objArray[i] = new Object[1];
        objArray[i][0] = studentList.get(i);
    }

    return objArray;
}

@Test(dataProvider="persons")
public void testRunPersonRules(int val) {
    System.err.println(val);
}

Is there a better way to accomplish this?

You can use streams:

Object[][] data = studentList.stream()
        .map(studentId -> new Object[]{studentId})
        .toArray(Object[][]::new);


Searching a list of pairs of numbers This works fine but I feel it can be better optimized.

#!/usr/bin/env py

s = "1:5.9,1.5:7,2:10,4:18,8:40"
load_value = raw_input("Enter a load value\n")
li =  s.split(',')
driver = None
for index,item in enumerate(li):
    if load_value == li[index].split(':')[1]:
        driver = li[index].split(':')[0]
        print driver
if not driver:
    print "Could not find driver value"

Output:

Enter a load value
5.9
1

Maybe you can use dict instead of str to store the driver-load values. If the driver is the key, you can search easily.

#!/usr/bin/env py

s = {'5.9':'1', '7':'1.5', '10':'2', '18':'4', '40':'8'}
load_value = raw_input("Enter a load value\n")
driver = s.get(load_value)
if driver is None:
    print "Could not find driver value"
else:
    print driver


Canonicalizing a large set of addresses using many regex substitutions I have a script that is standardizing a large amount of data in the database. The standardization involves applying over 500 regular expressions to the data.

Here is some quick pseudocode:

Load the records from the database
for each record
   for each regular expression ##predefined list of 500 regular expressions
      apply regular expression to record
    end;
    write record to file
end;

Here is the relevant block of code in Perl:

while(my @row = $queryHandle->fetchrow_array())
{
    $accountKey = @row[0];
    $addressLine1 = @row[1];
    $addressLine2 = @row[2];

    #first remove all special characters leaving only numbers and alphabets
    $addressLine1 =~ s/[^A-Za-z0-9 ]//g;
    $addressLine2 =~ s/[^A-Za-z0-9 ]//g;

    for my $regexRef (@regexesList)
    {
        #now standardize the addresses
        $addressLine1 =~ s/$regexRef->{pattern}/$regexRef->{output}/ig;
        $addressLine2 =~ s/$regexRef->{pattern}/$regexRef->{output}/ig;
    }

    my $standardizedAddress = $addressLine1 . $addressLine2;
    $standardizedAddress =~ s/\s+//g; #remove all white space
    #Et_Run_Log("the normalized address is: $normalizedAddress");

    print $dataFileHandle "${standardizedAddress}\n";
    $rowCount++;
}

An excerpt of regexList:

my %regexHash;
$regexHash{pattern} = "\bstr\b";
$regexHash{output} = "street";

#add regex hash to list
push(@regexesList, {%regexHash});

$regexHash{pattern} = "\brd\b";
$regexHash{output} = "road";
push(@regexesList, {%regexHash});

This is working code but the performance is abysmal. Currently the script has been running for 2.5hours and has written out 3.13 million records to the output file with about 7million to go.

Is this the best it can get? Is there another faster, or less slower way? Maybe writing each row to a file first and then run each regular expression on the whole file?

I would like to know if there is better way to implement this before I try the above mentioned alternative.

Nested loops can be problematic, but you can build regex which can be applied once instead of running foreach

my %replace = (
  str => { re => '\bstr\b', output => "street" },
  rd  => { re => '\brd\b', output => "road" },
);
my ($reCombined) = map qr/($_)/, join "|", map $_->{re}, values %replace;

and later inside while loop,

# my $string = "Foo str bar rd foo";
$string =~ s/$reCombined/$replace{$1}{output}/g;


Tasks and WaitAll\WhenAll Could you help me to speed up/optimize this code:

// Get API Keys
var keys = xRoot.Elements("key").Select(key => new { ID = int.Parse(key.Attribute("id").Value), VerificationCode = key.Attribute("verificationCode").Value });

// Get status of account using API Key
var tasks = keys.Select(key => EveOnlineClient.GetAccountStatusAsync(key.ID, key.VerificationCode));

// Get info about API Key
// Each API key may contain one or more game characters
var tasks2 = keys.Select(key => EveOnlineClient.GetAPIKeyInfoAsync(key.ID, key.VerificationCode).ContinueWith(x =>
    {
        var characters = x.Result.Characters;

        // Get the sheet for each character in each API key
        var tasks3 = characters.Select(character => EveOnlineClient.GetCharacterSheetAsync(key.ID, key.VerificationCode, character.ID));

        // Get skill in training for each character in each API key
        var tasks4 = characters.Select(character => EveOnlineClient.GetSkillInTrainingAsync(key.ID, key.VerificationCode, character.ID));

        Task.WaitAll(tasks3.ToArray());
        Task.WaitAll(tasks4.ToArray());
    }));

await Task.WhenAll(tasks.Concat(tasks2));

Every method of EveOnlineClient downloads XML to local cache if needed (if local cache has expired), parses it and returns as an object.

int.Parse(key.Attribute("id").Value)

XAttribute (and XElement) supports implicit cast to int (and other types), this means you can write just:

(int)key.Attribute("id")

One more advantage of this approach is that it uses the invariant culture, unlike the Parse() methods, which use the current culture by default. This is especially important for decimal values, since many cultures use , as the decimal separator, not ..

And if you're going to use casts for numeric types, you might want to use them for string too for consistency, instead of accessing Value.

var tasks
var tasks2
var tasks3
var tasks4

Those are not very good variable names, try to make names more descriptive, like accountStatusTasks.

Also, it's hard to advise more about the structure of the code when you don't use all those tasks in any way, except to wait for them. I assume that's not all they're good for.

There are not many cases where ContinueWith() is useful, when you can use await. Here, I would make the Select() lambda async, which also means you can use await Task.WhenAll() instead of Task.WaitAll(), which blocks a thread unnecessarily:

var tasks2 = keys.Select(async key =>
{
    var keyInfo = await EveOnlineClient.GetAPIKeyInfoAsync(key.ID, key.VerificationCode);

    var characters = keyInfo.Characters;

    // Get the sheet for each character in each API key
    var tasks3 = characters.Select(character => EveOnlineClient.GetCharacterSheetAsync(key.ID, key.VerificationCode, character.ID));

    // Get skill in training for each character in each API key
    var tasks4 = characters.Select(character => EveOnlineClient.GetSkillInTrainingAsync(key.ID, key.VerificationCode, character.ID));

    await Task.WhenAll(tasks3.Concat(tasks4));
});

Quite often, code that makes many requests to a single server in parallel is limited by ServicePointManager.DefaultConnectionLimit, which by default is set to just 2. But if you decide to change this, make sure to follow any guidelines about rate limiting of the API you're using; otherwise, you might easily end up blocked.


Factory for constructing directed graphs in C++ I want to implement a directed graph which has 3 main kind of edges:

self referencing edges
non directional edges between two nodes
directional edges between two nodes

Also I want to be able to do global operations on the graph, like e.g. counting or removing all edges fulfilling certain criteria.

The easiest program structure I came up with is:

#include <unordered_set>

using namespace std;

struct Edge;

struct Node
{
    Node(std::string name):name(name){}
    string name;
    std::unordered_set<Edge*> edges;
};

struct Edge
{
    Edge(Node * node1): node1(node1){}
    double weight;
    Node * node1;
};

struct EdgeBetweenTwoNodes: public Edge
{
    EdgeBetweenTwoNodes(Node * node1, Node * node2 ): Edge(node1), node2(node2){}
    Node * node2;
};

struct DirectionalEdge: public EdgeBetweenTwoNodes
{
    DirectionalEdge(Node * node1, Node * node2, bool direction ): EdgeBetweenTwoNodes(node1,node2), direction(direction){}
    bool direction;
};

struct EdgesContainer
{
    std::unordered_set<Edge*> all_edges;

    void register_new_edge(Edge * edge)
    {
        all_edges.insert(edge);
    }

    //do all kind of manipulations on all edges of a cetain type...
};

struct EdgeFactory
{
    EdgeFactory(){}

    void create_edge(Node* node1,EdgesContainer & edge_container)
    {
        Edge * edge = new Edge(node1);
        node1->edges.insert(edge);

        edge_container.register_new_edge(edge);
    }

    void create_edge(Node* node1, Node* node2, EdgesContainer & edge_container)
    {
        EdgeBetweenTwoNodes * edge = new EdgeBetweenTwoNodes(node1,node2);
        node1->edges.insert(edge);
        node2->edges.insert(edge);

        edge_container.register_new_edge(edge);
    }

    void create_edge(Node* node1, Node* node2, bool direction, EdgesContainer & edge_container)
    {
        DirectionalEdge * edge = new DirectionalEdge(node1,node2,direction);
        node1->edges.insert(edge);
        node2->edges.insert(edge);

        edge_container.register_new_edge(edge);
    }
};

int main()
{
    Node * A = new Node("A");
    Node * B= new Node("B");
    Node * C= new Node("C");

    EdgesContainer edges;

    EdgeFactory edge_factory;
    edge_factory.create_edge(A,edges);
    edge_factory.create_edge(A,B,edges);
    edge_factory.create_edge(A,C,1,edges);

    return 0;
}

What do you think about it? Is this the correct use of a so called "Factory?"

Don't use using namespace std; it pollutes the namespace and creates name collisions. Only do this with concrete classes and only within a namespace or cpp file.

new without delete => leaks. Instead use smart pointers or store them by value where it makes sense.

There is no need to create a class hierarchy for the edges, instead use a enum to differentiate: enum{BIDIR, FROM_1_TO_2, FROM_2_TO_1} and a self referential edge is a BIDIR and nodes 1 and 2 are equal.

unordered_set is far from the best data structure, it's slow to iterate over, access is very often a cache miss or two. Instead prefer data structures like std::vector where it is very fast to iterate over.

The best data structure depends a lot on what operations you will be doing on it, which you didn't mention so I can't really judge beyond saying that unordered_set is most likely a bad choice.


Implement clojure-a-like comp() function in python3 With a friend some time ago we wanted to write our own implementation of comp() clojure function in python3. we ended up writing the following function:

 def comp(*fs):
     """
     Takes a set of functions and returns a fn that is the
     composition of those fns. The returned fn takes a variable number of args,
     applies the rightmost of fns to the args, the next fn (right-to-left) to
     the result, etc.
     """
     if len(fs) == 1:
         return lambda *x: fs[0](*x)
     else:
         return lambda *x: fs[0](comp(*fs[1:])(*x))

In my oppinion it's still opaque and difficult to read. But it works as in:

 # Checking for single argument
 >>> [comp(lambda x: x*10,lambda x: x+2, lambda x: x*3, lambda x: x+3)(x) for x in range(6)]
 [110, 140, 170, 200, 230, 260]
 # Checking for multiple arguments
 >>> a = lambda x: sum(x)
 >>> b = lambda x, y, z: [x*1, y*2, z*3]
 >>> comp(a, b)(1, 2, 3)
 14

How could it be written better?

Your approach has one serious drawbacks: it creates N-1 new bound function instances on every invocation of the returned function with an argument length of N if big-O performance guarantees are relevant to you. To avoid this you can either use a loop or the reduce operation, both of which only create a fixed number of function instances (that doesn't depend on the number of functions to chain).

Additionally I believe that both variants are easier to understand than the code in your question because it doesn't rely on recursion.

Loop

def comp_loop(*fns):
    def comped(*x):
        for fn in reversed(fns):
            x = fn(*x)
        return x

    if len(fns) == 0:
        return lambda *x: x
    if len(fns) == 1:
        return fns[0]
    return comped

Reduce

from functools import reduce

def comp_reduce(*fns):
    if len(fns) == 0:
        return lambda *x: x
    if len(fns) == 1:
        return fns[0]
    return lambda *x: reduce(lambda accum, fn: fn(*accum), reversed(fns), x)


Better way of parsing string to boolean At the moment I am using this to check a boolean property in sharepoint 2007 using C# and .Net Framework 3.5

if (bool.Parse(listItem.Properties["Boolean Property"].ToString()) == false)

is there any better way of doing it ?

Edit

when my code try to convert property into string it gives me exception even when am validating if property is empty or not.

This Worked For me

using method "Contains Key" instead of "Properties.Containsfield"

object abc = "bool property";

foreach (SPListItem item in list.Items)
{
     if (item.Properties.ContainsKey(abc)

I am making some assumptions about your code, specifically that listItem is of type SPListItem and that listItem.Properties is a HashTable.  This is mainly to provide context for my explanation, but even if I am mistaken, most of my comments will still hold.  For the sake of context, let's assume your actual method looks like:

 void HandleBooleanListItemProperty( SPListItem listItem ){
    if (bool.Parse(listItem.Properties["Boolean Property"].ToString()) == false){
        // do something if the Boolean ListItem property is false
    }
    else
    {
        // do something if the Boolean ListItem property is true
    }
}

Validate that the object exists before you call ToString().  The way your method is currently written, you will get a NullReferenceException if the key "Boolean Property" is missing or the hashed value that corresponds to the "Boolean Property" key is null.  This refactoring will make the method look like:

void HandleBooleanListItemProperty2( SPListItem listItem ){
     object booleanPropertyValue = listItem.Properties["Boolean Property"];
     if ( ReferenceEquals( booleanPropertyValue, null ) ) {
         // do something if the Boolean ListItem property is missing
     }
     else if (bool.Parse(booleanPropertyValue.ToString()) == false){
         // do something if the Boolean ListItem property is false
     }
     else {
         // do something if the Boolean ListItem property is true
     }
 }

Use bool.TryParse instead of bool.Parse.  If the ToString() value of the hashed value corresponding to the key "Boolean Property" is anything other than "true" or "false" (case-insensitive), a FormatException will be thrown.  This refactoring will make the method look like:

 void HandleBooleanListItemProperty3( SPListItem listItem ){
     object booleanPropertyValue = listItem.Properties["Boolean Property"];
     bool parsedBooleanPropertyValue;
     if ( ReferenceEquals( booleanPropertyValue, null ) ) {
         // do something if the Boolean ListItem property is missing
     }
     else if (bool.TryParse(booleanPropertyValue.ToString(), out parsedBooleanPropertyValue) )
     {
         if(parsedBooleanPropertyValue == false){
            // do something if the Boolean ListItem property is false
         }
         else {
           // do something if the Boolean ListItem property is true
         }
     }
     else {
          // do something if the Boolean ListItem property's ToString() value was something other than "true" or "false"
     }
 }

If the hashed value that corresponds to the "Boolean Property" key is really a bool, then cast it before comparing.  Calling ToString() just throws away .NET's type-safety.  This refactoring will make the method look like:

 void HandleBooleanListItemProperty4( SPListItem listItem ){
     object booleanPropertyValue = listItem.Properties["Boolean Property"];
     if (booleanPropertyValue is bool)
     {
        bool castBooleanPropertyValue = (bool) booleanPropertyValue;
         if( castBooleanPropertyValue == false ){
            // do something if the Boolean ListItem property is false
         }
         else {
           // do something if the Boolean ListItem property is true
         }
     }
     else {
         // do something if the Boolean ListItem property is not a bool
         if ( ReferenceEquals( booleanPropertyValue, null ) ) {
             // do something if the Boolean ListItem property is missing
         }
         else {
             // do something if the Boolean ListItem property is something other than a bool
         }
     }
 }

Eliminate magic strings from your value accessor.  This requires removing the inline "Boolean Property" and declaring with that value, and then using the constant in order to access the value in every usage.  This decreases the possibility that a typos will cause bugs.  An article on magic strings is available at: http://codebetter.com/matthewpodwysocki/2009/03/19/functional-net-lose-the-magic-strings/ .  The resulting refactoring would look like:

 const string BooleanPropertyKey = "Boolean Property";

 void HandleBooleanListItemProperty5( SPListItem listItem ){
     object booleanPropertyValue = listItem.Properties[BooleanPropertyKey];
     if (booleanPropertyValue is bool)
     {
        bool castBooleanPropertyValue = (bool) booleanPropertyValue;
         if( castBooleanPropertyValue == false ){
            // do something if the Boolean ListItem property is false
         }
         else {
           // do something if the Boolean ListItem property is true
         }
     }
     else {
         // do something if the Boolean ListItem property is not a bool
         if ( ReferenceEquals( booleanPropertyValue, null ) ) {
             // do something if the Boolean ListItem property is missing
         }
         else {
             // do something if the Boolean ListItem property is something other than a bool
         }
     }
 }

Use the negation operator (!) instead of comparing to false.  This is much more readable and will save you time in the long run.  This final refactoring looks like:

 const string BooleanPropertyKey = "Boolean Property";

 void HandleBooleanListItemProperty6( SPListItem listItem ){
     object booleanPropertyValue = listItem.Properties[BooleanPropertyKey];
     if (booleanPropertyValue is bool)
     {
        bool castBooleanPropertyValue = (bool) booleanPropertyValue;
         if( !castBooleanPropertyValue ){
            // do something if the Boolean ListItem property is false
         }
         else {
           // do something if the Boolean ListItem property is true
         }
     }
     else {
         // do something if the Boolean ListItem property is not a bool
         if ( ReferenceEquals( booleanPropertyValue, null ) ) {
             // do something if the Boolean ListItem property is missing
         }
         else {
             // do something if the Boolean ListItem property is something other than a bool
         }
     }
 }


Vigenère cipher encryption in Scala I've worked out a functioning Vigenère encryption in Scala, and while it doesn't seem like my code is too hideous, I'm sure I could make it more functional and Scala-esque. Primarily, it seems like:

I've made the full key with a for-expression / zipWithIndex, I'm wondering if this is the best way to do this though. Would it be better to do this inside the encipher() function? Using an anonymous function? The way I have it now works, but I'm curious if there's a way to do so that'd be more in line with Scala best practices.
The actual encryption portion, specifically the output for case (c, i), is sort of messy/convoluted, and it seems like there might be a better way to do it.

Of course, I'll be grateful for any feedback. Definitely still getting my feet wet with Scala, and any help getting into a more functional state of mind would be appreciated.

object Vigenere {

// alphabet for use throughout object
private val alphabet = ('A' to 'Z').toList
//empty array to be filled with fillTabula() function
private var tabulaRecta = Array.ofDim[Char](26, 26)

/* fills the tabula recta array
** creates 26 rows of alphabets, each alphabet shifted
** left by one letter each iteration */
def fillTabula() = {
    for (row <- 0 to 25) {
        for (col <- 0 to 25) {
            tabulaRecta(row)(col) = alphabet((row + col) % 26)
        }
    }
}

/* create the full key by repeating the key until its as long
** as the plaintext. */
def makeFullkey(key: String, text:String): String = {
    val fkey = for (c <- text.zipWithIndex) yield c match {
            case (c, i) => key(i % key.length)
        }
        return fkey.mkString
}

def encipher(key: String, text:String): String = {
    // fill the tabula recta
    fillTabula()
    val fkey = makeFullkey(key, text)

    /* to encipher, zip the plaintext to return each letter with
    ** its index. if it's a space, just add space to ciphertext.
    ** if it's not a space, find the corresponding row in the
    ** tabula recta, then go to the appropriate column to
    ** return the ciphertext character.
    ** the second case here is a mess, there's got to be a nicer way. */

    val ciphertext = for (c <- text.zipWithIndex) yield c match {
        case (' ', _) => ' '
        case (c, i)   => tabulaRecta(alphabet.indexOf(fkey(i)))(alphabet.indexOf(c))
    }
    return ciphertext.mkString
}

def main(args: Array[String]) {
    val Array(text, key) = args

    println("Plaintext: " + text)
    println("Key: " + key)
    println("Ciphertext: " + encipher(key.toUpperCase, text.toUpperCase))

}
}

Edit: I realized that I could make the fullkey using zipWithIndex as well, and also found a better place to use toUpperCase. I've changed the code and updated my question accordingly.

I'm not a scala/fp expert but here is my 2 cents.

Your code reads very much like you have read the definition on wikipedia and translated it into scala. As the wikipedia definition is written as a human implemented algorithm you end up with a very procedural approach.

A few specific things:

tabulaRecta - this is constant, making it a var is a massive code smell
makeFullkey - you use zipWithIndex but don't use c, just use index
why can't tabulaRecta be a function?
why create a fullKey - can't you compute the key char as a function?


Base 10 to Base N conversion Before I begin using this code in vital systems, I wanted to quadruple-check that it was soundproof. I plan to implement this as part of a larger program, obviously not by itself. Are there any clear flaws (output is incorrect) or optimization issues in this code?

#include <iostream>

using namespace std;

int main() {
    int convertFromCopy, digit, convertTo;
    unsigned int convertFrom;
    string stringDigit, answer;

    cout << "Num (base 10): ";
    cin >> convertFrom;
    cout << "Base: ";
    cin >> convertTo;

    convertFromCopy = convertFrom;

    while (convertFrom != 0){
        digit = convertFrom % convertTo;

        if (digit < 10)
            stringDigit = '0' + digit;
        else
            stringDigit = digit - 10 + 'A';

        answer = stringDigit + answer;

        convertFrom /= convertTo;
    }
    cout << convertFromCopy << " written in base " << convertTo << " is: " << answer;
    return 0;
}

And as a note, the input will always be int, so I do not need to account for an incorrect input (i.e. if a char is entered where an int should go).

using namespace std;

You may have already seen Why is “using namespace std;” considered bad practice?

Personally, I just find it easier and more readable to write out std:: where necessary.

Declare where used

    int convertFromCopy, digit, convertTo;
    unsigned int convertFrom;
    string stringDigit, answer;

In old C, you used to have to define every variable at the beginning of a block.  In modern C and all C++, you can declare them anywhere.

    int convertTo;
    unsigned int convertFrom;

    std::cout << "Num (base 10): ";
    std::cin >> convertFrom;
    std::cout << "Base: ";
    std::cin >> convertTo;

    std::string answer;
    int convertFromCopy = convertFrom;

    while (convertFrom != 0){
        int digit = convertFrom % convertTo;

        std::string stringDigit;
        if (digit < 10) {
            stringDigit = '0' + digit;
        } else {
            stringDigit = digit - 10 + 'A';
        }

        answer = stringDigit + answer;

        convertFrom /= convertTo;
    }

    std::cout << convertFromCopy << " written in base " << convertTo << " is: " << answer;

I greatly prefer to use the block form for control statements (i.e. add {} to if and else here).  I find it both easier to read and less subject to editing errors.

Abstract into functions

    int convertTo;
    unsigned int convertFrom;

    std::cout << "Num (base 10): ";
    std::cin >> convertFrom;
    std::cout << "Base: ";
    std::cin >> convertTo;

    std::string answer = convertFromBaseTo(convertFrom, convertTo);

    std::cout << convertFrom << " written in base " << convertTo << " is: " << answer;

Note that the function removes the need for the convertFromCopy variable.

std::string convertFromBaseTo(unsigned int convertFrom, int convertTo) {
    std::string answer;

    while (convertFrom != 0){
        int digit = convertFrom % convertTo;

        char stringDigit;
        if (digit < 10) {
            stringDigit = '0' + digit;
        } else {
            stringDigit = digit - 10 + 'A';
        }

        answer.insert(answer.begin(), stringDigit);

        convertFrom /= convertTo;
    }

    return answer;
}

I changed stringDigit to a char because it's just a single character.  Adding to answer changed a bit as well.


Repetitive event handler for a toggling UI control I feel like this bit of code could have been written more elegantly, especially with the huge if/else statements. Can somebody help me break this down? It would really help me out in the future.

// Contact
var c = 0;
var contactHeight = $('#contact-keebs').outerHeight();
$('#mail-wrap').click(function (e) {

    e.preventDefault();

    if (c++ % 2 == 0) {

        $('#contact-button').addClass('project-button').css('width', '71px').text('Projects');
        $('.mail-icon').attr('src', site.theme_path + '/img/icon-projects.png');
        $('#contact-button').shuffleLetters({callback:function(){
            if ($(window).scrollTop() != 0) {
                $('html, body').animate({
                    scrollTop : 0
                },200, function() {
                    $('#contact-keebs').fadeIn(200);
                    $('body').css({
                        height   : contactHeight,
                        overflow : 'hidden'
                    });
                    $('#contact-info, #clients').addClass('animated fadeInUp');
                });
            } else {
                $('#contact-keebs').fadeIn(200);
                $('body').css({
                    height   : contactHeight,
                    overflow : 'hidden'
                });
                $('#contact-info, #clients').addClass('animated fadeInUp');
            }
        }
        });

    } else {

        $('#contact-button').removeClass('project-button').css('width', '96px').text('Get in touch');
        $('.mail-icon').attr('src', site.theme_path + '/img/icon-contact.png');
        $('#contact-button').shuffleLetters({callback:function(){
            if ($(window).scrollTop() != 0) {
                $('html, body').animate({
                    scrollTop : 0
                },200, function() {
                    $('#contact-info, #clients').removeClass('animated fadeInUp');
                    $('body').css({
                        height   : '',
                        overflow : ''
                    });
                    $('#contact-keebs').fadeOut(200);
                });
            } else {
                $('#contact-info, #clients').removeClass('animated fadeInUp');
                $('body').css({
                    height   : '',
                    overflow : ''
                });
                $('#contact-keebs').fadeOut(200);
            }
        }
        });
    }
});

HTML

// Button

    Get in touch

// Contact section

Title statement

Email

Clients &amp; Partners

Client 1

Client 2

Client 3

Client 4

Client 5

Client 6

Client 7

Client 8

// Contact
var contactHeight = $('#contact-keebs').outerHeight();

function in_keebs() {
    $('#contact-keebs').fadeIn(200);
    $('body').css({
        height   : contactHeight,
        overflow : 'hidden'
    });
    $('#contact-info, #clients').addClass('animated fadeInUp');
}

function out_keebs() {
    $('#contact-info, #clients').removeClass('animated fadeInUp');
    $('body').css({
        height   : '',
        overflow : ''
    });
    $('#contact-keebs').fadeOut(200);
}

function show_project_button(e) {
    e.preventDefault();

    $('#contact-button').addClass('project-button').css('width', '71px').text('Projects');
    $('.mail-icon').attr('src', site.theme_path + '/img/icon-projects.png');
    $('#contact-button').shuffleLetters({callback:function(){
        if ($(window).scrollTop() != 0) {
            $('html, body').animate({
                scrollTop : 0
            },200, in_keebs);
        } else {
            in_keebs();
        }
    }
    });

    $('#mail-wrap').click(hide_project_button);
}

function hide_project_button(e) {
    e.preventDefault();

    $('#contact-button').removeClass('project-button').css('width', '96px').text('Get in touch');
    $('.mail-icon').attr('src', site.theme_path + '/img/icon-contact.png');
    $('#contact-button').shuffleLetters({callback:function(){
        if ($(window).scrollTop() != 0) {
            $('html, body').animate({
                scrollTop : 0
            },200, out_keebs);
        } else {
            out_keebs();
        }
    }
    });

    $('#mail-wrap').click(show_project_button);
}

$('#mail-wrap').click(show_project_button);

First, I'm not at all convinced that these are the right function names, but I needed names and these at least seem somewhat related.  Part of the problem is that I don't really understand what you're doing, as I don't know what the underlying HTML is.

The insight to get rid of the outer if/else is that you are simply alternating between them.  Instead of using a variable for tracking, we change the onclick behavior at the end of each variant.  We pick one and start with it.  This gives us the same alternating behavior as the if/else did.  We can also get rid of c.

We're still stuck with the inner if/else blocks, but we can remove the repeated code into their own functions.

Note that I haven't tested this, as I don't have enough context to run it.


A Korean (Language) Dictionary Function : User enters a word in korean and the program translates (defines) it in english.

  What improvements can I make?

  Extra : What other challenges would you recommend that incorporates both JSON and python?

KoreanDictionary.py

import json
from difflib import get_close_matches

korean_data = json.load(open("korean_vocabulary.json"))

def my_translator(word):
    if word in korean_data.keys():
        return korean_data[word]
    elif len(get_close_matches(word,korean_data.keys(),n=1,cutoff=.69)) > 0:
        word = get_close_matches(word,korean_data.keys(),n=1,cutoff=.69)[0]
        return korean_data[word]
    else:
        return "Word does not exist."

if __name__ == '__main__':

    user_input = input("Define:")
    definition = str(my_translator(user_input)).replace('[','').replace(']','').replace("'",'')
    print(user_input + ": " + definition)

korean_vocabulary.json

{"안녕하세요": ["hello"],"죽어버려": ["GO DIE"],"상황": ["situation"],"어려워": ["hard; difficult"],
    "모르겠다": ["I don't know."],"조건": ["condition"],"정규직 직업": ["full-time job"],
    "탈락": ["eliminate; out"], "눈치": ["quick to catch; tactfulness"],"슬픈": ["sad"],
    "복잡한": ["complicated"],"완전": ["completely"],"반대": ["opposite"],"대박": ["jackpot; awesome"],
    "어꺠": ["shoulders"],"얼굴": ["face"]}

if word in korean_data.keys():
    return korean_data[word]

This is better written as:

trans = korean_data.get(word)
if trans:
    return trans

This:

elif len(get_close_matches(word,korean_data.keys(),n=1,cutoff=.69)) > 0:
    word = get_close_matches(word,korean_data.keys(),n=1,cutoff=.69)[0]
    return korean_data[word]

calls for a few changes. Firstly, you don't need an elif, simply an if - because the previous block returned. Also, you should avoid re-issuing that entire get_close_matches call, and instead write a temporary variable:

words = get_close_matches(word,korean_data.keys(),n=1,cutoff=.69)
if words:
    return korean_data[words[0]]

This:

return "Word does not exist."

is not really good practice. It's "in-band error information", i.e. you're mixing valid data with an error message. Instead, you should be raise-ing an exception, catching it in your calling code, and outputting an error message there.

  What other challenges would you recommend that incorporates both JSON and python?

Download or scrape a real Korean-English dictionary dataset, either incorporating it into your application or setting up a REST client to contact a translation server somewhere. If local, save it as a format more efficient than JSON - perhaps compressed pickle, or SQLite. There are many ways to store a local database, and each has its advantages and disadvantages, but that's a topic for another question.


Singleton Class for WordPress Plugin I'm developing a WordPress plugin using the singleton pattern with the following requirements:

There should ever exist one instance of the plugin
Another instance of the object should never be instantiated, created, cloned, copied etc in any way.
The singular plugin object and its public methods and vars should be easily accessible and usable in any scope, by anything including other plugins

The last one is a given with singleton pattern, but there is one concern I have:
 - __construct() should run only once, and should never run again. The code inside __construct() should never be reparsed and reprocessed however the singleton is instantiated.

I already tested the code and it seems to work, however the code in __construct() was being reprocessed every time i requested the instance of the object through get_instance() outside the plugin until I added self::$instance = $this; at the top.

I'm wondering whether I am violating the guidelines I listed above and also if it is possible for anyone to violate them through any kind of logic or method that they put into their plugins. Also whether this code will last at least a few PHP versions into the future.

    class particular_plugin {

        private static $instance = null;

        protected function __construct()
        {
            self::$instance = $this;

            // Numerous initialization variables and actions here
            // including calls to functions inside the class

        }

        public static function get_instance()
        {
            if (null === static::$instance) {
                static::$instance = new static();
            }

            return static::$instance;
        }

        private function __clone()
        {

        }

        private function __wakeup()
        {

        }

    }
    // Initializing the plugin here
    $particular_plugin = particular_plugin::get_instance();

    function particular_plugin_get()
    {
        // This function allows any plugin to easily retrieve this plugin object and access its public methods and vars as they exist at any given point in time
        return particular_plugin::get_instance();

    }

Forget everything but the instatiator, constructor and property, you don't call new static; you call new self(), then it jumps into the constructor, the way I do singleton is:

class Foo
{
    private static $init;

    private function __construct() {  }

    public static function init()
    {
        return static::$init = (
          null === static::$init ? new self() : static::$init
        );
    }
}

$foo = Foo::init();

This makes it so when new self() is called, it jumps to the constructor and assigns self::$init to really be an instance of the class


Python common file operations generalization Functions with common and simple file operations have started to repeat in my code. Many like this one, but with subtle differences for example:

def do_something_with_file(file_name, access_mode, data):
    with open(file_name, access_mode) as f:
        f.do_something(data)

This is my solution so far:

import pickle
from functools import partial

def _open_file(func, access_mode, file_name, *args, **kwargs):
    with open(file_name, access_mode) as f:
        return func(f, *args, **kwargs)

open_file = partial(_open_file, lambda f: f, 'r')

read_lines = partial(_open_file, lambda f: list(f), 'r')

write_lines = partial(_open_file, lambda f, d: f.writelines(d), 'w')

load_pickle = partial(_open_file, lambda f: pickle.load(f), 'rb')

save_pickle = partial(_open_file, lambda f, d: pickle.dump(d, f), 'wb')

Example usage:

write_lines('file.txt', data)
pickle = load_pickle('file.pickle')

It seems readable enough for simple tasks and more complicated function definitions could also be passed to partial instead of simple anonymous functions like above.

Does anyone see any drawbacks of using this or has suggestions on how it could be improved?

The code is straighforward and reads good; there is not much to say per se. I would just advise to use f.readlines instead of list(f).

However, there exist a pattern in Python when you need to wraps functions using a common behavior: decorators.

I suggest having a decorator taking the open_mode as parameter:

import pickle
from functools import wraps

def opening(access_mode='r'):
    def decorator(func):
        @wraps(func)
        def wrapper(filename, *args, **kwargs):
            with open(filename, access_mode) as f:
                return func(f, *args, **kwargs)
        return wrapper
    return decorator

@opening()
def read_lines(file_):
    return file_.readlines()

@opening('w')
def write_lines(file_, data):
    file_.writelines(data)

@opening('rb')
def load_pickle(file_):
    pickle.load(file_)

@opening('wb')
def save_pickle(file_, data):
    pickle.dump(data, file_)

Usage is the same.


Aggregate query for drawing temporal statistic I used to write simple Mongo queries, but this time I had to create an aggregate query. The purpose is to draw temporal statistic on the main page of my web app.

I'm not confident about his performance in production environment, so I'm looking for advice and review about this one.

db.runs.aggregate([
    {$project: {
        "month": {
                $cond: [
                    {$ifNull: ["$startTime", false]},
                    {$month:"$startTime"},
                    -1
                ]
            },
        "year": {
                $cond: [
                    {$ifNull: ["$startTime", false]},
                    {$year:"$startTime"},
                    -1
                ]
            },
        "nbItem": 1,
        "cleanUp.nb": 1,
        "startTime": 1
    }},
    {$match: {
        "startTime" : {$ne:null}
    }},
    {$group: {
        "_id": {
            "month":"$month",
            "year":"$year"
        },
        "nbRun":{$sum:1},
        "sumItem":{$sum:"$nbItem"},
        "sumInserted":{$sum:"$cleanUp.nb"}
    }}
])

As expected the result looks like:

{
    "_id" : {
        "month" : 2,
        "year" : 2017
    },
    "nbRun" : 5,
    "sumItem" : 567372,
    "sumInserted" : 35107
}
{
    "_id" : {
        "month" : 10,
        "year" : 2016
    },
    "nbRun" : 3,
    "sumItem" : 677722,
    "sumInserted" : 16090
}
...

Here your doing something very redondant :

if you move the $match stage at the begining of the aggregation, you no longer need to check wether startTime is null in your project stage:

Plus, a $match stage should always be at the beginning of you aggregation pipeline, as it will reduce the number of documents treated in the next stage.
from MongoDB documentation:

  Pipeline Optimization

  Place the $match as early in the aggregation pipeline as possible.
  Because $match limits the total number of documents in the aggregation
  pipeline, earlier $match operations minimize the amount of processing
  down the pipe.

  If you place a $match at the very beginning of a
  pipeline, the query can take advantage of indexes like any other
  db.collection.find() or db.collection.findOne().

so your query becomes:

db.runs.aggregate([
    {$match: {
        "startTime" : {$ne:null}
    }},
    {$project: {
        "month": {$month:"$startTime"},
        "year": {$year:"$startTime"},
        "nbItem": 1,
        "cleanUp.nb": 1,
        "startTime": 1
    }},
    {$group: {
        "_id": {
            "month":"$month",
            "year":"$year"
        },
        "nbRun": {$sum:1},
        "sumItem": {$sum:"$nbItem"},
        "sumInserted": {$sum:"$cleanUp.nb"}
    }}
])

Now we can push it even further by merging the $project and the $group stage into a single $group stage like this :

db.runs.aggregate([
    {$match: {
        "startTime" : {$ne:null}
    }},
    {$group: {
        "_id": {
            "month": {$month: "$startTime"},
            "year": {$year: "$startTime"}
        },
        "nbRun": {$sum:1},
        "sumItem": {$sum:"$nbItem"},
        "sumInserted": {$sum:"$cleanUp.nb"}
    }}
])

The query should run faster and is now way more readable.

Finally, you can look at the explain() of the query to understand exactly what happens. Make sure that the startTime field is properly indexed.

If the query still does not run fast enough, consider adding the field year and month directly in your documents to get rid of the $year and $month operators


Better implementation of checking function I have a checking function that checks the correct type for a class Check_Annotation, I was wondering if there is a better way to implement this, my current code right now is:

def _check_list_or_tuple (self, param, annot, value, check_history):
    assert isinstance(value, type(annot)), "AssertionError: 'x' failed annotation check(wrong type): value = {}, was type {} ...should be type {}".format(value, type_as_str(value), type_as_str(annot))
    if len(annot) == 1 and type(annot[0]) != list and type(annot[0])!= tuple and inspect.isfunction(annot[0]) == False:
        for x in value :
            check_history = ''
            check_history += str(annot[0])
            assert type(x) == annot[0], "AssertionError: 'x' failed annotation check(wrong type): value = {}, was type {} ...should be type {} \nlist[{}] check: {}".format(x, type_as_str(x), annot[0], annot.index(annot[0])+1, check_history)
    elif type(annot[0]) == list or type(annot[0]) == tuple or inspect.isfunction(annot[0]) == True:
        for i in range(len(value)):
            self.check(param, annot[0], value[i])
    elif len(annot) > 1:
        check_history = ''
        check_history += str(annot[0])
        assert type (value[0]) == annot[0], "AssertionError: 'x' failed annotation check(wrong type): value = {}, was type {} ...should be type {} \nlist[{}] check: {}".format(value[0], type_as_str(value[0]), annot[0], annot.index(annot[0])+1, check_history)
        check_history = ''
        check_history += str(annot)
        assert len(annot) == len(value), "AssertionError: 'x' failed annotation check(wrong number of elements): value = {} \nannotation had {} elements[{}]".format(value, len(annot), check_history)
        for x in value[1:]:
            check_history = ''
            check_history += str(annot[1])
            assert type(x) == annot[1], "AssertionError: 'x' failed annotation check(wrong type): value = {}, was type {} ...should be type {} \nlist[{}] check: {}".format(x, type_as_str(x), annot[1], annot.index(annot[1])+1, check_history)

To me it is a bit rough on implementation and repetitive code. Does anyone have an idea of a shorter way to go about doing this?

I don't have time to check all of this, but here are some ideas ...
 - place long strings in parenthesis and break them up into multiple lines
 - if those strings are used more than once, use variables for them
 - break out the for loops as a separate function

type_error_msg = ("AssertionError: 'x' failed annotation check(wrong type):"
                  " value = {}, was type {} ...should be type {}")
list_error_msg = ("AssertionError: 'x' failed annotation check(wrong type):"
        " value = {}, was type {} ...should be type {} \nlist[{}] check: {}")
elem_number_error_msg = ("AssertionError: 'x' failed annotation check(wrong "
         "number of elements): value = {} \nannotation had {} elements[{}]")

def _check_values(value, annot, index):
    for x in value:
        check_history = ''
        check_history += str(annot1)
        assert type(x) == annot, self.list_error_msg.format(x,
             type_as_str(x), annot, index,  check_history)

def _check_list_or_tuple (self, param, annot, value, check_history):
    assert isinstance(value, type(annot)), self.type_error_msg.format(value,
                                     type_as_str(value), type_as_str(annot))
    if (len(annot) == 1 and type(annot[0]) != list and
        type(annot[0])!= tuple and inspect.isfunction(annot[0]) == False):
        self._check_values(value, annot[0], 1)
    elif (type(annot[0]) == list or type(annot[0]) == tuple or
          inspect.isfunction(annot[0]) == True):
        for i in range(len(value)):
            self.check(param, annot[0], value[i])
    elif len(annot) > 1:
        check_history = ''
        check_history += str(annot[0])
        assert type (value[0]) == annot[0], self.list_error_msg.format(
            value[0], type_as_str(value[0]), annot[0],
            annot.index(annot[0])+1, check_history)
        check_history = ''
        check_history += str(annot)
        assert len(annot) == len(value), self.elem_number_error_msg.format(
                                        value, len(annot), check_history)
        self._check_values(value[1:], annot[1], 2)


Clean code attempt on codechef.com FCTRL I have been reading Clean Code and decided to start working problems on Codechef.com attempting to apply some of what I have learned.

Do I seem to be on the right track or am I way off?

The challenge is to find the number of trailing zeroes in the decimal form of N!, where 1 ≤ N ≤ 109.

I am more concerned with the coding style than the way I solved the problem but any comments are appreciated.

#include <iostream>
#include <vector>

int requestNumInts();
bool validateInputSize(const int totalNumInts);
std::vector<int>* createVector(int totalNumInputs);
void loadVector(std::vector<int>* numbersEntered);
void printNumTrailingZeros(const std::vector<int>* numbersEntered);
int findTrailingZeros(int numberToCalculateZeros);

int main() {
    std::ios::sync_with_stdio(false);
    std::vector<int>* numbers;
    numbers = createVector(requestNumInts());
    loadVector(numbers);
    printNumTrailingZeros(numbers);
    delete numbers;
    return 0;
}

int requestNumInts() {
    int totalNumInts = 0;
    while(!validateInputSize(totalNumInts))
    {
        std::cin >> totalNumInts;
    }
    return totalNumInts;
}

bool validateInputSize(const int totalNumInts) {
    const int MAX_NUMBER_OF_INPUTS = 1000000000;
    const int MIN_NUMBER_OF_INPUTS = 1;
    if(totalNumInts >= MIN_NUMBER_OF_INPUTS && totalNumInts <= MAX_NUMBER_OF_INPUTS) {
        return true;
    }
    else {
        return false;
    }
}

std::vector<int>* createVector(int totalNumInts) {
    std::vector<int>* numbers = new std::vector<int>(totalNumInts);
    return numbers;
}

void loadVector(std::vector<int>* numbersEntered) {
    for(unsigned int count = 0; count < numbersEntered->size(); ++count) {
        std::cin >> (*numbersEntered)[count];
    }
}

void printNumTrailingZeros(const std::vector<int>* numbersEntered) {
    for(unsigned int count = 0; count < numbersEntered->size(); ++count) {
        std::cout << findTrailingZeros((*numbersEntered)[count]) << std::endl;
    }
}

int findTrailingZeros(int numberToCalculateZeros) {
    int totalTrailingZeros = 0;
    const int FACTOR = 5;
    while(numberToCalculateZeros >= FACTOR) {
        numberToCalculateZeros /= FACTOR;
        totalTrailingZeros += numberToCalculateZeros;
    }
    return totalTrailingZeros;
}

Code Review

int requestNumInts();
bool validateInputSize(const int totalNumInts);
std::vector<int>* createVector(int totalNumInputs);
void loadVector(std::vector<int>* numbersEntered);
void printNumTrailingZeros(const std::vector<int>* numbersEntered);
int findTrailingZeros(int numberToCalculateZeros);

Personally I like to align the function names (this makes it easier to read). This is purely personal. Some like it some don't.

int               requestNumInts();
bool              validateInputSize(const int totalNumInts);
std::vector<int>* createVector(int totalNumInputs);
void              loadVector(std::vector<int>* numbersEntered);
void              printNumTrailingZeros(const std::vector<int>* numbersEntered);
int               findTrailingZeros(int numberToCalculateZeros);

Now that I have lined it up two things sprint to mind.

You are returning a vector by pointer (that's not good as there is no ownership semantics (who deletes it)). You should probably return by value. The optimizer will remove any copying and it prevents memory leaks. You can then pass the vector by reference to prevent copying in other situations.
You seem to be writing C code. If you implements this inside an object then a lot of you parameters don't need to be passed they are part of the object that is being manipulated.

Nice:

    std::ios::sync_with_stdio(false);

Pointer. Boo. Bad.

    std::vector<int>* numbers;

It is rare to see RAW pointers in C++ code. Pointers are usually wrapped inside smart pointers. But in this case you don't even need a pointer just use a normal std::vector as an object in place.

    std::vector<int> numbers = createVector(requestNumInts());

Using a delete is risky.

    delete numbers;

It is hard to tell if numbers was dynamically allocated! You actually have to go and look that up in the function createVector(). So if you change createVector() you also need to go through your code and find every place that calls createVector() to make sure they also use it correctly. Also its not exception safe. If an exception propagates through your code then you leak memory.

The main() function is special. If you don't specify a return then the compiler generates a return 0; for you. If your code can do nothing else apart from exit successfully then leave the return 0; out to indicate that there are no failure states. If there are error exit states then return 0; is an indication that the reader of the code should look for exit failures attempts.

Here:

int requestNumInts() {
    int totalNumInts = 0;
    while(!validateInputSize(totalNumInts))
    {
        std::cin >> totalNumInts;
    }
    return totalNumInts;
}

The first attempt will always fail. So why not use a do {} while() loop. This is designed for this situation. You always execute the code before doing the test.

Avoid if conditions that return true/false.

bool validateInputSize(const int totalNumInts) {
    const int MAX_NUMBER_OF_INPUTS = 1000000000;
    const int MIN_NUMBER_OF_INPUTS = 1;
    if(totalNumInts >= MIN_NUMBER_OF_INPUTS && totalNumInts <= MAX_NUMBER_OF_INPUTS) {
        return true;
    }
    else {
        return false;
    }
}

The above can be written as:
Much more readable.

    return (totalNumInts >= MIN_NUMBER_OF_INPUTS)
        && (totalNumInts <= MAX_NUMBER_OF_INPUTS);

Don't create the vector with new.

std::vector<int>* createVector(int totalNumInts) {
    std::vector<int>* numbers = new std::vector<int>(totalNumInts);
    return numbers;
}

RVO and NRVO will remove the copy that happens when you return by value. Also with C++11 and move semantics this makes this even more efficient. So never do this.

Also this whole function can be replaced with just a simple declaration in main.
Much simpler and more readable.

std::vector<int>   numbers(requestNumInts());

Good try:

void loadVector(std::vector<int>* numbersEntered) {
    for(unsigned int count = 0; count < numbersEntered->size(); ++count) {
        std::cin >> (*numbersEntered)[count];
    }
}

Couple of different ways to do this:

// Use the new foreach keyword
for(auto& val: numbers)
{
    std::cin >> val;
}

// Using iterators.
for(auto loop = numbers.begin(); loop != numbers.end(); ++loop)
{
    std::cint >>  (*loop);
}

// Or we can use the old classic C++03 std::transform
std::transform(std::begin(numbers), std::end(numbers),
               std::istream_iterator<int>(std::cin),
               std::begin(numbers),
               [](int& /*val1*/, int& val2){ return val2;});

Again nice effort with the printing.

void printNumTrailingZeros(const std::vector<int>* numbersEntered) {
    for(unsigned int count = 0; count < numbersEntered->size(); ++count) {
        std::cout << findTrailingZeros((*numbersEntered)[count]) << std::endl;
    }
}

Again some other options:

// Use the new foreach keyword
for(auto& val: numbers)
{
    std::cout << findTrailingZeros(val) << "\n";
}

// Using iterators
for(auto loop = numbers.begin(); loop != numbers.end(); ++loop)
{
    std::cout << findTrailingZeros(*loop) << "\n";
}

// Or we can use the old classic C++03 std::for_each
std::for_each(std::begin(numbers), std::end(numbers),
              [](int val){ std::cout << findTrailingZeros(val) << "\n";});

Also note the use of "\n" rather than std::endl. The std::endl adds a "\n" to the stream but then also calls flush. This is hardly ever what you actually want to do. Let the stream flush itself it makes it much more efficient to do so.

In your loops get used to use iterators to loop over containers. They are much more versatile and apply to all containers. Also you can pass them to functions very easily and they allow you to specify sub-ranges very trivially (event the foreach uses iterators underneath).

How I would do it.

First note that the output does not depend on previous values. You could cache them for speedy look-up but that seems overkill for such a simple algorithm. So there is no need to store the data in a vector.

 #include <iostream>

 int main()
 {
     std::ios::sync_with_stdio(false);

     int count;
     std::cin >> count;

     for(int loop=0; loop < count; ++count)
     {
          int value;
          std::cin >> value;

          std::cout << LeadingZero(value) << "\n";
     }
 }


Dictionary populated on demand I work with an embedded target that contains a list of commands and registers that may change from one device to another. So I need to build my commands list from metadata received from that particular target.

Unfortunately, the communication with the target is slow and the amount of commands is high. So I cannot build the full command's list at the beginning, but only when somebody requests it.

Of course, the list of commands should not be modifiable (by mistake), by the user which I assume is dumb. So I would like to prevent the instance to be easily modifiable in IPython.

For example:

device = Device()
device.commands.<tab>               # the user see all the available commands
device.commands.execute = 'oops'    # I want to prevent this to happen
device.commands['execute'] = 'oops' # Neither this.

So I wrote this design pattern:

import time

# The two following functions are the ones I use to communicate
# with my target.
def get_item_names():
    return ['item%d' % i for i in range(100000)]

def fetch_item(item): # Time consuming function
    time.sleep(0.1)
    return item

# The dictionary that is populated on demand.
class Foo(dict):
    __locked = False
    def __init__(self):
        # Populate elements with None elements.
        # This is required to be able to see the available commands
        # in IPython with the auto-completion turned on.
        for item in get_item_names():
            default = None
            self[item] = default

        # This is a very ugly workaround. With cProfiler, I can see
        # the addition of __setattr__ is very costy.
        self.__locked = True

    # The item is fetched only on demand
    def __getitem__(self, item):
        if item in self and super(Foo, self).__getitem__(item) == None:
            fetched = fetch_item(item)
            super(Foo, self).__setitem__(item, fetched)
            return fetched
        else:
            return super(Foo, self).__getitem__(item)

    @property
    def __members__(self):
        return self.keys()

    def __getattr__(self, item):
        return self[item]

    def __setattr__(self, key, value):
        if self.__locked:
            print('Not possible')
        else:
            super(Foo, self).__setattr__(key, value)

    def __setitem__(self, key, value):
        if self.__locked:
            print('Not possible')
        else:
            super(Foo, self).__setitem__(key, value)

Is that pattern good and what can I do to improve the bad performances with the lock mechanism? I naively tried to add setattr(self, '__setattr__', newsetattr) to __init__ but it does not work.

__missing__
I would have started to propose a simpler design using the __missing__ dunder so that, each time a user tries to access a command that has not been populated, the __missing__ method would be called and you could fetch the informations:
def Foo(dict):
    def __missing__(self, key):
        self[key] = item = fetch_item(key)
        return item

And voilà. I wouldn't try to enforce barriers to avoid users behaviour. After all, we're all consenting adults here.
Playing it nice with the interactive shell
This behaviour, however let the user clueless about all the possible commands available at the tap of the TAB key. They are still able to call get_item_names if need be. But you could automatically build them using some metaclass logic.
The class you want in the end could be:
def Foo:
    _<some_name> = None

    @property
    def <some_name>(self):
        if self._<some_name> is None:
            self._<some_name> = fetch_item(<some_name>)
        return self._<some_name>

    def __getitem__(self, key):
        return getattr(self, key)

with every <some_name> being returned by get_item_names. Read-only of the attributes are ensured by the property.
So you need to define your base class being:
class Foo(metaclass=CommandBuilder):
    def __getitem__(self, key):
        return getattr(self, key)

And build your metaclass like
class CommandBuilder(type):
    def __new__(cls, name, bases, dct):
        for command_name in get_item_names():
            _command_name = '_{}'.format(command_name)
            dct[_command_name] = None

            def getter(self):
                command_value = getattr(self, _command_name)
                if command_value is None:
                    command_value = fetch_item(command_name)
                    setattr(self, _command_name, command_value)
                return command_value

            dct[command_name] = property(getter)

        return super(CommandBuilder, cls).__new__(cls, name, bases, dct)

If you trully need to use a subclass of dict here, it wouldn't be much more difficult:
def Foo(dict, metaclass=CommandBuilder):
    def __missing__(self, key):
        item = getattr(self, key)  # will call the property
        super(Foo, self).__setitem__(key, item)
        return item

    def __setitem__(self, key, value):
         pass  # or raise an exception of your own

    def __delitem__(self, key):
        pass  # or raise an exception of your own

Immutability of the attributes is still guarantied by property.


Improvements to a ViewModelBase Every time I go to write an MVVM application (say every 4-6 months) I rewrite my ViewModelBase class. This is for a range of reasons but let's say either I don't have access to prior code, the previous code was built for a client and isn't my code, or I'm working on someone else's machine and still don't have the code.

So I rewrite the basic INotifyPropertyChanged implementation, and I think it's a little different each time. Here's what I've come up with today:

public class ViewModelBase : INotifyPropertyChanged
{
    protected void OnPropertyChanged<T>(Expression<Func<T>> expression)
    {
        var property = (MemberExpression)expression.Body;
        this.OnPropertyChanged(property.Member.Name);
    }

    protected void OnPropertyChanged(string name)
    {
        PropertyChangedEventHandler handler = PropertyChanged;
        if (handler != null)
        {
            handler(this, new PropertyChangedEventArgs(name));
        }
    }

    public event PropertyChangedEventHandler PropertyChanged;
}

I would like to get a nice implementation that is simple and small, and any improvements to this (what other functionality is required in a ViewModelBase?) would be great.

I've used the below for a while:

public abstract class PropertyChangedBase : INotifyPropertyChanged
{
    public event PropertyChangedEventHandler PropertyChanged;

    protected void RaisePropertyChanged(string propertyName)
    {
        var propertyChanged = this.PropertyChanged;

        if (propertyChanged != null)
        {
            propertyChanged(this, new PropertyChangedEventArgs(propertyName));
        }
    }

    protected bool SetProperty<T>(ref T backingField, T Value, Expression<Func<T>> propertyExpression)
    {
        var changed = !EqualityComparer<T>.Default.Equals(backingField, Value);

        if (changed)
        {
            backingField = Value;
            this.RaisePropertyChanged(ExtractPropertyName(propertyExpression));
        }

        return changed;
    }

    private static string ExtractPropertyName<T>(Expression<Func<T>> propertyExpression)
    {
        var memberExp = propertyExpression.Body as MemberExpression;

        if (memberExp == null)
        {
            throw new ArgumentException("Expression must be a MemberExpression.", "propertyExpression");
        }

        return memberExp.Member.Name;
    }
}

I can write my properties like this:

    private int id;

    public int Id
    {
        get
        {
            return this.id;
        }

        set
        {
            this.SetProperty(ref this.id, value, () => this.Id);
        }
    }