Replace any number of consecutive underscores with a single space I am coding the following interesting problem :

  Remove '_' and print with 1 space from a given string
  For example, for "w_e_b__services" output should be : "w e b services".
  Multiple underscore should be substituted with only with one space.

The following is my code, which has some code duplication. I can see there are some ways to get rid of that duplication but that complicates the code in a different way. It would be great to get some code review on this.

public class ReplaceUnderscore {

    private static final char UNDERSCORE = '_';
    private static final char SPACE = ' ';

    public static String replace(String input) {

        if (input.isEmpty()) {
            return input;
        }

        StringBuilder output = new StringBuilder();

        char prevChar = input.charAt(0);
        if (prevChar == UNDERSCORE) {
            output.append(SPACE);
        } else {
            output.append(prevChar);
        }

        for (int i = 1; i < input.length(); i++) {
            char currentChar = input.charAt(i);

            if (currentChar == UNDERSCORE) {
                if (currentChar != prevChar) {
                    output.append(SPACE);
                }
            } else {
                output.append(currentChar);
            }

            prevChar = currentChar;
        }

        return output.toString();
    }
}

And here is the junit test:

public class StripUnderscoreTest {

    @Test
    public void strip() throws Exception {

        assertEquals(" ", ReplaceUnderscore.replace("_"));
        assertEquals("a b c", ReplaceUnderscore.replace("a_b_c"));
        assertEquals("a b c", ReplaceUnderscore.replace("a__b__c"));
        assertEquals("a b c", ReplaceUnderscore.replace("a___b___c"));
    }
}

I think overall it is a nice implementation. It is clear and readeable. The use of constants is nice, makes it easy to refactor the code to make it more generic later on (e.g. replace with given character, not just space). Clear variable names (although I'd try and be consistent, and use current\previous, or cur/prev, most likely the former).

I have two remarks:

I am not a fan of the new line at the start of a function, it is too much, and makes me lose my focus.
I'd change the check if (currentChar != prevChar) to if (prevChar != UNDERSCORE). Although we can know from the check above that currentChar is an underscore, this makes it even more clear.

I don't really think you have what I'd see as duplication, but a possible other way to implement it would be to keep track of wether you are currently in a sequence of underscores. Example:

boolean previousWasUnderscore = false;
for (int i = 0; i < input.length(); i++) {
    final char currentChar = input.charAt(i);
    if (currentChar == UNDERSCORE) {
        if (!previousWasUnderscore ) {
            output.append(SPACE);
            previousWasUnderscore = true;
        }
    } else {
        output.append(currentChar);
        previousWasUnderscore = false;
    }
}


Munkres algorithm in Python: calculating maximum profit I'm using Munkres library from https://github.com/bmc/munkres/ to calculate maximum profit (reversed problem).

What do you think about this coding style?

def get_no_of_vowels(str):
    return sum(str.lower().count(c) for c in "aeiuoy")

def get_no_of_consonants(str):
    return sum(str.lower().count(c) for c in "bcdfghjklmnpqrstvwxz")

def count_letters(str):
    return sum(c.isalpha() for c in str)

def compute_ss(name, item):
    if not count_letters(item)%2:
        ss = get_no_of_vowels(name)*1.5
    else:
        ss = get_no_of_consonants(name)
    if gcd(count_letters(name), count_letters(item)) > 1:
        ss*=1.5
    return ss

if __name__ == "__main__":
    with open(sys.argv[1]) as f:
        for line in f:
            names = line.strip().split(';')[0].split(',')
            items = line.strip().split(';')[1].split(',')
            dic = collections.defaultdict(list)
            profit_matrix = []
            for name in names:
                row = []
                for item in items:
                    row.append(compute_ss(name, item))
                profit_matrix.append(row)
            m = Munkres()
            cost_matrix = make_cost_matrix(profit_matrix, lambda x: 1e10 - x)
            indexes = m.compute(cost_matrix)
            total = 0
            for row, column in indexes:
                value = profit_matrix[row][column]
                # print '(%.2f, %.2f) -> %.2f' % (row, column, value)
                total += value
            print "%.2f"%total

I just want to point out two sources of inefficiency:

1) The counting functions, e.g.:

def get_no_of_vowels(str):
    return sum(str.lower().count(c) for c in "aeiuoy")

This will loop over str 6 times, which is 5 more times than is necessary. A more efficient approach is:

def get_no_of_vowels(str):
    return sum(1 for s in str if s.lower() in "aeiouy" else 0)

We are creating a generator that will convert each character into a 1 or a 0, and then just sum them. That way, we just do one iteration. Also, "y" is not a vowel.

2) Your line-splitting code here:

        names = line.strip().split(';')[0].split(',')
        items = line.strip().split(';')[1].split(',')

You are splitting line twice. That's extra work. Prefer:

names, items = line.strip().split(';')
names = names.split(',')
items = items.split(',')

Hope that helps.


Compare dicts and record changes My goal is to continually update an object and record any changes and the times they were made at. The input has to be JSON but the output/storage format doesn't.

This function will check a JSON object/dictionary against a reference object and return a list containing the updated object and another object containing just the updates.

I've tested it a bit and I think it handles inconsistencies in the objects, such as the old object not having the correct nesting.

It only compares new to old, so any missing keys in the new version won't be removed from the old version. It's not memory-impervious but there's no recursion so depth itself isn't a problem.

How can I make my approach more efficient? (Faster, less steps involved)

def getFromDict(dataDict, mapList):
    return reduce(lambda d, k: d[k], mapList, dataDict)

def setInDict(dataDict, mapList, value):
    getFromDict(dataDict, mapList[:-1])[mapList[-1]] = value

def record_changes(obj, keys, key, value):
    for _ in xrange(len(keys)+10):
        try:
            setInDict(obj, keys+[key], value)
            return obj
        except:
            for i in range(len(keys)+1)[1:]:
                try:
                    assert isinstance(getFromDict(obj, keys[:i]), dict)
                except:
                    setInDict(obj, keys[:i], {})
    else:
        raise Exception

def compare_object(N, record):
    """
    :param:N new object
    :param:record old object
    """
    changes = {}
    D = N.copy()
    nested_keys = []
    for key in D:
        if isinstance(D[key], dict):
            nested_keys.append([key])
        else:
            if D[key] != record.get(key, None):
                record[key] = D[key]
                changes[key] = D[key]
    for keys in nested_keys:
        d = getFromDict(N, keys)
        for key in d:
            if isinstance(d[key], dict):
                nested_keys.append(keys+[key])
            else:
                try:
                    if d[key] != getFromDict(record, keys+[key]):
                        setInDict(record, keys+[key], d[key])
                        changes = record_changes(changes, keys, key, d[key])
                except:
                    # record = Help(D, record, keys+[key])
                    record = record_changes(record, keys, key, d[key])
                    changes = record_changes(changes, keys, key, d[key])
    return [record, changes]

Usage:

>>> new = {'a':{'b':1, 'q':0}, 'c':5}
>>> old = {'a':{'b':2, 'q':0}, 'f':9}
>>> print compare_object(new, old)
[{'a': {'q': 0, 'b': 1}, 'c': 5, 'f': 9}, {'a': {'b': 1}, 'c': 5}]

So you want a variation of dict.update that recursively update inner dicts; and you want it to record changes as you go.

I’m afraid that your solution is way too complicated for that:

compare_object has two for loops (for key in D and for key in d) that mainly perform the same kind of task. The former being an initialization to the outer loop of the latter.
compare_object copies its N parameter which is never modified and modify record in place (thus there is no need in returning it); which is at best misleading, at worse a bug.
record_change seems to be only an extremely verbose and convoluted way of calling setInDict while ensuring that each level of nesting will actually contain a dictionary.

As for the actual coding style:

Always specify the kind of exception you’re expecting in an except clause; as in you probably don't expect to catch IndexError when calling record_changes.
Raising Exception is also worth noting. You should either declare your own exception or raise a more specific one.
Iterating over keys in a dictionary and repeatedly calling d[key] reads poorly: it's better to iterate over the pairs of keys and values returned by items or iteritems (Python 2): for key, value in d.iteritems():.
As already stated around here AssertionErrors are not meant to control the flow of execution. If I run your code in optimized mode (-O switch on the command line), your code breaks.

Now before getting back to the algorithm, let me introduce you to dict.setdefault: it works pretty much the same than dict.get but instead of just returning the default value in case the key does not exist, it set the key to that default value if it doesn't exist before returning the value.

With that information in mind, let's take an empty file and build a function that update a dictionary and recursively update inner dictionaries.

The name should convey (at least part of) this:

def update_inner(orig, update):

The function should return values that actually changed:

    change = {}
    ...
    return change

The function should update values of the original dict with values from the update dict (and still record changes):

    change = {}
    for key, value in update.iteritems():
        if value != orig.get(key):
            orig[key] = change[key] = value
    return change

The function should recurse to update inner dictionaries:

def update_inner(orig, update):
    change = {}
    for key, value in update.iteritems():
        try:
            # Consider value is a dict, so recurse
            changed = update_inner(orig, value)
        except AttributeError:
            # We were wrong, value is not a dict, backup to simple update
            if orig.get(key) != value:
                orig[key] = change[key] = value
        else:
            # We were right, record the changes
            change[key] = changed
    return change

Wait no, there is something wrong… We are not keeping track of the keys used to go deeper into orig when doing the recursion… Find out why I talked about setdefault now?

Proposed improvements

def update_inner(orig, update):
    change = {}
    for key, value in update.iteritems():
        try:
            # Consider value is a dict, so recurse
            changed = update_inner(orig.setdefault(key, {}), value)
        except AttributeError:
            # We were wrong, value is not a dict, backup to simple update
            if orig[key] != value: # Direct access is OK thanks to setdefault above
                orig[key] = change[key] = value
        else:
            # We were right, record the changes
            change[key] = changed
    return change

You use it like so:

>>> record = {'a':{'b':2, 'q':0}, 'f':9}
>>> new = {'a':{'b':1, 'q':0}, 'c':5}
>>> update_inner(record, new)
{'a': {'b': 1}, 'c': 5}
>>> record
{'f': 9, 'a': {'q': 0, 'b': 1}, 'c': 5}


A custom Undo Manager I am working on building an UndoManager in C#. The concept is to store events and property changes in a Stack<Action<T>> instance. I believe it is working, but I am also looking for peer review.

What can I improve?  Do you see any bugs?

public abstract class UndoManager<T>
{
    /// <summary>
    /// Adds an Event to Stack
    /// </summary>
    /// <param name="undoOperation"></param>
    protected abstract void Add(Action<T> undoOperation);

    /// <summary>
    /// Returns the size of the stack
    /// </summary>
    /// <returns></returns>
    public abstract int Size();
    /// <summary>
    /// Holds all the Events in a stack
    /// </summary>
    protected Stack<Action<T>> MyUndoOperations { get; set; }
    protected Stack<Action<T>> MyRedoOperations { get; set; }

    /// <summary>
    /// Preforms the undo action. Pops the last event and gets a reference to the next event
    /// and fires the event
    /// </summary>
    public abstract void Undo();
    /// <summary>
    /// Preforms the Redo action.
    /// </summary>
    public abstract void Redo();

}

public class MyClass : UndoManager<MyClass>
{
    #region Getter and Setters
    private String myValue;
    public String MyValue
    {
        get { return this.myValue; }

        set
        {

            this.Add(x => x.myValue = value);// save the operation on the stack
            this.myValue = value;

        }
    }

    private String productName;
    public String ProductName
    {
        get { return productName; }

        set
        {
            this.Add(x => x.productName = value);// save the operation on the stack
            productName = value;
        }
    }
    #endregion

    /// <summary>
    /// Returns the size of the Action Event Stack
    /// </summary>
    /// <returns>Size of this.MyOperations</returns>
    public override int Size()
    {
        return this.MyUndoOperations.Count();
    }

    public MyClass()
    {

        this.MyUndoOperations = new Stack<Action<MyClass>>();
        this.MyRedoOperations = new Stack<Action<MyClass>>();
    }
    /// <summary>
    /// Pushes the last Action event on the Stack
    /// </summary>
    /// <param name="undoOperation">Last Action Event</param>
    protected override void Add(Action<MyClass> undoOperation)
    {
        //this.MyRedoOperations.Clear();
        this.MyUndoOperations.Push(undoOperation);
    }

    /// <summary>
    /// Preforms the undo action. Pops the last event and gets a reference to the next event
    /// and fires the event
    /// </summary>
    public override void Undo()
    {

        if (this.MyUndoOperations != null && this.MyUndoOperations.Any())
        {

            Action<MyClass> topAction = this.MyUndoOperations.Pop();// remove the very last event.
            this.MyRedoOperations.Push(topAction); // add to the redo stack

            if (this.MyUndoOperations.Any())
            {
                // get a reference (peek) to the last event
                Action<MyClass> lastAction = this.MyUndoOperations.Peek();
                lastAction(this);// fire event
                //this.Add(lastAction); // add to the undo stack
            }

        }

    }

    public override void Redo()
    {
        if (this.MyRedoOperations.Any())
        {

            Action<MyClass> lastAction = this.MyRedoOperations.Pop();
            lastAction(this);// fire event
            this.Add(lastAction);
        }

    }
}

What's most important, as @ChrisW wrote, you need two separate actions for Execute/Redo and Undo. A common command interface would be something like:

public interface ICommand
{
    // Executes this command.
    void Execute();

    // Undoes this command.
    void Undo();
}

The UndoManager should not be generic nor abstract. By making it generic, you only allow it to store a limited set of commands, which is unnecessary. The manager should not know anything about the command it stores, apart from the fact that it can invoke Execute and Undo on them.
There is no need to make it abstract, either. I would however extract its interface so that you can test it (mock it), or provide dummy implementations in special cases. I doubt you would have additional derived undo managers, since its behavior is well defined, so I would probably limit the interface to something like:

public interface IUndoManager
{
    // Executes the specified commmand and adds it to the Undo stack.
    void Execute(ICommand commmand);

    // Undoes the last command in the Undo stack.
    void Undo();

    // Redoes the last command in the Redo stack.
    void Redo();
}

If you want to let your callers know more about the command manager state, then you can extend the whole thing by adding properties like Name to the ICommand, or CanUndo/CanRedo to the ICommandManager. That will allow your callers (i.e. a menu item in your Edit menu) to know if there exists an undoable command, and what's its description. But at all costs avoid exposing "internal" stuff, like the size of your stack.
Although I understand it's conceptual code, I would avoid prefix class names and property names with My (MyClass, MyUndoOperations). It doesn't provide any useful information.


Email redirection script in python I am new to python and wrote a email redirect script. It extracts mails from a server and than resend them. I would like to get some feedback:

import sys
import smtplib
import poplib
import re
import argparse

def validate_email_address(address):
    email_pattern = '^[^@]+@[^@]+\.[^@]+$'
    if not re.match(email_pattern, address ):
        raise argparse.ArgumentTypeError('{} is not a valid email address'.format(address))
    return address

arg_parser = argparse.ArgumentParser()
arg_parser.add_argument('popServer', help='pop server host or address of extracted account')
arg_parser.add_argument('smtpServer', help='smtp server host or address of extracted account')
arg_parser.add_argument('address', help='extract messages from this account', type=validate_email_address)
arg_parser.add_argument('password', help='password of extracted account')
arg_parser.add_argument('target', help='redirect messages to this account', type=validate_email_address)
arg_parser.add_argument('-useTSL', action='store_true')
arg_parser.add_argument('-useSSL', action='store_true')
args = arg_parser.parse_args()
timeout = 10
try:
    if args.useSSL: pop_conn = poplib.POP3_SSL(args.popServer, timeout=timeout)
    else: pop_conn = poplib.POP3(args.popServer, timeout=timeout)
    pop_conn.user(args.address)
    pop_conn.pass_(args.password)
    mail_count = len(pop_conn.list()[1])
    print('Mail count:{}'.format(mail_count))
    if mail_count > 0:
        print('Requesting mails from {0} for {1}'.format(args.popServer, args.address))
        for i in range(1, mail_count + 1):
            try:
                print("Receiving mail {0}/{1}".format(i,mail_count))
                message = (pop_conn.retr(i)[1])
                pop_conn.dele(i)
                message = [line.decode('utf-8', errors='ignore') for line in message]
                message = '\n'.join(message)
                print("Redirecting mail {0}/{1}".format(i,mail_count))
                if(args.useSSL): smtp_conn = smtplib.SMTP_SSL(args.smtpServer, timeout=timeout)
                else: smtp_conn = smtplib.SMTP(args.smtpServer, timeout=timeout)
                if(args.useTSL): smtp_conn.starttls()
                smtp_conn.login(args.address, args.password)
                smtp_conn.sendmail(args.address, args.target, message.encode(encoding='ascii', errors='ignore'))
                smtp_conn.quit()
            except smtplib.SMTPException as SMTP_exception:
                print('Request of mail failed:{0}'.format(SMTP_exception))
                pass
            except poplib.error_proto as pop_error:
                print('Receiving of mail failed:{0}'.format(pop_error))
                pass
except poplib.error_proto as pop_error:
    print('Receiving of mail failed:{0}'.format(pop_error))
except OSError as os_error:
    print('Name resolution or server connection failed:{}'.format(os_error))

OK, here it goes:

Currently all of your code is bunched up in a code block. It would be better to follow the single responsibility principle and have functions for every job.

Therefore, I used a main function, a parse_args function, a pop_connect function to login with the POP3 server, a smtp_connect function to do the same with the SMTP server, a get_mail function to retrieve a mail from the server and catch any exception and a send_mail function to encode the message and send it.

For the SMTP connection I used the with..as functionality of python. This makes sure that the SMTP connection is closed, regardless of any errors in between. Also, it is sufficient to connect to the SMTP server once and then re-use that connection, unless your latency is really bad (and in that case, just increase the timeout, which I mad an optional argument). Have a look at this website to read a more thorough explanation on how to use a contextmanager on a function.

I also made some formats slightly simpler. If the arguments are in order, it is not necessary, to use "{0}{1}".format(x, y), "{}{}".format(x, y) is sufficient. On the other hand, I also used the fact that format understands a few rudimentary things, namely indices and attributes:

>>> l = [1,2,3]
>>> "{0[1]}".format(l)
2
>>> from collections import namedtuple
>>> Box = namedtuple("Box", "width height")
>>> b = Box(2, 3)
>>> "{0.width}, {0.height}".format(b)
2, 3

Finally, I used the if __name__ == "__main__": idiom, to allow importing these functions from another module without running all the code.

import sys
import smtplib
import poplib
import re
import argparse

from contextlib import contextmanager

def validate_email_address(address):
    email_pattern = '^[^@]+@[^@]+\.[^@]+$'
    if not re.match(email_pattern, address):
        raise argparse.ArgumentTypeError(
            '{} is not a valid email address'.format(address))
    return address

def parse_args():
    arg_parser = argparse.ArgumentParser()
    arg_parser.add_argument(
        'popServer', help='pop server host or address of extracted account')
    arg_parser.add_argument(
        'smtpServer', help='smtp server host or address of extracted account')
    arg_parser.add_argument(
        'address', help='extract messages from this account', type=validate_email_address)
    arg_parser.add_argument('password', help='password of extracted account')
    arg_parser.add_argument(
        'target', help='redirect messages to this account', type=validate_email_address)
    arg_parser.add_argument('--useTSL', action='store_true')
    arg_parser.add_argument('--useSSL', action='store_true')
    arg_parser.add_argument('--timeout', type=int,
                            help="Timeout for the servers", default=10)
    return arg_parser.parse_args()

def pop_connect(server, user, password, timeout=10, use_SSL=False):
    pop = poplib.POP3_SSL if use_SSL else poplib.POP3
    pop_conn = pop(server, timeout=timeout)
    pop_conn.user(user)
    pop_conn.pass_(password)
    return pop_conn

@contextmanager
def smtp_connect(server, user, password, timeout=10, use_SSL=False, use_TSL=False):
    try:
        smtp = smtplib.SMTP_SSL if use_SSL else smtplib.SMTP
        smtp_conn = smtp(server, timeout=timeout)
        if use_TSL:
            smtp_conn.starttls()
        smtp_conn.login(user, password)
        yield smtp_conn
    except smtplib.SMTPException as SMTP_exception:
        print('Request of mail failed:{}'.format(SMTP_exception))
        raise
    finally:
        try:
            smtp_conn.quit()
        except UnboundLocalError:
            pass

def get_email(pop_conn, i):
    try:
        return pop_conn.retr(i)[1]
    except poplib.error_proto as pop_error:
        print('Receiving of mail failed:{0}'.format(pop_error))

def send_mail(smtp_conn, to_email, from_email, message):
    message = '\n'.join(line.decode(
        'utf-8', errors='ignore') for line in message)
    smtp_conn.sendmail(from_email, to_email, message.encode(
        encoding='ascii', errors='ignore'))

def main():
    args = parse_args()
    try:
        pop_conn = pop_connect(args.popServer, args.address,
                               args.password, args.timeout, args.useSSL)
        mail_count = len(pop_conn.list()[1])
        print('Mail count:{}'.format(mail_count))
        if mail_count:
            print('Requesting mails from {0.popServer} for {0.address}'.format(args))
            with smtp_connect(args.smtpServer, args.address, args.password, args.timeout, args.useSSL, args.useTSL) as smtp_conn:
                for i in range(1, mail_count + 1):
                    print("Receiving mail {}/{}".format(i, mail_count))
                    message = get_email(pop_conn, i)
                    pop_conn.dele(i)
                    print("Redirecting mail {}/{}".format(i, mail_count))
                    send_mail(smtp_conn, args.address, args.target, message)
    except poplib.error_proto as pop_error:
        print('Receiving of mail failed:{}'.format(pop_error))
    except OSError as os_error:
        print('Name resolution or server connection failed:{}'.format(os_error))

if __name__ == "__main__":
    main()


SQL Table and Merge Statement I am new to the code review area of Stack and welcome all positive criticism.

I would be grateful if someone could review my code below and let me know where I can improve or have gone wrong that I cannot see.

The only thing that I have left is to write the delete and would appreciate help in doing this is possible too please.

My code is

    CREATE TABLE CRM_PERS_CONTACT_INFORMAITON(
 SEQ_ID                     INT IDENTITY(1,1) not null primary key,
 PERS_ID                    INT NOT NULL,
 PREFERRED                  BIT DEFAULT (0),
 CEASED_DATE                DATETIME,
 COMMUNICATION_TYPE         VARCHAR(50),
 COMMUNICATION_VALUE        VARCHAR(255),
 DATE_ADDED                 DATETIME DEFAULT (GETDATE()),
 ADDED_BY                   VARCHAR(50),
 UPDATED_DATE               DATETIME DEFAULT(GETDATE()),
 UPDATED_BY                 VARCHAR(50)
)
GO

CREATE TABLE CRM_PERS_CONTACT_INFORMAITON_HISTORY(
 SEQ_ID                     INT IDENTITY(1,1) not null primary key,
 PERS_ID                    INT NOT NULL,
 PREFERRED                  BIT DEFAULT (0),
 CEASED_DATE                DATETIME,
 COMMUNICATION_TYPE         VARCHAR(50),
 COMMUNICATION_VALUE        VARCHAR(255),
 DATE_ADDED                 DATETIME DEFAULT (GETDATE()),
 ADDED_BY                   VARCHAR(50),
 ACTION_TYPE                VARCHAR(20)
 )
 GO

CREATE PROCEDURE MERGE_CRM_PERS_CONTACT_INFORMATION

@SEQ_ID                     INT,
@PERS_ID                    INT ,
@PREFERRED                  BIT ,
@CEASED_DATE                DATETIME,
@DATE_ADDED                 DATETIME,
@ADDED_BY                   VARCHAR,
@UPDATED_DATE               DATETIME,
@UPDATED_BY                 VARCHAR,
@COMMUNICATION_TYPE         VARCHAR,
@COMMUNICATION_TYPE_VALUE   VARCHAR,
@ACTION_TYPE                VARCHAR

AS
BEGIN

MERGE CRM_PERS_CONTACT_INFORMAITON as Target
USING (select
 @PERS_ID
,@PREFERRED
,@CEASED_DATE
,@DATE_ADDED
,@ADDED_BY
,@UPDATED_DATE
,@UPDATED_BY
,@COMMUNICATION_TYPE
,@COMMUNICATION_TYPE_VALUE
) AS Source(
PERS_ID
,PREFERRED
,CEASED_DATE
,DATE_ADDED
,ADDED_BY
,UPDATED_DATE
,UPDATED_BY
,COMMUNICATION_TYPE
,COMMUNICATION_TYPE_VALUE
)
ON (Source.PERS_ID = Target.PERS_ID
AND Source.COMMUNICATION_TYPE = Target.COMMUNICATION_TYPE)
WHEN MATCHED THEN

    update SET
     COMMUNICATION_TYPE         = @COMMUNICATION_TYPE
    ,COMMUNICATION_VALUE        = @COMMUNICATION_TYPE_VALUE
    ,UPDATED_DATE               = GETDATE()
    ,UPDATED_BY                 = @UPDATED_BY
    ,CEASED_DATE                = @CEASED_DATE
    ,PREFERRED                  = @PREFERRED

WHEN NOT MATCHED BY TARGET THEN
    INSERT (
     PERS_ID
    ,PREFERRED
    ,CEASED_DATE
    ,COMMUNICATION_TYPE
    ,COMMUNICATION_VALUE
    ,DATE_ADDED
    ,ADDED_BY
    ,UPDATED_DATE
    ,UPDATED_BY
    )
    VALUES (
     @PERS_ID
    ,@PREFERRED
    ,@CEASED_DATE
    ,@COMMUNICATION_TYPE
    ,@COMMUNICATION_TYPE_VALUE
    ,@DATE_ADDED
    ,@ADDED_BY
    ,GETDATE()
    ,@UPDATED_BY
    )
WHEN NOT MATCHED BY SOURCE THEN
DELETE;

SET @ACTION_TYPE = $action

INSERT INTO CRM_PERS_CONTACT_INFORMAITON_HISTORY(
 PERS_ID
,PREFERRED
,CEASED_DATE
,COMMUNICATION_TYPE
,COMMUNICATION_VALUE
,DATE_ADDED
,ADDED_BY
,ACTION_TYPE
)
VALUES (
 @PERS_ID
,@PREFERRED
,@CEASED_DATE
,@COMMUNICATION_TYPE
,@COMMUNICATION_TYPE_VALUE
,GETDATE()
,@ADDED_BY
,@ACTION_TYPE
);
END
GO

When you declare your variables, like @COMMUNICATION_TYPE VARCHAR, you really need to specify a length.

MSDN

  When n is not specified in a data definition or variable declaration statement, the default length is 1. When n is not specified when using the CAST and CONVERT functions, the default length is 30

So go ahead and declare them 20, 50, and 250, respectively, as you did in your table declarations

Additionally, in order to SET @ACTION_TYPE = $action you need to output this from your MERGE() statement.

...
WHEN NOT MATCHED BY SOURCE THEN
DELETE
OUTPUT $action INTO @ACTION_TYPE;

More commonly MERGE() would be used with a set. That is, you would take in a table variable into your STORED PROCEDURE and your MERGE statement would conduct it's logic on each row. Thus, you could have some rows that were INSERTED, some that were UPDATED, and some that were DELETED. In this instance, you would store the OUTPUT to a table variable or temp table.

CREATE TABLE #MyTempTable(...)
...
MERGE
...
OUTPUT $action, deleted.*, inserted.* INTO #MyTempTable;

INSERT INTO CRM_PERS_CONTACT_INFORMAITON_HISTORY
SELECT(...) FROM #MyTempTable


Logging input and log file strings This is a piece of C89 code testing a function titled logging_function(), which takes two arguments and uses them to log a bit of information.

In this case, the first argument to the function is the string to be logged, and the second argument is a string holding a path in a filesystem to a log file. The first string is printed, with a timestamp, to stdout and the log file.

Note: although yes input on the main() function would be wonderful, I'd love it if you gave thoughts on the function logging_function() itself.

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

#define EXIT_SUCCESS 0
#define EXIT_FAILURE 1

typedef struct tm tm;

int logging_function(char input_string[], char log_file_string[])
{
    /*Initializations.*/
    char output_string[32767];
    FILE* log_file_stream;
    time_t current_time_epoch_format;
    tm* current_time_calandar_format;

    /*Creating timestamp in output string, amending first argument to output string, and logging.*/
    if ((strlen(input_string) + 23) > 32767) return EXIT_FAILURE;
    if ((current_time_epoch_format = time(&current_time_epoch_format)) == -1) return EXIT_FAILURE;
    if ((current_time_calandar_format = localtime(&current_time_epoch_format)) == NULL) return EXIT_FAILURE;
    if (strftime(output_string, 23, "[%d-%m-%Y %H:%M:%S] ", current_time_calandar_format) != 22) return EXIT_FAILURE;
    if ((log_file_stream = fopen(log_file_string, "a")) == NULL) return EXIT_FAILURE;
    if (printf("%s\n", strcat(output_string, input_string)) < 1) return EXIT_FAILURE;
    if (fprintf(log_file_stream, "%s\n", output_string) < 1) return EXIT_FAILURE;
    if (fclose(log_file_stream) == EOF) return EXIT_FAILURE;
    return EXIT_SUCCESS;
}

int main(int argc, char** argv)
{
    /*Initializations.*/
    int EXIT_CODE;

    /*Print the returned integer from logging_function and exit.*/
    printf("%d\n", (EXIT_CODE = logging_function(argv[1], argv[2])));
    exit(EXIT_CODE);
}

EXIT_SUCCESS and EXIT_FAILURE are defined by stdlib.h. I think you need not define them.

You can add the const qualifier to arguments of logging_function(). Those strings are not going to be modified.

You can call fprintf() twice, once with the strftime() output and then with input_string[] and avoid the strcat().


LeetCode - Weekly Contest 213 - Kth Smallest Instructions Hi I participated in Leetcode weekly contest 213 and am facing issue with submission for below question
Kth Smallest Instructions

Bob is standing at cell (0, 0), and he wants to reach destination:
(row, column). He can only travel right and down. You are going to
help Bob by providing instructions for him to reach destination.
The instructions are represented as a string, where each character is
either:
'H', meaning move horizontally (go right), or 'V', meaning move
vertically (go down). Multiple instructions will lead Bob to
destination. For example, if destination is (2, 3), both "HHHVV" and
"HVHVH" are valid instructions.
However, Bob is very picky. Bob has a lucky number k, and he wants the
kth lexicographically smallest instructions that will lead him to
destination. k is 1-indexed.
Given an integer array destination and an integer k, return the kth
lexicographically smallest instructions that will take Bob to
destination.
Example 1 :
Input: destination = [2,3], k = 1
Output: "HHHVV"
Explanation: All the instructions that reach (2, 3) in lexicographic
order are as follows: ["HHHVV", "HHVHV", "HHVVH", "HVHHV", "HVHVH",
"HVVHH", "VHHHV", "VHHVH", "VHVHH", "VVHHH"].
Constraints:
destination.length == 2
1 <= row, column <= 15
1 <= k <= nCr(row + column, row), where nCr(a, b) denotes a choose b.

I tried to create all the possible paths lexicographically and stop when count reached k, but it giving me TLE.
Below is my code :
public class Solution {
    String res = string.Empty;
    int count = 0;
    int K;
    public string KthSmallestPath(int[] destination, int k) {
        K=k;
        this.FindAllPaths("", destination[1], destination[0]);

        return res;
    }

    public void FindAllPaths(string currPath, int hRem, int vRem)
    {
        if(count == K)   return;
        if(hRem == 0 && vRem == 0)
        {
            res = currPath;
            count++;
            return;
        }

        if(hRem != 0)
        {
            this.FindAllPaths(currPath + 'H', hRem - 1, vRem);
        }

        if(vRem != 0)
        {
            this.FindAllPaths(currPath + 'V', hRem, vRem - 1);
        }
    }
}

Any idea on how can I  optimize the code so that it passes all the test cases.

A review of your current code
class Solution uses the fields
String res = string.Empty;
int count = 0;
int K;

to pass information around between the “main” KthSmallestPath() function and the FindAllPaths() helper function. That makes the logic difficult to understand. Even worse, it causes wrong results if the function is called more than once:
String res1 = sol.KthSmallestPath(new int [] { 2, 3 }, 5);
// HVHVH (correct)
String res2 = sol.KthSmallestPath(new int [] { 2, 3 }, 1);
// VVHHH (wrong)

Try to pass all information as function arguments. If that is not possible, make sure to reset all the commonly used fields at the start of the function.
The horizontal and vertical spacing is not consistent: There should be spaces around the operator in
K=k;

and I suggest to always use curly braces with if-statements, e.g. here:
if(count == K)   return;

Finally, I had to think twice about the element of the given destination array correspond to rows/columns and to horizontal/vertical steps. Assinging these values to local variables with an explaining comment can make this more clear:
int vRem = destination[0]; // destination row = # of vertical steps
int hRem = destination[1]; // destination column = # of horizontal steps

A better approach
In the “worst case”
String res = sol.KthSmallestPath(new int [] { 15, 15 }, 155117520);

your code builds recursively \$ \binom{30}{15} = 155117520\$ strings. This is a case where the “brute force” method simply does not work, and one has to find something more efficient.
In fact it is possible to find the k-th smallest permutation of a string with N characters in O(N) time and with O(N) memory, see for example

Finding the n-th lexicographic permutation of a string on Mathematics Stack Exchange,
Find n-th lexicographically permutation of a string | Set 2 on GeeksforGeeks.

The idea is to compute at every step how many permutations start with an H and how many start with a V and then choose the next letter accordingly.
As an example, compute the 5-th permutation of HHHVV.

There are \$ \binom{5}{2} = 10 \$ permutations, and  \$ \binom{4}{2} = 6 \$ start with H. Since  \$ 5 \le 6 \$ the first letter must be H, and it remains to compute the  5-th permutation of HHVV.

Next, there are \$ \binom{4}{2} = 6 \$ permutations of HHVV, and  \$ \binom{3}{2} = 3 \$ start with H. Since \$ 5 > 3 \$ the next letter must be V, and it remains to compute the second permutation of HHV.

This process is repeated until no Hs are left, and the remaining positions are filled with V.

Here is a possible implementation, using a recursive helper function:
public class Solution {

    // From https://rosettacode.org/wiki/Evaluate_binomial_coefficients#C.23
    static int binomial(int n, int k)
    {
        if (k > n - k)
        {
            k = n - k;
        }

        int c = 1;
        for (int i = 0; i < k; i++)
        {
            c = c * (n - i);
            c = c / (i + 1);
        }
        return c;
    }

    /*
     * Returns the `k`-th permutation of a string with `numH` 'H's and
     * `numV` 'V's in lexicographic order.
     */
    static string KthPermutation(int numH, int numV, int k)
    {
        if (numH == 0)
        {
            return new String('V', numV);
        }
        else
        {
            // Number of permutations starting with 'H':
            int c = binomial(numH + numV - 1, numV);
            if (k <= c)
            {
                return 'H' + KthPermutation(numH - 1, numV, k);
            }
            else
            {
                return 'V' + KthPermutation(numH, numV - 1, k - c);
            }
        }
    }

    public string KthSmallestPath(int[] destination, int k)
    {
        int numV = destination[0]; // destination row = # of vertical steps
        int numH = destination[1]; // destination column = # of horizontal steps
        return KthPermutation(numH, numV, k);
    }
}

It computes
String res = sol.KthSmallestPath(new int [] { 15, 15 }, 155117520);

in 0.6 milliseconds on my MacBook (using Mono).


Producing variants of a password based on command-line flags I created a python script that attempts to find weak passwords based on a keyword.  I really enjoy programming with Python but I don't have a ton of experience so I'm hoping to get feedback on whether I should change how I have implemented anything below so that I am in line with best practices.

The code below is basically just processing all of the arguments and making calls to specific functions based on what flags/arguments are passed in.

Are there any obvious changes that I should make to the code below?  Is this the correct way to handle arguments of this nature?

#get the password and options from the command line
opts, args = getopt.getopt(sys.argv[1:], ':o:t:d:g:necf')
# print opts
# print args
password = args[0].lower()

outputFile = ''
target = ''
data = ''
text = ''
numbersFlag = False
copyFlag = False
fullFlag = False
requestFlag = False

for opt, arg in opts:
    if opt == "-o": #output file
        outputFile = arg
    if opt == "-f": #generate full password list
        fullFlag = True
    elif opt == "-t": #target of the POST request
        requestFlag = True
        target = arg
    elif opt == "-d": #data for the POST requet
        requestFlag = True
        data = arg
    elif opt == "-c": #copy output to the clipboard
        copyFlag = True
    elif opt == "-g": #text to be searched for in POST response
        requestFlag = True
        text = arg
    # elif opt == "-e": #append extra character
    #   letters.append(dummyCharacters)
    elif opt == "-n": #append numbers to end
        numbersFlag = True

#load full or basic password list based on arguments passed in
passwords = fullSub(password) if fullFlag else basicSub(password)
if fullFlag:
    passwords = fullSub(password)
elif numbersFlag:
    passwords = appendNumbers(password)
else:
    passwords = basicSub(password)

#save passwords to file
if outputFile != '':
    f = open(outputFile, 'w')
    for password in passwords:
        f.write("".join(password) + '\n')
    f.close()
#copy passwords to clipboard
elif copyFlag:
    pwList = ''
    i=0
    for password in passwords:
        i+=1
        pwList += "".join(password) + '\n'
    print `i` + " passwords copied to the clipboard."
    pyperclip.copy(pwList)
#make request using passwords
elif requestFlag:
    #make sure all required values were passed in
    if data == '':
        print "You must provide data in order to make a HTTP request.  Example: -d email=test@test.com&password={0}"
        sys.exit()
    elif text == '':
        print "You must specify what text to search for in the response in order to make a HTTP request. Example: -g success:true"
        sys.exit()
    elif target == "":
        print "You must specify a target URL in order to make a HTTP request"
        sys.exit()
    makeRequests(target, data, passwords,text)
else:
    i = 0
    for password in passwords:
        i+=1
        print "".join(password)
    print `i` + " passwords generated."

Some remarks about your code:

Use if __name__ == '__main__':
In my opinion argparse is better then the getopt way,
Divide your code into functions for seperation of control
Use string formatting
Uphold PEP8 guidelines for a cleaner code

1.

def main():
    parse_options()
    do_stuff()

if __name__ == '__main__':
    main()

2.
Example of parsing options via argparse

def parse_options():
    parser = argparse.ArgumentParser(usage='%(prog)s [options] <password>',
                                     description='Password-Tool',
                                     formatter_class=argparse.RawDescriptionHelpFormatter,
                                     epilog=
'''
Examples
'''

                                        )
    parser.add_argument('-o', action="store_true", help='output file')
    ...
    args = parser.parse_args()

3.
When dividing code into seperate functions it makes for a more readable code, and that way it can be imported by other programs.

4.
When using print formatting looks better and correct way would be:

print '{} some text here'.format(i)


Arrow key input to change selected row and column in a grid of HTML elements Below is the working code but it can surely be simplified with some sort of changeCol() and changeRow() functions, or perhaps a single function?

$(document).ready(function(e){
    let keys = {};
    let selRow = 0;
    let selCol = 0;
    let rowCount = 5;
    let colCount = 5;

    //set initial highlits

    $('[data-row="0"]').addClass('hl');
    $('[data-col="0"]').addClass('hl');

    $(document).keydown(function(event){
        keys[event.which] = true;
    }).keyup(function(event){
        delete keys[event.which];
    });

        for (let row = 0; row < rowCount; row++) {
        for (let col = 0; col < colCount; col++) {
            let elPiece = $("<span>", {
                "class": 'piece',
                "data-col": col,
                "data-row": row,
                text: "O"
            });
            $('#board').append(elPiece)
        }
    }

    function gameLoop() {
        // w for north
        if (keys[87]) {
            shiftCol(selCol, -1);
        }
        //press s for south
        if (keys[83]) {
            shiftCol(selCol, 1);
        }

        //up arrow
        if (keys[38]) {
            $('[data-row="' + selRow + '"]').removeClass('hl');
            $('[data-col="' + selCol + '"]').addClass('hl');
            if (selRow === 0) {
                selRow = rowCount - 1;
            } else {
                selRow--;
            }
            $('[data-row="' + selRow + '"]').addClass('hl');
        }

        //down arrow
        if (keys[40]) {
            $('[data-row="' + selRow + '"]').removeClass('hl');
            $('[data-col="' + selCol + '"]').addClass('hl');
            if (selRow === rowCount - 1) {
                selRow = 0;
            } else {
                selRow++;
            }
            $('[data-row="' + selRow + '"]').addClass('hl');
        }

        //left arrow
        if (keys[37]) {
            $('[data-col="' + selCol + '"]').removeClass('hl');
            $('[data-row="' + selRow + '"]').addClass('hl');
            if (selCol === 0) {
                selCol = colCount - 1;
            } else {
                selCol--;
            }
            $('[data-col="' + selCol + '"]').addClass('hl');
        }

        //right arrow
        if (keys[39]) {
            $('[data-col="' + selCol + '"]').removeClass('hl');
            $('[data-row="' + selRow + '"]').addClass('hl');
            if (selCol === colCount - 1) {
                selCol = 0;
            } else {
                selCol++;
            }
            $('[data-col="' + selCol + '"]').addClass('hl');
        }
        // code to move objects and repaint canvas goes here

        setTimeout(gameLoop, 100);
    }
    gameLoop();
});
#board {
  width: 150px;
  height: 150px;
}

.piece {
  width: 30px;
  height: 30px;
  display: inline-block;
  background-color: black;
  color: white;
  text-align: center;
  line-height: 30px;
}

.hl {
  background-color: gray;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<div id="board">

</div>

Ok, after redoing it from scratch, here's a few points to consider:

Instead of checking if the key "exists" check if it's true/false
You can make it more flexible by making the size of the tile a variable (or even better, a variable for height and one for width)
It's better to store the tiles into an array, with their respective x and y variable. This way, you don't have to parse the DOM with jQuery each iteration of the loop, you only loop through the array and select the tile element.

Here's the new code:

HTML - Really simple, just the container for the tiles

CSS - We don't set the size of anything, we'll calculate it with JS

*{
  box-sizing: border-box;
  margin: 0;
  padding: 0;
}

body{
  background: #f1f1f1;
}

html,
body{
  width: 100%;
  height: 100%;
  display: flex;
  align-items: center;
  justify-content: center;
}

.board{
  display: flex;
  flex-wrap: wrap;
}

.tile{
  background-color: #000;
  color: #fff;
  display: flex;
  align-items: center;
  justify-content: center;
  font-size: .75rem;
}

.active{
  background: #aaa;
}

JS

//amount of tiles horinzontally and vertically
var tileAmountX = 15;
var tileAmountY = 15;

//actual column/row
var actualX = 0;
var actualY = 0;

//size in pixels of the tiles
var tileWidth = 40;
var tileHeight = 40;

//size in pixels of the board, calculated with tile size and tile amount
var boardWidth = tileWidth * tileAmountX;
var boardHeight = tileHeight * tileAmountY;

//array for storing the tiles
var tiles = [];

//keys object, default state are all off (false)
var keys = {left: false, top: false, right: false, down: false};

//set board size style
$('.board').css({
  width: boardWidth + 'px',
  height: boardHeight + 'px'
});

//generate tiles
for(var i = 0; i < tileAmountX; i++){
  for(var j = 0; j < tileAmountY; j++){

    //create tile and store it in temporary variable
    var el = $('
O
').appendTo('.board');

    //store that variable in the `tiles` array, with the respective `x` and `y` coordinate
    tiles.push({
      el: el,
      x: i,
      y: j
    });
  }
}

//move to right function, just changes the actualY value and check for overflow
function moveRight(){
  actualY = actualY < tileAmountY - 1 ? actualY + 1 : 0;
}

//move to left function, just changes the actualY value and check for overflow
function moveLeft(){
  actualY = actualY > 0 ? actualY - 1 : tileAmountY - 1;
}

//move to top function, just changes the actualX value and check for overflow
function moveTop(){
  actualX = actualX > 0 ? actualX - 1 : tileAmountX - 1;
}

//move to down function, just changes the actualX value and check for overflow
function moveDown(){
  actualX = actualX < tileAmountX - 1 ? actualX + 1 : 0;
}

//generic move function
function move(){
  //get the tiles that have the X or Y coordinate from actualX or actualY
  var starting = tiles.filter(function(val){
    return (val.x == actualX || val.y == actualY);
  });

  //remove the active class for all tiles
  for(var i = 0; i < tiles.length; i++){
    var tile = tiles[i];
    tile.el.removeClass('active');
  }

  //add the active class for the tiles that have the X or Y coordinate from actualX and actualY
  for(var i = 0; i < starting.length; i++){
     var tile = starting[i];
     tile.el.addClass('active');
  }
}

function loop(){

  //this is pretty self explanatory
  if(keys.right){
    moveRight();
  }else if(keys.left){
    moveLeft();
  }

  if(keys.top){
    moveTop();
  }else if(keys.down){
    moveDown();
  }

  //this function runs regardless of the user input
  move();

  //loop with 30fps
  setTimeout(loop, 1000/30);
}

//initiate loop
loop();

//event listener for keydown and keyup
$(window).on('keydown', function(e){
  if(e.keyCode==37){keys.left = true}else if(e.keyCode==39){keys.right = true};
  if(e.keyCode==38){keys.top = true}else if(e.keyCode==40){keys.down = true};
});

$(window).on('keyup', function(e){
  if(e.keyCode==37){keys.left = false}else if(e.keyCode==39){keys.right = false};
  if(e.keyCode==38){keys.top = false}else if(e.keyCode==40){keys.down = false};
});

If you want to see a live example, check this link:

https://codepen.io/tyrellrummage/full/bobxPp/

As you can see, even with a big board (15x15) the motion is very fluid and responsive.

There may be a better way to achieve this, but this is the best I can come up with!


VBA Excel Game - Testing My brother bet me that I couldn't make a game in Excel. The code is all over the place and buggy in parts. But I would appreciate it if anyone would give this game a go.

The music and the code has all been made by me, but the artwork was found online and I can't claim any of it is mine (although a few bits were photoshopped to make it work for me).

There's still a few bits of the game that don't work 100% but it shouldn't crash on you.

Full code

There's an installer or the zip file - it's probably best to just use the zip file.

Sub dogame()

Application.Visible = False

loadedgame = False

Do

    fminitScreen.Show
    playgame

    If loadedgame = False Then

        loadplayer
        fmNameHim.Show
        fmChooseWeapon.Show

            If Worksheets("options").range("B3").value = True Then

                fmrules.Show

            End If

        fminside1.Show

    ElseIf loadedgame = True Then

        Dim obj As Object
        Dim obj1 As Object

        Load fminside1
        Load fmoutside
        Load fmupstairs

        For Each obj In UserForms

                If obj.Name = player.formname Then

                            loadedgame = False
                            obj.Character.left = player.left
                            obj.Character.top = player.top
                            obj.Show
                            Exit For
                Else
                    If obj.Name = "fmMusic" Then

                    Else
                        Debug.Print "unloaded form" & obj.Name
                        Unload obj

                    End If

                End If

            Next obj

    End If

    fmranking.Show
    Set player = Nothing

Loop

End Sub

I'm going to go down through the code line by line. I may be harsh at times, but remember, I say these things so that you might become a better programmer.

Sub dogame()

The sub name is a bit hard to read. I personally like the .Net capitalization standards where methods are PascalCased. Isn't DoGame easier to read? Also, get into the habit of explicitly declaring scope.

Application.Visible = False

I like that you're hiding the Excel instance from the user, but what happens if an error occurs? Without an error handler to regain control and make Excel visible again, you'll end up with an instance of Excel that you can only kill via the task manager.

loadedgame = False

Remember those naming conventions I mentioned earlier? Variables should be camelCased. So, loadedGame. However, I think it's easier to read as isGameLoaded.

The next thing I want you to do is start using Option Explicit in all of your vba code. It forces you to declare all of your variables. This will turn typos into compile time errors. It's wonderful. Trust me, just do it.

Lastly, booleans default to false, so if you declare the variable using a Dim statement, there is no need to explicitly set it to false.

To recap so far:

Option Explicit

Public Sub
    On Error GoTo ErrHandler

    Dim isGameLoaded As Boolean

    Application.Visible = False

    '...

    Exit Sub

ErrHandler:
    Application.Visible = True

End Sub

Which brings us to the Do loop. Friend, that is just awful. You've intentionally created an infinite loop. Once the DoGame is called, it never exits. Never. The only way I can figure to kill this is to open the task manager and kill Excel.

What you should do is have a While loop that exits when the game is over. For example, consider this code where GetGameState is a function that returns a boolean.

Dim gameOver As Boolean

While Not gameOver

    gameOver = GetGameState

End While

fminitScreen.Show
playgame

Again with the capitalization. This is hard to read. Not much else to say other than if you must use Hungarian notation for your user forms, then you should at least use Microsoft's recommended notation. fminitScreen should be frmInitScreen. But really InitializationScreen or StartScreen would be much better names. (I was going to provide a link, but it seems that MS has scrubbed that document from the face of the Earth...)

Skipping ahead a bit, there's this.

If Worksheets("options").range("B3").value = True Then

First, we have no clue what this value is, other than it's some option of some kind. I recommend creating a well named function to retrieve this value.

Secondly, the statement can be simplified by omitting = True. This is essentially what happens right now.

 Worksheets("options").range("B3").value = True
' get value from worksheet
True = True
' is true equal to true?
True
' enter if statement

That's why you can write this like this.

If Worksheets("options").range("B3").value Then

Same thing here. Simplify it.

 ElseIf loadedgame = True Then

ElseIf isGameLoaded Then

This is bad. Could these names be more generic?

    Dim obj As Object
    Dim obj1 As Object

What really makes this terrible is the type doesn't even give us a hint because you've declare them as generic objects. You could literally stuff any class instance you wanted to into those variables.

It looks like you're using obj to iterate the UserForms collection

   For Each obj In UserForms

So why not declare it as a UserForm???

Dim obj As UserForm

Or better yet

Dim frm As UserForm


Java Node class for a Graph I'm using ArrayList<Nodes> to store adjacent nodes, and ArrayList<Integer> to store the corresponding weights of the vertices. Is this efficient if I only use it for small inputs? Is it efficient for large inputs as well?

class Node {
    public ArrayList<Node> adjacent = new ArrayList<Node>();
    public ArrayList<Integer> weights = new ArrayList<Integer>();
    boolean visited;
}

Is this efficient if I only use it for small inputs?

For small inputs, anything is efficient.
For big data, having each Node as such a large object would be inefficient.
I depends what kind of actions you would do over the graph. If you access it sequentially, there can be also a linked implementation like:

class LinkedNode {
    public Node firstChild;
    public Node nextSibling;
    boolean visited;
}

Here's an alternative, without objects but with arrays instead:

int[] edgeWeights;
int[] edgeIndicies;

At the begin, edgeIndicies will have values [0,1,2,...n].
Sorting the weights, you move the indicies instead, so if the indicies are the following:
[5,7,9,...] you know edgeWeights[5] is the smallest, edgeWeights[7] is the second smallest, ...etc.


Is ThreadPool QueueUserWorkItem best option for handling event? I have a WinForms application that has value change event coming from ble device.
The event can be hit from more than one device also one device can send a notification to event in every 4 secs. averagely so I am adding device data to a queue to use data after that. This method works but after a while stops working. Is ThreadPool the best option to handle this kind of situation?

protected void Characteristic_ValueChanged(GattCharacteristic sender, GattValueChangedEventArgs eventArgs)
    {
        ThreadPool.QueueUserWorkItem((x)=>
        {

            GattCharacteristic characteristic = sender;
            string data = GetByteData(characteristic.Service.Device.BluetoothAddress).Result;
            queue.Enqueue(new EventDataModel { IOData = characteristic, IOKey = characteristic.Service.Device.BluetoothAddress.ToString() });

        });

    }

Without seeing the rest of your code it's hard to know what's going on but this is always* bad:

string data = GetByteData(characteristic.Service.Device.BluetoothAddress).Result;

* as long as GetByteData is async and returning a Task<string>

You should queue an async delegate instead:

ThreadPool.QueueUserWorkItem(async x =>
    {
        GattCharacteristic characteristic = sender;
        string data = await GetByteData(characteristic.Service.Device.BluetoothAddress).ConfigureAwait(false);
        queue.Enqueue(new EventDataModel { IOData = characteristic, IOKey = characteristic.Service.Device.BluetoothAddress.ToString() });
    });

Note the use of await with ConfigureAwait(false). See Stephen Cleary's blog to learn more about why blocking on async code is such a bad idea.


Catching Errors in Factory Method with express.js My express.js application uses a lot of promises for interacting with a variety of services. Instead of including try/catch with each one, I've setup a fail safe factory method for all my express controllers. This should only be called when an unexpected error occurred. For example, 404 when trying a public API. I'm skeptical about how useful this is or if there is a much easier way to do this. Also looking for any other comments on this code or question.
//index.ts

import * as Express from 'express';
import { Auth } from '../class/Auth';

/**
 * Handler for async static functions recognizing that errors
 * should always be sent to the error handler. This is where
 * our error stone drops. This will eventuall replace go().
 * @param  callback function to be run
 * @return function for express to run
 */
function $(callback:Function):Function {
  return async (req:Express.Request, res:Express.Response, next:Express.NextFunction) => {
    try {
      await callback(req, res, next);
    } catch(e) {
      console.error("Something strange occurred",e);
      res.status(500).render('error', {
        error: "An internal error has occurred",
        errorCode: 500
      });
    }
  }
}

/**
 * Controller to log the user in
 * @param  req Express Request
 * @param  res Express Response
 */
function async loginController(req:Express.Request, res:Express.Response) {
  // verify parameters
  if(!(req.body['username'] && req.body['password'])){
    return res.status(400).end("Please include username and password");
  }

  var user = await Auth.login(req.body['username'], req.body['password']);

  if (user) {
    var authToken = Auth.generateAuthToken(user);
    res.json({
      authToken:authToken
    });
  } else {
    return res.status(401).end("Wrong username or password");
  }
}

var app = Express();
app.post('/api/login', $(loginController));
app.listen(8080);

// Auth.ts

import { User } from './User';

/**
 * Authentication base class
 */
export class Auth {
  /**
   * Log user in
   * @param email
   * @param password
   * @returns user logged in. Returns null if user is not found or authentication fails
   */
  public static async login(email:string, password:string):Promise<User> {
    var response = await fetch("example.com/api/login", {
      method: 'POST',
      headers: { 'Content-Type': 'application/json' },
      body: JSON.stringify({
        email: email
        password: password
      })
    });

    if(response.ok){
      return await response.json() as User;
    }

    // password or username was wrong
    if(response.status == 401){
      return null;
    }

    // an unknown error occurred
    throw Error(response.status);
  }

  /**
   * Generate AuthToken
   * @param user
   * @return token
   */
  public static generateAuthToken(user:User):string {
    return jwt.sign({ userid: user.id }, process.env.SECRET, {
      expiresIn: process.env.JWT_EXPIRY
    });
  }
}

```

First of all, by default ExpressJS does not support async routes. To solve that you can use express-async-errors plugin. Just add the following at the beginning of the file:
import 'express-async-errors'

Now ExpressJS will await promises and handle promise rejections. Then you can create custom errors handler:
export function handleApiErrors(): (err: unknown, req: Request, resp: Response, next: NextFunction) => unknown {
    return (err, _req, resp, next) => {
        if (err) {
          console.error(String(err))
          return resp.status(500).json({
              error: 'unknown error',
          })
        }
        next(err)
    }
}

Then include custom error handler just before app.listen:
app.use(handleApiErrors())

Now each route can return a promise and rejected promises will be returned as 500 response. You can also include http-errors to further improve error handling.

You can check the live example on torrent-stream-server

Other observations
null return
login has Promise<User> type, but you return null. If you don't get typescript error, you should consider enabling strictNullChecks option.
res.end and res.json
You mix res.json with res.end. Maybe some middleware handles, that res.end would return JSON. Or maybe you just return plain text response on error? In any case, it's confusing, so you should always use res.json() (you can chain it with end if you like, but I don't see a reason).
No spaces
I see that you don't like spaces. But it's hard to read with that style. You should at least add spaces after :. You should find a styleguide you like and follow it. Or you can just use prettier


my solution for a typesafe indexer I've searched the web if there is a generic indexer in c# (4.0 / WPF) but could not find anything about it.

So I came up with my own solution and would like to share it with you. I hope to get some feedback on improvements.

public dynamic this[string attribute, Type type]
{
    get
    {
        try
        {
            return Convert.ChangeType(base.GetAttribute(attribute), type);
        }
        catch (Exception)
        {
            return this[attribute];
        }
    }
}

Short explanation of how i use it: my class derives from a generic class which again derives from XmlElement (WPF). The generic class offers a generic method to add XmlAttributes where their can be of any data type.

Here are some examples of usage:

    task.AddAttribute<bool>("boolAttrib");
    bool boolAttrib = false;
    boolAttrib = task["boolAttrib", boolAttrib.GetType()];

    task.AddAttribute<DateTime>("datetimeAttrib", new DateTime(2013, 11, 4));
    DateTime datetimeAttrib = new DateTime();
    datetimeAttrib = task["datetimeAttrib", datetimeAttrib.GetType()];

As there is no such thing as a generic indexer and the "out" keyword cant be used this was the only solution i came up to.

Any advice to improve it is highly appreciated! Thanks! :)

You can use generics if you don't insist on it being an indexer:

public T GetAttribute<T>(string attribute)
{
    return (T)base.GetAttribute(attribute);
}

I'm not so sure about the other things you're doing. Convert.ChangeType() could just hide errors in your code. If you want to change the type of something, you should know about it and do it explicitly, not try to hide it.

The same applies to your catch. You should never catch (Exception) in library code, handle only the exceptions you know about (in this case, most likely just InvalidCastException). And returning a value that might be the wrong type on failure? That seems dangerous to me as well.


Reading the first n bits of a byte The way I am using this method assumes all data is byte-aligned, so I don't really care about the rest of the bits.

This method is written in an extension of BinaryReader which provides more methods when I'm reading binary data from files. The BinaryReader provided does not appear to support reading bits.

Is there a better way I could write this method? Also I'm not sure if there's a more direct way to build a bitmask so I did it the long way.

/* Returns the value of the first n bits. */
public byte ReadBits(byte n)
{
    byte val = base.ReadByte();
    byte sum = 0;
    for (int i = 0; i < n; i++)
        sum += (byte)(val & (1 << i));
    return sum;
}

I am using .NET 3.5

I have not tested this that well. If anyone finds a bug, please let me know.

// Throws away the unused bits
public byte ReadFirstNBits(int n)
{
    Contract.Requires(n >= 0 && n <= 8, "Code contract may be too slow for your needs");
    byte val = base.ReadByte();
    return val >> (8 - n);
}

// Throws away the unused bits
public byte ReadLastNBits(int n)
{
    Contract.Requires(n >= 0 && n <= 8, "Code contract may be too slow for your needs");
    byte val = base.ReadByte();
    byte mask = (byte)((1 << n) - 1);
    return val & mask;
}

You can try this for yourself if you have a Python prompt:

>>> a = 0b11001001
>>> a
201
>>> a >> 4
12
>>> a >> 5
6
>>> a >> 3
25
>>> a >> 6
3
>>> a >> 2
>>> for i in range(8): print bin(a >> i)

0b11001001
0b1100100
0b110010
0b11001
0b1100
0b110
0b11
0b1
>>>


Angularjs extends service As usual I'm wondering if there is a better way to do:

(The code extends a angular service)

(function(window, angular, undefined) {
        'use strict';
        angular.module('api.base', ['restangular'])
            .factory('Base', function(Restangular) {
                return function(route){
                    var elements = Restangular.all(route);
                    return {
                        one : function (id) {
                            return Restangular.one(route, id).get();
                        },
                        all : function (id) {
                            return elements.getList();
                        },
                        store : function(data) {
                            return elements.post(data);
                        },
                        copy : function(original) {
                            return Restangular.copy(original);
                        }
                    }
                }
            })
    })(window, angular);

    (function(window, angular, undefined) {
        'use strict';
        angular.module('api.post', ['api.base'])
            .factory('Post', function(Base) {
                function ngPost() {
                    this.prop = ['publish','draft'];
                    this.myMethod = function(){}
                };
                return angular.extend(Base('post'), new ngPost());
            })
    })(window, angular);

At first sight, your code is short and there is nothing wrong with it.

At second sight, there are a few things to improve:

Your semicolons are all over the place; you have both missing and pointless semicolons.
There is no point in declaring id in all since you clearly will not use it
ngPost is an old skool constructor, it's name really should start with an uppercase

These things I gleaned from JSHint, you should use it.

Furthermore, I could be wrong, it seems that you are mixing functions which I would put in posts together with functions which I would put in post. I am not sure that is the best approach.


Return Function Pointer based upon String I have the following function that takes a string and returns a function pointer; currently its a long if-else block; thats continuing to grow this will make it difficult to maintain.

The logic of this function is repeated but the return type has changed.

typedef int (*FP)(char** params, int numberOfParams);
typedef int (*FP1)(int x);

static FP getFunction1(char* name)
{
  if(strcmp(name, "str1") == 0)
  {
      return fun1;
  }
  else if(strcmp(name, "str2") == 0)
  {
      return fun2;
  }
}

static FP1 getFunction2(char* name)
{
  if(strcmp(name, "str1") == 0)
  {
      return fun3;
  }
  else if(strcmp(name, "str2") == 0)
  {
      return fun4;
  }
}

Is there a better way that this could be refactored to remove the duplicate code?

You could create a table mapping strings to functions:

typedef struct {
    char* name;
    FP    func;
} FP_mapping;

FP_mapping FP_map[] = {
    { "str1", fun1 },
    { "str2", fun2 },
    { NULL, NULL }
};

Then iterate over the array:

static FP getFunction1(char* name)
{
    for (int i=0; FP_map[i].name != NULL; i++)
    {
        if strcmp(FP_map[i].name, name)
        {
            return FP_map[i].func;
        }
    }
}


Optimizing a Solution to an Overdetermined System Hearthstone is a collectible card video game similar to Magic: The Gathering, where players play cards with certain values of "attack" and "health" for certain "mana" costs. After being introduced to some of the basics of R, I decided that I would find some of the "values" of these characteristics by finding the best solutions to an overdetermined system, as demonstrated here.

Let a card's mana cost, attack, health, and card value* be represented as the vector (m,a,h,v). Three cards are (1,1,2,1), (2,3,2,1), (4,4,5,1), and (7,9,5,1). This corresponds to the overdetermined system a+2h+v=1, 2a+3h+v=2, 4a+5h+v=4, 9a+5h+v=7. The below code would find values of a, h, and v that minimize the sum of the squared differences between the LHS and RHS of the four equations (more cards are shown than the example).

*Card value is the inherent value in having a card to play, as opposed to not having a card to play

numberOfCards = 9
numberOfEffects = 3 # Must be less than or equal to numberOfCards

# Mana, Attack, Health, Card Value

allCards = matrix(
    c(1,2,1,1),                       # Murloc Raider
    ncol = numberOfEffects + 1,
    byrow = TRUE
    )
allCards = rbind(allCards,c(2,2,3,1)) # River Crocolisk
allCards = rbind(allCards,c(2,3,2,1)) # Bloodfen Raptor
allCards = rbind(allCards,c(3,5,1,1)) # Magma Rager
allCards = rbind(allCards,c(4,2,7,1)) # Oasis Snapjaw
allCards = rbind(allCards,c(4,4,5,1)) # Chillwind Yeti
allCards = rbind(allCards,c(6,6,7,1)) # Boulderfist Ogre
allCards = rbind(allCards,c(7,7,7,1)) # War Golem
allCards = rbind(allCards,c(7,9,5,1)) # Core Hound

coefficientsMatrix = matrix(
    NA,
    nrow = numberOfEffects,
    ncol = numberOfEffects,
    byrow = TRUE)
for(i in 1:numberOfEffects) {
    for(j in 1:numberOfEffects) {
        coefficientsMatrix[i,j] = sum(allCards[,i+1] * allCards[,j+1])
    }
}

constantsVector = vector(mode = "integer", length = numberOfEffects)
for(i in 1:numberOfEffects) {
    constantsVector[i] = sum(allCards[,1] * allCards[,i+1])
}

effectValues = solve(coefficientsMatrix, constantsVector, tol = 1e-9)
cat(effectValues, "\n\n") # For demonstration of values of each effect

cardValueMatrix = allCards[,-1]

cardValues = vector(mode = "integer", length = numberOfCards)
for(i in 1:numberOfCards) {
    cardValues[i] = round(sum(cardValueMatrix[i,] * effectValues), digits = 3)
    cat("Card ", i, ": ", cardValues[i], "\n", sep = "")
}

I know of a couple things that could be improved in this code. The most glaring improvement that needs to be made is the creation of coefficientsMatrix; I've read that making a matrix of the correct size and filling it with NA only to fill in its values later is very memory-intensive. Additionally, I am unsure if the way I create the allCards matrix with rbind is the best way to do this.

Here is how you can completely vectorize your code, making use of matrix functions such as crossprod, colSums, and %*%. Without for loops, your code will be more concise and faster to execute.

The construction of allCards was indeed a problem. Incrementally growing an object like you did is very memory intensive: each time you add a row, a new object is built from scratch. On the other hand, your comment that I've read that making a matrix of the correct size and filling it with NA only to fill in its values later is very memory-intensive is not accurate. Here the object is created once and filled in-place by the for loops.

To summarize: growing an object iteratively is the worst. Initializing an object to its final size and filling it in place via for loops is preferable. Using a vectorized function to create the object in one shot is the best. Hope it helps!

numberOfCards <- 9
numberOfEffects <- 3

allCards <- rbind(
  c(1,2,1,1), # Murloc Raider
  c(2,2,3,1), # River Crocolisk
  c(2,3,2,1), # Bloodfen Raptor
  c(3,5,1,1), # Magma Rager
  c(4,2,7,1), # Oasis Snapjaw
  c(4,4,5,1), # Chillwind Yeti
  c(6,6,7,1), # Boulderfist Ogre
  c(7,7,7,1), # War Golem
  c(7,9,5,1)  # Core Hound
)

cardMana        <- allCards[, 1]
cardValueMatrix <- allCards[, -1]
coefficientsMatrix <- crossprod(cardValueMatrix)
constantsVector <- colSums(cardMana * cardValueMatrix)
effectValues <- solve(coefficientsMatrix, constantsVector, tol = 1e-9)
cat(effectValues, "\n\n")
cardValues <- cardValueMatrix %*% effectValues
cat(sprintf("Card %i: %.3f", seq(numberOfCards), cardValues), sep = "\n")


Can this be written more concise and aligned with the Functional Reactive Programming paradigm? Trying to learn FRP and Elm. Is the program below "ok" from the perspective of FRP and Elm? What could be improved? The program can be executed here: http://elm-lang.org/try

import Random
import Mouse
data Fruit = Apple | Orange | Banana | Melon | Guava
rand = Random.range 0 4 (Mouse.clicks)
fruit n = if | n == 0 -> Apple
             | n == 1 -> Orange
             | n == 2 -> Banana
             | n == 3 -> Melon
             | otherwise -> Guava
fruitText = toForm <~ (asText <~ (fruit <~ rand))
time = lift (inSeconds . fst) (timestamp (fps 40))
scene dy form = collage 300 300 [move (0, 100 * cos dy) form]
main = lift2 scene time fruitText

Two months late, but I will try to answer nevertheless. ;-)

Since your rand already makes sure the numbers will be valid indices for your fruits, you don't have to use so many if cases. The rest looks OK to me, but I made some annotations in the source.

import Random
import Mouse
-- Sometimes empty lines can improve readability.

data Fruit = Apple | Orange | Banana | Melon | Guava

-- For module global declarations, it is often helpful to
-- explicitly annotate the types.
fruits : [Fruit]
fruits = [Apple, Orange, Banana, Melon, Guava]

-- especially for functions :)
fruit : Int -> Fruit
fruit n = head <| drop n fruits

rand : Signal Int
rand = Random.range 0 ((length fruits) - 1) Mouse.clicks

-- exercise: Try to figure and annotate the remaining types. ;-)
fruitText = toForm <~ (asText <~ (fruit <~ rand))

time = lift (inSeconds . fst) (timestamp (fps 40))

scene dy form = collage 300 300 [move (0, 100 * cos dy) form]

main = lift2 scene time fruitText

btw: share-elm.com is a bit easier for showing elm code to someone than making him copy paste to elm-lang.org/try.
See: http://share-elm.com/sprout/52567327e4b0d6a98b1531c7


SQLAlchemy code for calling parameters I'm using the following function to process API parameters for an specific endpoint. Using SQLAlchemy and Flask, I want to get feedback in terms of simplifying calling SQL code and nested statements.

date_filter = r'\d{4}-((1[0-2]|[1-9])|0[1-9])-((3[01]|[12][0-9]|[1-9])|0[1-9])'

def bits_filter(request):
    """

    :param request:
    :return:(Model.Bits) List of bits
    """
    # Get parameters.
    log.info('/bits request: %s', request)
    # Date parameters.
    date_value = request.args.get('date')
    # Bit or newsletter.
    bit_type_value = request.args.get('bit_type')

    # Process parameters.
    date_value = date_value or settings.DATE_VALUE
    bit_type = bit_type_value or settings.BIT_TYPE
    log.info('bit_type: %r', bit_type)

    if date_value and isinstance(date_value, unicode):
        # Bit type.
        if date_value.lower() == u'latest':
            return Model.Bits.query.filter(Model.Bits.bit_type == bit_type).order_by(
                Model.Bits.published.desc()).limit(settings.max_bits).all()
        # Dated format.
        elif re.match(date_filter, date_value):
            return Model.Bits.query.filter(
                Model.Bits.bit_type == bit_type & Model.Bits.published == date_value).limit(
                settings.max_bits).all()
        else:
            log.error('Invalid date value parameter: %r', date_value)
            return None
    else:
        return Model.Bits.query.filter(Model.Bits.bit_type == bit_type).order_by(
            Model.Bits.published.desc()).limit(settings.max_bits).all()

I would re-structure code to improve on readability and avoiding code duplication while decreasing the nestedness depth level - remember that "Flat is better than nested".

I would probably first fail fast if date value is given and has an invalid format. Then, I would define a queryset variable which will help us keep the common parts of the query while building it.

Also, you may pre-compile the regular expression and define it as a proper Python constant - in upper case with underscores.

Something along these lines:

# fail fast if the date value is invalid
if date_value and not DATE_FILTER_PATTERN.match(date_value):
    log.error('Invalid date value parameter: %r', date_value)
    return None

queryset = Model.Bits.query.filter(Model.Bits.bit_type == bit_type)
if not date_value or date_value.lower() == 'latest':
    queryset = queryset.order_by(Model.Bits.published.desc())
else:
    queryset = queryset.filter(Model.Bits.published == date_value)

return queryset.limit(settings.max_bits).all()

where DATE_FILTER_PATTERN is defined as:

DATE_FILTER_PATTERN = re.compile(r'\d{4}-((1[0-2]|[1-9])|0[1-9])-((3[01]|[12][0-9]|[1-9])|0[1-9])')


Taking data from a survey I wrote this Ruby code that takes data from a survey.  Seeing as it is my first Ruby project, I know it can be written much better. How could some of this code be written in more idiomatic Ruby? This code is extracted from a Rails model (the rest of the model is tame).

def prepare_anova_data

  n = surveys_completed

  (1..n).each do |i|
    anova_summary = create_anova_summary!(no_clusters: i)

    Axis.all.each do |axis|
      clusters_for_axis = cluster(axis.id, i)

      clusters_for_axis.each_with_index do |cluster_axis_summary, index|

        cluster_name = cluster_axis_summary[:name]
        scores = cluster_axis_summary[:data]

        scores.each do |key, value|
          anova_summary.anova_datas.create!(no_clusters: i, cluster_no: index, question_code: key, mean_score: value)
        end
      end
    end
  end
end

def cluster(axis_id, no_clusters)

  # Get all the questions for the axis specified
  @questions ||= self.survey.questions.where(axis_id: axis_id).pluck('id')

  clusterer =diana_clusterer(@questions, no_clusters)

  # Calculate the mean scores per cluster, eg. Cluster 1: question 1: 2, question 2, 3.5 etc.
  series_data = []
  clusterer.clusters.each_with_index do |cluster, index|

    question_means_for_cluster = {}

    # For every question, go through the current cluster and add up all of the scores to
    # caluculate the cluster mean
    for i in 0...@questions.count
      sum = 0
      count = 0

      # data_items is an array containing each respondent's survey scores (as an array)
      cluster.data_items.each do |item|
        sum += item[i]
        count += 1
      end

      question_means_for_cluster[Question.survey_code(@questions[i])] = (sum.to_d / count).round(2)
    end

    # Chartkick requires a hash in this format
    series_data << {name: "Cluster #{ index + 1 }", data: question_means_for_cluster}
  end

  # Don't forget to add the overall mean scores to the result.
  series_data << overall_mean_values(clusterer, @questions)
end

def overall_mean_values(clusterer, questions)

  overall_question_means = {}

  # For each question, go through every item (array of individual survey scores) in every cluster and add up the scores
  # This gets the Survey Group average for each question in the survey
  for i in 0...questions.count
    sum = 0
    count = 0

    # Unpack the clusters here, we don't need the clusters, we just want to use the already retrieved data for
    # this survey group and merely sum it all up by question.
    clusterer.clusters.each do |cluster|
      cluster.data_items.each do |item|
        sum += item[i]
        count += 1
      end
    end

    overall_question_means[Question.survey_code(questions[i])] = sum.to_d / count
  end

  # This format is required for the Chartkick libary
  { name: 'Overall', data: overall_question_means }
end

private

def diana_clusterer(questions, no_clusters)

  # Get all the responses for the survey group into an array (of arrays)
  answers = []
  self.members.each do |member|
    answers << member.responses.where(question_id: questions).pluck('score')
  end

  # Use the AI4R library to create a cluster using the Diana Algorithm
  data_set = DataSet.new(data_items: answers, data_labels: questions)
  Diana.new.build(data_set, no_clusters)
end

Thanks for posting your code.  You've done a good job with:

formatting
variable and method names
relatively small, focused methods

You may be new to Ruby, but I don't think you're new to programming.

There's not much to be improved here.  A few Ruby idioms you missed, and a few matters of style that aren't all that important.

Matters of Style

Line width

Some of the lines are a bit long.  While there is no longer universal agreement that lines of code should be kept to less than 80 characters, there are many benefits to doing so.  One of them is that, when you paste code into StackOverflow, the reader won't have to bother with horizontal scroll bars.  Consider breaking long lines up.  Usually, you can just hit enter where you want it, indent, and the code will still work fine.  Sometimes, you'll need to add a \ (continuation) to the end of a line to let Ruby know that the rest of the statement is on the following line.

For example, this:

series_data << {name: "Cluster #{ index + 1 }", data: question_means_for_cluster}

could be formatted like this:

series_data << {
  name: "Cluster #{ index + 1 }",
  data: question_means_for_cluster
}

Vertical white space

There's more vertical white space than I'd use.  I'd get rid of most, if not all of it.  For example, instead of this:

  clusters_for_axis = cluster(axis.id, i)

  clusters_for_axis.each_with_index do |cluster_axis_summary, index|

    cluster_name = cluster_axis_summary[:name]

this:

  clusters_for_axis = cluster(axis.id, i)
  clusters_for_axis.each_with_index do |cluster_axis_summary, index|
    cluster_name = cluster_axis_summary[:name]

In general, I don't consider vertical white space within a method bad, in itself, but when it is used to separate lines into groups that have some similar purpose, I wonder if those lines could be moved into separate methods.

Code comments

Similarly, comments that say what the code is doing, while not bad, are better if they can be replaced with code that says what the comment used to say.  Often the lines after a comment can be moved into a method with the method name serving the purpose the comment used to serve.

Ruby idioms

Let's let Ruby do some of the busywork for us.

Replacing a loop with reduce

In this code:

  sum = 0
  count = 0

  # data_items is an array containing each respondent's survey scores (as an array)
  cluster.data_items.each do |item|
    sum += item[i]
    count += 1
  end

We are calculating a sum and a count.  The count doesn't need to be calculated: just use cluster.data_items.size.  To calculate the sum, this code using reduce is idiomatic Ruby:

  sum = cluster.data_items.reduce(:+)

Map

Ruby has a wonderful idiom for transforming one sequence to another, map.  Whenever you see code like this:

  series_data = []
  clusterer.clusters.each_with_index do |cluster, index|
    ...
    series_data << {
      name: "Cluster #{ index + 1 }",
      data: question_means_for_cluster
    }
  end

You can do this instead:

  series_data = clusterer.clusters.map.with_index do |cluster, index|
    series_data << {
      name: "Cluster #{ index + 1 }",
      data: question_means_for_cluster
    }
  end

Ruby frees you from the bother of creating the array and adding elements to it.  map can also be used in #diana_clusterer.

self

There are a few uses of self that you don't need.  For example, this:

self.members.each do |member|

could be:

members.each do |member|

See this SO question for more.


Guessing the JDK home directory My project is using a specific version of JDK which is jdk-6 and the default JDK_HOME path will vary from distribution to distribution, so I wrote a script to guess it. Is there any way to do this better?

#!/bin/bash
JAVA_HOME=""
JDK_TEMP=""
shopt -s extglob

JDK_ROOT_DIR=( /usr/java/ /usr/java/jdk/  /usr/j2se/  /usr/j2se/ /usr/j2sdk/ /usr/jdk/ /usr/lib/java/ /usr/lib/j2se/ /usr/lib/j2sdk/ /usr/lib/jdk/ /usr/lib/jvm/java/ /usr/lib/jvm/j2se/ /usr/lib/jvm/j2sdk/ /usr/lib/jvm/jdk/ /usr/local/java/ /usr/local/java/jdk/ /usr/local/jdk/ /opt/java/ /opt/j2se/ /opt/j2sdk/ /opt/j2sdk/ /opt/jdki/ /usr/lib/jvm/ )
for sub_dir in "${JDK_ROOT_DIR[@]}"
do

#                                                 #
# For each subdirectory inside the root directory #
#                                                 #
#                                                 #
    # For openjdk                                 #
    for jdk_dir in $sub_dir/java-*6*-openjdk*!(common) ; do
        if [[ -d $jdk_dir ]]; then
            echo  $jdk_dir
            export JAVA_HOME=$jdk_dir
            exit 0
        fi
    done

    # for IBM Java                                #
    for jdk_dir in $sub_dir/java-1_6_0-ibm-!(common) ; do
        if [[ -d $jdk_dir ]]; then
            echo $jdk_dir
            export JAVA_HOME=$jdk_dir;
            exit 0
        fi
    done

    # for raspbian java                           #
    for jdk_dir in $sub_dir/jdk-8-oracle-arm32-!(common) ; do
        if [[ -d $jdk_dir ]] ; then
            echo  $jdk_dir
            export JAVA_HOME=$jdk_dir
            exit 0
        fi
    done
done

echo "JDK directory not found."

Variables

The line containing the JDK_ROOT_DIR=( … ) definition is so long that it's unreadable and unmaintainable.  In fact, you listed /usr/j2se/ twice.  It would help to put one entry on each line.

The switch in naming in for sub_dir in "${JDK_ROOT_DIR[@]}" is confusing.  Each "root dir" is suddenly being called a "sub dir"?

Your export JAVA_HOME=$jdk_dir statements are pointless, since you immediately exit after setting the variable.  For that matter, the initial JAVA_HOME="" and JDK_TEMP="" assignments are also useless.

I know that you don't expect any whitespace or special characters in these directory names, but I would still make it a habit to double-quote variables according to best practice.

Globbing

Your three cases (for OpenJDK, IBM, and Raspbian) are nearly identical and could be handled by a nested loop.

You should also shopt -s nullglob to avoid unnecessarily doing the -d test on failed glob expansions.

Suggested solution

In addition, I think that failure to find the JDK directory should result in a non-zero exit code, and the error message should probably be printed to standard error rather than to standard output.

#!/bin/bash

shopt -s extglob nullglob

JDK_ROOT_DIRS=(
    /usr/java
    /usr/java/jdk
    /usr/j2se
    /usr/j2sdk
    /usr/jdk
    /usr/lib/java
    /usr/lib/j2se
    /usr/lib/j2sdk
    /usr/lib/jdk
    /usr/lib/jvm/java
    /usr/lib/jvm/j2se
    /usr/lib/jvm/j2sdk
    /usr/lib/jvm/jdk
    /usr/local/java
    /usr/local/java/jdk
    /usr/local/jdk
    /opt/java
    /opt/j2se
    /opt/j2sdk
    /opt/jdki
    /usr/lib/jvm
)
SUBDIR_GLOBS=(
    'java-*6*-openjdk*!(common)'            # OpenJDK
    'java-1_6_0-ibm-!(common)'              # IBM Java
    'jdk-8-oracle-arm32-!(common)'          # Raspbian Java
)

for root_dir in "${JDK_ROOT_DIRS[@]}" ; do
    for subdir_glob in "${SUBDIR_GLOBS[@]}" ; do
        for jdk_dir in "$root_dir"/$subdir_glob ; do
            if [ -d "$jdk_dir" ]; then
                echo "$jdk_dir"
                exit 0
            fi
        done
    done
done

echo "JDK directory not found." >&2
exit 1


Dynamic operators within DAGs (Airflow) I would like to know if what I did to achieve to goal of dynamic operators within an Airflow DAG (Directed Acyclic Graph) is a good or a bad practice.

The goal I had to achieve was:

  Create a 'x' amount of operators within
  a DAG based on the result of an API call. This DAG will run for example every week.

The program that I made works, but I don't know if it is a good practice for developing DAGs for Airflow. So my question is: is it a good practice or if not what would be a better solution to this problem?

The code I used to achieve this goal:

with DAG('my_dag', default_args=default_args, schedule_interval='@weekly') as my_dag:

    start = DummyOperator(
        task_id='start'
    )

    endpoint = 'www.example.com/api/dummies'
    r = requests.get(endpoint)
    dummies = r.json()

   for _, dummy in enumerate(dummies):
        dummy_operator = DummyOperator(
                    task_id='dummy_opr_{}'.format(dummy['id']
                    )
        start >> dummy_operator

It is a good practice for Airflow. Moreover, Airflow positioning it as one of their "killer-features". It is good to create dynamic tasks in DAG, it is pretty OK to create dynamically created DAGs. One thing I recommend you to think about is that it is not good to create hundreds and thousands of one-time-running tasks because it will lead to:

Filling your Airflow database with huge amount of garbage data
Inability to analyze DAGs visually. Here is the example of Airflow tree tab of tasks like these:

Here is the sample code for dynamic DAG creation:

from datetime import datetime
from airflow import DAG
from airflow.operators.python_operator import PythonOperator

def create_dag(dag_id, schedule, dag_number, default_args):

    def hello_world_py(*args):
        print('Hello World')
        print('This is DAG: {}'.format(str(dag_number)))

    dag = DAG(dag_id,
              schedule_interval=schedule,
              default_args=default_args)

    with dag:
        t1 = PythonOperator(
            task_id='hello_world',
            python_callable=hello_world_py,
            dag_number=dag_number)

    return dag

# build a dag for each number in range(10)
for n in range(1, 10):
    dag_id = 'hello_world_{}'.format(str(n))

    default_args = {'owner': 'airflow',
                    'start_date': datetime(2018, 1, 1)
                    }

    schedule = '@daily'

    dag_number = n

    globals()[dag_id] = create_dag(dag_id,
                                  schedule,
                                  dag_number,
                                  default_args)

You can read more about it here.


Custom data validation approach for store items and prices I came up with a validation design for my project and wanted to hear if there's anything I could improve.

Here's how it works. I have the models (code first)

public class Item : IEntity<Item>
{
    public Item()
    {

    }

    public int ID { get; set; }
    public string StoreCode { get; set; }
    public string UPCCode { get; set; }
    public string Description { get; set; }
    public double ABV { get; set; }
    public string URL { get; set; }
    public string Producer { get; set; }

    //each model has its validation method...
    public bool IsValid(Repository<Item> repository)
    {
        if (repository.Single(x => x.StoreCode == this.StoreCode) != null)
            return false;
        else
            return true;
    }
}

public class Price : IEntity<Price>
{
    public Price()
    {

    }

    public int ID { get; set; }
    public double Value { get; set; }
    public DateTime FetchedDate { get; set; }

    public bool IsValid(Repository<Price> repository)
    {
        var price = repository.Find(x => x.ItemID == this.ItemID).OrderByDescending(x => x.FetchedDate).Take(1).SingleOrDefault();

        if (price != null && price.Value == this.Value)
            return false;
        else if (price == null)
            return true;
        else
        {
            price = repository.Find(x => x.ItemID == this.ItemID && x.FetchedDate.ToString("yyyyMMdd") == this.FetchedDate.ToString("yyyyMMdd")).SingleOrDefault();
            return (price != null) ? false : true;
        }

    }
}

The IEntity interface is as such:

public interface IEntity<TEntity> where TEntity: class, IEntity<TEntity>
{
    int ID { get; set; }
    bool IsValid(Repository<TEntity> repository);

}

And in my repository class, I have an IsValid method, among other read/write methods.

public class Repository<TEntity> where TEntity: class, IEntity<TEntity>
{
    private EntitiesContext = new EntitiesContext ();
    private DbSet<TEntity> AppDbSet;

    public Repository()
    {
        this.AppDbSet = Context.Set<TEntity>();
    }

    public bool IsValid(TEntity entity)
    {
        return entity.IsValid(this);
    }

    //other Get, Single, Update, Add, Save methods...
}

And here's how I use it:

Repository<Price> priceRepo = new Repository<Price>();

//...gather data...

foreach (Price price in prices)
{
    if (priceRepo.IsValid(price))
        priceRepo.Add(price);
    }
}

Is that a good way to implement such a feature? Any areas for improvement (not in the validation methods themselves, but the approach in general)? This will be mostly used for a Web API project.

You are close to a standard validation implementation of Entity Framework, which is using IValidatableObject. It may be worth the effort to replace your validation by this readily available validation framework.

The advantage of using IValidatableObject is that it works together with validations applied by data annotations or fluent mappings. If, for example, you have ...

[Required]
public string UPCCode { get; set; }

... or ...

modelBuilder.Entity<Item>().Property(i => i.UPCCode).IsRequired();

... EF will carry out the validation when SaveChanges is executed. If validation fails, a DbEntityValidationException is thrown that contains a property EntityValidationErrors showing the validation errors.

All this happens without even implementing IValidatableObject. If however you also implement this interface, you can add custom validations that blend seamlessly with the ones from data annotations/mappings. Here's how to do that, reducing it to the essentials:

public class Item : IValidatableObject
{
    public int ID { get; set; }
    [Required]
    public string StoreCode { get; set; }

    public IEnumerable<ValidationResult> Validate(ValidationContext validationContext)
    {
        var context = (DbContext)validationContext.Items["Context"];
        var item = (Item)validationContext.ObjectInstance;
        if (context.Set<Item>().Any(c => c.StoreCode == item.StoreCode && c.ID != item.ID))
        {
            var error = string.Format("StoreCode '{0}' exists", item.StoreCode);
            yield return new ValidationResult(error, new[] { "StoreCode" });
        }
        yield return ValidationResult.Success;
    }
}

Now if this validation is violated, it will also throw a DbEntityValidationException with the custom message in EntityValidationErrors.

Notice that the context responsible for saving the item is wrapped in the validationContext. This is not so by default. You must override the context's DbEntityValidationResult method to add the context to validationContext.Items:

protected override DbEntityValidationResult ValidateEntity(DbEntityEntry entityEntry, IDictionary<object, object> items)
{
    items.Add("Context", this);
    return base.ValidateEntity(entityEntry, items);
}

You may have noticed another big advantage, compared to your approach: the Validate method returns meaningful information rather than just a boolean. I'm not even sure how you would supply feedback from your Validate method.

As you see, this is close enough to consider refactoring your own architecture, but also different enough to make this more than a trivial operation. It's more at the ground level, while your validation is in an abstraction layer.

Another difference is that the EF validation is holistic: it validates an entire object graph, while your validation validates individual entities. Both have their pros and cons. With EF's validation it's harder to get validation errors for one specific entity, with your validation it's hard to validate a mixed collection of objects.


Simple LINQ statement for report limit What would be the most efficent way to calculate this?

var reportLimit = 96*1024;

IEnumerable<int> memoryInUse =
  things
    .Where(sample => sample.IsOn)
    .Select(sample => sample.MemoryInMb)
    .ToArray();

int totalUnderReportLimit = memoryInUse.Where(ram => ram <= memoryCalcFactor).Sum();
int totalOverEqualReportLimit = memoryInUse.Count(ram => ram > memoryCalcFactor) * memoryCalcFactor;

return totalUnderReportLimit  + totalOverEqualReportLimit;

Well here's my take on it.  If you want efficient in terms of speed, write a loop.  LINQ is not for writing fast code, it's for writing concise code.

memoryInUse is just a collection of the memory counts of all the machines that are on.  You then essentially partition those machines by some memoryCalcFactor to do some calculation.

The call to ToArray(), while somewhat helpful (you do the filter/projection once) isn't really needed.  You can perform your calculation in one pass and therefore don't need it at all.  Look at the following lines in your calculation:

var x = memoryInUse.Where(ram => ram <= memoryCalcFactor).Sum();
var y = memoryInUse.Count(ram => ram > memoryCalcFactor) * memoryCalcFactor;
return x + y;

What are we doing here?

Adding up all the sizes that are less than some factor
Counting all those that are greater than that factor multiplying by that factor.
Adding the previous results

Looking at this at a much higher level, what are we doing here?  We're adding up the sizes limiting each size by some factor (a maximum).  Write your code to do that.

This is how I'd write it:

var memorySizes = machines
    .Where(machine => machine.IsOn)
    .Select(machine => machine.MemoryInMB);
var result = memorySizes.Sum(size => Math.Max(size, memoryCalcFactor));

Otherwise if you don't want to take the performance hit LINQ will give you and use a loop, here's the equivalent:

var sum = 0;
foreach (var machine in machines)
{
    if (machine.IsOn)
    {
        sum += Math.Max(machine.MemoryInMB, memoryCalcFactor);
    }
}