Japanese rehearsal system At the moment I am trying to create a Japanese rehearsal system, and it's working perfectly. However, I am not pleased with the code. Basically I had a .txt file called lesson1.txt. I saved it as UTF-8 so it can contain Japanese characters.

I had the problem that I couldn't directly use the Japanese characters in JavaScript.  I tried bypassing that issue by finding the Unicode code point of each character in PHP, echo that out in as a JavaScript array and use a JavaScript function to convert it back to the Japanese characters.

Now I figured this is not the way to go, because someone on this site told me this is an awful way of coding. My question is, how do I go by improving this? How can I get the Japanese characters into my JavaScript code the most efficient way? Also, have a look at how badly it is coded right now to get a better way of understanding my problem.

In case necessary: I used this PHP loop to get each Unicode point transferred into characters and saved into a JavaScript array:

$x = 0;
//$LineArray is an array in which each element contains a line of  lesson1.txt
    // so for example: $LineArray[0] = "あつい";
foreach($LineArray as $s)
{
    echo "TextCharacters[" . $x . "] = new Array();\n";
    for($i = 0; $i < mb_strlen($s,"utf-8"); $i++)
    {
        $char = mb_substr($s, $i, 1, "utf-8");
                    // REMOVE BOM
        if(strtoupper(bin2hex(iconv('UTF-8', 'UCS-2', $char))) != "FEFF")
        {
            echo "TextCharacters[" . $x . "].push(go(\"" .   strtoupper(bin2hex(iconv('UTF-8', 'UCS-2', $char))) . "\"));\n";
        }
    }
    $x++;
}

First off, post any and all code that you want reviewed. Just posting a little and saying the rest is "here" is not going to get you many good answers. Some of us dislike venturing to sites unknown to view someone else's code. If you post the rest of it and drop me a comment I will return to take a look at it if you wish. That being said, here is my take on your current code. Note: I reference the other two answers heavily, so this is merely an addition and not a true answer.

Incremental Variables

I don't really agree with Hermann's first comment. "x" is only a bad variable name if it is being used for something concrete and easily expressible in some other way. Here it is an incremental, a throw-away variable, so nothing is wrong with it. It's odd, "i" is usually used as the default incremental with "j" usually being secondary, but there's nothing wrong with "x". I do agree that it came out of no where and might be a little jarring, but if you move that line down to just above the loop it will be less so. Also, following jsanc623's example would also be acceptable.

For vs Foreach

The advantage to using a foreach loop over a for loop is that the first uses internal pointers that are dynamically updated, while the second uses external pointers that are statically updated. Another advantage is speed. A foreach loop is usually faster than a for loop. There are more advantages, but I'm going to stick with the ones that are important here. There is one instance where a for loop is better than a foreach loop. That is when you are looping over an associative array and using an external pointer inside of it that coincides with the internal pointer. So, instead of using a foreach loop in these circumstance, it may be best to use a for loop with array_keys() to access the array index manually. If, on the other hand, you are looping a numerically indexed array you can just use the key of the current element.

foreach( $LineArray AS $x => $s ) {

Variable Names

Again, I have to disagree with Hermann, there is nothing wrong with calling your variables an array, or a map, or anything else that indicates what they hold. Unnecessary? Sure, but at the same time it doesn't hurt anything and can actually help in some cases. Now, I won't say that calling a variable a "String" or an "Int" is good. PHP is very loosely typed, and that can easily change those contents, but adding an "array" or "obj" suffix to your variable lets those reading it know what it is supposed to be and is common enough.

I do agree with Hermann about $s however. It is a horrible name. I would expect to see $line or something similarly related to the array. The only single letter variables you should have are incremental (i, j) or something else that is commonly expressed with a single letter, such as coordinates (x, y, z).

Strings

I'm going to get argued with about this next suggestion, so let me just say up front that it is a stylistic choice that has some basis in fact. PHP allows for two methods of adding variables to a string. The first is escaping. Using double quotes to surround a variable or escape sequence you can escape the variable or sequence without any special syntax, just include them between the double quotes ("$var\n"). The second is concatenation and is the method you are currently using ('abc' . $var1 . "\n"). When concatenating it is best, in my opinion, to use single quotes instead of double. There's one major reason for this, double quotes tells PHP your string has entities that need escaping, whether they are PHP variables or manually escaped returns, tabs, etc... doesn't matter. So when you use double quotes PHP expects these escape routines and uses a small amount of extra processing power to parse the string for them. Now the processing power required and the time it takes are both negligible, but there is a difference and I like to point it out.

For Loops

Another thing about for loops. The arguments passed to a for or while loop are reparsed on each iteration. So passing a function in as an argument is usually a bad idea, at least if that function's return value is not meant to change i.e.(count() or in this case mb_strlen()). Both get the number of iterations the loop is going to go through. These numbers typically don't change between iterations so there is no need to recalculate it. It is just taking up extra processing power. If you move this function out of the argument list and set it as a variable it will run a bit faster, though it may be negligible in this case. jsanc623 touched on this, but I wanted to give the full reason.

Nested Functions

Again, I agree with Hermann, don't have long nested function calls in a statement. Declare it a variable before checking it, especially if you are going to be using it again elsewhere. This follows the DRY (Don't Repeat Yourself) Principle. Personally, I would break it up further, as I don't like nested function calls at all, but that might just be a stylistic choice. Here's how I'd write your if statement.

$bin = iconv('UTF-8', 'UCS-2', $char);
$hex = strtoupper( bin2hex( $bin ) );//Only one nested, and still not as desirable.
if( $hex != "FEFF" ) {

Functions

Again jsanc623 brings up a good point. This single function should be broken up into multiple functions. He doesn't come right out and say it, but it is implied. So I figured I'd go ahead and say it for him. Functions should do one thing and do it well. They should not be concerned with anything else. This is the Single Responsibility Principle.

Pushing to an Array

The following is equivalent to pushing onto the end of an array, and is usually the preferred method for appending array data.

TextCharacters[ 0 ] [] = go( '304A' );

Now, in your comment you said that you were pushing each element. Am I to assume you are doing so manually? If so, a better way would be to create an array of these hex values and loop over them to add them. Even better would be to create a JSON document, that way they could be accessed from both PHP and JS if needed.


optimizing project euler #83 solution Project Euler problem 83 asks:

  In the 5 by 5 matrix below,

131   673 234 103 18
201   96  342 965 150
630   803 746 422 111
537   699 497 121 956
805   732 524 37  331

  the minimal path sum from the top left to the bottom right, by moving left, right, up, and down, is

131 → 201 → 96 → 342 → 234 → 103 → 18 → 150 → 111 → 422 → 121 → 37 → 331

  and is equal to 2297.

  Find the minimal path sum, in matrix.txt (right click and 'Save
  Link/Target As...'), a 31K text file containing a 80 by 80 matrix,
  from the top left to the bottom right by only moving right and down.

I have solved the project euler problem 83 using uniform cost search. This solution takes about 0.6s to solve. I want to know if anyone can get the code to run relatively faster without changing the general outline of the program.

import bisect
f = open('matrix.txt')
matrix = [[int(i) for i in j.split(',')] for j in f]

def uniformCostSearch(startNode):
    frontier = []
    frontier.append(startNode)
    closedSet = set()
    while not len(frontier) == 0:
        currentNode = frontier.pop(0)
        if currentNode.x == 79 and currentNode.y == 79:
            return currentNode.priority
        else:
            if not (currentNode.x, currentNode.y) in closedSet:
                closedSet.add((currentNode.x, currentNode.y))
        possibleMoves = currentNode.neighbors()
        for move in possibleMoves:
            if not (move.x, move.y) in closedSet:
                try:
                    index = frontier.index(move)
                    if move.priority < frontier[index].priority:
                        frontier.pop(index)
                        bisect.insort_left(frontier, move)
                except ValueError:
                    # move is not in frontier so just add it
                    bisect.insort_left(frontier, move)
    return -1

class Node:
    def __init__(self, x, y, priority=0):
        self.x = x
        self.y = y
        self.priority = priority

    def neighbors(self):
        tmp = [Node(self.x + 1, self.y), Node(self.x, self.y + 1),
               Node(self.x - 1, self.y), Node(self.x, self.y - 1),]
        childNodes = []
        for node in tmp:
            if node.x >= 0 and node.y >= 0 and node.x <= 79 and node.y <= 79:
                node.priority = self.priority + matrix[node.y][node.x]
                childNodes.append(node)
        return childNodes

    def __eq__(self, node):
        return self.x == node.x and self.y == node.y

    def __ne__(self, node):
        return not self.x == node.x and self.y == node.y

    def __cmp__(self, node):
        if self.priority < node.priority:
            return -1
        elif self.priority > node.priority:
            return 1
        else:
            return 0

import bisect
f = open('matrix.txt')
matrix = [[int(i) for i in j.split(',')] for j in f]

You could consider using the numpy library, it'll handle multidimensional arrays more neatly and efficiently.

def uniformCostSearch(startNode):

Python convention is to use underscore_with_letters for function names and parameters

    frontier = []
    frontier.append(startNode)

Use frontier = [startNode], it'll be a touch faster

    closedSet = set()
    while not len(frontier) == 0:

Use while frontier:, it does the same thing and due to less operations is going to be a bit faster. You should at least use != rather then not ==

        currentNode = frontier.pop(0)

Popping from the front of a list will be inefficient. Actually, what you really want is a priority queue, see python's deque module.

        if currentNode.x == 79 and currentNode.y == 79:
            return currentNode.priority
        else:
            if not (currentNode.x, currentNode.y) in closedSet:

Move the not over next to in I think it reads clearer. I might also use a 2D array of bools rather then a set here.

                closedSet.add((currentNode.x, currentNode.y))

There isn't really any reason to check if the node is in the set here, you can add the node multiple times. Just add it unconditionally.

        possibleMoves = currentNode.neighbors()
        for move in possibleMoves:

No reason to store it in a local, just combine the previous two lines

            if not (move.x, move.y) in closedSet:
                try:
                    index = frontier.index(move)
                    if move.priority < frontier[index].priority:
                        frontier.pop(index)
                        bisect.insort_left(frontier, move)

Move the last three lines into an else block. That'll make sure only your index line can actually throw things that'll get caught.

                except ValueError:
                    # move is not in frontier so just add it
                    bisect.insort_left(frontier, move)

You do this in either case, I'd move it after the try/except block.
        return -1

class Node:
    def __init__(self, x, y, priority=0):
        self.x = x
        self.y = y
        self.priority = priority

    def neighbors(self):
        tmp = [Node(self.x + 1, self.y), Node(self.x, self.y + 1),
               Node(self.x - 1, self.y), Node(self.x, self.y - 1),]
        childNodes = []
        for node in tmp:
            if node.x >= 0 and node.y >= 0 and node.x <= 79 and node.y <= 79:

I suggest moving 79 into a constant. I'd also do < 80 rather then <= 79.

                node.priority = self.priority + matrix[node.y][node.x]
                childNodes.append(node)

I'd use a yield here, and make this a generator

        return childNodes

    def __eq__(self, node):
        return self.x == node.x and self.y == node.y

This bothers me because multiple nodes will compare equal despite having different priorities.

    def __ne__(self, node):
        return not self.x == node.x and self.y == node.y

    def __cmp__(self, node):
        if self.priority < node.priority:
            return -1
        elif self.priority > node.priority:
            return 1
        else:
            return 0

This really bothers me because you aren't defining your comparisons in a consistent way.


LeetCode: Number of steps to reduce a number to zero C# https://leetcode.com/problems/number-of-steps-to-reduce-a-number-to-zero/

  Given a non-negative integer num, return the number of steps to reduce
  it to zero. If the current number is even, you have to divide it by 2,
  otherwise, you have to subtract 1 from it.

  Example 1:

  Input: num = 14 Output: 6 Explanation:  Step 1) 14 is even; divide by
  2 and obtain 7.  Step 2) 7 is odd; subtract 1 and obtain 6. Step 3) 6
  is even; divide by 2 and obtain 3.  Step 4) 3 is odd; subtract 1 and
  obtain 2.  Step 5) 2 is even; divide by 2 and obtain 1.  Step 6) 1 is
  odd; subtract 1 and obtain 0. Example 2:

  Input: num = 8 Output: 4 Explanation:  Step 1) 8 is even; divide by 2
  and obtain 4.  Step 2) 4 is even; divide by 2 and obtain 2.  Step 3) 2
  is even; divide by 2 and obtain 1.  Step 4) 1 is odd; subtract 1 and
  obtain 0. Example 3:

  Input: num = 123 Output: 12

  Constraints:

  0 <= num <= 10^6

  public int NumberOfSteps (int num) {
        int count = 0;
         while( num !=0)
         {
             if(num %2==0)
             {
                 num = num/2;
                 count++;
             }
             else
             {
                 if( num == 1)
                 {
                     return count+1;
                 }
                 num = num-1;
                 count++;
             }
         }
         return count;
    }

Please review for performance

Not much to say for such a simple algorithm.  However there is an optimization that you could employ.  Check for odd instead of even, then convert to even.  Then divide by 2.  At most this cuts your iterations by half.

It would look like this:

public int NumberOfSteps(int num)
{
    int count = 0;
    while (num != 0)
    {
        if(num % 2 == 1)
        {
            num -= 1;
            ++count;
        }
        if(num > 0)
        {
            num /= 2;
            ++count;
        }
    }
    return count;
}


Upgrade Firefox Quantum with logging and version check This is the updated version of the script first posted here.

Specifically, I first wanted to save the logs to a log file at "$HOME/.logs/ffupgrade.log", but I'm not sure whether that is good practice or whether I should log to syslog. When logging to a file, I would add another variable logfile at the beginning and adapt the functions like so (similarly for success and warning):

logfile="$HOME/.logs/ffupgrade.log"

error_exit() {
    msg="$1"
    echo -e "$(date) ${red}ERROR:${nocolor} ${msg}" 1>&2
    echo "$(date) ERROR: $msg" >> "$logfile"
    exit 1
}

I'm also wondering if it's better to only run the commands that need it with sudo or if I should rerun the whole script as sudo after prompting for the password like this:

if [[ "$EUID" != 0 ]]; then
    warning "Please run as root"
    sudo "$0" "$@"
fi

As always, any other comments are welcome as well.

#!/bin/bash

# firefox_upgrade - program to upgrade firefox quantum

red="\033[1;31m"
yellow="\033[1;33m"
green="\033[1;32m"
nocolor="\033[0m"

error_exit() {
    msg="$1"
    echo -e "$(date) ${red}ERROR:${nocolor} ${msg}" 1>&2
    logger "ERROR: $msg"
    exit 1
}

warning() {
    msg="$1"
    echo -e "$(date) ${yellow}WARNING:${nocolor} ${msg}"
    logger "WARNING: $msg"
}

success() {
    msg="$1"
    echo -e "$(date) ${green}SUCCESS:${nocolor} ${msg}"
    logger "SUCCESS: $msg"
}

ffbin="/opt/firefox"
fflink="/usr/bin/firefox-quantum"

ffpath=
fffile=

# check if path argument was provided
if [ $# -ne 1 ]; then
    error_exit "usage: $0 firefox_quantum_path"
fi

echo "Checking for root access.."
if [[ "$EUID" = 0 ]]; then
    success "already root"
else
    sudo -k # make sure to ask for password on next sudo
    if sudo true; then
        success "correct password"
    else
        error_exit "wrong password"
    fi
fi

# parsing path and filename
ffpath="$1"
fffile="${ffpath##*/}"

# check if input is a valid file
if [ ! -f "$ffpath" ]; then
    error_exit "Invalid file! Aborting."
fi

# check the firefox version
ffpref="${fffile%.*}"
ffversion="${ffpref//[^0-9]/}"
ffinstalled_version="$($fflink --version | sed 's/[^0-9]*//g')"

vind=0
if [[ "$ffversion" < "$ffinstalled_version" ]]; then
    warning "The version to be installed is lower than the installed version"
    vind=1
elif [[ "$ffversion" = "$ffinstalled_version" ]]; then
    warning "The version to be installed is the same as the installed version"
    vind=1
fi

if [[ "$vind" = 1 ]]; then
    echo "Do you want to continue anyway?"
    select yn in "Yes" "No"; do
        case $yn in
            Yes ) : ;;
            No ) exit;;
        esac
    done
fi

# removing previous install, if existent
if [ -e "$ffbin" ]; then
    sudo rm -rf "$ffbin"
    #mv "$ffbin" "$ffbin.old"
    success "removed previous install"
else
    warning "$ffbin doesn't exist."
fi

# removing previous symlink, if existent
if [ -L "$fflink" ]; then
    sudo rm -f "$fflink"
    success "removed previous symlink"
else
    warning "$fflink doesn't exist."
fi

# copying the tar to /opt
if ! sudo rsync -ah --progress "$ffpath" "/opt/$fffile"; then
    error_exit "Couldn't copy the tarball. Aborting."
fi
success "tarball copied"

# unpacking the tar if successfully changed directory
if ! sudo tar -jxvf "/opt/$fffile" -C /opt; then
    error_exit "Could not extract file! Aborting."
fi
success "tarball was extracted"

# if unpack was successful, set permissions, create symlink, and remove tar
sudo chmod 755 "$ffbin"
success "permissions set"
sudo ln -s "$ffbin/firefox" "$fflink"
success "symlink created"
sudo rm "/opt/$fffile"
success "tarball removed from /opt"
success "Firefox was upgraded"
```

To sudo or not to sudo

  I'm also wondering if it's better to only run the commands that need it with sudo or if I should rerun the whole script as sudo after prompting for the password like this:

if [[ "$EUID" != 0 ]]; then
    warning "Please run as root"
    sudo "$0" "$@"
fi

Can the script do its job without root permissions? This script cannot, because it wants to modify files that are normally only editable by root, such as /opt and /usr/bin. Therefore, the script requires root permissions. A good way to handle this is to check if the user has root permission, and exit if that's not the case, for example:

if [[ "$EUID" != 0 ]]; then
    warning "Please run as root"
    exit 1
fi

There's really no need to include the complexity of getting root permission in the script. With the above simple check, the user can either switch to the root user, or rerun the script with sudo. The rest of the script can assume the privilege (and responsibility) of root.

About logging

  I first wanted to save the logs to a log file at "$HOME/.logs/ffupgrade.log", but I'm not sure whether that is good practice or whether I should log to syslog.

I don't really know. But I can reason about it.

Does this really belong to syslog? The script upgrades Firefox. Let's look at something similar, for example, what happens when you upgrade software using the standard package manager of a Linux system. I just upgraded something in a Debian system I have at hand, and it didn't log anything about it in syslog. It may be relevant whether the upgrade is happening on explicit user action or automatically. I would conclude that when a user upgrades a package manually, it's not noteworthy enough to add to syslog.

On the other hand, what happens if you keep logging to ~/.logs? It accumulates forever without cleanup. You might want to log somewhere else that is subject to log a rotation policy in your systems by default, or document how to set it up in the script. This could be a valid argument, except that I doubt your Firefox upgrade logs will ever accumulate to anything significant. So... I would not bother about rotation. If the script is only going to run as root, then I would log somewhere more visible than ~/.logs, let's say /var/log.

  When logging to a file, I would add another variable logfile at the beginning and adapt the functions like so (similarly for success and warning)

I would add a logger function to encapsulate that logic, overriding the logger program, and then you won't need to touch the other functions.

Use local for local variables

It's good to use local for local variables in functions to avoid setting values in the global scope, which can be a source of nasty errors.

Double quote variables used on the command line

$fflink here should be enclosed in double-quotes:

ffinstalled_version="$($fflink --version | sed 's/[^0-9]*//g')"

Comparing versions

I have a doubt about comparing versions.
The script removes all non-numeric characters, and then compares values lexicographically.
Are you sure that will work reliably?
It would have been good to include some example versions in comments, to help assure readers.

Also, to remove non-numeric characters, I would use tr -cd 0-9 instead of sed 's/[^0-9]*//g'.

Verify exit codes

The script does some things, and tells some things to the user that might not be true, for example here:

sudo chmod 755 "$ffbin"
success "permissions set"
sudo ln -s "$ffbin/firefox" "$fflink"
success "symlink created"
sudo rm "/opt/$fffile"
success "tarball removed from /opt"
success "Firefox was upgraded"

Even if those commands fail, the script will print success messages, and happily carry on.

Use better variable names

What is vind?

Use verbose mode more

I would add -v to all interesting commands that support it, for example rm, mv.

Replace 1>&2 with >&2

The default file descriptor redirected is 1, no need to spell it out explicitly.

Simple is better than complex

[ $# != 1 ] is probably more natural and therefore easier to understand than [ $# -ne 1 ].


General select statement to stored procedures I'm trying to write a general function to several specific stored procedures, but I'm afraid that it will cause errors that I don't see now. The function is to get the name of the stored procedure as a string, and get a list of objects that contain the parameters of the stored procedure. Does it look good to you?

public static DataTable SelectQuery(string storedProcedureName, List<object> parameters)
    {
        DataTable dt = new DataTable();
        OleDbConnection con = WorkerDB.GetConnection();
        OleDbCommand cmd = new OleDbCommand();
        cmd.CommandText = storedProcedureName;
        cmd.CommandType = CommandType.StoredProcedure;
        cmd.Connection = con;
        for (int i = 0; i < parameters.Count; i++)
        {
            cmd.Parameters.AddWithValue("?", parameters[i]);
        }
        try
        {
            con.Open();
            OleDbDataAdapter da = new OleDbDataAdapter(cmd);
            da.Fill(dt);
            da.Dispose();
        }
        catch (Exception ex)
        {

            throw ex;
        }
        finally
        {
            con.Close();
        }

        return dt;
    }

This is first refactored version of your code. I have changed following thing

If any object provide dispose method , prefer using statement.
Use dictionary rather than list of parameter as you have a possibility of specifying param names also
Current implementation sticks only to datatable , you might need to think about the datareader.
naming of variable should be explicit.

Few more things needs to be thought like out param if you need.

public static DataTable ExecuteDataTable(string queryName, CommandType commandType, Dictionary<string,object> parameters=null)
{
    using (OleDbConnection connection =  WorkerDB.GetConnection())
    {
        using (var command=new OleDbCommand())
        {
            connection.Open();
            command.CommandText = queryName;
            command.CommandType = commandType;
            command.Connection = connection;

            if (parameters != null)
            {
                foreach (var parameter in parameters)
                {
                    command.Parameters.AddWithValue(parameter.Key, parameter.Value);
                }
            }

            var dataAdapter = new OleDbDataAdapter(command);
            var dataTable = new DataTable();
            dataAdapter.Fill(dataTable);
            dataAdapter.Dispose();

            return dataTable;
        }
    }
}


File reader and streamer component running in its own thread on Android In an Android project,
I have a class whose job is to read lines from a file,
and then pump those lines to a message handling thread,
looping forever until told to stop.
The main operation of this class (file reading + message sending) must be on its own separate thread,
to run in parallel with whatever else the application is doing.
Multi-threaded programming is not my strongest suit,
so I'd like to reach out to those more knowledgeable than me in this area.

package net.bluetoothviewer;

import android.content.res.AssetManager;

import net.bluetoothviewer.util.AssetUtils;

import java.util.List;

public class MockLineByLineConnector implements DeviceConnector {

    public static final String SAMPLES_SUBDIR = "samples/line-by-line";

    private static final int SLEEP_MILLIS = 1000;

    private final MessageHandler messageHandler;
    private final AssetManager assets;
    private final String filename;

    private boolean running = false;

    public MockLineByLineConnector(MessageHandler messageHandler, AssetManager assets, String filename) {
        this.messageHandler = messageHandler;
        this.assets = assets;
        this.filename = filename;
    }

    @Override
    public synchronized void connect() {
        if (running) {
            return;
        }
        running = true;
        new Thread(new Runnable() {
            @Override
            public void run() {
                messageHandler.sendConnectingTo(filename);

                String mockFilePath = SAMPLES_SUBDIR + "/" + filename;
                List<String> lines = AssetUtils.readLinesFromStream(assets, mockFilePath);

                if (!lines.isEmpty()) {
                    loopLinesUntilStopped(lines);
                }

                messageHandler.sendConnectionLost();
            }

            private void loopLinesUntilStopped(List<String> lines) {
                messageHandler.sendConnectedTo(filename);

                while (running) {
                    for (String line : lines) {
                        if (!running) {
                            break;
                        }
                        messageHandler.sendLineRead(line);
                        try {
                            Thread.sleep(SLEEP_MILLIS);
                        } catch (InterruptedException e) {
                            // ok to swallow, this is our dedicated thread
                        }
                    }
                }
            }
        }).start();
    }

    @Override
    public synchronized void disconnect() {
        running = false;
    }

    @Override
    public void sendAsciiMessage(CharSequence chars) {
        // do nothing
    }
}

Do you spot issues with the way I work with the threads?
The app seems to work perfectly well, but it's not exposed to high-concurrency  scenarios that might trigger the problems caused by badly coded multi-threading.

Do I synchronize too much? Could I relax some of the synchronizations by making the running field volatile?

I'm aware that the AssetUtils.readLinesFromStream call within the synchronized region is suspicious, as it's an "alien method", but it has a trivial responsibility and I can trust it won't cause harm, so I think that can be acceptable in this case.

Using Thread and Runnable is a very low-level way of working with threads.
Can I improve this code by using higher level techniques that are recommended in Android development or in Java in general?

Are there any other improvements you'd recommend?

The class is part of the Bluetooth Viewer Android app, an open source project.
To play with it locally:

Clone the project from GitHub, and switch to tag v7 to get the same version as used in this question.
Build (in Android Studio, for example) and run it on a phone or emulator.
Enable mock devices: open the menu, select Settings, and check Enable mock devices.
Return to the main activity, click on the connect button, and select any of the mock devices.

Lines should start rolling on the screen,
coming from this class under review.

One synchronise to much :

@Override
public synchronized void disconnect() {
    running = false;
}

You are declaring this synchronised.  So if 2 threads are calling this method, the second one has to wait until the first one is finished.
Because you don't have an input variable, the output will always be the same, setting running false. In this case there are no race conditions what could affect the outcome of this method, so I wouldn't make it synchronised.

private static final :

You are making a great use of the private static final fields.
What I'm not liking is the next step :

String mockFilePath = SAMPLES_SUBDIR + "/" + filename;

Why don't you put the "/" also in the SAMPLES_SUBDIR?

It's the only place where you use this field so it could be easily in the SAMPLES_SUBDIR.

Comments :

While I'm not the first to say you have to document your code, the following case is for me strange :

@Override
public void sendAsciiMessage(CharSequence chars) {
    // do nothing
}

You can do 2 things here.

Throwing a UnsupportedOperationException so the next developer knows, this method you need to override is not supported here.
Putting more comment then do nothing, why doesn't this has to do nothing?  When I read that in a project that I have to take over, I'm thinking maybe TODO but why didn't he put TODO?  So I'll be searching for the why and loosing maybe valuable time over a stupid issue.

All over :

Nice readable code, as an outstander, who doesn't know the things the other classes need to do I still understand what this class has to do.


C++ conversion from std::string to arithmetic type using std::locale::classic() I recently had a bug where extracting a decimal from a string failed due to locale settings. That is, some locales use a , as a decimal point, rather than a .. An important goal is that the conversion function is deterministic.

I have usually used boost::lexical_cast for such tasks, but my understanding is that this is reliant on the global application locale. I have therefore implemented a variant of lexical_cast that uses the std::locale::classic "C" locale for the conversion.

#include <type_traits>
#include <locale>
#include <string>
#include <sstream>
#include <stdexcept>

namespace typeconv
{

/**
 * Convert @c str to a T
 *
 * @param str string to convert to a T
 * @return the value contained within @c as a T
 *
 * @pre @c str is arithmetic and can be converted to a T
 * @note std::locale::classic() is used for the conversion.
 *
 * @throws std::invalid_argument if str cannot be converted to an object of type T.
 */
template<typename T>
inline auto lexical_cast(const std::string& str)
    -> typename std::enable_if<std::is_arithmetic<T>::value, T>::type
{
    std::istringstream istr(str);
    istr.imbue(std::locale::classic());

    T val;
    istr >> val;

    if (istr.fail())
        throw std::invalid_argument(str);

    return val;
}

}

I plan to extend the template function in future to include other conversions, such as an arithmetic type to std::string. Any comments on the implementation are welcome.

I see two things I'd change here.

The first is that right now it supports only conversion from string to the specified (arithmetic) type. Sometimes you want (for example) to convert to a string, not just from one. To support that, we can add a second template parameter for the type of the input argument.

My second big point is that you're starting from a problem where the locale to use was hard-wired into boost::lexical_cast. But instead of fixing (what seems to me like) the real problem, you've just hard-wired a different locale.

Instead of that, I'd prefer to specify a locale to use by default, but also allow the user to specify a locale to use for the conversion, if they choose to do so.

Putting those together, we get a function something like this:

template<typename T, typename U>
inline auto lexical_cast(U const& in, std::string const &name = "C")
{
    std::stringstream istr;
    istr.imbue(std::locale(name));
    istr << in;

    std::string str = istr.str(); // save string in case of exception

    T val;
    istr >> val;

    if (istr.fail())
        throw std::invalid_argument(str);

    return val;
}

Now we can do things like:

// Treat as German input
auto x = lexical_cast<double>("1.234,567", "de");

or:

// convert to string in user's locale
auto s = lexical_cast<std::string>(123456.78, "");

Finally, I'd note one basic difference between this and boost::lexical_cast. This throws an exception if and only if it is unable to convert the input to a result of the output type (at all). Boost lexical_cast not only requires that you be able to convert the input to the output type, but also that when the conversion completes that it consume the entire input.

Consider, for example, something like:

int x = lexical_cast<int>("1 x");

With your lexical_cast, this will succeed because it successfully converted the 1 to an int. With Boost lexical_cast, this will throw an exception because even though it converted 1 to an int, that didn't use the x.


Connection to a database, do I need to create a new one and close it everytime? I currenctly have a class which does several requests to a database with several methods (called from main method) sharing this pattern :

public int getSomething(String id, File file) {
    Connection conn = null;
    PreparedStatement ps = null;
    ResultSet rs = null;
    int lines = 0;

    try {
        conn = openConnection();
        ps = conn.prepareStatement(SQL_QUERY.replace("?", id));
        ps.setFetchSize(1500);
        rs = ps.executeQuery();

        lines += nnpw.writeInFile(file, rs);
    } catch (Exception e) {
        e.printStackTrace();
    } finally {
        close(rs, ps, conn);
    }
    return lines;
}

The method openConnection() is in the main class and looks like this :

protected Connection openConnection() throws SQLException, ClassNotFoundException {
    Class.forName(jdbcParams.getString("jdbc.driver"));
    Connection connect;
    connect = DriverManager.getConnection(jdbcParams.getString("jdbc.url"), //$NON-NLS-1$
            jdbcParams.getString("jdbc.username"), //$NON-NLS-1$
            jdbcParams.getString("jdbc.password")); //$NON-NLS-1$
    return connect;
}

Would it be better if I had a Connection parameter in the main class, which I would open and close like this, and modify the close(rs, ps, conn) from above to close(rs, ps) ?

public MainClass {
    Connection conn;

    public static void main(String[] args) {
        try {
            int lines = 0;
            DBExtract ex = new DBExtract();
            ...
            conn = openConnection()
            lines += ex.getSomething("1", file, conn);
            lines += ex.getSomethingElse("1", file, conn);
            lines += ex.getSomethingDifferent("1", file, conn);
            conn.close()
        } catch(Exception e) {
            e.printStackTrace();
        }
    }
}

I have to agree with Praveen's comment that you should open a connection when you need it and close it when your are done with the Database operations.

But first I have to make a serious objection to this statement:

ps = conn.prepareStatement(SQL_QUERY.replace("?", id));

That is not the way you should use prepared statements!

By using that, you are opening yourself to SQL Injection. Consider what would happen if an id something like 'random-useless-name' OR 1 == 1; DROP TABLE sometablename; -- was given. Catastrophic failure!

Instead, use this:

ps = conn.prepareStatement(SQL_QUERY);
ps.setString(1, id);

You might want to take a look at the PreparedStatement tutorial to see how PreparedStatements are normally used. If you are using parameters in the query, use the setXXX methods!

I would say that yes, it is better if you extract the Connection parameter to the main method and provide it to your getSomething method(s). I would use it as a local variable if possible though. Also make sure to put the conn.close(); inside a finally statement and not at the end of the try.

public static void main(String[] args) {
    Connection conn = null;
    try {
        int lines = 0;
        DBExtract ex = new DBExtract();
        ...
        conn = openConnection();
        lines += ex.getSomething("1", file, conn);
        lines += ex.getSomethingElse("1", file, conn);
        lines += ex.getSomethingDifferent("1", file, conn);
    } catch(Exception e) {
        e.printStackTrace();
    } finally {
        if (conn != null)
            conn.close();
    }
}

In this way the caller could also control the transactional state of bulk operations, that is it can decide if and when to commit or to rollback.

Finally, using local variables in place of member variables reduces the chance of having connection leaks, because you really need to be sure to call the "close" method.

Future suggestions:

If you have an application that requires several database connections that should be able to be open simultaneously I recommend using c3p0 Connection Pooling

If you are working on a mid-size to large-size project and want some assistance with the database stuff, I recommend learning Hibernate

C3P0 and Hibernate can be used at the same time, but they require some configuration. They take a while to configure for the first time, but once you got it running they do a lot of things for you. (They're just suggestions though, you should prioritize the other things first!)


MSSQL query to look for duplicate record This query took 6 seconds to complete. How can I optimize it?
Total records in table is 166803.

SELECT ltrim(rtrim(CAST(cageID as nvarchar(max))))+ltrim(rtrim(CAST(trayNo as nvarchar(max)))) as _unique,*
from lf_transit_cage
where ltrim(rtrim(CAST(cageID as nvarchar(max))))+ltrim(rtrim(CAST(trayNo as nvarchar(max)))) in
(
    SELECT dt._unique FROM
    (
        SELECT ltrim(rtrim(CAST(cageID as nvarchar(max))))+ltrim(rtrim(CAST(trayNo as nvarchar(max)))) as _unique
        from lf_transit_cage
    ) as dt
    group by dt._unique
    HAVING COUNT(dt._unique)>1
)
order by cageID,trayNo

As mentioned in the comments, there are benefits to casting/storing that unique key in the table during the ETL process, especially if it's going to be used in other places than just this query.

Most likely, the performance hit is coming from using IN (typically results in a row by row lookup) and from de-duping with the casted key.  You could get a performance gain from JOINing the subequery instead of using IN.  You could also use ROW_NUMBER which, in my experience, is typically more performant than the GROUP BY with HAVING clause.

Here's my example using ROW_NUMBER and CTE's for easier reading:

--Calculate Unique NVARCHAR key
;WITH cte_lf_transit_cage AS (
    SELECT
      ltrim(rtrim(CAST(cageID as nvarchar(max))))+ltrim(rtrim(CAST(trayNo as nvarchar(max)))) as _unique,
      *
    FROM
      lf_transit_cage
)
--Get the Row Count
, cte_rowcount AS (
  SELECT
    _unique,
    ROW_NUMBER() OVER (PARTITION BY _unique ORDER BY cageID, trayNo) AS rowcnt
  FROM
    cte_lf_transit_cage
)
--Grab all instances of duplicate rows
SELECT
  ltc.*
FROM
  cte_lf_transit_cage ltc
WHERE
  EXISTS
    (SELECT unique FROM cte_rowcount rc WHERE rc._unique = ltc._unique AND rc.rowcnt > 1 )
ORDER BY
  ltc.cageID,
  ltc.trayNo

Also, was mentioned in the comments that you may not need to generate the _unique key depending on how the data is stored.  Might compare results to confirm:

--Get the Row Count
;WITH cte_rowcount AS (
  SELECT
    cageID,
    trayNo,
    ROW_NUMBER() OVER (PARTITION BY cageID, trayNo ORDER BY trayNo) AS rowcnt
  FROM
    lf_transit_cage
)
--Grab all instances of duplicate rows
SELECT
  ltrim(rtrim(CAST(ltc.cageID as nvarchar(max))))+ltrim(rtrim(CAST(ltc.trayNo as nvarchar(max)))) as _unique,
  ltc.*
FROM
  lf_transit_cage ltc
WHERE
  EXISTS
    (SELECT * FROM cte_rowcount rc WHERE rc.cageID = ltc.cageID AND rc.trayNo = ltc.trayNo AND rc.rowcnt > 1 )
ORDER BY
  ltc.cageID,
  ltc.trayNo


How can I improve this Node.js file's routing? Being new to Node.js, I'd like to know of a better way of implementing this:

server = http.createServer( function(req, res) {
if (req.url === '/') {
        fs.readFile('index.html', function(err, page) {
            res.writeHead(200, {'Content-Type': 'text/html'});
            res.write(page);
            res.end();
        });
}
 else if (req.url == '/new.html') {
        fs.readFile('new.html', function(err, page) {
                res.writeHead(200, {'Content-Type': 'text/html'});
                res.write(page);
                res.end();
        });
}
});
server.listen(8080);

Using something like this:

var Url = require('url')
...

// In request handler
var uri = Url.parse(req.url);
switch (uri.pathname) {
  case "/":
    ..
  case "/new.html":
    ..
}

Whether you wanna implement a static server?
If so, you can just use connect and its built-in static middlewave. With it, you just need to write the following code to implement a static server:

var connect = require("connect");

connect(
    connect.static(__dirname)
).listen(3000);

That's it.


Parallel computation to be merged, depending on single call Steps:

Get AMOUNT from a call to API endpoint /a, e.g. AMOUNT == 5 (in the code it is set to 2)
Use this AMOUNT to make AMOUNT calls to endpoint /b receiving JSON

Result might be: [{a: 1, b: 2}, {...}, {...}, {...}, {...}]

Use this AMOUNT to make AMOUNT calls to endpoint /c receiving JSON

Result might be: [{c: 3, d: 4}, {...}, {...}, {...}, {...}]

Merge such that there is a resulting array of JSON objects

Result might be: [{a:1, b:2, c: 3, d: 4}, {...}, {...}, {...}, {...}]

So pretty much it looks like this:

          a
         / \
        b   c
         \ /
        result

I'm really trying to understand how to write nice NodeJS code (coming from Python), so I'm specifically looking for the most elegant way to do this. I hope people can give advice on my current code below, as well as perhaps just suggesting different though clearly better methods. E.g. doing everything in async but still merging into the same result.

I'm also interested in which of the 3 styles of options (1/2/3) is best for writing NodeJS apps.

Promise = require("bluebird");
u = require("underscore")

function get_phase_count() {
    return new Promise(function(resolve, reject) {
        resolve(2); // AMOUNT == 2
    });
}

function get_phase_info(amount) {
    console.log(amount) // this would be used but now it is mocked
    return new Promise(function(resolve, reject) {
        // note that I got this AMOUNT calls solved to deliver this
        resolve([ { a: 1, b: 2 }, { a: 1, b: 2 } ]);
    });
}

function get_phase_votes(amount) {
    console.log(amount) // this would be used but now it is mocked
    return new Promise(function(resolve, reject) {
        // note that I got this AMOUNT calls solved to deliver this
        resolve([ { c: 3, d: 4 }, { c: 3, d: 4 } ]);
    });
}

function join_phase_promises(p1, p2) {
    return Promise.all([p1, p2]);
};

Using those functions I wrote 3 style options:

// Option 1
get_phase_count().then(function(amount) {
    join_phase_promises(get_phase_info(amount), get_phase_votes(amount)).then(function(args) {
        console.log(u.map(u.zip(args[0], args[1]), function(x) {return u.extend(x[0], x[1])}));
    })
});

// Option 2
get_phase_count()
.then(function(amount) {
    return join_phase_promises(get_phase_info(amount), get_phase_votes(amount));
})
.then(function(args) {
    console.log(u.map(u.zip(args[0], args[1]), function(x) {return u.extend(x[0], x[1])}));
})

// Option 3
get_phase_count().then(function(amount) {
    return join_phase_promises(get_phase_info(amount), get_phase_votes(amount));
}).then(function(args) {
    console.log(u.map(u.zip(args[0], args[1]), function(x) {return u.extend(x[0], x[1])}));
})

Option 1 defeats the whole purpose of Promises. You are nesting the callbacks. Option 2 and 3 are not different, just how they're formatted. That's up to personal preferences.

Now going with the fact that get_phase_count, get_phase_info and get_phase_votes are wrapper functions to API calls, let's not touch those. That leaves us with join_phase_promises. join_phase_promises is unnecessary as it's just a wrapper for Promise.all which you can simply use directly.

Now over to code styles, JS follows camelCase convention. This makes your function names, which are underscore-cased, look like outcasts. Suggesting you use camelCase instead.

If you happen to use Node.js 4+ (current version is 5.7), the native Promise is already available. No need to use Bluebird for that. You can also start using arrow functions to make your callbacks a bit more compact. Lastly, JS has a bunch of array operations and recently included destructuring. There's little need to use underscore.

If I were to write your function, it would go like

getPhaseCount().then(amount => Promise.all([
  getPhaseInfo(amount),
  getPhaseVotes(amount)
])).then(args => {
  let merged = args.reduce((p,c) => c.map((a, i) => ({...a, ...p[i]}) ), []);
  console.log(merged);
});

The above logic took me a while to figure out using only native operations, but what it does is to allow us to see the previous and current values using reduce. This way, on each item, map essentially creates a new array containing a merged version of the previous and current.


Parser for STL stereolithography data files I'm parsing STL files to load the coordinates. My code is:

void Importer::scanFileSTL_ASCII(
    QVector<QVector3D> &vertices, // Coordinates to be filled
    const QUrl &url)
{
    QFile file;
    file.setFileName(url.toLocalFile());
    if (!file.open(QIODevice::OpenModeFlag::ReadOnly | QIODevice::OpenModeFlag::Text))
        return;

    while (!file.atEnd()) {
        QByteArray line_BA = file.readLine();
        QString line = QString::fromLatin1(line_BA);
        // Split line by spaces
        QStringList words = line.split(QRegExp("\\s+"), QString::SkipEmptyParts);
        if (words.isEmpty())
            continue; // Skip this line and jump to next iteration/line
        if (words[0] == "vertex") {
            assert (words.length() == 4 && "STL file format is wrong: vertex-line must have 4 strings");
            float x = words[1].toFloat();
            float y = words[2].toFloat();
            float z = words[3].toFloat();
            vertices.append(QVector3D(x, y, z));
        }
    }

    return;
}

The code is tested, it works fine. But it is by far slower than the STL loader of asset import library. More accurate comparisons in release mode:

Import of a 13MB ASCII STL file:

Assimp library: 03.58 seconds
Our code: 04.70 seconds

Import of 287MB ASCII STL file:

Assimp library: 05.72 seconds
Our code: 44.19 seconds

I wonder if something is noticeably wrong with my code. Thanks.

Well, it's not much of a mystery.  Your code is simple and pretty. Their code is ugly because it performs exactly what it needs to do, and no more:

} else if (!strncmp(sz,"vertex",6) && ::IsSpaceOrNewLine(*(sz+6))) { // vertex 1.50000 1.50000 0.00000
    if (faceVertexCounter >= 3) {
        ASSIMP_LOG_ERROR("STL: a facet with more than 3 vertices has been found");
        ++sz;
    } else {
        if (sz[6] == '\0') {
            throw DeadlyImportError("STL: unexpected EOF while parsing facet");
        }
        sz += 7;
        SkipSpaces(&sz);
        positionBuffer.push_back(aiVector3D());
        aiVector3D* vn = &positionBuffer.back();
        sz = fast_atoreal_move<ai_real>(sz, (ai_real&)vn->x );
        SkipSpaces(&sz);
        sz = fast_atoreal_move<ai_real>(sz, (ai_real&)vn->y );
        SkipSpaces(&sz);
        sz = fast_atoreal_move<ai_real>(sz, (ai_real&)vn->z );
        faceVertexCounter++;
    }

A few differences in the amount of work it does:

There's no regex that needs to be compiled.
It's not splitting tokens into a list.  So, no list needs to be allocated and filled.  More importantly, no strings need to be allocated, copied, and destroyed.
The aiVector3D is allocated first, then each of the three numbers is parsed and written directly into the memory location of the coordinate, with no temporary variable assignments.

I'll also point out that your use of assert is improper.  You should only assert conditions that you know must be true due to logic.  You should not assert conditions that you hope to be true about the input.  If you compile with assertions disabled, then the validation code goes away!  That would introduce a bug, and possibly a security vulnerability.


Implement `Applicative Maybe` Typeclassopedia presents the following exercise:

  Maybe can easily be made an instance of Applicative; writing such an instance is left as an exercise for the reader.

Here's my implementation:

data Option a = Some a
                | None deriving Show

instance Functor Option where
   fmap _ None     = None
   fmap f (Some x) = Some (f x)

instance Applicative Option where
   pure x                = Some x
   (Some f) <*> (Some x) = Some (f x)
   _ <*> _               = None

How does this look? In particular, I'm curious if the second line of the Applicative Option looks good.

Should I be using fmap on the right-hand side instead of Some (f x)?

Looks good, follows the laws of Applicative AFAICT, and I'd leave it like this, with fmap you'd need to write more.


First all zero row of a matrix in C++ The problem of finding the first all zero row in a NxN matrix was featured in a letter to the CACM in 1974 by Frank Rubin who claimed it was a problem for which using a GOTO was better than alternatives.

But many languages have labeled break statements and other features that make the problem trivial. In fact, in JavaScript we need only write:

a.findIndex(row => row.every(x => x === 0))

which gives us the index of the first nonzero row or -1 if no such row exists. I'm interested in implementing this functional approach in C++ 14. As I don't have a huge amount of C++ experience, I came up with:

#include <iostream>
#include <vector>
#include <algorithm>
#include <cassert>
using namespace std;

bool isAllZeroRow(const vector<int>& row) {
  return all_of(row.begin(), row.end(), [](int x){return x==0;});
}

int firstAllZeroIndex(const vector<vector<int>>& m) {
  return distance(m.begin(), find_if(m.begin(), m.end(), isAllZeroRow));
}

vector<vector<int>> matrix1 = {{1, 2}, {0, 0}, {1, 1}};
vector<vector<int>> matrix2 = {{0, 0, 1, 2}, {3, 0, 0}, {0, 0, 0}};
vector<vector<int>> matrix3 = {{0, 0, 1, 2}, {3, 0, 0}, {0, 0, 5}};

int main() {
  assert(firstAllZeroIndex(matrix1) == 1);
  assert(firstAllZeroIndex(matrix2) == 2);
  assert(firstAllZeroIndex(matrix3) == 3); // not found
  cout << "ok\n";
}

I'm interested in hearing things like:

whether the lambda is okay, or should it be a named function or a C++ unary predicat
Are distance and find_if best expressed with iterators like I have done here?
Was it right to use the helper function, or is there a way in C++ to make things almost as concise as the JavaScript solution?

I know how to do this function with nested loops with a return in the inner loop, but I'm intrigued by trying out the functional forms and was wondering how I did.

avoid importing namespaces

You have using namespace std;. You are obviously working in a .cpp where it is usually fine to do this; but I (and many others) don't like to remove the std::. It is short enough to write it every time you are trying to access a function/class or whatever of the std scope. Use it; it makes things clearer (at least for me ;))

use free functions over member functions

Herb Sutter described it nice in one of his articles; please forgive me that I am not able to provide you with a short text but on stackoverflow there are plenty of topics like this.
E.g. click me

It is more generic to use the std::begin and std::end functions than the container specific one. Technically it is totally the same; it just makes it easier to change the container type.

keep things local

I think you know that you should keep your objects as local as possible. I don't see a good reason here to declare your matrix objects in the global scope.

lambdas

The lambdas are fine. You could use a lambda for the isAllZeroRow() function instead of declaring a free function for this, but this is also fine.

You can declare the lambda parameters as auto. You don't have to define types explicitly anymore. This safes us from some annoying typing and redundancy and - one of my most loved arguments - it's more generic.

You could also delete the other free function, too, and just create a local lambda object.

int main() {
    std::vector<std::vector<int>> matrix1 = {{1, 2}, {0, 0}, {1, 1}};

    auto firstAllZeroIndex = [](const auto& m) {
        return std::distance(std::begin(m), std::find_if(std::begin(m), std::end(m), [](const auto& row){
            return std::all_of(std::begin(row), std::end(row), [](int x){return x==0;});
        }));
    };
    assert(firstAllZeroIndex(matrix1) == 1);
}

more meaningful return type

You should use a return type which offers you the possibility to return an invalid statement. In c++17 we'll get the comfortable type std::optional, but for now we can use a std::pair<int, bool> statement. In this case, the int of the type has a valid value as long as the bool of the pair is true. I recommend this, because it makes the code more self-descriptive.

some praises

You are using the c++ headers; <cassert> is the right one, as every other header with the c prefix and without the .h. Good job :D

From my point of view your solution looks almost perfect (at least with the given requirements). You should be aware of the fact, that in this example you could use std::array types instead of std::vector and that it could be possible to resize your matrix objects which could lead to different row sizes in one matrix object. In the end I would write a class for this, to prevent me (and others) from doing such dumb stuff and getting frustrated ;)


Implementing a thread-safe LRUCache Here is the problem I've been trying to tackle:

  Design a thread-safe image caching server that can keep in memory only the ten most recently used images.

I chose to implement an LRU cache to solve this as follows:

'''
This module defines an LRUCache.

Constraints:

1. May only hold upto ten items at a time.
2. Must be able to synchronise multiple requests.
3. Must be able to update its cache.
'''

import random
import threading

class LRUCache(object):

    def __init__(self, cacheSize=10):

        # this variables maps image names to file locations
        self.Cache = {}

        # this variables maps image names to timestamp of last request
        # filed for them. Is volatile - requires local clock to never change,
        # or that there is a separate clock service that guarantees global
        # consistency.
        self.RequestTimestamps = {}

        self.CacheSize = cacheSize

    def insert(self, imageName, imageLocation):

        '''
        Insert a new image into our cache. If inserting would exceed our cache size,
        then we shall make room for it by removing the least recently used.
        '''

        with threading.Lock():

            if len(self.Cache) == self.CacheSize:
                self.removeLeastRecentlyUsed()

        self.Cache[imageName] = imageLocation
        self.RequestTimestamps[imageName] = time.time()

    def get(self, imageName):

        '''
        Retrieve an image from our cache, keeping note of the time we last
        retrieved it.
        '''

        with threading.Lock():

            if imageName in self.Cache:

                self.RequestTimestamps[imageName] = time.time()
                return self.Cache[imageName]

            else:

                raise KeyError("Not found!")

    def removeLeastRecentlyUsed(self):

        '''
        Should only be called in a threadsafe context.
        '''

        if self.RequestTimestamps:

            # scan through and find the least recently used key by finding
            # the lowest timestamp among all the keys.

            leastRecentlyUsedKey = min(self.RequestTimestamps, key=lambda imageName: self.RequestTimestaps[imageName])

            # now that the least recently used key has been found,
            # remove it from the cache and from our list of request timestamps.

            self.Cache.pop(leastRecentlyUsedKey, None)
            self.RequestTimestamps.pop(leastRecentlyUsedKey, None)
            return

        # only called in the event requestTimestamps does not exist - randomly
        # pick a key to erase.

        randomChoice = random.choice(self.Cache.keys())
        self.Cache.pop(randomChoice, None)

Questions:

Is this actually threadsafe?

I'm used to implementing locks on single resources, but not class
methods - would multiple threads, one accessing insert and the
other doing a get be forced to synchronised as well?

Or is my strategy of creating different locks in each method only able to prevent concurrent individual insert requests?

Ignore the GIL.
What would be a good strategy to test this code?

I have thought of the obvious strategy:

a) Insert ten items, make nine requests, attempt to insert a tenth
   and check if the item that was removed was the tenth.

but I am not sure if this is the only way or if there is a better way to implement it.

Thread-safeness

As others have pointed out in the comments, your implementation is not thread-safe. threading.Lock() returns a new lock each time it is called, so each thread will be locking a different lock. Instead, you should have a single lock as an instance member object:

def __init__(self, cacheSize=10):

    # this variables maps image names to file locations
    self.Cache = {}

    # this variables maps image names to timestamp of last request
    # filed for them. Is volatile - requires local clock to never change,
    # or that there is a separate clock service that guarantees global
    # consistency.
    self.RequestTimestamps = {}

    self.CacheSize = cacheSize

    self.lock = threading.Lock()

Then, you use it like so:

def insert(self, imageName, imageLocation):

    '''
    Insert a new image into our cache. If inserting would exceed our cache size,
    then we shall make room for it by removing the least recently used.
    '''

    with self.lock:

        if len(self.Cache) == self.CacheSize:
            self.removeLeastRecentlyUsed()

    self.Cache[imageName] = imageLocation
    self.RequestTimestamps[imageName] = time.time()

Additionally, using time.time() for access orders can cause inconsistent results: it's not guaranteed to have good precision, and is dependent on the system clock steadily increasing. If the system clock is manually set back, you lose your consistent ordering. Instead, a safer way would be to use an OrderedDict, where you remove and re-insert items as they are accessed, and use OrderedDict.popitem(False) to remove the least-recently inserted item.

PEP8 compliance

Your variables and methods are written with a mixture of PascalCase (Cache.RequestTimestamps), which is typically only used for class names, and camelCase (Cache.removeLeastRecentlyUsed, leastRecentlyUsedKey), which is typically not used in Python. You should use snake_case instead for variables and methods, and use it consistently:

def __init__(self, cache_size=10):

    # this variables maps image names to file locations
    self.cache = {}

    # this variables maps image names to timestamp of last request
    # filed for them. Is volatile - requires local clock to never change,
    # or that there is a separate clock service that guarantees global
    # consistency.
    self.request_timestamps = {}

    self.cache_size = cache_size

def remove_least_recently_used(self):

    '''
    Should only be called in a threadsafe context.
    '''

    if self.request_timestamps:

        # scan through and find the least recently used key by finding
        # the lowest timestamp among all the keys.

        least_recently_used_key = min(self.request_timestamps, key=lambda image_name: self.request_timestamps[image_name])

        # now that the least recently used key has been found,
        # remove it from the cache and from our list of request timestamps.

        self.cache.pop(least_recently_used_key, None)
        self.request_timestamps.pop(least_recently_used_key, None)
        return

    # only called in the event requestTimestamps does not exist - randomly
    # pick a key to erase.

    random_choice = random.choice(self.cache.keys())
    self.cache.pop(random_choice, None)

And while we're looking at this method...

Single exit only

While there are many arguments against the single-exit-only style, none of them apply here. There's no good reason to have the return inside of the if in Cache.removeLastRecentlyUsed. Instead, wrap the rest in an else:

if self.request_timestamps:

        # scan through and find the least recently used key by finding
        # the lowest timestamp among all the keys.

        least_recently_used_key = min(self.request_timestamps, key=lambda image_name: self.request_timestamps[image_name])

        # now that the least recently used key has been found,
        # remove it from the cache and from our list of request timestamps.

        self.cache.pop(least_recently_used_key, None)
        self.request_timestamps.pop(least_recently_used_key, None)
else:

    # only called in the event requestTimestamps does not exist - randomly
    # pick a key to erase.

    random_choice = random.choice(self.cache.keys())
    self.cache.pop(random_choice, None)


A wrapper for user authentication using cURL in PHP So I created a wrapper class to authenticate user via POST HTTP request.
    class Nucleolus
    {
        public const AUTHEN_USER_URL = "localhost:8000/authen/user";

        protected $ch;

        public function __construct(string $token)
        {
            $this->ch = curl_init();
            curl_setopt($this->ch, CURLOPT_URL, self::AUTHEN_USER_URL);
            curl_setopt($this->ch, CURLOPT_HTTPHEADER, [
                "Content-Type: application/json",
                "Authorization: Bearer " . $token,
            ]);
            curl_setopt($this->ch, CURLOPT_POST, 1);
            curl_setopt($this->ch, CURLOPT_RETURNTRANSFER, true);
        }

        public function authenticate(string $username, string $password, string $serverUrl = null)
        {
            $url = $serverUrl ? $serverUrl : $_SERVER['SERVER_NAME'];
            curl_setopt($this->ch, CURLOPT_POSTFIELDS, json_encode(compact('url', 'username', 'password')));

            return json_decode(curl_exec($this->ch), true);
        }

        public function __destruct()
        {
            curl_close($this->ch);
        }
    }

Then the other developers can do the following.
    $nucleolus = new Nucleolus($token);
    $result = $nucleolus->authenticate($username, $password);

and retrieve result as a PHP array.
Please review.

Let me be frank: this code is just a Frankenstein sewn from different unrelated parts. Basically here are two main problems:

sending HTTP requests is a distinct task and should be performed by another wrapper
splitting the main functionality between two methods just makes no sense. I do understand that you probably wanted to make use of constructor, but that's not how it is done

The HTTP request class
Basically there must be a class that does HTTP requests with methods to set particular settings, such as

setAuth(new BearerAuth($token))
setContentType()
setUrl()
etc

The error checking
Also, in this class, it is important to check for errors. Right now your code returns whatever is sent by the other party. But it is not necessarily would be JSON of a particular format. It could be HTML, JSON that contains some different data, an empty string, etc.

First of all the code must check for the curl_error. In case there is, it must throw an Exception
Then it must try to decode JSON. If it fails it must throw an Exception
Then it must check if the decoded data contains expected information. If it doesn't,  it must throw an Exception
only then it must return the data

The actual class
And then there could be a class to interact with a particular API, something like this
class Nucleolus
{
    protected $transport;
    protected $host;

    public function __construct(string $host, string $token)
    {
        $this->transport = new HTTPTransport;
        $this->transport->setAuth(new BearerAuth($token));
        $this->transport->setContentType('application/json');
    }

    public function authenticate(string $username, string $password, string $serverUrl = null)
    {
        $this->transport->setUrl($this->host."/authen/user");
        $result = $this->transport->post([
            'url' => $serverUrl ? $serverUrl : $_SERVER['SERVER_NAME'],
            'username' => $username,
            'password' => $password,
        ]);
        $data = JSON::decode($result);
        if ($data === true)
            return true;
        } else {
            throw new NucleolusException("Incorrect data from auth: $result");
        }
    }
}

Destructor is not really needed as PHP will destruct everything itself.


Modeling a capacitor with DC Bias I am modeling a capacitor which has its capacitance varying according to the DC Bias. The DC Bias is computed by taking the mean of the voltage across the capacitance.

class CAP:
    """
    Represents the Capacitor.
    """
    def __init__(self, nominal_capacitance):
        # Theoretical capacitance without DC Bias (i.e. voltage mean at 0)
        self.nominal_capacitance = nominal_capacitance # uF
        # Actual capacitance, initialized at the nominal value
        self.capacitance = nominal_capacitance # uF

        # Voltage control loop settings
        self.set_voltage_value = None # Value set in mV
        self.set_lower_voltage_window_bound = None # Window upper bound in mV
        self.set_upper_voltage_window_bound = None # Window lower bound in mV
        self.voltage = 0 # Voltage across the capacitor
        self.being_charge = False # Is it being charged/discharged by the control loop?

        # Measurements
        self.charge_counter = 0
        self.discharge_counter = 0

        self.voltage_charged = 0
        self.voltage_discharged = 0

        # List of the voltage across the capacitance
        self.timeline = [] # mV

    def update_capacitance(self):
        """
        Value according to the DC Bias characterstic approximation.
        """
        global timelines_resolution # time step between 2 voltages measurements in the list self.timeline

        DC_bias = np.mean(self.timeline) / 1000
        change_perc = (-0.0008*(DC_bias**4)+0.0474*(DC_bias**3)-0.7953*(DC_bias**2)-0.5447*DC_bias-0.4847)/100
        self.capacitance = self.nominal_capacitance * (1+change_perc)

The voltage across the capacitance is controlled via a control loop which will recharge or discharge the capacitor. The goal is to keep its voltage within boundaries (window).

The attribute self.timeline will have one measure point (voltage) added every timelines_resolution. I can get between 200 000 points to a few millions.

The conversion to a numpy array and the mean computation become quite long. On the otherhand, it is handy to work with a list and its append method since I do not know beforehand the number of point that will be measured.

Do you see any other way to compute this DC Bias or to make this faster?
At the moment, I call the function update_capacitance every 25000 points. I would like to increase this resolution.

Computing the mean of 25000 values is summing up the values and dividing by 25000.
Computing the mean of 50000 values (the original 25000 plus 25000 new values) is summing up the values and dividing by 50000.  This means (no pun intended) you are adding up the first 25000 values over and over again each time you compute the mean.  And adding up 25000 values takes time.  As the length of the list grows, the time it takes to sum it grows as well.  Total complexity: \$O(n^2)\$

You could create a running total, and compute the mean yourself.

class CAP:

    def __init__(self, ...):
        ....
        self._sum = 0
        self._count = 0

    def update_capacitance(self):
        ...
        self._sum += sum(self.timeline[self._count:])   # Only add in the new samples
        self._count = len(self.timeline)
        DC_bias = (self._sum / self._count) / 1000

Feel free to use np.sum() if it is faster, or to ensure the required precision in the sum.

Appending values one-at-a-time to timeline is itself a time consuming process.  If you know you are accumulating samples in blocks of 25000 points, you could pre-allocate a buffer of 25000 points with np.empty(), and fill that in.  When it is full, after summing this small block into the running total, you could np.concatenate() to a larger timeline array.  Or append it to a list of buffers, and created a new buffer for the next block, and concatenate all the blocks together at the end.

You could also create a list of the means of these smaller buffers, and compute the mean of that list.  This may help avoid precision issues you would encounter in totaling a few million points.


Continuous sequence against target number Post problem and my code written in Python 2.7. Any advice for code bugs, performance in terms of algorithm time complexity and code style are appreciated. I did PEB8 self-check in PyCharm.

BTW, a small issue in my code is, I feel this condition end_index == len(numbers) - 1 and cur_sum < target is a bit ugly, but I do not find a better way to remove it for safe boundary check purpose.

'''
Question: Given a sequence of positive integers A and an integer T, return whether there is a continuous sequence of A that sums up to exactly T
Example
[23, 5, 4, 7, 2, 11], 20. Return True because 7 + 2 + 11 = 20
[1, 3, 5, 23, 2], 8. Return True because 3 + 5 = 8
[1, 3, 5, 23, 2], 7 Return False because no sequence in this array adds up to 7
'''

def sum_target(numbers, target):
    start_index = 0 # inclusive
    end_index = 0 # inclusive
    cur_sum = numbers[0]
    while end_index < len(numbers):
        if end_index < len(numbers) - 1 and cur_sum < target:
            end_index += 1
            cur_sum += numbers[end_index]
        elif end_index == len(numbers) - 1 and cur_sum < target:
            return False
        elif cur_sum > target:
            cur_sum -= numbers[start_index]
            start_index += 1
        elif cur_sum == target:
            return True

    return False

if __name__ == "__main__":
    print sum_target([23, 5, 4, 7, 2, 11], 20)
    print sum_target([1, 3, 5, 23, 2], 8)
    print sum_target([1, 3, 5, 23, 2], 7)

In case there's no acceptable sequence, you have to go through all the elements anyway, so it can't be lower than O(n), and your code is already O(n).

The only optimization that you could add is a condition where if the current number alone is greater than the expected sum, you skip all values until the one just after the current value.

For instance, suppose you have [2, 3, 4, 25, 7, 8, 9] and you're looking for number 17. You don't want to check 2 + 3 + 4 + 25, 3 + 4 + 25, 4+25, you can skip directly to the index of 7.

Other than that I'd just remove the comments because I don't think they really add anything.


Wordcloud from all answers of a user here on CR Since I haven't really used Python's new async features yet, I took some older code of mine, which took all of my answers here on Code Review and generated a word cloud from them, and updated it to fetch the pages in an asynchronous way.

This script uses the py-stackexchange package for the API (don't forget to get your API key in order to increase the number of requests you can make to 10k). There are other packages for the API out there, but this one is easy to use IMO, especially for getting all questions/answers of one particular user. However, unfortunately (or luckily for me) it does not support getting the body of an answer (only of a question). So that part is done with aiohttp and BeautifulSoup, which is where the asynchronous part comes in. The text is split into words using nltk and the word cloud is generated via wordcloud.

To install everything:

$ pip install aiohttp bs4 lxml matplotlib nltk py-stackexchange wordcloud
$ python
>>> import nltk
>>> nltk.download('punkt')

Any and all feedback, especially on the use of the async stuff, is welcome. Maybe I should've split up fetching the page and processing it more? Maybe I missed some important performance trick?

import aiohttp
import asyncio
from bs4 import BeautifulSoup, SoupStrainer
from itertools import chain
import matplotlib.pyplot as plt
from nltk.tokenize import word_tokenize

import stackexchange
from wordcloud import WordCloud

API_KEY = '**redacted**'    # https://meta.stackexchange.com/q/261829/342577
CR = stackexchange.Site("CodeReview", API_KEY)

STRAINER = SoupStrainer(
    'div', attrs={'class': ['answer', 'answer accepted-answer']})

async def fetch(session, url, answer_id):
    async with session.get(url) as response:
        page = await response.text()
    soup = BeautifulSoup(page, "lxml", parse_only=STRAINER)
    try:
        answer_text = soup.select_one(
            f'div#answer-{answer_id} div.post-text').text
    except AttributeError:
        print("Failure:", url)
        return []
    else:
        print("Success:", url)
        return word_tokenize(answer_text)

async def fetch_all(urls, answer_ids):
    async with aiohttp.ClientSession() as session:
        jobs = [fetch(session, url, answer_id)
                for url, answer_id in zip(urls, answer_ids)]
        results = await asyncio.gather(*jobs)
    return results

if __name__ == "__main__":
    user = CR.user(98493)   # that's me
    user.answers.fetch()  # needed to initialize it...

    urls = (answer.url.replace(CR.domain, "codereview.stackexchange.com")
            for answer in user.answers)
    answer_ids = (answer.id for answer in user.answers)

    loop = asyncio.get_event_loop()
    words = list(chain.from_iterable(
        loop.run_until_complete(fetch_all(urls, answer_ids))))

    plt.figure()
    wordcloud = WordCloud(width=480, height=480, colormap="Blues")
    wordcloud = wordcloud.generate(" ".join(words))
    plt.imshow(wordcloud, interpolation="bilinear")
    plt.axis("off")
    plt.margins(x=0, y=0)
    plt.show()

The image produced by this code looks something like this.
Seems like I define, and talk about, a lot of functions...

Quick bits

You have some issues that some linters would pick up:

I would suggest moving your main code into a function. So that it doesn't pollute the global namespace.
You've got some trailing whitespace.
Add some docstrings to your code. Even something basic like "Fetch words in answers."
Your imports are kinda all over the place. I can't make any sense of them, and so I think they're just randomly placed there as and when you needed them.
I don't think print is the best tool for logging. I would suggest using logging.

Async

I'm not a fan of your current fetch and fetch_all functions. I would prefer it if fetch only called session.get. This may seem strange, but it means that you can change your code to allow for caching of objects or easier logging.

Given that I've not done any of this I've left it returning just a plain RequestContextManager. However if I were to expand on this I would change it to my own custom class. This is because then you can keep the data you want / need such as the page body in a cache. Using your own class also means that can guarantee values will exist, and hide ones you can't guarantee.

Moving the content of the old fetch into a fetch_all_words allows almost the exact same code, and allows us to build the word list without the use of a convoluted itertools and asyncio one-liner.

Interestingly since the majority of the content of the fetch_all_words function is not async code, there is little to no performance difference between using asyncio.as_completed and asyncio.gather. In a small test function I found that asyncio.as_completed performs as well or better than asyncio.gather.

Finally I make main an async function, as calling asyncio.run(main()) is simpler than building a loop and running until completion.

"""Stack Exchange word cloud generator."""
import asyncio
import logging
import itertools
import operator

import aiohttp
import bs4
import nltk.tokenize
import matplotlib.pyplot as plt
import stackexchange
import wordcloud

API_KEY = '**redacted**'
CR = stackexchange.Site("CodeReview", API_KEY)

STRAINER = bs4.SoupStrainer(
    'div',
    attrs={'class': ['answer', 'answer accepted-answer']}
)

async def fetch(session, url):
    return url, await session.get(url)

async def fetch_all(urls):
    async with aiohttp.ClientSession() as session:
        tasks = [fetch(session, url) for url in urls]
        for task in asyncio.as_completed(tasks):
            yield await task

async def fetch_all_words(answers):
    words = []
    async for url, resp in fetch_all(answers):
        answer_id = answers[url]

        async with resp as response:
            page = await response.text()
        soup = bs4.BeautifulSoup(page, "lxml", parse_only=STRAINER)
        answer = soup.select_one(f'div#answer-{answer_id} div.post-text')
        try:
            answer_text = answer.text
        except AttributeError:
            logging.error(url)
            answer_words = []
        else:
            logging.info(url)
            answer_words = nltk.tokenize.word_tokenize(answer_text)
        words.extend(answer_words)
    return words

async def main():
    """Main code."""
    logging.getLogger().setLevel(logging.INFO)

    user = CR.user(42401)
    user.answers.fetch()

    answers = {
        answer.url.replace(CR.domain, "codereview.stackexchange.com"): answer.id
        for answer in user.answers
    }
    words = await fetch_all_words(answers)

    plt.figure()
    wc = wordcloud.WordCloud(width=480, height=480, colormap="Blues")
    wc = wc.generate(" ".join(words))
    plt.imshow(wc, interpolation="bilinear")
    plt.axis("off")
    plt.margins(x=0, y=0)
    plt.show()

if __name__ == "__main__":
    asyncio.run(main())

Additional comments

The code heavily violates the SRP principle. Given that this is, I assume, an untested one-off script this doesn't matter much.

However in the future I think the changes to fetch_all makes fetch_all_words easier to split up to achieve this.
I have butchered your style.
This may be hard to believe but I rewrote the code around three times. I've not changed much, but I don't think much needs to be changed. I mostly focused on trying to get fetch_all and fetch a way I like.

Whilst I like my style more, it's not intended to be some subtle hint yours is bad.
You have a bug apparently "n't", "ll", "n't use" and "ca n't" are words I commonly say.
Also, "n't" appears on your image too.
Thanks for posting this, it was a fun little puzzle. First time I'd really looked into asyncio too!


Compress an ASCII string Given an ASCII string create a 'compressed' version of it. For example, aabccccddddddde would output a2bc4d7e whereas abc would output abc.

This is how I've implemented it. Is there a smarter way to do it without the need for two for loops?

import java.util.LinkedHashMap;
import java.util.Set;

public class CompressString {

  private static String compressString(String s) {
    LinkedHashMap<Character, Integer> charCountMap = createCharacterCountMap(s);

    StringBuilder result = new StringBuilder();
    Set<Character> keySet = charCountMap.keySet();
    for(Character c: keySet) {
      int val = charCountMap.get(c);
      if(val == 1) {
        result.append(c);
      } else {
        result.append(c.toString() + val);
      }
    }
    return result.toString();
  }

  private static LinkedHashMap<Character, Integer> createCharacterCountMap(String s) {
    char[] charArray =  s.toCharArray();
    int[] charCountArray = new int[128];
    LinkedHashMap<Character, Integer> charCountMap = new LinkedHashMap<Character, Integer>();
    for(int i=0; i<charArray.length; i++) {
      int intVal = charArray[i];
      charCountArray[intVal]++;
      charCountMap.put(charArray[i], charCountArray[intVal]);
    }
    return charCountMap;
  }

  public static void main(String[] args) {
    String s1 = "aabccccddddddde";
    String s2 = "abc";
    System.out.println(compressString(s1));
    System.out.println(compressString(s2));
  }

}

I'm commenting on the edited version only. It's fine, but

char cur = s.charAt(0);

throws for the empty string. You could special case it, but in general, it's better to do it the other way: Maintain a state containing a lastCharacter and a count. The initial state has count=0 (lastCharacter is initially arbitrary).

StringBuilder result = new StringBuilder();

You could pre-size it using s.length as it's a sure upper bound. Not very important except maybe for long strings.

 result.append(cur);

Your approach needs this action at the beginning, while mine needs it at the end. You need also a complicated condition like if((i == s.length() - 1) && (count > 1)) which I don't want to understand.

for(int i=1; i<s.length(); i++) {
  char nxt = s.charAt(i);

Saving a single character compared to next is not worth it, is it?

You should also fix your spacing, especially add a space after for and if. Use your IDE.

I'd propose something like (my spacing is not conform, please ignore)

private static String compressString(String s) {
    StringBuilder result = new StringBuilder();
    int count = 0;
    char last = ' '; // doesn't matter
    for (int i=0; i<s.length(); i++) {
        char c = s.charAt(i);
        if (count==0) {
            last = c;
            count = 1;
        } else if (c==last) {
            count++;
        } else if (count==1) {
            // While this ensures the string never grows,
            // it also creates the ambiguity mentioned below.
            result.append(last);
            last = c;
        } else {
            result.append(last).append(count);
            last = c;
            count = 1;
        }
    }
    if (count>0) {
        result.append(last).append(count);
    }
    return result.toString();
}

Note that whenever the string contains digits, it gets irreversibly mangled:

`11` -> `12`
`12` -> `12`


Conditionally replacing a placeholder in a form Given this code

if account_id:
    form = form.replace("<<ACCOUNT_ID>>", "<accountId>"+account_id+"</accountId>")
else:
    form = form.replace("<<ACCOUNT_ID>>", "")

I was wondering if I could remove the else tag to make the code simpler:

account_id_tag = ""
if account_id:
    account_id_tag = "<accountId>" + account_id + "</accountId>"
form = form.replace("<<ACCOUNT_ID>>", account_id_tag)

Or even using a ternary operator:

account_id_tag = "<accountId>" + account_id + "</accountId>" if account_id else ""
form = form.replace("<<ACCOUNT_ID>>", account_id_tag)

But even the last one uses less lines, I have the sensation that the original option (with else) is easy to read and to understand, and this is very important for my point of view.

There is not "you choose this" rule in this context. But to be able to decide, you should take few elements in consideration:

Are you working on an existing code (which is not yours)? If so, it would be better to follow the philosophy of the existing code, or ask the project leader in case of doubt.
If this is a new project, then you could opt for the third option which was first proposed on PEP 0308 because, in general, typing less makes you less error prone.


Find a specific Substring I'm writing a little Downloader that will look through directories online and download the content. The first prototype of my program is a success, now I just want to refine it and learn some more C#. The task is this:

Take this string:

http://example.free.pl/plus%20violent/Dark%20The%20Suns/All%20Ends%20In%20Silence/

and create the substring between the last two /

Result: All%20Ends%20In%20Silence

I find my current code brutish and probably not a clean, C# way of solving the problem:

    string GetDirectoryName(string directory)
    {
        int lastIndex  = directory.LastIndexOf("/");
        int count = directory.Count<char>(elem => elem == '/') - 1;

        int startIndex = 0;
        while (count > 0)
        {
            startIndex = directory.IndexOf("/", startIndex) + 1;
            --count;
        }

        return directory.Substring(startIndex, lastIndex - startIndex).Replace("%20", " ");
    }

Please advise me how I could make this code cleaner, clearer and following the C# methodology of programming.

The .NET framework will do this for you pretty cleanly with the right classes.

For example:

string url = "http://example.free.pl/plus%20violent/Dark%20The%20Suns/All%20Ends%20In%20Silence/";

DirectoryInfo di = new DirectoryInfo( new Uri(url).LocalPath );

The di.Name property contains "All Ends In Silence".


Would you utilise "Using" on a member which is going to be added to a list? I have an interface and a concrete class which I am using in the code below:

    /// <summary>
    /// Converts a DataSet read from the DB to a list of objects of a type
    /// TemplatePart.
    /// </summary>
    /// <param name="templatePartDataSet">DataSet containing the information.
    /// </param>
    /// <returns>List of unprocessed TemplateParts.</returns>
    private static List<ITemplatePart> DataSetToTemplatePart(DataSet templatePartDataSet)
    {
        var rawTemplateParts = new List<ITemplatePart>();
        foreach (DataTable table in templatePartDataSet.Tables)
        {
            var tableName = table.ToString();

            foreach (DataRow row in table.Rows)
            {
                if (string.IsNullOrEmpty(
                    row.Field<string>(Strings.TemplateSpreadSheet_Column_PartName)))
                {
                    continue;
                }

                // THE LINE RELATED TO MY QUESTION.
                var rawTemplatePart = TemplatePart.CreateTemplatePart(table, row);
                rawTemplateParts.Add(rawTemplatePart);
            }
        }
        return rawTemplateParts;
    }

TemplatePart is a simple class with 4 properties with the definition of:

{
    public class TemplatePart : ITemplatePart
    {
        #region Creator

        internal static TemplatePart CreateTemplatePart(DataTable table, DataRow row)
        {
            return new TemplatePart
            {
                PartName = row.Field<string>(Strings.TemplateSpreadSheet_Column_PartName),
                BasePart = row.Field<string>(Strings.TemplateSpreadSheet_Column_BasePart)
            };
        }

        #endregion // Creator

        #region Public Properties

        public string PartName { get; set; }
        public string BasePart { get; set; }
        public double? PriceDom { get; set; }
        public double? PriceInt { get; set; }

        #endregion // Public Properties
    }
}

and the ITemplatePart as:

namespace PriorityPriceGenerator2.Model.TemplatePart
{
    interface ITemplatePart
    {
    }
}

of course the interface hasn't been implemented completely. And also the concrete class doesn't have the IDisposable yet.

Would you prefer (/if it's necessary or even totally wrong) to do such thing in the marked line.

using (var rawTemplatePart = TemplatePart.CreateTemplatePart(table, row))
    rawTemplateParts.Add(rawTemplatePart);

And why? I want to know what happens to a member added to the list which is in the Using. Would it get destroyed and recreated in the List? Would it get destroyed because of using after I am done with the List ?

And please,
let me know if you see any other issues.

No, you shouldn't use using on the object that you add to the list.

When you add the object to the list, it's just the reference to the object that gets added. There is no copy of the object created for the list.

If you use using on the object, it will be disposed at the end of the code block, which is a single statement in your example. Right after you have added the object to the list it will be disposed.

Although nothing happens automatically when you dispose an object, it's customary that the object should be unusable after the Dispose method has been called. The IDisposable interface is intended for controlling the lifespan of objects, usually to free unmanaged resources when you are done using the object.

After you are done using the objects in the list, you should dispose them.

However, from what I can tell there isn't going to be any unmanaged resources in your object, so there wouldn't be any need for it to implement IDisposable. The (supposedly) database related objects are only used during the creation of the object, and so far there are nothing unmanaged in the class.


Get Rectangle Count from Given List of Co-ordinates Problem Description

  Write a program to find the maximum number of rectangles that can be
  formed both horizontal and vertical with the help of a given n number
  of coordinate(x,y) points. [x,y points will always be positive].

  The method getRectangleCount takes 1 input parameter which is an int
  array called coOrdinates.

  The method getRectangleCount takes the coordinates as a 2D integer
  array and returns the maximum number of rectangles as an integer.

public int getRectangleCount(int[][] coOrdinates)

  There is no need to count the rectangles that can be created from the
  overlapping rectangles.

Example

  Input : int coordinates[][] = { {1,1}, {7,1}, {1,4}, {1,5}, {7,4}, {7,5} }

  Output : Returns 3

  Explanation:

  Three rectangles can be formed.

  First -

    (1,4)----(7,4)
     |         |
     |         |
    (1,1)----(7,1)

  Second -

    (1,5)----(7,5)
     |         |
     |         |
    (1,1)----(7,1)

  Third -

    (1,5)----(7,5)
     |         |
     |         |
    (1,4)----(7,4)

My Concerns

Is it readable? Is it maintainable? I intentionally didn't comment in the code because I want to see if the code is self-explanatory or not.
for inside for inside if inside.... you get the picture... Is it nasty?
Can it be optimized, as it is from a programming challenge, time is a factor.

My Solution

You can browse the GitHub Project for more info.

public int getRectangleCount(int [][]coOrdinates) {

    // exception handling part coded here... forget about it

    final int MIN_POINTS_TO_CREATE_RECTANGLE = 4;

    // can't make a rectangle with less than 4 points
    if(coOrdinates.length < MIN_POINTS_TO_CREATE_RECTANGLE)
        return 0;

    final Point []points = new Point[coOrdinates.length];

    for(int i = 0; i < coOrdinates.length; i++)
        points[i] = new Point(coOrdinates[i][0], coOrdinates[i][1]);

    Arrays.sort(points); // points are sorted like (1,1) (1,3) (3,1) (3,3)....

    int rectangleCount = 0;
    for(int i = 0; i < points.length; i++) {
        Point leftDown = points[i];

        for(int j = i+1; j < points.length; j++) {
            Point leftUp = points[j];

            if(leftDown.getX() == leftUp.getX()) {

                for(int k = j+1; k < points.length; k++) {
                    Point rightDown = points[k];
                    Point probableRightUp = new Point(rightDown.getX(), leftUp.getY());

                    if((leftDown.getY() == rightDown.getY())
                                &&
                        probableRightUp.existsIn(points)) {

                        rectangleCount++;

                    }

                }

            }

        }

    }
    return rectangleCount;
}

I also have a custom Point class that implements Comparable<Point>. Point overrides compareTo() method like following

@Override
public int compareTo(Point p) {
    if(Integer.compare(this.getX(),p.getX()) == 0)
    {
        return Integer.compare(this.getY(), p.getY());
    }
    return Integer.compare(this.getX(),p.getX());
}

and method existsIn(Point []graph) works like as you think. It returns true if this exists in the graph else false.

Yes I will create a Points collection class later and will move existsIn(Point []graph). I will also change the signature to

public static boolean existsIn(Point []graph, Point toCheck) // Promise I will ;)

On the whole it's not at all bad, I find it readable and I'm sure that it does the job (one caveat to this is how does it handle cases where you have multiple instances of the same point e.g. {{1,1},{1,1},{1,2},{2,1}}), a few things that you could do to improve it..

For uniqueness and sorting consider using a TreeSet, this will have an impact on the rest of your algorithm though as you can no longer rely on iterating using an index (see below).
As @DaveJarvis says, it is preferable to have the square brackets immediately following the type - as they form a part of the type (2D array of ints vs int).
final int MIN_POINTS_TO_CREATE_RECTANGLE = 4; If this is the only piece of code to care that a rectangle has four corners it is fine to define it in method scope. If other methods might care, or if there is any chance it might need to be publicly visible then move it up to class level.
My personal preference is to always use braces even for one line conditionals. It makes your code slightly longer, but I find it more readable and it reduces the chances of bugs down the line (when someone wants to make it a two+ line conditional). This may relate to the question around Point uniqueness as you may consider adding a test for uniqueness (or using a Set) which would then make your conditional code longer.
Small things like breaking both the creation and sorting of the Point[] into new methods. It is almost trivial in this case but it will make your code easier to read, more testable and it lends itself to extensibility down the line (if for example you wanted to have different creation strategy down the line).
leftDown, leftUp and rightDown do not add anything to your algorithm, instead consider naming i,j, and k more sensibly.
If leftDown.getX() < leftUp.getX() you know you have reached a point where no other matches can ever be found. To fix this you can use a while loop instead

e.g

int j = i + 1;
//don't forget to handle index out of bounds
while (points[i].getX() == points[j++].getX()) { }

Similarly you can be sure that if k == points.length - 1 then you are not going to find a match.
Your k iteration is pointless if leftDown.getY() != rightDown.getY() so that test should move up and delay your creation of probableRightUp.
Your compareTo method can be made a little neater, either write the result of the initial compare to a variable and return it immediately if non-zero, or consider doing the comparison yourself

e.g

@Override
public int compareTo(Point p) {
    if (this.getX() < p.getX()) {
        return -1;
    }
    if (this.getX() > p.getX()) {
        return 1;
    }
    //and for Y
    ...
    return 0;
}

-. When you are coding your existsIn method start thinking in templates. Sure it's not necessary now, but it costs nothing to write public static <T> boolean existsIn(T[] array, T element) and you've got a utility function you can use forever.

One thing that would concern me about this method is the Cyclomatic Complexity due to the number of loops and conditionals. Consider how you could break things down, e.g The content of your k loop can be broken out into code which finds/verifies the rectangle is complete. As a rule each method should have a role as discrete as possible, again this makes things more testable and more readable (when coupled with sensible naming).

So using a TreeSet. Your initial code becomes:

final SortedSet<Point> points = new TreeSet<Point>(/* comparator goes here */);

for(int i = 0; i < coOrdinates.length; i++) {
    Point p = new Point(coOrdinates[i][0], coOrdinates[i][1]);
    points.add(p);
}

Your iterations would need to evolve to use the SortedSet#tailSet or SortedSet#subSet methods. Both of these are just new views on the same data so they are cheap operations.

A pseudo approach:

for (Point leftDown : points) {
    //You would pull this into a new method
    SortedSet<Point> potentialLeftUp =  points.subSet(leftDown, new Point(leftDown.getX(), somePrecalculatedMaxY);

    for (Point leftUp : potentialLeftUp) {
        //no testing needed as you know it shares X

        //now build subset (potentialRightDown) where X > leftDown.getX and Y == leftDown.getY (this will be more manual as you are X sorted)

        for (Point rightDown : potentialRightDown) {
            //build probableRightUp
            if (points.contains(probableRightUp) {
                 //increment count
            }
        }
    }
}