Goldbach's Conjecture I am doing an exercise on finding the nearest pair of prime numbers p₁ and p₂ such that:

p₁ ≤ p₂
p₁ + p₂ = n (for 4 ≤ n ≤ 10⁷)
p₁ and p₂ are primes
p₂ - p₁ is minimal

I think it should work perfectly except that it got a TLE. Can someone point out how can I avoid it?

import math
import random

def _try_composite(a, d, n, s):
    if pow(a, d, n) == 1:
        return False
    for i in range(s):
        if pow(a, 2 ** i * d, n) == n - 1:
            return False
    return True  # n  is definitely composite

def is_prime(n, _precision_for_huge_n=16):
    if n in _known_primes:
        return True
    if n in (0, 1) or any((n % p) == 0 for p in _known_primes):
        return False
    d, s = n - 1, 0
    while not d % 2:
        d, s = d >> 1, s + 1
    # Returns exact according to http://primes.utm.edu/prove/prove2_3.html
    if n < 1373653:
        return not any(_try_composite(a, d, n, s) for a in (2, 3))
    if n < 25326001:
        return not any(_try_composite(a, d, n, s) for a in (2, 3, 5))

_known_primes = [2, 3]
_known_primes += [x for x in range(5, 1000, 2) if is_prime(x)]

def main():
    n = int(input())
    if n > 4:
        for smaller in range(n // 2, -1, -1):
            if n - smaller >= smaller:
                if is_prime(n - smaller) and is_prime(smaller):
                    print (smaller, n - smaller)
                    flag = True
                    break
    else:
        print ('2 2')

main()

My solution consists of a single change:

Changed the is_prime function into a prime sieve (from this answer) + lookup

My final code (Also handled odd numbers just for fun, definitely not because I misread the question):

def main():
    import itertools
    import math
    izip = itertools.zip_longest
    chain = itertools.chain.from_iterable
    compress = itertools.compress
    def rwh_primes2_python3(n):
        """ Input n>=6, Returns a list of primes, 2 <= p < n """
        zero = bytearray([False])
        size = n//3 + (n % 6 == 2)
        sieve = bytearray([True]) * size
        sieve[0] = False
        for i in range(int(n**0.5)//3+1):
          if sieve[i]:
            k=3*i+1|1
            start = (k*k+4*k-2*k*(i&1))//3
            sieve[(k*k)//3::2*k]=zero*((size - (k*k)//3 - 1) // (2 * k) + 1)
            sieve[  start ::2*k]=zero*((size -   start  - 1) // (2 * k) + 1)
        ans = [2,3]
        poss = chain(izip(*[range(i, n, 6) for i in (1,5)]))
        ans.extend(compress(poss, sieve))
        return ans

    string2 = "Impossible"
    n = int(input())
    sieve = [t for t in rwh_primes2_python3(n) if t <= math.floor((n // 2) / 2) * 2 + 1][::-1]
    another = [t for t in rwh_primes2_python3(n) if t >= math.floor((n // 2) / 2) * 2 + 1]

    if n > 5 and n % 2 == 0:
        for smaller in sieve:
            if n - smaller in another:
                print (smaller, n - smaller)
                break
    elif n % 2 == 1 and n != 5:
        if n - 2 in another or n-2 in sieve: print (2, n-2)
        else: print ('Impossible')
    elif n == 4:
        print ('2 2')
    else:
        print ('2 3')

main()


Most idiomatic way of declaring a deck in Rust I'm new at Rust and I'm trying to implement a cards' deck on Rust, I'm trying to use most idiomatic code I can, but since I'm new at this language, I'd like to hear advice from more experienced programmers. There's my code.
(P.D: Thanks in advance! :>)

use std::fmt;

use rand::seq::SliceRandom;
use rand::thread_rng;

#[derive(PartialEq)]
pub struct Card<'a> {
    name: &'a str,
    value: i8,
    suit: char,
}

impl<'a> fmt::Debug for Card<'a> {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "{} of {}", self.name, self.suit)
    }
}

pub struct Deck<'a> {
    pub cards: Vec<Card<'a>>,
}

impl<'a> Deck<'a> {
    const SUITS: [char; 4] = ['♥', '♦', '♣', '♠'];
    const CARDS: [(&'a str, i8); 13] = [
        ("ACE", 11),
        ("TWO", 2),
        ("THREE", 3),
        ("FOUR", 4),
        ("FIVE", 5),
        ("SIX", 6),
        ("SEVEN", 7),
        ("EIGHT", 8),
        ("NINE", 9),
        ("TEN", 10),
        ("JACK", 10),
        ("QUEEN", 10),
        ("KING", 10),
    ];

    pub fn new() -> Deck<'a> {
        let mut deck: Vec<Card> = Vec::new();

        for suit in Deck::SUITS.iter() {
            for (card_name, card_value) in Deck::CARDS.iter() {
                deck.push(Card {
                    name: card_name,
                    value: *card_value,
                    suit: *suit,
                });
            }
        }
        let mut rng = thread_rng();
        deck.shuffle(&mut rng);

        Deck { cards: deck }
    }

    pub fn deal_card(&mut self) -> Card<'a> {
        if self.cards.is_empty() {
            self.initialize_deck()
        }
        self.cards.pop().unwrap()
    }

    pub fn get_initial_cards(&mut self) -> Vec<Card> {
        vec![self.deal_card(), self.deal_card()]
    }
    /// When the cards vector is empty, shuffle a new deck
    fn initialize_deck(&mut self) {
        *self = Deck::new();
    }
}

EDIT: This module is meaned to be used with a BlackJack game, that is the reason because the cards have those particular values

You’ve made a fairly classic mistake when it comes to modeling cards in software. Cards have different values depending on the game. In some games, an Ace == 1, others 11, sometimes it could be either in the same game (Blackjack for example). Non-face cards aren’t always their value either. In some games, they’re all “5 points”, regardless of the number on the card.

In short, the value of a particular card depends on the game, not the card. How might you redesign to account for that?


Validating required fields I basically check for required input fields and if they fail to validate, I need to go to another level of output, check for other required fields, and so on...

I can't get my head around this easy step-by-step validation. I already had it in a while-loop, but found it even more horrible than with beginner-like if-cases. I still think, though, that it could be written a bit simpler (and maybe improve readability).

It already is a quite simple thing to do, but in the course of development it will get bigger and bigger, so it needs to stay simple.

    // CHECK FOR REQUIRED PROPERTIES
    if($properties['step'] == 2)
    {
        $req = array('cshort','cid');
        foreach($req as $required)
        {
            if(!validate_var($req,$properties[$req]) || empty($properties[$req]))
            {
                $step = 1;
            }

        }
    }
    if($properties['step'] == 1)
    {
        $req = array('cshort');
        foreach($req as $required)
        {
            if(!validate_var($req,$properties[$req]) || empty($properties[$req]))
            {
                $step = 1;
                return $st
            }

        }
    }

Why do you only return a value if $properties['step'] == 1? If you could somehow change it so that both return, or both didn't need to, you could make it neater.

$req = array('cshort');
$return = FALSE;

if($properties['step'] == 1) {
    $req[] = 'cid';
    $return = TRUE;
}

foreach($req as $required) {
    $step = 1;
    if($return) {
        return $st;
        /*
          $step? what is this where did it come from?
          Unless you are in a function, you don't need to return.
          Just use $st/$step when you need it since you only ever
          gave it one value. Just check if it isset first.
          If you want to break from the loop, just use break,
          or continue to skip.
        */
    }
}

You mentioned expanding. If this gets too much bigger you could try a switch statement instead of multiple if statements. Assuming the rest don't use the same format, otherwise just tweaking what I gave above should work.

switch($properties['step']) {
    case 1:
        foreach($req as $required) {
        }
    break;
    case 2:
        $req[] = 'cid';
        foreach($req as $required) {
        }
    break;
}


Function for encoding strings in-place I recently wrote a function that replaces every white space with '%20', just for fun (and sharping my coding skills). The input string is terminated with extra white spaces, with the length that the string should have after encoding takes place. For example:

input = "Not encoded  " //2 white spaces in the end
output = "Not%20encoded"

Although I was able to implement such encoding in-place, I was still not pleased with the result, specially due to the inner loop that shifts the whole string to the right everytime a white space is found. Here is my solution, written in C#:

    public string encode(string str)
    {
        var array = str.ToCharArray();
        for (int i = 0; i < array.Length; i++) {
            if (array[i] != ' ') continue;

            //switch every char >> 2, from the end of the string to i
            for (int j = array.Length - 1; j > i; j--) {
                array[j] = array[j - 2];
            }

            array[i++] = '%';
            array[i++] = '2';
            array[i] = '0';
        }
        return new String(array);
    }

I'm looking for a solution that operates in O(n), possibly performing a single traversal on the input string. Additional feedback on how to improve performance/readability of my function are always very appreciated :)

Your solution:

copies the whole string twice (1. string.ToCharArray() 2. new string())
needs n bytes of extra storage (apart from the input and output; n is the size of the output)
potentially moves the characters in the string a lot (O(n2))
requires the input to already have free space for the expansion

Solution using StringBuilder:

copies the whole string twice (1. calls to Append() 2. StringBuilder.ToString())
needs about n bytes of extra storage
doesn't need to move characters at all
doesn't require the input to be in any special format

Based on this, StringBuilder is clearly the better solution.

But there is even better solution: just use string.Replace(). That's likely going to be quite efficient and it's much shorter than any other of the proposed alternatives.


Find related groups among list of users I have one of my first PowerShell scripts that will query a list of users in Active Directory. While doing so it will keep a tally in a hashtable that will update for every group increasing it count so that a final total can then be displayed.

This is used so that given a group of users that are performing the same job I can create a role based security group in AD and move all the related groups into it. Thereby reducing the amount of work and thought involved when creating new users and making changes to existing departments.

<#
    Script     : Count Group Memberships
    Created By : Matt Something
    Created    : November 28, 2012
    Modified   : November 28, 2012

    This script will take a list of samaccountnames from active directory and count the membership lists between them. This
    is used to track the similarities between accounts.
#>

param (
    [parameter(ValueFromRemainingArguments=$true)]
    [string[]]$userNamesToCheck = $(throw "Need to supply at least one username")
)

# Functions and Subroutines
Function Get-NamedModule {
    Param([string]$name)
    if(-not(Get-Module -name $name)) {
        if(Get-Module -ListAvailable | Where-Object { $_.name -eq $name }) {
            # Check the list of available module for $name. If present import it.
            Import-Module -Name $name
            $true
        }
        else { $false } # Module not available
    } # End if
    else { $true } # Module already loaded
} # End function

# Variable Declaration
$counts = @{}

# Main
# Import the activedirectory module if its not already.
[void](Get-NamedModule -name "activedirectory")

# Cycle through usernamestocheck and get their group memberships
$userNamesToCheck | ForEach-Object{
    # Get the group memberships from each user.
    $memberships = (Get-AdUser -Filter { SamAccountName -eq $_ } -Properties MemberOf).MemberOf

    # Add the groups to a table for counting
    $memberships | ForEach-Object{
        [void]($_ -match "^CN=(?<membership>.*?),")

        # Continue to build the table
        $singleGroup = $matches.membership

        If($counts.ContainsKey($singleGroup)){
            # This group already exists in the table.
            $counts.Set_Item($singleGroup, [int]($counts.($singleGroup)) + 1)
        } else {
            $counts.Add($singleGroup, 1)
        }
    }
}

Write-Host "$((Measure-Object -InputObject $userNamesToCheck).Count) users checked"
$userNamesToCheck
$counts.GetEnumerator() | Sort-Object -Property Value -Descending

Purposeful PowerShell Help

I see that you have some comments in your script. Those for the most part would be hidden inside the script and the only way someone would see them is to open it. This may or not be what you intended but PowerShell has its own help based comment system that can contain this information as well as make it easy for running users to be informed.

About the parameters

You are using throw with the idea that you want the parameter to be mandatory with a message to show. That functionality is better handled by advanced function parameters

param (
    [parameter(Mandatory=$true,
        HelpMessage='Expecting an array of samaccountnames')]
    [string[]]$userNames
)

Mandatory is self explanatory and HelpMessage

  Windows PowerShell displays this message in the prompt that appears when a mandatory parameter value is missing from a command.

Module checker

Obviously this script is dependent on the activedirectory module being present. You have created a function that will load the module if it has not been already. The is some improvement that can be made of this function. Namely no sense in checking twice for the module presence. It is also good practice to use approve verbs properly. Get could be misleading in this case when in fact you are trying to Confirm

After looking at it a little more this can be reduced to one line that can be evaluated as a boolean which should accomplish the same thing. If you choose to keep the function keep in mind the points I made above. We only need it once so it does not really need to be in a function. Perhaps it should be in a local PowerShell profile.

if(!(Import-Module -Name $name -PassThru -ErrorAction SilentlyContinue)){
    # Script should exit will useful message as we cannot continue.
}

There are 3 possible scenarios for when this is run.

Module $name is not loaded but available: Command will execute and import the module $name. Once imported the module object will be returned because of -PassThru. The command is cast as [boolean] would return true.
Module $name is loaded:  The module object will be returned because of -PassThru. The command is cast as [boolean] would return true.
Module $name is not available: The command will silently fail and $null should be returned. The command is cast as [boolean] would return false.

Grouping Logic

The way the hashtable is built here obviously works but can be completely replaced with the use of the cmdlet Group-Object. I would suspect that, at the time, you were not aware of it. It would handle the grouping and the counts that we need for this. Also there is no test to see if the users actually exist. Need to account for that else the script will just hard fail.

You can see this in the completed solution below.

Updated solution

All the points collected above are used down here.

<#
.SYNOPSIS
Get and groups AD groups of specified users

.DESCRIPTION
This script will take a list of samaccountnames from
active directory and count the membership lists between them.
This can be used to track the group similarities between accounts.

.PARAMETER Users
Specifies the array of samaccount names to be looked up.

.EXAMPLE
C:\PS> extension -Users jsmith, bjangles
#>

param (
    [parameter(Mandatory=$true,
        HelpMessage='Expecting an array of samaccountnames')]
    [string[]]$Users
)

if(!(Import-Module -Name activedirectory -PassThru -ErrorAction SilentlyContinue)){
    # Script should exit will useful message as we cannot continue.
    throw("The module $name must be present in order to continue.")
}

$userMemberships = ForEach($user in $users){
    try{
        # Get the memberships of this user.
        #Since it is possible the user has no groups we need to separate the logic for that and failure
        (Get-Aduser -Identity $user -Properties memberOf -ErrorAction SilentlyContinue).memberof | ForEach-Object{
            [pscustomobject]@{
                User = $User
                Group = $_
            }
        }
    } catch {
        Write-Warning "Error retriving the group membership of $user"
    }
}

# Collect all the groups and display counts
$userMemberships |
    Group-Object -Property Group |
    Select @{Name="Group";Expression={(Get-ADGroup $_.Name).Name}},Count |
    Sort-Object Count -Descending

# Output the number of users check so the output has context
Write-Host "$($users.Count) users checked" -ForegroundColor Green

Minor comments

Principal Group Members

For me this is just the Domain Users group. Potentially in other orgs they use the principal group member for something else.

Accounting for bad users

You might consider adding more verbose output concerning errors. If there is a user that could not be queried then the last line could be considered faulty. I would think the warning would be indication enough.

Calculated Expression

You will see a calculated expression used in the final output. The groups returned from Get-ADUser contain the full DistinguisedName of the groups. Using a calculated expression we just get the name from the dn. While you could use string manipulation the cmdlet is more efficient and precise.


Adding two numbers represented as linked lists I have some imminent interviews and want to sharpen my game before going into them. I'm running through some practice problems. This LeetCode challenge is to add two numbers represented as linked lists.

If you can critique this solution quite harshly, as though you would in a full-fledged SWE interview, I'd greatly appreciate it. I'm particularly concerned about memory usage and variable names.

# Definition for singly-linked list.
# class ListNode:
#     def __init__(self, x):
#         self.val = x
#         self.next = None

class Solution:
    def addTwoNumbers(self, l1: ListNode, l2: ListNode) -> ListNode:
        final_l = None
        curr_l = None
        remainder = 0
        while l1 or l2:
            digit = 0
            if l1:
                digit += l1.val
                l1 = l1.next

            if l2:
                digit += l2.val
                l2 = l2.next

            if remainder != 0:
                digit += remainder

            remainder = digit // 10
            digit = digit % 10

            if final_l is None:
                final_l = ListNode(digit)
                curr_l = final_l
            else:
                curr_l.next = ListNode(digit)
                curr_l = curr_l.next

        if remainder > 0:
            curr_l.next = ListNode(remainder)

        return final_l

A bit of scaffolding

In order to test/review your code, I had to write a bit of additional code. In case it can be relevant to you or other reviewers, here it is:

# Definition for singly-linked list.
class ListNode:
    def __init__(self, x):
        self.val = x
        self.next = None

    def __eq__(self, other):
        ret = other is not None and self.val == other.val and self.next == other.next
        # print(self, other, ret)
        return ret

    @classmethod
    def from_list(cls, l):
        ret = ln = cls(l.pop(0))
        while l:
            e = l.pop(0)
            ln.next = cls(e)
            ln = ln.next
        return ret

    def to_list(self):
        l = [self.val]
        return l if self.next is None else l + self.next.to_list()

class Solution:
    ...

    @staticmethod
    def ListNodeFromInt(n):
        return ListNode.from_list([int(d) for d in reversed(str(n))])

    @staticmethod
    def testAddTwoNumbersUsingNumbers(n1, n2):
        l1 = Solution.ListNodeFromInt(n1)
        l2 = Solution.ListNodeFromInt(n2)
        expected_add = Solution.ListNodeFromInt(n1 + n2)
        add = Solution().addTwoNumbers(l1, l2)
        print(n1, n2, n1 + n2, expected_add.to_list(), add.to_list())
        assert expected_add == add

    @staticmethod
    def unitTests():
        # Edge cases
        Solution.testAddTwoNumbersUsingNumbers(0, 0)
        Solution.testAddTwoNumbersUsingNumbers(342, 0)
        Solution.testAddTwoNumbersUsingNumbers(0, 342)
        # Same length
        Solution.testAddTwoNumbersUsingNumbers(342, 465)
        # Different length
        Solution.testAddTwoNumbersUsingNumbers(342, 46)
        # Return longer than input
        Solution.testAddTwoNumbersUsingNumbers(999, 999)

Solution.unitTests()

This is pretty poorly organised but it is quick and dirty.
Now starts the actual review

Overall review

Your code looks good. The API is a bit awkward but it is a limitation from the programming challenge platform.

A few details can be improved anyway.

Remove non-required checks

Instead of:

        if remainder != 0:
            digit += remainder

You can write:

        digit += remainder

Use builtins

The builtin divmod is not the most famous but it is convenient for a pretty usual task: compute both the quotient and the remainder.

Instead of:

        remainder = digit // 10
        digit = digit % 10

You can write:

        remainder, digit = divmod(digit, 10)

Different strategy

Instead of having a function to perform all the steps in one go, you could split the problem: define a function computing the sum of the 2 list as an integer and a function to convert that number into a list. Adding values bigger than 9 may be what we are trying to avoid though...


Passing variables by values without the dreaded if/switch/eval I have to pass a bunch of inputs and outputs between two Angular controllers and I've come up with a way to do it using $broadcast.  I'm using an "if" statement to make sure the inputs match the outputs, but there must be an easier way.

app.controller('Ctrl2', ['$scope', '$rootScope', function($scope, $rootScope) {
  var myArray = [$scope.input1, $scope.input2];
  $scope.textChange = function(whichChange) {
    if (whichChange == 1) {
      $rootScope.$broadcast('textChanged', $scope.input1, 1);
    } else if (whichChange == 2) {
      $rootScope.$broadcast('textChanged', $scope.input2, 2);
    } //else if, else if, else if, etc...
  };
}]);

But seems there should be a way I can replace that if statement with a single line, something like:

$rootScope.$broadcast('textChanged', $scope.relevantInput, relevantOutput);

Working Plunkr here

Same question applies to the case statement in controller Ctrl1, but I figure answering the question for one controller would answer it for the other.

Also: while comments on whether this is the best way to do this in Angular are certainly welcome my question is more about avoiding if/switch/eval.

Now The Angular Way™ to do this is to push off the data to a higher scope visible to both controllers and let two-way binding do its thing. This is typically done by using a parent controller. Scope operates similar to prototypal inheritance. If it's not in the child scope, it looks for it in the parent scope.

var app = angular.module('plunker', []);

app.controller('ParentController', function($scope) {
  $scope.data = {
    data1: 'data1',
    data2: 'data2',
  }
});

app.controller('Ctrl1', function($scope) {
  // Stuff local to Ctrl 1
});

app.controller('Ctrl2', function($scope) {
  // Stuff local to Ctrl 2
});

      Output 1: {{data.data1}}

Output 2: {{data.data2}}

      Input 1:
      <input type='text' ng-model='data.data1'>

Input 2:
      <input type='text' ng-model='data.data2'>

This works for small apps. When your apps become large, it's better if you use a service/factory/provider. Use it to hold your data, expose an API to mutate the data and make it emit events when data changes. That way, controllers can pull them in as dependencies, call the API and listen to data changes.

As for other things, Angular allows you to define controllers with implicit dependency format. It uses the name of the variables introduced to the function as the names of the dependencies.


Reading many kinds of numbers from a byte buffer I've been trying to find a way to collapse all these methods into one.

Current I have these methods

default byte readByte(long address) {
    return read(address, 1).get();
}

default int readShort(long address) {
    return read(address, 2).getShort();
}

default int readInt(long address) {
    return read(address, 4).getInt();
}

default long readLong(long address) {
    return read(address, 8).getLong();
}

default float readFloat(long address) {
    return read(address, 4).getFloat();
}

default double readDouble(long address) {
    return read(address, 8).getDouble();
}

They are used to read a certain amount of bytes of a memory address (ex 8 bytes for long, 4 for int, 2 for short, etc).

I find using the multiple methods is not very pleasing to look at.

In theory I'd like a solution similar to something like this:

default <T> T read(long address) {
  if (T instanceof Long) {

      return read(address, 8).getLong();
  } else if (T instanceof Integer) {

      return read(address, 4).getInt();
  } else if (T instanceof Short) {

      return read(address, 2).getShort();
  } else if (T instanceof Byte) {

      return read(address, 1).get();
  } else if (T instanceof Double) {

      return read(address, 8).getDouble();
  } else if (T instanceof Float) {

      return read(address, 4).getFloat();
  } else if (T instanceof Boolean) {

      return read(address, 1).get() == 1;
  }

  throw new RuntimeException("Unknown generic type!");
}

But I don't think that is possible due to the limitation of Java generics.

Can you think of a better/less redundant solution?

There is only ONE problem that will be hard to eliminate because I assert that "ByteBuffer" is not within your domain. And this problem is not related to redundancy in the manner of duplicate code. The bytes read can be derived from the method called:

read(address, 4).getInt();

So if you have any possibility to use "4" within "getInt()" then your code improves a little bit. Maybe extract the magic numbers as constants.

Everything else you assume is of an error type 2. Assuming redundancy where there is not redundancy and acting unnecessarily. So everything you do to the code trying to eliminate redundancy will introduce problems.

For example:

Your generic suggestion will divide responsibility into half. The caller has to synchronize the expected return type with the called method. In the original solution there is no necessity to care.

Leave the code presented as it is. There is no possibility to improve it in the way you expect it.


Should I unpack a Maybe with maybe or pass value around I'm beginning (for the third time in 5 years) to learn Haskell and decided to do a "More or less" console game (game pick a random number, you then have to guess which).
First the code, then my question :
import System.Random
import Text.Read(readMaybe)

main :: IO ()
main = do
  gen <- getStdGen
  let maxSecret = 1000
  let (secret, newGen) = randomR (1,maxSecret) gen :: (Int, StdGen)

  -- putStrLn $ show secret
  let oracle = compareToSecret secret
  askForNumber oracle 1 (1, maxSecret)

data Guess = SecretIsLess | SecretIsMore | Win deriving (Show)

askForNumber :: (Int -> Guess) -> Int -> (Int, Int) -> IO ()
askForNumber oracle tries range = do
  let (min, max) = range
  putStrLn $ "Guess the number between " ++ (show min) ++ " and " ++ (show max) ++ " !"
  guessInput <- getLine
  let guess = readMaybe guessInput :: Maybe Int

  case (oracle) <$> guess of
            Just SecretIsMore -> do
                  putStrLn "Guess more !"
                  askForNumber oracle (tries+1) (maybe min id guess, max)
            Just SecretIsLess -> do
                  putStrLn "Guess less !"
                  askForNumber oracle (tries+1) (min, maybe max id guess)
            Just Win -> do
                  putStrLn "You win !"
                  putStrLn $ "It took you " ++ (show tries) ++ " tries."
                  return ()
            Nothing -> do
                  putStrLn "That is not a number"
                  askForNumber oracle tries range

compareToSecret :: Int -> Int -> Guess
compareToSecret secret guess
  | secret > guess = SecretIsMore
  | secret < guess = SecretIsLess
  | secret == guess = Win

I'd like to improve that code to make it idiomatic and learn in the process.
I made a choice : at first I had
compareToSecret :: Int -> Int -> (Guess, Int)
and in the case of I had something like
Just (SecretIsMore, newmin) -> do askForNumber oracle (tries+1) (newmin, max)
Do you think it's better to kind of unpack with maybe there or to pass the value around and keep it pure ? Or is there another solution ?

Prelude
I think your code (sans bug) is fine. Your question is already a little nit-picky so my comments will mostly be nit-picks. I am not a Haskell expert either, so don't place too much authority in my comments. Evaluate them for yourself.
My comments are in a somewhat arbitrary order, but I've organized them with headers. The section pertaining specifically to your question is titled "What to do with the case".
The revised code is included at the end.
The Guess datatype
A datatype that is used precisely like this already exists, it's called Ordering and can be found in Data.Ord (although I believe it's included by default). No need to reinvent the wheel here. Especially because you'll see later that you can use already-made functions that work with Ordering.
Input validation
You correctly check that the input is a well-formed number, but you don't check that it's between the min and max value of the range. Here's an example of how this bug manifests itself.
Guess the number between 1 and 1000 !
50
Guess more !
Guess the number between 50 and 1000 !
20
Guess more !
Guess the number between 20 and 1000 !

Notice how the value goes down. The fix, which I will put in the section addressing your questions about Maybe, is to add another case that deals with a well-formed, but invalid guess.
Syntax and other small details
min and max are built-in functions, it's best not to shadow them. I would call them minValue and maxValue.
You can pattern match in function definitions so that you don't need to unpack range – you can instead have
askForNumber oracle tries (minValue, maxValue) = do
...

Function application has the highest precedence in Haskell, so you don't need parens around (show min). (oracle) <$> guess doesn't need the parens either.
maybe value id = fromMaybe value, so use that instead. You can get fromMaybe from Data.Maybe, but it might be included by default.
oracle
If you switch to using Ordering, you can nix your compareToSecret function and define oracle = compare secret.
What to do with the case
I think it's kind of clunky to extract a value from a Maybe after you've cased on it, which is what you're doing right now. A big utility of case is knowing that your value must be of a certain form in each branch. In that branch, guess must be of the form Just _. While you as a programmer know this to be true, it's much more helpful to have the type system be able to prove this.
As I see it, there are two ways to handle your case statement.
The first is to have two nested case statements. The first branches on guess. If guess is Nothing, it prompts the user for another input. If guess is Just g, it cases on oracle g.
The second is to do a slight modification of what you're doing. Instead of fmapping oracle, you can fmap the function \g -> (oracle g, g). This allows you to extract the guess safely without having to use Maybe. I believe this is what you were doing originally.
I prefer to avoid nesting cases when I can, so I opted for the second one.
If you would also like to validate the input (I chose to), then you will want to add a case at the beginning matching Just (_, guess) – i.e. one that ignores the comparison – to deal with invalid guesses. This pattern, however, will match both invalid and valid guesses. We need to add a guard that asserts that guess > max or guess < min. This looks like:
    ...
    Just (_, guess) | guess < min || guess > max -> do
      putStrLn "Guess out of range"
      askForNumber oracle tries range
    ...

Small note on using oracle
I use the function \g -> (oracle g, g) which might feel slightly clunky. Indeed, there are a few alternatives, but I warn you that they are somewhat arcane. In general, I would avoid using "cute" or "slick" code like this unless you know everyone who sets eyes on the code will understand it.
All of the below are equivalent (for your use case):
\g -> (oracle g, g)    -- plain definition
oracle &&& id          -- (&&&) needs to be imported from Control.Arrow
(,) =<< oracle         -- using the Reader Monad
(,) <$> oracle <*> id  -- using the Reader Applicative

I would be most inclined to use oracle &&& id because it'd be the most recognizable, but even it is a little arcane. I pretty much only included the last example because you'll see code that looks like f <$> g <*> h <*> i … a lot; it's a common idiom for other Applicatives (but I would advise against using it in this case). No matter what people say or do, there's nothing wrong with writing "plain" or "simple" Haskell.
Revised code
You'll find that not too much is different.
import System.Random
import Text.Read (readMaybe)

main :: IO ()
main = do
  gen <- getStdGen
  let maxSecret = 1000
  let (secret, newGen) = randomR (1,maxSecret) gen :: (Int, StdGen)

  -- putStrLn $ show secret
  let oracle = compare secret
  askForNumber oracle 1 (1, maxSecret)

askForNumber :: (Int -> Ordering) -> Int -> (Int, Int) -> IO ()
askForNumber oracle tries range@(minValue, maxValue) = do
  putStrLn $ "Guess the number between " ++ show minValue ++ " and " ++ show maxValue ++ " !"
  guessInput <- getLine
  let guessMaybe = readMaybe guessInput :: Maybe Int

  case (\guess -> (oracle guess, guess)) <$> guessMaybe of
    Nothing -> do
      putStrLn "That is not a number"
      askForNumber oracle tries range
    Just (_, guess) | guess < minValue || guess > maxValue -> do
      putStrLn "Guess out of range"
      askForNumber oracle tries range
    Just (LT, guess) -> do
      putStrLn "Guess less !"
      askForNumber oracle (tries+1) (minValue, guess)
    Just (GT, guess) -> do
      putStrLn "Guess more !"
      askForNumber oracle (tries+1) (guess, maxValue)
    Just (EQ, _) -> do
      putStrLn "You win !"
      putStrLn $ "It took you " ++ (show tries) ++ " tries."
```


Drop-down menu button highlighting and hiding I'm learning JS/jQuery at the moment and have built the following standalone script but I feel that it can probably be improved in some ways?

For example - is there a better way of switching the selected text on the .dropdown-menu li a click event rather than needing to replace the entire HTML?

Constructive advice welcome.

EDIT: Added HTML to help with context:

    <button type="button" class="button defaultbutton">Default Value </button>

Katie

Richard

Matthew

Sophie

Default Value

    <button type="button" class="button primarybutton">Click Me </button>

Katie

Richard

Matthew

Sophie

So currently the script checks if the user is clicking in a dropdown menu that needs an inverted caret.

$(function() {
// Handles initial dropdown click event, if any dropdown menu is visible close it
$('.dropdowncontain').click(function(e){
    if (!$('.dropdown-menu', this).is(':visible')) {
        $('.dropdown-menu').hide();
    }
    $('.dropdown-menu', this).toggle();
    // Stop the bubbling of the event to the document level
    e.stopPropagation();
});

// Handles hiding the dropdown menu's when the user clicks elsewhere in the document
$(document).click(function() {
    $('.dropdown-menu').hide();
});

// Handles updating the text in the button and highlighting the selected option
$('.dropdown-menu li a').click(function(e){
    var parentBtn = $(this).parents('.dropdowncontain').find('.defaultbutton');
    var that = $(this).parents('.dropdowncontain').find('.dropdown-menu li a');
    var selectedText = $(this).text();

        if (that.hasClass('highlightdropdown')){
            that.removeClass('highlightdropdown');
        }

    $(this).addClass('highlightdropdown');

    // If the containing parent div has a class with 'defaultbutton' add the invert class to the caret
    if (parentBtn.hasClass('defaultbutton')) {
        $(this).parents('.dropdowncontain').find('.button').html(selectedText+' ');
    }
    else {
        $(this).parents('.dropdowncontain').find('.button').html(selectedText+' ');
    }
    // Prevent the page jumping to the top when a link with no source is selected
    e.preventDefault();
});
});

There is no reason to swap the text, find the span in the button and toggle the class

var isDefault = parentBtn.hasClass('defaultbutton')
$(this).parents('.dropdowncontain').find('.button .caret').toggleClass("invert", isDefault);

The context selector is a bad idea

$('.dropdown-menu', this)

It is slow. Use find()

$(this).find('.dropdown-menu')

Another thing, $(this) is used over and over again. That is bad practice. It creates a new jQuery object each time. Store it in a variable and reuse that variable.

var myElement = $(this);
myElement.find('.dropdown-menu')...


Is there a shorter way to write the following if statements? I have the following code which uses four if statements to check the value in four different combo boxes.

if (!String.IsNullOrEmpty(tx_cmb_Circuits1.Text.Trim()))
    emp.CircuitEmail_Group_Rels.Add(
        new Circuit_Email_Group_Rel
        {
            Circuits = (Circuits) tx_cmb_Circuits1.SelectedItem,
            Group = emp
        });
if (!String.IsNullOrEmpty(tx_cmb_Circuits2.Text.Trim()))
    emp.CircuitEmail_Group_Rels.Add(
        new Circuit_Email_Group_Rel
        {
            Circuits = (Circuits) tx_cmb_Circuits2.SelectedItem,
            Group = emp
        });
if (!String.IsNullOrEmpty(tx_cmb_Circuits3.Text.Trim()))
    emp.CircuitEmail_Group_Rels.Add(
        new Circuit_Email_Group_Rel
        {
            Circuits = (Circuits) tx_cmb_Circuits3.SelectedItem,
            Group = emp
        });
if (!String.IsNullOrEmpty(tx_cmb_Circuits4.Text.Trim()))
    emp.CircuitEmail_Group_Rels.Add(
        new Circuit_Email_Group_Rel
        {
            Circuits = (Circuits) tx_cmb_Circuits4.SelectedItem,
            Group = emp
        });

Is there are shorter way to do the same. Please review my code.

Add all items to a list and do a foreach instead.

var buttons = new List<Button>
{
    tx_cmb_Circuits1,
    tx_cmb_Circuits2,
    ...
};

foreach ( var button in buttons )
{
    if ( !String.IsNullOrEmpty( button.Text.Trim() ) )
    {
        emp.CircuitEmail_Group_Rels.Add(
            new Circuit_Email_Group_Rel
            {
                Circuits = (Circuits) button.SelectedItem,
                Group = emp
            } );
    }
}

P.S.: Consider giving your variables better names. No real need for abbreviations like tx_cmb nowadays.


Printing hex dumps for diagnostics I have developed some free standing functions to provide hex dumps for buffer (std::string) contents.

You can see it working in action here.

The purpose is for debugging diagnostics to print buffer contents to a (logging) stream, in a format similar as wireshark does.

Any proposals for improvements are welcome.

Here's the code:

#include <iostream>
#include <iomanip>
#include <cctype>

std::ostream& render_printable_chars(std::ostream& os, const char* buffer, size_t bufsize) {
    os << " | ";
    for (size_t i = 0; i < bufsize; ++i)
    {
        if (std::isprint(buffer[i]))
        {
            os << buffer[i];
        }
        else
        {
            os << ".";
        }
    }
    return os;
}

std::ostream& hex_dump(std::ostream& os, const uint8_t* buffer, size_t bufsize, bool showPrintableChars = true)
{
    auto oldFormat = os.flags();
    auto oldFillChar = os.fill();

    os << std::hex;
    os.fill('0');
    bool printBlank = false;
    size_t i = 0;
    for (; i < bufsize; ++i)
    {
        if (i % 8 == 0)
        {
            if (i != 0 && showPrintableChars)
            {
                render_printable_chars(os, reinterpret_cast<const char*>(&buffer[i] - 8), 8);
            }
            os << std::endl;
            printBlank = false;
        }
        if (printBlank)
        {
            os << ' ';
        }
        os << std::setw(2) << std::right << unsigned(buffer[i]);
        if (!printBlank)
        {
            printBlank = true;
        }
    }
    if (i % 8 != 0 && showPrintableChars)
    {
        for (size_t j = 0; j < 8 - (i % 8); ++j)
        {
            os << "   ";
        }
        render_printable_chars(os, reinterpret_cast<const char*>(&buffer[i] - (i % 8)), (i % 8));
    }

    os << std::endl;

    os.fill(oldFillChar);
    os.flags(oldFormat);

    return os;
}

std::ostream& hex_dump(std::ostream& os, const std::string& buffer, bool showPrintableChars = true)
{
    return hex_dump(os,reinterpret_cast<const uint8_t*>(buffer.data()), buffer.length(),showPrintableChars);
}

Here's the test case:

int main()
{
    const char test[] = "abcdef123456\0zyxwvu987654";
    std::string s(test,sizeof(test));

    hex_dump(std::cout, s);

    return 0;
}

The output looks like expected:

61 62 63 64 65 66 31 32 | abcdef12
33 34 35 36 00 7a 79 78 | 3456.zyx
77 76 75 39 38 37 36 35 | wvu98765
34 00                   | 4.

Did you ever test it with buffers which are multiples of 8 long?
Because the last block won't get the nice printable display.
Are you sure the newline at the beginning of the output should be there?
Should the test-output really contain the implicit 0-terminator?
return 0; is implicit for main().

Modified test-function:

int main()
{
    const char test[] = "abcdef123456\0zyxwvu987654";
    std::string s(test, sizeof(test) - 1);

    std::cout << "---\n";
    hex_dump(std::cout, s);
    std::cout << "---\n";
    hex_dump(std::cout, "");
    std::cout << "---\n";
    hex_dump(std::cout, "12345678");
    std::cout << "---\n";
    hex_dump(std::cout, "1234567812345678");
    std::cout << "---\n";
}

Do you have the opening brace for functions on its own line or not? Seems 2+1 for, 1 against...
If you only want to output a single character, using a character is potentially more efficient than a string.
The conditional operator is excellent for choosing between two values.
Beware of the implementation-defined signedness of plain char. The character-classification-functions inherited from C expect the value of an unsigned char or EOF.
See std::isprint.
Consider marking internal functions static to avoid externally visible symbols and promote inlining.
If you don't actually ever use a return-value, why provide one?

static void render_printable_chars(std::ostream& os, const char* buffer, size_t bufsize)
{
    os << " | ";
    for (size_t i = 0; i < bufsize; ++i)
        os << (std::isprint((unsigned char)buffer[i]) ? buffer[i] : '.');
}

Don't play around with a useless flag just because. Tracing data-flow is quite a lot more involved than following the program-flow.
Testing whether a variable already has a value before setting it to the same is rarely anything but obfuscation and pessimisation.
Don't use std::endl unless you really want to flush. And if that's your intention, consider being more explicit by using std::flush.
If you use C++17, you might want to accept a std::string_view by value instead of a std::string by constant reference. It's generally more efficient.


Html Templating for a WIP static site generator, written in Javascript I wrote this to go in my simple static site generator. it takes any plaintext file (but meant for html) and injects the specified file contents and other data at matching tags. This is my first "big" JavaScript project, and most of my knowledge has come from several-year-old posts on StackExchange, YouTube, and Medium, so I cannot guarantee that I have followed modern conventions.
I have included test code, and It should run as-is however it does require a single index.html file in the same dir to test fileInject().
const fs = require('fs');

var test = `<!-- template:one -->
<!-- template:two -->
<!-- template:three -->
<!-- template:index.html -->
<!-- template:five -->`;

var data = [['one','apple'],['three','orange'],['five','banana']];

// Grab tags from template files. Does not return duplicates.
const getTags = (template) => {
    const regex = /\<\!\-\- template:(?<label>(?:(?!\<\!\-\-).)+) \-\-\>/g;
    var match, matches = [];
    while(match = regex.exec(template)) {
        if(!matches.includes(match)) {
            matches.push([match.groups.label, match[0]]);
        }
    }
    return matches;
}

// Inject data at matching tags. Tag object format: [['key','value'], ...]
// Silent on failure, but mostly because I do not know how to check if template.replace() succeeds or fails.
const dataInject = (template, tags) => {
    for(let tag of tags) {
        const regex = new RegExp(`\<\!\-\- template:${tag[0]} \-\-\>`, 'g');
        template = template.replace(regex, tag[1]);
    }
    return template;
}

// Find and inject file contents at matching tags.
// Logs errors on failure.
const fileInject = (template) => {
    const tags = getTags(template);
    for(let tag of tags) {
        try{
            fs.accessSync(tag[0]);
            const partial = fs.readFileSync(tag[0]).toString();
            template = template.replace(tag[1], partial);
        } catch(e) { console.log(`error injecting file '${tag[0]}'`) }
    }
    return template;
}

// Individual tests
//console.log(dataInject(test, data));
//console.log(fileInject(test));

//combined tests. Both work, however (if combined) dataInject() should be called first as fileInject() will finish quicker encounter fewer failures.
//console.log(dataInject(fileInject(test), data));
console.log(fileInject(dataInject(test, data)));

Here's a few pointers to help out:

I see you're using both let and var in there - be consistent and just choose one.
You're escaping a lot of regular expression characters that don't need to be escaped - it makes the regex less readable. i.e. /\<\!\-\- template:(?<label>(?:(?!\<\!\-\-).)+) \-\-\>/g could be written as /<!-- template:(?<label>(?:(?!<!--).)+) -->/g
Often it's good to name functions as verbs. i.e. just rename dataInject to injectData and fileInject to injectFile, it reads better.
Right now, if there was an error reading the template file, you'll always get the same message logged: error injecting file '<file name>'. It doesn't matter if node didn't have permission to access it, or if the file did not exist, you won't get any more information besides that log message. What's worse, you don't rethrow any error, so the program will continue trying to run even though the template has issues. I would just take out the try-catch completely, the original error usually has the most useful information to help debug problems.
I assume this injection logic is meant to be used by a server. With that use case in mind, you don't want to use the sync versions of the file-system functions - it means the server is unable to handle any other request while it's doing a file operation. Instead of fs.readFileSync(), use fs.readFile(), or even better, fs.promises.readFile().
The if (!matches.includes(match)) line isn't doing anything. matches is an array of arrays while match is a match object, match will never be inside matches.
Any reason why data is a list of lists and not an object (or a Map)? Why not just have data equal { one: 'apple', three: 'orange', ... }. If some function needs it in the list-of-list format, it can just call Object.entries() on it.
Finally, I just want to warn against re-inventing the wheel. There's plenty of great templating systems already out there (i.e. handlebars.js) that come with plenty of great features and built-in XSS protection. If your codebase only needs templates in one or two places, then I can understand preferring to throw together something simple instead of adding a new dependency, but if you're going to be using these templates a lot, it might be better to use a pre-built solution instead of maintaining your own.


HackerRank "Flowers" challenge I'm fairly new to C and wanted a general review of this code. It's a solution to this problem.

  You and your \$K-1\$ friends want to buy \$N\$ flowers. Flower number \$i\$ has
  cost \$c_i\$. Unfortunately the seller does not want just one customer to
  buy a lot of flowers, so he tries to change the price of flowers for
  customers who have already bought some flowers. More precisely, if a
  customer has already bought \$x\$ flowers, he should pay \$(x+1)*c_i\$ dollars
  to buy flower number \$i\$.

  You and your \$K-1\$ friends want to buy all \$N\$ flowers in such a way that
  you spend the least amount of money. You can buy the flowers in any
  order.

  Input:

  The first line of input contains two integers \$N\$ and \$K\$ (\$K \le N\$). The
  next line contains \$N\$ space separated positive integers \$c_1,c_2,\dotsc,c_N\$.

  Output:

  Print the minimum amount of money you (and your friends) have to pay
  in order to buy all \$N\$ flowers.

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

int comp_desc(const void * a, const void * b)
{
    int * aPtr = (int*)a;
    int * bPtr = (int*)b;
    return *bPtr - *aPtr;
}

int main()
{
    int flowersNeeded, numFriends, i;
    scanf("%d %d", &flowersNeeded, &numFriends);
    getchar();
    int flowerCosts[100];
    memset(flowerCosts, 0, sizeof(flowerCosts));
    for (i = 0; i < flowersNeeded; ++i)
    {
        scanf("%d", &flowerCosts[i]);
    }
    qsort(flowerCosts, flowersNeeded, sizeof(int), comp_desc);
    int flowersBought = 0;
    int moneySpent = 0;
    int multiplier = 1;
    for (i = 0; i < flowersNeeded; ++i)
    {
        moneySpent += flowerCosts[i] * multiplier;
        multiplier = (++flowersBought / numFriends) + 1;
    }
    printf("%d\n", moneySpent);
    return 0;
}

You should declare loop variables as close as possible to the loop

int flowersNeeded, numFriends, i;
// ... some code
for (i = 0; i < flowersNeeded; ++i)

The above code causes inefficiency and puts unecessary mental burden on the reader, you should use (in C99):

int flowersNeeded, numFriends;
// ... some code
for (int i = 0; i < flowersNeeded; ++i)

Avoid modifyng in place and assigning at the same time

multiplier = (++flowersBought / numFriends) + 1;

The above line is too much compact, usually you assign to a variable xor you increment a variable in place, I would explode it in two lines:

flowersBought++;
multiplier = (flowersBought / numFriends) + 1;

Sorry for being pedantic

The very same compiler you use to generate the binaries doubles as a code analysis tool:

gcc flowers.c -Wall -pedantic

Warns you about:

h.c: In function ‘main’:
h.c:15:5: warning: ignoring return value of ‘scanf’, declared with attribute warn_unused_result [-Wunused-result]
     scanf("%d %d", &flowersNeeded, &numFriends);
     ^
h.c:21:9: warning: ignoring return value of ‘scanf’, declared with attribute warn_unused_result [-Wunused-result]
         scanf("%d", &flowerCosts[i]);
         ^


Find All the Primes Between two Numbers I wrote a solution to the Coding Challenge SPOJ Prime in C++ using the Sieve of Eratosthenes. How much Ever I tried I could not get over the Time Limit on SPOJ which is 6s for Primes upto 1000000000.

Here is the Problem StateMent

  Input

  The input begins with the number t of test cases in a single line
  (t<=10). In each of the next t lines there are two numbers m and n (1
  <= m <= n <= 1000000000, n-m<=100000) separated by a space.

  Output

  For every test case print all prime numbers p such that m <= p <= n,
  one number per line, test cases separated by an empty line.

  Example

  Input:
    2
    1 10
    3 5

  Output:
    2 3 5 7

  3 5

  Warning: large Input/Output data, be careful with certain
  languages (though most should be OK if the algorithm is well designed)

How can I optimise this Code ?

/*
  * Generate Prime Numbers Between two given numbers.

  * Url : http://www.spoj.com/problems/PRIME1/
  * Dated : 16-Jun-2016

  * Performance Oriented , that's why I'm using C++. The AlgoRythm is the Famous Seive of Euractus
*/

#include <iostream>
#include <stdlib.h>
#include <vector>
#include <math.h>

using namespace std;

typedef long int bignum;
struct input {
    bignum start;
    bignum end;
};

// Functions
std::vector<input> parseInput();
void printPrimes(std::vector<bool> primes , bignum start , bignum end);
std::vector<bool> findPrimes(bignum start , bignum end);

int main(){
    system("clear");

    std::vector<bool> primes;
    std::vector<input> programInput;

    programInput = parseInput();

    for (int i = 0; i < programInput.size(); ++i){
        primes = findPrimes(programInput[i].start, programInput[i].end);
        printPrimes(primes , programInput[i].start, programInput[i].end);
    }
}

// Parse Input as an Array
std::vector<input> parseInput(){
    int testCases;
    bignum start;
    bignum end;

    cin >> testCases;

    std::vector<input> parsedInput(testCases);
    input currentInput;

    for (int i = 0; i < testCases; ++i){
        cin >> start >> end;
        currentInput = {start , end};
        parsedInput.push_back(currentInput);
    }

    return parsedInput;
}

// Print all the Primes
void printPrimes(std::vector<bool> primes , bignum start , bignum end){
    for (int i = start; i <= end; ++i){
        if (primes[i] == true){
            cout << i << '\n';
        }
    }
}

// Find all Prime Numbers between two numbers
std::vector<bool> findPrimes( bignum start , bignum end){

    // Define an array of Bits and Set all Bits to 1
    std::vector<bool> primeSeive(end);

    // Assign all Numbers upto end as True
    primeSeive.assign(end + 1, true);

    // 0 and 1 is not a prime
    primeSeive[0] = false;
    primeSeive[1] = false;

    for (int i = 2; i <= end ; ++i ){
        for (int j = 2; j <= (end / i); ++j){
            primeSeive[i*j] = false;
        }
    }

    return primeSeive;
}

Just do the sieve once, this should give you about 5-10x increase.

int main(){
    system("clear");

    std::vector<bool> primes;
    std::vector<input> programInput;

    programInput = parseInput();

    auto maxPrime = programInput.begin()->end;
    for(auto& m : programInput){
         maxPrime = max(maxPrime, m.end);
    }
    primes = findPrimes(0, maxPrime);

    for (int i = 0; i < programInput.size(); ++i){
        printPrimes(primes , programInput[i].start, programInput[i].end);
    }
}

Then avoid the vector copies by taking all vector parameters by const & and you should be golden.


Read contents of file with appropriate error handling (Rust) I am trying to learn basic Rust. Is this a good way to get the contents of a file? Am I handling errors the best way? Is there any way this could be more performant?

use std::fs::File;
use std::io::Read;

fn main() {
    let path = "bar/foo.txt";

    match read(path) {
        Ok(contents) => println!("{}", contents),
        Err(err) => println!("Unable to read '{}': {}", path, err),
    }
}

fn read(file: &str) -> Result<String, std::io::Error> {
    let mut file = match File::open(file) {
        Ok(mut f) => f,
        Err(err) => return Err(err),
    };

    let mut data = String::new();
    match file.read_to_string(&mut data) {
        Ok(_) => return Ok(data),
        Err(err) => return Err(err),
    }
}

Your code gives a warning:

warning: variable does not need to be mutable
  --> src/main.rs:15:12
   |
15 |         Ok(mut f) => f,
   |            ^^^^^
   |

When you move a value from one binding to another, the two bindings don't have to agree on mutability. It may sound strange, but it's valid to move from an immutable binding to a mutable binding. It's perfectly safe because in order to be able to move a value, there must not be any pointers to it, which ensures that you have exclusive access to the value. Therefore, you could write Ok(f) here instead of Ok(mut f).
The match / return Err pattern is so frequent that Rust has a shorthand for it: the ? operator (and its predecessor, the try! macro). We could rewrite your program using the ? operator like this:

use std::fs::File;
use std::io::Read;

fn main() {
    let path = "bar/foo.txt";

    match read(path) {
        Ok(contents) => println!("{}", contents),
        Err(err) => println!("Unable to read '{}': {}", path, err),
    }
}

fn read(file: &str) -> Result<String, std::io::Error> {
    let mut file = File::open(file)?;

    let mut data = String::new();
    file.read_to_string(&mut data)?;
    Ok(data)
}

The second match in read ends the function, so we don't need the ? operator. Instead, we could use map to replace the data in the Ok variant while keeping the Err the same, and then just return the result of map.

fn read(file: &str) -> Result<String, std::io::Error> {
    let mut file = File::open(file)?;

    let mut data = String::new();
    file.read_to_string(&mut data).map(|_| data)
}

Which one you use is up to you. :)
File::open accepts more than just &str. Its signature is:

fn open<P: AsRef<Path>>(path: P) -> Result<File>

You could make your own read function more generic by introducing a type parameter.

use std::path::Path;

fn read<P: AsRef<Path>>(file: P) -> Result<String, std::io::Error> {
    let mut file = File::open(file)?;

    let mut data = String::new();
    file.read_to_string(&mut data)?;
    Ok(data)
}

Look at the list of implementations for AsRef<Path> to see what types you can now pass to your function.


Header file that includes all standard C library headers The Standard C Library consists of various headers files.  Often only a few select ones are needed for given code.

Other times it is simply convenient coding to include them all in a .c file, even if that make the compilation time a bit slower.

Including all standard <*.h> is useful to help detect naming collisions of a .c file with an existing standard function, object, type, macro, etc.

Review Goal

How well does std.h accomplish the goal of including all standard header files via one .h file given that the set varies amongst C versions and implementations?

/*
 * std.h
 *  Created on: Mar 16, 2019, Author: chux
 */

#ifndef STD_H_
#define STD_H_

#include <assert.h>
#include <ctype.h>
#include <errno.h>
#include <float.h>
#include <limits.h>
#include <locale.h>
#include <math.h>
#include <setjmp.h>
#include <signal.h>
#include <stdarg.h>
#include <stddef.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>

#if defined __STDC__ && defined __STDC_VERSION__

#if __STDC_VERSION__ >= 199409
#include <iso646.h>
#include <wchar.h>
#include <wctype.h>
#endif

#if __STDC_VERSION__ >= 199901
#ifndef __STDC_NO_COMPLEX__
#include <complex.h>
#endif
#include <fenv.h>
#include <inttypes.h>
#include <stdbool.h>
#include <stdint.h>
#include <tgmath.h>
#endif

#if __STDC_VERSION__ >= 201112
#include <stdalign.h>
#ifndef __STDC_NO_ATOMICS__
#include <stdatomic.h>
#endif
#include <stdnoreturn.h>
#ifndef __STDC_NO_THREADS__
#include <threads.h>
#endif
#include <uchar.h>
#endif

#if __STDC_VERSION__ >= 201710
/* None added */
#endif

#endif

#endif /* STD_H_ */

In making a set of functions and types called say foo, I do not recommend including all header files in a foo.h, yet perhaps in foo.c.

Sample usage

// foo.h  (no std.h here)
#ifndef FOO_H_
#define FOO_H_

#include <stdint.h>
#include <time.h>

typedef struct {
    time_t t;
    uint32_t u32;
} foo;

void foo_this(foo *f);
void foo_that(foo *f);

#endif /* FOO_H_ */

foo.c or main.c

#include "foo.h"
#include "std.h"

int main(void) {
  foo f;
  foo_this(&f);
  foo_that(&f);
  printf("Hello World!\n");
  return 0;
}

The alternative spellings afforded in <iso646.h> seem to be a solution to a regional character set problem of years ago.  I reluctantly included <iso646.h> here but do see that a good candidate to exclude.  It defines macros for and, or, xor and others.

With this suggested usage:

#include "foo.h"
#include "std.h"

then any macros defined in "std.h" (and not used in the rest of the main) won't break the program.  Swapping these round:

#include "std.h"
#include "foo.h"

solves that problem, but then fails to diagnose missing includes in "foo.h".


TicTacToe between Gandalf and Saruman What can be improved?

#!/usr/bin/python
import random

def board():
    board_size = int(raw_input("Board size: "))
    board = []
    for i in range(0,board_size):
        board.append(list(range(board_size)))
    for parent in board:
        for child in parent:
            parent[child] = 0
    return board

def checkHorizontal(table,token):
    # HORIZONTAL
    for row in table:
        length = len(row)
        check_row = []
        for column in row:
            if column == token:
                check_row.append(token)
        if len(check_row) == length:
            return True

def checkVertical(table,token):
        # VERTICAL
        for column in zip(*table):
            check_col = []
            for item in column:
                if item == token:
                    check_col.append(token)
                if len(check_col) == len(table[0]):
                    return True

def checkDiagonal(table,player_no):
    if all([table[i][i]==player_no for i in range(len(table)) ]):
        return True
    if all([table[-i-1][i]==player_no for i in range(len(table))]):
        return True
    return False

def whoWon(who, table, token):
    if who == 'x':
        if(checkHorizontal(table,token)): return True
        elif(checkVertical(table,token)): return True
        elif(checkDiagonal(table,token)): return True
    else:
        if(checkHorizontal(table,token)): return True
        elif(checkVertical(table,token)): return True
        elif(checkDiagonal(table,token)): return True

def placeToken(token,table,user_inp,position_taken_msg):
    if(table[user_inp[0]][user_inp[1]] == 0):
        table[user_inp[0]][user_inp[1]] = token
    else:
        while(table[user_inp[0]][user_inp[1]] != 0):
            print position_taken_msg
            user_inp = [0,0,0]
            user_inp[0] = int(raw_input('Row(0 - 2): '))
            user_inp[1] = int(raw_input('Col(0 - 2): '))
            if(table[user_inp[0]][user_inp[1]] == 0):
                table[user_inp[0]][user_inp[1]] = token
                break

def gamePlay():
    table, game_over, x_turn, o_turn, position_taken_msg, x_token, o_token = board(), False, True, False, "Position already taken! Choose again!.", 1, 2
    while not game_over:
        if x_turn:
            print '\n','-'*len('gandalf\'s'),'Gandalf\'s Turn!'
        else:
            print '\n','-'*len('saruman\'s'),'Saruman\'s Turn!'
        user_inp, user_inp[0], user_inp[1] = [0,0,0], int(raw_input('Row(0 - 2): ')), int(raw_input('Col(0 - 2): '))

        if x_turn:
            placeToken(x_token,table,user_inp,position_taken_msg)
            x_turn, o_turn = False, True
            print '\n'.join([str(p) for p in table])
            if(whoWon('x',table, x_token)):
                print "\n\n\t##########\n\tGandalf Won!\n\t##########\n\n"
                game_over = True
        else:
            placeToken(o_token,table,user_inp,position_taken_msg)
            x_turn, o_turn = True, False
            print '\n'.join([str(p) for p in table])
            if(whoWon('o',table, o_token)):
                print "\n\n\t##########\n\tSaruman Won!\n\t##########\n\n"
                game_over = True

def main():
    gamePlay()

if __name__ == '__main__':
    main()

Coding style

There's a limit to how much untidiness a reviewer is willing to tolerate.
This code is right on the border.
Please read PEP8 and follow the recommendations carefully.
There is also a command line utility called pep8 that can check your program and tell you what to fix.
There are also editors like PyCharm that warn you about violations of coding conventions,
and help you fix them.

In my review below, I will try to not repeat what PEP8 can already tell about your code.

Return values

Many of the functions don't have an explicit return value.
Take for example this:

def checkVertical(table,token):
        # VERTICAL
        for column in zip(*table):
            check_col = []
            for item in column:
                if item == token:
                    check_col.append(token)
                if len(check_col) == len(table[0]):
                    return True

The nested loop might be exited with a return True.
If that doesn't happen,
there's no other return statement,
so the method will exit with None.
This may work OK,
as None is a falsy value in Python,
but it would be cleaner to add a return False at the end to make this a proper boolean function.

Strange logic

What is going on in this function?

def whoWon(who, table, token):
    if who == 'x':
        if(checkHorizontal(table,token)): return True
        elif(checkVertical(table,token)): return True
        elif(checkDiagonal(table,token)): return True
    else:
        if(checkHorizontal(table,token)): return True
        elif(checkVertical(table,token)): return True
        elif(checkDiagonal(table,token)): return True

Regardless of the value of who,
the same code gets executed.
And again, there's no explicit return statement in case none of the conditions match.
As it is, the function could be replaced with:

def whoWon(who, table, token):
    return checkHorizontal(table, token) or checkVertical(table, token) or checkDiagonal(table, token)

Separation of concerns

The board() function reads the board size and initializes a board.
That's two distinct responsibilities.
It would be better to split these to different functions.

Simplifications

The initialization of the board in the board() function can be simplified:

return [[0] * board_size for _ in range(board_size)]

checkHorizontal() can be similarly simplified:

def checkHorizontal(table, token):
    for row in table:
        if all(column == token for column in row):
            return True
    return False


Format log with a small macro logger The goal of this code is to format any log to STDOUT and STDERR with the file and the line of where the call is made. I took inspiration from different C loggers that I found on the internet. I want it to be small and efficient while using C++17.

I'm a bit suspicious about the call to std::data at line 30 not being null-terminated.

#include <iostream>
#include <string_view>
#include <array>

enum Log_level
{
    L_SILENT = 0,
    L_ERROR = 1,
    L_INFO = 2,
    L_DEBUG = 3,
};

static Log_level log_level = L_ERROR;

static constexpr std::array log_colors
{
    "",
    "\x1B[1;31m",
    "\x1B[1;34m",
    "\x1B[1;30m"
};

template<typename... Args>
void vlk_log_internal(const Log_level level, const std::string_view s, Args... args)
{
    std::FILE* output = (level == L_ERROR ? stderr : stdout);
    if (level <= log_level)
    {
        std::fprintf(output, "%s", log_colors[level]);
        std::fprintf(output, std::data(s), args...);
        std::fprintf(output, "\x1B[0m");
        std::fprintf(output, "\n");
    }
}

#define vlk_log(lvl, msg, ...) { vlk_log_internal(lvl, "[%s:%d] " msg, __FILE__, __LINE__, ##__VA_ARGS__); } (void)0

int main()
{
    log_level = L_SILENT;

    vlk_log(L_ERROR, "Error will not show");

    log_level = L_ERROR;

    vlk_log(L_ERROR, "Error %s", "msg");
    vlk_log(L_INFO, "Info will not show");

    log_level = L_INFO;

    vlk_log(L_INFO, "Info %s", "msg");
    vlk_log(L_DEBUG, "Debug will not show");

    log_level = L_DEBUG;

    vlk_log(L_DEBUG, "Debug %s", "msg");

    return 0;
}

Compiler Explorer link

Looking at the code, it is clear that you took a part from existing C code.
Overal the code is acceptable, although when I should accept it, I would recommend some changes.

As already indicated in another answer, you could use scoped enums. This would remove the L_ prefix, unfortunately if you would use <windows.h>, you have to find a new name for ERROR.

The log_colors variable uses some strings to color the terminal. A comment about the actual colors would be nice.  Separate variables with clear names even better.

vlk_log_internal accepts variadic arguments, which is really good. You could use forwarding references to prevent expensive copies.

The output variable looks strange outside of the if-statement, you could move it in or introduce early return instead.

std::fprintf, is strange in C++ code, I mostly try to get rid of it. This because you have to know the ugly syntax of format strings and it is easy to make mistakes as passing an std::string instead of the char-ptr.
This even leaks into your API.

Next up, you have a define which has to hide the function.
You even nicely put something at the end to allow a semicolon. I don't like, however the other constants force you.

PS: I don't see std::data


Is this a good way to catch all 404s in .NET without playing with IIS configuration? The situation is that its not always possible to play with the web.config or have access to IIS, that being said I had to come up with a way to catch all 404s in a .NET application, for aspx page extensions its fairly easily and the web is full of ideas... but for non aspx extensions I did the following, looking for advice, is this a good way? are their scenarios where it might fail? does it measure well in performance? thanks for advice

After checking my web.sitemap links and make my rewrites, the "else" block contains the following

string s = context.Server.MapPath(context.Request.AppRelativeCurrentExecutionFilePath);
if (!System.IO.File.Exists(s)){
    throw new HttpException(404, "Page not found");
}

and of course, the rest is handled by the application error handler
notice that all css, js, jpegs and other file types go through this check

edit: two problems seem to be persistent with regular error catch, html and friendly urls, they never seem to fire 404s (guessing there other file types, depends on isapi filters)

404 is an error and normally should be captured with a relative event. Since you have a custom module you can do the following in the module:

public void Init(HttpApplication context)
{
    // your code
    context.Error +=new EventHandler(OnError);
}

private void OnError(object sender, EventArgs e)
{
    int ec = ((HttpException)HttpContext.Current.Error).GetHttpCode();
    if (ec == 404)
    {
        // do whatever you want
    }
}

If you follow this path you will have the advantage to control all error codes your way and avoid early 404 drops in case you change your rewriting policy or logic.


Autocompleting locations Can this function be cleaner? It looks to me that I have too much nesting (Using + If + While).

Public Shared Function GetLocationsAutocomplete(ByRef userData As IUser, ByVal prefix As String) As List(Of AutocompleteItem)
    Dim conn As DbConnection = DbProvider.GetDbConnection(ConnectionStringHelper.GetConnectionString(userData))
    Dim retValue As List(Of AutocompleteItem) = Nothing

    Const SQL As String = " SELECT DISTINCT TOP 20 Location " & _
                          " FROM userdetailstable WHERE ClientID=@ClientID " & _
                          " AND Location IS NOT NULL AND LEN(Location) > 0 AND Location LIKE @Filter "
    Try
        conn.Open()
        Logger.WriteConnectionInfo("UserDataAccess.GetLocationsAutocomplete", Logger.ConnectionActionType.Opened)

        ' Getting data from DB:
        Using cmd As DbCommand = DbProvider.GetDbCommand(SQL, conn)
            cmd.Parameters.Add("@ClientID", DbType.Int32).Value = userData.ClientID
            cmd.Parameters.Add("@Filter", DbType.String).Value = String.Format("%{0}%", prefix)
            Using reader As DbDataReader = cmd.ExecuteReader()
                If reader.HasRows Then
                    retValue = New List(Of AutocompleteItem)
                    ' Populate retValue list with project names:
                    Dim item As AutocompleteItem = Nothing
                    While reader.Read()
                        item = New AutocompleteItem()
                        item.Value = reader.GetString("Location")
                        item.Label = item.Value
                        retValue.Add(item)
                    End While
                End If
            End Using
        End Using
    Finally
        conn.Close()
        conn.Dispose()
        Logger.WriteConnectionInfo("SearchDataAccess.GetLocationsAutocomplete", Logger.ConnectionActionType.Closed)
    End Try

    Return retValue

End Function

Your comments should actually give you a hint.

' Getting data from DB:
' ...
' Populate retValue list with project names:

These sound like the beginnings of good function names to me.

Private Function GetAutocompleteItems(reader as DbDataReader) as List(Of AutocompleteItem)
    If reader.HasRows Then
        retValue = New List(Of AutocompleteItem)
        ' Populate retValue list with project names:
        Dim item As AutocompleteItem = Nothing
        While reader.Read()
            item = New AutocompleteItem()
            item.Value = reader.GetString("Location")
            item.Label = item.Value
            retValue.Add(item)
        End While
    End If

    Return retValue
End Function

Which would then be called like this.

        ' Getting data from DB:
        Using cmd As DbCommand = DbProvider.GetDbCommand(SQL, conn)
            cmd.Parameters.Add("@ClientID", DbType.Int32).Value = userData.ClientID
            cmd.Parameters.Add("@Filter", DbType.String).Value = String.Format("%{0}%", prefix)
            Using reader As DbDataReader = cmd.ExecuteReader()
                retValue = GetAutocompleteItems(reader)
            End Using
        End Using

As you can see, just extracting this one function out considerably reduced the nesting.

I don't see much else to pick on other than this.

    Finally
        conn.Close()
        conn.Dispose()
        Logger.WriteConnectionInfo("SearchDataAccess.GetLocationsAutocomplete", Logger.ConnectionActionType.Closed)
    End Try

DbConnection obviously implements IDisposable, so why not use another Using block?


Reference counted dynamic byte array What?

I have a reference counted dynamic byte array written in C. I'm currently using this implementation in a FIFO fashion. Particularly reading data from files into the arrays, then parsing the data in the array byte by byte.

I'm looking for feedback on anything! Even the decision to use a dynamic byte array versus using a linked list or any other data structure.

Edit: I'm trying to follow the Linux Kernel coding style, so I'm looking for feedback on how well I follow that convention.

Files

Header

#ifndef DATA_BYTE_ARRAY
#define DATA_BYTE_ARRAY

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

#include "../memory.h"

/**
 * Dynamic array that grows when it's full (given that
 * the util functions in this module are used).
 *
 * For each grow it uses the current length doubled. See
 * byte_array_push for more info.
 */

struct byte_array {
        uint8_t refs;
        uint32_t length;
        uint32_t capability;
        uint8_t *bytes;
};

/**
 * Creates a byte array with the given length.
 *
 * Values of the returned struct:
 * - 1 reference
 * - All bytes will be set to 0
 * - Current position will be set to 0
 * - Capability will reflect how many bytes are allocated. Which is
 *   the same as given capability.
 */

struct byte_array *byte_array_create(uint32_t capability);

/**
 * Adds the given input at the current position of the array.
 *
 * Note that if the pointer to the byte array is NULL an assertion will crash
 * the program.
 *
 * If the byte array is full more memory will be allocated. The amount
 * of new memory that will be allocated is the current amount doubled.
 */

void byte_array_push(struct byte_array *a, uint8_t input);

void byte_array_incref(struct byte_array *a);

void byte_array_decref(struct byte_array *a);

#endif

Source

#include "byte_array.h"

#define NEW_CAPABILITY a->capability * 2

static void zero_init_bytes(struct byte_array *a,
                            uint32_t offset,
                            uint32_t nmemb)
{
        uint8_t *bytes = a->bytes;
        bytes += offset;
        memset(bytes, 0, nmemb);
}

struct byte_array *byte_array_create(uint32_t capability)
{
        struct byte_array *result = assert_malloc(sizeof(*result));
        result->refs = 1;
        result->length = 0;
        result->capability = capability;
        result->bytes = assert_calloc(capability, sizeof(uint8_t));

        return result;
}

void byte_array_push(struct byte_array *a, uint8_t input)
{
        assert(a != NULL);

        if (a->length == a->capability) {
                void *tmp = assert_realloc(a->bytes, NEW_CAPABILITY);

                a->bytes = tmp;

                zero_init_bytes(
                        a,
                        a->capability,
                        NEW_CAPABILITY - a->capability
                );

                a->capability = NEW_CAPABILITY;
        }

        a->bytes[a->length++] = input;
}

void byte_array_incref(struct byte_array *a)
{
        assert(a != NULL);

        a->refs++;
}

void byte_array_decref(struct byte_array *a)
{
        assert(a != NULL);

        if (--a->refs != 0) return;

        free(a->bytes);
        free(a);
}

This looks mostly good to me. Your code looks like fairly clean C. I have some comments:

I think capacity is a better name than capability.
I would avoid calling your header memory.h, since memory is a header in C++. Maybe checked_alloc.h instead? Also, include it as "memory.h" and let your build system figure out its path.
How is your error handling in the assert_...alloc functions? Do they just terminate the program?
Two of your public interface functions are without documentation. Are they not supposed to be part of the public interface? If not, put them in the implementation file (only) and make them static. If they are, document them as well.
Note that the assert() macro is only evaluated if your library is compiled without NDEBUG. This means that the code using it will be unchecked for production builds. It might be fine, but maybe you want checking in production as well; I'm just pointing it out.
As far as I can see, you adhere to the Linux coding style.
Consider typedef uint8_t byte or something like that to emphasize what the role of the uint8_ts are. Remember that you have no guarantee that CHAR_BIT is 8. (Maybe you want to use char instead, which is guaranteed to always be one byte.)
I would change if (a->length == a->capability) to if (a->length >= a->capability). The latter is a bit clearer, and if you at one point allow adding larger objects the code will still work.
Your macro looks nasty.

I'd do:

#define CAPACITY_GROWTH_FACTOR 2

/* ... */

a->capability *= CAPACITY_GROWTH_FACTOR;

Or even better, change the #define into a const size_t.
The nmemb variable isn't very readable. Change it to num_members or something like that.
Consider sorting your #includes alphabetically.
While I don't see anything wrong with your reference counting, you should thoroughly test it to verify that it works.
Regarding reference counting and your interface, ask yourself these questions:

What happens when you copy your array? What do you want to happen? How should a copy be made?
How should copies behave when you modify an array that has multiple references to it?
Will this be used in a multi-threaded environment?

The answers affect how the interface should be designed. Personally, it seems to me that the reference counting is on a very low level (the caller has to take care of it himself), meaning it would be easy to make mistakes. Make your interface easy to use correctly, and hard to use incorrectly. Reference counting might not even be necessary here, depending on what the array is meant to be used for.


Opening a file and printing its contents line by line Everything that I've read online says that the best way to open a file in python is:

with open('<filename>') as f:
    for line in f.read().splitlines():
        print line

That way you don't have to worry about forgetting to close the file. But I put this in an assignment in my python class and my professor marked points off saying that it was "VB style" (I have no idea what that means, Visual Basic style?) and that from now on I should open a file this way in python:

f=open('<filename>')
for line in f.read().splitlines():
        print line
f.close()

That seems messy to me and like you would run the risk of forgetting to close one of your files, but then again I'm just a student. What's the best way to open a file in Python?

Rather than using f.read().splitlines(), you can iterate the file object allowing you to use:

with open('<filename>') as f:
    for line in f:
        print line

Using with is better for various reasons, you don't have to remember to close the file, as you said in the question. It also closes the file, even if an error happened, which the second code doesn't. Take the following:

FILE_NAME = 'test_file.txt'
with open(FILE_NAME, 'w') as f:
    f.write('Hello World')

f = open(FILE_NAME)
try:
    with f:
        raise Exception()
        print f.read()
except Exception:
    try:
        print f.read()
    except ValueError:
        print 'I/O operation on closed file'

f = open(FILE_NAME)
try:
    raise Exception()
    print f.read()
except Exception:
    try:
        print f.read()
    except ValueError:
        print 'I/O operation on closed file'

f = open(FILE_NAME)
try:
    try:
        raise Exception()
        print f.read()
    finally:
        f.close()
except Exception:
    try:
        print f.read()
    except ValueError:
        print 'I/O operation on closed file'

This outputs two I/O error messages and one 'Hello World'. The single hello world is using the second way, without a finally to close the file. And so if you did want to use the second method, then you'd want to use something like:

try:
    f=open('<filename>')
    for line in f:
        print line
finally:
    f.close()

And, we all know which is easier to read an write.
For more information about the with statement you can look into PEP 343, which explains why it was added to Python.


Asynchronous task and close sql connection The following code compiles correctly:

    public ContextStatus Commit(string userName)
    {
        var status = new ContextStatus();

        var task = Task.Run(async () => { status = await CommitAsync(userName, new CancellationToken()); });
        task.Wait();

        return status;
    }

    public async Task<ContextStatus> CommitAsync(string userName, CancellationToken ct)
    {
        var status = new ContextStatus();

        try
        {
            if (this.Database.Connection.State == ConnectionState.Closed)
            {
                await this.Database.Connection.OpenAsync(ct).ConfigureAwait(false);
            }

            using (var command = this.Database.Connection.CreateCommand())
            {
                SqlParameter param = new SqlParameter()
                {
                    ParameterName = "@user",
                    SqlDbType = SqlDbType.VarChar,
                    Size = 127,
                    Value = userName
                };

                command.CommandText = "setcontextinfo";
                command.CommandType = CommandType.StoredProcedure;
                command.Parameters.Add(param);

                await command.ExecuteNonQueryAsync(ct).ConfigureAwait(false);
            }

            await base.SaveChangesAsync(ct).ConfigureAwait(false);
        }
        catch (DbEntityValidationException dbEx)
        {
            status.SetErrors(dbEx.EntityValidationErrors);
        }
        catch (DbUpdateException duEx)
        {
            status.SetErrors(duEx);
            if (!status.DatabaseErrors.Any())
                throw;
        }

        return status;
    }

Questions:

Will this CommitAsync method execute synchronously in the Commit method?
Is it best practive to pass new CancellationToken() in the Commit method?
Will the base.SaveChangesAsync() call in CommitAsync() close the connection if it was opened beforehand?

Will this CommitAsync method execute synchronously in the Commit method?

I wouldn't phrase it that way, CommitAsync always executes asynchronously, but Commit wait for it to complete synchronously.

  Is it best practive to pass new CancellationToken() in the Commit method?

It's better to use CancellationToken.None, to make the meaning clearer. Though functionally, the two are equivalent.

  Will the base.SaveChangesAsync() call in CommitAsync() close the connection if it was opened beforehand?

I don't think it will, a single connection can be used for multiple commands.

In general, you should create a new connection for each separate operation (which may comprise more than one command) you do and then dispose the connection, which closes it. This won't hurt efficiency, because connections are actually pooled, so closing the connection object means that the actual connection can be reused by another operation.

var status = new ContextStatus();

var task = Task.Run(async () => { status = await CommitAsync(userName, new CancellationToken()); });
task.Wait();

return status;

This is unnecessarily verbose and complicated. Instead, I would write it like this:

return Task.Run(() => CommitAsync(userName, new CancellationToken())).Result;

CommitAsync returns Task<ContextStatus>, so Task.Run() will also return that type. The Task's Result will give you the ContextStatus, synchronously waiting for the Task to complete before returning.

SqlParameter param = new SqlParameter()

Why not use var here too? The type of the variable is clear, so specifying the type just makes the code more verbose.


Finding the profile name for a join in my Facebook auth backend I've written a project for handling various Facebook things and I ran into the issue where one project named the users profile Profile while normally they would be named UserProfile.  When I was checking if the FacebookUser model was related to a User, I could just User.objects.get(userprofile__facebook=facebook_user) so I come up with a way to get the name, but it seems pretty ugly. Is there a cleaner way?

from django.conf import settings
from django.contrib.auth.models import User
from better_facebook.models import FacebookUser

# Get the name of the AUTH profile so that we can check if the user
# has a relationship with facebook. Is there a neater way of doing this?
_lowercase_profile_name = settings.AUTH_PROFILE_MODULE.split(".")[1].lower()
_facebook_relation = '{0}__facebook'.format(_lowercase_profile_name)

class FacebookBackend(object):
    """
    Authenticate against a facebook_user object.
    """
    def authenticate(self, facebook_user):
        assert(isinstance(facebook_user, FacebookUser))
        try:
            return User.objects.get(**{_facebook_relation: facebook_user})
        except User.DoesNotExist:
            return None

    def get_user(self, user_id):
        try:
            return User.objects.get(pk=user_id)
        except User.DoesNotExist:
            return None

You can use django.contrib.auth.get_user_model() instead of importing User directly. Note that I also explicitly included the ObjectDoesNotExist exception.

https://docs.djangoproject.com/en/2.2/topics/auth/customizing/

from django.conf import settings
from django.contrib.auth import get_user_model
from django.core.exceptions import ObjectDoesNotExist
from better_facebook.models import FacebookUser

class FacebookBackend(object):
    """
    Authenticate against a facebook_user object.
    """
    def authenticate(self, facebook_user):
        assert(isinstance(facebook_user, FacebookUser))
        try:
            UserModel = get_user_model()
            return UserModel.objects.get(userprofile__facebook=facebook_user)
        except ObjectDoesNotExist:
            return None

    def get_user(self, user_id):
        try:
            UserModel = get_user_model()
            return UserModel.objects.get(pk=user_id)
        except ObjectDoesNotExist:
            return None
```


Project Euler #11 - Largest product in a grid C++ https://projecteuler.net/problem=11

Any thoughts? Also, when measuring with chrono it takes supposedly 4 ms to compute, is this reliable value? Previous problems took little longer to compute so I'm suspicious, and I'm still a beginner so I'm not sure how long it "should" take or if chrono is good for measuring computing time.

#include <fstream>
#include <vector>
#include <string>

int main()
{
    std::ifstream input("grid.txt");
    std::vector<std::vector<int>> numbers(20, std::vector<int>(20));

    char c;
    std::string str;

    int row{ 0 };
    int column{ 0 };
    //convert from ascii to decimal
    while (input.get(c))
    {
        str += c;
        if (((int)c - 48 == -16) || ((int)c - 48 == -38)) //space or new line
        {
            numbers[row][column] = std::stoi(str);
            column++;
            str.clear();
        }
        if (column == 20)
        {
            row++;
            column = 0;
        }
    }
    int product;
    int maxProduct{ 0 };
    for (int i = 0; i < 20; i++)
    {
        product = 0;
        for (int j = 0; j < 20; j++)
        {
            // horizontal
            if (j >= 3)
            {
                product = numbers[i][j] * numbers[i][j - 1] * numbers[i][j - 2] * numbers[i][j - 3];
                if (product > maxProduct)
                    maxProduct = product;
            }

            if (j <= 16)
            {
                product = numbers[i][j] * numbers[i][j + 1] * numbers[i][j + 2] * numbers[i][j + 3];
                if (product > maxProduct)
                    maxProduct = product;
            }
            // vertical
            if (i >= 3)
            {
                product = numbers[i][j] * numbers[i - 1][j] * numbers[i - 2][j] * numbers[i - 3][j];
                if (product > maxProduct)
                    maxProduct = product;
            }

            if (i <= 16)
            {
                product = numbers[i][j] * numbers[i + 1][j] * numbers[i + 2][j] * numbers[i + 3][j];
                if (product > maxProduct)
                    maxProduct = product;
            }
            // diagonal
            if (i >= 3 && j >= 3)
            {
                product = numbers[i][j] * numbers[i - 1][j - 1] * numbers[i - 2][j - 2] * numbers[i - 3][j - 3];
                if (product > maxProduct)
                    maxProduct = product;
            }
            if (i <= 16 && j <= 16)
            {
                product = numbers[i][j] * numbers[i + 1][j + 1] * numbers[i + 2][j + 2] * numbers[i + 3][j + 3];
                if (product > maxProduct)
                    maxProduct = product;
            }
            if (i >= 3 && j <= 16)
            {
                product = numbers[i][j] * numbers[i - 1][j + 1] * numbers[i - 2][j + 2] * numbers[i - 3][j + 3];
                if (product > maxProduct)
                    maxProduct = product;
            }
            if (i <= 16 && j >= 3)
            {
                product = numbers[i][j] * numbers[i + 1][j - 1] * numbers[i + 2][j - 2] * numbers[i + 3][j - 3];
                if (product > maxProduct)
                    maxProduct = product;
            }
        }
    }

    return 0;
}

Input

Using magic numbers like 48 and -16 makes your code very cryptic and hard to understand. Consider comparing with the characters instead.

Algorithm / Logic

I think two things that could really simplify your code are:

Limit the loops iterations from [0, 0] to [16, 16]. This will make you avoid some unnecessary checks.
Only try to get the 4 numbers in the forward direction. There is no need to check in the backward direction as well since it's already checked in the forward direction.


Clone a LinkedList with each node having a next and a random pointer The LinkedList nodes can be 1-->2-->3-->4, where --> indicates next. For their random pointers: 1--> 2, 2--> 4 3-->2 4--1. There can be duplicate data values of the nodes. I'm looking to see if the solution can be improved upon.

class Node
{
int data;//Node data
Node next, random;//Next and random reference

//Node constructor
public Node(int data)
{
    this.data = data;
    this.next = this.random = null;
}
}

// linked list class
class LinkedList
{
Node head;//Linked list head reference

// Linked list constructor
public LinkedList(Node head)
{
    this.head = head;
}

// push method to put data always at the head
// in the linked list.
public void push(int data)
{
    Node node = new Node(data);
    node.next = this.head;
    this.head = node;
}

// Method to print the list.
void print()
{
    Node temp = head;
    while (temp != null)
    {
        Node random = temp.random;
        int randomData = (random != null)? random.data: -1;
        System.out.println("Data = " + temp.data +
                ", Random data = "+ randomData);
        temp = temp.next;
    }
    }

// Actual clone method which returns head
// reference of cloned linked list.
public LinkedList clone()
{
    // Initialize two references, one with original
    // list's head.
    Node origCurr = this.head, cloneCurr = null;

    // Hash map which contains node to node mapping of
    // original and clone linked list.
    Map<Node, Node> map = new HashMap<Node, Node>();

    // Traverse the original list to help copy into the HashMap
    while (origCurr != null) {
        cloneCurr = new Node(origCurr.data);
        cloneCurr.next = origCurr.next;
        cloneCurr.random = origCurr.random;
        map.put(origCurr, cloneCurr);
        origCurr = origCurr.next;
    }

    //return the head reference of the clone list.
    return new LinkedList(map.get(this.head));
}
}

public class Clone
{

public static void main(String[] args)
{
    // Pushing data in the linked list.
    LinkedList list = new LinkedList(new Node(5));
    list.push(4);
    list.push(3);
    list.push(2);
    list.push(1);

    list.head.random = list.head.next.next;
    list.head.next.random =
            list.head.next.next.next;
    list.head.next.next.random =
            list.head.next.next.next.next;
    list.head.next.next.next.random =
            list.head.next.next.next.next.next;
    list.head.next.next.next.next.random =
            list.head.next;

    // Making a clone of the original linked list.
    LinkedList clone = list.clone();

    // Print the original and cloned linked list.
    System.out.println("Original linked list");
    list.print();
    System.out.println("\nCloned linked list");
    clone.print();
}
}

Not a clone
Your "clone" is not a clone of the original list.  It is a single head node that points to the second node of the original list.  In order to make a real clone, all pointers in the cloned list should point only to other nodes in the cloned list.
Two passes
Your attempt isn't completely wrong.  After creating all the cloned nodes, you just need a second pass where you convert the original node pointers to cloned node pointers using the map you created.


Convert elementtree to dict Just needed a quick way to convert an elementtree element to a dict. I don't care if attributes/elements clash in name, nor namespaces. The XML files are small enough. If an element has multiple children which have the same name, create a list out of them:

def elementtree_to_dict(element):
    d = dict()
    if hasattr(element, 'text') and  element.text is not None:
        d['text'] = element.text

    d.update(element.items()) # element's attributes

    for c in list(element): # element's children
        if c.tag not in d:
            d[c.tag] = elementtree_to_dict(c)
        # an element with the same tag was already in the dict
        else:
            # if it's not a list already, convert it to a list and append
            if not isinstance(d[c.tag], list):
                d[c.tag] = [d[c.tag], elementtree_to_dict(c)]
           # append to the list
            else:
                d[c.tag].append(elementtree_to_dict(c))
    return d

Thoughts? I'm particularly un-fond of the not instance part of the last if.

def elementtree_to_dict(element):
    d = dict()

I'd avoid the name d its not very helpful

    if hasattr(element, 'text') and  element.text is not None:
        d['text'] = element.text

getattr has a third parameter, default. That should allow you to simplify this piece of code a bit

    d.update(element.items()) # element's attributes

    for c in list(element): # element's children

The list does nothing, except waste memory.

        if c.tag not in d:
            d[c.tag] = elementtree_to_dict(c)
        # an element with the same tag was already in the dict
        else:
            # if it's not a list already, convert it to a list and append
            if not isinstance(d[c.tag], list):
                d[c.tag] = [d[c.tag], elementtree_to_dict(c)]
           # append to the list
            else:
                d[c.tag].append(elementtree_to_dict(c))

Yeah this whole block is a mess. Two notes:

Put everything in lists to begin with, and then take them out at the end
call elementtree_to_dict once

return d

This whole piece of code looks like a bad idea.

<foo>
   <bar id="42"/>
</foo>

Becomes

{"bar" : {"id": 42}}

Whereas

<foo>
    <bar id="42"/>
    <bar id="36"/>
</foo>

Becomes

{"bar" : [{"id" : 42}, {"id": 36}]}

The XML schema is the same, but the python "schema" will be different. It'll be annoying writing code that correctly handles both of these cases.

Having said that, here's my cleanup of your code:

def elementtree_to_dict(element):
    node = dict()

    text = getattr(element, 'text', None)
    if text is not None:
        node['text'] = text

    node.update(element.items()) # element's attributes

    child_nodes = {}
    for child in element: # element's children
        child_nodes.setdefault(child, []).append( elementtree_to_dict(child) )

    # convert all single-element lists into non-lists
    for key, value in child_nodes.items():
        if len(value) == 1:
             child_nodes[key] = value[0]

    node.update(child_nodes.items())

    return node