Python 2 binary search I wanted to take a stab at implementing a binary search, and here's what I came up with. Is there anything I could have done better, from  a code clarity perspective, or code optimization perspective? Any and all criticism would be appreciated:

import random
# implement a binary search

def binary_search(alist, item):
    first = 0
    last = len(alist)-1
    found = False
    midpoint = None
    while True:
        midpoint = first + ((last-first) // 2)
        if alist[midpoint] == item:
            return midpoint
        elif alist[midpoint] < item:
            first = midpoint
        elif alist[midpoint] > item:
            last = midpoint

def _create_search_criteria(cap=10000):
    # choose a target to search for
    choice = random.randrange(0, cap/2)
    # create some data full of random numbers
    data = [random.randrange(0, cap/2) for i in xrange(cap)]
    # ensure the choice is nowhere in the data
    data = [d for d in data if d != choice]
    # put a single of instance of choice back into the data
    data[random.randrange(0, cap)] = choice
    # sort the data
    data = sorted(data)
    return data, choice

def test_binary_search():
    data, choice = _create_search_criteria()
    print 'Searching for: ' + str(choice)
    index =  binary_search(data, choice)
    print 'Found it at index: ' + str(index)
    assert data[index] == choice

for x in xrange(1000):
    test_binary_search()

See the source of Python's bisect module in the standard library to see the accepted implementations used for binary searches and binary insertions used in the language:

"""Bisection algorithms."""

def insort_right(a, x, lo=0, hi=None):
    """Insert item x in list a, and keep it sorted assuming a is sorted.

    If x is already in a, insert it to the right of the rightmost x.

    Optional args lo (default 0) and hi (default len(a)) bound the
    slice of a to be searched.
    """

    if lo < 0:
        raise ValueError('lo must be non-negative')
    if hi is None:
        hi = len(a)
    while lo < hi:
        mid = (lo+hi)//2
        if x < a[mid]: hi = mid
        else: lo = mid+1
    a.insert(lo, x)

insort = insort_right   # backward compatibility

def bisect_right(a, x, lo=0, hi=None):
    """Return the index where to insert item x in list a, assuming a is sorted.

    The return value i is such that all e in a[:i] have e <= x, and all e in
    a[i:] have e > x.  So if x already appears in the list, a.insert(x) will
    insert just after the rightmost x already there.

    Optional args lo (default 0) and hi (default len(a)) bound the
    slice of a to be searched.
    """

    if lo < 0:
        raise ValueError('lo must be non-negative')
    if hi is None:
        hi = len(a)
    while lo < hi:
        mid = (lo+hi)//2
        if x < a[mid]: hi = mid
        else: lo = mid+1
    return lo

bisect = bisect_right   # backward compatibility

def insort_left(a, x, lo=0, hi=None):
    """Insert item x in list a, and keep it sorted assuming a is sorted.

    If x is already in a, insert it to the left of the leftmost x.

    Optional args lo (default 0) and hi (default len(a)) bound the
    slice of a to be searched.
    """

    if lo < 0:
        raise ValueError('lo must be non-negative')
    if hi is None:
        hi = len(a)
    while lo < hi:
        mid = (lo+hi)//2
        if a[mid] < x: lo = mid+1
        else: hi = mid
    a.insert(lo, x)

def bisect_left(a, x, lo=0, hi=None):
    """Return the index where to insert item x in list a, assuming a is sorted.

    The return value i is such that all e in a[:i] have e < x, and all e in
    a[i:] have e >= x.  So if x already appears in the list, a.insert(x) will
    insert just before the leftmost x already there.

    Optional args lo (default 0) and hi (default len(a)) bound the
    slice of a to be searched.
    """

    if lo < 0:
        raise ValueError('lo must be non-negative')
    if hi is None:
        hi = len(a)
    while lo < hi:
        mid = (lo+hi)//2
        if a[mid] < x: lo = mid+1
        else: hi = mid
    return lo

# Overwrite above definitions with a fast C implementation
try:
    from _bisect import *
except ImportError:
    pass

Regarding your code, let us see what improvements can be made:

def binary_search(alist, item):
    first = 0
    last = len(alist)-1
    found = False
    midpoint = None
    while True:
        midpoint = first + ((last-first) // 2)
        if alist[midpoint] == item:
            return midpoint
        elif alist[midpoint] < item:
            first = midpoint
        elif alist[midpoint] > item:
            last = midpoint

The initial values of found and midpoint are never used and so can be removed:

def binary_search(alist, item):
    first = 0
    last = len(alist)-1
    while True:
        midpoint = first + ((last-first) // 2)
        if alist[midpoint] == item:
            return midpoint
        elif alist[midpoint] < item:
            first = midpoint
        elif alist[midpoint] > item:
            last = midpoint

My tests are being performed in Python 3.5, so the code may looks slightly strange. What happens when searching for something that does not exist?

def binary_search(alist, item):
    first = 0
    last = len(alist)-1
    while True:
        midpoint = first + ((last-first) // 2)
        if alist[midpoint] == item:
            return midpoint
        elif alist[midpoint] < item:
            first = midpoint
        elif alist[midpoint] > item:
            last = midpoint

alist = list(range(10, 20))
item = 9
print(binary_search(alist, item))

The function enters an infinite loop if the value is too small. What about when the value is too large?

def binary_search(alist, item):
    first = 0
    last = len(alist)-1
    while True:
        midpoint = first + ((last-first) // 2)
        if alist[midpoint] == item:
            return midpoint
        elif alist[midpoint] < item:
            first = midpoint
        elif alist[midpoint] > item:
            last = midpoint

alist = list(range(10, 20))
item = 10
print(binary_search(alist, item))

The program prints out zero. This is not correct either. Here is a rewrite of your code in hopes to correct the problem:

import random

def main():
    for _ in range(1000):
        array = sorted(random.sample(range(10000), 9000))
        value = random.randrange(10000)
        index = binary_search(array, value)
        if index is None:
            assert value not in array
        else:
            assert array[index] == value

def binary_search(array, value):
    start, stop = 0, len(array)
    while start < stop:
        offset = start + stop >> 1
        sample = array[offset]
        if sample < value:
            start = offset + 1
        elif sample > value:
            stop = offset
        else:
            return offset

if __name__ == '__main__':
    main()


Simplified Bin Packing using JavaScript This code determines the maximum number of boxes (one size) that can fit into the back of a truck, checking all possible orientations to ensure maximum Bin Packing excitement.  This is (hopefully) the final version of this code. Please share any modifications that you would make to this code below.

<!DOCTYPE html>
<html>
<script>

// This program determines the maximum number of boxes that can fit into the back of a truck

// Declaring box size in centimetres, and converting it into metres
var boxHeight = prompt("What is the height of your box in centimetres?") / 100;
var boxWidth = prompt("What is the width of your box in centimetres?") / 100;
var boxLength = prompt("What is the length of your box in centimetres?") / 100;

// Declaring truck size in metres
var truckHeight = prompt("What is the height of your truck in metres?");
var truckWidth = prompt("What is the width of your truck in metres?");
var truckLength = prompt("What is the length of your truck in metres?");

// Declaring variables used in equation
var heightA = boxHeight > 0 ? ((truckHeight / boxHeight) | 0) : 0;
var widthA = boxWidth > 0 ? ((truckWidth / boxWidth) | 0) : 0;
var lengthA = boxLength > 0 ? ((truckLength / boxLength) | 0) : 0;

var heightB = boxHeight > 0 ? ((truckHeight / boxHeight) | 0) : 0;
var widthB = boxWidth > 0 ? ((truckLength / boxWidth) | 0) : 0;
var lengthB = boxLength > 0 ? ((truckWidth / boxLength) | 0) : 0;

var heightC = boxHeight > 0 ? ((truckWidth / boxHeight) | 0) : 0;
var widthC = boxWidth > 0 ? ((truckHeight / boxWidth) | 0) : 0;
var lengthC = boxLength > 0 ? ((truckLength / boxLength) | 0) : 0;

var heightD = boxHeight > 0 ? ((truckWidth / boxHeight) | 0) : 0;
var widthD = boxWidth > 0 ? ((truckLength / boxWidth) | 0) : 0;
var lengthD = boxLength > 0 ? ((truckHeight / boxLength) | 0) : 0;

var heightE = boxHeight > 0 ? ((truckLength / boxHeight) | 0) : 0;
var widthE = boxWidth > 0 ? ((truckWidth / boxWidth) | 0) : 0;
var lengthE = boxLength > 0 ? ((truckHeight / boxLength) | 0) : 0;

var heightF = boxHeight > 0 ? ((truckLength / boxHeight) | 0) : 0;
var widthF = boxWidth > 0 ? ((truckHeight / boxWidth) | 0) : 0;
var lengthF = boxLength > 0 ? ((truckWidth / boxLength) | 0) : 0;

var multiplesA = heightA * widthA * lengthA;
var multiplesB = heightB * widthB * lengthB;
var multiplesC = heightC * widthC * lengthC;
var multiplesD = heightD * widthD * lengthD;
var multiplesE = heightE * widthE * lengthE;
var multiplesF = heightF * widthF * lengthF;

var arrayA = [multiplesB, multiplesC, multiplesD, multiplesE, multiplesF];
var arrayB = [multiplesA, multiplesC, multiplesD, multiplesE, multiplesF];
var arrayC = [multiplesB, multiplesA, multiplesD, multiplesE, multiplesF];
var arrayD = [multiplesB, multiplesC, multiplesA, multiplesE, multiplesF];
var arrayE = [multiplesB, multiplesC, multiplesD, multiplesA, multiplesF];
var arrayF = [multiplesB, multiplesC, multiplesD, multiplesE, multiplesA];

// Calculating the most efficient orientation of the boxes
var boxCount;

if (multiplesA > arrayA)
{
boxCount = multiplesA;
}

else if (multiplesB > arrayB)
{
boxCount = multiplesB;
}

else if (multiplesC > arrayC)
{
boxCount = multiplesC;
}

else if (multiplesD > arrayD)
{
boxCount = multiplesD;
}

else if (multiplesE > arrayE)
{
boxCount = multiplesE;
}

else
{
boxCount = multiplesF;
}

// Output answer
alert("You can fit up to " + boxCount + (1 == boxCount ? " box" : " boxes") + " into the truck");

</script>
</html>

I think a big first improvement for you will be to start DRYing out your code. You should create a function which returns the ratio of two sides -- there is much too much repetition in your code. Something like:

function get_ratio(x, y) {
    if (y === 0) {
        return 0
    }
    return x/y | 0
}


Swift Hackerrank Dynamic Array I've solved this question. Wasn't sure if I was suppose to use a dictionary in this case, as the question was to use a dynamic array, but I didnt know how else to do it. Was wondering if I can get feedback on my code or if there's a better way to approach it:

import Foundation

// reads input for N(number of sequences) and Q(number of queries)
let input = readLine()!.componentsSeparatedByString(" ").map { Int($0)! }
let numOfSequences = input[0]
let numOfQueries = input[1]
var lastAns = 0
var seqDictionary = [Int: [Int]]()

// loop through numOfQueries
for _ in 0..<numOfQueries {
    // each query is given in format [1, x, y] or [2, x, y]
    let query = readLine()!.componentsSeparatedByString(" ").map { Int($0)! }
    // check to see if query[0] is 1 or 2
    switch query[0] {
    case 1:
        // formula for seqDictionaryIndex
        // ^ is the XOR operator
        let seqDictionaryIndex = ((query[1] ^ lastAns) % numOfSequences)
        // since we initalized an empty dictionary, we gotta set the first key to a value to start the array so we can start appending
        guard seqDictionary[seqDictionaryIndex] != nil else { seqDictionary[seqDictionaryIndex] = [query[2]] ; continue }
        seqDictionary[seqDictionaryIndex]?.append(query[2])
    case 2:
        let seqDictionaryIndex = ((query[1] ^ lastAns) % numOfSequences)
        // calculate the size of the particular sequence in seqDictionary
        let size = seqDictionary[seqDictionaryIndex]?.count
        // formula for finding index in particular sequence in seqDictionary
        let index = query[2] % size!
        // set last answer to that element at the index calculated above
        lastAns = seqDictionary[seqDictionaryIndex]![index]
        print(lastAns)
    default: break
    }
}

The task is about dynamic arrays, and using an array (instead of a
dictionary, as you did) simplifies things considerably.

If you start with an array of numOfSequences empty arrays:

var seqList = [[Int]](count: numOfSequences, repeatedValue: [])

then the 1 x y query simplifies to

let seqIndex = ((x ^ lastAns) % numOfSequences)
seqList[seqIndex].append(y)

without the need to check for an empty dictionary value.

Some more suggestions:

Reading a list of integers occurs at two places in the program, that
justifies a separate function:

func readIntegers() -> [Int] {
    return readLine()!.componentsSeparatedByString(" ").map { Int($0)! }
}

(Remark: Usually, forced unwrapping with ! should be avoided,
but here we have a programming challenge with well-defined input data.)

To increase the legibility of the code, I would assign the three
parameters of a query to variables type, x, y.
The code then becomes almost self-explaining.

The entire code then looks like this:

func readIntegers() -> [Int] {
    return readLine()!.componentsSeparatedByString(" ").map { Int($0)! }
}

// Read  N(number of sequences) and Q(number of queries):
let input = readIntegers()
let (numOfSequences, numOfQueries) = (input[0], input[1])

var lastAns = 0
// Array of `numOfSequences` empty arrays:
var seqList = [[Int]](count: numOfSequences, repeatedValue: [])

for _ in 0..<numOfQueries {
    let input = readIntegers()
    let (type, x, y) = (input[0], input[1], input[2])

    switch type {
    case 1:
        let seqIndex = ((x ^ lastAns) % numOfSequences)
        seqList[seqIndex].append(y)
    case 2:
        let seqIndex = ((x ^ lastAns) % numOfSequences)
        let index = y % seqList[seqIndex].count
        lastAns = seqList[seqIndex][index]
        print(lastAns)
    default:
        fatalError("Unexpected query")
    }
}


Splitting code into separate methods and avoiding the Unit Of Work / Repository pattern Take a look a this service method:

public static void CreateSocialEvent(int employeeId)
{
    using (var dbContext = new EmployeeEntities())
    {
        using (var transaction = dbContext.Database.BeginTransaction())
        {
            try
            {
                // I would prefer the next two statements to be in a separate method :
                var employee = dbContext.Employees.SingleOrDefault(e => e.IsActive
                    && e.IsSocialEmployee);
                dbContext.SaveChanges();

                // I would prefer the next three statements to be in a separate method :
                var reminder = new Reminder
                    {
                        EmployeeId = employee.EmployeeId,
                        CreationDate = DateTime.Now
                    }
                dbContext.Reminders.Add(reminder);
                dbContext.SaveChanges();

                // I would prefer the next three statements to be in a separate method :
                var socialMailingListSubscriber = new socialMailingListSubscriber
                    {
                        EmployeeId = employee.employeeId
                    }
                dbContext.SocialMailingListSubscribers.Add(socialMailingListSubscriber);
                dbContext.SaveChanges();

                // I would prefer the next three statements to be in a separate method :
                var subscription = new Subscription
                    {
                        subscription.EmployeeId = employee.Id
                    }
                dbContext.Subscriptions.Add(subscription);
                dbContext.SaveChanges();

                // I would prefer the next three statements to be in a separate method :
                var department = dbContext.Department.Find(employee.DepartmentId)
                    .Include(d => d.Subscribers);
                employee.Subscribers = department.Subscribers;
                dbContext.SaveChanges();

                transaction.Commit();
            }
            catch (Exception)
            {
                transaction.Rollback();
                throw;
            }
        }
    }
}

As you saw from my comments, I would prefer to segregate each operation in it's own method, to respect the single responsibility principle, as I don't like having very long methods that do many things.

Say I create a Reminder service that handles Reminders and a SocialMailingList service that handles MailingLists, I would have to inject either the dbContext or the transaction into these other services or into the other methods.

If I choose to inject the transaction, it won't work because if I open a new dbContext there, I cannot assign the injected transaction : it has to come from the same dbContext.

If I inject the dbContext entirely, I will have to write more code just to handle the dbContext lifetime. So, I'm close to implementing the Unit Of Work / Repository Pattern, and that's what I want to avoid at all costs.

If I understand correctly, DbContext in itself is a Unit Of Work, and the DbSet collections act as Repositories (am I right?). I don't like abstracting out something that has already been abstracted for me.

In other words, what should I do?

Keep that long method there so I can keep the Entity-Framework code simple?
Implement the Unit Of Work / Repository pattern so I can control the lifetime of my transaction?

I've never implemented the Unit Of Work / Repository Pattern so far. I've read a lot on that, though, and the amount of code to write to implement it just scares me!

I don't need to mock anything, I don't need to support any other rdbms than MS SQL Server, I don't even unit test anything in this project.

If it's better to implement it, can you suggest the simplest way to do so?

What is the main purpose of the transaction? Is it to handle exceptions and then rollback? Or is there actually something to lock the database/table for everyone for during the creation of social media events. Can be that this is due to the demo code, but I see no need for the transaction itself.

If you remove the transaction, and just do SaveChanges once at the end, then you don't need the transaction. Which solves part of your problem I guess.

    public static void CreateSocialEvent(int employeeId)
    {
        using (var dbContext = new EmployeeEntities())
        {
            var employee = dbContext.Employees.FirstOrDefault(e => e.IsActive && e.IsSocialEmployee && e.EmployeeId == employeeId);
            if (employee == null)
            {
                throw new ArgumentException(string.Format("Unable to find employee by '{0}'", employeeId));
            }

            dbContext.Reminders.Add(new Reminder
            {
                EmployeeId = employee.EmployeeId,
                CreationDate = DateTime.Now
            });

            dbContext.SocialMailingListSubscribers.Add(new SocialMailingListSubscriber
            {
                EmployeeId = employee.EmployeeId
            });

            dbContext.Subscriptions.Add(new Subscription
            {
                EmployeeId = employee.EmployeeId
            });

            var department = dbContext.Departments
                .Include(d => d.Subscribers)
                .FirstOrDefault(d => d.DepartmentId == employee.DepartmentId);
            if (department == null)
            {
                throw new NullReferenceException("Departments could not be found");
            }
            employee.Subscribers = department.Subscribers;

            // Persist to database
            // if we fail to get here due to an exception,
            // the using statement will dispose and the exception will be thrown
            dbContext.SaveChanges();
        }
    }

And you could move some of the creation of object parts, like in the simplest way:

public class Reminder
{
    public int EmployeeId { get; set; }
    public DateTime CreationDate { get; set; }

    public static Reminder CreateForEmployee(int employeeId)
    {
        return new Reminder
        {
            EmployeeId = employeeId,
            CreationDate = DateTime.Now
        };
    }

    public Reminder() { }

    public Reminder(int employeeId)
    {
        this.EmployeeId = employeeId;
        this.CreationDate = DateTime.Now;
    }
}

So you could do:

    public static void CreateSocialEvent(int employeeId)
    {
        using (var dbContext = new EmployeeEntities())
        {
            var employee = dbContext.Employees.FirstOrDefault(e => e.IsActive && e.IsSocialEmployee && e.EmployeeId == employeeId);

            if (employee == null)
            {
                throw new ArgumentException(string.Format("Unable to find employee by '{0}'", employeeId));
            }

            // with a static method
            dbContext.Reminders.Add(Reminder.CreateForEmployee(employeeId));

            // or with the constructor
            dbContext.Reminders.Add(new Reminder(employeeId));
        }
    }

To make it fit on one line, if that is what you want. Or use DI to move the responsibility of creating the object out of the method.


Base 26 (letters) and Base 10 using recursion I was messing around with base conversion between decimal and Base26 (alphabet) with no 0th case because I needed it for the Google Sheets API in order to find specific cell values from a 3 dimensional list of all of the entered cell values on a spreadsheet. Remember that with spreadsheets, the columns are counted with no 0th case and A == 1 Z == 26 BA == 53.

Some test cases for conversion between the two bases would be

"zyz" == 18252, "aa" == 27, "zz" == 702

When I went to go write the code, I looked for simple solutions, but could not find anything using recursion so I wrote this in Python:

def base10ToBase26Letter(num):
    ''' Converts any positive integer to Base26(letters only) with no 0th
    case. Useful for applications such as spreadsheet columns to determine which
    Letterset goes with a positive integer.
    '''
    if num <= 0:
        return ""
    elif num <= 26:
        return chr(96+num)
    else:
        return base10ToBase26Letter(int((num-1)/26))+chr(97+(num-1)%26)

def base26LetterToBase10(string):
    ''' Converts a string from Base26(letters only) with no 0th case to a positive
    integer. Useful for figuring out column numbers from letters so that they can
    be called from a list.
    '''
    string = string.lower()
    if string == " " or len(string) == 0:
        return 0
    if len(string) == 1:
        return ord(string)-96
    else:
        return base26LetterToBase10(string[1:])+(26**(len(string)-1))*(ord(string[0])-96)

I am aware that the first function outputs lowercase letters which can be bypassed by using base10ToBase26Letter(num).upper().

I used this to test the consistency of the values being output:

for x in range(0,100000):
    if x == base26LetterToBase10(base10ToBase26Letter(x)):
        print(x)

Is recursion the best way to approach this problem efficiently, or should I be looking for something else?
If I were to look at readability, is this way the easiest to read?
Does this problem have a shorter solution?
It seems that after about 11 letter digits, this program is no longer accurate, is there a way to make the functions accurate further? (Even if such numbers will never be used in a spreadsheet)

As others already said, recursion doesn't quite fit here. But I’d say it doesn't fit because strings are immutable. In your implementation, you are either concatenating strings or slicing them, which are operations that will produce new strings each time where it shouldn't be necessary.

Instead, iteration is more natural in Python, and you have several builtins that can help you so. The first ones being reversed and enumerate that give you the ability to iterate over each letter in a string in reverse order while knowing the index of the letter: usefull when converting from base alphabet to base 10. And for the base 10 to alphabet conversion, you can extract individual "digits" using divmod and join the generated letters.

Lastly, I’d recommend to stick to the lower_case_with_underscore naming convention for functions as recommended per PEP8.

A_UPPERCASE = ord('A')
ALPHABET_SIZE = 26

def _decompose(number):
    """Generate digits from `number` in base alphabet, least significants
    bits first.

    Since A is 1 rather than 0 in base alphabet, we are dealing with
    `number - 1` at each iteration to be able to extract the proper digits.
    """

    while number:
        number, remainder = divmod(number - 1, ALPHABET_SIZE)
        yield remainder

def base_10_to_alphabet(number):
    """Convert a decimal number to its base alphabet representation"""

    return ''.join(
            chr(A_UPPERCASE + part)
            for part in _decompose(number)
    )[::-1]

def base_alphabet_to_10(letters):
    """Convert an alphabet number to its decimal representation"""

    return sum(
            (ord(letter) - A_UPPERCASE + 1) * ALPHABET_SIZE**i
            for i, letter in enumerate(reversed(letters.upper()))
    )

But, you could use recursion in _decompose in order to not have to reverse the string produced by join:

A_UPPERCASE = ord('A')
ALPHABET_SIZE = 26

def _decompose(number):
    """Generate digits from `number` in base alphabet, most significants
    bits first.
    """

    number -= 1  # Account for A in base alphabet being 1 in decimal rather than 0
    if number < ALPHABET_SIZE:
        yield number
    else:
        number, remainder = divmod(number, ALPHABET_SIZE)
        yield from _decompose(number)
        yield remainder

def base_10_to_alphabet(number):
    """Convert a decimal number to its base alphabet representation"""

    return ''.join(
            chr(A_UPPERCASE + part)
            for part in _decompose(number)
    )

def base_alphabet_to_10(letters):
    """Convert an alphabet number to its decimal representation"""

    return sum(
            (ord(letter) - A_UPPERCASE + 1) * ALPHABET_SIZE**i
            for i, letter in enumerate(reversed(letters.upper()))
    )


Rock, paper, scissors This is something I wrote as a reply to a deleted "Rock/paper/scissors [updated]" question. Since I can't post it as an answer (and the line-by-line analysis is gone as well), it might be nice to get a code review.

"""A game of rock/paper/scissors."""

import random

SIGNS = ("rock", "paper", "scissors")
VERBS = ("crushes", "covers", "cut")
YES = ("yes", "y")

class Counter(object):
    """Keep track of game session scores."""

    def __init__(self):
        self.rounds = 0
        self.losses = 0
        self.wins = 0
        self.draws = 0
        self.rounds_total = 0
        self.losses_total = 0
        self.wins_total = 0
        self.draws_total = 0

    def increment(self, state):
        """Increment losses, wins, draws.

        Args:
            state: "losses", "wins", or "draws".

        Raises:
            ValueError: If input is invalid.
        """
        if state not in ["losses", "wins", "draws"]:
            raise ValueError("Acceptable input: losses, wins, or draws.")
        for attr in (state, "%s_total" % state, "rounds", "rounds_total"):
            setattr(self, attr, getattr(self, attr) + 1)

    def flush(self):
        """Restart the current game counters, keep the total counters."""
        self.rounds = 0
        self.losses = 0
        self.wins = 0
        self.draws = 0

    def print_results(self):
        print "Games played: ", self.rounds
        print "Games won: ", self.wins
        print "Games lost: ", self.losses
        print "Games drawn: ", self.draws
        print "Total played: ", self.rounds_total
        print "Total won: ", self.wins_total
        print "Total lost: ", self.losses_total
        print "Total drawn: ", self.draws_total

class Sign(object):
    """A rock/paper/scissors sign."""

    def __init__(self, sign):
        global SIGNS

        if sign not in SIGNS:
            raise ValueError("Allowed signs are: %s." % SIGNS.join(", "))
        self.sign = sign

    def __eq__(self, other):
        try:
            if self.sign == other.sign:
                return True
        except AttributeError:
            pass
        return False

    def __gt__(self, other):
        if (self.sign == "rock" and other.sign == "scissors" or
            self.sign == "scissors" and other.sign == "paper" or
            self.sign == "paper" and other.sign == "rock"):
            return True
        return False

    def __lt__(self, other):
        return not self.__gt__(other)

def main(max_rounds=5):
    global SIGNS
    global VERBS
    global YES

    counter = Counter()
    while True:
        if counter.rounds == max_rounds:
            counter.print_results()
            again = raw_input("Play/again? (Y/n) ").lower()
            if again in YES:
                counter.flush()
            else:
                print "Bye!"
                break
        while True:
            player_sign = raw_input("Rock/paper/scissors? ").lower()
            if player_sign not in SIGNS:
                print "Only following words are valid: %s." % SIGNS.join(" ,")
                continue
            break
        player = Sign(player_sign)
        computer = Sign(random.choice(SIGNS))
        if player > computer:
            counter.increment("wins")
            verb = VERBS[SIGNS.index(player.sign)]
            print "Player wins: %s %s %s." % (player.sign, verb, computer.sign)
        elif player == computer:
            counter.increment("draws")
            print "It's a draw! Both played %s." % player.sign
        else:
            counter.increment("losses")
            verb = VERBS[SIGNS.index(computer.sign)]
            print "Player lost: %s %s %s." % (computer.sign, verb, player.sign)

if __name__ == "__main__":
    main(max_rounds=5)

So this all looks pretty good, with not much to review. I didn't see the previous question, so I will throw my two cents in here.

You define the following at the top of your file:

SIGNS = ("rock", "paper", "scissors")
VERBS = ("crushes", "covers", "cut")
YES = ("yes", "y")

which is fine. However in all the classes where you use these variable you denote them with the global keyword. This isn't code breaking, but it is unnecessary. The global key word is used for global variables. What you are using is global constants that are defined at module scope, so you can get rid of those redundant lines.

It was suggested in the comments that you replace the print_results function by __str__, but I would actually suggest replacing it by __repr__. In cases where __str__ is not defined __repr__==__str__ anyways. So:

def __repr__(self):
    return "\n".join(
        ["Games played: %d"%self.rounds,
         "Games won: %d"%self.wins,
         "Games lost: %d"%self.losses,
         "Games drawn: %d"%self.draws,
         "Total played: %d"%self.rounds_total,
         "Total won: %d"%self.wins_total,
         "Total lost: %d"%self.losses_total,
         "Total drawn: %d"%self.draws_total])

This will allow you to print the object nicely with print counter.

These are all ultra minor things now:

The block

def __eq__(self, other):
    try:
        if self.sign == other.sign:
            return True
    except AttributeError:
        pass
    return False

has a redundant pass and redundant branch. This could be condensed to:

def __eq__(self, other):
    try:
        return (self.sign == other.sign)
    except AttributeError:
        return False

In a similar vain, there is no need to branch in __gt__ method:

def __gt__(self, other):
    return (self.sign == "rock" and other.sign == "scissors" or
            self.sign == "scissors" and other.sign == "paper" or
            self.sign == "paper" and other.sign == "rock")

and the __lt__ method:

def __lt__(self, other):
    return not self.__gt__(other)

is both redundant and incorrect (as pointed out in the comments, this is actually a less than or equal to that is written here, as any value that is not greater than will return true) and can be removed (the default version calls __gt__ and should handle your problem fine).

Finally for clarity I would define VERBS as a dictionary. It is more code, but it is also more readable:

VERBS = {"rock":"crushes", "paper":"covers", "scissors":"cut"}

this allows you to do the following in the main function:

verb = VERBS[player.sign]

and

verb = VERBS[computer.sign]


Module with helper functions to initialize or reinitialize a Slick carousel I been getting into javascript patterns and would love to get some feedback. I notice that I got some general helper functions. Is this a good approach of dealing with it?

// carousel.js

var Carousel = (function() {
  var timeout = 340;

  function init() {
    applyForTheseQueries();

    $(window).on('resize', Helpers.debounce(function() {
      applyForTheseQueries();
    }, timeout));
  }

  function applyForTheseQueries() {
    var $carousel = $('.js-slick');
    var query = $carousel.data('query');
    var mediaQuery = Helpers.executeFunctionByName(query, window);

    if (mediaQuery && !$carousel.hasClass('init-done')) {
      $carousel.addClass('init-done').slick();
    } else if (!mediaQuery && $carousel.hasClass('init-done')) {
      $carousel.removeClass('init-done').slick('unslick');
    }
  }

  return {
    init: init
  };
})();

$(function() {
  Carousel.init();
});

// helpers.js

var Helpers = (function() {
  function executeFunctionByName(functionName, context) {
    var args = [].slice.call(arguments).splice(2);
    var namespaces = functionName.split(".");
    var func = namespaces.pop();

    for (var i = 0; i < namespaces.length; i++) {
      context = context[namespaces[i]];
    }

    return context[func].apply(context, args);
  }

  function debounce(func, wait, immediate) {
    var timeout;
    return function() {
      var context = this;
      var args = arguments;

      var later = function() {
        timeout = null;
        if (!immediate) func.apply(context, args);
      };

      var callNow = immediate && !timeout;
      clearTimeout(timeout);
      timeout = setTimeout(later, wait);

      if (callNow) func.apply(context, args);
    };
  };

  return {
    executeFunctionByName: executeFunctionByName,
    debounce: debounce
  };
})();

This approach seems fine. I did notice the following loop in Helpers. executeFunctionByName():

for (var i = 0; i < namespaces.length; i++) {
  context = context[namespaces[i]];
}
return context[func].apply(context, args);

Doesn't that just set context equal to the last element in namespaces? Why not just use a simple assignment without a loop?

The value for the timeout

 var timeout = 340;

Could be declared with all capitals as is idiomatic in JavaScript and many C-based languages, as well as a more descriptive name:

 const RESIZE_TIMEOUT = 340;

There are a few things that could be simplified.

In Carousel.init the resize callback could be simplified from

$(window).on('resize', Helpers.debounce(function() {
  applyForTheseQueries();
}, timeout));

To this:

$(window).on('resize', Helpers.debounce(applyForTheseQueries, timeout));

And if ecmascript-6 syntax is supported, then object initializer shorthand notation can be used - instead of :

return {
  init: init
}

just use:

return {
  init
}

And instead of

return {
  executeFunctionByName: executeFunctionByName,
  debounce: debounce
};

Simply to

return {
  executeFunctionByName,
  debounce
};

Another ecmascript-6 feature that could be used is the for...of loop. Instead of

for (var i = 0; i < namespaces.length; i++) {
  context = context[namespaces[i]];
}

Simplify this to

for (const namespace of namespaces) {
  context = context[namespace];
}


Creating a percentage complete method for a form I am a relatively new Rails developer and doctor doing research. In my Rails based research projects I often have questionnaires for which I provide a "% complete" parameter.

Here is an example.  You can see that each of the score attributes is given a value and then I can create a %. It looks awful and very un-Ruby-like. I would be keen once and for all to get an experts view and give me some steerage on how this could be cleaned up.

This method is in the Patient class:

 def percent_complete(user, patient)
   score = Score.where(user_id: user.id, patient_id: self.id).first
  if score.nil?
    return "0"
  end
  @status = 0
   score_array = [score.dx1, score.dxcon1, score.db1, score.dbcon1, score.biopsy, score.mgt]
   score_array.each do | element|
    if element == score.dx1
      @status = @status + 20
    elsif element == score.db1
      @status = @status + 20
    elsif element == score.dxcon1
      @status = @status + 20
    elsif element == score.dbcon1
      @status = @status + 20
    elsif element == score.biopsy
      @status = @status + 10
    elsif element == score.mgt
      @status = @status + 10
    end
   end
  @status
 end

Each Score belongs to a Patient (the Patient has many scores). Each Score belongs to a User (a User has many scores).

  score.dx1, score.dxcon1, score.db1, score.dbcon1, score.biopsy, score.mgt

These are the Score class attributes:

dx1 = diagnosis1 (string)
dxcon1 = diagnostic confidence1 (integer)
db1 = disease behaviour1 (string)
dbcon1 = confidence on disease behaviour (integer)
biopsy = should you biopsy? (boolean)
mgt = management (string)

I am now trying something like this:

 def percent_complete(user, patient)
    score = Score.where(user_id: user.id, patient_id: self.id).first
    if score.nil?
      return "0"
    end
    score_completeness = [score.dx1, score.dxcon1, score.db1, score.dbcon1, score.biopsy, score.mgt]
    @status = 0
    score_completeness.each do | field |
       @status += (100/6.0) unless field.blank?
    end
   @status
 end

Looking only at your last block:

Why return the string "0" instead of the number?
The hard coded array is cryptic, and a potential maintenance hassle. I'd recommend moving it to a method on the Score model to abstract things.
The hardcoded 100/6.0 is basically a magic number. It'd be better to get the length of whatever array you're working with than assume it's always 6.
Why is @status an instance variable? Your method is meant to return something. If it returns at the first return line, you never even set @status to anything, which'll lead to errors later on, if you expect it to be set. Conversely, if you don't expect it to be set, or don't intend to use it, why set it?

I imagine you could do something like this:

def percent_complete(user, patient)
  score = Score.where(user_id: user.id, patient_id: self.id).first
  return 0 if score.nil? || score.steps.empty?
  100.0 * score.completed_steps.length / score.steps.length
end

where Score#steps and Score#completed_steps look like:

def steps
  [dx1, dxcon1, db1, dbcon1, biopsy, mgt]
end

def completed_steps
  steps.reject(&:blank)
end


Extending window.location give access to query string elements I am attempting to add a property to access the query string elements on the window and am looking for feedback on this implementation.

if (!window.location.hasOwnProperty("query"))
  Object.defineProperty(window.location, "query", {
      get(){
        var query = document.location.search.substr(1);
        if (query === undefined || query === "")
          return {};

        var pairs = query.split("&");

        var mapped = pairs.map(function(v) {
          var temp = v.split("=");
          return { key: temp[0], val: temp[1] };
        });

        var obj = mapped.reduce(function(total, current){
            total[current.key] = decodeURIComponent(current.val)
                                         .replace(/\+/g, " ");
            return total;
        }, {});

        return obj;
      }
  });

Then I could us it like this

var myVal = window.location.query.customerId

For the most part this looks good to me - your code is easily understood and well formatted. However, there is still room for improvement.

Don't bother creating mapped, that logic could easily be integrated into the reduce function and avoids an extra loop over the array.

Don't assign to an object just to return it, instead of doing var obj = ... return obj just do return pairs.reduce(...

Parsing URLs is complicated. It's really easy to forget to encode / decode something when working with them. In this case, your code correctly decodes the query values, but fails to decode the query key - if my query is ?q%26=search, I would have to use document.location.query['q%26'] instead of the expected document.location.query['q&']. Is this likely to happen? Maybe not, but it should be handled.
What I mean to say is - don't reinvent the wheel. There is a build in URLSearchParams object which does the parsing for you.

What should happen if the URL is ?q=1&q=2? Should only the first value be kept? Only the last? What about ?q[]=1&q[]=2? Should this result in an array as PHP does or keep a single value? Ignoring these cases might be fine for your use case now, but if someone else uses your code, you should consider and document these edge cases.

Here's how I would implement this extension, ignoring the PHPism of converting keys using [] to arrays, and taking the last value if the same key is specified multiple times. I've defined it as a function for ease of testing, but it could just as well be dropped into your get() function.

function getQueryObject(query) {
  const result = {};
  const params = new URLSearchParams(query);
  for (const [key, value] of params) {
    result[key] = value;
  }
  return result;
}

;[
  '?q=1&q=2',
  '?q%26=URLUtils.searchParams&topic=api',
  '?q[]=a',
  '?q=with some spaces'
].forEach(query => {
  console.log(query, '-->', getQueryObject(query))
})


Convert hex encoded string to byte array The following should read a hex string such as "ece998a2fb" into vector 236 233 152 162 251. I was unable to find any built in method for doing this.

as.byte.vector <- function(hex.string) {
  vapply(seq(to=nchar(hex.string), by=2), function(idx) {
    as.numeric(paste("0x", substr(hex.string, idx, idx+1), sep=''))
  }, FUN.VALUE = double(1))
}

One improvement is you can use the vectorized function substring rather than substr. This way, you can precompute all hexadecimal pairs and feed them directly into paste and as.numeric which are also vectorized. This removes the need for vapply (a disguised and slowish for loop).

You could also use paste0 as a shortcut for paste(, sep = ""). paste0 has been around for what seems like a couple years. If you are concerned about compatibility with older versions of R, then yes, it is safer to stick with paste.

I could be wrong but I see no reason not to output a vector of integers instead of numerics. That integers use less memory and are not subject to floating point errors are two strong reasons to prefer them over numerics.

It is also a good idea to add input checks to test your assumptions. Here it seems that the input must be a character vector of length 1.

This comment is more subjective. You have crammed a lot of operations into a single statement. I can count 8 functions including the anonymous one you created. That really makes your code hard to understand.

In all, I would have written the function like this:

as.byte.vector <- function(hex.string) {
   stopifnot(is.character(hex.string), length(hex.string) == 1L)
   start.idx <- seq(from = 1L, to = nchar(hex.string), by = 2L)
   end.idx <- start.idx + 1L
   hex.pairs <- substring(hex.string, start.idx, end.idx)
   prefixed.pairs <- paste0("0x", hex.pairs)
   as.integer(prefixed.pairs)
}

I hope you will agree that it is easier to read when the code is broken out this way, with each statement using a very few functions, and where the names of the variables holding intermediate results help understand the process.

This should already be faster than your version, thanks to the replacement of vapply. If speed is a real concern, you could make significant improvement by replacing substring with the following regular expression trick:

hex.pairs <- strsplit(gsub("(..)", "\\1 ", hex.string), " ")[[1]]

(We insert a space every two characters, then split on white space.)


Helper utilities for easier benchmarking in C I have this tiny library for measuring the execution time in milliseconds and returning the duration plus the result:

execres.h

/******************
* File: execres.h *
******************/

#ifndef EXECRES_H
#define EXECRES_H

#include <sys/time.h>

typedef struct {
    unsigned long long  millis;
    void*               result;
} ExecResult, *PExecResult;

PExecResult execTime(void* (*code)(void*), void* param);

#endif  /* EXECRES_H */

execres.c

/******************
* File: execres.c *
******************/

#include "execres.h"
#include <stdlib.h>

PExecResult execTime(void* (*code)(void*), void* param)
{
    PExecResult result;
    struct timeval tva;
    struct timeval tvb;

    if (!code)
    {
        return NULL;
    }

    result = malloc(sizeof(ExecResult));

    if (!result)
    {
        return NULL;
    }

    gettimeofday(&tva, NULL);
    result->result = code(param);
    gettimeofday(&tvb, NULL);

    result->millis = (unsigned long long)
                     (tvb.tv_sec * 1000 + tvb.tv_usec / 1000 -
                      tva.tv_sec * 1000 - tva.tv_usec / 1000);
    return result;
}

main.c

/***************
* File: main.c *
***************/

#include <stdint.h>
#include <stdio.h>
#include "execres.h"

static uint64_t Fibonacci(uint64_t n)
{
    switch (n)
    {
        case 0:
            return 0;

        case 1:
            return 1;

        default:
            return Fibonacci(n - 1) + Fibonacci(n - 2);
    }
}

int main()
{
    PExecResult result;

    puts("Begin computation.");

    result = execTime((void* (*)(void*)) Fibonacci, (void*) 40);

    printf("Fibonacci took %u milliseconds and returned %llu.\n",
           (unsigned) result->millis,
           (uint64_t) result->result);

    return 0;
}

Nice bit of code!  Here are a few suggestions:

Initialize your variables when you declare them.  This helps shorten the code a bit and reduce possible bugs.
You need documenting comments within your code.  Especially since you are considering this as a library.
I would remove the NULL check on code in your execTime method, and document that a valid function pointer should be passed in.  I would expect the code to break if I tried to time NULL, not NULL to be returned back.  The standard library does this same thing with a few of their functions, like strlen().
Good job checking the return from malloc().
I would use the C11 standard function timespec_get() for your timing code.  It's a C standard (important!) function that has nanosecond precision.
If you want a better Fibonacci algorithm, here is an answer I wrote up a while ago on it.
Good job using puts() in place of printf() when you can.
You don't have to return 0 at the end of main(). The C standard  knows how frequently this is used, and lets us omit it.

  C99 & C11 §5.1.2.2(3)

  ...reaching the } that terminates the main() function returns a
  value of 0.


Calculating revenue from roller coaster rides Problem description:

  The task is to calculate the amount of money earned from roller
  coaster rides on any given day.  There is a queue in front of the
  attraction.  Visitors might be alone or in groups. All members of
  the same group will want to ride together.  The ride begins once
  the roller coaster runs out of sufficient space for the next group.
   Once a ride is over, passengers go back to the end of the queue
  in the same order.
  Each passenger pays one dirham to ride the roller coaster once.
  The roller coaster can only operate a limited number of times each day.

Here is my code:

public static class RollerCoaster
{
    private static int CountDirhamsEarnedInOneOperation(int numAvailablePlaces, LinkedList<int> numPeopleInEachGroup)
    {
        //Each passenger pays one dirham to take the ride once.
        int dirhamsEarned = 0;
        int numGroupsInQueue = numPeopleInEachGroup.Count();

        while (numPeopleInEachGroup.First() <= numAvailablePlaces && numGroupsInQueue > 0)
        {
            dirhamsEarned += numPeopleInEachGroup.First();
            numAvailablePlaces -= numPeopleInEachGroup.First();
            numPeopleInEachGroup.AddLast(numPeopleInEachGroup.First());
            numPeopleInEachGroup.RemoveFirst();
            numGroupsInQueue--;
        }

        return dirhamsEarned;
    }

    public static long CalculateDirhamsEarned(int numAvailablePlaces, int numOperationsPerDay, LinkedList<int> numPeopleInEachGroup)
    {
        long dirhamsEarned = 0;

        while (numOperationsPerDay > 0)
        {
            dirhamsEarned += (long)CountDirhamsEarnedInOneOperation(numAvailablePlaces, numPeopleInEachGroup);
            numOperationsPerDay--;
        }

        return dirhamsEarned;
    }
}

class Program
{
    static void Main(string[] args)
    {
        Console.WriteLine("Please enter:\nThe number of places available;\nThe number of times the attraction can function per day;\nThe number of groups in the queue.");
        string[] inputs = Console.ReadLine().Split(' ');

        int numAvailablePlaces = int.Parse(inputs[0]);
        int numOperationsPerDay = int.Parse(inputs[1]);
        int numGroupsInQueue = int.Parse(inputs[2]);
        LinkedList<int> numPeopleInEachGroup = new LinkedList<int>();

        Console.WriteLine("Please enter, line by line, the number of people in each group: ");
        for (int i = 0; i < numGroupsInQueue; i++)
        {
            int numPeopleInGroup = int.Parse(Console.ReadLine());
            numPeopleInEachGroup.AddLast(numPeopleInGroup);
        }

        long dirhamsEarned = RollerCoaster.CalculateDirhamsEarned(numAvailablePlaces, numOperationsPerDay, numPeopleInEachGroup);
        Console.WriteLine(dirhamsEarned.ToString());
    }
}

I'm a C# beginner, so I'd appreciate any kind of advice, though I am specifically looking for suggestions that help me improve the time-complexity of my algorithm. My program takes way too long to run when numOperationsPerDay is, say, 10,000,000.

One of the suggestions I would make is to improve your input handling. You don't handle invalid integers well at all.

string input = "invalid int";
int numAvailablePlaces = 0;

while (!int.TryParse(input, out numAvailablePlaces))
{
    Console.WriteLine("Please enter a valid integer for the number of places available.");
    input = Console.ReadLine();
}

And similarly for the other int's you grab. (You could also do them all in one loop.)

This method:

private static int CountDirhamsEarnedInOneOperation(int numAvailablePlaces, LinkedList<int> numPeopleInEachGroup)
{
    //Each passenger pays one dirham to take the ride once.
    int dirhamsEarned = 0;
    int numGroupsInQueue = numPeopleInEachGroup.Count();

    while (numPeopleInEachGroup.First() <= numAvailablePlaces && numGroupsInQueue > 0)
    {
        dirhamsEarned += numPeopleInEachGroup.First();
        numAvailablePlaces -= numPeopleInEachGroup.First();
        numPeopleInEachGroup.AddLast(numPeopleInEachGroup.First());
        numPeopleInEachGroup.RemoveFirst();
        numGroupsInQueue--;
    }

    return dirhamsEarned;
}

Does too much. Break the inside of that while loop to a new method.

Other than that, there's little to say in my opinion.


Rounding a time to its lowest quarter-hour I calculate the lowest quarter-hour using the following code:

public Date getQuarter() {
    Calendar calendar = Calendar.getInstance();
    int mins = calendar.get(Calendar.MINUTE);

    if (mins < 15) {
        mins = 0;
    } else if (mins < 30) {
        mins = 15;
    } else if (mins < 45) {
        mins = 30;
    } else {
        mins = 45;
    }

    calendar.set(Calendar.MINUTE, mins);
    calendar.set(Calendar.SECOND, 0);
    calendar.set(Calendar.MILLISECOND, 0);

    return calendar.getTime();
}

How can this become better / more elegant?

Java has integer division, so you can divide the minutes by 15 and multiply them again:

int mins = calendar.get(Calendar.MINUTE);
calendar.set(Calendar.MINUTE, mins / 15 * 15);

The division will round the number of 15-minute intervals down to the nearest integer, and multiplying it back gets you the interval you want.

To do this down to the milliseconds, do the same trick but with setTime() directly:

calendar.setTimeInMillis(calendar.getTimeInMillis() / 900000 * 900000);


Object-oriented PDO wrapper So, I've recently started learning OOP in PHP and learned $this keyword. I know it's a reference to the current object. So I wrote a class and couple of functions in it.

<?php
Class Connection{
var $databaseHost;
var $databaseUsername;
var $databasePassword;
var $databaseName;

var $query,$result,$conn,$rows;

function setVariables($databaseHost,$databaseUsername,$databasePassword,$databaseName){
    $this->databaseHost=$databaseHost;
    $this->databaseUsername=$databaseUsername;
    $this->databasePassword=$databasePassword;
    $this->databaseName=$databaseName;
}

function playingWithDatabase(){
    $this->conn = new PDO("mysql:host=$this->databaseHost;dbname=$this->databaseName;", $this->databaseUsername, $this->databasePassword);
    $this->conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    //Connect
    try{
        $this->result = $this->conn->prepare("SELECT * FROM student_data");
        $this->result->execute();
        $this->rows = $this->result->fetch(PDO::FETCH_NUM);
        //Dirty Debug
        echo " Displaying First Record

";
        echo $this->rows[1];
        echo "
";
        echo $this->rows[2];
        echo "
";
        echo $this->rows[4];
        echo "
";
        echo $this->rows[6];
        echo "
";
    }
    catch (Exception $e){
        echo "Failure";
    }
    return $this->rows;
}

So, as you can see I took a C# approach to get and set except I'm trying to set in another function and trying to connect to the database in that function.

I feel I'm doing a redundant job here.

  Shall I reduce the usage $this and if yes, How so?
  How will a professional PHP developer will do it?

Well, PDO wrapper is not a good object to exercise on, because

PDO is a wrapper already, and by writing your own wrapper you'd rather spoil most of PDO's excellent features
there is a very specific issue about database wrappers that makes them distinct from other objects - a database connection should be made only once per script execution, so you cannot instantiate it anywhere you need, but rather should pass around a single instance.

Having said that, there is a room for the improvement, either in database wrappers, OOP and general programming techniques.
Database wrapper related
Some mistakes are so common that I even wrote an article, Your first database wrapper's common mistakes. Some highlights from it:

error reporting doesn't actually exist in your wrapper
connection is made every time a function is called, slowing your app significantly and endangering your server to fail under too many connections
statefulness. this one is related to your question about $this. It's ok to keep the sate related to the connection, but not for the query result. So, all your class variables beside $conn just shouldn't be

OOP related

Database credentials as class variables. You aren't going to use $databaseHost more than once. So it makes no sense to make it a class variable. Create a PDO instance in the constructor, and then assign it to a class variable $conn. That's the only variable that would make sense to use.
function setVariables() is rather pointless. We use constructors for setting class variables. Such a function would make sense only if your class is static and therefore never gets instantiated explicitly
so you can tell that your class lacks  a constructor
your class is too diverse. It creates a connection, runs a specific query, and even formats the output as HTML. That's too much. Each class should be responsible for just a single job
moreover, everything said above is stuffed in a single function

General programming issues
Error reporting
Again, I've got an article that explains the basics, PHP error reporting. Some highlights:

NEVER EVER do anything like echo "Failure"; on error. It is both useless and harmful. If you don't know what to do with an error you get, then just let it go
neither you should wrap a code block in a try..catch if you don't know what to do with an exception. Just let it go
you could use a try..catch to wrap your entire code, and then write some useful handling in the catch block. the absolute minimum is to log the error message for the programmer and to show some nice excuse for a site user

Displaying results
It should be a different story, irrelevant to all database interactions. Gather all the required data first, collect it in a convenient array / structure, and then proceed to displaying your HTML page. Given there are situations when you don't have to show an HTML page but just send JSON-encoded data, or there is no data but just an HTTP header at all, it is natural that all output starts only after all database interactions are ended.
PDO wrapper
Given you called your exercise a PDO wrapper, it would be logical to show you a better code for this kind of a class. Well, you can tell I've got an article on that as well :)
Given the explanations above, your PDO wrapper should only let you to hold a database connection and to run a query. Everything else should be done by another class / procedure, even getting the query result.
PDO is made very wisely: it actually consists of two classes, PDO proper and PDOstatement, which is used to hold all the stuff specific to the particular query. Although you could wrap the latter too, let's use it as is, as PDOstatement is a very good class by itself. So, just like vanilla PDO, our wrapper would return an instance of PDOStatement that could be then used to retrieve the selected data.
class MyPDO extends PDO
{
    public function __construct($dsn, $username = NULL, $password = NULL, $options = [])
    {
        $default_options = [
            PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
            PDO::ATTR_EMULATE_PREPARES => false,
            PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
        ];
        $options = array_replace($default_options, $options);
        parent::__construct($dsn, $username, $password, $options);
    }
    public function run($sql, $args = NULL)
    {
        if (!$args)
        {
             return $this->query($sql);
        }
        $stmt = $this->prepare($sql);
        $stmt->execute($args);
        return $stmt;
    }
}

As of the running particular queries, it should be done in a completely distinct class / routine. Let's create a Student class that uses our PDO instance as a service:
class Student
{
    /* @var MyPDO */
    protected $db;

    protected $data;

    public function __construct(MyPDO $db)
    {
        $this->db = $db;
    }

    public function findAll()
    {
        return $this->db->run("SELECT * FROM student_data")->fetchAll();
    }
}

which uses function findAll() to return all the students' data. Whereas output should be a completely different story:
$mypdo = new MyPDO('mysql:host=localhost;dbname=test;charset=utf8');
$student = new Student($mypdo);
$list = $student->findAll();

echo " Displaying All Records

";
foreach ($list as $row) {
    echo $row['name'];
    echo "
";
    echo $row['email'];
    echo "
";
}


Creating connections and executing queries/updates I've recently learned how to use MySQL to store/retrieve data. I've created a MySQLManager class for creating connections and executing queries/updates. Would this be an efficient way to do so?

public class MySQLManager {

private String host;
private String port;
private String database;
private String user;
private String password;

public MySQLManager(String host, String port, String database, String user, String password){
    this.host = host;
    this.port = port;
    this.database = database;
    this.user = user;
    this.password = password;
}

private Connection createConnection(){
    Connection connection;
    try {
        Class.forName("com.mysql.jdbc.Driver");
        connection = DriverManager.getConnection("jdbc:mysql://" + host + ":" + port + "/" + database, user, password);
        return connection;
    } catch (SQLException e) {
        System.out.println("Could not connect to MySQL server! because: " + e.getMessage());
    } catch (ClassNotFoundException e) {
        System.out.println("JDBC Driver not found!");
    }
    return null;
}

public ResultSet executeQuery(String query){
    Connection connection = createConnection();
    Statement statement = null;
    ResultSet set = null;
    try {
        statement = connection.createStatement();
        set = statement.executeQuery(query);
        return set;
    } catch (SQLException e) {
        e.printStackTrace();
    } finally { //Close in order: ResultSet, Statement, Connection.
        try { set.close(); } catch (Exception e) { }
        try { statement.close(); } catch (Exception e) { }
        try { connection.close(); } catch (Exception e) { }
    }
    return null;
}

public void executeUpdate(String update){
    Connection connection = createConnection();
    Statement statement = null;
    try {
        statement = connection.createStatement();
        statement.executeUpdate(update);
    } catch (SQLException e) {
        e.printStackTrace();
    } finally {
        try { statement.close(); } catch (Exception e) { }
        try { connection.close(); } catch (Exception e) { }
    }
}
}

I would then use this to execute queries/updates:

MySQLManager manager = new MySQLManager(host, port, database, user, pass);
manager.executeQuary("SELECT * FROM table;");
manager.executeUpdate("Some update;");

Are there any better ways of doing this? Is there anything I can improve in my code?

You've written a wrapper around JDBC that automatically connects/disconnects, and swallows exceptions.  Don't do that.

JDBC wants you to establish connections explicitly because it's a high-overhead activity.  Connecting involves a TCP handshake, authentication, opening the requested database, and some work to set up a session (which includes some locale settings, for example).  You have just taken a good design decision of JDBC (and all other database APIs) and made it worse.

Furthermore, if every query/update is in its own connection, then you can't execute multiple statements in a transaction.  (MySQL users often don't appreciate the value of transactions.)

Finally, SQLException exists for a reason: they allow your code to gracefully handle failures.  You merely swallow the exceptions after printing a stack trace.  You might as well be writing your data to /dev/null if you don't care whether your writes succeeded.  Also, returning null instead of propagating an exception does not make your caller's code any simpler.

I strongly recommend discarding your entire MySQLManager class and using JDBC as is.


Getting hex colours from a image I am trying to get the hex colours from an image. The problem I am having is that for some reason randomly the code causes high CPU usage, which times out the browser and I am not sure how to optimise the code. I am not sure where the high CPU usage is. I've tried adding delays using setTimeout() but with no luck.

The code was original written in Javascript but I am converted it to Typescript,

Here is full code code which returns the hex values

import {Injectable} from '@angular/core';

@Injectable()
export class ExtractColours {

  private euclidean(p1, p2) {
    let s = 0;
    for (let i = 0, l = p1.length; i < l; i++) {
      s += Math.pow(p1[i] - p2[i], 2)
    }
    return Math.sqrt(s);
  }

  private calculateCenter(points, n) {
    let vals = [];
    let plen = 0;

    for (let i = 0; i < n; i++) { vals.push(0); }
    for (let i = 0, l = points.length; i < l; i++) {
      plen++;
      for (let j = 0; j < n; j++) {
        vals[j] += points[i][j];
      }
    }
    for (let i = 0; i < n; i++) {
      vals[i] = vals[i] / plen;
    }
    return vals;
  }

private kmeans(points, k, min_diff) {

        let plen = points.length;
        let clusters = [];
        let seen = [];

        while (clusters.length < k) {
          let idx = parseInt(String(Math.random() * plen));
          let found = false;
          for (let i = 0; i < seen.length; i++ ) {
            if (idx === seen[i]) {
              found = true;
              break;
            }
          }
          if (!found) {
            seen.push(idx);
            clusters.push([points[idx], [points[idx]]]);
          }
        }

        while (true) {
          let  plists = [];
          for (let i = 0; i < k; i++) {
            plists.push([]);
          }

          for (let j = 0; j < plen; j++) {
            let p = points[j];
            let smallest_distance = 10000000;
            let idx = 0;

            for (let i = 0; i < k; i++) {
              let distance = this.euclidean(p, clusters[i][0]);
              if (distance < smallest_distance) {
                smallest_distance = distance;
                idx = i;
              }
            }
            plists[idx].push(p);
          }

          let diff = 0;

          for (let i = 0; i < k; i++) {
            let old = clusters[i];
            let list = plists[i];
            let center = this.calculateCenter(plists[i], 3);
            let dist = this.euclidean(old[0], center);
            clusters[i] = [center, (plists[i])];
            diff = diff > dist ? diff : dist;
          }
          if (diff < min_diff) {
            break;
          }
        }

        return clusters;
      }

here I am converting the rgb to hex but I am also returning the alpha value

private rgbToHex(rgb) {

          function th(i) {
            let h = parseInt(i).toString(16);
            return h.length == 1 ? '0'+h : h;
          }

          let alpha = (parseInt(th(rgb[0]),16) + parseInt(th(rgb[1]),16) + parseInt(th(rgb[2]),16)) / 3;

        return parseInt(String(alpha)).toString(16) +'#' + th(rgb[0]) + th(rgb[1]) + th(rgb[2]) ;
      }

      public process_image(img, ctx) {
        let points = [];

        ctx.drawImage(img, 0, 0, 200, 200);

        let data = ctx.getImageData(0, 0, 200, 200).data;

        for (let i = 0, l = data.length; i < l;  i += 4) {
          let r = data[i];
          let g = data[i+1];
          let b = data[i+2];
          points.push([r, g, b]);
        }
        let results = this.kmeans(points, 3, 1);
        let hex = [];
        for (let i = 0; i < results.length; i++) {
          hex.push(this.rgbToHex(results[i][0]));
        }

        return hex;
      }
    }

General feedback

There are a few ways that some of the loops can be simplified (see below). These may or may not improve the high CPU issue.

The code makes some repeated calls to functions with the same values in close proximity (e.g. calling th() with each value of rgb twice in rgbToHex()). Storing the returned values and re-using the stored values would make the code more efficient.

I'd consider suggesting the use of functional approaches (E.g. with Array.map(), Array.reduce()) but I wouldn't in this case, given that execution time is an issue.

Targeted feedback

There are some places where const could be used for any variable that doesn't get re-assigned - e.g. vals in ExtractColours::calculateCenter(), plen in ExtractColours::kmeans(), etc. This avoid unintentional re-assignment.

There are places where parseInt() is called without a radix - e.g.

let idx = parseInt(String(Math.random() * plen));

and

let h = parseInt(i).toString(16);

The radix should always be specified1 because it doesn't always default to 10.

Let's look at that method ExtractColours::calculateCenter(). The variable plen could be assigned to points.length (like it is in ExtractColours::kmeans()), instead of incrementing that in the second for loop. And then the division of each array element by plen could be moved into the previous for loop.

And vals is declared and assigned like this:

let vals = [];
let plen = 0;

for (let i = 0; i < n; i++) { vals.push(0); }

This could be simplified using Array.fill()

const vals = Array(n).fill(0);

1https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt#Octal_interpretations_with_no_radix


Monte Carlo Pi (MASM) I'm currently trying to brush up on my assembly skills and, being at the FPU section of the tutorial, I implemented a very basic version of a Monte-Carlo-Algorithm to compute pi. I deliberately use the x87 instruction set instead of SSE because that's what I'm learning at the moment. What would be the pros and cons of SSE vs x87 for that particular case?

; Monte - Carlo PI

.686
.model flat, c
.stack 4096
.mmx
.xmm

.data
align 16
rand DWORD 0
maxu DWORD 07fffffffh
one DWORD 1.0
four DWORD 4.0
maxn DWORD 50000000
count DWORD 0
result REAL8 0.

.code
includelib MSVCRT
extrn exit : near

main proc

; ecx is loop counter
xor eax, eax
mov ecx, maxn

; init FPU and store const
; values 1 and 2^-32
finit
fild dword ptr [maxu]
fld dword ptr [one]
fdivr st(1), st(0)

lp:
; load two random numbers x and y
rdrand ebx
mov dword ptr[rand], ebx
rdrand ebx
mov dword ptr[rand + 4], ebx
fild dword ptr[rand]
fild dword ptr[rand + 4]

; normalize to[0, 1] and square
fmul st(0), st(3)
fmul st(0), st(0)
fxch st(1)
fmul st(0), st(3)
fmul st(0), st(0)
fadd st(0), st(1)

; if point is inside the circle increase counter
fcomi st(0), st(2)
ja greater
inc eax
greater:
fstp result
fstp result
loop lp

; pi/4 = #points inside/#all tries
mov count, eax
fild dword ptr [count]
fidiv dword ptr [maxn]
fmul four
fstp result

push 0
call exit

main endp
end main

Here are some things I've observed that may help you improve your code.

Prefer specific instructions to data storage

There is a constant 1 that is loaded into one but there is actually an instruction fld1 which you could use instead.  This would avoid both the storage area and the time taken to access it.

Take advantage of instructions that pop the stack

Many instruction have a version which pops the top of the FPU stack.  The code currently includes this:

greater:
    fstp result
    fstp result
    loop lp

However it appears that the only reason for those two fstp instructions is to pop the operands off the stack -- they're not actually needed later.  A better way to do that would be this:

    faddp st(1), st(0)

    ; if point is inside the circle increase counter
    fcomip st(0), st(1)
    ja greater
    inc eax
greater:
    loop lp

By using the p suffix on both the faddp and fcomip instructions, the stack is already adjusted.

Think carefully about instruction ordering

The fxch instruction is not really needed.  Instead, you could simply load each random number as needed onto the top of the stack by moving the second of the fild instructions to where the fxch is now.

Avoid branching where practical

Branching is a costly operation to the processor, so avoiding (that is conditional or unconditional jumps) saves cycles and time.  In this code, we have this:

    fadd st(0), st(1)
    ; if point is inside the circle increase counter
    fcomi st(0), st(2)
    ja greater
    inc eax
greater:
    fstp result
    fstp result
    loop lp

I've already addressed changing that to this by using the stack-popping versions to get this:

    faddp st(1), st(0)
    ; if point is inside the circle increase counter
    fcomip st(0), st(2)
    ja greater
    inc eax
greater:
    loop lp

We can do still better, though. The carry flag is set by the fcomip instruction if we should be incrementing eax. We can take advantage of this directly and avoid a branch:

    faddp st(1), st(0)
    ; if point is inside the circle increase counter
    fcomip st(0), st(2)
    adc eax, edx
    loop lp

This assumes that edx is set to 0 which can easily be done the same place that eax is initially set to zero.

Use stack space instead of named variables

Having named variables is very useful for starting out and troubleshooting, but doing so prevents your routine from being reentrant.  That is, if the code is called by more than one thread simultaneously, either or both will work incorrectly because there is only one copy of the variables.  If you instead allocate those on the stack, this potential problem is eliminated.

Consider refining the mathematics

At the moment, the loop scales each random number, squares each, adds the products and compares.  Mathematically, if \$a\$ and \$b\$ are the two random numbers, we calculate

\$(a k)^2 + (b k)^2 < 1\$ but we can certainly manipulate that because \$k\$ is a constant. Specifically, we can convert this to \$a^2 + b^2 < \frac{1}{k^2}\$.  This would allow calculating the constant term \$\frac{1}{k^2}\$ once outside the loop and minimizing the operations within the loop.

Consider reformatting the code

It compiles as is, but typical formatting of assembly language code put only labels and directives in the first column and all code is indented.

Consider the merits of SSE

To answer your question, yes, SSE would speed things up. In particular, you could calculate multiple numbers in parallel. This would allow the code to effectively use fewer loops for the same precision.


Create directories for outputs based on setting I have a dict with a series of bools to specify if a particular aspect of my program will be run (e.g. settings['A'] and settings['B']).

If they are run the results need to go into an aptly named directory.  The directory names (not the entire path) are also stored in the dict (e.g. settings['outputDirNameOut1'] and settings['outputDirNameA'])

In order to create the directories I'm using the following code.

for each in filePathList:
    path = os.path.dirname(each)

    if not os.path.exists(os.path.join(path, settings['outputDirNameOut1'])):
        os.makedirs(os.path.join(path, settings['outputDirNameOut1']))

    if settings['A'] is True and not os.path.exists(os.path.join(path, settings['outputDirNameA'])):
        os.makedirs(os.path.join(path, settings['outputDirNameA']))

    if settings['B'] is True and not os.path.exists(os.path.join(path, settings['outputDirNameB'])):
        os.makedirs(os.path.join(path, settings['outputDirNameThreshold']))

    if settings['C'] is True and not os.path.exists(os.path.join(path, settings['outputDirNameC'])):
        os.makedirs(os.path.join(path, settings['outputDirNameC']))
        os.makedirs(os.path.join(path, settings['outputDirNameOut2']))

    if settings['D'] is True and not os.path.exists(os.path.join(path, settings['outputDirNameOut2'])):
        os.makedirs(os.path.join(path, settings['outputDirNameOut2']))

I'm wondering if there's a simpler, easier to read way to accomplish this.

Any time there's a repeating pattern that's very obvious like this you
might consider using another loop, or a function, or some other
abstraction mechanism available.  To me this also looks like example
code - please post the actual code you're using!

So first a few comments:

x is True isn't very idiomatic unless you really want to check for
specifically one of the true values, True.  Otherwise just write
that as x (keeping in mind that some values like the empty tuple or
list are actually falsy.
Some expressions are repeated in the if test and the body of the
if - in those cases you likely want to have a variable to only
evaluate the expression once for better readability.
Consider using some functional idioms like
map to make
the code more succinct too.
Take a look at PEP8 for
variable naming - in Python it's usually
snake case.

So how with that said, how could this all look like?

for path in map(os.path.dirname, file_path_list):
    x = os.path.join(path, settings['outputDirNameOut1'])
    if not os.path.exists(x):
        os.makedirs(x)

    y = os.path.join(path, settings['outputDirNameA'])
    if settings['A'] and not os.path.exists(y):
        os.makedirs(y)

    z = os.path.join(path, settings['outputDirNameB'])
    if settings['B'] and not os.path.exists(z):
        os.makedirs(z)

    a = os.path.join(path, settings['outputDirNameC'])
    if settings['C'] and not os.path.exists(a):
        os.makedirs(a)
        os.makedirs(os.path.join(path, settings['outputDirNameOut2']))

    b = os.path.join(path, settings['outputDirNameOut2'])
    if settings['D'] and not os.path.exists(b):
        os.makedirs(b)

Now you just have to come up with more sensible variable names.  Also I
have the feeling that the double makedirs in the fourth case is a
copy&paste error?

Going with that assumption this can be simplified further:

names = ['outputDirNameOut1', 'outputDirNameA', 'outputDirNameB',
         'outputDirNameC', 'outputDirNameOut2']
dirs = [settings[name] for name in names]
for path in map(os.path.dirname, file_path_list):
    for dir in dirs:
        x = os.path.join(path, dir)
        if not os.path.exists(x):
            os.makedirs(x)

At that point I can't see any more obvious changes.

Edit: That assumption doesn't hold and I also skipped some part of the
logic.  Therefore I'd suggest writing and using a local function:

def foo(name):
    x = os.path.join(path, settings[name])
    if not os.path.exists(x):
        os.makedirs(x)

for path in map(os.path.dirname, file_path_list):
    foo('outputDirNameOut1')

    if settings['A']:
        foo('outputDirNameA')

    if settings['B']:
        foo('outputDirNameB')

    if settings['C']:
        foo('outputDirNameC')
        foo('outputDirNameOut2')

    if settings['D']:
        foo('outputDirNameOut2')


Object-to-array flattening function I have written a function that is designed to convert an object into an array of key/pair values ("flattening it") like so:

Input

{
  "a": true,
  "b": {
    "c": 500
  }
}

Output

[
  ["a", true],
  ["b", [
    ["c", 500]
  ]
]

Here is the code I have written to do it:

function isCyclicObject(object, seenObjects) {
  seenObjects = (seenObjects instanceof Array) ? seenObjects : [];

  if (object instanceof Object) {
    if (seenObjects.indexOf(object) !== -1) {
      return true;
    }

    seenObjects.push(object);

    for (var key in object) {
      if (object.hasOwnProperty(key) && isCyclicObject(object[key], seenObjects)) {
        return true;
      }
    }

    return false;
  }

  return false;
}

function flattenObject(object) {
  if (!(object instanceof Object) || isCyclicObject(object)) {
    return null;
  }

  var flattened = [];

  for (var key in object) {
    var value = object[key];

    if (value instanceof Object) {
      value = flattenObject(value);

      if (value === null) {
        return null;
      }
    }

    flattened.push([key, value]);
  }

  return flattened;
}

I don't really have any major concerns over my code, however I would really appreciate having someone review it and point out any mistakes or flaws in my code that I haven't noticed. I'm also not all too familiar with how to lay out my JS code, so I believe that could possibly be improved too.

And here is the code I wrote to test its functionality:

function compareArrays(first, second) {
  if (!(first instanceof Array) || !(second instanceof Array)) {
    return false;
  }

  if (first.length != second.length) {
    return false;
  }

  for (var i = 0; i < first.length; i++) {
    var firstValue = first[i],
        secondValue = second[i];

    if (firstValue instanceof Array && secondValue instanceof Array) {
      if (!compareArrays(firstValue, secondValue)) {
        return false;
      }
    } else if (firstValue != secondValue) {
      return false;
    }
  }

  return true;
}

function testFlattenObject(object, expected) {
  var flattened = flattenObject(object);

  var objectString = JSON.stringify(object),
      expectedString = JSON.stringify(expected),
      flattenedString = JSON.stringify(flattened);

  var failed = false;

  if (object instanceof Object) {
    failed = !compareArrays(flattened, expected);
  } else {
    failed = flattened !== null;
  }

  if (failed) {
    console.error("Test failed for " + objectString + ": expected " + expectedString + " but got " + flattenedString);
  } else {
    console.log("Test passed");
  }
}

testFlattenObject({}, []);

testFlattenObject({
  a: true
}, [
  ["a", true]
]);

testFlattenObject({
  a: true,
  b: {
    c: 500
  }
}, [
  ["a", true],
  ["b", [
    ["c", 500]
  ]]
]);

I guess correctness is the most important aspect - as I mentioned in a comment, isCyclicObject returns false positives for references to the same substructure.

I don't know if you intended to directly return null if a value is null (in the flattenObject function). Seems a little harsh to scrap the whole object if any one of its properties has a null value. I would have assumed the behavior should have been to skip pushing the property to the flattened array.

Performance-wise there is some room for improvement:

the function isCyclicObject is called every time the flattenObject function is called. This is not necessary - you only need to perform the check for cycles once.
an array is not the ideal data structure to use for seenObjects. I'd pick the native Set - don't worry, even IE (11) supports it. Read about it on MDN. Plus, to check if something is part of a set you get the neat set.has(something) method which is far better looking than array.indexOf(something) !== -1.
for ... in is not very optimizable by JavaScript engines and what's more it will cause the whole function it's part of to not be optimized. You can read about this here. The other argument against for ... in is that it needs to be accompanied by hasOwnProperty. I see you're doing that for isCylicObject but not for flattenObject. Instead of for ... in you could use Object.keys(obj) which returns an array of the object's own properties directly.

Style-wise:

don't expose seenObjects to the user. If the user accidentally types in a second parameter to seenObjects then strange things will happen and debugging it is going to be unpleasant. Instead you can initialise it in a closure like so:

function isCyclicObject(object) {
  var seenObjects = []

  // let's shadow the original function
  function isCyclicObject(object) {
    // check every property or and call traverse recursively
    // your original logic, except for the `seenObjects` initialization
  }

  return isCyclicObject(object)
}

This way you also eliminate that check which occurs every time, making the traversal faster.

prefer to use const and let over var unless you're targeting old browsers
prefer the more functional forEach/map/some in your case instead of for loops

EDIT:

Don't use ... instanceof Object to verify if something is an object. Objects with a null prototype will fail that test. Instead use `typeof ... === 'object' && ... !== null


Arduino Create and send data to Serial Port (Software Serial function) I have this code written and functioning that will write out the chars in cmd to a serial port for an input of two int values. As the majority of the byte values fed are constant, I created the following code that only requires receiving the values that change frequently.

void Create_Message(int Address, int Data)
{
  //Local Vars
  Address--;
  byte SvAdd = 1;
  byte FuCode = 6;
  byte AddHi = Address>>8;
  byte AddLo = Address;
  byte DatHi = Data>>8;
  byte DatLo = Data;
  byte AdDa[] = {SvAdd,FuCode,AddHi,AddLo,DatHi,DatLo};
  unsigned int crc=CRC(AdDa,sizeof(AdDa)/sizeof(AdDa[0]));
  byte CrcHi = crc & 0xFF;
  byte CrcLo = crc >> 8;
  /*U8 Slave Address, U8, Function Code, U16 Address, U16 Data, U16 CRC
  * to
  *[U8 Address Split High, U8 Address Split Low, U8 Data Split High, U8 Data Split Low, U8 Function Code]
  */
  //Send Data
  unsigned char cmd[8]={SvAdd,FuCode,AddHi,AddLo,DatHi,DatLo,CrcHi,CrcLo};
  digitalWrite(SSerialTxControl, RS485Transmit);  // Enable RS485 Transmit
  lcd.setCursor(0, 1);
  for (int i=0; i< 8; i++)
  {
    RS485Serial.write(cmd[i]);   // Send string someplace
    /*//Serial.print(cmd[i]); //echo back (debug)
    if (i>1)
    {
    lcd.print(cmd[i]);
    lcd.print(" ");
    }*/
   }
  digitalWrite(SSerialTxControl, RS485Receive);  // Disable RS485 Transmit
}

unsigned int CRC(byte AdDa[], int Size )
{
  unsigned int crc16 = 0xFFFF;
  int pos = 0;
  int i = 0;
  /*Write CRC*/
  for (pos = 0; pos < Size; pos++)
  {
    crc16 ^= AdDa[pos];          // XOR byte into least sig. byte of crc
    for (i = 8; i != 0; i--) // Loop over each bit
    {
      if ((crc16 & 0x0001) != 0)        // If the LSB is set
      {
        crc16 >>= 1;                    // Shift right and XOR 0xA001
        crc16 ^= 0xA001;
      }
      else                            // Else LSB is not set
      crc16 >>= 1;                    // Just shift right
    }
  }
  /*Note, this number has low and high bytes swapped,
  so use it accordingly (or swap bytes)*/
  return crc16;
}

I'm pretty new to writing code, but want to try and make this as efficient as possible. Any pointers as to how I could better optimize this function would be greatly appreciated.

CreateMessage is a misnomer. The message it creates vanishes as soon as the function returns. Its real purpose it to send a message. So call it appropriately.
Too many comments duplicating the functionality. For example, any time you feel you need a comment as in

digitalWrite(SSerialTxControl, RS485Transmit);  // Enable RS485 Transmit

it means that you really need a function

void enableRS485Transmit(....) {
    digitalWrite(SSerialTxControl, RS485Transmit);
}

The inner loop of CRC computation can be simplified. Notice that no matter what, the first thing you do is crc16 >>= 1. Factor it out:

    for (i = 8; i != 0; i--) {
        crc16 >>= 1;
        if (crc16 & 0x0001) {
            crc16 ^= 0xA001;
        }
    }

Granted, the compiler will likely notice that. Still it is good for the reviewers and maintainers.
The function takes 24 bytes of stack space. Seems like a bit too much. Granted, the compiler would likely optimize out most of it. Still, consider

    enum { Slave, Function, AddHi, AddLo, DataHi, DataLo, CrcHi, CrcLo, MsgSize};
    byte message[MsgSize] = {1, 6, Address>>8, Address, Data>>8, Data};
    unsigned int crc = CRC(message, CrcHi);
    message[CrcHi] = crc;
    message[CrcLo] = crc >> 8;

If your compiler has <stdint.h>, I strongly recommend using an explicit uint16_t rather then unsigned int (of unspecified bit width).


Search for all matching users and security groups using System.DirectoryServices.AccountManagement I am attempting to return all users and security groups that begin with the provided substring. The following does work but I suspect there may be a better way to write this and am interested in both improving the performance and the style of the Search function.

ActiveDirectory.cs

public static class ActiveDirectory
    {

        public static IEnumerable<Recipient> Search(string search)
        {
            var recipients = new List<Recipient>();

            using (var ctx = new PrincipalContext(ContextType.Domain))
            {
                search = $"{search}*";

                var userPrincipals = new List<UserPrincipal>
                {
                    new UserPrincipal(ctx) {DisplayName = search},
                    new UserPrincipal(ctx) {Name = search},
                    new UserPrincipal(ctx) {SamAccountName = search}
                };

                foreach (var principal in userPrincipals)
                {
                    foreach (var usr in new PrincipalSearcher(principal).FindAll())
                    {
                        if (!(usr is UserPrincipal user) || recipients.Any(m => m.Name == (user.DisplayName ?? user.Name) && m.Type == RecipientType.Group)) continue;
                        recipients.Add(new Recipient()
                        {
                            Name = user.SamAccountName,
                            DisplayName = user.DisplayName ?? user.Name,
                            Type = RecipientType.User
                        });
                    }
                }

                var groupPrincipals = new List<GroupPrincipal>
                {
                    new GroupPrincipal(ctx) {DisplayName = search},
                    new GroupPrincipal(ctx) {Name = search},
                    new GroupPrincipal(ctx) {SamAccountName = search}
                };

                foreach (var principal in groupPrincipals)
                {
                    foreach (var grp in new PrincipalSearcher(principal).FindAll())
                    {
                        if (!(grp is GroupPrincipal group) || recipients.Any(m => m.Name == (group.DisplayName ?? group.Name) && m.Type == RecipientType.Group)) continue;
                        recipients.Add(new Recipient()
                        {
                            Name = group.SamAccountName,
                            DisplayName = group.DisplayName ?? group.Name,
                            Type = RecipientType.Group
                        });
                    }
                }
            }

            return recipients;
        }

    }

Recipient.cs

public class Recipient
    {
        public Guid? Id { get; set; }
        public string Name { get; set; }
        public string DisplayName { get; set; }
        public RecipientType Type { get; set; }

        public Guid? ChannelId { get; set; }
    }

RecipientType.cs

public enum RecipientType
    {
        User,
        Group
    }

I don't know better ways to query for principals, so below is some general comments.

        if (!(usr is UserPrincipal user) || recipients.Any(m => m.Name == (user.DisplayName ?? user.Name) && m.Type == RecipientType.Group)) continue;

I think m.Type == RecipientType.Group should be m.Type == RecipientType.User

When I run your function I get an InvalidOpeartionException for this filter:

  new GroupPrincipal(ctx) {DisplayName = search}

You could consider to extend the function with an argument, that allows the client to specify the context type:

public static IEnumerable<Recipient> Search(string search, ContextType contextType)

The returned Principals from new PrincipalSearcher(principal).FindAll() is of the same type as the argument Principal so you can omit this check:

  if (!(grp is GroupPrincipal group)...

Both PrincipalSearcher and Principal implements IDisposable so all those objects should call Dispose() or be wrapped in a using() statement.

You do essentially the same in the two loops so you should only have one that can handle different types of data. Below I've tried to do that, where all disposable objects are disposed.

public static IEnumerable<Recipient> Search(string search, ContextType contextType)
{
  search = $"{search}*";

  using (PrincipalContext context = new PrincipalContext(contextType))
  {
    var recipients = new List<Recipient>();

    var filterTypes = new[]
    {
      new {
        Filters = new Func<Principal>[]
        {
          () => new UserPrincipal(context) {DisplayName = search},
          () => new UserPrincipal(context) {Name = search},
          () => new UserPrincipal(context) {SamAccountName = search}
        },
        RepicientType = RecipientType.User },
      new {
        Filters = new Func<Principal>[]
        {
          //() => new GroupPrincipal(context) {DisplayName = search},
          () => new GroupPrincipal(context) {Name = search},
          () => new GroupPrincipal(context) {SamAccountName = search}
        },
        RepicientType = RecipientType.Group },
    };

    foreach (var principalFilter in filterTypes)
    {
      foreach (var filterFunc in principalFilter.Filters)
      {
        using (var filter = filterFunc())
        using (var searcher = new PrincipalSearcher(filter))
        {
          foreach (var foundPrincipal in searcher.FindAll())
          {
            using (foundPrincipal)
            {
              if (recipients.Any(m => m.Name == (foundPrincipal.DisplayName ?? foundPrincipal.Name) && m.Type == principalFilter.RepicientType))
                continue;

              recipients.Add(new Recipient()
              {
                Name = foundPrincipal.SamAccountName,
                DisplayName = foundPrincipal.DisplayName ?? foundPrincipal.Name,
                Type = principalFilter.RepicientType
              });
            }
          }
        }
      }
    }

    return recipients;
  }
}

Here I use new Func<Principal>[] instead of just new Principal[] in order to signal, that the "client" (the loop) is responsible for cleaning up the created object.

I'm not sure I like this:

      foreach (Principal foundPrincipal in searcher.FindAll())
      {
        using (foundPrincipal)
        {
           ...

but it's doable.

Just for the exercise: a version that goes all in on LINQ with the same considerations as above:

  public static class ActiveDirectory
  {
    private static IEnumerable<TPrincipal> Search<TPrincipal>(
      this IEnumerable<Func<TPrincipal>> filters,
      IEqualityComparer<TPrincipal> equalityComparer,
      ContextType contextType) where TPrincipal : Principal
    {
      return filters
        .SelectMany(creator =>
        {
          using (TPrincipal principal = creator())
          using (var searcher = new PrincipalSearcher(principal))
          {
            return searcher.FindAll();
          }
        })
        .Cast<TPrincipal>()
        .Distinct(equalityComparer)
        .ToArray();
    }

    private static IEnumerable<Recipient> Convert<TPrincipal>(this IEnumerable<TPrincipal> principals, RecipientType recipientType, bool disposePrincipals = true) where TPrincipal : Principal
    {
      foreach (TPrincipal principal in principals)
      {
        Recipient recipient = new Recipient
        {
          Name = principal.SamAccountName,
          DisplayName = principal.DisplayName ?? principal.Name,
          Type = recipientType
        };

        if (disposePrincipals)
          principal.Dispose();

        yield return recipient;
      }
    }

    public static IEnumerable<Recipient> Search(string search, ContextType contextType)
    {
      search = $"{search}*";

      using (var context = new PrincipalContext(contextType))
      {
        var userFilters = new Func<UserPrincipal>[]
        {
          () => new UserPrincipal(context) {DisplayName = search},
          () => new UserPrincipal(context) {Name = search},
          () => new UserPrincipal(context) {SamAccountName = search}
        };

        var groupFilters = new Func<GroupPrincipal>[]
        {
          //() => new GroupPrincipal(context) {DisplayName = search},
          () => new GroupPrincipal(context) {Name = search},
          () => new GroupPrincipal(context) {SamAccountName = search}
        };

        var users = userFilters.Search(new PrincipalEqualityComparer<UserPrincipal>(), contextType);
        var groups = groupFilters.Search(new PrincipalEqualityComparer<GroupPrincipal>(), contextType);

        return users.Convert(RecipientType.User).Concat(groups.Convert(RecipientType.Group)).ToArray();
      }
    }
  }

  class PrincipalEqualityComparer<TPrincipal> : IEqualityComparer<TPrincipal> where TPrincipal : Principal
  {
    public bool Equals(TPrincipal x, TPrincipal y)
    {
      if (x != null && y == null || x == null && y != null) return false;
      return ReferenceEquals(x, y) || string.Equals(x.Name, y.Name) || string.Equals(x.DisplayName, y.DisplayName);
    }

    public int GetHashCode(TPrincipal obj)
    {
      return 0; // obj.GetHashCode();
    }
  }