Page for editing an item from an expenses list I have been working on a beginner project, learning React and Typescript. The project is an expense manager application and I have tried splitting most of my applications into simple short components.
For the EditExpensePage component I have connected it to the Redux store for the app, in order to be able to access the state and to dispatch actions.

I would like to get some feedback on the code I've written, and ways I could improve it.

The Expense interface :

export interface Expense {
    id: string;
    description: string;
    note?: string;
    amount: number;
    createdAt: number;
}

The EditExpensePage :

import * as React from "react";
import { RouteComponentProps } from "react-router";
import * as Redux from 'redux';
import {withRouter} from 'react-router';
import {connect} from 'react-redux';
import { Expense } from "../types/Expense";
import { AppState } from "../Store/configureStore";
import { compose } from "redux";
import ExpenseForm from "./ExpenseForm";
import { AppActions } from "../types/Actions";
import { addExpense, removeExpense } from "../Actions/expenses";

type PathParamsType = {
  id: string;
};
//component own props
type PropsType = RouteComponentProps<PathParamsType> & {
  id: string;
};

interface StateProps {
    expenses: Expense[]
}

interface DispatchProps{
    addExpense : (expense : Expense) => any,
    removeExpense : (expenseId : string) => any
}

type Props = PathParamsType & PropsType & StateProps & DispatchProps;

function GetSelectedExpense(id : string, expenses : Expense[]){
    for (var i=0 ; i<expenses.length; i++){
        if (expenses[i].id === id)
        {
            return expenses[i];
        }
    }
}

const EditExpensePage: React.FunctionComponent<Props> = (props): any => {
    let selectedExpense = GetSelectedExpense(props.match.params.id, props.expenses)
    return (
    <ExpenseForm description={selectedExpense?.description} amount={selectedExpense?.amount} onSubmit={(expense) => {

        for (let i = 0; i< props.expenses.length ; i++){
            if (props.expenses[i] === selectedExpense)
            {
                props.removeExpense(selectedExpense.id);
                props.addExpense(expense);
            }
        }

    }}/>
 )
};

const mapStateToProps = (state : AppState) :StateProps => {
    return({
        expenses : state.expenses
    })
}

const mapDispatchToProps = (dispatch : Redux.Dispatch<AppActions>) : DispatchProps => {
    return {
        addExpense : (expense : Expense) => dispatch(addExpense(expense)),
        removeExpense : (id : string) => dispatch(removeExpense(id))
    }
}

export default compose(
    withRouter,
    connect(mapStateToProps, mapDispatchToProps))(EditExpensePage);
```

I've noticed a few things, which I'm going to elaborate in the following.

Interfaces

First off, your interface Expense.
It looks good so far, what I'd like to see is something like a structure in it, let it be alphabetically sorted or sort it optionals first or both, something like:

export interface Expense {
    amount: number;
    createdAt: number;
    description: string;
    id: string;
    note?: string;
}

It increases readability and allows others to quickly parse all existing keys (it's a bit nitpicking here, but it can have a huge effect).

Next up:

type PathParamsType = {
  id: string;
};
//component own props
type PropsType = RouteComponentProps<PathParamsType> & {
  id: string;
};

interface StateProps {
    expenses: Expense[]
}

interface DispatchProps{
    addExpense : (expense : Expense) => any,
    removeExpense : (expenseId : string) => any
}

type Props = PathParamsType & PropsType & StateProps & DispatchProps;

My two pain points here are, PathParamsType as well as DispatchProps.
PathParamsType is not speaking for it self, try to give variables or interfaces a very consice name. Also I thought, you accidentially declared { id: string; } twice. There is some kind of repetition that could have been avoided. Please consider the following:

interface WithId {
    id: string;
}

/* Maybe name ComponentProps to something more speaking. */
type ComponentProps = RouteComponentProps<WithId> & WithId;

Also, you are declaring a props type below, why didn't you merge all in the first place.
Something along the lines like: type Props = RouteComponentProps<WithId> & WithId /* & so on ... */;.

!!Side note!! Avoid stuff like: type Foo = { bar: string; foo: number; } we have interfaces for that.

Your function typing seems also a bit odd to me.

interface DispatchProps{
    addExpense : (expense : Expense) => any,
    removeExpense : (expenseId : string) => any
}

any seems in appropriate here. If you don't return a value from a function use void. If that function never returns (e.g. you quit the application inside that function), use the keyword never. But please don't use any.

To sum the interface section up, your code from above could have looked a bit like this:

interface WithId {
    id: string;
}

interface StateProps {
    expenses: Expense[];
}

interface DispatchProps{
    addExpense : (expense : Expense) => void;
    removeExpense : (expenseId : string) => void;
}

type Props = RouteComponentProps<WithId> & WithId & StateProps & DispatchProps;

React Component

Let's get to the component itself.

const EditExpensePage: React.FunctionComponent<Props> = (props): any => {
 /* ... */
}

The same as in the DispatchProps interface, a function does not return any. You actually broke the type system here. The following is fully compliant to your function/ variable typing.

const IamBroken: React.FunctionComponent<Props> = (props): any => 5;

This works as IamBroken should be a functional component but the function itself returns any and any is 5 (hopefully that explanation helps, at least a bit). What you would have wanted instead is:

const IamNotBroken = (props): React.FunctionComponent<Props> => 5; /* TypeScript: ERROR!!!! */

To apply the above pattern to your component (two solutions are available):

/* Variant 1 */
const EditExpensePage = (props: Props) => {
 /* ... */
}

/* Variant 2 */
const EditExpensePage: React.FunctionComponent<Props> = (props) => {
 /* ... */
}

Both solutions are totally fine, if you ask me though, I prefer the first one, it's sleek and if needed the compiler is able to infere the React.FunctionComponent part.

Component internals, functional programming

Inside your component I've noticed a few things as well.
let selectedExpense = GetSelectedExpense(props.match.params.id, props.expenses). From what I have seen, that you never re-assign selectedExpense. Therefore it's better to keep such variables as constants: const selectedExpense.

I see what you want to achieve with the GetSelectedExpense function. But there are better way's to achieve this in TypeScript/JavaScript. Have a look at Arrays over at MDN. There are a few helpful methods for overall array handling. What you are looking for is something like find.
(Side Note: There is a powerful concept that is called destructuring, and it helps you to avoid to write props. over and over again).

const { expenses, match } = props; /* Concept of destructuring, see my side note above */
const selectedExpense: Expense | undefined = expenses.find(expense => expense.id === match.params.id);

The onSubmit function could be improved in the following way. First, you want to avoid inline array functions as props (many linters are warning about possible performance issues). While that seems to be true, it also doesn't add to readability as well. Have a look at the following (I'm making use of the useCallback hook):

const { expenses, selectedExpense, removeExpense, addExpense, match } = props;
const selectedExpense: Expense | undefined = expenses.find(expense => expense.id === match.params.id);

const onSubmit = useCallback((expense) => {
    expenses.filter(exp => exp.id === selectedExpense).map(exp => removeExpense(exp.id));
    addExpense(expense);
}, [removeExpense, addExpense, selectedExpense]);

return (<ExpenseForm description={selectedExpense?.description} amount={selectedExpense?.amount} onSubmit={onSubmit}/>);

Last but not least, your compose thingy at the end. It seems wrong and should be like this (because compose is coming from redux which shouldn't be abused to chain higher order components):

export default withRouter(connect(mapStateToProps, mapDispatchToProps)(EditExpensePage));


This perl script renames all files in a directory to have an equal amount of digits I took a lot of pictures with my camera and I wanted to make a time-lapse out of them. The camera saved the pictures as picture1, picture2 ... picture956 etc. but the time-lapse software I'm using only accepts numbers of equal length like this: picture001, picture002, picture003 etc.

I thought Perl would be a good fit for this kind of problem so I gave it a shot. This is a shorter and translated version of the original code so if anything is unclear I can change it to the longer version.

use strict;
use warnings;
use diagnostics;

print "Give the path to the directory where the pictures are stored:\n";
my $filename = <STDIN>;

chomp $filename;
chdir $filename or die "Couldn't change the directory: $!\n";
my @files  = <*>;
#gives a list of all filehandles in the given directory

my $amount_of_digits = int( log($#files)/log(10) +1);
#how many digits should the new number have?
#Example: If there are 400 files -> 3 digits per file: 001, 002 etc

foreach my $file (@files){
    next if($file =~ /^\.$/);
    next if($file =~ /^\.\.$/);
    #skip the . and .. files
    print "$file\n";
    if ($file =~ /(\d+)/){
        my $amount_of_padded_zeroes = $amount_of_digits - length($1);
        if($amount_of_padded_zeroes > 0){
            my $new_number =  '0' x $amount_of_padded_zeroes . $1;
            (my $new_name = $file) =~ s/$1/$new_number/;
            print "Changing name too: $new_name\n";
            rename ($file, $new_name) or die "Couldn't rename the file: $!";
        }
    }
}

print "Program completed. Press any key to continue.\n";
my $einde = <STDIN>;

The code works but I wanted to know if there is a better/cleaner/shorter/more Pearlesque way to do this. I'm learning Perl for uni so any feedback is welcome. I suspect this problem is so trivial in Perl that there might be readable one-liners that are able to replace all of this.

As you suspect, this can be done in fewer lines, but it's not a one-liner.

use strict;
use warnings;
use diagnostics;

Always a good idea.  Consider use warnings FATAL => "all" so that you don't miss any.

my $filename = <STDIN>;

A command-line argument or environment variable is the typical way to do this, in Perl and most other languages.  Directory variables oughtn't be named $filename.

  my $dir = ( shift or $ENV{TIMELAPSE_DIRECTORY} or die "usage: $0 directory\n" );

chdir $filename or die "Couldn't change the directory: $!\n";
…
rename ($file, $new_name) or die "Couldn't rename the file: $!";

The fat-arrow => can replace a comma and improve readability in some cases.  Dropping parens is another good readability boost, when done judiciously.  Unless the line number where die occurred is interesting, suppress it by appending a newline to the message.  And finally, it's a good habit to include in error messages the data that produced them, as in:

 rename $file => $new_name or die "Couldn't rename '$file' to '$new_name': $!\n";

my @files  = <*>;
#gives a list of all filehandles in the given directory

Filenames, not filehandles.  It would be reasonable to filter this list to contain actual files, excluding directories:

my @files  = grep -f, <*>;

And maybe even by name:

my @files = grep { -f and /\.( png | jpe?g | tga )$/xi } <*>;

my $amount_of_digits = int( log($#files)/log(10) +1);

$#files is the largest index in that zero-based array.  With 10 files, the last is $files[9] and the math returns 1 instead of the 2 we need.  The size of the array is one bigger and retrieved as @files in scalar context (log() imposes scalar context for us, which is convenient).

width is a good name for this variable.

Avoid an uncaught exception by checking that @files is non-empty.

And the parens around int can be omitted.

die "nothing to do!\n" unless @files;
my $width = int log(@files)/log(10) + 1;

But this is still no good!  log(1000)/log(10) is 3 in Perl and in real life.  But int( log(1000)/log(10) ) is 2!   This happens because floating-point math is imperfect.  Luckily Perl will let us cheat by taking the length of a number that's been silently converted  to a string:

my $width = length scalar @files;

foreach my $file (@files){

for is the idiomatic alternative to foreach.

    if ($file =~ /(\d+)/){
        my $amount_of_padded_zeroes = $amount_of_digits - length($1);
        if($amount_of_padded_zeroes > 0){
            my $new_number =  '0' x $amount_of_padded_zeroes . $1;
            (my $new_name = $file) =~ s/$1/$new_number/;
            print "Changing name too: $new_name\n";

sprintf is the function to use for leading zeroes.  It can go right in the s/// replacement by using the /execute modifier.  If the replace fails, skip to next file.

             (my $new_name = $file) =~ s/(\d+)/ sprintf "%0${width}d" => $1 /e or next;

rename ($file, $new_name) or die "Couldn't rename the file: $!";

It's good practice to ensure you have something to do, and that you aren't overwriting an existing file here:

next if $new_name eq $file;
die "$new_name (from $file) already exists!\n" if -f $new_name;

print "Program completed. Press any key to continue.\n";
my $einde = <STDIN>;

I think you know how I feel about this.  Should you decide to keep it, only the Enter key will actually proceed.

Putting it all together:

use strict;
use warnings FATAL => 'all';
use diagnostics;

my $dir = ( shift or $ENV{TIMELAPSE_DIRECTORY} or die "usage: $0 directory\n" );
chdir $dir or die "Couldn't change directory to $dir: $!\n";
my @files = grep { -f and /\.( png | jpe?g | tga )$/xi } <*>;
die "nothing to do!\n" unless @files;
my $width = length scalar @files;

for my  $file (@files) {
    (my $new_name = $file) =~ s/(\d+)/ sprintf "%0${width}d" => $1 /e or next;
    next if $new_name eq $file;
    die "$new_name (from $file) already exists!\n" if -f $new_name;
    rename $file => $new_name or die "Couldn't rename '$file' to '$new_name': $!\n";
}
print "Program completed.\n"

Reader exercise: improve this program to number the files from 1 to n, even if the original numbers don't start at 1 or have gaps!


Generating unique ID of an exception For classifying error reports, I need to generate an ID from an exception. That ID should be

(more or less) unique
a short alpha numeric string
equal for equivalent exceptions (even on different systems)

The following code seems to do the job:

    public static string GetExceptionID(this Exception ex)
    {
        if (ex == null) return "0000";

        int hash = 31;
        var innerException = ex;
        while (innerException != null)
        {
            hash ^= GetHash(innerException.GetType().ToString());

            hash ^= innerException.StackTrace
                                  .Split(new[] {Environment.NewLine}, StringSplitOptions.RemoveEmptyEntries)
                                  // If *.pdb file is provided, the file and line no. (e.g. "... in C:\Path...") is part of the stacktrace.
                                  // The next line cutting off that part
                                  .Select(line => line.Substring(0, line.IndexOf(" in ") < 0 ? line.Length : line.IndexOf(" in ")))
                                  .Aggregate(31, (currentHash, val) => currentHash ^= val.GetHash());

            innerException = innerException.InnerException;
        }

        return Convert.ToBase64String(BitConverter.GetBytes(hash))
            .Replace('+', 'P')
            .Replace('-', 'M')
            .Replace("=", "")
            .ToUpper();
    }

    // https://en.wikipedia.org/wiki/Jenkins_hash_function
    private static int GetHash(this string str)
    {
        if (str == null) return 1;

        int hash = 31;
        foreach (char c in str)
        {
            unchecked
            {
                hash += c;
                hash += hash << 10;
                hash ^= hash >> 6;
            }
        }

        hash += hash << 3;
        hash ^= hash >> 11;
        hash += hash << 15;

        return hash;
    }

Do you see any problems with the code?

Stack trace

I think you shouldn't work with the stack string but retrieve the stack and go through each frame yourself. I find it would make the analysis more reliable and you could get the information you need without extracting it from the string because you would have direct access to each part.

private static IEnumerable<dynamic> GetStackTrace(this Exception exception)
{
    var stackTrace = new StackTrace(exception, true);
    var stackFrames = stackTrace.GetFrames();
    var result = stackFrames?.Select(sf => new
    {
        Caller = (sf.GetMethod() as MethodInfo)?.ToShortString() ?? string.Empty,
        FileName = Path.GetFileName(sf.GetFileName()),
        LineNumber = sf.GetFileLineNumber(),
    });
}

This snippet comes from my Exception prettyfier.

Other suggestions

It would be a good idea to split this method into three or four methods.

the first one gets you the information you need from the stack - you could implement it either based on the string or on the raw stack trace
the second one would encode the data as Base64String
the third and last one would clean-up or encode the string generated by the second one
the fourth could be a helper encapsulating the three other methods

Example:

var ex.ToExceptionId()

where

public static string ToExceptionId(this Exception ex)
{
    return ex.GetStackTrace().ToBase64String().EncodeBase64String();
}

Now you can test and maintain each part of the exception id creation separately.


Input group with flex box I'm developing a Chrome extension so I can use bleeding-edge technologies like flexbox:

jsfiddle

HTML:

Classic search example

    <input type="text" id="example-search" />
    <button>Search</button>

CSS (cleared to highlight flexbox):

.input-wrapper {
    display: flex;
    flex-flow: row nowrap;
}

.input-wrapper > input {
    flex: 1 1 auto;
}

.input-wrapper > button {
    flex-shrink: 0;
}

I have two questions:

Is this the right way to work with flexbox?
Is there is a way to gracefully degrade this to use in websites?

Using flexbox for production:

I recently found a great article about my issue: flexbox in the real world

Raw insigsts from atricle:

use autoprefixer to support all old desktop and mobile browsers (they use old flexbox model syntax)
use progressive enhancement for IE8- (see Scenario 3 from article)

Unless you're overwriting properties set elsewhere, there's no reason for this line because this is the default for all flex containers:

flex-flow: row nowrap;

If your goal is to make this work on browsers with either of the old Flexbox implementations, there are 2 things to be aware of:

The March 2012 draft (IE10) does not have individual properties for flex-grow, flex-shrink, or flex-basis.  The only way to control them is via the flex shorthand.
The original draft does not allow you to have differing flex-shrink and flex-grow values.  It would be ok to translate flex: 1 1 auto to box-flex: 1, but you can't translate something like flex: 1 0 (with prefixes).  For your purpose, box-flex: 0 should work (though you might have problems with buttons in old Webkit browsers, see: https://stackoverflow.com/questions/16961192/flexbox-doesnt-work-with-buttons).


Program to find dollar words A dollar word is a word for which the sum of the values of the letters adds up to 100 ($1.00).

"a" has a value of 1 and "z" has a value of 26.  Special characters such as apostrophes are ignored.

First try looked like:

import string

valMap = {}
for index,item in enumerate(string.lowercase):
    valMap[item] = index +1

def isDollarWord(word):
    lowercase = word.lower().strip()
    total = 0
    for letter in lowercase:
        if letter in valMap:
            total += valMap[letter]
    return total == 100

words = open("C:\Users\astroboy\Downloads\UKACD17.TXT")

for line in words:
    if isDollarWord(line):
        print(line)

I started feeling kind of bad that words like "Hälleflinta" and "divorcée" might be denied their rightful place in the dollar word list.  No known rules for how to handle characters  with accents, so I made up my own as below (à would be counted as a).  That means replacing all diactritics with the plain letter.

import string
import unicodedata

valMap = {}
for index,item in enumerate(string.lowercase):
    valMap[item] = index +1

def remove_marks(word):
    unicode_word = word.decode('cp1252')
    return unicodedata.normalize('NFKD',unicode_word).encode('ascii','ignore')

def isDollarWord(word):
    lowercase = word.lower().strip()
    normalized = remove_marks(lowercase)

    total = 0
    for n in normalized:
        if n in valMap:
            total += valMap[n]

    return total == 100

words = open("C:\Users\astroboy\Downloads\UKACD17.TXT")

for line in words:
    if isDollarWord(line):
        print(remove_marks(line))

I'm posting for any feedback about this code.  See any room for improvement?

Strategic themes

Prefer comprehensions to loops: List comprehensions and dict comprehensions let you compress loops into one-liners.  It's also a nice feeling to initialize an object "all at once" rather than building it little by little.
Avoid special cases: if n in valMap is annoying.  If valMap were a defaultdict, then a failed lookup would naturally have a value of 0.
Use built-in functions: To compute a sum, use sum().  Converting the valMap to a defaultdict enables this further simplification.
Obey the single-responsibility principle: The isDollarWord() function does too much, and should be split up.  A word_value() function would be more useful than isDollarWord() — at the least, it allows for more interesting unit tests.  Once word_value() has been defined, comparison with 100 is trivial.

Localized issues

valMap is not descriptive enough for my taste.  I suggest LETTER_VALUES, in all caps to identify it as a constant.
isDollarWord() should be named is_dollar_word().  (You named remove_marks() correctly, though.)

File handling

You have a file descriptor leak.  Opening files using a with block is almost always preferable to a regular open() call.
Better yet, avoid hard-coding filenames, and let the input be specified on the command line or through standard input.  fileinput.input() is useful for this.

Internationalization and Python 3 compatibility

string.lowercase is locale-dependent, which, according to your specification, you don't want.  Furthermore, it has been removed in Python 3.  You want string.ascii_lowercase instead.
If you want to interpret the input as CP1252, specify an encoding when opening the file, so that it is decoded correctly even before your application even has a chance to get to the data.  If using fileinput.input(), use an openhook parameter (but beware of a bug in Python 2).
For Python 3 compatibility, remove_marks() should also call .decode('ascii') on its return value, to convert the byte string back into a text string.
Alternate ways to handle the internationalization problem include transliteration using transliterate or Unidecode.  German convention, for example, treats ö as oe and ß as ss.

Suggested implementation

from collections import defaultdict
import fileinput
from string import ascii_lowercase
from unicodedata import normalize

LETTER_VALUES = defaultdict(int,
    ((letter, index + 1) for index, letter in enumerate(ascii_lowercase))
)

def remove_marks(word):
    return normalize('NFKD', word).encode('ascii', 'ignore').decode('ascii')

def word_value(word):
    return sum(LETTER_VALUES[c] for c in remove_marks(word.lower()))

for line in fileinput.input(openhook=fileinput.hook_encoded('cp1252')):
    if word_value(line) == 100:
        print(line.strip())


Finding the number of words from a user input string or a text file I'm new to Java and is trying to solve the beginners problem of finding out the number of words in an user input string or a text file. I was just wondering if there are any alternatives to any of the steps that can improve efficiency as well as simplicity.

import java.io.BufferedReader;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.util.Scanner;

public class WordsCount{
    public static void main(String[] args) {

        try(Scanner sc1 = new Scanner(System.in)){
            String userInputOrTextFile = sc1.next();
            if (userInputOrTextFile.equalsIgnoreCase("userInput")){
                WordsCount.countUserInput();
            } else {
                WordsCount.countTextFile();
            }
        }
    }

    private static void countUserInput() {
        try(Scanner sc1 = new Scanner(System.in)){
            String s1 = sc1.nextLine();
            System.out.println(s1.split(" ").length + " words in the user input sentence." );
        }
    }

    private static void countTextFile() {
        int countingWords = 0;
        try(Scanner sc1 = new Scanner(new BufferedReader(new FileReader("xanadu.txt")))){
            while(sc1.hasNext()){
                sc1.next();
                countingWords++;
            }
        } catch (FileNotFoundException e){
            System.out.println("File not found");
        }
        System.out.println(countingWords + " words are in the xanadu.txt file");
    }
}

On Regex use
To correctly split on white space and count the amount of words you want, use trim().split('\\s+').length
You want to split on all whitespace, not just a single ones in between, and you need to trim first because otherwise you'll also get inaccurate results for any input beginning with spaces.
This will also work just as well for your file input -- not too sure why you use a different method.


Textual representation of a C++ function I need to create an instance of Show of a somewhat complicated type that represents a C++ function. The "serialization" gives me a Haskell interpretation of the said function.

It works as I wish but it looks utterly horrible, with the string concatenations, point-free conversions and nested let bindings.  How could I improve readability of the code?

instance Show Function where
    show (Function templateArgs returnT fname fArgs) =
        let toLowerIfPolymorphic argName =
               let lcArg = strToLower argName in
                    if lcArg `elem` (map snd templateArgs)
                    then lcArg
                    else id argName in
        concat [
            fname, " :: (",
            intercalate ", " (
                filter (/="") . map (
                \(concept, tIdent) ->
                    if not $ elem concept ["Any", "typename", "class"]
                    then concept ++ " " ++ strToLower tIdent
                    else ""
                ) $ templateArgs
            ), ") => ",
            intercalate " -> " (
                map toLowerIfPolymorphic . map (show . fst) $ fArgs
            ),
            (if not $ null fArgs then " -> " else ""),
            toLowerIfPolymorphic . show $ returnT
        ]

The Function is:

data Function = Function {
                    funcTemplateArgs :: [(Concept, Identifier)],
                    funcReturnT      :: CppType,
                    funcName         :: String,
                    funcArgs         :: [(CppType, Identifier)]
                }

CppType is basically a sum-type representing Scalar and Parameterzied types. Just synonyms for tuples. Concept / Identifier is a String synonym. Everything is an instance of Show.

Also I find the if condition then f else id very strange. "Do nothing if false" is somewhat silly.

I would clean this up by bringing what you can to the top level (hiding it using a module export list if need be) as new functions. There are also some sectioning/alias changes that I think would make it more readable.

toLowerIfPolymorphic :: [(Concept, Identifier)] -> Identifier -> Identifier
toLowerIfPolymorphic templateArgs argName
    | lcArg `elem` (snd $ unzip templateArgs) = lcArg
    | otherwise                               = argName
  where
    lcArg = strToLower argName

I often prefer guards to if _ then _ else _, it helps cut down on word soup (and I still don't understand why we don't have boolCase :: Bool -> a -> a -> a in the Prelude). Consider how snd . unzip more specifically captures your intent than using a more powerful but generic higher-order function like map.

The bit with ["Any", "typename", "class"] should work out better with some reordering. Instead of applying a partial transformation with a sentinel value (i.e. filter (not . null) with occasional "" values), drop the junk and then transform the good stuff.

showConstraints :: [(Concept, Identifier)] -> [String]
showConstraints templateArgs = map display . filter valid $ templateArgs
  where
    display (concept, ident) = concept ++ " " ++ strToLower ident
    valid (concept, _) = concept `notElem` ["Any", "typename", "class"]

Then your Show instance ends up looking something like this.

instance Show Function where
  show (Function templateArgs returnT fName fArgs)
     = fName
    ++ " :: ("
    ++ intercalate ", " (showConstraints templateArgs)
    ++ ") => "
    ++ intercalate " -> " (map (toLowerIfPolymorphic templateArgs . show . fst) $ fArgs)
    ++ (if null fArgs then "" else " -> ")
    ++ toLowerIfPolymorphic templateArgs (show returnT)

I'd probably take this one step further though to handle not having any constraints (in which case you don't want "() =>" to show up, presumably) and write one more helper function.

wrap :: String -> String -> String -> String
wrap _     ""      _   = ""
wrap start content end = start ++ content ++ end

Giving—

instance Show Function where
  show (Function templateArgs returnT fName fArgs)
      =  fName
      ++ " :: "
      ++ wrap "(" (intercalate ", "   $ showConstraints templateArgs)       ") => "
      ++ wrap ""  (intercalate " -> " $ map (lowerPoly . show . fst) fArgs) " -> "
      ++ lowerPoly (show returnT)
    where
      lowerPoly = toLowerIfPolymorphic templateArgs


SQL connection in static class and wrapper I need to make some calls to stored procedures in my SQL database from a .NET application, using C#.

I have something like this, and I put just one of the methods but the rest will be similar:

public static class ClientDataAccess
{
    public static void AddClient(Client client)
    {
        var connectionString = ConfigurationManager.ConnectionStrings["appDatabase"].ConnectionString;

        using (var connection = new SqlConnection(connectionString))
        using (SqlCommand cmd = new SqlCommand("Client_add", connection) { CommandType = System.Data.CommandType.StoredProcedure })

            // Add parameters

            connection.Open();
            command.ExecuteNonQuery();
        }
    }
}
}

When I needed to test the classes where it's used I had to create a wrapper to be able to mock it:

public class ClientDataAccessWrapper : IClientDataAccessWrapper
{
    public void AddClient(Client client)
    {
        ClientDataAccess.AddClient(client);
    }
}

Is this considered to be a good practice?

The class being static is not the only thing that makes testing more difficult then necessary.

var connectionString = ConfigurationManager.ConnectionStrings["appDatabase"].ConnectionString;

The hardcoded connection string name should be a parameter. You can have default constructor using a default connection string name but it's always a good idea to have another constructor that lets you use another value.

AddClient

If it's a ClientDatabase for CRUD about client objects then I find the client in name AddClient method redundant. Simply Add, Delete etc would be more appropriate.

By following this pattern you can have a single crud-generic interface for all types:

public interface IDataAccess<T>
{
    void Add(T obj);
    void Delete(T obj);
    ..
}

and later

public class ClientDataAccess : IDataAccess<Client>
{
    public void Add(Client obj) {..}
    public void Delete(Client obj) {..}
}


Comparison table for comparing products I'm attempting to create a comparison table to compare products depending on the add-ons they have. To grab the data and filter it I'm using LINQ To SQL.

Table's Layout (cut short):

Products Table
ID
Name
CategoryID
ProductAddons

Category Table
ID
Name

ProductAddon Table
ID
Amount
AddonID
ProductID

Addon Table
ID
Name

Example Data:

Products

ID      Name      CategoryID
1       Product1  1
2       Product2  1
3       Product3  1

Categories

ID      Name
1       Category1

ProductAddons

ID      Amount      AddonID      ProductID
1       1           1            1
2       2           2            1
3       1           3            1
4       2           1            2
5       3           2            2
6       1           2            3
7       1           3            3

Addons

ID      Name
1       Addon1
2       Addon2
3       Addon3

Currently I have this:

var addons = (from s in Products
              where s.Category.Name == "Category1"
              orderby s.ProductAddons.Count descending
              let adds = from a in s.ProductAddons
                         orderby a.Addon.Name
                         select new { Name = a.Addon.Name, Amount = a.Amount }
              select adds).ToList();

var compare = from c in addons
              let has = from z in addons.First().Union(addons.First().Except(c))
                      let add = (from a in c
                                 where a.Name == z.Name
                                 select a.Amount).FirstOrDefault()
                      select new { Name = z.Name, Amount = add }
              select has;

var compileResults = from c in addons.First()
                     let adds = from s in compare
                             let y = (from a in s
                                      where a.Name.Contains(c.Name)
                                      select a.Amount).First()
                             select y
                     select new { Addon = c.Name, Amounts = adds };

This would return (3 products, 3 Add-ons):

Addon          Amounts

Addon1         1
               2
               0

Addon2         2
               3
               1

Addon3         1
               0
               1

I can then loop through the results to build my compare table, which works fine. But to me the LINQ looks somewhat messy. Would there be a cleaner way to do this?

So it looks like what you want is some sort of outer join.  I think this should work for you:

var categoryFilter = "Category1";
var query =
    from addon in dc.Addons
    select new
    {
        Addon = addon.Name,
        Amounts =
            from product in dc.Products
            where product.Category.Name == categoryFilter
            join productAddon in dc.ProductAddons
                on new { AddonId = addon.Id, ProductId = product.Id }
                equals new { productAddon.AddonId, productAddon.ProductId }
                into pas
            from productAddon in productAddons.DefaultIfEmpty()
            select (int?)productAddon.Amount ?? 0,
    };


Security check using an SQL call In our latest Veracode scan for an application, I have come across the issue of Improper Resource Shutdown or Release.  It is pointing at a function.  Here's  what the code looks like:

Imports System.Data.SqlClient
Public Class DAL
    Public Shared ConnString As String = ConfigurationManager.ConnectionStrings("connection").ConnectionString

Public Shared Function CheckSecurity(ByVal strUserID As String, ByVal strOperation As String, ByVal strAppID As String) As Boolean
    Dim sbSQL As New StringBuilder
    Dim MyConnection As SqlConnection = New SqlConnection()
    Dim sqlCmd As SqlCommand = New SqlCommand

    MyConnection.ConnectionString = ConnString

    sbSQL.Clear()

    sbSQL.AppendLine("EXEC dbo.CheckSecurity @UserID, @AppID, @Operation")

    sqlCmd.CommandText = sbSQL.ToString
    sqlCmd.Connection = MyConnection

    With sqlCmd.Parameters
        .Clear()
        .Add("@UserID", SqlDbType.VarChar, 15).Value = strUserID
        .Add("@AppID", SqlDbType.VarChar, 50).Value = strAppID
        .Add("@Operation", SqlDbType.VarChar, 50).Value = strOperation
    End With

    Try
        If getDataTableFromSqlCmd(sqlCmd).Rows.Count > 0 Then
            CheckSecurity = True
        Else
            CheckSecurity = False
        End If

    Catch ex As Exception
        Throw New ApplicationException("SECURITY ACCESS ERROR")
    Finally
        If MyConnection.State = ConnectionState.Open Then
            MyConnection.Close()
        End If

        MyConnection.Dispose()
        sqlCmd.Dispose()

    End Try
End Function

Code for getDataTableFromSqlCMD:

Public Shared Function getDataTableFromSqlCmd(ByVal sqlCmd As SqlCommand) As DataTable
    Dim dt As New DataTable
    Dim MyAdapter As New SqlDataAdapter(sqlCmd)

    Try
        sqlCmd.CommandTimeout = m_iSQLTimeOut
        MyAdapter.Fill(dt)
        getDataTableFromSqlCmd = dt

    Catch ex As Exception
        Throw New ApplicationException("GET DATA TABLE ERROR")
    Finally
        sqlCmd.Dispose()
        MyAdapter.Dispose()
        dt.Dispose()
    End Try
End Function

As far as I can tell the resources in this code are being properly deallocated.  Am I missing something?

It needs more Using statements. For example:

Public Shared Function CheckSecurity(strUserID$, strOperation$, strAppID$) As Boolean
    Try
        Using da As New SqlDataAdapter("dbo.CheckSecurity", ConnString)
            Dim sc = da.SelectCommand, p = sc.Parameters, dt = New DataTable
            sc.CommandType = CommandType.StoredProcedure
            sc.CommandTimeout = m_iSQLTimeOut

            p.Add("@UserID", SqlDbType.VarChar, 15).Value = strUserID
            p.Add("@AppID", SqlDbType.VarChar, 50).Value = strAppID
            p.Add("@Operation", SqlDbType.VarChar, 50).Value = strOperation

            Return da.Fill(dt) > 0     ' .Fill returns the number of rows successfully added
        End Using           ' da is disposed here even if Exception occurs
    Catch ex As Exception
        Throw New ApplicationException("SECURITY ACCESS ERROR")
    End Try
    Return False
End Function

or

Public Shared Function CheckSecurity(strUserID$, strOperation$, strAppID$) As Boolean
    Try
        Using con = New SqlConnection(ConnString),
              cmd = New SqlCommand("dbo.CheckSecurity", con)

            cmd.CommandType = CommandType.StoredProcedure
            cmd.CommandTimeout = m_iSQLTimeOut

            cmd.Parameters.Add("@UserID", SqlDbType.VarChar, 15).Value = strUserID
            cmd.Parameters.Add("@AppID", SqlDbType.VarChar, 50).Value = strAppID
            cmd.Parameters.Add("@Operation", SqlDbType.VarChar, 50).Value = strOperation

            con.Open()
            Using reader = cmd.ExecuteReader
                Return reader.HasRows
            End Using
        End Using       ' con and cmd are closed and disposed here even if Exception occurs
    Catch ex As Exception
        Throw New ApplicationException("SECURITY ACCESS ERROR")
    End Try
    Return False
End Function

Some other examples https://stackoverflow.com/questions/24023575/how-to-pass-parameters-to-sqldataadapter, https://stackoverflow.com/questions/14566980/c-sharp-data-adapter-parameters


Project Euler # 48 Self powers in Python The series, \$1^1 + 2^2 + 3^3 + \ldots + 10^{10} = 10405071317\$.
Find the last ten digits of the series, \$1^1 + 2^2 + 3^3 + \ldots + 1000^{1000}\$.

def self_power(n):
    """returns sum of self powers up to n."""
    total = 0
    for i in range(1, n + 1):
        total += i ** i
    return str(total)[-10:]

if __name__ == '__main__':
    print(self_power(1000))

You can compute the sum of the powers with sum() and a generator expression:
total = sum(i ** i for i in range(1, n + 1))

instead of a for-loop. Retrieving the last 10 digits can be done with the modulus operator % instead of converting it to a string:
return total % (10 ** 10)

Your code already runs in fractions of a second. For even larger exponents it would be more efficient to compute only the last 10 digits of each intermediate result. This can be done with the three-argument form of pow():

pow(x, y[, z])
Return x to the power y; if z is present, return x to the power y, modulo z (computed more efficiently than pow(x, y) % z).

Putting it together:
def self_power(n):
    """returns sum of self powers up to n."""
    MOD = 10 ** 10
    total = sum(pow(i, i, MOD) for i in range(1, n + 1))
    return total % MOD

This runs a tiny bit faster:

$ # Your version:
$ python3 -m timeit 'import euler48; euler48.self_power0(1000)'
50 loops, best of 5: 9.55 msec per loop

$ # Improved version:
$ python3 -m timeit 'import euler48; euler48.self_power(1000)'
100 loops, best of 5: 2.19 msec per loop

But it makes a big difference if the numbers become larger:

$ # Your version:
$ python3 -m timeit 'import euler48; euler48.self_power0(10000)'
1 loop, best of 5: 6.65 sec per loop

$ # Improved version:
$ python3 -m timeit 'import euler48; euler48.self_power(10000)'
10 loops, best of 5: 28.6 msec per loop


Perlin Noise Generator I'm porting a 2-D Terrain Generator from Python to C++ as Python is too slow due to the number of features that have been added.

I'm new to C++ and would like to know of any criticisms that you can make of the code at this time before I continue with the simpler parts of the generation.  Right now I'm interested in maintaining my use of arrays rather than std::vector so any improvements besides that would be greatly appreciated.

I'm also aware that this code is currently uncommented, I intend to fix that in the future however right now I'm more interested in the code itself.

#include "stdafx.h"
#include <iostream>
#include <conio.h>

double** white_noise(int w, int h);
double** smooth_noise(double** noise, int w, int h, int octave);
double** perlin_noise(double** noise, int w, int h, int octaves, double persistance);
double interpolate(double a, double b, double alpha);

int main()
{
    const int w = 400;
    const int h = 400;

    double** noise = white_noise(w, h);
    double** perlinnoise = perlin_noise(noise, w, h, 8, 0.7);

    // Code that does things will go here(function calls to add sea-level etc.)

    for (int i = 0; i < w; i++) {
        delete[] noise[i];
        delete[] perlinnoise[i];
    }
    delete[] noise;
    delete[] perlinnoise;
}

double** white_noise(int w, int h) {
    double** noise = new double*[w];
    for (int i = 0; i < w; i++) {
        noise[i] = new double[h];
    }

    for (int i = 0; i < w; i++) {
        for (int j = 0; j < h; j++) {
            noise[i][j] = (float)rand() / RAND_MAX;
        }
    }

    return noise;
}

double** smooth_noise(double** noise, int w, int h, int octave) {
    double** smooth_noise = new double*[w];
    for (int i = 0; i < w; i++) {
        smooth_noise[i] = new double[h];
    }

    int period = 1 << octave;
    double frequency = 1.0 / period;

    for (int i = 0; i < w; i++) {
        int sample_i0 = i / period * period;
        int sample_i1 = (sample_i0 + period) % w;
        double horizontal_blend = (i - sample_i0) * frequency;

        for (int j = 0; j < h; j++) {
            int sample_j0 = j / period * period;
            int sample_j1 = (sample_j0 + period) % h;
            double vertical_blend = (j - sample_j0) * frequency;

            double top = interpolate(noise[sample_i0][sample_j0],
                noise[sample_i1][sample_j0],
                horizontal_blend);
            double bottom = interpolate(noise[sample_i0][sample_j1],
                noise[sample_i1][sample_j1],
                horizontal_blend);
            smooth_noise[i][j] = interpolate(top, bottom, vertical_blend);
        }
    }

    return smooth_noise;
}

double** perlin_noise(double** noise, int w, int h, int octaves, double persistance) {
    double** perlin_noise = new double*[w];
    for (int i = 0; i < w; i++) {
        perlin_noise[i] = new double[h];
    }
    for (int i = 0; i < w; i++) {
        for (int j = 0; j < h; j++) {
            perlin_noise[i][j] = 0;
        }
    }

    double*** smooth_noises = new double**[octaves];
    for (int i = 0; i < octaves; i++) {
        smooth_noises[i] = smooth_noise(noise, w, h, i);
    }

    double amplitude = 1;
    double totalamplitude = 0;

    for (int octave = octaves - 1; octave > 0; octave--) {
        amplitude *= persistance;
        totalamplitude += amplitude;

        for (int i = 0; i < w; i++) {
            for (int j = 0; j < h; j++) {
                perlin_noise[i][j] += smooth_noises[octave][i][j] * amplitude;
            }
            delete[] smooth_noises[octave][i];
        }
        delete[] smooth_noises[octave];
    }
    delete[] smooth_noises;

    for (int i = 0; i < w; i++) {
        for (int j = 0; j < h; j++) {
            perlin_noise[i][j] /= totalamplitude;
        }
    }

    return perlin_noise;
}

double interpolate(double a, double b, double alpha) {
    return a * (1 - alpha) + alpha * b;
}

Here are some things that may help you improve your code.

Strive for portable code

Several of the features of this code are either platform-specific or compiler-specific or both.  Specifically, #include <conio.h>, and #include "stdafx.h" both non-standard.  Further, neither are actually used so I would strongly recommend omitting them.

Prefer to avoid using new and delete directly

In modern C++, it's usually better to avoid explicit calls to new and delete.  In particular, it seems that the goal of this program is to create the perlinnoise matrix.  It is not clear whether you're intending to also use the noise array or if it's just a byproduct of the production of the perlinnoise array.   In any case, use of raw new and delete tends to lead to the problem mentioned in the next suggestion.

Consider using a better random number generator

You are currently using

noise[i][j] = (float)rand() / RAND_MAX;

However, that's the old C way to do things.  better solution, if your compiler and library supports it, would be to use the C++11 `std::uniform_real_distribution.  It looks complex, but it's actually pretty easy to use, as I'll demonstrate below.

Don't leak memory

If you use new and delete directly, you must take care to make sure they match in every instance.  This is quite easy to get wrong, and in fact, this code gets it wrong.  In particular, the perlin_noise routine leaks memory.  Track how long it takes you to find and fix the problem, and then compare it to the following suggestion.

Use objects

C++ is an object-oriented language and accomodating and encouraging that style of design is one of the key benefits of using C++.  For that reason, and because it makes your program cleaner, easier to maintain, easier to re-use and less prone to error, I'd highly recommend using objects for this program.

Use the Standard Template Library

There is a lot of functionality already available to you if you use the Standard Template Library (STL).  To illustrate this and a number of the previous points, here is a rewrite of your white_noise routine as an object.  First, we need a way to initialize it with random values, and then a way to address individual noise values by index.  This is easily done by deriving a WhiteNoise object from a std::valarray.

struct WhiteNoise : public std::valarray<double>
{
    WhiteNoise(int w, int h) :
        std::valarray<double>(w*h),
        width{w},
        height{h}
    {
        std::mt19937 gen;
        std::uniform_real_distribution<double> dis;
        for (auto &value : *this) {
            value = dis(gen);
        }
    }
    int width;
    int height;
};

Note that there is no explicit call to new or delete and that the width and height are embedded into the object rather than separate quantities.  If you create a similar PerlinNoise object, the main function could look like this:

int main()
{
    const int w = 400;
    const int h = 400;

    WhiteNoise wn(w,h);
    PerlinNoise pn(wn, 8, 0.7);

    // Create a bitmap just so that we have something to look at.
    makePGM(pn, "perlin.pgm");
}

The result: no memory leak and a much shorter, cleaner program (also faster in my testing, but you should test it on your machine).  The makePGM is just a function I wrote to create a bitmap image to visualize the result.


Image labeling: it's Monday again This command line program applies defined labels along with Monday's date to a directory of images.

The script has to handle not being run on a Monday. Others are asking for the script so I cleaned it up as much as I could. I want to make it as friendly and as cross-platform as possible.

Labels should be applied just above where the white border region ends. I couldn't guarantee that the border region would always be white, nor how big the border region could be.

Any tips on making it shorter, more elegant, more user-friendly or more resilient would be helpful. I'm considering moving the conditions and text of the actual labels to a separate text file for more flexibility.

import os
import datetime
from datetime import timedelta
import argparse
import glob
import PIL
from PIL import Image, ImageFont, ImageDraw

path_sep = os.path.sep

def get_date_string(optionalArg):
    #return date of most recent Monday
    # or optional date param given by user.
    if optionalArg is not None:
        return optionalArg
    else:
        today = datetime.datetime.today()
        if today.weekday() == 0:
            t = today
        else:
            t = today - timedelta(days=today.weekday())
        return t.strftime("%m/%d/%Y")

def significant_difference(v, previous_v):
    return abs(sum(v) - sum(previous_v)) > 200

def find_v_border(png):
    h = png.height
    png_ims = png.load()
    x = png.width/2
    last_v = png_ims[x,h-1]
    # walk up image from bottom, one pixel at a time.  when color changes, identify that as the place to write text:
    for y in range(h-1,0,-1):
        v = png_ims[x,y]
        if significant_difference(v,last_v):
            return y
        last_v = v

def find_left_border(png,bottom_border):
    png_ims = png.load()
    last_v = png_ims[1,bottom_border]
    for x in range(1,png.width/2):
        v = png_ims[x,bottom_border]
        # when big difference between v and last v, then return x
        if significant_difference(v,last_v):
            return x
        last_v = v

def draw_on_image(raw_img, new_img,date_text):
    global path_sep
    font_size = 16
    png = Image.open(raw_img)
    offset = font_size + 3
    bottom_border = find_v_border(png) - offset
    right_border = find_left_border(png,bottom_border) + 3
    fg="#000000"
    ttfpath = os.path.dirname(PIL.__file__)

    fnt = PIL.ImageFont.truetype(ttfpath + path_sep +'OpenSans-Bold.ttf',font_size)
    draw = PIL.ImageDraw.Draw(png)
    draw.text((right_border,bottom_border), date_text,font=fnt, fill=fg)
    del draw
    png.save(new_img, "PNG",quality=100)

# accept arguments for image file directory and optional date string
parser = argparse.ArgumentParser(description='Process given image.')
parser.add_argument('-p', help='path in which to find images to mark up...', required=True)
parser.add_argument('--d', help='optional to add date you want displayed')
args = parser.parse_args()
image_path = args.p

#path references start from current directory
image_path = os.path.abspath(os.curdir) + path_sep + image_path

image_date = get_date_string(args.d)

    # read all the png files in image_path:

all_files = glob.glob(image_path + path_sep +'*.png')
for file_name in all_files:
    #parse file name to determine text ("Server, Desktop, or All")
    if "server" in str.lower(file_name):
        text = " Server Systems"
        new_file_name =  "labelled_server.png"
    elif "desktop" in str.lower(file_name):
        text = " Desktop Desktop"
        new_file_name = "labelled_Desktop.png"
    else:
        text = " All Systems"
        new_file_name = "labelled_all.png"

    img_text = image_date + text
    new_file_name = image_path + path_sep +new_file_name

    draw_on_image(os.path.join(image_path,file_name),new_file_name,img_text)

print("{0} files labelled with {1}".format(len(all_files),image_date))

Top level docstring

It is good practice to start a file with a string delimited by triple quotes that explains the general purpose of your file.

Python treats it specially and displays it when queried for help(your_module) and saves it in the __doc__ special variable.

Other people will get to know the general purpose of the code without reading it all.

A very basic start would be:

"""
This script adds time-stamps to images,
with particular regards to Mondays.
"""

Confusing parameter name

You write def get_date_string(optionalArg): where optionalArg is not actually optional... I am forced to provide it, an optional argument is one like def foo(a=1):

When names contrast the code I become very confused, also I do not think that kind of returning if not None is so useful, but I am not sure...

Actually you only call the function from:

image_date = get_date_string(args.d)

So you can remove the argument and the None check and call it like:

image_date = args.d or get_date_string()

Or, the equivalent:

image_date = args.d if args.d is not None else get_date_string()

Reduce nesting

else:
    today = datetime.datetime.today()
    if today.weekday() == 0:
        t = today
    else:
        t = today - timedelta(days=today.weekday())
    return t.strftime("%m/%d/%Y")

Becomes:

today = datetime.datetime.today()
if today.weekday() == 0:
    t = today
else:
    t = today - timedelta(days=today.weekday())
return t.strftime("%m/%d/%Y")

Because you already returned, and when you return the function ends, so you can reduce nesting and make it clearer that the None check is just a precondition.

Inline ternary

I like ternaries because they are less powerful than conditionals, being less powerful means that they are easier to read and understand too:

if today.weekday() == 0:
    t = today
else:
    t = today - timedelta(days=today.weekday())

Becomes:

t = today if today.weekday() == 0 else today - timedelta(days=today.weekday())

Monday is equal to zero, but you should not care

You write today.weekday() == 0 to check if  weekday is Monday, but it is much more intuitive to read: today.weekday() == MONDAY

To allow this just add:

MONDAY = 0

at the top of your code, and you gained a lot of readability with a small effort.

Avoid single letter names

t tells me nothing about the purpose of this variable, try and think about a more descriptive name.

200 it was said...

def significant_difference(v, previous_v):
    return abs(sum(v) - sum(previous_v)) > 200

Why 200? This is very confusing, make a constant containing that value and give an explanatory name to it, because a random magic number 200 in the code just leaves me wondering...

Similar ...

The function find_v_border and find_left_border are similar, but sadly I cannot think of a way to abstract the similarity out... I just wanted to point this out as a possible further modularization possibility.

Not needed global

global is needed only when modifying global variables, so you may delete it from draw_on_image.

Re-use

Re-using singular functions is impossible now, as all the script will get executed if I import it, just putting the top-level code under a if __name__ == "__main__": will allow the re-use of single functions.

Function for deciding name and content

This block of code:

if "server" in str.lower(file_name):
    text = " Server Systems"
    new_file_name =  "labelled_server.png"
elif "desktop" in str.lower(file_name):
    text = " Desktop Desktop"
    new_file_name = "labelled_Desktop.png"
else:
    text = " All Systems"
    new_file_name = "labelled_all.png"

Only does one thing, so extracting it to a function looks like a good idea:

def find_text_and_new_name(file_name):
    if "server" in str.lower(file_name):
        return " Server Systems", "labelled_server.png"
    elif "desktop" in str.lower(file_name):
        return " Desktop Desktop", "labelled_Desktop.png"
    else:
        return " All Systems", "labelled_all.png"

Calling this from the top-level makes it more abstract, and you always want your top-level code to be as abstract as possible.

Increase generality

The draw_on_image function may easily be customized by the use of optional arguments, for example why should font always be 16? Making it an optional argument increases generality thus utility of your function.

(minor) Avoid unnecessary assignment

os.path.sep is never going to change, so you do not need to store it in another variable.


Importing text into PANDAS and counting certain words Aim: To improve the speed of the following code. Current timing is about 80~ hours :0

Purpose: The code imports a dataset which contains 1.9 million rows and two columns. One of these columns contain text posts of var length. I then loop through each of these rows and query the post against an imported function that returns a specific counter of variable length. The counter tells me about the presence of certain words in the text. On average the func takes less than  1 ms to return this counter. (Timer for the "Func" inserted at the end to prove this)

Overheads: The code i'm looking to improve is the loop. I accept a certain level of overhead with the "func" which can't be improved at this minute. I have considered looking at Spark or Dask to parallelize the loop and speed up the process. Suggestions are welcome

#Import data
import pandas as pd
from func import func
data = pd.read_csv('Dataset.csv')

print(len(data))
>> 1900000

print(data.columns)
>> Index(['type', 'body'], dtype='object')

#Create new DF
data2 = pd.Dataframe()

for post in data['post']:
    post = str(post)
    scores = func.countWords(posts)
    data2 = data2.append(scores,ignore_index=True)

print(scores)
>> Counter({0: 306,
     1: 185,
     2: 61,
     45: 31,
     87: 23,
     92: 5,
     94: 3,
     102: 30,})

 import time
 start = time.time()
 score = func.countWords("Slow down Sir, you're going to give yourself skin faliure!")
 end = time.time()
 print(end - start)
 >> 0.0019948482513427734

Several errors:

  I then loop through each of these loops

I take it you mean "I then loop through each of these columns".

  for post in data['post]:

missing end quote mark

  scores = Func.countWords(posts)

You imported func (lowercase) and now you're calling Func (uppercase)

  data2 = data2.append(scores,ignore_index=True)

append should take a row-type object. If the function returns a numeric, then you shouldn't be appending it. Instead you can do:

def post_to_count(post):
      return func.countWord(str(post))

scores = data['post'].apply(post_to_count)


Getting Rid of .Find in VBA Code I have a range of two columns that I want to search for a "key" and then find the corresponding value in the directly adjacent column. The key is stored in a dictionary that I populated earlier in the program. I then store the value with its corresponding key in the dictionary to display later.
I am using .Find to search the range for the key and then getting the value using .Offset to get the adjacent cell. The code follows:
Dim key As Variant
Dim filRange As Range
Dim found As Range

Set filRange = ws.Range("D2:E36419").SpecialCells(xlCellTypeVisible)

Dim count As Integer
Dim i As Long

For i = 0 To partsDict.count - 1
    key = partsDict.Keys(i)
    Set found = filRange.Find(key)

    If Not found Is Nothing Then
        count = found.Offset(0, 1).value
        partsDict(i) = count
    Else
        partsDict(i) = Empty
    End If

Next i

The code functions as expected and I am able to print all values later in the program. The issue is the program takes over 15 seconds to run. I've seen the .Find method is slow and feel like there is a better way to search and retrieve my values. Should I store the range in an array somehow? Use another dictionary? Thanks!

Set filRange = ws.Range("D2:E36419").SpecialCells(xlCellTypeVisible)

Hard coding range references will makes your code unnecessarily inflexible.  It is best to create a dynamic range reference that will resize itself to fit the data.

With ws
    Set filRange = .Range("D2:E" & .Rows.Count).End(xlUp)
End With

The filRange  is set to 2 columns.  I am assuming that column 1 is the key column and column 2 is the value column.  If this is the case then you should either adjust your fill range:

With ws
    Set filRange = .Range("D2" & Cells(.Rows.Count, "D").End(xlUp))
End With

Or adjust your search:

Set found = filRange.Columns(1).Find(key)

Range.CurrentRegion is a convenient way to create a dynamic range.

Set filRange = ws.CurrentRegion.Columns("D")

Question:

Do we think putting every pair in a new Dictionary structure would work? I'm thinking once everything is in the Dictionary, searching for my keys should be quick (does Dictionary use hashing?).

Answer:
Yes and yes.  Dictionaries use hashing for super fast look ups.  You may find this article interesting EXCEL VLOOKUP VS INDEX MATCH VS SQL VS VBA.
The reason that we use dictionaries in the first place is for the super fast look ups.  The problem in your project setup is that you are using Range.Find() for your lookups.
Its hard to give advice about what is the best approach with just a small snippet of code. Proving a more detailed question with all your relevant code, data samples, and perhaps a test stub will give you the best results.
Solution
Whatever you decide to do the key is to loop over the range values once and use the dictionary lookup up the values.  Personally, I would write a function that returns a dictionary that holds the filtered keys and values and compare it to partsDict.
Function GetFilteredRangeMap(Target As Range, KeyColumn As Variant, ValueColumnOffset As Variant) As Scripting.Dictionary

    Dim Column As Range

    Rem Set Column to the Visible Cells in the Key Column Range
    With Target.CurrentRegion.Columns(KeyColumn)
        On Error Resume Next
        Set Column = .SpecialCells(xlCellTypeVisible)
        On Error GoTo 0
    End With

    If Not Column Is Nothing Then
        Dim Map As New Scripting.Dictionary
        Dim Cell As Range
        For Each Cell In Column
            Map(KeyPrefix & Cell.Value) = Cell.Offset(ValueColumnOffset)
        Next
    End If

    Set GetFilteredRangeMap = Map
End Function

Usage

Dim Target As Range
Set Target = Sheet2.Range("A1").CurrentRegion

Dim Map As New Scripting.Dictionary
Set Map = GetFilteredRangeMap(Target, 1, 2)


Split amount into inputs passed I have a requirement where I get totalAmount and how much parts to divide that amount into as inputs. For the outputresponse i am using builder pattern and also avoiding null fields. Total amount is a big decimal (if null or less than 0, throw exception) and number of inputs can range between 1-3 (if outside of this range or non numeric, throw exception). I have written this code, but I am not convinced and think there may be a cleaner and better way that is also easy to understand just by looking at the code. Also while splitting, whatever additional cents are left, that would be added to 1st amount.
eg. if 100.01 is total amount that i have to divide into 3 parts, i should get 33.35 for first amount and 33.33 for recurring amounts. Please advise if there is a cleaner and better way to achieve this.
public OutputResponse splitAmount(BigDecimal totalAmount, int divideInto) {
if (!(1 <= divideInto && 3 >= divideInto)) {
            throw new Exception();
        }
        OutputResponse outputResponse;
        if (totalAmount != null && totalAmount.compareTo(BigDecimal.ZERO) > 0) {
            BigDecimal recurringAmounts = null;
            BigDecimal firstAmount = totalAmount;
            if (divideInto > 1) {
                recurringAmounts = totalAmount.divide(BigDecimal.valueOf(divideInto), 2, RoundingMode.FLOOR);
                firstAmount = totalAmount.subtract(recurringAmounts.multiply(new BigDecimal(divideInto - 1)));
            }

            outputResponse = OutputResponse.builder()
                    .firstAmt(firstAmount)
                    .secondPmtAmt(recurringAmounts)
                    .build();

            if (divideInto > 2) {
                outputResponse.setThirdPmtAmt(recurringAmounts);
            }
        } else {
            throw new Exception();
        }
}
```

Format
Use proper indentation, after a curly brace use a tab or 4 spaces.
Readability
Add a message with your exceptions. It's a terrible practice to just throw a regular exception and no message. You'd have to look at the line numbers in the stack trace in order to know where the exception message came from. It's better to have a user-friendly message describing the issue that occurred.
Do not mix ! with > or <. It's confusing to read.
Try to use negative or positive validation. For example, be consistent either checking if the inputs are wrong, or always check that their right. In other words have your error messages all at the top or bottom.
OutputResponse Is not a good name. It's meaningless. Assuming this is a class you made, consider changing the name. Also consider using an ArrayList rather than separate fields, that way you don't need to refactor to include a 4+ separations.
Example code:
public OutputResponse splitAmount(BigDecimal totalAmount, int divideInto) {
    if ((1 > divideInto || 3 < divideInto)) {
        throw new Exception("Divide into must be between 1-3");
    }

    if (totalAmount == null || totalAmount.compareTo(BigDecimal.ZERO <= 0) {
        throw new Exception("Total amount must be a number greater than 0!");
    }

    BigDecimal recurringAmounts = null;
    BigDecimal firstAmount = totalAmount;
    if (divideInto > 1) {
        recurringAmounts = totalAmount.divide(BigDecimal.valueOf(divideInto), 2, RoundingMode.FLOOR);
        firstAmount = totalAmount.subtract(recurringAmounts.multiply(new BigDecimal(divideInto - 1)));
    }

    OutputResponse outputResponse = OutputResponse.builder()
            .firstAmt(firstAmount)
            .secondPmtAmt(recurringAmounts)
            .build();

    if (divideInto > 2) {
        outputResponse.setThirdPmtAmt(recurringAmounts);
    }
}


Connection request to LinkedIn profiles on search page I implemented a script for a LinkedIn user to connect with all profiles on a LinkedIn search page and move to the next one automatically.

As I am a new javaScript programmer, could someone look at my code and tell me how to write better code?

// Every 1 second, click all the connect buttons on the result page
window.setInterval(function () {
    $("a:contains('Connect')").each(function (index, a) {
        $(this).trigger('click');
    });

    //Scroll down to the button of the page
    window.scrollTo(0, document.body.scrollHeight);
}, 1000);

//Click Next after every 5 seconds
window.setInterval(function () {
    $("a:contains('Next >')")[0].click();
}, 5000);

Steps to run and test are all given here.

It doesn't sound like a great idea to run something infinitely.
Especially, doing $("a:contains('Next >')")[0].click() periodically is flawed, because when there is no more Next links, the dom query will return empty result, the [0] index of which is undefined, and the .click() will throw a TypeError.

I suggest to make it possible to stop the periodic calls, especially the second one. See this related answer for a technique that you can use.

In this code, the parameters index and a of the anonymous function are never used:

$("a:contains('Connect')").each(function (index, a) {
    $(this).trigger('click');
});

You could omit them, using simply function () { in the function declaration.

Instead of using anonymous functions in the setInterval calls,
I suggest to use named functions, something like this:

var clickConnect = function() { ... };

var clickNext = function() { ... };

window.setInterval(clickConnect, 1000);
window.setInterval(clickNext, 5000);

When written this way, the window.setInterval calls are easier to read,
When using anonymous functions,
it can be hard to search for the delay parameter,
as you have to scan the code to see where the function ends and the delay parameter begins.


Forming the best possible poker hand This is what the function is doing: it gets two arrays of tuples with 5 items each - hand = [H1,H2,H3,H4,H5] and board = [B1,B2,B3,B4,B5]

What I need to do is check all arrays formed by 2 items from hand and 3 items from board, like combination = [Hn,Hm,Bi,Bj,Bk] (100 combinations in total)

Then I need to compare each one of the combinations against a dictionary to get the combination rank, and then return the best array (best rank) and the rank itself:

def check_hand(hand, board, dictionary_A, dictionary_B):

 best_hand = []
 first_int = True

 for h1 in range (0, 4):
  for h2 in range (h1+1, 5):
   for b1 in range (0, 3):
    for b2 in range (b1+1, 4):
     for b3 in range (b2+1, 5):
      hand_check = []
      hand_check.append(hand[m1])
      hand_check.append(hand[m2])
      hand_check.append(board[b1])
      hand_check.append(board[b2])
      hand_check.append(board[b3])
      hand_check = sort(hand_check) #Custom sort for my array of objects
      hand_ranks = "".join([str(hand_check[0].rank),str(hand_check[1].rank),str(hand_check[2].rank),str(hand_check[3].rank),str(hand_check[4].rank)])

      if (hand_check[0].suit == hand_check[1].suit and hand_check[1].suit == hand_check[2].suit and hand_check[2].suit == hand_check[3].suit and hand_check[3].suit == hand_check[4].suit):
       control = [dictionary_A[hand_ranks][0],dictionary_A[hand_ranks][1]]
      else:
       control = [dictionary_B[hand_ranks][0],dictionary_B[hand_ranks][1]]

      if first_int:
       best_hand = hand_check
       rank = control
       first_int = False
      elif (int(control[0]) > int(rank[0])):
       rank = control
       best_hand = hand_check
      elif (int(control[0]) == int(rank[0])):
       if (int(control[1]) > int(rank[1])):
        rank = control
        best_hand = hand_check

 return best_hand, rank[0]

I need to run this check for 2 million different hands and interact over 1000 times for every hand (Ideally I would run it for at least 100000 times for every hand, for a more statistically accurate result).
Any ideas on how to make it more efficient?

Examples:
I added some "prints" on the way for better understanding:

Hand: Q♥K♥Q♠K♠3♦
Board: 2♣2♥J♦6♥4♦

2♣2♥J♦Q♥K♥ - 2♣2♥6♥Q♥K♥ - 2♣2♥4♦Q♥K♥ - 2♣6♥J♦Q♥K♥ - 2♣4♦J♦Q♥K♥ -
   2♣4♦6♥Q♥K♥ - 2♥6♥J♦Q♥K♥ - 2♥4♦J♦Q♥K♥ - 2♥4♦6♥Q♥K♥ - 4♦6♥J♦Q♥K♥ -
   2♣2♥J♦Q♥Q♠ - 2♣2♥6♥Q♥Q♠ - 2♣2♥4♦Q♥Q♠ - 2♣6♥J♦Q♥Q♠ - 2♣4♦J♦Q♥Q♠ -
   2♣4♦6♥Q♥Q♠ - 2♥6♥J♦Q♥Q♠ - 2♥4♦J♦Q♥Q♠ - 2♥4♦6♥Q♥Q♠ - 4♦6♥J♦Q♥Q♠ -
   2♣2♥J♦Q♥K♠ - 2♣2♥6♥Q♥K♠ - 2♣2♥4♦Q♥K♠ - 2♣6♥J♦Q♥K♠ - 2♣4♦J♦Q♥K♠ -
   2♣4♦6♥Q♥K♠ - 2♥6♥J♦Q♥K♠ - 2♥4♦J♦Q♥K♠ - 2♥4♦6♥Q♥K♠ - 4♦6♥J♦Q♥K♠ -
   2♣2♥3♦J♦Q♥ - 2♣2♥3♦6♥Q♥ - 2♣2♥3♦4♦Q♥ - 2♣3♦6♥J♦Q♥ - 2♣3♦4♦J♦Q♥ -
   2♣3♦4♦6♥Q♥ - 2♥3♦6♥J♦Q♥ - 2♥3♦4♦J♦Q♥ - 2♥3♦4♦6♥Q♥ - 3♦4♦6♥J♦Q♥ -
   2♣2♥J♦Q♠K♥ - 2♣2♥6♥Q♠K♥ - 2♣2♥4♦Q♠K♥ - 2♣6♥J♦Q♠K♥ - 2♣4♦J♦Q♠K♥ -
   2♣4♦6♥Q♠K♥ - 2♥6♥J♦Q♠K♥ - 2♥4♦J♦Q♠K♥ - 2♥4♦6♥Q♠K♥ - 4♦6♥J♦Q♠K♥ -
   2♣2♥J♦K♥K♠ - 2♣2♥6♥K♥K♠ - 2♣2♥4♦K♥K♠ - 2♣6♥J♦K♥K♠ - 2♣4♦J♦K♥K♠ -
   2♣4♦6♥K♥K♠ - 2♥6♥J♦K♥K♠ - 2♥4♦J♦K♥K♠ - 2♥4♦6♥K♥K♠ - 4♦6♥J♦K♥K♠ -
   2♣2♥3♦J♦K♥ - 2♣2♥3♦6♥K♥ - 2♣2♥3♦4♦K♥ - 2♣3♦6♥J♦K♥ - 2♣3♦4♦J♦K♥ -
   2♣3♦4♦6♥K♥ - 2♥3♦6♥J♦K♥ - 2♥3♦4♦J♦K♥ - 2♥3♦4♦6♥K♥ - 3♦4♦6♥J♦K♥ -
   2♣2♥J♦Q♠K♠ - 2♣2♥6♥Q♠K♠ - 2♣2♥4♦Q♠K♠ - 2♣6♥J♦Q♠K♠ - 2♣4♦J♦Q♠K♠ -
   2♣4♦6♥Q♠K♠ - 2♥6♥J♦Q♠K♠ - 2♥4♦J♦Q♠K♠ - 2♥4♦6♥Q♠K♠ - 4♦6♥J♦Q♠K♠ -
   2♣2♥3♦J♦Q♠ - 2♣2♥3♦6♥Q♠ - 2♣2♥3♦4♦Q♠ - 2♣3♦6♥J♦Q♠ - 2♣3♦4♦J♦Q♠ -
   2♣3♦4♦6♥Q♠ - 2♥3♦6♥J♦Q♠ - 2♥3♦4♦J♦Q♠ - 2♥3♦4♦6♥Q♠ - 3♦4♦6♥J♦Q♠ -
   2♣2♥3♦J♦K♠ - 2♣2♥3♦6♥K♠ - 2♣2♥3♦4♦K♠ - 2♣3♦6♥J♦K♠ - 2♣3♦4♦J♦K♠ -
   2♣3♦4♦6♥K♠ - 2♥3♦6♥J♦K♠ - 2♥3♦4♦J♦K♠ - 2♥3♦4♦6♥K♠ - 3♦4♦6♥J♦K♠ -

Best Hand: 2♣2♥J♦K♥K♠ Rank: 3

Just wondering if there is a way to run the piece of code below more efficiently. I just started to get acquainted with parallel program and think this might be an answer? But I have no idea how to do it using imap or processes.

for h1 in range (0, 4):
  for h2 in range (h1+1, 5):
   for b1 in range (0, 3):
    for b2 in range (b1+1, 4):
     for b3 in range (b2+1, 5):
      hand_check = []
      hand_check.append(hand[m1])
      hand_check.append(hand[m2])
      hand_check.append(board[b1])
      hand_check.append(board[b2])
      hand_check.append(board[b3])

Use itertools for this kind of thing:

for h in itertools.combinations(hand, 2):
  for b in itertools.combinations(board, 3):
    hand_check = list(h) + list(b)

Also, I don't find hand_check to be a helpful name. In general I expect variable names to be nouns (unless they're Booleans, when predicates make sense), and when I parse hand_check as a noun I find that it's a check modified by hand. The smallest change would be to rename it hand_to_check; I prefer candidate_hand.

      if (hand_check[0].suit == hand_check[1].suit and hand_check[1].suit == hand_check[2].suit and hand_check[2].suit == hand_check[3].suit and hand_check[3].suit == hand_check[4].suit):

Ugh. Use set.

if len(set(card.suit for card in hand_check)) == 1:

      hand_check = sort(hand_check) #Custom sort for my array of objects
      hand_ranks = "".join([str(hand_check[0].rank),str(hand_check[1].rank),str(hand_check[2].rank),str(hand_check[3].rank),str(hand_check[4].rank)])

       control = [dictionary_A[hand_ranks][0],dictionary_A[hand_ranks][1]]
      else:
       control = [dictionary_B[hand_ranks][0],dictionary_B[hand_ranks][1]]

Firstly, why the quadruple-lookup? Why not

lookup_table = dictionary_A if only_one_suit else dictionaryB
control = lookup_table[hand_ranks]

?

Secondly, although Python's string manipulation is quite fast, since you're aiming to microoptimise you should think in terms of integers rather than strings. I'd delve into the fascinating world of perfect hashes to try to get a lookup table which fits into L2 cache, or maybe even L1 with luck (there are only 6188 multisets of 5 ranks, so less than 25 kB of lookup table suffices).

      if first_int:
       best_hand = hand_check
       rank = control
       first_int = False
      elif (int(control[0]) > int(rank[0])):
       rank = control
       best_hand = hand_check
      elif (int(control[0]) == int(rank[0])):
       if (int(control[1]) > int(rank[1])):
        rank = control
        best_hand = hand_check

If you initialise rank to [-1] then you should be able to eliminate first_int and simplify this to

      if (control > rank):
        rank = control
        best_hand = hand_check

Now, I've addressed micro-optimisation above, but the first rule of optimisation is to start by looking at the algorithm.

  What I need to do is check all arrays formed by 2 items from hand and
  3 items from board, like combination = [Hn,Hm,Bi,Bj,Bk] (100
  combinations in total)

  Then I need to compare each one of the combinations against a
  dictionary

is wrong. What you need to do is find the set of five cards formed by 2 cards from the hand and 3 cards from the board which has the highest score.

I'd suggest that you micro-optimise and then benchmark against a major restructuring with the following outline:

for suit in the_four_suits:
  hand_mask = build_mask((card for card in hand if card.suit == suit))
  board_mask = build_mask((card for card in board is card.suit == suit))
  check_royal_flush(hand_mask, board_mask)
  check_straight_flush(hand_mask, board_mask)
  check_flush(hand_mask)

hand_counts = count_by_ranks(hand)
board_counts = count_by_ranks(board)
check_four_of_a_kind(hand_counts)
check_full_house(hand_counts)
...

The key ideas are to do everything with bit manipulation and to return as soon as you know that no further checks will produce a hand better than the best you've already found. For count_by_ranks, if you allocate three bits per rank then overflow into the next rank is impossible since there are only four suits. So e.g. to check for a four of a kind:

total_counts = hand_counts + board_counts
possible_fours = total_counts & 0x4924924924
# possible_fours indicates ranks where we have all four cards between hand and board
# We take three cards from board, so rule out ranks where all four are in the board
possible_fours &= ~board_counts
# Similarly, rule out ranks where all four are in hand
possible_fours &= ~hand_counts
# And rule out ranks where three are in hand
possible_fours &= ~((hand_counts << 1) & (hand_counts << 2))
if possible_fours != 0:
  # We have a four-of-a-kind: just need to extract it


Creating an associated object on create/update My app is setup where there are musical artists and albums have message boards through a has_one polymorphic association.

class Artist < ApplicationRecord
  has_one :board, as: :boardable, dependent: :destroy

  after_create -> (artist) { BoardGenerator.create(artist) }
  after_update -> (artist) { BoardGenerator.update(artist) }
end

class Album < ApplicationRecord
  has_one :board, as: :boardable, dependent: :destroy

  after_create -> (album) { BoardGenerator.create(album) }
  after_update -> (album) { BoardGenerator.update(album) }
end

class Board < ApplicationRecord
  belongs_to :boardable, polymorphic: true, optional: true
end

For every artist and album created, I'd like to create a message board associated to it. I decided to accomplish this using the after_create/after_udpate callbacks where business logic is extracted to a service object:

class BoardGenerator
  def self.create(boardable)
    boardable.board = Board.create(name: boardable.name)
  end

  def self.update(boardable)
    boardable.board.update_attribute(:name, boardable.name)
  end
end

One of my main concerns is that the create/update methods are dependent on both models having a name attribute. Is there any way to mitigate the risk of this ever changing between the two?

I'm trying to get in the habit of creating single responsibility models (hence the service object). Still new to the concept though. Can someone tell me if I'm on the right track and/or how to improve the code I have now?

Updated Answer: It's funny, because now that I've revisited this since your comment, I've flip-flopped my thinking a little. I still feel like the BoardGenerator class is unnecessary.

  Revisiting this, I noticed you mentioned the BoardGenerator being unnecessary. If that was the case how would a board know when to be created or updated without having some sort of link to a boardable?

The real issue is keeping the board name synchronized with the album or artist, and coordinating the creation of these dependent objects. The "boardable" should be the one responsible for this synchronization operation.

First, let's think about this problem from the standpoint of code using the Artist class.

When creating a new "boardable" I would expect this:

artist = Artist.create(name: "Prince")
puts artist.board.name # Prints "Prince"

The board is created automatically and gets the same name as the artist. Your code accomplishes this through the use of a separate class. Since it is the Artist that needs the board, and the Artist needs the board to have the same name, this behavior should be encapsulated within the Artist class completely. You can accomplish this by utilizing the after_initialize callback in ActiveRecord:

class Artist < ApplicationRecord
  has_one :board, as: :boardable, dependent: :destroy

  after_initialize -> (artist) { create_board(name: artist.name) unless artist.persisted? }
end

Now there is the issue of updating the name of the board when a "boardable" updates its name. Again encapsulation is our friend. Overriding the name= method on a "boardable" allows the names to be synchronized:

class Artist < ApplicationRecord
  has_one :board, as: :boardable, dependent: :destroy

  after_initialize -> (artist) { create_board(name: artist.name) unless artist.persisted? }

  def name=(new_name)
    super.name = new_name
    board ||= create_board(name: new_name)
    board.name = new_name
  end
end

Now when we get proper synchronization on update as well, without this logic leaking to other classes, or requiring another class that has little logic in it at all:

artist = Artist.find 23
artist.name = "The Artist Formerly Known As Prince"
puts artist.board.name # Prints "The Artist Formerly Known As Prince"

It's been a while since I've written much Ruby, but I feel like this behavior could be pulled out into its own Module and included as a mixin to both the Artist and Album classes.

Original Answer: I used to think the Board should be responsible for this stuff, but I've since reconsidered (see above).

I don't see an issue with your design, other than the BoardGenerator seems unnecessary. I would just go with static methods on the Board class itself. No need to separate this logic.

Responses to comments:

Carl Edwards asked:

  Creating static methods wouldn't violate the principle of single mode responsibility?

The Single Responsibility Principal states that a class or method should only have one reason to change.

The rule is, if a Board is attached to a Boardable, keep the names of the two things in sync. The name of the Board is what gets affected, and the Board class should be responsible for that.

Encapsulating this logic in the Board class is much more important that tightly adhering to the SRP for one simple reason: encapsulation.

Encapsulating data and behavior is far more important in this case. I also don't see how it violates the SRP. If it does violate the SRP, encapsulation is far more important than a "principal," because encapsulation is one of the foundational aspects of good object oriented code.

Wow. I've written "encapsulation" a lot in the last two paragraphs, but I hope it really drives the point home that encapsulating the data and behavior of a Board is the most important aspect here.


Python Receipt Creator My code takes input from the user and creates a receipt with the information the program receives from the user:

import easygui

items = []
prices = []
amount = []
amounts = 0
finished = False
total_price = 0.00
list_num = -1

def take_item():
    global list_num, amounts, finished
    item = easygui.enterbox("What item is it?")
    if item == "done" or item == "None":
        finished = True
    else:
        how_many = easygui.integerbox("How many {0:} do you have?".format(item))
        amounts = how_many
        items.append(item)
        amount.append(how_many)
        list_num += 1

def pricing():
    global list_num, total_price
    if finished:
        pass
    else:
        try:
            price = easygui.enterbox("What is the price of 1 {0:}".format(items[list_num]))
        new_price = float(price)
        except ValueError:
            easygui.msgbox("Please enter a number.")
            pricing()
        else:
            total_price += float(price) * amounts
            prices.append(new_price)

def create_receipt():
    print "Item \t Amount \t Price \t Total Price"
    for i in range(0, list_num+1):
        print items[i], "\t\t", amount[i], "\t\t 3", prices[i]
    print "\t\t\t ", "$",total_price
    print "Thank you for using Tony's Receipt Maker!"
    print "-----------------------------------------------------------"

while 1:
    take_item()
    pricing()
    if finished:
        create_receipt()
    else:
        continue

Are there any ways to improve the efficiency of the code and shorten it? Is there anything I should improve in my code?

Globals

The problem is with your global variables.

You have way too many global variables and this is a problem because a global variable may, by definition, be modified from anywhere in your program.

This means that the reader of your program has to keep in mind all the values of all the variables at the same time.

Now for a small script like this this is no big deal, but imagine having not 5 global variables but 50 of them. Now you understand how hard it can become to follow un-organized code.

I suggest that each function should take the variables that it operates on as arguments and, if it modifies them, then it should return the new values back.

For example the function that prints out the final recipe can easily be adjusted to take as arguments the three arrays.

Premature optimization

I see that you keep a variable that indicates how long the input list is; that is premature optimization because you may just recalculate the length just before looping.

In fact in a user interface driven program you should not worry at all about performance because computers are very very fast nowadays.

To be complete there is an even more efficient way of looping, that is using the zip built in but that is a more advanced topic.

pricing

I also had a look at your pricing function.

I think the whole try except block should be extracted into its own function as getting an input that must be a number from the user is a pretty common task.

This will allow you to reuse that particular piece of code.


Simplifying linked list insert_after Is there a better way to insert after a node in a linked list? Currently I go through the node from the head of the list and find the node that has the value pos. If the node is not null at the end of the search then we found the position, so we do the insert.

void insert_after(List& list, int pos, int value)
{
    Node* newNode = new Node{value}, p;

    for (p = list.head; p && p->data != pos; p = p->next) {;}

    if (p)
    {
        newNode->next = p->next;
        p->next = newNode;
    }
}

I just don't know if this is the best way. Could it be improved at all?

Your solution is missing critical feedback, or alternatively, instead of feedback, fallback.

If the node is added successfully then the method should report back that success. Alternatively, it should fail, and report that fail. A simple bool return value should suffice.

If you choose to use a fallback, instead of feedback, then you should consider how the system should behave if the 'insert after' item is not found... insert at the beginning of the list? Append at the end? Whatever fallback you choose, you should implement and document it correctly.

Leaving the system as it is, though, is not a good option.

For loop blocks where there is no code in the block (a for loop with all the logic in the loop clauses), I recommend instead that you use a while loop....

Node* p = list.head;
while (p && p->data != pos) {
    p = p->next;
}
if (p) {
    ....
    return true;
}
return false;

As for whether there's a better way/algorithm, well, for a LinkedList, like you have, then no. The scan to locate the insert-point is needed.

Finally, you should only create the new Node{value} if you actually locate the insert point. There is no need to create the new node unless you are actually going to insert it.


Leetcode #826. Most Profit Assigning Work solution in Java (Sort + Binary Search) Problem Statement:

  We have jobs: difficulty[i] is the difficulty of the ith job, and profit[i] is the profit of the ith job.

  Now we have some workers. worker[i] is the ability of the ith worker, which means that this worker can only complete a job with difficulty at most worker[i].

  Every worker can be assigned at most one job, but one job can be completed multiple times.

  For example, if 3 people attempt the same job that pays $1, then the total profit will be $3.  If a worker cannot complete any job, his profit is $0.

  What is the most profit we can make?

  Example 1:

  Input:

  difficulty = [2,4,6,8,10]
  profit = [10,20,30,40,50]
  worker = [4,5,6,7]

  Output:

  100

  Explanation: Workers are assigned jobs of difficulty [4,4,6,6] and they get profit of [20,20,30,30] seperately.

  Notes:

  1 <= difficulty.length = profit.length <= 10000
  1 <= worker.length <= 10000
  difficulty[i], profit[i], worker[i]  are in range [1, 10^5]

Solution:

Time Complexity: O(n log n + w log n).
Space Complexity: O(n).

class Solution {
    private static class Job implements Comparable<Job> {
        private int difficulty;
        private int profit;

        public Job(int difficulty, int profit) {
            this.difficulty = difficulty;
            this.profit = profit;
        }

        public int getDifficulty() {
            return difficulty;
        }

        public int getProfit() {
            return profit;
        }

        //Jobs are ordered based on their difficulties
        @Override
        public int compareTo(Job otherJob) {
            int difference = this.getDifficulty() - otherJob.getDifficulty();

            //If both the jobs have the same difficulty, we want the more profitable job to be ordered first so that we get correct maxProfits
            difference = (difference == 0) ? (otherJob.getProfit() - this.getProfit()) : difference;

            return difference;
        }
    }

    private void updateDifficultyAndMaxProfits(Job[] jobs, int[] difficulties, int[] maxProfits) {
        for (int i = 0; i < jobs.length; i++) {
            Job job = jobs[i];
            difficulties[i] = job.getDifficulty();
            if (i == 0) {
                maxProfits[i] = job.getProfit();
            } else {
                maxProfits[i] = Math.max(job.getProfit(), maxProfits[i - 1]);
            }
        }
    }

    public int maxProfitAssignment(int[] difficulties, int[] profits, int[] workers) {
        Job[] jobs = new Job[difficulties.length];

        for (int i = 0; i < difficulties.length; i++) {
            jobs[i] = new Job(difficulties[i], profits[i]);
        }

        Arrays.sort(jobs);

        int[] maxProfits = new int[difficulties.length];
        updateDifficultyAndMaxProfits(jobs, difficulties, maxProfits);

        int totalMaxProfit = 0;

        for (int worker : workers) {
            int workerMaxProfitIndex = Arrays.binarySearch(difficulties, worker);

            //If there isn't an exact match we need to retrieve the index from the insertion point
            if (workerMaxProfitIndex < 0) {
                workerMaxProfitIndex = -(workerMaxProfitIndex + 2);
            }

            //Update totalMaxProfit only if there's at least one task that the worker can accomplish
            if(workerMaxProfitIndex >= 0){
                totalMaxProfit += maxProfits[workerMaxProfitIndex];
            }
        }

        return totalMaxProfit;
    }
}

Please review my code and let me know if there's room for improvement.

Algorithm

The algorithm is correct, but an optimization is possible.
When the input contains multiple jobs with the same difficulty,
then you can reduce the search space,
because at any difficulty level,
you're only interested in the most profitable job.
The time complexity would become \$ O(n \log n + w \log n')\$, where \$n'\$ is the number of unique difficulties.

Extract special treatment out of the loop

The loop in updateDifficultyAndMaxProfits has a special treatment for index 0.
Instead of evaluating a condition for each iteration of the loop,
since the input is guaranteed to have at least 1 element,
you could perform the special treatment before the loop begins,
and make the loop start from index 1, and no conditional.

Avoid modifying the input

You pass to the method updateDifficultyAndMaxProfits the difficulties array that was an input, and overwrite its content.
Although this saved you some space in memory,
it may not be acceptable, and not a good practice.
It would have been better to pass to the function a new array.

However, with the first tip at the top of this review,
this point will no longer matter,
because you will need a different approach to store the (difficulty, maxProfit) pairs that avoids redundant difficulty values,
and you won't know in advance the number of pairs you will need.
You may for example implement a computeDifficultyAndMaxProfits,
returning a list of pairs.

How about some streams and lambdas

Some elements of your program can be written more compactly with streams and lambdas, for example:

Job[] jobs = IntStream.range(0, profit.length)
    .mapToObj(i -> new Job(difficulty[i], profit[i]))
    .sorted(Comparator.comparingInt(job -> job.difficulty))
    .toArray(Job[]::new);

Since Job implements Comparable, the custom comparator is not necessary here.
Keeping the sorting logic outside the class like this is often more practical.
It's also compact to write.
If you needed to sort multiple times, you could store the lambda in a variable to avoid duplicating logic.

The final computation could also be written with streams and lambdas compactly:

return IntStream.of(workers)
    .map(w -> computeMaxProfit(w))
    .sum();


Turn a list of words into a dictionary I am looking for a way to turn a list of words that may have duplicates into a dictionary/map that counts the number of occurrences of words.  After spending some time with the problem, this seems to be one of the better ways, but maybe there are some downsides I am not seeing to this.

const magazine = "asdf ASDF wer wer";

This should produce a magazineMap of

const magazineMap = {
  asdf: 1,
  ASDF: 1,
  wer: 2
}

My solution to this was

function mapMagazine (magazine) {
    return magazine
        .split(' ')
        .reduce((initMap, word) => {
            return {
                ...initMap,
                [word]: (initMap[word] || 0) + 1
            }
        }, {});
}

Efficiency and potential problems
You could use a Map but as you are counting word occurrences using an object is a little more efficient.
Efficiency
Your code can be improved as it is highly inefficient with each word needing to create a new object and fill its properties from the previous one, then add or update then next word. You need only create one object and add properties to it as you find them.
function mapMagazine (text) {
    return text.split(' ')
        .reduce((map, word) => (
            map[word] = map[word] ? map[word] + 1 : 1, map
        ), {});
}

or using forEach,
function mapMagazine (text) {
    const map = {};
    text.split(' ').forEach(w => map[w] = map[w] ? map[w] + 1 : 1);
    return map;
}

or the (slightly) more performant version using a for loop
function mapMagazine (text) {
    const map = {};
    for (const w of text.split(' ')) { map[w] = map[w] ? map[w] + 1 : 1 }
    return map;
}

Problems
I do however see that your code would suffer from problems with a sting such as "A is a, as an a at the beginning,  a capitalised A." (Note it has a double space in it) It would return with some of the following properties...
{
    A : 1,
    "a," : 1,
    a : 2,
    "A." : 1,
    "beginning," : 1,
    "" : 1,  // empty string from splitting two spaces
     .. and the rest
 }

I would imagine you would want something a little more like.
{
    a : 5,
    beginning : 1,
     .. and the rest
 }

To do this you need a slight mod to the function. The text should be converted to lowercase, split can use a RegExp to split into words on white spaces or groups of white spaces, and a check for empty string that can result if the text is ended on a white space such as a full stop.
BTW does the function really map magazines?
function mapWords(text) {
    const map = {};
    for (const word of text.toLowerCase().split(/\W+/)) {
        word !== "" && (map[word] = map[word] ? map[word] + 1 : 1);
    }
    return map;
}


Implementing any_of in C++ I've decided to implement Python's any in C++. I've gone about this using templates to allow multiple types of data to be passed, instead of overloading the function multiple times. This is my first time using templates so I would really like feedback about my usage of them. I'm also fairly new with references and pointers, so I would like some criticism about my use of them as well. Of course, anything else is on the table and appreciated.

While writing this program, I realized std::any_of existed. So yes, I do know there's already a built-in method for this.

any.hpp

#ifndef ANY_HPP_INCLUDED
#define ANY_HPP_INCLUDED

/**
 * @author Ben Antonellis
**/

#include <vector>
#include <iostream>

/**
 * Returns True if any of the elements meet the callback functions parameters.
 *
 * @param elements - A list of elements.
 * @param callback - Callback function to invoke on each element.
 *
 * @return bool - True if parameters are met, False otherwise.
**/
template <typename List, typename Function>
bool any(List &elements, Function *callback) {
    for (auto element : elements) {
        if (callback(element)) {
            return true;
        }
    }
    return false;
}

#endif

And here's how I'm testing this function:

main.cpp

/**
 * @author Ben Antonellis
**/

#include "Any.hpp"

int main() {

    std::vector<double> doubleElements = {-1.0, -2.0, -3.0};
    std::vector<std::string> stringElements = {"Hello", "Goodbye", "Testing 123"};

    auto doubleFunc = [] (double number) {
        return number < 0;
    };
    auto stringFunc = [] (std::string string) {
        return string.length() >= 5;
    };

    /* Test Any Implementation */
    if (any(doubleElements, *doubleFunc)) {
        std::cout << "(any) Double Success." << std::endl;
    }
    if (any(stringElements, *stringFunc)) {
        std::cout << "(any) String Success." << std::endl;
    }

    return 0;
}

If you would like to compile and run, below is the script I'm using to compile and run this program.

run.sh

# Compile c++ program to .o file #
g++ -std=gnu++2a -c main.cpp # gnu++2a = Working draft for ISO C++ 2020 with GNU extensions #

# Compile .o file to executable #
g++ main.o -o app

# Run the executable #
./app

# Delete executable and .o files after running #
rm app main.o

The algorithm looks correct.

Regarding the function signature, I'd make three changes:

You're not modifying elements, so take it by constant reference rather than reference.
You don't need to specify a pointer to Function, Function is already a template parameter, and non-pointers could be valid (ex: a class with a () operator).
"callback" isn't a very useful name for this case.  "predicate" would be better.

While this form is fine, I'd also include a version that takes a start/end iterator instead of a container.

Also, there's no reason to include vector or iostream in any.hpp - only include things that are necessary.

For completeness in the testing method, you should also test with an empty collection, and a collection with no matches.