JavaScript Priority Queue implementation using a binary heap I'm currently going over Robert Sedgewick's Algorithms book. For the implementation of A priority queue using a binary heap I implemented the code using ES6. I believe to have more experience with Ruby but I have come to enjoy working with ES-6 using classes.
I would like feedback on how can the code be improved, if there is anything that can be optimized, if am following best practices or if I'm breaking any principles.
 class Heap {
      constructor() {
        this.n = 0;
        this.pq = [];
      }

      size() {
        return this.n;
      }

      isEmpty() {
        this.n === 0;
      }

      swim(k) {
        while(k > 1 && this.less(Math.floor(k / 2), k)){
          this.exch(Math.floor(k / 2), k);
          k = Math.floor(k / 2)
        }
      }

      sink(k) {
        while(2 * k <= this.n) {
          let j = 2 * k;

          if(this.pq[j + 1] != null) {
            if(k > 1 && this.less(j, j + 1)){
              j++;
            }
          }

          if(this.pq[k] > this.pq[j]) {
            break;
          }
          this.exch(k, j)
          k = j
        }
      }

      insert(v) {
        this.pq[++this.n] = v;
        this.swim(this.n);
      }

      delMax() {
        let max = this.pq[1];
        this.exch(1, this.n--);
        this.pq[this.n + 1] = null;
        this.sink(1);
        return max;
      }

      less(i, j) {
        return this.pq[i] < this.pq[j];
      }

      exch(i, j) {
        let temp = this.pq[i];
        this.pq[i] = this.pq[j];
        this.pq[j] = temp;
      }
    }

Here is some tests:
let heap = new Heap();
heap.insert("t")
heap.insert("p")
heap.insert("r")
heap.insert("n")
heap.insert("h")
heap.insert("o")
heap.insert("a")
heap.insert("e")
heap.insert("i")
heap.insert("g")
heap.insert("s")

console.log(heap.isEmpty())
console.log(heap.size())
heap.delMax()
console.log(heap.pq)

Naming
The name Heap is confusing, since this is a priority queue, not a heap. The heap is merely an internal private implementation detail that should not be exposed to the outside world. Just call it what it is: a PriorityQueue.
The same applies to heap. This one is a local variable with limited scope and obvious semantics, so I would be fine with pq as a name in this case.
Conversely, the pq field actually looks like it is a heap, not a priority queue, so it should probably be named heap.
delMax sounds like the method is deleting the highest priority element, but it is actually popping the highest priority element, i.e. it is returning it. pull, poll, pop, getMax, dequeue are all popular names for this operation.
In fact, in the beginning, I thought you don't even provide this operation at all, since I was led amiss by the name. Even in your own tests, you actually ignore the return value!
Bug
Your isEmpty() method does not return anything. It is evaluating an expression and then throwing away the result. It should be something like this instead:
isEmpty() {
    return this.n === 0;
}

Testing
The above bug is actually caught be the very tests you posted in your question. That seems to indicate that you wrote those tests but are not running them, otherwise you would have noticed.
You should regularly run your tests, ideally in an automated fashion and with automated verification of the results. I run my tests automatically every time I save, every time before I commit to my local Git repository, every time locally before I push to my remote Git repository and then again on the remote repository every time someone pushes to it, every time before and after a merge, every time before a release … you get the idea. Run them as often as possible.
Potential bug?
I have the feeling, although I have not tested it, that your priority queue will not deal well if I want to store null in it.
Triple equals
There is one place where you use the Abstract Equality Comparison Operator == or its negation !=. It is generally best if you forget about its existence and never use it.
Always use the Strict Equality Comparison Operator === or its negation !== instead.
Consistency
Sometimes you use the Abstract Equality Comparison Operator and sometimes the Strict Equality Comparison Operator. Sometimes you use 4 spaces for indentation, sometimes 2. Sometimes you use the term heap and sometimes priority queue (or pq) to refer to the priority queue.
Getters
isEmpty and size should probably be getters instead of normal methods:
get size() {
    return this.n;
}

get isEmpty() {
    return this.n === 0;
}

And the tests need to change accordingly as well:
console.log(pq.isEmpty);
console.log(pq.size);

Use abstractions internally
I am a big fan of using public abstractions also internally. Not everybody agrees with this, though.
So, personally, I would use the size getter in isEmpty instead of accessing the internal n field:
get isEmpty() {
    return this.size === 0;
}

That way, if someone extends your class and overrides some parts of it with a different implementation that doesn't use an n field, isEmpty will still work unchanged.
const over let
When ECMAScript 2015 introduced let and const, the general sentiment was let is the new var, you should always use let. Personally, I disagree, and I think const should be the new var, and you should always const unless you really, really, really need to re-assign it and can't find a way around. Then, and only then, use let.
In your code, heap, max, and temp are never reassigned, so you can use const for them instead.
Class fields
n and pq should probably be class fields. Note that class fields are currently a Stage 3 proposal, which means that while it is highly likely that they will end up unchanged in the ECMAScript Language Specification, they have not been accepted yet and have definitely missed the window for the 2020 release.
class PriorityQueue {
    n = 0;
    pq = [];

    // no constructor needed, all fields already initialized
}

Private methods and fields
swim, sink, less, and exch should be private methods, they shouldn't be part of the public API, same for the class field n.
pq (or heap) should probably also be private. You are using it externally in the tests, but I don't think this is something that should be exposed to the outside world.
class PriorityQueue {
    #n = 0;
    #heap = [];

    get size() {
        return this.#n;
    }

    get isEmpty() {
        return this.size === 0;
    }

    #swim(k) {
        while (k > 1 && this.#less(Math.floor(k / 2), k)) {
            this.#exch(Math.floor(k / 2), k);
            k = Math.floor(k / 2);
        }
    }

    #sink(k) {
        while (2 * k <= this.#n) {
            let j = 2 * k;

            if (this.#heap[j + 1] !== null) {
                if (k > 1 && this.#less(j, j + 1)) {
                    j++;
                }
            }

            if (this.#heap[k] > this.#heap[j]) {
                break;
            }
            this.#exch(k, j);
            k = j;
        }
    }

    insert(v) {
        this.#heap[++this.#n] = v;
        this.#swim(this.#n);
    }

    getMax() {
        const max = this.#heap[1];
        this.#exch(1, this.#n--);
        this.#heap[this.#n + 1] = null;
        this.#sink(1);
        return max;
    }

    #less(i, j) {
        return this.#heap[i] < this.#heap[j];
    }

    #exch(i, j) {
        const temp = this.#heap[i];
        this.#heap[i] = this.#heap[j];
        this.#heap[j] = temp;
    }
}

Note that private methods are also in Stage 3.
API limitations
As currently implemented, the values stored in your priority queue and the priorities assigned to the values are actually the same thing. This is very limiting:

You cannot have an ordering of priorities that is different from the natural ordering of the values. For example, you cannot have a priority queue where the priority is based on the length of a string instead of its lexicographic ordering.
You can only store values in your priority queue that have a total ordering. Note that in ECMAScript this is trivially true, because all objects are totally ordered with respect to each other, but the ordering is not always intuitive, or do you know offhand what the result of { b: "a", a: "b" } < ["a", 2] is?

Typically, priority queue implementations resolve this in one of two ways:

Each value is associated with a numeric priority.
The priority queue is instantiated with a comparison function that expresses the total ordering relation between the values.

Solution #1 would mean that you change the signature or your insert method to something like this:
insert(v, p)

and then use p as the key for the heap.
Solution #2 would mean that you change the signature of the constructor to something like this:
constructor(f)

and then use f inside less as the comparison function instead of <.
Here is a rough sketch of what that would look like for option #1, the only changes are in insert and less:
insert(v, p) {
    this.#heap[++this.#n] = { element: v, priority: p };
    this.#swim(this.#n);
}

#less(i, j) {
    return this.#heap[i].priority < this.#heap[j].priority;
}

The usage would then look like this:
pq.insert("tally", 2);
pq.insert("plus", 1);
pq.insert("rust", 8);
pq.insert("no", 127);

The version for option #2 would look something like this:
#comparator;

constructor(f) {
    this.#comparator = f;
}

#less(i, j) {
    return this.#comparator(this.#heap[i], this.#heap[j]);
}

And the usage like this:
const pq = new PriorityQueue((a, b) => a.length < b.length);

Further API additions
Being a collection, your priority queue should implement the Iterable interface.
All major data structures in the ECMAScript standard library have methods entries, keys, and values. It makes sense to conform to that interface as well.


Remapping of arrays on structs (code style) I'm working on a piece of code that remaps a set of signals (1-D integer arrays) onto a set of multi channel samples. I hope the typedefs and the remapping clearifies what I mean. If not, please let me know.
#include <stdint.h>

#define SIGNAL_LENGTH 1024

/**
* Multiple linear input signals as read by a previous input stage
*/
typedef struct {
    // Mics inside
    int32_t micInside0[SIGNAL_LENGTH];
    int32_t micInside1[SIGNAL_LENGTH];
    int32_t micInside2[SIGNAL_LENGTH];
    int32_t micInside3[SIGNAL_LENGTH];
    // Mics aux
    int32_t micAux0[SIGNAL_LENGTH];
    int32_t micAux1[SIGNAL_LENGTH];
    int32_t micAux2[SIGNAL_LENGTH];
    int32_t micAux3[SIGNAL_LENGTH];
} InputSignals_t;

/**
* Single input struct to be passed to the following filtering stage
*/
typedef struct {
    // Mic inside 0
    int32_t micInside0a;
    int32_t micInside0b;
    int32_t micInside0Select;
    // Mic inside 1
    int32_t micInside1a;
    int32_t micInside1b;
    int32_t micInside1Select;
    // Mic inside 2
    int32_t micInside2a;
    int32_t micInside2b;
    int32_t micInside2Select;
    // Mic inside 3
    int32_t micInside3a;
    int32_t micInside3b;
    int32_t micInside3Select;
} FilterSample_t;

/**
* Map the linear input signals to interleved multi-channel filter input signals.
*/
void mapMicFilters(InputSignals_t *inputSignals, FilterSample_t filterSamples[SIGNAL_LENGTH]) {
    for(int i = 0; i < SIGNAL_LENGTH; i++) {
        FilterSample_t *filterSample = &filterSamples[i];

        // I am rather unhappy with the look of the following
        // code section where the mapping is done.

        // Mic inside 0
        filterSample->micInside0a = inputSignals->micInside0[i];
        filterSample->micInside0b = 0;
        filterSample->micInside0Select = 1;

        // Mic inside 1
        filterSample->micInside1a = inputSignals->micInside0[i];
        filterSample->micInside1b = 0;
        filterSample->micInside1Select = 1;

        // Mic inside 2
        filterSample->micInside2a = inputSignals->micInside0[i];
        filterSample->micInside2b = 0;
        filterSample->micInside2Select = 1;

        // Mic inside 3
        filterSample->micInside3a = inputSignals->micInside0[i];
        filterSample->micInside3b = 0;
        filterSample->micInside3Select = 1;
    }
}

I am rather unhappy with the look of the code section where the mapping is done.The reason for this is that the lines become very long when the signal names become more specific (therefore longer). But a line break within the statements would really mess up the readability:
filterSample->micInside0a =
    inputSignals->micInside0[i];
filterSample->micInside0b =
    0;
filterSample->micInside0Select =
    1;

Do you have any suggestions on how to format the code? In dynamic languages I would just take a hashmap with the variable names and loop through it. But I have (and want) to use C.
Thank you!

Whenever I see (e.g.):
int foo0, foo1, foo2, foo3, ..., fooN;

I think that can be replaced more cleanly with an array. So, I'd rather have:
int foo[N];

Thus, I'd change:
int32_t micInside0[SIGNAL_LENGTH];

(et. al.) into:
int32_t micInside[4][SIGNAL_LENGTH];

That sort of thing occurs in several more places for different variables as well.

A good style rule is to use shorter names for argument names and stack/automatic variables [that have shorter scope/duration] and longer names for globals.
For example: FilterSample_t filterSamples[SIGNAL_LENGTH] is a bit redundant and could be shortened to (e.g.) FilterSample_t fslist[SIGNAL_LENGTH]
Likewise, FilterSample_t *filterSample = &filterSamples[i]; is a bit long and could be shortened to: FilterSample_t *fscur = &fslist[i];
To me, the longer names are "too much eye candy".
Also, with the shorter names, the lines will be shorter, so they're less likely to need to be wrapped to multiple lines [with the indenting].

If you add an additional struct, you can reorganize/simplify micInside* and micAux*.

As to the indenting of the second [wrapped] line that you didn't like, personally, I do that 10 times / day. I've found that to be very clean. But, I've been doing that for 20+ years, so I'm used to it.

Note that you did [for all "X", where "X" is one of 0,1,2,3]:
filterSample->micInsideXa = inputSignals->micInside0[i];

But, I think that was a typo and you meant to do:
filterSample->micInsideXa = inputSignals->micInsideX[i];

I realize that school classes teach the use of i, j, etc. But, I've found that more descriptive names can make the code clearer.

Anyway, I've refactored your code incorporating these ideas.
Caveat: I had some difficulty with the remapping to arrays and I had to guess a bit about some of the relationships. (e.g.) CHANMAX for "max # of channels" was a guess, based on my experience with video and audio processing and seeing "mic" (as in "microphone") and "aux" (as in "auxiliary audio").
And I may have messed this up by transposing the array indexes, but it should give you an idea of what I'm driving at:
#include <stdint.h>

#define SIGNAL_LENGTH 1024

#define CHANMAX     4

/**
* Multiple linear input signals as read by a previous input stage
*/
typedef struct {
    // Mics inside
    int32_t micInside[CHANMAX][SIGNAL_LENGTH];

    // Mics aux
    int32_t micAux[CHANMAX][SIGNAL_LENGTH];
} InputSignals_t;

typedef struct {
    int32_t mic_a;
    int32_t mic_b;
    int32_t mic_select;
} micfilter_t;

/**
* Single input struct to be passed to the following filtering stage
*/
typedef struct {
    micfilter_t filt_micinside[CHANMAX];
} FilterSample_t;

/**
* Map the linear input signals to interleved multi-channel filter input signals.
*/
void
mapMicFilters(InputSignals_t *inputSignals, FilterSample_t fslist[SIGNAL_LENGTH])
{
    for (int isamp = 0; isamp < SIGNAL_LENGTH; isamp++) {
        FilterSample_t *fscur = &fslist[isamp];
        InputSignals_t *inpbase = &inputSignals[isamp];

        // I am rather unhappy with the look of the following
        // code section where the mapping is done.

        for (int ichan = 0;  ichan < CHANMAX;  ++ichan) {
            micfilter_t *fchan = &fscur->filt_micinside[ichan];
            int32_t *inpcur = inpbase->micInside[ichan];
            fchan->mic_a = inpcur[isamp];
            fchan->mic_b = 0;
            fchan->mic_select = 0;
        }
    }
}


The code reads from a specified offset to the specified number of bytes in a file The code reads the specified number of bytes from a specified offset in a file, I have handled the partial reads and bytes > buf_size conditions. Could anyone review the code? and say whether the handling strategy is good. The constraint I am  not allowed to use system calls other than open, close, read, write,Thanks a ton.

#include<stdio.h>
#include<stdlib.h>
#include<unistd.h>
#include<sys/types.h>
#include<error.h>
#include<errno.h>
#include<fcntl.h>
#define buf_size 5

int main(int argc, char *argv[])
{
    int bytes;
    int offset;
    int fd;
    char *file;
    char buf[buf_size];
    int rlen = 0;
    int len = 0;
    int i = 0;

    if (argc != 4)
        error(1, 0, "Too many or less than the number of arguments");
    file = argv[1];
    offset = atoi(argv[2]);
    bytes = atoi(argv[3]);
    fd = open(file, O_RDONLY);
    printf("The values of the file descriptor is : %d\n", fd);
    if (fd == -1)
        error(1, errno, "Error while opening the file\n");

    while (1) {
        rlen = read(fd, buf, offset);
        if (rlen == -1)
            error(1, errno, "Error in reading the file\n");
        len = len + rlen;
        if(len == offset) {
            len = 0;
            while (1) {
                rlen = read(fd, buf, bytes);
                if (rlen == -1)
                    error(1, errno, "Error in reading the file\n");
                if (rlen == 0)
                    return 0;
                len = len + rlen;
                for (i = 0; i < rlen; i++)
                    putchar(buf[i]);
                if (len == bytes) {
                    printf("\nSuccess\n");
                    return 0;
                }
                bytes = bytes - rlen;
            }
        }
    }
}
```

Fails with large offset, bytes

read(fd, buf, offset); reads out of buf[] bounds when offset > buf_size.  Same for bytes.  This should be tested.

Unclear why buf[buf_size] is so small (5).  How about 4096 or 1 Meg?

Handle large offset

I'd expect code to handle offsets far larger than the sizeof buf.  Looping on read(fd, buf, offset); if needed.

int math

Files sizes can well exceed INT_MAX.  For offset, bytes, I'd use long long.

Unneeded loop

Rather than a for() loop for (i = 0; i < rlen; i++) putchar(buf[i]);, use write(1, ...).

Missing close()

Function Limitations

OP has "not allowed to use system calls other than open, close, read, write".  What about error(), printf(), atoi(), putchar() - or are those classified differently?

With such a limit, looks like a lot of includes.

Algorithm flaw?

The first while (1) loop looks wrong.

Either the first iteration will get the expect offset number of bytes and proceed to the 2nd while ().

Or

it will read insufficient number of bytes (for reasons other than no -more bytes will ever exist) and loop again, trying to read offset bytes again.  I'd expect that offset would have been reduced by the previous rlen.  Without that reduction, if(len == offset) may never be satisfied as reading too many offset bytes is then possible.

I'd expect 2 while loops that are not nested.  First to read the offset bytes, next to read the bytes.

Further this could be sub-function calls.  Something like:

if (read_bytes(handle, offset, no_echo) == OK) {
  if (read_bytes(handle, bytes, echo_to_stdout) == OK) {
    success();
  }
}
fail();


Storing and retrieving student scores in three subjects This is written to find percentages of three subjects. Original HackerRank practice problem is here.

  Input Format

  The first line contains the integer N, the number of students. The next  N lines contains the name and marks obtained by that student separated by a space. The final line contains the name of a particular student previously listed.

  Output Format

  Print one line: The average of the marks obtained by the particular student correct to 2 decimal places.

This code is working but I need better way of writing this code.

a = input()
a = int(a)
dict = {}
f = []
n = {}
for i in range(a):
        f = input().split()
        name = f[0]
        n['phy'] = float(f[1])
        n['che'] = float(f[2])
        n['math'] = float(f[3])
        f = []
        dict[name] = n
        n ={}
nam = input();
per = (dict[nam]['phy']+dict[nam]['che']+dict[nam]['math'])/3
print("%.2f"%per)

PEP 8, the official Python style guide, specifies four spaces per level of indentation.  Since whitespace is significant in Python, that is a pretty strong convention.

The challenge states that the first line contains N, the number of students.  So, don't call that value a in the code, and don't use the variable n to mean something else.  It's confusing.

The variable i is never used.  It is customary to use _ as the name of a "throwaway" variable whose value does not matter.

Clearing f and n at the end of the loop is cumbersome and unnecessary.  Avoid cryptic variable names like f and n whose meanings are not obvious.  Even names like nam and per are unnecessarily unfriendly.  (Learn from Ken Thompson's biggest regret.)

For this problem, there is no need to store the scores for each subject separately.  You can just store the average right away.

scores = {}
n = int(input())
for _ in range(n):
    line = input().split()
    name = line.pop(0)
    scores[name] = sum(float(score) for score in line) / len(line)
print("%.2f" % scores[input()])


Method on Laravel model to find courses related to a course I have a Laravel project that has courses. I want to take the related courses from one.

Should I create the method in the model (the php class) or in the controller?

Currently I have this code in the model:

class Course extends Model {

    public function relatedCourses() {

        $all_courses = new Collection;

        // Similar name
        $words = explode($this->name, " ");
        foreach ($words as $word) {

            $courses = Course::where(function($query) use ($word) {
                $query->where('id', '<>', $this->id)->where('name_es', 'LIKE', "%$word%")->orWhere('name_cat', 'LIKE', "%$word%");
            })->get();

            $all_courses = $all_courses->merge($courses);
        }

        return $all_courses;
    }

}

Should I move it to the controller?

No, this code belongs in the Model (or a related repository if you where using such a design, though thats kind of overkill with Active Record).

You can however reduce the number of queries by using the foreach loop to build a single query, instead of making a query per word:

public function relatedCourses()
{
    $words = explode($this->name, " ");
    return Course::where(function($query) use ($words) {
        $query->where('id', '<>', $this->id);
        foreach ($words as $key => $word) {
            if($key==0){
                $query->where('name_es', 'LIKE', "%$word%")->orWhere('name_cat', 'LIKE', "%$word%");
            }else{
                $query->orWhere('name_es', 'LIKE', "%$word%")->orWhere('name_cat', 'LIKE', "%$word%");
            }
        }
    })->get();
}


For-Loop with numpy array too slow I'm trying to speed up the execution of the following problem:

I have two functions which are used together.

Here are the functions explained:

The first function simulates N coin flips and returns an array of strings with the results:

def throw_a_coin(N):
    return np.random.choice(['H','T'], size=N)

The second function takes in number_of_samples, sample_size
and returns sample_probs.
The function simulates number_of_samples runs of sample_size coin flips and stores the result of num_heads / sample size in sample_probs for each run.

Here is the code:

def make_throws(number_of_samples, sample_size):
    sample_probs = np.zeros(number_of_samples)
    for i in range(number_of_samples):
        num_heads = sum(throw_a_coin(sample_size) == "H")
        sample_probs[i] = float(num_heads) / sample_size
    return sample_probs

Now I want to simulate multiple runs with different sample_sizes and 200 runs for each sample size. So sample_size = 1 I would do 200 runs of 1 coin flip, for sample_size = 10 I would do 200 runs of 10 coin flips and so on.

Here is the code that takes so long:

mean_of_sample_means = np.zeros(len(sample_sizes))
std_dev_of_sample_means = np.zeros(len(sample_sizes))

for i in range(len(sample_sizes)):
    prob = make_throws(200, sample_sizes[i])
    mean_of_sample_means[i] = np.mean(prob)
    std_dev_of_sample_means[i] = np.std(prob)

I am pretty sure that this process can be improved by getting rid of the for loops and using array-operations instead. But I just cannot think of how to apply throw_a_coin or make_throws to an array.

My toy example I build from your snippets:

import numpy as np

def throw_a_coin(N):
    return np.random.choice(['H','T'], size=N)

def make_throws(number_of_samples, sample_size):
    sample_probs = np.zeros(number_of_samples)
    for i in range(number_of_samples):
        num_heads = sum(throw_a_coin(sample_size) == "H")
        sample_probs[i] = float(num_heads) / sample_size
    return sample_probs

sample_sizes = [np.random.randint(1, 1e3) for idx in range(500)]

mean_of_sample_means = np.zeros(len(sample_sizes))
std_dev_of_sample_means = np.zeros(len(sample_sizes))

for i in range(len(sample_sizes)):
    prob = make_throws(200, sample_sizes[i])
    mean_of_sample_means[i] = np.mean(prob)
    std_dev_of_sample_means[i] = np.std(prob)

cProfile (ordered by cumtime) reveals the problem (runs in 82.359 seconds):

ncalls  tottime  percall  cumtime  percall filename:lineno(function)
 133/1    0.003    0.000   82.359   82.359 {built-in method builtins.exec}
     1    0.002    0.002   82.359   82.359 fast_flip.py:1(<module>)
   500    0.940    0.002   82.200    0.164 fast_flip.py:8(make_throws)
100000   79.111    0.001   79.111    0.001 {built-in method builtins.sum}
100000    0.058    0.000    2.148    0.000 fast_flip.py:4(throw_a_coin)
100000    1.455    0.000    2.090    0.000 {method 'choice' of 'mtrand.RandomState' objects}
100000    0.207    0.000    0.635    0.000 fromnumeric.py:2456(prod)
100000    0.026    0.000    0.429    0.000 _methods.py:34(_prod)
101500    0.409    0.000    0.409    0.000 {method 'reduce' of 'numpy.ufunc' objects}
     6    0.000    0.000    0.176    0.029 __init__.py:1(<module>)

There is a steep gap after buildins.sum, i.e. you spend most time there. We can use np.sum instead (pushing it down to 3.457 seconds):

ncalls  tottime  percall  cumtime  percall filename:lineno(function)
 133/1    0.003    0.000    3.457    3.457 {built-in method builtins.exec}
     1    0.002    0.002    3.457    3.457 fast_flip.py:1(<module>)
   500    0.905    0.002    3.307    0.007 fast_flip.py:8(make_throws)
100000    0.046    0.000    1.869    0.000 fast_flip.py:4(throw_a_coin)
100000    1.287    0.000    1.823    0.000 {method 'choice' of 'mtrand.RandomState' objects}
201500    0.702    0.000    0.702    0.000 {method 'reduce' of 'numpy.ufunc' objects}
100000    0.172    0.000    0.536    0.000 fromnumeric.py:2456(prod)
100000    0.136    0.000    0.532    0.000 fromnumeric.py:1778(sum)
100000    0.023    0.000    0.378    0.000 _methods.py:31(_sum)
100000    0.021    0.000    0.364    0.000 _methods.py:34(_prod)
     6    0.000    0.000    0.178    0.030 __init__.py:1(<module>)

further we can replace the strings "H" and "T" with a Boolean and stay in numpy for longer (down to 1.633 seconds):

ncalls  tottime  percall  cumtime  percall filename:lineno(function)
 133/1    0.003    0.000    1.633    1.633 {built-in method builtins.exec}
     1    0.001    0.001    1.633    1.633 fast_flip.py:1(<module>)
   500    0.101    0.000    1.485    0.003 fast_flip.py:11(make_throws)
100000    0.169    0.000    0.893    0.000 fast_flip.py:4(throw_a_coin)
100000    0.724    0.000    0.724    0.000 {method 'uniform' of 'mtrand.RandomState' objects}
100000    0.122    0.000    0.491    0.000 fromnumeric.py:1778(sum)
100000    0.024    0.000    0.354    0.000 _methods.py:31(_sum)
101500    0.334    0.000    0.334    0.000 {method 'reduce' of 'numpy.ufunc' objects}
     6    0.000    0.000    0.178    0.030 __init__.py:1(<module>)

next we can get rid of throw_a_coin and instead sample a numer_of_samples x sample_size array of uniformly distributed random numbers and threshold them. This also allows us to vectorize the for loop and stay in numpy even longer (0.786 seconds):

ncalls  tottime  percall  cumtime  percall filename:lineno(function)
 133/1    0.003    0.000    0.786    0.786 {built-in method builtins.exec}
     1    0.003    0.003    0.786    0.786 fast_flip.py:1(<module>)
   500    0.053    0.000    0.634    0.001 fast_flip.py:4(make_throws)
   500    0.526    0.001    0.526    0.001 {method 'uniform' of 'mtrand.RandomState' objects}
     6    0.000    0.000    0.179    0.030 __init__.py:1(<module>)

Here is the code:

import numpy as np

def make_throws(number_of_samples, sample_size):
    # True == Heads
    throws = np.random.uniform(size=(number_of_samples, sample_size)) > 0.5
    sample_probs = np.sum(throws, axis=1) / sample_size
    return sample_probs

sample_sizes = [np.random.randint(1, 1e3) for idx in range(500)]

mean_of_sample_means = np.zeros(len(sample_sizes))
std_dev_of_sample_means = np.zeros(len(sample_sizes))

for i in range(len(sample_sizes)):
    prob = make_throws(200, sample_sizes[i])
    mean_of_sample_means[i] = np.mean(prob)
    std_dev_of_sample_means[i] = np.std(prob)

At this point we could start to tackle the for loop, but it would start to get very micro optimized. One might consider aggregating the results in prob and then using np.mean(prob, axis=1) and np.std(prob, axis=1) outside the loop, but that only nets like 20ms so it's more of a personal preference thing.


Inline Sum() vs query Sum() vs Where on a List object Just after a bit of feed back or peoples ideas on which practice is best.

I currently have a method but am unsure of the best way to approach it from a readability point of view or even from best practices point of view.  Or perhaps it doesn't really matter at all and they are all fine?

My three options I explored were:

1)

function double GetObjectArea(CustomObject obj, CustomEnum source)
{
   return obj.ListOfApplications.Sum(p => p.Source == source ? object.Area : 0);
}

2)

function double GetObjectArea(CustomObject obj, CustomEnum source)
{
   var query = from record in obj.ListOfApplications
               where record.Source == source
               select object.Area

   return query.Sum();
}

3)

function double GetObjectArea(CustomObject obj, CustomEnum source)
{
   return obj.ListOfApplications.Where(p.Source == source).Sum(object.Area);
}

I would go with the third, with the second as secondary choice.

The first mixes addition logic and comparison logic: it's not immediately clear that the zero is meant to exclude those objects, as opposed to giving a default for object.Area.

The second is better, but it hides the point behind a lot of unnecessary syntax.  The fact that you're working on object.Area and not record.Area gets lost.

The third is even better.  It can be conveniently read left-to-right, and all the information is grouped near what we're doing (condition is after the where, sum elements are after the sum).

Personally, I would use

double GetObjectArea(CustomObject obj, CustomEnum source)
{
   return obj.ListOfApplications.Count(p => p.Source == source) * object.Area;
}


Merging an overlapping collection of intervals I'm getting myself acquainted with Rust and decided to implement the classical overlapping intervals problem.

Here is a statement of the problem:

  Given a collection of intervals, write a function that merges all
  overlapping intervals and prints them out.

  For example, given [1, 3], [2, 6], [8, 10], and [7, 11], the function
  should print [1, 6], [7, 11]. Or given [5, 12], and [8, 10] the
  function should print [5, 12].

  You can assume that the first element of each interval is always less
  or equal than the second element of the interval.

This is my implementation:

use std::cmp;
use std::fmt::Write;

//--------------- Range ---------------//

#[derive(Copy, Clone)]
struct Range{
    start: i32,
    end: i32,
}

fn new_range(start: i32, end: i32)->Range{
    Range{
        start: start,
        end: end,
    }
}

impl Range{
    fn overlaps(&self,other: &Range) -> bool{
        (other.start >= self.start && other.start <= self.end)
        || (other.end >= self.start && other.end <= self.end)
    }

    fn merge(&mut self, other: &Range){
        self.start = cmp::min(self.start, other.start);
        self.end = cmp::max(self.end, other.end);
    }

    fn to_string(&self)->String{
        format!("[{},{}]",self.start,self.end)
    }
}

//--------------- RangeStack ----------------//

struct RangeStack{
    ranges: Vec<Range>,
}

fn range_stack_with_ranges(ranges : &Vec<Range>)->RangeStack{
    let mut raw_ranges = ranges.to_vec();
    raw_ranges.sort_by(|a,b| a.start.cmp(&b.start));

    let mut range_stack = RangeStack{
        ranges: Vec::new(),
    };

    for range in raw_ranges.iter(){
        range_stack.add(range);
    }

    range_stack
}

impl RangeStack{

    //private
    fn add(&mut self,range :&Range){
        if self.ranges.len() == 0 {
            self.ranges.push(range.clone());
        }else if self.ranges.last_mut().unwrap().overlaps(range){
                self.ranges.last_mut().unwrap().merge(range);
        }else{
                self.ranges.push(range.clone());
        }
    }

    fn to_string(&self) -> String {

        let mut res = String::new();
        for range in self.ranges.iter() {
            write!(&mut res, "{}", range.to_string()).unwrap();
        }

        res
    }
}

//--------------- Main ---------------//

fn main() {
    let v = vec![
        new_range(3,6),
        new_range(1,5),
        new_range(7,11),
        new_range(9,12),
        new_range(4,8),
    ];

    let rs = range_stack_with_ranges(&v);
    print!("Result: {}\n",rs.to_string());
}

I would appreciate it if someone could review the Rust code and let me know if there is anything that is not idiomatic. In particular, I'm not sure if my usage of mut would be considered ideal.

Some style issues:

Space before {
Spaces around ->
Space after ,
No space before :
Space before else
Indent else blocks 4 characters

Make constructor functions part of the impl:

impl Range {
    fn new(start: i32, end: i32) -> Range {
        Range {
            start: start,
            end: end,
        }
    }
}

Instead of creating a to_string method, implement Display instead. This allows you to automatically get .to_string(), but even better, means you can format the object without extraneous allocation:

impl fmt::Display for Range {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "[{},{}]", self.start, self.end)
    }
}

No need to use iter() when using a for loop on a slice or Vec, just take a reference:

for range in &self.ranges { ... }

Use pattern matching to avoid all the unwrap calls in RangeStack::add:

fn add(&mut self, range: &Range) {
    if let Some(last) = self.ranges.last_mut() {
        if last.overlaps(range) {
            last.merge(range);
            return;
        }
    }

    self.ranges.push(range.clone());
}

Never take a &Vec<T> as an argument. It needlessly limits what types you can accept. In your case, it also allows you to avoid the allocation of the vector in your main. Use a slice instead:

fn with_ranges(ranges: &[Range]) -> RangeStack { ... }

You probably always want to implement Debug, usually done via derive:

#[derive(Debug, Copy, Clone)]
struct Range { ... }

Consider making your with_ranges constructor more generic by implementing FromIterator:

impl FromIterator<Range> for RangeStack {
    fn from_iter<I>(iterator: I) -> Self
        where I: IntoIterator<Item=Range>
    {
        let mut raw_ranges: Vec<_> = iterator.into_iter().collect();
        // ...
    }
}

All together, this now looks like

use std::{cmp, fmt};
use std::iter::FromIterator;

#[derive(Debug, Copy, Clone)]
struct Range {
    start: i32,
    end: i32,
}

impl Range {
    fn new(start: i32, end: i32) -> Range {
        Range {
            start: start,
            end: end,
        }
    }

    fn overlaps(&self, other: &Range) -> bool {
        (other.start >= self.start && other.start <= self.end)
        || (other.end >= self.start && other.end <= self.end)
    }

    fn merge(&mut self, other: &Range) {
        self.start = cmp::min(self.start, other.start);
        self.end = cmp::max(self.end, other.end);
    }
}

impl fmt::Display for Range {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "[{},{}]", self.start, self.end)
    }
}

#[derive(Debug, Clone)]
struct RangeStack {
    ranges: Vec<Range>,
}

impl RangeStack {
    fn add(&mut self, range: &Range) {
        if let Some(last) = self.ranges.last_mut() {
            if last.overlaps(range) {
                last.merge(range);
                return;
            }
        }

        self.ranges.push(*range);
    }
}

impl fmt::Display for RangeStack {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        for range in &self.ranges {
            try!(write!(f, " {}", range));
        }
        Ok(())
    }
}

impl FromIterator<Range> for RangeStack {
    fn from_iter<I>(iterator: I) -> Self
        where I: IntoIterator<Item=Range>
    {
        let mut raw_ranges: Vec<_> = iterator.into_iter().collect();
        raw_ranges.sort_by(|a,b| a.start.cmp(&b.start));

        let mut range_stack = RangeStack {
            ranges: Vec::new(),
        };

        for range in &raw_ranges {
            range_stack.add(range);
        }

        range_stack
    }
}

impl<'a> FromIterator<&'a Range> for RangeStack {
    fn from_iter<I>(iterator: I) -> Self
        where I: IntoIterator<Item=&'a Range>
    {
        iterator.into_iter().cloned().collect()
    }
}

fn main() {
    let v = [
        Range::new(3, 6),
        Range::new(1, 5),
        Range::new(7, 11),
        Range::new(9, 12),
        Range::new(4, 8),
    ];

    let rs: RangeStack = v.iter().collect();
    print!("Result: {}\n", rs);
}


Two player random number guessing game I've created a two-player number guessing game in Haskell. My main objective was to practice dealing with "state" in a purely functional language (such as player scores, whose turn it is, etc.).
Here are the rules:
Gameplay:
Two players will take turns guessing a random number between 1 and 10. Answers will be typed into the command line.
Scoring:

If a player guesses the number correctly, they will be awarded 5 points
If a player is within two (inclusive) from the answer, the player will be awarded 3 points.
If a player is within three (inclusive) from the answer, they will be awarded 1 point.
If a player is 7 or more points off, they will lose a point. The score may not be negative.
All other offsets will result in zero points.
The game will continue until the one of the players reaches 10 points.

Caveats:

This is was not designed to be an exercise in enjoyable game design -- obviously the optimal solution is to always choose five, which doesn't make for a lot of excitement. :D
I am aware that Control.Monad.State exists, but I want to practice tracking state without it.
I know that the "mutual recursion" is difficult to follow. I would love some suggestions for getting rid of that which do not involve nesting if statements.

import Data.Char
import System.Random

main = do
    stdGen <- getStdGen
    play 0 0 P1 stdGen

play :: Int -> Int -> Player -> StdGen -> IO ()
play p1Score p2Score player stdGen
    | p1Score < 10 && p2Score < 10 = continueGame p1Score p2Score player stdGen
    | otherwise = putStrLn $ show (determineWinner p1Score p2Score) ++ " wins!"

continueGame :: Int -> Int -> Player -> StdGen -> IO ()
continueGame p1Score p2Score player stdGen = do
    putStr $ show player ++ "'s turn. Pick a number between 1 and 10: "
    chosenNumber <- getLine
    if isInteger chosenNumber
        then do
            let (randomNumber, newGen) = randomR (1, 10) stdGen :: (Int, StdGen)
            putStrLn $ "The answer is " ++ show randomNumber

            let pointsEarned = calcPointsEarned randomNumber (read chosenNumber)
            let newP1Score = min (max (p1Score + calcPointsEarnedForPlayer player P1 pointsEarned) 0) 10
            let newP2Score = min (max (p2Score + calcPointsEarnedForPlayer player P2 pointsEarned) 0) 10

            putStrLn $ "P1 Score: " ++ show newP1Score
            putStrLn $ "P2 Score: " ++ show newP2Score

            play newP1Score newP2Score (changeTurn player) newGen
        else do
            putStrLn "The input must be an integer"
            play p1Score p2Score player stdGen

data Player = P1 | P2 deriving (Show, Eq)

isInteger :: String -> Bool
isInteger = and . map isNumber

changeTurn :: Player -> Player
changeTurn player
    | player == P1 = P2
    | otherwise = P1

calcPointsEarned :: Int -> Int -> Int
calcPointsEarned actualAnswer chosenAnswer
    | offset == 0 = 5
    | offset <= 2 = 3
    | offset <= 3 = 1
    | offset >= 7 = (-1)
    | otherwise = 0
    where offset = abs $ chosenAnswer - actualAnswer

calcPointsEarnedForPlayer :: Player -> Player -> Int -> Int
calcPointsEarnedForPlayer actualTurn player pointsEarned
    | actualTurn == player = pointsEarned
    | otherwise = 0

determineWinner :: Int -> Int -> Player
determineWinner p1Score p2Score
    | p1Score > p2Score = P1
    | otherwise = P2
```

I'd contend that State is pure and functional, but I think translating your current code to use State is an excellent exercise so I'll leave that up to you.
The first thing I'd address is making your types do more of the bookkeeping. Well designed types lend themselves to correct-by-construction solutions.
data Player = P1 | P2 deriving Show

data Game = Game { turn :: Player, p1 :: Int, p2 :: Int } deriving Show

Prefer pattern matching to equality testing, it is frequently more terse. Decreased line noise often means increased readability.
changeTurn :: Player -> Player
changeTurn P1 = P2
changeTurn P2 = P1

Your determineWinner function has a (currently unreachable) logic error. If both player's scores are equal then it prefers handing victory to player two. This may not matter in your code as written, but if your code changes or you begin property testing or some other unforeseeable future event comes to pass, it could begin mattering unexpectedly. Handling ties is “morally” the right thing to do.
Also as-is it isn't really determining a winner by the rules of the game, only which player's score is higher.
winner :: Game -> Maybe Player
winner (Game _ p1 p2) =
    case (max p1 p2 >= 10, p1 > p2, p2 > p1) of
      (True, True, False) -> Just P1
      (True, False, True) -> Just P2
      (_, _ , _)          -> Nothing

Don't validate and then parse, parse and allow for failure. If your validation code is separate from your parsing code you risk them drifting out of sync and causing errors. In this case, use Text.Read.readMaybe from base and leave out your isInteger function entirely.
It's a good idea to separate as much of your pure game logic from IO actions as possible. It's easier to test, easier to understand, and enables you to reuse functionality you otherwise couldn't.
updateRound :: Int -> Game -> Game
updateRound n (Game P1 p1 p2) = Game P2 (boundScore $ p1 + n) p2
updateRound n (Game P2 p1 p2) = Game P1 p1 (boundScore $ p2 + n)

clamp :: Ord a => a -> a -> a -> a
clamp lo val hi = lo `max` val `min` hi

boundScore :: Int -> Int
boundScore n = clamp 0 n 10

This also obviates the need for changeTurn.
It's also usually handy to separate your display logic from your control logic, even if both are IO actions. It might be useful to you if you make use of the REPL while developing, your types often shouldn't include multiple copies of the same information (e.g., two numbers and their difference) as that carries a risk of the values getting out of sync. Those derived values might be all you want to see while working though.
displayGame :: Game -> IO ()
displayGame (Game _ p1 p2) = do
    putStrLn $ "P1 Score: " ++ show p1
    putStrLn $ "P2 Score: " ++ show p2

That said, it's good to separate your control logic from your sources of input and output also. It makes your program testable without needing to muck about with piping stdin and stdout. There's also a very elegant transformation when you decide to begin using State, but I'll leave that to you to figure out.
gameRound :: Int -> Int -> Game -> (Maybe Player, Game)
gameRound guess answer game =
  let
    score = calcPointsEarned guess answer
    nextGame = updateRound score game
  in
    (winner nextGame, nextGame)

receiveGuess :: Player -> IO Int
receiveGuess player = do
    putStr $ show player ++ "'s turn. Pick a number between 1 and 10: "
    input <- getLine
    case readMaybe input of
      Nothing -> do
        putStrLn "The input must be an integer"
        receiveGuess player
      Just guess -> pure guess

play :: Game -> IO ()
play game = do
    answer <- randomRIO (1, 10)
    guess <- receiveGuess (turn game)
    putStrLn $ "The answer is " ++ show answer
    let (mPlayer, nextGame) = gameRound guess answer game
    displayGame nextGame
    case mPlayer of
      Just player -> putStrLn $ show player ++ " wins!"
      Nothing -> play nextGame


GET with paging, sorting and filtering I have a GET method that handles paging, filtering, and sorting. Development has been fairly rapid and I'm not completely sold on some of the implementation. A few assumptions have to be made for this method: I'm using System.Dynamic.Linq. I want to put as much work as possible on SQL Server. The page will make use of both the resulting record set after filtering and paging have been applied, as well as the count of the filtered record set before paging is applied. I have full control of the entity definitions, and each has a [Key] attribute on only one property. Currently, I'm only interested in seeing the IsActive records.

Any thoughts on improvements are welcome. Thanks in advance.

    public IList<TEntity> Get<TParamater>(IList<Expression<Func<TEntity, TParamater>>> includeProperties, out int recordCount, Dictionary<string,string> filter, bool filterExact,
    string orderBy = null, bool orderByAsc = true, int pageNumber = 0, int perPage = 10)

    {
        //IQueryable that will eventually contain the record set
        IQueryable<TEntity> query = _context.Set<TEntity>();
        //IQueryable that will get the count of post-filter, but pre-page records
        IQueryable<TEntity> countQuery = _context.Set<TEntity>();
        List<TEntity> page;

        //countQuery mirrors query through the filter assignments
        query = query.Where("IsActive = true");
        countQuery = countQuery.Where("IsActive = true");

        if (filter.Count > 0)
        {
            string _template = filterExact ? "{0}.ToString().Equals(@0)" : "{0}.ToString().Contains(@0)";
            string _clause = "";
            foreach (KeyValuePair<string, string> entry in filter)
            {
                _clause = string.Format(_template, entry.Key);
                query = query.Where(_clause, entry.Value);
                countQuery = countQuery.Where(_clause, entry.Value);
            }
        }
        //countQuery's job is finished at this point
        recordCount = countQuery.Count();

        if (includeProperties != null)
        {
            foreach (var include in includeProperties)
            {

                query = query.Include(include);
            }
        }

        if (orderBy != null)
        {
            query = query.OrderBy(orderBy + ((orderByAsc) ? " ascending" : " descending"));
        }
        else
        {
            //Using reflection to find the [Key] attribute to use as a default ordering because Skip/Take requires an ordered set
            Type entityType = typeof(TEntity);
            var keyField = entityType.GetProperties()
                .Single(p => Attribute.IsDefined(p, typeof(KeyAttribute)));
            query = query.OrderBy(keyField.Name);
        }

        if (pageNumber > 0)
        {
            int _start = (pageNumber - 1) * perPage;
            query = query.Skip(_start);
            query = query.Take(perPage);
        }

        page = query.ToList();

        return page;
    }

//countQuery mirrors query through the filter assignments

I don't think that's necessary. You could have only one query for the first part of the method, then do recordCount = query.Count(); and then keep using query as you do now.

This way, you have less code and less chance of the count getting out of sync.

query.Where("IsActive = true")

I realize Dynamic Linq is an option, but I think you shouldn't use it when you don't have to. If possible, I would constraint TEntity to some Entity base class or IEntity interface (maybe you already do that) and then write:

query.Where(entity => entity.IsActive);

query = query.Skip(_start);
query = query.Take(perPage);

I don't see any reason why spread this over two lines:

query = query.Skip(_start).Take(perPage);

List<TEntity> page;

…

page = query.ToList();

return page;

The variable page does not serve any purpose, get rid of it:

return query.ToList();


x64 Assembly zeroing an array (8 bytes at a time) Is there a better way of implementing this other than using simd instructions?

What is the best way of dealing with arrays not divisible by 8, as in the code where if there are less than 8 bytes left to zero they just get zeroed 1 by 1?

Maybe it is faster to check how many bytes there are left and then zero them 2 bytes or 4 bytes at a time?

Does the checking outweigh the cost of doing them 1 by 1?

This is just a test for me to try to learn assembly so any, even small, improvements and tips are greatly appreciated.

Thank you

.code
ZeroArray proc
    cmp edx, 0
    jle Finished ; Check if count is 0

    cmp edx, 8
    jl SetupLessThan8Bytes ; Check if counter is less than 8

    mov r8d, edx ; Storing the original count
    shr edx, 3   ; Bit shifts the counter to the right by 3 (equal to dividing by 8), works because 2^3 is equal to 8
    mov r9d, edx ; Stores the divided count to be able to check how many single byte zeros the program has to do

MainLoop:
    mov qword ptr [rcx], 0 ; Set the next 8 bytes (qword) to 0
    add rcx, 8             ; Move pointer along the array by 8 bytes
    dec edx                ; Decrement the counter
    jnz MainLoop           ; If counter is not equal to 0 jump to MainLoop

    shl r9d, 3   ; Bit shifts the stored divided counter to the left by 3 (equal to multiplying by 8), 2^3 again
    sub r8d, r9d ; Subs the counts from eachother, if it equals zero all bytes are zeroed, otherwise r8d equals the amount of bytes left
    je Finished

SetFinalBytesLoop:
    mov byte ptr [rcx], 0 ; Sets the last byte of the array to 0
    inc rcx
    dec r8d
    jnz SetFinalBytesLoop

Finished:
    ret

SetupLessThan8Bytes:
    mov r8d, edx ; Mov the value of edx into r8d so the same code can be used in SetFinalBytesLoop
    jmp SetFinalBytesLoop

ZeroArray endp
end

Shave off a byte

    cmp  edx, 0
    jle  Finished            ; Check if count is 0

Using cmp is certainly not wrong, but the optimal way to check for any inappropriate counter value would be to use the test instruction.

    test edx, edx
    jle  Finished            ; Check if count is 0

Bypassing when the counter is zero is fine, but perhaps a negative counter value should rather be considered an error and handled accordingly?

Don't loose yourself in jumping around

    cmp  edx, 8
    jl   SetupLessThan8Bytes ; Check if counter is less than 8
    mov  r8d, edx            ; Storing the original count
    ...
    ...
SetupLessThan8Bytes:
    mov  r8d, edx
    jmp  SetFinalBytesLoop

When the counter in EDX is smaller than 8, you jump to SetupLessThan8Bytes where you just make a convenient copy of the counter and then jump again to SetFinalBytesLoop.
If you move the instruction that makes a copy of the original counter to right before where you compare the counter to 8, you can save yourself from writing 3 lines of code (a label, a mov, and a jmp). Moreover the program becomes clearer.

    mov  r8d, edx            ; Storing the original count
    cmp  edx, 8
    jl   SetFinalBytesLoop   ; Check if counter is less than 8

You don't even have to compare to 8 at all!

When you shift the counter in EDX 3 times to the right in order to find out how many qwords you have to process, you can look at the zero flag. If the ZF is set (meaning no qwords at all), you instantely know that the counter is in the range [1,7], and so the above snippet becomes:

    mov  r8d, edx            ; Storing the original count
    shr  edx, 3              ; Equal to dividing by 8
    jz   SetFinalBytesLoop   ; Jump if counter is less than 8

Easier calculation of leftovers

    mov  r9d, edx
    ...
    shl  r9d, 3
    sub  r8d, r9d
    je   Finished
SetFinalBytesLoop:

The way you find out about the number of left over bytes is too complicated. It's correct but needlessly involved. Basically all it takes is anding the original counter with 7 to extract the lowest 3 bits. Simpler, shorter, and using one register less which in future programs will always be handy:

    and  r8d, 7
    jz   Finished
SetFinalBytesLoop:

Smaller instructions are generally better

With the 32-bit immediate value, the mov instruction in the MainLoop is quite long (7 bytes). You can store the zero in RAX and move that to memory. This also eliminates the need for the mention "qword ptr ":

    xor  rax, rax            ; Equivalent to MOV EAX, 0
MainLoop:
    mov  [rcx], rax          ; Set the next 8 bytes (qword) to 0
    add  rcx, 8              ; Move pointer along the array by 8 bytes
    dec  edx                 ; Decrement the counter
    jnz  MainLoop            ; If counter is not equal to 0 jump to MainLoop

Your program with all the above applied

    xor  rax, rax
    test edx, edx
    jle  Finished            ; Check if count is LE 0
    mov  r8d, edx            ; Copy of the original count
    shr  edx, 3              ; Gives number of qwords
    jz   SetFinalBytesLoop   ; Jump if counter is less than 8
MainLoop:
    mov  [rcx], rax          ; RAX=0 Set the next 8 bytes (qword) to 0
    add  rcx, 8              ; Step per 8 bytes
    dec  edx                 ; Dec the counter
    jnz  MainLoop
    and  r8d, 7              ; Remainder from division by 8
    jz   Finished
SetFinalBytesLoop:
    mov  [rcx], al           ; AL=0 Sets the last bytes of the array to 0
    inc  rcx                 ; Step per 1 byte
    dec  r8d                 ; Dec counter
    jnz  SetFinalBytesLoop
Finished:
    ret

I've moved the xor rax, rax higher up in the code so SetFinalBytesLoop can benefit from using the register AL vs the immediate 0.

The optimization

The most important optimization that you can apply to your program is making sure that the qword value that you write is aligned on a qword boundary, so a memory address that is divisible by 8.
The extra alignment loop will at most iterate 7 times.

    xor  rax, rax
    test edx, edx
    jle  Finished            ; Check if count is LE 0

    jmp  TestAligned
AlignLoop:
    mov  [rcx], al
    inc  rcx
    dec  edx
    jz   Finished
TestAligned:
    test rcx, 7              ; Is this a qword aligned address?
    jnz  AlignLoop           ; Not yet!

    mov  r8d, edx            ; Copy of the (reduced) original count
    shr  edx, 3              ; Gives number of qwords
    jz   SetFinalBytesLoop   ; Jump if counter is less than 8
MainLoop:
    mov  [rcx], rax          ; RAX=0 Set the next 8 bytes (qword) to 0
    add  rcx, 8              ; Step per 8 bytes
    dec  edx                 ; Dec the counter
    jnz  MainLoop
    and  r8d, 7              ; Remainder from division by 8
    jz   Finished
SetFinalBytesLoop:
    mov  [rcx], al           ; AL=0 Sets the last bytes of the array to 0
    inc  rcx                 ; Step per 1 byte
    dec  r8d                 ; Dec counter
    jnz  SetFinalBytesLoop
Finished:
    ret


Fetching data from your database in a constructor I have this item (an item is an object linked to a xml view), to build it I need 2 things, get labels from codes, and get text from the strings.xml android file.

please ignore the long to int and int to long convertion, for some reason the database value where randomly set to long and int but they could all be int,
I will refactor this later.

refManager is calling a DAOto fetch data from the database.

I have a for loop, building several OperationInstallItem

//constructor

public OperationInstallItem(final Resources resources, final RefManager refManager,
            final ModuleCategoryEnum category, final ModeleOperationEntity modeleOperationEntity,
            boolean isMandatory) {
        //basic init
        this.category = category
        this.isMandatory = isMandatory;
        this.refTypeOperation = modeleOperationEntity.getRefTypeOperation();
        this.refTypeInstallation = Long.valueOf(modeleOperationEntity.getRefTypeInstallation());

        //label init
        this.operationLabel = refManager.getOperationLabel(Ints.checkedCast(refTypeOperation));
        this.installationShortLabel = parametrageManager.getInstallationShortLabelt(refTypeInstallation);
        this.installationLongLabel = parametrageManager.getInstallationLongLabel(refTypeInstallation);
        this.title = resources.getString(R.string.operation_intallation_title)

        //icons specific to this class
        CodeTypeOperation codeTypeOperation =
                CodeTypeOperation.getEnumByCode(Ints.checkedCast(refTypeOperation));
        addModuleAdapterIconId = codeTypeOperation.getIcon();
        moduleAdapterIconId = codeTypeOperation.getIcon();
        moduleAdapterIconselectedId = codeTypeOperation.getIconSelected();
        moduleFragmentClass = TDBAddModuleFragment.class;
    }

Most of my collueages are uneasy with passing a manager as a constructor arguments but noone of
them can explain me why. "it's just not done this way"

same thing with Resources resources that allow you to get String values from the strings.xml file doing this resources.getString(R.string.operation_intallation_title),
according to them, it is bad to pass it in the constructor.

I don't get it, I need those labels/title, if I do this before the instanciation and pass it as parameters it's the exact
same thing. no performance gain, nothing is different.

Or is there something I'm missing?

First of all, it violates the "tell don't ask" principle: if your OptionInstallItem is not inherently coupled to the database and its actions (and as you do not even record the reference to the refmanager, it obviously isn't) it should not go about scavenging in your whole application to get its job done. Always give every class exactly what it needs to operate, not the "master key" for self service.

Then, your way makes it unnecessarily hard to test. If you have a simple object along the lines of

public MyClass(String myLabel) {
    this.myLabel = myLabel;
}

Testing is as simple as can be:

MyClass testObject = new MyClass("test");
Assert....

On the other hand, if you have

public MyClass(DatabaseInterfaceThingy database) {
    this.myLabel = database.doSomeQueryForMyLabel("me");
}

... say hello to mockito to even instantiate that object.

And send your colleagues over to Codereview to learn a few things and be able to tell you "why" the next time ... ;-)


Matrix with spiral increasing values This generates a square matrix with spiral inward increasing values. How can I reduce this code?

Input Number = 3

Output

1 2 3

8 9 4

7 6 5

import math
row=int(input("Input nomber := "))
lis=[[0 for i in range(0,row)]for j in range(0,row)]
y=row
a=1
c=0
z=0
x=1
k=1
f=row/2
f=math.ceil(f)
for b in range(0,f):
    if b==k:
        row=row-1
        x=x+1
        z=z+1
        k=k+1
    for c in range(z,row):
        lis[b][c]=a
        a=a+1
    for d in range(x,row):
        lis[d][row-1]=a
        a=a+1
    for e in range(row-1,z,-1):
        lis[row-1][e-1]=a
        a=a+1
    for f in range(row-2,z,-1):
        lis[f][z]=a
        a=a+1
for i in range(0,y):
    print()
    for j in range(0,y):
        print(lis[i][j],end="\t")

Covering what others suggested (e.g. @Srivaths), please follow at least PEP8 Python PEP8

Then, put your code in some function.
Give meaningful names to variables. (What's a, c, x, z ... why are you not using b? )

You don't need import math -- instead of f=row/2
f=math.ceil(f), you can do f = row // 2 (assuming you use python 3).

Note that you can solve the problem more generally, for m x n matrix, which you can initialize as:
matrix = [[0 for col in range(nCols)] for row in range(nRows)]
(see the answer from StackOverflow, provided by @OK).
This
 matrix = [[0] * m] * n, as pointed in the comments, won't work because of list references.

Now, we can also observe that you can fill the "outer rectangle", i.e. matrix[0][:] = range(1, nCols + 1); then, you can fill the rightmost column

cnt += nCols
for row in range(1, nRows):
  matrix[row][rightIdx] = cnt
  cnt += 1
# Bottom row:
matrix[bottomIdx][leftIdx:] = reversed(range(cnt, cnt + nCols - 1) # might be off by 1;
# Complete first column

# Put this in a while loop;

This problem is similar -- once you have the matrix, print it in a spiral order:
Geeks for geeks website.

Also, you can check the solution of LeetCode Prob 54, but I would encourage you to try solving the problem yourself first.
Problem 54 Solution for Problem 54

And here is my solution to Problem 54, similar to Solution 2 (Leetcode Solution link above): My solution :)


Comment on the style of this C++ class for a 2D Vector I wrote a template class for a 2D vector that is intended to provide 2D vector arithmetic to be used in a Physics simulation program. Note that x and y are public or ease of use since encapsulation here would just decrease the readability of code that uses the class.

Is the general style of my code good and does it comply with modern C++ best practices? Thank you for the suggestions.

template <typename T>
class Vector2D final {
public:
    T x;
    T y;

    // Defaulted constructors
    Vector2D(const Vector2D<T>& v) = default;
    ~Vector2D() = default;

    Vector2D(T ix, T iy);

    // Member Utility Functions
    void Set(T ix, T iy);
    T Magnitude();

    // Overloaded Compound Operators
    Vector2D<T>& operator+=(const Vector2D<T>& rhs);
    Vector2D<T>& operator*=(const Vector2D<T>& rhs);
    Vector2D<T>& operator-=(const Vector2D<T>& rhs);
    Vector2D<T>& operator/=(const T& rhs);
    Vector2D<T>& operator=(const  Vector2D<T>& rhs);
};

// Non-Compound Operators.
template <typename T> Vector2D<T> operator+(Vector2D<T> lhs, const Vector2D<T>& rhs);
template <typename T> Vector2D<T> operator*(Vector2D<T> lhs, const Vector2D<T>& rhs);
template <typename T> Vector2D<T> operator-(Vector2D<T> lhs, const Vector2D<T>& rhs);
template <typename T> Vector2D<T> operator/(Vector2D<T> lhs, const T& rhs);

template <typename T>
Vector2D<T>::Vector2D(T ix, T iy)
        : x(ix)
        , y(iy)
{
}

template <typename T>
void Vector2D<T>::Set(T ix, T iy) {
    x = ix;
    y = iy;
}

template <typename T>
T Vector2D<T>::Magnitude() {
    return x*x + y*y;
}

template <typename T>
Vector2D<T>& Vector2D<T>::operator+=(const Vector2D<T>& rhs) {
    x += rhs.x;
    y += rhs.y;
    return *this;
}

template <typename T>
Vector2D<T>& Vector2D<T>::operator*=(const Vector2D<T>& rhs) {
    x *= rhs.x;
    y *= rhs.y;
    return *this;
}

template <typename T>
Vector2D<T>& Vector2D<T>::operator-=(const Vector2D<T>& rhs) {
    x -= rhs.x;
    y -= rhs.y;
    return *this;
}

template <typename T>
Vector2D<T>& Vector2D<T>::operator/=(const T& rhs) {
    x /= rhs;
    y /= rhs;
    return *this;
}

template <typename T>
Vector2D<T>& Vector2D<T>::operator=(const Vector2D<T>& rhs) {
    x = rhs.x;
    y = rhs.y;
    return *this;
}

template <typename T>
Vector2D<T> operator+(Vector2D<T> lhs, const Vector2D<T>& rhs) {
    lhs += rhs;
    return lhs;
}

template <typename T>
Vector2D<T> operator*(Vector2D<T> lhs, const Vector2D<T>& rhs) {
    lhs *= rhs;
    return lhs;
}

template <typename T>
Vector2D<T> operator-(Vector2D<T> lhs, const Vector2D<T>& rhs) {
    lhs -= rhs;
    return lhs;
}

template <typename T>
Vector2D<T> operator/(Vector2D<T> lhs, const T& rhs) {
    lhs /= rhs;
    return lhs;
}

Some examples of using this with float. I intend to only use it with floating point types (float, double, long double etc) and have tested it with other types but will present float here for brevity.

Vector2D<float> v1 = {1.0, 2.0};
Vector2D<float> v2 = {2.0, 3.0};

v1 += v2;
assert(v1.x == 3.0 && v1.y == 5.0);
v1 -= v2;
assert(v1.x == 1.0 && v1.y == 2.0);
v1 *= v2;
assert(v1.x == 2.0 && v1.y == 6.0);
v1 /= 2.0;
assert(v1.x == 1.0 && v1.y == 3.0);

v1.Set(1.0, 2.0);
v2.Set(2.0, 3.0);
Vector2D<float> v3 = {0.0, 0.0};

v3 = v1 + v2;
assert(v3.x == 3.0 && v3.y == 5.0);
v3 = v1 - v2;
assert(v3.x == -1.0 && v3.y == -1.0);
v3 = v1 * v2;
assert(v3.x == 2.0 && v3.y == 6.0);
v3 = v1 / 2.0f;
assert(v3.x == 0.5 && v3.y == 1.0);

This looks pretty good! I don't think you need to change much, but there are some things missing that you will likely need:

normalize() or normal() method to either normalize a vector or return a normalized version of the vector. If you're doing physics, you'll need normalized vectors for lots of operations, probably.
Scalar multiply (operator*(const T)). You'll likely need to scale vectors frequently and will do that by multiplying by a scalar.
operator==() and/or operator!=(). You should define one in terms of the other.
Possibly a squared magnitude. Sometimes taking the square root in the Magnitude() function is a bottleneck and you don't need it (for example to see if a point is within a given radius of another point). It can be faster to get the squared magnitude of the vector between the two points if that's something you do a lot.


A type for prime numbers I have recently discovered that in type theory there is a concept of a "predicate type" which is a type \$A\$ formed out of all members of the underlying type \$U\$ that satisfy a given predicate function pred :: u -> Bool. I wondered if we can do that in Haskell, so I took this library for primes and decided to make a type for the prime numbers. This is how it looks:

module Data.Numbers.Primes.Type
    (Prime
    , getValue
    , getIndex
    , primeIndex
    , getPrime
    , maybePrime
    ) where

import Data.List (elemIndex)
import Data.Numbers.Primes

data Prime int = Prime { getValue :: int, getIndex :: Int } deriving Show

instance Integral int => Enum (Prime int) where
    toEnum = getPrime
    fromEnum = getIndex

instance Eq (Prime int) where
    x == y = getIndex x == getIndex y

instance Ord (Prime int) where
    x `compare` y = getIndex x `compare` getIndex y

-- | If a given number is prime, give its index.
primeIndex :: (Integral n, Integral i) => n -> Maybe i
primeIndex x | isPrime x = fromIntegral <$> elemIndex x primes
             | otherwise = Nothing

-- | Give n-th prime.
getPrime :: (Integral n, Integral int) => n -> Prime int
getPrime n = Prime (primes !! fromIntegral n) (fromIntegral n)

-- | If a given number is prime, give it back wrapped as such.
maybePrime :: (Integral n, Integral int) => n -> Maybe (Prime int)
maybePrime x | isPrime x = Prime (fromIntegral x) <$> primeIndex x
             | otherwise = Nothing

I'm certain this code is flawed in many ways yet unbeknownst to me, of which I hope the condescending reader would let me know so I could improve. However, there are a few points that I'm suspicious about even now:

Am I dealing with index type the right way?

Is it good to "cache" a prime's index inside the data object?

Am I doing it right?
Would it be possible and/or better to cache the indices implicitly via a memoizing function? On the pros side I see one less explicit data field which consistency could be compromised, on the contras side that we will not be able to easily save and restore data objects inbetween program runs.

Is an Int the right type for storing indices of primes, in the light of the facts that no prime has a negative index and there is a type Word for bounded unsigned integer values? (For some reason, Prelude makes no use of Word for, say, functions that deal with list indices, so, in suspicion that this design choice may have had a good motivation, I for now refrained from using the Word either.)

Maybe the index type should rather be polymorphic over the Integral class?

Is it good to use fromIntegral everywhere to make the types of the functions that deal with indices more general?

Is this type actually safe as it's intended to be? That is, can I be sure there is no way to construct a Prime data object containing an arbitrary value or an inconsistent index? For example, if I used a newtype (as I did in one of the drafts) and a derived Enum instance, I would be able to toEnum any number into prime. Does the design ensure this will not be the case ever anymore? (At least, in a reasonable usage scenario involving a non-malevolent user.)

Unfortunately, it's not safe. That's due to the records. If I know any Prime, I can construct a new Prime:

import Data.Numbers.Primes.Type

example :: Prime -> Prime
example p = p {getIndex = 0, getValue = 0} -- whoops

So you want to get rid of the records in your type and write the getters by hand:

data Prime int = Prime int Int

getValue :: Prime int -> int
getValue (Prime v _) = v

getIndex :: Prime int -> Int
getIndex (Prime _ idx) = idx

Alternatively rename them, rebind them and don't export the record selectors:

data Prime int = Prime { _getIndex :: Int, _getValue int }

getValue :: Prime n -> n
getValue = _getValue

getIndex :: Prime n -> Int
getIndex = _getIndex

Also, you can improve maybePrime:

maybePrime :: Integral n => n -> Maybe (Prime n)
maybePrime x = Prime x <$> primeIndex x

Either x is prime and primeIndex returns Just idx, or it isn't. There is no need to check x twice. maybePrime is usually called a "smart constructor", by the way.

  Am I dealing with index type the right way?

That depends on your use-case. If you need the index of the Prime often, it makes sense to cache it. If you don't need the index often, don't. You could provide an IndexedPrime that always contains the Index, though:

type Index = Int
data IndexedPrime int = IndexedPrime Index int

newtype Prime int     = Prime int

You could split those into separate modules, but that's a matter of personal preference.

  Is an Int the right type for storing indices of primes, in the light of the facts that no prime has a negative index and there is a type Word for bounded unsigned integer values?

That's a good question. The answer is: yes, Int is a right type, but it's not the right type. Most of the time, you want to use the result of getIndex in some operation that wants an Int, not a Word, just like you want to use take (a - b). Note that I said that Int is a correct type. You can of course use Word.

  Is it good to use fromIntegral everywhere to make the types of the functions that deal with indices more general?

It can lead to interesting behaviour. For example, I can use

exampleInput :: Int
exampleInput = 13^9 + 33 + 13 -- yes, that's a prime. found by lucky guess :)

exampleOutput :: Maybe (Prime Word8)
exampleOutput = maybePrime exampleInput

-- exampleOutput = Just (Prime {getValue = 195, getIndex = 333})

That's why I changed maybePrime's type, to ensure that the conversion has to be explicit by the user before they apply maybePrime.

Overall, good ideas and implementation, but try to keep the fromIntegral parts in your own code to a minimum. By the way, if you're interested in predicative types, have a look at Liquid Haskell.


Component state delegation with React I'm currently reviewing some different ways to have reusable components enforcing both most of the style & logic. The use case is a generic MainList with children components (eg. MainListToolbar, MainListContent, etc.) that can all be used together to create multiple mostly agnostic lists for business objects (eg. UserList, DeviceList, etc.).

For instance, I want to abstract away a multi-selectable logic and design in MainListContent while still being able to access the related internal state in the parent UserList (eg. this.state.selectedItems). Ideally, the multi-selectable logic (eg. onListItemClick, onListItemDoubleClick, etc.) should not leak at all into UserList.

I first thought about extending some MainListComponent (instead of Component), but I've never seen it in the React world and looks like a bad pattern to me.

I got something working with a state delegation pattern that enables to have an UserList that implements generic children elements and inherits the abstracted logic (eg. listItem selection) in it's own state to use it any way it wants.

Is there something flawed in this pattern/code? Is there a more elegant/safe way to achieve that?

const {Component} = React;

class MainList extends Component {
 render() {
    const {children} = this.props;
   return (

{children}

    );
  }
}

class MainListContent extends Component {
  constructor(props) {
   super(props);
    this.state = {
     selectedItems: []
    };
   this.handleListItemClick = (ev) => {
     const {selectedItems} = this.state;
     const value = ev.target.innerHTML;
      this.setState({
       selectedItems: !selectedItems.includes(value) ? selectedItems.concat(value) : selectedItems.filter(item => item !== value)
      });
    }
  }
  setState(nextState) {
    super.setState(nextState);
    const {onSetState} = this.props;
    if (onSetState) {
      onSetState(nextState);
    }
  }
  render() {
    const {title, items} = this.props;
    const {selectedItems} = this.state;
   return (

{title}

          {items.map((item, index) => (
{item}
))}

    );
  }
}

class UserList extends Component {
  constructor(props) {
   super(props);
    this.state = {};
    this.delegateSetState = this.setState.bind(this);
  }
  render() {
    const {items} = this.props;
    const {selectedItems} = this.state;
   return (
      <MainList>
        <MainListContent ref={ref => { this.mainListContent = ref; }} onSetState={this.delegateSetState} title="Users" items={items} />

count: {selectedItems ? selectedItems.length : 0}

      </MainList>
    )
  }
}

const Root = props => (
  <UserList items={['foo', 'bar', 'baz']}></UserList>
);

ReactDOM.render(
  <Root />,
  document.getElementById('root')
);

One prevailing pattern is stateless components. You will most likely have components that require state, but it is easier to follow the flow of data (and write tests) if your components rely solely on props. The stateless pattern is somewhat similar to yours (in that they are both mutating state in the parent), but instead of proxying calls to setState on the top level component AND having to re-implement setState in each component, child components just call the methods passed from the top level to mutate the state and then receive the new data in the form of props.

From React Docs:

  In an ideal world, most of your components would be stateless
  functions because in the future we’ll also be able to make performance
  optimizations specific to these components by avoiding unnecessary
  checks and memory allocations. This is the recommended pattern, when
  possible.

Here is your example using stateless components (except for the top level component):

const { Component } = React;

const MainList = (props) => {
   const {children} = props;
   return (

{children}

   );
}

const MainListContent = (props) => {
   const {title, items} = props;
   return (

{title}

{
            items.map((item, index) => (

 props.onSelectItem(item.name)}
                   style={{cursor: 'pointer', fontWeight: item.selected ? 'bold' : 'normal'}}
               >{item.name}

              ))
        }

   );
}

class UserList extends Component {
  constructor(props) {
    super(props);
    this.state = {
        items: props.items.map(item => ({
            selected: false,
            name: item
        }))
    }
  }
  onSelectItem(itemName) {
    let { items } = this.state;
    this.setState({
        items: items.map(item => {
            if (item.name === itemName) {
                item.selected = !item.selected;
            }
            return item;
        })
    });
  }
  render() {
    const { items } = this.state;
   return (
      <MainList>
        <MainListContent ref={ref => { this.mainListContent = ref; }} onSelectItem={this.onSelectItem.bind(this)} title="Users" items={items} />

count: {items.filter(item => item.selected).length}

      </MainList>
    )
  }
}

const Root = props => (
  <UserList items={['foo', 'bar', 'baz']}></UserList>
);

ReactDOM.render(
  <Root />,
  document.getElementById('root')
);


Avoid --compiler-options in makefile using nvcc I am working with CUDA and I am currently using this makefile to automate my build process. However, there are multiple --compiler-options, basically one for each flag I want to pass to gcc.

How can I avoid repeating it over and over?

(Any suggestion about improving my makefile is also really welcome)

CC=nvcc
LIBS=-lm
CFLAGS=-Wno-deprecated-gpu-targets --compiler-options -Wextra --compiler-options -Wall --compiler-options -O3 --compiler-options -Wno-unused-result --compiler-options -Wno-unused-parameter

all:    main

main:   main.o
    $(CC) $(CFLAGS) $(LIBS) main.o -o main

main.o: main.cu
    $(CC) $(CFLAGS) $(LIBS) -c main.cu -o main.o

clean:
    rm -f *.o main

As mentioned in the comments, define two macros:

COMPILER_OPTIONS := -Wextra -Wall -O3 -Wno-unused-result -Wno-unused-parameters
CUDA_COMPILER_OPTIONS := $(foreach option, $(COMPILER_OPTIONS), --compiler-option $(option))

C in CFLAGS stands for compiler, and should contain only the compile phase options. Don't send CFLAGS to the linker, it is surely not interested in, say, -Wall. Conventionally the link phase options are passed in LDFLAGS.
I am surprised that

    $(CC) $(CFLAGS) $(LIBS) main.o -o main

did pick the math library. The linker processes the command line once and sequentially. As written, there is no undefined symbols lm could resolve by the time it is processed. The bulletproof way is to specify libraries after objects:

    $(CC) $(CFLAGS) main.o -o main $(LIBS)

Nitpick. CC is conventionally assumed to produce .o from .c. Since your source is .cu I recommend to alias nvcc to something else. NVCC perhaps.


Manipulating arrays to extract unique objects and count occurrences The data describes cycling activities (key1) and chunks of them (key2). In total there are around 1,000 key1 objects and arrays under key1 are 1-100 long.

Is there a simpler, more semantic, faster, or otherwise better way than to use two nested map in step 1?
I realise that step 3 is longer than it should be and there has to be a better way to do it. How can step 3 be improved?

// Original array
const arr = [
    {key1: [{key2: {id: 1, name: 'a'}}]},
    {key1: [{key2: {id: 2, name: 'b'}}]},
    {key1: [{key2: {id: 2, name: 'b'}}, {key2: {id: 3, name: 'c'}}]}
];
console.log(arr);

// Step 1: Extract meaningful information from original array
const arrOfArrOfObj = arr
    .map(category => category.key1
        .map(subCategory => (
                {
                    id: subCategory.key2.id,
                    name: subCategory.key2.name
                }
            )
        )
    );
console.log(arrOfArrOfObj);

// Step 2: Make the array one dimensional
const arrOfObj = [].concat(...arrOfArrOfObj);
console.log(arrOfObj);

// Step 3: Remove duplicates and count object occurrences.
let dedupedArrWithCount = [];
l = arrOfObj.length;
for (let i = 0; i < l; i++) {
    let objExists = false;
    for (let j = 0; j < dedupedArrWithCount.length; j++) {
        // Two objects are identical if their ids are identical.
        if (arrOfObj[i].id === dedupedArrWithCount[j].id) {
            objExists = true;
            dedupedArrWithCount[j].count += 1;
        }
    }
    if (!objExists) {
        dedupedArrWithCount.push({
            id: arrOfObj[i].id,
            name: arrOfObj[i].name,
            count: 1
        })
    }
}

console.log(dedupedArrWithCount);

Responding to your questions

  “Is there a simpler, more semantic, faster, or otherwise better way than to use two nested map in step 1?”

While I like the benefits of functional approaches, they are often slower because of the extra function calls.

Steps 1 and 2 could be simplified by using two for...of loops (since ecmascript-6 features like const are also used). Also, instead of constructing a new object to return in the nested map callback, you can just return subCategory.key2 (though perhaps you simplified the data and the original data contains more properties that aren't needed in the end).

const arrOfObj = [];
for (let category of arr) {
    for (let subCategory of category.key1) {
        arrOfObj.push(subCategory.key2);
    }
}

This generally works faster, at least for the small dataset supplied - see this jsPerf test for comparison

  “I realise that step 3 is longer than it should be and there has to be a better way to do it. How can step 3 be improved?”

I was thinking of using JSON.stringify() to make an array of counts in step 1 and then use JSON.parse() in step 3 for lookups in constant time, but apparently that was slower, possibly because of the original data set which only has 1 collision. Maybe for a larger dataset that would be faster. See this jsPerf for a comparison.

const arr = [
    {key1: [{key2: {id: 1, name: 'a'}}]},
    {key1: [{key2: {id: 2, name: 'b'}}]},
    {key1: [{key2: {id: 2, name: 'b'}}, {key2: {id: 3, name: 'c'}}]}
];
const counts = {};
for (let category of arr) {
  for (let subCategory of category.key1) {
    const countKey = JSON.stringify(subCategory.key2)
    counts[countKey] = (counts[countKey] || 0) + 1;
  }
}
const dedupedArrWithCount = [];
for (let key in counts) {
  const obj = JSON.parse(key);
  dedupedArrWithCount.push(Object.assign(obj, {
    count: counts[key]
  }));
}
console.log(dedupedArrWithCount);

Other feedback

The variable l is declared without any keyword:

let dedupedArrWithCount = [];
l = arrOfObj.length;

Unless this code is wrapped in an IIFE, l becomes a global variable, which can lead to unintentional consequences if that name is used later. Read more about this topic in this post about global variables.  It is advisable to use const (or let if there was a need to re-assign it).

In that same vein, dedupedArrWithCount could be declared with the const keyword, since it is never re-assigned, just mutated using the push method.

One other thing I noticed is that

const arrOfObj = [].concat(...arrOfArrOfObj);

Is equivalent to

const flatArr = arrOfArrOfObj.flat();

Using .flat() appears to run faster in Firefox and Safari but not Chrome, at least when I ran this jsPerf.


Optimizing Miller-Rabin Primality Test in Python I'm writing a Miller-Rabin primality test in Python. While I've briefly looked at how others approached the problem, I decided to try solving the problem on my own. Aside from my choice of language, how could I go about optimizing this code? Any suggestions are welcome and don't hesitate to be brutally honest.

def miller_rabin_primality_test(n):

    def mr_check_one(n):
        m = n - 1
        n = n - 1
        k =  1

        while n % 2**k == 0:
            m = n / 2**k
            k = k + 1

        return(m)

    def mr_check_two(n, m, a = [2, 3]):

        for i in range(0, len(a)):
            a = a[i]
            b = pow(a, m, n)
            i = 0

            if(b == n - 1 or b == 1):
                return True

            while(b != n - 1 and i < 7):
                b = pow(b, 2, n)
                i = i + 1

            if(b != n - 1):
                return False
            else:
                return True

    m =  mr_check_one(n)
    r = mr_check_two(n, m)

    return(r)

One obvious change to make is that mr_check_one(n) does a much more expensive loop than necessary. Here is a much simpler and faster version.

def mr_check_one(n):
    m = n - 1

    n = n - 1
    k =  1
    while n % 2 == 0:
        k += 1
        n /= 2

    return(m / 2**k)

Also, your second function seems really broken. You allegedly loop over a, but in your first time through you redefine a, reset i and return before going through the loop more than once.


Calculating scores for predictions of football scores I am writing a program which calculates the scores for participants of a small "Football Score Prediction" game.

Rules are:

if the match result(win/loss/draw) is predicted correctly: 1 point
if the match score is predicted correctly(exact score): 3 points (and +1 point because 1st rule is automatically satisfied)

Score is calculated after every round of matches (10 matches in a round).

The program I've written is as follows:

import numpy as np
import re
players = {0:'Alex',1:'Charlton',2:'Vineet'}
results = np.array(range(40),dtype='a20').reshape(4,10)
eachPrediction = np.array(range(20),dtype='a2').reshape(2,10)
score = np.zeros(3)
correctScore=3
correctResult=1
allPredictions = []
done = False

def takeFixtures():
    filename='bplinput.txt'
    file = open(filename)
    text = file.readlines()
    fixtures = [line.strip() for line in text]
    file.close()
    i=0
    for eachMatch in fixtures:
        x=re.match("(.*) ([0-9]+) ?- ?([0-9]+) (.*)", eachMatch)
        results[0,i]=x.group(1)
        results[1,i]=x.group(2)
        results[2,i]=x.group(3)
        results[3,i]=x.group(4)
        i+=1

def takePredictions(noOfParticipants):
    for i in range(0,noOfParticipants):
        print("Enter predictions by "+players[i]+" in x-y format")
        for i in range(0,10):
            eachFixturePrediction = raw_input("Enter prediction for "+results[0,i]+" vs "+results[3,i]+": ")
            x=eachFixturePrediction.split('-')
            eachPrediction[0,i]=str(x[0])
            eachPrediction[1,i]=str(x[1])
        allPredictions.append(eachPrediction)

def scoreEngine():
    for i in range(0,len(players)):
        for j in range(0,10):
            resultH=int(results[1,j])
            resultA=int(results[2,j])
            result=resultH-resultA
            predictionH=int(allPredictions[i][0][j])
            predictionA=int(allPredictions[i][1][j])
            pResult = predictionH-predictionA
            if result == pResult or (result<0 and pResult<0) or (result>0 and pResult>0):
                score[i]+=correctResult
                if resultH==predictionH and resultA==predictionA:
                    score[i]+=correctScore

noOfParticipants=len(players)
takeFixtures()
takePredictions(noOfParticipants)
scoreEngine()
print("Scores are:")
print(score)
for player in players:
    print(players[player]+" has scored "+ str(score[player])+" points" )

The file used to take list of fixtures looks like this:

West Bromwich Albion 0-1 Tottenham Hotspur
Manchester City 2-2 Liverpool
Queens Park Rangers 0-0 Norwich City
Arsenal 1-0 Stoke City
Everton 3-3 Aston Villa
Newcastle United 3-2 Chelsea
Reading 2-1 Sunderland
West Ham United 1-0 Swansea City
Wigan Athletic 2-2 Southampton
Fulham 0-1 Manchester United

I want advice on how this program can be improved in any way (decreasing the code/ making it efficient).

Style

The first thing to do is to follow PEP 8: it's a style guide that says how you should indent your code, name your variables, and so on. For example, prefer results[0, i] = x.group(1) to results[0,i]=x.group(1).

Files

Opening files in Python should be done using the with idiom: your file is then guaranteed to be close correctly. takeFixtures now becomes:

def takeFixtures():
    with open('bplinput.txt') as file:
        for eachMath in file:
            x=re.match("(.*) ([0-9]+) ?- ?([0-9]+) (.*)", eachMatch.strip)
            results[0,i]=x.group(1)
            results[1,i]=x.group(2)
            results[2,i]=x.group(3)
            results[3,i]=x.group(4)
            i+=1

Data structures

I you didn't use numpy, you could have written results[i] = x.groups(), and then used zip(*results) to transpose the resulting matrix. More generally, as pointed out by Josay, you should use Python data structures, and only switch to numpy if you find out that this is where the inefficiency lies.

Formatting

The last thing that the other reviewers didn't mention is that concatenating strings is poor style, and format() should be preferred since it's more readable and more efficient:

for player in players:
    print("{} has scored {} points".format(players[player], score[player]))


Is there any way to stop the lag in this Gif opener? Recently, I made this gif opener using tkinter:
import tkinter as tk
from tkinter import filedialog as fd
import time
import threading

def getfile():
     file = fd.askopenfilename()
     window = tk.Toplevel()
     def run_gif():
          i = 0
          while True:
               try:
                    photo = tk.PhotoImage(file = file,format="gif -index" +' '+ str(i))
                    gif = tk.Label(window, image=photo)
                    gif.grid(row=0,column=0)
                    i += 1
                    time.sleep(0.0001)
               except:
                    i = 0
     gif_thread=threading.Thread(target=run_gif)
     gif_thread.start()
     window.mainloop()

def display():
     root = tk.Tk()
     root.title(".gif opener")
     root.geometry('50x100')
     root.configure(bg = "lightgreen")
     label = tk.Label(root, text = ".gif opener")
     label.pack()
     button = tk.Button(root, text = "File", command = getfile)
     button.place(x = "50", y= "50")
     root.mainloop()

display()

However, every time I open a gif, the animation is really laggy.
Is there any way to make the gifs flow smoother? Any help will be appreciated.

Your problem may be similar to this one:
https://stackoverflow.com/questions/50904093/gif-animation-in-tkinter-with-pill-flickering-on-every-other-frame
I tried your code on gif which has a 2nd disposal method.
Works, but loops over frames too quickly. GIFs with the 1st disposal method fail.
Also pay attention to the frame delay/frame rate. Different GIFs can have different values.
You can use third party libraries like Pillow to get file information.

from PIL import Image

f = Image.open("tenor.gif")
print(f.info) # duration in ms
print(f.disposal_method)
print(f.n_frames) # number of frames

Output example:
{'version': b'GIF89a', 'background': 140, 'transparency': 140, 'duration': 160, 'extension': (b'NETSCAPE2.0', 795), 'loop': 0}
2
12

You can use some custom label classes.
https://stackoverflow.com/a/43770948/8349697
Consider using the PyQt5 library. I tried some test gifs with QtWidgets, and it works much better than tkinter. PyQt can display animated gifs using QMovie.


Made a simple gui string encrypter, just to obfuscate plaintext I realize this won't stop people who know what they're doing, just a project I decided to undertake while I'm trying to learn Python and Tkinter. Would love some input!

import Tkinter
import base64

class crypt_mod(Tkinter.Tk):
    def __init__(self, parent):
        Tkinter.Tk.__init__(self, parent)
        self.parent = parent
        self.initialize()

    def initialize(self):
        self.grid()

        plain_label = Tkinter.Label(text='Plain Text:')
        plain_label.grid(column=0, row=0, sticky='E')

        self.ptxt = Tkinter.StringVar()
        self.plain_text = Tkinter.Entry(self, textvariable=self.ptxt, width=50)
        self.plain_text.grid(column=1, row=0, padx=5, pady=5)
        self.plain_text.bind("<Return>", self.encrypt)

        self.enc_button = Tkinter.Button(self, text='Encrypt', command=self.encrypt, relief='groove')
        self.enc_button.grid(column=2, row=0, padx=5, pady=5)

        self.dec_button = Tkinter.Button(self, text='Decrypt', command=self.decrypt, relief='groove')
        self.dec_button.grid(column=2, row=1, padx=5, pady=5)

        plain_label = Tkinter.Label(text='Base64:')
        plain_label.grid(column=0, row=1, sticky='E')

        self.etxt = Tkinter.StringVar()
        self.e_text = Tkinter.Entry(self, textvariable=self.etxt, width=50)
        self.e_text.grid(column=1, row=1, padx=5, pady=5)

        self.clear_button = Tkinter.Button(self, text='Clear Both', command=self.clear_both, relief='groove')
        self.clear_button.grid(column=1, row=2, padx=5, pady=5)

        self.htxt = Tkinter.StringVar()
        self.history = Tkinter.Text(width=50, height=10, wrap='word', spacing1=2, font=('courier',8), state='disabled')
        self.history.grid(column=0, row=3, columnspan=3, padx=5, pady=5)

        self.clear_history_button = Tkinter.Button(self, text='Clear History', command=self.clear_history, relief='groove')
        self.clear_history_button.grid(column=1, row=4, padx=5, pady=5)

    def encrypt(self):
        self.etxt.set(base64.b64encode(str(self.ptxt.get())))
        self.history.config(state='normal')
        self.history.insert(Tkinter.END, 'encrypt:\n' + base64.b64encode(str(self.ptxt.get())) + '\n')
        self.history.config(state='disabled')
        self.history.see(Tkinter.END)
        #print "encrypt click!"

    def decrypt(self):
        self.ptxt.set(base64.b64decode(str(self.etxt.get())))
        self.history.config(state='normal')
        self.history.insert(Tkinter.END, 'decrypt:\n' + base64.b64decode(str(self.etxt.get())) + '\n')
        self.history.config(state='disabled')
        self.history.see(Tkinter.END)
        #print "decrypt click!"

    def clear_both(self):
        self.etxt.set('')
        self.ptxt.set('')

    def clear_history(self):
        self.history.config(state='normal')
        self.history.delete(0.0, Tkinter.END)
        self.history.config(state='disabled')
        pass

if __name__ == "__main__":
    app = crypt_mod(None)
    app.title('Base64 Crypto')
    app.mainloop()

import Tkinter
import base64

class crypt_mod(Tkinter.Tk):

Do you know about PEP8? It's a style guide for Python that lots of Python developers follow. One of the things it says is that class names should use the CapWords convention, eg. CryptMod. Note that Tkinter doesn't necessary follow those rules, as it's a really old module.

    def __init__(self, parent):
        Tkinter.Tk.__init__(self, parent)
        self.parent = parent
        self.initialize()

self.parent seems to be unused.

    def initialize(self):
        self.grid()
            ....

Not much to say here: the usual GUI building code.

    def encrypt(self):
        self.etxt.set(base64.b64encode(str(self.ptxt.get())))
        self.history.config(state='normal')
        self.history.insert(Tkinter.END, 'encrypt:\n' + base64.b64encode(str(self.ptxt.get())) + '\n')
        self.history.config(state='disabled')
        self.history.see(Tkinter.END)
        #print "encrypt click!"

Avoid creating twice your encrypted text: your code would be clearer if you had a encrypted variable. It's not terribly important here, but could be in future projects where you will have more lines of code. Consider using format() (recommended in Python 3) instead of building your strings by concatenation: "decrypt:\n {} \n".format(encrypted). (Those comments also apply to decrypt.)


Installing Mods With Python (v 2.0) My original code is here. I made some changes to it based on your suggestions, but then I lost my flash drive and had to redo it. I have a suspicion that my second version was better than this one, so could you all help me out one more time?

import os, shutil, zipfile

def subtractname(path):
    newpath = str()
    path = path.split("\\")
    path.pop()
    for x in range(len(path) - 1):
        newpath += (path[x] + "\\")
    newpath += path[len(path) - 1]
    return newpath

def pathtoname(path):
    path = path.split("\\")
    newpath = path[len(path) - 1]
    newpath = newpath[:-4]
    return(newpath)

def makepath(path):
    try:
        os.makedirs(path)
    except FileExistsError:
        shutil.rmtree(path)
        os.makedirs(path)

def getscds(targetfolder):
    global scdlist
    filelist = os.listdir(targetfolder)
    scdlist = []
    for file in filelist:
        if '.scd' in file:
            scdlist.append(os.path.join(targetfolder, file))
    return scdlist

def unzip(file, target):
    modscd = zipfile.ZipFile(file)
    makepath(target)
    modscd.extractall(target)

def checkdirs(target):
    isfile = 0
    targetlist = os.listdir(target)
    for files in targetlist:
        if not os.path.isfile(files):
            isfile = isfile + 1
    if isfile == len(os.listdir(target)):
        return False
    else:
        return True

def install(folderlist):
    newpathlist = []
    for folder in folderlist:
        pathlist = getscds(folder)
        for path in pathlist:
           newpath = os.path.join(subtractname(path), ("f_" + pathtoname(path)))
           newpathlist.append(newpath)
           unzip(path, newpath)
           if checkdirs(newpath):
            shutil.copy(path, r"C:\Program Files (x86)\Steam\steamapps\common\supreme commander 2\gamedata")
            newpathlist.remove(newpath)
    return newpathlist

a = input()
b = [a]
install(install(install(b)))

A note: this code SHOULD actually work if you run it, but I haven't incorporated error handling yet.

I know that the last three lines can be condensed into one, but aside from that is there anything that could be improved here? I'm not really looking for naming conventions or comments, but I'm fine with it if you want to throw them in along with some more practical suggestions.

EDIT: I just tried running this on a friend's computer, and it may not work after all. Ideas?

I'll leave an in-depth analysis to the true Python experts, but here are just some quick notes:

Variable and function names: As far as I'm aware, variable and function names should have words/elements separated by underscores (e.g., check_dirs, is_file)

main() function: Instead of having your functionality on the first/default level of indentation, it's good form to wrap it in a def main() and call it via:

if __name__ == '__main__':
    main()

Return values: You can simply return a bool directly rather than having an if/else construction.  e.g.,

return isfile != len(os.listdir(target))

vs.

if isfile == len(os.listdir(target)):
    return False
else:
    return True

Use of global: In my experience, any time I have to use global some_var, it's indicative of bad design.  You should either be using local variables, function arguments, or the constant should be well-defined and not hidden by any local fields.