Adding object to database, using linq or sql connection I have a table called tblDelegates in my database which I need to populate with some data.  I have created a class called Delegates which has various properties to hold the bulk of the information.

I am fairly new to C#, so I wanted to get the answers to a few questions as to why you should be doing things in a particular way and also to get some advice on how I could improve my code.

Please can someone take a look at the questions below and give me a few pointers as to whether this is the correct way to do things.

Is this the correct way to do this, by first creating an instance of an object and then adding the fields to it before adding the object to the database?
This will sound like a total noob question (so I apologies), but why bother having to create the delegates class object, why could/should you not just add the fields directly into the database?
In my Delegates object, should I also add the fields such as Created_User Created_Dttm Updated_User Updated_Dttm, or am I ok leaving them out of this object like I have done below.
Is the most efficient/quickest way to do this using LINQ like I have done below, or should I be using a SQL connection, or is it just preference?
Can anyone tell me if the formatting of my code is correct, or if I should separate out the steps into separate functions perhaps?

Code to add data to database

        var del = new Delegates
        {
            Pending = true,
            ManagerAlias = row.Cells[4].Text,
            ManagerName = row.Cells[1].Text + " " + row.Cells[2].Text,
            DelegateAlias = ADLookup.AD("alias"),
            DelegateName = ADLookup.AD("GivenName") + ADLookup.AD("sn"),
            DelegateEmail = ADLookup.AD("mail")
        };

        var dc = new LinqToSqlDataContext();
        var Del = new tblDelegate
        {
            Pending = del.Pending,
            Manager_Alias = del.ManagerAlias,
            Manager_Name = del.ManagerName,
            Delegate_Alias = del.ManagerAlias,
            Delegate_Name = del.DelegateName,
            Delegate_Email = del.DelegateEmail,
            Created_User = ADLookup.ADCurrentUser(),
            Created_Dttm = DateTime.Now,
            Updated_User = ADLookup.ADCurrentUser(),
            Updated_Dttm = DateTime.Now
        };

        dc.tblDelegates.InsertOnSubmit(Del);
        dc.SubmitChanges();

Delegates Class

public class Delegates
{
    public bool Pending { get; set; }
    public string ManagerAlias {get;set;}
    public string ManagerName {get;set;}
    public string DelegateAlias {get;set;}
    public string DelegateName {get;set;}
    public string DelegateEmail {get;set;}

    public Delegates()
    {
    }

    public Delegates(bool pending, string manageralias, string managername, string delegatealias, string delegatename, string delegateemail)
    {
        Pending = pending;
        ManagerAlias = manageralias;
        ManagerName = managername;
        DelegateAlias = delegatealias;
        DelegateName = delegatename;
        DelegateEmail = delegateemail;
    }
}

As @ckuhn203 pointed out, the naming is pretty misleading, but it seems like your domain model calls that entity a Delegate. To dissipate ambiguity I'd call it something like DelegateEntity.

One convention in modeling, is to keep types singular, since an entity represents one single row in that tblDelegates table.

Now, in your insert code, the del instance is not needed. Leaves us with this (notice the name changes) - I'd recommend passing in your connection string to the context's constructor, instead of relying on the one in the designer:

    var context = new LinqToSqlDataContext(connectionString);
    var item = new tblDelegate
    {
        Pending = true,
        Manager_Alias = row.Cells[4].Text,
        Manager_Name = row.Cells[1].Text + " " + row.Cells[2].Text,
        Delegate_Alias = ADLookup.AD("alias"),
        Delegate_Name = ADLookup.AD("GivenName") + ADLookup.AD("sn"),
        Delegate_Email = ADLookup.AD("mail"),
        Created_User = ADLookup.ADCurrentUser(),
        Created_Dttm = DateTime.Now,
        Updated_User = ADLookup.ADCurrentUser(),
        Updated_Dttm = DateTime.Now // shouldn't this one be null?
    };

    context.tblDelegates.InsertOnSubmit(item);
    context.SubmitChanges();

A very important thing to keep in mind, is that a data context implements an interface called IDisposable, which means any instance of that type should be properly disposed, either by explicitly calling its .Dispose() method, or (the better way) by wrapping its usage inside a using block:

using (var context = new LinqToSqlDataContext())
{
    var item = new tblDelegate
    {
        Pending = true,
        Manager_Alias = row.Cells[4].Text,
        Manager_Name = row.Cells[1].Text + " " + row.Cells[2].Text,
        Delegate_Alias = ADLookup.AD("alias"),
        Delegate_Name = ADLookup.AD("GivenName") + ADLookup.AD("sn"),
        Delegate_Email = ADLookup.AD("mail"),
        Created_User = ADLookup.ADCurrentUser(),
        Created_Dttm = DateTime.Now,
        Updated_User = ADLookup.ADCurrentUser(),
        Updated_Dttm = DateTime.Now // shouldn't this one be null?
    };

    context.tblDelegates.InsertOnSubmit(item);
    context.SubmitChanges();
}

The context instance only exists within the using scope, and when the code exits that scope, context.Dispose() gets called, implicitly. Make it a habit to always put IDisposable instances in a using block.

Other than that, you're not following the naming convention for parameters: alllowercase is hard to read - should be camelCase:

public Delegates(bool pending, string managerAlias, string managerName, string delegateAlias, string delegateName, string delegateEmail)
{
    Pending = pending;
    ManagerAlias = managerAlias;
    ManagerName = managerName;
    DelegateAlias = delegateAlias;
    DelegateName = delegateName;
    DelegateEmail = delegateEmail;
}

But, instead of having a constructor with [potentially] dozens of parameters, you should simply remove all constructors (the parameterless one is there by default), and instantiate a Delegates like this instead:

var delegates = new Delegates {
                                  Pending = true,
                                  ManagerAlias = "foo",
                                  ManagerName = "bar",
                                  DelegateAlias = "_foo",
                                  DelegateName = "_bar",
                                  DelegateEmail = "foo@bar.com"
                              };

This syntax is the object initializer syntax - notice that you don't need to specify the () after new Delegates, because the syntax is calling the type's default constructor and the compiler knows this, so ()'s are redundant. Of course this syntax can't work if your type doesn't have a default, parameterless constructor.

If you have a linq-to-sql data context, you probably have a designer with a .dbml too; in there you can specify new, more C#-friendly names for tblDelegates and its columns, LINQ-to-SQL uses the read-only Source property to know which column it's referring to - this means you can have a table called SOME_UGLY_NAME in your SQL backend, and refer to it as SomeMuchBetterName in your C# code!

...this also means tblDelegates can be mapped to DelegateEntity ;)


How to refactor multiple if statements? The code I want to refactor look as following:

public bool CheckPay(Int64 orderID)
{
    vpc_OrderInfo = orderID;
    RequestParams =
        string.Format(
            "vpc_Version={0}&vpc_Command={1}&vpc_AccessCode={2}&vpc_MerchTxnRef={3}&vpc_Merchant={4}&vpc_User={5}&vpc_Password={6}",
            vpc_Version, vpc_Command, vpc_AccessCode, vpc_MerchTxnRef, vpc_Merchant, vpc_User, vpc_Password);

    string[] response = SendRequest(vpc_VirtualPaymentClientURL, RequestParams).Split('&');
    bool ResponseCode = false;
    bool exist = false;
    Total = 0;
    TransactionDate = DateTime.Now;
    TransactionId = CardNo = vpc_Message = string.Empty;

    var query = new StringBuilder();
    try
    {
        foreach (string s in response)
        {
            if (string.IsNullOrWhiteSpace(s))
                throw new Exception(string.Format("Message='{0}' ", s));

            if (s.IndexOf("vpc_SecureHash") == -1)
            {
                string vpcMessage = s.Split('=')[1];
                query.AppendFormat("{0}-{1}|", s.Split('=')[0], vpcMessage);

                if (s.IndexOf("vpc_DRExists") > -1)
                {
                    exist = vpcMessage == "Y";
                }
                if (s.IndexOf("vpc_TxnResponseCode") > -1)
                {
                    ResponseCode = vpcMessage == "0";
                }
                if (s.IndexOf("vpc_Amount") > -1)
                {
                    Total =
                        double.Parse(vpcMessage.Length > 2
                                            ? vpcMessage.Insert(vpcMessage.Length - 2, ",")
                                            : "0," + vpcMessage);
                }
                if (s.IndexOf("vpc_BatchNo") > -1)
                {
                    TransactionDate = DateTime.Parse(vpcMessage.Insert(6, "-").Insert(4, "-"));
                }
                if (s.IndexOf("vpc_TransactionNo") > -1)
                {
                    TransactionId = vpcMessage;
                }
                if (s.IndexOf("vpc_CardNum") > -1)
                {
                    CardNo = vpcMessage;
                }
                if (s.IndexOf("vpc_Message") > -1)
                {
                    vpc_Message = vpcMessage;
                }
            }

            serviceLog.InfoFormat("Message: " + s);
        }
    }
    catch (Exception ex)
    {
        serviceLog.Error("fatal error", ex);
    }
    return exist && ResponseCode;
}

I'm not sure if I can get rid of these multiple if statements.

The "exist" and "ResponseCode" are boolean values and I need them to return bool type from method. Other values are class properties.

This isn't related to your question, but you are not using exceptions properly.

try
{
    foreach (string s in response)
    {
        if (string.IsNullOrWhiteSpace(s))
            throw new Exception(string.Format("Message='{0}' ", s));

        //...
    }
}
catch (Exception ex)
{
    serviceLog.Error("fatal error", ex);
}

You are using the exception as a way to break out of the for loop. This is exactly what the break statement is for. Instead, you create a new Exception, which must populate the stack trace, and then the code starts to unwind the stack looking for the closest matching catch. This is an expensive process for something that is very basic. This could be replaced with:

foreach (string s in response)
{
    if (string.IsNullOrWhiteSpace(s))
    {
        serviceLog.Error("fatal error", string.Format("Message='{0}' ", s));
        break;
    }

    //...
}

In addition, you are throwing the base class Exception. This means that the most specific catch you can write is for the base class. This will catch all exceptions, including things like StackOverflowException and OutOfMemoryException. Your code will then try to continue on with no knowledge that these much more serious exceptions were swallowed up into a log message.

When you throw an exception in your code, it should be a sub-class of Exception that is as specific as you can be. This may be a predefined exception of an exception class you explicitly declare. By doing this, you can use the type system to handle different types of exceptions differently and not catch the exceptions you don't mean to handle.


Foreach-loop for and print commands How can I make the following code shorter or efficient (maybe with other loops or other nice ideas), and keep the current functionality?

my $examine = shift; my $detect = 0;
foreach my $ProteinDB (@DB) {
    my $Set =  $ProteinDB->{'ID'};
    if($Set =~ /$examine/) {
        $detect = 1;
        my @Pointer = keys(%$ProteinDB);
        foreach my $key (@Pointer) {
            if($key ne "ID" && $key ne "SQ") {
                print "$key " . $ProteinDB->{$key} . "\n";
                print "\n";
            }
        }
        print "SQ  " . $ProteinDB->{"SQ"} . "\n";
        print "//\n";
    }
}
if($detect == 0) {
    print "Error : For the ID $examine NO-Entry could be Found !\n";
}

It's nice code and easy to understand, but I don't have that experience that I can tell what I can do more with this code.  I am gonna keep trying to change it until it's shorter.

This is shorter, but not necessarily more efficient,

my $examine = shift; my $detect = 0;
for my $ProteinDB (@DB) {
    next if $ProteinDB->{'ID'} !~ /$examine/;
    $detect = 1;
    print "$_ $ProteinDB->{$_}\n\n"
      for grep $_ ne "ID" && $_ ne "SQ", keys %$ProteinDB;
    print "SQ  $ProteinDB->{SQ}\n//\n";
}
$detect or print "Error : For the ID $examine NO-Entry could be Found !\n";

and if you push it little more,

my $examine = shift; my $detect = 0;
print map {
    $detect = 1;
    my $ProteinDB = $_;
    (
      (map "$_ $ProteinDB->{$_}\n\n", grep $_ ne "ID" && $_ ne "SQ", keys %$ProteinDB),
      "SQ  $ProteinDB->{SQ}\n//\n"
    );
  }
  grep $_->{ID} =~ /$examine/, @DB;
$detect or print "Error : For the ID $examine NO-Entry could be Found !\n";


Parsing and saving JSON result from GoEuro API I am new to python so I decided to write a script that parses a json array and writes data in a file.

I wrote the code that does that but I think it can be done way better, So I was hoping to get feedback on how this code can be enhanced and more inline with python best practices.

from urllib2 import urlopen
import json

def get_json(url):
    response = urlopen(url)
    data = response.read().decode("utf-8")
    return json.loads(data)

url = ("http://api.goeuro.com/api/v2/position/suggest/en/Berlin")
json_array = get_json(url)

f = open('data.txt', 'w')

for x in json_array:
    gp = x["geo_position"]
    l = gp["latitude"]
    o = gp["longitude"]
    f.write(unicode(x["_id"]).encode("utf-8") +", " +     unicode(x["name"]).encode("utf-8") + ", " + unicode(x["type"]).encode("utf-8")+ ", " + unicode(l).encode("utf-8") + ", " + unicode(o).encode("utf-8") + unicode("\n").encode("utf-8"))

f.close()

Also, what is the best way for not having an empty new line at the end of the file ?

The Python documentation recommends using with when dealing with
files at the bottom of 7.2.1 of the Python tutorial, this has the benefit that you do not have to close the file yourself.
Although not necessary often a main function is made, so that the
other functions also can be used as a module. (6.1.1. Executing
modules as scripts)
The long line with the write statement can be split over multiple
lines, you do not need the line joining character (\) in this case
because, a closing bracket is still expected at the end of the
line.
The newline problem, and simular problems can be solved with the
join function of strings

I took the liberty to create an extra function and all and all i changed your code to:

from urllib2 import urlopen
import json

def get_json(url):
    response = urlopen(url)
    data = response.read().decode("utf-8")
    return json.loads(data)

def json_to_formatted_text(geo_data):
    formatted_lines = []
    for x in geo_data:
        gp = x["geo_position"]
        l = gp["latitude"]
        o = gp["longitude"]
        line = (unicode(x["_id"]).encode("utf-8")  + ", " +
                unicode(x["name"]).encode("utf-8") + ", " +
                unicode(x["type"]).encode("utf-8") + ", " +
                unicode(l).encode("utf-8")         + ", " +
                unicode(o).encode("utf-8")         +
                unicode("\n").encode("utf-8"))
        formatted_lines.append(line)
    return unicode("\n").encode("utf-8").join(formatted_lines)

def main():
    url = ("http://api.goeuro.com/api/v2/position/suggest/en/Berlin")
    json_array = get_json(url)
    out_text   = json_to_formatted_text(json_array)
    with open('data.txt', 'w') as f:
        f.write(out_text)

if __name__ == "__main__":
    main()


What does the Bob say? (clojure version) I'm doing the exercices from clojureacademy.
Here are the instructions for this one.

  Bob is a lackadaisical teenager. In conversation, his responses are very limited.

  Returns "Whatever." if given phrase is one of the following inputs:

  "Tom-ay-to, tom-aaaah-to."
  "Let's go make out behind the gym!"
  "It's OK if you don't want to go to the DMV."
  "Ending with ? means a question."
  "1, 2, 3"

  Returns "Woah, chill out!" if given phrase is one of the following inputs:

  "WATCH OUT!"
  "WHAT THE HELL WERE YOU THINKING?"
  "ZOMG THE %^@#$(^ ZOMBIES ARE COMING!!11!!1!"
  "1, 2, 3 GO!"
  "I HATE YOU"

  Returns "Sure." if given phrase is one of the following inputs:

  "Does this cryogenic chamber make me look fat?"
  "4?"

  Returns "Fine. Be that way!" if given phrase is one of the following inputs:

  ""
  " "

Here is my code in src/ex1_bob/core.clj:

(ns ex1-bob.core
  (:gen-class))

; responses
; =======================
(def whatever "Whatever.")
(def chill-out "Woah, chill out!")
(def sure "Sure.")
(def fine "Fine. Be that way!")

; triggers
; =======================
(def sure-triggers
  ["Does this cryogenic chamber make me look fat?" "4?"])
(def whatever-triggers
  ["Tom-ay-to, tom-aaaah-to."
   "Let's go make out behind the gym!"
   "It's OK if you don't want to go to the DMV."
   "Ending with ? means a question."
   "1, 2, 3"])
(def chill-out-triggers
  ["WATCH OUT!"
   "WHAT THE HELL WERE YOU THINKING?"
   "ZOMG THE %^*@#$(*^ ZOMBIES ARE COMING!!11!!1!"
   "1, 2, 3 GO!"
   "I HATE YOU"])

(defn has-phrase
  "return `true` if the given phrase is found in the collection, `false` otherwise"
  [phrase coll]
  (if (some #(= phrase %) coll)
    true
    false))

(defn response-for
  "return `true` if the given phrase is found in the collection, `false` otherwise"
  [phrase]
    (cond
      (has-phrase phrase whatever-triggers)  whatever
      (has-phrase phrase chill-out-triggers) chill-out
      (has-phrase phrase sure-triggers)      sure
      (= (clojure.string/trim phrase) "")    fine))

I also wrote some tests in test/ex1_bob/core_test.clj:

(ns ex1-bob.core-test
  (:require [clojure.test :refer :all]
            [ex1-bob.core :refer :all]))

; This does not work because `is` is a macro, and it seems it does not like to
; be used in a `map` or with `comp`.
;
; (deftest non-working-response-for-chill-out-triggers
;   (doall (map (comp is #(= % chill-out) response-for) chill-out-triggers)))

(deftest response-for-chill-out-triggers
  (is (every? #(= % chill-out) (map response-for chill-out-triggers))))

(deftest response-for-whatever-triggers
  (is (every? #(= % whatever) (map response-for whatever-triggers))))

(deftest response-for-sure-triggers
  (is (every? #(= % sure) (map response-for sure-triggers))))

(deftest response-for-empty-string
  (is (every? #(= % fine) (map response-for [""]))))

Two things make me a bit unhappy about this code:

the has-phrase function seems a bit silly. Do I really have to write a function to return true/false if an element is found in a collection? I feel that there must be a function that does this already
I first wrote my unit test with:

(deftest non-working-response-for-chill-out-triggers
  (doall (map (comp is #(= % chill-out) response-for) chill-out-triggers)))

The idea is to have one assertion for each item, which I assumed would give me a better error message in case of a failure (because I know which item failed). But it seems that is is a macro and somehow, the interpreter doesn't like this.

First, working top-down, your triggers at the top should be in sets (#{}), not vectors (you'll see why a little farther down):

(def sure-triggers
  #{"Does this cryogenic chamber make me look fat?"
    "4?"})

(def whatever-triggers
  #{"Tom-ay-to, tom-aaaah-to."
    "Let's go make out behind the gym!"
    "It's OK if you don't want to go to the DMV."
    "Ending with ? means a question."
    "1, 2, 3"})

(def chill-out-triggers
  #{"WATCH OUT!"
    "WHAT THE HELL WERE YOU THINKING?"
    "ZOMG THE %^*@#$(*^ ZOMBIES ARE COMING!!11!!1!"
    "1, 2, 3 GO!"
    "I HATE YOU"})

has-phrase has a couple things that can be improved:

Clojure ends predicates with a ? by convention. I'd rename it to has-phrase?.
some already returns a truthy/falsey value. (if pred? true false) is unnecessary.

Just reduce it to:

(defn has-phrase?
  "Return whether or not the given phrase is found in the collection"
  [phrase coll]
  (some #(= phrase %) coll))

BUT, now that you're using sets instead of vectors, this whole function is unnecessary:

(whatever-triggers "1, 2, 3")
=> "1, 2, 3" ; Truthy

(whatever-triggers "Not in the set")
=> nil

Sets themselves can be used as functions. If the item is in the set, they return the item back (a truthy result), else they return nil (falsey). This will also be much faster, since now you don't need to have some iterate potentially the entire vector (although obviously speed isn't a factor here). This changes reponse-for to:

(defn response-for
  "return `true` if the given phrase is found in the collection, `false` otherwise"
  [phrase]
  (cond
    (whatever-triggers phrase) whatever
    (chill-out-triggers phrase) chill-out
    (sure-triggers phrase) sure
    (= (clojure.string/trim phrase) "") fine))

This could be changed using some some trickery. some returns the first truthy value returned by its predicate. If you pass it pairs of [trigger-set response], you can iterate over the the pairs to find the first one that satisfies it, then return the response:

(defn response-for2 [phrase]
  (some (fn [[trigger-set response]]
          (when (trigger-set phrase)
            response))

        [[sure-triggers sure]
         [chill-out-triggers chill-out]
         [whatever-triggers whatever]
         [#{"" " "} fine]]))

Now, in this particular example, I wouldn't recommend doing this. It's not as clear as what you already had. I have used (some #(when ...)) before though, and it works great in some cases. I just thought I'd show you it here in case you run into a similar problem where this can be applied.

I'm sure something could be done using a map mapping triggers to responses, but I just did another Clojure review, and my brain's tapped out.


Nagios plugin webdav check 2 Original post: nagios plugin webdav check

According to the good advice I have received I've made some modifications to the code:

#!/bin/bash
#
# check a WebDAV server
# connect using a username and password and upload & delete a test file

#Exit codes
STATE_OK=0
STATE_WARNING=1
STATE_CRITICAL=2
STATE_UNKNOWN=3

FILENAME=test_monitor
FQFN=/tmp/$FILENAME

# Verify the type of input and number of values
# Display an error message if the (input) is not correct
# Exit the shell script with a warning status
[ $# -ne 4 ] && { echo "Usage: $0 <hostname> <username> <password> <webdav_dir>"; exit $STATE_UNKNOWN; }

HOSTNAME=$1
USERNAME=$2
PASSWORD=$3
WEBDAVDIR=$4
BASEURL=http://$HOSTNAME/$WEBDAVDIR

# create temp file
truncate -s 1M $FQFN
if [ ! -f $FQFN ]; then
    echo "Test file not found!"; exit $STATE_UNKNOWN;
fi

# check if curl is installed
command -v curl >/dev/null 2>&1 || { echo >&2 "Curl is not installed. Script cannot run."; exit $STATE_UNKNOWN; }

function _curl {
curl --fail -u $USERNAME:$PASSWORD "$@"
}

# connect to server and upload file
_curl -s -T $FQFN $BASEURL/ > /dev/null

status=$?

case $status in
    0)
        echo "OK - File uploaded on WebDAV Server: $HOSTNAME"
        # remove test file
        _curl -X DELETE $BASEURL/$FILENAME
        exit $STATE_OK
        ;;
    1)
        echo "CRITICAL - Cannot upload file on $HOSTNAME"
        exit $STATE_CRITICAL
        ;;
    6)
        echo "CRITICAL - Could not resolve host $HOSTNAME"
        exit $STATE_CRITICAL
        ;;
    26)
        echo "CRITICAL - Cannot open $FQFN for upload"
        exit $STATE_CRITICAL
        ;;
    22)
        echo "CRITICAL - The requested URL returned error"
        exit $STATE_CRITICAL
        ;;
    127)
        echo "CRITICAL - Command not found"
        exit $STATE_CRITICAL
        ;;
    *)
        echo "UNKNOWN - "
        exit $STATE_UNKNOWN
esac

Do you have any more recommendations regarding syntax or script functionality?

I am thinking about the removal of the test file. It is done if the upload is successful, but can it be assumed that if the upload is successful, the delete will be also? In my tests this has always been the case.

Is there a chance for the delete command to fail? And should I modify the script somehow to accommodate for that?

Thanks for incorporating the various suggestions and posting a new code review.  This is looking really excellent.  Some nits and answers to your concerns:

creating a function for curling is good.  "$@" is the right choice as well.  But it would be nice if the stuff within squiggly brackets was indented like:

function _curl {
    curl --fail -u $USERNAME:$PASSWORD "$@"
}

I would stick with deleting the temporary file each time so the test is complete each time.  One scenario that might happen would be for the disk to fill up, and so no WebDAV writes are possible, but your test succeeds because it has the space of the existing file to work with or maybe there's code so it doesn't rewrite the file for performance reasons.  Either way leads to a false negative check which is best avoided.
It would be more thorough and nice to check the return value from the delete as well.  Doing this with a nested case statement could get unwieldy and certainly duplicates a lot of code.  If you can turn the case statement into its own function that could bucket the return values for creates or deletes that might be easier to maintain.  There might be other nagios checks where you want to check the output of curl similarly which would be good to keep in mind when designing this curl return evaluator.
Aren't you linking to the wrong prior question?  nagios plugin webdav check was how I got here.  :)


Import Excel, unpivot data, output tab-delimited for SQL Import I have a client GL file (Excel) that needs to be regularly imported to SQL Server (as tab-delimited txt).

Sample (simplified):

PayCode  PayDesc   Dept1   Dept2   Dept3
========================================
REG      Regular   00001   00001   00002
VAC      Vacation  00011   00012   00013
HOL      Holiday   00021   00022   00022

However, the number of columns (departments) may change over time, and SQL Server doesn't support importing files with an uncertain column count. So rather than recode the SQL import each time a change occurs, I decided to "unpivot" the data prior to import, so the resulting table structure will always be static:

PayCode  PayDesc    DeptCode  GLCode
====================================
REG      Regular    Dept1     00001
REG      Regular    Dept2     00001
REG      Regular    Dept3     00002
VAC      Vacation   Dept1     00011
VAC      Vacation   Dept2     00012
VAC      Vacation   Dept3     00013
HOL      Holiday    Dept1     00021
HOL      Holiday    Dept2     00022
HOL      Holiday    Dept3     00022

I've got the script functioning, but being a PS lightweight, I thought I would solicit for enhancement ideas. I feel like I'm missing good pipeline opportunities due to inexperience.

Code updated using feedback from briantist

$srcDir = "C:"
$srcTab = "Sheet1"
$srcFile = Join-Path -Path $srcDir -ChildPath "ClientGL.xlsx"
$outFile = Join-Path -Path $srcDir -ChildPath "ClientGL.txt"
$delim = "`t"

## Export from Excel to tab-delimited .txt
Write-Host "Launch Excel"
$Excel = New-Object -comobject Excel.Application
$Excel.Visible = $False
$Excel.displayalerts = $False
Write-Host "Open $srcFile"
$Workbook = $Excel.Workbooks.Open($srcFile)
$Workbook.Worksheets.Item($srcTab).Select()
Write-Host "Export $outFile as Tab-Delimited TXT"
$Workbook.SaveAs("$outFile",-4158)
Write-Host "Quit Excel"
$Excel.Quit()
If(Get-Process -name excel){Stop-Process -name excel}

## Import data and headers
Write-Host "Import $outFile data"
$inData = Import-Csv -Path $outFile -Delimiter $delim
Write-Host "Import $outFile headers"
$inHeaders = (Get-Content -Path $outFile -TotalCount 1) -split $delim

## Build output datatable
Write-Host "Build output datatable"
$outData = New-Object System.Data.DataTable
[Void]$outData.Columns.Add("PayCode",[string])
[Void]$outData.Columns.Add("PayDesc",[string])
[Void]$outData.Columns.Add("DeptCode",[string])
[Void]$outData.Columns.Add("GLCode",[string])

## Unpivot Department columns, and add results to datatable
Write-Host "Unpivot each Dept column and add data to datatable"
for($col=2; $col -lt $inHeaders.Count; $col++)
{
    $dept = $inHeaders[$col]  #current $col header name
    foreach($row in $inData)
    {
        $dr = $outData.NewRow();
        $dr.Item("PayCode") = $row.PayCode
        $dr.Item("PayDesc") = $row.PayDesc
        $dr.Item("DeptCode") = $dept  #header name
        $dr.Item("GLCode") = $row.$dept  #row.col value
        $outData.Rows.Add($dr)
    }
}

## Output modified file
Write-Host "Output modified $outFile"
$outData | Export-Csv -Path $outFile -Delimiter $delim -notypeinformation -Force
Write-Host "Remove quotes from file"
(Get-Content -Path $outFile) | ForEach-Object {$_ -replace '"', ""} | Out-File -FilePath $outFile -Force

Path Joining

$srcDir = "C:"
$srcFile = "$SrcDir\ClientGL.xlsx"
$srcTab = "Sheet1"
$outFile = "$SrcDir\ClientGL.txt"

Although it may seem unnecessary in this case, you should know that Join-Path exists.

$srcDir = "C:"
$srcFile = $SrcDir | Join-Path -ChildPath "ClientGL.xlsx"
$srcTab = "Sheet1"
$outFile = Join-Path $SrcDir "ClientGL.txt"

The nice thing about using this (again, unnecessary here) is that it takes care of leading and trailing path separators, so the following:

Join-Path "C:\" "\Windows"
Join-Path "C:" "Windows"

Will both produce C:\Windows. That makes it great for user input.

Write-Host vs Write-Verbose

This can be debated, but some (myself included) take a cautious approach with Write-Host. The reason is that it forces the output to the screen, instead of letting the caller decide whether they want status messages.

Ultimately, you take this on a case-by-case, and sometimes Write-Host is the right tool, and maybe it is for you in this case.

Using Write-Verbose instead, you would normally not see the output, unless you call your script with -Verbose, and then you will see it.

To add support for this, you need to add a parameter block to your script, even if you aren't going to take any parameters.

Top of script:

[CmdletBinding()]
param()

That will automatically add support for -Verbose.

Aliases

If(ps excel){kill -name excel}

and

(Get-Content $outFile) | % {$_ -replace '"', ""} | Out-File -FilePath $outFile -Force

ps is an alias for Get-Process.
kill is an alias for Stop-Process.
% is an alias for ForEach-Object.

I love PowerShell's aliases, but when it comes to writing a script that's meant to be re-used often or might ever be read by other people, I am of the school that one should use the full cmdlet/function name, and use full parameter names.

I use aliases when I'm using PowerShell as a shell and typing commands to run immediately, or to quickly prototype something.

For me it's a matter of clarify and readability, but this is of course personal preference (I have met at least one person who feels differently and uses aliases everywhere).

Reading the first line of a file

$inHeaders = (Get-Content $outFile | Select-Object -First 1).Split("`t")

If you're using PowerShell 2.0, the above line will read every line of the file, and even though Select-Object will only give you back the first 1, it has no way of stopping the pipeline. If your file is large, this could take a very long time.

If you're using PowerShell 3+, then the above will actually only read the first line because Select-Object will stop the pipeline.

Note that Get-Content has a -TotalCount parameter though, so you can use:

$inHeaders = (Get-Content $outFile -TotalCount 1).Split("`t")

and it will read just the first line.

.Split() vs -Split

$inHeaders = (Get-Content $outFile | Select-Object -First 1).Split("`t")

Here you're using the .Split() method of a [String]. PowerShell also has a -Split operator. The operator has a lot of nifty features that from what I've seen no one ever uses, because they don't know they exist or (in my case) they always forget about them when needed.

But the main difference that you probably will remember is that the operator takes a regular expression, and the method takes a string.

There's nothing wrong with either, just wanted you to be aware of the differences.

Accepting Parameters

$srcDir = "C:"
$srcFile = "$SrcDir\ClientGL.xlsx"
$srcTab = "Sheet1"
$outFile = "$SrcDir\ClientGL.txt"

Consider making these parameters that can be passed to the script. You could also make them have defaults that way.

Top of script:

[CmdletBinding()
param(
    [Parameter(
        Mandatory=$false
    )]
    [String]
    $Path = "C:" ,

    [Parameter(
        Mandatory=$false
    )]
    [String]
    $srcTab = "Sheet1"
)

$srcFile = "$Path\ClientGL.xlsx"
$outFile = "$Path\ClientGL.txt"

Now you can call the script like so:

.\MyScript.ps1 -Path "C:\Some\Other\Path\" -srcTab "Expenses"

Now you can see why using Join-Path would save you some headache because you're accepting user input!

Just to break down this param() block a bit:

[Parameter()] can take multiple comma separated options; I've shown you Mandatory. Mandatory means mandatory to be supplied by the caller not to have a value, so when you use a default value like we are here, you don't want to make the parameter mandatory. Parameters are by default optional, so this was unnecessary; I was just demonstrating it.

The entire [Parameter()] attribute is also optional, and could be left out.

[String] is specifying the data type for the parameter. PowerShell will try to coerce input values into this type if possible. Data type is also optional.

$Path: this is the name of the parameter. It is the only part that's mandatory. I used $Path instead of $srcDir because PowerShell has conventions for certain names to represent certain things.

So generally any cmdlet or function that accepts a path will use -Path. Same for -ComputerName, -Identity, -Credential.

Also note that all of these pieces could have been on a single line. I like to separate them for clarity.

For more on these, have a look at Create a Truly Advanced PowerShell Function and the about_Functions_Advanced and about_Functions_Advanced_Parameters help topics.


Dynamically generating a list of events This is a simple list of items in a list, which allows the user to dynamically generate that list of events. Then a controller action does the work of serializing that into the database.

The issue is that there's the PHP-generated HTML segment, and there's a separate JavaScript segment to do the additions (for when someone presses the "add new page" button). This is not only duplicated; when it's done inline in the JavaScript it's extremely ugly to look at (look at the length of that line!).

Is there a better way of doing this?

<?php
/** @var $this Zend_View */

$this->headLink()->appendStylesheet($this->baseUrl('css/redmond/jquery-ui-1.8.5.custom.css'));

$this->headScript()->appendFile($this->baseUrl('js/jquery.js'));
$this->headScript()->appendFile($this->baseUrl('js/jquery-ui-1.8.5.custom.min.js'));

$this->headScript()->captureStart(); ?>
//
<?php $this->headScript()->captureEnd(); ?>

    <form action="<?= $this->url(array('controller' => 'admin', 'action' => 'applyEvents')) ?>" method="post">

                Add a new Event

            <? foreach ($this->events as $event) { ?>

                    <input name="dates[]" class="datebox" style="width: 120px;" type="text" value="<?= $event->GetDate()->format('Y-m-d') ?>" />

                        <input name="contents[]" type="text" value="<?= $event->GetMessage() ?>" style="width: 100%;" />

            <? } ?>

        <input type="submit" name="submit" value="Apply" />
        <input type="submit" name="submit" value="Cancel" />
    </form>

I'm assuming from looking the code you're using Zend Framework. I've solved similar issues in Zend and Symfony using php partials.

You could create a partial _list_item.phtml

        <input name="dates[]" class="datebox" style="width: 120px;" type="text" value="<?php echo $date ?>" />

        <input name="contents[]" type="text" value="<?php echo $message ?>" style="width: 100%;" />

Then in your view:

<?php foreach ($this->events as $event): ?>
    <?php echo $this->partial('list_item.phtml', array(
        'date' => $event->GetDate()->format('Y-m-d'),
        'message' => $event->GetMessage(),
    )); ?>
<?php endforeach; ?>

Then for your javascript:

function AddNewEvent()
{
    var today, html, temp;
    // call str_replace because javascript doesn't like new lines in strings
    html = '<?php echo str_replace(PHP_EOL, "\n", $this->partial('list_item.phtml', array(
        'date' => $event->GetDate()->format('Y-m-d'),
        'message' => '',
    ))); ?>';
    $(this).after(html);
    temp = $(this).next();
    CreateDateboxes($('.datebox', temp));
    $('.ui-icon-trash', temp).click(RemoveParent);
}

Viola! No template duplication because the affected code is in a partial. No excess javscript templating libraries, although if this kind of thing is prevalent in your application I'd suggest some refactoring and implementing such a templating library.

This approach will have the least impact on your work flow, little to no learning curve, and a short investment in development time for big decrease duplication (bosses love hearing that kind of thing).


A shorter way for getting variables I have the following piece of code in my project, and that a few times:

$this->uniqueSessionKey = $settings->get('uniqueSessionKey') ? $settings->get('uniqueSessionKey') : $_SERVER['HTTP_USER_AGENT'] . time();

I was wondering if there's a better way of writing this, especially the part where it checks if $settings->get(...) is not false and then uses the previously checked value. As it uses a database connection it's probably not very efficient checking for the same thing twice.

Is there a better way of writing this? I'm already using the shortest if-else that I'm aware of.
If it's not possible, I will put the $settings->get(...) in a variable anyway, and then check if it's false. I've tried using || but that returned false (?) so I probably did that wrong.

Note that I don't want to check it for multiple values, so I don't need to use for etc.

Since PHP 5.3 you can use the shortcut $a = $b? : $c:

$this->uniqueSessionKey = $settings->get('uniqueSessionKey') ?  : $_SERVER['HTTP_USER_AGENT'] . time();

--

Just a note: you can not use this if you want check if the var $a is set. In this case you must use the original form: $a = isset($b)? $b : $c;

Otherwise, you can write a custom function in this way:

function getIfSet(&$a, $default = null) {
      return isset($a)? $a : $default;
}

$a = getIfSet($b,$c);

Passing var by reference (&$a) avoid the PHP notice if $a is not set.


Make Simultaneous HTTP Requests That Resolve Into a Single Return This week, in an ongoing job search, I was tasked with the following pre-screening question:

  Make two simultaneous http requests. Use the language and library of your choice. Make sure the results are combined and resolve into a single return.

For this I chose JavaScript and jQuery, culminating in the following Gist and jsFiddle.

fiddle.js:

var gistID = '275cd1b76574497327761b33fb08ad4d';

function Source(url){
 this.url = url;
};

var source = new Source("/gh/gist/response.html/" + gistID + "/");
var sources = [source,source];

function asyncGroup() {

 var promises = [];

 sources.forEach(function(source,index,sources){

  var deferred = jQuery.Deferred();

  promises.push(deferred.promise());

  $.ajax({url: source.url, processData: false}).always(function(data){
   sources[index].element = '<img src="data:image/png;base64,' + data + '"/>';
   deferred.resolve();
  });
 });
 return promises;
}

$.when.apply(null, asyncGroup() ).done(function( status ) {

 sources.forEach(function(source){$("#myDiv").append($(source.element));});
 $("#myDiv").show();
});

One thing I would like to change is to accredit Fiddle Author Casiano Rodriguez-Leon for their base64 testing image, provided it remains in the implementation. Perhaps this is best done in fiddle.manifest, I am not sure.

Beyond that, I know that much can be improved, and I'm all ears.

Compatibility aside, you can skip jQuery. Everything you need is present in vanilla JS. jQuery AJAX can be replaced by fetch and deferreds can be replaced by native Promise. As an added bonus, use template literals to easily construct strings.

// Work on the very basic data, the IDs
const ids = [
  '275cd1b76574497327761b33fb08ad4d',
  '275cd1b76574497327761b33fb08ad4d'
];

// Map IDs into urls
const urls = ids.map(id => `/gh/gist/response.html/${id}/`);

// Construct a function that transforms the Response object into an img
const responseToImage = res => `<img src="data:image/png;base64,${res.text()}"/>`;

// For each, send request, convert response to img, and store promise in array
const promises = urls.map(url => fetch(url).then(responseToImage));

// Wait until all promises resolve, then append image
Promise.all(promises).then(images => {
  const container = $('#myDiv').show();
  images.forEach(image => container.append(image));
});

As for your code, a few things

var gistID = '275cd1b76574497327761b33fb08ad4d';

function Source(url){
 this.url = url;
};

var source = new Source("/gh/gist/response.html/" + gistID + "/");
var sources = [source,source];

This code doesn't look scalable. If say I wanted to add more images, different images, there is no way for this code to do that without having to modify several lines of code. In this case, gistId has to be abandoned, you'd have to write new Source(...) for each item, and update the sources array.

Also, insider tip: It's a bad sign if a really simple code update results in modification of several lines. Also, writing code this way results to bad habits which can hurt in the long run.

function asyncGroup() {

 var promises = [];

 sources.forEach(function(source,index,sources){

  var deferred = jQuery.Deferred();

  promises.push(deferred.promise());

  $.ajax({url: source.url, processData: false}).always(function(data){
   sources[index].element = '<img src="data:image/png;base64,' + data + '"/>';
   deferred.resolve();
  });
 });
 return promises;
}

The $.ajax function already returns a Deferred-like, jqXHR object. You don't need to "wrap" it in a Deferred, this is already redundant. You can simply push its return value to the array. Additionally, when doing 1:1 conversion from one array to another, array.map is the function you want. With these, you can remove all that boilerplate code to transform source.url to an array of deferreds.

$.when.apply(null, asyncGroup() ).done(function( status ) {
 sources.forEach(function(source){$("#myDiv").append($(source.element));});
 $("#myDiv").show();
});

You can cache the result of $('#myDiv') in a variable so that you don't have to call it repeatedly. Additionally, jquery.append accepts HTML strings as argument. No need to convert it first.


Project Euler #4 "Largest Palindrome product" in Python Given

  A palindromic number reads the same both ways. The largest palindrome made from the product of two 2-digit numbers is 9009 = 91 × 99.

  Find the largest palindrome made from the product of two 3-digit numbers.

Solution

import time

def reverse(num):
    dreverse = 0
    while (num > 0):
        dreverse = dreverse * 10 + num % 10
        num /= 10
    return dreverse

def is_palindrome(num):
    if (num < 0):
        raise ValueError('Non-negative integers only')
    return num == reverse(num)

def main():
    largest_palindrome = 0
    lower = 999
    while lower >= 100:
        upper = 999
        while upper >= lower:
            if upper * lower <= largest_palindrome:
                break
            if is_palindrome(upper * lower):
                largest_palindrome = upper * lower
            upper -= 1
        lower -= 1
    return largest_palindrome

start_time = time.time()
print main()
print("--- %s seconds ---" % (time.time() - start_time))
o/p: 906609
--- 0.0150721073151 seconds ---

I am including timing metric from now on. This solution seems \$O(N^2)\$. Can I improve it further?

You are recomputing the product upper * lower three times:

    while upper >= lower:
        if upper * lower <= largest_palindrome:
            break
        if is_palindrome(upper * lower):
            largest_palindrome = upper * lower

You should save it in a variable:

    while upper >= lower:
        product = upper*lower
        if product <= largest_palindrome:
            break
        if is_palindrome(product):
            largest_palindrome = product

Also, how about this version of is_palindrome:

def is_palindrome(n):
  s = str(n)
  return (s == s[::-1])

Update

Replace your loops with for and xrange:

largest = 0
for lower in xrange(999,99,-1):
    for upper in xrange(999,lower-1,-1):
        ...

It makes the intent of your code a lot clearer, and it is easier to correctly write the loop.

(The following is note quite right)

Finally, if upper*lower <= largest_palindrome, you actually return from the function:

        if upper * lower <= largest_palindrome:
            return largest_palindrome

The return will essentially break out of both while loops. Using just break will only break out of the inner loop. Of course, the remaining cases will execute very quickly since they will only last one iteration of the inner loop, but it is still a good optimization to know about.


Singleton with a volatile and synchronized instance I created a singleton to have a unique instance of ExecutorService in my application.

I whould like to know which is the better of the two classes below.

First one :

public class ExecutorServiceSingleton {
    private static final Logger LOG = LoggerFactory.getLogger(ExecutorServiceSingleton.class);
    private ScheduledExecutorService scheduledExecutorService;

    private static ExecutorServiceSingleton instance;

    private ExecutorServiceSingleton() {
        scheduledExecutorService = Executors.newScheduledThreadPool(1);
    }

    /**
     * Method to run the task
     */
    public void runTask(Runnable task) {
        scheduledExecutorService.submit(task);
    }

    /**
     * Get the instance : If the instance is null, then we return a new one. If the instance is diff than null, then, we use the instance value and we return it.
     *
     * @return the instance
     */
    public static ExecutorServiceSingleton getInstance() {
        if (instance == null) {
            instance = new ExecutorServiceSingleton();
        }
        LOG.info("Instance: " + instance);
        return instance;
    }
}

Second one :

public class ExecutorServiceSingleton {
    private static final Logger LOG = LoggerFactory.getLogger(ExecutorServiceSingleton.class);
    private ScheduledExecutorService scheduledExecutorService;

    private volatile static ExecutorServiceSingleton instance;

    private ExecutorServiceSingleton() {
        scheduledExecutorService = Executors.newScheduledThreadPool(1);
    }

    /**
     * Method to run the task
     */
    public void runTask(Runnable task) {
        scheduledExecutorService.submit(task);
    }

    /**
     * Get the instance : If the instance is null, then we return a new one. If the instance is diff than null, then, we use the instance value and we return it.
     *
     * @return the instance
     */
    public static ExecutorServiceSingleton getInstance() {
        if (instance == null) {
            synchronized (ExecutorServiceSingleton.class) {
                if (instance == null) {
                    instance = new ExecutorServiceSingleton();
                }
            }
        }
        LOG.info("Instance: " + instance);
        return instance;
    }
}

The memory model of the jvm doesn't allow double checked locking like that to work like you think it does. The jvm is allowed to assign the object before running the constructor.

In java class loading is already lazy and perfectly thread safe. So the naive

private static final ExecutorServiceSingleton instance = new ExecutorServiceSingleton();

is fine for most purposes.

If you really want the lazy ness to go further then something like:

private static class Nested{
    private static final ExecutorServiceSingleton instance = new ExecutorServiceSingleton();
}

public static ExecutorServiceSingleton getInstance() {

    return Nested.instance;
}

will do what you need.


Creating user accounts as an administrative user I have a site where user accounts can be created in two ways: by new users (sign up) and by admin users. Since the creation is handled differently in each case, I decided to split the logic into two separate functions. I have a working implementation but I'm pretty new to Rails so I don't know if it is robust and secure. I am looking for feedback on how to improve it.

users_controller.rb

before_action :signed_in_user, only: [:home, :edit, :update, :destroy]
before_action :correct_user, only: [:edit, :update]
before_action :admin_user, only: [:create_by_admin, :destroy]

.
.
.

def new
    if !signed_in?
        @user = User.new
        @user.confirmation_code = User.new_confirmation_code
    elsif signed_in? && current_user.admin?
        @user = User.new
        render 'new_by_admin' # basically a new.html.erb but with a dropdown for the admin setting
    else
        redirect_to root_url
    end
end

def create
    if signed_in? && !current_user.admin?
        redirect_to root_url
    elsif signed_in? && current_user.admin?
        create_by_admin
    else
        create_by_user
    end
end

def create_by_user
    @user = User.new(user_params)
    if @user.save
        UserMailer.confirmation_email(@user).deliver
        flash[:success] = "Great! Now check your email for activation instructions."
        redirect_to root_url
    else
        render 'new'
    end
end

def create_by_admin
    @user = User.new(admin_params)
    if @user.save
        @user.update_attributes(confirmed: true)
        flash[:success] = "User for " + @user.name + " created."
        redirect_to users_path
    else
        render 'new_by_admin'
    end
end

.
.
.

private
    def user_params
        params.require(:user).permit(:name, :email, :password, :password_confirmation)
    end

    def admin_params
        params.require(:user).permit(:name, :email, :password, :password_confirmation, :admin)
    end

Essentially, all create requests are routed to the create action, and from there sent to the appropriate method (depending on whether the requesting user is a new user or an existing user who is an admin). The differences between the methods is that new users receive an email to activate their accounts upon signup and cannot set themselves as admin, whereas admin users create accounts that are already confirmed and can set them as admin.

Is this a silly way to do it? Should I create an admin_controller that handles all administrative requests?

It's fine to create 2 methods to break up the logic a bit - however, there are other things I'd look into before tackling the user vs admin stuff:

Firstly: Ruby style prescribes 2-spaces indentation - not tabs, not 4 spaces.

Second, if you're going to use them, make those create_by* methods private. Don't mix 'em with the regular controller actions.

Third: String interpolation:

flash[:success] = "User for #{@user.name} created."

But success is an uncommon flash key. You can use the default notice message type directly in your redirect

redirect_to root_url, notice: "User for #{@user.name} created."

If you absolutely want to use success, you can use add_flash_types to add it, and use redirect_to ..., success: '...'.

And then you have this:

render 'new_by_admin' # basically a new.html.erb but with a dropdown for the admin setting

Don't use two templates - that's a giant hassle to maintain. Use a conditional in the view itself to show or not show the dropdown. I'd also add an admin? helper like this

# in application_controller.rb
def admin?
  current_user.try(:admin?)
end
helper_method :admin?

(I'm assuming that current_user is nil if you're not signed in).

So now you can check for admins in one go rather than both check signed_in? and then check current_user.admin?

In the new view you get this:

<% if admin? %>
  your dropdown
<% end %>

And you don't need to 2 templates. Your if..elsif..else in #create can also become the more readable:

if admin?
  ...
elsif signed_in? # regular user
  ...
else
  ...
end

Your code also makes me wonder how you handle passwords. If an admin creates a user, I imagine you'll want to send login instructions and password to the new user.

Right now, it looks like the admin picks a password for other people, but you should already have a "reset password" method (for users who've forgotten their password) that will create a random password and email it to the user. It shouldn't be the admin's job to make up passwords.


Map rental cost strategy It is the case where data contains some field indicating what type of strategy should be picked.

I mean, I often ignore solving the problem like shown below and duplicate the problems that strategy pattern is out to solve in the first place. Is there a pattern for it or is it not viable at all in this case?

Recent example on how I have solved it:

public class MapRentalCostStrategy : IMapper<EquipmentType, IRentalCostStrategy>
{
    private readonly Dictionary<EquipmentType, IRentalCostStrategy> _knownRentalCostStrategy;

    public MapRentalCostStrategy(IMutablePriceConfiguration mutablePriceConfiguration)
    {
        _knownRentalCostStrategy = new Dictionary<EquipmentType, IRentalCostStrategy>
        {
            { EquipmentType.Heavy, new HeavyRentalCostStrategy(mutablePriceConfiguration)},
            { EquipmentType.Specialized, new SpecializedRentalCostStrategy(mutablePriceConfiguration)},
            { EquipmentType.Regular, new RegularRentalCostStrategy(mutablePriceConfiguration)}
        };
    }

    public Func<EquipmentType, IRentalCostStrategy> Create => equipment =>
    {
        if (!_knownRentalCostStrategy.ContainsKey(equipment))
            throw new ArgumentException();

        return _knownRentalCostStrategy[equipment];
    };
}

where EquipmentType is an enum.

Maybe I should inject a service that returns this dictionary? In any case, it seems I am delegating the responsibility (in this case its "object creation", it should not be mappers responsibility to deal with the instantiation of the dictionary).

Maybe I am on the wrong track? Is there a better way?

I think a singleton/enum like pattern would be beneficial. The curious part is the IMutablePriceConfiguration object seems to vary based on the situation.

Remove the IMutablePriceConfiguration as a dependency for the cost strategy objects.

Next:

public sealed class EquipmentType
{
    public static readonly Heavy = new EquipmentType(1, "Heavy", new HeavyRentalCostStrategy());
    public static readonly Specialized = new EquipmentType(2, "Specialized", new SpecializedRentalCostStrategy());
    public static readonly Regular = new EquipmentType(3, "Regular", new RegularRentalCostStrategy());

    public static readonly IEnumerable<EquipmentType> All = new EquipmentType[]
    {
        Heavy,
        Specialized,
        Regular
    };

    public static EquipmentType Find(int id)
    {
        return All.SingleOrDefault(e => e.Id == id);
    }

    public static EquipmentType Find(string name)
    {
        return All.SingleOrDefault(e => e.Name == name);
    }

    private IRentalCostStrategy CostStrategy { get; set; }
    public string Name { get; private set; }
    public int Id { get; private set; }

    private EquipmentType(int id, string name, IRentalCostStrategy costStrategy)
    {
        Id = id;
        Name = name;
        CostStrategy = costStrategy;
    }

    public double CalculateRentalCost(IMutablePriceConfiguration mutablePriceConfiguration)
    {
        return CostStrategy.CalculateRentalCost(mutablePriceConfiguration);
    }
}

The IMutablePriceConfiguration becomes an argument to the CalculateRenderCost method on the EquipmentType class
The EquipmentType class has a private constructor so you are guaranteed to only have those three instances available. No one can incorrectly (or maliciously) create equipment types and their cost strategies.
You can access each equipment type in an enum-like syntax:

EquipmentType.Regular

You can find by id or name:

EquipmentType.Find(1);
EquipmentType.Find("Specialized");

You can blindly loop over all available equipment types:

foreach (var equipmentType in EquipmentType.All)
{
    // ...
}

As long as you have an IMutablePriceConfiguration object, you can blindly call the CalculateRentalCost method and get a result back. No need for ifs or switchs peppered all over your code.


Stable reversed Tartaglia's triangles Inspired by Minimising the triangle

I am writing a fully tested program to solve the following problem:

  A triangle needs a good foundation. Every row in the triangle is derived from the sum of the two values below it. However, there can be
  no repeated values, if a value shows up more than once the triangle
  crumbles. Find the base which minimises the value in the top of the
  triangle satisfying the condition of no duplicates.

Example:

      20
    8   12
 [3]   5   7
1   2  [3]  4

Here 3 occurs twice, so the triangle is invalid.

My main concern is that my code has time complexity \$O(n!)\$ in the function stable_triangles, so it is not scalable at all.

"""
Finds `Reversed Tartaglia's (or Pascals') triangles` where there are
no repeated values.
"""

import doctest
from itertools import permutations

def next_level(level):
    """
    Builds the layer above of the triangle by
    summing the near pairs of numbers.

    >>> next_level([1, 4, 5])
    [5, 9]
    """
    return [curr + level[index + 1]
               for index, curr in enumerate(level[:-1])]

def make_triangle(base):
    """
    Builds a full triangle by `next_level` until the
    top has length 1.

    >>> make_triangle([1,2,3])
    [[1, 2, 3], [3, 5], [8]]
    """
    triangle = [base]
    while len(triangle[-1]) > 1:
        triangle.append(next_level(triangle[-1]))
    return triangle

def all_unique(lst):
    """
    Are the items all unique?

    >>> all_unique([4, 6, 7])
    True
    >>> all_unique([4, 4, 6, 7])
    False
    """
    return len(lst) == len(set(lst))

def flatten1(lst):
    """
    Flattens one level, shallow.

    >>> flatten1([ [1,2], [3,4] ])
    [1, 2, 3, 4]
    """
    return [i for sublst in lst for i in sublst]

def is_stable(triangle):
    """
    A triangle is stable if no number is repeated,
    that is: all the numbers are unique.

    >>> is_stable([[1,2,3], [3,5], [8]])
    False
    >>> is_stable([[1,2], [2]])
    False
    >>> is_stable([[1,3], [4]])
    True
    """
    return all_unique(flatten1(triangle))

def stable_triangles(base_length):
    """
    Yields many stable triangles with the given base length.

    >>> flattend_sum = lambda tri: sum(flatten1(tri))
    >>> min(stable_triangles(3), key=flattend_sum)
    [[2, 1, 4], [3, 5], [8]]
    >>> min(stable_triangles(4), key=flattend_sum)
    [[2, 3, 1, 6], [5, 4, 7], [9, 11], [20]]
    >>> min(stable_triangles(5), key=flattend_sum)
    [[6, 4, 1, 2, 7], [10, 5, 3, 9], [15, 8, 12], [23, 20], [43]]
    >>> next(stable_triangles(6))
    [[1, 2, 4, 7, 5, 8], [3, 6, 11, 12, 13], [9, 17, 23, 25], [26, 40, 48], [66, 88], [154]]

    # >>> next(stable_triangles(7)) # This takes about a minute. Comment out for faster testing.
    # [[1, 2, 4, 7, 5, 8, 10], [3, 6, 11, 12, 13, 18], [9, 17, 23, 25, 31], [26, 40, 48, 56], [66, 88, 104], [154, 192], [346]]
    """
    for base in permutations(range(base_length * 2), base_length):
        triangle = make_triangle(list(base))
        if is_stable(triangle):
            yield triangle

if __name__ == "__main__":
    doctest.testmod()
    for base_length in range(10):
        print(next(stable_triangles(base_length)))

This code is written very idiomatically, so I don't have any style points.

For speed, the first thing to do is use PyPy. That gives a 5x throughput improvement.

Then one should consider your algorithm. A quick run with cProfile gives

   Ordered by: internal time

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
  8420076    1.231    0.000    1.231    0.000 p.py:5(<listcomp>)
  1414943    0.966    0.000    0.992    0.000 p.py:14(all_unique)
  1414943    0.557    0.000    2.462    0.000 p.py:8(make_triangle)
  1414943    0.504    0.000    0.504    0.000 p.py:18(<listcomp>)
  8420076    0.477    0.000    1.708    0.000 p.py:4(next_level)
        8    0.275    0.034    4.347    0.543 p.py:23(stable_triangles)
  8421251    0.114    0.000    0.114    0.000 {method 'append' of 'list' objects}
12667944/12667894    0.109    0.000    0.109    0.000 {built-in function len}
  1414943    0.077    0.000    1.610    0.000 p.py:20(is_stable)
  1414943    0.037    0.000    0.541    0.000 p.py:17(flatten1)
        1    0.020    0.020    4.466    4.466 p.py:1(<module>)

This means that next_level is taking a considerable amount of time. I noticed it's better written

[left + right for left, right in zip(level[:-1], level[1:])]

but this doesn't change speed much.

The first useful thing I would consider is interspersing generation and testing to prevent many calls to next_level. This also prevents the need to keep the whole triangle - one only needs the base and current row. This can be done by making triangle yield rows instead.

from itertools import permutations

def next_level(level):
    return [left + right for left, right in zip(level[:-1], level[1:])]

def make_triangle(base):
    row = base

    while row:
        yield row
        row = next_level(row)

def is_stable(triangle):
    seen = set()

    for row in triangle:
        if not seen.isdisjoint(row) or len(row) != len(set(row)):
            return False
        seen.update(row)

    return True

def stable_triangles(base_length):
    for base in permutations(range(base_length * 2), base_length):
        if is_stable(make_triangle(base)):
            yield make_triangle(base)

This is pretty fast comparatively. Not fast enough, though!

Here's where we get to the really interesting parts.

Consider some unique triangle:

    9
  3   6
1   2   4

and the next

     26
    9  17
  3   6  11
1   2   4   7

It's not a coincidence that one is a superset of another. In fact, is must be so. For any non-trivial unique triangle, there are subsets you can find by dropping the left or right hand side.

So consider building them up in this manner, filtering as early as possible. One problem is that we need a value limit to prevent infinite recursion. Luckily, I can choose the same rule you already have to avoid changing behaviours:

def next_level(level):
    return [left + right for left, right in zip(level[:-1], level[1:])]

def make_triangle(base):
    row = base

    while row:
        yield row
        row = next_level(row)

def is_stable(triangle):
    seen = set()

    for row in triangle:
        if not seen.isdisjoint(row) or len(row) != len(set(row)):
            return False
        seen.update(row)

    return True

def stable_triangle_bases(base_length, limit):
    if base_length < 0:
        raise ValueError("Base length must be nonnegative")

    if not base_length:
        yield ()
        return

    for base in stable_triangle_bases(base_length - 1, limit):
        for i in range(limit):
            if is_stable(make_triangle(base + (i,))):
                yield base + (i,)

if __name__ == "__main__":
    for base_length in range(40):
        print(list(next(stable_triangle_bases(base_length, limit=base_length * 2))))

This works really well - up to about length 300 or so, in fact.

I suppose we could try to speed this up more, but that seems beside the point. We want to find the minimum. Well, this isn't totally pointless - we can just run a min with a good key on our result. Unfortunately, though, I haven't managed to get past

                                1001
                             523     478
                         309     214     264
                     195     114     100     164
                 125      70      44      56     108
              79      46      24      20      36      72
          48      31      15       9      11      25      47
      27      21      10       5       4       7      18      29
  14      13       8       2       3       1       6      12      17

with that code.

Further, a limit of base_length * 2 is not optimal. Consider the triangle

                              1000
                           485     515
                       273     212     303
                   171     102     110     193
               112      59      43      67     126
            73      39      20      23      44      82
        46      27      12       8      15      29      53
    28      18       9       3       5      10      19      34
17      11       7       2       1       4       6      13      21

!

Raising the limit makes things even slower - an unbounded limit would take forever!

One conceptually easy, but extremely effective, change is to apply a heuristic to the search. This says that if we have found a triangle of cost \$k\$ and our new path could not give us a cost lower than that, do not try the path.

I originally avoided this because I could not think of a strong enough heuristic. It turns out, though, that it's really simple. Consider this unfinished triangle:

       ??
     ??  ??
    9  ??  ??
  3   6  ??  ??
1   2   4   x  ??

We are going to place onto x next, for all values that will fit. Fill in the unknowns on the base with the minimal values that fit, pushing larger numbers to the edges.

In this case, we cannot add 1, 2, 3 or 4, so we fill in a 5. Treat the x as a 0 for now:

       38
     19  19
    9  10   9
  3   6   4   5
1   2   4   x   5

Note that the duplicates here are allowed, as removing them requires knowing the value of x.

Further, consider the contribution of x:

       4x
      x  3x
    0   x  2x
  0   0   x   x
0   0   0   x   0

We can add these two triangles together, giving a top value of

4x + 38

as our heuristic. Now when outputting results, we can ignore any branches where the heuristic brings us above (or equal to) that. (If you want all of the best options, use strict inequality. If you only need one, use non-strict inequality.)

Here is some code to try. It is not particularly pretty, but it works and is pretty fast.

from itertools import chain, count

def pascal_row(row):
    val = 1
    yield val
    for k in range(row):
        val *= row - k
        val //= k + 1
        yield val

def make_triangle(row):
    while row:
        yield tuple(row)
        row = [left + right for left, right in zip(row[:-1], row[1:])]

def heuristic_minimum(base_so_far, length):
    used = set().union(*make_triangle(base_so_far))
    unused = (x for x in count(1) if x not in used)

    factors = pascal_row(length - 1)
    raw_cost =  sum(a * b for a, b in zip(base_so_far, factors))
    x_factor = next(factors)
    raw_cost += sum(a * b for a, b in zip(unused, sorted(factors, reverse=True)))

    return x_factor, raw_cost

def is_stable(triangle):
    seen = set()

    for row in triangle:
        if not seen.isdisjoint(row) or len(row) != len(set(row)):
            return False
        seen.update(row)

    return True

def stable_triangle_bases(base_length, upper_bound, full_length):
    if base_length < 0:
        raise ValueError("Base length must be nonnegative")

    if not base_length:
        yield ()
        return

    for base in stable_triangle_bases(base_length - 1, upper_bound, full_length):
        x_factor, min_const_cost = heuristic_minimum(base, full_length)

        for i in count(1):
            cost = i * x_factor + min_const_cost

            if cost >= upper_bound.value:
                break

            if is_stable(make_triangle(base + (i,))):
                if base_length == full_length:
                    upper_bound.value = cost

                yield base + (i,)

class UpperBound:
    def __init__(self):
        self.value = float("inf")

if __name__ == "__main__":
    base_length = 10

    bases = stable_triangle_bases(
        base_length,
        UpperBound(),
        base_length
    )

    for result in bases:
        print(result)

This will print the improving bound as time progresses.

To top this off, here's a nice big triangle:

                                      4497
                                  2301    2196
                              1298    1003    1193
                           778     520     483     710
                       479     299     221     262     448
                   296     183     116     105     157     291
               181     115      68      48      57     100     191
           109      72      43      25      23      34      66     125
        65      44      28      15      10      13      21      45      80
    38      27      17      11       4       6       7      14      31      49
20      18       9       8       3       1       5       2      12      19      30


Python script to identify regex matches in all subdirs and write dict of matches to MongoDB I've been attempting to teach myself Python over the past couple of months and have been setting myself practical challenges to learn different aspects of the language.

I have a deep structure of subdirectories containing .rtf files. The objectives of the following code are as follows:

Examine every file in every subdirectory to identify "flagged words" defined in expression.
For each instance a flagged word is identified in a file, capture the file name and the line within that file in which the flagged word appears.
Construct a dictionary, in which flagged words are keys and filenames and lines are stored as values.
Insert each key/value pair in the dictionary into a MongoDB collection

The code is as follows:

from pymongo import MongoClient
import os
import re
import json

# connect to mongodb

client = MongoClient('localhost', 27017)
db = client['test-database']
restrictions = db['restrictions']

# create empty dictionary

d = {}

# setup regular expressions

expression = "sexual offences|reporting restriction|reporting restrictions|anonymous|anonymously|secret"
pattern = re.compile(expression)

# read the source files

for dname, dirs, files in os.walk("path/to/top/level/directory"):

    for fname in files:
        fpath = os.path.join(dname, fname)

        with open(fpath, 'r') as f:
            source = f.readlines()

            for i, line in enumerate(source):

                for match in re.finditer(pattern, line):
                # set the matched flagword as the key and the filename and line as the value
                    d.setdefault(match.group(0), [])
                    file_instance = fname, line
                    print file_instance
                    d[match.group(0)].append(file_instance)

# Update the restrictions database

for key, value in d.iteritems():
    xref_id = db.restrictions.insert_one({'flagged_word': key, 'instance': value})

An example document in MongoDB generated by this code:

    {
        "_id" : ObjectId("5925e0d94fb263cb1417bb73"),
        "flagged_word" : "restriction",
        "instance" : [
            [
                "Family3.txt ",
                "ii)\tAn application for a reporting restriction order (\"reporting restriction order\") to restrict or prohibit the publication of:\n"
            ]
    }

The code works as expected, but I have a nagging feeling that the same objectives could be achieved with a far less clunky approach in the code (particularly where all of the for loops are concerned).

In the spirit of wishing to learn how to write clearer, more Pythonic code, I wonder if anyone could offer some advice on how this code could be improved. Many thanks in advance for taking the time to read my code.

f.readlines() is redundant. Just iterate over the file: for i, line in enumerate(f):, or, as you are not using i at all: for line in f:
d.setdefault becomes redundant if you use d = collections.defaultdict(list)


Finding the average, the five smallest, and the five largest numbers in an array I am going though some PHP arrays exercises on w3resource. I have just completed the following:

Write a PHP script to calculate and display average temperature, five lowest and highest temperatures.

Recorded temperatures : 78, 60, 62, 68, 71, 68, 73, 85, 66, 64, 76, 63, 75, 76, 73, 68, 62, 73, 72, 65, 74, 62, 62, 65, 64, 68, 73, 75, 79, 73

Here is my code:
<?php
    echo "
";

    $temperatures = array(78, 60, 62, 68, 71, 68, 73, 85, 66, 64, 76, 63, 75, 76, 73, 68, 62, 73, 72, 65, 74, 62, 62, 65, 64, 68, 73, 75, 79, 73);

    function listvalues($value)
    {
        echo "$value, ";
    }

    function printAverage($array)
    {
        $total = 0;
        foreach($array as $element)
        {
            $total += $element;
        }
        echo number_format($total / count($array), 1);
    }

    echo "Recorded temperatures : ";
    array_walk($temperatures, "listvalues");
    echo "
";

    echo "Average Temperature is : ";
    printAverage($temperatures);
    echo "
";

    //sort the temperatures in ascending order for both of the following lists.
    sort($temperatures);

    //print the first 5 values
    echo "List of five lowest temperatures : ";
    for($i = 0; $i < 5; $i++)
    {
        echo "$temperatures[$i], ";
    }
    echo "
";

    //print the last 5 values
    echo "List of five highest temperatures : ";
    for($i = count($temperatures) - 5; $i <= count($temperatures) - 1; $i++)
    {
        echo "$temperatures[$i], ";
    }
    echo "
";

    echo "
";
?>

And here is its output:

Recorded temperatures : 78, 60, 62, 68, 71, 68, 73, 85, 66, 64, 76, 63, 75, 76, 73, 68, 62, 73, 72, 65, 74, 62, 62, 65, 64, 68, 73, 75, 79, 73,
Average Temperature is : 69.8
List of seven lowest temperatures : 60, 62, 62, 62, 62,
List of seven highest temperatures : 76, 76, 78, 79, 85,

(I paid no attention to fixing the commas at the end of each part of the output, for now.)
My questions are:

Is this an appropriate use of array_walk()? Is it more appropriate to use a  foreach() loop?

Is ($total / count($array) the easiest way to find the average of an array, and is that the best way to format it for readability?

Can you spot parts of my code which you think could be coded better?

There are a number of built-in functions in PHP that would make your code shorter and more direct.

The first is implode. implode takes and optional glue string and an array of values, and joins them together into a single string. So for implode(", ", array(1, "a", 3)), it would return "1, a, 3". This avoids the need to loop over the elements of an array to print them with a separator, and to catch the last case where there should be no separator.

The second is array_sum. This, as its name suggests, takes an array and sums the values. This also avoids an explicit loop.

The third is array_slice. This function will take an array, and return a portion of it based on the given parameters. It can even be used to index in reverse with a negative offset.

Putting it all together, we get the following:

<?php
    echo "
";

    $temperatures = array(78, 60, 62, 68, 71, 68, 73, 85, 66, 64, 76, 63, 75, 76, 73, 68, 62, 73, 72, 65, 74, 62, 62, 65, 64, 68, 73, 75, 79, 73);

    echo "Recorded temperatures : ";
    echo implode(", ", $temperatures);
    echo "
";

    echo "Average Temperature is : ";
    echo number_format(array_sum($temperatures) / count($temperatures), 1);
    echo "
";

    //sort the temperatures in ascending order for both of the following lists.
    sort($temperatures);

    //print the first 5 values
    echo "List of five lowest temperatures : ";
    echo implode(", ", array_slice($temperatures, 0, 5));
    echo "
";

    //print the last 5 values
    echo "List of five highest temperatures : ";
    echo implode(", ", array_slice($temperatures, -5, 5));
    echo "
";

    echo "
";
?>


Rot47 an NSString Category As a Java native, I am always a little worried when doing Objective-C because of memory leaks and pointers flying at my head...

All comments are welcome on the Category below.  The Category functions are correct.

GitHub

#import "NSString+r47.h"

@implementation NSString (r47)
-(NSString *) r47String
{
    const char *_string = [self cStringUsingEncoding:NSASCIIStringEncoding];
    int stringLength = [self length];
    char newString[stringLength+1];
    int x;
    for( x=0; x<stringLength; x++ )
    {
        unsigned int aCharacter = _string[x];
        if( 0x20 < aCharacter && aCharacter < 0x7F ) // from ! to ~
            newString[x] = (((aCharacter - 0x21) + 0x2F) % 0x5E) + 0x21;
        else  // Not an r47 character
            newString[x] = aCharacter;
    }
    newString[x] = '\0';
    NSString *rotString = [NSString stringWithCString:newString encoding:NSASCIIStringEncoding];
    DLog(@"%@ = %@",self,rotString);
    return rotString ;
}

@end

The question title suggested that this would be a Rot47 category on NSString, but it's not.  It's an r47 category (which tells me nothing... what's "r47"?).

This is Objective-C, not C or C++ or any other language where cryptic terseness is favored.  We prefer verboseness.  The category should be called Rot47.  This should be @implementation NSString (Rot47).  The method should be called - (NSString *)rot47String;

In Objective-C, we also prefer our opening braces go on the same line, not on their own line.  For example:

for (;;) {

Braces are technically optional, but you really shouldn't think of them as such.  Apple even agrees and made them non-optional in swift.  You've left them out of your if/else blocks, which is a great way to leave your code open for future bugs.

When we implement categories on Foundation classes, we should take care to ensure their portability.  The method should be designed to work no matter what project we import this file into and no matter what the contents of the string.

The problem here is that your method doesn't do that.  Your method works fine presuming a particular encoding, but for anything else it will produce incorrect results.

Rather than a char array, we need a unichar array.

NSUInteger length = [self length];
unichar letters[length + 1];

[self getCharacters:letters range:NSMakeRange(0,length)];

Now letters is an array of unichars for us to iterate over and transform correctly.


Determining if a level exists This is the code that I'm using. I'm working on LINQ TO SQL and I'm using this model in my program:

How can I model this class in a database

The person must have to enter the level, for example 1.2, 1.1.4, etc. The program must check if  the level is available or busy. The function ToIntArray is an extension and consists of splitting the text and converting the resulting string[] to int[].

The first condition, as you can see, is an inconsistency which I have in this post:

Problems with nullable types in a LINQ function

private void AddButton_Click(object sender, RoutedEventArgs e)
{
    NorthwindDataContext cd = new NorthwindDataContext();

    int[] levels = LevelTextBox.Text.ToIntArray('.');
    string newGroupName = NameTextBox.Text;

    Objective currentObjective = null;
    int? identity = null;

    for (int i = 0; i < levels.Length - 1; i++ )
    {
        int currentRank = levels[i];

        if (identity == null)
        {
            currentObjective = (from p in cd.Objective
                                where p.Level == currentRank && p.Parent_ObjectiveID == null
                                select p).SingleOrDefault();
        }
        else
        {
            currentObjective = (from p in cd.Objective
                                where p.Level == currentRank && p.Parent_ObjectiveID == identity
                                select p).SingleOrDefault();
        }

        if (currentObjective == null)
        {
            MessageBox.Show("Levels don't exist");
            return;
        }
        else
        {
            identity = currentObjective.ObjectiveID;
        }
    }

    if (currentObjective != null)
    {
        if (levels.Last() == currentObjective.Level)
        {
            MessageBox.Show("Level already exists");
            return;
        }
    }
    else
    {
        var aux = (from p in cd.Objective
                   where p.Parent_ObjectiveID == null && p.Level == levels.Last()
                   select p).SingleOrDefault();

        if (aux != null)
        {
            MessageBox.Show("Level already exists");
            return;
        }
    }

    var newObjective = new Objective();
    newObjective.Name = NameTextBox.Text;
    newObjective.Level = levels.Last();
    newObjective.Parent_ObjectiveID = currentObjective == null ? null : (int?)currentObjective.ObjectiveID ;

    cd.Objective.InsertOnSubmit(newObjective);
    cd.SubmitChanges();

    MessageBox.Show("The new objective has added successfully");
    NameTextBox.Clear();
    LoadObjectives();
}

A few  comments:

You should move this code from the AddButton_Click to some business logic class.
Maybe you should add some validation of LevelTextBox.Text, before using its value.
You should split this code into different methods.

  You have three responsibilities here:

  Find out  if the level exists at all.
  Find out if the specified level already exists.
  Create a new level.

  The fact that all three actions use the same data doesn't mean you need to put them in one method.

Maybe I get it wrong somewhere, but I can't understand how this code checks if the level already exists. I can't see how the code will ever get into the MessageBox.Show("Level already exists"); in both places.


Using classes vs. using functions I am trying to learn how to tell if my code will be better off creating a class as opposed to using many functions. I am a new Python programmer trying to get the hang of when classes should be implemented.

Determines whether the username is valid or invalid:

def is_good_user(user):
    is_user_good = True
    if user in database:
        print "Sorry! Username is taken! Create another."
        is_user_good = False
    if not 5 < len(user) < 25:
        print "Username must be at least 6 characters!"
        is_user_good = False
    if is_ascii(some_string="") is not True:
        print "Please create username that follows ASCII formatting."
        is_user_good = False
    return is_user_good

Determines whether the password is valid or invalid:

def is_good_password(password):
    count_upper, count_lower = 0, 0
    for characters in password:
        if characters.isupper():
            count_upper += 1
        if characters.islower():
            count_lower += 1
    is_password_good = True
    if len(password) <= 10:
        print "Password is too weak, must be more than 10 characters long!"
        is_password_good = False
    if set(database).intersection(password):
        print "Password Must Contain Alphanumeric Characters!"
        is_password_good = False
    if count_upper < 1 or count_lower < 1:
        print "Password must contain at least one uppercase and one lowercase character!"
        is_password_good = False
    return is_password_good

These two functions are amalgamated into one function to create a username and password:

def create_user(database):
    while True:
        user = raw_input("Enter a New Username: ")
        if is_good_user(user):
            print "Great Job! Username Successfully Created! Time to Create a Password!"
            break
        print "That's not a good username, please try again!"

    while True:
        passcode = raw_input("Enter a New Password: ")
        if is_good_password(passcode):
            print "Welcome! Username & Password successfully created!"
            break
        print "That's not a good password, please try again!"

    database[user] = passcode
    make_digest(password=passcode, salt=None)
    dump_data()

Should I add in a class for readability of my code? Being new at Python, I'd like to take up the challenge if adding a class is necessary.

It's a design choice.
A class is "meant to be" a collection that encapsulates functionality and "behavior" in code. The purpose is to expose specifically an interface where a class's instances can live on their own.

A function/method is an independent piece of code that in theory "shouldn't" relate to other pieces of code outside of it.

So for the example you've described, you could create a class named "User". And user can include these functions as it's members.
This does make it more readable and easier to debug.
However do not take unrelated functions and dump them into a class just cuz!
Examples are utility type functions, they could be kept in the vicinity of each other but should not be clubbed together as a class.

An additional benefit of classes is that they can be Inherited from. So more complex classes can inherit the basic functionality and add to it, while using the features already in the base class. You can achieve a similar effect by having "higher" versions of functions that calls more basic level functions. There again at least in my design perspective, I make a class where the whole functionality will be inherited as opposed to functions that are stand-alone.


Selecting only non-empty cells in a row Using Pandas to read a 14k lines CSV with many empty cells - what I call "Emmenthaler data", lots of holes :-) . A very short, simplified sampler (urlbuilder.csv):

"one","two","three"
"","foo","bacon",""
"spam","bar",""

The cells contain information to match against this web API, like this:

http://base_URL&two="foo"&three="bacon"
http://base_URL&one="spam"&two="bar"

Leaving values empty in the URL (``...one=""&two="bar"`) would give wrong results. So I want to use just the non-empty fields in each row. To illustrate the method:

import pandas as pd

def buildurl(row, colnames):
    URL = 'http://someAPI/q?'
    values = row.one, row.two, row.three  # ..ugly..

    for value,field in zip(values, colnames):
        if not pd.isnull(value):
            URL = '{}{}:{}&'.format(URL, field, value)

    return URL

df = pd.read_csv('urlbuilder.csv', encoding='utf-8-sig', engine='python')
colnames = list(df.columns)

for row in df.itertuples():
    url = buildurl(row, colnames)
    print(url)

It works and it's probably better than a cascade of if not isnull's. But I still have this loud hunch that there are much more elegant ways of doing this. Yet I can't seem to find them, probably I'm not googling for the right jargon.

Please comment

For anything related to building URL query strings from key-values pairs, you should let urllib.parse.urlencode do the work for you. It can also handle quoting and other special cases for you. To do that, you have to convert your data to a dictionary.

This is easy if you start with a pd.Series:

from urllib.parse import urlencode

def build_url_params(serie):
    parameters = serie[~pd.isnull(serie)].to_dict()
    return urlencode(parameters)

Then you just need to provide Series to this function instead of tuples:

def populate_url_params(df):
    df['URL parameters'] = df.apply(build_url_params, axis=1)

if __name__ == '__main__':
    df = pd.read_csv('urlbuilder.csv', encoding='utf-8-sig', engine='python')
    populate_url_params(df)
    print(df)

Or if you want to go the manual iteration route, you can use df.iterrows() instead of df.itertuples().


Zip-like construction of objects from parameter collections This little function can make object creation much easier.
Sometimes you need to switch between representing your data as objects vs. having the members in separate containers.

Example:

class Foo {
public:
    Foo(int a_, double b_): a(a_), b(b_){}

    int a;
    double b;
};

And then:

std::vector<int> as = {1,2,3,4,5};
std::vector<double> bs = {6.0,7.0,8.0,9.0,10.0};
std::vector<Foo> foos = zipConstruct<Foo>(as, bs);

cout << foos[3].a << ", " << foos[3].b << endl; // 4, 9

Using:

template <typename T, typename... Arg>
std::vector<T> zipConstruct(std::vector<Arg> const&... argVec)
{
    std::vector<size_t> sizes = {argVec.size()...};
    assert(std::all_of(sizes.begin(), sizes.end(), [&](size_t s){return s=sizes[0];}));
    size_t size = sizes[0];

    std::vector<T> result;
    result.reserve(size);

    for (size_t i = 0; i < size; ++i)
    {
        result.emplace_back((argVec[i])...);
    }

    return result;
}

Does this make sense?

I see one error:

assert(std::all_of(sizes.begin(), sizes.end(), [&](size_t s){return s=sizes[0];}));
                                                                     ^

This is an assignment, not a test. You probably meant s == sizes[0].
You can help this by passing s as a const value:

assert(std::all_of(sizes.begin(), sizes.end(), [&](size_t const s){return s == sizes[0];}));

Additionally, you may not mind destroying your old vectors (via move). So it may be worth looking at using move construction on the result vector:

std::vector<T> zipConstruct(std::vector<Arg>&... argVec)
{                                //        ^^^  pass by reference as you may modify them

      result.emplace_back(std::move(argVec[i])...);
                      //  ^^^^^^^^^^   Add standard move to get move semantics.


Command-line English Hangman game Introduction
As a Python neophyte, I implemented the Hangman game as a practice, which operates case-insensitively on the 26 English letters.
My biggest concern at the moment is my use of bytearray to store the word, but I might have made other beginner mistakes as well.  I'd like to locate them and improve accordingly.
Code
import itertools

class Hangman:

    def __init__(self, target, guesses):
        assert target.isascii() and target.isalpha()

        self.target = bytearray(target.lower(), 'utf-8')
        self.guesses = guesses
        self.display = bytearray(itertools.repeat(ord('_'), len(target)))

    def run(self):
        while True:
            print()
            print()
            print(''.join(chr(byte) for byte in self.display))
            print()
            print(f'Remaining guesses: {self.guesses}')

            guess = input('Guess a letter: ')

            if len(guess) != 1:
                print('Invalid guess.')
                continue
            elif not guess.isascii() or not guess.isalpha():
                print('Non-letter guessed.')
                continue

            guess = ord(guess)

            if self.display.count(guess) != 0:
                print('Already guessed this letter.')
                continue

            if self.target.count(guess) == 0:
                self.guesses -= 1
            else:
                self.display = bytearray(
                    (
                        guess if target_c == guess else display_c
                    ) for display_c, target_c in zip(self.display, self.target)
                )

            if self.display == self.target:
                print()
                print()
                print(''.join(chr(byte) for byte in self.target))
                print('You won.')
                break
            elif self.guesses == 0:
                print()
                print()
                print(''.join(chr(byte) for byte in self.target))
                print()
                print('You lost.')
                break

if __name__ == '__main__':
    # Merriam-Webster Word of the Day: October 2, 2020
    hangman = Hangman('pachyderm', 6)
    hangman.run()

Example session
$ python hangman.py

_________

Remaining guesses: 6
Guess a letter: a

_a_______

Remaining guesses: 6
Guess a letter: e

_a____e__

Remaining guesses: 6
Guess a letter: i

_a____e__

Remaining guesses: 5
Guess a letter: o

_a____e__

Remaining guesses: 4
Guess a letter: u

_a____e__

Remaining guesses: 3
Guess a letter: r

_a____er_

Remaining guesses: 3
Guess a letter: y

_a__y_er_

Remaining guesses: 3
Guess a letter: c

_ac_y_er_

Remaining guesses: 3
Guess a letter: h

_achy_er_

Remaining guesses: 3
Guess a letter: p

pachy_er_

Remaining guesses: 3
Guess a letter: d

pachyder_

Remaining guesses: 3
Guess a letter: m

pachyderm
You won.

Use more functions
Currently, the entirety of your hangman game lies inside the run() call. You can split certain parts of it to their own functions, such as checking for win condition, asking for character input, displaying the current state etc.
Empty prints
Use \n to get a new line in console, instead of putting in empty print statements.
Checking guessed character
For strings, you can just check if the character is in string or not, since you have no need for the count of repetitions itself.
ord, chr and bytearray
They are of no use. You do not need a bytearray of ordinals, simply splitting the word into a list would be enough (not really needed though).
Type hints
With newer python, type hinting feature has also become available. Use them to assist you with the type of values for the variables.

So, your program could also be rewritten like:
class Hangman:
    def __init__(self, target: str, guesses: int):
        assert self.is_ascii_alphabetical(
            target
        ), "Provided secret word is not ascii alphabetical string."
        self.secret_word = target
        self.guesses = guesses
        self.guessed_word = ["_"] * len(target)
        self._finish = False

    def is_ascii_alphabetical(self, value: str) -> bool:
        return value.isascii() and value.isalpha()

    def display(self, real_word: bool = False):
        if not real_word:
            print(f"\n\n{''.join(self.guessed_word)}\n")
        else:
            print(f"\n\n{self.secret_word}\n")

    def read_character(self) -> str:
        while True:
            guess = input("Guess a letter: ")
            if len(guess) != 1:
                print("Invalid guess.")
                continue
            elif not self.is_ascii_alphabetical(guess):
                print("Non-letter guessed.")
                continue
            if self.is_guess_repeat(guess):
                print("Already guessed this letter.")
                continue
        return guess.lower()

    def update_guessed_word(self, character: str) -> None:
        self.guessed_word = [
            new if new == character else old
            for old, new in zip(self.guessed_word, self.secret_word)
        ]

    def is_guess_repeat(self, guess: str) -> bool:
        return guess in self.guessed_word

    def is_guess_valid(self, guess: str) -> bool:
        if guess not in self.secret_word:
            return False
        return True

    def is_game_over(self) -> bool:
        return self.guesses <= 0

    def is_game_won(self) -> bool:
        return "_" not in self.guessed_word

    def run(self):
        while not self._finish:
            self.display()
            print(f"Remaining guesses: {self.guesses}")
            guess = self.read_character()
            if self.is_guess_valid(guess):
                self.update_guessed_word(guess)
            else:
                self.guesses -= 1
            if self.is_game_won():
                self.display(True)
                print("You won.")
                self._finish = True
            elif self.is_game_over():
                self.display(True)
                print("You lost.")
                self._finish = True

if __name__ == "__main__":
    # Merriam-Webster Word of the Day: October 2, 2020
    hangman = Hangman("pachyderm", 6)
    hangman.run()