Formatting options for a long IF statement?

I've used the second snippet since I've found that much easier/faster to understand:

Here you could omit the else keyword:

if (empty($row)) { // if the row doesn't exist
    return $this->updateRow($id);
}
else { // the row does exist
...
}

The following is the same:

if (empty($row)) { // if the row doesn't exist
    return $this->updateRow($id);
}
// the row does exist
...

Then, you could create an explanatory variable to make the comments unnecessary:

$rowExists = !empty($row)
if (!rowExists) {
    return $this->updateRow($id);
}
...

Another explanatory variable with an inverted condition:

$rowExists = !empty($row)
if (!rowExists) {
    return $this->updateRow($id);
}

$updatedToday = (date('j', time()) == date('j', strtotime($row['date_added'])));
if ($updatedToday) {
    return 0;
}

Explanatory variables give names to the expression and reveal the developer's intent, so maintainers will know what's the purpose of (date('j', time()) == date('j', strtotime($row['date_added']))) faster (it should mean that we have an update today) (he or she doesn't have to figure it out or reverse engineer).

It also makes bug fixing faster since a maintainer easily can validate whether the purpose is wrong (we need to check whether we have an update this week instead of this day!) or the implementation (to check that we have an update today or not we should use created field instead of date_added, for example).
Yet another explanatory variables and flattened conditonals:

$neverIgnored = empty($row['ignore']);
if ($neverIgnored) {
    return $this->updateRow($id);
}

$ignoredLastWeek = (time() - strtotime($row['ignore']) < 3600 * 24 * 7);
if ($ignoredLastWeek) {
    return 0;
}

return $this->updateRow($id);

Final code:

$rowExists = !empty($row)
if (!rowExists) {
    return $this->updateRow($id);
}

$updatedToday = (date('j', time()) == date('j', strtotime($row['date_added'])));
if ($updatedToday) {
    return 0;
}

$neverIgnored = empty($row['ignore']);
if ($neverIgnored) {
    return $this->updateRow($id);
}

$ignoredLastWeek = (time() - strtotime($row['ignore']) < 3600 * 24 * 7);
if ($ignoredLastWeek) {
    return 0;
}

return $this->updateRow($id);

You could also create a boolean isUpdateNeeded() function:

function isUpdateNeeded(...) {
    $rowExists = !empty($row)
    if (!rowExists) {
        return true;
    }

    $updatedToday = (date('j', time()) == date('j', strtotime($row['date_added'])));
    if ($updatedToday) {
        return false;
    }

    $neverIgnored = empty($row['ignore']);
    if ($neverIgnored) {
        return true;
    }

    $ignoredLastWeek = (time() - strtotime($row['ignore']) < 3600 * 24 * 7);
    if ($ignoredLastWeek) {
        return false;
    }

    return true;
}

if (isUpdateNeeded(...)) {
    return $this->updateRow($id);
}
return 0;

References:

Clean Code by Robert C. Martin, G19: Use Explanatory Variables
Clean Code by Robert C. Martin, Bad Comments, p67: Don’t Use a Comment When You Can Use a Function or a Variable
Refactoring: Improving the Design of Existing Code by Martin Fowler, Introduce Explaining Variable
Flattening Arrow Code


Magic numbers

You have a lot of magic numbers in your code. Consider to express them as meaningful constants. ( 4000, 112, 15, 16, 640, 12, 8f, 11f)

Style

Be consistent with your coding style.

string s = System.Text.Encoding.ASCII.GetString(_screenMemory, i, 1);
var pf = new PointF(xLoc, yLoc);

at the first line you explicitly declare s as string  also it is quite obvious by the right hand side that it is a string. On the second line you use var as expected.

There is nothing wrong with declaring implict by using the type, but here you should have either used var also or instead of var PointF.

Naming

Use meaningful names for variables. Mr.Maintainer will thank you for doing so. s and pf are poor names.

Using statement

Instead of creating and disposing the Graphics you should enclose this inside a using statement, so in case of an exception the Graphics object is properly disposed.

Declaring variables

Variables should be declared as nearly as possible of their use ( here: the brushes ).

Refactoring

This won't do anything regarding speed, if the bottleneck are the FillRectangle() and DrawString() methods.

There is no sense in entering the loop or doing anything if _resetInvoked==true. So using a guard condition to return early is the way to go.

private void Screen_Paint(object sender, PaintEventArgs e)
{

    if (_resetInvoked) { return; }
    _resetInvoked = true;

    using (Graphics bmpGraphics = Graphics.FromImage(_screenBitmap))
    {

        int xLoc = 0;
        int yLoc = 0;

        bmpGraphics.Clear(Color.Black);

        for (int i = 0; i < 4000; i += 2)
        {

            if (((xLoc % 640) == 0) && (xLoc != 0))
            {
                yLoc += 12;
                xLoc = 0;
            }

            var bgBit = (byte) ((_screenMemory[i + 1]) & 112);
            SolidBrush bgBrush = _backgroundBrushesArray[bgBit/16];
            bmpGraphics.FillRectangle(bgBrush, xLoc + 2, yLoc + 3, 8f, 11f);

            var fgBit = (byte)((_screenMemory[i + 1]) & 15);
            SolidBrush fgBrush = _foregroundBrushesArray[fgBit];

            var currentText = System.Text.Encoding.ASCII.GetString(_screenMemory, i, 1);
            var currentPoint = new PointF(xLoc, yLoc);
            bmpGraphics.DrawString(currentText, _renderFont, fgBrush, currentPoint);

            xLoc += 8;
        }

    }

    e.Graphics.DrawImage(_screenBitmap, new Point(0, 0));
    _resetInvoked = false;
}


Probability suite for an array of elements

A few little things:

Don't put comments in between function declarations and curly braces (or anything similar to that like classes or methods)

That's hard to read and an eye sore. Put your function comments above the function itself. Ideally using standard docblock format.

/**
* Generates a valid, random probability distribution for a given array of elements, that can be used in conjunction with "probSelect()".
*
* @param   array    $arr        An array of elements
* @param   float    $control    A value that decides how much mass is allowed to be unilaterally dumped onto one element. A high value would permit distributions where most of the mass is concentrated on one element.
* @return  array                An associative array where the keys are the elements in the original array, and the values are their probabilities.
*/
function probGen(array $arr, float $control = 0.01)
{

Use better variable names

What does $arr mean? I can guess that it is probably an array but my IDE can already tell me that. What's actually in that array? $elements would be a better name based on the function comments. A better name would be $elementsOfSomething where something accurately describe the characteristic of those elements.

Always use curly braces for control structures

Although it is perfectly valid syntax to omit curly braces when a control structure only contains one line of code. But it is a best practice to always use them as they make the code more rreadable and prevent future errors. Future you or another developer may want to add a line to a control block and introduce a hard to find bug because they didn't realize the curly braces weren't there.

if($var <= $sum)
    return $key;

should be:

if($var <= $sum) {
    return $key;
}

Use echo over print()

print() is an alias of echo but there are minor differences between the two. Although they don't come into play here, it is the PHP convention to use echo for outputting content.

Don't output content from your functions

Your functions that do work and output content but only when there is an error are inconstant and doing too much. If you have an error, let the function report that back through a special return value (like false or null) or by throwing an exception. Let the calling code worry about reporting back the error and let the function focus on doing one thing and one thing only (just like a good OOP class should be doing).

You can simply statements that check for a Boolean to return a Boolean

Your statement return (sum($arr) == 1)?true:false; is checking if a statement is true and returning true if it is. false if it is false. So you can return the result of your conditional without having to explicitly return true or false because you already have that value:

return (sum($arr) == 1); // will return true or false

An alternative randx() function

The internet seems like it is already full of example functions that will generate a random float. They tend to be similar to the one below. Is there any reason you did not choose to go this route? It has fewer function calls so it should be more performant.

function frand($min, $max, $decimals = 0) {
  $scale = pow(10, $decimals);
  return mt_rand($min * $scale, $max * $scale) / $scale;
}

echo "frand(0, 10, 2) = " . frand(0, 10, 2) . "\n";

The PHP community prefers // to # for comments

Although # is a valid syntax for a one line comment in PHP, it is common practice to use //.


Script for copying files to the appropriate directory

Notes:

don't use single letter variable names for top level bindings
leave spaces around syntax like = and <-
use meaningful variable names for system values (such as file contents)
inline single uses of tiny functions, such as your findhs e
give your core algorithm, w, a meaningful name
don't use long names for local parameters like filepaths. fs is fine.
use pointfree style for arguments in the final position
use when instead of if .. then ... else return ()
use the filepath library for nice file path construction
your only use of f and g is to compose them. so do that and name the result.
use section syntax for lambdas as function arguments to HOFs
indent consistently
use mapM_ when the result you don't care about.

Resulting in:

import Data.Char
import Data.List
import Control.Monad
import System.Directory
import System.FilePath

clean = takeWhile (\x -> x == 'c' || x == 'h' || isDigit x)
      . dropWhile (/='c')

process path = do
   y <- readFile path

   let a = clean y

   when ("ch" `isPrefixOf` a) $ do
         createDirectoryIfMissing False a
         g <- getCurrentDirectory
         writeFile (g </> a </> path) y

main = do
    y <- getCurrentDirectory
    e <- getDirectoryContents y
    mapM_ process $ filter (isSuffixOf ".hs") e


Consider following the Single Responsibility Principal and break the function up into smaller functions that do less.

Note: This answer makes use of the loopEvos() that is provided in the updated version of the source files on GitHub.

void loopEvos (Grid* grid, size_t speed) {
    nodelay(stdscr, TRUE); // Getting of the input is asyncronous
    // Infinitely loops until any key is pressed
    while (true) {
        grid->evolve();
        displayGrid(grid);

        printw("Press any key to end execution...\n");

        // If there has been something inputted, then break out of this sub-loop
        if (getch() != ERR) {
            break;
        }

        usleep((__useconds_t)speed * 1000); // Sleep the desired amount of time (in micro seconds converted to milliseconds)
    }
}

Whenever code is written future expansion of the program/function must be considered. In the function above there is the code :

        if (c == ' ') {
            grid->evolve();
        } else if (c == 'q') {
            return;
        } else if (c == '\n') {
            loopEvos(grid, speed);
            nodelay(stdscr, FALSE);
        }

It would be is easier to expand the code if another type of control statement was used instead. If the code was

        switch (c) {
            case ' ':
                grid->evolve();
                continue;
            case 'q':
                return;
            case '\n':
                loopEvos(grid, speed);
                nodelay(stdscr, FALSE);
                continue:
            default :
                printw("Invalid character input\n");
                return;
        }

It would be easier to handle any additional input characters such as
KEY_UP, KEY_LEFT, KEY_DOWN and KEY_UP.

        switch (c) {
            case ' ':
                grid->evolve();
                continue;
            case 'q':
                return;
            case '\n':
                loopEvos(grid, speed);
                nodelay(stdscr, FALSE);
                continue;
        // The following code is modified from procs/procs.hpp in the GitHub account.
            case 's':
                askFileSave(grid);
                continue;
            case 'c':
                changeSettings(&speed);
                continue;
            case KEY_UP:
                if (cursorPosY >= 1) {
                    cursorPosY -= 1;
                }
                continue;
            case KEY_LEFT:
                if (cursorPosX >= 2) {
                    cursorPosX -= 2;
                }
                continue;
            case KEY_DOWN:
                if (cursorPosY < (grid->y - 1)) {
                    cursorPosY += 1;
                }
                continue;
            case KEY_RIGHT:
                if ((cursorPosX / 2) < grid->x - 1) {
                    cursorPosX += 2;
                }
                continue;
            default :
                printw("Invalid character input\n");
                return;
        }

It might be easier to read, understand and modify the outer loop if it was a do
while loop rather than a while true loop

void startEvolutionLoop (Grid* grid, size_t speed) {
    clear();

     int userInput;
     do {
         // First of all displays the grid with supporting information
         displayGrid(grid);
         printw("[Space] Evolve once        [Enter] Run evolutions        [q] Exit\n");

         // Gets the most recently touched character
         userInput = getch();

         if (userInput == ' ') {
             grid->evolve();
         } else if (userInput == 'q') {
             continue;
         } else if (userInput == '\n') {
             loopEvos(grid, speed);  // from procs/procs.hpp in the GitHub account.
             nodelay(stdscr, FALSE);
         }
     } while (userInput != `q`);
}


Calculating the sum of array elements to the left and right

This problem is a common problem in coding challenges, and it's a nice one, because there's a really elegant solution which is really fast. It does not even need dynamic programming.... just a little trick of logic.

Your solution takes each position, and for each position, sums all values to the left, and right, then it repeats that until it hits a match. Your solution thus scans all N elements about N/2 times (on average, you scan half the data until you hit a match). This makes your solution a time-complexity of \$O(n^2)\$. If you double the size of the input, the solution takes 4 times longer.

Now, a simple solution is to scan the entire data once, and calculate the sum of all the values:

int sum = 0;
for(int i = 0; i < N; i++){
    sum += arr[i];
}

Note, in the code above, I have added spaces to the expressions to make them more readable.

Now, with that value, if your position is 0, the sum to the left is 0, and to the right is sum.

If you move your cursor to the right, the sum to the left is now the value at element 0 ar[0], and to the right is the sum - ar[0].

So, you can loop until you find the match.....

int left = 0;
int right = sum;
for(int i = 0; i < N; i++){
    left += arr[i];
    right -= arr[i];
    if (left == right) {
        return i;
    }
}

See how, as you go, you can "shift" the value from the one side to the other? This makes the solution a simple \$O(n)\$ complexity.


One general comment -> YOU ARE NOT ESCAPING $id_menu IN YOUR SQL QUERY!

There is a section on how to do this in the CodeIgniter Database Query Documentation.

I'm not familiar with Code Igniter but there is nothing obvious that jumps out at me as to why this would be slow.


There are a few issues with this code (problem of style):

Node is a class with all members public to List. Instead of putting everything private, and List as a friend, you should do this:

class List
{
  struct Node // struct has everything public
  {
    int value;
    Node *next;
  };
  Node *head, *last;

This is less code, and everything in Node is invisible outside of List anyway, because List::Node is private.
You should use initializer lists for data members, and add a constructor for Node:

struct Node // has everything public
{
  int value;
  Node *next;

  Node(int x) : value{ x }, next{ nullptr } {} // <---- here
};

This will allow you to write client code like this:

List()
: head{ new Node(0) }, last{ head }, size{ 0 }
{
}

I know you said the interface of the classes is not the issue, but this is a big problem:

class List
{
// ...
  public:    // <---
  int size;  // <---

The only way this code is acceptable, is if this client code is valid:

List mylist;
mylist.size = -5; // is this valid? what should it do?

If client code is not allowed to alter this value freely, it should not be a public, non-const member of the class.


Building XML elements from a list using LINQ and foreach

The best way to discuss performance is by measuring it. I suggest you use BenchmarkDotNet and never roll your own solution.

Onto your question: it is safe to assume that LINQ will in most cases be slower than the iterative approach. Not only does it allocate more (for example through lambdas) but it's also more expensive in runtime because of interface dispatching and other aspects.

There is obviously a balance to be struck between readability and performance -- if you have no performance issues then LINQ might very well be the best option although I would argue that in this case the iterative approach is very readable as well (if not more).

Something you can consider: use a HashSet<T> instead of a List<T> if your usecase allows it. Performing Contains() is O(n) for a list but O(1) for a Set.


API method to validate Facebook OAuth token

You have a lot of null checks (well, two at least), which I think leads to less readable code. Also, it shouldn't really be necessary to have them.

if (authToken == null) this isn't really something this method should concern itself with. In my opinion, getUserFromDatabase should handle that case (via IllegalArgumentException, which is a RuntimeException). Although it's a case that really shouldn't happen in the first place. Why would getAuthToken return null instead of throwing a not found exception?

You should also always try to reduce the level of nesting you have, to increase readability. If you have

if (something) {
    doSomething();
    return;
} else {
    doALotMore();
}

write it as:

// we handle all the exceptional stuff here, and return
if (somethingWentWrong) {
    handleIt();
    return;
}

// the expected code goes here
toDefaultStuff();

With those two points, your code would look something like this:

@ApiMethod(name = "getUserData", path = "get_user_data")
public Bean getUserData(@Named("token") String token) {

    [...]

    try {
        authToken = ServerFacebookHelper.getAuthToken(token);
    } catch (NoConnectionException e) {
        response.setData("Exception occurred");
        return response;
    } catch (InvalidTokenException ite) {
        response.setData("Token invalid, please log in");
        return response;
    } catch (...) {
        ...
    }

    user = getUserFromDatabase(authToken);
    if (user == null) {
        user = registerUserInDatabase(authToken);
        // removed setData duplication
    }
    response.setData(user.getPersonalisedWelcome);
    return response;
}

if (user == null) also doesn't seem like a good way of handling it. Why is user null? Currently, we don't know, it could be a number of reasons (user not found, problems with the database conncetion, incorrect query, etc). This is why returning null for functionality is generally not a great idea, because the caller will have to guess quite a bit.

One solution would be to add a isUserInDatabase(authToken), but that would add additional db queries.

You could also throw a UserNotFound exception, but it doesn't seem like an exceptional situation, so that is also not ideal (at least in this situation; generally, not finding a user would be exceptional for a getUserFromDatabase method).

Another option would be to add an exists method to your user class, but again, that doesn't seem all that clean either.

The question is what the difference between registering and getting really is (and what data is set in user; for registering, this seems to only be the token, which means that you get an incomplete user object returned, which also doesn't seem ideal), and why you don't handle it in completely different methods (ie why getUserData does something - registering a user - if the user doesn't exist already (instead of having another method for that)).


let me know what I can improve in this code.

For a real-world code review, code should be correct, maintainable, reasonably efficient, and, most importantly, readable.

Writing code is a process of stepwise refinement.

Here's your code. Consider it as a first draft.

func BubbleSort(arr []int) []int {
    keepRunning := true
    for keepRunning {
        keepRunning = false
        for i := 0; i < len(arr)-1; i++ {
            a := arr[i]
            b := arr[i+1]
            if a > b {
                arr[i], arr[i+1] = b, a
                keepRunning = true
            }
        }
    }
    return arr
}

Simplify the code. Express the do ... until construct directly. Remove unnecessary variables. Remove the redundant return value. And so on.

func BubbleSort(a []int) {
    for {
        swap := false
        for i := 1; i < len(a); i++ {
            if a[i-1] > a[i] {
                a[i-1], a[i] = a[i], a[i-1]
                swap = true
            }
        }
        if !swap {
            return
        }
    }
}

Optimize the code. Each pass sorts at least one element to the top.

func BubbleSort(a []int) {
    for i := len(a); i > 0; i-- {
        swap := false
        for j := 1; j < i; j++ {
            if a[j-1] > a[j] {
                a[j-1], a[j] = a[j], a[j-1]
                swap = true
            }
        }
        if !swap {
            return
        }
    }
}

Seek further optimizations. Elements from and above the last swap are sorted.

func BubbleSort(a []int) {
    for i := len(a); i > 1; {
        swap := 0
        for j := 1; j < i; j++ {
            if a[j-1] > a[j] {
                a[j-1], a[j] = a[j], a[j-1]
                swap = j
            }
        }
        i = swap
    }
}

Playground: https://play.golang.org/p/jpoUO5nsJEo

Output:

[7 5 6 9 8]
[5 6 7 8 9]

Write tests using the Go testing package.

Run benchmarks using the Go testing package.

  Comment: I was wondering if I could remove the keepRunning bool
  somehow. and in the above case the swap variable – STEEL

The swap state variable is used to end the sort as soon as possible. It is not necessary.

For example,

func BubbleSort(a []int) {
    for i := 1; i < len(a); i++ {
        for j := len(a) - 1; j >= i; j-- {
            if a[j-1] > a[j] {
                a[j-1], a[j] = a[j], a[j-1]
            }
        }
    }
}


Find the number of substrings of a numerical string greater than a given num string

Since I misunderstood the question. I offer up a second take. I have not convinced myself that log10 will increase the efficacy of the script but I use it anyway to show the concept of using math instead of len on strings.

The code has two generators, one to truncate the number from the left, or moving the X coordinate of the cruel pair. Another generator to move the Y coordinate.

The generator that moves the Y coordinate starts at the same length as the query string is. If the query string is longer it yields 0.

The main function relies on the fact that if the condition are meet, all following conditions in the current substring are meet and we and we can simply add those. This means that the only check done is the check where the is of equal length + 1.

from math import log10

def iterate_number_ints(num):
    length = int(log10(num))+1
    for i in range(length-1):
        yield num%(10**(length))
        if num // 10 ** (length - 2) % 10 == 0:
            yield 0
        length -= 1

def iterate_sub_number(num, lower_limit):
    if num > 0:
        length = int(log10(num))
        lower_limit_length = int(log10(lower_limit+1))
        for n in range(lower_limit_length, length+1):
            yield num//(10**(length-n))
    yield 0

def main():

    queries = [int(x) for x in ["78945884875", "22", "00048", "77", "501"]]
    target = 75201

    for k in range(len(queries)):
        ans = 0
        for t in iterate_number_ints(target):
            if t == 0:
                continue
            length_t = int(log10(t))
            for l in iterate_sub_number(t, queries[k]):
                if l == 0:
                    continue
                length_l = int(log10(l))
                if l > queries[k]:
                    ans += length_t-length_l+1
                    break

        print(queries[k], ans)

if __name__ == '__main__':
    main()

It might not be a complete solution, i can't test it, but it reduces the numbers of cases that are checked. But i should give some pointers.


Display image from live webcam as taken, with four different color filters and in B/W

First, get rid of all the unneeded whitespace. Use consistent amount between functions (Python's official style-guide, PEP8, recommends two).
PEP8 also recommends using spaces in lists, after the commas, and lower_case for all variables and functions (your T in threshold_slow violates this).
Don't use magic numbers in your code. Give them readable names and if necessary make them global constants:

WIDTH, HEIGHT = 640, 480

Next, since your images are already numpy arrays, use that fact. Your (unused) threshold_slow function can be replaced by a single line using numpy.where:

def threshold_fast(T, image):
    return np.where(image >= T, 255, 0)

Note that this does not modify the image inplace. It is a bad practice to do that and return a modified/new object. You should decide, either return a new object or modify in place and return None.
The import cv is not used (and I could not even find a way to install it anymore).
Tuple assignment works also without a list on the left side, just do color, b, g, r, y, p = grab_frame(cam). The same is true when returning a tuple (return color, b, g, r, y, p).
Arguably, I would split up your grab_frame code into subfunctions like red(image), green(image), blue(image), yellow(image), black_and_white(image).

def red(image):
    """Copy only the red channel from image"""
    out = np.zeros_like(image)
    # for some reason red is in the last channel
    out[:, :, 2] = image[:, :, 2]
    return out
...

While this move will not make your code shorter, it will make it more readable.
Note that the canonical order is red, green, blue (RGB). If at all possible I would stick to that. I'm not sure why openCV would deviate from that.
You should at least add a docstring to each of your functions as a rudimentary documentation. See above for a short example.
You can use while True instead of while(1). No parenthesis needed and True is unambiguous (even for those people who both know C-like languages, where 0 is False and shell scripting languages like bash, where non-zero is False).
I would also add a tile(images, rows) function that puts your images into rows and columns. You could just use the itertools recipe grouper for this:

from itertools import zip_longest

def grouper(iterable, n, fillvalue=None):
    "Collect data into fixed-length chunks or blocks"
    # grouper('ABCDEFG', 3, 'x') --> ABC DEF Gxx"
    args = [iter(iterable)] * n
    return zip_longest(*args, fillvalue=fillvalue)

Since you seem to want to use different amounts of tiles, and different effects, it might make sense to keep a list of functions to apply to the base image, so that in the end you only need one call:

def identity(x):
    return x

def tile(images, cols, fillvalue=None):
    return np.vstack(np.hstack(group)
                     for group in grouper(images, cols, fillvalue))

funcs = identity, red, black_and_white, canny
images = (func(image) for func in funcs)
# arrange them in a 2x2 grid
cv2.imshow('HORIZONTAL', tile(images, cols=2, fillvalue=np.zeros_like(image)))

If the number of images is not evenly divisible by the number of columns, the row is filled up with blank images.
You should put your main calling code under a if __name__ == "__main__" guard to allow importing from this script.


You can simplify paths using forward slashes:

GOOGLE_DRIVE_DIRECTORY = 'C:/Users/Jeff/Google Drive/Manifest_Destiny'
MAIN_BACKUP_DIRECTORY = 'C:/Users/Jeff/Desktop/Manifest_Destiny_Backups/md_backup_{0}'
EXTERNAL_DRIVE_DIRECTORY = 'F:/My Files/Manifest_Destiny_Backups/md_backup_{0}'

Instead of converting a date to a string, which may be locale dependent too, and then replacing characters in it, better to use strftime to generate exactly the format that you want:

def get_backup_directory(base_directory):
    date = datetime.datetime.now().strftime('%Y-%m-%d_%H%M')
    return base_directory.format(date)

If a variable contains a format, I would suffix it with _fmt or _format to clarify.

I don't really like global constants inside methods, so I would split copy_files like this:

def copy_files_to(srcdir, dstdir):
    for file in os.listdir(srcdir):
        file_path = os.path.join(srcdir, file)
        if os.path.isfile(file_path):
            shutil.copy(file_path, dstdir)

def copy_files(dstdir):
    copy_files_to(GOOGLE_DRIVE_DIRECTORY, dstdir)

The advantage of this is that copy_files_to is easier to unit test. I think it's also good to use more specific names for the directory variables to clarify which one is a source and destination.


Your code is very procedural. Do this, do that, do another thing. There's nothing inherently wrong with this, but the only OOP in the code you've shown is that of pygame itself. There's a little bit hinted at by the game and player objects, and perhaps the logbook dictionary, which appears to hold some other kind of object. But the way you interact with all of these things is all very procedural.

So, how can you better leverage OOP?

The biggest change I would suggest is to avoid big long series of attribute accesses. Things like self.game.player.logbook[p.name].is_explored represent at best a misuse of OOP. What does this code try to do? It tries to answer if a player has explored a planet. How does it do so? By asking if the planet's name appears in the player's logbook, and if the resulting object has the right attribute set.

So how should this question look in code? It should mention as few of the other objects as possible. Ideally it would look like one of the following lines; choose one and stick with it:

player.has_explored(planet)
planet in player.explored_planets
planet.explored_by(player)
player in planet.explorers
game.planet_explored_by(planet, player)

Now, you might have to get your local planet or player variable from some other collection, but you don't want to have to know it's tracked in the game's player's logbook, indexed by planet's name, and stored in an an attribute called is_explored. Only the tiny function that answers that question should have to know any of that extra stuff, and then if you ever move the information, only that one function has to be updated.

Once you've chosen the new way to ask if a planet has been discovered and explored, update your code to use it. Then look for repetition, or otherwise unnecessary checks. They're distracting to read, and possibly slow your code down. For example, you currently create a list planets_to_blit that includes only planets that have been discovered. Then you iterate over this list to draw halos, but first check again whether the planet was discovered.

If you break the function into multiple pieces (say one helper that retrieves the planets, and another that draws halos), it's hard to know whether the check was performed before calling the second helper. However, instead of repeating the check as an if, consider adding it as an assert in the halo drawing helper. This documents to the reader of the code that the helper should only receive planets that have been discovered, and the resulting exception helps you to find callers that didn't make sure.

There are other small or cosmetic changes I would suggest:

Don't be explicit about comparing against True and False; prefer if x: ... or if not x: ...
Leverage some of the self-documenting features of python: in particular consider using docstrings instead of a comment before a function (on the flip side, consider avoiding the docstring-like string literals anywhere else in a function; use comments or extra functions if you need these)
Use whitespace a bit more liberally: tastes differ, but I like to group related lines into blocks and use empty lines before and after, and I like to have spaces after the # character
Be careful with spelling: in specific, hoover is a vacuum cleaner; hover is the term you're looking for

Now I want to circle back to your question about side effects. Once you've split up your code into smaller helper methods, a good split will often have some of the following properties for each method:

The method only relies on knowledge about one area - this is the first thing I talked about
The method will either only retrieve and filter data, or it will only modify data - for example, one method could retrieve all discovered planets, and another could draw lines for the planets that are passed to it
The method doesn't assume things that aren't in its name - for example a get_planets method wouldn't filter the results to just the discovered planets

If we look at your original code, the view_solarsys function violates all of these to some degree. It would be a lot nicer if it looked more like this:

def draw_solarsys(self):
    """Draw the discovered planets, and links between them."""

    discovered_planets = self.get_discovered_planets()
    for planet in discovered_planets:
        self.draw_SFO_for_planet(planet)

    mouse_pos = pygame.mouse.get_pos()

    for planet in discovered_planets:
        self.draw_planet_with_aura(planet)

        if planet.rect.collidepoint(mouse_pos):
            self.draw_planet_info(planet)
            if self.game.map_active:
                self.handle_clicks(planet)

Is that perfect? Of course not. But I think it helps.


Your code looks like its good quality code. And yes, you’re right to be concerned. Most likely, after you’re gone, somebody else will (eventually) maintain, or change the code that you’ve written to date. I suggest that you comment and write the code so that a maintainer understands its purpose at first read.

For example ...

/*
 * Look quickly over the database to make sure nothing else is signed that ought to be unsigned.
 * TODO: Explain why unsigned is better than signed…
 */

$db = Database::instance();
$all_tables = array();
$result = $db->query('SHOW TABLES');
foreach($result as $unique_table)
{
   $show_created_table = $db->query("SHOW CREATE TABLE `".current($unique_table)."`");
   $all_tables[current($show_created_table->current())] = end($show_created_table->current());
}
echo var_dump($all_tables);

It's worth doing for yourself, for memory sake, if you haven't touched the code for a longtime.
It will also help the maintainer understand the intented purpose of the function. Now, if your project is large and complex, I'd recommend that you use phpdoc -- it's a good standard.


You are using the builtin append() function, you can read its documentation what it does.

In summary, if the destination slice has enough capacity (cap()) to accomodate the elements you're appending to it, no new array will be allocated, but the destination slice will be resliced and the elements will be copied there.

If the destination slice does not have enough capacity, a new array will be allocated, and the current content of the destination slice will be copied into that, and the elements you're appended will follow. A slice referring to the newly allocated array will be returned.

In your case since you're removing elements, the slice will always have enough capacity, so no new array will be allocated. But still, the elements after the element/index being removed has to be copied forward by 1. In case of big slices this will be relatively much work. And it has to be as many times as many elements/indices you want to remove.

Your implementation does not work though. It causes a run-time panic if the removable indices slice does not contain the last index of the input slice, because your code increments nextIndex beyond len(indices) and indexes it like indices[nextIndex]. To make it work, you should use the following if:

if nextIndex < len(indices) && i == indices[nextIndex] {

Also it is much faster to allocate the new slice with len = len(input) - len(ind) than with len = 0 as you did; and use simple assignment to append 1 element instead of calling the built-in append() function:

s2 := make([]int, len(s)-len(idx))

sort.Ints(idx)
nexti, j := 0, 0
for i := range s {
    if nexti < len(idx) && i == idx[nexti] {
        nexti++
    } else {
        s2[j] = s[i]
        j++
    }
}

Next note that we don't need a new slice, we can do the removal "in-place", in the input slice:

sort.Ints(idx)
nexti, j := 0, 0
for i := range s {
    if nexti < len(idx) && i == idx[nexti] {
        nexti++
    } else {
        s[j] = s[i]
        j++
    }
}

s = s[:len(s)-len(idx)]

And as the final, fastest solution:

We will use the original slice, and we will not copy elements 1-by-1. Elements between the removable indices form a contigous part which we can copy in one step using the built-in copy() function. Note that the first part (before the first removable index) is already in place, we don't even have to copy that.

func remove4(s, idx []int) []int {
    if len(idx) == 0 {
        return s
    }
    sort.Ints(idx)

    prev := idx[0]
    for i := 1; i < len(idx); i++ {
        v := idx[i]
        copy(s[prev-i+1:], s[prev+1:v])
        prev = v
    }
    copy(s[prev-len(idx)+1:], s[prev+1:])

    return s[:len(s)-len(idx)]
}

Try all the examples on the Go Playground.

One final note:

By excluding certain elements from a slice by reslicing it, the underlying array will still contain reference to them, the value will remain in memory. It is recommended that whenever an element removed from a slice, always zero its place in the underlying array (its respective element in the slice) so the value will not remain in memory needlessly. This becomes even more critical if your slice contains pointers to big data structures.

In my example I used int, so no need to zero it.

As an easy example: if you want to remove the last element of a slice:

var s []*SomeType

// Remove last element, but first zero its value:
s[len(s)-1] = nil
s = s[:len(s)-1]


Optionally running some code on a background thread

Maybe.

Depends, as always, what happens if you get multiple threads running.

So you start this thread going, your code will move on, and if you start this again, and it hasn't finished the previous round of sending emails.   What happens?

If its all thread safe, then no problem,   but it might send some emails out of order.
If its not thread safe, then hilarity ( or calamity ) ensues :-)


How do I reduce the if..else soup in this Controller function?

Seeing I was asked to post my comment as an answer, I've set about typing this little thing up. It's, essentially, a slightly more verbose version of my initial comment.

Well, all in all, a controller action is a method, so the same "rules" should apply: 1 type of request goes to 1 distinct action method. Your method seems to be dealing with 3 distinct types of requests:

Client requests form page
Client submits form
Ajax submission of form-data

So I'd create 3 distinct actions. That way you can get rid of all those if (Request::ajax()) branches. You should still check if the request is an ajax request, but unless you're in the ajax controller, you can just redirect or return an error.
Since in 2/3 cases, you're validating the same form, you are, of course, free to stash that logic in a private helper method, or in some helper method in the model layer.

Something along these lines (I haven't got much experience with laravel, mind you):

class YourController extends BaseController
{
    public function updateForm()
    {
        //create the form, pass it to the view
    }

    public function ajaxUpdate()
    {
        if (!Request::ajax())
        {
            Response::json(
                array(
                    'message' => 'Invalid request',
                    'redirect' => 'some url'
                )
            );
        }
        $validator = $this->validateInput(Input::all());
        //process resulst
    }

    public function postUpdate()
    {
        if (Request::ajax())
        {
            Response::json(
                array(
                    'message' => 'Invalid request',
                    'redirect' => 'some url'
                )
            );
        }
        $validator = $this->validateInput(Input::all());
        //process resulst
    }

    private function validateInput($submission)
    {//No type-hint, because I'm not sure what type Input::all() returns
        $rules = array(
            'password' => 'required',
            'password_repeat' => 'required|same:password',
            'terms_accepted' => 'required'
        );
        $validator = Validator::make($submission, $rules);
    }
}

Now this may range anywhere from pretty-darn copy-paste ready to bizarre, but it's just to serve as an example of how I would set about this. To my eye, this is pretty clean code, easy to maintain, and, most important of all: Each action method deals with 1 specific request. In order not to repeat myself, I've created a private validateInput method, too, but I take it that's pretty self-evident ;-)


Well, unfortunately you can't get any better than this (as far as I have read). And why would you use an iterator in this case ?

I'd however change a bit of the structure of your code, which allows one to easily change the code if there's any need. I didn't changed too many things, just separated the logic into three different functions and added a for loop to get rid of some repetition in your code.

As for the plotting calls, you kinda' can't change the number of calls. Those are the perks of plotting things (in Python at least).

import matplotlib.pyplot as plt
from mpl_toolkits.mplot3d import Axes3D
import numpy as np

fig = plt.figure()
ax = fig.gca(projection='3d')
ax.set_aspect("equal")

def x_y_edge(x_range, y_range, z_range):
    xx, yy = np.meshgrid(x_range, y_range)

    for value in [0, 1]:
        ax.plot_wireframe(xx, yy, z_range[value], color="r")
        ax.plot_surface(xx, yy, z_range[value], color="r", alpha=0.2)

def y_z_edge(x_range, y_range, z_range):
    yy, zz = np.meshgrid(y_range, z_range)

    for value in [0, 1]:
        ax.plot_wireframe(x_range[value], yy, zz, color="r")
        ax.plot_surface(x_range[value], yy, zz, color="r", alpha=0.2)

def x_z_edge(x_range, y_range, z_range):
    xx, zz = np.meshgrid(x_range, z_range)

    for value in [0, 1]:
        ax.plot_wireframe(xx, y_range[value], zz, color="r")
        ax.plot_surface(xx, y_range[value], zz, color="r", alpha=0.2)

def rect_prism(x_range, y_range, z_range):
    x_y_edge(x_range, y_range, z_range)
    y_z_edge(x_range, y_range, z_range)
    x_z_edge(x_range, y_range, z_range)

def main():
    rect_prism(np.array([-1, 1]),
               np.array([-1, 1]),
               np.array([-0.5, 0.5]))
    plt.show()

if __name__ == '__main__':
    main()

NOTE: I've also added if __name__ == '__main__'. By doing the main check, you can have that code only execute when you want to run the module as a program and not have it execute when someone just wants to import your module and call your functions themselves.


Extract Students with All Enrolments in the Past

Well, since you said that you are going to join to some other tables it seems like you only need the RefNo that meets your criteria. You can use HAVING and remove the CTE all together.

DECLARE @Enrollments TABLE(RecordID int, RefNo char(3), EndDate datetime)
INSERT INTO @Enrollments VALUES
(1,'AAA','1/1/2018'),
(2,'AAA','1/2/2018'),
(3,'BBB','1/1/2016'),
(4,'BBB','1/5/2016'),
(5,'CCC','1/5/2016'),
(6,'CCC','1/1/2018')

--Get the RefNo's that don't have a future date
SELECT
    RefNo
FROM
    @Enrollments
GROUP BY
    RefNo
HAVING MAX(EndDate) < CAST(GETDATE() AS DATE)

--Get all rows for RefNo's that don't have a future date
SELECT
    RecordID
    ,RefNo
    ,EndDate
FROM
    @Enrollments
WHERE
    RefNo IN(
             SELECT RefNo
             FROM @Enrollments
             GROUP BY RefNo
             HAVING MAX(EndDate) < CAST(GETDATE() AS DATE)
             )

If you are just needing the RefNo's to limit results of another table. Then you can use the first query with IN similarly to how the second query is written.

You can see the execution plan here comparing the CTE method you have compared to the sub-query I provided in the second query. On my instance, the plan broke down as follows:

Query Cost (Relative to the Batch)
INSERT INTO @Enrollments....24%
SUBQUERY Example using IN...16%
INSERT INTO @Enrollments....24%
Query with CTE Example......36%

So, the CTE was slower. Granted a lot is going to come into play in your live environment with indexes, what joins you are doing etc. But at least this is another option you can consider.

Lastly, depending on your indexes and joins it's sometimes faster to bring stage a temporary result set in a CTE, Table Variable or Temp Table. In most of my cases, Temp Tables have been the fastest but this could be different for your environment. You already have the CTE method down, though I would do it slightly different. Here are some examples.

--stage the RefNo in a temp table
IF OBJECT_ID('tempdb..#RefNo') IS NOT NULL DROP TABLE #RefNo

SELECT
    RefNo
INTO #RefNo
FROM
    @Enrollments
GROUP BY
    RefNo
HAVING MAX(EndDate) < CAST(GETDATE() AS DATE)

SELECT
    ...
FROM
    YourTable t
    INNER JOIN
    #RefNo n on t.RefNo = n.RefNo

--use a CTE to limit the RefNo. Understand the optimizer may not logically execute this how it is written
WITH cte AS(
    SELECT
        RefNo
    FROM
        @Enrollments
    GROUP BY
        RefNo
    HAVING MAX(EndDate) < CAST(GETDATE() AS DATE))

SELECT
    ...
FROM
    YourTable t
    INNER JOIN
    cte c on t.RefNo = c.RefNo

--using a table variable, which I wouldn't do in most cases
DECLARE @RefNo TABLE (RefNo CHAR(3))

INSERT INTO @RefNo
    SELECT
        RefNo
    FROM
        @Enrollments
    GROUP BY
        RefNo
    HAVING MAX(EndDate) < CAST(GETDATE() AS DATE)

SELECT
    ...
FROM
    YourTable t
    INNER JOIN
    @RefNo rn on t.RefNo = rn.RefNo


Chain of responsibility and handlers dependent on each other

I'm not to sure about your base handler. It has methods like handle() and handleField() that look very similar. One will probably do. I also noted that the save() method is completely missing from your code. Why? You use it, so it should be there. Or at least in an usage example.

As to your actual problem with saving, you can hand over the id's as an argument to the next save() in the clain. Something like:

public function save(array $insertIds = [])
{
    // ..... put needed insert ids into data here ....
    // then perform the insert and get the new insert id
    $newInsertId = $this->insertRow($this->table,$this->data);
    // add that to the array of insert ids
    $insertIds[$this->table] = $newInsertId;
    // progress down the chain, if possible
    if ($this->nextHandler()) {
        $this->nextHandler->save($insertIds);
    }
}

Note that in the $insertIds array the keys are table names, and the values are the actual insert ids. This way all insert ids are available to save() methods down the chain.

It could be useful to return the $insertIds.

Note that the order of chaining is important. You have to think about that when you create the chain. Perhaps it would be a good idea to allow for multiple next handlers.


Calculating the number of coins in money change

In general we want to initialize a variable directly to the value it needs, rather than to a temporary "invalid" value. This reduces complexity, and helps to prevent the accidental use of the "invalid" value as if it were a real input.

We should also put variable declarations as close to the point of use as practical. (e.g. z could be declared at the point of assignment from input). It's best for variables to only exist in the scope in which they are needed.

Note that the input variable is effectively reassigned 3 times, and used to represent 3 different things in the program. If we split the program into separate stages, this becomes clearer:

int main(void)
{
    float input = -1;

    // here "input" is invalid - it's just a placeholder.

    // get user input (dollars):
    {
        do
        {
            printf("The amount of changed owed(in dollars) is ");
            input = get_float();
        }
        while (input < 0);
    }

    // here "input" is the amount of dollars as a float

    // convert input to cents:
    {
        input = input * 100;
        input = round(input);
    }

    // here "input" is the number of cents, as a float

    // calculate number of coins for change:
    {
        int z = input; // note we actually want the number of cents as an int...
        int counter = 0;

        ...

        printf("The number of minimum coins needed is %d\n", counter);
    }
}

It's best to avoid reusing variables like this. Any name given to such a variable is inaccurate or very general (e.g. "input"). Also, when changing some part of the program, we have to understand and modify a much larger amount of code than would otherwise be necessary.

Here, we can avoid reusing the variable by splitting the program up using functions, e.g.:

float get_dollar_input()
{
    while (true)
    {
        printf("The amount of change owed (in dollars) is: ");

        const float dollars = get_float(); // variable initialized to actual value :)

        if (dollars < 0)
        {
            printf("Input must not be negative.");
            continue;
        }

        return dollars;
    }
}

int convert_to_cents(float dollars)
{
    return (int)round(dollars * 100);
}

int calculate_minimal_coins(int cents)
{
    int counter = 0;

    // ...

    return counter;
}

int main(void)
{
    const float dollars = get_dollar_input();
    const int cents = convert_to_cents(dollars);
    const int coins = calculate_minimal_coins(cents);

    printf("The minimum number of coins needed is %d\n", coins);
}

When calculating the change, we do a lot of subtraction in a loop. For a large input (e.g. $200,457,298.46), this could take a looooooong time. We can use division to find the count of each coin, and the remainder (modulus) operator to apply the subtraction:

int calculate_minimal_coins(int cents)
{
    const int quarters = cents / 25;
    cents %= 25;

    const int dimes = cents / 10;
    cents %= 10;

    const int nickels = cents / 5;
    cents %= 5;

    const int pennies = cents; /* unnecessary, but explanatory */

    return quarters + dimes + nickels + pennies;
}

One last thing: We should never use floating point variables to represent exact monetary values. It would be better to ask the user for the number of cents as an integer (or perhaps to ask for two integers: one for dollars, and one for cents).


Customizing errors based on HTTP status code in golang

Your code is fine, but don't use fmt.Fprintf if you don't need formatting.
It will scan your string for placeholders (like %s):
fmt.Fprintf(os.Stdout, "100%sure")
// 100%!s(MISSING)ure

Use Write directly:
os.Stdout.Write([]byte("100%sure"))
// 100%sure

And don't forget to handle returned error.
A bit more about io.Writer
Quote from documentation:

Writer is the interface that wraps the basic Write method.
Write writes len(p) bytes from p to the underlying data stream. It returns the number of bytes written from p (0 <= n <= len(p)) and any error encountered that caused the write to stop early. Write must return a non-nil error if it returns n < len(p). Write must not modify the slice data, even temporarily.

I guess that implementations of Write method are encouraged to write all data in a single call. If we digg the sources a bit we'll find, that under the hood most Write methods are implemented with loops.
On the other hand io.Reader may return with partial read and nil error, so methods like io.ReadFull and ioutil.ReadAll are quite handy.


Waiting for a lock to release with Thread.Sleep()?

An easier answer might be to use a ManualResetEvent. You can block the current thread for a timeout period until another thread signals the ManualResetEvent.

http://philiprieck.com/blog/archive/2004/01/27/WaitHandleSample.aspx

http://www.yoda.arachsys.com/csharp/threads/waithandles.shtml

Basically, blocking thread calls mre.Wait(), processing thread works until it calls mre.Set(). No sleeping, none of this other stuff.

An important note, always Dispose of your ManualResetEvents. Leaking handles is a bad call, this object reaches out into the COM world.


Safely order a list of objects by DateTime

You call this method SafelyOrderListByDateTimeDescending but there is nothing safe about it. I'd be really surprised if I found that a sorting function actually modified the objects I wanted to sort.

propInfo.SetValue(obj, default(DateTime).ToString(), null);

This shouldn't be there. You either need another method that sets default values for uninitialized properties so that the user knows his objects are modified or... you need another really safe approach. This means you need a special comparer that you can use with the OrderByDescending

class DateTimeComparer<T> : IComparer<T>
{
    private readonly PropertyInfo _property;

    public DateTimeComparer(string propertyName) => _property = typeof(T).GetProperty(propertyName);

    public int Compare(T left, T right)
    {
        return GetDateTimeOrDefault(left).CompareTo(GetDateTimeOrDefault(right));
    }

    private DateTime GetDateTimeOrDefault(T obj)
    {
        return
            DateTime.TryParse(_property.GetValue(obj) as string, out DateTime result)
            ? result
            : default(DateTime);
    }
}

It does not modifiy anything. Internally it either uses the value provided by the property or uses the default(DateTime). The constructor requires that you specify the name of the property to compare. (It's C# 7).

Example:

var result = values.OrderByDescending(x => new DateTimeComparer<YourType>("YourProperty"));


Formatting fractions found in a given string to html fraction

Well, regex is often a bad idea, but it's one of the case I find it way better than the alternative :

$raw='30 1/2 et 22 3 / 4';
echo preg_replace('/([0-9]+)\s*\/\s*([0-9]+)/', '$1&frasl;$2',$raw);

Just test it and see for yourself.


The 3 lowest-hanging VBA performance fruit are:

    Application.ScreenUpdating = False
    Application.EnableEvents = False
    Application.Calculation = xlManual

Just make sure to restore them at the end of your sub, and/or if your method encounters an error and stops, else your senior people won't be able to use Excel afterwards and will blame you for breaking it.

Used like so:

Sub/Function ()

    Application.ScreenUpdating = False
    Application.EnableEvents = False
    Application.Calculation = xlManual

        < Code >

    Application.ScreenUpdating = True
    Application.EnableEvents = True
    Application.Calculation = xlAutomatic '/ Assuming it was set to automatic to begin with

End Sub/Function

And with some (very basic) error handling:

Sub/Function ()

    On Error Goto CleanFail

    Application.ScreenUpdating = False
    Application.EnableEvents = False
    Application.Calculation = xlManual

        < Code >

    Application.ScreenUpdating = True
    Application.EnableEvents = True
    Application.Calculation = xlAutomatic '/ Assuming it was set to automatic to begin with

CleanExit:
    Exit Sub/Function

CleanFail:
    '/ Resets the Application settings, *then* raises the error
    On Error Goto 0
    Application.ScreenUpdating = True
    Application.EnableEvents = True
    Application.Calculation = xlAutomatic '/ Assuming it was set to automatic to begin with
    Err.Raise(Err.Number) '/ Or insert your own error handling here

End Sub/Function


Make a function.

function doStuff(obj, c) {
  obj.css({overflow: 'hidden', color: c}).fadeIn(400);
}

Then call it

$('#submenu').click(function () {
  doStuff($('#geregeld'), 'green');
});

(simplified example)

Feel free to mess around: http://jsfiddle.net/rQFHt/


This line is long and hard to read with many commands jammed inside:

mysql --defaults-file=~/.my.cnf -e "create database $MYSQL_DB; create user "$MYSQL_USER"@localhost; set password for "$MYSQL_USER"@localhost = PASSWORD(\""$MYSQL_USER_PASS"\"); GRANT ALL PRIVILEGES ON "$MYSQL_DB".* TO "$MYSQL_USER"@localhost IDENTIFIED BY '"$MYSQL_USER_PASS"'; flush privileges;"

A more readable way to write this:

cat << EOF | mysql --defaults-file=~/.my.cnf
create database $MYSQL_DB;
create user "$MYSQL_USER"@localhost;
set password for "$MYSQL_USER"@localhost = PASSWORD(\""$MYSQL_USER_PASS"\");
GRANT ALL PRIVILEGES ON "$MYSQL_DB".* TO "$MYSQL_USER"@localhost IDENTIFIED BY '"$MYSQL_USER_PASS"';
flush privileges;
EOF

Drop the -r here, as that's useful for recursively removing directories, but you have a simple file here:

rm -r ~/.my.cnf

It seems the script is designed to setup a single WordPress site per system. It would be useful to extract the logic of conducting a WordPress site, so that you could setup multiple sites per system easily if needed.


XML node removal method with 5 arguments

The method has definitely too many arguments and the reason for that is because it does too much. When you look at its name you could think it removes a XML node but under the cover it does three things:

it loads a document
it then does its job of removing a node
it then saves the document under new name

If you properly separated these three concerns your APIs would have much simpler signatures.


Transform a string into an array based on grouping rules

You could perhaps use regular expressions, provided the spec stays simple. For instance

function stringToArray(string) {
  var segments = [];
  string.replace(/(\[([^\]]+)\]|[^,])/g, function (m0, m1, m2) {
    segments.push(m2 || m1); // push bracket content (m2) or single char (m1)
  });
  return segments;
}

The code uses replace to map/scan the string (it doesn't actually bother with replacing anything), since it accepts a callback function.

The pattern used will match either:

The content between [ and ] as a contiguous string: \[([^\]]+)\]
Single, non-comma character outside brackets: [^,]

And due to the g (global) flag in the pattern, it'll repeat matching until the end of the string.

This depends on bracket not being nested or unbalanced (it can't handle that). It's also assumes anything that's not a comma should be captured. I.e. even if the character @ isn't part of the spec, it'll still get treated as a "valid" character and included in the output. Similarly it'll skip over multiple commas in a row, even if such a string is invalid.

So it's kinda rough, and I can't guarantee it'll cover all cases, but it gives the correct output for your test cases.


I don't like repeated calls like this config.getDatabase().getHost() + ":" + config.getDatabase().getPort() + .... config.GetDatabase() should be called once and stored in a local variable.

You will run into trouble when writing unit tests. Right now you have no way of mocking out the database access which forces you to run against a real MySql database which tends to make unit testing a pain in the backside.
Create an interface for Database against which all your business logic should work and inject it into the classes responsible for creating the database queries. Then during unit testing you can pass in a mock-implementation which simply checks that the methods are preparing the statements correctly without having to run a query against an actual database.


Custom data validation approach for store items and prices

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

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

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

... or ...

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

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

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

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

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

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

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

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

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

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

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


I don't know anything about Android programming, so bear with me.

Unused variable

private AppCompatActivity activity = this;

This is unused - and even if it were used, you could just replace all references of it with this.

runPointsList is similarly unused.

Outer class this references

This code:

        @Override
        public void onClick(View v) {
            final Intent intent = new Intent(MainActivity.this, RunsListActivity.class);
            startActivity(intent);
        }

is justified in referencing MainActivity.this because it's an inner class. This code:

    permissionsHandler = new PermissionsHandler(this, MainActivity.this);

is not. You should be able to drop MainActivity. .

Multiple exceptions

This:

    } catch (JSONException e) {
        Log.e(Utils.getPackageName(MainActivity.this), "There was a JSONException whilst trying to save the journey points.");
        e.printStackTrace();
    } catch (IOException e) {
        Log.e(Utils.getPackageName(MainActivity.this), "There was an IOException whilst trying to save the journey points.");
        e.printStackTrace();
    }

should be able to have its two handlers collapsed to one with catch (JSONException|IOException e) syntax, since they do almost the same thing.


It's hard to say exactly how much it'll affect execution speed, but your code isn't written how I'd write it.

First, I'd try to minimize the complexity of the initial condition. I'd only return if root was a null pointer. Unless the tree is tiny, you're probably going to gain more from a minimal test than from being able to skip a level of recursion.

Second, when you need to swap things, it's better to use swap to do the job. One of the interesting points of swap is that it's fairly common to specialize it, so you don't want to call std::swap directly. Rather, you typically want a using declaration to bring std::swap into scope, followed by a call to swap with an unqualified name, so if the namespace of the objects being swapped contains a swap, that'll be found via ADL, and otherwise lookup will fall back to std::swap.

using std::swap;
swap(root->left, root->right);
invertTree(left);
invertTree(right);

This may or may not improve speed for your particular case, but it certainly can in some cases (often enough that you usually want to follow this basic pattern for swapping items in C++).


I have added comments directly in the code whenever I have something to say.

import java.util.EmptyStackException;
import java.util.Objects;

// 'Implementation' does not add any information.
//public class StackImplementation<T> {
public class Stack<T> {

    private StackNode<T> top;
    // Not really needing 'head':
    // private StackNode<T> head;
    private int size;

    // Added 'private'. Now even the code in the same package cannot get to
    // 'StackNode':
    private static class StackNode<T> {

        private T value;
        private StackNode<T> next;

        // Really do not need 'public' since this class is visible ONLY in the
        // 'Stack':
        /*public*/ StackNode(T val) {
            this.value = val;
            // JVM already initializes reference fields to 'null' by default:
            // this.next = null;
        }

        // Same argument against use of 'public' as in the constructor:
        /*public*/ T getValue() {
            return value;
        }

        /*public*/ StackNode<T> getNext() {
            return next;
        }

        /*public*/ void setNext(StackNode<T> nextNode) {
            this.next = nextNode;
        }

        // '@Override' would not hurt here. If you somehow mistyped the name of
        // the method being override, and it has a @Override, compiler will
        // notify you:
        @Override
        public String toString() {
            // You can do just:
            return Objects.toString(value);
            // if (value == null) {
            //     return "null";
            // }
            // return value.toString();
        }
    }

    // In this case you can omit the constructor altogether. JVM initializes
    // reference fields to 'null' by default, and integer fields to zero by
    // default:
    // public StackImplementation() {
    //     head = new StackNode(0);
    //     size = 0;
    //     top = head;
    // }

    public void push(T val) {
        // Since Java 7, you can do "diamond inference":
        StackNode<T> cur = new StackNode<>(val);
        // StackNode<T> cur = new StackNode<T>(val);
        cur.setNext(top);
        top = cur;
        size++;
        // You can remove the 'return' whenever it is the very last statement of
        // a method:
        // return;
    }

    // peek() does not return the topmost node but rather its value:
    public T peek() {
        checkNotEmpty();
        return top.getValue();
    }

    public T pop() {
        checkNotEmpty();
        // if (size == 0 || top.getNext() == null) {
        //     throw new java.util.NoSuchElementException();
        // }

        T poppedElement = top.getValue();
        // Can omit 'this' here:
        top = top.getNext();
        // this.top = this.top.getNext();
        size--;
        return poppedElement;
    }

    public void clear() {
        top = null;
        size = 0;
        // The rest is up to garbage collector.
    }

    public int size() {
        return size;
    }

    public boolean isEmpty() {
        return size == 0;
    }

    @Override
    public String toString() {
        if (size == 0) {
            return "[]";
        }

        StringBuilder sb = new StringBuilder("[").append(top.getValue());
        StackNode<T> node = top.getNext();

        while (node != null) {
            sb.append(" -> ").append(node.getValue());
            node = node.getNext();
        }

        return sb.append("]").toString();
    }

    private void checkNotEmpty() {
        if (size == 0) {
            throw new EmptyStackException();
        }
    }

    public static void main(String[] args) {
        Stack<Integer> st = new Stack<>();
        System.out.println(st.size());
        st.push(1);
        st.push(10);
        st.push(100);
        st.push(null);
        System.out.println(st);
        System.out.println(st.size());
        System.out.println(st.peek());
        System.out.println(st.pop());
        System.out.println(st.peek());
        System.out.println(st.pop());
        System.out.println(st.isEmpty());
        st.clear();
        System.out.println(st.isEmpty());
        System.out.println(st.size());
        st.peek(); // Exception!
    }
}

Hope that helps!


Select products and config values from MySQL

Several things...

Indentation

This:

if ( $id != 0 ) {
try {
// etc.
} catch (PDOException $exception) { // to handle error
    echo "Error: " . $exception->getMessage();
}
    }

Would read easier as such:

if ( $id != 0 ) {
    try {
        // etc.
    } catch (PDOException $exception) { // to handle error
        echo "Error: " . $exception->getMessage();
    }
}

Comments

Many of your comments are of very little use, when the code speaks for itself. For instance:

    } catch (PDOException $exception) { // to handle error

Or:

// select all data

Or even:

// store retrieved row to a variable

SQL

You will save yourself and Mr. or Ms. Maintainer a good deal of grief if you format your SQL in a way that SQL programmers will be familiar with. For instance this:

$query = "SELECT p.product_id AS product_id, p.product_title AS product_title, p.product_description AS product_description, categories.category_name AS category_name, p.product_category AS product_category, p.product_image AS product_image, p.product_image_thumb AS product_image_thumb FROM products p LEFT JOIN categories ON p.product_category=categories.category_id WHERE p.product_id = ? LIMIT 1";

Could instead be:

$query = "SELECT
            p.product_id AS product_id,
            p.product_title AS product_title,
            p.product_description AS product_description,
            categories.category_name AS category_name,
            p.product_category AS product_category,
            p.product_image AS product_image,
            p.product_image_thumb AS product_image_thumb
        FROM products p
        LEFT JOIN categories ON p.product_category=categories.category_id
        WHERE p.product_id = ? LIMIT 1";

I will say that it is good that you are using PDO. More experienced PHP programmers will hopefully have more to say about PHP code, I'm mostly a SQL guy.


A WinForm application that filters users

I don't think that checking the state of the box twice is an issue.  However, I do think this could be written in a more general way that would be easier to add more boxes / fields to later.  For example, sticking with the current class structure:

bool FieldMatchesFilterGroup(string field, params CheckBox[] group)
{
    return group.All(box => !box.Checked)
        || group.Any(box => box.Checked && box.Text == field);
}

bool UserMatchesFilters(SomeUser user)
{
    return FieldMatchesFilterGroup(user.color, checkBoxRed, checkBoxBlue)
        && FieldMatchesFilterGroup(user.number, checkBoxOne, checkBoxTwo);
}

This could be made considerably more general, but at the cost of also becoming more complex, so whether that's worth doing depends on how you plan to use this code.  The FieldMatchesFilterGroup method above is using LINQ, but you could do the same thing with a foreach loop:

bool FieldMatchesFilterGroup(string field, params CheckBox[] group)
{
    bool any = false, found = false;
    foreach(var box in group)
    {
        if(box.Checked)
        {
            any = true;
            found |= (box.Text == field);
        }
    }
    return !any || found;
}

Also, just as a style thing, the typical C# naming convention is UpperCamelCase for methods.


private String replaceWithEnvironmentVariables(String specialChars) {

Naming. specialChars are no special chars, it's String in which variables get substituted. Maybe pattern or alike.

String result="";

Spacing. Needless assignment as it gets overwritten 3 lines below. Should be

String result = specialChars;

boolean flag=true;

Naming... flag means actually nothing.

String temp = "";

And neither does temp. A single meaningless name is sometimes tolerable (especially in short methods), but with more, it's getting harder and harder to understand.

char dollarCheck = '$';

Dollar cheque? Check? ✔? It should be a constant and called e.g.

private static static DOLLAR_CHAR = '$';

or better "variable start char" or alike. Prefer to express what it means to what it is, as the latter is obvious.

    if (index == -1){
        result = result.replaceAll("[{},$]", "");
        return result;
    }

I'd personally throw an exception for malformed inputs like foo}bar.

if(slashCheck==result.charAt(i))

That's Yoda style, the form var == expr is more common.

Now I've got lost... you parse the string somehow... OK, let's ignore it.

String sub = temp.replaceAll("[{},$/]", "");
if(sub==null || sub.length()==0){

Know your libraries. sub can't be null. Instead of sub.length()==0, you can use sub.isEmpty().

sub="";

This line is useless now.

... catch ...

Just leave it out. Let the user deal with the problem. You may know, that e.g., using an empty String is acceptable in case of an error, but you surely don't know it here. Just document the exception and let the poor user deal with it. Here, "user" means you in another piece of the program.

NullPointerException

I see a single place where it can be thrown:

String returnSub = getEnvironmentVariableValue(sub);
result = result.replace(temp, returnSub);

Don't let this happen. Maybe throw a new MissingEnvironmentVariableException(sub), maybe allow the caller to specify a replacement, e.g., like this

private String replaceWithEnvironmentVariables(
        String pattern,
        Map<String, String> specificReplacements,
        String defaultReplacement) {...}

where specificReplacements contains the default values for missing variables and defaultReplacement gets used as a last resort. Allow defaultReplacement to be null, but then throw an exception if it should be needed.


Loop through range, selecting grouped data?

Following on your (shared) need of tightening up your code:

Get rid of unused variables

As per your code you don't use neither creditCol nor cel, so get rid of them

Avoid useless variables

You don't need myWS and can simply go

With Worksheets("Raw - edited")

where you also want to use Worksheets instead of Sheets since the former is the collection of sheets with actual data in cells while the latter is the collection that takes "chart" sheets too.

You don't need any lastRow, startRow, endRow and i variable.

Just loop through a "well" selected range using SpecialCells property of Range object and one of your already declared variable of Range type for the task.

Use Areas property of Range object

this will let you loop through the actual "blocks" (as you're naming them) of the parent range, i.e. the groups of contiguos cells that range is made of

Choose proper row counter column

As already pointed out in previous answers, from your data example "Credit " column has blank cells that may prevent you from picking the actual last one, while "Header" column seems to be the best candidate (and of which you already know column index,being it "H")

So choose this latter or loop through all relevant columns and calculate the maximum lastRow between them (make a Function for this task)

Proper use of Find method of Range object

It always "remembers" and uses the last user's choice of some options, were it made from Excel UI or any VBA code.

So you'd better always specify those options and ensure they're those you actually need

Find(What:="Debit", LookIn:= xlValues, LookAt:= xlWhole, MatchCase:= False)

Final result

All what above toghether results in the following code

Sub strike_out_Matching_Debit_and_Credits()
Dim debitCol As Long
Dim block As Range

With Worksheets("Raw - edited")
    DebitCol = .Rows(1).Find(What:="Debit", LookIn:= xlValues, LookAt:= xlWhole, MatchCase:= False)
    For Each block In .Columns("H").SpecialCells(xlCellTypeConstants, xlTextValues).Areas
        If block.Count > 1 Then
            check_Values block.Offset(, debitCol - 8), block.Offset(, debitCol - 7)
            check_for_Offset_Values block.Offset(, debitCol - 8), block.Offset(, debitCol - 7)
        End If
    Next block
End Sub

This also assumes that:

"Debit" is there for sure in some cell of row 1
column "H" has at least one cell with text value

were these assumptions unsafe then you'd add some "entry" checks, possibly wrapped in a specific function returning a boolean to tell your main Sub whether to proceed or  exit


Here's that conditional, reformatted with line continuations for readability:

If Cells(iCntr, 12).Value = "Mule" _
    Or Cells(iCntr, 11).Value = "*R1*" _
    Or Cells(iCntr, 11).Value = "*R2*" _
    Or Cells(iCntr, 7).Value = "*Mule*" _
    Or Cells(iCntr, 6).Value = "*Unassigned*" _
    Or Cells(iCntr, 12).Value = "PS" _
    Or Cells(iCntr, 7).Value = "Marketing" _
    Or Cells(iCntr, 12).Value = "V1" _
    Or DatePart("m", Cells(iCntr, 16).Value) > DatePart("m", Date) _
Then
    Rows(iCntr).Delete
End If

There are a number of problems here:

Cells (and Rows) are implicit references to the active sheet; this means you're accessing the active worksheet without explicitly saying so - so if the macro takes a long while to run and the user decides to activate another sheet while it's running, your macro will fail to do what it was written for.
You're accessing the active sheet multiple times per row, and doing many checks that you may not need to be doing - if Cells(iCntr,11).Value is "*R1*", then every single next check is happening even though you already know you want to delete that row.

So how do you make it so that you only validate what needs to be validated, and that you don't access the worksheet more than you need to?

First make sure you specify Option Explicit at the top of the module. Option Explicit forces you to always declare all variables you're using, and that alone can prevent many bugs, since without it VBA will happily compile a typo.

Then make it one single read:

Dim target As Worksheet
Set target = Application.ActiveSheet

Dim rowValues()
Dim i As Long
For i = 10000 To 1 Step -1
    rowValues = target.Range(target.Cells(i, 1), target.Cells(i, 20)).Cells
    If IsUpForDeletion(rowValues) Then target.Rows(i).Delete
Next

Now, in one single worksheet operation, we've got an array that contains the values of columns 1-20 for row i; we call a function that returns a Boolean value when the row is "up for deletion", and so we remove that row when that function returns True.

The question is, what would this function do? Remember, we want it to only check what it needs to, and return as soon as it knows it should be returning True.

There's a little hack involving Select Case True that can help here:

Private Function IsUpForDeletion(ByRef rowValues()) As Boolean

    Dim result As Boolean
    result = True

    Select Case True
        Case rowValues(1, 5) = "*Unassigned*"
        Case rowValues(1, 6) = "*Mule*"
        Case rowValues(1, 6) = "Marketing"
        Case rowValues(1, 10) = "*R1*"
        Case rowValues(1, 10) = "*R2*"
        Case rowValues(1, 11) = "PS"
        Case rowValues(1, 11) = "V1"
        Case IsFutureMonth(rowValues(1, 15))
        Case Else
            result = False
    End Select

    IsUpForDeletion = result

End Function

Private Function IsFutureMonth(ByVal value As String) As Boolean

    If Not IsDate(value) Then
        IsFutureMonth = False
        Exit Function
    End If

    IsFutureMonth = DatePart("m", CDate(value)) > Month(Date)

End Function

VBA will evaluate each Case until it finds a condition that evaluates to True, and then immediately jump out and return. Because we're working with an in-memory 2D array here, and not accessing any cells, this is going to run much faster.

Notice the column indices are off by one - that's because VBA arrays are zero-based by default. If you want to refer to column 11 with an 11, then you could use Option Base 1 at the top of the module, and do this:

Private Function IsUpForDeletion(ByRef rowValues()) As Boolean

    Dim result As Boolean
    result = True

    Select Case True
        Case rowValues(1, 6) = "*Unassigned*"
        Case rowValues(1, 7) = "*Mule*"
        Case rowValues(1, 7) = "Marketing"
        Case rowValues(1, 11) = "*R1*"
        Case rowValues(1, 11) = "*R2*"
        Case rowValues(1, 12) = "PS"
        Case rowValues(1, 12) = "V1"
        Case IsFutureMonth(rowValues(1, 16))
        Case Else
            result = False
    End Select

    IsUpForDeletion = result

End Function

For even faster code, make one single read, and iterate an array instead of iterating a worksheet and making 10000 worksheet reads - this code should run orders of magnitude faster than your original code:

Option Base 1

Sub DoSomething()

    Dim target As Worksheet
    Set target = Application.ActiveSheet

    Application.ScreenUpdating = False
    Application.Calculation = xlCalculationManual

    Dim rowValues()
    Dim i As Long
    rowValues = target.Range(target.Cells(1, 1), target.Cells(10000, 20)).Cells
    For i = 10000 To 1 Step -1
        If IsUpForDeletion(rowValues, i) Then target.Rows(i).Delete
    Next

    Application.ScreenUpdating = True
    Application.Calculation = xlCalculationAutomatic

End Sub

Private Function IsUpForDeletion(ByRef rowValues(), ByVal i As Long) As Boolean

    Dim result As Boolean
    result = True

    Select Case True
        Case rowValues(i, 6) = "*Unassigned*"
        Case rowValues(i, 7) = "*Mule*"
        Case rowValues(i, 7) = "Marketing"
        Case rowValues(i, 11) = "*R1*"
        Case rowValues(i, 11) = "*R2*"
        Case rowValues(i, 12) = "PS"
        Case rowValues(i, 12) = "V1"
        Case IsFutureMonth(rowValues(i, 16))
        Case Else
            result = False
    End Select

    IsUpForDeletion = result

End Function

Private Function IsFutureMonth(ByVal value As String) As Boolean

    If Not IsDate(value) Then
        IsFutureMonth = False
        Exit Function
    End If

    IsFutureMonth = DatePart("m", CDate(value)) > Month(Date)

End Function


Vertex cover approximation in JavaScript

As you can see after adding edges to the graph, G[v] has the list of neighbours of v, correct?
Therefore the length of G[v] for the vertex v is equal to the number of edges joining v.
Notice that each edge u-v appears twice in G, once in the neighborhood set of u, and once in the neighborhood of v (G[u] contains v and G[v] contains u both referring to the single edge u-v).
So these being said let's have a look at the code.

We iterate over all vertices in the outermost for loop;
for each vertex v in the set of vertices, we iterate over all edges connected to it.

Since each edge has appeared twice in the list (as mentioned above), we are basically counting (or visiting) each edge twice using these two nested loops.
Hence, the complexity of these two for loops could be O(E) where E is the number of edges in the graph.

Notice that the outer loop, iterates through all vertices any way. So, the complexity of the runtime cannot be less than O(V). This being said the complexity of the algorithm is the maximum of O(E) and O(V), which is exactly what O(V+E) means.


Create hash generator with configurable hash algorithm

Right now you're stuck with the use of a constant. GetHashAlgorithm should receive, as a parameter, a Type. You then have 3 options for method signature :

The first one has the advantage of being sure that the Type inherits HashAlgorithm.

string GetHashAlgorithm<T>() where T : HashAlgorithm
string GetHashAlgorithm(Type hashAlgorithmType)
string GetHashAlgorithm(string typeName)

You shouldn't use a constant, as it is... constant. You then need to recompile your code to change the algorithm, which isn't good.

You could retrieve this string from your app.config file, which means you could configure it without recompiling.

If you don't mind being stuck with constant algorithm, I still don't think you should const them. Retrieve the HashAlgorithm from a factory maybe :

public class HashAlgorithmFactory
{
    public HashAlgorithm Create(/*Parameters to decide which algorithm maybe?*/)
    {
        //Conditions to decide which algorithm to return according to parameter
        return SHA256.Create();
    }
}

Also, is there a reason why you don't use your method CreateSalt in your CreateHash method instead of providing a salt parameter? I think it would make sense, but I don't have full knowledge of your needs.