A simple C WinAPI program for terminating processes via process image names (Also, see the next iteration.)
I have this small program for terminating processes via their respective process image names (.exe files):
#include <stdio.h>
#include <windows.h>
#include <TlHelp32.h>

int main(int argc, char* argv[]) {
    if (argc != 2) {
        puts("processkiller.exe PROCESS_NAME");
        return EXIT_SUCCESS;
    }

    PROCESSENTRY32 entry;
    entry.dwSize = sizeof(PROCESSENTRY32);
    HANDLE snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, NULL);

    if (Process32First(snapshot, &entry) == TRUE) {
        while (Process32Next(snapshot, &entry) == TRUE) {
            if (strcmp(entry.szExeFile, argv[1]) == 0) {
                HANDLE hProcess = OpenProcess(PROCESS_ALL_ACCESS,
                                              FALSE,
                                              entry.th32ProcessID);
                TerminateProcess(hProcess, 0);
                CloseHandle(hProcess);
            }
        }
    }

    CloseHandle(snapshot);
    return EXIT_SUCCESS;
}

Note that you need to run processkiller.exe in administrator mode in order to actually terminate the requested processes.
Critique request
Please tell me anything that comes to mind.

There is a subtle bug:
Process32First(snapshot, &entry)

already fills entry with the information about the first process in the snapshot. Your code misses that entry because Process32Next() is called immediately. The loop structure should be
if (Process32First(snapshot, &entry) == TRUE) {
    do {
        // ... do something with `entry`...

    } while (Process32Next(snapshot, &entry) == TRUE);
}

instead. Other things that come into my mind:

If the program is called with the wrong number of arguments then the error/usage message should be printed to the standard error and the program should terminate with a non-zero exit code, e.g. EXIT_FAILURE.
The return value of CreateToolhelp32Snapshot() is not checked.
PROCESS_ALL_ACCESS is not needed in the call to OpenProcess, only PROCESS_TERMINATE.
The return values of OpenProcess() and TerminateProcess() are not checked. I would expect a diagnostic message if they fail. In particular, TerminateProcess() and CloseHandle() should only be called if OpenProcess() succeeded.
It may be a matter of taste, but == TRUE can be omitted when checking a boolean condition.
As a user of this tool I would expect some feedback to see if a matching process was found, and how many processes were killed.


Optimizing MD5 OpenSSL Implementation in C for Precomputed Hash I am trying to port a piece of code from Python to C. What the code does is that it generates a list of precomputed MD5 hash from a plaintext wordlist.
The code also has its variations in SHA-1 and SHA-2 families in all 3 programming languages. The structure is the same.
I originally wrote the code in Bash, which was slow. So I ported it to Python which was significantly faster. Now, I have successfully ported the code to C, in hoping that it will run even faster. However, even with
-O faster flag on in gcc, the code still run slower than the Python version (the difference in execution time increases exponentially with the input size).
I have also doubted about the efficiency of the OpenSSL crypto libraries, but it seems to me that they are relatively well-established after reading through their Documentation.
I am guessing that it is the nested loop that I implemented in the C version of the code that is slowing the whole thing down.
Any suggestion to increase the performance?
Bash Version:

#/!bin/bash

while read line

do

printf $line | md5

done

Python Version:

import hashlib

infile = 'wordlist'

outfile = open("precomputed","a")

with open(infile, "r") as inf:

    for line in inf:

        outfile.write(hashlib.md5(line.strip().encode('utf8')).hexdigest()+'\n')

C Version:
//-----------Libraries
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <openssl/md5.h>

//------------------------------Main Function------------------------//
int main()
{

//------Define infile, outfile, file length. Define string to be read.---//
  FILE *infile, *outfile;
  char *string = NULL;
  size_t len = 0;
  ssize_t read;
//------Open File stream for read(r) and write (w). Error Handling.--//

  //Part of MD5 Hash Function (Taken out of While Loop for Optimization)
  int md5;
  unsigned char result[MD5_DIGEST_LENGTH];
  //

  infile = fopen("file.txt", "r");

  if (infile == NULL)
    exit(EXIT_FAILURE);

  outfile = fopen("MD.txt","w");

  if (outfile == NULL)
    exit(EXIT_FAILURE);

//-------------Read line-by-line in using a while loop.--------------//
  while ((read = getline(&string, &len, infile)) != -1) {

     string[strcspn(string, "\n")] = 0; // Remove newline '\n'

//-------------------------MD5 Hash Function-------------------------//

      MD5(string, strlen(string), result);

      //output
      for(md5 = 0; md5 < MD5_DIGEST_LENGTH; md5++)

      fprintf(outfile,"%02x",result[md5]); //convert the hash to hex

      fprintf(outfile,"\n"); //newline for the output file

  }

  free(string); //free string
  fclose(infile); // close file streams
  fclose(outfile);
  exit(EXIT_SUCCESS); //Program Ends
}

I can provide the execution time with different size of input if requested. Any help will be appreciated.

I'll start with a review of your current code before suggesting possible performance
improvements.

Various comments in your code do not add information and can be removed, for example

//------------------------------Main Function------------------------//
int main()

free(string); //free string

exit(EXIT_SUCCESS); //Program Ends

Always use curly braces { } with if-statements, even if the if or else part consists only
of a single statement. That helps to avoid errors if the code is edited later.

Declare variables at the narrowest scope where they are used, and not at the top
of the function. For example

FILE *infile = fopen("file.txt", "r");
if (infile == NULL) {
    exit(EXIT_FAILURE);
}

FILE *outfile = fopen("MD.txt","w");
if (outfile == NULL) {
    exit(EXIT_FAILURE);
}

or

for (int md5 = 0; md5 < MD5_DIGEST_LENGTH; md5++) { ... }

Declaring md5 at the top does not increase the performance.

Some variable names can be improved (of course this is partially opinion-based):

char *string is actually the current line.
size_t len is not the length of the current string,
but the capacity of the buffer (re)allocated by getline().
ssize_t read is – strictly speaking – not the number of bytes read into the
buffer because it excludes the NUL character.
int md5 is not an MD5 value but an index into the buffer containing the
MD5 hash.
I would rename unsigned char result[MD5_DIGEST_LENGTH] to md5hash.

Fix compiler warnings:

MD5(string, strlen(string), result);
// Passing 'char *' to parameter of type 'const unsigned char *' converts between pointers to integer types with different sign

You already use different exit codes to indicate success or failure of the program,
which is good. In addition, it is helpful to print some message (to standard error)
in the error case.

According to the C11 standard, the declaration of main should be one of

int main(void) { /* ... */ }
int main(int argc, char *argv[]) { /* ... */ }

(see for example What should main() return in C and C++?
on Stack Overflow). The final return statement can be omitted, there is an implicit
return 0.

Performance improvements

After reading a line from the input file, the string is traversed twice:

while ((read = getline(&string, &len, infile)) != -1) {
    string[strcspn(string, "\n")] = 0; // Remove newline '\n'
    MD5(string, strlen(string), result);

    // ...
}

First to find a terminating newline character, and then again to determine the length.
This is not necessary because getline() returns the number of characters written to
string, i.e. a newline character can only be at position read - 1:

while ((read = getline(&string, &len, infile)) != -1) {
    // Remove trailing newline character
    if (read > 0 && string[read - 1] == '\n') {
        read -= 1;
        string[read] = 0;
    }

    MD5(string, read, result);

    // ...
}

However, the impact of this change depends on the line length, and I could not observe
a significant difference in my test.

In order to find further performance bottlenecks, I profiled the program now with
Xcode/Instruments (using the input file generated by ./crunch 7 7 1234567890)

This immediately revealed that most of the time is spent in fprintf():

Possible reasons are:

String formatting is slow.
All stdio print operations are thread-safe, and therefore have to aquire and release
locks for each call.

The solution is to:

Write a custom function for converting the MD5 hash to a hex string.
Call printf for the entire string instead of each single byte so reduce the number
of function calls.

On my computer (a 1.2 GHz Intel Core m5 MacBook) this reduced the time for processing
the above file from 24.5 seconds to 4.4 seconds.

Putting it together

With all those modifications, we have

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <openssl/md5.h>

// Format the data as a hexadecimal string. The buffer must have
// space for `2 * length + 1` characters.
const char *hexString(unsigned char *data, size_t length, char *buffer) {
    const char *hexDigits = "0123456789abcdef";
    char *dest = buffer;
    for (size_t i = 0; i < length; i++) {
        *dest++ = hexDigits[data[i] >> 4];
        *dest++ = hexDigits[data[i] & 0x0F];
    }
    *dest = 0;
    return buffer;
}

int main(void) {
    FILE *infile = fopen("file.txt", "r");
    if (infile == NULL) {
        perror("Cannot open input file");
        exit(EXIT_FAILURE);
    }
    FILE *outfile = fopen("MD.txt","w");
    if (outfile == NULL) {
        perror("Cannot open output file");
        exit(EXIT_FAILURE);
    }

    // Read file line-by-line
    char *line = NULL;
    size_t linecap = 0;
    ssize_t lineLength;
    while ((lineLength = getline(&line, &linecap, infile)) != -1) {
        if (lineLength > 0 && line[lineLength - 1] == '\n') {
            // Remove newline character
            lineLength -= 1;
            line[lineLength] = 0;
        }

        // Compute MD5 hash
        unsigned char md5hash[MD5_DIGEST_LENGTH];
        MD5((unsigned char*)line, lineLength, md5hash);

        // Print hash as hex string
        char hexBuffer[2 * MD5_DIGEST_LENGTH + 1];
        fputs(hexString(md5hash, MD5_DIGEST_LENGTH, hexBuffer), outfile);
        fputc('\n', outfile);
    }
    free(line);

    // Close output files
    fclose(infile);
    fclose(outfile);
}

Further suggestions

The names of input and output file are compiled into the program, which makes it inflexible. Possible alternatives are

Pass the file names as arguments on the command line, or
Make your program read from standard input and write to standard output.

Implement a -h option to show a short help/usage of the program.


Generating abbreviations for strings One of many aspects of a project I am responsible for is generating an abbreviation, that is no more than n characters, for a given string. We use this in the organization for a couple things, and it only happens once for each "thing", so performance isn't hyper-critical.

An example is something like "The Main Test Company" — when asking for a 3-character abbreviation, I want to be able to generate a string like MTC. Now this would also be the same string as My Tiny Cat, but the idea is that a given company can be abbreviated and it can be done safely and effectively.

Also, if the abbreviations aren't large enough for the requested characters (say we request 4 characters from My Awesome Company) it should, for each abbreviation character, start adding characters to the total (so our example is MYAC).

I wrote the whole thing in F#, and it seems pretty readable and followable, I'm also including some test strings to verify functionality. Any comments are welcome.

let genAbbr ignoreList padding charCount (str : string) =
    if str.Length <= charCount then
        match padding with
        | Some c -> str |> Seq.append (String(c, charCount - str.Length)) |> Seq.toArray
        | None -> str.ToCharArray()
        |> Array.map Char.ToUpper
        |> String
    else
        let words =
            [|' '|]
            |> Array.append (str.ToCharArray())
            |> Array.fold (fun (acc, cs) c ->
                match c |> Char.IsUpper, c = ' ', cs with
                | true, _, [] -> (acc, [c])
                | true, _, cs -> (cs::acc, [c])
                | _, true, [] -> (acc, [])
                | _, true, cs -> (cs::acc, [])
                | _, _, cs -> (acc, c::cs)) ([], [])
            |> fst
            |> Array.ofList
            |> Array.map (Array.ofList >> Array.rev >> String)
            |> Array.rev
            |> Array.map (fun s -> s.ToCharArray())
        let ignoreWords = words |> Array.filter (fun w -> ignoreList |> Array.contains ((w |> String).ToLower()) |> not)
        let abbrCs =
            if ignoreWords |> Array.length >= charCount then ignoreWords |> Array.map Seq.head
            else if words |> Array.length >= charCount then words |> Array.map Seq.head
            else
                let firstCaps = words |> Array.map (fun s -> if s.Length > charCount then s.[0..charCount] else s) |> Array.length
                printfn "%A %A %A" ignoreWords words firstCaps
                words
                |> Array.fold (fun (acc, r) w ->
                    if w.Length > r then (w.[0..(r - 1)]::acc, 1)
                    else (w::acc, r - (w.Length - 1))) ([], charCount - firstCaps + 1)
                |> fst
                |> Array.ofList
                |> Array.rev
                |> Array.concat
                |> Array.map Char.ToUpper
        abbrCs |> Array.take charCount |> String

And lastly, some POC's:

["The Main Test Company"; "Main Test Company"; "The Main Company"; "The Company"; "Company"; "MainCompany"; "The MainCompany"; "MainTestCompany"; "SomeRX"; "SomeCompanyT"]
|> List.map (genAbbr [|"the"|] None 3)
// Should be ["MTC"; "MTC"; "TMC"; "THC"; "COM"; "MAC"; "TMC"; "MTC"; "SRX"; "SCT"]

Thanks for any commentary, and I look forward to this being brutally decimated. :)

Some possible improvement points:

Splitting out functions for word grouping, padding, and word filtering
Removing unnecessary branching
Moving from fold to self-contained recursive functions that require less post processing
Use a HashSet with a StringComparer for filtering

Using these techniques, and avoiding regexes (like my previous answer), we can get a solution that may be easier to reason about:

    open System.Collections.Generic

    let getWords (str:string) =
        let word rev_chars =
            (rev_chars |> List.rev |> Array.ofList |> String).ToUpper()

        let add_word rev_chars acc =
            if rev_chars <> [] then (word rev_chars) :: acc else acc

        let rec loop (idx:int) (rev_chars:char list) (acc: string list) prev_lower =
            if idx < str.Length then
                let c = str.[idx]
                let rev_chars, acc =
                    match str.[idx] with
                    | ' '                                      -> []             , (add_word rev_chars acc)
                    | c when Char.IsUpper(c)                   -> [c]            , (add_word rev_chars acc)
                    | c                                        -> (c::rev_chars) , acc
                loop (idx + 1) rev_chars acc (Char.IsLower c)
            else
                let acc = add_word rev_chars acc in List.rev acc

        loop 0 [] [] false

    let pad (str:string) len padchar =
        if str.Length < len then str + String(padchar, (len - str.Length)) else str

    // Filter a list of words, if we can 'afford' it, starting from the back
    let filterWords (ignoreList:HashSet<string>) charCount words =
        let rec loop revWords acc =
            match revWords with
            | [] -> acc
            | word::revWords when ignoreList.Contains(word) ->
                let getLen = List.fold (fun len s -> len + String.length s) 0
                let maxLenWithout = (getLen words) + (getLen acc)
                let numOtherParts = List.length revWords + List.length acc
                if maxLenWithout >= charCount && numOtherParts >= charCount then loop revWords acc
                else loop revWords (word::acc)
            | word::revWords -> loop revWords (word::acc)

        loop (List.rev words) []

    let genAbbr (ignoreList:HashSet<string>) padding charCount (str : string) =
        let words = getWords str |> filterWords ignoreList charCount

        // Given a list of words, generate a sequence of abbreviations
        let rec abbrs before (words: string list) : string seq = seq {
                match words with
                | [] -> ()
                | word::words ->
                    for i = 1 to word.Length do
                        let before = before + word.Substring(0, i)
                        yield before + (words |> List.map (fun w -> w.Substring(0, 1)) |> String.concat "")
                    yield! abbrs (before + word) words }

        (abbrs "" words)
        |> Seq.takeWhile (fun s -> s.Length <= charCount)
        |> Seq.maxBy (fun s -> s.Length)
        |> fun s ->
            match padding with | None -> s | Some padchar -> pad s charCount padchar

To call it:

    let ignored = let h = HashSet<string>(StringComparer.OrdinalIgnoreCase) in h.Add("the") |> ignore; h

    ["The Main Test Company"; "Main Test Company"; "The Main Company"; "The Company"; "Company"; "MainCompany"; "The MainCompany"; "MainTestCompany"; "SomeRX"; "SomeCompanyT"]
    |> List.map (fun w -> w,  (genAbbr ignored None 3 w))


Where should I place the constant? The following bit of code requires a string.maketrans table that is only used inside a single function.

I have written three versions of the program that place the table at different locations in the code. The differing placement of the table results in quite varying performance in the creation of DNA instances and calls to the to_rna method.

So, which one of these variants is preferable? Or is there even a different, better solution?

Version 1:

from string import maketrans

DNA_RNA_TRANSLATION_TABLE = maketrans('GCTA', 'CGAU')

class DNA(object):
    """Represent a strand of DNA."""

    def __init__(self, dna_string):
        self.dna_string = dna_string

    def to_rna(self):
        """Return the RNA complement of the DNA strand."""
        return self.dna_string.translate(DNA_RNA_TRANSLATION_TABLE)

Version 2:

from string import maketrans

class DNA(object):
    """Represent a strand of DNA."""

    __rna_translation_table = maketrans('GCTA', 'CGAU')

    def __init__(self, dna_string):
        self.dna_string = dna_string

    def to_rna(self):
        """Return the RNA complement of the DNA strand."""
        return self.dna_string.translate(self.__rna_translation_table)

Version 3:

from string import maketrans

class DNA(object):
    """Represent a strand of DNA."""

    def __init__(self, dna_string):
        self.dna_string = dna_string

    def to_rna(self):
        """Return the RNA complement of the DNA strand."""
        translation_table = maketrans('GCTA', 'CGAU')
        return self.dna_string.translate(translation_table)

I will answer your question in an unexpected way : the better solution is, as far as I can tell, much more straightforward than what you are doing.

Indeed, you don't need a class : your class has an __init__ and a single method to_rna, it might as well be a single function.

from string import maketrans

def dna_to_rna(dna_string):
    """Return the RNA complement of the DNA strand."""
    return dna_string.translate(maketrans('GCTA', 'CGAU'))

I think see this the neatest solution. Then if on only want to create the table only once for performance reason, you can put it a variable outside the function but it's probably not needed.


Replace all instances of \ with / My main goal is to perform the conversion on a list of strings. Is this a sensible approach or should I refactor?

Please note that I cannot use Boost in my work.

#include <iostream>
#include <list>
#include <string>
#include <algorithm>

char fix_delim(char ch) {
    return ch == '\\' ? '/' : ch;
}

std::string& fix_str(std::string& str) {
   std::transform(str.begin(), str.end(), str.begin(), fix_delim);
   return str;
}

int main() {
   std::list<std::string> files;
   files.push_back("folder\\file1");
   files.push_back("folder\\file2");

   //simple example of fix_delim
   std::string one("folder\\file1");
   std::transform(one.begin(), one.end(), one.begin(), fix_delim);

   //Is this a good way to solve this problem of replacing all instances of \ with / in a list of strings?
   std::transform(files.begin(), files.end(), files.begin(), fix_str);

   return 0;
}

Seems to me that the right algorithm for this job is std::replace:

std::replace(input.begin(), input.end(), '\\', '/');

The easiest way to apply to a collection of strings is probably to use std::for_each with a lambda:

std::for_each(files.begin(), files.end(),
              [](std::string &s) { std::replace(s.begin(), s.end(), '\\', '/'); });

If you can't use a lambda, you can write a tiny function object instead:

class path_dos2unix {
    void operator()(std::string &s) {
        std::replace(s.begin(), s.end(), '\\', '/');
    }
};

...and then invoke that:

std::for_each(files.begin(), files.end(), path_dos2unix());


Longer LINQ Chaining Anyone have ideas on how to improve this code at all?

string user = User.Identity.GetUserId();

        IQueryable<Article> t =
            db.Articles.Include(a => a.Categories)
                .Include(b => b.Shared)
                .Where(s => s.Private & s.createdby == User.Identity.Name);
        IQueryable<Article> q = db.Articles.Include(a => a.Shared).Where(s => s.Shared.Any(x => x.User == user));
        IQueryable<Article> art = t.Concat(q);
        IQueryable<Category> cat = db.Categories.Where(x => art.Any(c => c.categoryID == x.Id));

Especially looking to see if anyone has a fun way of putting all this into 1 query.

The first and most obvious improvement would be better variable naming:

IQueryable<Article> privateArticlesCreatedByUser =
    db.Articles.Include(article => article.Categories)
        .Include(article => article.Shared)
        .Where(article => article.Private && article.createdby == User.Identity.Name);

IQueryable<Article> articlesSharedWithUser =
    db.Articles.Include(article => article.Shared)
        .Where(sharedArticle => sharedArticle.Shared.Any(shareRecipient => shareRecipient.User == user));

IQueryable<Article> articlesCreatedByOrSharedWithUser = privateArticlesCreatedByUser.Concat(articlesSharedWithUser);

IQueryable<Category> categories = db.Categories
    .Where(category => articlesCreatedByOrSharedWithUser.Any(articleCategory => articleCategory.categoryID == category.Id));

Obviously these names may not perfectly align with what the data means (as they're basically educated guesses on my part), but already they're a lot easier for somebody else to understand.

I also changed your & to an &&, since it then saves an equality check sometimes.

I also notice that you are including shared articles occurs twice. This might be too trivial an improvement and it's debatable if it will improve readability at all, but you could try this:

var sharedArticles = db.Articles.Include(article => article.Shared);

IQueryable<Article> privateArticlesCreatedByUser =
    sharedArticles.Include(article => article.Categories)
        .Where(article => article.Private && article.createdby == User.Identity.Name);

IQueryable<Article> articlesSharedWithUser = sharedArticles
        .Where(sharedArticle => sharedArticle.Shared.Any(shareRecipient => shareRecipient.User == user));

IQueryable<Article> articlesCreatedByOrSharedWithUser = privateArticlesCreatedByUser.Concat(articlesSharedWithUser);

IQueryable<Category> categories = db.Categories
    .Where(category => articlesCreatedByOrSharedWithUser.Any(articleCategory => articleCategory.categoryID == category.Id));


Creating a styled component inside a component function In one of my React components, I want to use props when creating a styled-component to determine which tag to use. Specifically I am looking for what's in props.headingLevel to decide which heading level to create. E.g. h1 or h2.

All of the styled-components examples show components being created outside of the React component class or function, however I have only got this working by creating the styled-component inside the function when props is available. I know you can use props inside of the styled-component via the props passed to the styled-component itself within the React function but at this point it's too late.

So my question is twofold. Is it acceptable to create styled-components inside React components functions? And, is there a better way to manage dynamic tags, e.g. creating a styled-component tag based on context or props passed?

Here is my component:

// Modules
import React from 'react';
import styled from 'styled-components';
import PropTypes from 'prop-types';

// Data
import styleData from '../../../data/styles';

const SupplementaryTitle = styled.span`
    display: block;
    text-align: right;
`

const Title = (props) => {
    const HeadingLevel = props.headingLevel ? props.headingLevel : 'h2';
    const StyledHeading = styled(HeadingLevel)`
        margin: 0;
        font-weight: ${styleData['typography_settings']['title'][props.typographySize]['weight']};
        font-size: ${styleData['typography_settings']['title'][props.typographySize]['size']};
        font-family: ${styleData['typography_settings']['title'][props.typographySize]['font']};
        line-height: ${styleData['typography_settings']['title'][props.typographySize]['line_height']};
    `

    const hasSupplementaryTitle = props.supplementaryTitle;

    if (hasSupplementaryTitle) {
        return (
            <StyledHeading>
                {props.children}
                <SupplementaryTitle>{props.supplementaryTitle}</SupplementaryTitle>
            </StyledHeading>
        )
    }

    return (
        <StyledHeading>{props.children}</StyledHeading>
    )
}

Title.propTypes = {
    children: PropTypes.string.isRequired,
    headingLevel: PropTypes.string,
    typographySize: PropTypes.oneOf([
        'small', 'medium', 'large'
    ]).isRequired,
    supplementaryTitle: PropTypes.string,
}

export default Title;

You generally should not to nest your components. Though when the component is small enough, like a one-liner, it's fine to do so.

In your specific case, you can make use of withComponent method to change the tag name of an existing component:

const Heading = styled.h2`
    margin: 0;
    ...lots of css...
`;

const Title = (props) => {
    const StyledHeading = Heading.withComponent(props.headingLevel || "h2");
    ...
};

You'd still be defining the StyledHeader inside another component, but most of the code would live outside.

Additionally...

I noticed these long lines:

font-weight: ${styleData['typography_settings']['title'][props.typographySize]['weight']};

Looks like this styleData is some sort of theme. I suggest you look into  built-in theming support of styled components before rolling your own.
The variable hasSupplementaryTitle can be eliminated. It's conventional to perform an existence check by just:

if (props.supplementaryTitle) {


Object oriented design for access control Problem Statement:

  Design an employee swap in swap out system.  The system will have a machine which records the swap in and swap out.  The user can also login in a portal where he can check his swap in /swap out time. He can correct his time also.

  There will be managers for employee who can check the entry for all the employees which are under them and correct their subordinates timings also.

  Similarly, a HR person can only view every Employee's records.

I chalked out the following skeleton. I am new to design and would like any pointers on how to approach such problems.

class User{
    String name;
    String designation;
    int id;
}

class AccessGroup{
    int priority;
    String name;
    Boolean read;
    Boolean write;
    //Setter - Getter methods
}

static class AccessManagement{
    HashSet<String, PriorityQueue<AccessGroup> accessControlTable;

    public void addUser(User u, AccessGroup g);
    public void addUserToGroup(User u, AccessGroup g);
    public void removeUser(User u);
    public void removeUserFromGroup(User u, AccessGroup g);
    public AccessGroup getPriorityAccessGroupForUser(User u);
    public PriorityQueue<AccessGroup> getUserGroups(User u);
    public boolean isValidUser(User u);
    public boolean hasReadPermission(User u);
    public boolean hasWritePermission(User u);
}

class Mediator{
    String url="";

    public List<Records> getRecords(User u);
    public void updateRecordForUser(User u, Record r, Value v){
        if(!AccessManagement.isValidUser(u))
            throw Exception();

        if(AccessManagement.hasWritePermission(u)){
            try{
                Connection conn = DriverManager.getConnection(url);
                Statement statement = conn.createStatement();
                statement.executeQuery("Update r in TimeStamps where id= u.id");
            }
            catch(Exception e){
                //Do something
            }
        }
    }
}

AccessGroup should contain a Set of permissions to be extensible, rather than have the Read permission and Write permission directly attached. Otherwise you end up with a dozen attributes hanging off AccessGroup in a year's time.
(this also means you can just union the permissions on a user to see what they can do, instead of messing with AccessGroup.getXXXX attributes)
You're overloading the terms 'add' and 'remove'. You should createUser and deleteUser
The User class is quite anemic. Perhaps you could use composition to hide the entire AccessManagement class inside it, maybe by having an AuthenticatedUser subclass that knows about its groups and permissions.
Your mediator should have specific methods to swapIn and swapOut. TBH I'd model it as an event stream so there would be no updates, only inserts.


Concept for saving context information My Problem

I have a library containing business logic which is being used from 3 different projects:

Website
Local WPF Application
WCF Service

The library uses Entity Framework and a connection string which is read out of config on first use and saved in a static variable. (The config file for all 3 consumer projects have the same keys)

Now I've changed the WCF Service, so that one service can serve different users, meaning different databases. Of course now the static connection string only holds one connection string so I need a concept of changing the used connection string depending on who called the service. Along with the connection string I have some more user-specific data which I stored in a static global class.

My Solution

On the WCF Service I use a custom authorization policy which changes the threads principal user to my own custom identity. On each call the service authenticates the caller, and changes the threads identity to the specific object which now contains the connection string and all the specific data previously stored in static global. If there is no thread principal set (or at least not from the type I expected), I assume the caller is not the service but the website or local application. Then I store the Identity in static variable like I did before.

My Code

/// <summary>
/// Represents the settings for the current context (user database).
/// Usually for web and app this is always the database defined in config-file and therefor stored in static
/// For WCF service the context can vary depending on the caller if the current principal is set
/// </summary>
public class ContextIdentity : IIdentity
{
    public static ContextIdentity LocalIdentity;
    public static ContextIdentity Current
    {
        get
        {
            if (Thread.CurrentPrincipal.Identity is ContextIdentity)
            {
                // Return the threads Context
                return Thread.CurrentPrincipal.Identity as ContextIdentity;
            }
            else
            {
                // Return the Local context
                if (LocalIdentity == null)
                {
                    LocalIdentity = new ContextIdentity();
                }
                return LocalIdentity;
            }
        }
    }

    public ContextIdentity()
    {
        // Load from File
        _name = "Default";
        Server = ConfigurationManager.AppSettings["DatabaseServer"];
        Database = ConfigurationManager.AppSettings["DatabaseName"];
        User = ConfigurationManager.AppSettings["DatabaseUser"];
        Password = ConfigurationManager.AppSettings["DatabasePass"];
        IntegratedSecurity = "True".Equals(ConfigurationManager.AppSettings["DatabaseIntegratedSecurity"]);
    }

    public ContextIdentity(string contextName, string server, string database, string user, string password, bool integratedSecurity)
    {
        // Load from File
        _name = contextName;
        this.Server = server;
        this.Database = database;
        this.User = user;
        this.Password = password;
        this.IntegratedSecurity = integratedSecurity;
    }

    #region Context Data Connection

    public string Server { get; set; }

    public string Database { get; set; }

    public string User { get; set; }

    public string Password { get; set; }

    public bool IntegratedSecurity { get; set; }

    // Other stuff needed...

}

The general approach seems reasonable to me, thought I believe it means you have to very careful around anything that can switch threads, like TPL or async-await.

public static ContextIdentity LocalIdentity;

Why is this public? From your description, it seems to me that users should never read this.

If this is so that users can set this, maybe a setter on Current would make more sense.

Also, you should use properties instead of public fields.

return Thread.CurrentPrincipal.Identity as ContextIdentity;

You already know that Indentity is ContextIdentity, so you should use a cast, not as. That way, if you make a mistake, you're going to get an exception, not null.

_name = "Default";

Why is _name a field, not a property like the others? If you don't want users changing the name, you can make the setter private.

"True".Equals(ConfigurationManager.AppSettings["DatabaseIntegratedSecurity"])

This is not Java, you can use == "True".


Create an IObservable from a method I have such a method that performs a long query search against some data.

Task<List<SearchResult>> Search(string query){ ... }

I have tried various way to create an IObservable form of it but failed. At the end after some google help I wrote such a method.

private IObservable<SearchResult[]> Search(string query)
{
    return Observable.Create((IObserver<SearchResult[]> observer)=>
        {
            List<SearchResult> result = new List<SearchResult>();
            foreach (TestsGroupMeta group in Engine.Groups)
            {
                string name = group.ToString();
                if (name.IndexOf(query, StringComparison.InvariantCultureIgnoreCase) != -1)
                {
                    result.Add(new SearchResult{ Name = name, Type = "Group"});
                }

                foreach (TestMethodMeta method in group.Methods)
                {
                    name = method.ToString();
                    if (name.IndexOf(query, StringComparison.InvariantCultureIgnoreCase) != -1)
                    {
                        result.Add(new SearchResult {Name = name, Type = "Method"});
                    }
                }
            }

            observer.OnNext(result.ToArray());
            observer.OnCompleted();

            return () => {};
        });
}

Is such a method correctly written? If not, what would be correct?

using the TaskObservableExtensions from System.Reactive.Threading.Tasks (in the Rx Linq DLL) it is easy:

IObservable<List<SearchResult>> observable = Search(queryString).ToObservable()

I'd consider the code there to be "correctly written" (I'm in no way linked to the code nor its authors.)

If you want to rewrite your Search method to work directly with Observables I'd recommend using their full power:

private IObservable<SearchResult> Search(string query)
    {
        return Observable.Create<SearchResult>(
            (observer, cancellationToken) => Task.Factory.StartNew(
                () =>
                {
                    foreach (TestsGroupMeta group in Engine.Groups)
                    {
                        if (cancellationToken.IsCancellationRequested)
                        {
                            break;
                        }

                        string name = group.ToString();
                        if (name.IndexOf(query, StringComparison.InvariantCultureIgnoreCase) != -1)
                        {
                            observer.OnNext(new SearchResult { Name = name, Type = "Group" });
                        }

                        foreach (TestMethodMeta method in group.Methods)
                        {
                            name = method.ToString();
                            if (name.IndexOf(query, StringComparison.InvariantCultureIgnoreCase) != -1)
                            {
                                observer.OnNext(new SearchResult { Name = name, Type = "Method" });
                            }
                        }
                    }

                    observer.OnCompleted();
                },
                cancellationToken,
                TaskCreationOptions.LongRunning,
                TaskScheduler.Default));
    }

An observer is now able to process SearchResults as soon as they are found (as opposed to wait for the completion of the computation yielding all results at once) and can unsubscribe anytime which in turn will cause a cancellation of the result computation. If some function still needs all SearchResultsat once there's always Observable.ToEnumerable and friends. In addition, possible parallelization is now obvious (e.g. using Parallel.ForEach) but still not trivial (hinted at by the inclusion of TaskCreationOptions and TaskScheduler, sensible options for Parallel.ForEach not included). Also clients must then be prepared to consume 'out-of-order' search results.

Potential further improvements:

check for cancellation at more / more appropriate places
split Task/Observable logic and searching
handle (now asynchronous) errors (e.g. by trying the foreach and calling observer.OnError)
specify culture forToString
employ a more 'functional' style by leveraging SelectMany (suits my personal taste)

Otherwise you could at least use Observable.Start which gets rid of the otherwise unused observer:

private IObservable<SearchResult[]> Search(string query)
{
    return Observable.Start(() =>
    {
        List<SearchResult> result = new List<SearchResult>();
        foreach (TestsGroupMeta group in Engine.Groups)
        {
            string name = group.ToString();
            if (name.IndexOf(query, StringComparison.InvariantCultureIgnoreCase) != -1)
            {
                result.Add(new SearchResult { Name = name, Type = "Group" });
            }

            foreach (TestMethodMeta method in group.Methods)
            {
                name = method.ToString();
                if (name.IndexOf(query, StringComparison.InvariantCultureIgnoreCase) != -1)
                {
                    result.Add(new SearchResult { Name = name, Type = "Method" });
                }
            }
        }
        return result;
    });
}


Enforcing C# Task to exist only once at any given point of time for a given ID There were multiple times in different applications that I needed to accomplish the following behavior with C# Task and I did it in a certain way, and would like to receive an insight whether it's the best way to achieve the desired effect, or there are other better ways.

The issue is that in certain circumstances I would like a specific Task to exist only in one instance. For example, if someone requests, let's say a list of products by executed a method like Task GetProductsAsync(), and someone else tries to request the same thing, it wouldn't fire another task, but rather return already existing task. When the GetProductsAsync finishes, all of those callers who had previously requested the result will receive the same result. So, there should ever be only one GetProductsAsync execution at a given point of time.

After failed trials to find something similar and well known design pattern to solve this issue, I came up with my own implementation:

public class TaskManager : ITaskManager
    {
        private readonly object _taskLocker = new object();
        private readonly Dictionary<string, Task> _tasks = new Dictionary<string, Task>();
        private readonly Dictionary<string, Task> _continuations = new Dictionary<string, Task>();

        public Task<T> ExecuteOnceAsync<T>(string taskId, Func<Task<T>> taskFactory)
        {
            lock(_taskLocker)
            {

                if(_tasks.TryGetValue(taskId, out Task task))
                {
                    if(!(task is Task<T> concreteTask))
                    {
                        throw new TaskManagerException($"Task with id {taskId} already exists but it has a different type {task.GetType()}. {typeof(Task<T>)} was expected");
                    }
                    else
                    {
                        return concreteTask;
                    }
                }
                else
                {
                    Task<T> concreteTask = taskFactory();
                    _tasks.Add(taskId, concreteTask);
                    _continuations.Add(taskId, concreteTask.ContinueWith(_ => RemoveTask(taskId)));
                    return concreteTask;
                }
            }
        }

        private void RemoveTask(string taskId)
        {
            lock(_taskLocker)
            {
                if(_tasks.ContainsKey(taskId))
                {
                    _tasks.Remove(taskId);
                }

                if(_continuations.ContainsKey(taskId))
                {
                    _continuations.Remove(taskId);
                }
            }
        }
    }

The idea is that we will have a single instance of TaskManager throughout the application lifetime. Any async Task request that should be executed only once at a given point in time, will call ExecuteOnceAsync providing the factory method to create the Task itself, and desired application wide unique ID. Any other task that will come in with the same ID, the Task manager with reply with the same instance of Task created before. Only if there are no other Tasks with that ID, the manager will call the factory method and will start the task. I have added locks around code task creation and removal, to ensure thread safety. Also, in order to remove the task from the stored dictionary after Task has been completed, I've added a continuation task using ContinueWith method. So, after a task has been completed, both the task itself, and its continuation will be removed.

From my side this seems to be a pretty common scenario. I would assume there is a well established design pattern, or perhaps C# API that accomplishes this exact same thing. So, any insights or suggestions will be very appreciated.

What's the point of _continuations? I can see three options which use a single dictionary:

Just store and return concreteTask. If this fails because the continuation is GC'd, document this for the benefit of maintainers.
Just store and return the continuation. If this fails because the continuation is executed multiple times, document it.
Store the task and return the continuation, so the first caller has the hard reference. I can't think how this could fail unless the caller actively discards the task.

TL;DR: if there are obvious ways of simplifying the code which break for non-obvious reasons, document the reasons so that the maintainer doesn't break it.

A further advantage of using a single dictionary would be that you could use ConcurrentDictionary instead of the manual locking. (In fact you could do this even if you do need both copies of the task, using ConcurrentDictionary<string, Tuple<Task, Task>>). The code might be as simple as

    public class TaskManager : ITaskManager
    {
        private readonly ConcurrentDictionary<string, Task> _tasks = new ConcurrentDictionary<string, Task>();

        public Task<T> ExecuteOnceAsync<T>(string taskId, Func<Task<T>> taskFactory)
        {
            var task = _tasks.GetOrAdd(
                taskId,
                id => taskFactory().ContinueWith(_ => _tasks.TryRemove(taskId, out var _)));
            return (task as Task<T>) ??
                throw new Exception($"Task with id {taskId} already exists but it has a different type {task.GetType()}. {typeof(Task<T>)} was expected");
        }
    }

I would like to add: I like the level of detail in the exception message. That should be very helpful in tracking down bugs if it is thrown and logged.


Use given digits and operators to generate an expression that yields a requested value The exercise is as follows:

  Given K digits, show all the arithmetic expressions that yield a requested value (N), using four basic arithmetic operations. Each number should be used exactly one time, and the order of the digits may be changed. Don't use parenthesis.

  Example:

    K = 1, 6, 7, 9

    N = 10

    One possible solution: 1 * 7 + 9 - 6

    Another one: 9 + 7 - 6 / 1

My code:

import itertools
from typing import (Sequence, Iterable, Iterator,
                    Union, Optional,
                    List, NamedTuple)

ALLOWED_OPERATORS = ('*', '/', '+', '-')  # , '**)
REPEAT_NUMBERS = False
DigitsSequence = Union[str, Sequence[Union[str, int]]]

class Answer(NamedTuple):
    solution: Optional[int]
    expression: str

def normalize_input(digits: DigitsSequence) -> List[str]:
    try:
        new_digits = list(map(str, digits))  # type: ignore
        if not all(map(str.isdecimal, new_digits)):
            raise TypeError
    except TypeError:
        print("Digits must be an iterable containing strings.")
        return []
    return new_digits

def any_order(items: Sequence, length: int=None, allow_repeats: bool=True) \
              -> Iterable:
    if length is None:
        length = len(items)

    if allow_repeats:
        yield from itertools.product(items, repeat=length)
    else:
        yield from itertools.permutations(items, r=length)

def zip_to_str(expression: Iterable) -> str:
    return ''.join(''.join(i) for i in expression)

def get_answer(digits: List[str], operators: Sequence) -> Answer:
    expression = zip_to_str(zip(digits, operators)) + digits[-1]
    try:
        return Answer(solution=eval(expression), expression=expression)
    except ZeroDivisionError:
        return Answer(solution=None, expression=expression)

def solver(digits: DigitsSequence, solution: int) -> Iterator[str]:
    assert isinstance(solution, int), "Solution must be a number."
    all_digits = normalize_input(digits)

    for operators in any_order(ALLOWED_OPERATORS, length=len(all_digits)-1):
        for digits_option in any_order(all_digits, allow_repeats=REPEAT_NUMBERS):
                answer = get_answer(digits_option, operators)
                if answer.solution == solution:
                    yield answer.expression

if __name__ == '__main__':
    for solution in solver('1679', solution=27):
        print(solution)

The logic in normalize_input() is a bit bizarre. First, to your credit, you are doing DbC (design by contract) and enforcing a pre-condition. I would rather see the not all test performed immediately upon entry, rather than after computing new_digits. But then your error check raises and immediately catches. Please don't do that. Just use an if instead. Returning an exception to the caller might be better than returning an empty list. If we're going down the type safety route, I guess I'm wondering why a DigitsSequence would let a bad input to sneak in in the first place. I'm a little sad that we need to admit potentially non-numeric str, and can't make that type the output of some validation function.

You wrote "Digits must be an iterable containing strings." where "Digits must be a numeric string." seems more accurate. A different test would have been to assert str(int(digits)) == digits. You're using defensive programming, but it's not clear just who you're defending against.

I'm looking at any_order. I am relatively new to PEP484 annotations. I feel allow_repeats: bool=True completely makes sense, but OTOH length: int=None seems a bit odd, as None clearly doesn't appear on the number line. Would a sentinel of -1 instead of None maybe be a better idiom here? Or should I be viewing this in terms of the java int vs Integer distinction, where potentially we can always have a null object? If None is the appropriate pythonic way to phrase it (definitely true before PEP484), then should we maybe dispense with annotations for that particular parameter?

The specification is fairly clear:

  Each number should be used exactly one time,

therefore I recommend deleting the REPEAT_NUMBERS global, and inverting the allow_repeats default value.

expression = zip_to_str(zip(digits, operators)) + digits[-1]

This is insane. I think you're making the observation that len(digits) = 1 + len(operators), but the code never enforces that. I would much much rather see zip(digits[:-1], operators) and then tack on the last element. Say what you mean and mean what you say.

solver() has a 2nd parameter of solution. For consistency, I would prefer to see Answer have a 2nd parameter of solution.

I have my doubts about DigitsSequence:
(1) maybe it's the sort of thing that offers false assurances, which gives type systems a bad name, or
(2) maybe what we really wanted was to pass in the results of list(map(int, digits)).

I am reading this line:

for operators in any_order(ALLOWED_OPERATORS, length=len(all_digits)-1):

I have no idea why the signature of any_order mentions length: int=None. That makes no sense to me. I recommend no default for that parameter.

I am reading this line:

    for digits_option in any_order(all_digits, allow_repeats=REPEAT_NUMBERS):

It essentially asks for permutations of all_digits. I'm not understanding how digits_option reflects that; it appears to me to be an ill-chosen name. I recommend digit_permutation instead.

It appears to me that Answer is not quite an accurate name - PotentialAnswer or CandidateAnswer seem closer. To retain the current name, we'd have to push the if answer.solution == solution test down into that class.


Animal Age Calculator Could someone suggest ways to simplify this code, or any other tips? All help is much appreciated. Moreover, should I comment code above or below the line of code (I have to comment as this is a requirement)?

#This imports the time and random modules
import time, random

#This defines a function called comment
def comment():
    #This makes the variable startage a global variable rather than a local variable
    global startage
    #This checks if choice is equal to 1 or equal to 2
    if choice == 1 or choice == 2:
        if startage < 25:
            print("You're a young cat or dog")
        elif startage > 25 and startage < 50:
            print("You're a middle aged cat or dog")
        elif startage > 50:
            print("You're an old cat or dog")
    elif choice == 3 or choice == 4:
        if newage < 25:
            print("You're a young cat or dog")
        elif newage > 25 and startage < 50:
            print("You're a middle aged cat or dog")
        elif newage > 50:
            print("You're an old cat or dog")
    else:
        pass

print("Welcome to the animal age calculator")
while True:
    time.sleep(1)
    choice = int(input("Which age would you like to convert?(Enter the corresponding number)\n1. Human ---> Dog\n2. Human ---> Cat\n3. Dog ---> Human\n4. Cat ---> Human\n5. Generate a random human age\n"))
    time.sleep(1)
    if choice >= 1 and choice <= 4:
        startage = int(input("What age would you like to start with?\n"))
    if choice == 1:
        print("You have chosen Human ---> Dog and the Human age is", startage)
        if startage < 3:
            newage = startage*11
        else:
            newage = 22+(startage-2)*4
    elif choice == 2:
        print("You have chosen Human ---> Cat and the Human age is", startage)
        if startage == 1:
            newage = 15
        else:
            newage = 25+(startage-2)*4
    elif choice == 3:
        print("You have chosen Dog ---> Human and the Dog age is", startage)
        if startage < 33:
            newage = startage/11
        else:
            newage = (((startage-22)/4)+2)
    elif choice == 4:
        print("You have chosen Cat ---> Human and the Cat age is", startage)
        if startage == 15:
            newage = 1
        else:
            newage = (((startage-22)/4)+2)
    elif choice == 5:
        print("You have chosen to generate a random human age.")
        time.sleep(1)
        randomage = random.randint(1,122)
        print(randomage)
    time.sleep(1)
    if choice >= 1 and choice <= 4:
        print("Your new age is:", newage)
    time.sleep(1)
    comment()
    restart = (input("Would you like to restart?(Yes or No)")).lower()
    while True:
        if restart == "yes":
            pass
        elif restart=="no":
            break

My two most important remarks concern comments and global variables.

Python is a language that is designed to be easily readable.  Your three comments simply restate the obvious, and are therefore obnoxious or harmful.  (Redundancy simply doubles the burden of code maintenance, as you have to ensure that your comments are consistent with the code.)

The most important kind of documentation in Python is the docstring — and you didn't write any!

The use of global variables makes it difficult to understand and maintain your code.  The presence of a global variable means that a reassignment could have effects beyond the scope of a function.  That defeats one of the principal advantages of packaging code into functions, which is to make it clear what the inputs and outputs are for a particular chunk of code.

Simple rewrite

Your "restart" feature doesn't actually work.  I've removed it in the code below, and am leaving it as an exercise to you to fix it.

import time, random

def comment(human_equiv_age):
    """
    Print a remark based on the human-equivalent age.
    """
    if human_equiv_age < 25:
        print("You're a young cat or dog")
    elif human_equiv_age < 50:
        print("You're a middle aged cat or dog")
    else:
        print("You're an old cat or dog")

def main():
    """
    Ask the user for inputs to an animal age conversion, and print
    the result.
    """
    print("Welcome to the animal age calculator")
    time.sleep(1)
    choice = int(input(
        "Which age would you like to convert? (Enter the corresponding number)\n"
        "1. Human ---> Dog\n"
        "2. Human ---> Cat\n"
        "3. Dog ---> Human\n"
        "4. Cat ---> Human\n"
        "5. Generate a random human age\n"
    ))
    time.sleep(1)
    if choice == 5:
        print("You have chosen to generate a random human age.")
        time.sleep(1)
        print(random.randint(1,122))
        time.sleep(1)
    elif 1 <= choice <= 4:
        input_age = int(input("What age would you like to start with?\n"))
        if choice == 1:
            print("You have chosen Human ---> Dog and the Human age is", input_age)
            chrono_age = input_age
            human_equiv_age = 11 * chrono_age if chrono_age < 3 else 22 + (chrono_age - 2) * 4
            output_age = human_equiv_age
        if choice == 2:
            print("You have chosen Human ---> Cat and the Human age is", input_age)
            chrono_age = input_age
            human_equiv_age = 15 if chrono_age == 1 else 25 + (chrono_age - 2) * 4
            output_age = human_equiv_age
        if choice == 3:
            print("You have chosen Dog ---> Human and the Dog age is", input_age)
            human_equiv_age = input_age
            chrono_age = human_equiv_age / 11 if human_equiv_age < 33 else (human_equiv_age - 22) / 4 + 2
            output_age = chrono_age
        if choice == 4:
            print("You have chosen Cat ---> Human and the Cat age is", input_age)
            human_equiv_age = input_age
            chrono_age = 1 if human_equiv_age == 15 else (human_equiv_age - 22) / 4 + 2
            output_age = chrono_age
        time.sleep(1)
        print("Your new age is:", output_age)
        time.sleep(1)
        comment(human_equivalent_age)

main()

Some additional remarks:

Python supports double-ended inequalities, like 1 <= choice <= 4.  Note that I haven't used that feature in the comment() function, though.  Can you see the bug in your code, and why I didn't use double-ended inequalities there?
Make the distinction between calendar years and human-equivalent years clear.  Those two quantities don't really have the same units, and that made your comment() function messy.
Your formulae for choices 3 and 4 are the same.  Surely, the Cat→Human calculation is not the inverse of the Human→Cat calculation.

There are other changes I would make, but I'll leave it at this to keep the review simple.


Is this an efficient way to implement Merge Sort in C++? I am studying C++ and I wrote the following Merge Sort algorithm. I need some comments from skilled C++ programmers to understand where it can be improved, in terms of execution time, memory usage and "style". I am aware this question is rather generic but really, any comment is appreciated, even more if well motivated.

Is it ok to return a new vector by value every time the recursive function is called?

template<typename T>
vector<T> mergeSort(vector<T> const &v, unsigned long s, unsigned long e){
    if (e-s == 0) return vector<T>{}; //return empty vector
    else if (e-s == 1) return vector<T>{v[s]}; //return vector with single element
    else{
        unsigned long mid = (e+s)/2;
        vector<T> v1 = mergeSort(v, s, mid);
        vector<T> v2 = mergeSort(v, mid, e);
        vector<T> vr (e-s); // allocate the space needed

        typename vector<T>::iterator p1 = v1.begin();
        typename vector<T>::iterator p2 = v2.begin();
        typename vector<T>::iterator pr = vr.begin();

        while (p1 != v1.end() && p2 != v2.end()) *pr++ = (*p1 < *p2) ? *p1++ : *p2++;
        while (p1 != v1.end()) *pr++ = *p1++;
        while (p2 != v2.end()) *pr++ = *p2++;

        return vr;
    }
}

int main(int argc, const char * argv[]) {
    vector<int> v {2, 7, -1, 4, 2, 6, -10};
    vector<int> vsorted = mergeSort(v, 0, v.size());
    return 0;
}

A single most important property of a merge sort is stability: equal elements maintain their relative order. This

    *pr++ = (*p1 < *p2) ? *p1++ : *p2++;

is a very typical mistake. In case *p1 == *p2, the element will be merged from the second array. Stability is lost. Not a problem for integers, of course, but keep in mind. The fix is simple:

    *pr++ = (*p2 < *p1) ? *p2++ : *p1++;


Python custom generator of object not efficient Let's consider the following class Signal which defines a multiphasic signal:

class Signal:
    def __init__(self, fq, phases):
        self.fq = fq
        self.phases = phases

I created a function that generates Signal to test in another function.

# -*- coding: utf-8 -*-

import itertools

def duo_SBs_to_test(SB1, SB2, frequency_step = 1, shift_step = 50, max_shift = 20000, permutations = True):

    upper_frequency = frequency_step + 1/(2*max(sum(SB1.phases), sum(SB2.phases)))*1000000

    if SB1.fq <= 100 and SB2.fq <= 100:
        r1 = 0.1
        r2 = 0.1
        upper1 = SB1.fq + r1*SB1.fq + frequency_step
        upper2 = SB2.fq + r2*SB2.fq + frequency_step
    elif SB1.fq <= 100 and SB2.fq > 100:
        r1 = 0.1
        r2 = 0.2
        upper1 = SB1.fq + r1*SB1.fq + frequency_step
        upper2 = SB2.fq + r2*SB2.fq + frequency_step
        if upper2 > upper_frequency:
            upper2 = upper_frequency
    elif SB1.fq > 100 and SB2.fq <= 100:
        r1 = 0.2
        r2 = 0.1
        upper1 = SB1.fq + r1*SB1.fq + frequency_step
        upper2 = SB2.fq + r2*SB2.fq + frequency_step
        if upper1 > upper_frequency:
            upper1 = upper_frequency
    else:
        r1 = 0.2
        r2 = 0.2
        upper1 = SB1.fq + r1*SB1.fq + frequency_step
        upper2 = SB2.fq + r2*SB2.fq + frequency_step
        if upper1 > upper_frequency:
            upper1 = upper_frequency
        if upper2 > upper_frequency:
            upper2 = upper_frequency

    for s, f1, f2 in itertools.product(range(0, max_shift+shift_step, shift_step),
                                       range(int(SB1.fq - r1*SB1.fq), int(upper1), frequency_step),
                                       range(int(SB2.fq - r2*SB2.fq), int(upper2), frequency_step)):

        yield (Signal(f1, SB1.phases), Signal(f2, SB2.phases), s)
        if permutations:
            yield (Signal(f2, SB2.phases), Signal(f1, SB1.phases), s)

Surprisingly, the performance isn't great. It's my first time creating a generator, and I guess I missed something.

%timeit res = [elt for elt in duo_SBs_to_test(SB1, SB3, frequency_step = 1,
shift_step = 50, max_shift = 20000, permutations = True)]
829 ms ± 11.2 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

%timeit res = [elt for elt in duo_SBs_to_test(SB1, SB3, frequency_step = 1,
shift_step = 50, max_shift = 20000, permutations = True)]
842 ms ± 13.7 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

%timeit for elt in duo_SBs_to_test(SB1, SB3, frequency_step = 1,
shift_step = 50, max_shift = 20000, permutations = True): continue
765 ms ± 5.81 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

%timeit for elt in duo_SBs_to_test(SB1, SB3, frequency_step = 1,
shift_step = 50, max_shift = 20000, permutations = True): continue
768 ms ± 5.66 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

The former function I was using was storing frequency_to_test in a list, shifts in a numpy array before running the itertools.product() and storing in a list the Signals_to_test. Yet, it was just as fast...

EDIT:

SB1 = Signal(50, [300, 50, 900])
SB3 = Signal(80, [300, 50, 900])

Thanks for the tips :)

EDIT2: Next step:

def compute(SB1, SB2, frequency_step = 1, shift_step = 50, max_shift = 20000, permutations = True):

    result = list()
    for duo_generated in duo_SBs_to_test(SB1, SB2, frequency_step, shift_step, max_shift, permutations):
        if not condition(duo_generated):
            continue
        else:
            result.append(duo_generated)

    return result

condition() is a function returning a boolean.

Your ifs structure is needlessly complicated. They only set r1 and r2 based on simple rules (r1 [resp. r2] is 0.1 if SB1.fq [resp. SB2.fq] is lower than 100 and 0.2 otherwise) and compute upper1 and upper2 using the same formula. So you can use the simpler:

r1 = 0.1 if SB1.fq <= 100 else 0.2
r2 = 0.1 if SB2.fq <= 100 else 0.2
upper1 = (1 + r1) * SB1.fq + frequency_step
upper2 = (1 + r2) * SB2.fq + frequency_step

You can also use min to make these values not go over upper_frequency. But this is still some redundancies, especially given the fact that, later, you use r1 & SB1 and r2 & SB2 in the same kind of pattern in your ranges. So better extract out a function:

def frequency_range(SB, step, higher_frequency):
    r = 0.1 if SB.fq <= 100 else 0.2
    high = (1 + r) * SB.fq + step
    low = (1 - r) * SB.fq
    return range(int(low), int(min(high, higher_frequency)))

And the main function is now:

def duo_SBs_to_test(SB1, SB2, frequency_step=1, shift_step=50, max_shift=20000, permutations=True):
    max_phases = max(sum(SB1.phases), sum(SB2.phases))
    upper_frequency = frequency_step + 1/(2 * max_phases) * 1000000

    parameters = itertools.product(
            range(0, max_shift + shift_step, shift_step),
            frequency_range(SB1, frequency_step, upper_frequency),
            frequency_range(SB2, frequency_step, upper_frequency))

    for s, f1, f2 in parameters:
        yield Signal(f1, SB1.phase), Signal(f2, SB2.phase), s
        if permutations:
            yield Signal(f2, SB2.phase), Signal(f1, SB1.phase), s

Now the only little change that I can think of that could improve performances is to avoid testing permutations at each iteration. So you could perform two "radicaly" different iterations depending on the value of permutations:

def duo_SBs_to_test(SB1, SB2, frequency_step=1, shift_step=50, max_shift=20000, permutations=True):
    max_phases = max(sum(SB1.phases), sum(SB2.phases))
    upper_frequency = frequency_step + 1/(2 * max_phases) * 1000000

    parameters = itertools.product(
            range(0, max_shift + shift_step, shift_step),
            frequency_range(SB1, frequency_step, upper_frequency),
            frequency_range(SB2, frequency_step, upper_frequency))

    if permutations:
        for s, f1, f2 in parameters:
            yield Signal(f1, SB1.phase), Signal(f2, SB2.phase), s
            yield Signal(f2, SB2.phase), Signal(f1, SB1.phase), s
    else:
        for s, f1, f2 in parameters:
            yield Signal(f1, SB1.phase), Signal(f2, SB2.phase), s

Now, as regard to your next step, you’re basically just reinventing filter. Just use that instead:

def compute(SB1, SB2, frequency_step = 1, shift_step = 50, max_shift = 20000, permutations = True):
    return list(filter(condition, duo_SBs_to_test(SB1, SB2, frequency_step, shift_step, max_shift, permutations))

And if that compute function is called in a for loop, you can also remove the need to convert it to a list.


Display Thousands/Millions of different background images Kindly help me check this code, I want to be able to make it load thousands/millions of different pictures as HTML background without crashing or slowing down the browser.

The images can be created dynamically, or linked to its folder, I was able to do that for up to 1000 images/colors per canvas, and add up many more canvas, but at 30 canvas, which is 30,000 images/colors the browser stops to respond(slowish). Tested only on google chrome, so help me achieve tens of millions random colors/pictures.

FYI: The images must be displayed all at once.

  Rihana Photo Mosaic or picturemosaics this is similar to what i want to achieve, as you can see about 36,000 images without any performance issue, they even boast of more than 5 million pictures. Mine should be easier because my pictures are to be random or in a pattern not necessarily being in a mosaic form. If you have another method or way to achieve it, that would be helpful too.

<html>
<body>
  <script>
  var cInt=0;
  var loopCInt=0;
  while (loopCInt<3){
   var canvasName = document.createElement("canvas");
   canvasName.setAttribute('width', "1000");
   canvasName.setAttribute('height', "1000");
   var canvasIdName="myCanvas"+cInt;
   canvasName.setAttribute('id', canvasIdName);
   document.body.appendChild(canvasName);
    var c = document.getElementById(canvasIdName);
    var ctx = c.getContext("2d");
    var i = 0;
    var right=0;
    var tops=0;
    var goDown=0;
    while (i <= 1000) {
      ctx.beginPath();
      ctx.rect(right, goDown, 30, 30);
      ctx.fillStyle = '#'+Math.random().toString(16).slice(-6);
      ctx.fill();
      i++;
      right=right+30;
      tops++;
      if (tops==30){
        goDown=goDown+30;
        right=0;
        tops=0;
      }
    }

  loopCInt++;
  cInt++;
}
</script>
</body>
</html>

Sluggishness

Memory limits

Take a look at a random folder of images on your computer, and observe it's size. Thousands of images will amount to gigabytes of space, which means all that needs to be loaded into memory. You can simply not do that reliably.

There is also not really a point in rendering things that the user cannot see.

Thousands or millions of images?

Consider why you think you need to display thousands or millions of images. Consider a pretty sizeable screen of 1920 by 1080 pixels. If you have 30 pixels in width and 30 pixels of height, you can fit 60 * 36, or 2304 images on screen. At some point you will not be able to make out anything.

Multiple canvas elements

There does not seem to be a reason to use multiple canvas elements. If you want a background with hundreds of images, you can redraw the same canvas as you scroll. In fact, this is probably the best way of dealing with your current situation.

Coding style

Wrap your code in an IIFE

Wrap your code in an Immediately Invoked Function Expression. This causes the variables to not spill into other code you or others write. An IIFE looks something like this:

(function () {
  // ...
})();

Look at your variable names

Some of your variable names do not really make sense. cInt seems an iteration of your canvas element. I am not sure why you use loopCInt at all. Replace it with canvasIdentifier or canvasIteration. canvasName is actually a CanvasElement, not a name. goDown looks like a function name at first glance, but is actually the number of pixels from the top, so just name it top. right is actually what you would refer to as left if you worked with absolutely positioned elements, namely the number of pixels from the left edge.

Bookkeeping

You are using a while loop, and because of that have a lot of lines related to bookkeeping. Since you are managing both a row and a column in a single loop, it is not very readable either. I would recommend replacing it with two for loops.

Factor out functions

You want to start factoring out functions with some kind of behaviour earlier than later in your code. You currently have one giant blob of code that, while it does what you want to do, is a nightmare to read through when you expand it. A pretty obvious thing to abstract out is to move things to draw the scene into their own function. You can then call that function to draw or redraw things.

Factor out magic numbers/strings

You currently set "1000" for the width and height, and draw squares of 30 by 30. Move these numbers/strings to their own variable that you can easily change. This allows you to re-use them and quickly change them when you decide, for example, to have a border of 10 pixels around your canvas.

Example

Below I applied some of these to your code. I also put in an example where the canvas is redrawn when you scroll the screen. I added a random number generator that can be seeded, so that I can show off that you can create a consistent screen. You can probably improve on that, considering that it now generates too many colors.

// https://stackoverflow.com/a/19301306/2209007
function RandomGenerator() {
  var m_w = 123456789;
  var m_z = 987654321;
  var mask = 0xffffffff;

  // Takes any integer
  this.seed = function(i) {
      m_w = i;
      m_z = 987654321;
  }

  // Returns number between 0 (inclusive) and 1.0 (exclusive),
  // just like Math.random().
  this.random = function()
  {
      m_z = (36969 * (m_z & 65535) + (m_z >> 16)) & mask;
      m_w = (18000 * (m_w & 65535) + (m_w >> 16)) & mask;
      var result = ((m_z << 16) + m_w) & mask;
      result /= 4294967296;
      return result + 0.5;
  }

  this.randomHexColor = function()
  {
    var hexFFFFFF = 16777215;

    return '#' + Math.floor(this.random() * hexFFFFFF).toString(16);
  }
}

(function() {
  var canvas = document.getElementById('background-canvas');
  var ctx = canvas.getContext("2d");
  var windowWidth = Math.max(document.documentElement.clientWidth, window.innerWidth || 0);
  var windowHeight = Math.max(document.documentElement.clientHeight, window.innerHeight || 0);
  canvas.setAttribute('width', windowWidth);
  canvas.setAttribute('height', windowHeight);

  var imageWidth = 30;
  var imageHeight = 30;

  var randomGenerator = new RandomGenerator();

  function draw(offset)
  {
    window.requestAnimationFrame(function () {
      randomGenerator.seed(1);

      for (var top = -offset; top < windowHeight; top += imageHeight) {
        for (var left = 0; left < windowWidth; left += imageWidth) {
          if (offset < -imageHeight) {
            randomGenerator.randomHexColor();
            continue;
          }
          ctx.beginPath();
          ctx.rect(left, top, imageWidth, imageHeight);
          ctx.fillStyle = randomGenerator.randomHexColor();
          ctx.fill();
        }
      }
    });
  }

  draw(0);

  document.addEventListener('scroll', function() {
    var scrollTop = window.pageYOffset || document.documentElement.scrollTop || document.body.scrollTop || 0;

    draw(scrollTop);
  });
})();
#background-canvas {
  width: 100vw;
  height: 100vh;
  position: fixed;
  top: 10px;
  left: 10px;
  z-index: -1;
}

body {
  height: 200vh;
}
<canvas id="background-canvas"></canvas>


Guess the Number Game in Python I have been using Python to learn programming, but I am just recently beginning to utilize classes. I have a basic game below. Please review and let me know what I should improve on.

import random

class Game:

    def __init__(self):
        self.winningNumber = random.randint(0,100)
        self.humanGuess()

    def humanGuess(self):
        self.humanGuessNumber = int(input("Guess from 0-100"))
        self.comGuess()
        return self.humanGuessNumber
    def comGuess(self):
        self.comGuessNumber = random.randint(0,100)
        self.conclude()
        return self.comGuessNumber
    def conclude(self):
        if abs(self.winningNumber - self.humanGuessNumber)< abs(self.winningNumber - self.comGuessNumber):
            print("You win")
        else:
            print("You suck")

game = Game()

You defined the class Game. However, this is an extremely vague name. Even if you have already named your module something like guessthenum, you should still give the class a specific name. I would select GuessTheNum as the class name.

Next up, we have your initialization function:

def __init__(self):
    self.winningNumber = random.randint(0,100)
    self.humanGuess()

Nothing much to talk about here, other than my minor nitpick that you should add a space after your commas. I do have something to talk about the humanGuess and comGuess functions:

def humanGuess(self):
    self.humanGuessNumber = int(input("Guess from 0-100"))
    self.comGuess()
    return self.humanGuessNumber

def comGuess(self):
    self.comGuessNumber = random.randint(0,100)
    self.conclude()
    return self.comGuessNumber

The problem with the humanGuess function is that it forces input to be passed using the input function. In my opinion, classes should not engage in any form of IO unless strictly necessary (for example, a class for reading files).

Following this change, there is no need to return anything from humanGuess and comGuess. After all, the data is stored in the class.

To fix these problems, I would change both all three functions to this:

def __init__(self):
    self.winningNumber = random.randint(0, 100)

def humanGuess(self, n):
    self.humanGuessNumber = n

def comGuess(self):
    self.comGuessNumber = random.randint(0, 100)

Next, the conclude function:

def conclude(self):
    if abs(self.winningNumber - self.humanGuessNumber)< abs(self.winningNumber - self.comGuessNumber):
        print("You win")
    else:
        print("You suck")

Once again, following the no IO rule, I restructured the function such that it would return a boolean value indicating if the player wins:

def conclude(self):
    return (
        abs(self.winningNumber - self.humanGuessNumber)
        < abs(self.winningNumber - self.comGuessNumber)
    )

Now that we've cleared up the functions, let's take a look at the whole class:

class GuessTheNum:
    def __init__(self):
        self.winningNumber = random.randint(0, 100)

    def humanGuess(self, n):
        self.humanGuessNumber = n

    def comGuess(self):
        self.comGuessNumber = random.randint(0, 100)

    def conclude(self):
        return (
            abs(self.winningNumber - self.humanGuessNumber)
            < abs(self.winningNumber - self.comGuessNumber)
        )

I find that the structure of this class is unwieldy. If I were to design a GuessTheNum class, I would have just a single isWin function. It would take in my guess, generate a computer guess, then tell me if I suck or not.

Here's the way I would have written the full program:

class GuessTheNum:
    def __init__(self):
        self.winningNumber = random.randint(0, 100)

    def isWin(self, n):
        self.humanGuessNumber = n
        self.comGuessNumber = random.randint(0, 100)
        return (
            abs(self.winningNumber - self.humanGuessNumber)
            < abs(self.winningNumber - self.comGuessNumber)
        )

gtn = GuessTheNum()
n = int(input("Guess a number from 0 to 100: "))
if gtn.isWin(n):
    print("You win")
else:
    print("You suck")

EDIT 1:

@jonrsharpe reminded me that I had a DRY violation: 0 and 100 was repeated in a few places. Here's the updated version:

class GuessTheNum:
    def __init__(self, lower, upper):
        self.lower, self.upper = lower, upper
        self.winningNumber = random.randint(self.lower, self.upper)

    def isWin(self, n):
        self.humanGuessNumber = n
        self.comGuessNumber = random.randint(self.lower, self.upper)
        return (
            abs(self.winningNumber - self.humanGuessNumber)
            < abs(self.winningNumber - self.comGuessNumber)
        )

lower, upper = 0, 100
gtn = GuessTheNum(lower, upper)
n = int(input("Guess a number from %d to %d: " % (lower, upper)))
if gtn.isWin(n):
    print("You win")
else:
    print("You suck")

EDIT 2:

@TheBlackCat gave a few suggestions and reminded me of the PEP style. This fixes another DRY with random.randint.

class GuessTheNum:
    def _gen_number(self):
        return random.randint(self.lower, self.upper)

    def __init__(self, lower=0, upper=100):
        self.lower, self.upper = lower, upper
        self.win_num = self._gen_number()

    def isWin(self, human_guess):
        com_guess = self._gen_number()
        return (
            abs(self.win_num - human_guess)
            < abs(self.win_num - com_guess)
        )

lower, upper = 0, 100
gtn = GuessTheNum(lower, upper)
n = int(input("Guess a number from %d to %d: " % (lower, upper)))
if gtn.isWin(n):
    print("You win")
else:
    print("You suck")


Single method string formatter Because I don't want to download an entire library for a single function (which I do understand, was made better than I made mine), I decided to implement my own string formatting function.

I am not very proud of it, as I find it pretty ugly and unreadable.
This is only a string formatter with string-only arguments.
Efficiency is not a concern for me.

#include<iostream>
#include<unordered_map>
#include<string>

using std::string;
string formatString(string format, const std::unordered_map<string, string>& args) {
    string ret;

    string::size_type bracketLoc;
    while((bracketLoc = format.find_first_of('{')) != string::npos) {
        // Handling the escape character.
        if(bracketLoc > 0 && format[bracketLoc - 1] == '\\') {
            ret += format.substr(0, bracketLoc + 1);
            format = format.substr(bracketLoc + 1);
            continue;
        }

        ret += format.substr(0, bracketLoc);
        format = format.substr(bracketLoc + 1);

        bracketLoc = format.find_first_of('}');
        string arg = format.substr(0, bracketLoc);
        format = format.substr(bracketLoc + 1);

        auto it = args.find(arg);
        if(it == args.end()) {
            ret += "(nil)";
        } else {
            ret += it->second;
        }
    }

    ret += format;

    return ret;
}

int main() {
    std::cout << formatString("Hello, {Name}! {WeatherType} weather, right?", {
        {"Name", "Midnightas"},
        {"Fruit", "Apple"}
    });

    return 0;
}

Compile with -std=c++11.
The above program will output Hello, Midnightas! (nil) weather, right?.

You have a problem if you want to have the escape-character immediately before a replacement, because you don't support escaping the escape-character.
I suggest dispensing with escape-characters, and simply replace an empty replacement {} with an opening brace {.
There's no reason to modify the format-string at all, and thus receiving it by copy. Doing so is supremely inefficient. Well, using named arguments at all already is, so it might not matter too much.
Do you know the ternary operator cond ? true_exp : false_exp? Using it would simplify things.
Avoid allocations, thus avoid std::string. At least the format string should be C++17 std::string_view, maybe also the map should be too.
If you template it, you don't even really have to decide for the caller:

template <class Args = std::unordered_map<std::string_view, std::string_view>>
std::string formatString(std::string_view format, const Args& args)


Verifying IPv6 addresses While trying to learn Python I stumbled upon a question which asks you to write a code that is able to take an input and verify it to see whether it meets the IPv6 Criteria.

I wrote the following code:

valid_characters = ['A', 'B', 'C', 'D', 'E', 'F', 'a', 'b', 'c', 'd', 'e', 'f', ':', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9']
address = input('Please enter an IP address: ')
is_valid = []

for i in range(len(address)):
    current = address[i]
    for j in range(len(valid_characters)):
        check = valid_characters[j]
        if check == current:
            a = 1
            is_valid.append(a)

address_list = address.split(":")
invalid_segment = False

for i in range(len(address_list)):
    current = address_list[i]
    if len(current) > 4:
        invalid_segment = True

if len(address) == len(is_valid) and len(address_list) == 8 and invalid_segment == False:
    print("It is a valid IPv6 address.")

elif invalid_segment:
    print("It is not a valid IPv6 address.")

else:
    print("It is not a valid IPv6 address.")

Although the code works and for the sample cases that were provided it is successfully able to confirm whether the entered IP address is IPv6 or not, I feel like my solution is not really elegant enough and that there should be a better way to do this.

I would really appreciate it if someone could point me to the right direction for how I should condense or improve my code.

Please note that I am new to Python, with little to no experience.

Python has a philosophy of "Batteries Included".  Don't reinvent what's in the standard library!

In particular, this code is much more easily written if we take advantage of IPv6Address:

import ipaddress

address = input('Please enter an IP address: ')

try:
    addr = ipaddress.IPv6Address(address)
except ipaddress.AddressValueError:
    print(address, 'is not a valid IPv6 address')
else:
    if addr.is_multicast:
        print(address, 'is an IPv6 multicast address')
    if addr.is_private:
        print(address, 'is an IPv6 private address')
    if addr.is_global:
        print(address, 'is an IPv6 global address')
    if addr.is_link_local:
        print(address, 'is an IPv6 link-local address')
    if addr.is_site_local:
        print(address, 'is an IPv6 site-local address')
    if addr.is_reserved:
        print(address, 'is an IPv6 reserved address')
    if addr.is_loopback:
        print(address, 'is an IPv6 loopback address')
    if addr.ipv4_mapped:
        print(address, 'is an IPv6 mapped IPv4 address')
    if addr.sixtofour:
        print(address, 'is an IPv6 RFC 3056 address')
    if addr.teredo:
        print(address, 'is an IPv6 RFC 4380 address')


Shuffle deck of cards in JavaScript I know randomness in programming can be a deep topic, and searching the topic seems to result in Fisher-Yates being the best method, but I'm curious what people would say about this method.

Basically, since I have the cards as objects, I just add a property of a random number then sort the entire array based on those random numbers.

const suits = [
  {
    digit: 'H',
    word: 'hearts'
  },
  {
    digit: 'C',
    word: 'clubs'
  },
  {
    digit: 'D',
    word: 'diamonds'
  },
  {
    digit: 'S',
    word: 'spades'
  }
]
const cardsWithoutSuits = [
  {
    numeric: 1,
    word: 'ace',
    digit: 'A'
  },
  {
    numeric: 2,
    word: 'two',
  },
  {
    numeric: 3,
    word: 'three',
  },
  {
    numeric: 4,
    word: 'four',
  },
  {
    numeric: 5,
    word: 'five',
  },
  {
    numeric: 6,
    word: 'six',
  },
  {
    numeric: 7,
    word: 'seven',
  },
  {
    numeric: 8,
    word: 'eight',
  },
  {
    numeric: 9,
    word: 'nine',
  },
  {
    numeric: 10,
    word: 'ten',
  },
  {
    numeric: 11,
    word: 'jack',
    digit: 'J'
  },
  {
    numeric: 12,
    word: 'queen',
    digit: 'Q'
  },
  {
    numeric: 13,
    word: 'king',
    digit: 'K'
  }
]

function createDeck(decks = 1){
  let deck = [];
  for (let i = 0; i < decks; i++) {
    suits.forEach( x => {
      cardsWithoutSuits.forEach( y => {
        deck.push({
          numeric: y.numeric,
          word: y.word,
          suit: x.word,
          phrase: `${y.word} of ${x.word}`,
          abbr: `${y.hasOwnProperty('digit') ? y.digit : y.numeric}${x.digit}`
        })
      })
    })
  }
  return deck;
}

function shuffle(array){
  array.forEach( x =>{
    x.ran = Math.random();
  })
  array.sort( (a, b) =>{
    return a.ran - b.ran;
  })

  return array;
}

let deck = shuffle(createDeck(2));
console.log(deck);

There has already been some discussion by @Oh My Goodness and @Blindman67 in the comments about the time complexity of this code vs Fisher-Yates. I don't really have much to add to that discussion but will offer some suggestions about the code on a deeper level below.

Like I mentioned in my answer to your post Simple math game in JavaScript some variable names could more accurately hint at the semantic meaning of the value - for example, instead of x.ran = Math.random();, a name like sortVal would hint that the value is to be used for sorting.

array.forEach( x =>{
  x.sortVal = Math.random();
})

The arrow function in the shuffle() method is quite short and thus can be simplified to a single line:

array.sort( (a, b) => a.sortVal - b.sortVal)

The forEach block above it could also be simplified:

array.forEach( x => x.sortVal = Math.random())

In this case, x.sortVal would be returned in each iteration but that doesn't make a difference.

The Array sort method Array.prototype.sort() returns the array so the shuffle() function could be simplified to have the return statement on the same line as the call to that method:

  return array.sort( (a, b) => a.sortVal - b.sortVal)

Additionally, any value that doesn't get re-assigned could be declared using const, which would help avoid any unintentional re-assignment - e.g. deck within shuffle(), and deck at the end of the code.


"Longest Collatz sequence" in C slower than in Python 3 This tight-looped memoization-less C solution to Project Euler's problem 14 seems efficient, but ran much slower than a similarly trivial Python 3 program. For the desired upper of 1000000, the C version ran practically forever, while Python 3 fairly quickly printed the correct result.

#include <stdio.h>

#define upper 1000000

int main() {
    int max = 1;
    int maxl = 1;

    for (int n = 1; n < upper; ++ n) {
        int c = n; // After v5r's answer: Should be `long c = n;`
        int cl = 1;

        while (c != 1) {
            if (c % 2 == 0) {
                c /= 2;
            }
            else {
                c = 3 * c + 1;
            }
            ++ cl;

            if (cl > maxl) {
                max = n;
                maxl = cl;
            }
        }
    }

    printf("%i\n", max);

    return 0;
}

Python 3:

#! /usr/bin/env python3

max = 1
maxl = 1

for n in range(1, 1000000):
    c = n
    cl = 1

    while c != 1:
        if c % 2 == 0:
            c /= 2
        else:
            c = 3 * c + 1
        cl += 1

        if cl > maxl:
            max = n
            maxl = cl

print(max)

Assuming int is 32 bit long on your system, an integer overflow occurs on c with upper = 1000000. The multiplication in c = 3 * c + 1 can cause c to grow quite large, eventually exceeding the \$2^{31}-1\$ upper limit of a 32-bit signed integer, and thus rolling over into negative values. Obviously, the algorithm does not work correctly for negative values of c, and gets stuck.

In Python on the other hand, none of this occurs, since it extends integers to arbitrary length automatically, and thus avoids overflows.

You can "postpone" this problem (i.e. make your program work for larger values of upper) by using larger (and unsigned) integer types, such as uint64_t from stdint.h for c up to \$2^{64}-1\$.

However, if you want your C code to work for arbitrarily large upper values, you'll have to use an external library for arbitrary-precision integers, such as GNU GMP (or roll your own, which is really not advisable except as an exercise). The disadvantage is slightly worse performance, which would likely be comparable to the Python version for such a simple and arithmetics-heavy program.


Am I using copy ctors/move ctors/shared_ptr correctly? Here is my simple example: there is a class A with few primitive members and a class B with few primitive members but also a collection of objects of type A.

This is my A class:

#pragma once

#include<string>

class Hero
{
private:
    long id;
    std::string name;
    int level;
    static long currentId;
public:
    Hero();
    Hero(std::string name, int level);
    Hero(const Hero &hero);
    Hero(Hero &&hero);
    Hero& operator=(const Hero &hero);
    Hero& Hero::operator=(const Hero &&hero);
    long GetId() const { return this->id; }
    std::string GetName() const { return this->name; }
    int GetLevel() const { return this->level; }
    void SetName(const std::string &name);
    void SetLevel(const int &level);
};

#include"Hero.h"

long Hero::currentId = 0;

Hero::Hero(std::string name, int level):name(name), level(level), id(++currentId)
{

}

Hero::Hero():name(""), level(0), id(++currentId)
{

}

Hero::Hero(const Hero &hero):id(hero.id), name(hero.name), level(hero.level)
{

}

Hero::Hero(Hero &&hero):id(hero.id), name(hero.name), level(hero.level)
{

}

Hero& Hero::operator=(const Hero &hero)
{
    this->id = hero.id;
    this->name = hero.name;
    this->level = hero.level;
    return *this;
}

Hero& Hero::operator=(const Hero &&hero)
{
    this->id = hero.id;
    this->name = hero.name;
    this->level = hero.level;
    return *this;
}

void Hero::SetName(const std::string &name)
{
    this->name = name;
}

void Hero::SetLevel(const int &level)
{
    this->level = level;
}

This is my B class:

#pragma once

#include"Hero.h"
#include<vector>
#include<memory>

class Race
{
private:
    static long currentId;
    long id;
    std::string name;
    std::vector<std::shared_ptr<Hero>> heroes;
    Race(const Race &race); //disable copy constructor
    Race& operator =(const Race &race); //disable assign operator
public:
    Race();
    explicit Race(std::string name);
    std::string GetName() const { return this->name; }
    void SetName(std::string name);
    void AddHero(Hero &&hero);
    std::vector<std::shared_ptr<Hero>> GetHeroes() const { return heroes; }
};

   #include "Race.h"

long Race::currentId = 0;

Race::Race(std::string name):name(name), heroes(std::vector<std::shared_ptr<Hero>>()), id(++currentId)
{

}

Race::Race():name(""), heroes(std::vector<std::shared_ptr<Hero>>()), id(++currentId)
{

}

void Race::SetName(std::string name)
{
    this->name=name;
}

void Race::AddHero(Hero &&hero)
{
    heroes.push_back(std::make_shared<Hero>(hero));
}

And this is my main:

#include "stdafx.h"

#include"Race.h"

int _tmain(int argc, _TCHAR* argv[])
{
    Hero elvesHero1("Demon hunter", 8);
    Hero elvesHero2("Warden", 4);
    Race nightElf("Night elf");
    nightElf.AddHero(std::move(elvesHero1));
    nightElf.AddHero(std::move(elvesHero2));
    std::vector<std::shared_ptr<Hero>> heroes=nightElf.GetHeroes();
    return 0;
}

One remark: When the debugger hits nightElf.AddHero(std::move(elvesHero1)); it enters Hero's copy constructor. Shouldn't it go through move constructor?

I just want start on the right foot, so I am waiting for your tips.

LE: If I modify the AddHero method like this:

void Race::AddHero(Hero &&hero)
{
    heroes.push_back(std::make_shared<Hero>(std::move(hero)));
}

it gives the parameter std:move(hero) instead of hero.  It enters move constructor instead of copy constructor.

Firstly, in your header file, you don't need the Hero:: qualification on your move assignment operator. Secondly, you are defining it as Hero& Hero::operator=(const Hero &&hero);. Think about what this is saying - it's saying if you have an rvalue reference to a Hero object, then:

You cannot modify it because it is const
You want to utilize move semantics (specifically, std::move) which will modify the original object and leave it with an empty value.

Hence, your move constructor should always be of the form Hero& Hero::operator=(Hero&& hero). The implementation should also be using std::move:

Hero& operator=(Hero&& hero)
    : id(std::move(hero.id),
      name(std::move(hero.name),
      level(std::move(hero.level)
{ }

For your AddHero methods, a Hero&& hero might be an rvalue reference, but without the std::move for its elements, you're basically saying "take an rvalue reference to Hero, and call copy constructors on each of its elements when you utilize make_shared<Hero>".

Note that moving vs copying for basic types like int or long won't have any performance difference.

That being said, you're doing a lot of work here for no benefit. The rule is, unless you have written a custom destructor, you don't need to write custom copy constructors, copy assignment operators or move assignment operators - the compiler will generate defaults which are correct. It is only when you are managing memory (hence have a delete or delete[] in a destructor somewhere) that you need to implement these.

Finally, a few other small points. Your constructors should be taking strings by const &, ie, explicit Race(const std::string& name). Usage of this-> to return member variables is a bit unidiomatic in C++, likewise in copy constructors and the like, instead of this->id = hero.id; it's more idiomatic to simply write id = hero.id;.

I'd suggest reading the Rule of Three post on stackoverflow to get a better understanding of copy construction and copy assignment.


Akka actor to calculate a soccer league rank I am new to Scala / Akka and this is my first attempt at writing an actor.

The task that I am trying to accomplish is: given a sequence of soccer matches, generate a ranking based on the usual rules of soccer (winner gets 3 points, loser gets 0 points, and 1 point each in case of a draw).

Here is my current ranker actor:

package soccerLeague.akka

import akka.actor.Actor
import soccerLeague.common.{LeagueRules, MatchResult, TeamWithPoints}

// Stateful Ranker actor that maintains a rank.
// The rank gets updated every time a MatchResult message is received.
class RankerActor(rules: LeagueRules) extends Actor {

  private var rank: Seq[TeamWithPoints] = List()
  private var rankMap = Map[String, Int]()

  def receive = {
    case MatchResult(team1, score1, team2, score2) => {
      updateRank(MatchResult(team1, score1, team2, score2))
      sender ! rank
    }
    case Message("getRank") => {
      sender ! rank
    }
  }

  private def updateRank(matchRes: MatchResult): Unit = {

    def calcDeltaPoints(score1: Int, score2: Int): Int = {
      if(score1 == score2) rules.drawScore
      else if(score1 > score2) rules.winScore
      else rules.lossScore
    }

    val newPoints1 = rankMap.getOrElse(matchRes.team1, 0) + calcDeltaPoints(matchRes.score1, matchRes.score2)
    val newPoints2 = rankMap.getOrElse(matchRes.team2, 0) + calcDeltaPoints(matchRes.score2, matchRes.score1)

    rankMap += (matchRes.team1 -> newPoints1, matchRes.team2 -> newPoints2)

    // Sort by score, in reverse order (i.e. the team with most points is first).
    // Teams that have the same score are sorted in alphabetical order.
    // TODO: this *can* be improved by only moving around the two teams that are changing.
    rank = rankMap.toSeq
      .map { case (t, s) => new TeamWithPoints(t, s) }
      .sortBy { case TeamWithPoints(team, points) => (-points, team) }
  }

}

I have a few supporting case classes:

case class MatchResult(team1: String, score1: Int, team2: String, score2: Int)
case class LeagueRules(winScore: Int, drawScore: Int, lossScore: Int)
case class TeamWithPoints(team: String, points: Int)

What are ways that I can make this code better and more idiomatic?
How can I avoid deconstructing the MatchResult (in the receive method) and then reconstructing it again when I want to pass it down to the updateRank method?

For starters you could try sending commands to you Actor instead of Nouns(source: Reactive Messaging Patterns with the Actor Model).

Put commands into a companion object for Actor.
Name commands in past tense.
Remove your "literal match" [Message("getRank")] and create a Command(type-safet) to handle the method.

See this thread for information on naming Commands.
https://stackoverflow.com/questions/12510535/naming-convention-for-commands-events