Handling arguments in a Python-like range() function in JavaScript I'm making a Python-like range() function (see Python docs) using JavaScript. This function can take from 1 to 3 arguments. There are 3 variables (start, stop and step) that need to have some value, either from arguments, or a default value.

If only one argument is passed, start is 0, stop is the first argument and step is 1.
If two arguments are passed, start is the first argument, stop is the second argument and step is 1.
If three arguments are passed, start is the first argument, stop is the second argument and step is the third argument.

I'm aware that since ECMAScript 6 JavaScript has default parameters, but the situation here is more complex. Therefore, I used rest parameters to get an array of all arguments and then assign them to appropriate variables.

const range = (...args) => {
  if (args.length < 1 || args.length > 3) {
    throw new Error('This function takes between 1 and 3 arguments')
  }
  const [start, stop, step] = {
    1: [0, ...args, 1]
   ,2: [...args, 1]
   ,3: args
  }[args.length]
}

I'm using the solution presented in the article Replacing switch statements with Object literals. The object keys are numbers of arguments and values are arrays with values for start, stop and step. I select the array which matches args.length and use destructuring assignment to assign the array elements respectively to start, stop and step.

This code is quite short, but I'm not sure whether it's readable. I came up with an alternative solution, using simple if-else blocks. I have to declare the variables at the beginning here, because let and const are block-scoped, so declaring them inside the if-else blocks would have no effect on the rest of the function.

const range = (...args) => {
  let [start, stop, step]
  if (args.length === 1) {
    ;[start, stop, step] = [0, ...args, 1]
  } else if (args.length === 2) {
    ;[start, stop, step] = [...args, 1]
  } else if (args.length === 3) {
    ;[start, stop, step] = args
  } else {
    throw new Error('This function takes between 1 and 3 arguments')
  }
}

This version maybe is more readable, but I don't like it very much. It repeats the [start, stop, step] fragment four times, and also I can't use const (and I'm trying to use const whenever possible).

Another solution that came to my mind is to assume as default the situation when two or three parameters are passed, and when one parameters is passed, assign start to stop and 0 to start.

const range = function(start, stop, step = 1) {
  if (arguments.length < 1 || arguments.length > 3) {
    throw new Error('This function takes between 1 and 3 arguments')
  }
  if (stop === undefined) {
    ;[start, stop] = [0, start]
  }
}

This also doesn't look good to me. I think that for someone reading this code it would be very difficult to understand what it does. Also, this code forces me to use a regular function instead of arrow function (because arrow functions don't have arguments) and violates two ESLint rules which I'm using: prefer-rest-params and no-param-reassign.

What is the best way to solve this problem? Should I modify one of the solutions that I already have, or should I try something completely different?

Edit

I made a variation of the third option which doesn't violate any of my ESLint rules:

const range = (...args) => {
  if (args.length < 1 || args.length > 3) {
    throw new Error('This function takes between 1 and 3 arguments')
  }
  let [start, stop, step = 1] = args
  if (stop === undefined) {
    ;[start, stop] = [0, start]
  }
}

I like the edited version.  I'd suggest just one small change:

const range = (...args) => {
  if (args.length < 1 || args.length > 3) {
    throw new Error('This function takes between 1 and 3 arguments')
  }
  let [start, stop, step = 1] = args
  ;[start, stop] = (stop === undefined) ? [0, start] : [start, stop]
}

Aside from reducing the visual clutter of the if statement, in general I find that expressions (which always execute) are easier to reason about than branching statements, even though the resulting logic is the same.


Singleton SpinLock: Making Random Thread-Safe Is this a valid and safe use of .NET's System.Threading.SpinLock?

  Why am I doing this?

  Random's public methods are not thread-safe. I could be calling from
  any thread which I do not know until run-time; they are from the
  Thread Pool, and it could be called fairly frequently (up to a hundred
  times a second), so I would rather not create a new Random object each
  time.

public static class SingleRandom
{
    private static Random random;
    private static SpinLock spinLock;

    static SingleRandom()
    {
        random = new Random();
        spinLock = new SpinLock();
    }

    public static int Next()
    {
        bool gotLock = false;
        spinLock.Enter(ref gotLock);
        int rv = random.Next();
        if(gotLock)
            spinLock.Exit(false); // ASSUMPTION not IA64, TODO what if ARM?
        return rv;
    }

    public static int Next(int min, int max)
    {
        bool gotLock = false;
        spinLock.Enter(ref gotLock);
        int rv = random.Next(min, max);
        if (gotLock)
            spinLock.Exit(false); // ASSUMPTION not IA64, TODO what if ARM?
        return rv;
    }

    public static double NextDouble()
    {
        bool gotLock = false;
        spinLock.Enter(ref gotLock);
        double rv = random.NextDouble();
        if (gotLock)
            spinLock.Exit(false); // ASSUMPTION not IA64, TODO what if ARM?
        return rv;
    }
}

SingleRandom is a really bad name for your class. Single means float in C# context, so your name implies that its a Random which generates float values, which is not true. ThreadSafeRandom or SynchronizedRandom are the examples of better naming.
Are you sure that using SpinLock in you case improves performance in any way? Somehow i think that a simple lock will be just as fast, while being a lot more readable.

public static int Next()
{
    lock (random)
    {
        return random.Next();
    }
}

I am not sure that making your class static is the best way to go. In my experience in such cases its almost always better to make a non-static class and then to either inject its instance or initialize some static readonly field.


How to parse arrays back and forth to function in a smart way Inside my main function I got a lot of these.

var statsArray = [];
statsArray = pushStats(r, htmlRecp, '', statsArray);
statsArray = pushStats(e, '', htmlExpe, statsArray);
etc...

Then I got statsArray function where I insert or update new items in the array.

function pushStats(currKey, lineRecp, lineExpe, statsArray){
    var selectStats = statsArray.filter(function(obj){
        return obj.key == currKey
    })
    if (!selectStats.length) {
        statsArray.push(
            {
                'key'       : currKey,
                'receipt'   : lineRecp,
                'expense'   : lineExpe
            }
        )
    } else {
        if (lineRecp) {
            selectStats[0].receipt = lineRecp;
        } else {
            selectStats[0].expense = lineExpe;
        }
    }
    return statsArray;
}

My question is this, how can I do this smarter?

statsArray = pushStats(aa, bb, cc, statsArray);

Is this the only way to do this? To send statsArray back and forth like this?

My question is this, how can I do this smarter?
statsArray = pushStats(aa, bb, cc, statsArray);

Is this the only way to do this? To send statsArray back and forth like this?

This question is the result of a misunderstanding: You're not "passing [the array] back and forth." Arrays are objects. Variables don't directly contain objects, they contain a value called an object reference which says where the object is elsewhere in memory. So what you're doing is passing that object reference into your function and returning that reference out of it. The actual array isn't passed into and out of the function.
Since you're not changing the value of statsArray in your function (you're just changing state the object it refers to contains), you don't need to return it, and you don't need to assign that return value. Just remove the return statsArray; from the end of the function and
var statsArray = [];
pushStats(r, htmlRecp, '', statsArray);
pushStats(e, '', htmlExpe, statsArray);
// ...

Let's explain why that works:
After this line
var statsArray = [];

in memory you have this:

                    +−−−−−−−−−−−+
statsArray−−−−−−−−−>|  (Array)  |
                    +−−−−−−−−−−−+
                    | length: 0 |
                    +−−−−−−−−−−−+

The variable statsArray contains an object reference for the array, which exists elsewhere in memory. (An object reference is a bit like a memory address.)
That means that during the first call:
pushStats(r, htmlRecp, '', statsArray);

...the statsArray parameter contains that same reference, and both the variable and the parameter refer to the same array:

                            +−−−−−−−−−−−+
statsArray−−−−−−−−−−−+−−−−−>|  (Array)  |
                    /       +−−−−−−−−−−−+
                    |       | length: 0 |
                    |       +−−−−−−−−−−−+
+−−−−−−−−−−−−−−−−+  |
| (Call Context) |  |
+−−−−−−−−−−−−−−−−+  |
| statsArray     |−−+
| ...            |
+−−−−−−−−−−−−−−−−+

Changes that pushStats makes to the state of the array change the array, not the reference. So if pushStats pushes a new entry onto the array, you get this:

                            +−−−−−−−−−−−+
statsArray−−−−−−−−−−−+−−−−−>|  (Array)  |
                    /       +−−−−−−−−−−−+
                    |       | length: 0 |      +−−−−−−−−−−+
                    |       | 0         |−−−−−>| (Object) |
+−−−−−−−−−−−−−−−−+  |       +−−−−−−−−−−−+      +−−−−−−−−−−+
| (Call Context) |  |                          | ...      |
+−−−−−−−−−−−−−−−−+  |                          +−−−−−−−−−−+
| statsArray     |−−+
| ...            |
+−−−−−−−−−−−−−−−−+

After the call returns, the call context is cleaned up, and you have

                    +−−−−−−−−−−−+
statsArray−−−−−−−−−>|  (Array)  |
                    +−−−−−−−−−−−+
                    | length: 0 |      +−−−−−−−−−−+
                    | 0         |−−−−−>| (Object) |
                    +−−−−−−−−−−−+      +−−−−−−−−−−+
                                       | ...      |
                                       +−−−−−−−−−−+

The key thing here is that the array is an object, and pushStats just changes the state of that object, via the reference you pass into it.

Side note: You can make the function a bit more efficient using Array#find instead of Array#filter:
function pushStats(currKey, lineRecp, lineExpe, statsArray){
    var selectStats = statsArray.find(function(obj){
        return obj.key == currKey
    });
    if (!selectStats) {
        statsArray.push(
            {
                'key'       : currKey,
                'receipt'   : lineRecp,
                'expense'   : lineExpe
            }
        );
    } else {
        if (lineRecp) {
            selectStats.receipt = lineRecp;
        } else {
            selectStats.expense = lineExpe;
        }
    }
    return statsArray;
}

It needs a polyfill for obsolete browsers, but it has the advantage that it doesn't create an unnecessary array, and stops looping through statsArray as soon as it finds the entry you want.


Python Implementation: Summer of 69 SUMMER OF '69: Return the sum of the numbers in the array, except
  ignore sections of numbers starting with a 6 and extending to the next
  9 (every 6 will be followed by at least one 9). Return 0 for no
  numbers.

My solution:

def summer_sum(a_list):
    """
    Use a stack to solve the problem.

    1. If current num is '6', push '0' on the stack.
    2. If current num is not '6' and the stack is empty, add it.
    3. If current num is '9' and the stack is not empty, pop the stack.
    """

    exclude_stack = []
    total = 0

    for num in a_list:
        if num == 6:
            exclude_stack.append(0)

        elif not len(exclude_stack):
            total += num

        elif num == 9 and len(exclude_stack):
            exclude_stack.pop()

    return total

Questions:

Is this implementation correct?
Is the time complexity of the this implementation O(n)?
Is there a better/faster way to solve the problem?

Your code looks good. It's always nice to see people following PEP 8. There is a small issue though with how you check if a list is empty or not. Currently you write
elif not len(exclude_stack): or elif num == 9 and len(exclude_stack): but PEP 8 actually has the following recommendation for this kind of tests:

  For sequences, (strings, lists, tuples), use the fact that empty
  sequences are false.

Yes: if not seq:
     if seq:

No:  if len(seq):
     if not len(seq):

So, in your case, it should be elif not exclude_stack: and elif num == 9 and exclude_stack:.

Additionally, the details on implementation in the docstring look redundant, I would remove them and leave only the task description.

The main question I have regarding your implementation is: Do you really need to have the stack to keep track if you are inside the 6-9 section? If I print exclude_stack for this input summer_sum([6] * 10 + [9] * 10), it will look like this:

[0]
[0, 0]
[0, 0, 0]
[0, 0, 0, 0]
[0, 0, 0, 0, 0]
[0, 0, 0, 0, 0, 0]
[0, 0, 0, 0, 0, 0, 0]
[0, 0, 0, 0, 0, 0, 0, 0]
[0, 0, 0, 0, 0, 0, 0, 0, 0]
[0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
[0, 0, 0, 0, 0, 0, 0, 0, 0]
[0, 0, 0, 0, 0, 0, 0, 0]
[0, 0, 0, 0, 0, 0, 0]
[0, 0, 0, 0, 0, 0]
[0, 0, 0, 0, 0]
[0, 0, 0, 0]
[0, 0, 0]
[0, 0]
[0]
[]

This doesn't really look like the most optimal solution memory-wise. How about keeping a track of the 6-9 sections by simply incrementing/decrementing some integer variable instead? Something like this:

def summer_sum(a_list):
    """Solution for Summer 69 challenge"""
    section_depth = 0
    total = 0
    for num in a_list:
        if num == 6:
            section_depth += 1
        elif section_depth == 0:
            total += num
        elif num == 9 and section_depth > 0:
            section_depth -= 1
    return total

I assume this will be also a bit faster as you won't have to pop or append to a list.

My hands are also itching to remove the total variable and make a generator function, but this may be a bit overboard:

def summer_sum(a_list):
    """Solution for Summer 69 challenge"""

    def free_numbers(numbers):
        section_depth = 0
        for num in numbers:
            if num == 6:
                section_depth += 1
            elif section_depth == 0:
                yield num
            elif num == 9 and section_depth > 0:
                section_depth -= 1

    return sum(free_numbers(a_list))


Creating a model record for a database I'm trying to expand my use of classes and better understand how/when to use them. Please review the code and let me know if I'm on the right track or where I need to make improvements. For example, should I remove the database call from the class, why or why not?

<?php
class Model{

    private $_db;

    private $first;
    private $last;
    private $gender;
    private $birthYear;
    private $agency;
    private $picture;

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

    public function __get($property) {
        if (property_exists($this, $property)) {
            return $this->$property;
        }
    }

    public function __set($property, $value) {
        if (property_exists($this, $property)) {
            $this->$property = $value;
        }

        return $this;
    }

    public function add_model(){
        $statement = $this->_db->prepare("insert into models (first_name, last_name, gender, birth_year, agency, picture) values (:first, :last, :gender, :birth_year, :agency, :picture)ON DUPLICATE KEY UPDATE gender = VALUES(gender), birth_year=VALUES(birth_year), agency=VALUES(agency), picture=VALUES(picture)");

        $statement->bindParam(':first', $this->first);
        $statement->bindParam(':last', $this->last);
        $statement->bindParam(':gender', $this->gender);
        $statement->bindParam(':birth_year', $this->birthYear);
        $statement->bindParam(':agency', $this->agency);
        $statement->bindParam(':picture', $this->picture);

        $result = $statement->execute();
        if ($result){
            return true;
        } else {
            return false;
        }
    }

}
?>

Your model should not know anything about how it gets created, that is what a repository is for. What if at some point in the future you changed from a database to a REST API? Now all of your models are invalid. If you use a repository, you could just add a new repository for the new API and the models are none-the-wiser.

So your model should just be this (notice I changed the name of the class, you should use descriptive names):

class PersonModel{

    private $first;
    private $last;
    private $gender;
    private $birthYear;
    private $agency;
    private $picture;

    public function __get($property) {
        if (property_exists($this, $property)) {
            return $this->$property;
        }
    }

    public function __set($property, $value) {
        if (property_exists($this, $property)) {
            $this->$property = $value;
        }
        return $this;
    }
}

And then you have a repository:

class PersonRepository {

    private $_db;

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

    public function create($person){
        $statement = $this->_db->prepare("insert into models (first_name, last_name, gender, birth_year, agency, picture) values (:first, :last, :gender, :birth_year, :agency, :picture)ON DUPLICATE KEY UPDATE gender = VALUES(gender), birth_year=VALUES(birth_year), agency=VALUES(agency), picture=VALUES(picture)");

        $statement->bindParam(':first', $person->first);
        $statement->bindParam(':last', $person->last);
        $statement->bindParam(':gender', $person->gender);
        $statement->bindParam(':birth_year', $person->birthYear);
        $statement->bindParam(':agency', $person->agency);
        $statement->bindParam(':picture', $person->picture);

        $result = $statement->execute();
        if ($result){
            return true;
        } else {
            return false;
        }
    }

    public function read($id) {
        //...
    }

    public function list() {
        //...
    }

    public function update($person) {
        //...
    }

    public function delete($person) {
        //...
    }
}

This keeps your database layer separate from your data layer and separate from your presentation layer. The more separation you can do here, the better off you are going to be when it comes to updating and maintaining this code.

I would recommend a good read through SOLID principles to get an idea.


Javascript: Canvas drawing - storing mouse coordinates I have a small JS application that allows a user, after clicking on a canvas, to draw a picture.

Once the user clicks the mouse, the user is allowed to drag the mouse wherever they want within the canvas and a line will be drawn from where they started moving the mouse to where the mouse stopped.

Only by clicking again will the drawing cease.

The code I have for this is below, and one concern of mine is how I'm storing the coordinates of the mouse's position each time it moves.

var draw = false;
var coords = [];
var canvas = document.getElementById('canvas');
var context = canvas.getContext('2d');
canvas.addEventListener('click', function (event) {
    coords = [];
    draw = !draw;
});
canvas.addEventListener('mousemove', function (event) {
    if (draw) {
        context = canvas.getContext("2d");
        var coord = { 'x': event.x - this.offsetLeft, 'y': event.y - thisoffsetTop };
        coords.push(coord);
        var max = coords.length - 1;
        if (typeof coords[max - 1] !== "undefined") {
            var curr = coords[max], prev = coords[max - 1];
            context.beginPath();
            context.moveTo(prev.x, prev.y);
            context.lineTo(curr.x, curr.y);
            context.stroke();
        }
    }
});

I have a feeling that I could be storing the coordinates more efficiently, rather than just add them to an ever-increasing Array. Is this the right approach, or is there a more efficient way of handling this sort of storage?

Firstly, I want to give you a thumbs up for a clear description of your code. It really helps with reading.

And another before-we-begin thing: you've got a missing . in your longest line of the code, near the end: -  thisoffsetTop };.

Main changes

Indeed, as you do not need to store all the coordinates, it's not efficient to save them all.
To draw a line between the current and previous point, all you need are those two points.
So, the first thing we can do with the code is to refactor that away: use prevCoord instead of an array.

Get rid of the array

First, we remove the coords declaration and replace it with a prevCoord.
Without adding initialization, it'll stay at undefined for the first round.
We're fine with that.

// old
var coords = [];

// new
var prevCoord;

Slightly different condition

We do not need to compute the max anymore -- the original code uses it to grab the "current" and "previous" coordinate, but we're moving away from that.
Also, the condition is now different.
We just need to ask if prevCoord is undefined.

// old
var max = coords.length - 1;
if (typeof coords[max - 1] !== "undefined") {

// new
if (typeof prevCoord !== "undefined") {

Simplify the contents of if

Let's see what happens inside.
Before we incorporate our change, let's notice that in your first line within if, you've got an expression coords[max].
That's actually the element you've just push'd, right?
So we can replace coords[max] with just coord (the current coordinate).

Then, we also not need the prev = coords[max - 1] part because we already have the previous coordinate stored in prevCoord.

In other words, that line's a goner because we have the refs we need at hand.

// old
var curr = coords[max], prev = coords[max - 1];

// new
// not needed: curr is coord and prev is prevCoord

Now, the drawing commands become the following:

// old
context.moveTo(prev.x, prev.y);
context.lineTo(curr.x, curr.y);

// new
context.moveTo(prevCoord.x, prevCoord.y);
context.lineTo(coord.x, coord.y);

Store the current as previous

Now, the main part.

The idea is not that far from what you have here.
Instead of pushing into array, you can just overwrite the prevCoord:

prevCoord = coord

Of course, you cannot do it as soon as you grab the coord from the event and offset stuff -- you need the prevCoord to draw the line.
So, just overwrite it after drawing.

// old
coords.push(coord);
if (typeof prevCoord !== "undefined") {
    // ...
}

// new
if (typeof prevCoord !== "undefined") {
    // ...
}
prevCoord = coord;

See -- just after using the prevCoord to draw a line between previous and current, we don't need the prevCoord anymore.
We can use this variable to prepare for the next cycle: on next mouse move, the "current" will become "previous"... meaning that when we run this function the next time, prevCoord will be just what it needs to be: the coordinate that was "mouse overed" the previous time.

Polish up

There's one more place let to change: the click handler.
Just as you reset your array, you should reset the prevCoord back to undefined.

// old
coords = [];

// new
prevCoord = undefined;

Full code

Here's the complete deal.

var draw = false;
var prevCoord;
var canvas = document.getElementById('canvas');
var context = canvas.getContext('2d');
canvas.addEventListener('click', function (event) {
    prevCoord = undefined
    draw = !draw;
});
canvas.addEventListener('mousemove', function (event) {
    if (draw) {
        context = canvas.getContext("2d");
        var coord = { 'x': event.x - this.offsetLeft, 'y': event.y - this.offsetTop };
        if (typeof prevCoord !== "undefined") {
            context.beginPath();
            context.moveTo(prevCoord.x, prevCoord.y);
            context.lineTo(coord.x, coord.y);
            context.stroke();
        }
        prevCoord = coord
    }
});
<canvas id="canvas" style="border: 1px solid red"></canvas>

More suggestions

A couple of things...

Redefining context

You have, on your fourth line, in the global scope a var context = ..., and then inside the if (draw) you have a context = ....
There's no need to run that each time.
The context doesn't change.

// old
if (draw) {
    context = canvas.getContext("2d");

// new
if (draw)

Check for undefined

You don't need to use typeof x != 'undefined' here.
It's used when you're not sure if x has been declared.
A simple check would do.

// old
if (typeof prevCoord !== "undefined") {

// new
if (prevCoord !== undefined) {

That's all!


Efficient way of code reuse for finding the largest prime factor I was doing timing with variations of function largest_prime_factor. I was able to write a better one largest_prime_factor2. There were some repetitions in the new function so I wrote another function largest_prime_factor3.

I thought the 3rd one has to be faster than the 2nd because I broke it up into more functions but it wasn't faster but slower.

I wanted to know whether my method of code reuse is good or not?
Is there a better way of reusing code in Python?
Did I miss something due to which the new function became slower?

My functions alongwith the testing() function used to test all three.

def largest_prime_factor(num):
    '''returns the largest prime factor of a number'''
    ans = 0
    if num % 2 == 0:
        ans = 2
        num /= 2
        while num % 2 == 0:
            num /= 2

    i = 3
    while i <= num:
        if num % i == 0:
            ans = i
            num /= i
            while num % i == 0:
                num /= i

        i += 2

    return ans

def largest_prime_factor2(num):
    '''returns the largest prime factor of a number'''
    ans = 0
    if num % 2:
        pass
    else:
        ans = 2
        while True:
            num //= 2
            if num % 2:
                break

    i = 3
    while i <= num:
        if num % i:
            pass
        else:
            ans = i
            while True:
                num //= i
                if num % i:
                    break

        i += 2

    return ans

def largest_prime_factor3(num):
    '''returns the largest prime factor of a number'''
    def check(j):
        nonlocal ans
        nonlocal num
        if num % j:
            return
        ans = j
        while True:
            num //= j
            if num % j:
                return
    ans = 0
    check(2)
    i = 3
    while i <= num:
        check(i)
        i += 2

    return ans

def testing():
    from timeit import Timer
    import random

    def tests(f, times):
        def test1(f):
            f(random.randint(1, 1000))
        def test2(f):
            f(random.randint(100000, 1000000))

        print(f.__name__)
        print(Timer(lambda: test1(f)).timeit(number = times))
        print(Timer(lambda: test2(f)).timeit(number = times//10))
        print()

    tests(largest_prime_factor, 10000)
    tests(largest_prime_factor2, 10000)
    tests(largest_prime_factor3, 10000)

if __name__ == "__main__":
    testing()

The timing results that I found:

largest_prime_factor
0.338549207387318
16.599112750324068

largest_prime_factor2
0.21575289594063918
12.086949738861868

largest_prime_factor3
0.36032199674939847
18.893444539398857

This format of results is produced by the testing function. See that for details.

Usually, splitting your code into different smaller functions is a good idea as it makes things easier to read, to reuse and to test.

However, this comes at a (cheap) price as far as performances are concerned because functions calls are not for free (nor are function definitions).

Now, for additional advices, whenever you want to compare performances of functions, it might also be a good idea to return what they compare. It's not good having a fast function if it doesn't return the right thing. By using assert, you could ensure you get notified quickly whenever something goes wrong.

This being said, I'd like to run and comment your code but I don't have a python3 handy and it seems that it is the version you are using so I'll try to do this later :-)


Finding string duplicates I'm looking for ways to optimize a program in C++ I wrote to count all the duplicates in a string and print the duplicated letters and the number of times my program takes all 256 ASCII inputs and only prints the duplicates once with the max number of duplicates. I had a really hard time writing it and needed to ask for help and a few people had some things to say about my design. You can read it here if you like.

How can I write my design better? And what can I do in the future to write better programs in class and in the workplace?

#include<iostream>
#include<string>
#include<vector>
#include<algorithm>
using namespace std;
vector <char> alreadyprintedcharacters;
void findrepeats(string const&);
int main()
{
    string input;
    cout << "Enter the input : ";
    getline(cin, input);
    findrepeats(input);
    return 0;
}
void findrepeats(string const &in)
{
    int trackerOfDuplicates = 0;
    int asciiArray[256];
    char ch;
    int charconv;
    for (int i = 0; i < 256; i++) // creates my refference array for the comparison and sets all the values equal to zero
        asciiArray[i] = 0;
    for (unsigned int i = 0; i < in.length(); i++)
    {
        ch = in[i];
        charconv = static_cast<int>(ch);
        asciiArray[charconv]++;
        /*if (asciiArray[charconv] == 0)
        {
            asciiArray[charconv] = 1;
        }
        else if (asciiArray[charconv] > 0)
        {
            asciiArray[charconv] = asciiArray[charconv]++;
        }*/

    }
    bool trip = false;
    for (unsigned int i = 0; i < in.length(); i++)
    {
        char static alreadyprinted;
        char ch = in[i];

        if ((asciiArray[ch] > 1) && (ch != alreadyprinted) && (find(alreadyprintedcharacters.begin(), alreadyprintedcharacters.end(), ch)== alreadyprintedcharacters.end()))
        {
            cout << in[i] << " : " << asciiArray[ch] << endl;
            trip = true;
            alreadyprinted = ch;
            alreadyprintedcharacters.push_back(alreadyprinted);
        }

    }
    if (trip == false)
        cout << "No repeated characters were found.\n";
}

A function does one thing, and one thing only.

Break your findrepeats() function apart. As the name indicates, only search for repeats, nothing else. Especially no output. When you are done, return asciiArray.

Maintain a clear separation of input and output, and the actual logic.

Keeping your functions short increases readability a lot, it also makes testing for bugs and restructuring your program later on a lot easier. In addition to that, it also reduces the number of local variables in a function, which aids further with readability.

int asciiArray[256];
for (int i = 0; i < 256; i++) // creates my refference array for the comparison and sets all the values equal to zero
    asciiArray[i] = 0;

Write it shorter:

int asciiArray[256] = {0};

Or use at least a builtin:

int asciiArray[256];
memset(asciiArray, 0, 256);

See also Array intialization

As mentioned in the other answer, don't try to iterate over the input string twice. You don't need to. asciiArray is all you need to generate the output, as long as the order doesn't matter.

vector <char> alreadyprintedcharacters;

Never use global variables. Just don't. That is bound to have side effects sooner or later.

Try e.g. calling findrepeats() twice now. The second call will not work as expected, as it is still (wrongly) using the exemption list from the first run.

This variable is only used inside findrepeats(), so put it in the local scope where it belongs.

int asciiArray[256];
char ch;

ch = in[i];
charconv = static_cast<int>(ch);
asciiArray[charconv]++;

Something is horribly wrong here, can you guess what?

Try to remember what char actually is. 256 distinct values, but was it 0..255 or was it rather -128..127?

You don't remember?
That's right. It's not defined what the default interpretation for a standalone char is, that depends on your compiler.

char alone is perfectly safe to transfer input to output, as signed or unsigned doesn't matter for that.

But if you want to use it as an array index, you must explicitly state that you want an unsigned char, or your program will crash horribly if the input contains anything outside the ASCII range (0..127), as it may be interpreted as a negative value.

Your compiler probably refused to accept char as an array index, and this problem is exactly the reason why. static_cast<int>(ch) only broke the type check, but your application will still crash.


Genomic Range Query in Python Recently, I worked on one of the Codility Training - Genomic Range Query (please refer to one of the evaluation report for the detail of the task.

The solution in Python is based on prefix sum, and was adjust in accordance to this article.

import copy

def solution(S, P, Q):
    length_s = len(S)
    length_q = len(Q)

    char2digit_dict = {'A':1, 'C':2, 'G':3, 'T':4}
    str2num = [0]*length_s

    count_appearances = [[0, 0, 0, 0]]*(length_s+1)
    cur_count = [0, 0, 0, 0]

    for i in range(0, length_s):
        str2num[i] = char2digit_dict[S[i]]

        cur_count[str2num[i]-1] += 1
        count_appearances[i+1] = copy.deepcopy(cur_count)

    results = []
    for i in range(0, length_q):
        if Q[i] == P[i]:
            results.append(str2num[Q[i]])

        elif count_appearances[Q[i]+1][0] > count_appearances[P[i]][0]:
            results.append(1)
        elif count_appearances[Q[i]+1][1] > count_appearances[P[i]][1]:
            results.append(2)
        elif count_appearances[Q[i]+1][2] > count_appearances[P[i]][2]:
            results.append(3)
        elif count_appearances[Q[i]+1][3] > count_appearances[P[i]][3]:
            results.append(4)

    return results

However, the evaluation report said that it exceeds the time limit for large-scale testing case (the report link is above).

The detected time complexity is \$O(M * N)\$ instead of \$O(M + N)\$. But in my view, it should be \$O(M + N)\$ since I ran a loop for \$N\$ and \$M\$ independently (\$N\$ for calculating the prefix sum and \$M\$ for getting the answer).

Could anyone help me to figure out what the problem is for my solution? I get stuck for a long time. Any performance improvement trick or advice will be appreciated.

I'm not sure that I trust Codility's detected time complexity. As far as I know, it's not possible to programmatically calculate time complexity, but it is possible to plot out a performance curve over various sized datasets and make an estimation.

That being said, there are a number of unnecessary overhead in your code and so it is possible that Codility is interpreting that overhead as a larger time complexity.

As for your code, Python is all about brevity and readability and generally speaking, premature optimization is the devil.

Some of your variable names are not in proper snake case (str2num, char2digit_dict).
List memory allocation increases by a power of 2 each time it surpasses capacity, so you really don't have to pre-allocate the memory for it, it's marginal.
You convert your string into a list of digits, but then only ever use it in a way that could be fulfilled with your original dict. The list of digits is not really needed since you are already computing a list of prefix sums.
In the C solution it was important to calculate the len of the P/Q list first so that it's not recalculated every time, but in Python, range (xrange in Python2) is evaluated for i exactly once.
It's not necessary to deepcopy cur_count as it only contains integers, which are immutable. You can copy the list by just slicing itself, list[:]
Instead of constantly making references to count_appearances (up to 4x each iteration), you could just make the reference once and store it. This will also make it easier to update your references if the structure of P and Q were to change in any way.

Cleaning up your code in the ways I mentioned gives me:

def solution(S, P, Q):
    cost_dict = {'A':1, 'C':2, 'G':3, 'T':4}

    curr_counts = [0,0,0,0]
    counts = [curr_counts[:]]
    for s in S:
        curr_counts[cost_dict[s]-1] += 1
        counts.append(curr_counts[:])

    results = []
    for i in range(len(Q)):
        counts_q = counts[Q[i] + 1]
        counts_p = counts[P[i]]

        if Q[i] == P[i]:
            results.append(cost_dict[S[Q[i]]])
        elif counts_q[0] > counts_p[0]:
            results.append(1)
        elif counts_q[1] > counts_p[1]:
            results.append(2)
        elif counts_q[2] > counts_p[2]:
            results.append(3)
        elif counts_q[3] > counts_p[3]:
            results.append(4)

    return results

which gets a perfect score on Codility.


Making a 2D dict readable I am building a 2d dictionary with an altered list at the end Afterwords I want to find the min of each list. Is there a better way to do it? Better being faster or at least less of an eyesore.

#dummy function, will be replaced later so don't worry about it.
def alter(x, y, z,):
    a = list(x)
    for i, val in enumerate(x):
         a[i] = val + y + z
    return a

masterStats = {}
askStats = [0, 1, 2]
for numberServed in range(3):
    masterStats[numberServed] = {}
    for timeToServe in range(3):
        masterStats[numberServed][timeToServe] = alter(askStats, numberServed, timeToServe)

for numberServed in masterStats.keys():
    for timeToServe in masterStats[numberServed].keys():
        print(min(masterStats[numberServed][timeToServe]))

for numberServed in masterStats.keys():
    print(numberServed)
    for timeToServe in masterStats[numberServed].keys():
        print("    " + str(timeToServe))
        print("        " + str(masterStats[numberServed][timeToServe]))

Python's variable names are conventionally named lowercase_width_underscores.

Loop using .items() not .keys() if you will need the values, so your final loop can be:

for numberServed, numberServedStats in masterStats.items():
    print(numberServed)
    for timeToServe, timeToServeStats in numberServedStats,items():
        print("    " + str(timeToServe))
        print("        " + str(timeToServeStats))

The whole thing can be made more efficient by using numpy:

askStats = numpy.array([0, 1, 2])

x = askStats[None,None,:] # put askStats in the third dimension
y = numpy.arange(3)[:, None, None] # put number served in the first dimension
z = numpy.arange(3)[None, :, None] # put time to serve in the second dimension

stats = x + y + z # same as in alter

We create a new array by adding together three existing arrays. The [:,None, None] parts control how they are added. Essentially, the colon indicates the column the data will be spread across. The None indicates where the data will be duplicated. So numpy.arange(3)[:,None] gets treated like

[
    [0, 0, 0],
    [1, 1, 1],
    [2, 2, 2]
]

Whereas numpy.arange(3)[None, :] gets treated like

[
    [0, 1, 2],
    [0, 1, 2],
    [0, 1, 2]
]

This lets us do the complete loop and adjust function in just that one expression.

for line in stats.min(axis=2).flatten():
    print(line)

The min method reduces the 3 dimensional array to 2d array by minimizing along dimension 2. Flatten() converts that 2d array into one long one dimensional array.

for row_index, row in enumerate(stats):
    print(row_index)
    for col_index, col in enumerate(row):
        print("    ", col_index)
        print("        ", col)

It probably won't help much here, but for bigger cases using numpy will be more efficient.


Pointer handle - absolute follow-up This is a follow-up to:

Pointer handle - follow-up
Pointer class/handle

Please review my pointer class.

template<typename T>
class Ptr {
public:
    Ptr(T* t, int s = 1) : sz{s<1 ? throw std::logic_error("Invalid sz parameter") : s} {
        sz = s;
        p = new T[sz];
        std::copy(t,t+sz,p);
    }

    Ptr(const Ptr& t) : Ptr(t.p, t.sz) { }

    Ptr& operator=(Ptr copy) {
        std::swap(copy.sz, sz);
        std::swap(copy.p, p);
        return *this;
    }

    Ptr(Ptr &&t) :p{t.p}, sz{t.sz} {
        t.p = nullptr;
        t.sz = 0;
    }

    Ptr& operator=(Ptr &&t) {
        std::swap(t.p,p);
        std::swap(t.sz,sz);
        return *this;
    }

    T& operator*() {
        check_range(index);
        return p[index];
    }

    T& operator[](int i) {
        check_range(i);
        return p[i];
    }

    T* get() const {
        return p;
    }

    void operator+=(int i) {
        check_range(index+i);
        index += i;
    }

    void operator-=(int i) {
        operator+=(-i);
    }

    Ptr operator+(int i) {
        Ptr old{*this};
        old += index+i;
        return old;
    }

    Ptr operator-(int i) {
        return operator+(-i);
    }

    Ptr& operator++() {
        operator+=(1);
        return *this;
    }

    Ptr operator++(int) {
        Ptr old{p+index};
        operator++();
        return old;
    }

    Ptr& operator--() {
        operator-=(1);
        return *this;
    }

    Ptr operator--(int) {
        Ptr<T> old{p+index};
        operator--();
        return old;
    }

    ~Ptr() {
        delete[] p;
    }

private:
    T* p;
    int sz;
    int index = 0;

    void check_range(int i) {
        if (i < 0 || i > sz-1) {
            throw std::out_of_range("out of range");
        }
        if (p+i == nullptr) {
            throw std::out_of_range("null pointer");
        }
    }

};

Ptr(T* t, int s = 1): sz{s<1 ? throw std::logic_error("Invalid sz parameter") : s}
{
    sz = s; //this is unnecessary
    p = new T[sz];
    std::copy(t,t+sz,p);
}

Here you are writing to sz twice, first in the initializer list then in the constructors body. In general it's best if you just write only once in the initializer list if possible:

Ptr(T* t, int s = 1):
    sz{s<1 ? throw std::logic_error("Invalid sz parameter") : s}
{
    p = new T[sz];
    std::copy(t,t+sz,p);
}

In the move constructor:

Ptr(Ptr &&t):
    p{t.p},
    sz{t.sz}
{
    t.p = nullptr;
    t.sz = 0;
}

we are changing the pointer and size in this class but not the index. I would "move" everything and take the index from the other class here as well.
We need to set t.p to nullptr so when t gets destructed the the call to delete[] p; will be a no-op, so that part is necessary. However I don't see why the sz needs to be set to 0 because it's a primitive and therefore not in the destructor. Because the range checking function always checks p for a nullptr we can't index the old object anyway so I think I would just remove that expression.

Ptr(Ptr &&t):
    p{t.p},
    sz{t.sz},
    index{t.index}
{
    t.p = nullptr;
}

(Disclaimer: I'm not especially experienced with c++ move semantics so if you think it's more readable to keep explicitly setting every element to a "null" type of value for readability please make a comment, I'd like to know what people think about that.)


REST call helper function returns empty response on timeout errors Please review my code where I want to handle the timeout errors and send parsed JSON response if any. There are times when a POST request does not ALWAYS return JSON code.

def use_service(branch: str, relative_url: str, request_type: str = 'GET', data=None, retry=1):
    """
    :param – relative_url – the relative url to be submitted
    :param – request_type – the type of request being submitted (POST, GET, PUT, etc.)
    :param – data – the data to be submitted with the request if necessary.
    """
    logger.info(f'getting data from {relative_url}')
    url = envs.get(branch, '') + relative_url
    resp = {}
    status = False

    for x in range(retry):
        try:
            if request_type == 'GET':
                print(f'GET call to {url}')
                resp = requests.get(url)
            elif request_type == "POST":
                print(f'POST call to {url}')
                if branch in envs.keys():
                    resp = requests.post(
                        url,
                        json=data,
                    )
                else:
                    print(f'{branch} : Attempted to push data: {data}')
            status = resp.ok
            if resp.ok:
                resp = resp.json()
                print(f'{resp}')
            else:
                resp.raise_for_status()
        except HTTPError as err:
            logger.exception(f'HTTP Error: {err}')
        except Timeout as err:
            logger.exception(f'Connection timed out {err}')
        except ConnectionError:
            logger.exception(f'A network problem has occured (DNS failure, refused connection)')
        except RequestException as err:
            logger.exception(f'Other error occurred: {err}')
        except Exception as err:
            logger.exception(f'Unknown Exception: {err}')

    if not status:
        resp = []

    return resp

General Remarks

Avoid catching Exception, catch a specific exception instead to avoid silencing errors.
Don't return the empty list as an indication of failure. Either return None or (preferably, in case of failure) raise an exception.
It's nice to see you adopt typing as an important part of your code. I recommend being more consistent with it in places where you already use it, and using it even more. For example, two of the parameters in your function were not annotated, neither was the return value. Use type aliases to describe non-trivial types, such as input and output data of your requests (i.e., type of data and return type). A final example I'll show in the code is the use of enums instead of strings, which can prevent you from making mistakes.
I'm not going to say the function is "too complex". Yet. But if it grows to handle more HTTP methods (say, PUT, HEAD, ...), or the handling of the existing HTTP methods grows, you should seriously consider decomposing it into multiple functions or by incorporating object-oriented design.
Good job on the documentation attempt, but try to follow a set style in the future. Examples include PEP 287, NumPy, Google. A general guideline is given in PEP 257. This includes advantages such as consistency and automated generation of HTML docs using e.g. sphinx-doc.
Some of the control flow is a bit confusing. I've made edits to improve it (see below) and noted some explanations of the issues. I generally recommend you to investigate these further while writing the code.
I highly recommend you to read a style guide and stick to it, if you haven't already. I also suggest installing a linter so that you can be warned of dubious code style and possible bugs while writing the code. I'm fairly sure a linter would've complained about quite a bit of the remarks I'll give below. I'll give a concrete recommendation for a linter at the end of this answer.

Further recommended reads:

The Zen of Python (PEP 20).
PEP8 style guide (PEP 8), although you can occasionally find me diverging from the guidelines.

To apply a couple of quotes from PEP 20:

  Errors should never pass silently.

Your except Exception as err: guard would catch most of the exceptions that exist in Python, and thus also catches a potential AttributeError that may occur in your code. See my comments in the code for more details.

  In the face of ambiguity, refuse the temptation to guess.

I've had to guess multiple times to come to a purpose for some parameters, either because non-descript names, or because of missing type information. E.g., without looking at the function body, can I know whether I can pass a list as the data parameter? What is the branch parameter?

Personal Remarks

A couple of personal pet peeves related to your code:

Work on your variable names. I personally dislike names like data or resp since they don't really give much information. Similarly, the retry parameter is a bit misleading. Ideally, variable names should give me all the information required to understand their purpose (although this is often not possible when keeping them brief). Additionally, I'd like better names for err, but I name my caught exceptions exc too often so I'm not in a position to critique that :)
Don't spam the user with info logs that should be debug logs. The user doesn't care that your function is doing this or that. I suspect these may be debugging statements (use a debugger!), so use logger.debug so you don't have to change it later. Definitely never use print, since disabling that is non-trivial.

I've left a bunch of comments in your code while I was reading through it, and I've also rewritten a couple snippets to illustrate the difference.

"""Modules should always start with a docstring describing their exports.

Since this is just a snippet of one function, I'm assuming your module has one.
If not, write one.
"""
from typing import Any, Optional

import enum
import os
from json.decoder import JSONDecodeError

import requests
from requests.exceptions import HTTPError, RequestException, Timeout

# COMMENT: If I left these here, ignore these, they are just to make the code
# run.
class Logger:
    """Logging stub for review."""

    def debug(self, msg: str) -> None:
        print(msg)

    def info(self, msg: str) -> None:
        print(msg)

    def warn(self, msg: str) -> None:
        print(msg)

    def exception(self, msg: str) -> None:
        print(msg)

logger = Logger()
envs = os.environ

# COMMENT: Prefer custom data types for enumerated data instead of simple
# string literals such as 'get', 'post', ... This way, mistakes can be caught
# early by static analysis tools such as mypy, e.g. misspelling 'post' as
# 'psot'.
class RequestMethod(enum.Enum):
    """Enumeration of various request methods."""

    GET = 1
    POST = 2
    # PUT, DELETE, OPTION, HEAD, ...

ResponseType = Any  # TODO: Update the response type.

# COMMENT: PEP8 dictates line length to be limited to 79 characters. I,
# personally, feel like this is too short, yet this serves as an example of a
# possible method of breaking up your function signatures in case of overly
# long lines. This allows you to specify your types and default values so that
# they stand out immediately. Parameters are indented at two increments in
# levels, and the return type indented at the same level as the function name.
# This way, you can not confuse the return type signature, the parameter list,
# or the body of the function.
def use_service(
        branch: str,
        relative_url: str,
        # COMMENT: Nitpicky, but the correct terminology is "request method",
        # not "request type": https://tools.ietf.org/html/rfc7231#section-4
        # Both are pretty much used interchangeably though.
        request_method: RequestMethod = RequestMethod.GET,
        # COMMENT: Try to annotate all of your parameters, even those with
        # default values. Especially for `data`, which has a default value of
        # `None`, a type hint can be useful documentation. Additionally, mypy,
        # the state-of-the-art static type checker for Python, is sometimes
        # unable the parameter type, even if there are default values.
        # COMMENT: Give your parameters descriptive names. `data` is too
        # generic of a word. `retry` is less generic, and from just looking at
        # the name, I can get an idea of its purpose, but explicitly calling it
        # "number of tries" makes it immediately clear what the full purpose
        # is without reading the documentation.
        post_data: Optional[Any] = None,
        num_tries: int = 1
        # Always document your return types. mypy WILL complain about it if run
        # in strict mode.
) -> Optional[ResponseType]:
    """Perform a request and retry if it failed.

    Perform a request of type ``request_type`` to the given ``relative_url``
    and return the parsed JSON response. If the request fails for any reason,
    retry it until ``num_tries`` tries have been performed. For ``POST``
    requests, the sent data is to be supplied by the ``post_data`` parameter.

    .. todo:: Document the purpose of the ``branch`` parameter.

    :param branch: ???
    :param relative_url: The relative URL to be submitted.
    :param request_type: The type of request being submitted.
    :param post_data: The data to be submitted with the request, if necessary.
    :param num_tries: The number of times the request should be tried before
                      giving up.
    :returns: The parsed JSON response, or `None` if the request failed after
              all tried attempts.

    """
    # COMMENT: Documentation is often overlooked but vital, both for you as
    # well as your colleagues. In my opinion, good documentation for a function
    # contains three things:
    #   1. A brief description of what the function does.
    #   2. An overview of the parameters I should/can give it, as well as their
    #      purpose.
    #   3. A description of what I can expect to receive from the function as
    #      a return value.
    # See PEP257 for docstring conventions.
    # I've rewritten the documentation into a more machine-readable format,
    # reST. It can be parsed and used to generate HTML documentation by tools
    # such as sphinx-doc. Very useful when writing an API.
    # Documentation often also includes type information on the parameters and
    # the return type. This is vital. However, I've emitted this from the
    # docstring since such information is already available as type hints.
    # Sphinx has plugins to extract type information from type hints.
    # COMMENT: This should really be a debug statement rather than info. Also,
    # I'm assuming the logger is imported/constructed somewhere outside of the
    # snippet provided.
    logger.debug(f'getting data from {relative_url}')

    # COMMENT: What is `envs`? If it's just accessing environment variables,
    # consider using `os.environ` instead, especially if its a third-party
    # package.
    # COMMENT: The `url` variable can get a name improvement, IMO.
    url = envs.get(branch, '') + relative_url
    # COMMENT: These variables are now useless. They could've also used a
    # better name.
    # resp = {}
    # status = False

    # COMMENT: Although this loop works, it would ALWAYS retry, regardless of
    # there being any exceptions. If the request was fine, it would do it again
    # if `num_tries` is high enough. I've renamed the parameter to `num_tries`
    # instead of `num_retries` so that a `num_tries` value of 2 will try twice.
    # For a parameter named `num_retries` given value 2, I would expect the
    # request to be performed at most 3 times: Once initially, and two REtries.
    # COMMENT: We don't use variable `x` anywhere in the loop. Use an
    # underscore instead so that it is not tied to a variable.
    for _ in range(num_tries):
        try:
            # COMMENT: Because we use an enum, we can now compare literally,
            # which is more efficient than string comparisons and decreases
            # the change of misspellings.
            # COMMENT: Since these handlers are short, I'll allow it, but if
            # you get more options and longer code, consider using classes
            # and polymorphism. E.g.:
            # req_maker = some_way_of_obtaining_an_object(req_method)
            # req_maker.perform(url)
            if request_method is RequestMethod.GET:
                # COMMENT: You changed these to prints in the edit, I changed
                # them back to logger calls. As a rule of thumb: Use a logger,
                # never print. Exceptions should be rare and only for small
                # personal scripts. I also changed them to debug logs, since
                # they're not informative to the user.
                logger.debug(f'GET call to {url}')
                # COMMENT: `resp` can get a better name, more descriptive of
                # its purpose. I'm assuming here you're contacting a REST API.
                api_resp = requests.get(url)
            elif request_method is RequestMethod.POST:
                logger.debug(f'POST call to {url}')
                if branch in envs.keys():
                    # COMMENT: In cases like these, indent your arguments with
                    # two levels, so that you don't confuse it for a new block
                    # in the body. Also, there's plenty of controversy over
                    # where to put the closing parenthesis in these cases,
                    # and whether or not to allow a comma after the final item.
                    # I'm personally not a fan, but that's just me.
                    api_resp = requests.post(
                            url,
                            json=post_data,
                    )
                else:
                    # COMMENT: Some style guides disallow the usage of string
                    # interpolation, I don't personally mind it.
                    logger.warn(
                            f'{branch} : Attempted to push data: {post_data}')
                    # COMMENT: You should really do *something* other than just
                    # logging here. In your original code, here, `resp` would
                    # not get defined, and thus the next line (`resp.ok`) would
                    # fail with an `AttributeError`, which would get caught as
                    # an "Unknown exception", and potentially retry the loop
                    # because of this bug.
                    # It seems like this is an unrecoverable problem with the
                    # input, so it's better to just return and stop the rest
                    # of the evaluation of the function. Even better: Raise an
                    # exception.
                    return None

            # COMMENT: I'm going to completely rewrite this part.
            # status = api_resp.ok
            # if api_resp.ok:
            #     resp = api_resp.json()
            #     print(f'{resp}')
            # else:
            #     resp.raise_for_status()

            # `raise_for_status` raises an exception if status != 2xx.
            # In your original code, you first check the status, and if the
            # status != 2xx, you call `raise_for_status`, which itself checks
            # if status != 2xx and raises. Why?
            api_resp.raise_for_status()
            # This may throw an exception. As you mention, the API "does not
            # ALWAYS return JSON", so you should expect exceptions. You should
            # expect parsing errors anyway if relying on external data.
            json_resp = api_resp.json()
            logger.debug(json_resp)
            # Return the data early instead of assigning to a variable
            # returning it at the end of the function. This breaks free from
            # the loop, thus immediately fixing the issue with too many tries
            # in the loop.
            return json_resp
        # Let's get to the exception handling...
        # COMMENT: It should come as no surprise now, I don't like `err`.
        # Either use something like `http_err` or just simply `e`, the standard
        # convention.
        except HTTPError as err:
            logger.exception(f'HTTP Error: {err}')
        except Timeout as err:
            logger.exception(f'Connection timed out {err}')
        except ConnectionError:
            # COMMENT: Don't use the `f` prefix if you're not interpolating.
            # COMMENT: Split the string for line length. Remember that this
            # works without requiring a `+`.
            logger.exception(
                    'A network problem has occured '
                    '(DNS failure, refused connection)')
        except RequestException as err:
            logger.exception(f'Other error occurred: {err}')
        # COMMENT: Don't catch Exception. Many style guides will discourage you
        # from doing this. `Exception` covers most of the possible exceptions
        # in Python programs, including `AttributeError`, which caused the bug
        # in the `else` branch of the POST request handler.
        # except Exception as err:
        #     logger.exception(f'Unknown Exception: {err}')

        # COMMENT: Explicitly catch JSONDecodeError so you can handle JSON
        # parsing errors explicitly.
        except JSONDecodeError as e:
            logger.exception(f'Failed to parse JSON: {e}')

    # COMMENT: When you have nothing meaningful to return from a function,
    # either don't return anything, or explicitly return `None`. Returning the
    # empty list is confusing: What if the response JSON was actually an empty
    # list? We'd have no way of discriminating a failure from an empty
    # response!
    # Better yet, we should raise an exception if all tries failed.
    return None

Wrapping Up

The improvements I've proposed definitely leave a lot of room for further improvement. Especially if you're planning on extending the functionality shown (which, likely, you will or even have to, given the nature of the code), I highly recommend to start decomposing now. For example, extract handling code for each of the HTTP methods into its own individual function, and perform some efficient dispatching by either putting the functions in a dict with keys corresponding to the HTTP methods, or implementing it using OO and polymorphism. The former is likely the better approach for this code, opting for the latter is a good idea if you plan on adding lots of functionality to these requests.

Bonus: Linter

If you want my recommendation for a Python linter, I run flake8 on my personal code with the following plugins:

flake8 --version
3.7.9 (aaa: 0.6.2, cohesion: 0.9.1, flake-mutable: 1.2.0, flake8-annotations-complexity: 0.0.2, flake8-annotations-coverage: 0.0.3, flake8-bandit: 2.1.2, flake8-blind-except: 0.1.1, flake8-broken-line: 0.1.1, flake8-bugbear: 19.8.0, flake8-comprehensions: <cached_property.cached_property object at 0x10e93d350>, flake8-docstrings: 1.5.0, pydocstyle: 4.0.1, flake8-eradicate: 0.2.3, flake8-pie: 0.2.2, flake8-print: 3.1.4, flake8-pyi: 19.3.0, flake8-string-format: 0.2.3, flake8-tidy-imports: 2.0.0, flake8-todo: 0.7, flake8-tuple: 0.4.0, flake8-variables-names: 0.0.1, flake8_builtins: 1.4.1, flake8_pep3101: 1.2.1, flake8_quotes: 2.1.0, import-order: 0.18.1, logging-format: 0.6.0, mccabe: 0.6.1, naming: 0.8.2, pycodestyle: 2.5.0, pyflakes: 2.1.1, rst-docstrings: 0.0.10, warn-symbols: 1.1.1) CPython 3.7.5 on Darwin

But there's a bunch of warnings that I tend to disable here as I see fit. Another option is wemake-python-styleguide, which is a very strict plugin for flake8 (too strict for me, even). mypy is also great as a static type checker to find type bugs early.


Retrieving OS Platform .NET Core I'm retrieving the operating system platform with .NET Core with system.runtime.interopservicesI have created the code below and was wondering if there was any other way that I could improve it since I haven't been able to get a switch statement to work in the OperatingSystem class.

/// <summary>
/// Operating System Class
/// For Retrieving The OS
/// Platform.
/// AE : 9/08/2018
/// </summary>
class OperatingSystem
{
    /// <summary>
    /// Operating System Platforms.
    /// </summary>
    public enum OSPlatform
    {
        Windows,
        OSX,
        Linux,
        Not_Supported
    }

    /// <summary>
    /// Return Operating System
    /// </summary>
    public static OSPlatform GetPlatform
    {
        // Switch Statement Not Compatitable
        get
        {
            if (RuntimeInformation.IsOSPlatform(System.Runtime.InteropServices.OSPlatform.Windows))
            {
                return OSPlatform.Windows;
            }
            else if (RuntimeInformation.IsOSPlatform(System.Runtime.InteropServices.OSPlatform.OSX))
            {
                return OSPlatform.OSX;
            }
            else if (RuntimeInformation.IsOSPlatform(System.Runtime.InteropServices.OSPlatform.Linux))
            {
                return OSPlatform.Linux;
            }
            else
            {
                return OSPlatform.Not_Supported;
            }
        }
    }
}

The class is being called like this,

static void Main()
{
    switch (OperatingSystem.GetPlatform)
    {
        case OperatingSystem.OSPlatform.Windows:
            Console.WriteLine("Goto Windows Class");
            break;
        case OperatingSystem.OSPlatform.OSX:
            Console.WriteLine("Goto OSX Class");
            break;
        case OperatingSystem.OSPlatform.Linux:
            Console.WriteLine("Goto Linux Class");
            break;
        default:
            Console.WriteLine("OS Not Supported");
            break;
        }
        Console.ReadKey();
    }

Review

Don't use underscores in enum values NotSupported
The enum value with integer value 0 represents its default value; I would make this NotSupported in order to avoid some bias
GetPlatform is a method name; prefer Platform as property name
Since the platform won't change while your program is running, consider using a Lazy to retrieve the platform once.

Don't like if-statements?

Let's first refactor the enum to match aforementioned conventions:

public enum OSPlatform
{
    NotSupported,
    Windows,
    OSX,
    Linux,
}

We could then use a Lazy:

 static readonly Lazy<OSPlatform> platform = new Lazy<OSPlatform>(GetPlatform, true);
 public static OSPlatform Platform => platform.Value;

And refactor the intitial method using a helper generator using a simple ValueTuple to avoid the boiler-plate if-statements.

private static IEnumerable
    <(OSPlatform Platform, RuntimeOSPlatform RuntimePlatform)?> EnumeratePlatforms()
{
    yield return (OSPlatform.Windows, RuntimeOSPlatform.Windows);
    yield return (OSPlatform.OSX, RuntimeOSPlatform.OSX);
    yield return (OSPlatform.Linux, RuntimeOSPlatform.Linux);
}

private static OSPlatform GetPlatform()
{
    return EnumeratePlatforms().FirstOrDefault(p
       => RuntimeInformation.IsOSPlatform(p.Value.RuntimePlatform))?.Platform ?? default;
}

I've created an alias to avoid redundant namespace code:

using RuntimeOSPlatform = System.Runtime.InteropServices.OSPlatform;

If you want to have some fun, you could refactor this solution to use Expression and some reflection to avoid creating hard-coded tuples.


Flatten an array of integers in Python The goal is to flatten an array of nested arrays of integers.
For example [1, [2], [3, [4]]] should return [1, 2, 3, 4]

I'm particularly looking for some feedback on the following:

Is usage of TypeError exception justified?
Did I miss some valid edge cases in my tests?

Here is my solution including unit tests:

#!/usr/bin/env python

import unittest

def flatten_list(array):
    """ Returns a flattened array of integers from arbitrarily nested arrays of integers.

    >>> flatten_list([[1, 2, [3]], 4])
    [1, 2, 3, 4]
    """

    flat_array = []
    for element in array:
        if isinstance(element, int):
            flat_array.append(element)
        elif isinstance(element, list):
            flat_array += flatten_list(element)
        else:
            raise TypeError("Unsupported type ({})".format(
                type(element).__name__))
    return flat_array

class FlattenListTestCase(unittest.TestCase):
    """Tests for flatten_list"""

    def test_empty_list(self):
        self.assertEqual(flatten_list([]), [])

    def test_single_integer(self):
        self.assertEqual(flatten_list([3]), [3])

    def test_already_flattened(self):
        self.assertEqual(flatten_list([1, 2, 3, 4]), [1, 2, 3, 4])

    def test_single_element_multiple_nested(self):
        self.assertEqual(flatten_list([[1]]), [1])

    def test_arbitary(self):
        self.assertEqual(flatten_list(
            [1, [2], [3, 4], [[5]], 6]), [1, 2, 3, 4, 5, 6])

    def test_empty_sublists(self):
        self.assertEqual(flatten_list([1, [[], 2]]), [1, 2])
        self.assertEqual(flatten_list([[]]), [])

    def test_invalid_input(self):
        with self.assertRaises(TypeError):
            flatten_list(3)
        with self.assertRaises(TypeError):
            flatten_list([3, "2"])
        with self.assertRaises(TypeError):
            flatten_list(None)

if __name__ == '__main__':
    unittest.main()

Python normally promotes duck typing, yours is more statically typed.
You may want to use yield, as your current code possibly uses more memory than needed.
You can check if an item is iterable, and iterate it. Otherwise return the item.

You can do this in two ways either via LBYL or EAFP.

Just using yield can get:

def flatten_list(array):
    for element in array:
        if isinstance(element, int):
            yield element
        elif isinstance(element, list):
            yield from flatten_list(element)
        else:
            raise TypeError("Unsupported type ({})".format(
                type(element).__name__))

Using LBYL and duck typing can then get:

import collections

def flatten_list(array):
    for element in array:
        if isinstance(element, collections.abc.Iterable):
            yield from flatten_list(element)
        else:
            yield element

Using EAFP and duck typing can instead get:

def flatten_list(array):
    for element in array:
        try:
            element = iter(element)
        except TypeError:
            yield element
        else:
            yield from flatten_list(element)

Finally as a side note, if you expect the initial array to have a depth exceeding one thousand, then you may want to instead perform the stack manipulation yourself. So you don't get a stack overflow error.
This can be done with something like:

def flatten(it):
    to_yield = [iter(it)]
    sentinel = object()
    while to_yield:
        item = next(to_yield[-1], sentinel)
        if item is sentinel:
            del to_yield[-1]
            continue
        try:
            item = iter(item)
        except TypeError:
            yield item
        else:
            to_yield.append(item)


Imrpove the following code without if/else I want to avoid using multiple if and else condition here

  if (decode) {
                Imagepath = bucket + "/" + key;
                Result.pages.push({
                        Id: Processid,
                        ValidTicket: decode ? true : false
                });
        } else {
                //checking for barcode if qr code is not present
                Quagga.decodeSingle({
                        src: filename,
                        numOfWorkers: 0, // Needs to be 0 when used within node
                        decoder: {
                                readers: ['code_128_reader',
                                        'ean_reader',
                                        'ean_8_reader',
                                        'code_39_reader',
                                        'code_39_vin_reader',
                                        'codabar_reader',
                                        'upc_reader',
                                        'upc_e_reader',
                                        'i2of5_reader'
                                ] // List of active readers
                        },
                }, function (result, error) {
                        //if guaggajs returns the result
                        if (result) {
                                console.log("bar code done");
                                //sometimes result is not empty but codeResult is empty
                                if (result.codeResult) {
                                        var barcode = result.codeResult.code;
                                        var Imagepath = '';
                                        Imagepath = bucket + "/" + key;
                                        Result.pages.push({
                                                Id: Processid,
                                                ValidTicket: barcode ? true : false
                                        });
                                        console.log("final array is" + Result);
                                } else {
                                        var Imagepath = '';
                                        Imagepath = bucket + "/" + key;
                                        Result.pages.push({
                                                Id: Processid,
                                                ValidTicket: false
                                        });

                                }
                        } else {
                                var Imagepath = '';
                                Imagepath = bucket + "/" + key;
                                Result.pages.push({
                                        Id: Processid,
                                        ValidTicket: false
                                });

                        }
                });
        }

I would separate them out into functions to make the code more readable. So the first if/else would become:

if (decode)
  doDecode()
else
  doEncodeSingle();

I would also break the doDocdeSingle up:

Quagga.decodeSingle({...},
  handleDecodeResult );

The last if/else is currently structured as:

if (result) {
  if (result.codeResult) {
    // Handle Code
  } else {
    // Handle no-code
  }
} else {
  // No result
}

I often find it cleaner to reverse the condition and structure it as:

if (!result) {
  // No result
} else if (result.codeResult) {
  // Handle Code
} else {
  // Handle no-code
}

Which removes a level of nesting. Again I would try and make each if section a single function call.


Python image sequence file concatenation I need to iterate over a large (in the thousands) list of files in a network folder and "concatenate" any image sequences into just one entry with the range of images (first image, last image in sequence). I am calling this "sequence pruning" and I created the following code which does work, but it seems incredibly un-pythonic to me and runs slowly. I'm certain there is a MUCH better way to do this, so I am looking for review to help clean/speed this up.

To elaborate a bit further on the issue lets say I have this as a list of files for the input:

img.001.png
img.002.png
img.003.png
img_other.001.png
random_file.txt
yet another seq.0000.png
yet another seq.0001.png
yet another seq.0002.png
yet another seq.0021.png
yet another seq.0030.png

In the end I want to return something like this:

img.001.png, [1-3]
img_other.001.png
random_file.txt
yet another seq.0000.png, [0-30]

FYI I can safely assume that the image sequence number is always going to be a series of digits at the very end of the filename (before the extension of course). However I cannot assume that they will be perfectly sequential, as there are sometimes "gaps" between numbers.

Here's my current code, python 2.7:

import os

def split_padding(path):
    file, ext = os.path.splitext(path)
    pad_int = 0

    while file[pad_int * -1 - 1].isdigit():
        pad_int += 1

    if pad_int == 0:
        return file, '0', ext

    clean_file = file[0:pad_int * -1]
    padding = file[pad_int * -1:]

    return clean_file, padding, ext

def strip_padding(path):
    file, ext = os.path.splitext(path)

    while file[-1].isdigit():
        file = file[:-1]

    return file

def prune_files(paths):
    '''
    sequences get put into arrays like so:
    [x_folder, z_folder, [test_a.000.png, 0, 2], [test_b.000.tif, 0, 3], test_C.000.png]
    :return: [file1, file2, [first_file, seq_start, seq_end]]
    '''
    paths.sort(key=lambda s: s.lower()) # list has to be sorted for this to work

    # this odd bit of code turns all sequences into arrays of images.
    pruned_list = []
    switch = True
    for c, path in enumerate(paths):
        if c == 0:
            pruned_list.append(path)
            continue
        if not os.path.splitext(path)[1] in ['.png', '.tif', '.tiff', '.exr', '.jpg', '.jpeg']:
            pruned_list.append(path)
            continue

        test = paths[c-1]
        if strip_padding(path) == strip_padding(test):
            if switch:
                pruned_list[-1] = [pruned_list[-1]]
                switch = False
            pruned_list[-1].append(path)
        else:
            pruned_list.append(path)
            switch = True

    # so now lets convert that to the format we want to return
    for c, item in enumerate(pruned_list):
        if type(item) == list:
            pruned_list[c] = [item[0], int(split_padding(item[0])[1]), int(split_padding(item[-1])[1])]

    return pruned_list

if __name__ == "__main__":
    test_dir = "some directory"
    print prune_files([path for path in os.listdir(test_dir) if os.path.isfile(os.path.join(test_dir, path))])

Many, many problems:

Low level dealing with strings. Python is a high-level programming language - don't reinvent the wheel. Regex and string methods now are replacing your low-level code in my rework of it. Good rule of thumb is to avoid any code that has a lot of array indexing in Python.
Sorting doesn't need a function if you use sorted
Long comprehensions and generators are usually unreadable and unmaintainable so avoid those at all costs. Especially avoid adding needless logic in them too.
Extension list is a constant so you might as well extract that to the top.
Simpler or more standard output could have made the code much simpler but it wasn't clear if that was the requirement.
Hardcoding args shouldn't be done when sys.argv and argparse are so easy to use.

tl;dr fixed code. Code could be a lot simpler if the output format was a bit more standard but you can probably modify the code with minimal effort:

#!/usr/bin/env python3

import os
import re
import sys

FILE_EXTS = [
    'exr',
    'jpeg',
    'jpg',
    'png',
    'tif',
    'tiff',
]

SEQUENCE_PATTERN = r'(.*)\.([0-9]+).(.{3,4})$'

def sequences_strigifier(sequences):
    output_string = ''
    for key, seq_info in sequences.items():
        if not seq_info:
            output_string += '{}\n'.format(key)
            continue

        if seq_info['start_index'] == seq_info['end_index']:
            output_string += '{}.{}.{}\n'.format(key,
                                                 seq_info['start_index_str'],
                                                 seq_info['ext'])
            continue

        output_string += '{}.{}.{}, [{}-{}]\n'.format(key,
                                                      seq_info['start_index_str'],
                                                      seq_info['ext'],
                                                      seq_info['start_index'],
                                                      seq_info['end_index'])

    return output_string.strip()

def find_image_sequences(directory):
    '''
    sequences get put into arrays like so:
    [x_folder, z_folder, [test_a.000.png, 0, 2], [test_b.000.tif, 0, 3], test_C.000.png]
    :return: [file1, file2, [first_file, seq_start, seq_end]]
    '''
    sequences = {}

    sorted_candidate_list = sorted(os.listdir(directory))

    for candidate_path in sorted_candidate_list:
        full_candidate_path = os.path.join(directory, candidate_path)
        if not os.path.isfile(full_candidate_path):
            sequences[candidate_path] = None
            continue

        matches = re.match(SEQUENCE_PATTERN, candidate_path)
        if not matches:
            sequences[candidate_path] = None
            continue

        filename = matches.group(1)
        sequence_index = matches.group(2)
        extension = matches.group(3)

        if not extension in FILE_EXTS:
            sequences[candidate_path] = None
            continue

        if not filename in sequences:
            sequences[filename] = {
               'ext': extension,
               'start_index_str': sequence_index,
               'start_index': int(sequence_index),
               'end_index': int(sequence_index),
            }
            continue

        sequences[filename]['end_index'] = int(sequence_index)

    return sequences_strigifier(sequences)

if __name__ == '__main__':
    if len(sys.argv) < 2:
        print('Usage: {} <dirname>'.format(sys.argv[0]))
        exit(1)

    print(find_image_sequences(sys.argv[1]))


Initializing local variable to generic interface Below is my working code written as an exercise in wildcards in generics and Java 8 type inference.  My question will be about the commented line in union() method.

I needed to create an instance of the interface type that does not include a static factory, so I was left with (seemingly) two choices:

Use concrete implementation (as in my code below)
Use Reflection to retrieve the actual run-time class and call its constructor (similar to my earlier question).

I chose to use the first approach and used a constructor of a concrete class HashSet<E> (that implements Set). I picked HashSet among other publicly available implementations rather arbitrarily.

My reasoning was that any would suffice, since it will be returned by a method as just Set<E> and, though it will retain its HashSet-specific members, they will not interfere with a client process.

Was my decision safe and sound or is there a legitimate way to break my method code?

import java.util.*;

public class TypeInference {

    public static <E> Set<E> union (Set<? extends E> s1, Set<? extends E> s2) {
        Set<E> result = new HashSet<E>();  // or any other concrete Set
        result.addAll(s1);
        result.addAll(s2);
        return result;
    }

    public static void main(String[] args) {

        Set<Integer> ints = new TreeSet<>(Arrays.asList(1, 2, 3, 4, 5));
        Set<Double> doubles = new TreeSet<>(Arrays.asList(1.0, 2.0, 3.0, 4.0, 5.0));

        Set<Number> numbers = union(ints, doubles);
        System.out.println(numbers);
    }

}

This is as safe as your Set implementation for result. For example, swapping for a TreeSet gave the following error:

Exception in thread "main" java.lang.ClassCastException:
            java.lang.Integer cannot be cast to java.lang.Double
    at java.lang.Double.compareTo(Double.java:49)
    at java.util.TreeMap.put(TreeMap.java:568)
    at java.util.TreeSet.add(TreeSet.java:255)
    at java.util.AbstractCollection.addAll(AbstractCollection.java:344)
    at java.util.TreeSet.addAll(TreeSet.java:312)

This is understandable since TreeSet imposes an ordering, which in turns calls the to-be-added Collection-type's compareTo() with the existing entries. This has nothing to do with type incompatibility at compile-time (the code compiles), rather it's a limitation/feature of the TreeSet implementation.

In your linked question @rolfl suggested giving a Supplier to the method, which I will advocate here too:

public class TypeInference {

    public static <E> Set<E> union (Set<? extends E> s1, Set<? extends E> s2,
            Supplier<Set<E>> supplier) {
        Set<E> result = supplier.get();
        result.addAll(s1);
        result.addAll(s2);
        return result;
    }

    public static <E> Set<E> union (Set<? extends E> s1, Set<? extends E> s2) {
        return union(s1, s2, () -> new TreeSet<>(Comparator.comparing(Object::toString)));
    }

    public static void main(String[] args) {
        Set<Integer> ints = new TreeSet<>(Arrays.asList(1, 2, 3, 4, 5));
        Set<Double> doubles = new TreeSet<>(Arrays.asList(1.0, 2.0, 3.0, 4.0, 5.0));
        System.out.println(union(ints, doubles));
        System.out.println(union(ints, doubles,
                () -> new TreeSet<>(Comparator.comparing(Number::doubleValue))));
    }
}

The two-argument union() uses a TreeSet that compares by the entries' toString(), which lets us store both Integer and Double values together in the Set. Alternatively, calling union() with a TreeSet supplier that compares by Number.doubleValue() gives us only 5 distinct elements:

[1, 1.0, 2, 2.0, 3, 3.0, 4, 4.0, 5, 5.0]
[1, 2, 3, 4, 5]

Rounding back to your question, I suppose HashSet() is a reasonably safe bet, since it doesn't do anything fanciful other than the usual hashCode()/equals() checks. And as you correctly inferred, since the method return type is just a Set, callers of union() should not have any other expectations - the Set interface simply says "this is how I am intended to be used.".


Window class for a game engine I am writing my own game engine, and I wanted to share with my own window class written in C++ using GLFW as the windowing API. Is there someone who can help me better this class? I'm not a complete beginner in C++ but not an expert.

Window.h:

#pragma once

#include <GL\glew.h>
#include <GLFW\glfw3.h>
#include <iostream>

namespace Engine
{
    namespace graphics
    {
        class Window
        {
        private:

            int m_Width, m_Height;
            const char* m_Title;
            bool m_VSync;
            bool Init();

        public:

            GLFWwindow* m_Window;
            Window(const char* title, int width, int height, bool vsync);
            ~Window();
            void Clear(float x, float y, float z);
            void Update();
            bool Closed() { return glfwWindowShouldClose(m_Window); }
        };
    }
}

Window.cpp:

#include "Window.h"

namespace Engine
{
    namespace graphics
    {
        static void key_callback(GLFWwindow* window, int key, int scancode, int action, int mods)
        {
            if (key == GLFW_KEY_ESCAPE && action == GLFW_PRESS)
                glfwSetWindowShouldClose(window, GLFW_TRUE);
        }

        void framebuffer_size_callback(GLFWwindow* window, int width, int height)
        {
            glViewport(0, 0, width, height);
        }

        bool Window::Init()
        {
            if (!glfwInit())
            {
                std::cout<<"ERROR::GLFW::INIT : GLFW failed to initialize"<<std::endl;
                return false;
            }

            glfwWindowHint(GLFW_CONTEXT_VERSION_MAJOR, 3);
            glfwWindowHint(GLFW_CONTEXT_VERSION_MINOR, 3);

            m_Window = glfwCreateWindow(m_Width, m_Height, m_Title, NULL, NULL);
            glfwSetKeyCallback(m_Window, key_callback);
            glfwSetFramebufferSizeCallback(m_Window, framebuffer_size_callback);

            glfwMakeContextCurrent(m_Window);

            if (glewInit() != GLEW_OK)
            {
                glfwTerminate();
                std::cout << "ERROR::GLEW::INIT : GLEW failed to initialize"<<std::endl;
                return false;
            }

            if (m_VSync)
            {
                glfwSwapInterval(1);
                std::cout << "WINDOW::OPTIONS::VSYNC::ON" << std::endl;
            }
            else
            {
                std::cout << "WINDOW::OPTIONS::VSYNC::OFF" << std::endl;
            }
            return true;
        }

        Window::Window(const char* title, int width, int height,bool vsync)
        {
            m_Title = title;
            m_Width = width;
            m_Height = height;
            m_VSync = vsync;
            Init();
        }

        void Window::Clear(float x, float y, float z)
        {
            glClearColor(x, y, z, 1.0f);
            glEnable(GL_DEPTH_TEST);
            glClear(GL_COLOR_BUFFER_BIT |GL_DEPTH_BUFFER_BIT);
        }

        void Window::Update()
        {
            int width, height;

            glfwGetFramebufferSize(m_Window, &width, &height);

            glViewport(0, 0, width, height);

            glfwSwapBuffers(m_Window);
            glfwPollEvents();
        }

        Window::~Window()
        {
            glfwTerminate();
            glfwDestroyWindow(m_Window);
        }
    }
}

Window::Init() performs OpenGL initialization and creates the GL window, I do not completely agree with this: I'd avoid to perform expensive tasks inside ctor (which should, IMO, just initialize the object in a consistent state). Initialization may be done lazily when required (better, I think) or made explicit with a public Initialize() method (which has the disadvantage to be another function to call and the overhead of the checks to determine if it has been called or not but where you can use virtual functions without worries).

Your actual Window::Init() returns a bool which is simply unused. However when initialization fails the object is not in a valid state and any other function call may simply crash. Calling code has to any clue and it can't gracefully exit. Feel free to throw an exception (even easier if you have a separate Initialize() method) instead of returning a bool:

if (!glfwInit())
    throw std::runtime_error("GLFW failed to initialize.");

Also note that if you log something to console then errors should go to std::cerr instead of std::cout:

std::cerr << "ERROR::GLFW::INIT : GLFW failed to initialize" << std::endl;

If you properly handle error cases (instead of returning an unused flag to signal an error) then you do not need to repeat clean-up code in your initialization function because dtor will take care of it:

Window::~Window()
{
    if (m_IsInitialized)
        glfwTerminate();

    if (m_Window != nullptr)
        glfwDestroyWindow(m_Window);
}

About m_Window: why it's public? There is almost never a good reason to have a public field. Note that if, for any reason, you have to expose this pointer to the external world then I strongly suggest to use std::shared_ptr, you may also go one step further and implement a custom delete function that will call glfwDestroyWindow() (I think it's much easier to add few assertions in this case then track an access violation because memory has been released somewhere). Also this opens the door to more complex scenarios (for example using std::move() to transfer window ownership to another Window instance).

Jumping back to ctor, you can use an initializer list:

Window::Window(const char* title, int width, int height, bool vsync)
    : m_Title(title),
      m_Width(width),
      m_Height(height),
      m_VSync(vsync)
{
}

It does not really matter for PODs however you may (and should) declare all those members as const (and * const) then you must use this syntax.

You're not validating inputs, that's a bad thing! Is, for example, a negative width allowed? Throw the right exception as soon as possible and you won't need to hunt a bug later.

if (width < 0)
    throw std::invalid_argument("Width cannot be negatie.");


Generating a warning when I/O size limit is exceeded A function is doing IO operation . If the IO reaches a size more than warning limit storage array will send the event to host. The same function will fetch the event.

I have used the verb-noun pattern and the name of the function is as below.

  "Get-SoftAlertAfterRunIO"

It is not making sense as it is not only getting, it is also generating.

Another name I can use is:

  Generate-SoftAlert

But it is not an approved verb. What could be the equivalent verb for generate?

Or is there any good name to reflect the behavior of this code.

The full code is given below

    function Generate-SoftAlert  {
    param
    (
        $VolumeObject,
        $Size
    )

    $Path = $VolumeObject.Path
    $IOTimeStamp = StartIO -Path $Path -LimitSizeInGB $Size

    $IOStarttimeInArray = $IOTimeStamp.IOStarttimeInArray
    $IOStarttimeInHost = $IOTimeStamp.IOStartTimeInHost

    $IOEndtimeInArray = $IOTimeStamp.IOEndtimeInArray
    $IOEndtimeInHost = $IOTimeStamp.IOendTimeInHost

    $VolumeName = $VolumeObject.Name
    $IsSoftAlertInArray= Test-SoftAlertInArray -VolumeName $VolumeName -FromTimeStamp $IOStarttimeInArray -ToTimestamp $IOEndtimeInArray

    $DiskNumber = $VolumeObject.DiskNumber
    $SoftAlert = Get-SoftAlertInHost -After $IOStarttimeInHost -Before $IOEndtimeInHost -DiskNumber $DiskNumber
    $IsSoftAlert = ($SoftAlert -ne $null)

    $Message = $SoftAlert.Message
    $IsLogSense = $false
    If($IsSoftAlert -eq $true)
    {
        $IsLogSense = Test-LogSenseMessage -LogMessage $Message -DiskNumber $DiskNumber -thresholdInGb $WarningLimit
    }

    return [pscustomobject]@{
        SoftAlert = $IsSoftAlert
        LogSense = $IsLogSense
        SoftAlertInArray = $IsSoftAlertInArray
    }
}

Apply Approved Verbs for Windows PowerShell Commands article from Windows PowerShell Cmdlet Concepts, use New-SoftAlert:

  Common Verbs

  Windows PowerShell uses the VerbsCommon enumeration class to
  define generic actions that can apply to almost any cmdlet…

  Verb (alias): New (n)

  Action: Creates a resource. (The Set verb can also be used when creating a resource that includes data, such as the Set-Variable
  cmdlet.)

  Comments: For this action, do not use verbs such as Create, Generate, Build, Make, or Allocate.


Stock picker that tells when to buy and when to sell The task of this project was to create a method that takes in an array of stock prices, one for each hypothetical day. From there, the method should return the best day to buy and the best day to sell the stock to maximize return.

A few ground rules:

Stocks need to be bought before they can be sold
It is possible that the lowest priced day can be the last day, and the highest price day could be the first day.

With that, and a relatively new working knowledge of Ruby, here's what I came up with. It works, but I'm sure there could be improvements. Be gentle!

def stock_prices (array)
    $largest_difference = 0
    array.each_with_index {|value, index|
        array.each {|i|
            $difference = value -  i
            if ($difference <= $largest_difference) && (index < array.rindex(i))
                $negative_array = [] << $difference
                $negatives = [index, array.rindex(i)]
                $largest_difference = $difference
            end
        }
    }
    if $negative_array.nil?
        puts "The stock should be bought and sold at [0, 1], respectively"
    else
        puts "The stock should be bought and sold at #{$negatives}, respectively"
    end
end

stock_prices([17,3,6,9,15,8,6,1,10])
#stock_prices([25,2,10,9])
#stock_prices([10,12,5,3,20,1,9,20])
#stock_prices([10,9,8,7,6])
#stock_prices([18,17,17,16,15])

The code is a pretty sensible solution for this problem.

Never put $ before a variable in Ruby, it becomes global and may be modified from anywhere.

I think that function programming is more apt to algorithmic problems than imperative. I solved it like this:

def stock(prices)
  ((0...prices.length).to_a)
    .repeated_permutation(2)
    .select {|start, finish| finish > start}
    .max_by {|start, finish| prices[finish] - prices[start]}
end

It is more close to how you would describe the problem, anyway you may be intersted in PBRTM, the main programming style supported by Ruby. (Programming By Reading The Manual) Ruby gives you many many library functions and you may enjoy scrolling through them to find the ones that help you shorten your code.

Getting to the real meat, both yours and my approach run in O(N^2) time complexity. That means that for a million (10^6) stock prices, the code must make (10^12) operations, that is not feasible.

I suggest something like this:

I find min and max of array. Is max after min? If so I am done. Otherwise I delete the least far from average and start again.

This is almost linear, but may not be optimal.

OR

You may sort the data O(N log N) and try first and last, if in the array the index of last is after the index of first, done. Otherwise delete the least far from average between first and last and repeat. Also this may fail to provide the optimal result in edge cases.

All in all, a quadratic solution is easy but slow but a quasilinear one is hard but fast.


Make constexpr array of function values I have a function f(i, j) that accepts two std::size_t arguments and I want to make a constexpr std:array out of its values on a 2d grid, (i = 0, ..., rows-1; j = 0, ..., cols-1) in, say, column-major order.

The code:

#include <array>
#include <tuple>
#include <type_traits>

template<std::size_t rows, std::size_t cols>
constexpr auto make_2d_index_table()
{
    std::array<std::array<std::size_t, 2>, rows * cols> table{};
    for (std::size_t i = 0; i < table.size(); ++i)
        table[i] = {i % rows, i / rows};

    return table;
}

template<std::size_t rows, std::size_t cols, class Func>
constexpr std::array<std::invoke_result_t<Func, std::size_t, std::size_t>, rows * cols>
    make_array_of_2d_func_values(Func&& func)
{
    return apply([&func](auto... row_col)
    {
        return std::array<std::invoke_result_t<Func, std::size_t, std::size_t>, rows * cols>
            {std::apply(std::forward<Func>(func), row_col)...};
    }, make_2d_index_table<rows, cols>());
}

Usage:

constexpr auto arr = make_array_of_2d_func_values<5, 3>(
    [](auto i, auto j) { return i + 2 * j; });

The object of the type returned by a function may not be default constructible. So I can't just create an array<invoke_result_t<...>, rows * cols> arr{} of default constructed objects and then initialize it in the for loop like in make_index_table().

Is this a good solution?

One simple improvement: the return type of make_array_of_2d_func_values() can be auto, to save writing std::array<std::invoke_result_t<Func, std::size_t, std::size_t>, rows * cols> twice.

I think you may be able to specialize std::tuple_size and std::get for the index table so that it doesn't require any storage even when invoked with non-constexpr arguments; I'm still experimenting but expect an update to this answer!