Plotting points representing many events on a canvas I have a web app which uses Node canvas to plot points on a graph. Each point is called an 'event'. An event is an array of usually 6 'parameters' represented as numbers. My app allows users to plot 2 different numbers from each event on an XY graph.

E.g. say one event is:

// Event 1 (e1) is
[69876, 7, 210089, 122000, 7676, 189909]

// Event 2 (e2) is
[78, 23782, 66976, 253990, 2310, 88213]

If the user chooses to plot the 3rd and 5th parameter then then e1 is represented as 210089, 7676 and e2 is 66976, 2310. A typical plot might look like:

Typically, a graph is 300 pixels. In this case above, the maximum values any parameter can be is 262144. So to figure out where to plot I just work out the ratio:

300 / 262144 = 0.001144409

Then multiple the ratio by the x, y numbers.

So e1 will be shown at 240, 9 (210089 * 0.001144409 = 240 etc) and e2 will be shown at the pixels 77, 3.

This all works fine when there are less than 100k events. However, when there are, say, 800k events, this becomes very slow.

Is there any way of doing this more efficiently by somehow preprocessing the data? So say there are 800k events and you know the maximum value of each one is 262144 and that the graph size will be 300px, could some sort of data binning be done?

For example, the numbers 400, 550, 650 will all appear in the first pixel (since anything from 0 to 873 (262144 / 300) will fall into the first pixel on a 300px graph). Is there a method I could use to condense all this data down so that I can loop through much quicker and plot?

The code is:

for (var i = 0; i < dataAsNumbers.length; i++) {

    x = dataAsNumbers[i][paramX];
    y = dataAsNumbers[i][paramY];

    pointX = getPointOnCanvas({
        value: x
        ratio: ratio
    });

    pointY = getPointOnCanvas({
        value: y,
        ratio: ratio
    });

    toPlotColors.push({
        pointX: pointX,
        pointY: pointY
    });
}

paintDotsAtOnce(toPlotColors);

var getPointOnCanvas = function(params) {

    var value = params.value;
    var ratio = params.ratio;

    return Math.floor(ratio * value);
};

var paintDotsAtOnce = function(plotColors) {

    plotColors.forEach(function(plotColor){

        if(plotColor.plotDot) {
           memoized(plotColor.pointX, plotColor.pointY, plotColor.color);
        }
    });
};

var paintDotS = function(pointX, pointY, color) {
        context.fillStyle = color;
        context.fillRect(pointX, pointY, 1, 1);
};

var memoized = memoize(paintDotS);

So if dataAsNumbers.length is 800k, it has to loop that many times. I'm worndering if data binning could reduce the need to do this.

Optimising for performance.

Javascript has a way encouraging people to write code that is terribly inefficient.

You are lucky the problem is one that can get a lot of benefit from just some minor changes.

Memorize (binning) is not going to help, it will actually significantly decrease performance and massively increase your memory load. The crazy thing is you are already binning all the data in your code.

First the rendering

var paintDotS = function(pointX, pointY, color) {
        context.fillStyle = color;
        context.fillRect(pointX, pointY, 1, 1);
};

You are only plotting single pixels. For javascript to draw a pixel via the fillRect method there are 100's of CPU cycles needed, and all you do is set a single 32bit word.

To render pixels write them directly to a pixel buffer. When all the pixels have been rendered move the pixel buffer to the canvas.

var imageData = ctx.getImageData(0,0,ctx.canvas.width,ctx.canvas.height);
var dat32 = new Uint32Array(imageData.data.buffer);

The variable dat32 holds each pixel as a 32bit integer. The channel order is from high byte Alpha to low byte Red. Eg to write a yellow you would assign an array item dat32[index] = 0xFF00FFFF;

To write a pixel at a coord x,y

 const w = 300;  // the canvas width in pixels
 dat32[x + y * w] = colour;

Loops and function calls

A big performance killer is the context change required when you call a function. This is compounded if the function being called is indirectly called from another. Many times that calling function does a lot of vetting to ensure nothing goes wrong so more cpu cycles used for no reason.

The arrays iteration methods, forEach, Map, etc are agonisingly slow when compared to standard loops.

your code

var paintDotsAtOnce = function(plotColors) {
    plotColors.forEach(function(plotColor){
        if(plotColor.plotDot) {
           memoized(plotColor.pointX, plotColor.pointY, plotColor.color);
        }
    });
};

Array.forEach calls a function, that starts, then maintains the iteration, (first context change) it calls your iterator function (second context change), your iterator calls the memoized function (third context change) it makes a memory assignment and then calls your paintDots function (fourth context change).

Remember you are just setting 1 32bit word and you are already 4 levels deep into the call stack, each needs to create closures and context, and the memoize need to allocate memory. You paint the pixel and the whole process need to be undone.

Memory and Objects.

Creating objects is slow, like snail pace slow.

Your code

for (var i = 0; i < dataAsNumbers.length; i++) {
    x = dataAsNumbers[i][paramX];  // 1st copy x
    y = dataAsNumbers[i][paramY];  // 1st copy y
      // 1st object
    pointX = getPointOnCanvas({  // 4th copy of x in the function return
        value: x           // 2nd copy of x
        ratio: ratio
    });                    // 3rd copy of x in the function
      // 2nd object
    pointY = getPointOnCanvas({ // 4th copy of y in the function return
        value: y,          // 2nd copy of y
        ratio: ratio
    });                    // 3rd copy of x in the function
      // 3rd object
    toPlotColors.push({
        pointX: pointX,    // 5th copy of x
        pointY: pointY     // 5th copy of y
    });
}
paintDotsAtOnce(toPlotColors);

var getPointOnCanvas = function(params) {

    var value = params.value;
    var ratio = params.ratio;

    return Math.floor(ratio * value);
};

You could not have found a worse way to do this. remember you are writing 1 32Bit pixel, but you create x,y coordinates, then copy x to an object, which you pass to a function in which creates another copy, you return the computed value and store it in yet another copy. Then do it again for y. And once y is done you make yet another copy to be pushed to an array as a reference (even more memory used) (remember that memorize will also be making a copys).

X and y should be immediate, on the heap and then forgotten, no variables need be created, no object constructed, no allocation requests made.

The performance rewrite.

Move one 32Bit word to an array of pixels for each item in dataAsNumbers.

const imageData = ctx.getImageData(0, 0, ctx.canvas.width, ctx.canvas.height);
const dat32 = new Uint32Array(imageData.data.buffer);
const w = canvas.width;
const color = 0xFFFFFFFF; // white
for (var i = 0; i < dataAsNumbers.length; i++) {
     dat32[(dataAsNumbers[i][paramX] * ratio | 0) +
           (dataAsNumbers[i][paramY] * ratio | 0) * w] = color;
}
ctx.putImageData(imageData, 0, 0);

Your code did not show where plotColor and plotDot came from. But they should be handled in the same way as above, without making function calls or creating objects. Everything is immediate and on the heap, and not making  allocation requests.

The beauty is that it is also has binned the data for you as the pixel array. You can make a copy and next time you need that data set just dump the pixel array to the canvas, in less than 1 ms


Sympy reduction of a multivariate, bounded, under-determined system This is bad. I've been hacking away at Sympy for a few hours and the only solution I've come up with to yield the data I want is brute-force:
Code
from sympy import Interval, Naturals0, Q, Symbol, refine, solveset

N = 10
digit_interval = Interval.Ropen(0, N)
digits = digit_interval.intersect(Naturals0)  # equivalent to range(N)

# These don't seem to have any useful impact on solveset
assumptions = {
    'nonnegative': True,
    'noninteger': False,
    'real': True,
}

a = Symbol('a', **assumptions)
b = Symbol('b', **assumptions)
c = Symbol('c', **assumptions)
symbols = (a, b, c)

left = a / (b + c)
right = 3
soln = solveset(left - right, symbols[-1], digits)

# Useless - has no effect
for sym in symbols:
    soln = refine(soln, Q.positive(N - sym))

print(f'Set solution to {left}={right} for {symbols[-1]} is {soln}')

# Number of variables is so small that recursion is fine(ish)
def recurse_determine(inner_soln, vars=(), depth=0):
    if depth == len(symbols) - 1:
        yield from (vars + (v,) for v in inner_soln)
    else:
        for val in digits:
            yield from recurse_determine(
                inner_soln.subs(symbols[depth], val),
                vars + (val,),
                depth+1,
            )

for values in recurse_determine(soln):
    print(', '.join(str(v) for v in values))

The crux of the problem is that it seems difficult (impossible?) to have Sympy find a solution to such a multivariate integer problem and meaningfully constrain it to known bounds, so I'm left doing the brute-force approach: iterate through every possible value of every variable, and only when a (probably invalid) selection of variable values has been substituted can I ask Sympy to tell me that the result is an empty set. It works (for about the worst definition of "works") but there has to be a better way.
I should add that the bounds will remain the same but the equation will be subject to change in the future, so a suggested solution that replaces Sympy with (e.g.) hard-coded Numpy or bare Python will not help me.
Output
Set solution to a/(b + c)=3 for c is Intersection(FiniteSet(a/3 - b), Range(0, 10, 1))
0, 0, 0
3, 0, 1
3, 1, 0
6, 0, 2
6, 1, 1
6, 2, 0
9, 0, 3
9, 1, 2
9, 2, 1
9, 3, 0

You're more running into the limitations of SymPy than anything. Most of the things you are doing should work, they just aren't implemented presently.
Your best bet for solving this kind of problem in a way that is smarter than naive brute force iteration of all possible values is to use the Diophantine solvers:
>>> diophantine(a/(b + c) - 3, t, syms=(a, b, c))
{(3*t_0, t_1, t_0 - t_1)}

This tells you that a solution set is
a = 3*t0
b = t1
c = t0 - t1

where t0 and t1 are arbitrary integers.  Given the form of the solution, it is quite easy to see that for a, b, c to be in the range [0, N], t1 must be in [0, N] and t0 must be in [0, N/3].
>>> N = 10
>>> for t0 in range(0, N//3 + 1):
...     for t1 in range(0, N + 1):
...         a, b, c = 3*t0, t1, t0 - t1
...         if all(0 <= i <= N for i in (a, b, c)) and b + c != 0:
...             print(a, b, c)
3 0 1
3 1 0
6 0 2
6 1 1
6 2 0
9 0 3
9 1 2
9 2 1
9 3 0

Actually in this case, brute force is also fine because there are only N^3 = 1000 combinations, but Diophantine reduced it to only N^2, so that's an improvement asymptotically. There's no need to do it recursively. You can build a substitution dictionary and pass it to subs (like expr.subs({a: 1, b: 2, c: 3})).
You can see the documentation for more information on what kinds of Diophantine equations are implemented. I'll leave it to you to implement the general code to do the above. It really depends on what form your equations take in general as to what their Diophantine solutions will look like, including whether SymPy can solve them at all.


Copying into two different folders on a machine simultaneously I am trying to copy files from machineB and machineC into machineA as I am running my shell script on machineA.

If the files are not in machineB, then it should be there in machineC for sure. I will try copying the files from machineB first, and if it is not in machineB, then I will try copying the same files from machineC.

I am copying the files in parallel using GNU parallel library and it is working fine. I am currently copying two files in parallel.

Earlier, I was copying the PRIMARY_PARTITION files in PRIMARY folder using GNU parallel and once that was done. I was then copying the SECONDARY_PARTITION files in the SECONDARY folder using same GNU parallel so it is sequential as of now with regards to the PRIMARY and SECONDARY folders.

Now I decided to copy files in PRIMARY and SECONDARY folder simultaneously.

#!/bin/bash

export PRIMARY=/test01/primary
export SECONDARY=/test02/secondary
readonly FILERS_LOCATION=(machineB machineC)
export FILERS_LOCATION_1=${FILERS_LOCATION[0]}
export FILERS_LOCATION_2=${FILERS_LOCATION[1]}
PRIMARY_PARTITION=(550 274 2 546 278) # this will have more file numbers
SECONDARY_PARTITION=(1643 1103 1372 1096 1369 1568) # this will have more file numbers

export dir3=/testing/snapshot/20140103

do_Copy() {
  el=$1
  PRIMSEC=$2
  scp david@$FILERS_LOCATION_1:$dir3/new_weekly_2014_"$el"_200003_5.data $PRIMSEC/. || scp david@$FILERS_LOCATION_2:$dir3/new_weekly_2014_"$el"_200003_5.data $PRIMSEC/.
}
export -f do_Copy

parallel --retries 10 -j 5 do_Copy {} $PRIMARY ::: "${PRIMARY_PARTITION[@]}" &
parallel --retries 10 -j 5 do_Copy {} $SECONDARY ::: "${SECONDARY_PARTITION[@]}" &
wait

echo "All files copied."

The script is working fine for me, but I am trying to see is there any better way of doing the same thing using GNU Parallel instead of using &.

I am trying to see is there any better way of doing the same thing using GNU Parallel instead of using &.

Essentially you have two different commands and two different parameter lists:

Copy to PRIMARY, these files: 550 274 2 546 278
Copy to SECONDARY, these files: 1643 1103 1372 1096 1369 1568

These are two file-sets, two distinct operations. I don't think there's an easy way to do this with a single parallel process. A not-so-easy way can be:
organize the inputs in pairs, like this:

parallel -N2 do_Copy {1} {2} ::: 550 $PRIMARY 274 $PRIMARY 1643 $SECONDARY  # and so on

This way it will work with one command.
But constructing the argument list dynamically from your list of filenames will be tricky and potentially error prone.
The original solution is simple and easier to understand,
which are preferable properties.

Exporting variables

You don't need to export PRIMARY and SECONDARY.

You only need to export FILERS_LOCATION_1 and FILERS_LOCATION_2,
because these will be used by do_Copy,
which will be called by parallel (and also needs to be exported, as you correctly did).

Notice that the read-only FILERS_LOCATION array is pointless.
You could just as well set the FILERS_LOCATION_* variables directly:

export FILERS_LOCATION_1=machineB
export FILERS_LOCATION_2=machineC

Improving do_Copy

In this method,
you have two scp commands that are almost duplicates,
except the destination part.
To avoid the duplication,
you could just loop over the possible destinations,
and break out of the loop as soon as a copy was successful:

do_Copy() {
  el=$1
  PRIMSEC=$2
  for host in $FILERS_LOCATION_1 $FILERS_LOCATION_2; do
      echo "scp david@$host:$dir3/new_weekly_2014_"$el"_200003_5.data $PRIMSEC/." && break
  done
}

If the destination files might be exactly the same as the local files,
then you'll be better off using rsync -u instead of scp,
to avoid unnecessary file transfers.


Finding students who appear on a list I'm new in Python and consequently I am searching for a specific solution for code development. I already asked some pythonic question today and want to present my whole problem.  I have found some solution, but I really have interest to develop easier and clearer code in Python, so all improvements are welcome.

For example, in some school, the students are ordered to some of 3 different classes. Now I have some set of graduated students and want to know, how many graduated students are in some class.

I create next code:

physic = ['id_1', 'id_2', 'id_4']
chemistry = ['id_5', 'id_7']
math = ['id_3', 'id_8', 'id_9']

graduatedPhysic = []
graduatedChemistry = []
graduatedMath = []

classes = [physic, chemistry, math]
graduated = [graduatedPhysic, graduatedChemistry, graduatedMath]

Every class has the different number of students (given by students IDs) and gratuated Classes are at the first empty. I combine it also to list of lists.

Then I use my code:

graduatedlist = ['id_2', 'id_5', 'id_7']

for stud in graduatedlist:
    for pos1 in range(len(graduated)):
        for pos2 in range(len(classes[pos1])):
            if stud.find(classes[pos1][pos2]) != -1:
                graduated[pos1].append((stud))

This code works, but does not look great... I would be very grateful for comments! For example, I tried to implement the idea with itertools.product():

for item_1, item_2 in itertools.product(range(len(graduated)), range(len(classes[item_1])):

I have a problem with a declaration of item_1.

I have also a feeling that the lists isn't the best solution for implementation, but for DataFrame I need the column of similar length and every class can have the different number of the students.

For the special interest are two points:

How I really can improve my for-loop?
What I can use instead of list declaration?

I would strongly recommend creating a class.

class Student:
    def __init__(self, id, graduated):
        self.id = id
        self.graduated = graduated

Then your problem is easier.

graduatedchemistry = [student for student in chemistry if student.graduated]

To cleanly get graduates from all classes then you can make a function

def getgraduates(students):
    return [student for student in students if student.graduated]

graduatedchemistry = getgraduates(chemistry)
graduatedmath = getgraduates(math)


Simple JavaScript Module "Loader" I have written a simple JS module "loader" (loader is in quotes because it doesn't actually load the files) that is designed to resolve simple dependencies between modules. It is heavily inspired by module.js and takes some of it's design from it.

Here is the code itself:

(function() {
  function create() {
    var definitions = {},
        instances = {};

    // Returns whether or not a module with the provided id is defined.
    var defined = function(id) {
      return definitions.hasOwnProperty(id);
    };

    // Define a module with the provided id and definition.
    var define = function(id, definition) {
      if(defined(id)) {
        throw new Error('module already defined: ' + id);
      }

      definitions[id] = definition;
    };

    // Undefine a module with the provided id.
    var undefine = function(id) {
      if (!defined(id)) {
        throw new Error('module not defined: ' + id);
      }

      delete definitions[id];
      delete instances[id];
    };

    // Require a module with the provided id.
    var require = function(id) {
      var stack = [];

      var internalRequire = function(id) {
        if(!defined(id)) {
          throw new Error('module not defined: ' + id);
        }

        // If we have already seen this id on the require stack, we've got
        // some form of cyclic dependency.
        if (stack.indexOf(id) !== -1 && stack.push(id)) {
          throw new Error('cyclic dependency: ' + stack.join(' -> '));
        } else {
          stack.push(id);
        }

        // If we already have an instance for this module, return it.
        if (instances.hasOwnProperty(id)) {
          return instances[id];
        } else if (typeof(definitions[id]) === 'function') {
          // Otherwise if our definition is a function, call it and pass the
          // require method to it.
          return instances[id] = definitions[id].call(null, internalRequire);
        } else {
          // Otherwise just return the definition itself (useful for objects
          // e.g. constants).
          return instances[id] = definitions[id];
        }
      };

      return internalRequire(id);
    };

    return {
      define: define,
      undefine: undefine,
      require: require
    };
  }

  window.module = create();
})();

And some example usage:

module.define('person', function() {
  var Person = function(name) {
    Object.defineProperty(this, 'name', {
      value: name
    });
  };

  Person.prototype.speak = function(text) {
    return this.name + ': ' + text;
  };

  return Person;
});

module.define('main', function(require) {
  var Person = require('person');

  var jack = new Person('Jack'),
      rocky = new Person('Rocky');

  console.log(jack.speak('Hi Rocky!'));
  console.log(rocky.speak('Hi Jack!'));
});

I don't really have any "burning questions" about my code; I'd just like some feedback on what people think of it and whether anyone can see any improvements.

The names define and defined are too similar, which could lead to mistakes. I suggest renaming the latter to isDefined.

You could flatten some of the conditionals when all branches leave the block. For example:

// If we have already seen this id on the require stack, we've got
// some form of cyclic dependency.
if (stack.indexOf(id) !== -1 && stack.push(id)) {
  throw new Error('cyclic dependency: ' + stack.join(' -> '));
}
stack.push(id);

// If we already have an instance for this module, return it.
if (instances.hasOwnProperty(id)) {
  return instances[id];
}

if (typeof(definitions[id]) === 'function') {
  // Otherwise if our definition is a function, call it and pass the
  // require method to it.
  return instances[id] = definitions[id].call(null, internalRequire);
}

// Otherwise just return the definition itself (useful for objects
// e.g. constants).
return instances[id] = definitions[id];


Achieving a rotated Vector magnitude I am trying to convert this velocity into another 2 dimensional vector at some specified angle:

float maximumVelocity = 1.5f;
float angleInDegrees = 23f;
Vector2 actualVelocityAs2DimensionalVector = {?,?}

At first, I was going to take 1.5, and rotate it after converting the degrees to radians, like so:

double radians = Math.toRadians(angleInDegrees);
Vector2 actualVelocityAs2DimensionalVector = new Vector2(x * Math.cos(radians) - y * Math.sin(radians),
                                                         x * Math.sin(radians) + y * Math.cos(radians));

Is this the fastest way to achieve this? This will be happening a lot in my application, and I would think all the cos and sin calls are somewhat expensive.

You're calling Math.sin() and Math.cos() twice as many times as you need to.

double cosine = Math.cos(radians)
double sine = Math.sin(radians)
Vector2 rotated = new Vector2(x*cosine - y*sine, x*sine + y*cosine)

If the angle never changes, then just save the values cosine and sine so you don't have to compute them over and over again.


Delete duplicate files I have this function which looks through ALL files found in a directory (including sub folders) and deleted the duplicated file.

private static void DeleteDuplicateFiles(string directoryPath)
    {
        Console.WriteLine("Finding files...");

        // get all files
        var allFiles = Directory.GetFiles(directoryPath, "*.*", SearchOption.AllDirectories);
        Console.WriteLine("Files found: {0}", allFiles.Length);

        // get duplicate files
        var dupFiles = allFiles.Select(f =>
        {
            using (var fs = new FileStream(f, FileMode.Open, FileAccess.Read))
            {
                return new
                {
                    FileName = f,
                    FileHash = BitConverter.ToString(SHA1.Create().ComputeHash(fs))
                };
            }
        }).GroupBy(f => f.FileHash).Select(g => new { FileHash = g.Key, Files = g.Select(z => z.FileName).ToList() })
        .SelectMany(f => f.Files.Skip(1)).ToList();
        Console.WriteLine("Duplicate files: {0}", dupFiles.Count);

        // delete deuplicate files
        dupFiles.ForEach(File.Delete);

        Console.WriteLine("{0} files were deleted", dupFiles.Count);

    }

This algorithm isn't the most efficient obviously, but what I have in mind is to:

Group files by their size (so files with unique size wont get processed)
Calculate the hash of the remaining files.

Is this the best solution or is there a better way of doing it?

To search files you're using Directory.GetFiles(), nothing wrong with that but, especially in conjunction with SearchOptions.AllDirectories, it may take some time to search for them all. You should switch to Directory.EnumerateFiles() because you can consume enumeration immediately and search will progress on-the-fly (in this case you do not give any progress feedback then you do not need to know the total number of files to process in advance).

It has few advantages:

You can start processing immediately, reducing the total perceived execution time.
You use less memory than reading the whole list (not such an issue nowadays).
It's easy to be parallelized.

And we then arrive to the second point: parallelization. It's always tricky to make I/O bound code parallel but in this case you have two main tasks:

Read file content.
Calculate hash.

Even a very simple parallel approach will benefit from overlapping next read operation with previous hash calculation. Roughly something like this:

var allFiles = Directory.EnumerateFiles(directoryPath, "*.*", SearchOption.AllDirectories);

var hashedFiles = allFiles.AsParallel()
    .Select(x => new { Path = x, Hash = CalculateHash(x) });

You may want to play with options to limit the maximum number of concurrent reads. Also note that I moved the hash calculation in its own separate function. I'd do the same also for the other tasks, it's true that this method is pretty small but you will gain in clarity, proof of concept:

public static void DeleteDuplicateFiles(string searchPath)
{
    foreach (var file in FindDuplicateFiles(searchPath))
        TryDeleteFile(file);
}

private static IEnumerable<string> FindDuplicateFiles(string searchPath)
{
    var hashedFiles = FindAllFiles(searchPath)
        .AsParallel()
        .Select(x => new { Path = x, Hash = CalculateHash(x) });

    return FindDuplicates(hashedFiles);
}

And so on, I think you get what I mean. Some of those functions may also be reused and to understand overall algorithm you just need to read few lines of code, no need to dig into implementation details.

I hope you noted I used a TryDeleteFile() function instead of directly calling File.Delete(). This is because I/O operations may fail and error may be transitory (for example another program or Windows Shell may keep temporarily keep the file open (without sharing for deletion) but it may be deleted few seconds later. In this function you simply try to delete the file and if you get an IOException with HRESULT for file in use then you simply wait and retry few times.

One last thing: do not mix your logic with presentation. This code can be easily reused outside a console application. You may also want to provide a different output (for example coloring messages) but if you mix them together then any change will be a pain. Introduce an interface IProgressWriter for example like this:

public interface IProgressWriter
{
    void Trace(string message);
    void Error(string messsage);
}

In this case I wrote a somehow generic one but you might want a specialized one (for example to later switch to a GUI application without touching a single line of code in your logic). Note that you can do it both with an interface and with events, like this:

public sealed class DuplicatedFilesSleuth
{
    public string SearchPath { get; set; }

    public void FindDuplicates() { ... }

    public void DeleteDuplicates() { ... }

    public event EventHandler<DuplicatedFilesSleuthEventArgs> Hashing;

    public event EventHandler<DuplicatedFilesSleuthEventArgs> Deleting;
}

Note that, as t3chb0t pointed out in a comment, if you do not expose callable methods but data then you should use IProgress<T>/Progress<T> instead of another custom interface.


C++ Error/exception handling I know the following code are horrible, so I'd like a review. Criticism are welcome. I want to know what went wrong, and how I can avoid the same mistake in the future.

Version 1
const unsigned int set_max {32};
std::vector<unsigned int> base;
base.reserve(argc-1);
for (int i = 1; i < argc; ++i) {
  unsigned long tmp;
  try { tmp = std::stoul(argv[i], nullptr, 10); }
  catch (std::invalid_argument) { std::cerr << "Invalid base! Conversion failed.\n"; return -2; }
  catch (std::out_of_range) { std::cerr << "Invalid base! Too large.\n"; return -2; }
  if (tmp > std::numeric_limits<unsigned int>::max()) { std::cerr << "Invalid base! Too large.\n"; return -2; }
  if (tmp < 2) { std::cerr << "Invalid base! Must at least be 2.\n"; return -2; }
  if (tmp > set_max) { std::cerr << "Invalid base! Must be within " << set_max << ".\n"; return -2; }
  base.push_back(static_cast<unsigned int>(tmp));
}

Version 2
const int set_max {32};
std::vector<unsigned int> base;
base.reserve(argc-1);
try { for (int i = 1; i < argc; ++i) {
  unsigned long tmp; try { tmp = std::stoul(argv[i], nullptr, 0); }
  catch (std::invalid_argument                         ) { throw std::string("Conversion failed"                        ); }
  catch (std::out_of_range                             ) { throw std::string("Too large"                                ); }
  if    (tmp > std::numeric_limits<unsigned int>::max()) { throw std::string("Too large"                                ); }
  if    (tmp < 2                                       ) { throw std::string("Must at least be 2"                       ); }
  if    (tmp > set_max                                 ) { throw std::string("Must be within " + std::to_string(set_max)); }
  base.push_back(static_cast<unsigned int>(tmp));
} } catch (std::string &err_msg) { std::cerr << "Invalid base! " << err_msg << ".\n"; return -2; }

Version 3
const int set_max {32};
std::vector<unsigned int> base;
base.reserve(argc-1);
for (int i = 1; i < argc; ++i) {
  unsigned long tmp;
  try {
    try {
      tmp = std::stoul(argv[i], nullptr, 0);
    }
    catch (std::invalid_argument) {
      throw std::string("Conversion failed");
    }
    catch (std::out_of_range) {
      throw std::string("Too large");
    }
    if (tmp > std::numeric_limits<unsigned int>::max()) {
      throw std::string("Too large");
    }
    if (tmp < 2) {
      throw std::string("Must at least be 2");
    }
    if (tmp > set_max) {
      throw std::string("Must be within " + std::to_string(set_max));
    }
  }
  catch (std::string &err_msg) {
    std::cerr << "Invalid base! " << err_msg << ".\n";
    return -2;
  }
  base.push_back(static_cast<unsigned int>(tmp));
}

Readability

Version 1:
This is unreadable. If you presented that me in a professional context I would literally fire you (for real).

Version 2:
This is arguably readable. But you are making it hard for yourself and other people. You are probably going to be breaking your companies coding guidelines and I can see your colleagues complain. But its all subjective. I would not like you if we were working on the same code base.

Version 3:
This is a reasonable readable version.

Code Review
Your use of exceptions is a bit much. Exceptions are great when you can not correct the problem at the point of the error and need to transfer control to a higher level context and thus can take a reasonable action based on context.
Here you are throwing exceptions and catching them in the same function (i.e you are not moving to a higher context). It's not terrible, but you can achieve the same effect with error codes (and the error codes do not cross an interface boundary so are OK). I am not going to argue for error codes or against them. In this case that is a style thing and how your team likes to handle the situation.
The only issue I have is that exception that are throw can potentially change over time, and you don't catch the any (catch (...)) and thus can potentially have an accidental application termination with no error message.
I don't know how many arguments this takes. But it exits on the first parameter error. You may want to check all the parameters and generate more errors (if possible). This potentially will allow the user to fix all the parameters at once rather than having to run the application multiple times fixing one parameter at a time.


Modifying __getattr__ to return a nested attribute instead of the named object I'm writing an application with two basic class types:

DocumentObject
Property (of document object).

Property instances are attributes of the DocumentObject class and The Property class has several simple attributes of its own (value, unit, etc.) and some methods.
In trying to make the scripting as user friendly as possible, I would like
objectName.propertyName
to return the value attribute of the Property instance, not the Property instance itself. Of course, it is possible to write objectName.propertyName.value but most of the time, the user will be interacting with the value, not the Property instance.
It seems it should be possible to implement this behaviour using modified __getattr__ and __setattr__ methods in DocumentObject like in the following example:
Input
class Property():
    def __init__(self, name, value, unit='mm'):
        self._name = name
        self.value = value
        self._unit = unit
        self._isVisible = True

    @property
    def name(self):
        return self._name

    @property
    def unit(self):
        return self._unit

    @property
    def isVisible(self):
        return self._isVisible

    @isVisible.setter
    def isVisible(self, value):
        self._isVisible = bool(value)

class DocumentObject():
    def __init__(self, properties):
        object.__setattr__(self, 'properties', dict())
        self.properties = properties

    def __getattr__(self, name):
        if "properties" in vars(self):
            if name in self.properties:
                return self.properties[name].value
            else: raise AttributeError
        else: raise AttributeError

    def __setattr__(self, key, value):
        if key in self.properties:
            self.properties[key].value = value
        else:
            object.__setattr__(self, key, value)

brick = DocumentObject({'length': Property('length',10), 'width': Property('width',5)})
print(brick.properties["length"].name)
print(brick.length)

Output
length
10

Questions:

Is is good practice to do this?
Are there likely to be some negative consequences of this decision?
Is there a more elegant solution that I have missed?

Do not write trivial getters/setters; this is not Java/C++/etc. It's more Pythonic to simply have everything public under most circumstances. Private-ness is not enforced anyway and is more of a suggestion.
Consider using @dataclass
You can drop empty parens after your class definition
Under what circumstances would it be possible for properties to be missing from DocumentObject? I'm not sure why you check for this. Won't it always be in vars?
Use type hints.
Use snake_case, not camelCase for variables
Do not allow arbitrary __setattr__ on your DocumentObject other than to registered properties
Do not accept a dict with redundant keys; just accept an iterable of properties
IMO it's less surprising to have the document's actual properties dict override any request to a property named "properties" than the other way around

Suggested:
from dataclasses import dataclass
from typing import Dict, Iterable, Union

@dataclass
class Property:
    name: str
    value: float  # ?? or maybe Any
    unit: str = 'mm'
    is_visible: bool = True

PropDict = Dict[str, Property]

class DocumentObject:
    def __init__(self, properties: Iterable[Property]):
        self.properties: PropDict = {p.name: p for p in properties}
        self.__setattr__ = self._setattr

    def __getattr__(self, name: str) -> Union[PropDict, float]:
        if name == 'properties':
            return self.properties
        return self.properties[name].value

    def _setattr(self, key: str, value: float):
        self.properties[key].value = value

brick = DocumentObject((
    Property('length', 10),
    Property('width', 5),
))
print(brick.properties['length'].name)
print(brick.length)
brick.width = 7
print(brick.width)


Use presets to change a form I intend to populate some fields in a form based on some presets values. The user choose the presets from a list. The following code is working, but as I'm a newbie, I would like to know if I done it the proper way and if it can be improved.

Preset obj:

var obj = {
    "preset_a":{
        "input":{
            "field_a":"value",
            ...
            "field_b":"value",
        },
        "select":{
            ...
        },
        "radio":{
            ...
        }
    },
    "preset_b":{
        "input":{
            "field_a":"value",
            ...
            "field_b":"value",
        },
        "select":{
            ...
        },
        "radio":{
            ...
        }
    }
};

The script:

function applyPresets(preset)
{
    if (typeof obj[preset] != 'undefined')
    {
        if (typeof obj[preset]['input'] != 'undefined')
        {
            jQuery.each(obj[preset]['input'], function( k, v ) {
                jQuery("input#"+ k).val(v);
            });
        }

        if (typeof obj[preset]['select'] != 'undefined')
        {
            jQuery.each(obj[preset]['select'], function( k, v ) {
                jQuery("select#" + k).val(v);
            });
        }

        if (typeof obj[preset]['radio'] != 'undefined')
        {
            jQuery.each(obj[preset]['radio'], function( k, v ) {
                jQuery("fieldset#" + k + " > input[type=radio]").val(v);
            });
        }
    }
    else
    {
        alert('Preset [ ' + preset + ' ] not found!');
        return;
    }

    submitSettings();
}

If the property name has spaces or special characters, there is no need to quote it (and it looks more clear):

var obj = {
    preset_a: {
        input: {
            field_a: "value",
            field_b: "value",
        }
        "i need quotes": "value"
    }
}

I tried to improve the function a little bit. I did it on the fly so i'm pretty sure that something could be wrong. Check it out:

function applyPresets(preset){
    // as Quill said, the return(s) statement has more sense at the top of the method
    if( typeof obj[preset] == 'undefined' ){
        alert('Preset [ ' + preset + ' ] not found!');
        return;
    }
    for( var i in obj[preset] ){
        if( obj[preset].hasOwnProperty(i) ) { // .hasOwnProperty() can be used to determine whether an object has the specified property as a direct property of that object, very useful here
            $.each(obj[preset][i], function(k, v){ // since the code seems very similar we can improve it avoiding more loops
                if( i == 'radio' ){ // if the code below doesn't match, we will need this `if`
                    $('fieldset#' + k + ' > input[type="radio"]').val(v);
                }
                else $(i + '#' + k).val(v); // while they match we don't need more code
            });
        }
    }
    submitSettings();
}

In summary: We use an in loop through the obj[preset] object. This gives us the name of i, so we can easily discard undefined(s) thanks to the .hasOwnProperty() method. The last step is avoid the loops. The syntax is quite the same so we can work inside only one .each() loop. If the selector changes we can always use the lovely if statement.

I made a jsFiddle with the code if you want to see it clearly.


Find-like script for windows I always read questions about a Windows solution to find a file in a directory structure like find and grep.
I created this working c++ program, where you give the program a path and a file name to find.
#include <iostream>
#include <string>
#include <sys/stat.h>
#include <windows.h>

#ifndef INVALID_FILE_ATTRIBUTES
constexpr INVALID_FILE_ATTRIBUTES((DWORD) - 1)
#endif

BOOL IsDir(const std::string &path)
{
DWORD Attr;

Attr = GetFileAttributes(path.c_str());
if (Attr == INVALID_FILE_ATTRIBUTES)
    return FALSE;

return (BOOL)(Attr & FILE_ATTRIBUTE_DIRECTORY);
}

std::string sanitizePath(const std::string &path)
{
std::string fspath = path;

while (*(fspath.rbegin()) == '/' || *(fspath.rbegin()) == '\\')
    fspath.pop_back();

return fspath;
}

int findFiles(std::string &fspath, const std::string &fs)
{
static size_t i = 0;
WIN32_FIND_DATA FindFileData;

std::string destpath = fspath + std::string("\\*.*");

HANDLE hFind = FindFirstFile(destpath.c_str(), &FindFileData);

if (hFind != INVALID_HANDLE_VALUE)
    do {
        std::string fullpath = std::string(fspath) + std::string("\\") + std::string(FindFileData.cFileName);

        if (*(fullpath.rbegin()) == '.')
            continue;
        else if (FindFileData.dwFileAttributes &FILE_ATTRIBUTE_DIRECTORY)
            findFiles(fullpath, fs);
        else
        {
            if (FindFileData.cFileName == fs)
            {
                std::cout << fs << " was found in -> " << fspath << std::endl;
                i++;
            }
        }
    } while (FindNextFile(hFind, &FindFileData));

FindClose(hFind);
return i;
}

int main(int argc, char **argv)
{
if (argc <= 2)
{
    std::cerr << "No path or filename provided" << std::endl;
    return EXIT_FAILURE;
}

const char *path = argv[1];
const char *fileSearched = argv[2];

if (!IsDir(path))
{
    std::cerr << "Path doesn't exist" << std::endl;
    return EXIT_FAILURE;
}

std::string fspath = sanitizePath(path);
std::string fs = fileSearched;

std::cout << "Searching " << fspath << " and its subdirectories" << std::endl;

int i = findFiles(fspath, fs);

if (i == 0)
    std::cout << fs << " was not found in " << fspath << " or its subdirectories" << std::endl;
else if (i == 1)
    std::cout << fs << " was found once";
else
{
    std::cout << fs << " was found " << i << " times" << std::endl;
    std::cout << "Beware same filename does NOT automatically mean same file content." << std::endl;
}

return EXIT_SUCCESS;
}

it is used like this
find <path> <filename>

It is case sensitive and gives you whether and where the file was found and how many times.
I used an online code beautifier for indentation.
The sanitize method is for removing trailing "/" or "\".
It compiled and ran to multiple tests.
Please give me you opinions especially about its professionalism aka do code in big companies look like that?
Keep in mind, I graduated some time and failed so far in getting internship or a job in programming in my country.

As a general rule, I'd tend to prefer the built in C++ bool over the Windows-specific BOOL.

Your sanitizePath seems less than ideal, at least to me. Passing them parameter by reference, then creating a local copy from the reference seems like kind of a waste. If you want a local copy, just pass by value. Regardless of that, std::string already provides a function to search for the position you want, so I'd probably use that.
 std::string sanitizePath(std::string const &input) {
     auto pos = input.find_last_not_of("/\\");
     return input.substr(0, pos+1);
 }

After a few decades of using C++, I've gotten to the point that almost any time I see an open and matching close operation, my immediate reaction is to use RAII--define a class that does the open in its ctor, and the matching close in its dtor. It looks to me like file searching fits this pattern, and probably benefits from the RAII treatment.

Likewise, if I'm going to iterate through a collection of objects, my reaction is to think about whether I can define an actual iterator, so I can off-load as much work as possible onto standard algorithms (and such).

I'd avoid using std::endl. In this case, it probably doesn't make a huge difference, but I'd still form the habit of using '\n' when you want a new-line.

You might want to consider using a std::filesystem::recursive_directory_iterator, which already implements an iterator interface for file searching. Using it, the code for findFiles would reduce down to something like this:
 int findFiles(std::filesystem::path const& fspath, const std::string& fs)
 {
     std::filesystem::recursive_directory_iterator di { fspath };
     int count = 0;

     for (auto const& file : di) {
         if (file.path().filename() == fs) {
             std::cout << file.path().string() << "\n";
             ++count;
         }
     }
     return count;
 }


HTTP client similar to cURL I'm writing my own HTTP client, kinda like cURL. (I already know I'm reinventing the wheel, this is more or less getting an inside look of HTTP 1.x before 2 becomes a thing.)

So far pages download perfectly fine using OS provided socket libs. (Linux/Mac)
So what the problem/question is, how can I better my client? One of the main problems with it, is that it's slow. Like 1.00 - 1.30 seconds slow. I know this may not sounds like much, but the 1 second is mainly because of the timeout of the recv. Or maybe I cant do much more except do what other HTTP clients do and cache data.

char   buf[CHUNK_SIZE+1];
    //now it is time to receive the page
    memset(buf, 0, sizeof(buf));
    std::string htmlcontent;

    struct timeval tv;

    tv.tv_sec = 1;
    setsockopt(socket, SOL_SOCKET, SO_RCVTIMEO,(struct timeval *)&tv,sizeof(struct timeval));

    while(true){
        tmpres = recv(socket, buf, CHUNK_SIZE, 0);

        if(tmpres < 1)
        {
            break;
        }

        htmlcontent += buf;
        memset(buf, 0, tmpres);
    }

So as you can see, the recv timeout is 1 second. I'm hoping to maybe manipulate this in someway so, it's instead 500 milliseconds for timeout or perhaps look at another method in general when dealing with chunk data from a HTTP server. And of course I googled some topics, but a lot of them had pretty basic examples. Mostly what I have already written.

Since it looks like you don't wait for a specific length of data I assume that you have no idea how to get the length of the data up-front. Since you are willing to learn how HTTP/1.x works I recommend you to read the specifications and watch out for something like "Content-length" and "Transfer-Encoding: chunked". Once you've understood why these are important and how to correctly deal with them you can write your code so that it does not timeout.


Python GPA Calculator As part of a project I made a GPA calculator. It takes in the class name and letter grade for that class and adds them to a list (eventually I'll add some features that require me to have the grades and classes paired to each other) then adds points according to the grade and then divides by 6 the number of classes I have and gives me a total GPA.

        #GPA Calc

#Lists
classes= []
grades = []

#Collects the data of Class names and Grades in Letter Form
def collect():
    i = 0
    while (i <= 5):
        className = input("Enter Class Name: ")
        classes.append(className)
        i = i +1

    print(classes)
    y = 0
    while (y <=5):
        grade = input("Enter Your Grade For Each Class Listed in Order (Letter Form): ")
        grade = grade.upper()
        grades.append(grade)
        y = y + 1
    calculate()

def calculate():
    total= 0
    for element in grades:
        if element == "A+":
            total = total + 4.0
        elif element == "A":
            total = total + 4.0
        elif element == "A-":
            total = total + 3.7
        elif element == "B+":
            total = total + 3.3
        elif element == "B":
            total = total + 3.0
        elif element == "B-":
            total = total + 2.7
        elif element == "C+":
            total = total + 2.3
        elif element == "C":
            total = total + 2.0
        elif element == "C-":
            total = total + 1.7
        elif element == "D":
            total = total + 1.0
    gpa = total / 6
    print(gpa)

collect()

It all works the way it should, but if someone more experienced could tell me a better way to work with the class names and the grade associated and if there is a more efficient way to calculate the grades with some kind of dictionary or some other way, that would be appreciated.

Avoid global variables

There's no good reason for the variables classes and grades to be defined and manipulated in global scope.
For example, it would be better to pass grades into the calculate function as a parameter.

Consider a table-driven solution

The long if-else chain is tedious.
You could instead use a dictionary to map the grade notes to scores:

score_map = {
    'A': 4,
    'C': 2,
    'C-': 1.7,
    'D': 1,
    # ... and so on
}
total = 0
for grade in grades:
    total += score_map[grade]

This approach also makes it easy to use a list-comprehension that's more compact than a for loop:

return sum(score_map[grade] for grade in grades) / 6

Usability

Making the user enter the class names seems rather pointless.
The program simply prints them back, without doing anything interesting with them.

Style

The parentheses are redundant here:

while (y <= 5):

Instead of y = y + 1 you can use the augmented assignment form y += 1.

Python has a style guide called PEP8 with a utility to spot such issues.


Conditional assignment in XSLT? I've written some XSLT where I want to assign values to some variables.

Depending on whether or not a condition is met, I want the variable to be populated with a computed value and I want it to have a default value otherwise.

I've written the following code that appears to work, but I think that there may be a better way.

<xsl:variable name='HasValue' select='count($Catalog/lookup) &gt; 0' />
<xsl:variable name='Product' >
    <xsl:choose><xsl:when test='$HasValue'>
      <xsl:value-of select='$Catalog/lookup/Product' />
    </xsl:when><xsl:otherwise>UNK</xsl:otherwise></xsl:choose>
</xsl:variable>
<xsl:variable name='Area' >
    <xsl:choose><xsl:when test='$HasValue'>
      <xsl:value-of select='$Catalog/lookup/Area' />
    </xsl:when><xsl:otherwise>UNK</xsl:otherwise></xsl:choose>
</xsl:variable>

Yes, my XSLT processor uses XSLT 2.0

IMHO your code is fine. The "problem" with XSLT variables is that they are NOT variables but more resemble constants: once set they cannot be changed anymore. Also, their scope is restricted to anything that follows their definition until the closing tag. In particular they are NOT visible in any ancestor tag. The combination of both results in the type of constructs that you have used above.

What is most disturbing is the fact that you usually have to repeat the testing of the criterion again and again if several variables depend on the same criterion. Again there's nothing generic that you can do about this, but of course, you can always at least create another variable holding the result of the test and then use that several times.

Depending on the context of your XSLT I would suggest two alternatives which may or may not be more elegant:

a) works in XSLT 1.0 and 2.0: If you just define the variables to be used as parameters to a template-call consider writing the template-call twice once in the when as once in the otherwise portion. This way you will end up with just one <choose>.

b) Works only well in XSLT 2.0 (or a little awkward in XSLT with node-set extensions): move the check of the criterion and the selection of the definition values into a template and return the "set" of values to the assigned in a node-set. In a way you can regard this node-set as a record whose attributes have been set according to the criterion. See example below.

<?xml version="1.0" encoding="UTF-8"?>
<xsl:stylesheet
    version="1.0"
    xmlns:xsl="http://www.w3.org/1999/XSL/Transform">
  <xsl:output method="xml" version="1.0" encoding="UTF-8" indent="yes" />

  <xsl:template name="get_values">
    <xsl:param name="catalog"/>

      <xsl:choose>

        <xsl:when test="count($catalog/lookup) &gt; 0">
          <PRODUCT VALUE="{$catalog/lookup/Product}"/>
          <AREA VALUE="{$catalog/lookup/Area}"/>
        </xsl:when>

        <xsl:otherwise>
          <PRODUCT VALUE="UNK"/>
          <AREA VALUE="UNK"/>
        </xsl:otherwise>

      </xsl:choose>

  </xsl:template>

  <xsl:template match="/">

    <xsl:variable name="Catalog" select="."/>

    <xsl:variable name="my_config">
      <xsl:call-template name="get_values">
        <xsl:with-param name="catalog" select="$Catalog"/>
      </xsl:call-template>
    </xsl:variable>

    <xsl:message>
      The value of PRODUCT is <xsl:value-of select="$my_config/PRODUCT/@VALUE"/>
      and the value of AREA is <xsl:value-of select="$my_config/AREA/@VALUE"/>
    </xsl:message>

  </xsl:template>

</xsl:stylesheet>

This solution is definitely not shorter than yours but it resembles more other standard programming languages. The record type approach may come in especially handy if you have a lot of templates requiring the attributes in the record.


Method that reads an input file of a particular format and creates objects corresponding to the file This is essentially a best practices question about reading a file and making sure it follows a specific format. My current implementation works, but it's a while loop with a bunch of if/else statements. I think it wouldn't be too readable for other developers.

/**
 * Request the server to add the theatre details found in the file indicated by the path.
 * The details include the theatre ID (unique across all theatres), the seating dimension, and
 * the floor area (square metres). All theatres have as many seats in a row as they do rows, and the number of
 * rows is indicated by the seating dimension.
 *

 *
The file format is, each line consists of
 * "THEATRE" "\t" theatre ID; "\t" seating dimension; "\t" floor area;
 *

 *
If there is no file associated with the path, or the file format is incorrect, then
 * the request fails.
 *
 * @param path The path indicating the file to use for the initialisation.
 * @return An empty string if the initialisation is successful, otherwise a
 * message explaining what went wrong beginning with ERROR (e.g. "ERROR incorrect format").
 */
public String initialise(String path)
{
    //IO
    File theatreFile = new File(path);
    Scanner fileScanner = null;

    try
    {
        fileScanner = new Scanner(theatreFile);
    }
    catch (FileNotFoundException e)
    {
        return ResponseMessages.FILE_NOT_FOUND_ERR_MSG.getDescription();
    }

    String currentID = null;
    int numRows;
    int floorSpace;

    /*Loop through input file tokens, check if format is correct and if so, create a theatre object
    that corresponds with the given data. */
    while(fileScanner.hasNext())
    {
        if(fileScanner.hasNext(THEATRE_NAME_MARKER))
        {
            fileScanner.next();

            if(fileScanner.hasNext(THEATRE_CODE_MARKER))
            {
                currentID = fileScanner.next();

                if (getTheatreIDs().contains(currentID))
                {
                    return ResponseMessages.DUPLICATE_CODE_ERR_MSG.getDescription();
                }

                if (fileScanner.hasNextInt())
                {
                    numRows = fileScanner.nextInt();

                    if (fileScanner.hasNextInt())
                    {
                        floorSpace = fileScanner.nextInt();
                    }
                    else
                    {
                        return ResponseMessages.FILE_FORMAT_ERR_MSG.getDescription();
                    }
                }
                else
                {
                    return ResponseMessages.FILE_FORMAT_ERR_MSG.getDescription();
                }
            }
            else
            {
                return ResponseMessages.FILE_FORMAT_ERR_MSG.getDescription();
            }
        }
        else
        {
            return ResponseMessages.FILE_FORMAT_ERR_MSG.getDescription();
        }

        Theatre newTheatre = new Theatre(currentID, numRows, floorSpace);
        theatres.add(newTheatre);
    }

    fileScanner.close();
    return ResponseMessages.FILE_FOUND_SUCCESS_MSG.getDescription();
}

Additionally, here's an example of what an input file may look like:

THEATRE T2  10  400
THEATRE T1  7   200
THEATRE T3  12  600
THEATRE 215 21  1200

As you are asking for best practices: methods and exceptions.

First of all, your code does everything in one method, where you could (and should) clearly divide the parts of

looping over the file
parsing a single line
checking uniqueness of the theatres

As you return with a failure anyway if a single line fails, you might as well throw an exception and stop the complete process. If you want to stick to the interface that returns an error/success condition as a result value, you may well catch and return in the outermost method.

Rough sketch:

private static class MyFileParsingException extends RuntimeException {
    ... add a constructor with a message here
}

public String initialise(String pathString) {
    try {
        Path filePath = Paths.get(pathString);
        List<Theater> theatres = Files.lines(filePath)
            .map(this::parseLineToTheater)
            .collect(Collectors.toList());
        checkUniqueness(theatres );
        this.theatres = theatres; // only set if successful
        return ... // success
    }
    catch(IOException e) {
        return ResponseMessages.FILE_NOT_FOUND_ERR_MSG.getDescription();
    }
    catch(MyFileParsingException e) {
        return e.getMessage();
    }
}

private Theater parseLineToTheater(String inputLine) throws MyFileParsingException {
    // parse single line, throw MyFileParsingException with the
    // appropriate message, return the theater
}

private void checkUniqueness(List<Theater> allReadTheaters) throws MyFileParsingException {
    // check, whether the ids are unique, throw MyFileParsingException
    // with DUPLICATE_CODE_ERR_MSG if you find a duplicate
}

Here, you have a trivial outer loop (or in fact a stream), which uses the nio.Files methods (which have been around since java 8, i.e. more than 4 years now - time to start using them), and two more methods which can trivially be unit-tested without even creating a file.

A little twist is extending RuntimeException instead of Exception for the hommade business exception, so that it can be used in a lambda expression. As it extends RuntimeException, the throws declaration is technically not really necessary on the methods, but I like to have them there so that the reader immediately sees that this exception type is to be expected.


Agent that steps and turns on a grid The following function uses a cartesian system and has an agent who moves on the table like: F - forward, L - turn left, R - turn right.

The code works fine but I would like to make it more efficient:

import java.util.Scanner;

public class HelloWorld {
  public static void main (String[] args) {
      int x = 0, y = 0;                                 //initial coordinates
      int direction = 0;                                //initial direction
    Scanner scanner = new Scanner(System.in);
    System.out.print("Introduce the command: ");
    String inputString = scanner.nextLine();            //get the command

    for (int i = 0; i < inputString.length(); i++){
        char c = inputString.charAt(i);                 //breaking the string into chars
        if(c == 'l') {                                  //determining the direction
            direction++;
        } else if (c == 'r') {
            direction--;
        } else if (c == 'f') {
            if(direction < 0) {
                direction +=4;
            }
            if(direction % 4 ==0) {                     //moving forward in the wanted direction
                y++;
            } else if (direction % 4 ==1) {
                x--;
            } else if (direction % 4 == 2) {
                y--;
            } else if(direction % 4 == 3) {
                x++;
            }
        }
    }
    System.out.println("Result: ["+x+","+y+"]");
  }
}

Consider using two variables — an x and a y component — to store the direction.  The left and right turns are based on cross product principles.

Then, each forward move can be done using unconditional additions with no modulo operations.  An additional advantage is that the meaning of the direction is self-evident.  (It's not immediately obvious what direction = 1 would mean in your code.)

import java.util.Scanner;

public class CartesianAgent {
    public static void main(String[] args) {
        int x = 0, y = 0;
        int dx = 0, dy = 1;

        Scanner scanner = new Scanner(System.in);
        System.out.print("Introduce the command: ");
        String command = scanner.nextLine();

        for (int i = 0; i < command.length(); i++) {
            switch (command.charAt(i)) {
              case 'l':
                int old_dx = dx;
                dx = -dy;
                dy = old_dx;
                break;
              case 'r':
                old_dx = dx;
                dx = dy;
                dy = -old_dx;
                break;
              case 'f':
                x += dx;
                y += dy;
                break;
            }
        }
        System.out.printf("Result: [%d,%d]%n", x, y);
    }
}

Alternatively, store the direction as a single integer, and use lookup tables to decode it.

According to the convention you chose, direction = 0 points up (in the +y direction), and turns left as direction increases (assuming a standard Cartesian grid that follows the right-hand rule).  I consider that to be weird.  You should either use

direction = 0 points up, then turns clockwise as direction increases (like navigation headings), or
direction = 0 points to the right (in the +x direction), then turns counterclockwise as direction increases (like polar coordinates).

The modulo operation should be done when turning, rather than when advancing.

import java.util.Scanner;

public class CartesianAgent {
    private static final int[] DX = {  0, +1,  0, -1 },
                               DY = { +1,  0, -1,  0 };

    public static void main(String[] args) {
        int x = 0, y = 0, direction = 0;

        Scanner scanner = new Scanner(System.in);
        System.out.print("Introduce the command: ");
        String command = scanner.nextLine();

        for (int i = 0; i < command.length(); i++) {
            switch (command.charAt(i)) {
              case 'l':
                direction = (direction + 3) % 4;
                break;
              case 'r':
                direction = (direction + 1) % 4;
                break;
              case 'f':
                x += DX[direction];
                y += DY[direction];
                break;
            }
        }
        System.out.printf("Result: [%s,%s]%n", x, y);
    }
}

Printing the result using System.out.printf() is a bit more readable than concatenating the string using +.


Prim's minimal spanning algorithm Is there a way to speed up this code so it is \$O(n^2)\$ instead of \$O(n^3)\$? I'd appreciate if someone showed me how to do so.

public void minimalSpanning(int sVertex)
{
    source = sVertex;

    boolean[] mstv = new boolean[maxSize];

    for (int j = 0; j < gSize; j++)
    {
        mstv[j] = false;
        edges[j] = source;
        edgeWeights[j] = weights[source][j];
    }

    mstv[source] = true;
    edgeWeights[source] = 0;

    for (int i = 0; i < gSize - 1; i++)
    {
        double minWeight = Integer.MAX_VALUE;
        int startVertex = 0;
        int endVertex = 0;

        for (int j = 0; j < gSize; j++)
            if (mstv[j])
                for (int k = 0; k < gSize; k++)
                    if (!mstv[k] && weights[j][k] < minWeight)
                    {
                        endVertex = k;
                        startVertex = j;
                        minWeight = weights[j][k];
                     }
        mstv[endVertex] = true;
        edges[endVertex] = startVertex;
        edgeWeights[endVertex] = minWeight;
    } //end for
} //end minimalSpanning

Just some points:

Naming:

What does mstv mean? I assumed that ms in mstv stands for minimal spanning, but what is tv then? Television?

Try clearing that up with a more "reasonable" name.
Bad Practices:

This is definitely prone to bugs:

    for (int j = 0; j < gSize; j++)
        if (mstv[j])
            for (int k = 0; k < gSize; k++)
                if (!mstv[k] && weights[j][k] < minWeight)
                {
                    endVertex = k;
                    startVertex = j;
                    minWeight = weights[j][k];
                 }

Even though there might be no bugs in the code above, accidental mistakes do occur more if you lack braces.

Instead, add the braces, which makes the code both more readable and less error-prone:

for (int j = 0; j < gSize; j++) {
    if (mstv[j]) {
        for (int k = 0; k < gSize; k++) {
            if (!mstv[k] && weights[j][k] < minWeight) {
                endVertex = k;
                startVertex = j;
                minWeight = weights[j][k];
            }
        }
    }
}

(Note that this is using my coding habits; you may move the braces where you are comfortable with)


Award Budget Cuts implementation in Java PRAMP Award Budget Cuts

  The awards committee of your alma mater (i.e. your college/university) asked for your assistance with a budget allocation problem they’re facing. Originally, the committee planned to give N research grants this year. However, due to spending cutbacks, the budget was reduced to newBudget dollars and now they need to reallocate the grants. The committee made a decision that they’d like to impact as few grant recipients as possible by applying a maximum cap on all grants. Every grant initially planned to be higher than cap will now be exactly cap dollars. Grants less or equal to cap, obviously, won’t be impacted.

  Given an array grantsArray of the original grants and the reduced budget newBudget, write a function findGrantsCap that finds in the most efficient manner a cap such that the least number of recipients is impacted and that the new budget constraint is met (i.e. sum of the N reallocated grants equals to newBudget).

  Example:

input:  grantsArray = [2, 100, 50, 120, 1000], newBudget = 190

output: 47 # and given this cap the new grants array would be
           # [2, 47, 47, 47, 47]. Notice that the sum of the
           # new grants is indeed 190

My approach:

import java.util.Arrays;

class Solution {

  static double findGrantsCap(double[] grantsArray, double newBudget) {
    // your code goes here
    int len = grantsArray.length;

    Arrays.sort(grantsArray);

    //Calculate the newBudget
    double modBudget = newBudget;
    double cap;
    double sum = 0;
    double avg;
    for (double elem: grantsArray)
      sum += elem;

    avg = sum/len;

    if( avg == (int)newBudget)
        return (newBudget)/len;
    else
    {
      Arrays.sort(grantsArray);
      modBudget -= grantsArray[0];
      cap = modBudget/(len - 1);
      for( int i = 1; i < len-1; i++ )
      {
          System.out.println(cap);
           if( (int)cap > grantsArray[i] )
           {
             modBudget -= grantsArray[i];
             cap = modBudget/(len - i - 1);
           }
      }
    }
    return cap;
  }

  public static void main(String[] args) {
    double [] grantsArray = {2,100,50,120,167};
    double newBudget = 400;

    System.out.println(findGrantsCap(grantsArray,newBudget));
  }

}

I have the following questions with regards to the above code:

How can I further improve the algorithm, time or space wise?
Is my solution satisfying all the test cases and why is it failing if any?
Have I made any gross violations to the Java Coding conventions?

Reference

I believe that the problem in the first place is not well defined (or we are trying to solve a different problem). And I mean...

The problem states the following:

  write a function findGrantsCap that finds in the most efficient manner a cap such that the least number of recipients is impacted and that the new budget constraint is met

So it focuses on the recipients impacted and not on the amounts.

e.g

Input:  grantsArray = [2, 100, 50, 120, 1000], newBudget = 190

If I pick for cap the 100 value the array becomes like this

Input:  grantsArray = [2, 100, 50, 1, 37], newBudget = 190

Now only 2 recipients affected (of course the sum of the array elements meets the newBudget constraint), but it's totally unfair.


Modified Fibonacci Implementation Purpose

This question comes from a HackerRank problem called Fibonacci Modified. I think I have coded a valid solution, but would like feedback about my approach, implementation, and of course, any errors / edge cases that I may have missed.

Task

Implementation

public class FibonacciModifiedImpl implements FibonacciModified {

    @Override
    public BigInteger calculateNextTerm(final BigInteger currentTerm, final BigInteger lastTerm) {
        if (-1 == lastTerm.compareTo(BigInteger.ZERO) ) {
            throw new IllegalArgumentException("last term must be non-negative");
        }

        if (-1 == currentTerm.compareTo(lastTerm)) {
            throw new IllegalArgumentException("current term must be greater than or equal to last term");
        }

        final BigInteger squaredTerm = new BigDecimal(Math.pow(currentTerm.doubleValue(), 2)).toBigInteger();
        return squaredTerm.add(lastTerm);
    }

    @Override
    public BigInteger calculateNthTerm(final BigInteger firstTerm, final BigInteger secondTerm, final int n) {
        if (-1 == firstTerm.compareTo(BigInteger.ZERO) ) {
            throw new IllegalArgumentException("first term must be non-negative");
        }

        if (-1 == secondTerm.compareTo(firstTerm)) {
            throw new IllegalArgumentException("second term must be greater than or equal to last term");
        }

        if (n < 1) {
            throw new IllegalArgumentException("number of terms must be greater than 0");
        }

        switch (n) {
            case 1: {
                return firstTerm;
            }

            case 2: {
                return secondTerm;
            }

            default: {
                int counter = 2;
                BigInteger currentTerm = secondTerm;
                BigInteger lastTerm = firstTerm;
                while (counter < n) {
                    BigInteger nextTerm = calculateNextTerm(currentTerm, lastTerm);
                    lastTerm = currentTerm;
                    currentTerm = nextTerm;
                    counter++;
                }

                return currentTerm;
            }
        }
    }
}

Generally looks good. Except

    final BigInteger squaredTerm = new BigDecimal(Math.pow(currentTerm.doubleValue(), 2)).toBigInteger();

There is absolutely no reason to cast Math in. The computations are supposed to be exact, which Math is notoriously lacking. I recommend

    final BigInteger squaredTerm = currentTerm.multiply(currentTerm);


Populate a networkx graph with tuples of varying lengths I've got a list with tuples of variable length containing nodes, out of which I want to build a network:

node_list = [("one", "two"), ("eins", "zwei", "drei"),
             ("un", "deux", "trois", "quattre"), ("two", "zwei", "deux")]

Each tuple represents a subgroup in the network. I want to add each node with edges to any other member of the same tuple.

It is straightforward to use .add_edges_from(), which only expects a tuple with two nodes. For a tuple with three entries one would have to write .add_edges_from(("eins", "zwei"), ("eins", "drei"), ("zwei", "drei")). Tuples with four members require even more code.

Now I am looking for the most efficient way to populate a network given a list like node_list. My idea is:

import networkx as nx

G = nx.Graph()
for node_tuple in node_list:
    for node1 in node_tuple:
        for node2 in node_tuple:
            G.add_edge(node1, node2)

It's probably very inefficient because it sees every node twice and also adds self-loops.

Tuples of varying lengths are a bit unusual.  Perhaps those would be better as lists instead.

itertools is your friend.  You can write, equivalently:

import itertools

G = nx.Graph()
for node_tuple in node_list:
    G.add_edges_from(itertools.product(node_tuple, node_tuple))

If you want to list each pair of nodes just once instead of twice, and including self-edges use itertools.combinations_with_replacement(node_tuple, 2) instead.


Creating a ticker thread - version 2 In this question, I asked for feedback on a class that provided my program with periodical signals.
I have rewritten that class based on the feedback from the accepted answer. As this again uses a complex library I've never worked with before, I'd really appreciate some feedback!

The class runs a function in a separate thread using std::async. Every interval seconds, this function sends a 'tick' to the other parts of my program using my ZeroMQ wrapper classes Client and Message. This is thread-safe, because all other behaviour in my program is also triggered via messages read by the same ZeroMQ socket that is reading these 'ticks'.

Some notes:

In the accepted answer to the previous question, it was suggested to use chrono literals, but as I am restricted to gcc 4.8.5 and thus c++11, I can't do this, so I am using an int respresenting a number of seconds.
I instantiate Client in the asynchronously called function so it cannot be used from another thread, but I should probably move the instantiation to the constructor as it uses a ZeroMQ context that is created in another thread.
The accepted answer to the previous question included some generalizations. I left these out of my implementation, because they were unnecessary in this application and this kept the code simpler.
async_future is not used, but I need to store it, otherwise async won't return.

And finally, the actual code:

Ticker.hpp

#ifndef TICKER_HPP
#define TICKER_HPP

#include <future>
#include "ipc/Client.hpp"
#include "ipc/Message.hpp"

namespace sdp {

    class Ticker {
    private:
        const std::chrono::seconds interval;
        std::promise<void> done_promise;
        std::shared_future<void> done_future;
        std::future<void> async_future;
    public:
        Ticker(unsigned int interval);
        Ticker(const Ticker& orig) = delete;
        virtual ~Ticker();
        void stop();
    private:
        void tickfunction();
    };
}

#endif /* TICKER_HPP */

Ticker.cpp

#include "Ticker.hpp"
#include <chrono>

using namespace std;
using namespace sdp;

Ticker::Ticker(unsigned int interval)
: interval(interval), done_promise(), done_future(done_promise.get_future()) {
    async_future = async(launch::async, &Ticker::tickfunction, this);
}

Ticker::~Ticker() {
    stop();
}

void Ticker::stop() {
    done_promise.set_value();
}

void Ticker::tickfunction() {
    Client socket("inproc://ticker");

    std::future_status status;
    do {
        status = done_future.wait_for(interval);
        if (status == std::future_status::timeout) {
            Message message;
            message.addEmptyPart();
            message.add("tick");
            socket.send(message);
        }
    } while (status != std::future_status::ready);
}

Here are some things that may help you further improve your code.

Clearly separate interface and implementation

The Ticker.hpp includes two headers, ipc/Client.hpp and ipc/Message.hpp which are not actually part of the interface.  They're part of the implementation and so those #includes should be moved to the .cpp file instead.

Don't throw an exception from a destructor

A destructor really really shouldn't throw an exception, but yours might.  Consider this simple main():

int main() {
    sdp::Ticker t(1);
    t.stop();
}

This will throw an error because stop attempts to set the done_promise value and then the destructor calls stop and attempts to set it again.  That second attempt is guaranteed to throw an error.

Set an exception in the promise

It seems likely that your code for constructing and sending a message could throw an exception.  If they do, the program will seem to silently fail.  Consider using std::promise::set_exception to propagate the error out to the caller so that it could be handled.

In particular, the code for tickfunction might include something like this:

    try {
        throw runtime_error("aaaah!");
    } catch (...) {
        valuePosted = true;
        throw;
    }

The calling function could then include something like this:

try {
    t.get();
} catch (std::exception &err) {
    std::cout << err.what() << '\n';
}

This allows you to catch an exception that might have been thrown within the spawned thread.

Consider making the class generic

One can easily imagine that doing something on a regular basis (i.e. every x seconds) might be useful beyond this singular usage.  For that reason, it might be useful to think about either passing a void function into the constructor or creating a template that uses a Callable type for later invocation.


Mini Binary Tree "library" This is my first attempt at learning binary trees. I'm looking for general feedback on how it could be made cleaner, more idiomatic, or more efficient.

data Tree a = ETree | Node (Tree a) (Tree a) a deriving (Show)

tInsert :: Ord o => o -> Tree o -> Tree o
tInsert x ETree = Node ETree ETree x
tInsert x (Node lT rT a)
    | x < a = Node (tInsert x lT) rT a
    | x > a = Node lT (tInsert x rT) a
    | otherwise = Node lT rT x

-- Folds a list into a tree
tFromListL, tFromListR :: Ord o => [o] -> Tree o
tFromListL = foldl (flip tInsert) ETree
tFromListR = foldr tInsert ETree

-- Turns a tree into a list
tToList :: Ord o => Tree o -> [o]
tToList ETree = []
tToList (Node lT rT a) = (tToList lT) ++ [a] ++ (tToList rT)

-- Splits a list roughly in half (as part of balancing)
splitInHalf :: [a] -> ([a],[a])
splitInHalf xs = splitAt (round $ (fromIntegral $ length xs) / 2.0) xs

-- Returns how unbalanced a node is
tUnbalancedBy :: Tree a -> Int
tUnbalancedBy ETree = 0
tUnbalancedBy (Node lT rT _) = absDiff (tDepth lT) (tDepth rT)

-- Arranges a list in such a way that it forms a more balanced tree
balanceList :: [a] -> [a]
balanceList xs = let (fH,sH) = splitInHalf xs in (reverse fH) ++ sH

-- "Inefficient balance"
tIneffBalance :: Ord o => Tree o -> Tree o
tIneffBalance = tFromListL . balanceList . tToList

-- Finds the min/max values of a tree
tMin, tMax :: Ord o => Tree o -> o
tMin ETree = error "tMin called on an Empty Tree"
tMin (Node lT _ a) = case lT of
    ETree           -> a
    (Node lT' _ _)  -> tMin lT'

tMax ETree = error "tMax called on an Empty Tree"
tMax (Node _ rT a) = case rT of
    ETree           -> a
    (Node _ rT' _)  -> tMax rT'

-- Find the max depth of a tree
tDepth :: Tree a -> Int
tDepth ETree = 0
tDepth (Node lT rT _) = 1 + max (tDepth lT) (tDepth rT)

-- Finds how many nodes a tree contains
tSize :: Tree a -> Int
tSize ETree = 0
tSize (Node lT rT _) = 1 + (tSize lT) + (tSize rT)

absDiff :: Int -> Int -> Int
absDiff x y = abs $ x - y

-- Checks if a node is balanced
tIsBalanced :: Tree a -> Bool
tIsBalanced ETree = True
tIsBalanced n
    | tUnbalancedBy n > 2 = False
    | otherwise = True

-- Checks if a value is an element of the tree
tElem :: Ord o => o -> Tree o -> Bool
tElem x ETree = False
tElem x (Node lT rT a)
    | x < a = tElem x lT
    | x > a = tElem x rT
    | otherwise = True

tDelete :: Ord o => o -> Tree o -> Tree o
tDelete _ ETree = ETree
tDelete _ n@(Node ETree ETree _) = n -- Or give "Not found" error?
tDelete tD n@(Node lT rT a)
    | tD < a = Node (tDelete tD lT) rT a
    | tD > a = Node lT (tDelete tD rT) a
    | otherwise = case (lT,rT) of
        (ETree,t)   -> t
        (t,ETree)   -> t
        (t,t')      -> let fMin = tMin t' in Node t (tDelete (tMin t') t') fMin

My main concerns are:

The balancing algorithm. I thought it would work beautifully at first, then I realized that it basically just turns the tree into a "V". It's more efficient then a linked list, but still not very balanced
The 2 "tToList*" functions. Should I use foldl or foldr? They seem to act fairly equivalently; although foldl is easier to use because you typically read a list from the left
The "tDelete" function. It works (from my testing), but it looks very ugly. I basically just wrote it case-by-case, which made it very long.

Function Visibility

In the code there's some functions that seem to be intended for internal use only. To hide these from external modules you should have a "proper" module declaration that exports those functions you want to make "public".

module MyTree ( Tree(ETree, Node), tInsert, tFromListL, tFromListR, ... ) where

this allows you to hide (and change) implementation details of your exposed function.

Balancing Your Tree

Your algorithm to balance that tree is something along the lines of \$\mathcal{O}(3 * n)\$. That's bad compared to an almost constant time-effort when balancing in tree form. Then again why are you even exposing this functionality?

If you allow a tree to be unbalanced, you shouldn't expose an operation to balance it and vice versa. You should strongly consider extracting balanced behaviour into either a separate type and using instance to expose a common "interface" or extract this into a completely separate module.

Something like this:

class (Ord a) => Tree a where
    tInsert, tDelete :: a -> Tree a -> Tree a
    tElem :: a -> Tree a -> Bool
    tIsBalanced :: Tree a -> Bool
    tSize, tDepth :: Tree a -> Int
    tMin, tMax :: Tree a -> a
    tToList :: Tree a -> [a]

data BalancedTree a = Empty | Node (BalancedTree a) (BalancedTree a) a deriving (Show)

instance NormalTree BalancedTree where
-- | because we are an instance of NormalTree we can access tInsert'
    tInsert = balance . tInsert'
-- and so on and so forth

data NormalTree a = Empty | Node (Tree a) (Tree a) a deriving (Show)

instance Tree NormalTree where
    tInsert = tInsert'

    -- | default implementation for insert without balancing
    tInsert' :: a -> Tree a -> Tree a

  Note that I haven't even used this myself and I'm not sure whether
  this works. The code should give you a gist as to how this could /
  should work and isn't even intended to be syntactically correct.

This defines the "interface" of Tree in a class and then uses data types to implement said "interface".

Errors In Expected Circumstances

When you call tMax or tMin respectively on an ETree (which would be better named as Empty) an error is raised. That's an IMO really ugly and declarative way of solving this. It'd be preferrable to have a pure function. This requires a small change to the type-signature:

tMin, tMax :: Ord o => Tree o -> Maybe o

Which in turn allows you to do the following:

tMin Empty = Nothing
tMin (Node lT _ a) = case lT of
    Empty -> a
    (Node lT' _ _) -> tMin lT'

This function is total and pure. As such it's much easier and cleaner to use.


Haskell game loop with keyboard handler This is function, that works correctly, but it seems to be too ugly.
panel argument is never used for anything but pushing into refreshGamePanel,
and it seems to be too much case statements. How can I cleanup this?

runGameLoop :: Game -> Panel -> Curses GameResult
runGameLoop game panel
  = if victory game then return Victory else
      refreshGamePanel game panel >> render >> getPanelWindow panel >>=
        flip getEvent (Just 1)
        >>=
        \ event' ->
          case event' of
              Nothing -> runGameLoop game panel
              Just event -> case event of
                EventSpecialKey (KeyFunction 2) -> return Restart
                EventSpecialKey (KeyFunction 10) -> return Quit
                EventSpecialKey key' -> case keyToDirection key' of
                  Nothing -> runGameLoop game panel
                  Just dir -> runGameLoop (makeMove game dir) panel
                _ -> runGameLoop game panel

There is a nice language extension in GHC called ViewPatterns. Together with LambdaCase that fature allows you to shorten your code, I think. And do notation will be much more readable for imperative part.

runGameLoop game panel | victory game = return Victory
                       | otherwise = continue
    where
      anyEvent = gatPanelWindow panel >>= flip getEvent (Just 1)
      isRestart = (EventSpecialKey (KeyFunction 2) ==)
      isQuit = (EventSpecialKey (KeyFunction 10) ==)
      continue = do
          refreshGamePanel game panel
          render
          anyEvent >>= \case
              Just (isRestart -> True) -> return Restart
              Just (isQuit -> True) -> return Quit
              Just (EventSpecialKey (keyToDirection -> Just dir)) ->
                  runGameLoop (makeMove game dir) panel
              _ -> runGameLoop game panel

Another variant is to consider using of Monad Maybe instance

runGameLoop game panel = nextStep where
    anyEvent = gatPanelWindow panel >>= flip getEvent (Just 1)
    nextStep = if victory game then return Victory else do
        refreshGamePanel game panel
        render
        liftM react anyEvent >>= \case
            Just continuation -> continuation
            Nothing -> runGameLoop game panel
    react event' = do
        event' >>= \case
            EventSpecialKey (KeyFunction 2) -> return (return Restart)
            EventSpecialKey (KeyFunction 10) -> return (return Quit)
            EventSpecialKey key -> do
                dir <- keyToDirection key
                return (runGameLoop (makeMove game dir) panel)
            _ -> Nothing


Extending Python list functionalities I am extending the functionalities of a Python list and I would like to include a method to normalize a vector to the [0, 1] range, by using element-wise operations. I came out with this solution, but find that using two classes does not seem clean. The main motivation for using two classes is that the output of data - min(data) from normalize() returns a Python list (due to how __sub__() was implemented), and that native list does not seem to have __truediv__() implemented.

How can I achieve the normalize() method and avoid the creation of the intermediate _BaseList class? The project I am working on has very constrained memory and I cannot use Numpy.

class _BaseList(list):
    def __init__(self, data):
        super().__init__(data)

    def __sub__(self, value):
        if type(value) in (int, float):
            return [elem - value for elem in self]
        elif type(value) is list and len(value) == len(self):
            return [a - b for a, b in zip(value, self)]

    def __truediv__(self, value):
        if type(value) in (int, float):
            return [elem / value for elem in self]
        elif type(value) is list and len(value) == len(self):
            return [a / b for a, b in zip(value, self)]

class Array(_BaseList):

    def __init__(self, data=None):
        super().__init__(data)

    def normalize(self):
        print(type(self))
        return _BaseList((self - min(self))) / float(max(self) - min(self))

You say you need the extra class, "due to how __sub__() was implemented"; because it returns a vanilla list, with a list comprehension. However, note:

If you replace e.g.

return [elem - value for elem in self]

with:

return _BaseList(elem - value for elem in self)

__sub__ will return a _BaseList instead; and
If you put those four methods in one class calling normalize would still work anyway, because you do convert to _BaseList after the subtraction.

In fact, three methods, because the __init__ is redundant - if all your subclass method does is call the superclass version, you can just let Python handle that for you.

The Zen of Python states that

  Errors should never pass silently.

However, in both __truediv__ and __sub__, if value is not one of the three specified types (or it's a list of the wrong length), they quietly return None. Instead, you should raise TypeError if the user passes a value that can't be handled.

Speaking of specified types, don't write things like:

if type(value) in (int, float)

If you need to check types, use isinstance - this supports inheritance better:

if isinstance(value, (int, float)):

This is particularly important when you're using inheritance yourself - what happens if you try to subtract a _BaseList from another _BaseList? Python supplies abstract base classes that can help make this usage more generic.

Also note that there is another numeric type: complex - as Mathias Ettinger pointed out in the comments you can use Number when you want to cover all three cases.

There's a lot of duplication between the __sub__ and __truediv__ implementations; you could factor this out by extracting to a method that also takes the operator to apply, then use the operators defined in operator.

from collections.abc import Sequence
from operators import sub, trudiv

class Array(list):

    def normalize(self):
        min_ = min(self)  # calculate this once
        return (self - min_) / (max(self) - min_)

    def __sub__(self, other):
       return self._process(other, sub)

    def __truediv__(self, other):
       return self._process(other, truediv)

    def _process(self, other, op):
        if isinstance(other, Sequence):
            if len(other) == len(self):
                return Array(op(a, b) for a, b in zip(self, other))
            raise ValueError('cannot operate on a sequence of unequal length')
        return Array(op(a, other) for a in self)

In use:

>>> a = Array((7, 8, 9))
>>> a
[7, 8, 9]
>>> a - 2
[5, 6, 7]
>>> a - [1, 2, 3]
[6, 6, 6]
>>> a.normalize()
[0.0, 0.5, 1.0]
>>> a - 'abc'
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 6, in __sub__
  File "<stdin>", line 12, in _process
  File "<stdin>", line 12, in <genexpr>
TypeError: unsupported operand type(s) for -: 'int' and 'str'
>>> a - [1, 2, 3, 4]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 6, in __sub__
  File "<stdin>", line 13, in _process
ValueError: cannot operate on a sequence of unequal length

It might also be worth implementing __repr__, so you can tell more easily when you have a vanilla list and when you have an Array.


Efficient and fast way to send data from Android to server I have an app that will be sending images ranging from few KB to ~20MB and I need to write a code that will do that in the fastest and most efficient way. I currently have a code that does this, but I fear that it's inefficient and slow:

// Read bitmap from file
Bitmap bitmap = null;
Uri uri = Uri.fromFile(new File("camera/myfile.jpg"));
InputStream stream = null;

try {
    stream = getContentResolver().openInputStream(uri);
    BitmapFactory.Options options = new BitmapFactory.Options();
    bitmap = BitmapFactory.decodeStream(stream, null, options);
} catch (IOException e) {
    Log.e(TAG, e.getMessage(),e);
    return null;
} finally {
    try {
        if (stream != null) stream.close();
    } catch (Exception e) {}
}

// Send image data to server
ByteArrayOutputStream byteStream = null;

try {
    // Create HTTP objects
    DefaultHttpClient httpClient = new DefaultHttpClient();
    BasicHttpParams hparams = new BasicHttpParams();
    HttpConnectionParams.setConnectionTimeout(hparams,10*1000);
    httpClient.setParams(hparams);
    HttpPost httpPost = new HttpPost(my_servers_url);

    byteStream = new ByteArrayOutputStream();
    bitmap.compress(Bitmap.CompressFormat.JPEG, 100, byteStream);

    // Convert ByteArrayOutputStream to byte array. Close stream.
    byte[] byteArray = byteStream.toByteArray();
    byteStream.close();
    byteStream = null;

    // Send HTTP Post request
    httpPost.setEntity(new ByteArrayEntity(byteArray));
    String response = EntityUtils.toString(httpClient.execute(httpPost).getEntity(), HTTP.UTF_8);

} catch (Exception e) {
    e.printStackTrace();
} finally {
    try {
        if (byteStream != null) byteStream.close();
    } catch (Exception e) {}
}

My concern is that the entirety of the image is loaded into a byte array on the device before being sent to the server. Although this code works, it is slower than what I'm satisfied with.

Can this code be faster and more efficient? Would it be better to use HttpURLConnection instead? If so, how would I write the code?

The code can be made more memory efficient if you refactor it to use HttpURLConnection.

If you wish to write your own solution to upload the video file instead of using a 3rd party library, you can use HttpURLConnection to create a "read-write" while loop.  In this while-loop, while bytes have been read, read either InputFileStream up to a maxBufferSize OR available bytes left in the InputFileStream and store it into a buffer array (max array size is maxBufferSize). Then write this buffer array into HttpURLConnection's DataOutputStream.

If you can post new code that implements this approach in a background thread/service, then we could review that.


JavaScript moneyFormat() function Using JSX syntax I wrote a function that takes either a float or an integer as a parameter and returns a formatted string. I'd like any feedback for refactoring the code.

const moneyFormat = (float) => {
  let answer = [];
  let firstHalf;
  let secondHalf;

  // check if whole number
  if (float % 1 === 0) {
    float = Math.trunc(float);
    firstHalf = float.toString();
    secondHalf = '.00';
  } else {
    float = float.toFixed(2);
    const splitNumArr = float.split('.');
    firstHalf = splitNumArr[0];
    secondHalf = '.' + splitNumArr[1];
  }

  let everyThree = 0;
  // push chars into answer array
  for (let i = firstHalf.length - 1; i >= 0; i--) {
    answer.push(firstHalf[i]);
    everyThree += 1;
    // insert space for every three chars
    if (everyThree % 3 === 0) {
      answer.push(' ');
    }
  }
  // reverse array and add secondHalf
  answer = answer.reverse().join('') + secondHalf;
  return answer;
}

console.log(moneyFormat(2310000.159897)); // '2 310 000.16'
console.log(moneyFormat(1600)); // '1 600.00'

Bad variable names. dollars and cents would be more appropriate that firsthalf and secondhalf... I mean, c'mon, these variables don't represent "halfs" of anything. float isn't very descriptive either. Perhaps rawvalue would be better, and maybe result_buffer would be better than answer.
The entire code for whole numbers is unnecessary. When you run toFixed() on a whole number it becomes a float (string) anyway.
Rather than creating a new variable and arbitrarily swapping between let and const, why not just reassign float (or in my version, raw_value). You won't need it anymore after that anyway.
You might wanna consider casting it to a float in case a string is passed to the function, in which case your toFixed would fail.
It doesn't make a hue difference whether you declare your variables at the top or as needed when you're working on a small function, but be consistent. And if you do declare them all at the top, I would suggest doing it in a single statement for a small performance boost. In this case, I would personally just declare them as you go to keep the function concise.
When you're doing a backwards loop like that you don't need all three statements in the for loop. You could just do for (let i = firstHalf.length; i--;). That said, looping backwards and then reversing (or looping at all for that matter) makes for smelly code. You can just use a little regular expression to chunk the chars up: /.{1,3}/g
Don't reassign a variable just to return it, just return it. It's less work for the computer that way.

const moneyFormat = (raw_value) => {
  raw_value = parseFloat(raw_value).toFixed(2).split('.');
  let dollars = raw_value[0],
    cents = '.' + raw_value[1],
    format_buffer = dollars
      .split('').reverse().join('') // reverse it
      .match(/.{1,3}/g) // chunk it
      .map(c=>c.split('').reverse().join(''))
      .reverse(); // reverse it again
  return format_buffer.join(' ') + cents;
}

console.log(moneyFormat(2310000.159897)); // '2 310 000.16'
console.log(moneyFormat(1600)); // '1 600.00'
console.log(moneyFormat("1600"));
console.log(moneyFormat("234123.2134123"));


Building a dict/list of lists I have code which is matching features within regions. The first thing it does it matches features from one region to an entire database of region features.

I have a mapping from query feature index (qfx) into database feature index (ax).
A database feature index has associated with it a region id (rid), region feature index (fx), feature score (fs), and feature rank (fk).
There is also a flag marking each of those mappings as valid or invalid.

The problem is that I want to go from this mapping of query_feature_index-to-database_feature_index (qfx2_ax) into a mapping from region_index-to-feature_match (rid2_fm, rid2_fs, and rid2_fk)

I have the following code which does this (cleaned up just a little bit for clarity). On the left hand side is the rough percentage of time each step takes. The bulk of the time seems to be eaten up by appending to the lists in the default dictionaries.

  0.0      for qrid in qrid2_nns.iterkeys():
  0.0          (qfx2_ax, _) = qrid2_nns[qrid]
  0.0          (qfx2_fs, qfx2_valid) = qrid2_nnfilt[qrid]
  0.0          nQKpts = len(qfx2_ax)
               # Build feature matches
  0.0          qfx2_nnax = qfx2_ax[:, 0:K]
  0.4          qfx2_rid  = ax2_rid[qfx2_nnax]
  0.5          qfx2_fx   = ax2_fx[qfx2_nnax]
  0.2          qfx2_qfx = np.tile(np.arange(nQKpts), (K, 1)).T
  0.1          qfx2_k   = np.tile(np.arange(K), (nQKpts, 1))
               # Pack valid feature matches into an interator
  0.4          valid_lists = [qfx2_[qfx2_valid] for qfx2_ in (qfx2_qfx, qfx2_rid, qfx2_fx, qfx2_fs, qfx2_k,)]
  0.0          match_iter = izip(*valid_lists)
  0.0          rid2_fm = defaultdict(list)
  0.0          rid2_fs = defaultdict(list)
  0.0          rid2_fk = defaultdict(list)
               # This creates the mapping I want. Can it go faster?
 19.3          for qfx, rid, fx, fs, fk in match_iter:
 17.2              rid2_fm[rid].append((qfx, fx))
 17.0              rid2_fs[rid].append(fs)
 16.2              rid2_fk[rid].append(fk)

My gut feeling is that I could pass over the data twice, first counting the number of entries per region, then allocating lists of that size, and then populating them, but I'm afraid that indexing into python lists might take a comparable amount of time.

Then I was thinking I could do it with list comprehensions and build a list of (qfx, fx, fs, and fk), but then I'd have to unpack them, and I'm unsure about how long that will take.

It seems to me that I can't get much better than this, but maybe someone out there knows something I don't. Maybe there is a numpy routine I'm unaware of?

I'm more looking for guidance and insights than anything else before I start coding up alternatives.

What would you think of

match_iter = izip(*valid_lists)
rid2 = defaultdict(list)
for qfx, rid, fx, fs, fk in match_iter:
    rid2[rid].append(qfx, fx, fs, fk)

Seems like that might cut the time in the final loop by almost 50%. The extraction from match_iter doesn't change, so it will take the same amount of time. The append will take about the same amount of time as one of the appends you're already doing. Since two appends will be eliminated, I think the time they're taking will go away.