Max-Heap arraylist based priority queue written in Java I have a program that I'm writing that needs to use a priority queue as part of an algorithm. I specifically need to order (String, Integer) pairs for example (Bread, 3), (Beer, 5), (Eggs,2), etc.

I'd appreciate any comments on my code style and how I've written my class.

import javafx.util.Pair;
import java.util.ArrayList;
import java.util.Collections;

public class FPPriorityQueue implements PriorityQueue {

private ArrayList<Pair<String, Integer>> heapArray;

public FPPriorityQueue(){
    super();
    heapArray = new ArrayList<Pair<String, Integer>>();
}

public boolean empty(){
    return true;
}

private Integer getParentData(int index){
    int parentIndex = index/2;
    Pair<String, Integer> parentData = heapArray.get(parentIndex);
    return parentData.getValue();
}

private void swap(Integer old, Integer knew){
    Collections.swap(heapArray,old,knew);

}

private void shiftUp(int index){

    int parentIndex;

    if(index != 1){
        parentIndex = index/2;
        int childData = heapArray.get(index).getValue();
        int parentData = getParentData(index);

        if(parentData < childData){
            swap(parentIndex,index);
            shiftUp(parentIndex);
        }
    }
}

private void shiftDown(int index){

    if(heapArray.size()> (index*2)+1) {
        int leftChildValue = heapArray.get(index * 2).getValue();
        int rightChildValue = heapArray.get((index * 2) + 1).getValue();

        int indexValue = heapArray.get(index).getValue();

        if(indexValue < leftChildValue || indexValue < rightChildValue)
        {
            if(leftChildValue > rightChildValue){
                int leftChildIndex = index*2;
                swap(index,leftChildIndex);
                shiftDown(leftChildIndex);
            }
            else{
                int rightChildIndex = (index*2)+1;
                swap(index,rightChildIndex);
                shiftDown(rightChildIndex);
            }
        }
    }

}

public void insert(String key, Integer value){

    Pair<String, Integer> element = new Pair<String, Integer>(key,value);
    Pair<String, Integer> nullElement = new Pair<String, Integer>("NULL",0);
    //add the element to the last position in the list
    if(heapArray.isEmpty()){
        heapArray.add(0,nullElement);
        heapArray.add(1,element);
    }
    else{
        heapArray.add(heapArray.size(), element);
        shiftUp(heapArray.size()-1);
    }

}

public Pair<String, Integer> removeMax(){

    Pair<String, Integer> returnValue = heapArray.remove(1);
    Collections.swap(heapArray,heapArray.size()-1,1);
    shiftDown(1);

    return returnValue;

}
}

Don't override with the default behavior

    private ArrayList<Pair<String, Integer>> heapArray;

    public FPPriorityQueue(){
        super();
        heapArray = new ArrayList<Pair<String, Integer>>();
    }

This could be simply

    private List<Pair<String, Integer>> heap = new ArrayList<>();

You don't need a custom constructor.  This will declare and initialize it.

I prefer the name heap to heapArray.  It's simpler and more accurate.

In the latest Java, you don't have to specify Pair<String, Integer> twice.  It's smart enough to figure it out if you just say <>.

In general, it is preferable to use interfaces as types rather than implementations.  Among other reasons, it allows you to change implementations easily.

Huh?

public boolean empty(){
    return true;
}

What's this do?  If it was called isEmpty, I'd think it was returning whether or not the heap was empty.  As is, I would expect empty to do something, perhaps clear the heap.

Whatever it's supposed to do, it doesn't seem to be doing it.

Don't rely on your inputs

private void shiftUp(int index){

    int parentIndex;

    if(index != 1){
        parentIndex = index/2;
        int childData = heapArray.get(index).getValue();
        int parentData = getParentData(index);

        if(parentData < childData){
            swap(parentIndex,index);
            shiftUp(parentIndex);
        }
    }
}

What if index is less than 1?

private void shiftUp(int index) {
    if (index > 1) {
        int parentIndex = index / 2;
        int parentData = heap.get(parentIndex).getValue();
        int childData = heap.get(index).getValue();

        if (parentData < childData) {
            swap(parentIndex, index);
            shiftUp(parentIndex);
        }
    }
}

Changing != to > handles index values less than 1.  And it's free.  We're already doing a comparison.  Why not do the better one?

We don't need getParentData.  We have to calculate parentIndex anyway, so we can just fetch directly.

I added some extra whitespace, because I find code easier to read that way.

Don't do unnecessary work

public void insert(String key, Integer value){

    Pair<String, Integer> element = new Pair<String, Integer>(key,value);
    Pair<String, Integer> nullElement = new Pair<String, Integer>("NULL",0);
    //add the element to the last position in the list
    if(heapArray.isEmpty()){
        heapArray.add(0,nullElement);
        heapArray.add(1,element);
    }
    else{
        heapArray.add(heapArray.size(), element);
        shiftUp(heapArray.size()-1);
    }

}

We don't need nullElement every time.  Consider

public void insert(String key, Integer value) {
    Pair<String, Integer> element = new Pair<>(key,value);

    //add the element to the last position in the list
    if (heapArray.isEmpty()) {
        heap.add(new Pair<String, Integer>("NULL", 0));
        heap.add(element);
    } else {
        heap.add(element);
        shiftUp(heap.size()-1);
    }
}

We don't have to explicitly say that we want to put things in the last position.  That's how the single argument add works already.

I personally am not crazy about the half-cuddled else {, and it's not the Java standard.  So I fully cuddled:  } else {.

We only create nullElement in the one edge case now.  The rest of the time, we don't bother.  But we can actually do better.  Consider

public FPPriorityQueue() {
    heap.add(new Pair<String, Integer>("NULL", 0));
}

This will create the null element the one time you need it, at the beginning.  And this is the kind of thing that you do in a constructor.

You don't need the explicit super().  Java's smart enough to do that for you when you're just calling the default constructor.

Now we can just say

    public void insert(String key, Integer value) {
       //add the element to the last position in the list
        heap.add(new Pair<String, Integer>(key, value));
        shiftUp(heap.size() - 1);
    }

It adds to the end of the list.

And because we previously changed shiftUp to handle the empty case, we don't need to prevent calling shiftUp in that case.

I'm not sure that we need the null element.  The math is a little more complex without it but still doable.

Don't Repeat Yourself

private void shiftDown(int index){

    if(heapArray.size()> (index*2)+1) {
        int leftChildValue = heapArray.get(index * 2).getValue();
        int rightChildValue = heapArray.get((index * 2) + 1).getValue();

        int indexValue = heapArray.get(index).getValue();

        if(indexValue < leftChildValue || indexValue < rightChildValue)
        {
            if(leftChildValue > rightChildValue){
                int leftChildIndex = index*2;
                swap(index,leftChildIndex);
                shiftDown(leftChildIndex);
            }
            else{
                int rightChildIndex = (index*2)+1;
                swap(index,rightChildIndex);
                shiftDown(rightChildIndex);
            }
        }
    }

}

Consider

private void shiftDown(int index) {
    int left = index * 2;
    int right = left + 1;
    if (heap.size() > right) {
        int leftChildValue = heap.get(left).getValue();
        int rightChildValue = heap.get(right).getValue();

        int indexValue = heap.get(index).getValue();

        if (indexValue < leftChildValue || indexValue < rightChildValue) {
            if (leftChildValue > rightChildValue) {
                swap(index, left);
                shiftDown(left);
            } else {
                swap(index, right);
                shiftDown(right);
            }
        }
    }
}

This way we set left and right before using them.  So we can't accidentally pick the wrong value one place.

Don't reinvent the wheel

If this is an exercise to improve your understanding of how a heap works, that's fine.  There's a reinvent-the-wheel tag that you can use to let us know.  But if you just needed to solve the problem, an easier way is to use a PriorityQueue with a custom Comparator or a custom type that extends Comparable.  Then Java will handle all the fiddly bits of managing the heap.

public class PairComparator implements Comparator<Pair<String, Integer>> {

    public int compare(Pair<String, Integer> a, Pair<String, Integer> b) {
        return b.getValue().compareTo(a.getValue());
    }

}

If the order is backwards, switch a and b in the return line.

I forget if you have to implement equals as well.

You'd use this like

        PriorityQueue<Pair<String, Integer>> queue = new PriorityQueue<>(new PairComparator());

Then you can call queue.add and queue.poll to insert and remove items.

You don't provide test code, so I haven't tried to compile and test any of this.  Beware of typos, etc.


Reorder a list in Python to avoid repeats I am trying to check if a list has any consecutive repeating elements and then reorder it such that the repeats are avoided. If that is impossible, then return False. For example:

checkRepeat([1,2])
Out[61]: [1, 2]

checkRepeat([1,2,2])
Out[62]: [2, 1, 2]

checkRepeat([1,2,2,1,1])
Out[63]: [1, 2, 1, 2, 1]

checkRepeat([1,2,2,1,1,3,3,3,3])
Out[64]: [1, 3, 1, 3, 2, 1, 3, 2, 3]

checkRepeat([1,2,2,1,1,3,3,3,3,3])
Out[65]: [3, 1, 3, 2, 3, 1, 3, 1, 3, 2]

checkRepeat([1,2,2,1,1,3,3,3,3,3,3])
Out[66]: [3, 1, 3, 1, 3, 1, 3, 2, 3, 2, 3]

checkRepeat([1,2,2,1,1,3,3,3,3,3,3,3])
Out[67]: False

Here is what I have. Is there a more elegant solution?

from itertools import groupby
from collections import Counter

def checkRepeat(lst):
    """Returns a list that has no repeating elements. Return False if such a list can't be found"""

    def hasRepeat(lst):
        """Returns true if there are any repeats"""
        return len([x[0] for x in groupby(lst)]) < len(lst)

    offset=numIter=0
    if Counter(lst).most_common()[0][1]*2 > len(lst)+1: #checks if it will be impossible to find a working order
        return False

    while hasRepeat(lst):
        for i,curElt in enumerate(lst):
            try:
                if lst[i]==lst[i+1]:
                    lst[i+1],lst[(i+offset) % len(lst)] = lst[(i+offset) % len(lst)],lst[i+1] #swap j+1 with j+offset. wrap around the list
            except:
                break
        offset+=1
        numIter+=1
    return lst

Ideally a function will return a particular type of value. Python may be dynamic and allow you to return anything, but it's less surprising behaviour if you don't return two types of value. Instead, consider returning None or an empty list. Both of which will still evaluate as False when tested because of Python's truthiness. I'd personally suggest None, as an empty list might be mistaken for a useable list. Python will automatically use None as the return value in the absence of any other argument, so just using return returns None.

You should use snake_case for naming variables and functions, so checkRepeat should be check_repeat. I also don't like lst as a name. It's good that you're avoiding shadowing the built in list, but lst is unclear and can look like 1st. Perhaps iterable or source could be better names.

hasRepeat doesn't need to be a nested function, you could have it separated as it's own. Having it nested implies that you're trying to do a more complicated structure when really it's just because you call hasRepeat in checkRepeat. Also, checkRepeat isn't a good name as it's actually removing repeating values (if it can). Instead perhaps something like reorder_repeats? It would not be the clearest name because it's not an easy process to describe, but the docstring can provide help there too.

You should have spaces either side of your mathermatical operators, it makes them more readable and is recommended by the style guide. So this:

offset=numIter=0

should be this:

offset = numIter = 0

Also try to avoid inline comments, they make lines exceedingly long. They should be used sparingly for short comments. If your comment is a sentence it will almost always need its own line. The style guide also says to keep below 79 characters per line, which this comment invalidates:

if Counter(lst).most_common()[0][1]*2 > len(lst)+1: #checks if it will be impossible to find a working order

Split it onto the line above and it's much more readable.

# Check if it will be impossible to find a working order
if Counter(lst).most_common()[0][1] * 2 > len(lst) + 1:

Instead of curElt try the name element or item. Shortened words often make for odd names that people can easily interpret differently to you. Except that you don't end up using this value at all. Instead you access elements by their index, which I personally think is more readable in this case and better, but then you could just loop with this:

    for i in range(len(source)):

What's the point of numIter? It's the exact same value of offset but never even used? If it was a testing value you should remove it now. While we're on it, I don't entirely understand offset, a comment about it would be helpful when you're incrementing the values.

Don't use a bare except. It's very bad practice because you might ignore all sorts of errors that you should really be paying attention to. In this case, it seems like IndexError is what you're expectin to happen, so use that specifically.

The line where you swap values is too long and hard to read, instead you should calculate the index of your second value on the line before:

            if lst[i] == lst[i+1]:
                index = (i + offset) % len(lst)  # Wrap around the list
                lst[i+1], lst[index] = lst[index], lst[i+1]

Now that it's clear that index is the same value on either side, it's obvious that you're swapping values. You can make this comment inline since it's a brief note about the meaning of an unusual calculation.

Here's how I'd put the script together:

from itertools import groupby
from collections import Counter

def has_repeat(source):
    """Returns true if there are any repeats"""

    return len([x[0] for x in groupby(source)]) < len(source)

def check_repeat(source):
    """Returns a list that has no repeating elements or None if such a list can't be found"""

    # Check if it will be impossible to find a working order
    if Counter(source).most_common()[0][1] * 2 > len(source) + 1:
        return

    offset = 0
    while has_repeat(source):
        for i in range(len(source)):
            try:
                if source[i] == source[i+1]:
                    index = (i + offset) % len(source)  # Wrap around the list
                    source[i+1], source[index] = source[index], source[i+1]
            except IndexError:
                break
        offset += 1
    return source


Sampling with weighted probabilities In an exam question I need to output some numbers self.random_nums with a certain probability self.probabilities:

I have written the following program that successfully returns the correct answer and also a test at the bottom which confirms that everything is working well.

Here my questions:

Are there any better ways that this problem could be solved? Does my
program go against certain conventions that I should regard?
Why is the unittest ignoring the rounding?
Are there any style issues in my program?

Any other comments are appreciated

from __future__ import division  # in case of python 2
import numpy as np
from matplotlib import pyplot as plot

class RandomGen(object):
    def __init__(self):
        self.random_nums = np.array([-1, 0, 1, 2, 3])
        self.probabilities = np.array([0.01, 0.3, 0.58, 0.1, 0.01])

        # generate cumulative probabilities and add a 0 to the beginning and remove the 1 at the end
        self.cum_probabiliies = np.cumsum(self.probabilities)
        self.cum_probabiliies = np.delete(self.cum_probabiliies, self.cum_probabiliies.size - 1)
        self.cum_probabiliies = np.append([0], self.cum_probabiliies)

    def next_num(self, i):
        num = 0
        while num < i:
            rnd = np.random.uniform(low=0.00, high=1)  # between 0 and 0.99 (less than the high)
            x = (self.cum_probabiliies[self.cum_probabiliies <= rnd]).size - 1
            number = self.random_nums[x]
            yield number
            num += 1

    def get_expected_probabilities(self):
        print("Expected probabilities:")
        target = dict(zip(self.random_nums, self.probabilities))
        target = {k: round(v, 2) for k, v in target.items()}
        print(target)
        return target

    def get_actual_probabilities(self, n):
        # create histogram with corresponding buckets
        hist = np.histogram(n, bins=np.append(self.random_nums, max(self.random_nums + 1)))

        s = sum(hist[0])
        print("Actual Probabilities:")
        effective_probabilities = hist[0] / s
        actual = dict(zip(hist[1], effective_probabilities))
        actual = {k: round(v, 2) for k, v in actual.items()}
        print(actual)
        return actual

if __name__ == "__main__":
    r = RandomGen()
    n = list(r.next_num(10000))
    print("Output:")
    print(n)
    print()
    r.get_expected_probabilities()
    print()
    r.get_actual_probabilities(n)

    plot.hist(n)
    plot.show()

As test I suggest the following:

import unittest
import man_exercise

class TestRandomGen(unittest.TestCase):
    def test_next_num(self):
        iterations = 10000
        delta = iterations / 50000

        r = man_exercise.RandomGen()
        n = list(r.next_num(iterations))

        target = r.get_expected_probabilities() # dictionary with numbers and their expected probabilities
        effective = r.get_actual_probabilities(n) # dictionary with numbers and their actual probabilities

        for k,_ in target.items():
            self.assertAlmostEqual(target[k], effective[k], delta=delta)

Allow native iteration

It is reasonable to expect to be able to iterate over a RandomGen[erator], but the built-in for loop fails right now.

All you need to allow looping with a standard for loop is renaming next_num to __iter__

Use native iteration

    num = 0
    while num < i:
        # ... snip ...
        num += 1

Is not nice, you are manually incrementing a counter in a high level language. Just use (x)range (depending on the Python version):

for num in range(i):
    # ... snip ...


Moving character in Unity3D based on accelerometer input I am building a infinite vertical platformer for mobile platforms using Unity3D. I am using the accelerometer to move the character left and right on the screen.

if (Input.acceleration.y > -0.2f && Input.acceleration.y < 0.2f) {
                        maxSpeed = 17;
                }

        if (Input.acceleration.y > -0.5f && Input.acceleration.y < -0.2f) {
            maxSpeed = 25;
        }
        if (Input.acceleration.y > 0.2f && Input.acceleration.y < 0.5f) {
            maxSpeed = 25;
        }
        if (Input.acceleration.y > -0.7f && Input.acceleration.y < -0.5f) {
            maxSpeed = 25;
        }
        if (Input.acceleration.y > 0.5f && Input.acceleration.y < 0.9f) {
            maxSpeed = 25;
        }
        if (Input.acceleration.y < -0.9f) {
            maxSpeed = 40;
        }
        if (Input.acceleration.y  > 0.9f) {
            maxSpeed = 40;
        }

Basically the farther the device is tilted the faster the character moves. I'm pretty sure there is a way better way to write this code but I'm pretty terrible at math and I can't figure it out. Any suggestions on cleaning this up a bit?

I know absolutely nothing about c# or unity3d, so if there's a language specific assist, I have no idea about it!

I notice each condition checks Input.acceleration.y, so that's one bit of information that repeats. I also notice we have three speeds, yet we have seven conditionals!

First of all, the negatives are confusing. Find the absolute value first.

I'm going to assume a number followed by an "f" means float. So maybe we need something like:

float acceleration = Math.Abs(Input.acceleration.y);

Great, now it's going to be easier to handle this!

Now all we need is a simple check like so:

if (acceleration < 0.2f) {
    maxSpeed = 17;
} else if (acceleration < 0.9f) {
    maxSpeed = 25;
} else {
    maxSpeed = 40;
}

Your original numbers were a little weird, but if you meant for such a strange pattern, correct me!


Credit card validator in Python I have began taking the CS50 course on EDX the past couple of days, and one of the tasks to accomplish was to write a credit card validator in C. I wrote it in C, and then I thought that I could go about the same thing in Python, and wrote the same program in Python. For those of you unfamiliar with the problem, here is the description.

Because I took the program and converted from C to Python, it is probably not going to be written in the most Pythonic way. I tried to add some Pythonic ways of doing things, but there are probably many more things I could have done. If you find anything that could be done in a better, faster, or more Pythonic way, please let me know. Thanks.

def main():
    # cc_number = int(input("Enter a valid credit card number: "))

    cc_number = 12345678912356789
    if not checksum(cc_number):
        print("INVALID")
    else:
        print(check_provider(cc_number))

def check_provider(number):
    if len(str(number)) < 13 or len(str(number)) > 16:
        return 'INVALID'

    if ((list(str(number)))[:2]) == ['3', '4'] or ((list(str(number)))[:2]) == ['3', '7']:
        return 'AMEX'
    elif ((list(str(number)))[:2]) == ['5', '1'] or ((list(str(number)))[:2]) == ['5', '2'] \
            or ((list(str(number)))[:2]) == ['5', '3'] or ((list(str(number)))[:2]) == ['5', '4'] \
            or ((list(str(number)))[:2]) == ['5', '5']:
        return 'MASTERCARD'
    elif ((list(str(number)))[0]) == '4':
        return 'VISA'
    else:
        return 'INVALID'

def checksum(number):
    list_number = list(str(number))
    odd_indices = []
    even_indices = []

    last_index = len(list_number) - 1

    rev_list = list(reversed(list_number))

    for i in rev_list[1::2]:
        odd_indices.append(i)

    for i in rev_list[::2]:
        even_indices.append(i)

    sum_odd = sum(split_to_digits(''.join(int_to_str(mul(odd_indices, 2)))))
    sum_even = sum(split_to_digits(''.join(int_to_str(even_indices))))
    s = sum_odd + sum_even

    print(s)

    if s % 10 == 0:
        return True
    return False

def mul(list, x):
    return [(int(n) * 2) for n in list]

def split_to_digits(n):
    return [int(i) for i in str(n)]

def int_to_str(n):
    return [str(x) for x in n]

def str_to_int(n):
    return [int(x) for x in n]

main()

General notes:

use if __name__ == '__main__': to avoid the main() function to be executed when the module is imported
I'd pass around the credit card number as a string instead of converting it to string in every single validation step
add docstrings to each of the defined functions

Regarding check_provider() function:

you can check the length to be in range in one go:

if not(13 <= len(str(number)) <= 16):

I would improve the way you distinguish between cards by introducing a mapping between brands and regular expressions (like it was done in pycard module). Then, match each of the expressions one by one until you find a match or a brand was not found:

import re

BRANDS = {
    'Visa': re.compile(r'^4\d{12}(\d{3})?$'),
    'Mastercard': re.compile(r'''
        ^(5[1-5]\d{4}|677189)\d{10}$|  # Traditional 5-series + RU support
        ^(222[1-9]|2[3-6]\d{2}|27[0-1]\d|2720)\d{12}$  # 2016 2-series
    ''', re.VERBOSE),
    'American Express': re.compile(r'^3[47]\d{13}$'),
    'Discover': re.compile(r'^(6011|65\d{2})\d{12}$'),
}

def check_provider(number):
    """Checks a credit card number and returns a matching brand name, or INVALID if no brand matched."""
    for brand, regexp in BRANDS.items():
        if regexp.match(number):
            return brand
    return 'INVALID'

Regarding implementing the Luhn algorithm, check the pycard's implementation - it is quite clean and understandable.


Simple language quiz This code is for a simple language quiz.  It fetches two words and related audio files via a JSON call, presents the user with an image that matches one of the words, and challenges the user to make the match.  My code works but it's a little repetitious and ugly.  I'd like to see how the experts would propose rewriting this.

function getNewWords() {
        {
            var Category = "animals";
            var BaseURL = "http://localhost:61741/VocabGame/play?cat=";
            var URL = BaseURL + Category;

            $.getJSON(URL, {
                tagmode: "any",
                format: "json"
            }, function (data) {
                var choiceA = data.nouns[0].Pinyin;
                var choiceB = data.nouns[1].Pinyin;
                $('#ChoiceA').html(choiceA);
                $('#ChoiceB').html(choiceB);

                var root = "../../Content/Audio/";
                var mp31 = root + data.nouns[0].Audio1 + ".mp3";
                var ogg1 = root + data.nouns[0].Audio1 + ".ogg";
                var mp32 = root + data.nouns[1].Audio1 + ".mp3";
                var ogg2 = root + data.nouns[1].Audio1 + ".ogg";

                attachMouseEnter(mp31, ogg1, mp32, ogg2);

                var random = Math.random();
                if (random >= 0.5) {
                    correctAnswer = "ChoiceA";
                    Search(data.nouns[0].English);
                }
                else {
                    correctAnswer = "ChoiceB"
                    Search(data.nouns[1].English);
                }

            });
        }

    };

    function playAudio1(mp31, ogg1) {
        var mp3_src = mp31;
        var oga_src = ogg1;

        $('#jquery_jplayer_1').jPlayer('setMedia', {
            oga: oga_src,
            mp3: mp3_src
        });

        $('#jquery_jplayer_1').jPlayer("play");
    };

    function playAudio2(mp32, ogg2) {
        var mp3_src = mp32;
        var oga_src = ogg2;

        $('#jquery_jplayer_1').jPlayer('setMedia', {
            oga: oga_src,
            mp3: mp3_src
        });

        $('#jquery_jplayer_1').jPlayer("play");
    };
        function attachMouseEnter(mp31, ogg1, mp32, ogg2) {
            $('#ChoiceA').die('mouseenter.audio1event');
            $('#ChoiceA').live('mouseenter.audio1event', function () {
                playAudio1(mp31, ogg1);
            });

            $('#ChoiceB').die('mouseenter.audio2event');
            $('#ChoiceB').live('mouseenter.audio2event', function () {
                playAudio2(mp32, ogg2);
            });
    }

I think if you generalize the problem to having multiple choices instead of just 2, it would make things a lot simpler. It gives the added flexibility of adding more choices later on if needed, but most importantly, you can cut down explicit references to element A, or element B, or audio A, or audio B, throughout your codebase.

Doing so would mean that you tweak the HTML slightly, and instead of referring to DOM elements directly by their id such as choiceA, or choiceB, you rely on their position or index. The HTML of all choices may then look something like,

..

..

Keeping that in mind, here are the changes I would suggest. Create an object that represents an audio to avoid worrying about passing mp3/ogg variables all over.

function AudioFile(fileName) {
    var root = "../../Content/Audio/";
    this.mp3 = root + fileName + '.mp3';
    this.ogg = root + fileName + '.ogg';
}

Your playAudio1 and playAudio2 functions are identical. They can accept the AudioFile object, and be combined into one.

function playAudio(audio) {
    var player = $('#jquery_jplayer');
    player.jPlayer('setMedia', {
        oga: audio.ogg,
        mp3: audio.mp3
    });
    player.jPlayer('play');
}

By getting away from two choices, you can treat the number of choices as an array, and attach the required event handler for each item in the array alike. Also it seems like you are rebinding the live events after each AJAX call. That's just a waste of resources. You could explicitly bind after each AJAX call, or attach live handlers on document load or something. Here's the revamped getNewWords and attachMouseEnter methods.

function getNewWords() {
    var Category = "animals";
    var BaseURL = "http://localhost:61741/VocabGame/play?cat=";
    var URL = BaseURL + Category;

    $.getJSON(URL, didGetNewWords);

    var containers = $('.choice');

    function didGetNewWords(data) {
        data.nouns.forEach(function(noun, index) {
            var container = containers.get(index);
            container.html(noun.Pinyin);
            var audio = new AudioFile(noun.Audio1);
            attachMouseEnter(container, audio);
        });

        // Pick a random answer by index
        var answerIndex = Math.floor(Math.random() * data.nouns.length);
        correctAnswer = containers.get(answerIndex).id;
        Search(data.nouns[answerIndex].English);
    }
}

// Play this audio when this element is hovered over
function attachMouseEnter(element, audio) {
    $(element).bind('mouseenter', function() {
        playAudio(audio);
    });
}


Constant Time implementation of String Uniqueness Algorithm I've written what I think is a constant time implementation of a uniqueness check. I've seen many algorithms online, which run in linear time, but was wondering whether my conclusion that this runs in constant time was correct.

I've seen many linear versions of this algorithm, but I haven't seen any that run in O(1).

Any feedback on style, flaws in the code, or how to improve the algorithm would be greatly appreciated.

from string import printable

def is_unique(string):
    """Checks whether a string is unique or not.

    Big O:

      O(S * C) - Constant Time

      Where:

        `S` is the characters of the string being checked for uniqueness
        `C` is a set of all ASCII characters (not including the extended set)

    """
    if len(string) == 1:
        # A single character is always going to be unique
        return True

    if len(string) > len(printable):
        # If there are more characters than the ASCII character set,
        # then we know there are duplicates
        return False

    found_character = bytearray(len(printable))

    for character in string:
        for index, ascii_value in enumerate(printable):
            if character == ascii_value:
                if found_character[index] == True:
                    return False

                # Set the index of the ASCII value in our bytearray to True
                found_character[index] = 1
                break

    return True

is_unique(string='abc')

Critique

You have to describe something for which your input scale, which in this would be the length of your string, so in this regard your implementation is still linear. The part where you try every single possible index for storing a specific character is overkill, since it is often possible to directly calculate it through some kind of hashing. It would therefore be much faster to use a dictionary or the equivalent.

Alternative

We can solve this problem very simply for a more general case (any sequence of items is unique) by turning it into a set:

is_unique = lambda xs: len(xs) == len(set(xs))

Note that you might want to do some extra work if you want to only have printable characters, such as by applying a filter first.


Grade point average (GPA) calculator For anyone who's unfamiliar with this grading system.

I'd like a general review of my code, and I have a few questions:

Are plain types okay for the letter grades and credit hours, or could they be public typedefs?
Should operator<< need to call calculateGPA(), or should I create a private accessor and have that called?  In order to maintain encapsulation, I'll need to keep operator<< as a friend.
Is the for-loop in calculateGPA() ideal for this purpose, or are there one or more STL functions that can do the same thing but more cleanly?

GPA.h

#ifndef GPA_H
#define GPA_H

#include <iosfwd>
#include <vector>
#include <utility>

class GPA
{
private:
    std::vector<std::pair<unsigned, unsigned> > grades;
    double calculateGPA() const;

public:
    void add(char, unsigned);
    friend std::ostream& operator<<(std::ostream&, GPA const&);
};

unsigned convertLetterGrade(char);

#endif

GPA.cpp

#include "GPA.h"
#include <ostream>
#include <stdexcept>

void GPA::add(char letterGrade, unsigned creditHours)
{
    unsigned numericalGrade = convertLetterGrade(letterGrade);
    grades.push_back(std::make_pair(numericalGrade, creditHours));
}

double GPA::calculateGPA() const
{
    unsigned sumGradePoints = 0;
    unsigned sumCreditHours = 0;

    for (auto iter = grades.cbegin(); iter != grades.cend(); ++iter)
    {
        sumGradePoints += iter->first * iter->second;
        sumCreditHours += iter->second;
    }

    double totalGPA = static_cast<double>(totalGradePoints) / sumCreditHours;
    return totalGPA;
}

unsigned convertLetterGrade(char letterGrade)
{
    switch (letterGrade)
    {
        case 'a':
        case 'A': return 4;
        case 'b':
        case 'B': return 3;
        case 'c':
        case 'C': return 2;
        case 'd':
        case 'D': return 1;
        case 'f':
        case 'F': return 0;
        default: throw std::logic_error("Invalid Letter Grade");
    }
}

std::ostream& operator<<(std::ostream& out, GPA const& obj)
{
    return out << obj.calculateGPA();
}

Main.cpp

#include "GPA.h"
#include <cstddef>
#include <iostream>

int main()
{
    std::cout << "# Classes: ";
    std::size_t numClasses;
    std::cin >> numClasses;

    GPA gpa;

    for (std::size_t i = 0; i < numClasses; ++i)
    {
        std::cout << "\n(" << i+1 << ") Letter Grade: ";
        char letterGrade;
        std::cin >> letterGrade;

        std::cout << "    Credit Hours: ";
        unsigned creditHours;
        std::cin >> creditHours;

        gpa.add(letterGrade, creditHours);
    }

    std::cout << "\n*** Total GPA: " << gpa;
}

Note that I am completely unfamiliar with the american(?) grade system and GPAs -- I am reviewing the coding style and approach, not whether or not the solution is correct.

General notes:

Your GPA class is not really a class. It's actually just a function. It could just as well be implemented as double calculateGPA(std::vector<std::pair<char, unsigned>>). Beware of the golden hammer, especially when it comes to classes.
You can use std::tolower() to halve the size of your switch.
vector comes after utility in the alphabet :-)
C++11 allows writing >> in nested templates.
Your program won't compile: totalGradePoints is called sumGradePoints.
You can probably use float instead of double.
Make the local variables in calculateGPA() floating point numbers from the start, and drop the cast.
obj should be called gpa or something like that.
Write unit tests.

  Are plain types okay for the letter grades and credit hours, or could they be public typedefs?

Either is fine. Using a typedef makes the intent of a variable a bit clearer. This added distinction between types could help the developer realizing he or she is making a mistake. However, a typedef is sadly just a type alias and does not create a new type! In other words, you do not get any type-safety benefits of doing so.

To get full type safety, you would need to create a (templated) wrapper class around the types in question. Doing so would be a nice exercise when you start to learn about templates.

  Should operator<< need to call calculateGPA(), or should I create a private accessor and have that called?  In order to maintain encapsulation, I'll need to keep operator<< as a friend.

I would cache the result of calculateGPA and provide an accessor to it. Have operator<< call that accessor. This means that your class would need to invalidate the cached value whenever grades is modified.

  Is the for-loop in calculateGPA() ideal for this purpose, or are there one or more STL functions that can do the same thing but more cleanly?

The for loop is sufficiently short and sweet, and makes just a single pass (unlike most other solutions I can think of). You could always use std::accumulate and a lambda, but you would have to do so for both sumCreditPoints and sumGradeHours.

Am I guilty of converting you to the const& style? :-)


Display image from live webcam as taken, with four different color filters and in B/W I have a live webcam windows which means there are six subdivided windows in a single window live. And show pictures in colored and black and white. Is there anyway I can make the code minimalistically minimized? I believe the code is considerably long.

import cv2
import cv
import numpy as np
import matplotlib.image as mpimg
from matplotlib import pyplot as plt

def threshold_slow(T, image):
    # grab the image dimensions
    h = image.shape[0]
    w = image.shape[1]
    d = image.shape[2]
    # loop over the image, pixel by pixel
    for y in range(0, h):
        for x in range(0, w):
            for z in range(0, d):
                # threshold the pixel

                if image[y, x,z] >= T:
                    image[y, x,z] = 255
                else:
                    image[y, x,z] = 0

    # return the thresholded image
    return image
def grab_frame(cam):

    #cv2.namedWindow("test")

    #img_counter = 0

    while True:
        ret, color1 = cam.read()
        #r = 100.0 / color1.shape[1]
        r = 640.0 / color1.shape[1]
        #r = 0.25
        dim = (100, int(color1.shape[0] * r))
        dim = (640,480)
        # perform the actual resizing of the image and show it
        color = cv2.resize(color1, dim, interpolation = cv2.INTER_AREA)

        #color = color1.copy()
        b = color.copy()
        # set green and red channels to 0
        b[:, :, 1] = 0
        b[:, :, 2] = 0

        g = color.copy()
        # set blue and red channels to 0
        g[:, :, 0] = 0
        g[:, :, 2] = 0

        r = color.copy()
        # set blue and green channels to 0
        r[:, :, 0] = 0
        r[:, :, 1] = 0

        #y= color.copy()
        #gray = cv2.cvtColor(l,cv2.COLOR_RGB2GRAY)
        #_,y = cv2.threshold(gray, 60, 255, cv2.THRESH_BINARY)
        #y = cv2.cvtColor(y, cv2.COLOR_GRAY2RGB)

        y = cv2.add(r,g)

        d = color.copy()
        gray1 = cv2.cvtColor(d,cv2.COLOR_RGB2GRAY)
        _,p = cv2.threshold(gray1, 60, 255, cv2.THRESH_BINARY)
        p = cv2.cvtColor(p, cv2.COLOR_GRAY2RGB)
        #threshold_slow(220,p)
        return [color,b,g,r,y,p]

cam = cv2.VideoCapture(0)

    #cv2.waitKey(0)
while(1):
    ret, color = cam.read()
    [color,b,g,r,y,p] = grab_frame(cam)
    horiz = np.hstack((color,b,g))
    #verti = np.vstack((color,r))
    horiz1 = np.hstack((r,y,p))
    verti = np.vstack((horiz,horiz1))
    cv2.imshow('HORIZONTAL', verti)

    if not ret:
        break
    k = cv2.waitKey(1)

    if k%256 == 27:
        # ESC pressed
        print("Escape hit, closing...")
        break

cam.release()

cv2.destroyAllWindows()

First, get rid of all the unneeded whitespace. Use consistent amount between functions (Python's official style-guide, PEP8, recommends two).
PEP8 also recommends using spaces in lists, after the commas, and lower_case for all variables and functions (your T in threshold_slow violates this).
Don't use magic numbers in your code. Give them readable names and if necessary make them global constants:

WIDTH, HEIGHT = 640, 480

Next, since your images are already numpy arrays, use that fact. Your (unused) threshold_slow function can be replaced by a single line using numpy.where:

def threshold_fast(T, image):
    return np.where(image >= T, 255, 0)

Note that this does not modify the image inplace. It is a bad practice to do that and return a modified/new object. You should decide, either return a new object or modify in place and return None.
The import cv is not used (and I could not even find a way to install it anymore).
Tuple assignment works also without a list on the left side, just do color, b, g, r, y, p = grab_frame(cam). The same is true when returning a tuple (return color, b, g, r, y, p).
Arguably, I would split up your grab_frame code into subfunctions like red(image), green(image), blue(image), yellow(image), black_and_white(image).

def red(image):
    """Copy only the red channel from image"""
    out = np.zeros_like(image)
    # for some reason red is in the last channel
    out[:, :, 2] = image[:, :, 2]
    return out
...

While this move will not make your code shorter, it will make it more readable.
Note that the canonical order is red, green, blue (RGB). If at all possible I would stick to that. I'm not sure why openCV would deviate from that.
You should at least add a docstring to each of your functions as a rudimentary documentation. See above for a short example.
You can use while True instead of while(1). No parenthesis needed and True is unambiguous (even for those people who both know C-like languages, where 0 is False and shell scripting languages like bash, where non-zero is False).
I would also add a tile(images, rows) function that puts your images into rows and columns. You could just use the itertools recipe grouper for this:

from itertools import zip_longest

def grouper(iterable, n, fillvalue=None):
    "Collect data into fixed-length chunks or blocks"
    # grouper('ABCDEFG', 3, 'x') --> ABC DEF Gxx"
    args = [iter(iterable)] * n
    return zip_longest(*args, fillvalue=fillvalue)

Since you seem to want to use different amounts of tiles, and different effects, it might make sense to keep a list of functions to apply to the base image, so that in the end you only need one call:

def identity(x):
    return x

def tile(images, cols, fillvalue=None):
    return np.vstack(np.hstack(group)
                     for group in grouper(images, cols, fillvalue))

funcs = identity, red, black_and_white, canny
images = (func(image) for func in funcs)
# arrange them in a 2x2 grid
cv2.imshow('HORIZONTAL', tile(images, cols=2, fillvalue=np.zeros_like(image)))

If the number of images is not evenly divisible by the number of columns, the row is filled up with blank images.
You should put your main calling code under a if __name__ == "__main__" guard to allow importing from this script.


Load/Save application sessions I'm working on load/save application sessions functionality, and came up with this code:

public class SessionManager
{
    private static ISessionStorage storage = new DbTableSessionStorage();
    private static StudySession session;
    public static StudySession Current
    {
        get
        {
            if (session == null) {
                throw new Exception("Load or start new session before using SessionManager.Current");
            }

            return session;
        }
    }

    public static void LoadSession(int sessionId)
    {
        session = storage.LoadSession(sessionId);
    }

    public static void Save(StudySession session, int prefferedId = -1)
    {
        storage.SaveSession(session, prefferedId);
    }
}

public class DbTableSessionStorage : ISessionStorage
{
    public void SaveSession(StudySession studySession, int prefferedId = -1)
    {
        Session session = DSDatabase.Context["USER"].Read<Session>(prefferedId);
        if (session == null) {
            session = new Session();
        }

        if (prefferedId != -1) {
            session.Id = prefferedId;
        }

        session.Name = studySession.Name;
        session.Timestamp = studySession.Timestamp == DateTime.MinValue
                                ? DateTime.Now
                                : studySession.Timestamp;

        session.GroupId = studySession.GroupId;
        session.TestId = studySession.TestId;
        session.Position = studySession.Position;
        session.SoundOn = studySession.SoundOn;

        DSDatabase.Context["USER"].Save(session);
    }

    public StudySession LoadSession(int id)
    {
        StudySession result = new StudySession();

        Session session = DSDatabase.Context["USER"].Read<Session>(id);

        if (session != null) {
            result.Id = session.Id.Value;
            result.Name = session.Name;
            result.Timestamp = session.Timestamp;
            result.GroupId = session.GroupId;
            result.TestId = session.TestId;
            result.SoundOn = session.SoundOn;
        }

        return result;
    }
}

I am using it in this way:

public class Chapter {
//....

    public static Chapter[] GetChildren(int parentId)
    {
        string sqlQuery = baseSqlQuery + Environment.NewLine +
                 "WHERE CHAPTERS.GroupId=? AND CHAPTERS.ParentId=?";

        return DSDatabase.RunQuery<Chapter>(sqlQuery, SessionManager.Current.GroupId, parentId).ToArray();
    }

//....
}

It seems to me as not a very good solution, because my code becomes tightly coupled and hides the dependencies. I'm looking now for better solutions to this issue.

Current property

You should never throw the Exception. There is almost always a more appropriate exception already available or you should create a new one. The name of the exception usually already indicates the reason for it.

In this case I would pick the InvalidOperationExctption.

LoadSession method

Is the user allowed to re-load the session? If not then either throw an exception if he tries it again or return the already loaded session with the coalesce operator:

session = session ?? storage.LoadSession(sessionId);

I would also prefer if the method returned the loaded session too so I can immediately use it without switching to the property:

return (session = storage.LoadSession(sessionId));

You can assign it and return it in a single line.

StudySession vs Session classes

I woudn't create another session object. I think you should just reuse the Seesion you get from the database. It already has everything you need. You are just copying the values.

Your code would become much simpler without this mappings.

Chapter class

To loosen the coupling you would need to make the methods of this classes non-static and pass to the constructor an instance of the DSDatabase or its interface and maybe a SessionManager but I don't entirely understand what it is good for. Actually you need a group id for the GetChildren query. So maybe pass it as a parameter.


Getting a string from a range of possible paths I have a scenario where I need to grab the first occurrence of a string from an object, but only if the match occurs in one of the path's that have been pre-defined.

{ id: 'I60ODI', description: 'some random description' }
{ foo: 'bar', description: { color: 'green', text: 'some description within text' } }

When provided either of the two objects above, I would expect the solution to return either some random description or some description within text, provided that the two possible paths are obj.description and obj.description.text. New paths might also need to be added in the future, so it needs to be easy to add them.

Here is the solution I have implemented so far, but to me it doesn't seem optimal.

// require the ramda library
const R = require('ramda');

// is the provided value a string?
const isString = R.ifElse(R.compose(R.equals('string'), (val) => typeof val), R.identity, R.always(false));
const addStringCheck = t => R.compose(isString, t);

// the possible paths to take (subject to scale)
const possiblePaths = [
    R.path(['description']),
    R.path(['description', 'text'])
];
// add the string check to each of the potential paths
const mappedPaths = R.map((x) => addStringCheck(x), possiblePaths);

// select the first occurrence of a string
const extractString = R.either(...mappedPaths);

// two test objects
const firstObject = { description: 'some random description' };
const secondObject = { description: { text: 'some description within text' } };
const thirdObject = { foo: 'bar' };

console.log(extractString(firstObject)); // 'some random description'
console.log(extractString(secondObject)); // 'some description within text'
console.log(extractString(thirdObject)); // false

I would really appreciate it if a seasoned functional programmer might provide me with some alternative approaches to implementation. Thanks.

A few points:

your isString function isn't exactly checking if it's a string, as it's also returning false if not. The typeof check can be simplified to R.is(String)
It's probably better to push all the inputs to the edge of the logic, in this case making paths an argument too. Just splits out our logic an our inputs a bit better.
Instead of wrapping each path in R.path, you can make a function which turns all paths into path functions
For applying the list of path functions to the object, we can use R.juxt

Possible refactoring:

const isString = R.is(String)
const makePaths = R.map(R.path)

const extractStringFor = (paths) =>
  R.compose(
    R.defaultTo(false),
    R.find(isString),
    R.juxt(makePaths(paths))
  )

const firstObject = { description: 'some random description' };
const secondObject = { description: { text: 'some description within text' } };
const thirdObject = { foo: 'bar' };

const paths = [['description'], ['description', 'text']]
const extractString = extractStringFor(paths)

extractString(firstObject), // 'some random description'
extractString(secondObject), // 'some description within text'
extractString(thirdObject) // false

caveat: This runs all paths against each object, and then find the match that's a string. That could be optimised though, potentially with R.takeWhile/R.reduceWhile or something.


Most occuring digit in an array of numbers I stumbled upon this exercise , and I am wondering if there is a more efficient way of writing this. It is written from high school level knowledge.

  Find the most occuring number of an array of numbers, with each number only counting one per number. For example in 2222 the 2 only counts ONCE. If you have 2 or more digits with the same amount of occurrences write out a random one. For example X[3] = {20; 21; 30}, either write out 0 or 2.

I could've used dynamic arrays, but that's not what I'm interested in here.

#include <iostream>
#include <time.h>
#include <stdlib.h>

using namespace std;

struct numbers
{
int number;
int howmany;
};

int main()
{
int X[20], Z[9], sizex, i, j, temp, howmany_found = 0, howmany_elem = 0, max = 0;
numbers Y[10];
bool found;

srand(time(NULL));

cout << "Array size: " ;
cin >> sizex;

for (i = 0; i < sizex; i++)
{
    cout << i + 1 << " element: " << " " ;
    cin >> X[i];
}

for (i = 0; i <= 9; i++)
{
    Y[i].number = i;
}

for (i = 0; i <= 9; i++)
{
    for (j = 0; j < sizex; j++)
    {
    found = false;
    temp = X[j];

    while (found != true && temp >= 1)
    {
        if (temp % 10 == i)
        {
            found = true;
            howmany_found++;

        }
        temp /= 10;
    }
    }
    Y[i].howmany = howmany_found;
    howmany_found = 0;
}

for (i = 0; i <= 9; i++)
{
    if (Y[i].howmany > max)
        max = Y[i].howmany;

    if (Y[i].howmany > 0)
    cout << "(" << Y[i].number << "," << Y[i].howmany << ")" ;
}

for (i = 0; i <=9; i++)
{
    if (Y[i].howmany == max)
    {
        Z[howmany_elem] = Y[i].number;
        howmany_elem++;
    }
}

if (howmany_elem > 1)
{
int RandIndex = rand() % howmany_elem ;
cout << endl;
cout << Z[RandIndex] ;
}
else
{
    cout << endl;
    cout << Z[0] ;
}

return 0;
}

Outer loop unnecessary
If there are N numbers, each with M digits, your solution is using 10 * N * M operations to compute the solution.  This is because for each digit (e.g. 0, 1, 2), you look at every digit in every number.  But you could make your solution 10 times faster by only looking at each digit once.  All you need to do is to track duplicate digits for each new number.
Here is how that would work:
for (i = 0; i <= 9; i++)
{
    Y[i].number = i;
    Y[i].howmany = 0;
}

for (i = 0; i < sizex; i++)
{
    int foundDigits = 0;

    for (int curNumber = X[i]; curNumber > 0; curNumber /= 10) {
        int digit = curNumber % 10;
        int bit   = 1 << digit;

        if ((foundDigits & bit) == 0) {
            foundDigits |= bit;
            Y[digit].howmany++;
        }
    }
}

Here, foundDigits is a bitmask which contains a 1 bit for each digit already seen in the current number.  This is what is used to track duplicate digits.  When a non-duplicate digit is seen, we can just increment the Y[digit].howmany array directly.
struct numbers unnecessary
Actually, the numbers structure is unnecessary because Y[i].number is always equal to i.  So you can just have Y be an array of integers holding the count of each digit.  Wherever you currently use Y[i].number, just replace that with i instead.


Updating state with React and Websockets I'm working on a React app with websocket. In this app there is a socket event listener which returns 3 types: add, change, and remove. Certain state update operations will be performed based on the type received.

Do I properly mutate the users state? I'm not sure whether it follows the principle of React regarding updating states.

ws.on("connect", () => {
  console.log("connected");

  ws.emit("load");
});

ws.on("feed", response => {
  const prev = response.old_val;
  const next = response.new_val;
  const type = response.type;
  setUsers(users => {
    let currentUser = [...users];
    let index;
    switch (type) {
      case "add":
        return [...users, next];
      case "change":
        index = users.findIndex(u => u.id === next.id);
        currentUser[index].name = next.name;
        return currentUser;
      case "remove":
        index = users.findIndex(u => u.id === prev.id);
        currentUser.splice(index, 1);
        return currentUser;
      default:
        return currentUser;
    }
  });
});

This seems reasonable. The most important part is that you do not mutate the react state in-place. By doing let currentUser = [...users]; you copy the state and instead mutate the copy, then assign it as the updated state.

An alternative would be to use .map and .filter to make your changes. I see this pattern more often than the state copy approach.

Example:

ws.on("feed", response => {
  const { new_val: next, type } = response; // Use destructuring and inline renaming.

  setUsers(users => {
    switch (type) {
      case "add":
        return [...users, next];

      case "change":
        return users.map(u => {
          // Only change the user specified in the message
          if (u.id === next.id) return {...u, name: next.name }
          return u;
          // or return users.map(u => u.id !== next.id ? u : ({...u, name: next.name})) if you prefer one-liners
        })

      case "remove":
        return users.filter(u => u.id !== next.id);

      default:
        return users;
    }
  });
});


Class that runs a query I have made this class that runs a query. I want to optimize my code and want to have some expert advice.

Here is the class:

class testClass extends Model
{
    protected $_var1;
    protected $_var2;
    protected $_var3;

    public function __construct($var1,$var2, $var3)
    {
        $this->_var1= $this->$var1;
        $this->_var2= $this->var2;
       $this->_var3= 8;  // i need this 8 if no value is passed

    }

    public function testMethod(){
       $query = "SELECT * from table where id = ".this->_var1." AND name= ".$this->var2." AND x= ".$this->var3;
            return $runQuery($query);
    // $runquery is another method…not in concern here..it returns the results…just take my words :)
    }
}

Usage of class:

  $r= new testClass($var1, $var2, $var3);
            $result = $r->testMethod();

How can I improve this?  Or is this even a good way to do it?

I'm not sure what you want to achieve with the constructor.

public function __construct($var1,$var2, $var3)
    {
        $this->_var1= $this->$var1;
        $this->_var2= $this->var2;
        $this->_var3= 8;  // i need this 8 if no value is passed

    }

If you want to set the properties to the given parameters, you should do

 $this->_var1= $var1;

And with

$this->_var3= 8;  // i need this 8 if no value is passed

you always set property $_var3 to 8, nonetheless if $var3 is being passed.

For setting a default value to a paramter you can write it like this

public function __construct($var1, $var2, $var3 = 8)
        {
            $this->_var1 = $var1;
            $this->_var2 = $var2;
            $this->_var3 = $var3;

        }

so if you call

$r = new testClass($var1, $var2, 10);

$this->_var3 would be set to 10, and if you call

$r = new testClass($var1, $var2);

$this->_var3 would be set to the default 8.

In your testMethod you're using $this->var2 & $this->var3, without the underline. These properties are not defined.

return $runQuery($query);

remove the $ in the function's name, or it would be taken as variable.

In your SQL-Query there's a column named "name". In all cases where the column types are strings, you have to write the value in single quotes.

$query = "SELECT * from table where id = ".this->_var1." AND name= '".$this->_var2."' AND x= ".$this->_var3;

For debugging purposes, set

ini_set('display_errors', 1);
error_reporting(E_ALL);

to the starting script.


Invoking function stored in a struct using Rust I'm new to Rust, and I'm writing a basic command line input parser as a learning exercise. Commands entered on the command line are evaluated using the Cmd struct which contains the name of the command and a function to call when invoked. Though the code works, I find the use of the get_invocation function to be a little weird.

pub struct Cmd<T: FnMut()> {
    name: String,
    invocation: T,
}

impl<T: FnMut()> Cmd<T> {
    pub fn new(name: &str, invocation: T) -> Cmd<T> {
        Cmd {
            name: String::from(name),
            invocation,
        }
    }

    pub fn invoke(self) {
        self.get_invocation()();
    }

    fn get_invocation(self) -> T {
        self.invocation
    }
}

I am aware that I could use (self.invocation)() to directly call my struct function in invoke(), but that would involve setting the invoke self parameter to mutable, which would require me to make each instance of Cmd mutable if I wish to use the invoke() function. This is something I would like to avoid as, conceptually, nothing in the struct is being mutated. Is there a less odd / more idiomatic way to deal with stored function invocation in Rust, while still maintaining immutability? I also would appreciate any other pointers/tips on my code.

There are two distinct applications of mutability in Rust:

Bindings (variables) can be mutable or immutable.
Borrows (references) can be mutable or immutable.

Here, you're not using any borrows, so we're only concerned with bindings.

Note that values are not intrinsically mutable or immutable. Therefore, when you move a value from one binding to another, you can change the mutability. The methods invoke and get_invocation take ownership of the Cmd, therefore they can decide to declare self as mutable, even if the original value is not declared as mutable. For example, this is valid:

impl<T: FnMut()> Cmd<T> {
    pub fn invoke(mut self) {
        (self.invocation)();
    }
}

fn help() {
    println!("not implemented yet :)");
}

fn main() {
    let cmd = Cmd::new("help", help);
    cmd.invoke();
}

If you were to take self by mutable reference, rather than by value, then the original variable would indeed need to be declared as mutable. You could not invoke the function with only an immutable reference, because FnMut closures can't be invoked from an immutable borrow. If that's a problem, switch to Fn instead.

impl<T: FnMut()> Cmd<T> {
    pub fn new(name: &str, invocation: T) -> Cmd<T> {
        Cmd {
            name: String::from(name),
            invocation,
        }
    }

    pub fn invoke(&mut self) {
        (self.invocation)();
    }

    fn get_invocation(&mut self) -> &mut T {
        &mut self.invocation
    }
}

fn help() {
    println!("not implemented yet :)");
}

fn main() {
    let mut cmd = Cmd::new("help", help);
    cmd.invoke();
}


User controlled rotation with arcBall I am trying to visualize a pointcloud centered around the origin. I also want to have a user controlled rotation for which I found the arcball.

rMat is the model matrix, passed into the shader.

inline QVector3D getArcballVector(const QPoint& pt, int width, int height)
{
    QVector3D P = QVector3D(1.0*pt.x ()/width * 2 - 1.0,
                            1.0*pt.y ()/height * 2 - 1.0,
                            0);
    P.setY (-P.y ());

    float OP_squared = P.lengthSquared ();
    if (OP_squared <= 1)
        P.setZ (std::sqrt(1 - OP_squared));  // Pythagore
    else
        P.normalize ();  // nearest point
    return P;
}

void PointCloud::mouseMoveEvent(QMouseEvent * e)
{
    if(e->buttons ()==Qt::LeftButton)
    {
        if(!rotPos.isNull () && rotPos!=e->pos ())
        {
            //rotate using an arcBall for freeform rotation
            QVector3D vec1 = getArcballVector (rotPos, width (), height ());
            QVector3D vec2 = getArcballVector (e->pos (), width (), height ());

            //use bisector to get half-angle cos and sin from dot and cross product
            // for quaternion initialisation
            QVector3D vec3 = vec1+vec2;
            vec3.normalize ();
            QVector3D rotaxis = QVector3D::crossProduct (vec1, vec3);
            double cos = QVector3D::dotProduct (vec1, vec3);

            QQuaternion quat (cos,rotaxis);
            quat.normalize ();
            //we want to left-multiply rMat with quat but that isn't available
            //so we'll have to do it the long way around
            QMatrix4x4 rot;
            rot.rotate (quat);
            rMat=rot*rMat;

            updateGL ();
        }
        rotPos=e->pos ();
    }
}

void PointCloud::mousePressEvent(QMouseEvent *e)
{
    if(e->buttons ()==Qt::LeftButton)
    {
        rotPos=e->pos ();
    }
}

One point to make is that most arcball examples use acos to get the rotation angle for the axis-angle representation and use that to create the matrix and only mention using quaternions in passing (and if they do use quaternions they still end up using axis-angle). I found that I can derive the quaternion values directly using the dot and cross products of the first vector with the bisector of the 2 vectors (vec3).

Are there any glaring inefficiencies? I could move the update code to a QTimer activated slot but the usage of the program has the user mostly ignoring the screen while concentrating on the road.

After some research I found that it's slightly more efficient to get the half quaternion directly from the double quaternion:

QVector3D rotaxis = QVector3D::crossProduct (vec1, vec2);
double cos = QVector3D::dotProduct (vec1, vec2);

// rotaxis and cos describe the double quat
// add 1 to cos so normalizing will 0.5 lerp between the null quat and the double quat
QQuaternion quat (1+cos,rotaxis);
quat.normalize ();

This saves the normalization of the bisector.

The rotation matrix can be constructed from a non-unit quaternion without first normalizing it. You do have to provide your own function to do so because the Qt algorithm assumes a unit quaternion:

inline QMatrix4x4 quatToMat(QQuaternion q)
{
    //based on algorithm on wikipedia
    // http://en.wikipedia.org/wiki/Rotation_matrix#Quaternion
    float w = q.scalar ();
    float x = q.x();
    float y = q.y();
    float z = q.z();

    float n = q.lengthSquared();
    float s =  n == 0?  0 : 2 / n;
    float wx = s * w * x, wy = s * w * y, wz = s * w * z;
    float xx = s * x * x, xy = s * x * y, xz = s * x * z;
    float yy = s * y * y, yz = s * y * z, zz = s * z * z;

    float m[16] = { 1 - (yy + zz),         xy + wz ,         xz - wy ,0,
                         xy - wz ,    1 - (xx + zz),         yz + wx ,0,
                         xz + wy ,         yz - wx ,    1 - (xx + yy),0,
                               0 ,               0 ,               0 ,1  };
    QMatrix4x4 result =  QMatrix4x4(m,4,4);
    result.optimize ();
    return result;
}

Using that eliminates all normalization from in mouseMoveEvent except to get the arcballVectors themselves.


Extracting values from refs pointing to form inputs I have a form component with multiple refs pointing to form inputs. On submit, those inputs' values need to be passed the into form callback, structured as an object.

Currently, it's achieved like this:

class MyComponent extends React.Component {
    render() {
        return (
            <form onSubmit={() => this.onSubmit()}>
                <input ref="firstName">
                <input ref="lastName">
                <input ref="age">
                <input ref="sex">
            </form>
        );
    }
    onSubmit() {
        let firstName = this.refs.firstName.value.trim();
        let lastName = this.refs.lastName.value.trim();
        let age = this.refs.age.value.trim();
        let sex = this.refs.sex.value.trim();

        this.props.onSubmit({
            firstName,
            lastName,
            age,
            sex
        });
    }
}

These lines let firstName = this.refs.firstName.value.trim(); look like a code duplication to me. Could they be rewritten into something more elegant? ES2015 answers are welcome.

These lines let firstName = this.refs.firstName.value.trim(); look like a code duplication to me. Could they be rewritten into something more elegant?

Looks ok to me. These fields are too few to be generalized by some generalization logic.

Now if the form does grow, you can just get a reference to the <form> and do a quick querySelectorAll for all input elements, including select and textarea that have a name.

var inputs = this.refs.form.querySelectorAll('input[name]');

You can then loop through them, get the name and value and use it as key and value for your object. For simple cases, this works. A slightly more complicated case is handling complex names like foo[a] and foo[b] which is usually interpreted as {foo: {a : 'value', b: 'value'}}.


Processing files in directories with multithreading I am processing files in directories. I want to use multi-threading in two ways:

10 threads to process files/folders concurrently
10 threads to process all lines in each file concurrently

In this code, FolderProcessor implements (1), and DocProcessor implements (2). Is there any flaw in this implementation? My testing seems to show that (2) works and reduces time a lot, but (1) doesn't really reduce the processing time.

class FolderProcessor{
      ...
 void processFolder(String inputPath, String outputPath){

            File inputFolder = new File(inputPath);
            String[] filenames = inputFolder.list();

            ExecutorService pool = Executors.newFixedThreadPool(10);

            for (String filename : filenames) {
                String filePath = inputPath + filename;
                File inputfile = new File(filePath);
                if (inputfile.isDirectory()) {
                    processFolder(filePath, outputPath + filename);
                } else {
                    pool.execute(new Runnable() {
                        public void run() {
                            log.info("Start processing " + filePath);
                            Writer.write(filePath, outputPath);
                        }
                    });
                }
            }
            pool.shutdown();
            try {
                pool.awaitTermination(Long.MAX_VALUE, TimeUnit.NANOSECONDS);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }
}

class DocumentWriter{
    ...
 public static void write(String inputFile, String outputFile) {
        try {

            File test_output = new File(outputFile);
            test_output.getParentFile().mkdirs();
            test_output.createNewFile();
            FileWriter write_test_output = new FileWriter(outputFile);

            List<Document> docs = DocPrecessor.processDocs(inputFile);
            for (Document doc : docs) {
               ...
            }
        } catch (IOException e) {
            e.printStackTrace();
        }
    }
}

class DocPrecessor{
    ......
    public static List<Document> processDocs(String filePath) {

    BufferedReader br = null;
    ExecutorService pool = Executors.newFixedThreadPool(10);
    List<Document> processedDocs = new ArrayList<>();
    try {
        ...
        String line = null;
        int docNo = 0;
        List<Future<Document>> tasks = new ArrayList<>();
        while ((line = br.readLine()) != null) {
            Callable<Document> callable = new TextThread(line, ++docNo);
            tasks.add(pool.submit(callable));
        }
        for (Future<Document> task : tasks) {
            try {
                processedDocs.add(task.get());
            } catch (InterruptedException e) {
                log.error("InterruptedException Failure: " + line);
            } catch (ExecutionException e) {
                log.error("Thread ExecutionException e: " + line);
                e.printStackTrace();
            }
        }
        pool.shutdown();
        try {
            pool.awaitTermination(Long.MAX_VALUE, TimeUnit.NANOSECONDS);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
    } catch (IOException e) {
        e.printStackTrace();
    } finally {
        try {
            if (br != null)
                br.close();
        } catch (IOException ex) {
            ex.printStackTrace();
        }
    }
    return processedDocs;
}

FolderProcessor
if (inputfile.isDirectory()) {
    processFolder(filePath, outputPath + filename);
}

This is why you're not seeing any benefits from parallelism; you're processing subfolders within the same thread!  But even so, you'd be making a new ExecutorService each invocation; pool should be a class-level field:
private final ExecutorService pool;

public FolderProcessor(ExecutorService pool){
    this.pool = pool;
}

After running some tests, I noticed that your path resolution is broken; the path separator isn't being added for subpaths!  You can use all the operating system's rules by switching to Paths:
String filePath = inputFolder.toPath().resolve(filename).toString();

Some quick cleanup and a change to lambda notation (Java 8 is so much cleaner):
class FolderProcessor{
    private static final Logger log = Logger.getGlobal();
    private final ExecutorService pool;

    public FolderProcessor(ExecutorService pool){
        this.pool = pool;
    }

    void processFolder(String inputPath, String outputPath){
        File inputFolder = new File(inputPath);

        for (String filename : inputFolder.list()) {
            String filePath = inputFolder.toPath().resolve(filename).toString();
            String writeTo = new File(outputPath).toPath().resolve(filename).toString();
            if (new File(filePath).isDirectory()) {
                pool.execute(()->processFolder(filePath, writeTo));
            } else {
                pool.execute(()->{
                    log.info("Start processing " + filePath);
                    DocumentWriter.write(filePath, writeTo);
                });
            }
        }
    }
}

Note: My IDE warns me that inputFolder.list() could throw a NullPointerException if the File isn't a directory.  While your current usage ensures that it is, you may want to put in a check down the line.
DocumentWriter
Since we're missing the meat of the method (the contents of the for loop), it's hard to make too many recommendations.  I am concerned with the shift in naming convention (Java uses camelCase), and the lack of checking for errors in file creation.  Also, you create a file and discard it as soon as you've made your checks, when you could save the FileWriter some work by passing it along.
After some cleanup:
class DocumentWriter{
    private static final Logger log = Logger.getGlobal();

    public static void write(String inputPath, String outputPath) {
        try {
            File outputFile = new File(outputPath);
            if(outputFile.getParentFile().mkdirs()){
                outputFile.createNewFile();
                FileWriter outputWriter = new FileWriter(outputFile);

                List<Document> docs = DocProcessor.processDocs(inputPath);
                for(Document doc : docs){
                    // Missing code
                }
            }else{
                // You may want to log this
                log.config("File could not be created: " + outputPath);
            }
        } catch (IOException e) {
            e.printStackTrace();
        }
    }
}

DocProcessor
I assumed you wanted the typo fixed.  There's not much else to say about this class, since all the logic is either tied up in a class we can't see (TextThread) or omitted.  I will say that if you're running this method in parallel, you're going to be creating an ExecutorService for each invocation, possibly creating hundreds of threads.  A shared ExecutorService, (could be static, but preferably a final instance variable) would be a better solution.
Other Notes
You probably want to use Executors.newWorkStealingPool(10) since it keeps the threads active while waiting for I/O operations, which will give you better performance.  newFixedThreadPool(10) tends to work better for CPU-intensive operations as opposed to I/O-intensive operations.


Get the total counted stock take lines for each hour between two dates I have the following table structure:

Tables

stocktakebinline
stocktakebinlinebatch

A stocktakebinline has a one to many relationship with stocktakebinlinebatch. A line consists of many batches.

Columns

stocktakebinline

lineid | reference
-------+----------
1      | Product A
2      | Product B
3      | Product C

stocktakebinlinebatch

id | lineid | batchnumber | instock | countedstocklevel | counteddate
---+--------+-------------+---------+-------------------+------------
1  | 1      | Batch A     | 3.0     | null              | null
2  | 1      | Batch B     | 1.0     | 5.0               | 2017-02-15 10:14
3  | 2      | Batch C     | 2.0     | 2.0               | 2017-02-15 11:30
4  | 2      | Batch D     | 0.0     | 0.0               | 2017-02-15 11:31
5  | 2      | Batch E     | 6.0     | null              | null
6  | 3      | Batch F     | 5.0     | 5.0               | 2017-02-15 13:58

I have omitted some columns for brevity. Also, the date fields are valid psql  dates and are not strings, I have just written them that way so they are easier to read.

I am trying to calculate how many lines have been counted every hour from a given start date to now. The output will look like this:

counted | date
--------+--------
0       | 2017-02-15 09:00
1       | 2017-02-15 10:00
1       | 2017-02-15 11:00
0       | 2017-02-15 12:00
1       | 2017-02-15 13:00
0       | 2017-02-15 14:00

A line is considered counted if any of it's batches have been counted.

To achieve this, I have written the following query:

with tempCountedHours as
(
    select
        count(*) as counted,
        b.date
    from stocktakebinline l
    join
    (
        --Gets minutes and how many where counted in that hour for all lines.
        select
            lineid,
            date_trunc('hour', counteddate) as date
        from stocktakebinlinebatch
        where
            --Where the stock level is over 0 or if it is 0,
            --then only zero stocks that have been counted.
            counteddate is not null
            and (instock > 0 or (instock = 0 and countedstocklevel > 0))
        group by
            lineid,
            date_trunc('hour', counteddate)
    ) b on l.id = b.lineid
    group by
        b.date
    order by
        b.date asc
)
select
    *
from tempCountedHours
union
(
    select
        0 as counted,
        d.date
    from
    (
        --Get all of the hours between the created date of the stock take
        --and right now, except those that were counted.
        select
            date_trunc('hour', d) as date
        from generate_series(
            '2017-02-15 09:00',
            now(),
            '1 hour'::interval
        ) d
        except
        select date from tempCountedHours
    ) d
)
order by
    date asc;

To try and make some sense of this, I am using the following logic:

Get all of the hours where a line has been counted (and how many have been counted).
Get all of the hours between the two dates and exclude ones which have lines that are counted
In the end, union the two data sets and show 0 counted for hours that have not been counted and display the counted amount for hours that have been counted.

This query works fine, however it's quite large. Is there a way I can simplify this query?

It's probably easier to switch to a Left Join instead of Union/Except:

with tempCountedHours as
(
    select
        count(*) as counted,
        b.date
    from stocktakebinline l
    join
    (
        --Gets minutes and how many where counted in that hour for all lines.
        select
            lineid,
            date_trunc('hour', counteddate) as date
        from stocktakebinlinebatch
        where
            --Where the stock level is over 0 or if it is 0,
            --then only zero stocks that have been counted.
            counteddate is not null
            and (instock > 0 or (instock = 0 and countedstocklevel > 0))
        group by
            lineid,
            date_trunc('hour', counteddate)
    ) b on l.id = b.lineid
    group by
        b.date
    order by
        b.date asc
)
select
    COALESCE(tch.counted, 0), -- set missing data to zero
    d.date
from
 (
    --Get all of the hours between the created date of the stock take
    --and right now
    select
        date_trunc('hour', d) as date
    from generate_series(
        '2017-02-15 09:00',
        now(),
        '1 hour'::interval
 ) d
left join tempCountedHours as tch
on d.date = tch.date
order by
    d.date asc;


Rearranging DOM elements using inconsistent criteria Before you read ahead: ES6 may not be used.

Consider a web-page showing a report of some data that is structured as follows:

<button id='sort-by-category'>Category</button>
<button id='sort-by-timestamp'>Timestamp</button>

        ...

        ...

        ...

Such that each section in the report body needs to be sorted according to the custom data- attributes. The catch is that not all attributes are created equal; for example: sorting by timestamp is straightforward, but sorting by item-id isn't because we also want to group items of the same category together.

Here's the implementation of that in JS:

(function () {
    "use strict";

    var categoryOrder = {
        "fruit" : 0,
        "vegetable" : 1
    };

    function compareStrings(string1, string2) {
        if (string1 < string2) {
            return -1;
        } else if (string1 > string2) {
            return 1;
        }
        return 0;
    }

    function compareCategories(element1, element2) {
        var category1 = element1.getAttribute("data-category-name");
        var category2 = element2.getAttribute("data-category-name");

        if (category1 !== category2) {
            return categoryOrder[category1] - categoryOrder[category2];
        }

        category1 = +element1.getAttribute("data-item-id");
        category2 = +element2.getAttribute("data-item-id");

        return category1 - category2;
    }

    // dates are ISO-8601
    function compareTimestamps(element1, element2) {
        var timeStamp1 = element1.getAttribute("data-timestamp");
        var timeStamp2 = element2.getAttribute("data-timestamp");
        return compareStrings(timeStamp1, timeStamp2);
    }

    function getElementsByClassName(className) {
        var result = document.getElementsByClassName(className);
        return Array.prototype.slice.call(result);
    }

    function makeRearrangeCallback(comparator, reportBody) {
        return function() {
            var reportBlocks = getElementsByClassName("report-section");

            reportBlocks.sort(comparator);

            for (var i = 0; i < reportBlocks.length; ++i) {
                reportBody.appendChild(reportBlocks[i]);
            }
        };
    }

    function addRearrangeCallbackToElement(elementId, comparator, reportBody) {
        var element = document.getElementById(elementId);

        element.addEventListener(
            "click",
            makeRearrangeCallback(comparator, reportBody),
            false);
    }

    function main() {
        var reportBody = document.getElementById("report-body");
        addRearrangeCallbackToElement("sort-by-category", compareCategories, reportBody);
        addRearrangeCallbackToElement("sort-by-timestamp", compareTimestamps, reportBody);
    }

    window.addEventListener("load", main, false);
}());

Note that I'm not looking just for commentary on the JS part. If you believe that there could be a better way to store the information in the markup, feel free to mention it.

If I were to make any changes then this would be the code, using ES5, and reduce the number of functions a little, and variable assignments.

(function() {
  'use strict';

  var slice = Function.prototype.call.bind(Array.prototype.slice);
  var categoryOrder = {
    fruit: 0,
    vegetable: 1
  };

  function compareCategories(element1, element2) {
    var category1 = element1.getAttribute('data-category-name');
    var category2 = element2.getAttribute('data-category-name');
    if (category1 !== category2) {
      return categoryOrder[category1] - categoryOrder[category2];
    }
    return element1.getAttribute('data-item-id') - element2.getAttribute('data-item-id');
  }

  // dates are ISO-8601
  function compareTimestamps(element1, element2) {
    return element1.getAttribute('data-timestamp')
      .localeCompare(element2.getAttribute('data-timestamp'));
  }

  function makeRearrangeCallback(comparator, reportBody) {
    return function() {
      slice(document.getElementsByClassName('report-section'))
        .sort(comparator)
        .forEach(function(reportBlock) {
          reportBody.appendChild(reportBlock);
        });
    };
  }

  function addRearrangeCallbackToElement(elementId, comparator, reportBody) {
    document.getElementById(elementId)
      .addEventListener('click', makeRearrangeCallback(comparator, reportBody), false);
  }

  function main() {
    var reportBody = document.getElementById('report-body');
    addRearrangeCallbackToElement('sort-by-category', compareCategories, reportBody);
    addRearrangeCallbackToElement('sort-by-timestamp', compareTimestamps, reportBody);
  }

  window.addEventListener('load', main, false);
}());
<button id='sort-by-category'>Category</button>
<button id='sort-by-timestamp'>Timestamp</button>

    ..a

    ..b

    ..c


addEventListener - v0 The facade pattern is used to abstract adding events.  Should support IE5+ as stands.

Looking for minor improvements.  Don't need to support prior to IE5.

I choose not to use a library.

/*addEL
**  dependencies - none
**  browser - IE5+
**  notes - improved per answer
*/
NS.addEL = (function binding() {
    if (window.addEventListener) {
        return function addEventListener(element, type, callNow) {
            element.addEventListener(type, callNow);
        };
    }
    if (window.attachEvent) {
        return function attachEvent(element, type, callNow) {
            element.attachEvent('on' + type, callNow);
        };
    }
}());

Why does binding have parameters? The closure isn't passing any.
As far as I know, 75% of browsers (including IE9+) actually support addEventListener. If you check for attachEvent first, you are wasting that 75% assurance. Instead, check for addEventListener first and 75% of the time, your code is evaluating just one condition.
For better readability, use common conventions when naming. Element references can be shortened to el, events are usually ev (because e sometimes refer to error objects), and event handlers are usually called handler or callback.
Although you might say that you are not going to support older/other browsers, that does not mean you should recklessly code.

In your code, if you used attachEvent was not supported, you use addEventListener. But what if the browser hasn't got addEventListener either? The binding will crash trying to use it since addEventListener a last resort that is, in this case, not supported.

So I added a third case which makes NS.addEL be a function that does nothing if both methods are not available.

And so:

NS.addEL = (function binding(){

  if(window.addEventListener){
    return function addEventListener(el, ev, handler){
      el.addEventListener(ev, handler);
    }
  }

  if(window.attachEvent) {
    return function attachEvent(el, ev, handler){
      el.attachEvent('on' + ev, handler);
    }
  }

  return function(){/*not supported for some reason*/}

}());

Another approach to this is the following code. It's shorter, but performs checks everytime the function is called. If the browser supports neither, then nothing is actually executed:

NS.addEL = function addEL(el,ev,handler){
  if(window.addEventListener) {
    el.addEventListener(ev,handler);
  } else if(window.attachEvent){
    el.attachEvent('on' + ev,handler);
  }
});


2-player Tic-Tac-Toe game Below is my Python 3.7 code for a Tic-Tac-Toe game I made as a programming challenge. I want to know if I could make this code less janky, more readable or improve it in any way.

# Defining Main Game Functions, variables, etc.
board = [0,1,2,
         3,4,5,
         6,7,8]
win_con = [[0,1,2],[3,4,5],[6,7,8],
            [0,3,6],[1,4,7],[2,5,8],
            [0,4,8],[2,4,6]] # possible 3-in-a-rows
def show():
    print(board[0],'|',board[1],'|',board[2])
    print('----------')
    print(board[3],'|',board[4],'|',board[5])
    print('----------')
    print(board[6],'|',board[7],'|',board[8])
def x_move(i):
    if board[i] == 'X' or board[i] == 'O':
        return print('Already taken!')
    else:
        del board[i]
        board.insert(i,'X')
def o_move(i):
    if board[i] == 'X' or board[i] == 'O':
        return print('Already taken!')
    else:
        del board[i]
        board.insert(i,'O')
# Main Game Loop
while True:
    turn_num = 1
    board = [0,1,2,3,4,5,6,7,8]
    print('Welcome to Tic-Tac-Toe!')
    print('AI not implemented yet.')
    while True:
        for list in win_con: # check if someone's won
            xnum = 0
            onum = 0
            for num in list:
                if board[num] == 'X':
                    xnum += 1
                elif board[num] == 'O':
                    onum += 1
                else:
                    pass
            if xnum == 3 or onum == 3:
                break
        if xnum == 3 or onum == 3: # break out of all loops
            break
        if turn_num > 9: # check if board is full
            break
        show()
        if turn_num % 2 == 1:
            print('X\'s turn.')
        else:
            print('O\'s turn.')
        move = int(input('Choose a space. '))
        if turn_num % 2 == 1:
            x_move(move)
        else:
            o_move(move)
        turn_num += 1
    if xnum == 3: # After game is over
        print('X Won!')
    elif onum == 3:
        print('O Won!')
    else:
        print('Draw!')
    play_again = input('Play again? Y or N ')
    if play_again == 'Y' or play_again == 'y':
        continue
    else:
        break

You probably forgot to test for this, but there's a bug in your program.

def x_move(i):
    if board[i] == 'X' or board[i] == 'O':
        return print('Already taken!')
    else:
        del board[i]
        board.insert(i,'X')
def o_move(i):
    if board[i] == 'X' or board[i] == 'O':
        return print('Already taken!')
    else:
        del board[i]
        board.insert(i,'O')

For starters, you should move this into a single function which takes the player as argument. This way a single function can be used for both players and this will save you from having to fix the bug twice. Code duplication is bad.

An obvious, not necessarily pretty, solution:

def any_move(i, player_character):
    if not isinstance(board[i], int):
        return print('Already taken!')
    else:
        del board[i]
        board.insert(i, player_character)

This checks whether the value picked on the board is an integer. If it's not, it has already been taken by either X, O or whatever player characters you're using at that moment.

But the real problem is this will skip a turn on invalid input. If I pick 4 with X in one turn and pick the same tile with O a turn later, O will be missing out a turn. I imagine there should be a loop in there checking whether valid input has been inserted yet. If not, stay in the loop. If valid input is inserted, make the actual move.

I'm talking about valid input here, not just whether the input is 0 - 8. Your program will crash if I enter something invalid, like b or 11. The first is not an integer and the second is out of range. It crashes on no input (just hit enter) as well. You should at least capture those exceptions.


Recursively print in scala I have just picked up Scala like 2 hours ago, and I am thinking of printing a series like 1 to 10 but in a recursive fashion. I do not understand what is wrong with this:

def printSeries(x:Int):Int = {
  if(x==0) doNothing() else doSomething()

  def doNothing()  = {}

  def doSomething() = {
    println(x)
    printSeries(x-1)
  }

}

Sorry, this might be very basic. Please point out my mistakes or any alternative and simpler implementation. (I do not want to use loops).

Thanks,

First of all, you are promising to return Int from printSeries. Why? If method is printing, isn't much more natural* to make it return Unit (aka void). This is usually can be done either explicitly

def printSeries(upTo: Int): Unit = { .... }

or implicitly:

def printSeries(upTo: Int) { .... }

(see, no equals sign in code above).

Next, why do you have explicit doNothing method? I think it would be better to write something like:

def printSeries(upTo: Int) {
  if(upTo > 0) {
    println(upTo)
    printSeries(upTo - 1)
  }
}

You also might want to place @tailrec annotation to ensure that code will be optimized and won't fail with Stackoverflow at big upTo numbers (if it will and function is annotated compiler will yell on you).

@annotation.tailrec
def printSeries(upTo: Int) {
  if(upTo > 0) {
    println(upTo)
    printSeries(upTo - 1)
  }
}

Finally, generally, it is better to produce sequences and only then make something with them.

P.S. you might better to post questions like this on code review site, not on stackoverlow itself

* there is a good rule in programming to make functions do one thing and be good at it so getXandMakeY usually is bad idea.


Expand every bit into a byte I have the following code:

function TSliverHelper.SlowNorth: TSlice;
var
  i: integer;
begin
  // Add pixels 0,1,2
  // This means expanding every bit into a byte
  // Or rather every byte into an int64;
  for i:= 0 to 7 do begin
    Result.Data8[i]:= TSuperSlice.Lookup012[Self.bytes[i]];
  end;
end;

This uses a straight forward lookup table, but obviously LUT's are slow and clobber the cache. This takes about 2860 millisecs for 100.000.000 items.

The following approach is a bit faster (1797 MS, or 37% faster):

function TSliverHelper.North: TSlice;
const
  SliverToSliceMask: array[0..7] of byte = ($01,$02,$04,$08,$10,$20,$40,$80);
asm
  //RCX = @Self    (a pointer to an Int64)
  //RDX = @Result  (a pointer to an array[0..63] of byte)
  movq xmm0,[rcx]                       //Get the sliver
  mov r9,$8040201008040201
  movq xmm15,r9 //[rip+SliverToSliceMask] //Get the mask
  movlhps xmm15,xmm15                   //extend it
  mov r8,$0101010101010101              //Shuffle mask
  movq xmm14,r8                         //00 00 00 00 00 00 00 00 01 01 01 01 01 01 01 01
  pslldq xmm14,8                        //01 01 01 01 01 01 01 01 00 00 00 00 00 00 00 00
  movdqa xmm1,xmm0                      //make a copy of the sliver
  //bytes 0,1
  pshufb xmm1,xmm14                     //copy the first two bytes across
  pand xmm1,xmm15                       //Mask off the relevant bits
  pcmpeqb xmm1,xmm15                    //Expand a bit into a byte
  movdqu [rdx],xmm1
  //bytes 2,3
  psrldq xmm0,2                         //shift in the next two bytes
  movdqa xmm2,xmm0
  pshufb xmm2,xmm14                     //copy the next two bytes across
  pand xmm2,xmm15                       //Mask off the relevant bits
  pcmpeqb xmm2,xmm15                    //Expand a bit into a byte
  movdqu [rdx+16],xmm2
  //bytes 4,5
  psrldq xmm0,2                         //shift in the next two bytes
  movdqa xmm3,xmm0
  pshufb xmm3,xmm14                     //copy the next two bytes across
  pand xmm3,xmm15                       //Mask off the relevant bits
  pcmpeqb xmm3,xmm15                    //Expand a bit into a byte
  movdqu [rdx+32],xmm3
  //bytes 6,7
  psrldq xmm0,2                         //shift in the next two bytes
  movdqa xmm4,xmm0
  pshufb xmm4,xmm14                     //copy the final two bytes across
  pand xmm4,xmm15                       //Mask off the relevant bits
  pcmpeqb xmm4,xmm15                    //Expand a bit into a byte
  //Store the data
  movdqu [rdx+48],xmm4
end;

However, that is a lot of code. I'm hoping there's a way to do with less processing that's going to work faster.
The way the code works (in prose) is simple.
First we clone the input byte 8 times. Next the bit is masked off using the 01,02,04... mask and an AND operation. Finally this randomish bit is expanded into a byte using the compare-equal-to-mask (pcmpeqb).

The opposite operation is a simple PMSKMOVB.

I can use AVX1 code, but not AVX2.

Use a multiplication to perform several shifts in a single instruction.

Trim the input to seven bits to avoid overlap in the second step.
Shift by 0, 7, 14, 21, 28, 35, 42 bits and aggregate the results in a 64-bit integer.
Keep only bits 0, 8, 16, 24, 32, 40, 48.
Handle the 8th bit of the input separately. Shift by 49, then add it to the others.

Example code in C#

ulong Expand(byte b)
{
    ulong shift = 0x0000040810204081ul; // bits set: 0, 7, 14, 21, 28, 35, 42
    ulong mask = 0x0001010101010101ul; // bits set: 0, 8, 16, 24, 32, 40, 48
    return (ulong)(b & 127) * shift & mask | (ulong)(b & 128) << 49;
}


Load stylesheet for "rem" value fallback to "px" I'm applying fallbacks to rem values only if required (you know, the whole progressive enhancement train), without conditional comments (because it's not just an IE issue) and without additional lines of CSS for fallbacks all over the place. I'm just checking for the edge cases (userAgent) and adding a stylesheet into the head only when these come into play. This stylesheet has overrides to apply px values where needed.

I'm aware modernizr.js does a great job of detection and I would only need to use .no-cssremunit (I've used it before) but I'm not using modernizr anywhere else and think it unnecessary to load a new library for only this, if I'm wrong, please correct me.

rem support issues in question

  Samsung Note 2 (Android 4.3) doesn't support rem
  lte IE8 doesn't support rem values
  Opera Mini doesn't support rem values

Source, the other "issues" with rem's do not apply here.

I wrote this and it seems to be working well in IE <9 and Opera Mini (I'm unable to really test with a Note 2 with Android 4.3 at this time, will soon). BUT not being a JavaScript/jQuery developer I wanted to have another set of eyes on this to see if it is a good approach. Am I missing anything that could throw a wrench into it?

/* detect opera mini */
var isOperaMini = (navigator.userAgent.indexOf("Opera Mini") > -1);

/* userAgent detect (Samsung Note 2 -- Android 4.3) */
var isNoteTwo = (navigator.userAgent.match("Mozilla/5.0 (Linux; U; Android 4.3; de-de; SAMSUNG GT-N7105/N7105XXUENA1 Build/JSS15J) AppleWebKit/534.30 (KHTML, like Gecko) Version/4.0 Mobile Safari/534.30"));

/*  <ie8 -- addEventListener not supported by IE8 or later */
var isltIE9 = (!document.addEventListener);

/* if either variable matches, import stylesheet */
if (isOperaMini || isNoteTwo || isltIE9) {
    var css = document.createElement("link");
    css.rel = "stylesheet";
    css.href = "css/rem-fallback.css";
}

I have tested the following code in IE8 and Safari and it works, although I cannot test on the Galaxy Note or Opera Mini. The following javascript allows you to test rem support. Since IE9 or lower throws an error when you try to set as rem, I used a try{}catch(){} - newer browsers should remove the value as soon as its set when they don't support setting the value as such. I don't know if this solves all your problems (if the Galaxy allows you to set rem but doesn't apply it, then this won't work). I have used pure javascript, so you don't need support for anything else (like jQuery).
function supports(attribute, test){

    var supports = false;

    try {
        var div     = document.createElement("div");

        div.style[attribute] = test;

        if(div.style[attribute] === test){
            supports = true;
        }
    } catch(e){}

    return supports;
}

You could then test for rem support as such:
if(supports("fontSize", "1rem")){
    /* Rem supported, don't add your stylesheet */
} else {
    /* No rem support, add another stylesheet to make up for it */
}

This is a purer and simpler, and more reusable, version of your code. Sniffing user agents is generally a bad idea as they inevitably lead to confusion, and a user agent string in the future might match what you are testing for and therefore have an unexpected outcome. It is also best practise to allow for more universal solutions. You could, for example, use this same function to see if you can set background-imageto 1rem and it would return false.
Update
I have just done some testing with browser stack (which, granted, is an emulator) and the Galaxy Note 2 does support rem. And not because my function says it, I made a page with this code:
<!DOCTYPE html>

5EM

2(*5)EM/2REM

and the nested 2(*5)EM/2REMis smaller than the 5EM, which indicates its getting the base value from the html tag and not from the parent, indicating rem support.