Multiple null Checks or try/catch NullPointerException There is A LOT of information online stating that you should NEVER catch a NullPointerException.  Generally I agree, but I am wondering about this one case.

I have inherited code that requires me to access data that I need in the following way

context.getGrandParent().getParent().getChild().isRequired()

There is no guarantee that any of the objects in this hierarchy will not be null.
 I have to enter a block if isRequired() returns true.  First, and what I initially wrote, with null checks:

if(context != null
   && context.getGrandParent() != null
   && context.getGrandParent().getParent() != null
   && context.getGrandParent().getParent().getChild() != null
   && context.getGrandParent().getParent().getChild().isRequired()
){
   // continue with business logic
} else {
   LOG.error("Unable to determine if processing is required.");
}
// continue with other inherited code

Setting aside that I could refactor this, perhaps for readability, wouldn't it make more sense to do the following?

boolean isRequired = false;
try {
   isRequired = context.getGrandParent().getParent().getChild().isRequired();
} catch (NullPointerException npe) {
   LOG.error("Unable to determine if processing is required.");
}

if(isRequired){
   // continue with business logic
}
// continue with other inherited code

The problem with catching NullPointerException is “which one did you catch?”  A null can be returned from getGrandParent(), and using that return value without checking will cause the exception.  OR a bug in getGrandParent() might cause an exception while trying to find the parent’s parent, and you are obscuring the bug by assuming the NullPointerException results from a properly returned null value.

You can use Optional to properly capture the null and not call subsequent function.

Optional<Boolean> isRequired = Optional.ofNullable(context)
                          .map(Context::getGrandParent)
                          .map(GrandParent::getParent)
                          .map(Parent::getChild)
                          .map(Child::isRequired);

if (!isRequired.isPresent()) {
    LOG.error("Unable to determine if processing is required.");
} else if (isRequired.get()) {
    // continue with business logic
}

The Context::, GrandParent::, Parent::, Child:: class types are, of course, WAG's.  You'll need to supply the corrent types based on the type returned by the previous stage.

Alternately, you could use .getOrElse(Boolean.FALSE) at the end of the .map() chain.


RESTfulPHP / controller / structure Allow for stuff like:

API/ExampleObjects
API/ExampleObjects/SOME_INTEGER_ID/Children
...

with children knowing their owners (aka parents).

Full repo here

Please be brutally honest if something can be improved!
I couldn't add the rest tag as I don't have 150 rep yet.

<?php

class RestApi {

    private
        $id,
        $owner
    ;

    public function __construct( $params = false ) {

        if( $params['request'] ) {
            // grab first piece of the request
            if( strlen($request = trim(strstr($params['request'], '/'), '/')) > 0 ) {
                // php 5.2 compatible
                $pieces = explode('/', $params['request']);

                if( is_numeric($pieces[0]) ) {
                    $this->setId( $pieces[0] );
                    $params['request'] = $request;
                }
                unset( $pieces );
            } else {
                // if the request is numeric, then it's an id
                if( is_numeric($params['request']) ) {
                    $this->setId( $params['request'] );
                    $params['request'] = '';
                }
            }
        }
    }

    public function getInstance( $params ) {
        if( isset($params['owner']) && is_object( $params['owner'] ) ) {
            $this->setOwner( $params['owner'] );
        }

        // if resource starts with "api/", strip it
        if( strpos($params['request'], 'api/' ) === 0 ){
            $params['request'] = substr($params['request'], 4);
        }

        if( $params['request'] ) {
            if( strlen($request = trim(strstr($params['request'], '/'), '/')) > 0 ) {
                // php 5.2 compatible
                $pieces = explode('/', $params['request']);
                $class = $pieces[0];
                unset( $pieces );
            } else {
                $class = $params['request'];
            }

            $class = get_class($this) . $class;

            $instance = new $class(array('request' => &$request));
            $instance->setOwner( $this );

            if( $request ) {
                return $instance->getInstance(array(
                    'request' => $request
                ));
            }

            return $instance;
        }
    }

    public function getOwner() {
        return $this->owner;
    }

    protected function setOwner( $owner ) {
        $this->owner = $owner;
    }

    public function getId() {
        return $this->id;
    }

    protected function setId( $id ) {
        $this->id = (int)$id;
    }

}

Good

Private attributes for storing state in the class (id, owner).
Getter and setter methods are simple.

Bad
Not a RestApi

REST is an architectural style it can't be represented by an object.  It is the wrong level of abstraction for an object to represent an architectural style.
Your RestApi object has public methods: getInstance, getId and getOwner. These have nothing to do with representational state transfer.

Constructor

The constructor should only create the object. By doing so much in your constructor you make it difficult to extend your class (going against the Open / Closed principle). It also makes it harder to unit test this code.
If you construct your object with an array that doesn't contain the key 'request' your code will emit an error notice.
unset($pieces); is unnecessary. Don't worry about memory management.

getInstance

This method should not be called getInstance.
This method does too much.
It can call setOwner when it should only be getting an instance.
Also, why should the class that you create depend on your RestApi object? get_class($this) will return 'RestApi' or 'RestApiDerived' if you derive another class from it. Why should the object that you create be so tightly coupled to the RestApi class?

I'm actually left confused by this class. The naming of methods and the object itself doesn't help me to understand what sort of objects this class represents. It doesn't map to anything in the real world that I can understand.


helper function to determine if html text is wrapped with a certain tag I have this code that I've made to parse a string of HTML and tell me if it is all enclosed within a specified tag:

const textIsWrappedWithTag = (text, tag) => {
  const tempDocument = new DOMParser().parseFromString(text, "text/xml");
  if (tempDocument.getElementsByTagName("parsererror").length) {
    return false;
  }
  if (tempDocument.children.length === 1 && tempDocument.children[0].tagName.toLowerCase() === tag.toLowerCase()) {
    return true;
  }
  return false;
};

My test cases do what I want them to do:

const yes = '

one

two

';
const no = '
one

two

three
';
const no2 = '

one

two

three

';

textIsWrappedWithTag(yes, 'p');
> true
textIsWrappedWithTag(no, 'p');
> false
textIsWrappedWithTag(no2, 'p');
> false

I played around with this in the console to develop it, but I'm not super famliar with DOMParser and parsererror, so I want to be sure that I'm not overlooking something

Your code looks good and concise.

Your test cases cover almost every branch of the code, which is also good.

You can improve the variable names:

The temp in tempDocument is not necessary. Therefore the variable name should better be document, or if that is too long, doc. Be careful though when abbreviating variable names, and don't make them ambiguous.
The no test case should rather be called parsererror. I had to run the code to see that it really tests this branch of the code. By improving the name of the variable, you make this more obvious.

There is a test case missing for a tag name other than p.

if (doc.children.length === 1 && doc.children[0].tagName.toLowerCase() === tag.toLowerCase()) {
  return true;
}
return false;

If you want, you can write this code a bit shorter:

return doc.children.length === 1
    && doc.children[0].tagName.toLowerCase() === tag.toLowerCase();

Your current code is a bit longer, but it allows you to set a breakpoint on the return true statement, which may be useful when debugging the code. But since you already have automated tests running, you will probably not need to set a breakpoint there. Your choice. Many programmers prefer the shorter form, but the longer form is not bad per se.

The expression doc.children.length === 1 is always true. This is because you are parsing the text using the text/xml type, and the XML standard requires exactly 1 root element. Everything else will end up in the parsererror branch. To verify this, you should add a test case for an empty string. And a test case for an XML comment, such as .

When I first read the function name textIsWrappedWithTag, I didn't know exactly what to expect from that function. My first thought was that textIsWrappedWithTag('

text

', 'p') would return true, since there is some text that is enclosed in a p tag. You could try to find a better name for that function, maybe topLevelElementIs.

By the way, the DOM API talks about elements, while the HTML standard calls them tags. If you want your function to feel like it belongs to the DOM API, you should call it textIsWrappedWithElement instead.


Compile and run C++ code I have created this bash script for Compile And Run c++ code, car++.sh.
As I'm new to bash scripting, I'm not sure about good bash programming practices.

Please review it to see if you could find any anomaly.

#!/bin/bash

found=false;
output=;
o_is_set=false;

#retrieve output_filename from arguments as provided by user
for arg in "$@"
do
    if [ "$o_is_set" = true ]
    then
        output="$arg";
        found=true;
        break;
    fi

    if [[ $arg == -o ]]
    then
        o_is_set=true;
    fi
done

#else retrieve from file name of first .cpp or .c++ file specified
if [ "$found" = false ]
then
    for file in "$@"
    do
        ext=$(echo $file | tr '[:upper:]' '[:lower:]');

        if [[ $ext == *.cpp ]]
        then
            output=$(basename "$file" ".cpp");
            found=true;
            break;
        fi

        if [[ $ext == *.c++ ]]
        then
            output=$(basename "$file" ".c++");
            found=true;
            break;
        fi
    done
fi

#else retrieve from first .c file specified
if [ "$found" = false ]
then
    for file in "$@"
    do
        ext=$(echo $file | tr '[:upper:]' '[:lower:]');

        if [[ $ext == *.c ]]
        then
            output=$(basename "$file" ".c");
            found=true;
            break;
        fi
    done
fi

#output file name is not specified by user and there is no C++ or C file,
#lets see what g++ says about it
if [ "$found" = false ]
then
echo;
    g++ -Wall -std=c++11 "$@";
    exit "$?";
fi

run="./$output";

g++ -Wall -o "$output" -std=c++11 "$@" && $run;

It's a good start, but I see a number of things that may help you improve your code.

Try to reduce the scope of variables

The variables o_is_set and found are essentially global variables, but don't need to be.  Instead, the code might be structured as a series of functions, reducing the linkage within the code.

Use standard option parsing

One could use getopts for parsing command line options, or plain old bash for that.  For example, consider this code for parsing a -o option with its corresponding argument:

while [[ $# > 1 ]]
    do
        opt="$1"
        case $opt in
            -o)
            OUTPUT="$2"
            shift
            ;;
            *)
                # ignore unknown option
            ;;
        esac
        shift
    done

Use make instead

The basic operation of this file is to create an executable from either a .cpp or .c file using the gcc compiler and then running the resulting executable.  One can do essentially the same thing with a single line:

#!/bin/bash
make $1 && ./$1

For example, if we have nuts.c in the current working directory and the file above is called car we can execute ./car nuts and make will run, find the nuts.c file, create an executable nuts and then run it.


Calling Web API from MVC generic way of approach Our Project architecture is AngularJS + MVC + WebAPI.  Angular JS would call MVC action and MVC action further calls API methods. Below are some of the action methods we used for calling api  methods.
Note that our project is in its final stage, and as such we cannot modify the existing architecture. We have to use AngularJS + MVC + WebAPI.

We are having lot of controllers and each controller contains lot of get/post methods.   I need suggestion about below part of code. because similar to below piece of code only we are using across all the controllers with different method names. Is there any other better way/  Generic way to call webapi from MVC.

    public async Task<ActionResult> GetStates()
                {

                    httpClient = HttpContext.Items["httpclient"] as HttpClient;
                    HttpResponseMessage response = await httpClient.GetAsync(httpClient.BaseAddress + "api/Admin/GetStates");
                    if (response.IsSuccessStatusCode)
                    {
                        string stateInfo = response.Content.ReadAsStringAsync().Result;
                        if (!string.IsNullOrEmpty(stateInfo))
                        {
                            return new ContentResult { Content = stateInfo, ContentType = "application/json" };
                        }
                    }

                    return new JsonResult();
                }
                public async Task<ActionResult> ClientsInfo()
                {

                    httpClient = HttpContext.Items["httpclient"] as HttpClient;
                    HttpResponseMessage response = await httpClient.GetAsync(httpClient.BaseAddress + "api/Admin/GetClients");
                    if (response.IsSuccessStatusCode)
                    {
                        string roleInfo = response.Content.ReadAsStringAsync().Result;
                        if (!string.IsNullOrEmpty(roleInfo))
                        {
                            return new ContentResult { Content = roleInfo, ContentType = "application/json" };
                        }
                    }

                    return new JsonResult();
                }

                public async Task<ActionResult> GetRoles()
                {
                    httpClient = HttpContext.Items["httpclient"] as HttpClient;
                    HttpResponseMessage response = await httpClient.GetAsync(httpClient.BaseAddress + "api/Roles");
                    if (response.IsSuccessStatusCode)
                    {
                        string roleInfo = response.Content.ReadAsStringAsync().Result;
                        if (!string.IsNullOrEmpty(roleInfo))
                        {
                            return new ContentResult { Content = roleInfo, ContentType = "application/json" };
                        }
                    }

                    return new JsonResult();
                }
...........
//for post and delete actions similarly

I agree as some others have said that in this case, having a layer between Angular and your API is simply bloat / overhead unless there are specific reasons for the extra abstraction.  I understand that you are late in the project development so my review will reflect that but as an aside I have to point out that your management should have a COMPLETE understanding of the technical debt incurred by any potential "Escalation of Commitment" mistakes made by them.  Especially considering Web API will eventually be deprecated: https://blog.tonysneed.com/2016/01/06/wcf-is-dead-long-live-mvc-6/

That being said, your code for the most part is fine but I would suggest a couple of architectural changes.

1. Keep your controllers thin

Controllers only responsibility should be to retrieve data and pass that data to the view.  In this case, your view is JSON, so your controller should ONLY make a cal to get data.

As the code currently is it is also more difficult to test than it needs to be.

2. Encapsulate methods

As you have pointed out, you are winding up with a lot of duplicate code.  I would suggest solving both problems by introducing another layer of indirection.

Example Code

  Controller Code

public class MyController : Controller {
    private IApiClient _apiClient;

    public MyController() : this(new WebApiClient()) {
        // Poor-man's DI in case you don't have an IoC container.
        // If you do, do not include this constructor
    }

    public MyController(IApiClient apiClient) : base() {
        this._apiClient = apiClient; // assign injected dependency
        // This injected dependency streamlines your testing process
        // because the results are more easily mocked than direct access
        // to the HttpContext as was done in the actions before
    }

    public async Task<ActionResult> GetStates() {
        string relativePath = "api/Admin/GetStates";
        return await this.ApiGet(relativePath);
    }

    public async Task<ActionResult> GetClients() {
        string relativePath = "api/Admin/GetClients";
        return await this.ApiGet(relativePath);
    }

    public async Task<ActionResult> Roles() {
        string relativePath = "api/Roles";
        return await this.ApiGet(relativePath);
    }

    protected async Task<ActionResult> ApiGet(string relativePath) {
        ApiResponse response = await this.ApiClient.GetAsync(relativePath);
        ActionResult result = response.IsSuccessful ? new ContentResult { Content = response.Data, ContentType = "application/json" } as ActionResult : new JsonResult();
        return result;
    }

    protected IApiClient ApiClient {
        get {
            return this._apiClient;
        }
    }
}

  The ApiResponse class

[Serializable]
public class ApiResponse {
    private bool _successful;
    private string _data;

    public ApiResponse() {
        this._data = string.Empty;
        this._successful = false;
    }

    public string Data {
        get {
            return this._data;
        } set {
            this._data = value;
        }
    }

    public bool IsSuccessful {
        get {
            return this._successful;
        } set {
            this._successful = value;
        }
    }
}

  The IApiClient and WebApiClient types

public interface IApiClient {
    Task<ApiResponse> GetAsync(string relativePath);
}

public class WebApiClient : IApiClient {
    private System.Net.Http.HttpClient _httpClient;

    public WebApiClient() {
    }

    protected Uri BuildCompleteUri(string relativePath) {
        // Using a URI will help us to validate the URL before making an invalid call.
        Uri returnUri = new Uri(VirtualPathUtility.Combine(this.Client.BaseAddress.ToString(), relativePath));

        return returnUri;
    }

    public async Task<ApiResponse> GetAsync(string relativePath) {
        Uri completeUri = this.BuildCompleteUri(relativePath);
        ApiResponse response = new ApiResponse();
        HttpResponseMessage apiResponse = await this.Client.GetAsync(completeUri);
        if (apiResponse.IsSuccessStatusCode) {
            string responseData = apiResponse.Content.ReadAsStringAsync().Result;
            if (!string.IsNullOrWhiteSpace(responseData)) {
                response.IsSuccessful = true;
                response.Data = responseData;
            }
        } else {
            // TODO: Log Errors
        }

        return response;
    }

    public HttpClient Client {
        get {
            if (this._httpClient == null) {
                this._httpClient = HttpContext.Current.Items["httpclient"] as HttpClient;
            }

            return this._httpClient;
        }
    }
}

The above code structure keeps your controllers thin, reduces code duplication, allows for extension points if you need to add more information of your own to the ApiResponse type, and dramatically simplifies your unit testing process because the IApiClient type is easily mocked.

You could even take it a step further and use the DI container to register a factory method that injects the HttpClient instance into the WebApiClient type, which would also simplify the testing process of the WebApiClient.

I also renamed your controller actions so they are consistent with the API method they invoke.


Solution to knapsack problem exceeds time and RAM limits My goal is to code the knapsack problem algorithm. The problem is that a knapsack has a given weight limit, W. I have a collection of items which have a given weight and value in a file that looks like:

# test.txt
1234  4456
4321  7654
...   ...

Where value is the first column, and weight is the second. My algorithm seeks to maximize the value of items in the knapsack while staying at or beneath the weight limit W. This is what I've come up with so far:

import numpy as np
#reading the data:
values = []
weights = []
test = []
with open("test.txt") as file:
  for line in file:
    value, weight = line.split(" ")
    values.append(int(value))
    weights.append(int(weight.replace("\n","")))

W = values.pop(0)
weights
size = weights.pop(0) +1
weights = [0] + weights
values = [0] + values

#Knapsack Algorithm:

hash_table = {}
for x in range(0,W +1):
  hash_table[(0,x)] = 0

for i in range(1,size):
  for x in range(0,W +1):
    if weights[i] > x:
      hash_table[(i,x)] = hash_table[i - 1,x]
    else:
      hash_table[(i,x)] = max(hash_table[i - 1,x],hash_table[i - 1,x - weights[i]] + values[i])

My idea is to use a "hash table" as a part of a dynamic programming technique in order to speed up my code. But I have tried to run it on a big input file and I am out of RAM (and it works very slowly).

Can you review my code and give me some hints how can I speed up my program?

Part of the reason you're running out of memory is that you are allocating all of your items into memory twice. Once in the two lists weights and values, and once again in hash_table.

Looking at your program, I don't see a need for you to keep your weights and values allocated as you do. You iterate through them one at a time in your outer for loop. What I'd do is make use of a generator and wrap the file reading in a function:

def read_file():
    with open('knapsack.txt') as fh:

        # these value names are a bit more descriptive, and
        # you can use a next() call on this generator
        # to extract these values
        WEIGHT, SIZE = map(int, next(fh).strip().split())
        yield WEIGHT, SIZE

        for line in fh:
            yield map(int, line.strip().split())

This way you can do tuple unpacking in a for loop:

iterator = read_file()
# here's that next call I mentioned
WEIGHT, SIZE = next(iterator)

# iterate over the remaining values
for weight, value in iterator:
    # do something

This will keep copies of your values from proliferating throughout the execution of your program when you really don't need them.

I'd also look into enumerate, since you need the index for part of your hash_table keys, but you also need the weight and value as well. This eliminate repeated lookups that slow down your code:

for i, (w, v) in enumerate(read_file(), start=1):
    for x in range(WEIGHT + 1):
        ...

To show the effect:

# repeated index lookup
python -m timeit -s 'x = list(range(1, 10000))' 'for i in range(len(x)): a = x[i] + 2'
500 loops, best of 5: 792 usec per loop

# no repeated index lookup
python -m timeit -s 'x = list(range(1, 10000))' 'for i, j in enumerate(x): a = j + 2'
500 loops, best of 5: 536 usec per loop

It doesn't appear that you really need the leading 0 0 row on weights and columns, either, since you start the index at 1, skipping it. Avoiding the addition of lists here cuts down on overhead, and you can specify enumerate to start at a given value with the start kwarg as I've done above.

The goal here should be to iterate over a collection as little as possible, so a refactored version might look like:

def read_file():
    with open('knapsack.txt') as fh:

        # these value names are a bit more descriptive, and
        # you can use a next() call on this generator
        # to extract these values
        WEIGHT, SIZE = map(int, next(fh).strip().split())
        yield WEIGHT, SIZE

        for line in fh:
            yield map(int, line.strip().split())

iterator = read_file()
WEIGHT, SIZE = next(iterator)
hash_table = {(0, i): 0 for i in range(WEIGHT + 1)}

for i, (w, v) in enumerate(iterator, start=1):
    for j in range(WEIGHT + 1):
        if w > j:
            hash_table[(i, j)] = hash_table[(i - 1, j)]
        else:
            hash_table[(i, j)] = max(
                hash_table[(i - 1, j)],
                hash_table[(i - 1, j - w)] + v
            )

This doesn't avoid all of the memory issues, however. You are dealing with a relatively large file and housing that in a dictionary will lead to heavy memory usage. As noted in the Wikipedia article, the solution you have implemented will have a worst-case space complexity of O(nW), which for this file is approximately O(n * 2000000)


Add 1 to an integer represented as an array of digits Problem Statement

This is another LeetCode question I came across, where I need to add 1 to a number.

The number is represented as an integer array where each index position is a digit and therefore a number from 0-9. There are no leading zeros except for the number 0 itself.
The index position 0, that is arr[0] represents the MSB and the index position n-1, that is
arr[n-1] represents the LSB.

Thus, we need to add one to the LSB and return the number as an integer array. Here is the code which I wrote (it got accepted, passed all test cases) :

class Solution
{
    public int[] plusOne(int[] digits)
    {
        int carry = 0,
        len = digits.length;

        int i = len - 1;

        if (len == 0)
            return digits;

        if (len == 1 && digits[0] < 9)
        {
            digits[0] += 1;
            return digits;
        }

        if (len == 1 && digits[0] == 9)
        {
            digits[0] = 0;
            digits = resize(digits, 2);
            return digits;
        }

        do
        {
            digits[i] += 1;

            if (digits[i] == 10)
            {
                digits[i] = 0;
                carry = 1;
            }
            else
            {
                carry = 0;
                return digits;
            }

            i--;
        }
        while (carry == 1 && i > -1);

        if (carry == 1)
        {
            digits = resize(digits, len + 1);
            carry = 0;
        }

        return digits;
    }
    private int[]  resize(int[] arr, int size)
    {
        int[] copy = new int[size];
        int i;

        copy[0] = 1;

        for (i = 1; i < size; i++)
            copy[i] = arr[i-1];

        return copy;
    }
}

Analysis

Running Time : O(n) in the worst case
Total Space  : O(n) in the worst case
Worst Case   : When adding 1 to a m-digit number results in an (m+1) digit number
(eg. 999 + 1 = 1000)

Questions

Is there a better way to solve this problem? (I mean, a different but more efficient algorithm than mine)
I got a performance of only 29% with this solution no matter how many times I submitted the code, so definitely my algorithm is inefficient
Instead of using a resizing array, should I be using an array list if I want better performance?
Can ArrayList handle the overflow condition better than resizing array?
Should I be using the resizing array technique at all?
Is it less efficient?
Is O(n) the best possible running time for the problem?
I don't see how we can do it O(log n) or less.

Further Notes

Here is a link to the Github repository which has the above code with more comments: PlusOne.java

Simpler implementation

You can simplify a lot the iteration through the digits. Resizing with an ArrayList would definitely be worse, as it's simply a wrapper of an array and you would need to box/unbox the ints to Integer.

This uses the same algorithm, but with less instructions and it will therefore be a bit faster:

public int[] plusOne(int[] digits) {
  int len = digits.length;
  if (len == 0)
    return digits;

  for (int i = len - 1; i >= 0; i--) {
    if (digits[i] == 9) {
      digits[i] = 0;
    } else {
      digits[i]++;
      return digits;
    }
  }

  return newArray(len + 1);
}

New array

In the case where we create a new array, we know that it will be filled by a 1 followed by all zeros. Since in Java all variables not explicitly initialized have their default values (0 in case of int), we can simply do:

private int[] newArray(int size) {
  int[] arr = new int[size];
  arr[0] = 1;
  return arr;
}

This will save us one read and one write for every position.

Benchmark

I did a test with JMH, comparing both solutions.

Parameters:

# Warmup: 5 iterations, 1 s each
# Measurement: 5 iterations, 1 s each
# Benchmark mode: Average time, time/op

Input data sets:

"mixed": Starts with {0,0,0,0,0,0,0,0,0,0}, and gets increased in every iteration;
"10000x9": It's an array composed of 10 000 '9's, like {9, 9, 9, 9, ...}. This is reset back to all-9s in every iteration.

Results:

Benchmark              Mode  Cnt      Score     Error  Units
Test.fill              avgt    5   1852.195 ▒ 101.178  ns/op
Test.new_10000x9       avgt    5   8793.836 ▒ 176.757  ns/op
Test.new_mix           avgt    5      3.957 ▒   0.040  ns/op
Test.original_10000x9  avgt    5  15761.188 ▒  46.132  ns/op
Test.original_mix      avgt    5      4.209 ▒   0.026  ns/op

The fill test case was just to figure out the approximate overhead of resetting the array to 9's in the 10000x9 tests. If we remove that time from the results of the 10000x9 test cases, we see that the new implementation is about twice as fast.


Calculator Program which can operate on multiple numbers All the calculator programs I have seen so far seem long, complicated and they can only operate on two numbers. I am trying to write one that is simple, short, and it can operate on as many numbers as you want. After implementing suggestions from my previous question regarding use of operator.XXX and functools.reduce I've got a new version for review:

import operator
import functools
amount_of_numbers = int(input("How many numbers?"))
my_operator = input("Which operator (*, /, +, -)?")
if my_operator == "/":
    numbers=[float(input("Number: ")) for _ in range(amount_of_numbers)]
else:
    numbers = [int(input("Number: ")) for _ in range(amount_of_numbers)]

OPERATORS = {
    '+': operator.add,
    '-': operator.sub,
    '*': operator.mul,
    '/': operator.floordiv
}

INIT_VALUE = {
    '-': 0,
    '/': 1.00,
    '+': 0,
    '*': 1
}
result = functools.reduce(OPERATORS[my_operator], numbers, INIT_VALUE[my_operator])
print (result)

After a little back and forth, we got the suggestions of the previous question implemented. However there are still some minor issues:

Kind of strange mixing int and float calculations – I know you got 0 when doing divisions, but that is more of a test number issue than a fault in code. As it currently stands you do float numbers for the division, but your operator is operator.floordiv which rounds of the division.

If you want to use floats when dividing, then change to operator.div, and if you want to use integer division remove the float(input("Number: ")) ..., and let the operator.floordiv remain.
No need for the initializing values when using functools.reduce – The newer reduce function does what is expected when reducing, so you don't need this at all in the new code.
The advice to use doctest still applies – This both provides examples for use, and verifies that your code does what it is expected to do.
Don't use top level code, include it in main() - Move all of your code at the top level into a function, which you call from if __name__ == '__main__':, as this cleans up the code interface, allows your function to be called if imported in another file, and it looks nice.

Refactored code

When implementing all of this, and using integer division the code looks like this:

import operator
import functools
import doctest

OPERATION = {
    '+': operator.add,
    '-': operator.sub,
    '*': operator.mul,
    '/': operator.floordiv
}

def calculate(numbers=None, operator=None):
    """Apply the operator to all numbers, and print result.

    >>> calculate([1, 2, 3, 4], '+')
    10
    >>> calculate([1, 2, 3, 4], '-')
    -8
    >>> calculate([1, 2, 3, 4], '*')
    24
    >>> calculate([120, 2, 3, 4], '/')
    5
    """

    if not numbers:
        amount_of_numbers = int(input("How many numbers? "))
        numbers = [int(input("Number: ")) for _ in range(amount_of_numbers)]

    if not operator:
        operator = input("Which operator (*, /, +, -)? ")

    result = functools.reduce(OPERATION[operator], numbers)
    print(result)

if __name__ == '__main__':
    doctest.testmod()
    calculate()

Note how this way of making it a function allows for 'proper' use as a function, whilst still allowing for user to input the missing numbers and/or operator of choice.

Another possible extension would be to add another parameter to control whether it is printing the result, or returning it for further calculations. Possibly even adding a loop around it, so you can repeat calculating other stuff?


First Project Euler solution To continue learning I decided to try solving some of project Euler problems. Here is the first solution.

The things I'm interested the most from the review are:

The performance of the code
Overall review of the code structure, styling rules and naming conventions.

Problem: 1

  Find the sum of all the multiples of 3 or 5 below 1000.

#----------------------------------------------------------------------------------
def calculate_the_sum(threshold = 10):
    """Calculates the sum of all multiples of 3 or 5
    below the provided threshold.

    Arguments:
       type:<int> -Preforms the calculations on all values up to this value.
    Return:
       type:<int> -The sum of numbers that are multiples of 3 or 5..
    """
    return sum(n for n in range(threshold) if n % 3 == 0 or n % 5 == 0)

#----------------------------------------------------------------------------------
def main():

    threshold = 1000
    print(' Calculates the sum of all numbers that are multiples of 3 or 5')
    print(' The Sum of numbers below {} is: {}'.format(threshold,calculate_the_sum(threshold)))

#----------------------------------------------------------------------------------
if __name__ == "__main__":
    main()

For the first problem I suggest you some performance improvements. You can use inclusion-exclusion technique for this problem.

So first improvement

def calculate_the_sum(threshold = 10):
    return(sum(range(3, threshold, 3)) + sum(range(5, threshold, 5)) - sum(range(15, threshold, 15)))

def main():
    threshold = 1000000
    print(' Calculates the sum of all numbers that are multiples of 3 or 5')
    print(' The Sum of numbers below {} is: {}'.format(threshold,calculate_the_sum(threshold)))

if __name__ == "__main__":
    main()

I have tested both solutions in ideone (Python 3) for threshold=1000000. Your solution ~ 0.20s. My ~ 0.05s.

Next – you can use well known formula for the sum of arithmetic progression.


Finding the object and the URL associated with a notification I have a Notification model which inherits from the ActiveRecord class. Each notification is associated with a specific type of action, which is determined by an integer field (action) whose value is between 0 and 6. A notification is also associated with a model (e.g. User or Project) whose ID is stored as the object_id field.

Inside a view, I am trying to prepare the notification text and url, which looks something like this:

# notif_string prepares the text of notification.
# For example: "username commented on"
= link_to(notif_string(notification) + notification.objectname,
            notification.url)

The following methods in the Notification class where flagged as complex by Code Climate:

# Finds the object on which action is performed.
# Action can be of following types:
# 0: Commented on Project,
# 1: Followed Project,
# 2: Forked,
# 3: Followed User,
# 4: Created Project,
# 5: Commented on Issue
# 6: Created Issue
def objectname
  case action
  when 0
    return Project.find(Comment.find(object_id).polycomment_id.to_i).name
  when 5
    return Issue.find(Comment.find(object_id).polycomment_id.to_i)
                .friendly_text
  when 3
    return User.find(object_id).username
  when 1, 2, 4
    return Project.find(object_id).name
  when 6
    return Issue.find(object_id).friendly_text
  end
end

# Find the url to which user which be directed when they click on the
# notification item
def url
  case action
  when 0
    return Project.find(Comment.find(object_id).polycomment_id.to_i).urlbase
  when 5
    return Issue.find(Comment.find(object_id).polycomment_id.to_i).show_url
  when 1, 2, 4
    return Project.find(object_id).urlbase
  when 6
    return Issue.find(object_id).show_url
  else
    return "/#{actor.username}"
  end
end

Yes, my cases are almost repeated but I am not sure how can reduce them. Most of the blogs suggest that I use polymorphism but again I don't know how I can apply it here.

First, object_id is a built-in method of Object in Ruby, so I recommend renaming this field to model_id to avoid any clashes.

You want polymorphic classes for the views of different notification types. Since these classes will only be used by views, we'll make them all inherit from a common base class named NotificationView which will provide all the information the view needs:

class NotificationView
  attr_reader :action_description, :object_name, :url
end

Every subclass of NotificationView will initialize by setting these fields according to a given Notification model. For example:

class IssueCreatedNotificationView < NotificationView
  def initialize(notification)
    issue = Issue.find(notification.model_id)
    @action_description = "created issue"
    @object_name = issue.friendly_text
    @url = issue.show_url
  end
end

(I would put these classes in /app/models/notification_views/)

Now you need a method that constructs the appropriate NotificationView given a Notification. Since the type of a notification is determined by an integer field, you will need a case statement. But you'll only need a single case statement in your code because you can take advantage of polymorphism once you have a NotificationView instance.

I would put this method in the Notification class:

def view
  case action
  when 2
    ProjectForkedNotificationView.new(self)
  when 6
    IssueCreatedNotificationView.new(self)
  # etc...
  end
end

Then in your controller, you can get a NotificationView by calling Notification#view. For example:

@view = Notification.find(params[:id]).view

And you can use this object in your view:

= link_to(@view.action_description + @view.object_name, @view.url)

You may want to add some utility methods to the NotificationView base class, if you need them in your views, for example:

def message
  "#{action_description} #{object_name}"
end


Insert contact information from form fields into MySQL database when button is clicked I am just a beginner trying to do my due diligence and research things properly. I am trying to go about the correct way of adding data into a database. I have read articles on the following:

Not showing ConnectionString in plaintext. I currently store it in App.Config and I am still trying to figure out how to encrypt it.
using keyword to deal with disposable items.
catch try finally to deal with exceptions.
parameterized SQL statements to deal with SQL Injection.

I am looking for any advise to make my code better. I have revised it several times taking into account all the articles I have read about the topics above.

private void Btn_InsertData_Click(object sender, EventArgs e)
{
    if (TextboxesAreNullorEmpty() == true)
    {
        MessageBox.Show("Please fill out all fields!");
        return;
    }
    else
    {
        try
        {
            using (MySqlConnection conn = new MySqlConnection(Properties.Settings.Default.MySQLDBConnection))
            using (MySqlCommand cmd = new MySqlCommand())
            {
                conn.Open();
                cmd.Connection = conn;
                cmd.CommandText = "INSERT INTO `sites` (`ContractNumber`, `SiteName`, `SitePhoneNumber`, `SiteAddLine1`, `SiteAddLine2`, `SiteAddCity`, `SiteAddCounty`, `SiteAddPostcode`, `SiteAddCountry`) VALUES (@contract_name, @site_name, @site_phone_number, @site_add_line1, @site_add_line2, @site_add_city, @site_add_county, @site_add_postcode, @site_add_country)";
                cmd.Prepare();
                cmd.Parameters.AddWithValue("@contract_name", this.txtBox_ContractNumber.Text);
                cmd.Parameters.AddWithValue("@site_name", this.txtBox_SiteName.Text);
                cmd.Parameters.AddWithValue("@site_phone_number", this.txtBox_SitePhoneNumber.Text);
                cmd.Parameters.AddWithValue("@site_add_line1", this.txtBox_SiteAddLine1.Text);
                cmd.Parameters.AddWithValue("@site_add_line2", this.txtBox_SiteAddLine2.Text);
                cmd.Parameters.AddWithValue("@site_add_city", this.txtBox_SiteAddCity.Text);
                cmd.Parameters.AddWithValue("@site_add_county", this.txtBox_SiteAddCounty.Text);
                cmd.Parameters.AddWithValue("@site_add_postcode", this.txtBox_SiteAddPostcode.Text);
                cmd.Parameters.AddWithValue("@site_add_country", this.txtBox_SiteAddCountry.Text);
                cmd.ExecuteNonQuery();
                MessageBox.Show("Site Details Added");
            }
        }
        catch (Exception ex)
        {
            MessageBox.Show(string.Format("An error occurred {0}", ex.Message), "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
        }
        finally
        {
           ClearAllTextBoxes();
        }
    }

}

Not showing ConnectionString in plaintext. I currently store it in App.Config and I am still trying to figure out how to encrypt it.

Anything you can decrypt locally, a malevolent (and sufficiently skilled) user can too. More information given in the answer here.

There are other ways to avoid this:

Add a webservice backend layer. The frontend only connects to the backend (no connectionstring needed for that), and the backend has the connection string (which is out of reach for the frontend and its users).
Use Integrated Security, i.e. using the current Windows account for logging in to the database; as opposed to providing a username and password in the connection string. But you'd need to provide the needed rights to all of your users' Windows account, and then everyone is working under their own account (some companies do this, most don't).

  catch try finally to deal with exceptions.

Your current implementation seems okay, but I do wonder about the finally.

If there is no error, the textboxes get cleared. That seems okay. But if there is an error, the textboxes still get cleared? That doesn't seem user friendly. I'd expect you'd want to specifically keep the content of the textboxes so the user can retry saving the data.

try
{
    //save the data

    ClearAllTextBoxes();
}
catch (Exception ex)
{
    MessageBox.Show(string.Format("An error occurred {0}", ex.Message), "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
}

If an exception occurs when saving the data; the exception will skip over the ClearAllTextBoxes(); and go straight to the catch. It will never empty the textboxes.
However, if no exception is encountered, then the code will continue with ClearAllTextBoxes(); and therefore clear the textboxes.

  Minor comment
  You may want to put a return; in your catch. Not necessary at the moment, but it will become relevant if there is more code after your try catch.

In general, it's not good to put data handling (the SQL query) inside the UI. You'd want to put at least one layer of separation between the two.

However, you did mention you're a beginner; so I'm not going to hammer this point right now. If this project is a simple test application; you don't have to implement that layer of separation.

But I would advise trying to separate the two in future projects. You don't want a bad habit to form.

This is nitpicking, but

if (TextboxesAreNullorEmpty() == true)

really grinds my gears. == true is redundant, you can simply use

if (TextboxesAreNullorEmpty())

In order to reduce nesting, you can change your code. As it currently stands, you have:

if (TextboxesAreNullorEmpty())
{
     MessageBox.Show("Please fill out all fields!");
     return;
}
else
{
      //Do the work
}

You can omit the else.

if (TextboxesAreNullorEmpty())
{
     MessageBox.Show("Please fill out all fields!");
     return;
}

//Do the work

This works exactly the same way. The only difference is that you have less indentation in your code. While one extra level of indentation may not be an issue; a sufficiently complex algorithm can have several ìnstances of redundant indentation, at which point it does become a relevant consideration.

But since you are a beginner, this isn't really a big issue right now. Just keep it in mind for the future, to avoid a bad habit.

Although you're not yet at the point of abusing it; you should start looking into breaking up the code you have and putting it into multiple smaller methods.

Currently, you have a single method that does many different things:

Handle the button click event
Validate the form (this is already correctly abstracted into a separate method)
Set up the SQL connection and command
Update the data in the database
Handle errors

I would suggest you add two extra methods here:

Instead of putting the logic inside the click event handler, have the click event handler call a StartSave() method which does the work. The benefit of this becomes clear once you can have multiple triggers to save the data (since the act of saving data isn't uniquely connected to a single user control anymore).
Put the entire try catch into a separate method, e.g. UpdateDatabase(). Ideally, you'd want to use method parameters here for all the input variables; which will give you a layer of separation between your form controls and the SQL parameters.


Updating a dictionary of trajectories after a step has been made I am using a dictionary to save possible trajectories in a game. A trajectory is defined as a list of numbers separated by _. For example '3_7_2_5' is a trajectory of 4 steps. I use a dictionary as I assign a value to each trajectory. The meaning of this value does not matter for the purpose of my question. I also save the trajectories in separated dictionaries if they have different numbers of steps.
I want to update the dictionary in such a way that only the trajectories starting from '1' are preserved. Moreover, I want to remove the '1' from the name, since I don't need to keep listing a step that has already been made.
# here I create the initial dictionaries

pts=[{},{},{}]

for j in range(20):
    k=random.choice(range(3))
    path=str(k)
    for d in range(len(pts)):
        k=random.choice(range(4))
        pts[d][path]=k
        path+='_'+str(k)

print 'initial dictionaries =',pts

# here I make the update

ind=1
new_pts=[{},{},{}]
path=str(ind)
for d in range(len(pts)-1):
    for path in pts[d+1]:
        if path[:len(str(ind))]==str(ind):
            new_pts[d][path[len(str(ind))+1:]]=pts[d+1][path]

pts=new_pts

print 'updated dictionaries =',pts

As you can see, the first element of the old list pts has been discarded. The second element has been used to create the first element of the updated list and so on.
Now, it seems to me that my algorithm is not very efficient. For updating the dictionary I am using a for loop over all keys, even though most of them are going to be discarded.
Is there a better, faster way to do this?

Basic Review

You should import print_function from __future__. So you can use print like you can in Python 3.

Your variable names are poor.

j should be _.
What do j, k and d mean?
Why not just type out parts rather than use pts?

The way you're generating key value pairs in not amazing. If you make a function to build 20 keys then it would be much easier to understand. This is as things are split into two core aspects.

Building key
Building the dictionary

You should really use some functions.

You should really follow PEP 8. Your code is really hard to read because it looks like a block of characters, rather than a Python program.

Functional changes

A trajectory is defined as a list of numbers separated by _.

You should make it a tuple of numbers, (3, 7, 2, 5).

I also save the trajectories in separated dictionaries if they have different numbers of steps.

I see no reason to do this.

You may benefit from using a trie instead.
Since you're just printing the new dictionaries it doesn't make much sense.
However it looks exactly like what you want datatype wise.
I have included a build and an as_dict method to make understanding how it works a little simpler. You can easily remove the need for build by using it to build the trie directly from generate_keys.

from __future__ import print_function
import random

def generate_keys(amount, choices):
    for _ in range(amount):
        yield tuple(
            random.choice(choices[i])
            for i in range(random.randint(1, len(choices)))
        )

class Trie(dict):
    value = DEFAULT = object()

    @classmethod
    def build(cls, mapping):
        trie = cls()
        for keys, value in mapping.items():
            node = trie
            for key in keys:
                node = node.setdefault(key, cls())
            node.value = value
        return trie

    def _as_dict(self, path):
        for key, value in self.items():
            keys = path + (key,)
            if value.value is not self.DEFAULT:
                yield keys, value.value
            for item in value._as_dict(keys):
                yield item

    def as_dict(self):
        return dict(self._as_dict(()))

paths = {}
for key in generate_keys(20, [range(3), range(4), range(5)]):
    paths[key] = random.randrange(10)

trie = Trie.build(paths)
paths_new = trie[1].as_dict()

# Sort the output so it's easier to read
print('Initial dictionary =', dict(sorted(paths.items())))
print('Inital Trie =', trie)
print('Updated dictionaries =', dict(sorted(paths_new.items())))


Instance of one class to contain arbitrary number of instances of another class in Python I'm trying to see if there is a better way to design this. I have a class Animal that is inherited by Male and Female classes (Female class has an additional attribute). I also have a class called Habitat, an instance of each would contain any number of Animals including 0.

class Animal:

    def __init__(self, life_span=20, age=0):
        self.__life_span = life_span
        self.__age = age

    def get_life_span(self):
        return self.__life_span

    def get_age(self):
        return self.__age

    def age(self):
        self.__age += 1

class Female(Animal):

    __gender = 'female'

    def __init__(self, pregnant=False):
        self.__pregnant = pregnant

    def impregnate(self):
        self.__pregnant = True

class Male(Animal):

    __gender = 'male'

class Habitat:

    def __init__(self, list_of_animals=[Male(), Female()]):
        self.__list_of_animals = list_of_animals

    def __add_male(self):
        self.__list_of_animals.append(Male())

    def __add_female(self):
        self.__list_of_animals.append(Female())

    def add_animal(self):
        if random.choice('mf') == 'm':
            self.__add_male()
        else:
            self.__add_female()

    def get_population(self):
        return self.__list_of_animals.__len__()

Before I add any more functionality, I would like to find out if that's a proper way to design these classes. Maybe there is a design pattern I can use? I'm new to OOP, any other comments/suggestions are appreciated.

In Habitat.__init__, you've committed a classic beginner's fallacy: using a list as a default argument.

In Python, every call to a function with a default argument will use the same default object. If that object is mutable (e.g. a list) any mutations to that object (e.g. .append) will affect that one object. In effect, all your Habitats will end up using the exact same list unless you specify a non-default argument.

Instead, use None as the default argument, and test for it:

def __init__(self, animals=None):
    if animals is None:
        animals = [Male(), Female()]


Merge two sorted arrays that can be of different sizes I think the algorithm implementation itself is reasonably efficient, but I'm not sure about proper use of good practices in software development in the code (variable naming, idioms, comments, error handling in inputs etc).

I'm attempting to get a first developer job, and since interviewers also evaluate how "clean" the code is, the main point of the question is what is considered good practice, but efficiency improvements are also welcome.

mergeSortedArrays = function(arr1, arr2){
  if(!(Array.isArray(arr1) && Array.isArray(arr2))){
    throw new TypeError("Need two arrays to merge")
  };

  if(arr2.length === 0 && arr1.length === 0){
    throw new RangeError("Cannot merge empty arrays")
  };

  let i = 0;
  let j = 0;

  const targetSize = arr1.length + arr2.length;
  const mergedArray = [];
  // main loop
  while(mergedArray.length < targetSize){
    /**
     * Works until completion for arrays of the same size
     */
    if(arr1[i] < arr2[j]){
      valueToPush = arr1[i]
      i++
    }else{
      valueToPush = arr2[j]
      j++
    }
    /**
     * For arrays with different sizes, it is safe to push the remainder to the sorted array, given that both inputs are sorted.
     */
    if(valueToPush === undefined){
      const remainingItems = j > arr2.length ? arr1.slice(i) : arr2.slice(j)
      mergedArray.push(...remainingItems)
      break
    }

    mergedArray.push(valueToPush)
  }
  return mergedArray;
}
```

I think considering approach that you chose, your code is alright. A few points:

What I don't like is that you get error when passing 2 empty arrays? Why would you do that? Imagine I am generating arrays of different sizes and using your code to merge and sometimes I would pass 2 empty arrays. Should I really need to handle that as special case in my code? No, just return empty array too.
I'd eliminate targetSize variable and check for j < arr2.length && i < arr1.length instead.
Checking valueToPush for undefined feels a bit off for me. It works, but doesn't really feel right. In many languages you would get something like IndexOutOfRange error or something. Maybe it's idiomatic in Javascript (probably not), but I think nicer and cleaner would be to check if you are still in range of array, ex j < arr2.length.


BouncyCastle RSA Implementation This is my implementation of 1024bit(can be changed) RSA. Is there anything I'm doing wrong?

public class Rsa:IEncryption
{
    public AsymmetricCipherKeyPair Keys { get;private set; }

    private readonly Pkcs1Encoding _engine;

    public Rsa()
    {
        Keys = GenerateKeys();
        _engine = new Pkcs1Encoding(new RsaEngine());
    }

    public byte[] Encrypt(byte[] buffer)
    {
        return Encrypt(buffer, 0, buffer.Length);
    }

    public byte[] Decrypt(byte[] buffer)
    {
        return Decrypt(buffer, 0, buffer.Length);
    }

    public byte[] Encrypt(byte[] buffer, int offSet, int length)
    {
        return RsaProcessor(buffer, offSet, length, Keys.Public);
    }

    public byte[] Decrypt(byte[] buffer, int offSet, int length)
    {
        return RsaProcessor(buffer, offSet, length,Keys.Private);
    }

    private byte[] RsaProcessor(byte[] data,int offset,int length, AsymmetricKeyParameter key)
    {
        _engine.Init(!key.IsPrivate, key);

        var blockSize = _engine.GetInputBlockSize();

        var result = new List<byte>();
        for (var i = offset; i < offset+length; i += blockSize)
        {
            var currentSize = Math.Min(blockSize, offset + length - i);
            result.AddRange(_engine.ProcessBlock(data, i, currentSize));
        }
        return result.ToArray();
    }

    public static AsymmetricCipherKeyPair GenerateKeys()
    {
        var rsaKeyParams = new RsaKeyGenerationParameters(BigInteger.ProbablePrime(512, new Random()),
                                                          new SecureRandom(), 1024, 25); //Unsure about the certinaty parameter
        var keyGen = new RsaKeyPairGenerator();
        keyGen.Init(rsaKeyParams);

        return keyGen.GenerateKeyPair();
    }
}

Style

Almost all the naming of the parameters and methods are good.
RsaProcessor() sounds like a noun. Based on the naming guidlines you should use verbs or verb phrases to name your methods. Maybe a simple name like Process() would be sufficient here.

In the RsaProcessor() method I would suggest to introduce as Boolean forEncryption. In this way the meaning of this line

_engine.Init(!key.IsPrivate, key);

will be obvious.

private byte[] Process(byte[] data, int offset, int length, AsymmetricKeyParameter key)
{
    Boolean forEncryption = !key.IsPrivate;

    _engine.Init(forEncryption , key);

    var blockSize = _engine.GetInputBlockSize();

    var result = new List<byte>();
    for (var i = offset; i < offset+length; i += blockSize)
    {
        var currentSize = Math.Min(blockSize, offset + length - i);
        result.AddRange(_engine.ProcessBlock(data, i, currentSize));
    }
    return result.ToArray();
}

Otherwise your code seems to look good.

Regarding your Unsure about the certinaty parameter See https://stackoverflow.com/a/3087161/2655508

  The RSA key generation requires prime numbers. However, it's impossible to generate absolute prime numbers. Like any other crypto libraries, BC uses probable prime numbers. The certainty indicate how certain you want the number to be prime. Anything above 80 will slow down key generation considerably.


Gathering metadata on files in GitHub I am a relatively new programmer and am currently working on something with the egit GitHub client library which requires me to iterate over a bunch of values and if a condition is met, add a bunch of stuff to a HashMap then add it to a List.

Example:

List<Map<String, Object>> files = new ArrayList<>();

for (TreeEntry file : fileList) {
  if ("blob".equals(file.getType())) {
    files.add(new HashMap<String, Object>() {
      {
        put("sha", file.getSha());
        put("path", file.getPath());
        put("name", GitHubHelper.getNameFromPath(file.getPath()));
        put("size", Objects.toString(file.getSize()));
        put("commits", GitHubHelper.getCommits(config, repo, file.getPath()));
      }
    });
  }
}

As far as I understand it, this is almost the same as

Map<String, Object> map = new HashMap<>();
map.put(key, value)

with the only exception that the class is anonymous, which in this case doesn't bother me since I'm adding it to a list and returning it anyway.

Questions:

Is the Double-Brace Initialization example different in performance and, in this case in particular, is there any reason why I might choose to do it the regular way? As far as I understand DBI, this should be about the same in terms of performance or am I wrong?
Same as Question #1, however, what if in addition to all of this, in this end, I want to return two things as opposed to one. Does that change anything? In my case, I also want to return the repository object these files belong to.

return new HashMap<Repository, List<Map<String, Object>>>() {
    {
        put(repo, files);
    }
};

To me, the code looks kind of nice like this. But it kind of feels as if this isn't really the right way. Is there any way I can do this better (tidier/more efficient performance-wise)?

Java 8

Since you are on Java 8, you should consider using its stream-based processing feature to achieve what you need in a more expressive way.

First, you need a method that accepts a TreeEntry and gives you the desired Map object:

private Map<String, Object> extractDetails(TreeEntry file) {
    Map<String, Object> map = new HashMap<>();
    map.put("sha", file.getSha());
    map.put("path", file.getPath());
    map.put("name", GitHubHelper.getNameFromPath(file.getPath()));
    map.put("size", Objects.toString(file.getSize()));
    map.put("commits", GitHubHelper.getCommits(config, repo, file.getPath()));
    return map;
}

The idea here is to be able to use this as a method reference, e.g. ThisClass::extractDetails, for map()-ping the filter()-ed elements, before collect()-ing toList():

// Assuming fileList is a Collection implementation
List<Map<String, Object>> files = fileList.stream().filter(f -> "blob".equals(f.getType()))
                                            .map(ThisClass::extractDetails)
                                            .collect(Collectors.toList());

Double brace initialization

This StackOverflow question shows the pitfalls of using it. In short, yes, there will be a performance impact, and it doesn't end there. Due to the anonymous classes it creates, you may get an unusual number of compiled class files, and not to mention the small but significant gotchas surrounding class equivalence testing (potentially) and final classes.


Django controller method to list the top characters, guilds, and killers I've been reading the Chapter 8 of this book.

Here's the view I'm currently coding. It list all characters/guilds according to the type of URL.

/ranking -> List Top 50 Characters

/ranking/guild -> List top guilds

/ranking/pvp -> list top killers

def ranking(request, type):
    if type=='':
        queryset_list = Character.objects.annotate(num_resets=Count('resets')).order_by('-resets').select_related('account')[:50]
        paginator = Paginator(queryset_list, 10)

        page = request.GET.get('page')
        try:
            queryset = paginator.page(page)
        except PageNotAnInteger:
            queryset = paginator.page(1)
        except EmptyPage:
            queryset = paginator.page(paginator.num_pages)
            context = {'object_list' : queryset, 'states':STATES, 'class_type':CLASS_TYPE}
        template = 'ranking_character.html'
    elif type=='guild':
        queryset_list = Guild.objects.all()
        paginator = Paginator(queryset_list, 10)

        page = request.GET.get('page')
        try:
            queryset = paginator.page(page)
        except PageNotAnInteger:
            queryset = paginator.page(1)
        except EmptyPage:
            queryset = paginator.page(paginator.num_pages)

        context = {'object_list' : queryset}
        template = 'ranking_guild.html'

    elif type=='pvp':
        queryset_list = Character.objects.filter(pk_count__gt=0).annotate(total=Count('pk_count')).order_by('-pk_count')[:50]
        paginator = Paginator(queryset_list, 10)

        page = request.GET.get('page')
        try:
            queryset = paginator.page(page)
        except PageNotAnInteger:
            queryset = paginator.page(1)
        except EmptyPage:
            queryset = paginator.page(paginator.num_pages)
            context = {'object_list' : queryset, 'class_type':CLASS_TYPE}
        template = 'ranking_pvp.html'

    return render(request,template,context)

It's possible to make it more "generic"? How can I abstract this different queryset?

I think you can just make a function to get the queryset and re-use it:

def queryset_from_page():
    page = request.GET.get('page')
    try:
        return paginator.page(page)
    except PageNotAnInteger:
        return paginator.page(1)
    except EmptyPage:
        return paginator.page(paginator.num_pages)

Also why is context being defined at different indentation levels?

    except EmptyPage:
        queryset = paginator.page(paginator.num_pages)

    context = {'object_list' : queryset}

vs

    except EmptyPage:
        queryset = paginator.page(paginator.num_pages)

        context = {'object_list' : queryset, 'class_type':CLASS_TYPE}

In one case you define context everytime, other times only when a particular branch in the exception handling is taken: is this a bug?


Calculating array size in C++ This code shows two ways to calculate size of array of any type. I would like to know, which should be preferred? Is there any advantage/disadvantage?

#include <iostream>

// First method
template<typename Arr>
std::size_t array_size_1(const Arr& arr)
{
  return sizeof(arr)/sizeof(arr[0]);
}
// Second method
template<typename T, std::size_t sz>
std::size_t array_size_2(const T (&arr)[sz])
{
  return sz;
}

int main() {
  int arr[] = {1,2,3,4,5};
  std::string sarr[] = {"abc", "def", "ghj"};
  std::cout << array_size_1(arr) << "\n";
  std::cout << array_size_2(sarr) << "\n";
  return 0;
}

Both suffer (equal amounts) from the problem that when you call them, the compiler has to go instantiate a new function from the template. The code all ends up inlined after optimization, but it does make more work for the compiler than something like

#define NELEM(x) (sizeof(x) / sizeof (x)[0])

The benefit of array_size_2 is that if you try to call it as

int *p = nullptr;
std::vector<int> v;
size_t a = array_size_2(p);  // ERROR
size_t b = array_size_2(v);  // ERROR

the compiler will reject the call as ill-formed. Whereas if you do the same thing with array_size_1 or NELEM, you'll just get for example sizeof(int*) / sizeof(int) or sizeof(vector<int>) / sizeof(int), which isn't meaningful.

So array_size_2 saves you from accidentally writing something meaningless. Especially in cases like

int foo(int a[]) {
    return array_size_1(a);  // OOPS!
}

where int a[] is just syntactic sugar for int *a and there's no actual array in existence at all.


Python Conway's Game of life One of my first serious Python programs, realized Conway's Game of Life in Python. Not sure that get_cell_state and update_table functions are good as they might be. Would be glad to see any improvements or critique. Thank you for reviewing ^^.

import time
import random
import copy
import os

black_square_symbol = chr(int('25A0', 16))  # a nice unicode black square symbol

def get_cell_state(table, row, col):
    if row in range(len(table)) and col in range(len(table[0])):
        # little hack to return 0 if row and col are out of rang
        return 1 if table[row][col] else 0
    return 0

def get_neighboring_cells(table, row, col):
    sum = 0
    for row_shift in (-1, 0, 1):
        for col_shift in (-1, 0, 1):
            if row_shift or col_shift:
                #  checking for funcion to not check the state of the cell itself
                sum += get_cell_state(table, row + row_shift, col + col_shift)
    return sum

def generate_table(height, width, rand_seed=time.time()):
    random.seed(rand_seed)  #  giving a seed if user specifies
    table = [[None] * width for _ in range(height)]  # generating the table frame
    for row in range(height):
        for col in range(width):
            table[row][col] = random.choice([True, False])
    return table

def update_table(table, height, width):
    new_table = copy.deepcopy(table)  # deep copy to avoid mutability issues
    for row in range(height):
        for col in range(width):
            neighboring_cells = get_neighboring_cells(table, row, col)
            if neighboring_cells < 2 and table[row][col]:
                # underpopulation
                new_table[row][col] = False
            elif neighboring_cells > 3 and table[row][col]:
                # overpopulation
                new_table[row][col] = False
            elif neighboring_cells == 3 and not table[row][col]:
                # replication
                new_table[row][col] = True
    return new_table

def print_table(table):
    os.system('cls')  # clearing up the screen to print new table
    for row in table:
        for elem in row:
                print(black_square_symbol if elem else ' ', end='')
        print()  # newline

def main():
    os.system('color f0')  # making the background white and text black
    height = int(input('Enter table width: '))
    width = int(input('Enter table height: '))
    rand_seed = input('Enter seed to generate the table(leave blank if dont want to specify')
    if rand_seed:
        table = generate_table(width, height, float(rand_seed))
    else:
        table = generate_table(width, height)
    year = 0
    while True:
        print_table(table)
        print('year:', year)
        year += 1
        table = update_table(table, height, width)
        time.sleep(1)

if __name__ == '__main__':
    main()

Quite nicely structured. some remarks

main() - while you have the guard and also an extra printing function there is a little mess in main(). It has got some game logic (the loop), it has got some UI (input and printing). split it into a run() function with parameters and a interactive main(). parameters to run() should be an initial board and the visualisation callback.
some non-portable OS calls - while acceptable for a test program you should think of a better solution. functions used by an importer should not depent on that. there are also patterns to throw import errors on wrong OS to give correct and immediate information.
seeding - usually shall be done once at program start only. you gave the user the chance to reproduce a board - nicely done. but what about the fallback (which btw. gives a ValueError currently)? What happens if someone imports your module and tries to generate random boards without seed? you will generate identical boards. check why, this is a very important lesson.
deepcopy - you don't need a copy, you want an empty table. this is confusing. implement something like empty_table()


Sorting object with null values underscore sortBy I'm sorting my object by its property ActivityOrder, which will sometimes contain null values if the user has not explicitly stated an order which an activity should appear.

Since null will always appear top most in the sort (unless reversed), it messed up my sort.

The solution I came up with was to sort by the id if activityOrder was null since the ID will always be greater than the activityOrder within our application and then there is some logical order to the sort too.

var data = [{
        "id": 150,
        "name": "Andrew",
        "activityOrder": null
}, {
        "id": 151,
        "name": "Andrew",
        "activityOrder": null
}, {
        "id": 152,
        "name": "Andrew",
        "activityOrder": 1
}];

data = _.sortBy(data, function(o) {
        if (o.activityOrder === null)
            return o.id;
        return o.activityOrder
});

_.each(data, function(x) {
        $('#cnt').append("
" + x.Name + " - Order " + x.activityOrder + "
")
})

I had also tried amending the value if the activityOrder null to 99999999999 and resetting it back to null after doing whatever. I didn't like the idea of this one.

data = _.sortBy(data, function(o) {
        if (o.activityOrder === null)
            o.activityOrder = 99999999999;
        return o.activityOrder
});

//Do whatever

//Reset Activity Order back
_.each(d, function(o) {
        if (o.activityOrder === 99999999999) {
                o.activityOrder = null;
        }
});

How best would you sort an object array by a property which contains null values?

flapdoodle!

if (o.activityOrder === null)
        o.activityOrder = 99999999999;

Dude, don't do this. Really.

Sorting idiom

Javascript supports sorting pretty much as you'll see in other languages.
Collections have sorting.
Sort functions can take a function delegate for customized sorting.
Typically, compare the desired values and return an integer that means less-than, equal-to, greater-than. Typically -1, 0, 1 respectively.
handling null does not require converting it to a valid value in your set domain.
Read the documentation!


Speed up processing between VBA and IE I am looking to speed up the exchange from vba to IE. The sendkeys works, but I was curious if there were a better way to do this?

The site that it routes to is a form, but there is no submit button. The only way to pull the data is to tab to the next box or click somewhere on the screen. I was hoping, however, to have all of this automated through VBA.

Thoughts?

Public Declare Function SetForegroundWindow Lib "user32" (ByVal HWND As Long) As Long

Function FillInternetForm()
Dim HWNDSrc As Long
Dim ie As Object

Set ie = CreateObject("InternetExplorer.Application")
    'create new instance of IE. use reference to return current open IE if
    'you want to use open IE window. Easiest way I know of is via title bar.

HWNDSrc = ie.HWND

ie.Navigate "http://helppointinfo.farmersinsurance.com/OCR/Labor_Rates/laborrates.asp"
    'go to web page listed inside quotes
ie.Visible = True
While ie.Busy
    DoEvents  'wait until IE is done loading page.
  Wend
ie.Document.getElementById("DirectZip").Value = Sheets("NAT").Range("C2").Value

SetForegroundWindow HWNDSrc

Application.SendKeys "{TAB 11}", True
DoEvents
Application.SendKeys "{NUMLOCK}", True

End Function

Public Sub RunRates()

Call FillInternetForm

End Sub

Just reviewing what you've got here...

Indentation

The code would read much better with proper indentation:

Function FillInternetForm()
    Dim HWNDSrc As Long
    Dim ie As Object

    Set ie = CreateObject("InternetExplorer.Application")
    HWNDSrc = ie.HWND

    ie.Navigate "http://helppointinfo.farmersinsurance.com/OCR/Labor_Rates/laborrates.asp"
    ie.Visible = True
    While ie.Busy
        DoEvents  'wait until IE is done loading page.
    Wend
    ie.Document.getElementById("DirectZip").Value = Sheets("NAT").Range("C2").Value

    SetForegroundWindow HWNDSrc

    Application.SendKeys "{TAB 11}", True
    DoEvents
    Application.SendKeys "{NUMLOCK}", True

End Function

Public Sub RunRates()
    Call FillInternetForm
End Sub

Call Instruction

As answered in this StackOverflow question, the Call instrucation is a relic from ancient versions of VB, it's not needed and, IMO, only adds clutter.

Public Sub RunRates()
    FillInternetForm
End Sub

Coupling

The FillInternetForm function is needlessly coupled with the Excel object model - Sheets("NAT").Range("C2").Value should be passed as a String parameter to the function:

Function FillInternetForm(ByVal DirectZipValue As String)
    '...
    ie.Document.getElementById("DirectZip").Value = DirectZipValue
    '...
End Function

Public Sub RunRates()
    FillInternetForm Sheets("NAT").Range("C2").Value
End Sub

Function?

VB functions are procedures with a return value. If it's not specified, then it's returning a Variant - here FillInternetForm is never assigned a return value, and whatever it would be returning wouldn't be used. In other words, you have a procedure (Sub), not a function. The signature should be modified like this:

Public Sub FillInternetForm(ByVal DirectZipValue As String)

I like things explicit - if a member is going to be Private, it needs a Private access modifier; if it's going to be Public, I don't like relying on VB's "defaults", mostly because I code in different languages where these defaults differ (C#). Having explicit access modifiers eliminate the possible confusion, but that might be only me.

Lastly, I don't understand why FillInternetForm would have to press NUM LOCK, this looks misplaced, and has a side-effect that could be surprising to whoever is running that code.


Leap year functionality in Ruby and JavaScript I did a line-by-line translation and am wondering if the JavaScript code looks idiomatic.

Original Ruby code:

class Year
  def initialize(number)
    @number = number
  end

  def leap?
    divisible_by?(400) || ( divisible_by?(4) && !divisible_by?(100))
  end

  private
  def divisible_by?(a_number)
    @number % a_number == 0
  end
end

JavaScript Implementation looks like this:

var Year = function( number ) {
  this.number = number;
  function isLeap() {
    return isDivisibleBy(400) || ( isDivisibleBy(4) && !isDivisibleBy(100));
  }
  function isDivisibleBy(a_number) {
    return this.number % a_number == 0
  }
  return isLeap();
}

Unfortunately, the two pieces of code are not equivalent, so definitely not idiomatic. Javascript does not have classes like other languages, so it has a unique way of creating class equivalents.

Let's look at what your Javascript code does first, so we can understand how to fix it.

var Year = function( number ) {

Going ok so far. We could also simply declare Year as a function: function Year(number) {..., but this is acceptable.

  this.number = number;

Also good. When we say new Year(2014), we'll assign it to the number property of the instance.

  function isLeap() {
    return isDivisibleBy(400) || ( isDivisibleBy(4) && !isDivisibleBy(100));
  }

Ok, so the method body is fine, but this is not being assigned as a method on the object, so we won't be able to do something like yearInstance.isLeap(). Instead, isLeap can only be used from the context of this method.

  function isDivisibleBy(a_number) {
    return this.number % a_number == 0
  }

This is meant to be a private method. Unfortunately, Javascript does not have a traditional concept of private scope. More on this later.

  return isLeap();
}

This return statement is...tricky. In Javascript, we can return an object from a constructor, and the value returned would be that object and not an instance of the function we are instantiating. I'll admit, that's confusing. Here's an example:

function A() {
    return new B();
}

Now, if we call new A(), we actually get back a new B. But there are caveats. If the return value is not an object (so a primitive like a boolean or a number not boxed), then we will not get back the return value, we'll get the object we are trying to instantiate. So now back to your code. From the constructor, we return the result of a call to isLeap. First, isLeap is not being called on any object, so it will always return false. Because false is a primitive, the result is discarded, and we end up exactly with what we expect. Because of the trickiness of Javascript constructors and returns, even if we get what we want, we should still not have a return statement. It is inappropriate in a constructor, and can lead to unexpected bugs. For more, see this StackOverflow answer.

Now we can break this code down to be more idiomatic.

function Year(number) {
    this.number = number;
}

Year.prototype.isLeap = function() {
    return this.isDivisibleBy(400) || (this.isDivisibleBy(4) && !this.isDivisibleBy(100));
}

Year.prototype.isDivisibleBy = function(aNumber) {
    return this.number % aNumber === 0;
}

Here, we've broken out the methods from the constructor. Because Javascript is a prototypical language, we create methods by adding them to the prototype of the constructor, Year.prototype in this case. We see isLeap is now a property on the Year prototype, meaning we can now call it like yearInstance.isLeap(). isDivisibleBy is also a property on the Year prototype. This means that, here, it's a public method as well. Like I said before, Javascript does not have the same concept of private visibility.

We can't make a private method as a property on the object. There are a few ways around this. The simplest is to prepend the method name with an underscore (like Year.prototype._isDivisibleBy). This does not make the method uncallable, but is a common convention to indicate that the method is meant to be private.

An alternate way (which I am not going to show) is to use a closure to hide the function.

Inside isLeap, all of the calls to isDivisibleBy must now be prefixed with this. because it is now a method.

Inside isDivisibleBy, we now see == is replaced with ===. This is not required in any way, and can sometimes boil down to personal preference (see JSLint). Basically, it compares value and type when checking for equality. Also, a_number has been changed to aNumber. Generally, I find that Javascript following similar camelCasing conventions as Java.


Using SendAsync to send multiple emails My method for sending emails with Send() is getting very slow when I send more than ten messages.
Sample

1 (1 sec)
2 (2 sec)
3 (4 sec)
4 (8 sec) and so on...

so I started to try SendAsync()
public Boolean sendSMTPMail(string subject, string body, customer[] recipients, string attachmentFileName = null)
{
    string css = "<style>*{font-family: Arial, Helvetica, sans-serif;}</style>";
    SmtpClient client = new SmtpClient();
    //some client settings..

    MailMessage mm = new MailMessage();
    mm.IsBodyHtml = true;
    mm.From = new MailAddress(MainForm.from, MainForm.fromName, Encoding.UTF8);
    mm.Subject = subject;

    foreach (var recipient in recipients)
    {
        Application.DoEvents();
        mm.To.Clear();
        mm.To.Add(new MailAddress(recipient.getEmail()));
        mm.AlternateViews.Add(getEmbeddedImages(css + doReplacements(body, recipient)));

        try
        {
            //client.Send(mm);
            client.SendAsync(mm, "userToken");
            //client.SendMailAsync(mm);
        }
        catch (Exception)
        {
            return false;
        }
    }
    return true;
}

Additional Information about my methods:
getEmbeddedImages replaces imagePath with needed cid:xxxx and adds the image so it can be sent as a part of the email and not only as attachment.
doReplacements replaces strings in my body to get the email text more recipient specific:
Hello my name is %name%,
I am %age% years old.

will be translated to
Hello my name is John Doe,
I am 100 years old.

Questions:

Is this the right way to do it?
Is it important to send different userTokens?
For what is this token needed anyway? Obviously it doesn't matter what I fill in there.

From the SendAsync docs:

  After calling SendAsync, you must wait for the e-mail transmission to
  complete before attempting to send another e-mail message using Send
  or SendAsync.

So no, this may not work properly. You need to register an event handler on the SmtpClient.SendCompleted event so that you know the message has sent successfully.

I'm going to be frank about this, the SmtpClient has an awful API. Your confusion is not your fault.

Your best bet would be to create a dictionary of recipients to use as a queue.

Send the first message and pass the dictionary key in as the token.
When the message completed event occurs, use the token to remove the recipient from the dictionary and send the next message.
Repeat until the queue is empty.

This is going to be a slow, fairly synchronous process, so you may want to spin up a background thread or task to run this on, if your program has other work to do while this is going on.

I really encourage you to thoroughly read through the documentation I linked to. It's an old part of the framework and not very user (or async) friendly.

Or forget all that and just use the newer SendMailAsync(MailMessage) method.

Just be sure to await the result.

    try
    {
        await client.SendMailAsync(mm);
    }

Which means that you'll need to modify the signature of your method to be compatible with async/await.


LeetCode 149: Max Points on a Line I'm posting my code for a LeetCode problem copied here. If you would like to review, please do so. Thank you for your time!
Problem
Given n points on a 2D plane, find the maximum number of points that lie on the same straight line.
Example 1:

Input: [[1,1],[2,2],[3,3]]
Output: 3

^
|
|        o
|     o
|  o
+------------->
0  1  2  3  4

Example 2:

Input: [[1,1],[3,2],[5,3],[4,1],[2,3],[1,4]]
Output: 4

^
|
|  o
|     o        o
|        o
|  o        o
+------------------->
0  1  2  3  4  5  6

Inputs
[[1,1],[2,2],[3,3]]
[[0,0],[94911151,94911150],[94911152,94911151]]
[[1,1],[2,2],[3,3],[131,131],[3111,3111],[31,31],[13,13],[113,11],[5,5]]

Outputs
3
2
8

Code
#include <vector>
#include <map>
#include <utility>
#include <algorithm>

struct Solution {
    static const inline int maxPoints(const std::vector<std::vector<int>> &points) {
        int max_points = 0;

        for (int index_i = 0; index_i < points.size(); index_i++) {
            int overlap = 0;
            int y_on = 0;
            int curr = 0;
            map<pair<int, int>, int> points_map;

            for (int index_j = index_i + 1; index_j < points.size(); index_j++) {
                if (points[index_i] == points[index_j]) {
                    overlap++;

                } else if (points[index_i][0] == points[index_j][0]) {
                    y_on++;

                } else {
                    const int x = points[index_i][0] - points[index_j][0];
                    const int y = points[index_i][1] - points[index_j][1];
                    const int gcd = greatest_common_divisor(x, y);
                    points_map[ {x / gcd, y / gcd}]++;
                    curr = std::max(curr, points_map[ {x / gcd, y / gcd}]);

                }

                curr = std::max(curr, y_on);
            }

            max_points = std::max(max_points, curr + overlap + 1);
        }

        return max_points;
    }

private:
    // std::gcd
    static inline const int greatest_common_divisor(const int a, const int b) {
        if (not b) {
            return a;

        } else {
            return greatest_common_divisor(b, a % b);
        }
    }
};

References

Problem

Discuss

Solution

Code Review

Prefer int to const inline int. See Previous review.

Prefer to pass by const reference

Prefer to put the & next to the type (not the parameter).
Its part of the type information (note this is the opposite of C).
  static const inline int maxPoints(const std::vector<std::vector<int>> &points)

This is absolutely fine (I have not problems with it).
        int max_points = 0;

But most (I have no real metrics for this, but in my experience) tend to use "CamelCase" rather than "Snake_Case" naming convention.

Prefer to use pre-increment rather than post increment
        for (int index_i = 0; index_i < points.size(); index_i++) {

The reason is that C++ code can simply be changed by changing the type used. Now pre/post fix increment on an integer has exactly the same type. But if we change the type used for the iterator then pre/post increment can be slightly different in cost. The standard implementation of iterators (the most common thing used in loops) will give you a slightly more expensive operation (in theory) than a pre-increment. Thus the standard advice is to always use the pre-increment to make sure you always use the most efficient operation.

Be aware of the new range based operator.
Not sure it totally helps here. But keep in mind the range based operator:
        for (int index_i = 0; index_i < points.size(); index_i++) {
            auto& item = points[index_i];

            // STUFF
        }

        // can be simplified to:
        for (auto& item: points) {

            // STUFF
        }

Your assuming that using namespace std; is somewhere above you.
            map<pair<int, int>, int> points_map;

Don't. Even using using namespace std; is considered bad pactice.

Not very common to see the use of not. It is much more common to use !.
        if (not b) {

        ----

        if (!b) {

For most C developers this will be the normal.


Find the closest number in a sorted list to a given target number Given a list of n integers sorted in ascending order, find the closest
  number (value) in the list to a given target number. How quickly can
  you do it?

I'm interested to know if there's any feedback for my code.

def getClosestValue(arr, target):
    n = len(arr)
    left = 0
    right = n - 1
    mid = 0

    # edge case
    if (target >= arr[n - 1]):
        return arr[n - 1]
    # BSearch solution: Time & Space: Log(N)

    while (left < right):
        mid = (left + right) // 2 # find the mid

        if (arr[mid] == target):
            return arr[mid]

        if (target < arr[mid]):
            # If target is greater than previous
            # to mid, return closest of two
            if (mid > 0 and target > arr[mid - 1]):
                return findClosest(arr[mid - 1], arr[mid], target)
            # update right
            right = mid
        else:
            if (mid < n - 1 and target < arr[mid + 1]):
                return findClosest(arr[mid], arr[mid + 1], target)
            # update i
            left = mid + 1
    return arr[mid]

# findClosest
# We find the closest by taking the difference
# between the target and both values. It assumes
# that val2 is greater than val1 and target lies
# between these two.

def findClosest(val1, val2, target):
    if (target - val1 >= val2 - target):
        return val2
    else:
        return val1

# Sample code
arr = [1, 2, 3, 4, 5, 5, 8, 10]
target = 7
print(getClosestValue(arr, target))

Conventions

The Style guide to follow in Python is PEP8. If you do so, your code blends right into the python community and it is immediately easier to follow. On that regard you'll see that the functions name's don't follow the recommended style.

getClosestValue

Should actually be:

get_closest_value

The relevant quote from PEP8 for this case

  Use the function naming rules: lowercase with words separated by
  underscores as necessary to improve readability.

Still in this topic, all the parenthesis used in the ifs are redundant and break the style.

if (target >= arr[n - 1]):

Should be turned into:

if target >= arr[n - 1]:

Although i suspect that this may be a habit brought from other languages.

Edge cases

You contemplated the case where the value is the highest or above, which gives you a quick way out:

# edge case
if (target >= arr[n - 1]):
    return arr[n - 1]

However there is no corresponding edge case for when the value is the lowest or below. I believe you merely forgot it.

#edge case - first or below all
if target <= arr[0]:
    return arr[0]

Logic

Now both edge cases are covered, so the while can be simplified to only have the navigation to the corresponding item if there is one.

while (left < right):
    mid = (left + right) // 2 # find the mid
    if arr[mid] == target:
        return arr[mid]

    if target < arr[mid]:
        right = mid
    else:
        left = mid + 1

After this last while if there isn't an exact match, calculate the nearest to the one you ended on, and return it:

if target < arr[mid]:
    return find_closest(arr[mid - 1], arr[mid], target)
else:
    return find_closest(arr[mid], arr[mid + 1], target)

Note that i don't need to check if mid - 1 or mid + 1 is a valid index, because if it wasn't it would mean that the value was below or above all elements, and those cases were already covered in the beginning of the function.
This is not only simpler but also more efficient since it only checks for the closest on the very end and not at each iteration.

Taking @bipll suggestion you can restructure the while a bit. Considering you are only getting if arr[mid] == target at the very last iteration you can first check for < or >. This avoids making one extra check that will fail most of the times:

while left < right:
    mid = (left + right) // 2  # find the mid
    if target < arr[mid]:
        right = mid
    elif target > arr[mid]:
        left = mid + 1
    else:
        return arr[mid]

When i have a simple condition with a return on both cases i rather write them inline since its easy to read and a bit more concise:

def find_closest(val1, val2, target):
    return val2 if target - val1 >= val2 - target else val1

But don't go over the top on this. It can easily be harder to read depending on the complexity of the condition.

Full Modified Version

For you to get a better picture of all the changes i mentioned, i'll leave you with the full modified version:

def get_closest_value(arr, target):
    n = len(arr)
    left = 0
    right = n - 1
    mid = 0

    # edge case - last or above all
    if target >= arr[n - 1]:
        return arr[n - 1]
    # edge case - first or below all
    if target <= arr[0]:
        return arr[0]
    # BSearch solution: Time & Space: Log(N)

    while left < right:
        mid = (left + right) // 2  # find the mid
        if target < arr[mid]:
            right = mid
        elif target > arr[mid]:
            left = mid + 1
        else:
            return arr[mid]

    if target < arr[mid]:
        return find_closest(arr[mid - 1], arr[mid], target)
    else:
        return find_closest(arr[mid], arr[mid + 1], target)

# findClosest
# We find the closest by taking the difference
# between the target and both values. It assumes
# that val2 is greater than val1 and target lies
# between these two.
def find_closest(val1, val2, target):
    return val2 if target - val1 >= val2 - target else val1


Reading OS X file tags I'm investigating Go for use in our internal network / office project management system, and need a way to check and edit the color tags that you can set in Finder on files or folders (extended attributes, xattrs).  This is my first Go code.  Is this a reasonable way to go about it, and is the interface something that makes sense?

Using a const() "fake enum" type thing, with a type TagColor like this - is this going to annoy users who just want a straight int, or is it actually helpful for keeping things typed?  Likewise putting a String() method for the type - I find it pretty helpful, but is this just making things needlessly bloated?

I want it to be a friendly interface to work with, and not be over-engineered or over-complex.

This is currently just this one file:

/*
 * OSX Tags
 * (C) 2014 Daniel Fairhead
 * MIT Licence
**/

package osxtags

import "fmt"

import "github.com/AlexSi/xattr"
import "howett.net/plist"

type TagColor int

const (
    None   TagColor = 0
    Gray   TagColor = 1
    Green  TagColor = 2
    Purple TagColor = 3
    Blue   TagColor = 4
    Yellow TagColor = 5
    Red    TagColor = 6
    Orange TagColor = 7
)

func (t TagColor) String() string {
    switch t {
    case None:
        return "None"
    case Gray:
        return "Gray"
    case Green:
        return "Green"
    case Purple:
        return "Purple"
    case Blue:
        return "Blue"
    case Yellow:
        return "Yellow"
    case Red:
        return "Red"
    case Orange:
        return "Orange"
    }
    return "unknown"
}

type ColorTag struct {
    Color TagColor
    Name  string
}

func GetColors(filename string) ([]ColorTag, error) {

    // pull out the raw metadata:

    colormeta, err := xattr.Get(filename, "com.apple.metadata:_kMDItemUserTags")

    if err != nil {
        return nil, err
    }

    // decode it from a binary plist:

    colList := []string{}

    _, err = plist.Unmarshal(colormeta, &colList)

    if err != nil {
        return nil, err
    }

    // split the stupid "\n" items into (color, name) pairs.

    toReturn := make([]ColorTag, len(colList), len(colList))

    for i, col := range colList {
        fmt.Sscanf(col, "%s\n%d", &toReturn[i].Name, &toReturn[i].Color)
    }

    return toReturn, nil

}

Using a custom type (with underlying type int) and using const to define the predefined colors is the perfect way to go.

If a user would want to use int, it is still possible by using a simple type conversion:

var myIntColor = 1 // Gray
var mycolor TagColor = TagColor(myIntColor)

Although by not using the predefined constants, you increase the possibility of errors (as the compiler will not be able to provide compile-time checks).

Note that your constant declaration can be a lot more compact and idiomatic by using the iota predeclared identifier which is auto-incremented, so you can use = iota to give value to all your colors. And also quoting from the spec: Constant declarations:

  Within a parenthesized const declaration list the expression list may be omitted from any but the first declaration. Such an empty list is equivalent to the textual substitution of the first preceding non-empty expression list and its type if any. Omitting the list of expressions is therefore equivalent to repeating the previous list.

This means you only have to write the initialization value for the first constant, and if you omit the rest, the same will be used for the rest (but iota will be auto-incremented of course). So your constant declaration can be this:

const (
    None TagColor = iota
    Gray
    Green
    Purple
    Blue
    Yellow
    Red
    Orange
)

Specifying a String() method for your type is also recommended, as many of the standard library functions check and use that if provided, for example the fmt package when printing values:

col := Orange
fmt.Println(col) // Will print "Orange"

The fmt package checks if the type of the value you passed has a String() method, and if it does, it will be called and its result will be printed.

Also note that you can "automatically" generate the String() method for your constants with go generate. There is a stringer tool which can be used with go generate and it generates a .go source file doing more or less what you did, but with a more efficient code. You can read more about go generate in the blog post Generating code.