Rock, Paper Scissors Game After spending a few months learning python I decided to build a basic Rock Paper Scissors game.

Basic construct of the game:

Allows you to pick best of 3,5,7
Allows you to put in the name
Computer choices are completely random

I know there are probably a million different ways to write this code. But based on the way that I built it, are there any ways to make my code more efficient?

# Rock Paper Scissors

import random as rdm

print("Welcome to Rock/Paper/Scissors!!! \n")

gl = input("Would you like to play a best of 3, 5 or 7: ")
while gl not in ["3", "5", "7"]:
    gl = input("Incorrect Response, please select 3, 5, or 7: ")

gl = int(gl)

human_1 = input("Please enter your name: ")

GameOptions = ['Rock', 'Paper', 'Scissors']

hmn_score = 0
cpt_score = 0

rps_running = True

def rps():
    global cpt_score, hmn_score
    while rps_running:
        hmn_temp = input("""Please select from the following:
                                            1 - Rock
                                            2 - Paper
                                            3 - Scissors
                        \n""")
        while hmn_temp not in ["1", "2", "3"]:
            print("That was not a acceptable input!")
            hmn_temp = input("""Please select from the following:
                                    1 - Rock
                                    2 - Paper
                                    3 - Scissors
                \n""")

        hmn_final = int(hmn_temp) - 1

        print('You Chose: ' + GameOptions[hmn_final])

        cpt = rdm.randint(0, 2)

        print('Computer Chose: ' + GameOptions[cpt] + '\n')

        if hmn_final == cpt:
            print('Tie Game!')

        elif hmn_final == 0 and cpt == 3:
            print('You Win')
            hmn_score += 1
        elif hmn_final == 1 and cpt == 0:
            print('You Win')
            hmn_score += 1
        elif hmn_final == 2 and cpt == 1:
            print('You Win')
            hmn_score += 1
        else:
            print('You Lose')
            cpt_score += 1

        game_score()
        game_running()

def game_score():
    global cpt_score, hmn_score
    print(f'\n The current score is {hmn_score} for you and {cpt_score} for the computer \n')

def game_running():
    global rps_running, gl
    if gl == 3:
        if hmn_score == 2:
            rps_running = False
            print(f"{human_1} Wins!")

        elif cpt_score == 2:
            rps_running = False
            print(f"Computer Wins!")

        else:
            rps_running = True

    elif gl == 5:
        if hmn_score == 3:
            rps_running = False
            print(f"{human_1} Wins!")
        elif cpt_score == 3:
            rps_running = False
            print(f"Computer Wins!")
        else:
            rps_running = True

    elif gl == 7:
        if hmn_score == 4:
            rps_running = False
            print(f"{human_1} Wins!")
        elif cpt_score == 4:
            rps_running = False
            print(f"Computer Wins!")
        else:
            rps_running = True

rps()

Names

I've renamed:

hmn_* -> human_*
cpt_* -> computer_*
g1 -> max_score
human_1 -> human_name
game_score -> print_scores
game_running -> check_scores
rps -> start
rps_running -> running
rdm -> random
GameOptions -> GAME_OPTIONS (we usually use pascal case for a class in Python, and capitalization for variable constants)

Reason for all this renaming was to make it clear to an outside reader what all the variables mean without having to look past their declaration. If you ever revisit this code, you don't want to have to dive deep into it every time.

As for renaming the functions, we are now able to tell what they do without diving into their bodies. For example, print_scores clearly tells us that it prints the status of the scores.

check_scores

Currently you have three different outer ifs corresponding to max_score equaling 3, 5 or 7. In each of these ifs, you check if human_score or computer_score are greater than half of the total possible score. This entire function can be simplified by making this comparison work for any value of max_score:

def check_scores():
    global running, max_score

    if human_score > max_score / 2:
        running = False
        print(f"{human_1} Wins!")

    elif computer_score > max_score / 2:
        running = False
        print("Computer Wins!")

Since check_scores can only be possibly called if running == True, we don't need to reassign it to True in the else, so we can get rid of that.

start

You can make the input call for human_temp a function such that the prompt isn't specified twice in the code:

def get_human_temp():
    return input("""Please select from the following:
                        1 - Rock
                        2 - Paper
                        3 - Scissors
        \n""")

This changes the human_temp while loop to:

human_temp = get_human_temp()

while human_temp not in ["1", "2", "3"]:
    print("That was not a acceptable input!")
    human_temp = get_human_temp()

Ah, this is a do-while loop! If you're on Python 3.8, you can use the walrus operator:

while (human_temp := get_human_temp()) not in ["1", "2", "3"]:
    print("That was not a acceptable input!")

For your entire chain of elifs checking whether the human won, a more concise way to do is it to check if human_final - 1 == computer. This works for any human_final except 0. To make it work for 0, we would need to check human_final + 2 == computer. We can combine these two checks concisely as follows:

if human_final == computer:
    print('Tie Game!')

elif computer in (human_final - 1, human_final + 2):
    print('You Win')
    human_score += 1

else:
    print('You Lose')
    computer_score += 1

I believe that elif human_final == 0 and computer == 3 was a subtle bug in your original code, computer should have been checked against 2.

You can think of the elif as checking whether human_final is one ahead of computer in GAME_OPTIONS, while accounting for wrapping around GAME_OPTIONS.

Global State

There's a lot of global state in your program (show by all the global calls). We can use a class to store the state for each specific game. This state includes max_score (describes when a particular game ends), human_name (describes who is playing the game) and human_score / computer_score / running (describes current state of the game). Let's call this class Game, with an initialization method like this:

def __init__(self, max_score, human_name):
    self.max_score = max_score
    self.human_name = human_name

    self.human_score = 0
    self.computer_score = 0
    self.running = False

We would then put all of your methods using global state in Game, with self prepended to all the variables we have here in our __init__ method.

As for the code that is run before we even start the game (the code responsible for fetching human_name and max_score), we can put this in an if __name__ == "__main__" block. This makes it so we can use Game from another module without having all the input-specific code running.

Here's the final code, with some very slight consistency changes (such as standardizing the type of quotation mark you use, and getting rid of unnecessary fs at the beginning of unformatted strings):

# Rock Paper Scissors

import random

def get_human_temp():
    return input("""Please select from the following:
                        1 - Rock
                        2 - Paper
                        3 - Scissors
        \n""")

GAME_OPTIONS = ["Rock", "Paper", "Scissors"]

class Game:
    def __init__(self, max_score, human_name):
        self.max_score = max_score
        self.human_name = human_name

        self.human_score = 0
        self.computer_score = 0
        self.running = False

    def print_scores(self):
        print(f"\n The current score is {self.human_score} for you and {self.computer_score} for the computer \n")

    def check_scores(self):
        if self.human_score > self.max_score / 2:
            self.running = False
            print(f"{self.human_name} Wins!")

        elif self.computer_score > self.max_score / 2:
            self.running = False
            print("Computer Wins!")

    def start(self):
        self.running = True

        while self.running:
            while (human_temp := get_human_temp()) not in ["1", "2", "3"]:
                print("That was not a acceptable input!")

            human_final = int(human_temp) - 1
            print(f"You Chose: {GAME_OPTIONS[human_final]}")

            computer = random.randint(0, 2)
            print(f"Computer Chose: {GAME_OPTIONS[computer]}\n")

            if human_final == computer:
                print("Tie Game!")

            elif computer in (human_final - 1, human_final + 2):
                print("You Win")
                self.human_score += 1

            else:
                print("You Lose")
                self.computer_score += 1

            self.print_scores()
            self.check_scores()

if __name__ == "__main__":
    print("Welcome to Rock/Paper/Scissors!!! \n")

    max_score = input("Would you like to play a best of 3, 5 or 7: ")

    while max_score not in ["3", "5", "7"]:
        max_score = input("Incorrect Response, please select 3, 5, or 7: ")

    max_score = int(max_score)
    human_name = input("Please enter your name: ")

    game = Game(max_score, human_name)
    game.start()


Passwords storage class I'm writing a tool, to manage our application, specifically to store passwords in encrypted view. I made a class which is based on two posts:

https://stackoverflow.com/questions/20852664/python-pycrypto-encrypt-decrypt-text-files-with-aes

and:

https://stackoverflow.com/questions/4102761/python-config-parser-cache-to-reduce-i-o-on-an-embedded-system

#!/usr/bin/env python

import os

import StringIO
import hashlib

from Crypto import Random
from Crypto.Cipher import AES

from lib.shared import ConfigParser

class RDSCryptor(object):

    def __init__(self, rdsmanager_local_path):

        password = 'password'
        self.key = hashlib.sha256(password).digest()

        self.passfile_enc = os.path.join(rdsmanager_local_path, 'conf', 'credentials.txt.enc')
        self.passfile_clear = os.path.join(rdsmanager_local_path, 'conf', 'credentials.txt')

    def pad(self, s):

        return s + b"\0" * (AES.block_size - len(s) % AES.block_size)

    def encrypt(self, message):

        message = self.pad(message)
        iv = Random.new().read(AES.block_size)
        cipher = AES.new(self.key, AES.MODE_CBC, iv)

        return iv + cipher.encrypt(message)

    def decrypt(self, ciphertext):

        iv = ciphertext[:AES.block_size]
        cipher = AES.new(self.key, AES.MODE_CBC, iv)
        plaintext = cipher.decrypt(ciphertext[AES.block_size:])

        return plaintext.rstrip(b"\0")

    def encrypt_file(self):

        with open(self.passfile_enc, 'rb') as fo:
            plaintext = fo.read()
        enc = self.encrypt(plaintext)

        with open(self.passfile_enc + '.enc', 'wb') as fo:
            fo.write(enc)

    def decrypt_file(self):

        with open(self.passfile_enc, 'rb') as fo:
            ciphertext = fo.read()

        dec = self.decrypt(ciphertext)

        return dec

    def get_credentials(self, section, option):

        buf = StringIO.StringIO(self.decrypt_file())

        config = ConfigParser.ConfigParser()
        config.readfp(buf)

        return config.get(section, option)

Cleartext file (it's not stored in project's repository) looks like:

[cloudlibrary]
clc_user = username
clc_password = password

[kantar_smtp]
smtpconnect_user = username
smtpconnect_password = password

Thet - this class used in main tool's script with:

...
# Cloudlibrary access data
crypto = RDSCryptor(rdsmanager_local_path)

clc_user = crypto.get_credentials('cloudlibrary', 'clc_user')
clc_password = crypto.get_credentials('cloudlibrary', 'clc_password')

# Sendmail credentials
smtpconnect_user = crypto.get_credentials('kantar_smtp', 'smtpconnect_user')
smtpconnect_password = crypto.get_credentials('kantar_smtp', 'smtpconnect_password')
...

All I want is to hide plaintext from the source code - it's not intended to be super-secure against hackers.

What is wrong or can it be done better here?

You don't need to declare password to just use on the next line and never again. Use the literal directly:

self.key = hashlib.sha256('password').digest()

You have some confusing names, like s and iv. Renaming these to be more explicit, like string for s would make it more readable. As would adding docstrings or (less preferably) comments to explain the context of what each function is exactly. At the very least your overall class should have a docstring for a user to know about the general practice of the class.


LeetCode: Intersection of two arrays ii C# https://leetcode.com/problems/intersection-of-two-arrays-ii/

Please review for performance, I am having a problem with using TryGetValue, if someone can explain how this can be used in here.

  Given two arrays, write a function to compute their intersection.

Example 1:

Input: nums1 = [1,2,2,1], nums2 = [2,2]
Output: [2,2]
Example 2:

Input: nums1 = [4,9,5], nums2 = [9,4,9,8,4]
Output: [4,9]
Note:

  Each element in the result should appear as many times as it shows in
  both arrays. The result can be in any order.

using System.Collections.Generic;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace ArrayQuestions
{
    /// <summary>
    /// https://leetcode.com/problems/intersection-of-two-arrays-ii/
    /// </summary>
    [TestClass]
    public class IntersectionOfTwoArraysii
    {
        [TestMethod]
        public void IntersectionOfDouble2Test()
        {
            int[] nums1 = {1, 2, 2, 1};
            int[] nums2 = {2, 2};
            CollectionAssert.AreEqual(nums2, Intersect(nums1,nums2));
        }

        public int[] Intersect(int[] nums1, int[] nums2)
        {
            if (nums1.Length > nums2.Length)
            {
                return Intersect(nums2, nums1);
            }
            Dictionary<int, int> nums1Count = new Dictionary<int, int>();
            List<int> res = new List<int>();
            foreach (var num in nums1)
            {
                if (!nums1Count.ContainsKey(num))
                {
                    nums1Count.Add(num,1);
                }
                else
                {
                    nums1Count[num]++;
                }
            }

            foreach (var num in nums2)
            {
                if (nums1Count.ContainsKey(num) && nums1Count[num] > 0)
                {
                    res.Add(num);
                    nums1Count[num]--;
                }
            }

            return res.ToArray();
        }
    }
}

if (!nums1Count.ContainsKey(num))
            {
                nums1Count.Add(num,1);
            }
            else
            {
                nums1Count[num]++;
            }

I think it can be written as:

nums1Count.TryGetValue(num, out int count);
nums1Count[num] = count + 1;

You don't have to test the return value of TryGetValue() in this case, because TryGetValue() sets count to default (0) if it returns false.

The loop could also be written as a LINQ sequence as:

var nums1Count = data.GroupBy(i => i).ToDictionary(gr => gr.Key, gr => gr.Count());

Likewise:

            if (nums1Count.ContainsKey(num) && nums1Count[num] > 0)
            {
                res.Add(num);
                nums1Count[num]--;
            }

can be changed to:

if (nums1Count.TryGetValue(num, out int count) && count > 0)
{
  res.Add(num);
  nums1Count[num]--;
}


SQL paged search results I'm trying to figure out a way to simplify implementing paged searching in sql server.

My restrictions are:

has to be in a proc.
can't use dynamic sql.

Here's my sample query:

DECLARE @TenantId int
DECLARE @MemberStatusId int
DECLARE @SearchTerm varchar(50)
DECLARE @ColumnName varchar(30)
DECLARE @PageNum int
DECLARE @RecCount int

;
WITH RowPages AS (
    SELECT TOP( @PageNum * @RecCount )
            RowNumber = ROW_NUMBER() OVER( ORDER BY
                CASE
                    WHEN @ColumnName = 'Name' THEN P.LastName
                    WHEN @ColumnName = 'PhoneNumber' THEN M.PhoneNumber
                    WHEN @ColumnName = '' THEN P.LastName
                END ASC,
                CASE
                    WHEN @ColumnName = 'DateAdded' THEN P.DateAdded
                END DESC
                ),
            TotalCount = COUNT(M.MemberId) OVER ( PARTITION BY NULL),
            M.MemberId, M.ExternalId, M.PhoneNumber, P.FirstName, P.LastName, P.EmailAddress
        FROM membership.Members M (nolock)
            INNER JOIN core.Persons P (nolock) on (P.PersonId = M.MemberId)
        WHERE (M.TenantId = @TenantId)
          AND ((@MemberStatusId = 0) or (M.MemberStatusId = @MemberStatusId))
          AND (
                (@SearchTerm = '""')
                OR (CONTAINS( (P.FirstName, P.LastName, P.EmailAddress), @SearchTerm))
            )
)
SELECT TotalCount, MemberId, ExternalId, PhoneNumber,
        FirstName, LastName, EmailAddress
    FROM RowPages
    WHERE RowNumber > ((@PageNum - 1) * @RecCount)
    ORDER BY RowNumber

What this does is perform a paged search of a couple tables.  You can pass in the page number and # records per page as well as the column to sort by and it has a full text search component.  This is one of the simpler queries of this style we have.

What I'm trying to figure out is how to optimize this.  The biggest costs in the execution plan are in regards to the full text search.  There is a table value function [FullTextMatch] which has a cost of 100%.

Ideas?

It looks pretty good to me. With a cost of 100% on the FullTextMatch, I'd consider running this as a standard statement and hard code the variables just to see what the execution plan says.
I do have one question though. Your case statement for @ColumnName = 'DateAdded'... doesn't this throw an error when @ColumnName <> 'DateAdded' as you end up with something like:

order by P.LastName asc, desc


Removing a specific character and its neighbors from a string Assume we have an string that contains the character 'c'. This character can be located anywhere within the string. The exercise is to remove 'c', and the characters that, if they exist, are on its immediate left or right. I have devised an algorithm, assuming that 'c' = '*'. Its flowchart can be found here and the code is as follows:

In C#:

class Program
{
    static void Main(string[] args)
    {
        string input = Console.ReadLine();

        Console.WriteLine(RemoveAsterix(input));
        Console.ReadKey();
    }

    static string RemoveAsterix(string myString)
    {
        if (!(myString.Contains('*')))
        {
            throw new Exception("You need an asterix!");
        }

        List<char> myStringSplit = myString.ToCharArray().ToList();

        if (myStringSplit[0] == '*')
        {
            myStringSplit.RemoveRange(0, 2);

        }

        if (myStringSplit[myStringSplit.Count - 1] == '*')
        {
            myStringSplit.RemoveRange(myStringSplit.Count - 2, 2);

        }

        for (int i = 1; i < myStringSplit.Count - 1; i++)
        {
            if (myStringSplit[i] == '*')
            {
                myStringSplit.RemoveRange(i - 1, 3);
                break;
            }
        }

        return new string(myStringSplit.ToArray());

    }
}

And in Python:

def RemoveAsterix(myString):
    myStringList = list(myString)

    if '*' not in myString:
        raise ValueError("String must contain asterix")

    if myStringList[0] == '*':
        del myStringList[0:2]

    if myStringList[-1] == '*':
        del myStringList[-2:]

    for i in range(1, len(myStringList) - 2):
         if myStringList[i] == '*':
             del myStringList[i-1:i+2]
             break;

    return ''.join(myStringList)

def Main():
    print(RemoveAsterix(input("Enter a string: ")))

Main()

Please also review my algorithm.

Generalise

Having a method RemoveAsterix isn't a scaleable approach to solving the general case.  A better method might be:

static string RemoveLettersSurroundingCharacter(string sourceString, char charToRemove)

Passing in the character to remove also helps you to remove a lot of the hard coded * references in your code.

Variable names

Avoid names that start with 'my'.  It reads very much like example code that isn't meant for production, although some people also use it as a naming convention when referring to member variables of a class (which you aren't).

Algorithm

As has already been said in the comments, your algorithm is quite naive.  It works for simple cases, but doesn't seem to have been tested for many edge cases that you might expect to encounter.

**1         => 1
***         => Crash
1234***567  => 123*567
1234**      => 1234
ab*cd*ef    => ad*ef

Your algorithm will remove any leading *, any trailing * and the first instance of * outside of these positions.  This leaves its processing in a bit of a weird state of completeness.  If you only want to process a single *, then you should check to ensure that the string only contains a single * and error if it doesn't.  If you want to support removal of any number of *, then you need to think about what to do when you encounter two * next to each other.  Does the second one get ignored (because it has been removed when the first is encountered) or does it extend the removal.  This is the difference between: 12**34 turning into 134 or 14.

Use specific exceptions where available

You want to throw the most specific exception that you can.  Generally speaking, you should avoid throwing and catching base exceptions.  So, rather than:

throw new Exception("You need an asterix!");

It would be better to:

throw new ArgumentException("You need an asterix!", "myString");


Accessing a subset of keys from a nested dictionary in Python I am trying to access data from Coinmarketcap API in Python and came up with the following code:

def fetch_coin_prices(**kwargs):
    """Retrieve cryptocurrency data from CoinMarketCap and return a dictionary
    containing coin names with their current prices.

    Keyword arguments to this function are mapped to the CoinMarketCap API,
    refer to their documentation for their meaning:
    https://coinmarketcap.com/api/
    """
    response = requests.get(
        'https://api.coinmarketcap.com/v2/ticker/',
        params=kwargs
    )
    response.raise_for_status()
    coin_data = response.json()

    return coin_data.get('data', {}).values()

data = fetch_coin_prices(limit=100, start=0, sort='id')

I am trying to write this data to a Redis HMSET that accepts a dictionary and I only want the properties 'id', 'name', 'symbol', 'rank', 'price' and 'volume_24h'. The price and volume properties are nested and therefore I haven't found a way to get everything in 1 go. My redis hmset needs to store data in key value form where key is the coin id and value is the CSV of the current coin.

hmset = {}
fieldnames = ['id', 'name', 'symbol', 'rank' ,'price', 'volume_24h']
for row in data:
    subset = {}
    subset['id'] = row.get('id', None)
    subset['name'] = row.get('name', None)
    subset['symbol'] = row.get('symbol', None)
    subset['rank'] = row.get('rank', None)
    subset['price'] = row.get('quotes', {}).get('USD', {}).get('price', None)
    subset['volume_24h'] = row.get('quotes', {}).get('USD', {}).get('volume_24h', None)

    if all([subset[key] for key in subset]):
        csv_string = io.StringIO()
        csv_writer = csv.DictWriter(csv_string, fieldnames=fieldnames, extrasaction='ignore' , lineterminator=':')
        csv_writer.writerows(data_subset)
        hmset[subset['id']] = csv_string.getvalue()

This is what I have come up with so far but it looks very ugly in my opinion to access keys. Is there a better way to do this?

I previously have written a helper for this, as I've come across it too. If you pass the object and the keys to the object then you can do the same.

def get_nested(obj, keys)
    try:
        for key in keys:
            obj = obj[key]
    except KeyError:
        return None
    return obj

This makes usage:

subset['price'] = get_nested(row, 'quotes USD price'.split())

This allows you to extend the keys argument to a minilanguage for other common methods you'd want to apply to returned objects. One case I had was since JavaScript doesn't like maps much (not objects) some APIs tend to  convert dictionaries to arrays, and so I had to search a list of objects for which object I wanted, as I couldn't index the list as I'd get incorrect data at times.


How clear is this Ruby code? Elsewhere, there was a question about finding an elegant solution to a particular problem, and the following solution was presented.

I'm curious whether this solution is elegant from the perspective of easy to understand for a Ruby programmer (which I am no longer).

merged_file = File.open("merge_out.txt", "w")

files = ARGV.map { |filename| File.open( filename, "r") }

lines = files.map { |file| file.gets }

while lines.any?
    next_line = lines.compact.min
    file_id = lines.index( next_line )
    merged_file.print next_line
    lines[ file_id ] = files[ file_id ].gets
end

The question is not whether it is efficient, but just "how long does it take a Ruby programmer to understand what this does?"

I had no trouble figuring it out what it does, but I'm not sure I'd call it "easy to understand".

All the declarations are implicit and thus if you're familiar with all the in's and outs of Array and the map method, it's pretty straight forward what the code is attempting to do. Some of the logic takes a bit of thinking about,

I did puzzle a bit over this line

file_id = lines.index( next_line)

I think the most puzzling part is the unstated requirement that the input files are already sorted. Otherwise this line doesn't work. Once I figured out that this line meant that the input files were sorted, the rest fell into place.

The exit condition is also a bit tricky to figure out.

The code really feels like a translation to ruby from a more explicitly functional language, it doesn't feel "ruby-like" at all.


BadNeighbors - Topcoder puzzle I'm learning Dynamic Programming, following the topcoder guide. I just solved the BadNeighbors puzzle. It passes all the test cases, but I don't have much experience, so I don't know if it's good.

  Puzzle Description:

  Each of the town's residents is willing to donate a certain amount, as specified in the int[] donations, which is listed in clockwise order around the well. However, nobody is willing to contribute to a fund to which his neighbor has also contributed. Next-door neighbors are always listed consecutively in donations, except that the first and last entries in donations are also for next-door neighbors. You must calculate and return the maximum amount of donations that can be collected.

def circle(lst):
    if len(lst) == 1:
        return lst[0]
    attempt_1 = _circle(lst, 0, len(lst)-1)
    attempt_2 = _circle(lst, 1, len(lst))
    return max(attempt_1, attempt_2)

def _circle(lst, start, end):
    prev, best = 0, 0
    for i in xrange(start, end):
        val = lst[i]
        current = val + best
        best = max(prev, best)
        prev = current
    return best

Edit: Check the comments of my selected answer for the full resolution.

Your code doesn't actually work for some tricky corner cases. Take e.g. [1, 10, 1, 1, 9]. It should be pretty obvious that 19 is the proper answer, but your code returns 11. To build a working solution, let's leave the circularity out, you have already figured how to get the result for a circular array running twice over a linear array.

As ususal with DP we will consider the array incrementally, and in an array dp[j] we will store the value of the maximum donation from the first j neighbors when the j-th neighbor is one of the donors. It is relatively straightforward to see that then:

dp[j+1] = max(dp[:j]) + lst[j+1]

Note that the max excludes the value over dp[j], as Python slices are right-exclusive.

We can turn this idea into code doing something like:

def mncs(lst):
    # MaximumNonContiguousSum
    dp = [0, 0]
    for item in lst:
        dp.append(max(dp[:-1]) + item)
    return max(dp)

This has a dreadful performance, \$O(n)\$ space and \$O(n^2)\$ time, because we are continuously rescanning the full dp array for the maximum. If we are only after the value of the maximum, and not the indices of the neighbors that produce it, we can minimize the overhead by keeping three numbers only, the previous maximum plus the last two entries:

def mncs(lst):
    dp = [0, 0, 0]
    for item in lst:
        m = max(dp[:-1])
        dp = [m, dp[-1], m + item]
    return max(dp)

Since we do constant work per entry, and keep a constant number of values as state, this solution uses \$O(1)\$ space and takes \$O(n)\$ time.

For completeness:

def mnccs(lst):
    # MaximumNonContiguousCircularSum
    return max(mncs(lst[1:]), mncs(lst[:-1]))


Test whether or not a 4x4 matrix is a magic square Here is what I have. I have it outputting most of the sums just so I can check there values. I think the problem is with the value of the elements in the array storing the column sums. I would greatly appreciate any feedback.

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

void fillMatrix(int matrix[][4],const int SIZE);
int rowSum(int matrix[][4],const int SIZE,int row[]);
int columnSum(int matrix[][4], const int SIZE, int column[]);
bool isMagic(int matrix[][4], const int SIZE,int row[],int column[]);

int main()
{
const int SIZE=4;
int matrix[SIZE][SIZE];
int row[4],column[4];//arrays to be filled with row and column sums.
char response=0;

cout<<"This program determines whether or not a 4x4 square matrix is a magic square.\n";
do
{
    fillMatrix(matrix,SIZE);
    rowSum(matrix,SIZE,row);
    columnSum(matrix,SIZE,row);

    if(isMagic(matrix,SIZE,row,column))
        cout<<"This is a magic square.\n\n";
    else {
        cout<<"This is not a magic square.\n\n";
    }
    cout<<"To end this program, enter q. To check another matrix, enter any other letter.\n";
    cin>>response;
}while(response!='q'&&response!='Q');

return 0;
}

void fillMatrix(int matrix[][4],const int SIZE)
{
for(int i=0;i<4;i++)
{
    cout<<"Enter four values for row "<<i+1<<".\n";
    for(int j=0;j<4;j++)
    {
        cin>>matrix[i][j];
    }
}
}

int rowSum(int matrix[][4],const int SIZE,int row[4])
{
int i=0;
int rowsum=0;
for(i=0;i<4;i++)
{
    rowsum=0;
    for(int j=0;j<4;j++)
    {
        rowsum+=matrix[i][j];
    }
   row[i]=rowsum;
    cout<<row[i]<<endl;
}
return row[i];

}
int columnSum(int matrix[][4], const int SIZE, int column[4])
{
int j=0;
int columnsum=0;
for(j=0;j<4;j++)
{
    columnsum=0;
    for(int i=0;i<4;i++)
    {
        columnsum+=matrix[i][j];
    }
    column[j]=columnsum;
    cout<<column[j]<<endl;
}
return column[j];
}

bool isMagic(int matrix[][4], const int SIZE,int row[4],int column[4])
{
int rightdiagonalsum=0, leftdiagonalsum=0, check;

for(int i=0;i<4;i++)
{
    rightdiagonalsum+=matrix[i][i];
}
cout<<rightdiagonalsum<<endl;
for(int i=0;i<4;i++)
{
    leftdiagonalsum+=matrix[i][3-i];
}
cout<<leftdiagonalsum<<endl;
for(int i=1;i<4;i++)
{
    if (row[i]==row[i-1])
    {
        check=row[i];
    }
    else {
        return false;
    }
}
for(int j=0;j<4;j++)
{
    if (column[j]!=check)
    {
        cout<<column[j]<<"*****";//For some reason, the value of column[j] is 0.
        return false;
    }
}

if (rightdiagonalsum!=check||leftdiagonalsum!=check)
{
    return false;
}

return true;
}

The size of the matrix is hard-coded all over the place.  You made a good attempt to define const int SIZE = 4 in main(), and you pass SIZE to your functions.  However, you still hard-code int matrix[][4], int row[4], and int column[4] in the function signatures.  In addition, fillMatrix() says "Enter four values for row ".

There are several possible remedies.

The simplest is to #define SIZE 4, and don't bother passing the size around anymore.  It's more of a C-style approach, but at least the size will be centrally defined.
You could store the matrix in a std::array<std::array<int>> (C++11), a std::vector<std::vector<int>>, or a std::valarray<std::valarray<int>>.  Then the helper functions could obtained the dimensions by calling .size().
You could flatten the matrix into a one-dimensional array, and pass the size.
You could define a Matrix class.


DateTime Extensions StartOfWeek, StartOfMonth and WorkingDays I have Developed an Extension for DateTime in order to get the start of the week or start of the month (1-xx-xxxx) given a DateTime, and the working days of a month given a datetime and a list with bank days (Type DayOfWeek)

Any suggestions to improve it?

 public static class DateTimeExtensions
{
    public static DateTime StartOfWeek(this DateTime dt, DayOfWeek startOfWeek)
    {

        int diff = ((dt.DayOfWeek - startOfWeek)) % 7;

        if (startOfWeek == DayOfWeek.Sunday)
            return dt.AddDays((7+(startOfWeek - dt.DayOfWeek)) % 7).Date;
        return dt.AddDays(-1 * diff).Date;
    }

    public static DateTime StartOfMonth(this DateTime dt)
    {
        return new DateTime(dt.Year, dt.Month, 1);
    }

    public static int WorkingDays(this DateTime dt, IList<DayOfWeek> freeDays)
    {
        var workingDays = 0;
        var current = new DateTime(dt.Year, dt.Month, 1);
        var nextMonth = current.AddMonths(1);

        while (current < nextMonth)
        {
            if (!freeDays.Contains(current.DayOfWeek))
                workingDays++;
            current =current.AddDays(1);
        }
        return workingDays;
    }
    public static int WorkingDays(this DateTime dt, IList<DayOfWeek> freeDays, IList<DateTime> bankDays)
    {
        var workingDays = 0;
        var current = new DateTime(dt.Year, dt.Month, 1);
        var nextMonth = current.AddMonths(1);

       while (current < nextMonth)
       {
           if (!freeDays.Contains(current.DayOfWeek) && !bankDays.Contains(current))
               workingDays++;
            current =current.AddDays(1);
    }
    return workingDays;
   }

}

First I'd like to pinpoint few details about methods' interface. You have an overloaded WorkingDays method, what I'd change:

Name does not explain what this functions does, I have to check its signature. Does it count the number of working days? Does it return a list of those days? For which period? Let's call it CountWorkingDaysInMonth() and there will be no need to guess.
Both freeDays and bankHolidays are not working days. There may be more: days company is closed, paid holidays and so on. Surely do not want to add a list for each one. Just take one changing signature to accept IEnumerable<DateTime> (and caller may easily use Enumerable.Concat() to build the right list.
Validate your inputs.

Inside the function I'd try to help the reader using the methods you already defined:

var beginningOfCurrentMonth = dt.StartOfMonth();
var beginningOfNextMonth = beginningOfCurrentMonth.AddMonths(1);

Note the explicative names and the fact that I'm reusing StartOfMonth() instead of duplicating its code (you'll see later why).

Let's introduce a generator (useful if you're working with consecutive dates):

IEnumerable<DateTime> AllDaysInInterval(DateTime start, DateTime end)
{
    Debug.Assert(end > start);

    for (var current = start; current < end; current = current.AddDays(1))
        yield return current;
}

Declare a local function to determine if a day is a working day or not:

bool IsWorkingDay(DateTime day)
    => !freeDays.Contains(day.DayofWeek) && !notWorkingDays.Contains(day);

Now you can count them:

return AllDaysInInterval(beginningOfCurrentMonth, beginningOfNextMonth)
    .Count(IsWorkingDay);

Short, easy to understand and you now created few reusable functions you may need elsewhere. Putting things all together:

public static int CountWorkingDaysInMonth(this DateTime dt,
    IEnumerable<DayOfWeek> freeDays,
    IEnumerable<DateTime> notWorkingDays)
{
    if (freeDays == null)
        throw new ArgumentNullException(freeDays);

    if (notWorkingDays == null)
        throw new ArgumentNullException(notWorkingDays);

    var beginningOfCurrentMonth = dt.StartOfMonth();
    var beginningOfNextMonth = beginningOfCurrentMonth.AddMonths(1);

    return AllDaysInInterval(beginningOfCurrentMonth, beginningOfNextMonth)
        .Count(IsWorkingDay);

    bool IsWorkingDay(DateTime day)
        => !freeDays.Contains(day.DayofWeek) && !notWorkingDays.Contains(day);
}

Now it's time to go back to StartOfMonth(). Is it always 1st the first day of the month? No. Not every date is a valid date in every calendar. You may, or may not, need to care about this but you should clearly document it (and be aware of).

In StartOfWeek() you can make your code easier to read.

int diff = ((dt.DayOfWeek - startOfWeek)) % 7; has unnecessary parenthesis and diff does not say anything about its content. Also it's not used if startOfWeek is Sunday.
-1 * diff should simply be -diff.
IMO (7+(startOfWeek - dt.DayOfWeek)) % 7 is clearer if moved outside, to a local variable.

What to do? Just (temporary!) introduce a local function to calculate the number of days from the beginning of the week:

int NumberOfDaysFromBeginningOfWeek()
{
    if (startOfWeek == DayOfWeek.Sunday)
        return (7 + (startOfWeek - dt.DayOfWeek)) % 7;

    return (dt.DayOfWeek - startOfWeek) % 7;
}

Is it what we want? Doesn't it look too complicate? It does then it's probably wrong. Algorithm must be much simpler than that. Let's first calculate the number of days elapsed from the beginning of the week without any special case:

int daysFromBeginningOfWeek = dt.DayOfWeek - startOfWeek;

What's wrong with that? Well, we're relying with enumeration values but week may begin any day (it depends on the culture). What may happen if our day is before the beginning of the week? Number goes negative. Let's add the number of days in a week and it will be right:

int NumberOfDaysFromBeginningOfWeek()
{
    int count = dt.DayOfWeek - startOfWeek;
    if (count < 0)
        return count + 7;

    return count;
}

Note that I'm simplifying this code with a huge assumption: there are always seven days in a week. It's not true and you should read the value from the calendar. Final code will be slightly more complicate than this but I guess you've got the idea. Putting things together:

public static DateTime StartOfWeek(this DateTime dt, DayOfWeek startOfWeek)
{
    return dt.AddDays(-NumberOfDaysFromBeginningOfWeek());

    int NumberOfDaysFromBeginningOfWeek()
    {
        int count = dt.DayOfWeek - startOfWeek;
        if (count < 0)
            return count + 7;

        return count;
    }
}

In this last case local function is more educational than useful, reader should absolutely feel free (!) to replace it with a local variable (with a proper descriptive name.)

Please note: I wrote all this code here without any testing. Do not underestimate date/time handling and write your own huge, big, extensive unit testing to include all corner cases, calendars and cultures you may think of and, most importantly, never ever make any assumption about dates. If there is a property somewhere (Calendar, Culture or DateTime) then use it.


Redirect for subdomains and secondary domains I've just edited my .htaccess file. All works as it should, but I'm not too familiar with regular expressions and want to make sure that there aren't any unintended consequences.

I want to:

Add www to the domain if it's not there (i.e. domain.com becomes www.domain.com)
redirect from secondary.primary.com to www.secondary.com (the site is a subdomain of another site but the two aren't related and the primary domain shouldn't be included in the URL for the site).
redirect domain.com/staging to staging.domain.com.

Here are my rules:

# Add www if not there
RewriteCond %{HTTP_HOST} ^secondary\.com$ [NC]
RewriteRule (.*) http://www.secondary.com/$1 [R=301,L]

# Prevent search engine from using secondary domain i.e. secondary.primary.com and instead use secondary.com
RewriteCond %{HTTP_HOST} ^(www\.)?secondary\.primary\.com$ [NC]
RewriteRule (.*) http://www.secondary.com/$1 [R=301,L]

# Prevent access to staging through secondary.com/staging and instead use staging.secondary.com
RewriteCond %{HTTP_HOST} ^secondary.com [NC]
RewriteRule ^(.*)$ http://www.secondary.com$1 [L,R=301]
RedirectMatch 301 ^/staging/(.*)$ http://staging.secondary.com/$1

Your third rule…

# Prevent access to staging through secondary.com/staging and instead use staging.secondary.com
RewriteCond %{HTTP_HOST} ^secondary.com [NC]
RewriteRule ^(.*)$ http://www.secondary.com$1 [L,R=301]

… is basically a sloppier version of your first rule with a misleading comment.

The remaining two mod_rewrite rules can be combined as:

# Canonicalize hostname of secondary domain
RewriteCond %{HTTP_HOST} ^$secondary\.com$|^(www\.)?secondary\.primary\.com$ [NC]
RewriteRule (.*) http://www.secondary.com/$1 [R=301,L]

Note that if your site supports HTTPS, the rule would redirect HTTPS requests to unencrypted HTTP.


Static web server with a bit of error handling I am looking for a review on this code, which is a simple static web server, with a small bit of error handling. It's feeling a bit "messy" / "imperative" at the moment, so advice would be welcome.

import Network.Socket hiding (send, sendTo, recv, recvFrom)
import Network.Socket.ByteString
import qualified Data.ByteString.Char8 as C
import qualified Data.ByteString as B
import Data.List
import Control.Concurrent
import Control.Exception (SomeException, try)
import qualified Data.Map.Strict as Map
import System.FilePath.Posix
import System.Directory (doesFileExist)
import Text.Printf

port = 8080
incomingBufferSize = 16384
mimeTypes = Map.fromList [
    (".htm", "text/html"),
    (".html", "text/html"),
    (".js", "application/javascript"),
    (".css", "text/css"),
    (".png", "image/png"),
    (".jpg", "image/jpeg"),
    (".jpeg", "image/jpeg")
  ]
defaultMime = "application/octet-stream"
headerOkText = "HTTP/1.1 200 OK\r\nContent-Type: %s\r\n\r\n"
header404 = "HTTP/1.1 404\r\n\r\n"
header500 = "HTTP/1.1 500\r\n\r\n"

main = do
  sock <- socket AF_INET Stream 0
  setSocketOption sock ReuseAddr 1
  bind sock $ SockAddrInet port iNADDR_ANY
  listen sock sOMAXCONN
  mainLoop sock

mainLoop :: Socket -> IO ()
mainLoop sock = do
  (conn, _) <- accept sock
  forkIO $ handle conn
  mainLoop sock

handle :: Socket -> IO ()
handle conn = do
  incoming <- recv conn incomingBufferSize
  let unsafeLocation = extractLocation incoming
  if isSafeLocation unsafeLocation then
    do
      fileContents <- try $ response unsafeLocation
      send conn $ contentsOr500 fileContents
  else
    send conn $ C.pack header404
  close conn
  where
    extractLocation = C.unpack . C.tail . head . tail . C.split ' '
    isSafeLocation location = not $ ".." `isInfixOf` location

contentsOr500 :: Either SomeException B.ByteString -> B.ByteString
contentsOr500 (Left _) = C.pack header500
contentsOr500 (Right contents) = contents

response :: String -> IO (B.ByteString)
response requestedLocation = do
  exists <- doesFileExist requestedLocation
  if exists then
    do
      file <- B.readFile requestedLocation
      return $ fullResponse file $ takeExtension requestedLocation
    else
      return $ C.pack header404

fullResponse :: B.ByteString -> String -> B.ByteString
fullResponse contents extension = C.pack headerWithMime `B.append` contents
  where
    headerWithMime = printf headerOkText $ mimeForExtension extension
    mimeForExtension = flip (Map.findWithDefault defaultMime) mimeTypes

My initial reaction to this code is that it would be clearer if you bring some functions outside main = do.

One of the great things about Haskell is it's explicit type system and safety; your code doesn't leverage this as there are no function type declarations.


Storing realm objects using Kotlin extensions I am a Java Android Developer and I'm approaching Kotlin.

I have defined the following class:

open class Player : RealmObject() {
    ...
}

And I defined the following two extensions, one for the generic RealmObject class and one for the specific Player class:

fun RealmObject.store() {
    Realm.getDefaultInstance().use { realm ->
        realm.beginTransaction()
        realm.copyToRealmOrUpdate(this)
        realm.commitTransaction()
    }
}

fun Player.store(){
    this.loggedAt = Date()
    (this as RealmObject).store()
}

What I want is if I call .store() on any RealmObject object, the RelamObject.store() extension will be called BUT if i call .store() on a Player instance the extension that will be called will be Player.store().
(No problem for now)
I don't want to copy paste the same code, i love to write less reuse more.
So i need that internally the Player.store() will call the generic RealmObject.store()

I got it. The code I wrote up there is actually working as expected :D

Is this the good way? Or there is some better way?

Please correct me if I got you wrong, but we're talking here about simple inheritance:

open class RealmObject {

    fun store() { /*RealmObject impl*/ }

}

open class Player : RealmObject() {

    override fun store() {
     /*Player impl*/
     super.store() /*RealmObject impl*/

   }
    ...
}

Your wanted behaviour fits:

Player().store() // executes override function - Player impl, then RealmObject impl
RealmObject().store() // executes RealmObject implementation

If you want to keep it as extension functions, your code is valid, but isn't really an eye-candy.

Both classes Player and RealmObject would have the same function store(), but they are 'unrelated' - thats why we don't see any override. Extension functions extend statically and they can shadow your implementation, which makes your code very hard to read and the behaviour becomes at some point not understandable.

Please use extension functions wisely and try to never use it for inheritance.


RAII style API wrapper for PyMongo I just watched Raymond Hettinger's talk on making Python more Pythonic and realized I should be putting a lot of his ideas into practice, particularly wrapping API's in a class that makes everything simpler and easy to use. Here's what I'm doing to wrap PyMongo:

from pymongo import MongoClient

class MongoDB(object):
    """Provides a RAII wrapper for PyMongo db connections.
    Available collection functions limited to those in
    attributes_to_pass. Number of simultaneous connection
    users also tracked. """

    attributes_to_pass = ["update", "insert", "count"]
    client = None
    num_users = 0

    def __init__(self, db, collection):
        MongoDB.client = MongoDB.client or MongoClient()
        self.collection = MongoDB.client[db][collection]
        MongoDB.num_users += 1

    def __enter__(self, *args):
        return self

    def __exit__(self, type, value, traceback):
        MongoDB.num_users -= 1
        if MongoDB.num_users is 0:
            self.client.close()

    def __getattr__(self, attr):
        if attr in MongoDB.attributes_to_pass:
            return getattr(self.collection, attr)
        else:
            return getattr(self, attr)

def main():
    with MongoDB(db = "db1", collection = "c1") as m:
        print(m.count())
        m.update({"jello":5} , {"hello":"you"}, upsert = True)
        print(m.count())
        m.insert({"joe":6})
        with MongoDB(db ='db1', collection = 'c2') as j:
            j.insert({"joe":6})
            print(j.count())

if __name__ == "__main__":
    main()

I would really appreciate all suggestions on how to make this better.

Great that you provided a docstring! You made one minor formatting mistake though, there should be a blank line between your brief single line summary and the rest of the docstring. It's recommended in the style guide partially for readability and partially for script parsers.

class MongoDB(object):
    """Provides a RAII wrapper for PyMongo db connections.

    Available collection functions limited to those in
    attributes_to_pass. Number of simultaneous connection
    users also tracked. """

It seems like attributes_to_pass is actually a constant. If that is the case it should be UPPER_SNAKE_CASE to be clear that it is, especially when mixed with attributes that often change like client and num_users. It should also be a tuple, as tuples are immutable so will not change unless updated in the source code.

ATTRIBUTES_TO_PASS = ("update", "insert", "count")

Also, when you call on these constants you refer to the class name. While that's possible I'd personally say it's less readable. I had thought you were calling on a class you imported for a second. You can just pass self instead of the classname.

def __getattr__(self, attr):
    if attr in self.ATTRIBUTES_TO_PASS:
        return getattr(self.collection, attr)
    else:
        return getattr(self, attr)


Creating a directory structure from JSON I have a JSON file:

{
'Document':[

[{'fields': {'name': 'js/main.js', 'content': 'hello'}, 'pk': 284, 'model': 'Document'}],

[{'fields': {'name': 'css/main.css', 'content': '2'}, 'pk': 287, 'model': 'Document'}],

[{'fields': {'name': 'about_us.html', 'content': 'again hello'}, 'pk': 306, 'model': 'Document'}]],

'Package':
[{'fields': {'package_type': 'THEME', 'base_package': None, 'created_date': '2015-05-25T15:39:16.781Z', 'name': '25_may', 'rating_avg': 0.0, 'user': 2, 'rating_count': 0, 'is_published': True}, 'pk': 129, 'model': 'Package'}]

}

I want to generate directory structure based on JSON data:

25_may
|- css
|  |- main.css
|- js
|  |-main.js
|- about_us.html

Code to achieve above:

def my_func():
    package_name = response['Package'][0]['fields']['name']
    for doc in response['Document']:
        filename = os.path.join(package_name, doc[0]['fields']['name'])
        content = doc[0]['fields']['content']
        if not os.path.exists(os.path.dirname(filename)):
            os.makedirs(os.path.dirname(filename))
        with open(filename, "w") as f:
            f.write(content)

Is there an efficient and better way to do this?

It seems fine, but I suggest to improve some minor improvements:

filename is not a simple file name but has directory element too, so I'd call it path
Instead of calling os.path.dirname twice, I'd call it once and cache the result in a local variable

Like this:

package_name = response['Package'][0]['fields']['name']
for doc in response['Document']:
    path = os.path.join(package_name, doc[0]['fields']['name'])
    basedir = os.path.dirname(filename)
    if not os.path.exists(basedir):
        os.makedirs(basedir)

    content = doc[0]['fields']['content']
    with open(filename, "w") as f:
        f.write(content)

Btw, the JSON structure looks a bit odd,
with several arrays with a single element.
Makes me wonder why these arrays are there at all,
instead of their contained objects directly.

I'd also be concerned about format changes in the JSON:
your code is tightly coupled to an unintuitive structure,
and if anything changes later,
it might be difficult to migrate the implementation.


checking is ellipse is filled or not following code below works fine but feels kind of a mess. any one has a better way to do this
I am creating a basic "Enter Pin" to unclick screen on a app. So if user enter 1st digit, than ellipse1 should be filled. and if user enter 2nd digit, than ellipse1 and ellipse2 should be filled. and if user enter 3rd digit, than ellipse1, ellipse2, and ellipse3 should get filled. and so on
in view class, I have 4 ellipse, representing 4 digit pin. if user enter a digit, ellipse color get filled.
in viewmodel, I am a method that check if digit was pressed. if it is than fill ellipse color
in helperclass, i am just checking if ellipse is transpaent (not pressed), or fill color (key was pressed)
view class
 <Ellipse Fill="{Binding EllipseFill1}" />
 <Ellipse Fill="{Binding EllipseFill2}" />
 <Ellipse Fill="{Binding EllipseFill3}" />
 <Ellipse Fill="{Binding EllipseFill4}" />

ViewModel class
  List<int> CreatePin = new List<int>; //4 digit pin is store here
  public void OnKeypadTapCommand(string value)
    {
        //Set color of Pin
        EllipseFill1 = helperCLass.SetColorPin(CreatePin, Brush.White, 1);
        EllipseFill2 = helperCLass.SetColorPin(CreatePin, Brush.White, 2);
        EllipseFill3 = helperCLass.SetColorPin(CreatePin, Brush.White, 3);
        EllipseFill4 = helperCLass.SetColorPin(CreatePin, Brush.White, 4);
    }

Heloperclass
    public Brush SetColorPin(List<int> valueList, Brush value, int PinNumber)
    {
        //Pin#1
        if (PinNumber == 1)
        {
            if (valueList.Count == 0)
                return Brush.Transparent;
            else
                return value;
        }
        //Pin#2
        else if (PinNumber == 2)
        {
            if (valueList.Count == 0 || valueList.Count == 1)
                return Brush.Transparent;
            else
                return value;
        }
        //Pin#3
        else if (PinNumber == 3)
        {
            if (valueList.Count == 3 || valueList.Count == 4)
                return value;
            else
                return Brush.Transparent;
        }
        //Pin#4
        else if (PinNumber == 4)
        {
            if (valueList.Count == 4)
                return value;
            else
                return Brush.Transparent;
        }
        else
        {
            return Brush.Transparent;
        }

    }

you could change the helper method to use switch if you're using C# 7.0 or higher then you can do this :
switch(PinNumber)
{
    case 1 when valueList.Count == 0:
    case 2 when valueList.Count == 0 || valueList.Count == 1:
    case 3 when valueList.Count == 3 || valueList.Count == 4:
    case 4 valueList.Count == 4:
        return value;
    default:
        return Brush.Transparent;
}

if not, then you can focus on readability by doing something like this :
var isPin1 = PinNumber == 1 && valueList.Count != 0;
var isPin2 = PinNumber == 2 && valueList.Count == 0 || valueList.Count == 1;
var isPin3 = PinNumber == 3 && valueList.Count == 3 || valueList.Count == 4;
var isPin4 = PinNumber == 4 && valueList.Count == 4;

return isPin1 || isPin2 || isPin3 || isPin4 ? value : Brush.Transparent;


Is there a way to optimize this aggregate postgreSQL statement by using CASE? I have a working SQL statement which shows the following:

Player
Champion
Role
Total Kill/Death Ratio
Total Kill+Assist/Death Ratio
Total Win %
Difference between Ratio from one month ago
Difference between KA Ratio from one month ago
Difference between win % from one month ago

However, I don't think it's quite as optimized as it could be:

with CTERatio as (

select tt.playerid, tt.champid, tt.roleid,
    sum(tt.words) as kills,
    sum(tt.words2) as deaths,
    sum(tt.words3) as assists,
    sum(tt.words)/sum(tt.words2) as ratio,
    (sum(tt.words)+sum(tt.words3))/sum(tt.words2) as kdar,
    avg(tt.complete) as win
from schema.records tt
group by tt.playerid, tt.champid, tt.roleid
having count(*) > 2
order by ratio desc

),

hist as (

select tt.playerid, tt.champid, tt.roleid,
    sum(tt.words) as kills,
    sum(tt.words2) as deaths,
    sum(tt.words3) as assists,
    sum(tt.words)/sum(tt.words2) as ratio,
    (sum(tt.words)+sum(tt.words3))/sum(tt.words2) as kdar,
    avg(tt.complete) as win
from schema.records tt
where tt.sys_time < date_trunc('day', NOW() - interval '1 month')
group by tt.playerid, tt.champid, tt.roleid
having count(*) > 2
order by ratio desc

)
select c.playerid as player, c.champid as champion, c.roleid as rolee,
    round(c.ratio, 2) as ratio, round(c.kdar, 2) as kdar, round(c.win, 2) as win,
    round(c.ratio - h.ratio, 2) as ratiochange,
    round(c.kdar - h.kdar, 2) as kdarchange,
    round(c.win - h.win, 2) as winchange
from CTERatio c, hist h
where c.playerid = h.playerid
and c.champid = h.champid
and c.roleid = h.roleid
order by c.ratio desc

Is it possible I can incorporate using CASE instead of WITH? Such as:

avg(c.kills) - avg
(
case when tt.sys_time <  date_trunc('day', NOW() - interval '1 month')
then h.words
end
)

Naming conventions

I think your aliases are pretty cryptic.

This:

from schema.records tt

That:

from CTERatio c, hist h

It's better to use aliases that mean something. tt, c, h... think of Mr. Maintainer going back over your code having to make changes to it.

I will compliment you on re-labeling column names words, words2, words3. Those are pretty bad column names and your re-labeling makes them much more meaningful.

Indenting/Formatting

Your CTEs are beautiful in how they are formatted. Your final select statement, not so much.  How about this:

select
    c.playerid as player,
    c.champid as champion,
    c.roleid as rolee,
    round(c.ratio, 2) as ratio,
    round(c.kdar, 2) as kdar,
    round(c.win, 2) as win,
    round(c.ratio - h.ratio, 2) as ratiochange,
    round(c.kdar - h.kdar, 2) as kdarchange,
    round(c.win - h.win, 2) as winchange

Performance

This aggregate is expensive, especially used twice:

having count(*) > 2

This means the SQL engine will have to scan the whole table for each column to see if the count is greater than 2. Try to find a unique column to COUNT() on rather than the whole table, like having count(c.playerid) > 2

You have many sum() and round() aggregates, can we cut down on them somehow? It's hard to give specifics since we don't know what the data looks like, but try to limit those as much as you can.

Both of your CTEs seem identical. I'm curious as to why you are selecting the same dataset twice. You could do a self join instead.

Here is how I would write this script, bearing in mind I don't know what the data looks like:

with CTERatio as (
-- calculate ratio
select
    rec.playerid,
    rec.champid,
    rec.roleid,
    sum(rec.words) as kills,
    sum(rec.words2) as deaths,
    sum(rec.words3) as assists,
    sum(rec.words)/sum(rec.words2) as ratio,
    (sum(rec.words)+sum(rec.words3))/sum(rec.words2) as kdar,
    avg(rec.complete) as win
from schema.records as rec
group by rec.playerid, rec.champid, rec.roleid
having count(*) > 2
order by ratio desc

),
-- join ratio to itself to find highest ratio
select
    cte_r1.playerid as player,
    cte_r1.champid as champion,
    cte_r1.roleid as rolee,
    round(cte_r1.ratio, 2) as ratio,
    round(cte_r1.kdar, 2) as kdar,
    round(cte_r1.win, 2) as win,
    round(cte_r1.ratio - h.ratio, 2) as ratiochange,
    round(cte_r1.kdar - h.kdar, 2) as kdarchange,
    round(cte_r1.win - h.win, 2) as winchange
from CTERatio as cte_r1
  inner join CTERatio as cte_r2
    on cte_r1.playerid = cte_r2.playerid
    and cte_r1.champid = te_r2.champid
    and cte_r1.roleid = cte_r2.roleid
order by cte_r1.ratio desc
;


Loop months between a time span yyyy-mm and yyyy-mm I'm trying to come up with a more elegant solution for a loop in months that I wrote long time ago in C# 1.1

I use a helper class Month but I'd prefer to get rid of it and make the code shorter and more readable.

I receive a start period in format yyyy-mm and end period yyyy-mm and I need to execute something for every month between those 2 periods.

public void LoopMonths(string startM, string endM)
{
        //startM = "2015-02";
        //endM = "2018-11";

        int startYear = int.Parse(startM.Split('-')[0]);
        int startMonth = int.Parse(startM.Split('-')[1]);
        int endYear = int.Parse(endM.Split('-')[0]);
        int endMonth = int.Parse(endM.Split('-')[1]);

        Month month = new Month(startYear, startMonth);
        Month monthEnd = new Month(endYear, endMonth);

        while (month <= monthEnd)
        {
            // do something ...
            month = Month.NextMonth(month);
        }
}

public class Month
{
    public DateTime StartDate { get; set; }
    public DateTime EndDate { get; set; }

    public Month(int year, int month)
    {
        StartDate = new DateTime(year, month, 1);
        EndDate = new DateTime(year, month, DateTime.DaysInMonth(year, month));
    }

    public static Month NextMonth(Month month)
    {
        DateTime next = month.StartDate.AddMonths(1);
        return new Month(next.Year, next.Month);
    }

    public static bool operator <=(Month month1, Month month2)
    {
        return month1.StartDate <= month2.StartDate;
    }

    public static bool operator >=(Month month1, Month month2)
    {
        return month1.StartDate >= month2.StartDate;
    }
}

These are all good sugestions but to my taste not linq-ish enough and not really reusable. Since I don't like writing the same code serval times you might want to encapsulate some of this logic for later use.

You can extract two pieces. First you have a date-time range so create a Range struct. Here's a very simple one (as an example without any parameter validation - actually T should be IComparable so you can assure the range is valid).

readonly struct Range<T>
{
    public Range(T min, T max)
    {
        Min = min;
        Max = max;
    }

    public T Min { get; }

    public T Max { get; }
}

Then you are interpolating between min and max. Put this in an extension that will consume a Range and do the rest:

public static IEnumerable<T> Interpolate<T>(this Range<T> range, Func<T, T> next) where T : IComparable
{
    var current = range.Min;
    while (current.CompareTo(range.Max) <= 0)
    {
        yield return (current = next(current));
    }
}

Now all you need is to parse the date so how about one more helper for that too?

static class Range
{
    public static Range<DateTime> FromDateTime(string min, string max, string format)
    {
        return new Range<DateTime>(
            DateTime.ParseExact(min, format, CultureInfo.InvariantCulture),
            DateTime.ParseExact(max, format, CultureInfo.InvariantCulture)
        );
    }
}

And we're ready to go. Create a range and let the extension work.

var range = Range.FromDateTime("2015-02", "2018-11", "yyyy-MM");
range.Interpolate(d => d.AddMonths(1)).Dump();

Now you have nice components that you can use in many more places and maybe in several projects.


Analyzing distances between clusters of orders I wrote the Python class below, which does what I want it to do, but the data structure is a mess. Was wondering if there was a better structure I could use to get the same results but with better readable code.

Idea here is we retrieve a dataset from SQL(constructor), cluster the dataset into distinct keys(constructor), iterate through the keys and isolate the matching criteria in the dataset(organizer), pass those data chunks to map_loc_to_lat_long which will find all possible combinations of rows in the chunk and find the straight line distance between all the combination Lat Longs.

class OrderProximityModel:
    def __init__(self, date):
        self.date = str(date)
        self.order_data = OrderProxDao().Load_Order_Lat_Long_Date_Zone_Data(self.date)
        self.distinct = set([str(row.Requirements) + ' ' + str(row.Route_Date) for row in self.order_data])

    def organizer(self):
        container = []
        for date_zone in self.distinct:
            latlng = list(filter(lambda x: str(x.Requirements) + ' ' + str(x.Route_Date) == date_zone, self.order_data))
            for i in self.map_loc_to_lat_long(latlng):
                container.append((i[0][0][0], i[0][0][1], i[0][0][2], i[0][0][4], i[0][0][5], i[0][0][6]))
        InsertHelpers(container).chunk_maker(100)
        return True

    def map_loc_to_lat_long(self, grouped_zone):
        converted = {}
        for row in grouped_zone:
            converted[row.LocationKey] = [row.Latitude, row.Longitude, row.DA, row.Route_Date, row.Requirements, row.DA]
        grouped_combos = self.combo_creator(converted.keys())
        return map(lambda combo: ([converted[combo[0]][2:] + [combo[0]] + [combo[1]] +
                                  [StraightLineDistance().dist_cal(converted[combo[0]][0],
                                                                   converted[combo[0]][1],
                                                                   converted[combo[1]][0],
                                                                   converted[combo[1]][1])]],
                                  ), grouped_combos)

    @staticmethod
    def combo_creator(inputs):
        out = []
        for index, value in enumerate(inputs):
            for nex_value in inputs[index + 1:]:
                out.append((value, nex_value))
        return out

self.date = str(date)

This is a pet peeve of mine. Stringly-typed variables are usually a bad idea. If you receive a datetime object, you should usually keep it as datetime until you actually need it to be a string.

Load_Order_Lat_Long_Date_Zone_Data

If at all possible, shorten this method name. Also, methods are lowercase by convention.

self.distinct = set([str(row.Requirements) + ' ' + str(row.Route_Date) for row in self.order_data])

Here you make a generator, construct a list and then convert it to a set. Skip the list - the set constructor can accept generators directly. Better yet, if you're in a sane version of Python, just use a set literal (and use a format string):

self.distinct = {'%s %s' % (row.Requirements, row.Route_Date) for row in self.order_data}

Your container = [] / container.append() loop can be replaced by proper use of a generator. Same with out.

latlng does not (and should not) be materialized to a list. It should be left as a generator, since you only iterate over it once.

This:

container.append((i[0][0][0], i[0][0][1], i[0][0][2], i[0][0][4], i[0][0][5], i[0][0][6]))

can be:

container.append(tuple(i[0][0][j] for j in (0, 1, 2, 4, 5, 6)))


JavaScript implementation of Symbol Table (Dictionary) I'm currently going over Robert Sedgewick Algorithms book. Here I'm implementing a Symbol Table using a Linked List. The Sequential Search Symbol table is implemented in JavaScript. The book mentiones that this Abstract Data Structure is not a good solution when dealing with big amounts of data.
I would like feedback on the implementation and on following JavaScript best practices. I'm coming from Ruby so having this implementation follow the best practices in JavaScript is a plus for me.

// Symbol Table using Sequential search (Linked List)
//
class SequentialSearchST {

  constructor() {
    this.first = null;
  }

  get(key) {
    for(let x = this.first; x != null; x = x.next) {
      if(x.key === key) {
        return x.val
      }
    }
    return null;
  }

  put(key, val) {
    for(let x = this.first; x != null; x = x.next) {
      if(x.key === key) {
        return x.val = val
      }
    }
    this.first = new Node(key, val, this.first);
  }

}

class Node {
  constructor(key, val, next) {
    this.key = key;
    this.val = val;
    this.next = next;
  }
}

st = new SequentialSearchST();
st.put('s', 0)
st.put('e', 1)
st.put('a', 2)
st.put('r', 3)
st.put('c', 4)
st.put('h', 5)
st.put('e', 6)
st.put('x', 7)
st.put('a', 9)
st.put('m', 10)
st.put('p', 11)
st.put('l', 12)
st.put('e', 13)
console.dir(st.get('s'));

Iterators When you have a data structure that needs to be iterated over sometime, consider providing an iterator to make things easier and more abstract for consumers. That is, you don't really want to write
for(let x = this.first; x != null; x = x.next) {

every time, nor would you want consumers of SequentialSearchST to have to do that if they wanted to examine the data manually. Instead, put an iterator on the class, and call that iterator by using for..of whenever needed.
class SequentialSearchST {
  *[Symbol.iterator]() {
    for (let node = this.first; node; node = node.next) {
      yield node;
    }
  }

Variable names x is not an informative variable name. Like I did above, maybe call a node node instead.
Semicolons Either use semicolons wherever appropriate, or don't use semicolons. Anything in-between looks inconsistent and can result in you eventually getting tripped up by the rules of Automatic Semicolon Insertion. If you're a beginner in JS, I'd highly recommend using semicolons. You can use a linter to enforce a particular style - to warn you of potential bugs before they turn into runtime errors that you need to spend time tracking down.
Declare your variables before using them. If you don't, you'll either implicitly create a global variable (an easy source of bugs), or an error will be thrown (in strict mode, and strict mode is recommended). Change
st = new SequentialSearchST();

to
const st = new SequentialSearchST();

Class usage A class is useful when you need to tie together data with methods that operate on that data. If you have only data, a class doesn't provide any benefit, and is simply noise, IMO. If I were you, I'd remove the Node class and change
this.first = new Node(key, val, this.first);

to
this.first = { key, val, next: this.first };

null Rather than assigning null as the initial value, and returning null when no matching node is found, you might consider if simply omitting a return statement entirely would do the trick. It's perfectly fine for a function call to return undefined; omitting null requires less code, less potential for inconsistencies, and one less thing to think about. (If this is for an exercise that requires null to be used, that's fine)
(If you do use null, make sure to verify it by using !== null, not != null; sloppy comparison requires more cognitive overhead than strict comparison)
This is how I'd refactor it:

class SequentialSearchST {
  *[Symbol.iterator]() {
    for (let node = this.first; node; node = node.next) {
      yield node;
    }
  }
  get(key) {
    for (const node of this) {
      if (node.key === key) {
        return node.val;
      }
    }
  }
  put(key, val) {
    for (const node of this) {
      if (node.key === key) {
        node.val = val;
        return;
      }
    }
    this.first = { key, val, next: this.first };
  }
}

const st = new SequentialSearchST();
st.put('s', 0)
st.put('e', 1)
st.put('a', 2)
st.put('r', 3)
st.put('c', 4)
st.put('h', 5)
st.put('e', 6)
st.put('x', 7)
st.put('a', 9)
st.put('m', 10)
st.put('p', 11)
st.put('l', 12)
st.put('e', 13)
console.dir(st.get('s'));

Another approach Maps hold key-value pairs. (Objects do too, but objects can only contain string and symbol keys, which is a limitation.) If this was a real-world problem I had to solve, I'd just use a Map:

const st = new Map();
st.set('s', 0)
st.set('e', 1)
st.set('a', 2)
st.set('r', 3)
st.set('c', 4)
st.set('h', 5)
st.set('e', 6)
st.set('x', 7)
st.set('a', 9)
st.set('m', 10)
st.set('p', 11)
st.set('l', 12)
st.set('e', 13)
console.dir(st.get('s'));


Goroutine wait select operation on multiple channels I'm trying to understand an SO answer for this question Golang select statement with channels and waitgroup.

The author of the now accepted answer goes on to say that OP's code is almost similar to this, when the function is replaced with the actual code

package main

import (
    "errors"
    "fmt"
)

func main() {
    errs := make(chan error, 1)
    success := make(chan bool, 1)
    errs <- errors.New("Some error")
    success <- true

    select {
    case err := <-errs:
        fmt.Println("error", err)
    case <-success:
        fmt.Println("success")
    }

    select {
    case err := <-errs:
        fmt.Println("error", err)
        // panic(err)
    case <-success:
        fmt.Println("success")
    }
}

What I don't understand is why are there two select wait operations? I understand that both the error and bool channels are written at the same time and there could be one handled by the select at any time, but why does OP have two instances select? Would like to understand the reason behind possibly a better way to achieve the same functionality.

Not sure whether this site is the best place to ask this question, but the answer is quite simple. I'll answer the question, give a few alternatives, pro's and con's and some actual code-review on the little snippet you provided.

  Do not communicate by sharing memory; instead, share memory by communicating

This is the main idea behind channels. When talking about communicating and sharing memory, it's obvious we're talking about concurrency. The example you have isn't really dealing with concurrency all that much. Such an artificial example will almost always be fairly limit in explaining the mechanisms/concepts behind something that is, effectively, a weapon in the armoury of concurrent programming. Either way, worth checking out the effective go document for more on this.

To answer your question:

Looking at the select spec:

  A "select" statement chooses which of a set of possible send or receive operations will proceed.

Then, from the steps that a select statement executes, there's 2 steps to pay attention to:

  one or more of the communications can proceed, a single one that can proceed is chosen via a uniform pseudo-random selection. Otherwise, if there is a default case, that case is chosen. If there is no default case, the "select" statement blocks until at least one of the communications can proceed.

And:

  The statement list of the selected case is executed.

This means that, even if both channels are ready to be read/received from, the select statement will only ever read from a single channel. Which channel that is, is not specified (other than the channel is chosen via a uniform pseudo-random selection).

If you'd have:

errs := make(chan error, 1)
success := make(chan struct{}, 1)
defer func() {
    close(errs)
    close(success)
}()
errs <- errors.New("some error")
success <- struct{}{}

select {
case err := <-errs:
    fmt.Printf("Receiver error: %+v\n", err)
case <-success:
    fmt.Println("Success received")
}

You would have no way of knowing which channel you'd read from.

Some code review:

You may have noticed, too, that I've changed a couple of things. This being a code-review site and all, here's why I made the changes I did:

Success or done channels are either unbuffered (indicating success/done on close), or are best defined as chan struct{}. A boolean can be false or true, indicating that the value does have a meaning to its receiver. Again as per spec, a type struct{} is defined as 0 bytes in size (slightly more optimised than bool), and clearly indicates the channel serves to signal something, rather than communicate something.
errors.New("Some error")  should be "some error". Error values should start with a lower-case letter. Check golang CodeReviewComments for details. There's a lot of conventions there, and overall the community seems to have adopted them.
I added a defer func(){}() to close the channels. It's bad form to not close channels...

Either way, the double select statements are there to ensure both channels are read.

More details

Be that as it may, having a quick look at the linked post, seeing as a waitgroup is used in the accepted answer, I'd probably prefer something more like this:

errs := make(chan error) // not buffered
success := make(chan struct{}) // not buffered
go func() {
    errs <- errors.New("some error")
}()
go func() {
    success <- struct{}{}
}()
err := <-errs
fmt.Printf("Received error: %+v\n", err)
close(errs) // we're done with this channel
<-success
close(success) // all done
fmt.Println("Success")

The trade-off being: You will always wait for the error channel before reading the success channel. The advantage: no waitgroup required, no verbose select statement...

Should you want to maintain the random-like output (having success and error printed out in a random order), you can easily replace the ordered reads from the channels 2 select statements, or use a waitgroup and read from the channels in routines:

func main() {
    errs := make(chan error) // not buffered
    success := make(chan struct{}) // not buffered
    wg := sync.WaitGroup{}
    // start reading from channels in routines
    wg.Add(2)
    go readRoutine(&wg, errs)
    go readRoutine(&wg, success)
    // routines are needed because the channels aren't buffered, writes are blocking...
    go func() {
        errs <- errors.New("some error")
    }()
    go func() {
        success <- struct{}{}
    }()
    wg.Wait() // wait for reads to have happened
    close(errs) // close channels
    close(success)
}

func readRoutine(wg *sync.WaitGroup, ch <-chan interface{}) {
    defer wg.Done()
    v := <-ch
    if err, ok := v.(error); ok {
        fmt.Printf("Received error: %+v\n", err)
        return
    }
    fmt.Println("Received success")
}

Now we're almost there... The rule of thumb is that the routine creating the channels should also be responsible for closing the channels. However, we know that the anonymous routines are the only 2 that are writing to the channels, and they are created and spawned by main, and they even access its scope. There's 2 changes we can make, therefore:

go func() {
    errs <- errors.New("some error")
    close(errs) // we can close this channel
}()
go func() {
    close(success) // we don't even have to write to this channel
}()
wg.Wait()
// channels are already closed

Hope this answered your question, provided some useful links to resources that can help you answer future questions, and maybe gave you a few ideas of how to use channels.


asMap-implementation for Java (based on Arrays.asList) I am always tired of the very verbose syntax in Java for creating small maps. Arrays.asList(T... a) is very convenient, and therefore I want something similar for a map.

public class MapUtils {
    public static <K, V> MapBuilder<K, V> asMap(K key, V value) {
        return new MapBuilder<K, V>().entry(key, value);
    }

    public static class MapBuilder<K, V> extends HashMap<K, V> {
        public MapBuilder<K, V> entry(K key, V value) {
            this.put(key, value);
            return this;
        }
    }
}

/* Example of how to use asMap */
public class Example {
    public void example() {
        Map<String, String> map = MapUtil.asMap("key", "value").entry("key2", "value2");
    }
}

/* Example of how the one way to create an inline Map in Java */
public class Before {
    public void before() {
        Map<String, String> map = new HashMap<String, String>() {{
            put("key", value");
            put("key2", value2");
        }};
    }
}

Any inputs on how to improve this implementation of MapUtils? Do you consider the asMap function a good idea, or am I too lazy to write some extra characters?

In my opinion MapBuilder should contain a Map, not extend one. Another possible design would be to use a single call with varargs:

public class MapUtil {
    public static <K,V> Map<K,V> asMap(Map.Entry<K,V>... entries) {
        Map<K,V> map = new HashMap<K,V>();
        for(Map.Entry<K,V> entry : entries) {
            map.put(entry.getKey(), entry.getValue());
        }
        return map;
    }

    public static <K,V> Map.Entry<K,V> e(final K k, final V v) {
        return new Map.Entry<K, V>() {
            public K getKey() {
                return k;
            }
            public V getValue() {
                return v;
            }
            public V setValue(V value) {
                throw new UnsupportedOperationException("Not supported");
            }
        };
    }
}

import static very.useful.MapUtil.*;
...
Map<String, Integer> map = asMap(e("x",1),e("y",2),e("z",3));


Anything I'm missing on this delegate implementation? Just as the question states and to be sure, am I missing anything on this? Something important/obvious that I overlooked? As is, I can save any functor, function pointer and member function pointer using this delegate class (copy ctor and assignment operator left out for brevity, as were the versions with parameters).

template<class Type>
void Deleter(void* object){
    Type* obj_ptr = static_cast<Type*>(object);
    delete obj_ptr;
}

template<class Functor>
struct DelegateHelper{
    typedef typename Functor::result_type result_type;

    DelegateHelper(Functor func)
        : func_(func)
    {}

    result_type operator()(){
        return func_();
    }

    Functor func_;
};

template<class Obj, class R>
struct DelegateHelper<R (Obj::*)()>{
    typedef R (Obj::*FuncPtr)();

    DelegateHelper(Obj* obj_ptr, FuncPtr func)
        : object_(obj_ptr)
        , func_(func)
    {}

    R operator()(){
        return (object_->*func_)();
    }

    Obj* object_;
    FuncPtr func_;
};

template<class R>
struct DelegateHelper<R (*)()>{
    typedef R (*FuncPtr)();

    DelegateHelper(FuncPtr func)
        : func_(func)
    {}

    R operator()(){
        return (*func_)();
    }

    FuncPtr func_;
};

template<class Sig>
struct Delegate;

template<class R>
struct Delegate<R ()>{
    typedef R (*SinkPtr)(void*);

    Delegate()
        : object_(0)
        , func_(0)
        , deleter_(0)
    {}

    template<class F>
    Delegate(F func)
        : object_(new DelegateHelper<F>(func))
        , func_(&Sink<DelegateHelper<F> >)
        , deleter_(&Deleter<DelegateHelper<F> >)
    {}

    template<class C, class F>
    Delegate(C* obj, F func)
        : object_(new DelegateHelper<F>(obj, func))
        , func_(&Sink<DelegateHelper<F> >)
        , deleter_(&Deleter<DelegateHelper<F> >)
    {}

    ~Delegate(){
        if(deleter_)
            (*deleter_)(object_);
    }

    R operator()(){
        return (*func_)(object_);
    }

    template<class Type>
    static R Sink(void* obj_ptr){
        Type* object = (Type*)obj_ptr;
        return (*object)();
    }

    typedef void (*DeleteFunc)(void*);

    void* object_;
    SinkPtr func_;
    DeleteFunc deleter_;
};

For the delegate in effect, see here on Ideone.

You are missing const and volatile qualified method pointer versions.

Example:

template<class Obj, class R>
struct DelegateHelper<R (Obj::*)() const>{
    typedef R (Obj::*FuncPtr)() const;

    DelegateHelper(const Obj* obj_ptr, FuncPtr func)
        : object_(obj_ptr)
        , func_(func)
    {}

    R operator()(){
        return (object_->*func_)();
    }

    const Obj* object_;
    FuncPtr func_;
};

Also, you may (probably) need specializations or internal delegates for handling of functions with void return values. Someone else may be able to fill in the details, but it seems some compilers are OK with

return function_returning_void();

And others are not.

Another thing that makes the boost version and some others larger is being able to accept & or * arguments for the object value when calling methods.


Shifting chars (Caesar Cipher) Currently, I'm writing a program that encrypts/decrypts text using Caesars cipher.  The code below shifts a character.

    /// <param name="originalChar"> char to shift </param>
    /// <param name="shift"> amount to shift by </param>
    /// <param name="letterA"> letter A in ASCII </param>
    /// <param name="lettera"> letter a in ASCII </param>
    /// <param name="letterCount"> range of alphabet </param>
    /// <returns> shifted letter </returns>
    private static char Shifter(char originalChar, int shift, int letterA, int lettera, int letterCount)
        => (char) (letterA + (char.ToUpper(originalChar) - letterA + shift + letterCount) % letterCount
                + (char.IsLower(originalChar) ? lettera - letterA : 0));

This happens by shifting the char around the upper case alphabet then if it was originally a lower case char move it up to the lowercase part of ASCII.

I don't think my current method of doing this is particularly efficient and I can't think of anything to improve it.

private static char Shifter(char originalChar, int shift, int letterA, int lettera, int letterCount)

seeing this method declaration makes me shiver.
Let us start with the name of that method. Shifter is a noun but method names should be made out of a verb or a verb phrase hence Shift would be the way to go.

It is always the best if a method declaration can be understood without reading the documentation, so if we rename int shift to int shiftAmount it will become a lot clearer.

The small difference between int letterA and int lettera can be easily overlooked which is bad. So it better should be for instance int upperA and int lowerA but maybe we should think about this some more.

By reading the documentation

/// <param name="letterA"> letter A in ASCII </param>
/// <param name="lettera"> letter a in ASCII </param>

I assume this values are always the same A and a (correct me if I am wrong) hence it doesn't serve any real purpose why this parameters should exist in the method declaration. These values should be extracted to const int either method scoped or private static int class members.

/// <param name="letterCount"> range of alphabet </param>

What is a range of alphabet ? Should it be the number of characters in an alphabet ?

Having a parameter named originalChar doesn't add any value. Maybe just char letter would be better and easier to understand ?

Although you are passing a char to this method I would change the documentation from

/// <param name="originalChar"> char to shift </param>

to (intergated the name change too)

/// <param name="letter"> letter to shift </param>

I agree with @EthanBierlein about the expression-bodied members part. You really shouldn't do something just because it can be done. The expression-bodied members does have a value but IMO only for small amount of code. Having a line of code which is spawned over two lines is just to much code.

This ternary expression

char.IsLower(originalChar) ? lettera - letterA : 0

should better be expressed by a simple if statement because the result is added to a variable and adding 0 just doesn't make a difference. IMO the indent become more clear.

Implementing the mentioned points will then lead to

private static int upperA = 'A';
private static int lowerA = 'a';
/// <param name="letter"> letter to shift </param>
/// <param name="shiftAmount"> amount to shift by </param>
/// <param name="alphabetLetterCount"> count of letters in alphabet </param>
/// <returns> shifted letter </returns>
private static char Shift(char letter, int shiftAmount, int alphabetLetterCount)
{
    var result = upperA;

    result += (char.ToUpper(letter) - upperA + shiftAmount + alphabetLetterCount) % alphabetLetterCount;

    if (char.IsLower(letter))
    {
        result += lowerA - upperA;
    }

    return (char)result;
}

This implementation assumes that the code doesn't need to pass the turkey test.


C++: Smart Pointer Implementation Please could somebody verify that my method of implementing Smart Pointer is correct, and if there is maybe a more efficient way of achieving it.

#ifndef SHARED_PTR
#define SHARED_PTR

#include <iostream>
#include <mutex>

using namespace std;

template <class T>
class RCObject
{
    T* rawObj;
    unsigned int count;
    mutex m_Mutx;

public:
    RCObject() :rawObj(nullptr), count(0){}

    ~RCObject()
    {
        deref();
    }

    RCObject(T* _rawObj):rawObj(_rawObj)
    {
        count = 0;
    }

    int getCount() const
    {
        return count;
    }

    T* get()
    {
        if (rawObj != nullptr) {
            return rawObj;
        }
        return nullptr;
    }

    void ref()
    {
        m_Mutx.lock();
        count++;
        m_Mutx.unlock();
    }

    void deref()
    {
        m_Mutx.lock();
        count--;
        m_Mutx.unlock();
        if (count == 0)
        {
            delete rawObj;
            count = 0;
        }
    }
};

template <class T>
class SharedPtr
{
    RCObject<T>* m_Rc;
public:
    SharedPtr(T* rawObj)
    {
        m_Rc = new RCObject<T>(rawObj);
        m_Rc->ref();
    }

    ~SharedPtr()
    {
        m_Rc->deref();
    }

    SharedPtr()
    {
        m_Rc = new RCObject<T>();
    }

    SharedPtr(const SharedPtr& rhs)
    {
        m_Rc = rhs.m_Rc;
        m_Rc->ref();
    }

    SharedPtr& operator= (const SharedPtr& rhs) {
        if (this != &rhs) {
            m_Rc->deref();
            m_Rc = rhs.m_Rc;
            m_Rc->ref();
        }
        return *this;
    }

    int getRefCount()
    {
        return m_Rc->getCount();
    }

    T* get() const
    {
        return m_Rc->get();
    }
    T* operator->() {
        return m_Rc->get();
    }
};
#endif

Safety

The whole point of the smart pointers is that you give up ownership of the pointer; so it (the smart pointer) will delete it if something goes wrong. Unfortunately your smart pointer fails this first test.

SharedPtr(T* rawObj)
{
    m_Rc = new RCObject<T>(rawObj);  // if this throws you have
                                     // leaked your object the one
                                     // thing a smart pointer should
                                     // never do.
    m_Rc->ref();
}

If that new throws you leak the pointer that was given to you.

Probably the best way to handle this is the initializer try catch:

SharedPtr(T* rawObj)
try
    : m_Rc(new RCObject<T>(rawObj))
{
    m_Rc->ref();
}
catch(...)
{
    delete rawObj;
    throw;
}

Rule of Three

Your RCObject does not obey the rule of three. Now this is a special case as it should only be used by SharedPtr and I can't see a wrong usage. But to protect against future misuse you should make this (RCObject) a private member of SharedPtr.

Though you have the three methods you require for SharedPtr to implement the rule of three you don't actually do it. Su you leak m_Rc in a couple of situations.

{
    SharedPtr  ac;
}//  Leaked the m_Rc

SharedPtr  ac1;
SharedPtr  ac2;
ac1 = ac2;  // leaked m_Rc from ac1.

RefCount Issue

I don't like setting the Count to zero in RCObject constructors as that requires you to call ref() after creating the object to correctly initialize it.

RCObject() :rawObj(nullptr), count(0){}

The whole point of the constructor is to put the object into a valid state so it can immediately be used.

And here is the bug caused by that misuse.

SharedPtr()
{
    m_Rc = new RCObject<T>();
    // You forgot to increment the ref count here.
    // When this shared pointer is destroyed the count will go to -1
    // not zero.

    // Currently this is not causing an issue because your class is
    // so simple but this breaks the invariant that everybody thinks
    // you have and thus as the class is expanded it will cause an
    // error at some point.
}

Pointless Test

Don't see the point in the test.

T* get()
{
    if (rawObj != nullptr) {
        return rawObj;
    }
    return nullptr;
}

if the pointer is null you will return nullptr. Why not just return the pointer?

Learn to use RAII

void ref()
{
    m_Mutx.lock();
    count++;
    m_Mutx.unlock();
}

Any time you see the pattern:

SetUp
Work
TearDown

You should think RAII and wrap it. It may seem usless; but as your class expands it will ensure that your class stays exception safe. Also there are so many helper classes out there that it is not really that much work.

void ref()
{
    std::lock_guard<std::mutex> lock(m_Mutx);
    count++;
}

Stop using leading underscore in identifiers

RCObject(T* _rawObj):rawObj(_rawObj)
{}

The rules for a leading underscore are non trivial and most people don't know them. So even if you do most other people will get confused and panicky.

The case where you use them it is not required anyway:

// This is perfectly valid.
// Does what you expect and quite normal in C++ code.
RCObject(T* rawObj)
    : rawObj(rawObj)
{}

Other Stuff.

There are a whole bunch of other stuff you should do take a look at these three articles I wrote:

Smart-Pointer - Unique Pointer
Smart-Pointer - Shared Pointer
Smart-Pointer - Constructors


Python set based solution to Subset Sum Working on the 2 element subset sum problem (finding two numbers in a list that sum to a given value), I came up with a relatively short solution code-wise.

The code could fit on a single line but I'm not super familiar with python so is this is a naive way to solve/implement it?

How efficient is creating a set and intersecting it with a list vs using a loop?

l = [10, 7, 11, 3, 5, 11]
k = 22

#create a set of the differences between K and L including duplicate elements that sum to K
diff = {k-x for i, x in enumerate(l) if k != 2*x or x in l[i+1:len(l)]}

#Return true if an element of Diff is in L
print(bool(diff.intersection(l)))

Usually Python is not terribly fast when it comes to "hand-written" loops. So my prediction (also supported by some preliminary profiling using timeit, see below) would be that a "hand-written" loop is slower than the C loop used in the implementation of set.intersection. The effect should/will become more significant for larger lists.

A quick non-performance related note: you can substitute l[i+1:len(l)] by l[i+1:]. This is because the end of a slice defaults to the end of the sequence.

import timeit

SETUP = """
l = [10, 7, 11, 3, 5, 11]
k = 22

diff = {k-x for i, x in enumerate(l) if k != 2*x or x in l[i+1:]}
"""
print(
    sum(timeit.repeat("bool(diff.intersection(l))", setup=SETUP,
                      repeat=10, number=100000)) / 10
)
print(
    sum(timeit.repeat("bool([i for i in l if i in diff])", setup=SETUP,
                      repeat=10, number=100000)) / 10
)


GroupBy by nodeJS or ES6 of very nested JSON Objects with condition The following code should return an array (flatFeatures) which will include feature.name and the most far subscription.expDate. for example if I have some subscriptions with the same features, the flatFeatures array will include distinct of all features from all the subscriptions but for every feature we will have the most later date of the subscription it belongs to. Something like that in SQL:
SELECT Features.Name, MAX(subscriptions.expDate)
FROM Subscriptions
INNER JOIN Plans ON Subscriptions.planId = Plans.planId
INNER JOIN Features ON Plans.PlanId = Features.PlanId

But i need it to be done by NodeJS for the JSON i already have from MongoDB.
    const flatFeatures = new Array();

    allActiveSubscriptions.forEach(subscription => {

      if (subscription.plan && subscription.plan.featuresList) {
        subscription.plan.featuresList.forEach(feature => {
          const existingFlatFeature = flatFeatures.find(flatFeatureItem => {
            return (flatFeatureItem.name === feature.name);
          });

          if (existingFlatFeature) {
            existingFlatFeature.expDate = (subscription.expDate > existingFlatFeature.expDate) ? subscription.expDate : existingFlatFeature.expDate;
          } else {
            flatFeatures.push({
              name: feature.name,
              expDate: subscription.expDate
            });
          }

        });
      }
    });

A few thoughts;

Most would write const flatFeature = []; to initialize an array in javascript, though what you have is certainly correct.
You should strive to de-nest your code.  Having code unnecessarily placed in if or if-else blocks increases the number of paths through your code, making it harder to follow and unit test.  In this case, you could de-nest by changing your first if to something like if (!subscription.plan || !subscription.plan.feature) return; which clearly indicates that there is no work to do in this function unless those conditions are met.
You should strongly consider using an object to map your flatFeature names to the objects encountered with greatest expiration date.  That eliminates the looping find() operation to determine if an existing flatFeature with that name has already been encountered, making this a constant time lookup.  This may not be a big deal if there are 100 subscriptions being iterated, but what if there are 1 million?
Taking above approach of building a name-keyed object gives you flexibility to continue to work against those keyed feature names or you can simply use Object.values(obj) to get the array of feature objects.
Stylistically, your code looks somewhat "dense" and has long horizontal lines (especially your ternary) that make code harder to read and understand at a glance.  Using javascript's destructuring can oftentimes help in keeping your code more terse and thus more easily read.

Putting it together, I might use something like:
const features = allActiveSubscriptions.reduce(
  (obj, sub) => {
    const { plan: { featuresList }, expDate } = sub;

    if (!featuresList) return obj;

    featuresList.forEach( ({ name }) => {
      const existFeat = obj[name];

      if (!existFeat) {
        obj[name] = { name, expDate };
        return;
      }

      if (expDate > existFeat.expDate) {
        existFeat.expDate = expDate;
      }
    });

    return obj;
  },
  {}
);

// if you want an array
const featArray = Object.values(features);

In terms of worst-case operational efficiency, this makes your function perform in O(m x n) as opposed to O(m x n x p) as it is currently (where m is number of subscriptions, n is maximum feature list size, and p number of unique feature names).