Implementation of the FNV-1a hash algorithm for 32- and 64-bit Are there any improvements to these two classes which implement FNV-1a for hashing? Particularly, I'm looking for any reuse opportunities (possibly via a common abstract superclass derived from HashAlgorithm?).

GitHub

public sealed class Fnv1a32 : HashAlgorithm
{
    private const uint FnvPrime = unchecked(16777619);

    private const uint FnvOffsetBasis = unchecked(2166136261);

    private uint hash;

    public Fnv1a32()
    {
        this.Reset();
    }

    public override void Initialize()
    {
        this.Reset();
    }

    protected override void HashCore(byte[] array, int ibStart, int cbSize)
    {
        for (var i = ibStart; i < cbSize; i++)
        {
            unchecked
            {
                this.hash ^= array[i];
                this.hash *= FnvPrime;
            }
        }
    }

    protected override byte[] HashFinal()
    {
        return BitConverter.GetBytes(this.hash);
    }

    private void Reset()
    {
        this.hash = FnvOffsetBasis;
    }
}

and

public sealed class Fnv1a64 : HashAlgorithm
{
    private const ulong FnvPrime = unchecked(1099511628211);

    private const ulong FnvOffsetBasis = unchecked(14695981039346656037);

    private ulong hash;

    public Fnv1a64()
    {
        this.Reset();
    }

    public override void Initialize()
    {
        this.Reset();
    }

    protected override void HashCore(byte[] array, int ibStart, int cbSize)
    {
        for (var i = ibStart; i < cbSize; i++)
        {
            unchecked
            {
                this.hash ^= array[i];
                this.hash *= FnvPrime;
            }
        }
    }

    protected override byte[] HashFinal()
    {
        return BitConverter.GetBytes(this.hash);
    }

    private void Reset()
    {
        this.hash = FnvOffsetBasis;
    }
}

If you were being completely asinine you could move the Initialize method to a new common ancestor class. The amount of code you would write for that common class would exceed the amount of code you would save from duplication though, in other words, not worth it. Alternatively, you can move the Reset logic in to the Initialize method, and remove Reset entirely, and just call Initialize in the constructor.

I think two factors come in to play here:

The basic Object-model of the Hashing systems is pretty well structured. The common-code is already abstracted to higher inheritance levels, so the amount of required duplication is really small (Kudos to the .net library team).
By design, the FNV1a algorithm is simple and fast. The only difference between the 64 and 32 bit versions is the value and type of the primes and initializers. Because these values are different the core algorithm (due to the low-level bitwise operations) need to be different.

In other words, your code is as good as it gets. Good job. Now, when you get around to the 128, 256, 512, and 1024-bit versions of the FNV1a algorithm, you may find some other opportunities for logic sharing. I suspect that the loops required using a common int-based infrastructure will make things possible.


K&R Exercise 1-20: tabs to spaces I'm learning C with K&R 2nd Ed. I've just completed the exercise 1-20 and I would know if my code is correct (i.e. answering the question) and if my style is not too bad. Or just some feebacks to improve myself!

  K&R Exercise 1-20 p.34

  Write a program detab that replaces tabs in the input with the proper number  * of blanks to space to the next tab stop.  Assume a fixed set of tab stops, say every n
  columns.  Should n be a variable or a symbolic parameter?

More info here

I put '-' because it's more readable than ' '. But you can totally change it.

#include <stdio.h>
#define COLUMN 8

int main()
{
    int c, i, nc;
    nc = 0;

    while ((c = getchar()) != EOF)
    {
        if (c != '\t' && c != '\n')
            nc = (nc + 1) % COLUMN;

        if (c == '\n')
            nc = 0;

        if (c == '\t') {
            for (i=1; i<=(COLUMN - nc); ++i) {
                putchar('-');
            }
            nc = 0;
        }
        else
            putchar(c);
    }

return(0);
}

Here is an alternate algorithm:

while ((c = getchar()) != EOF) {
    switch (c) {/* switch instead of if-else... */
    case '\n':
        nc = 0;
        putchar(c);
        break;
    case '\t':
        for (i = COLUMN - nc % COLUMN, nc += i; i > 0; --i) {
            putchar('-');
        }
        break;
    default:
        ++nc; /* increment is faster than increment plus divide */
        putchar(c);
        break;
    }
}

nc keeps track of the current column position within the line.


A basic pure Python hash map using a tree I would like feedback on my first attempt to understand how hash tables work. I've read some on how hash tables use modulo operations to find the location of a key.

Can you help me understand, compare the modulo approach with my attempt to use a tree?

import random
import hashlib
from collections import defaultdict
import timeit

def insert(key, value, table=[]):
    alphabet = lambda: [[] for i in xrange(23)]
    if not table:
        table = alphabet()
    get_hash = lambda x: hashlib.sha1(str(x)).hexdigest().upper()
    key_hash = get_hash(key)
    entry = table
    for i, char in enumerate(key_hash):
        ordinal = ord(char) - 48
        entry = entry[ordinal]
        #entry is a list representing 'char' at index 'i'
        entry_len = len(entry)
        if entry_len is 0:
            entry.extend((key, [value]))
            break
        elif entry_len is 2:
            extant_key, extant_values = entry[0], entry[1]
            if key == extant_key:
                extant_values.append(value)
                break
            del entry[:]
            #entry = []
            extant_hash = get_hash(extant_key)
            for i in xrange(i + 1, len(key_hash)):
                char, extant_char = key_hash[i], extant_hash[i]
                if char == extant_char:
                    entry.extend(alphabet())
                    ordinal = ord(char) - 48
                    entry = entry[ordinal]
                    #entry = []
                else: break
            if char != extant_char:
                entry.extend(alphabet())
                char_ord = ord(char) - 48
                entry[char_ord].extend((key, [value]))
                extant_ord = ord(extant_char) - 48
                entry[extant_ord].extend((extant_key, extant_values))
                #entry has empty lists except 2
                break
            else: pass
                #hash collision, to be continued
    return table

def get(key, table, pop=False):
    key_hash = hashlib.sha1(str(key)).hexdigest().upper()
    entry = table
    for i, char in enumerate(key_hash):
        ordinal = ord(char) - 48
        entry = entry[ordinal]
        entry_len = len(entry)
        if entry_len is 0:
            raise KeyError(key)
        elif entry_len is 2:
            items = entry[1]
            if pop:
                del entry[:]
                if not any(item for item in entry):
                    del entry
            return items
    raise KeyError(key)

def make_defaultdict(kv):
    d = defaultdict(list)
    for items in kv:
        k, v = items
        d[k].append(v)
    return d

def make_my_table(kv):
    t = []
    for items in kv:
        k, v = items
        t = insert(k, v, t)
    return t

kv = [(random.randint(1000, 9999), random.randint(1000, 9999))
     for i in xrange(1000000)]

dd = make_defaultdict(kv)
mt = make_my_table(kv)

validate = []
for key in dd.keys():
    if dd[key] == get(key, mt):
        validate.append(True)
    else:
        validate.append(False)

defaultdict_time = timeit.timeit(lambda: make_defaultdict(kv), number=1)
my_table_time = timeit.timeit(lambda: make_my_table(kv), number=1)

print 'Time to build defaultdict', defaultdict_time
print 'Time to build my table', my_table_time
print 'Both tables are identical', all(i for i in validate)

Time to build defaultdict 0.342477702698
Time to build my table 4.21419315689
Both tables are identical True

I wanted to know how your program worked and so I removed all the module level checking code and used the following:

table = []
def check(key, value):
    global table
    table = insert(key, value, table)
    print table
    print get(key, table)

check(5, 'value')
check('key', 'value2')
check('key', 'value3')
check('key2', 'value4')

This gave me a somewhat strange output.

[[], [], [], [], [], [], [], [], [], [], [], [], [], [], [], [], [], [5, ['value']], [], [], [], [], []]
['value']
[[], [], [], [], [], [], [], [], [], [], [], [], [], [], [], [], [], [[], [], [], [], [], [], ['key', ['value2']], [], [], [], [], [], [], [], [], [], [], [], [], [5, ['value']], [], [], []], [], [], [], [], []]
['value2']
[[], [], [], [], [], [], [], [], [], [], [], [], [], [], [], [], [], [[], [], [], [], [], [], ['key', ['value2', 'value3']], [], [], [], [], [], [], [], [], [], [], [], [], [5, ['value']], [], [], []], [], [], [], [], []]
['value2', 'value3']
[[], [], [], [], [], [], [], [], ['key2', ['value4']], [], [], [], [], [], [], [], [], [[], [], [], [], [], [], ['key', ['value2', 'value3']], [], [], [], [], [], [], [], [], [], [], [], [], [5, ['value']], [], [], []], [], [], [], [], []]
['value4']

The keys 5 and key collide and so the results should be a list of two items, but you spam it with lots of items.
This can be due to two things:

A bad algorithm that spams list.append
Your algorithm being a 'hash-tree' not hash-table.

From the looks of your algorithm it's probably the latter.
This is as you do:

entry = table
for i, char in enumerate(key_hash):
    ordinal = ord(char) - 48
    entry = entry[ordinal]

This is fine and all, but then it's not a hash-table.

A hash-table should be a 2D-table of items. Where items here is just a [key, value] pair.

If you change your code to a hash-table then it'd be so much more simpler.
The algorithm is hash key, modulo it, append to table:

def add(key, value, table):
    hashed_key = hash(key) % len(table)
    table[hased_key].append([key, value])

This should bring up some concerns:

What if the size of the table changes?
What if the key is already in the table?
What if the key is already in the table and the size of the table changes?

These can be resolved, (1) by using a class, (2) by checking the list.

class Dict(object):
    def __init__(self, size=8192):
        self._table = [[] for _ in xrange(size)]
        self._mod = size

    def _get_item_index(self, key):
        hashed_key = hash(key) % self._mod
        list_ = self._table[hashed_key]
        return list_, next((i for i in xrange(len(list_)) if list_[i][0] == key), None)

This then makes insert and get easy to make.

def insert(self, key, value):
    list_, index = self._get_item_index(key)
    if index is None:
        list_.append([key, [value]])
    else:
        list_[index][1].append(value)

def get(self, key):
    list_, index = self._get_item_index(key)
    if index is None:
        raise KeyError(key)
    return list_[index][1]

Your method seems alright, it's hard to read, and is way more complex then it needs to be, so I'd recommend at least making a _get_item_index if you do carry on with your hash tree.
But it seems faster than my one.

For in-depth review of the code:

If I were to make _get_item then I'd look at the get function first.
I don't think that you should have pop in that function, and so I'll remove it.
This leaves you with:

def get(key, table):
    key_hash = hashlib.sha1(str(key)).hexdigest().upper()
    entry = table
    for i, char in enumerate(key_hash):
        ordinal = ord(char) - 48
        entry = entry[ordinal]
        entry_len = len(entry)
        if entry_len is 0:
            raise KeyError(key)
        elif entry_len is 2:
            return entry[1]
    raise KeyError(key)

From this we can see the basic looping structure. I'd first merge ordinal with entry, to get entry = entry[ord(char) - 48].
You shouldn't use 1 is 1 instead use 1 == 1, it makes more sense as 1 == 1.0 is true where 1 is 1.0 is not.
As we need to get the item, entry, even if it's not there (we'll be using this for update too), we should change the ifs to just return the entry.
Finally you should notice that entry_len == 0 is the same as not entry.
Merging these all together gets us:

def _get_item(key, table):
    key_hash = hashlib.sha1(str(key)).hexdigest().upper()
    entry = table
    for i, char in enumerate(key_hash):
        entry = entry[ord(char) - 48]
        if not entry:
            return entry
        elif len(entry) == 2:
            return entry
    else:
        raise KeyError(key)

To use it is simple:

def get(key, table):
    item = _get_item(key)
    if item and item[0] == key:
        return item[1]
    raise KeyError(key)

We still need to get _get_item working for update however.

You should be able to see that the code is roughly the same for all except elif entry_len is 2:.
To help with this we should break out of the loop so that we're in a 'separate environment'.
To make things simpler I'd also return if key == entry[0].
And so:

elif len(entry) == 2:
    if key == entry[0]:
        return entry
    break

You should notice that we don't want the new code to run if we are using the other version of this function.
To do this we can pass a flag that returns the entry after exiting the for.

if extend:
    return entry

Finally we're left with:

extant_key, extant_values = entry[0], entry[1]
del entry[:]
#entry = []
extant_hash = get_hash(extant_key)
for i in xrange(i + 1, len(key_hash)):
    char, extant_char = key_hash[i], extant_hash[i]
    if char == extant_char:
        entry.extend(alphabet())
        ordinal = ord(char) - 48
        entry = entry[ordinal]
        #entry = []
    else: break
if char != extant_char:
    entry.extend(alphabet())
    char_ord = ord(char) - 48
    entry[char_ord].extend((key, [value]))
    extant_ord = ord(extant_char) - 48
    entry[extant_ord].extend((extant_key, extant_values))
    #entry has empty lists except 2
    break
else: pass
    #hash collision, to be continued

This leads to the following changes:

Using both extant_key and extant_value is more verbose than I like, and would just use extant with the slice item[0:2].
Use izip rather than xrange(i + 1, len(key_hash)).
This is quite simple add from itertools import izip at the top, and then use:

for char, extant_char in izip(key_hash[i + 1:], extant_hash[i + 1:]):

Use item[ord(...) - 48] rather than setting intermarry variables.
Raise an error if you don't break out the for loop.
Change the if to char != extant_char and then break.
This adds a little bit of readability, and prevents large amounts of indents.
Finally return entry[ord(char) - 48].

This should lead to something like:

extant = entry[0:2]
del entry[:]
extant_hash = get_hash(extant[0])
for char, extant_char in izip(key_hash[i + 1:], extant_hash[i + 1:]):
    if char != extant_char:
        break
    entry.extend(alphabet())
    entry = entry[ord(char) - 48]
else:
    raise KeyError(key)
entry.extend(alphabet())
entry[ord(extant_char) - 48].extend(extant)
return entry[ord(char) - 48]

Finally I should note that the two KeyErrors raised in this function, aren't really errors with the key, it's actually that there is actually a sha1 collision.
And so I'd change the errors to custom errors.

What is the likelihood of these? Not very likely but my favorite programmers.SE answer makes me think that ruling this out is not a good idea.


JavaScript (multiple) inheritance / mixins I'm quite new to JavaScript, and I'm trying to understand the idea behind inheritance in JavaScript. I've read a bit about it, and I don't want to mess with prototypes (which I don't yet fully understand) so, I wrote this code using the xtend module for node.js.

var xtend = require("xtend");

// Human constructor.
// Human is a base class, it can introduce itself, and knows it's age.
function createHuman(name, age) {

    function getName() {
        return name;
    }

    function getAge() {
        return age;
    }

    return {
        getName : getName,
        getAge : getAge,
    };

}

// Boy constructor
// Boy *is a* Human
// Boy can play football (but not yet)
function createBoy(name, age) {

    function playFootball() {
        console.warn("Football skills not implemented yet, but trying", "-", this.getName());
        // TODO: implement this
    }

    return xtend(createHuman(name, age), {
        playFootball: playFootball,
    })

}

// Girl constructor
// Girl *is a* Human
// Girl can sing (well, almost)
function createGirl(name, age) {

    function singASong() {
        console.warn("Singing not implemented yet, but trying", "-", this.getName());
        // TODO: implement this
    }

    return xtend(createHuman(name, age), {
        singASong: singASong,
    })

}

// Hermaphrodite constructor
// Hermaphrodite *is a* Girl **and** Hermaphrodite *is a* Boy, so we have multiple inheritance?
// Hermaphrodite can do what boys and girls can
function createHermaphrodite(name, age) {
    return xtend(createGirl(name, age), createBoy(name, age));
}

// Tests:
var boy = createBoy("John", 12);
var boy2 = createBoy("Frank", 10);
var girl = createGirl("Daisy", 7);

var herm = createHermaphrodite("Mel", 24);

console.info("Boy:  ", boy.getName(), boy.getAge());
console.info("Boy:  ", boy2.getName(), boy2.getAge());
console.info("Girl: ", girl.getName(), girl.getAge());
console.info("Hermaphrodite: ", herm.getName(), herm.getAge());

boy.playFootball();
boy2.playFootball();
girl.singASong();

herm.playFootball();
herm.singASong();

As you can see, I'm not using the usual Class approach, but instead have a kind of constructors that create objects. The inheritance is made using the xtend module, which copies properties from another objects. This - I think - also allows for multiple inheritance (see Hermaphrodite).

Questions:

Can this code be considered good practice? I always thought of JS as a more functional (than OO) language, so not trying to imitate classes, not using the new keyword and so on sounded good to me.
Is this fast / memory-efficient? I mean - there is an additional object created every time the sub-class instance is created, just to copy the properties (for example, if I create a Boy instance, there is a Human created just to copy the properties to Boy).
Is there a better / faster / more efficient (or simply: proper) way of doing this? (Pointing to one, remember that I'm a beginner, so be descriptive or I might not understand the answer.)

Q1: I wouldn't say it was good or particularly bad practice for some of the reasons I will go into below.

Q2: It's not memory efficient if you compare the code you have to a class / prototype based solution.

Q3: Take a look below, i've tried to keep it to as skeletal structure as possible to demonstrate how prototypes can be very useful.

xtend

xtend is great for smaller, intricate pieces of code and it has helped me loads recently. The caveat: In larger code bases you want to avoid the level of blind inheritance that xtend provides. Potentially in the future you may create a class that uses an object that has many layers of inheritance as an array of options for example. When that object squares up to the media and asked it's name and occupation, how does it know where all its components came from and what they do? In your code, how does the boy know he's a boy as well as a human for example.

Public / Private Methods

OOP in JS : Public / Private Variables will explain alot, if you are used to OOP in other languages this will probably click straight away.

Your first function is a class constructor with two private methods declared. I prefer to only set information in a constructor and then use public methods (prototypes) to deal with the workload.

Prototypes

Many objects can use prototypes and they're never duplicated within each object that is created.

Take this piece of code for example, first the class structure:

function CreateHuman(params) {
    // We create a new human object using params as initial data
    this.human = {
        name : params.name,
        age : params.age,
        gender : 'unknown'
    }
}

// first prototype, updates the gender with the one specified in the call
CreateHuman.prototype.setGender = function (gender) {
    this.human.gender = gender;
}

CreateHuman.prototype.setName = function (name) {
    this.human.name = name;
}

CreateHuman.prototype.setAge = function (age) {
    this.human.age = age;
}

// this method does more work than any of the others
CreateHuman.prototype.setHobby = function() {

    // If the gender has been set, the hobby will be applied
    // You could use functions here as well

    switch(this.gender) {
        case('male') :
            this.person.hobby = 'football';
            this.person.hobbyActive = (this.age >= 6) ? true : false;
            break;
        case('female') :
            this.person.hobby = 'singing';
            this.person.hobbyActive = (this.age >= 4) ? true : false;
            break;
        deafult :
            this.person.hobby = 'no hobby';
            this.person.hobbyActive = true;
    }
}

// And a return method to get the object from the class
CreateHuman.prototype.getPerson = function() {
    return this.person;  // We return the object with all collected data
}

We could then use the class very easily to create a number of humans with different names, ages and genders.

var params =
{
    name : "Dave",
    age : 30
};

I've included the params in this example as you may just need one object created and feeding it params is an easy way to make sure your code is useable by anyone else that might use your class.

var human = new CreateHuman(params);
var people = {}; // Make people an object to hold all the humans created

// Call our first prototype function
human.setGender('male');
// 2nd prototype
human.setHobby();

people.human1 = human.getPerson();

people.human1 if logged in the console would be:

Object {name : 'dave',
        age : 30,
        gender : 'male',
        hobby : 'football',
        hobbyActive : true
       }

Start again:

human.setName('Alexa');
human.setAge(28);
human.setGender('female');
human.setHobby();

people.human2 = human.getPerson()

// And so on

I hope that gives you an insight into prototypes, they will be far more useful to you than object inheritance (though I admit it does have its uses).


Printing the status of a database insertion as JSON The controller is currently inserting a record into the database and returning a JSON response as shown below in the try-catch block. I can see the Inserted Successfully within the try-catch block!. Although,
I haven't seen this message Error in Inserting Record! so far since my webservice haven't errored out till now.

Please let me know if the approach I have followed to display just two lines of JSON is an efficient / conventional way of doing similar things ? If it can be improved, please suggest.

@RequestMapping(value = "/insertRecord", method = RequestMethod.POST)
 public Map<String, String> insertRecord
  (
   @RequestParam(value = "type_id", defaultValue = "0") Integer type_id,
   @RequestParam(value = "tempID", defaultValue = "0") Integer tempID,
   @RequestParam(value = "status", defaultValue = "") String successORFailure,
   @RequestParam(value = "notes", defaultValue = "") String message
  ) {

   Map<String, String> response =  new HashMap<String,String>();

   try {

    insertRecordDao insertDao = (insertRecordDao) context.getBean("insertRecordDao");
    insertDao.insertAuditInfo(type_id, tempID, successORFailure, message);
    response.put("message", "Inserted Successfully within the try-catch block!");
    response.put("status", "SUCCESS");

   } catch (Throwable th) {
    th.printStackTrace();
    response.put("message", "Error in Inserting Record!");
    response.put("status", "ERROR");

   }

    return response;

  }

Here's a JSON response from my POSTMAN client:

This looks like you are trying to create a REST interface. If this is true, the following remarks apply:

First of all, your path should not include the action. The resource should be named "record" and the action is determined by the HTTP verb, i.e. "insert" is implied by POST, retrieve by GET, modify/update by PUT, etc.
Success or failure should primarily be communicated via HTTP status. For a POST, customary responses are either code 200 (OK) with the complete object as a result (often modified by the insertion, e.g. having a unique id now) or 204 (no content) to signify that everything is OK without sending the object back.
Likewise, failure should be communicated as a status code in the 500 range (server error, e.g. if the database connection fails) or 400 range (bad request, e.g. wrong data types transmitted by the client)

Generally, look around for "REST best practices", you'll find lots of information on the topic.


Creating a booking based on a POST request I want to make a booking entry. I want to ask whether the code that i wrote below is rightly format or not?

I haven't tried it, because there's still more code that needs to be written. I just want to ask this before I continue to write.

<?php
    require 'Connection.php';

    //Check Connection
    if ($conn->connect_error){
        die("Connection Failed: ". $conn->connect_error);
    }

    //Create Variable Submitted Booking
    $MarketingName = $_POST["Marketing"];
    $CustomerName = $_POST["CustName"];
    $CustomerEmail = $_POST["CustEmail"];
    $HouseID = $_POST["IDHouse"];
    $UnitNo = $_POST["UnitNo"];
    $Remarks = $_POST["Remarks"];

    //Select All Data in Database
    $sql = $conn->prepare ("SELECT Property_No FROM cust_bookedfr WHERE Property_No = '" . $UnitNo . "' ");
    $result = mysqli_query($conn, $sql);

    if ($result->num_rows > 0) {
        //Validate Username
        echo "Property Unit is already taken.";

    } else {
        echo "Creating a Booking.";

        if (filter_var($Marketing, FILTER_SANITIZE_STRING)) {
            if (filter_var($CustName, FILTER_SANITIZE_STRING)) {
                if (filter_var($Cust_Email, FILTER_SANITIZE_EMAIL)) {
                    if (filter_var($HouseID, FILTER_VALIDATE_INT)) {
                        if (filter_var($UnitNo, FILTER_SANITIZE_STRING)) {
                            if (filter_var($Remarks, FILTER_SANITIZE_STRING)) {
                                //Insert the Username and Password into Database
                                $sql2 = $conn->prepare ("INSERT INTO cust_bookedfr (NULL, Marketing_Name, Cust_Name, Cust_Email, Property_Name, Property_ID, Property_No, Property_Qty, Property_Price, Remarks, DateAccess)
                                VALUES ('" . $MarketingName . "', '" . $CustomerName . "', '" . $CustomerEmail . "', 'Fajri Residential', '" . $HouseID . "', '" . $UnitNo . "', '', '', '" . $Remarks . "', NOW())");
                                if ($conn->query($sql2) === TRUE) {
                                    echo "New record created successfully";
                                } else {
                                    echo "Error: " . $sql2 . "<br>" . $conn->error;
                                }
                            }
                        }
                    }
                }
            }
        }
    }
$sql->close();
$sql2->close();
$conn->close();

?>

If you are going to use a prepared statement (please do) then go the extra step of using placeholders and bound parameters. Your queries are not currently stable/secure.
If you don't truly need Property_No for anything, just return COUNT from your first query, and evaluate that value as a boolean (loose truthy comparison).  This is cleaner and more deliberate, I reckon.
Your code is suffering from excessive tabbing (aka "arrowhead" code). This increases the likelihood that you or future devs will need to tediously scroll horizontally to review/manage the code.
If all of the conditions must be true to reach the INSERT query, then just use one condition statement with the expressions separated by &&.


Fraction class implemented in Objective-C Inspired by this question: Fraction (rational number) structure with custom operators, I have written this class for doing some simple work with fractions.

Fraction.h:

#import <Foundation/Foundation.h>

@interface Fraction : NSObject

@property int numerator, denominator;

-(void) print;
-(void) setNumerator: (int) n;
-(void) setDenominator: (int) d;
-(void) setTo: (int) n over: (int) d;
-(double) convertToNum;
-(void) add: (Fraction*) f;
-(void) subtract: (Fraction*) f;
-(void) multiply: (Fraction*) f;
-(void) divide: (Fraction *) f;
-(void) reduce;

@end

Fraction.m:

#import "Fraction.h"

@implementation Fraction

@synthesize numerator, denominator;

-(void) print
{
    NSLog(@"%i/%i", numerator, denominator);
}

-(void) setNumerator: (int) n
{
    numerator = n;
}

-(void) setDenominator: (int) d
{
    denominator = d;
}

-(void) setTo: (int) n over: (int) d
{
    numerator = n;
    denominator = d;
}

-(double) convertToNum
{
    if (denominator) return (double) numerator / denominator;
    else return NAN;
}

-(void) add: (Fraction*) f
{
    numerator = numerator * f.denominator + denominator * f.numerator;
    denominator *= f.denominator;

    [self reduce];
}

-(void) subtract: (Fraction*) f
{
    numerator = numerator * f.denominator - denominator * f.numerator;
    denominator *= f.denominator;

    [self reduce];
}

-(void) multiply: (Fraction *) f
{
    numerator *= f.numerator;
    denominator *= f.denominator;

    [self reduce];
}

-(void) divide: (Fraction *) f
{
    numerator *= f.denominator;
    denominator *= f.numerator;

    [self reduce];
}

-(void) reduce
{
    int u = numerator;
    int v = denominator;
    int temp;

    while (v)
    {
        temp = u % v;
        u = v;
        v = temp;
    }

    numerator /= u;
    denominator /= u;
}

@end

main.m:

#import "Fraction.h"

int main(int argc, const char* argv[])
{
    @autoreleasepool
    {
        Fraction *frac1 = [[Fraction alloc] init];
        Fraction *frac2 = [[Fraction alloc] init];

        [frac1 setTo: 1 over: 3];
        [frac2 setTo: 4 over: 5];
        [frac1 add: frac2];
        [frac1 subtract: frac2];
        [frac1 multiply: frac2];
        [frac1 divide: frac2];
        NSLog(@"%g", [frac1 convertToNum]);
        NSLog(@"%g", [frac2 convertToNum]);
    }
}

Is there anything that I'm doing blatantly wrong?  Are there some set practices I am not abiding by?  Anything that I'm missing?

@property
You declare properties in the header file, but you don't actually use them as anything more than instance variables, which suggests you may not quite understand their full power.
An Objective-C property is three things.

An instance variable
A setter
A getter

When you write @property int numerator;, you have created all three of these things.
Whether or not you write anything else, the class now has a private instance variable known as _numerator, a getter method in the form of - (int)numerator; and a setter method in the form of - (void)setNumerator:(int)numerator;
So given a fraction variable called myFrac, all of the following is instantly valid:
myFrac.numerator = 3;
NSLog(@"%i", myFrac.numerator);
[myFrac setNumerator: 4];
NSLog(@"%i", [myFrac numerator]);

The square bracket notation is perfectly valid, though when dealing with properties in Objective-C, we tend to prefer just using the dot notation.
What's more is that this fact makes these methods superfluous:
-(void) setNumerator: (int) n
{
    numerator = n;
}

-(void) setDenominator: (int) d
{
    denominator = d;
}

Except... not completely.  The first one is definitely unnecessary.  The second, however... it needs some logic.  Zero is not a valid denominator.  We need to prevent the case of the user setting the denominator to zero.  I think this is one of the rare cases where throwing an exception in Objective-C might be okay.  And in fact, why don't we just throw the exact exception that'd be thrown if we did actually divide by zero?
- (void)setDenominator:(int)denominator {
    if (denominator == 0) {
        // no reason to build a NSException object, just:
        __unused int divisionByZero = numerator/denominator;
    }
    _denominator = denominator;
}

I mean, I see that in converting it to a double, you deal with this, but what about when you print?  You really shouldn't let the object exist in an invalid state.
And you might have noticed I used a mysterious _denominator there.  As of Xcode 4 (I think, maybe even older), properties are autosynthesized in the .m file to their name prefixed with an underscore.  The @synthesize directive is no longer needed, and all Objective-C developers I know have abandoned it in favor of using the autosynthesized version.  The underscored name also is a huge clue that this is a direct access instance variable and not anything else.

-(void) setTo: (int) n over: (int) d
{
    numerator = n;
    denominator = d;
}

You follow this pattern of using short names for the arguments, and I think it's probably simply to avoid the name class with your synthesized property names.  And now you've just discovered another reason why the auto-synthesized underscore prefixed property names are great!  Although, truthfully, we should be going through the setters, so this method should look like this:
- (void)setTo:(int)numerator over:(int)denominator {
    self.numerator = numerator;
    self.denominator = denominator;
}

And if you put an NSLog statement in setDenominator:, you'll see that this method will now go through that method to set the value.
By the way, notice how I keep moving your opening braces to the correct line?

-(void) print
{
    NSLog(@"%i/%i", numerator, denominator);
}

As far as I know, methods like this, across the board, are a no-no in OOP.  Instead, classes should have an instant method which returns a string to be printed by whoever is using the class if they want to print the class.  In most languages, the method is called toString(), in Objective-C, we call it description.  Your description method should look like this:
- (NSString *)description {
    return [NSString stringWithFormat:@"%i/%i",numerator,denominator];
}

One thing to note is the behavior of NSLog.  NSLog doesn't just work for NSString objects.  It works any object.  As all Objective-C objects have a description method (which at a minimum is inherited from their super class), NSLog prints the value returned by the object's description method.  As such, by implementing description, see what happens if you call:
NSLog(@"%@", myFrac);

-(double) convertToNum
{
    if (denominator) return (double) numerator / denominator;
    else return NAN;
}

This method is actually problematic for me.  There's no telling what type of variable someone might need out of your fraction.  Whose to say they don't need a float?  Or an int?  Or an NSInteger?  There are a lot of possible types of number data types that one might need...
So, for starters, I'll point you to other Objective-C classes with method names like doubleValue, intValue, integerValue, etc.  As such, a method that returns the double representation of an object should probably be called doubleValue.
But perhaps what may be even more important that writing a method for all the different numeric data types... why don't we make use of Objective-C class categories and the NSNumber class?
In the .h file for this class, add the following:
@interface NSNumber (Fraction)

+ (NSNumber *)numberWithFraction:(Fraction *)fraction;

@end

And now in the .m file, assuming you've properly changed over to a method called doubleValue, add the following:
@implementation NSNumber (Fraction)

+ (NSNumber *)numberWithFraction:(Fraction *)fraction {
    return [NSNumber numberWithDouble:[fraction doubleValue]];
}

@end

Now anyone who has imported this file has also imported the ability to seemlessly create NSNumber objects with a single method call, and it will feel just like creating a NSNumber object with any other data type.


On-the-fly destructors Due to my needing to use C libraries inside C++, I kept finding myself wanting to be able to ensure that the free function got called, even if an exception got thrown. UTILS_SCOPE_EXIT ensures that the function will be called at the end of scope. External documentation shows how to use it, and also clarifies that it is not allowed to have multiple calls on the same line (because __COUNTER__ is not always supported):

utils.hpp

#pragma once
#include <functional>

namespace utils { namespace internal {

class ScopeExit_ {
public:
    ScopeExit_(std::function<void()> callback)
        : callback_{ callback }
    {}
    ~ScopeExit_()
    {
        callback_();
    }
private:
    std::function<void()> callback_;
};

#define UTILS_INTERNAL_CAT_HELPER(x, y) x ## y
#define UTILS_INTERNAL_CAT(x, y) UTILS_INTERNAL_CAT_HELPER(x, y)
#define UTILS_SCOPE_EXIT(...) utils::internal::ScopeExit_ UTILS_INTERNAL_CAT(utils_internal_ScopeExit_, __LINE__){ __VA_ARGS__ }

} }

This can be used like so:

#include <iostream>

int main()
{
    UTILS_SCOPE_EXIT([]{ std::cout << "1" << std::endl; });
    UTILS_SCOPE_EXIT([]{ std::cout << "2" << std::endl; });
    UTILS_SCOPE_EXIT([]{ std::cout << "3" << std::endl; });
    std::cout << "4" << std::endl;
    std::cout << "5" << std::endl;
}

Outputs:

4
5
3
2
1

Efficiency
The main issue with std::function<void()> is that it is heavy in memory usage. It comes in at a minimum of 32 bytes in G++ and (if memory serves me right) 40 bytes in VC++2015 32-bit.
See: http://coliru.stacked-crooked.com/a/9503ca27f1faded8
Solution
Don't use std::function<void()>.

A lightweight implementation
I promise you that we can reduce the on-scope-exit object's size to a tiny 1 byte when all we want to do is take an action at the end of a scope by using lambdas and templates.
Note: The implementation requires the <utility> and <type_traits> headers.
Step 1: Make a helper function that creates a lambda that calls a callable object.
template <typename F, typename... Args>
auto make_lambda( F&& f, Args&&... f_args ) noexcept
{
    // important to capture by copy to prevent argument lifetime issues
    return [=] () mutable -> std::result_of_t<F( Args... )>
    {
        return std::forward<F>( f )( std::forward<Args>( args )... );
    };
}

This function creates a lambda that calls the argument callable. We do this so that we can store it for later use; for our use case, in a destructor so that it is called at the end of a scope.
As of C++14, the noexcept specifier is not part of a function's type, so when C++17 comes around and changes that, you'll probably want to come back to this function. See this for more information: https://stackoverflow.com/q/33589752/2296177

Step 2: A function to generate the object that calls the stored lambda in its destructor.
template<class F>
auto on_scope_exit( F&& f ) noexcept
{
    class unique_scope_exit_t final
    {
    private:
        using fn_t = decltype( make_lambda( std::move( f ) ) );

        fn_t fn_;

    public:
        ~unique_scope_exit_t()
            noexcept( noexcept( fn_() ) )
        {
            fn_(); // call it in our destructor
        }

        unique_scope_exit_t( F&& fn ) // can be extended for complex lambdas
            noexcept( std::is_nothrow_move_constructible<fn_t>::value )
            : fn_{ make_lambda( std::move( fn ) ) } // take ownership and store the lambda
        {}

        unique_scope_exit_t( unique_scope_exit_t&& rhs )
            noexcept( std::is_nothrow_move_constructible<fn_t>::value )
            : fn_{ std::move( rhs.fn_ ) }
        {}

        unique_scope_exit_t( unique_scope_exit_t const& ) = delete;
        unique_scope_exit_t& operator=( unique_scope_exit_t const& ) = delete;
        unique_scope_exit_t& operator=( unique_scope_exit_t&& ) = delete;
    };
    return unique_scope_exit_t{ std::move( f ) }; // we take ownership of argument 'f'
}

Having a class definition that is local to our function allows us to use the deduced type of the template type argument.
Take advantage of the <type_traits> header to make it optimal by declaring everything noexcept when possible.
You can provide a variadic template to the constructor in order to make full usage of make_lambda() and so that you can support complex end-of-scope actions. I leave this implementation to you, it is trivial.
The C++17 standard will guarantee copy elision for the function return.

As promised, one of our objects takes a single byte when its lambda doesn't capture anything. That is:
auto test = on_scope_exit( [] { std::cout << "exiting\n"; } );
static_assert( sizeof( decltype( test ) ) == 1, "!" );

This is as it implies... the size of our generated object only takes as many bytes as it needs.
We have achieved an implementation that is both flexible and efficient (in speed and size).
Sample usage
Similar to your usage, however there are no macros involved (personal preference). You can easily wrap this in a macro if it makes you happy.
#include <iostream>

int main()
{
    // remember to store the call's return to prevent it from running early
    auto exit1 = on_scope_exit( [] { std::cout << "1\n"; } );
    auto exit2 = on_scope_exit( [] { std::cout << "2\n"; } );
    auto exit3 = on_scope_exit( [] { std::cout << "3\n"; } );

    std::cout << "4\n";
    std::cout << "5\n";
}

Full demo: http://coliru.stacked-crooked.com/a/da1afa40ae073523

Due to comments.
Other possible implementations
As noted by Loki Astari and Nikita Kakuev, the previous implementation can increase the executable's size if it is used with many (very many) different types.
In a regular use case, I still think this is a better approach than constantly constructing and destroying 32+ bytes. For example, VC++2015's std::function<void()> is 40 bytes in 32-bit mode and 64 bytes in 64-bit mode.
As Nikita Kakuev points out here, make_lambda()'s signature might be over-generalized for this specific case, so it could be omitted in favour of a more succinct on_scope_exit().
The original thought was that you could make unique_scope_exit_t's constructor take a variadic template in order to allow complex end-of-scope actions by storing callables whose operator() have parameters.
If this isn't desired, we can simply remove make_lambda() and provide the following implementation of on_scope_exit() with a reduced executable size hit:
template<class F>
auto on_scope_exit( F&& f )
    noexcept( std::is_nothrow_move_constructible<F>::value )
{
    class unique_scope_exit_t final
    {
        F f_;

    public:
        ~unique_scope_exit_t()
            noexcept( noexcept( f_() ) )
        {
            f_();
        }

        explicit unique_scope_exit_t( F&& f )
            noexcept( std::is_nothrow_move_constructible<F>::value )
            : f_( std::move( f ) )
        {}

        unique_scope_exit_t( unique_scope_exit_t&& rhs )
            noexcept( std::is_nothrow_move_constructible<F>::value )
            : f_{ std::move( rhs.f_ ) }
        {}

        unique_scope_exit_t( unique_scope_exit_t const& ) = delete;
        unique_scope_exit_t& operator=( unique_scope_exit_t const& ) = delete;
        unique_scope_exit_t& operator=( unique_scope_exit_t&& ) = delete;
    };
    return unique_scope_exit_t{ std::move( f ) };
}

This provides semantics similar to a std::unique_ptr<> solution while making intent clear and keeping size down to what is actually used (std::unique_ptr<> does store a pointer of type T after all). Usage remains as before.


Comparing objects with a tolerance I have N child classes of the same base class. The base contains abstract methods to convert the class to a byte array and vice versa.

It also contains a method to compare that instance to two other instances of the same type. Result shall be true if this instance is within the range of the other two instances (each property value is within the range of the corresponding property values).

Collections shall be compared by comparing each value.

Because there are N childs I really don't want to implement the compare method in each child (huge amount of testing and boring code) so I use a property bag in the base class and implement the compare method there.

Child property implementation:

    public int IntValue
    {
        get { return m_IntValue; }
        set
        {
            m_IntValue = value;
            AddOrUpdateProperty(value);
        }
    }
    public string StringValue
    {
        get { return m_StringValue; }
        set
        {
            m_StringValue = value;
            AddOrUpdateProperty((IComparable)value);
        }
    }
    public byte[] ByteArray
    {
        get { return m_ByteArray; }
        set
        {
            m_ByteArray = value;
            AddOrUpdateProperty(value);
        }
    }

Methods to add or update the property bag Dictionary<string, object> m_PropertyBag. Only properties that implement IComparable are allowed.

private Dictionary<string, object> m_PropertyBag = new Dictionary<string, object>();

protected void AddOrUpdateProperty(IComparable value, [CallerMemberName]string name = null)
{
    AddOrUpdateProperty(value, name);
}

protected void AddOrUpdateProperty<T>(IEnumerable<T> value, [CallerMemberName]string name = null)
    where T : IComparable
{
    AddOrUpdateProperty(value, name);
}

private void AddOrUpdateProperty(object value, string name)
{
    if (value == null) { throw new ArgumentNullException("value"); }
    if (name == null) { throw new ArgumentNullException("name"); }
    if (name == string.Empty) { throw new ArgumentException("Name must not be empty.", "name"); }
    if (name.Contains(" ")) { throw new ArgumentException("Name must not contain any white spaces.", "name"); }

    if (m_PropertyBag.ContainsKey(name)) { m_PropertyBag[name] = value; }
    else { m_PropertyBag.Add(name, value); }
}

Here is the compare method for the whole bag.

public bool Compare(Base min, Base max)
{
    if (GetType() != min.GetType() || GetType() != max.GetType())
            throw new InvalidOperationException("Not the same type.");

    var result = true;
    foreach (var property in m_PropertyBag)
    {
        if (property.Value is IEnumerable)
        {
            var values = (IEnumerable)property.Value;
            var minValuesEnum = ((IEnumerable)min.m_PropertyBag[property.Key]).GetEnumerator();
            var maxValuesEnum = ((IEnumerable)max.m_PropertyBag[property.Key]).GetEnumerator();

            foreach (var value in values)
            {
                result &= minValuesEnum.MoveNext();
                result &= maxValuesEnum.MoveNext();
                if (!result) return false;

                result &= Compare(value, minValuesEnum.Current, maxValuesEnum.Current);
                if (!result) return false;
            }
        }
        else
        {
            result &= Compare(property.Value, min.m_PropertyBag[property.Key], max.m_PropertyBag[property.Key]);
            if (!result) return false;
        }
    }
    return result;
}

Here is the compare method for a single IComparable property.

public bool Compare(object value, object min, object max)
{
    var comparableObject = (IComparable)value;
    return comparableObject.CompareTo(min) >= 0 && comparableObject.CompareTo(min) <= 0;
}

It is a good idea to use consistent styles throughout your code.  Right here, you use braces around a 1-line if/else block:

if (m_PropertyBag.ContainsKey(name)) { m_PropertyBag[name] = value; }
else { m_PropertyBag.Add(name, value); }

Right here, you do not:

if (GetType() != min.GetType() || GetType() != max.GetType())
        throw new InvalidOperationException("Not the same type.");

I recently switched to always using braces, but whatever you decide, be consistent.

I don't think you need all those assignments to result when you are just going to return if the value is false:

var result = true;
foreach (var property in m_PropertyBag)
{
    if (property.Value is IEnumerable)
    {
        var values = (IEnumerable)property.Value;
        var minValuesEnum = ((IEnumerable)min.m_PropertyBag[property.Key]).GetEnumerator();
        var maxValuesEnum = ((IEnumerable)max.m_PropertyBag[property.Key]).GetEnumerator();

        foreach (var value in values)
        {
            result &= minValuesEnum.MoveNext();
            result &= maxValuesEnum.MoveNext();
            if (!result) return false;

            result &= Compare(value, minValuesEnum.Current, maxValuesEnum.Current);
            if (!result) return false;
        }
    }
    else
    {
        result &= Compare(property.Value, min.m_PropertyBag[property.Key], max.m_PropertyBag[property.Key]);
        if (!result) return false;
    }
}
return result;

You can do this instead:

foreach (var property in m_PropertyBag)
{
    if (property.Value is IEnumerable)
    {
        var values = (IEnumerable)property.Value;
        var minValuesEnum = ((IEnumerable)min.m_PropertyBag[property.Key]).GetEnumerator();
        var maxValuesEnum = ((IEnumerable)max.m_PropertyBag[property.Key]).GetEnumerator();

        foreach (var value in values)
        {
            if (!minValuesEnum.MoveNext() ||
                !maxValuesEnum.MoveNext() ||
                !Compare(value, minValuesEnum.Current, maxValuesEnum.Current);
            {
                return false;
            }
        }
    }
    else if (!Compare(property.Value, min.m_PropertyBag[property.Key], max.m_PropertyBag[property.Key]))
    {
        return false;
    }
}
return true;

I'm not sure what is going to, or should, happen if neither the if nor the else if statements execute.  Should there be an else { return false; } statement in there?


Merge 2 arrays, when one is larger and can accomodate smaller How will you merge two sorted arrays, when The longer array has empty spaces to accomodate the second array. Complexity: O (items in longer  + items in smaller).  Request for making code concise, clean and optimal.

public final class MergeArrays {

    private MergeArrays() {}

    private static void check(Integer[] big, Integer[] small) {
        if (big == null && small == null) {
            throw new NullPointerException("The input arrays are null.");
        }

        if (big.length < small.length) {
            throw new IllegalArgumentException("The arrays is smaller smaller array");
        }
    }

    public static Integer[] mergeSortedArrays(Integer[] big, Integer[] small) {
        check(big, small);
        int limit = 0;
        for (Integer val : big) {
            if (val == null) { break; }
            limit++;
        }
        return mergeSortedArrays(big, limit, small);
    }

    private static Integer[] mergeSortedArrays(Integer[] big, int limit, Integer[] small) {
        assert big != null;
        assert small != null;

        int limitIndex = limit - 1;
        int bigIndex = big.length - 1;
        int smallIndex = small.length - 1;

        while (smallIndex >= 0) {
            if (limitIndex < 0 || big[limitIndex] < small[smallIndex]) {
                big[bigIndex--] = small[smallIndex--];
            } else {
                big[bigIndex--] = big[limitIndex--];
            }
        }

        return big;
    }

Do not throw NullPointerException when arguments are null. NPE is thrown when you are trying to access properties and methods of null reference. Not when you are checking on some preconditions. Throw IllegalArgumentException as you do in length validation. In other method you use assertions. Chose one method and stick with it, dont mix them.

You don have to define empty constructor. It is created implicitly.


A straightforward method for computing the Binomial probability mass function I've been writing up code to test an alternate way of calculating the Binomial probability mass function. The Binomial random variable is the number of heads out of N coin tosses with coins that have probability p of being heads.

The algorithm is pretty simple, we loop through the N coin tosses and update the probability that there are k successes as:

pmf[k] = pmf[k-1] * p +  pmf[k] * (1 - p)

Since we know pmf[k] = 0 for k > n (there can't be more successes than coins tossed so far), we evaluate this line N(N+1)/2 times.

The algorithm works well, is exact, and avoids a number of common pitfalls when computing the Binomial distribution. This is the best implementation I've managed so far, which takes ~190 ms to run on my Dell XPS-15 for N = 15000 and long double p = 0.5.

template <typename T>
std::vector<T> ComputePmf(T p, unsigned int N) {
  std::vector<T> pmf(N + 1, 0.0);
  pmf[0] = 1.0;

  auto k = 0;
  for (auto n = 0; n < N; n++) {
    for (k = n + 1; k > 0; --k) {
      pmf[k] += p * (pmf[k - 1] - pmf[k]);
    }
    pmf[0] *= (1 - p);
  }
  return pmf;
}

Ordinarily I'd hang up my hat and move on, but when I implemented the same algorithm in Julia it ran in 150 ms! Since then I've been stuck trying to figure out how to make my c++ version faster - I've tried:

using different data types (long double works best, maybe due to
SIMD alignment?),
std::array versus std::vector (no difference),
other ways of factoring the inner loop (this is the best I've come up with),
clang and gcc compilers (gcc does slightly better),

It seems like the only real information that I'm not exploiting is the very sequential nature of the data access in this algorithm. Any suggestions?

High powers of small-magnitude numbers tend toward zero.  When you get subnormal floating-point numbers, then there's a heavy speed penalty; this point is reached sooner with double than with long double (and reached even sooner still using float).

A simple way to improve the speed may be to disable subnormals for this function - e.g. compile with g++ -ffast-math to push subnormal numbers to zero.


Extracting grouped objects into a flattened list I need to optimize this javascript method because it is a bit slow, I don't want to use the flat() command, as for some reason my angular6 app does not understand the vanilla flat() command, or display some annoying warning messages.

So check the below:

  my original object
  my result object desired
  my slow solution (works, but is slow)

1. my original object:

  const data = [
      {
        "id": 1,
        "name": "Application",
        "groups": [
          {
            "groupName": "",
            "configurations": [
              {
                "id": 17,
                "icon": "access_time",
                "title": "Daily Order Cut-Off Time",
                "description": "Daily Order Cut-Off Time",
                "code": "daily-order-cut-off-time",
                "value": "09:35",
                "valueType": "Time",
                "configurationTypeId": 11,
                "definition": {
                  "step": "none"
                },
                "isDefault": false
              }
            ]
          }
        ]
      },
      {
        "id": 3,
        "name": "Processing",
        "groups": [
          {
            "groupName": "",
            "configurations": [
              {
                "id": 1078,
                "icon": "flash_on",
                "title": "Auto Process",
                "description": "Will process all orders that are in the same batch",
                "code": "processing-auto-process",
                "value": "0",
                "valueType": "Boolean",
                "configurationTypeId": 6,
                "definition": null,
                "isDefault": false
              },
              {
                "id": 1074,
                "icon": "subdirectory_arrow_right",
                "title": "Allow Under Picking",
                "description": "Allow under pick when processing order?",
                "code": "processing-allow-under-picks",
                "value": "0",
                "valueType": "Boolean",
                "configurationTypeId": 6,
                "definition": null,
                "isDefault": false
              }
            ]
          }
        ]
      }
    ];

2. object desired:

 [
      {
        "id": 17,
        "code": "daily-order-cut-off-time",
        "value": "09:35"
      },
      {
        "id": 1078,
        "code": "processing-auto-process",
        "value": "0"
      },
      {
        "id": 1074,
        "code": "processing-allow-under-picks",
        "value": "0"
      }
    ]

My slow solution:

const result = data.map(module => module.groups.map(configurations => configurations.configurations.map(config => ({ id: config.id, code: config.code, value: config.value })))).reduce((l,n) => l.concat(n), []).reduce((l2,n2) => l2.concat(n2),[]));

As commented, the ES6 spread operator flattens like you want.  The syntax is a little weird for multiple invocations:

result = [].concat(...[].concat(...data.map( d => d.groups.map( g => g.configurations.map( c => ({id:c.id, code:c.code, value:c.value}))))))

The arr.flat() method does the same thing more simply.  It takes an optional depth argument to indicate how many levels to descend when flattening.  In your case, depth is 2.

.flat is a recent feature that may not exist on your platform.  Most notably, Node 10 and MS Edge do not have it, while both of those do have the spread operator:

data.map( d => d.groups.map( g => g.configurations.map( c => ({id:c.id, code:c.code, value:c.value})))).flat(2)


Make multiple conditions in if expression more readable My logic has to be written in this way, but I think the if expression is too long to read.

How can I make it more readable?

  if @tool_cvt_video_info_test.save and check_and_update_firmware_model_and_version(params["camera_ip"]) and save_uploaded(params[:videos])

    format.html { redirect_to @tool_cvt_video_info_test, notice: 'Video info test was successfully created.' }
    format.json { render action: 'show', status: :created, location: @tool_cvt_video_info_test }
  else
    format.html { render action: 'new' }
    format.json { render json: @tool_cvt_video_info_test.errors, status: :unprocessable_entity }
  end

Given the little context you have provided in the question, there isn't much to recommend, other than whitespace changes.

if  @tool_cvt_video_info_test.save and
    check_and_update_firmware_model_and_version(params["camera_ip"]) and
    save_uploaded(params[:videos])
  format.html {
    redirect_to @tool_cvt_video_info_test,
    notice: 'Video info test was successfully created.'
  }
  format.json {
    render action: 'show',
    status: :created,
    location: @tool_cvt_video_info_test
  }
else
  format.html { render action: 'new' }
  format.json {
    render json: @tool_cvt_video_info_test.errors,
    status: :unprocessable_entity
  }
end

I am a bit suspicious of this code, though.  It's saving some information about a video test, possibly updating the camera's firmware, and saving an uploaded video?  That's a lot of functionality to put in one if-condition, and if anything goes wrong, the user probably won't get sufficient feedback as to exactly what failed.


Refactor Ruby code from Hacker News API. Trouble parsing the JSON data I made a command line Hacker News reader in Ruby that pulls the latest 10 stories from HN and allows the user to cycle through them or open them with Launchy. This was to play around with Ruby and APIs.

The data that I pulled from HN was messy, like an array of hashes with arrays and hashes inside. I feel like my code is messy, and I am unsure of how it can be made cleaner.

Any advice? I'm still a begninner.

news.rb --

class News
  def initialize title, url, author
    @title = title
    @url = url
    @author = author
  end

  # Format display of each story.
  def to_s
    "\n\tTitle:  #{@title}\n
    \tAuthor: #{@author}\n
    \tUrl: #{@url}\n\n"
  end

  # Insert current object's url into a variable accessible by Launchy
  def get_url
    @url
  end

end

# Show the news story, and ask if user wants open link or move on.
def show_story
    puts @stories[@counter]
    @link = @stories[@counter].get_url
    puts "To view this story in your browser, type OPEN\n
    To see the next story, type NEXT"
    @start = gets.chomp.downcase
    shuffle
  end

# Determine if user wants to open link or move on.
def shuffle
  if @start == "open"
    Launchy.open(@link)
    next_story
  elsif @start == "next"
    @counter += 1
    show_story
  else
    puts "Sorry, you didn't type a correct command.\n
    To view this story in your browser, type OPEN\n
    To see the next story, type NEXT"
    @start = gets.chomp.downcase
    shuffle
  end
end

# After opening link, ask if user still wants to see next story
def next_story
  puts "Would you still like to see the next story? Type NEXT"
  @start = gets.chomp.downcase
  shuffle
end

runner.rb --

require 'json'
require 'rest-client'
require 'launchy'
require_relative 'news.rb'

# Pull the latest/top 10 stories from Hacker News
data = RestClient.get('http://api.thriftdb.com/api.hnsearch.com/items/_search?limit=10&sortby=product(points,pow(2,div(div(ms(create_ts,NOW),3600000),72)))%20desc&pretty_print=true')

# Parse using JSON
parsed = JSON.parse data

# Take out the everything not under "Results" key
results = parsed.reject { |key, value| key != "results"}

# Create array and slap the value of "Results" into it
pull_items = []
pull_items = results["results"].each do |items|
  pull_items << items
end

# Create an array and make each "Item" into its own item
pull_info = []
pull_items.each do |x|
  pull_info << x["item"]
end

# Create a new object for each news story
@stories = pull_info.map do |s|
  News.new s["title"], s["url"], s["username"]
end

# Logo and Welcome Message
puts <<STRING
  _   _            _               _   _
 | | | |          | |             | \\ | |
 | |_| | __ _  ___| | _____ _ __  |  \\| | _____      _____
 |  _  |/ _` |/ __| |/ / _ | '__| | . ` |/ _ \\ \\ /\\ / / __|
 | | | | (_| | (__|   |  __| |    | |\\  |  __/\\ V  V /\\__ \
 \\_| |_/\\__,_|\\___|_|\\_\\___|_|    \\_| \\_/\\___| \\_/\\_/ |___/
        Welcome to Hacker News... from the Command Line!
STRING

# View the first story
puts "\nTo see the latest post, press any key"
start = gets
@counter = 0
show_story

Redundant operations
After reading the data from the web service, you go over it a number of times, which seems redundant:

results = parsed.reject { |key, value| key != "results"}

Why is that important? You go over the whole JSON, and discard everything - but you gain nothing. Simply use only what you need, no need to actively remove it.

pull_items = []
pull_items = results["results"].each do |items|
  pull_items << items
end

This is simply a very convoluted way of saying pull_items = results['results']...

pull_info = []
pull_items.each do |x|
  pull_info << x["item"]
end

Ruby's idiomatic way of saying the same thing is pull_info = pull_items.map { |x| x['item'] }

@stories = pull_info.map do |s|
  News.new s["title"], s["url"], s["username"]
end

If this is what you actually wanted, you don't really need all the temporary artifacts in the middle - simply do it in one swoop:

@stories = results['results'].map do |r|
  item = r['item']
  News.new item['title'], item['url'], item['username']
end

Naming Conventions
It is conventional in ruby to name getters by the name of the member, without the get_XXX. For that convention, there is even a shortcut - attr_reader. So, instead of writing def get_url you should do:

class News
  attr_reader :url
  ...
end

@link = @stories[@counter].url

and it will just work...

Code Encapsulation
A ruby file should contain a single class. It should not contain any floating methods.
Actually, there should not be any floating methods. Create a different class, or a module, which will contain the show_story, shuffle, and next_story. This will also force you to name the module, and make it more obvious what is its role in your application.


Custom error-logging I'm writing a static class to log my application errors into a text file. I am planning on using this as a library on any application I intend to develop in the future. Please let me know how I can improve this more and what could be changed so it would perform better.

public static class Logger
{
    public static void Log(Exception ex)
    {
        StreamWriter sw = null;
        try
        {
            sw = new StreamWriter(AppDomain.CurrentDomain.BaseDirectory + "\\Log.txt", true);
            sw.WriteLine(
                DateTime.Now.ToString() + " : " + "\r\nSource " + ex.Source.ToString().Trim() +
                "\r\nMessage : " + ex.Message.ToString().Trim() +
                "\r\nInner Exceptions: " + Convert.ToString(ex.InnerException) +
                "\r\nException thrown from: " + getExceptionGeneratedMethod(ex) +
                "\r\nLine Number: " + getExceptionGeneratedLineNumber(ex) +
                "\r\nStack Trace: " + ex.StackTrace.ToString() ;
            sw.Flush();
            sw.Close();
        }
        catch (Exception e)
        {
            sw = new StreamWriter(AppDomain.CurrentDomain.BaseDirectory + "\\Log.txt", true);
            sw.WriteLine("Error Could not be logged!!");
            throw ex;
        }
    }

    /// <summary>Writes a log file in the root directory with a custom message.
    /// <param name="message">String that will be displayed as a message</param>
    /// </summary>
    public static void writeCustomErrorLog(string message)
    {
        StreamWriter sw = null;
        try
        {
            sw = new StreamWriter(AppDomain.CurrentDomain.BaseDirectory + "\\PassiveLog.txt", true);
            sw.WriteLine(DateTime.Now.ToString() + " : " + "\n Custom Message: " + message);
            sw.Flush();
            sw.Close();
        }
        catch (Exception ex)
        {
            throw ex;
        }
    }

    /// <summary>Writes a log file in the a directory  passed in as a location with a custom message and a custom file-name.
    /// <param name="message">String that will be displayed as a message</param>
    /// <param name="location">String value that accepts a location for the log file to be generated</param>
    /// <param name="fileName">String value that accepts a file-name for the log file to be generated</param>
    /// </summary>
    public static void writeCustomErrorLog(string location, string message,string fileName)
    {
        StreamWriter sw = null;
        try
        {
            sw = new StreamWriter(location + "\\"+fileName+".txt", true);
            sw.WriteLine(DateTime.Now.ToString() + " : " + "\n" + message);
            sw.Flush();
            sw.Close();
        }
        catch (Exception ex)
        {
            throw ex;
        }
    }

    /// <summary>Returns the Mathod name the exception occured
    /// <param name="ex">The <c>exception</c> must be passed as a parameter</param>
    /// <returns>returns the method name where the exception occured as a <c>string</c></returns>
    /// </summary>
    public static string getExceptionGeneratedMethod(Exception ex)
    {
        var s = new StackTrace(ex);
        var thisasm = Assembly.GetExecutingAssembly();
        string methodname = s.GetFrames().Select(f => f.GetMethod()).First(m =>
        m.Module.Assembly == thisasm).Name.ToString();
        return methodname;
    }

    /// <summary>Returns the line number on which the exception occured
    /// <param name="ex">The <c>exception</c> must be passed as a parameter</param>
    /// <returns>returns the line number where the exception occured as a <c>integer</c> value</returns>
    /// </summary>
    public static int getExceptionGeneratedLineNumber(Exception ex)
    {
        var st = new StackTrace(ex, true);
        var frame = st.GetFrame(0);
        int line = frame.GetFileLineNumber();
        return line;
    }
}

Reusable methods

Notice that you have several methods that essentially do the same thing. Log and both writeCustomErrorLog methods follow the same approach:

StreamWriter sw = null;
try
{
    sw = new StreamWriter("some file path", true);
    sw.WriteLine("some string");
    sw.Flush();
    sw.Close();
catch (Exception ex)
{
    throw ex;
}

This can easily be put into a reusable method:

private void WriteMessageToFile(string filepath, string message)
{
    StreamWriter sw = null;
    try
    {
        sw = new StreamWriter(filepath, true);
        sw.WriteLine(message);
        sw.Flush();
        sw.Close();
    catch (Exception ex)
    {
        throw ex;
    }
}

And then your methods become much simpler and less repetitive:

public void writeCustomErrorLog(string location, string message,string fileName)
{
    string filePath = location + "\\" + fileName +".txt";
    string logMessage = DateTime.Now.ToString() + " : " + "\n" + message;

    WriteMessageToFile(filePath, logMessage);
}

Catching and throwing

try
{
   // ...
}
catch (Exception ex)
{
    throw ex;
}

Catching an exception should be done when you want to handle the exception.
Throwing an exception should be done when you don't want to handle the exception (or raise a new exception)

There is no point to catching an exception, only to then throw it. That is essentially like queueing at a supermarket checkout when you don't have any items you want to buy. There's no point to doing so.

The try/catch can be removed altogether in these cases. Note that the try/catch could be useful if you actually did something with it:

try
{
   // ...
}
catch (Exception ex)
{
    Trace.Write("Exception occurred here!");

    throw;
}

Note the different between throw ex; and throw;.

throw ex; resets the stacktrace to where you call throw ex;. This effectively removes deeper stack trace information
throw; retains the stack trace of the exception as it was initially raised. This does not remove data.

Throwing an existing exception is rarely a good idea.

Hardcoding

AppDomain.CurrentDomain.BaseDirectory + "\\Log.txt"
AppDomain.CurrentDomain.BaseDirectory + "\\PassiveLog.txt"

Don't hardcode your location, nor your filename.

You intend to use this in future projects. It's not unforeseeable that you're going to want to decide where to put the file, or what you should name it (especially if you want to use more than one log file in the same application).

Also, I have no idea what a "passive log" is. The name isn't very descriptive.

String concatenation

DateTime.Now.ToString() + " : " + "\r\nSource " + ex.Source.ToString().Trim() +
"\r\nMessage : " + ex.Message.ToString().Trim() +
"\r\nInner Exceptions: " + Convert.ToString(ex.InnerException) +
"\r\nException thrown from: " + getExceptionGeneratedMethod(ex) +
"\r\nLine Number: " + getExceptionGeneratedLineNumber(ex) +
"\r\nStack Trace: " + ex.StackTrace.ToString()

Do not concatenate strings with +.

Strings are immutable. That means that you can't change a string, you can only create a new string (and the old one will be discarded.

Let's take a simple example:

"a" + "b" + "c" + "d"

These concatenations are done step by step. Look at the needed memory allocation:

Create a 1 character string (a)
Create a 1 character string (b)
Create a 2 character string (a+b)
Create a 1 character string (c)
Create a 3 character string (ab+c)
Create a 1 character string (d)
Create a 4 character string (abc+d)

To concatenate 4 strings, you're had to allocate 7 strings.
For a 4 character result, you've effectively allocated 13 characters' worth of strings.

This problem quickly grows out of proportion and leads to bad performance, massive memory usages, and eventual OutOfMemoryExceptions being thrown. Furthermore, avoiding this is very simple:

Option 1 - String.Format

This is more useful for cases where you are replacing a fixed set of placeholders.

string result = String.Format("{0} is the father of {1}", nameOfFather, nameOfChild);

Note that interpolated strings are a fairly recent addition that simplify the syntax:

string result = $"{nameOfFather} is the father of {nameOfChild}";

Option 2 - StringBuilder

This is more useful for cases where you dynamically generate a string, instead of replacing a fixed set of placeholders:

StringBuilder sb = new StringBuilder();

sb.Append(nameOfFather);
sb.Append(" is the father of ");
sb.Append(nameOfChild);

string result = sb.ToString();

I repeat again: Do not concatenate strings with +. Especially since interpolated strings have been added; there's no reasonable "simply syntax" argument to using + concatenation.

  Addendum

  Piedar made a really interesting remark in the comments. Apparently, string + string concatenation is now converted to String.Concat
   by the compiler. This effectively negates the issue I'm pointing at.

  However, I still urge you to not do + concatenation on big string anyway, from a readability perspective. value + " seconds" is readably enough, but you're pasting a lot of different things together and the code starts looking bulky and ugly.
  However, this is a style (and, in extreme cases, readability) argument, not a technical one.

Statics

Everything you've listed is static. Why?

This issue is strongly related to why you hardcoded your values. You think that your single log is a catch-all for any future application's needs and you'll never need to tweak it or change it. You're effectively making it so that a future consumer of your library has no configuration options.

This is inherently bad design. This is equivalent to Word being able to save files in a single (hardcoded) directory, or Firefox telling you what your homepage should be without alowing you to change it.

Good applications (and libraries) give control to the user, they don't tell the user what to do.
Note: there is nothing wrong with providing a default behavior, but you need to give your user the option of changing the default behavior if they so choose.

Separation of concerns

Other than not allowing the user to change key settings, you're also handling too much here. Your one class is handling many responsibilities:

Deciding where to put the file
Accessing and writing to the file
Deciding how to format the log messages

These things should ideally be split into separate classes.

I understand that the need for separating this into classes is not that great at the moment. However, once you implement the other suggested changes (configuration options), your code will quickly grow to a size where the need for separation becomes more apparent.

Ideally, this is a better logging approach:

LogFile allows you to create a log with a specific configuration (filename, location)
ExceptionLogMessageFormatter takes an exception and turns it into a log message (string).
Logger takes a pre-formatted string message and writes it to a pre-configured logfile.

You're currently only logging exceptions, but the need for separation becomes more apparent when you want to also log other objects. If you want to e.g. log an object of type Person, you'll create a separate PersonLogMessageFormatter class, but you will be able to reuse the same LogFile class (a different file with different settings, but the same class in code!) and Logger class (which logs the string message to this different log).

Don't reinvent the wheel!

This is the most important lesson to draw here. However, it does come with a caveat, which I will mention at the end.

What you've built here is a fairly simple tool, for a problem that is not new to the field of software development. (That's not an insult - in case you interpreted it as such).
Odds are that someone is already going to have created a library to handle this exact job. And they have:

NLog
Log4Net
Elmah

These tools are very well developed, offer an incredibly amount of configuratbility, and are able to handle fringe cases that you probably haven't even thought of but will eventually have to implement a solution for.

Personally, I'm a big fan of NLog. I've never come across a need for logging that could not be handled by NLog. I like how it works, and I find that it gives me a perfect balance of good default behavior and exceedingly configurable custom behavior if I need it.

If you were going to write a story about a world based on medieval fantasy with magic and multiple humanoid races; I would suggest that you watch e.g. the Lord of the Rings movies (or read the books of course).
Even if your story is going to be different, it gives you a nice overview of the ins and outs of your story's setting, pitfalls to avoid, how to create interesting an immersive plots, ...

Similarly, if you want to write a logging framework, I suggest that you first try working with existing tools.
Even if your tool is going to do something new; seeing the existing framework gives you a good overview of what features you're going to need to add to your tool to make it user friendly, and it helps you draw the line on how much configuration options you need to add to it to fulfill most needs.

  Caveat
  However, writing your own logging framework from scratch (with no knowledge of existing frameworks) can be rewarding in and of itself as a training exercise.

  Based on your code, I surmise that you are a beginner, at least to creating reusable libraries. So if writing this library benefits you for training purposes, don't let my review stop you.

  But if your only goal is to create a functionally usable tool, then I do suggest first looking for existing solutions, either to already solve your problem or even just to get some inspiration/ideas.


Converting date format in range of cells This code works, but I was wondering if there's a way to get excel to identify the date data more easily. This comes from an imported file where the date field(s) come in as text and in a European format.

Company specific information has been changed.  the function FndLast gets the last row of the data.

Sub FormatTheDates()

Dim rng As Range
Dim lstRow As Long
Dim StartRow As Long

Set rng = sheets("MySheet").Cells.Find("Keyfield")

If Not rng Is Nothing Then
    With sheets("MySheet")
    ****Data starts one row below key field***
        StartRow = rng.Row + 1
        lstRow = FndLast(1, .Cells)
        Set rng = .Range(.Cells(StartRow, 5).Address, .Cells(lstRow, 5).Address)
        rng.NumberFormat = "m/d/yyyy"
        rng.value = rng.value
    End With
End If

Set rng = sheets("MySheet2").Cells.Find("Keyfield")

If Not rng Is Nothing Then
    With sheets("MySheet2")
        StartRow = rng.Row + 1
        lstRow = FndLast(1, .Cells)
        Set rng = .Range(.Cells(StartRow, 7).Address, .Cells(lstRow, 7).Address)
        rng.NumberFormat = "m/d/yyyy"
        rng.value = rng.value
    End With
End If

End Sub

The fndlast function takes a 1,2, or 3 as a parameter and finds the last row, column or cell on a sheet. I use this as opposed to usedrange as it returns the last one with data.

Function FndLast(choice As Long, rng As Range)

' 1 = last row
' 2 = last column
' 3 = last cell

Dim lrw As Long
Dim lCol As Long

Select Case choice

Case 1:
    On Error Resume Next
    FndLast = rng.Find(What:="*", _
                    After:=rng.Cells(1), _
                    LookAt:=xlPart, _
                    LookIn:=xlFormulas, _
                    SearchOrder:=xlByRows, _
                    SearchDirection:=xlPrevious, _
                    MatchCase:=False).Row
    On Error GoTo 0

Case 2:
    On Error Resume Next
    FndLast = rng.Find(What:="*", _
                    After:=rng.Cells(1), _
                    LookAt:=xlPart, _
                    LookIn:=xlFormulas, _
                    SearchOrder:=xlByColumns, _
                    SearchDirection:=xlPrevious, _
                    MatchCase:=False).Column
    On Error GoTo 0

Case 3:
    On Error Resume Next
    lrw = rng.Find(What:="*", _
                   After:=rng.Cells(1), _
                   LookAt:=xlPart, _
                   LookIn:=xlFormulas, _
                   SearchOrder:=xlByRows, _
                   SearchDirection:=xlPrevious, _
                   MatchCase:=False).Row
    On Error GoTo 0

    On Error Resume Next
    lCol = rng.Find(What:="*", _
                    After:=rng.Cells(1), _
                    LookAt:=xlPart, _
                    LookIn:=xlFormulas, _
                    SearchOrder:=xlByColumns, _
                    SearchDirection:=xlPrevious, _
                    MatchCase:=False).Column
    On Error GoTo 0

    On Error Resume Next
    FndLast = rng.Parent.Cells(lrw, lCol).Address(False, False)
    If Err.Number > 0 Then
        Last = rng.Cells(1).Address(False, False)
        Err.Clear
    End If
    On Error GoTo 0

End Select
End Function

As I said, all of this works, and fairly quickly, but it feels like the Range.value = range.value seems unnecessary. Is it, or is this just an Excel quirk that I need to work around in order for my code to work?

In solving the overall problem of date conversion, your code makes a few assumptions that have tripped me up in the past.

Always use Option Explicit
Set references to the workbook and your worksheets
Make a habit of trying to use arrays instead of cells directly from the worksheet

Using these habits along will greatly improve your code. In particular, notice in the example below how all the variables end up being fully referenced back to the parent sheet and workbook so that you (and the VBA) never gets confused.

I found it less useful to have a single function that determined the last row or column or cell, since that generated more code than I usually use to find it. Instead, I arrived at a different decomposition of the problem into functions.

While you created a function to find the last row/cell/column, you also have a repeated need to convert the dates within an area. So the "main" routine breaks down into an easily understandable set of repeatable steps (functions):

Option Explicit

Public Sub main()
    Dim wb As Workbook
    Dim targetDateSheet As Worksheet
    Dim myDateArea As Range
    Set wb = ThisWorkbook

    Set targetDateSheet = wb.Sheets("MySheet1")
    Set myDateArea = GetDataArea(targetDateSheet, "Start")
    ConvertDates myDateArea

    Set targetDateSheet = wb.Sheets("MySheet2")
    Set myDateArea = GetDataArea(targetDateSheet, "Keyfield")
    ConvertDates myDateArea
End Sub

The GetDataArea function quickly determines where the dates are based on a search for the key field:

Private Function GetDataArea(ByRef targetSheet As Worksheet, _
                             ByRef keyField As String) As Range
    Dim theArea As Range
    Dim lastRow As Long
    Dim lastCol As Long
    Dim numRows As Long
    Dim numCols As Long
    With targetSheet
        Set theArea = .Cells.Find(keyField)
        If Not theArea Is Nothing Then
            lastRow = .Cells(.Cells.Rows.Count, theArea.Cells(1, 1).Column).End(xlUp).Row
            lastCol = .Cells(theArea.Cells(1, 1).Row, .Cells.Columns.Count).End(xlToLeft).Column
            numRows = lastRow - theArea.Cells(1, 1).Row
            numCols = lastCol - theArea.Cells(1, 1).Column + 1
            '--- assume the first row contains headers for each column, so offset
            Set GetDataArea = theArea.Offset(1, 0).Resize(numRows, numCols)
        Else
            theArea = Nothing
        End If
    End With
End Function

I've found it much quicker and simpler to find the last row and column using the above method, rather than the Find method.

Converting the dates in the given range then breaks down into these pseudo-steps:

copy the cell range into a memory array (for speed and ease of referencing)
loop through the entire array and detect the date data and convert it to the Excel serial date (numerical) value
copy the converted array back to the worksheet range and set the desired date format

It's very important NOT to assume that the text string you find is read by excel as a date string. It may or may not be recognized that way. In order to guarantee you're converting the string to an Excel date value, you should use the DateSerial function as shown and then format the worksheet cells as you need.

This becomes:

Private Sub ConvertDates(ByRef theArea As Range)
    Dim area As Variant
    area = theArea                               'copied to memory array

    '--- look for a string pattern with two "/"s and assume that value
    '    is a European-format date string
    Dim i As Long
    Dim j As Long
    For i = LBound(area, 1) To UBound(area, 1)
        For j = LBound(area, 2) To UBound(area, 2)
            Dim pos1 As Long
            Dim pos2 As Long
            pos1 = 0
            pos2 = 0
            pos1 = InStr(1, area(i, j), "/", vbTextCompare)
            If pos1 > 0 Then
                pos2 = InStr(pos1 + 1, area(i, j), "/", vbTextCompare)
            End If
            If (pos1 > 0) And (pos2 > 0) Then
                Dim dd As Long
                Dim mm As Long
                Dim yy As Long
                dd = CLng(Left(area(i, j), pos1 - 1))
                mm = CLng(Mid(area(i, j), pos1 + 1, pos2 - pos1 - 1))
                yy = CLng(Right(area(i, j), Len(area(i, j)) - pos2))
                area(i, j) = DateSerial(yy, mm, dd)
            End If
        Next j
    Next i

    '--- now put the array back onto the worksheet and set the
    '    number format for those cells to the desired
    theArea = area
    theArea.Cells.NumberFormat = "dd-mmm-yyyy"
End Sub

Note that I'm testing each value to make certain it's a date with two "/" appearing before I attempt to convert it.

For convenience, here's the whole module in a single area:

Option Explicit

Public Sub main()
    Dim wb As Workbook
    Dim targetDateSheet As Worksheet
    Dim myDateArea As Range
    Set wb = ThisWorkbook

    Set targetDateSheet = wb.Sheets("MySheet1")
    Set myDateArea = GetDataArea(targetDateSheet, "Start")
    ConvertDates myDateArea

    Set targetDateSheet = wb.Sheets("MySheet2")
    Set myDateArea = GetDataArea(targetDateSheet, "Keyfield")
    ConvertDates myDateArea
End Sub

Private Function GetDataArea(ByRef targetSheet As Worksheet, _
                             ByRef keyField As String) As Range
    Dim theArea As Range
    Dim lastRow As Long
    Dim lastCol As Long
    Dim numRows As Long
    Dim numCols As Long
    With targetSheet
        Set theArea = .Cells.Find(keyField)
        If Not theArea Is Nothing Then
            lastRow = .Cells(.Cells.Rows.Count, theArea.Cells(1, 1).Column).End(xlUp).Row
            lastCol = .Cells(theArea.Cells(1, 1).Row, .Cells.Columns.Count).End(xlToLeft).Column
            numRows = lastRow - theArea.Cells(1, 1).Row
            numCols = lastCol - theArea.Cells(1, 1).Column + 1
            '--- assume the first row contains headers for each column, so offset
            Set GetDataArea = theArea.Offset(1, 0).Resize(numRows, numCols)
        Else
            theArea = Nothing
        End If
    End With
End Function

Private Sub ConvertDates(ByRef theArea As Range)
    Dim area As Variant
    area = theArea                               'copied to memory array

    '--- look for a string pattern with two "/"s and assume that value
    '    is a European-format date string
    Dim i As Long
    Dim j As Long
    For i = LBound(area, 1) To UBound(area, 1)
        For j = LBound(area, 2) To UBound(area, 2)
            Dim pos1 As Long
            Dim pos2 As Long
            pos1 = 0
            pos2 = 0
            pos1 = InStr(1, area(i, j), "/", vbTextCompare)
            If pos1 > 0 Then
                pos2 = InStr(pos1 + 1, area(i, j), "/", vbTextCompare)
            End If
            If (pos1 > 0) And (pos2 > 0) Then
                Dim dd As Long
                Dim mm As Long
                Dim yy As Long
                dd = CLng(Left(area(i, j), pos1 - 1))
                mm = CLng(Mid(area(i, j), pos1 + 1, pos2 - pos1 - 1))
                yy = CLng(Right(area(i, j), Len(area(i, j)) - pos2))
                area(i, j) = DateSerial(yy, mm, dd)
            End If
        Next j
    Next i

    '--- now put the array back onto the worksheet and set the
    '    number format for those cells to the desired
    theArea = area
    theArea.Cells.NumberFormat = "dd-mmm-yyyy"
End Sub


Is it possible to optimize the following boolean checks? The following code works and it prints out only True as it is supposed to, for all the different cases:

class User(object):
  def __init__(self, key):
    self.key = key

def is_ok(user_dbs, self_db):
  count = len(user_dbs)
  if not self_db:
    return not count
  return (count != 1 or self_db.key == user_dbs[0].key) and count != 2

print 'Should print only True:'
print is_ok([],                 None)    is True   # found nothing and no self
print is_ok([],                 User(1)) is True   # found nothing and self
print is_ok([User(1)],          None)    is False  # found one and no self
print is_ok([User(1)],          User(1)) is True   # found one and it is you
print is_ok([User(2)],          User(1)) is False  # found one and it is not you
print is_ok([User(1), User(2)], None)    is False  # found two and no self
print is_ok([User(1), User(2)], User(1)) is False  # found two and one of them is you
print is_ok([User(2), User(1)], User(1)) is False  # found two and one of them is you
print is_ok([User(2), User(3)], User(1)) is False  # found two and none of them is you

The whole thing is to check if the username is available for a new user or when an existing user is updating his current username.

The user_dbs can have maximum 2 records as it has a limit in the actual query. So here is the breakdown:

If there are 2 records then we know for sure that the particular username is not available.
If there are 0 records then we know for sure that this username is available.
If there is 1 record then:

If it's a new user then the username is not available
If it's an existing user and that user is yourself then the username is available, otherwise not.

So can we optimize somehow the is_ok function above?

Try it live: repl.it/XHC

I came up with a different solution by utilizing Python's magic method for == which is __eq__(self, other).

I define it in the User class like so:

class User(object):
    def __init__(self, key):
        self.key = key
    def __eq__(self, other):
        return self.key == other.key if isinstance(other, User) else False

This allows you to do the obvious User(1) == User(2) testing but also this allows you to test membership in a collection using the in keyword:

User(1) in [User(1)] # This will return True
User(1) in [User(2)] # This will return False

We can then change the condition slightly to make the is_ok function like this:

def is_ok(user_dbs, self_db):
    count = len(user_dbs)
    if not self_db:
        return not count
    return count == 1 and self_db in user_dbs or count == 0

This has the advantage of being a bit more readable, and hence Pythonic. This might come in handy elsewhere as well.

Another minor point is that if you ever rename key to something else or change the structure of your User class, this code will still work (remember to update __eq__!), while most of the alternatives will break.


Comparing numbers and making a calculation I have a piece of javascript I wrote for a recent work project that is essentially taking a deviceorientation event object, taking either the beta value or gamma value of that object and makes a calculation.

While I can't necessarily put my exact code here just in case that breaks any of the agreements I signed, the general idea is as follows. Just a note: this code is already headed for production and I'm not looking for someone to do my work for me. I'm asking this so I can use it for new code in the future. I like to keep functions small and avoid using complex structures like the one below, but my time in this field is still limited and I could not think of a better way to handle this.

The names of functions/variables have been changed to protect the identity of the innocent. I'm also skipping a few of the in between functions as I'm just wondering if there's a cleaner way to handle this calculation.

Code starts with adding an event listener:

window.addEventListener('deviceorientation', doSomeStuff, false);

doSomeStuff eventually makes a call to makeCalculation and passes along the event object:

function doSomeStuff(event){
    // Other stuff happens...
    makeCalculation(event);
}

At this point, I want to make a calculation based on the beta or gamma value of that event object. I have a minimum and maximum range I want stay within, as well as a slight alteration to the calculation when the value is between another range within the min-max range. I will use event.gamma as an example and ignore the logic to determine whether I want to use beta or gamma;

function makeCalculation(e){
    var value;

    if (Math.round(e.gamma) === 0) {
        value = 0;
    } else if (e.gamma < -2) {
        value = // Some Calculation
    } else if (e.gamma > 2) {
        value = // Some Other Calculation
    } else {
        value = // Yet a different calculation
    }

    return value;
}

Is there a better way to make number comparisons of this nature without having to creating a nasty looking if/else if block?

Given your conditions, there's no way around the if-else statements. You have to enforce the conditions somehow.

Instead of if-else, another option is a switch statement, but the outcome is essentially the same:

switch (true) {
    case Math.round(e.gamma) === 0:
        value = 0;
        break;
    case e.gamma < -2:
        value = -2;
        break;
    case e.gamma > 2:
        value = 2;
        break;
    default:
        value = 99;
}


strategy pattern, good practices on how to switch between classes i'm using an external API that posts products and orders into a marketplace, and it provides a class that in each request i have to specify an endpoint, user and password (token), it's something like this:

if($env == 'testing') {
    $endpoint = 'testing';
    $user = 'testing';
    $pass = 'testing';
} else {
    $endpoint = 'production';
    $user = 'production';
    $pass = 'production';
}

$itemSender = new ItemSender($endpoint, $user, $pass);
$orderSender = new OrderSender($endpoint, $user, $pass);

my first thought was to use the strategy pattern so i can have both testing and production environment, or even others environments, implementing an interface, i built something lie this:

interface EnviromentInterface () {

    function getEndpoint();
    function getUser();
    function getPassword();
}

class TestingEnvironment implements EnviromentInterface {

    private $endpoint;
    private $user;
    private $password;

    function getEndpoint() {
        return $this->endpoint;
        // return getFromConfigFile('API_ENDPOINT_TESTING');
    }

    function getUser() {
        return $this->user;
    }

    function getPassword() {
        return $this->password;
    }

}

class ProductionEnviroment implements EnviromentInterface {

    private $endpoint;
    private $user;
    private $password;

    function getEndpoint() {
        return $this->endpoint;
        // return getFromConfigFile('API_ENDPOINT_PRODUCTION');
    }

    function getUser() {
        return $this->user;
    }

    function getPassword() {
        return $this->password;
    }

}

and i would overwrite the parent's class construct to use the environment object, something like this:

class CustomItemSender extends ItemSender
{

    function __construct(EnviromentInterface $env)
    {
        parent::__construct($env->getEndpoint(), $env->getUser(), $env->getPassword());
    }
}

the problem is: how can i switch between these environment objects? i thought about using a switch/case, but if i need to implement more environments i have to add another if to the switch/case statment.

what would be a good aproach in this case?

important: all info/credentials about the testing and production environments are inside a config file, that in this case is an .env file

You are probably looking for factory method pattern, instead of strategy pattern.

The CustomItemSender class is a good indicator of this.
Whenever you extend a class to only override the constructor, you probably need a factory instead. It means you need somewhat more complex way of constructing the object and that implies creational pattern. Strategy is a behavioral pattern (and in fact, your code does not implement this pattern).

The EnvironmentInterface is basicaly a data structure, it is immutable which is good, but it has no behaviour, which is not so good. It is just a tuple. Maybe you don't really need this structure and stay with tripplet of arguments.

Polymorphism only makes sense to abstract varying behaviour, not data. Anyway polymorphism won't help you avoid ifs/switches entirely. It will help you reduce the amount of them to minimum. You might still need some ifs in the creational part, but once a polymorhic object is created, it no longer repeats those ifs because it encapsulates the beforehand chosen branch.

Sometimes ifs/switches can be replaced by key-value maps though (such a map could be owned by a factory...).

Getting config values from a file using global function like yours getFromConfigFile(string $key) is not very wise btw. Because it either loads the file again on every request for a key. Or it loads it only once but stores the loaded data to a global variable thus having side effects on globle state which is a bad practice. You might want to load all config from the file to lets say array (possibly encapsulated in an objet) and serve the individual config values from it. And btw the function knows the location of the config file globally/magically and that is also not very good. You might need a factory for the config object, which will load the config from a given a file.

function loadConfigFromJsonFile(string $filename): array
{
  // add error handling
  return json_decode(file_get_contents($filename), true);
}

class SenderFactory
{

  private string $endpoint;
  private string $user;
  private string $password;

  public function __construct(string $endpoint, string $user, string $password)
  {
    $this->endpoint = $endpoint;
    $this->user = $user;
    $this->password = $password;
  }

  public function createOrderSender(): OrderSender
  {
     return new OrderSender($this->endpoint, $this->user, $this->password);
  }

  public function createItemSender(): ItemSender
  {
     return new ItemSender($this->endpoint, $this->user, $this->password);
  }
}

function createSenderFactory(string $env, array $config): SenderFactory
{
  // the if has to go somewhere anyway
  // add some error handling too (missing config keys, unknown $env, etc.)
  if ($env === 'testing') {
    return new SenderFactory($config['API_ENDPOINT_TESTING'], 'testing', 'testing');
  } else {
    return new SenderFactory($config['API_ENDPOINT_PRODUCTION'], 'production', 'production');
  }
}

// ...

$config = loadConfigFromJsonFile($configPath);
$factory = createSenderFactory($env, $config);
$orderSender = $factory->createOrderSender();
$itemSender = $factory->createItemSender();
```