My binary search algorithm implementation in JavaScript I have implemented binary search algorithm in JavaScript:

var myArray = [],
    searchNum = undefined;

// below function is used to capture the
// commandline parameters for array and the
// number to be searched
(function(){
    process.argv.forEach(function (val, index, array) {
        var idx = 0, ar = undefined;

        try{

            // get the commandline argument for
            // array values
            if(index === 2){
                myArray = myArray.concat(val.split(",").map(function(num){
                    return parseInt(num);
                }));
            }

            // third index is the number to be searched.
            if(index === 3){
                searchNum = parseInt(val)
            }

        }catch(e){
            console.log(e)
        }

    });
})();

console.log(" SEARCH NUMBER ",searchNum," in array ",myArray);

console.log(binary_search(myArray,searchNum,0,myArray.length));

function binary_search(numberArray, numberToSearch, lowIndex, maxIndex){
    var totalLength = maxIndex - lowIndex;

    var midIndex = parseInt(totalLength/2),
        str = "";

    /*
        If Lower Index is equal to Higher Index,
        that means number is not found and hence there is
        a collision in pointers and hence return
        as 'Can't be found'
    */
    if(lowIndex === maxIndex){
        return "can't be found";
    }

    /*
        setting the actual middle index
        by adding the computed middle index with lower index.
    */
    midIndex = lowIndex + midIndex;

    // if number found
    if(numberArray[midIndex] === numberToSearch){

        str = " Number "+numberToSearch+" found at position "+midIndex;

        return str;

    // if number in middle index is less than the number to be searched
    // set the lower Index to new value i.e. a index position next higher to
    // middle Index
    }else if(numberArray[midIndex] < numberToSearch){

        lowIndex = midIndex + 1;

    // number to be searched is less than the number present at middle Index
    // set new maxIndex value i.e. index which is previous position to the
    // middle index
    }else if(numberArray[midIndex] > numberToSearch){

        maxIndex = midIndex;

    }else{

        return "can't be found";

    }

    return binary_search(numberArray, numberToSearch, lowIndex, maxIndex);
} // end of method binary_search

When I run the above code the output is as follows,

E:\RahulShivsharan\MyPractise\DesignPatternsInJavaScript>node binarySearch.js 12,34,56,78,90 24
 SEARCH NUMBER  24  in array  [ 12, 34, 56, 78, 90 ]
can't be found

E:\RahulShivsharan\MyPractise\DesignPatternsInJavaScript>node binarySearch.js 12,34,56,78,90 34
 SEARCH NUMBER  34  in array  [ 12, 34, 56, 78, 90 ]
 Number 34 found at position 1

Can you please review my code and please suggest me if there is any room for
further improvement.

Parsing command line arguments

I mentioned this in a previous review,
it seems I wasn't specific enough,
but this is an inefficient and unnatural way to parse command line arguments:

    process.argv.forEach(function (val, index, array) {
        var idx = 0, ar = undefined;

        try{

            // get the commandline argument for
            // array values
            if(index === 2){
                myArray = myArray.concat(val.split(",").map(function(num){
                    return parseInt(num);
                }));
            }

            // third index is the number to be searched.
            if(index === 3){
                searchNum = parseInt(val)
            }

        }catch(e){
            console.log(e)
        }

    });

What's wrong with it?

For each index 0, 1, 2, 3, the loop compares the index against 2 and 3 repeatedly
If there are not enough arguments, the loop should not even begin
The command line parsing logic fails its purpose by not failing in case the arguments are invalid

The natural way to parse would be:

Check process.argv.length and fail if invalid
Check the type of each argument and fail if invalid

Consider this alternative, no looping, nice and simple:

function parseArgs(prog, argv) {
  if (argv.length != 2) {
    throw `usage: node ${prog} NUMS_CSV NUM`;
  }

  function validInt(s) {
    var num = parseInt(s, 10);
    if (isNaN(num)) {
      throw "Not a valid number: " + s;
    }
    return num;
  }

  return {
    nums: argv[0].split(',').map(validInt),
    target: validInt(argv[1])
  };
}

var args = parseArgs(process.argv[1], process.argv.slice(2));

Parsing integers

When using parseInt, it's recommended to specify the radix parameter, because although base 10 is a common default, it's not guaranteed across different implementations.
So to parse base 10 numbers, write parseInt(x, 10) instead of parseInt(x).

API design

The binary_search function searches for an element and returns two kinds of strings as result:

can't be found
Number N found at position X

This is very limiting.
It would be better to have binary_search return an index or -1 if not found,
and move the logic of formatting a string result into a dedicated function.


JSON photo data parsing So this original piece of code:

const imgUrls = {};

function onXhrLoad() {
  const json = JSON.parse(this.response);
  const photos = json.photos ? json.photos.photo : [json.photo];
  for (const {id, url_o} of photos) {
    imgUrls[id] = url_o;
  }
}

The original code assumed that the biggest available url to an image in the object "photos" is "url_o", but sometimes that size is not available so I wanted to change the "url_o" in the last 2 lines to the biggest size available in "photos" using the array size (array "size" in the modified code is the order of sizes from biggest to smallest).

so I modified the code as follows:

const imgUrls = {};
const sizes = ['url_o', 'url_6k', 'url_5k', 'url_4k', 'url_3k', 'url_k', 'url_h', 'url_l'];

function onXhrLoad() {
  const json = JSON.parse(this.response);
  const photos = json.photos ? json.photos.photo : [json.photo];
  for (var p of photos) {
    for (var s of sizes) {
        if (s in p) {
            var url = p[s];
            break;
        }
    }
    const id = p.id;
    imgUrls[id] = url;
  }
}

As requested in a comment, there is an example response at this link.
In this response there is no url_o, the biggest is url_6k

My question is if there is a better way to change the "url_o" than going through two for loops and an if condition.

find the first "biggest" url size name. A more convenient way is using Array.prototype.find() method:
p.id can be used directly as a key for object imgUrls - instead of generating constant const id = p.id; for each p(photo)

const imgUrls = {};
const sizes = ['url_o', 'url_6k', 'url_5k', 'url_4k', 'url_3k', 'url_k', 'url_h', 'url_l'];

function onXhrLoad() {
  const json = JSON.parse(js);
  const photos = json.photos ? json.photos.photo : [json.photo];
  for (var p of photos) {
      imgUrls[p.id] = p[sizes.find((s) => p[s])];
  }
}


Parsing XML in C# .NET I am working on a way to parse data using xml.

The file that I am given contains lines that look like this:

George | Washington | Carver

or someone else can send me someting like  this

Carver | Washington | George

And so on...

No matter what the format is, whoever sends me the file will also send me rules on how to parse the file. In the first example, it's First Name | Middle Name | Last Name. And in the second example, it's Last Name | Middle Name | First Name

Instead of writing a special case for each possibility, I created an XML file to describe the meta data.

<file>
    <first>0</first>
    <middle>1</middle>
    <last>2</last>
</file>

For instance, in this case. The tag first corresponds to 0 indicating that first name occurs at the 0th position.

Intuitively, I thought about creating a dictionary, with the key set to be the tag, and the value to be the text. Like such...

    public static IDictionary<string, string> GetLookupTable(string xmlContents)
{
    XElement xmlElement = XElement.Parse(xmlContents);

    IDictionary<string, string> table = new Dictionary<string, string>();

    foreach (var element in xmlElement.Elements())
    {
        table.Add(element.Name.LocalName, element.Value);
    }

    return table;
}

However, I'm not really familiar with .NET implementation of things, which led me  to question some stuff.

Would it be better to just traverse XElement instead of creating a dictionary? I don't think this is a good idea since I believe that XElement traversal may invovle an unordered tree traversal to get what I need. Doing this for each property (I have more than just 3) would be very inefficient. I am just speculating here...
Is retrieval from dictionary constant time? I know that in Java HashMap has constant get. If that was the case for c# as well, then this would seem like a better route to go as I would just traverse once, and then be able to retrieve whatever I need in constant time.

You can use the order of XML nodes as field order:

<importformat>
    <firstname/>
    <middlename/>
    <lastname/>
</importformat>

and parse it like that:

public static string[] GetFieldsOrder(string xmlContents)
{
    return XElement.Parse(xmlContents)
        .Elements()
        .Select(element => element.Name.LocalName)
        .ToArray();
}


Diamond kata in JavaScript I'm just starting to learn JavaScript. I implemented the Diamond kata, and I would appreciate if anybody could give me some feedback. I'm particularly concerned about learning the most 'idiomatic' way to do things in JavaScript, style conventions, etc.

The diamond kata consists of the following problem statement:

Given a letter, print a diamond starting with ‘A’ with the supplied letter
at the widest point.

For example: print-diamond ‘C’ prints

  A
 B B
C   C
 B B
  A

The production code (test code in the Gist):

// Diamond Kata with a TDD approach. First attempt
'use strict';

module.exports.Diamond = Diamond;

function Letter(char, spaces) {
  this._char = char;
  this._spaces = spaces;
}
Letter.prototype.print = function print() {
  return this._char + (
    (this._spaces > 0)
      ? (Array(this._spaces + 1).join(' ') + this._char + '\n')
      : '\n'
  );
}

// Singleton list with all letters
var LETTERS = new function () {
  // Init all upper-case letters
  for (let char=65; char <= 90; char++) {
    let character = String.fromCharCode(char);
    this[character] = new Letter(character, (char - 65) + (char - 65 - 1));
  }
}();
LETTERS.__proto__.iterateLetters = function (character, callback) {
  var chars = Object.keys(this);
  var last = chars.indexOf(character);
  var pos = 0;
  var direction = 1;
  // Forwards
  while (pos >= 0) {
    callback(this[chars[pos]], pos, last);
    if (pos === last) {
      // Reached final character, return to 'A'
      direction = -1;
    }
    pos += 1 * direction;
  }
}

function Diamond (letter) {
  if (typeof(letter) !== 'string' && !(letter instanceof String)) {
    throw new Error('Letter must be an string');
  }
  this._letter = letter;
};
Diamond.prototype.process = function process() {
  var prefix = function (max, current) {
    return Array(1 + max - current).join(' ');
  }
  var result = '';
  LETTERS.iterateLetters(this._letter, function (letter, current, last) {
    result += prefix(last, current);
    result += letter.print();
  });
  return result;
}

This code is meant to execute at a "Node --harmony" environment, although except from the module exportation, this is just plain JavaScript.

A working demo is available here.

This is written using a TDD approach. The test code, as well as this same snippet, can be found in the Gist.

I think you are overthinking it.  Or maybe this is just not the right problem for TDD? This seems to be a typical interview / Project Euler (kind of) question. Something that doesn't require more testing than running it several times in console.

Why do I say this?--Tests are typically needed when the borderline conditions for the input are not trivially found and the algorithm being tested may take on multiple ways, not easily seen from its definition.  In this case we have a very clearly stated borderline condition for inputs (our inputs are characters A-Z) and the algorithm may only take on two paths (a base-case, when 'A' is given) and the "other" cases (given any letter other than 'A'). The algorithm isn't defined for any other input.

Below is a simple version of this problem:

function diamondKata(character) {
    var a = 'A'.charCodeAt(), frame, line,
        width = character.toUpperCase().charCodeAt() - a,
        i = 0, lines = [];

    function blanks(n) { return new Array(n).join(' '); }

    function makeLine() {
        return blanks(width + 1 - i) + frame +
            blanks(i * 2) + frame;
    }
    while (i++ < width) {
        frame = String.fromCharCode(a + i);
        lines.push(makeLine());
    }
    if (lines.length) {
        lines.unshift(blanks(width + 1) + 'A');
        lines = lines.concat(
            lines.slice(0, lines.length - 1).reverse());
    } else {
        lines.push('A');
    }
    return lines.join('\n');
}

Some points about your code: while operations with strings are generally very fast in JavaScript (highly optimized C code), it is still better to create long strings by first pushing all chunks into an array, and then to join it all in one go.  The reason for this is that creating strings by consequent concatenation will make a lot of intermediate memory allocations, which would be avoided if the string is created at once.

More on your code: type checking is frowned upon, unless its some library code, where the algorithm may take on multiple paths depending on what kind of object it works with.  When you check types only to throw a type error, you are working against the benefits you get from dynamically type-checked language: with dynamically type-checked languages you get these type errors for free (the environment already does that for you).

Compare this to C-like languages, where after dereferncing a pointer and making wrong assumption about the type of the object pointed by the derefernced pointer will spell disaster for the program, unless checked.

Hardcoding the codepoint assigned to characters would be a bad thing to do in any language, unless it's an optimization justified by a profiling session.


Counting density of Python code I created a tool to examine Python 3 source code. It measures the density of program. More info in docstring (to not reinvent the wheel).

What could be done to increase its usability? Any edge cases that I didn't think of?

#!/usr/bin/env python3

'''
Module counts how dense is Python source code in
terms of lexical entities per LoC.
'''

import ast

def traverse(node, line_nos):
    '''
    go through the AST and count lines
    '''
    children = list(ast.iter_child_nodes(node))

    try:
        current_line = node.lineno
    except AttributeError:
        current_line = 0

    count = 1
    line_nos.update([current_line])

    if len(children) == 0:
        return 1

    for child in ast.iter_child_nodes(node):
        count += traverse(child, line_nos)

    return count

def count_code_entities(source):
    '''
    parse the source&count
    '''
    line_nos = set()
    tree = ast.parse(source)

    # len - 1 to compensate for default value 0
    # which cannot hold any code
    return traverse(tree, line_nos), len(line_nos) - 1

def main():
    '''
    parsin&stuff
    '''
    from argparse import ArgumentParser, FileType
    parser = ArgumentParser()
    parser.add_argument('infile',
                        help='source file to analyze',
                        type=FileType(),
                        default='-',
                        nargs='?',)

    args = parser.parse_args()

    with args.infile as file:
        source = file.read()
        name = file.name

    count, number = count_code_entities(source)
    print(name, ': ', round(count/number, 4), 'entities/line of code')

if __name__ == '__main__':
    main()

The docstrings are vague. Documentation needs to be precise and complete. It should answer questions like "what arguments should I pass?" and "what does it return?" For example, "go through the AST and count lines" should become something like, "Return the number of nodes in the AST rooted at node, and update the set line_nos with the line numbers of these nodes, plus the line number 0 if any node has no line number."
It's not clear that the "plus the line number 0 if any node has no line number" behaviour is needed. Instead of doing this and then having to subtract one later to compensate, why not avoid adding the line number 0 in the first place?
When traversing an AST, it's often convenient to use the ast.NodeVisitor class, like this:

class DensityVisitor(ast.NodeVisitor):
    """AST visitor that accumulates the count of nodes and the set of line
    numbers in the AST.
    """
    def __init__(self):
        self.node_count = 0
        self.line_numbers = set()

    def visit(self, node):
        self.node_count += 1
        try:
            self.line_numbers.add(node.lineno)
        except AttributeError:
            pass
        self.generic_visit(node)

    @property
    def density(self):
        """The density of code (nodes per line) in the visited AST."""
        return self.node_count / len(self.line_numbers)

and then in main you'd write:

visitor = DensityVisitor()
visitor.visit(ast.parse(source))
print('density (nodes/line) =', round(visitor.density, 4)))

Update: Why would you prefer ast.NodeVisitor over explicitly walking the tree using ast.iter_child_nodes plus recursion? Well, because of separation of concerns — the logic for walking the tree is delegated to the ast.NodeVisitor base class, and you only have to specify what happens at each node. (Another possibility would be to use ast.walk.)


Conversion from decimal to negabinary and vice versa in Haskell Recently I set about to writing conversion functions in Haskell between decimal and negabinary. I tried to write them in as much functional a style as possible, also aiming at brevity. Below I present my take at the task. I'd be interested in your refinements of the method I employ or any comments whatsoever.

These are my two functions for converting back and forth:

type NegaBinary = String

dec2negbin :: Int -> NegaBinary
dec2negbin = reverse .
             map intToDigit .
                 unfoldr (\x -> if x==0 then Nothing else Just(abs(xrem x (-2)), xdiv x (-2)))
    where
      xrem a b = if b > 0 then rem a b else (-(rem a (-b)))
      xdiv a b = if b > 0 then div a b else (-(div a (-b)))

negbin2dec :: NegaBinary -> Int
negbin2dec = sum . map (\x -> (snd x)*(-2)^(fst x)) . zip [0..] . reverse . map digitToInt

I defined the NegaBinary type just for clarity. The dec2negbin employs the unfoldr function for sequential divisions and remainder calculations. I also used two local functions: xrem and xdiv, because the standard functions (rem and div) had the undesirable behavior of always approaching the dividend from below (meaning that in div a b => c b*c is always lower than a). What I wanted is for it to approach in the direction from zero, so the behavior is different for positive and negative dividends. I know the definition of dec2negbin is a bit bulky, so improvements are welcome.

A couple of things I noticed:

xrem and xdiv are exactly the same except they use a different division function. You could therefore replace them with a single function that takes the type of division as a parameter:

dec2negbin :: Int -> NegaBinary
dec2negbin = reverse .
             map intToDigit .
                 unfoldr (\x -> if x==0 then Nothing else Just(abs(doDiv rem x (-2)), doDiv div x (-2)))
    where
      doDiv division a b = if b > 0 then division a b else (-(division a (-b)))

In your second function you could use zipWith which applies a given function whilst zipping instead of doing a zip then a map:

negbin2dec :: NegaBinary -> Int
negbin2dec = sum . zipWith raise [0..] . reverse . map digitToInt
    where
        raise e x = x * (-2) ^ e


Change the switch code there is a other way to write in JS the switch statement ,
This code is working :)

_update: function(oHtml, oConfig) {
        var _this = this;
        var oParent;
        if (oConfig.attributes != null) {
            oConfig.attributes.forEach(function(oAttr) {
                //debugger;
                oParent = oHtml.find("#" + oAttr.id);
                switch (oAttr.action) {
                    case 'upd':
                        _this._updateAttr(oParent, oAttr);
                        break;
                    case 'new':
                        _this._insertAttr(oParent, oAttr);
                        break;
                    case 'del':
                        _this._removeAttr(oParent, oAttr);
                }
            });
            return oHtml;
        }
    },

We are using require JS

You can try something like this, saving the names of the functions in a variable, and then accessing the variable with the name of the action, you can also save the reference of the functions of the variable in the properties and you would not have to use 'this'

_update: function(oHtml, oConfig) {
    var _this = this;
    var oParent;
    var actionsToExecute = {
      upd : '_updateAttr',
      new : '_insertAttr',
      del : '_removeAttr'
    }
    if (oConfig.attributes != null) {
        oConfig.attributes.forEach(function(oAttr) {
            //debugger;
            oParent = oHtml.find("#" + oAttr.id);
            _this[actionsToExecute[oAttr.action]](oParent,oAttr)
        });
        return oHtml;
    }
},


Happy Birthday Program I'm trying to teach myself C. I've managed to make this program I call HappyToYou.c (inspired by this video):

#include <stdio.h>
//#include <stdlib.h> //malloc would need this.
#include <string.h>
#include <err.h>

void sing(char *start, char *end){
    printf("%s%s.\n", start, end);
}

int main(int argc, char *argv[]){
    if(argc!=3) errx(1, "Give a holiday and a name as arguments.");

    char *happy="Happy ";
    char start[strlen(happy)+strlen(argv[1])+1];
    strcpy(start, happy);
    strcat(start, argv[1]);
    char *to=" to you";

    char *dear=" dear, ";
    char person[strlen(dear)+strlen(argv[2])+1];
    strcpy(person, dear);
    strcat(person, argv[2]);

    sing(start, to);
    sing(start, to);
    sing(start, person);
    sing(start, to);

    return 0;
}

I think it is free from bugs (as far as I know). I'm pretty sure it can be trimmed down but I'm not sure how.

There're some bugs in the code. For example, if the user provides more inputs than needed, which the program could safely ignore (or warn about), it'll still exit with an error and an incorrect message.

The comparison should be updated to: if (argc < 3)

Although I think the first answer is ok, I think the readability of the code counts, and proper indentation makes a huge difference. You should develop this habit from the start and I'd find the following minor modifications slightly more preferable, IMHO:

#include <stdio.h>
#include <error.h>

int main(int argc, char *argv[]) {
    /* notice the logical operator update here; prevents bogus error
       message if user provides more than 2 inputs */
    if (argc < 3)
        errx(1, "Give a holiday and a name as arguments.");

    printf(
        "Happy %1$s to you.\n"
        "Happy %1$s to you.\n"
        "Happy %1$s, dear %2$s!\n"
        "Happy %1$s to you.\n",
        argv[1],
        argv[2]
    );

    return 0;
}

Also, I'd recommend that you get in the habit of writing user-visible messages using user-friendly terminology, especially when error conditions are detected.

For example, instead of saying "Give a holiday and name as arguments", which relies on programmer terminology (e.g. non-programmers are unlikely to know what "arguments" are), try to be user-friendly. Consider if the program instead said the following during the error and helped the user correct it:

Some inputs appear to be missing. Please enter the holiday and your name.
Usage: HappyToYou <holiday> <name>
Example: HappyToYou Birthday Bob

Few things are more frustrating than error messages that are uninformative or wrong because the former doesn't help you resolve the issue and the latter sends the user down the wrong path in trying to find a solution.


Displaying TimeSpan as largest interval (with units) - Part II [This is a follow-up question to: Displaying TimeSpan as largest interval (with units). The code listed here has been refactored since the original question was posed. Please note that the scope of this question is limited to the declaration and implementation of the listed method.]

The method listed below is used in a call center application to display an approximation of remaining time. For example, a call center telephone operator might inform a caller that they could perform their desired action after N time has elapsed, e.g., "Yes, that's right, you can order samples again in 2 days." Some arbitrary rules revolving around the interpretation of time are embedded within the method, e.g., an interval greater than 50 minutes is considered to be one hour; these business rules need to remain as is.

With that in mind, are there any suggestions on how this routine might be further enhanced and/or made more efficient?

public static string LargestIntervalWithUnits(TimeSpan interval)
{
    if (interval < TimeSpan.Zero)
    {
        throw new ArgumentOutOfRangeException("interval",
                                              "Negative Timespans are not supported.");
    }

    // Numeric literals defined per business rules
    const double MinimumHoursDeemedOneDay = 22.0;
    const double MinimumMinutesDeemedOneHour = 50.0;
    const double MinimumSecondsDeemedOneMinute = 40.0;
    const double MinimumMillisecondsDeemedOneSecond = 500.0;

    int timeValue;
    string timeUnits;

    if (interval.TotalHours > MinimumHoursDeemedOneDay)
    {
        timeValue = (int)Math.Ceiling(interval.TotalDays);
        timeUnits = "day";
    }
    else if (interval.TotalMinutes > MinimumMinutesDeemedOneHour)
    {
        timeValue = (int)Math.Ceiling(interval.TotalHours);
        timeUnits = "hour";
    }
    else if (interval.TotalSeconds > MinimumSecondsDeemedOneMinute)
    {
        timeValue = (int)Math.Ceiling(interval.TotalMinutes);
        timeUnits = "minute";
    }
    else if (interval.TotalMilliseconds > MinimumMillisecondsDeemedOneSecond)
    {
        timeValue = (int)Math.Ceiling(interval.TotalSeconds);
        timeUnits = "second";
    }
    else
    {
        return "now";
    }

    return string.Format("{0:#,##0} {1}{2}",
                         timeValue,
                         timeUnits,
                         (timeValue == 1 ? string.Empty : "s"));
}

Your code is looking good, but I find the control flow in this version a bit easier to follow:

// Defined per business rules.
private static readonly TimeSpan MinimumIntervalDeemedOneDay = TimeSpan.FromHours(22);
private static readonly TimeSpan MinimumIntervalDeemedOneHour = TimeSpan.FromMinutes(50);
private static readonly TimeSpan MinimumIntervalDeemedOneMinute = TimeSpan.FromSeconds(40);
private static readonly TimeSpan MinimumIntervalDeemedOneSecond = TimeSpan.FromMilliseconds(500);

public static string LargestIntervalWithUnits(TimeSpan interval)
{
    if (interval < TimeSpan.Zero)
    {
        throw new ArgumentOutOfRangeException("interval",
                                              "Negative Timespans are not supported.");
    }

    if (interval > MinimumIntervalDeemedOneDay)
    {
        return FormatTime(interval.TotalDays, "day");
    }

    if (interval > MinimumIntervalDeemedOneHour)
    {
        return FormatTime(interval.TotalHours, "hour");
    }

    if (interval > MinimumIntervalDeemedOneMinute)
    {
        return FormatTime(interval.TotalMinutes, "minute");
    }

    if (interval > MinimumIntervalDeemedOneSecond)
    {
        return FormatTime(interval.TotalSeconds, "second");
    }

    return "now";
}

private static string FormatTime(double value, string units)
{
    var ceiling = (int)Math.Ceiling(value);
    return string.Format("{0:#,##0} {1}{2}",
                         ceiling,
                         units,
                         (ceiling == 1 ? string.Empty : "s"));
}


Calculating the number of elements of an underlying data structure I read many comments on this question that one really shouldn't use local classes for functionality like which my code provides, but I couldn't really put things into perspective and decide what better alternatives are there, if any (in terms of solving the problem locally). Some mentioned lambdas, although they also pinpointed that with recursion problems can arise if it is declared with the auto keyword. I would like to keep the logic inside of this method. Is it clean code or not? What do you think? (please don't mention (int)size_t casting; it's personal)

The algorithm itself calculates the number of elements of the underlying data structure, excluding folders (AnyMap instances).

typedef std::map<std::string, boost::any> AnyMap;
using boost::any_cast;

class MyExtendedAnyMap
{
private:
    AnyMap data; // stores values and other AnyMaps as well that act as a folder basically

public:
    int size() const
    {
        if(data.empty())
            return 0;

        int calcedSize = 0;

        struct GoRec{
            static void go(const AnyMap& anyMap, int& calcedSize){
                for(const auto& kv : anyMap){
                    if(kv.second.type() == typeid(AnyMap)){
                        go(any_cast<const AnyMap&>(kv.second), calcedSize);
                    }
                    else{
                        calcedSize++;
                    }
                }
            }
        };

        for(const auto& kv : data){
            if(kv.second.type() == typeid(AnyMap)){
                GoRec::go(any_cast<const AnyMap&>(kv.second), calcedSize);
            }
            else{
                calcedSize++;
            }
        }

        return calcedSize;
    }

    // ...
}

Don't Repeat Yourself

Your looping code appears twice. Once in the body of size() and once in the body of GoRec::size(). You only need it once. In fact, the entire structure of GoRec is kind of weird and unnecessary. I also don't like how you named the function go(), made it void, and have it take a reference... Instead, let's write a free function:

size_t size_of(AnyMap const& data)
{
    size_t size = 0;
    for(const auto& kv : data){
        if(kv.second.type() == typeid(AnyMap)) {
            size += size_of(any_cast<const AnyMap&>(kv.second));
        }
        else{
            size++;
        }
    }
    return size;
}

That way:

int size() const {
    return size_of(data);
}

And we have no repetition and no unnecessary extra case for empty map.

Know Your Interface

There are several different overloads for boost::any_cast. You are using the wrong one. This is the one you want:

template<typename ValueType> const ValueType * any_cast(const any * operand);

This overload will not throw, and will return a null pointer if you're attempting to cast to the wrong type. Now, the overload you're using will throw if it's the wrong type - but you're explicitly checking that first. Effectively, you're re-implementing what any_cast is doing under the hood, which is bad.

If you used the other overload, you could let the any internal implementation remain private. And the code ends up looking more direct:

size_t size_of(AnyMap const& data)
{
    size_t size = 0;
    for(const auto& kv : data){
        if (auto submap = any_cast<AnyMap>(&kv.second)) {
            size += size_of(*submap);
        }
        else{
            size++;
        }
    }
    return size;
}


Creating an enumerator based on an ATL collection I am designing a COM wrapper for a C++ library using ATL. Currently I am a little bit confused about enumerators that are based on ATL collections. I already wrote several enumerators based on STL collections, but since there is no ICollectionOnATL-interface (such as the ICollectionOnSTL-interface), I am a little bit confused if my implementation is correct.

Here is how I am creating the enumerator at the moment:

STDMETHODIMP CCollectionOnATL::get_NewEnum(IEnumITEM** enumerator)
{
    // IItem implements IUnknown.
    typedef CInterfaceArray<IItem> Container;
    typedef CComEnum<IEnumITEM, &IID_IEnumITEM, IItem*, _CopyInterface<IItem>> CComEnumITEM;

    // Copy all elements to an array that can be enumerated.
    Container items;
    auto elements = m_coll.GetCount();
    //items.SetCount(elements);

    // See below for the definition of m_coll.
    POSITION pos = m_coll.GetHeadPosition();

    for (size_t element = 0; element < elements; element++)
    {
        auto current = m_coll.GetNext(pos);

        // Automatically calls AddRef!
        items.Add(current->m_value);
    }

    // Create an enumerator over the array.
    // Using `new` is valid here (since enumerators do not implement FinalConstruct).
    CComObject<CComEnumITEM>* pEnum = new CComObject<CComEnumITEM>();
    pEnum->Init(&items[0], &items[elements - 1], nullptr, ATL::AtlFlagTakeOwnership);

    // Return the enumerator.
    return pEnum->QueryInterface(enumerator);
}

The member m_coll is defined as CRBMap:

CRBMap<ULONG, IItem*> m_coll;

There are some points with this implementation, where I am not really sure if I understood them correctly:

Iterating the CRBMap should give me an ordered list, where the lowest key is the first and the highest key is the last element. However, I am not sure if there's a "better" (in terms of simplicity) way to perform the iteration.
Usage of CInterfaceArray to initialize the enumerator:

pEnum->Init(&items[0], &items[elements - 1], nullptr, ATL::AtlFlagTakeOwnership);

Is the initialization correct? Especially I am wondering about the third parameter, that references an IUnknown-instance to keep alive for as long as the enumerator lives. Currently I am passing nullptr, but I think the right value should be the items-array, which however does not implement IUnknown.
Regarding the enumerator initialization: Was my choose of ATL::AtlFlagTakeOwnership correct in here? I am not completely sure, because the actual ownership of the instances is taken by m_coll.
Since the size of the final array is known, I could initialize it (and therefor prevent relocations/copy-steps) using items.SetCount(elements);. However, the MSDN only states that CInterfaceArray::Add automatically calls AddRef. There's no information about SetAt or operator[]. Could I simply change the way I add items to the array to

items[element] = current->m_value;

without any change in the behaviour of reference counting?

Also if you have any suggestions on how I can further improve this code, please let me know!

Okay, I finally got this working. Here are my results after hours of trying to get this right:

Alternatively it is possible to use a while-loop, which feels somehow more natural, a little bit like a foreach loop:

POSITION pos = m_coll.GetStartPosition();

while (pos != nullptr)
{
    IItem* entry = m_coll.GetNextValue(pos);
    entry->get_Address(&address);
}

Speaking of foreach, I wrote a small macro that makes it possible to actually write it like a foreach-loop:

#define FOREACH(T, entry, collection) \
    POSITION pos = collection.GetHeadPosition(); \
    T entry; \
    while (pos != nullptr && (entry = collection.GetNextValue(pos)) != nullptr)

Now a loop can be written like this:

FOREACH(IItem*, item, m_coll)
{
    item->FooBar();
}

However this only works for CRBMaps, for CAtlMap you have to change the macro to use GetStartPosition() instead (Hooray consistent design!). For other collections this needs to be changed entirely. Also for initializing the enumerator is easier with the traditional for-based apprach, as shown below.
Turns out that the best way appears to be using a traditional old-fashioned array, because that's the fastest and simplest way to store data inside the enumerator. The initialization then receives a pointer to the first and a pointer to the first element after the last one of the array. The third parameter is used to keep the host of the enumerator (typically the creator/this) alive. If IUnknown is ambigouos here, use this->GetUnknown();.
The flag was chosen right in this scenario, because the array storing a snapshot of entries that can be iterated is out of control after the enumerator has been created and therefor also needs to released by the enumerator.
That's another plus for an traditional array.

Here's the complete code:

STDMETHODIMP CCollectionOnATL::get_NewEnum(IEnumITEM** enumerator)
{
    // Define the type of the enumerator.
    typedef CComEnum<IEnumITEM, &IID_IEnumITEM, IItem*, _CopyInterface<IItem>> IItemEnumerator;

    // Copy all elements to an array that can be iterated.
    size_t elements = m_coll.GetCount();
    IItem** items = new IItem*[elements];
    POSITION pos = m_coll.GetHeadPosition();

    for (size_t element = 0; element < elements; element++)
    {
        IItem* item = m_coll.GetNextValue(pos);

        // Add a reference of the current item to the enumeration.
        item->AddRef();
        items[element] = item;
    }

    // Create an enumerator for the array.
    CComObject<IItemEnumerator>* pEnum = new CComObject<IItemEnumerator>();
    HRESULT hr;

    if (FAILED(hr = pEnum->Init(&items[0], &items[elements], this->GetUnknown(), ATL::AtlFlagTakeOwnership)))
        return hr;

    return pEnum->QueryInterface(enumerator);
}


Banking application - Optimizing java multi-threading code I created a simple Bank application to make multiple transactions between multiple accounts. It is working as expected. But I want to know can I make the code more optimized.

public class BankingApplication {
    public static void main(String[] args) throws  Exception{
        ExecutorService service = Executors.newFixedThreadPool(100);
        Bank bankObj = new Bank();
        for(int i =0; i <= 10000; i++){
            service.submit(bankObj);
        }
        service.shutdown();
        service.awaitTermination(1, TimeUnit.DAYS);
        bankObj.getTotalBalanceWithStats();
    }
}
class Account{
    String accountName;
    int balance;
    Object lock1 = new Object();
    Object lock2 = new Object();
    Object lock3 = new Object();
    Account(String name, int balance){
        this.accountName = name;
        this.balance = balance;
    }
    void deposit(int amount){
        synchronized (lock1) {
            this.balance = getBalance() + amount;
        }
    }
    void withdraw(int amount) throws Exception {
        synchronized (lock2) {
            int balance = getBalance() - amount;
            if(balance <= 0){
                throw new Exception("Invalid Transaction");
            }
            this.balance = getBalance() - amount;
        }
    }
    int getBalance(){
        synchronized (lock3){
            return this.balance;
        }
    }
}
class Bank implements Runnable{

    List<Account> accountList = new ArrayList<>();
    Bank(){
        for(int i=0; i <= 30; i++){
            accountList.add(new Account("A"+i, 1000));
        }
        getTotalBalanceWithStats();
    }

    public void run() {
        Random random = new Random();
        Account from = null;
        Account to = null;
        while(true) {
            int a1 = random.nextInt(30);
            int a2 = random.nextInt(30);
            if(a1 == a2) continue;
            from = accountList.get(a1);
            to = accountList.get(a2);
            break;
        }
        doTransaction(from, to, random.nextInt(400));
    }

    private void doTransaction(Account from, Account to, int nextInt) {
        System.out.println("Transaction from " + from.accountName + " to " + to.accountName);
        try {
            from.withdraw(nextInt);
            to.deposit(nextInt);
        } catch (Exception e){
            System.out.println(e.getMessage());
        }
    }

    void getTotalBalanceWithStats(){
        int total = 0;
        for(Account acc : accountList){
            System.out.println("Account " + acc.accountName + " balance " + acc.getBalance());
            total += acc.getBalance();
        }
        System.out.println("Total Balance " + total);
    }
}

As per my knowledge I feel deposit(), withdrawal() and getBalanace() API are independent of each other so creating different mutex for each process.

Some scenarios which I am trying to test

What I am trying to achieve is when Account1 is trying to send money to Account2 at the same time Account3 can also send money to Account2 or vice versa.

When Account1 is sending money to Account2 at same time Account2 can also send money to Account1.

When Account1 is sending money to Account2 and at same time Account3 is sending money to Account1. withdraw() method of Account1 should not block deposit().

This code is broken.

Imagine an Account with $100, two threads, one depositing $1, the other withdrawing $50.

Thread one acquired lock1, and begins executing:

this.balance = getBalance() + amount;

and reads $100 ...

Thread two acquired lock2, reads the balance (still $100), subtracts $50, stores the new balance ($50) and releases lock2.

... (back to thread one) ... adds $1, for a total of $101, which it stores in balance.

Free money!  The withdrawal of $50 was forgotten.

You must lock all read-modify-write operations of an object's member with the same lock.


Color The Fence codeforces - TLE I am trying to solve this problem:

  Igor has fallen in love with Tanya. Now Igor wants to show his
  feelings and write a number on the fence opposite to Tanya's house.
  Igor thinks that the larger the number is, the more chance to win
  Tanya's heart he has.

  Unfortunately, Igor could only get v liters of paint. He did the math
  and concluded that digit d requires \$a_d\$ liters of paint. Besides, Igor
  heard that Tanya doesn't like zeroes. That's why Igor won't use them
  in his number.

  Help Igor find the maximum number he can write on the fence.

  Input The first line contains a positive integer v (0 ≤ v ≤ \$10^6\$). The
  second line contains nine positive integers \$a_1\$, \$a_2\$, ..., \$a_9\$
  (1 ≤ \$a_i\$ ≤ \$10^5\$).

  Output Print the maximum number Igor can write on the fence. If he has
  too little paint for any digit (so, he cannot write anything), print
  -1.

https://codeforces.com/problemset/problem/349/B

My solution that works correctly, but times out:

from collections import defaultdict

v = int(input())
a = [int(i) for i in input().split()]
cache = {}

def dfs(v):
    if v in cache:
        return cache[v]
    res = 0
    for i, num in enumerate(a):
        if num == v:
            res = max(res, i+1)
        if num < v:
            x = dfs(v-num)
            if x > 0:
                s = str(i+1) + str(x)
            else:
                s = str(i+1)
            sorted_nums = sorted([str(i) for i in s], reverse=True)
            l = int("".join(sorted_nums))
            res = max(res, l)
    cache[v] = res
    return res

ans = dfs(v)
if ans == 0:
    print(-1)
else:
    print(ans)

How do I optimize it in terms of time complexity?

There is no need to comment your code. It is totally broken. Very first thing you need to do is to calculate your complexity. ACM is not about code quality, it is not about language. It is about algorithms. You can write the ugliest code ever but can it be executed? Not a problem if you have satisfying algorithm complexity.

You can not just write different code without thinking about the algorithm and how your changes influence complexity. Moreover, you have to write code that will follow your algorithm.

General strategies for optimizing code for programming challenges? What? If there where such strategies no one would be challenging anymore. That is why it is so difficult and those competitions exist even now.

There are classes of problems that can be solved effectively with specific algorithm\approach. Competition task itself never describes what class or problems it is and what algorithm to use, you have to find it out by yourself. Determine the class of the problem you are trying to solve and then choose an algorithm that will satisfy the problem's memory/time requirements.

First of all. The problem you are trying to solve can be recognized as "implementation" when it didn't relate to any specific class directly. You have to do what it says. Implementation class requires you to be flexible minded, especially in this case because you have time requirements that brute-force didn't meet.

Algorithm

We have a set of digits with their costs. How to construct the largest possible number from it? We have to research the relation between numbers order and digits they have.

$$
A > B =
\begin{cases}
  true  & \text{if } dc(A) > dc(B) & \text{(1)}
  \\
  true  & \text{if } dc(A) = dc(B) \text{ and } \\
        & \text{if } h(A) > h(B) \text{ or } h(A) = h(B) \text{ and } t(A) > t(B) & \text{(2)}
  \\
  false & \text{else}
\end{cases}
$$

, where
\$ X = \left( x_1, x_2, \dots, x_n \right) \$ - number or vector of \$ n \$ digits
\$ dc(X) = |X| = n \$ - is number of digits in number (digits count)
\$ h(X) = x_1 \$ - returns digit of the highest order (head), e.g. \$ h(123) = 1 \$
\$ t(X) = X' = \left( x_2, \dots, x_n \right) \$ - returns number without the highest order digit (tail), e.g. \$ t(123) = 23 \$

Let's look at those rules and figure out rules that will give us the biggest possible number using the given set of digits.

\$ \text{(1)} \$ consequence: more digits you have - better.  That means we have to be greedy and take as much as possible digits with the lowest price.

\$ \text{(2)} \$ consequence: is it obvious that some times we will have enough amount of paint to replace digit with the bigger one but not enough to add another one. Now we know that we have to replace digits starting from the number's head.

Now let's take a look at how much digits we will have in the worst case. It is around \$ 10^6 \$.

With such a number of characters to output, we can not output by a single character at a time because input\output operations took too much time. I hope there is no need to prove that IO takes a long time.

Let's consider using python's big numbers. We will not look at their implementation, just pretend that we have \$ 10^6 \$ digits to write and we know exactly what is it.

start = time.time()
num = 1;
for _ in range(10**5):
    num = num * 10 + 1
finish = time.time()
print (finish - start)

Takes 4 seconds to complete and it is still \$ 10^5 \$. No, we can not use python's big integers.

We can not build a string because it is an immutable object and we will copy it every time on change, same as with big integers.

More or less acceptable will be to make a list with the required amount of digits in it and then use str.join(). Let's stick with this approach for a while and continue research.

So, first, we will find the highest digit with the lowest price \$ d \$, determine the length of the resulting number \$ n = \lfloor \frac{v}{\text{lowest cost}} \rfloor \$ and make a resulting list of \$ d \$ repeated \$ n \$ times.

Now, while we have some paint to spare, we will loop through digits from the highest to lowest and try to replace as many head digits as possible.

But yeah, now when we spell it, you can see that you do not actually need that list at all. You need to know how much digits to write and who it will be starting from the head of the result.

One can say: "hey, this is greedy problem class, not implementation" - yeah we are all smart now.

Implementation

Here is my implementation of described algorithm:

from copy import deepcopy

class Digit(object):
    def __init__(self, _cost, _value):
        self.cost = _cost
        self.value = _value

def main():
    v = int(input())
    a = list(map(int, input().split()))
    a = tuple(Digit(cost, digit+1) for digit, cost in enumerate(a))
    low_digit = deepcopy(min(a, key=lambda digit: digit.cost*10 - digit.value))

    if low_digit.cost > v:
        print(-1)
        return

    digits = v // low_digit.cost
    v -= digits * low_digit.cost
    for i in range(9):
        a[i].cost -= low_digit.cost
    low_digit.cost = 0

    for better in reversed(a):
        if better.cost > 0 and better.value > low_digit.value:
            count = v // better.cost
            v -= count * better.cost
            digits -= count
            print(str(better.value) * count, end='')
    print(str(low_digit.value) * digits)

if __name__ == "__main__":
    main()


Check if we find new value from a request I have created a script where I add a timestamp to each value that has been found with a code that I have written:
import random
from datetime import datetime, timedelta
from typing import Dict, List
import time

class RequestFilter:
    """Tracks requests and filters them to prevent hammering."""

    def __init__(self, cooldown: timedelta):
        self._cooldown = cooldown
        self._requests: Dict[str, datetime] = {}

    def filter(self, requests: List[str], time: datetime) -> List[str]:
        """Filter requests to only those that haven't been made
        previously within our defined cooldown period."""
        # Get filtered set of requests.
        filtered = [
            r for r in list(set(requests))
            if (
                    r not in self._requests or time - self._requests[r] >= self._cooldown
            )
        ]
        # Refresh timestamps for requests we're actually making.
        for r in filtered:
            self._requests[r] = time

        print(self._requests)
        return filtered

if __name__ == '__main__':
    from time import sleep
    request_filter = RequestFilter(timedelta(minutes=5))

    firstReq = []
    for _ in range(random.randint(1,5)):
        firstReq.append(f"US {random.randint(1, 10)}")

    for _ in range(100):

        newReq = []
        for _ in range(random.randint(2, 8)):
            newReq.append(f"US {random.randint(1, 10)}")

        if len(newReq) > len(firstReq):
            print(request_filter.filter(newReq, datetime.now()), datetime.now())
            sleep(1)
            firstReq = newReq
        else:
            print("Length is not bigger, testing again in 3 sec...")
            time.sleep(3)
            firstReq = newReq

As you can see at the very bottom i'm checking if the list size from previous request is less than newest request (at this momentit just random function but it would be reading from a HTML later on) and if it is, that means that something has been added to a webpage and we want to see what value has been added. If the value has already a timestamp then we check "filter" and see if it has been over 5 minutes difference in the timestamp and if its true then we should say "New value has been found!"
However my improvements in general here is that I am not quite happy with the way that I check for length of firstReq vs newReq. and reason if that could be etc if I request a page and it has US 3 and then the newReq has value US 6 but it still will have 1 > 1 which is false but still different value which will not print due to the 1 > 1. My question is, how can I improve the code that I could skip the < function and check maybe the sizes directly?

Global code
Move the code after your __main__ check into a function, since it's still in global scope.
PEP8
firstReq should be first_req, and the same for new_req.
Dict comprehension
I would rearrange your filtered comprehension to be only a dict, and to use dict.update. In other words,
filtered = {r: self._requests.get(r) for r in requests}

self._requests.update({
    r: time
    for r, orig_time in filtered.items()
    if orig_time is None or time - orig_time >= self._cooldown
})

List comprehension
    firstReq = []
    for _ in range(random.randint(1, 5)):
        firstReq.append(f"US {random.randint(1, 10)}")

can be
first_req = [
    f"US {random.randint(1, 10)}"
    for _ in range(random.randint(1, 5))
]

Diagnostic printing
    print(self._requests)

is out-of-place. Remove it, and if you still want this to happen, issue an equivalent statement from main(), perhaps printing the entire RequestFilter and overriding __str__ to return pprint.pformat(self._requests).
Change detection
Apparently what you actually want is to return a boolean that is true if self._requests has changed. This can be done like:
filtered = {r: self._requests.get(r) for r in requests}

old_requests = dict(self._requests)

self._requests.update({
    r: time
    for r, orig_time in filtered.items()
    if orig_time is None or time - orig_time >= self._cooldown
})

return old_requests == self._requests


My first Hangman game in Python This is my first game and I'm really proud of it, but I think it's horribly messy. Is there any way to improve it?

#HANGMAN

# Valid Word Checker

def valid_word(word):

    ill_chars = ["!","@","#","$","%","^","&","*","(",")",",",".", " ","1","2","3","4","5","6","7","8","9","0"]
    for i in word:
        if i in ill_chars:
            print(f"\nError: It must be a letter. No symbols, numbers or spaces allowed. \n\n '{i}' is not allowed\n")
            return False
    if len(word) > 12:
        print("\n\nError: Word is too long. Use a word with eight characters or less.")
        return False
    return True

word_list = []
spaces = []
guessed_letters = []

ill_chars = ["!","@","#","$","%","^","&","*","(",")",",",".", " ","1","2","3","4","5","6","7","8","9","0"]

head ="    O"
armL = "   /"
torso = "|"
armR = "\\"
legL = "/"
legR = " \\"

hangman_parts = [head, armL, torso, armR, legL, legR]
hangman_progress = ["",
                    "|",
                  "\n|",
                  "\n|"]

hangman_final = ["|~~~~|\n"]

# Check if word is valid

wordisValid = False
while True:
    word = input("\n\n\nChoose any word\n\n\n").lower()
    if valid_word(word) == True:
        wordisValid = True
        break
    else:
        continue

# Add to list
for i in word:

    word_list.append(i)
    spaces.append("_ ")

# Main Game Loop
bad_letter = 0

while wordisValid == True:

    print("".join(hangman_final))
    print("\n\n\n\n")
    print("".join(spaces))
    print("\n\nThe word has: " + str(len(word)) + " letters.")
    print("\nYou've tried the following letters: " + "\n\n" + "".join(guessed_letters) + "\n\n")

    # Winning Loop

    if "".join(spaces) == word:
        print(f"YOU WIN! The word was: {word}")
        break
    # Choose Letters

    player_guess = input("\n\nPlease choose a letter: \n\n\n\n").lower()
    guessed_letters.append(" " + player_guess)

    if player_guess in ill_chars:
        print(f"\nError: It must be a letter. No symbols, numbers or spaces allowed. \n\n '{player_guess}' is not allowed\n")
    elif len(player_guess) > 1:
        print("\nError: You must use one letter.\n")
    elif player_guess == "":
        print("\nError: No input provided.\n")

    # Wrong Letter
    elif player_guess not in word_list:

        bad_letter += 1

        if bad_letter == 1:
            hangman_final.append(hangman_progress[1] + head)

        elif bad_letter == 2:
            hangman_final.append(hangman_progress[2] + "    " + torso)

        elif bad_letter == 3:
            hangman_final.pop(2)
            hangman_final.append(hangman_progress[2] + armL + torso)

        elif bad_letter == 4:
            hangman_final.pop(2)
            hangman_final.append(hangman_progress[2] + armL + torso + armR)

        elif bad_letter == 5:
            hangman_final.append(hangman_progress[3] + "   " + legL)

        elif bad_letter == 6:
            hangman_final.pop(3)
            hangman_final.append(hangman_progress[3] + "   " + legL + legR)
            print("\n\nThe word was: " + word)
            print("\n\n\n\n\n\n\n" + "".join(hangman_final))
            print(" YOU GOT HUNG ")
            break

        print("".join(hangman_final))
        print("\n\n\n\n")
        print("".join(spaces))
        print("\n\nThe word has: " + str(len(word)) + " letters.")
        print("\nYou've tried the following letters: " + "\n\n" + "".join(guessed_letters) + "\n\n")
        print(f"\n\n\n{player_guess} is not in the word. Try again.\n\n")

    # END GAME
    if bad_letter == 6:
        break

    # Add letters guessed to a list
    counter = 0
    for i in word:

        if player_guess == i:
            spaces[counter] = player_guess
        counter += 1

Even though it's working, I'm not sure that I have the right idea when it comes to making this type of project.

While it's clear that you're new to python, it's still pretty good that you got it to run first time. Good job!

Input Validation

Currently, you have a list of forbidden characters. While that can work, by default python allows unicode input. That means there's literally thousands of letters someone can input. I suggest instead using a whitelist of valid inputs. Like this:

VALID_LETTERS = "abcdefghijklmnopqrstuvwxyz"  # This won't ever change, and typically this sort
# of things  is a module-level variable.

def valid_word(word):
    for letter in word:
        if letter not in VALID_LETTERS:
            print(f"Letter {letter} is not a valid letter. Please use just a-z.")
            return
    if len(word) > 8:  # This was 12 in your script. Little mistake? And you might want a
        # module-level constant for this as well.
        print("\n\nError: Word is too long. Use a word with eight characters or less.")
        return False
    return True

main() and game()

Generally, in python we never execute code at the moment the module is loaded. Read here for details, and how we guard against it. I'll come back to this later. For now, it means we should put as much as possible into functions - lets call them main() and game().

main() will be the main menu function. Since we don't have a true menu, this will basically start a game, and perhaps ask if the player wants to play again.

game() will be a function that lets us play a single game. Most of this will be a loop that gets player input and calculates the game state. Basically everything in your script that isn't another function should be in the game() function.

I'll cut some variables we don't need later on. I'll explain them when we get to where we use them.

Like this:

def game():
    # Variable naming is important. This is an example of a good name.
    guessed_letters = []

    # ill_chars = [...]  We already have this variable before here!

    # Lets assign the parts directly. We can index them as in "3rd part".
    # And for ourselves, we comment the meaning to make sure we still know what it means a year
    # from now.
    hangman_parts = [
        "    O",  # Head
        "   /",  # Left Arm
        "|" ,  # Torse
        "\\",  # Right Arm
        "/",  # Left Leg
        " \\"  # Right Leg
    ]
    hangman_progress = ["",
                        "|",
                      "\n|",
                      "\n|"]

    hangman_final = ["|~~~~|\n"]

    # Check if word is valid

    # Naming: should be word_is_valid, to be consistent with your other variables.
    word_is_valid = False
    while not word_is_valid:
        word = input("\n\n\nChoose any word\n\n\n").lower()
        word_is_valid = valid_word(word)
    # With this loop condition, it'll keep asking until valid_word returns True. So we can just
    # assign that value to word_is_valid directly.

    # Since this is the word we're trying to guess, lets just push it upwards in
    # the console a lot, so we won't see it all the time:
    print("\n" * 100)  # Prints 100 newlines.

    # We don't need word_list, just word will do. We don't need spaces either.
    while True:  # Main game loop. We don't need to check for anything, we'll just use break or
        # return to get out.

Now we're going to use a list comprehension. It's almost the same as a generator expression, but it gives us a true list, so we can ask python how long it is:

        # Calculate how many bad letters we have with a list comprehension:
        bad_letter = len([letter for letter in guessed_letters if letter not in word])
        draw_hangman(bad_letter)
        if bad_letter > 5:
            print("\n\n\n\nYOU GOT HUNG")
            return
        print("\n\n\n\n")

You used to draw your hangman at two different places, while it's more practical to do it in one place. It also ensures that if you want to change something, you'll only have to change it once.

I've also changed how to draw it. It's much more useful to have a function for this drawing operation, and have it calculate how to draw from the number of bad letters so far. This makes the drawing independent of the current game state. Here's the version I came up with, based on your modifications of hangman_final:

def draw_hangman(bad_letter):
    print(hangman_final, end="")
    if bad_letter > 0:  # Head
        print(hangman_progress[1] + hangman_parts[0], end="")
    if bad_letter == 2:  # Torso and arms
        print(hangman_progress[2] + "    " + hangman_parts[2], end="")
    elif bad_letter == 3:
        print(hangman_progress[2] + "".join(hangman_parts[1:3]), end="")
    elif bad_letter > 3:
        print(hangman_progress[2] + "".join(hangman_parts[1:4]), end="")
    if bad_letter > 4:  # Legs
        print(hangman_progress[3] + "   " + hangman_parts[4], end="")
    if bad_letter > 5:
        print(hangman_parts[5], end="")

Here we need to print the letters we guessed, and underscores otherwise. We can calculate and print it in one line:

        print(" ".join(letter for letter in word if letter in guessed_letters else "_"))

For every letter, this will print it if it's contained in guessed_letters, and otherwise print an underscore. This is a generator expression. This one is almost equal to the function:

def make_word(word, guessed_letters):
    result = []
    for letter in word:
        if letter in guessed_letters:
            result.append(letter)
        else:
            result.append("_")
    return result

I say almost, because it's not exactly a list that comes out of it, even if it acts the same if you use it in a for loop or feed it to a function that does, like "".join(). It also acts more like a loop than a function, but that's not really important right now.

We feed the result to the join. And note that we feed it to " ".join(), with a space in between, which will put a space between every letter like you did before.

        print(f"\n\nThe word has: {len(word)} letters.")  # f-strings are shorter, and easy to use.
        print(f"\nYou've tried the following letters:\n\n{''.join(guessed_letters) }\n\n")
        # For a string literal inside an f-string, use the other type of quotes

        # This is another generator expression - this one returns a not-quite-list of booleans, and
        # the all() builtin function returns True if and only if all results of the generator
        # expression are True.
        if all(letter in guessed_letters for letter in word):
            print(f"You WIN!\nThe word was: {word}")
            return

        input_valid = False
        while not input_valid:
            new_letter = input("\n\nPlease choose a letter: \n\n\n\n").lower()
            # VERY GOOD that you lowercase it! Prevents tons of weird behaviour.
            if len(new_letter) > 1:
                print("Please enter a single letter only.")
            elif new_letter not in VALID_LETTERS:
                print(f"{new_letter} is not a valid letter. Please enter a letter a-z")
            elif not new_letter:  # Empty strings evaluate to False
                print("Please enter a letter before pressing enter.")
            elif new_letter in guessed_letters:
                print("You already guessed {new_letter} before!")
            else:
                guessed_letters.append(new_letter):
                input_valid = True

This is pretty close to your input. But I put it in a loop, so if we have an invalid input, we don't have to traverse the entire game loop again. I use a pretty simple signalling boolean to keep it going, but perhaps there's more elegant ways to do this. Never wrong to keep it simple, though.

Right now, to play the game, all you have to do is call the game() function. But perhaps we want to play multiple games in series? Lets make our main() function:

def main():
    game()
    while "y" in input("Play again? [Y/n]").lower():
        game()

Why is this in a separate function? Again, for the sake of extensiblility. Perhaps, we want to import our game from another module. If so, we don't want it to run when we import, but when we call a function. So we end the script with this guard:

if __name__ == "__main__":
    main()

If we execute this file directly, this will run our main() function and let us play the game. But if we import it, it won't, so we can call the function when we want it.


Printing patterns in Haskell Write a function Int -> Char -> Char -> [String] that will create the appropriate pattern.
For example: mapM_ putStrLn (pattern 48 '.' '+')
................................................
.+..+..+..+..+..+..+..+..+..+..+..+..+..+..+..+.
+.++.++.++.++.++.++.++.++.++.++.++.++.++.++.++.+
................................................

This idea is inspired by Writing nested for loops to produce certain output (but more general).
I am pretty satisfied with my code to solve this problem:
pattern :: Int -> Char -> Char -> [String]
pattern len a b = map (take len . cycle) [[a], [a, b, a], [b, a, b], [a]]

main :: IO()
main = mapM_ putStrLn (pattern 48 '.' '+')

Seems fine. However, depending on the context and the rest of the program, you can relax pattern's type:

pattern :: Int -> a -> a -> [[a]]

Also, since every putStrLn is basically hPutStrLn stdout, you could first glue all lines together with unlines and then print them with a single action:

main = putStr (unlines (pattern 48 '.' '+'))


ListBox for a many-to-many relationship I'm looking for a good and easy to use solution for creating MultiSelectLists in MVC for many-to-many relationships.

I have this following example code and it works fine, but it just takes a lot of code.  It would be cool if it were shorter, smarter, or even made generic somehow, so that it's easy to create MultiSelectLists in future projects.

My Database using Entity Framework Code First:

Books, and authors, where one book, can have multiple authors.

public class DataContext : DbContext
{
    public DbSet<Book> Books { get; set; }
    public DbSet<Author> Authors { get; set; }
}
public class Book
{
    public int Id { get; set; }
    public string Name { get; set; }

    //Allows multiple authors for one book.
    public virtual ICollection<Author> Authors { get; set; }
}
public class Author
{
    public int Id { get; set; }
    public string Name { get; set; }

    [NotMapped]
    public int[] SelectedBooks { get; set; }
    public virtual ICollection<Book> Books { get; set; }
}

Controller

    public ActionResult Edit(int id = 0)
    {
        Author author = db.Authors.Find(id);
        if (author == null)
        {
            return HttpNotFound();
        }
        ViewData["BooksList"] = new MultiSelectList(db.Books, "Id", "Name", author.Books.Select(x => x.Id).ToArray());

        return View(author);
    }

    [HttpPost]
    public ActionResult Edit(Author author)
    {
        ViewData["BooksList"] = new MultiSelectList(db.Books, "Id", "Name", author.SelectedBooks);

        if (ModelState.IsValid)
        {
            //Update all the other values.
            Author edit = db.Authors.Find(author.Id);
            edit.Name = auther.Name;

        //------------
            //Make adding items possible
            if (edit.Books == null) edit.Books = new List<Book>();

            //Remove the old, add the new, instead of finding out what to remove, and what to add, and what to leave be.
            foreach (var item in edit.Books.ToList())
            {
                edit.Books.Remove(item);
            }
            foreach (var item in author.SelectedBooks)
            {
                edit.Books.Add(db.Books.Find(item));
            }
        //-------------
        // This is the code i want to simplify
        // Would be cool with a generic extention method like this:
        // db.ParseNewEntities(edit.Books, author.SelectedBooks);
        // I just don't know how to code it.

            db.SaveChanges();
            return RedirectToAction("Index");
        }
        return View(author);
    }

View

    @Html.ListBox("SelectedBooks",(MultiSelectList)ViewData["BooksList"])

Rather than using ViewData I might consider using ViewBag (if on MVC 3 and above) or even wrapping Author within a ViewModel.  That what you won't need the cast on the view (if using a viewModel).

Otherwise your MultiSelectList code is only one line.  Don't see you getting much better than that :)

Your viewmodel might look like

public class AuthorViewModel
{
   public Author BookAuthor { get; set; }
   public MultiSelectList BookList { get; set; }
}

[HttpGet]
public ActionResult Edit(int id = 0)
{
    Author author = db.Authors.Find(id);
    if (author == null)
    {
        return HttpNotFound();
    }

    var viewModel = new AuthorViewModel
    {
       BookAuthor = author;
       BookList = new MultiSelectList(db.Books, "Id", "Name", author.Books.Select(x => x.Id).ToArray());
    }

    return View(viewModel);
}

Your view will need to be typed to the view model and you could then write

Html.ListBox("SelectedBooks",Model.BookList);

I'm not 100% sure then if you need to re-bind it back on the post if there is a problem.  If you do, I would consider doing that after the Model.IsValid unless of course your "Index" action usings the ViewData["BookList"] value?

EDIT:
Ok, what about this then.

    internal class BookEqualityComparer : IEqualityComparer<Book>
    {
        public bool Equals(Book x, Book y)
        {
            return x.Id == y.Id;
        }

        public int GetHashCode(Book obj)
        {
            return obj.GetHashCode();
        }
    }

Then your code in the Post will be:

    // Take the values common to both lists based on the EqualityComparer
    edit.Books.Union(author.SelectedBooks, new BookEqualityComparer ());

And I would consider making the Books DbSet lazy loaded.

public class DataContext : DbContext
{
    private DbSet<Book> _books;
    public DbSet<Book> Books
    {
       get { return _books ?? (_book = new DbSet<Book>()); } // TBH not 100% sure you can set DbSet like this but worth a shot
       set { _books = value; }
    }

    public DbSet<Author> Authors { get; set; }
}


Checking whether one string contains all the characters of another string I am working on a CodeWars kata that requires the code to run under a certain time limit. This code passes all the tests; but, it doesn't run fast enough. I don't know what else I can trim down or change.

Addition: I tried looking up a String.split-join solution. But, that won't work. The split will replace every occurrence of a character. I need to check for only one at a time. I even tried using the split-join with RegEx that only took one occurrence of a character. But, that was just adding more code on top of what I already had.

Putting the strings into arrays is too slow.

Someone suggested that I try a different way of looking at the problem by rearranging the characters. Maybe I need to sort them which will group the duplicate characters together.

Kata description:

  If all the characters in the second string are found in the first string, then return true; otherwise, return false.

I included some test cases at the end.

function scramble(str1, str2) {

var str2Length = str2.length;
var str11 = "";

for(i = 0; i < str2Length; i++){
        str11 = str1.replace(str2[i], '');
        if(str11 === str1){
            return false;
        }
        str1 = str11;
}
// If all the characters have been found in the second string, then return true.
    return true;

}

scramble('rkqodlw','world');
console.log("should be true");

scramble('cedewaraaossoqqyt','codewars');
console.log("should be true");

scramble('katas','steak');
console.log("should be false");

scramble('scriptjava','javascript');
console.log("should be true");

scramble('scriptingjava','javascript');
console.log("should be true");

scramble('jscripts','javascript');
console.log("should be false");

scramble('aabbcamaomsccdd','commas');
console.log("should be true");

function scramble(world, seed) {
    var arr = new Array(256);
    var i = 0;

    for (i = 0; i < 256; i++) {
      arr[i] = 0;
    }

    for (i = 0; i < world.length; i++) {
      arr[world.charCodeAt(i)] += 1;
    }

    for (i = 0; i < seed.length; i++) {
      arr[seed.charCodeAt(i)] -= 1;
      if (arr[seed.charCodeAt(i)] < 0) {
        return false;
      }
    }

    return true;
}

console.log(scramble('rkqodlw','world'));
console.log("should be true");

console.log(scramble('cedewaraaossoqqyt','codewars'));
console.log("should be true");

console.log(scramble('katas','steak'));
console.log("should be false");

console.log(scramble('scriptjava','javascript'));
console.log("should be true");

console.log(scramble('scriptingjava','javascript'));
console.log("should be true");

console.log(scramble('jscripts','javascript'));
console.log("should be false");

console.log(scramble('aabbcamaomsccdd','commas'));
console.log("should be true");

Pretty sure it's much faster. As for why, it's about complexity. First a loop of 256 iterations (so nothing), then a loop O(n), then another O(n) loop, where n is the length of either string.

Your code is O(n*n): a loop on the string's length, then the .replace call which is also O(n).

Explanation of the code

Each letter is an ASCII character. ASCII characters have a number representation from 1 to 128 (in the code 256 to be safe).

So I create an array of 256 values, filling it with zeros. Then I count the letters in the first string: increasing the count for the corresponding letter by one. I get the number representation of the letter with .charCodeAt.

Then, for the second string, I decrease the count of each letter by 1 as I encounter the characters. If I end up with a negative number, it means there was more of said letter in the second string than in the first string, and I return false.

The end result is a much faster algorithm.

To be fair, anyone doing C or C++ would come up with this solution.

Another solution

Another solution, less efficient but still meeting the speed criterion I assume, using objects:

function scramble(world, seed) {
    var obj = {};
    var i = 0;

    for (i = 0; i < world.length; i++) {
      arr[world[i]] = (arr[world[i]] || 0) + 1;
    }

    for (i = 0; i < seed.length; i++) {
      arr[seed[i]] = (arr[seed[i]] || 0 ) - 1;
      if (arr[seed[i]] < 0) {
        return false;
      }
    }

    return true;
}

It's another way of doing things even if you don't know the concept of number representation for letters.


Project Euler 4: Largest palindrome product in Functional Programming I wanted to practice functional programming (fp) without using any library but using vanilla JS only. So I took the 4th problem from project euler:

  A palindromic number reads the same both ways. The largest palindrome
  made from the product of two 2-digit numbers is 9009 = 91 × 99.

  Find the largest palindrome made from the product of two 3-digit
  numbers.

My solution in fp looks like this:

/*jshint esversion: 6 */
(function () {
  'use strict';

  function* multiply(factor1, factor2) {
    let palindromeNumbers = 0;

    while (true) {
      const product = factor1 * factor2;
      if (isSymmetric(product) && palindromeNumbers < product) {
        palindromeNumbers = product;
      }
      if (factor1 <= 100) {
        factor1 = 999;
        factor2--;
      } else if (factor2 <= 100) {
        yield true;
        return palindromeNumbers;
      } else {
        factor1--;
      }
    }
  }
  const isEqual = (value, compare) => {
    if (value.length != compare.length) {
      return false;
    }
    if (value.length === 1 && value[0] === compare[0]) {
      return true;
    }
    return value[0] === compare[0] &&
      isEqual(value.slice(1), compare.slice(1));
  };
  const isSymmetric = n => {
    const asArray = n.toString()
      .split('');
    const mid = Math.floor(asArray.length / 2);

    const half1 = asArray.slice(0, mid);
    const half2 = asArray.slice(asArray.length - mid)
      .reverse();
    return isEqual(half1, half2);
  };

  const getAllPalindromeNumbers = multiply(999, 999);

  while (getAllPalindromeNumbers.next()
    .value !== true) {}
  const solution = getAllPalindromeNumbers.next()
    .value;
})();

First I wanted to solve this using recursion. But I reached the stack size pretty quickly. Therefore I opted for generators. But I'm not satisfied with my solution especially because of the multiply generator:

I'm am mutating palindromeNumbers, factor1, and factor2
I'm using a while loop twice

Is it possible to solve this problem yet still be consistent with fp, i.e. no mutations and no loops? And of course: Any other improvement suggestions are welcomed.

For the cross product I used this code snippet:
https://stackoverflow.com/a/36234242

Slow as hell but seems to be fp with vanilla JS only:

/*jshint esversion: 6 */
(function () {
  'use strict';
  const isSymmetric = n => {
    const asString = n.toString();
    return asString === asString.split("")
      .reverse()
      .join("");
  };
  const range = num => Array.from(new Array(num), (_, i) => num - i);
  const from999To100 = range(999)
    .filter(x => x >= 100);

  //https://stackoverflow.com/questions/12303989/cartesian-product-of-multiple-arrays-in-javascript/36234242#36234242
  const cartesianProduct = arr => arr.reduce(
    (a, b) => a.map(
      x => b.map(
        y => x.concat(y)))
    .reduce((a, b) => a.concat(b), []), [
      []
    ]);

  const combinations = cartesianProduct([
    from999To100, from999To100
  ]);
  const solution = combinations.map(x => x[0] * x[1])
    .filter(isSymmetric)
    .reduce((acc, x) => acc < x ? x : acc, 0);
  console.log("solution ", solution);
})();


Find the sum of Fibonacci sequence I have already done many of the Project Euler questions in other languages before, but whenever learning a new language, I like to do the problems again in that language.

Here is my elixir version of

  Find the sum of all Fibonacci numbers below 4,000,000.

stream = Stream.unfold({0,1}, fn {a, b} -> {a, {b, a + b}} end)
Enum.reduce_while(stream, 0, &(
    cond do
        &1 < 4000000 and rem(&1, 2) == 0 ->
            {:cont, &2 + &1}
        &1 < 4000000 ->
            {:cont, &2}
        true ->
            {:halt, &2}
    end
))

Can anyone spot a way to make my code fit the elixir paradigm more? Are there things I could improve?

I'd improve two things:

take advantage of ability to specify multiple clauses for anonymous function,
use underscore for big numbers to improve code readability.

My take on your code:

{0, 1}
|> Stream.unfold(fn {a, b} -> {a, {b, a + b}} end)
|> Enum.reduce_while(0, fn
  value, acc when value < 4_000_000 and rem(value, 2) == 0 -> {:cont, acc + value}
  value, acc when value < 4_000_000 -> {:cont, acc}
  _value, acc -> {:halt, acc}
end)


Overloading email sender utility class methods take single or array arguments I'm writing a simple utility class for sending an email. I'm not sure what's the proper way to present this to a consumer. Should I force them to use an array in send()? Or should I provide an overload so they can pass a single argument?

public class SimpleSender {

    public static void send(String smtpServer, String[] to, String[] cc, String from,
                            String subject, String body, boolean HTML, String fullFilePath) {
        SendEmail(smtpServer, to, cc, from, subject, body, HTML, fullFilePath);
    }
    public static void send(String smtpServer, String[] to, String[] cc, String from,
                            String subject, String body, boolean HTML) {
        SendEmail(smtpServer, to, cc, from, subject, body, HTML, null);
    }

    // Should I provide this overload and similar ones?
    public static void send(String smtpServer, String to, String[] cc, String from,
                            String subject, String body, boolean HTML) {
        SendEmail(smtpServer, new String[] {to}, cc, from, subject, body, HTML, null);
    }
    private static void SendEmail(String smtpServer, String[] to, String[] cc, String from, String subject, String body, boolean HTML, String fullFilePath) {
     .... Send email
    }
}

I know it's a bit of overkill for just a simple email utility, but in general, what's a good practice for these situations?

Your code has a particular Code Smell:

  Too many parameters: a long list of parameters is hard to read, and makes calling and testing the function complicated. It may indicate that the purpose of the function is ill-conceived and that the code should be refactored so responsibility is assigned in a more clean-cut way.

One way to fix this code smell is by extracting the parameters to a EmailOptions class (or EmailDraft or something).

public class EmailOptions {
     private final List<String> recepients;
     private final List<String> cc;
     private final List<String> bcc;

     public void setHTML(boolean html) {
         this.html = html;
     }

     ...
}

Now that's only a stub but there's a lot you can add to it. You can even add a addReceipient(String email) method (also include some kind of e-mail validation if you feel like it).

In the end however, your method would just be this:

public void sendEmail(EmailOptions email) {
    ...
}

Which surely is easier to grasp than all those String parameters!


Concatenating 2 array variable names to create a new variable name I'm developing a room booking system using PHP for a school project.

I'm trying to create a variable name by concatenating values from two arrays together.
here are the two arrays:

$day = array('Mon', 'Tue', 'Wed', 'Thu', 'Fri');
$B = array('P1B', 'P2B', 'P3B', 'P4B', 'P5B', 'P6B');

and then here is the code I'm currently using to try concatenate them:

// getting all the values from the table using $_POST. There are 120 different values so it is best to create these variables using a procedure

for ($l = 0; $l < count($day); ++$l) {
    for ($k = 0; $k < count($B); ++$k) {
        $days = $day[$l];
        $week = $B[$k];
        ${$days . $week} = $_POST["'" . $days.$week . "'"];
    };
};

Is the syntax of the concatenated variable correct? Is there a better way of doing this?

The final variable names of the concatenated variable should look something like:

$MonP1B
$MonP2B
$MonP3B
$MonP4B
$MonP5B
$MonP6B
$TueP1B

etc.

I have to agree with bumperbox here - why create separate variables instead of using an array to store them all, as follows:

$day = array('Mon', 'Tue', 'Wed', 'Thu', 'Fri');
$B = array('P1B', 'P2B', 'P3B', 'P4B', 'P5B', 'P6B');
$arr = array();

for ($l = 0; $l < count($day); ++$l) {
    for ($k = 0; $k < count($B); ++$k) {
        $days = $day[$l];
        $week = $B[$k];
        $arr[$days . $week] = $_POST["'" . $days.$week . "'"];
    };
};

Which would generate:

Array
(
    [MonP1B] =>
    [MonP2B] =>
    [MonP3B] =>
    ...
    [FriP4B] =>
    [FriP5B] =>
    [FriP6B] =>
)

And which you can access via $arr['FriP5B'] for example.

Now, as to how you can improve your code, see this:

$days_of_the_week = array('Mon', 'Tue', 'Wed', 'Thu', 'Fri');
$number_of_days = count($days_of_the_week);
$rooms = array('P1B', 'P2B', 'P3B', 'P4B', 'P5B', 'P6B');
$number_of_rooms = count($rooms);
$booked_day_room = array();

for ($day = 0; $day < $number_of_days; ++$day) {
    for ($room = 0; $room < $number_of_rooms; ++$room) {
        $days_room = $days_of_the_week[$day] . $rooms[$room];
        $booked_day_room[$days_room] = isset($_POST[$days_room]) ?: $_POST[$days_room];
    };
};

print_r($booked_day_room);

What we did:

Used descriptive variable names
Cached the count() function calls
Stored everything in 1 array, so we don't have to chase 100 different variables

Why are we using an array over multiple variables? Multiple variables might be a bit faster, and they might use more or less the same memory, however from a code readability standpoint, you want one array as it is easier to keep track of in the code.


Is it a valid use case of using java.util.MissingResourceException? I have a factory method that returns an Object based on a argument passed to it :

public class Factory {
   private Map<String,CustomObject> objectMap;

   public CustomObject getObject(String key){
       if(objectMaps.containsKey(key)) return objectMap.get(key);
       throw new MissingResourceException("Object not found",CustomObject.class.getName(),key);
   }
}

I wanted an opinion on whether the use of java.util.MissingResourceException is valid. I don't want create a custom exception for above scenario. I want to use one of the existing ones. If possible which other existing exception can be used in above case. I was thinking of IllegalStateException,but it didn't seem the right choice.

This is easy to answer: The Exception does not belong to your problem domain. It is not "your" exception. So its use is not appropriate.

Another point to mention is: MissingResourceException is used when the code would expect a static resource provided by a deployment or packaging. Resource-files may be somthing like images or i18n-files. Furthermore configuration-files and everything that is expected to be available in a static way. So your use case does not match the proper usage either.

My suggestion is not to reuse java exceptions that have special usages within the java language unless you are working on the same "abstraction level". You should inherit your own exceptions from "Exception" or far better "RuntimeException" (not signature relevant) and match the exceptions to YOUR problem domain. Everything else is misleading and you have a hard time to identify the proper exception for YOUR context. I do not think that it even exists because of the different abstraction levels. JAVA Exceptions deal with the JAVA language semantics. Everything you do as a developer is nearly always on a higher abstraction level.

JAVA Documentation:

https://docs.oracle.com/javase/7/docs/api/java/util/MissingResourceException.html

https://docs.oracle.com/javase/7/docs/api/java/lang/IllegalStateException.html


WebAPI Authentication Do you see any security issues with this authentication approach or have any suggestions?

WebAPI is REST-based.  The user logs in by calling the Authentication Controller and expects a 401 if unauthenticated or a 200 with a session token in the header.

GitHub

Session session = null;
try
{
    session = Models.User.Login(user);
}
catch (Exception ex)
{
    var errResponse = new HttpResponseMessage(System.Net.HttpStatusCode.Unauthorized)
    {
        Content = new StringContent("Invalid Username or Password"),
        ReasonPhrase = ex.InnerException.ToString()
    };
    throw new HttpResponseException(errResponse);
}

On every call except Authentication the request is intercepted and expects a session header.  If a session exists in the database then the request is authenticated.

protected async override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
{
    try
    {
        //Unauthorized
        if (HttpContext.Current.Request.Headers[SessionHeader] == null)
        {
            var errResponse = new HttpResponseMessage(System.Net.HttpStatusCode.Forbidden)
            {
                Content = new StringContent(SessionError),
                ReasonPhrase = SessionError
            };
            return errResponse;

        }

        string sessionHeader = HttpContext.Current.Request.Headers[SessionHeader];

        Session session = Session.Lookup(sessionHeader);

        if (session == null)
        {
            throw new SecurityException("No session record exists");
        }
        else if (session.Expired)
        {
            throw new SecurityException("Session Expired");
        }
        else
        {
            session.LastAccess = DateTime.Now;
            session.Save();
            var identity = new SecurityPrincipal(session);
            Thread.CurrentPrincipal = identity;
            HttpContext.Current.User = identity;

        }

The session lookup is done with Entity Framework

public static Session Lookup(string sessionId) {
    using (var context = new SingleAppContextCustom())
    {
        var session = (from s in context.Sessions.IncludeChildren()
                        where s.SessionId == sessionId
                        select s).FirstOrDefault();

        return session;
    }
}

In general, you want to give as little information as possible for why access was denied.  So I would change that section of code to:

if (session == null || session.Expired)
{
    throw new SecurityException("Access Denied");
}

Presumably this exception gets converted to an appropriate HTTP status code.

SendAsyc is an odd name for a function that enforces security, but maybe you have a reason for that.  That said, it seems like there's something missing since I don't see where it is async or returns a Task.


Create a function with an undetermined number of successive calls As part of a programming challenge, we are tasked with creating a function with an undetermined number of successive calls. As an example, let's say the function returns simply the sum of the provided arguments, it should work as follows :

sum(4)() // 4
sum(4)(5)() // 9
sum(4)(5)(9)() // 18
sum(4)(5)(9)(1)() // 19
// etc...

The problem is simplified by the allowed empty function call at the end as an indication of end of calls.

I have worked on a solution that does the job but using global variables inside the function itself :

var sum = function (a) {
    if (!sum.init) {
        sum.total = 0;
        sum.init = true;
    }
    if (!arguments.length) {
        sum.init = false;
        return sum.total;
    }
    sum.total += a;
    return sum;
};

This solution works but uses state, global variables and function object trickery which is not ideal. My question here is whether there is a way to solve the problem in a purely recursive way.

As a side note, I do not believe the problem can be solved if that last empty call () is not provided, but if I'm wrong please let me know.

This solution uses a functional approach with currying, which i find more elegant since it doesn't have to rely on global variables

function sum(total) {
    return function () {
        if (arguments.length == 0) {
         return total;
        } else {
         return sum(total + arguments[0]);
        }
    }
}

console.log(sum(4)()) // 4
console.log(sum(4)(5)()) // 9
console.log(sum(4)(5)(9)()) // 18
console.log(sum(4)(5)(9)(1)()) // 19

The first call to ´sum´, returns the inner function. depending on the number of arguments passed to the inner function, it returns the value of sum's argument as it is or it calls again ´sum´ with the updated running total (thus returning again the inner function to be called again).

The limitation (of this implementation) is that you HAVE TO make a final call with no arguments to have to final value.

See this answer (Method 3: Infinite Level Currying) for an example of using .valueOf to get the result out at every call, even those with arguments.


Loader animation using Custom View in Android I created a loader animation for an Android app: a simple 3 dots loader.

Is this the correct way to animate by using TimerTask?

public class ThreeDotsLoader extends View {

    Paint dot1;
    Paint dot2;
    Paint dot3;

    int defaultColor;
    int highlightColor;

    boolean animationStarted = false;

    TimerTask task;
    Timer timer;

    int count = 1;

    public ThreeDotsLoader(Context context, AttributeSet attrs) {
        super(context, attrs);
        defaultColor = Color.rgb(44, 78, 82);
        highlightColor = Color.WHITE;

        initPaint();
    }

    @Override
    protected void onDraw(Canvas canvas) {
        super.onDraw(canvas);
        Log.d("custom", "ondraw");
        canvas.drawCircle(50, 50, 15, dot1);
        canvas.drawCircle(100, 50, 15, dot2);
        canvas.drawCircle(150, 50, 15, dot3);
    }

    private void initPaint(){
        dot1 = new Paint();
        dot1.setColor(defaultColor);
        dot1.setStyle(Paint.Style.FILL);
        dot1.setAntiAlias(true);

        dot2 = new Paint();
        dot2.setColor(defaultColor);
        dot2.setStyle(Paint.Style.FILL);
        dot2.setAntiAlias(true);

        dot3 = new Paint();
        dot3.setColor(defaultColor);
        dot3.setStyle(Paint.Style.FILL);
        dot3.setAntiAlias(true);
    }

    public void startLoading(){
        task = new TimerTask() {

            @Override
            public void run() {

                switch(count){
                case 1:
                    dot1.setColor(highlightColor);
                    dot2.setColor(defaultColor);
                    dot3.setColor(defaultColor);
                    count++;
                    break;
                case 2:
                    dot1.setColor(defaultColor);
                    dot2.setColor(highlightColor);
                    dot3.setColor(defaultColor);
                    count++;
                    break;
                case 3:
                    dot1.setColor(defaultColor);
                    dot2.setColor(defaultColor);
                    dot3.setColor(highlightColor);
                    count = 1;
                    break;
                }
                animationStarted = true;
                postInvalidate();
            }
        };
        timer = new Timer();
        timer.schedule(task, 0, 300);

    }

    public void stopLoading(){
        animationStarted = false;
        timer.cancel();
    }
}

I would avoid "new Timer();" for this use case.
The timer task it changing the paint object on a thread other than the main UI thread, which is very likely to lead to non-deterministic behavior.

I recommend using an ObjectAnimator ( see the Property animation guide )

If you need to support pre-API 11 releases, you can use NineOldAndroids.


Cosine Similarity on Huge Dataset I have a very large data file full of movie ratings that I am looking at for work. I wanted to do this in a clean and very effective manner. The ratings file contains on a per column by column basis:

userID, movieID, rating...

I have parsed the files, and I am now trying to compute cosine similarity of all 100,000 ratings for each movie. Thus, I'm using an ADT Hashmap to store the values of the ratings of each movie as follows HashMap. For each 1000 or so movie, I'm to compute the cosine Similarity. This is what i have done so far, what do you guys think?

import java.util.*;
import java.io.*;

public class MovieRatingParser {
    static HashMap<String, Double> ratings = new HashMap<>();

    public void parseMovieFile() throws FileNotFoundException, IOException {
        //Create an ArrayList to store movies
        ArrayList<Movie> movies = new ArrayList<Movie>();
        try {
        //Create a buffered file reader for FileReader to read in movies.dat
            BufferedReader br = new BufferedReader(new FileReader("movies.dat"));

            String readFile = br.readLine();
            while (readFile != null) {
                //Use String split delimiter to load each movie one by one
                //File delimiter is “\\|"
                String[] tokenDelimiter = readFile.split("\\|");
                String movieID = tokenDelimiter[0];
                String movieTitle = tokenDelimiter[1];

                Movie movieToAdd = new Movie(movieID, movieTitle);
                movies.add(movieToAdd);
                readFile = br.readLine();
            }
            br.close();
        } catch (FileNotFoundException e) {
            System.out.println("file was not Found!");
        }
        System.out.println("==============================================");

    }

    public static void parseRatingFile() throws FileNotFoundException, IOException{

        try {
            BufferedReader br = new BufferedReader(new FileReader("ratings.dat"));
            String readFile = br.readLine();
            while (readFile != null) {
                String[] tokenDelimiter = readFile.split("\\|");
                String userID = tokenDelimiter[0];
                String movieID = tokenDelimiter[1];
                double rating = Double.parseDouble(tokenDelimiter[2]);

                ratings.put(movieID, rating);
                readFile = br.readLine();
            }
            br.close();
        } catch (FileNotFoundException e) {
            System.out.println("File was not Found!");
        }
    }

    public static double computeCosineSimilarity(HashMap<String, Double> movieA, HashMap<String, Double> movieB) {

        double dotProduct = 0.0;
        double normA = 0.0;
        double normB = 0.0;
        parseRatingFile();
        for (int j = 0; j < ratings.size(); j++) {
            movieA.put(ratings.get(3), ratings.values());
        }

        for (int i = 0; i < movieA.size(); i++) {
            dotProduct += movieA[i] * movieB[i];
            normA += Math.pow(movieA[i], 2);
            normB += Math.pow(movieB[i], 2);
        }

        return dotProduct / (Math.sqrt(normA) * Math.sqrt(normB));
    }

}

What can I do to improve the code? It looks very sloppy.

I'm not familiar with the algorithm you've implemented. So I cannot point to improvements there. But some things in the code can be enhanced.

Use informative error messages. For instance, instead of:

    ...
    } catch (FileNotFoundException e) {
        System.out.println("file was not Found!");
    }
    ...

consider something like:

    ...
    } catch (FileNotFoundException e) {
        String detailedMessage =
               format("File [%s] was not found. Reason was [%s]!", "movies.dat", e.getMessage());
        // BTW "movies.dat" can be extracted into constant.
        System.out.println(detailedMessage);
    }
    ...

In the latter snippet you can see that error message includes detailed info about what really happened. And please note [] that surround variable data: such placeholders not only help to see corner cases in log (for example, when empty name of input file was specified by mistake) but do grep (or any other text search) efficiently.

Consider try-with-resources. That will reduce amount of boilerplate code when dealing with readers.

Move parsing logic, e.g.:

...
String[] tokenDelimiter = readFile.split("\\|");
String userID = tokenDelimiter[0];
String movieID = tokenDelimiter[1];
double rating = Double.parseDouble(tokenDelimiter[2]);
...

into separate helper method like it's already done for computeCosineSimilarity().

After all "little" improvements are done you will see the code more clearly. Then you can concentrate on the algorithm (e.g. on pure logic), add checks for corner cases (like empty input file), use strict math for floating point numbers, handle encoding of input files gracefully, improve overall processing speed for large files, etc.