Calculation of image offsets for performing cropping I am using the java.awt.Rectangle class to construct subsets of a GEOTIFF file. In order to do this I would need to specify the x,y offsets, height and width of each subset image. In my particular case I would need to crop the original GEOTIFF image bottom to top. The java.awt.Rectangle class specifies that the origin point is to be found in the upper left hand corner.

Please let me know if the calculation of the image offsets are correct and/or there is a better way of calculating the image offsets. The crop(r) method is a proprietary method used to crop subsets of the original image.

int width = 40;
int height = 34;
int cellSize = 3600;
int xOffset = 0;
int yOffset = 0;
int pixelWidth = cellSize * width;
int pixelHeight = cellSize * height;
for (int i = 0 ; i < width; i++)
{
    for (int j = 0; j < height; j++)
    {
        yOffset = pixelHeight - cellSize *(j+1) ;
        Rectangle r = new Rectangle(xOffset,
                                    yOffset,
                                    cellSize,
                                    cellSize);
        crop(r);
    }
    xOffset = pixelWidth - cellSize * (i+1);
}

int width = 40;
int height = 34;
int cellSize = 3600;
int xOffset = 0;
int yOffset = 0;
int pixelWidth = cellSize * width;
int pixelHeight = cellSize * height;

I would find this easier to follow in a different order:

final int CELL_SIZE = 3600;

final int WIDTH = 40;
final int HEIGHT = 34;

final int PIXEL_WIDTH = CELL_SIZE * WIDTH;
final int PIXEL_HEIGHT = CELL_SIZE * HEIGHT;

int xOffset = 0;
int yOffset = 0;

Now  I see a natural progression of values and have the variables separate from the constants.  The variables are also closest to where they are changed.

I changed the values that never change to final to reflect that.

The ALL_CAPS notation is common for constants.

for (int i = 0 ; i < width; i++)
    {
    for (int j = 0; j < height; j++)
        {
        yOffset = pixelHeight - cellSize *(j+1) ;
        Rectangle r = new Rectangle(xOffset,
                                    yOffset,
                                    cellSize,
                                    cellSize);
        crop(r);
        }
      xOffset = pixelWidth - cellSize * (i+1);
     }

You can write this as

for (int xOffset = PIXEL_WIDTH - CELL_SIZE; xOffset >= 0; xOffset -= CELL_SIZE)
{
    for (int yOffset = PIXEL_HEIGHT - CELL_SIZE; yOffset >= 0; yOffset -= CELL_SIZE)
    {
        Rectangle r = new Rectangle(xOffset,
                                    yOffset,
                                    CELL_SIZE,
                                    CELL_SIZE);
        crop(r);
    }
}

Then you don't need i or j at all and you don't have to calculate the offsets further.

I also changed the indent of the brackets to match the more common practice of lining up with the for declarations rather than the block contents.

You may want to do this in the other order:

for (int xOffset = 0; xOffset < PIXEL_WIDTH; xOffset += CELL_SIZE)
{
    for (int yOffset = 0; yOffset < PIXEL_HEIGHT; yOffset += CELL_SIZE)
    {

As that is easier to read.  Not sure how much order matters.

If you need the original order exactly, you may need an extra loop.  The original order started with an xOffset of 0 and then went to PIXEL_WIDTH - CELL_SIZE.

If you are committed to the i and j method, consider starting them at 1, as so

for (int i = 1 ; i <= width; i++)
    {
    for (int j = 1; j <= height; j++)
        {
        yOffset = pixelHeight - cellSize * j;

Then you don't have to add 1 to i and j each time.


injectOptions jQuery plugin Over the past year I've found myself moving from back-end development to something more akin to full-stack development. I've created the following jQuery plugin as a test of my hopefully-not-so-fledgling JavaScript/jQuery skills. The plugin is used to inject <option> tags into the element(s) specified, and should follow the guidelines for creating basic plugins posted here.

(function($) {
  $.fn.injectOptions = function(opts) {
    opts = opts.map(function(opt) {
      if (typeof opt === 'string')
        return '<option>'+opt+'</option>';
      return '<option value="'+opt.key+'">'+opt.val+'</option>';
    });
    var optHTML = opts.reduce(function(html, opt) { return html+opt; });
    this.each(function() { $(this).html(optHTML); });
    return this;
  };
}(jQuery));

//example:
var fooArr = ["foo", "bar", "baz", "faz"];
var quxArr = [
  { key: 1, val: "qux"},
  { key: 2, val: "quux"},
  { key: 3, val: "quuux"},
];

$(".foo-options").injectOptions(fooArr);
$(".qux-options").injectOptions(quxArr);
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<select class="foo-options">
</select>
<select class="qux-options">
</select>
<select class="qux-options">
</select>

Any/all suggestions are welcome!

This code does not seem right:

var optHTML = opts.reduce(function(html, opt) { return html+opt; });
this.each(function() { $(this).html(optHTML); });

Here your .reduce() function is aggregating ALL the HTML for ALL of the options, which I think is an appropriate use of reduce function.  But I do not understand why there is call to each() here as this can only refer to a single element.  In fact I am guessing this doesn't even have an each() method on it , since this is an Element object.  Shouldn't this just be:

var optHTML = opts.reduce(function(html, opt) { return html+opt; }, '');
$(this).html(optHTML);

I am not seeing great utility in separate map and reduce steps. This causes you to iterate your input options an extra time that could be considered removed if you wanted to simply consolidate all this logic into the reduce step.  If the use cases are for small lists, maybe this performance isn't important and the added "clarity" of having two separate steps is acceptable.

Does it truly makes sense to support this extension on an arbitrary jQuery collection?  Right now, as this code is written, if I had HTML like this:

<select class="foo-options">
</select>
<select class="foo-options">
</select>

And I called your extension on $('.foo-options'), you would end up with duplicated options for these two different selects. Is this desirable behavior?

Rather than writing HTML strings and having to hold the aggregated string in memory, you should give some consideration to actually creating the DOM Nodes and appending them.

Along with suggestion on removing redundant iterations over options, might have you function looking like this:

$.fn.injectOptions = function(opts) {
  // not shown - perhaps validate opts is array with at least 1 element
  // and fail out if this is not the case

  // simply iterate the passed options appending each element as you go
  opts.forEach( opt => {
      var val, txt;
      if (typeof opt === 'string') {
          val = opt;
          txt = opt;
      } else {
          val = opt.key;
          txt = opt.val;
      }
      $(this).append( $.('<option>', { value: val, text: txt }) );
}


Variable declaration outside or inside when using on of the methods of Array.prototype The purpose of this script is to enforce some rules for validating a keyboard shortcuts definition. In my case I want to reject duplicates keys improperly added in the combo properties. This code rejects key combinations like CtrlAltPPwhich are not valid.

I would like to know what are the best practices regarding variable declaration placement (in/out) when method like Array.prototype.forEach are used.

Which version of the code would you suggest as better? Also, for my understanding in the first snippet, variables declared inside the function in .forEach are hoisted to that function and not to the check() function, is my assumption correct?

window.app = {
    combos: {
        a: 'Ctrl-Alt-P',
        b: 'Ctrl-Ctrl-Alt-P', // invalid
        c: 'Ctrl-Alt-P-P'// invalid
    },
    check: function () {
        var result = true,
            combos = Object.keys(this.combos).forEach(function (combo) {
                var comboWin = this.combos[combo],
                    parts = comboWin.split('-').sort(),
                    item,
                    itemNext;
                for (var i = 0, len = parts.length; i < len; i++) {
                    item = parts[i];
                    itemNext = parts[i + 1];
                    if (item === itemNext) {
                        result = false;
                        alert('DUPLICATED PART IN: '+ comboWin + ' : ' + item);
                    }
                }
            }.bind(this));
        return result;
    }
}

window.app.check();

 window.app = {
        combos: {
            a: 'Ctrl-Alt-P',
            b: 'Ctrl-Ctrl-Alt-P', // invalid
            c: 'Ctrl-Alt-P-P'// invalid
        },
        check: function () {
            var result = true,
                comboWin,
                parts,
                item,
                itemNext;
            combos = Object.keys(this.combos).forEach(function (combo) {
                comboWin = this.combos[combo],
                parts = comboWin.split('-').sort(),
                item,
                itemNext;
                for (var i = 0, len = parts.length; i < len; i++) {
                    item = parts[i];
                    itemNext = parts[i + 1];
                    if (item === itemNext) {
                        result = false;
                        alert('DUPLICATED PART IN: ' + comboWin + ' : ' + item);
                    }
                }
            }.bind(this));
            return result;
        }
    }

    window.app.check();

Concerning your variable scope, I think it is safer to define it inside the function as it will assure you get the value inside it. That way you won't be able to access a filled in parts variable after your function - in this case it makes no difference, but once you have a couple more things going on it is best to declare your variables with as little scope as possible. This is because you variables will be cleaned up automatically after your function has ended (and garbage collection has come by), which means you are freeing up memory and not filling it with data you won't use anymore.

I have been able to eliminate almost all your variables because of returns. Instead of storing something when you just need to use it to return, just return it directly. It cuts down on a lot of variables, and a lot of code.

The point is that you don't need to use variables unless you need to use them later. After deciding what variables to use, you have to decide what scope you need. Keep the scope as small as possible, so it's as predictable as possible to see what variable is available where and reduce memory consumption.

window.app = {
    combos: {
        a: 'A-A-B-C-C',
        b: 'A-B-C',
        c: 'A-B-C-C-D'
    },
    check: function () {
        // If like declaring variables at the top, it keeps the rest consistent
        return Object.keys(this.combos).forEach(function(combo){
            // `combo` is already defined, and since you don't use
            // this.combos anymore after this, dont store it in a var
            var parts = this.combos[combo].split('-').sort();
            // Also, I also recently came across this
            // clever piece on Code Review:
            return parts.forEach(function(part, index){
                // To check duplicates, just check
                // if first index != last index
                if(index != parts.lastIndexOf(part)){
                    document.write('DUPLICATED PART IN: ' + combo + ' : ' + part + '
');
                    // You can just return here.
                    // Breaks the loop and returns!
                    return false;
                }
                return true;
            });
        }, this);
        return result;
    }
}

document.write('>>>' + (window.app.check() ? 'Completed without duplicates' : 'Duplicates found.'));

I also replaced your alert with document.writes as alerts are just... annoying in my opinion. Check the snippet above to see a nice result.

One more thing: if you do want to bind a this arguments to forEach, it already provides a quick way to do that: the second argument of the forEach function is actually defined as thisArg:

arr.forEach(callback[, thisArg])

Your assumption is correct: variables declared within blocks are scoped only to that block. If, however, you omit the keyword var, they will be avail;able outside your block as they are usually appended to the top-level object in the chain, usually the window.

document.write('Executing a function that uses var to declare local scope.
');

+function(){
    var foobar = 'Foo Bar';
}();

document.write(typeof foobar !== 'undefined' ? '>>Foobar is ' + foobar : '>> Foobar is undefined.
');

/*-------------------------------*/

document.write('Executing a function that does not use var to declare local scope.
');

+function(){
    foobar = 'Foo Bar';
}();

document.write(typeof foobar !== 'undefined' ? '>> Foobar is: ' + foobar : '>> Foobar does not exist
');

/* Note: using the + sign is equal to wrapping the function in brackets etc... Its create a temporary scope an executes in it. */


Project Euler #357: Prime generating integers Project Euler Problem 357 asks:

  Consider the divisors of 30: 1,2,3,5,6,10,15,30.
  It can be seen that for every divisor d of 30, $$d+30/d$$ is prime.

  Find the sum of all positive integers n not exceeding 100 000 000
  such that for every divisor d of n, $$d+n/d$$ is prime.

import time
start = time.time()

import math

def divisor_generator(n):
    '''Generates the divisiors of input num'''
    large_divisors = []
    for i in range(1, int(math.sqrt(n) + 1)):
        if n % i == 0:
            yield i
            if i*i != n:
                large_divisors.append(n / i)
    for divisor in reversed(large_divisors):
        yield divisor

def is_prime(n):
    '''simple prime tester'''
    if n == 2 or n == 3 :
        return True
    for i in range(2,int(n**(1/2))+1):
        if n % i == 0:
            return False
    return True

def test_condition (divisor_array, num):

    ''' Testing the condition of d+n/d by taking the input as array of divisor and num'''

    if len(divisor_array) %2 != 0: # the divisors of num = d3 is [1,d2,d3], and d2*d2=d3 hence d2+d3/d2 = 2d2 which is not prime
        return False
    if sum(divisor_array) %2 != 0: # the divisors of num = d4, [1,d2,d3,d4] (1+d4)=Prime and (d2+d3)==Prime hence sum([1,d2,d3,d4]) must be even
        return False
    if len(divisor_array)%2 == 0:
        for i in range(len(divisor_array)//2):
            if is_prime(divisor_array[i] + divisor_array[-i-1]) == False:
                return False
        return True

Count = 0
for num in range(2,31,2): #since for odd num we do not have prime numbers

    divisor_list_num = list(divisor_generator(num))

    if test_condition(divisor_list_num,num) == True:

        Count += num

print(Count)
end = time.time()
print(end-start,"sec")

It's good up to \$\text{num} = 10^6\$ but then it gets slow after that.  I find that at it takes 16 min to find the \$\text{num} < 10^7\$.

How can I inrease the speed of my code ?

def divisor_generator(n):
    '''Generates the divisiors of input num'''
    large_divisors = []
    for i in range(1, int(math.sqrt(n) + 1)):
        if n % i == 0:
            yield i
            if i*i != n:
                large_divisors.append(n / i)
    for divisor in reversed(large_divisors):
        yield divisor

I would rename this just divisors, but other than that it seems like a reasonable general-purpose divisor enumerator for very small numbers with a couple of caveats: the doc comment has a typo and uses the wrong name for the input, and if this is supposed to work in Python 3 (which I'm guessing it is from the use of range rather than xrange) then it should use integer division: n // i.

However:

This task doesn't call for a general-purpose divisor enumerator. IMO it would make more sense to return pairs (i, n // i).
To scale to larger numbers it's worth finding the prime factorisation and constructing the divisors from them. The best way I know to do that is to build an array like Eratosthenes' sieve, but instead of storing a Boolean to indicate prime/composite store a prime divisor. (I generally store the lowest prime divisor, but it depends on what you want to do with it). Then you can find the prime factorisation by chaining down the array.

def is_prime(n):
    '''simple prime tester'''
    if n == 2 or n == 3 :
        return True
    for i in range(2,int(n**(1/2))+1):
        if n % i == 0:
            return False
    return True

If you're going to be testing a lot of numbers from just over 0 to N for primality then you definitely want to use a sieve, effectively testing them in parallel and caching the results.

There are a couple of PEP8 violations in this function: space before colon, no space after comma.

def test_condition (divisor_array, num):

    ''' Testing the condition of d+n/d by taking the input as array of divisor and num'''

    if len(divisor_array) %2 != 0: # the divisors of num = d3 is [1,d2,d3], and d2*d2=d3 hence d2+d3/d2 = 2d2 which is not prime
        return False

Nice special case, but it can be generalised a long way. How can you restate the test "num can be split as i and num/i which are not coprime"?

Argue this through all the way and it gives you a completely different high-level algorithm which should be much more efficient. But since this is Project Euler, I'm not going to say more than that hint.

    if sum(divisor_array) %2 != 0: # the divisors of num = d4, [1,d2,d3,d4] (1+d4)=Prime and (d2+d3)==Prime hence sum([1,d2,d3,d4]) must be even
        return False

I don't think I follow this argument. Why does the sum of more than two factors matter?

    if len(divisor_array)%2 == 0:

This condition is unnecessary, because if it failed then we've already returned False.

        for i in range(len(divisor_array)//2):
            if is_prime(divisor_array[i] + divisor_array[-i-1]) == False:
                return False
        return True

It would be more Pythonic to use all(comprehension). Also, this highlights my point about returning pairs of divisors rather than using a general-purpose divisor enumerator.


Confirm the ending of a string Challenge:

  Check if a string (first argument, str) ends with the given target
  string (second argument, target).

  This challenge can be solved with the .endsWith() method. But for the
  purpose of this challenge, do not use it.

I would like to especially have my solution critiqued.

function confirmEnding(str, target){
  let lengthOfString = str.length;
  let lengthOfTarget = target.length;
  let subtractLengths = lengthOfString - lengthOfTarget;
  let lastIndexOfString = str.lastIndexOf(target);

  if(str.includes(target) === true){
    if(lastIndexOfString === subtractLengths){
      return true;
    }
    else{
      return false;
    }
  }
  else{
    return false;
  }
}

Test Cases:

confirmEnding("Bastian", "n") should return true.
Passed
confirmEnding("Congratulation", "on") should return true.
Passed
confirmEnding("Connor", "n") should return false.
Passed
confirmEnding("Walking on water and developing software from a specification are easy if both are frozen", "specification") should return false.
Passed
confirmEnding("He has to give me a new name", "name") should return true.
Passed
confirmEnding("Open sesame", "same") should return true.
Passed
confirmEnding("Open sesame", "pen") should return false.
Passed
confirmEnding("Open sesame", "game") should return false.
Passed
confirmEnding("If you want to save our world, you must hurry. We dont know how much longer we can withstand the nothing", "mountain") should return false.
Passed
confirmEnding("Abstraction", "action") should return true.

Your solution is far too complicated.  It's also inefficient, because it uses functions .lastIndexOf() and .includes(), both of which analyze the entire str looking for target, whereas an optimal solution should look only starting at a known position at the end of str.

Here are two simple solutions:

function confirmEnding(str, target) {
  return str.startsWith(target, str.length - target.length);
}

function confirmEnding(str, target) {
  return target == str.substring(str.length - target.length);
}

console.log(
  false == confirmEnding("s", "try long target strings") &&
  false == confirmEnding("", "try an empty str") &&
  true  == confirmEnding("Bastian", "n") &&
  true  == confirmEnding("Congratulation", "on") &&
  false == confirmEnding("Connor", "n") &&
  false == confirmEnding("Walking on water and developing software from a specification are easy if both are frozen", "specification") &&
  true  == confirmEnding("He has to give me a new name", "name") &&
  true  == confirmEnding("Open sesame", "same") &&
  false == confirmEnding("Open sesame", "pen") &&
  false == confirmEnding("Open sesame", "game") &&
  false == confirmEnding("If you want to save our world, you must hurry. We dont know how much longer we can withstand the nothing", "mountain") &&
  true  == confirmEnding("Abstraction", "action")
)

The first solution, using .startsWith(), should be efficient, but it might be considered "cheating" to use .startsWith() even though only .endsWith() is prohibited.

The second solution is slightly simpler, but it involves creating a substring, so it would be less efficient.


Controlling a Windows Service from a WPF app I have a WPF Control Panel app, where I'm trying to stay close to an MVVM architecture. The control panel (the "the CP") is for a WCF service ("the Scheduler"), hosted in a Windows Service ("the Host"). My main concern, and topic for review here, is how I am avoiding blocking the CP responsiveness for too long. The Host control operations sometimes take longer than 3 seconds, which is long for a UI.

Because the start and stop methods are typed void, and I have been advised to steer well clear of using async void methods, instead I use a method I derived from an example somewhere:

private void ExecuteWithTimeout(Action task, int? timeout = null)
{
    timeout = timeout ?? HostServiceConstants.ServiceControlTimeout;
    var source = new CancellationTokenSource();
    var timedTask = Task.Factory.StartNew(task, source.Token).ContinueWith((t) =>
        {
            if (t.Exception != null)
            {
                t.Exception.Handle(ex =>
                    {
                        var frame = new StackFrame(1);
                        var methodName = frame.GetMethod().Name;
                        _logger.Warn("'ExecuteWithTimeout' exception from: '{0}': {1}", methodName, ex.Message);
                        return true;
                    });
            }
        });
    bool done = timedTask.Wait(timeout.Value);
    if (!done)
    {
        source.Cancel();
        var frame = new StackFrame(1);
        var methodName = frame.GetMethod().Name;
        _logger.Warn("'{0}' timed out after {1} milliseconds.", methodName, timeout);
    }
}

This is called from the viewmodel, when a Command is invoked from the view:

void ExecuteStartHostCommand()
{
    Exception exception = null;
    ExecuteWithTimeout(() =>
        {
            try
            {
                _host.Start();
            }
            catch (Exception ex)
            {
                exception = ex;
            }
        });
    if (exception != null)
    {
        _logger.Error("Exception caught while trying to execute 'StartHostCommand': " + exception.Message, exception);
        if (exception is HostServiceException)
        {
            MessageBoxFacility.ServiceControlError((HostServiceException)exception);
        }
        else
        {
            throw exception;
        }
    }
}

Here, _host.Start() is a call to the model, which in turn uses a System.ServiceProcess.ServiceController to start the Host. My logging is showing such calls time out quite often, after 3000ms, but then said logging goes on to sow that the Host stopped anyway, as expected. I would like to avoid the kludgy looking ExecuteWithTimeout and rather use await, but that would involved using some async void methods, which I have been advised to avoid.

The code I have shown here is still quite experimental, and I have little experience controlling long running tasks so many steps away from the UI, so please ignore smaller  discrepancies etc.

async void methods should generally be avoided, yes, but this looks like it's one of the cases where they make sense.

var frame = new StackFrame(1);
var methodName = frame.GetMethod().Name;
_logger.Warn("'ExecuteWithTimeout' exception from: '{0}': {1}", methodName, ex.Message);

I don't see how is methodName useful here, it's always going to be Handle.

var frame = new StackFrame(1);
var methodName = frame.GetMethod().Name;
_logger.Warn("'{0}' timed out after {1} milliseconds.", methodName, timeout);

I think [CallerMemberName] would be simpler here.

throw exception;

This means that the stack trace of the exception is going to be rewritten. If you don't want to wrap the exception in another exception, you can use ExceptionDispatchInfo to preserve the stack.


Splitting strings in Java I wrote a function which takes in input a list of strings that is splitted by comma. Then each generated string is trimmed.

Example:

INPUT:    [[Cat, Dog, Snake]]
OUTPUT:   Cat Dog Snake

In particular, I'm interested in removing parenthesis and commas.
This is the code I wrote, but I think it is awful and not so efficient because I use so many String and LinkedList variables.

private List<String> splitListOfString(LinkedList<String> list)
{
String mylist = list.toString(); //is made by parenthesis, I do not want
mylist = mylist.substring(2);  //delete the first 2
mylist = mylist.substring(0, mylist.length() - 2); //delete the last 2
List<String> thelist = Arrays.asList(mylist.split(","));
List<String> thelist2 = new LinkedList<String>();
for (String string : thelist) {
    thelist2.add(string.trim());
}
    return thelist2;
}

The line of code involving the trim function is compulsory to avoid something like " string", that is undesirable. But I do not think that this is the best way to write this code.
How would you improve this?

I'm not sure how you got the original list-within-a-list input, but have you looked into Java Streams? Here's a tutorial that may help.

It looks like you're taking in a LinkedList, turning it into a string to manually delete the [[ and ]], splitting on comma, then adding each item to a new list.

Why not do something like the following:

private static List<String> splitListOfString(LinkedList<String> list) {
    List<String> myList = new LinkedList<>();
    list.stream().forEach(item -> myList.add(item));
    return myList;
}

In the above code, you're leaving the elements in the list (instead of converting to string), retrieving each item, then adding to your output list. It may need to be modified slightly to match your input.


Subclass SKSpriteNode I decided to subclass SKSpriteNode for a game test in Swift today and soon realised that it was not as simple as my Objective-C background suggested. After a little hunting around and a bit of testing in Playgrounds I have come up with the following (See below). I have not tested this in a project as I am just starting out on my next game but thought I would add this here to see if I am on the right track with this, does what I have done look right, any suggestions or comments would be most helpful.

I am pretty happy with this, but I was just curious if there was anything I was not doing, or something that I was doing that I should not be.

// ------------------------------------------------------------------- **
// SKSpriteNode subclass, NSCoding not implemented
// ------------------------------------------------------------------- **

class SimpleSprite: SKSpriteNode {
    var simpleName: String
    var simpleType: Double

    init(simpleName: String, simpleType: Double) {
        self.simpleName = simpleName
        self.simpleType = simpleType
        let spriteColor = SKColor.redColor()
        let spriteSize = CGSize(width: 10.0, height: 10.0)
        super.init(texture: nil, color: spriteColor, size: spriteSize)
    }

    required init?(coder aDecoder: NSCoder) {
        // Class does not want to be NSCoding-compatible
        fatalError("init(coder:) has not been implemented")
    }
}

let simpleSprite = SimpleSprite(simpleName: "SimpleRabbit", simpleType: 22.1)
simpleSprite.simpleName
simpleSprite.color

// ------------------------------------------------------------------- **
// SKSpriteNode subclass, NSCoding-compatible
// ------------------------------------------------------------------- **

class CodedSprite: SKSpriteNode {
    var simpleName: String
    var simpleType: Double

    init(simpleName: String, simpleType: Double) {
        self.simpleName = simpleName
        self.simpleType = simpleType
        let spriteColor = SKColor.whiteColor()
        let spriteSize = CGSize(width: 10.0, height: 10.0)
        super.init(texture: nil, color: spriteColor, size: spriteSize)
    }

    required init?(coder aDecoder: NSCoder) {
        self.simpleName = aDecoder.decodeObjectForKey("SIMPLE_NAME") as String
        self.simpleType = aDecoder.decodeDoubleForKey("SIMPLE_TYPE")
        super.init(coder: aDecoder)
    }

    override func encodeWithCoder(aCoder: NSCoder) {
        aCoder.encodeObject(self.simpleName, forKey: "SIMPLE_NAME")
        aCoder.encodeDouble(self.simpleType, forKey: "SIMPLE_TYPE")
        super.encodeWithCoder(aCoder)
    }

}

let codedSprite = CodedSprite(simpleName: "CodedRabbit", simpleType: 22.1)
codedSprite.simpleName
codedSprite.color

I'm going to ignore the non-coded class.  I'm not sure why you'd include both of these.

With that said, here are some things I noticed about the coded class.

Rather than init(simpleName:simpleType:), why don't we expand on our super class's init and also accept the arguments our super class's init takes:

init(texture: SKTexture?, color: UIColor, size: CGSize, name: String, type: Double) {
    self.simpleName = name
    self.simpleType = type
    super.init(texture: texture, color: color, size: size)
}

Then, if we want an initializer that takes just some of these values and uses defaults for the rest, we should make convenience initializers:

convenience init(name: String, type: Double) {
    self.init(texture: nil, color: UIColor.whiteColor(), size: CGSizeMake(10.0, 10.0), name: name, type: type)
}

And we probably should override any of SKNode's constructors which we intend to keep available:

override convenience init(texture: SKTexture?, color: UIColor, size: CGSize) {
    self.init(texture: texture, color: color, size: size, name: "", type: 0.0)
}

Ultimately, the point is, all of our initialization logic should go in as few constructors as possible.  All of our other constructors should call these few designated constructors.


Re-implementing basic list operations I'm re-implementing the common core functions that work on list as a learning exercise. I'm new to Haskell and often find myself staring at what I've written trying to determine the simpler or more elegant solution I'm certain exists. I'm also unsure about my naming conventions, since there are some things the style guides I've read don't really cover. I'd appreciate any advice or criticism.

foldl

foldl' :: (b -> a -> b) -> b -> [a] -> b
foldl' f accumulator [] = accumulator
foldl' f accumulator (x:xs) =
  seq accumulator' $ foldl' f accumulator' xs
  where accumulator' = f accumulator x

I wrote this for the ListOps https://exercism.io challenge, which had a requirement about strictness.

foldr

foldr :: (a -> b -> b) -> b -> [a] -> b
foldr f accumulator []     = accumulator
foldr f accumulator (x:xs) = f x (foldr f accumulator xs)

length

length :: [a] -> Int  -- Deliberately not using a fold
length = length' 0

length' :: Int -> [a] -> Int
length' accumulator [x]    = 1
length' accumulator (x:xs) = 1 + length' accumulator xs
length' accumulator []

map

map :: (a -> b) -> [a] -> [b]
map f []     = []
map f [x]    = [f x]
map f (x:xs) = f x:map f xs

filter

filter :: (a -> Bool) -> [a] -> [a]
filter _ []    = []
filter f (x:xs)
  | f x         = x : filter f xs
  | otherwise   = filter f xs

(++)

(++) :: [a] -> [a] -> [a]
xs ++ [] = xs
xs ++ ys = foldr (:) ys xs

foldl'

Your implementation is fine, but generally, seq is written infix, like a `seq` b. Therefore, I’d write it like this:

foldl' :: (b -> a -> b) -> b -> [a] -> b
foldl' f accumulator [] = accumulator
foldl' f accumulator (x:xs) =
    accumulator' `seq` foldl' f accumulator' xs
  where accumulator' = f accumulator x

I’ve also indented the second to last line to visually distinguish it more from the where clause.

foldr

Your implementation looks good to me.

length

You mention that you’re explicitly avoiding fold, but why? There’s no reason to avoid it here. Still, even without using it, your implementation can be improved.

Usually, helper functions like length' are nested inside a where clause, sometimes idiomatically named go. Additionally, your 1-element case is unnecessary, since (x:xs) will match a 1-element list. Finally, your use of an accumulator is pointless, since you never actually change it from 0, and your function isn’t tail recursive. Therefore, I would write length like this if you really want to avoid a fold:

length :: [a] -> Int
length = go 0
  where go accumulator [] = accumulator
        go accumulator (x:xs) = go (accumulator + 1) xs

This actually uses the accumulator, which allows it to be tail recursive.

map

Again, I would use a fold here. An implementation with foldr would look like this:

map :: (a -> b) -> [a] -> [b]
map f = foldr (\x xs -> f x : xs) []

However, if you want to write the recursion manually, your 1-element case is once again redundant. You could simplify your implementation to this:

map :: (a -> b) -> [a] -> [b]
map _ []     = []
map f (x:xs) = f x : map f xs

filter

Once again, I would use a fold here:

filter :: (a -> Bool) -> [a] -> [a]
filter f = foldr (\x xs -> if f x then x : xs else xs) []

If you want to use explicit recursion, though, your implementation looks fine.

(++)

Your 0-element case is redundant here because foldr will handle the 0-element case just fine on its own. You can eliminate it entirely:

(++) :: [a] -> [a] -> [a]
xs ++ ys = foldr (:) ys xs


Are there pitfalls to this solution to read messages from a queue in parallel? I've posted a question on stackoverflow: How can I consequently read messages from a queue in parallel? I would like my own answer to be reviewed.
Situation
We have one message queue. We would like to process messages in parallel and limit the number of simultaneously processed messages.
Our solution
Based on answers given and some research of our own, we've come to a solution. We're using a SemaphoreSlim to limit our number of parallel Tasks.
Are there any pitfalls to this solution?
I'm also interested in any better solutions.
static string queue = @".\Private$\concurrenttest";

private static async Task Process(CancellationToken token)
{
    MessageQueue msMq = new MessageQueue(queue);
    msMq.Formatter = new XmlMessageFormatter(new Type[] { typeof(Command1) });
    SemaphoreSlim s = new SemaphoreSlim(15, 15);

    while (true)
    {
        await s.WaitAsync();
        await PeekAsync(msMq);
        Command1 message = await ReceiveAsync(msMq);
        Task.Run(async () =>
        {
            try
            {
                await HandleMessage(message);
            }
            catch (Exception)
            {
                // Exception handling
            }
            s.Release();
        });
    }
}

private static Task HandleMessage(Command1 message)
{
    Console.WriteLine("id: " + message.id + ", name: " + message.name);
    Thread.Sleep(1000);
    return Task.FromResult(1);
}

private static Task<Message> PeekAsync(MessageQueue msMq)
{
    return Task.Factory.FromAsync<Message>(msMq.BeginPeek(), msMq.EndPeek);
}

public class Command1
{
    public int id { get; set; }
    public string name { get; set; }
}

private static async Task<Command1> ReceiveAsync(MessageQueue msMq)
{
    var receiveAsync = await Task.Factory.FromAsync<Message>(msMq.BeginReceive(), msMq.EndPeek);
    return (Command1)receiveAsync.Body;
}

Readability

Use keyword var any time that the initialization of the variable clearly tells what the variable represents. Avoid abbreviations in variable names.

var messageQueue = new MessageQueue(queue);
var semaphore = new SemaphoreSlim(15, 15);

Configuration

Avoid hard-coded settings:

var semaphore = new SemaphoreSlim(15, 15);

Make use of a pattern to allow the number of concurrent processing jobs to be configurable (instance property, configuration file, ..).

Main Loop

The main loop while (true) is considered a code smell by some because it's not clear what the exit conditions are. But what's definately an issue is that the loop never exits. Always make sure the loop has a graceful exit. Since there's a cancellation token provided, why not use it to drive the loop?

sand-box mode:

while (!token.IsCancellationRequested)
{
    // .. loop body
}

explicit mode:

while (true)
{
    token.ThrowIfCancellationRequested();
    // .. loop body
}

Threading Integrity

Once you have acquired the semaphore, you should immediately use the try-finally block to make sure to always release it.

await semaphore.WaitAsync();

// What if the code below throws an exception? The semaphore is not released!
await PeekAsync(messageQueue);
var message = await ReceiveAsync(messageQueue);

await semaphore.WaitAsync();
try
{
    // .. code while holding lock
}
finally
{
    semaphore.Release();
}

MessageQueue

There is no reason to peek before receiving, because both operations are blocking on the same trigger - an incoming message.

await PeekAsync(messageQueue);
var message = await ReceiveAsync(messageQueue);

You can replace the above with:

var message = await ReceiveAsync(messageQueue);


Replacing characters in a Java string using iteration and recursion I am new to the whole Data Structures thing and I want to get better at writing efficient code. So, I have been practicing some problem sets. The questions is - Replace characters in a string using iteration and recursion approach. I am still learning the whole recursion approach. But I tried to design an algorithm using the divide and conquer approach. I would like to know if there is any where I can fix my code and make it better.

I wrote the code using both approaches. Iteration and Recursion. I will be really thankful if I can get valuable pointers to improve my code and make it more efficient.

public class MainClass {

    public static void main(String[] args) {
        // TODO Auto-generated method stub

         String text = "Hello World !"
                + "Hello";

         if(!text.isEmpty())
         {
         //Method 1 Iteration approach
         System.out.println("Replacing Characters using Iteration Approach");
         System.out.println(replace(text,'o','s'));

         //Method 2 Recursion approach
         char [] temp = new char[text.length()];
         temp = text.toCharArray();

        recursiveapproach(temp,0,temp.length,'o','s');
        System.out.println("Replacing characters using Recursion Approach");
        System.out.print(String.valueOf(temp));
         }

        else
        {
            System.out.println("Empty String");
        }

    }

    public static String replace(String str, char ch,char r)
    {
        char[] c = new char [str.length()]; // O(1);

            c = str.toCharArray(); //assuming it to be O(1)
            for(int i=0;i<c.length;i++)//O(n)
            {
                if(ch==c[i]) // O(1)
                {
                    c[i] = r; // O(1)
                }
            }

            //for(int i=0;i<c.length;i++)
                //System.out.print(c[i] + "\t");

        String s = String.valueOf(c); //// O(1)

        return s; //// O(1)

        // Time complexity is O(n) for the above code.

    }

     public static void replacechar(char[] a,int start,char c,char r)
        {

            if(a[start]== c)
            {
                a[start] = r;

            }
        }
        public static void recursiveapproach(char[] a,int start,int end,char c,char r)
        {

            int mid;

          int s = a.length-1;

            if(start<end)
            {
                mid = start + (end - start)/2;

                recursiveapproach(a,start,mid,c,r);

                recursiveapproach(a,mid+1,end,c,r);

            }

                if(start==end && start<=s)
                {

                replacechar(a,start,c,r);

                }

        }

}

First of all, good work!

Now a couple of points:

Whitespace

You abuse the usage of white space in your code. This applies to indentation as well.

\$\mathcal{O}(?)\$

You are right about the running time of both your methods being \$\mathcal{O}(n)\$ (in all cases). However,

str.toCharArray(); //assuming it to be O(1)

runs in linear time as well (apart from counting the time it takes to ask the memory subsystem for a new character array). The issue here is that, the returned array is not the same array somewhere in the str, and that's why: if it were the same array, it would run in constant time, yes, but it would allow a programmer to modify the state of the string through the character array.

API design

You can improve the API of your methods a bit; see below.

Naming

The names c and r are not descriptive. Consider renaming to targetChar(acter) and replacementChar(acter), respectively.

replacechar(...)

Consider renaming to replaceChar(acter), and declare it private since it is an auxiliary method.

Summa summarum

All in all, I had this in mind:

public static String replace(String str,
                             char targetChar,
                             char replaceChar) {
    // O(n): Strings are immutable. For that very reason, toCharArray()
    // returns the copy of the internal character array, so that the user
    // cannot mutate the string. Copying done in linear time.
    char[] c = str.toCharArray();

    for (int i = 0; i < c.length; ++i) {
        if (c[i] == targetChar) {
            c[i]= replaceChar;
        }
    }

    return String.valueOf(c);
}

public static void recursiveApproach(String text,
                                     char targetChar,
                                     char replacementChar) {
    recursiveApproach(text.toCharArray(), targetChar, replacementChar);
}

public static void recursiveApproach(char[] a,
                                     char targetChar,
                                     char replacementChar) {
    recursiveApproach(a, 0, a.length, targetChar, replacementChar);
}

public static void recursiveApproach(String text,
                                     int fromIndex,
                                     int toIndex,
                                     char targetChar,
                                     char replacementChar) {
    recursiveApproach(text.toCharArray(),
                      fromIndex,
                      toIndex,
                      targetChar,
                      replacementChar);
}

public static void recursiveApproach(char[] a,
                                     int fromIndex,
                                     int toIndex,
                                     char targetChar,
                                     char replacementChar) {
    int rangeLength = toIndex - fromIndex;

    if (rangeLength < 1) {
        return;
    }

    if (rangeLength == 1) {
        replaceChar(a, fromIndex, targetChar, replacementChar);
    } else {
        int mid = fromIndex + rangeLength / 2;
        recursiveApproach(a, fromIndex, mid, targetChar, replacementChar);
        recursiveApproach(a, mid, toIndex, targetChar, replacementChar);
    }
}

private static void replaceChar(char[] a,
                                int index,
                                char targetChar,
                                char replacementChar) {
    if (a[index] == targetChar) {
        a[index] = replacementChar;
    }
}

Hope that helps.


Generic Cache-Aware Data Adapter I've written a generic data adapter that can make use of a cache, for use in a data access layer. It presents an interface allowing CRUD operations, as well as retrieving all objects of a certain type.

IEntity.cs:

using System;

namespace GenericDataAdapter
{
    public interface IEntity
    {
        Guid Id { get; set; }
    }
}

IDataAdapter.cs:

using System;
using System.Collections.Generic;
using System.Threading.Tasks;

namespace GenericDataAdapter
{
    public interface IDataAdapter<T> where T : IEntity
    {
        Task<IEnumerable<T>> ReadAllAsync();
        Task<(bool, T)> ReadAsync(Guid id);    // bool indicates whether entity was present in data source
        Task SaveAsync(T newData);  // update if present, create if not present
        Task DeleteAsync(Guid id);
    }
}

CacheDataAdapter.cs:

using System;
using System.Collections.Generic;
using System.Threading.Tasks;

namespace GenericDataAdapter
{
    public class CacheDataAdapter<T> : IDataAdapter<T> where T : IEntity
    {
        private IDataAdapter<T> PrimaryDataSource;
        private IDataAdapter<T> Cache;

        public CacheDataAdapter(IDataAdapter<T> primaryDataSource, IDataAdapter<T> cache)
        {
            PrimaryDataSource = primaryDataSource;
            Cache = cache;
        }

        public Task<IEnumerable<T>> ReadAllAsync()
        {
            return PrimaryDataSource.ReadAllAsync();
        }

        // can potentially return stale data, due to SaveAsync()/DeleteAsync() not being atomic
        public async Task<(bool, T)> ReadAsync(Guid id)
        {
            var (presentInCache, cacheData) = await Cache.ReadAsync(id);
            if (presentInCache)
            {
                return (true, cacheData);
            }

            var (presentInPrimary, primaryData) = await PrimaryDataSource.ReadAsync(id);
            if (presentInPrimary)
            {
                await Cache.SaveAsync(primaryData);
                return (true, primaryData);
            }
            else
            {
                return (false, default(T));
            }
        }

        public async Task SaveAsync(T newData)
        {
            await Cache.SaveAsync(newData);
            await PrimaryDataSource.SaveAsync(newData);
        }

        public async Task DeleteAsync(Guid id)
        {
            await Cache.DeleteAsync(id);
            await PrimaryDataSource.DeleteAsync(id);
        }
    }
}

There are two points I'd like feedback on, though all comments are welcome:

In CacheDataAdapter.ReadAllAsync(), should I use await? It doesn't seem like I should, since I'm just passing through the return value of PrimaryDataSource.ReadAllAsync(), but I'm not sure.
I'm not sure whether unifying the create and update operations into SaveAsync() is a good idea; it puts the responsibility for fully initializing objects on the application. That's why I'm using GUIDs as the identifier, so the application can generate unique IDs without having to consult the database first.

Typically this scenario is done with a decorator pattern where you would wrap just the one and have the cache functionality in the one class. You would put the code in the CacheDataAdapter that I assume is currently in the code of the IDataAdapter cache.  Most of the time this will simplify your code as you can call base method to get the values if not in the backing store for the cache.

For example if you are using the ConcurrentDictionary or MemoryCache they will have methods to GetOrCreate or AddOrUpdate that will take a lamba expression that you can call into your base method to get the value if missing.  Since you are doing async work while caching you should check out the AsyncLazy

If you don't want to do the decorator pattern I would swap Save and Delete to do it in the primary store before removing them from the cache.  You could have a situation where the save fails in the primary but your cache is updated to say it's good.

As far if you should await the ReadAllAsync or not it doesn't matter a lot.  I personally wouldn't await it but either way is fine.  See answer on SO


A multi-threaded key-value cache that only has thread contention while querying for same key simultaneously Maybe the title fails to make myself clear, more explanation to the requirements: Implement a thread-safe key-value cache. User of this cache will use an unique key to query for data. If the data is not found in cache, it should call a data provider to load data and then return it.

The provider could take long time to load and return data, so if more than one user (e.g. user A and User B) are querying by using the same key (e.g. key1) at the same time , user B should be blocked until the data is loaded (by user A's query); but if user B is querying a different key (e.g. key2) and the corresponding data is already in the cache, the data should be returned to user B immediately from the cache , while user A is still waiting for data for key1 being loaded.

Please help to review my implementation below (the code can compile and is testable), I wonder

Is this implementation correct ? Is there a scenario that could break it ?
Are there better solutions? I think we can also use Condition, but don't see there's any fundamental difference ...

Cache.java

import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.ConcurrentHashMap;
import java.util.function.Function;

public class Cache <K,V> {
    private static class Wrapper<V> {
        V value;
    }

    private final Function<K,V>  provider;
    private final ConcurrentMap<K,Wrapper<V>> cache;

    public Cache(Function<K, V> provider) {
        if(provider == null) throw new NullPointerException("Need a valid provider!");
        this.provider = provider;
        this.cache = new ConcurrentHashMap<>();
    }

    public V get(K key){
        Wrapper<V> wrapper = cache.get(key);
        if(wrapper ==null){
            //cache miss
            wrapper = new Wrapper();
            Wrapper<V> savedWrapper = cache.putIfAbsent(key, wrapper);
            if(savedWrapper==null) savedWrapper=wrapper;
            synchronized (savedWrapper){
                if(savedWrapper!=wrapper){
                    //if savedWrapper is not the one created by this thread then another thread has already set the data with the same key
                    //in this case the data is ready as long as we get into the critical section (freed by the other thread that just loaded data)
                    //we don't need to do anything, just exit critical section and return the data
                }
                else{
                    //get data from provider and save it, if there is another thread querying the same key now,
                    //it will get blocked until data is set in this thread
                    savedWrapper.value = provider.apply(key);
                }
            }
            return savedWrapper.value;
        }
        else{
            synchronized (wrapper){ //the other thread might be loading data
                return  wrapper.value;
            }
        }
    }
}

I'm working on similar thing at the moment. So my thoughts are that you might use most popular EhCache in order to keep something you already have.
  ehCache.get(key) -> if its there, you return, if it's not there, we call provider.
  When we call provider - we need to wait for response, so I'm thinking about something like
 Question about your data provider - can you have parallel calls? Do you have some throttling there? If so, you could have some concurrent map for calls to store responses for keys. Once API is done you might store response in Ehcache so you have that for future.
 I think EhCache is better than just concurrent map for "caching" because you might define amount of entities, eliminate them, have a disk-backed cache, etc.


Data-structure exploration On my quest to learn Ruby I figured exploring the proper usage of different data-structures would be invaluable, including strings, arrays and hashes.

I decided to build a hash containing the numbers 1 till 100 inclusive. These hashes would contain:

Whether the number is prime
What it's FizzBuzz output would be
The n-th Fibonacci number.

In the end the number itself turned into a value as well. I intended the number to be the key and the other 3 values to be a hash of that key but it got too complicated.

As a side effect my project got quite bulky and should probably be split up. Somehow. My gut tells me I should move the FizzBuzz and Fibonacci parts to their own module and import them like I did with the already existing prime module.

On top of that, I got a couple of concerns:

Is output += "Fizz" the idiomatic way of adding a string to a string?
Is my usage of the hash proper or is it not supposed to be used like this?
Did I adhere to variable naming rules?
How is the general readability?

Code:

require 'prime'

class Fibonacci
    def initialize(max)
        @max = max
        @list = [1, 1, 2]
        for i in 3..@max do
            @list << @list[i-2] + @list[i-1]
        end
    end
    def get_nth(n)
        return @list[n]
    end
end

numbers = []

def fizzbuzz(number)
    output = ""
    if (number % 3 == 0)
        output += "Fizz"
    end
    if (number % 5 == 0)
        output += "Buzz"
    end
    if output == ""
        output = number
    end
    return output
end

Fib = Fibonacci.new(100)

for number in 1..100 do
    hash = {
        :Number => number,
        :FizzBuzz => fizzbuzz(number),
        :Prime => Prime.instance.prime?(number),
        :Fibonacci => Fib.get_nth(number)
    }
    numbers << hash
end

The results can be called like this:

Primes:

numbers.each { |number| puts number[:Number], number[:Prime] }

FizzBuzz:

numbers.each { |number| puts number[:FizzBuzz] }

Fibonacci:

numbers.each { |number| puts number[:Number], number[:Fibonacci] }

Try it

I'm not convinced that there is much value in combining the results of three separate problems into one data structure, but I'll play along.

Note that the standard indentation for Ruby is two spaces.

Fibonacci

I don't think it's a good idea to have Fibonacci objects that differ only in their max.  What is the point of the max, anyway?  It just introduces a possible failure if you try to request an entry beyond the max.  Why not have a "smart" singleton that extends itself as necessary?

The get_nth method is a natural candidate for overloading the [] operator, so that you can write Fibonacci[3].

Opinions may differ as to the convention, but I would consider 0 to be the zeroth Fibonacci number, followed by 1, 1, 2, 3, 5, …

class Fibonacci
  @list = [0, 1]

  def self.[](nth)
    while @list.size <= nth
      @list << @list[-2] + @list[-1]
    end
    @list[nth]
  end
end

FizzBuzz

It would probably be a good idea to make FizzBuzz adhere to the same interface as Fibonacci.

Your results are sometimes numbers and sometimes strings.  That's allowable, but not a practice that I would recommend.

The conditions can be written a bit more succinctly using the value if condition syntax and implicit return.

class FizzBuzz
  def self.[](n)
    output = ''
    output += 'Fizz' if n % 3 == 0
    output += 'Buzz' if n % 5 == 0
    output = n.to_s if output.empty?
    output
  end
end

numbers

Instead of appending to an empty array, build it "all at once" using Range#collect.

numbers = (1..100).collect do |n|
  {
    :Number => n,
    :FizzBuzz => FizzBuzz[n],
    :Prime => Prime.instance.prime?(n),
    :Fibonacci => Fibonacci[n]
  }
end


Implementation of merge sort algorithm in C++ This is my implementation of "merge sort" in C++. I'm still a newbie at C++ so:

Have I understood how merge sort works and implemented it in the right way?
Have I used any bad practices?
How could the code be improved upon in terms of efficiency?

Criticism is welcome and appreciated!

#include <iostream>
#include <vector>
#include <string>
#include <sstream>
using namespace std;

vector<float> merge(vector<float> firstHalf, vector<float> secondHalf){
    vector<float> combined;
    for(int i = firstHalf.size() + secondHalf.size(); i > 0;  i--){//merge two vectors
        if(firstHalf.back() > secondHalf.back() && !firstHalf.empty()){
            combined.push_back(firstHalf.back());
            firstHalf.pop_back();
        }else if(!secondHalf.empty()){
            combined.push_back(secondHalf.back());
            secondHalf.pop_back();
        }
    }
    vector<float> revCombined;//reverse merged vectors. Vectors don't have pop_front and I didn't want to use lists.

    for(int i = 0; i < combined.size(); i++){
        revCombined.push_back(combined[combined.size()-i-1]);
    }
    return revCombined;
}

vector<float> mergeSort(vector<float> &inputArray){//for example [9, 8, 1] as input
    if(inputArray.size() > 1){
        vector<float> firstHalf;
        vector<float> secondHalf;

        for(int i = 0; i < inputArray.size()/2; i++){//auto round the input array because size() returns int
            firstHalf.push_back(inputArray[i]);
        }//first half = [9, 8]

        for(int i = inputArray.size()/2; i < inputArray.size(); i++){
            secondHalf.push_back(inputArray[i]);
        }//second half = [1]

        return merge(mergeSort(firstHalf), mergeSort(secondHalf));
    }
    else{
        return inputArray;
    }
}

vector<string> split(string str, char delimiter) {
    vector<string> internal;
    stringstream ss(str); // Turn the string into a stream.
    string tok;

    while(getline(ss, tok, delimiter)) {
        internal.push_back(tok);
    }

    return internal;
}
vector<float> floatVectorInput(){
    string inputString;
    getline(cin, inputString);

    vector<string> stringArray = split(inputString, ' ');

    vector<float> array;
    for(int i = 0; i < stringArray.size(); i++){
        array.push_back(stof(stringArray[i]));
    }
    return array;
}

int main(){
    cout << "Array to sort (separate by spaces): " << endl;
    vector<float> inputArray = floatVectorInput();

    vector<float> sorted = mergeSort(inputArray);

    cout << endl << "Sorted Array:" << endl;
    for(int i = 0; i < sorted.size(); i++){
        cout << sorted[i];
        if(i == sorted.size()-1){
            cout << endl << endl;
        }else{
            cout << ", ";
        }
    }
    return 0;
}

merge calls back() on empty vector

The logic in your merge function has a bug.

if (firstHalf.back() > secondHalf.back() && !firstHalf.empty()) { … }

The first problem is that you access firstHalf.back() before the check that !firstHalf.empty(). The checks should be in reversed order.

The second problem is that you're also accessing secondHalf.back() without checking whether !secondHalf.empty().

While the code could be fixed with less changes, I suggest that you look at the problem again: Merging only really makes sense if neither of the halves is empty. So let's break the problem into two parts:

Merge two non-empty containers.
Append the remaining elements to the already merged container.

std::vector<float> combined{};
// Merge two non-empty containers.
while (!firstHalf.empty() && !secondHalf.empty()) {
    if (firstHalf.back() > secondHalf.back()) {
        combined.push_back(firstHalf.back());
        firstHalf.pop_back();
    } else {
        combined.push_back(secondHalf.back());
        secondHalf.pop_back();
    }
}
// Append the remaining elements to the already merged container.
while (!firstHalf.empty()) {
    combined.push_back(firstHalf.back());
    firstHalf.pop_back();
}
while (!secondHalf.empty()) {
    combined.push_back(secondHalf.back());
    secondHalf.pop_back();
}

This version is not only correct but also simpler to understand.

Input does not handle excess spaces correctly

If you feed your program with input that puts more than one space between two numbers, your program will crash.

While it is a legitimate choice to require exactly one space between subsequent numbers, it turns out that you can make the behavior more user-friendly and simplify the code at the same time.

std::vector<float> floatVectorInput()
{
    std::vector<float> numbers{};
    std::string line{};
    std::getline(std::cin, line);
    std::istringstream iss{line};
    float x;
    while (iss >> x) {
        numbers.push_back(x);
    }
    if (!iss.eof()) {
        throw std::runtime_error{"Garbage input"};
    }
    return numbers;
}

Instead of first splitting into a vector of strings, I'm reading the floats from the std::istringstream directly. This will skip over any amount of white-space as desired.

The !iss.eof() check is there because I want to make sure that we stop because the input is exhausted, not because there was something that is not a floating-point number.

Avoid using namespace std

I know that many C++ tutorials want you to put

using namespace std;

into every file. I don't think that this is good advice and recommend against doing it. Importing namespaces is a complex operation and you probably don't understand all its implications. For example, what will the following code print with and without the using namespace std;?

#include <iostream>
#include <utility>

//using namespace std;

void swap(double x, double y)
{
  std::clog << "swap(" << x << ", " << y << ")\n";
}

int main()
{
  int a = 1;
  int b = 2;
  swap(a, b);
  swap(3, 4);
}

   With the offending line commented out, the code will print swap(1, 2) and swap(3, 4) as you'd probably expect. However, with using namespace std, it will only print the second line.

What happened?

   By using namespace std, we've made std::swap (defined in <utility>) visible. Since our own swap takes doubles as parameters, calling it with an argument of type int is not a perfect match. What the C++ compiler does is adding an implicit conversion from int to double. However, if there is also a function that doesn't require this conversion to happen, the compiler will prefer it. It just so happens that std::swap is a template that will be a better match in this case. So why is only the first call resolved to std::swap then? This is because std::swap takes its arguments as mutable references. A temporary object (like an integer literal in this case) doesn't bind to a mutable reference.

I understand that this is complicated stuff for a beginner and you probably shouldn't have to worry about it at this point. But if you're using namespace std, you'll have to know it or you won't understand your code.

That said, using namespace std is also frowned upon in production-quality code (written by people who ought to understand the aforementioned language rules) so you're teaching yourself a bad habit by using it. Just be explicit and prefix symbols from the standard library with std::. It will tell the reader at a  glance where the symbol comes from which makes understanding the code easier for readers of any level of experience.

Be const correct

Your mergeSort function doesn't modify its argument (sorts the vector in-place) but actually returns a new, sorted vector. Therefore, make its argument const.

std::vector<float> mergeSort(const std::vector<float>& inputArray);
//                           ^^^^^

You should do this with any variable that you don't intend to modify.

Use the correct integer types

This comment is actually wrong.

  auto round the input array because size() returns int

std::vector::size is of type std::vector::size_type which is probably an alias for std::size_t. In any case, it is an unsigned integer type. Compiling your code with warnings enabled will warn you about this mixing of signed and unsigned types.

Unfortunately, std::size_t is an unsigned integer for historic reasons. This meas that you cannot let inidces go below zero which makes the loop conditions a bit more complicated in some places.

Make double your go-to floating-point type

Unless you have a good reason to use float, prefer double for its greater range and precision. On modern hardware, you'll hardly notice a performance difference.

That's not to say there are no valid use-cases for float. But unless you have such a case, your default-choice should be for double.

Prefer '\n' over std::endl by default

std::endl does more than outputting a new line. It also flushes the output stream. Which is an expensive operation.

Sometimes, this is just what you want. For example, in this line,

cout << "Array to sort (separate by spaces): " << endl;

you've used it correctly. The text must be visible before the instruction on the subsequent line gets executed or the program might start blocking for user input before the unlucky user was asked to provide some.

However, in all other places in your program, there is no need to flush. And output flushing is slow. It probably doesn't make any measurable difference for your small program but I've seen code where replacing std::endl with '\n' literally made the code ten times faster! And as a bonus: it's also less typing.

Consider using auto type deduction (C++11)

C++11 introduced a great feature: auto type deduction. This means that if you're declaring a variable and immediately initialize it with some value, you don't have to spell out its type. The compiler will deduce it for you.

So, for example, you could replace

vector<float> sorted = mergeSort(inputArray);

by

auto sorted = mergeSort(inputArray);

and the compiler will figure out for you that sorted is of type vector<float>.

Using auto systematically can help you avoid repeating the same information over and over again and also avoid doing some silly mistakes that happen when you think you know the type.

(Applying the earlier advice, we should use const auto here, of course.)

Avoid index-based for loops (C++11)

You're using C-style for loops throughout your code. While they are not wrong, they're less readable than ideal and allow you to make some avoidable bugs with the index bookkeeping. In some cases, they might also perform worse than ideal.

Since C++11, you can iterate over any standard library container using the following idiom, known as range-based for loop.

Before:

for (int i = 0; i < stringArray.size(); i++) {
    array.push_back(stof(stringArray[i]));
}

After:

for (auto&& s : stringArray) {
    array.push_back(stof(s));
}

Notice how I am using auto type deduction here. The type of s will be deduced to std::string& here but you don't necessarily have to worry about it. You can always use auto&& if you don't care about the actual type. Prefer const auto& if you don't intend to modify the object, though. Don't use plain auto as it will make an unnecessary copy.

Prefer standard library algorithms over raw loops (intermediate level)

The previous item notwithstanding, you should try to avoid raw loops in your code at all, if possible. (Search for Sean Parent's great “Better Code” talks and in particular for his “no raw loops” advice.)

For example, the loop from above could become this.

std::transform(
      std::begin(stringArray),
      std::end(stringArray),
      std::back_inserter(array),
      [](const std::string& s){ return std::stof(s); }
);

I understand that this might not be very easy to understand for a beginner and it is perfectly acceptable to write your own loops at this point. But once you become more familiar with C++, standard algorithms are definitely something to look into.

(Taking this further, production code would of course use the standard library's std::stable_sort algorithm instead of rolling its own merge sort implementation. But implementing algorithms yourself for learning is a good exercise.)

Make the code generic (intermediate level)

Your sorting logic can currently only be used for sorting std::vector<float>. You could generalize it to work with any container of any type that defines a comparison function.

This is not a defect in your code. If you only have to sort std::vector<float>, that's fine. In fact, it is better to write specific code that is correct than to write generic code that is flawed. Once you've learned about templates and move semantics, you can attempt to make your code generic.

Avoid unnecessary copies

I liked this comment in your code.

  Reverse merged vectors. Vectors don't have pop_front and I didn't want to use lists.

Indeed, using lists would be a bad idea here (and almost everywhere else, too). However, there is no need to create another vector. For one, you can reverse the elements of a vector in-place. In this case, however, an even better solution would be to use a std::deque (which has pop_front) instead of a std:vector.

Of course, you could also re-work your code such that you don't actually pop the items off the front of your vector but merely iterate over it.

There are some other places where your code makes copies that could be avoided. For example, the splitting into the firstHalf and secondHalf vectors isn't necessary. You could instead always pass a reference to the original vector and a pair of indices. Alternatively (and preferably) use iterators.

An expert would probably also try to avoid the allocations for the merged intermediary results and instead use a single working array. But that is probably too complicated for now.

The naming could be improved slightly

Your code is generally very readable but at some places, I'd suggest looking carefully at the chosen names. For example, internal doesn't really tell me anything useful. More as a matter of taste, I'd avoid names like stringArray in favor of strings. Especially since it is a std::vector and not a std::array.

Use tools to detect easy bugs

Some of the issues pointed out in this review could easily have been found by tools.

The most important tool is your compiler. Always compile with a high level of warnings enabled and treat them as errors. For GCC and Clang, I recommend you use the -Wall, -Wextra, -Werror and-pedantic options. There are even more warnings available but these should cover a good part.

If your C++ implementation supports it, build debug builds with a “checked STL”. That is, a modified standard library that performs more checking than required (or even allowed) by the standard. When using GCC, this is really simple. All you have to do is compile with the -D_GLIBCXX_DEBUG option.

This cannot find all issues, however. So in addition, I recommend that you compile your code with a sanitizer. GCC and Clang both support the -fsanitize=TOOL flag where TOOL is the name of the sanitizer to use. I recommend that you use address and undefined routinely. Using these switches will instrument your binaries with some run-time checks.

Alternatively – or, preferably, in addition – you can use Valgrind which is a tool to instrument your binaries on the fly.

Instrumentation of your code is only useful, however, if you have appropriate tests that execute the instrumented code. In your case, testing would be easy. Just write a function that calls mergeSort with a number of interesting inputs. “Interesting” in this case might be: an empty vector, a vector of a single element, a sorted vector, a vector sorted in reverse, a vector with duplicate elements, a random vector, etc. Of course, the test should also verify that the output is sorted correctly. Re-run these tests whenever you make modifications to your code to be alerted of newly introduced bugs immediately.


Codewars: Pick peaks Pick peaks
Wrote my "hello world" on Thursday, so definitely a beginner. I know some Python, so not a novice to programming in general.
Question

For example, the array arr = [0, 1, 2, 5, 1, 0] has a peak at position 3 with a value of 5 (since arr[3] equals 5).
The output will be returned as an object with two properties: pos and peaks. Both of these properties should be arrays. If there is no peak in the given array, then the output should be {pos: [], peaks: []}.
Example: pickPeaks([3, 2, 3, 6, 4, 1, 2, 3, 2, 1, 2, 3]) should return {pos: [3, 7], peaks: [6, 3]} (or equivalent in other languages)
All input arrays will be valid integer arrays (although it could still be empty), so you won't need to validate the input.
The first and last elements of the array will not be considered as peaks (in the context of a mathematical function, we don't know what is after and before and therefore, we don't know if it is a peak or not).
Also, beware of plateaus !!! [1, 2, 2, 2, 1] has a peak while [1, 2, 2, 2, 3] does not. In case of a plateau-peak, please only return the position and value of the beginning of the plateau. For example: pickPeaks([1, 2, 2, 2, 1]) returns {pos: [1], peaks: [2]} (or equivalent in other languages)
Have fun!

Solution
function pickPeaks(arr)
{
   const obj = {
      pos: [],
      peaks: []
   };
   let i = 1;
   while(i < arr.length-1)
   {
      let run = false;
      let peak = false
      if(new Set(arr.slice(i, i+2)).size === 1)  // There is a run of numbers.
      {
         previous = arr[i-1] < arr[i];
         var j = i;
         while(arr[j] === arr[i]) j++;  // Consume the run.
         next = arr[j] < arr[i]
         if(previous && next) peak = true;  // Was the run a plateau?
      }
      else if(arr[i] > arr[i-1] && arr[i] > arr[i+1]) peak = true;
      if(peak)
      {
         obj.pos.push(i);
         obj.peaks.push(arr[i]);
      }
      i = run ? j : i+1;
   }
   return obj
}

Unfinished ?
You have the variable run set to false and then use it in a ternary to set i. run is never set to true. I assume you intended to set it true after scanning a plateau.
JavaScript Style

In JavaScript we put the opening { on the same line and else on the same line as the closing }

Spaces between operators. eg arr[i+1]; becomes arr[i + 1];

Always declare variables. The variables next and previous are undeclared and are thus automatically declared in global scope. Global scoped variables are significantly slower, and highly likely to conflict with other globals if you are unaware. Use the directive "use strict" (strict mode) which will throw an error when you use undeclared variables.

Use ; consistently. JavaScript requires them and will insert them automatically if you do not add them where required. The insertion rules are complex and unless you are familiar with them it is best to always use them.

Use meaningful names. obj could be result. Better yet you can declare the arrays pos and peaks as variables and build the object on the return. eg return {pos, peaks} this saves you having to indirectly reference the arrays when you use them. (see rewrite)

Learn the difference between function scope var and block scope let, const and use appropriately.

Always declare vars at the top of the function. You declared j as a var in the middle of the function.

Always delimit code blocks with {} eg while (foo) j++; should be while (foo) { j++; } with the exception of empty blocks. eg while (arr[i++] !== 0); I know that C like languages let you skip the {} but after years of coding I can tell you it is a very hard bug to spot when you change code and forget to delimit the block.

Avoid single use variables unless doing so will make a line too long. Eg next = arr[j] < arr[i]; if (previous && next) { becomes just if (previous && arr[j] < arr[i]) {

Use the simplest logic. the line if (new Set(arr.slice(i, i + 2)).size === 1) { creates a new Array, a new Set, populates the Set by generating a hash for each array item. That is a lot of overhead (you generate a Hash twice for each inner array item) that can replaces with if (arr[i] === arr[i+1]) { with only the i + 1 being overhead

Always try to reduce indexing into arrays by assigning array items into variables. Generally do this if you use the item 2 or more times. For engines like V8 (Chrome and Node) this can provide some worthwhile performance improvements. Firefox this makes no difference however it does make the code less noisy and thus easier to read..

Rewrite
I think your solution is a little over complicated, especially indexing into the array so many times. Nor is there a need to make a special case of plateaus.
The rewrite is a simpler solution that uses the variables climbing to track positions where the current item is greater than the previous. If the item is less than the previous then record the peak to be the climbing position. The plateau is thus automatically handled, and the number of lines and variables is greatly reduced.
The rewrite runs on average 15 times faster on the same data.
However to be fair the slow point is the use of the Set in the statement  if(new Set(arr.slice(i, i+2)).size === 1) If you change that line to if (arr[i] === arr[i + 1]) { and add run = true in the plateau statement block, your function is then only about 30% slower than the rewrite (see second rewrite)
"use strict";   // not needed if this was a module
function pickPeaks(arr) {
    const pos = [], peaks = [];
    var i = 1, climbing, prevVal = arr[0];
    while (i < arr.length) {
        const val = arr[i++];
        if (val < prevVal && climbing) {
            pos.push(--climbing);
            peaks.push(arr[climbing]);
            climbing = 0;
        } else if (val > prevVal) { climbing = i }
        prevVal = val;
    }
    return {pos, peaks};
}

And rewriting you algorithm using the points in this answer we get...
function pickPeaks(arr){
    const pos = [], peaks = [];
    var j, i = 1;
    while (i < arr.length) {
        const val = arr[i], nextVal = arr[i + 1];
        let run = false, peak = false;
        if (val === nextVal) {
            j = i;
            while (arr[j] === val) { j++ }
            if (arr[i - 1] < val && arr[j] < val) { peak = true }
            run = true;
        } else if (val > arr[i - 1] && val > nextVal) { peak = true }
        if (peak) {
            pos.push(i);
            peaks.push(val);
        }
        i = run ? j : i + 1;
   }
   return {pos, peaks};
}


Enemy patrol script using NavMeshAgent I'm piecing together an Enemy script to attach to my characters. This is the portion that related the a game objects ability to patrol to given locations. Basically, patrolPoisitions are exactly what they say; they are defined in the editor. This is only the pertinent portion of the code, so there might be some straggler variables. I think I deleted them all though.

I am still very new to game development as well as Unity, so are there any suggestions on how to improve this code?

public float BaseAcceleration = 10.0f;
public float BaseSpeed = 5.0f;
public float IncreasedAccelerationFactor = 10.0f;
public float IncreasedSpeedFactor = 5.0f;
public float AlertCoolDownTime = 5.0f;
public float AlertDistance = 15.0f;
public float AttackDistance = 10.0f;
public float StoppingDistance = 0.0f;

private NavMeshAgent navMeshAgent = null;
public List<Vector3> patrolPositions = new List<Vector3>();
public float PatrolHoldTime = 0.0f;
private Vector3 StartPosition;
private int patrolPositionIndex = 0;
private bool isAtPatrolPosition = false;
private Vector3 CurrentDestination;

void Start () {
            navMeshAgent = GetComponent<NavMeshAgent> ();
            StartPosition = transform.position;
    }

void Update () {
            if (CurrentTarget) {
                    navMeshAgent.SetDestination (CurrentTarget.position);
            } else {
                    Patrol ();
            }

            SetNavMeshProperties ();
    }

#region Patrol Methods
void Patrol() {
            Vector3 Position = transform.position;
            if (patrolPositions != null && patrolPositions.Count > 1) {
                    Vector3 Destination = patrolPositions [patrolPositionIndex];
                    if (Destination != Position && CurrentDestination != Destination) {
                            Debug.Log (String.Format ("Patrol plotting route to {0}", Destination.ToString ()));
                            navMeshAgent.SetDestination (Destination);
                            CurrentDestination = Destination;
                            isAtPatrolPosition = false;
                    } else if (Destination == Position && !isAtPatrolPosition) {
                            isAtPatrolPosition = true;
                            Debug.Log (String.Format ("Patrol waiting {0} seconds", PatrolHoldTime));
                            StartCoroutine (Wait (PatrolHoldTime));
                    }
            } else if (Position != StartPosition) {
                    navMeshAgent.SetDestination (StartPosition);
            }
    }
void SetNavMeshProperties() {
            navMeshAgent.acceleration = BaseAcceleration;
            navMeshAgent.speed = BaseSpeed;
            navMeshAgent.stoppingDistance = StoppingDistance;
    }

IEnumerator Wait(float duration) {
            yield return new WaitForSeconds (duration);
            if (patrolPositionIndex < patrolPositions.Count - 1) {
                    patrolPositionIndex++;
            } else {
                    patrolPositionIndex = 0;
            }
            isAtPatrolPosition = false;
            Debug.Log (String.Format ("Patrol position index {0}", patrolPositionIndex.ToString ()));
    }
#endregion

Your indentation and Java-style braces make it very hard to follow. Compare this:

void Patrol() {
            Vector3 Position = transform.position;
            if (patrolPositions != null && patrolPositions.Count > 1) {
                    Vector3 Destination = patrolPositions [patrolPositionIndex];
                    if (Destination != Position && CurrentDestination != Destination) {
                            Debug.Log (String.Format ("Patrol plotting route to {0}", Destination.ToString ()));
                            navMeshAgent.SetDestination (Destination);
                            CurrentDestination = Destination;
                            isAtPatrolPosition = false;
                    } else if (Destination == Position && !isAtPatrolPosition) {
                            isAtPatrolPosition = true;
                            Debug.Log (String.Format ("Patrol waiting {0} seconds", PatrolHoldTime));
                            StartCoroutine (Wait (PatrolHoldTime));
                    }
            } else if (Position != StartPosition) {
                    navMeshAgent.SetDestination (StartPosition);
            }
    }

to that:

void Patrol()
{
    Vector3 Position = transform.position;
    if (patrolPositions != null && patrolPositions.Count > 1)
    {
        Vector3 Destination = patrolPositions[patrolPositionIndex];
        if (Destination != Position && CurrentDestination != Destination)
        {
             Debug.Log(String.Format("Patrol plotting route to {0}", Destination.ToString()));
             navMeshAgent.SetDestination(Destination);
             CurrentDestination = Destination;
             isAtPatrolPosition = false;
        }
        else if (Destination == Position && !isAtPatrolPosition)
        {
             isAtPatrolPosition = true;
             Debug.Log(String.Format("Patrol waiting {0} seconds", PatrolHoldTime));
             StartCoroutine(Wait(PatrolHoldTime));
        }
    }
    else if (Position != StartPosition)
    {
        navMeshAgent.SetDestination(StartPosition);
    }
}

I'd remove this comment:

// Use this for initialization

And I'd avoid #region blocks - you're not showing your entire class, but regions are typically a sign that too many things are going on, and can often be extracted into their own class (or method, when the region is defined within a method's body).

I don't understand why a method called Wait would return an IEnumerator, and it seems a bit counter-intuitive that Wait would have the side-effect of changing the patrolPositionIndex - could the method have a more descriptive name?

These initializations are redundant, since they're initializing to the default value for their type:

private NavMeshAgent navMeshAgent = null;

public float StoppingDistance = 0.0f;

public float PatrolHoldTime = 0.0f;

private int patrolPositionIndex = 0;

private bool isAtPatrolPosition = false;


Mergesort, instrumented for counting reads and writes I have just finished an implementation of mergesort, and I was wondering if I can further reduce the amount of reads and writes. I have looked at it for a while, and to my eyes, I cannot see anything that could really influence the amount of r/w.

public void doMergeSort(int[] array)
{
    doMergeSort(array, 0, array.length - 1);
}

public void doMergeSort(int[] array, int low, int high)
{
    // If Low Bound Is Geater/Equal Than High Bound, Child Arrays Have Been Split As Far As They Can
    if(low < high)
    {
        int middle = low + (high - low) / 2;

        doMergeSort(array, low, middle);
        doMergeSort(array, middle + 1, high);

        merge(array, low, middle, high);
    }
}

private void merge(int[] parentArray, int low, int middle, int high)
{
    int pIndex = low;
    int lIndex = 0;
    int rIndex = 0;

    // Declare Arrays For The First And Second Half Of The Parent Array
    int[] leftArray = new int[(middle + 1) - low];
    int[] rightArray = new int[high - middle];

    // Populate The Left/Right Arrays
    populateArray(parentArray, leftArray, low, middle);
    populateArray(parentArray, rightArray, middle + 1, high);

    // Loop While The Left/Right Array 'Search' Indices Are Less Than The Arrays Lengths
    while(lIndex < leftArray.length && rIndex < rightArray.length)
    {
        // Check If The Left Array Value If Smaller Than The Right Array Value (Value At Respective Index)
        reads += 2;
        if(leftArray[lIndex] < rightArray[rIndex])
        {
            reads++;
            writes++;
            parentArray[pIndex] = leftArray[lIndex];

            lIndex++;
        }
        else
        {
            reads++;
            writes++;
            parentArray[pIndex] = rightArray[rIndex];

            rIndex++;
        }

        pIndex++;
    }

    // If Looped Over All Right Array Values, Populate The Rest Of The Parent Array With Left Array Values
    while(lIndex < leftArray.length)
    {
        reads++;
        writes++;
        parentArray[pIndex] = leftArray[lIndex];

        lIndex++;
        pIndex++;
    }

    // If Looped Over All Left Array Values, Populate The Rest Of The Parent Array With Right Array Values
    while(rIndex < rightArray.length)
    {
        reads++;
        writes++;
        parentArray[pIndex] = rightArray[rIndex];

        rIndex++;
        pIndex++;
    }
}

private void populateArray(int[] originalArray, int[] newArray, int start, int end)
{
    int i = 0;

    for(int j = start; j <= end; j++)
    {
        reads ++;
        writes++;
        newArray[i] = originalArray[j];

        i++;
    }
}

A minor "bug"

One of the main advantages of mergesort is that it is a stable sorting algorithm.  That is, it guarantees that two elements will not be swapped unnecessarily if they have the same value.

For that reason, the comparison in merge() should be implemented with:

if (leftArray[lIndex] <= rightArray[rIndex])

… so that it prefers taking from leftArray first if there is a tie.

For ints, it makes no practical difference whether the sort is stable, because two ints that are equal are indistinguishable.  But it's still a good idea to use <= rather than < for the comparison, to stay true to the mergesort algorithm.

Bounds convention

In Java, a common convention is to specify a range using inclusive lower indexes and exclusive upper indexes.  Examples of this convention can be seen in Arrays.copyOfRange(original, from, to) and string.substring(beginIndex, endIndex).  A main advantage of this convention is that endIndex - beginIndex is the number of elements in that range, which often leads to fewer ± 1 adjustments in the code.

For example, with that convention, you would get rid of the awkward array.length - 1 in doMergeSort(), and the couple instances of (middle + 1) in merge().  Also, populateArray() would have j < end as the loop condition, which is more idiomatic.  (To be fair, you would have to change the test in doMergeSort() to if (low + 1 < high).)

Copying array elements

populateArray() should be replaced by Arrays.copyOfRange() (or perhaps a wrapper around Arrays.copyOfRange() if you need the instrumentation to count reads and writes).  Similarly, the loops to copy the leftover leftArray and rightArray elements could be replaced by System.arraycopy().  Both techniques would not only save a few lines of code, but might also be slightly faster than a hand-written loop.

The post-increment operator is handy for updating index counters with less verbosity:

if (leftArray[lIndex] <= rightArray[rIndex]) {
    parentArray[pIndex++] = leftArray[lIndex++];
} else {
    parentArray[pIndex++] = rightArray[rIndex++];
}

In summary, I don't see anything that can be done about the number of reads and writes, because that's just how the mergesort algorithm works.  But you might be able to shorten the code a bit, and squeeze out a tiny (likely imperceptible) performance gain.


Call a text-editor from within a Python script to get user input Motivation

I'd like to know how integrate the use of text editors (e.g. emacs, vim, etc.) into command-line Python programs.

Features

I'd like to be able to do the following:

Call a text editor from within a program in order to edit a file or buffer
Regain control after the text editor exits
Use temporary files, i.e. don't leave anything lying around after the program exits
Prepopulate the file or buffer with data
Get the edited text back for use in the program

Implementation

I wrote a small module named callvim.py (betraying my preferred choice of text editor) which creates a temporary file, populates it with some text, opens it with a text editor (defaults to vim), and prints the modified text:

#!/usr/bin/env python
# -*- encoding: ascii -*-
"""callvim.py

Demonstrates calling a text-editor (e.g. Vim) from within a Python script,
including passing input to the editor and reading output from the editor.
"""

import tempfile
import os
from subprocess import call

# Get the text editor from the shell, otherwise default to Vim
EDITOR = os.environ.get('EDITOR','vim')

# Set initial input with which to populate the buffer
initial_message = "Hello world!"

# Open a temporary file to communicate through (`tempfile` should avoid any filename conflicts)
#
# NOTE: Don't autodelete the file on close!
#       We want to reopen the file incase the editor uses a swap-file.
#
with tempfile.NamedTemporaryFile(suffix=".tmp", delete=False) as tf:

    # Write the initial content to the file I/O buffer
    tf.write(initial_message)

    # Flush the I/O buffer to make sure the data is written to the file
    tf.flush()

    # Open the file with the text editor
    call([EDITOR, tf.name])

# Reopen the file to read the edited data
with open(tf.name, 'r') as tf:

    # Read the file data into a variable
    edited_message = tf.read()

    # Output the data
    print(edited_message)

click is a great library for command line processing and it has some utilities, click.edit() is portable and uses the EDITOR environment variable.  I typed the line, stuff, into the editor.  Notice it is returned as a string. Nice.

So, rather then write any code, you could consider replacing what your working on with these two lines:

(venv) /tmp/editor $ export EDITOR='=mvim -f'
(venv) /tmp/editor $ python
>>> import click
>>> click.edit()
'stuff\n'

Check out the docs https://click.palletsprojects.com/en/7.x/utils/#launching-editors  My entire experience:

/tmp $ mkdir editor
/tmp $ cd editor
/tmp/editor $ python3 -m venv venv
/tmp/editor $ source venv/bin/activate
(venv) /tmp/editor $ pip install click
Collecting click
  Using cached https://files.pythonhosted.org/packages/fa/37/45185cb5abbc30d7257104c434fe0b07e5a195a6847506c074527aa599ec/Click-7.0-py2.py3-none-any.whl
Installing collected packages: click
Successfully installed click-7.0
You are using pip version 19.0.3, however version 19.3.1 is available.
You should consider upgrading via the 'pip install --upgrade pip' command.
(venv) /tmp/editor $ export EDITOR='=mvim -f'
(venv) /tmp/editor $ python
Python 3.7.3 (v3.7.3:ef4ec6ed12, Mar 25 2019, 16:52:21)
[Clang 6.0 (clang-600.0.57)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import click
>>> click.edit()
'stuff\n'
>>>

I would check out the source code for click.edit() if you want to write your own.