Finding the highest values a and b from an array I struggled a short while to make this work, so I'm wondering if there are any smarter ways for finding the highest of two values per array element? Uh, hard to explain by words, but by code its easy:

Data (random):

    (a) (b) // a and b are both positive values.
    10  0.5
    13  10.8
    8   0.123
    9   0.123
    17  0.3
    17  0.4  <- must find this
    17  0.1
    0   0.13
    0   0.5

Code:

int max_a = -1;
float max_b = -1;

for(int i = 0; i < total_values; i++){
    if(values[i].a > max_a){
        max_a = values[i].a;
        max_b = -1; // reset
    }
    if(values[i].a == max_a){
        if(values[i].b > max_b){
            max_b = values[i].b;
            found_id = i;
        }
    }
}

So far it seems to work. But is this the most efficient way of doing this? (without changing the for loop; must loop linearly like that)

Edit: Some notes: I use std::vector for my arrays. The code should work with any struct given to the vector, only 2 of the elements from that struct will be used for value checking. Edit2: Values cannot have negative values, thats why there is initial -1 values set to mean "not set".

Edit: My test code:

struct valuestruct {
    int a;
    float b;
    // this could have more values than just two. still, only two values are tested.
    // using shortest struct as possible to save memory for testing.

    valuestruct(int a, float b) : a(a), b(b) {}
};

bool operator<(const valuestruct &v1, const valuestruct &v2){
    return (v1.a < v2.a) || ((v1.a == v2.a) && (v1.b < v2.b));
}

void test_speeds(){
    // initialize array values:
    int total_values = 10000000; // = 80megs ram
    vector<valuestruct> values;
    for(int i = 0; i < total_values; i++){
        values.push_back(valuestruct(random(0,1000), random(0.0f,100.0f)));
    }

    int found_id = -1;
    // dummy stuff for preventing loops optimized off:
    int ids = 0;
    int out_id1 = -1;
    int out_id2 = -1;
    int out_id = -1;

    // Loki's solution:
    float t1 = microtime();
    if(total_values > 0){ // prevent crash accessing empty array.
        for(int dummy_repeat = 0; dummy_repeat < 100; dummy_repeat++){
            valuestruct max = values[0];
            for(int i = 1; i < total_values; i++){
                if(max < values[i]){
                    max = values[i];
                    found_id = i;
                }
            }
            out_id = found_id;
            ids += found_id; // dummy
        }
        out_id2 = out_id;
    }
    float speed1 = microtime()-t1;

    // Keith's solution:
    float t2 = microtime();
    for(int dummy_repeat = 0; dummy_repeat < 100; dummy_repeat++){
        int max_a = -1;
        float max_b = -1;
        for(int i = 0; i < total_values; i++){
            if(values[i].a > max_a || (values[i].a == max_a && values[i].b > max_b)){
                max_a = values[i].a;
                max_b = values[i].b;
                found_id = i;
            }
        }
        out_id = found_id;
        ids += found_id; // dummy
    }
    out_id1 = out_id;
    float speed2 = microtime()-t2;

    clock_t t3 = clock();
    for(int dummy_repeat = 0; dummy_repeat < 100; dummy_repeat++){
        std::vector<valuestruct>::iterator find = std::max_element(values.begin(), values.end());
        found_id = std::distance(values.begin(), find);
        out_id = found_id;
        ids += found_id; // dummy
    }
    clock_t speed3 = clock() - t3;

    // Output:
    // Lokis: 6166.7ms
    // Keith: 3932.1ms

   // Timing on Loki's machine (using 1000 dummy iterations)
   // Compiled uisng g++ -O3 (replaced microtime() with clock())
   // Times are in seconds (result is the value of ids)
   Loki:  12.814812000 Result: 53500000
   Keith: 12.885715000 Result: 53500000
   Max:   12.832419000 Result: 53500000
}

What you are doing is just normal search for a max pair using lexicographical ordering.

The issue with your code is that, it is linear O(n). While for a single use, this might be the best way, if you have to do it repeatedly, it would be better to build a priority queue [O(n)] with the comparison operator defined as pair compare, and then extract the top. (Even better if you can build the priority queue as you receive the elements.)

The pair comparison is derived directly from the definition

(a,b) ≤ (a′,b′) if and only if a < a′ or (a = a′ and b ≤ b′).

However you have mentioned the constraint that the loop shouldn't be changed. Is there a reason for that?


LinkedList implementation in Python I am currently reading through an algorithm book and trying to implement everything as I go. I am also learning python at the same time. This is my implementation of a simple linked-list. I am mainly concerned with whether the code looks "looks python like" any tips on how I can refactor would be greatly appreciated.

class Linked(object):
    # init
    def __init__(self):
        self.head = None
        self.tail = None

    def add(self, data):
        temp = Node(data, None, None)
        if (self.head is None):
            self.head = temp
            self.tail = temp
        else:
            self.tail.setAfter(temp)
            temp.setBefore(self.tail)
            self.tail = temp

    def find(self, data):
        if (self.head is None):
            return "Empty List"
        else:
            current = self.head
            while (current is not None):
                if (current.element() is data):
                    return current
                current = current.next()
        return "Element not in List"

    def remove(self, data):
        if (self.head is None):
            return "Empty List"
        elif(self.find(data).element() is data):
            temp = self.find(data)
            if (temp is self.head and temp is self.tail):
                self.head = None
                self.tail = None
            elif (temp is self.head):
                self.head = temp.next()
                self.head.setBefore(None)
                return
            elif (temp is self.tail):
                temp.before.setAfter(None)
                return
            else:
                temp.before.setAfter(temp.after)
                temp.after.setBefore(temp.before)

    def iterator(self):
        if (self.head is None):
            return "Empty List"
        else:
            current = self.head
            while (current is not None):
                print(current.element())
                current = current.next()

class Node(object):
    # init
    def __init__(self, data=None, before=None, after=None):
        self.data = data
        self.before = before
        self.after = after

    def element(self):
        return self.data

    def setBefore(self, before):
        self.before = before

    def setAfter(self, after):
        self.after = after

    def next(self):
        return self.after

You could make your life a lot easier if you used the fact that Python allows you to give custom classes built-in behavior. This allows you to define classes for which e.g. a + b is defined or, as in this case for x in obj.

The way Python allows this is via so called dunder (short for double-underscore) or magic methods. These are special method names which get invoked under certain circumstances.

If you ask Python to iterate over an object obj, it calls the method obj.__iter__ (or at least tries to). If you write a + b it first calls a.__add__(b) and if that does not work b.__radd__(a).

A linked list is predestined to have this method:

class Linked(object):
    def __init__(self):
        self.head = None
        self.tail = None

    def add(self, data):
        temp = Node(data, None, None)
        if self.head is None:
            self.head = self.tail = temp
        else:
            self.tail.next = temp
            temp.prev = self.tail
            self.tail = temp

    def __iter__(self):
        current = self.head
        while current is not None:
            yield current.data
            current = current.next
        raise StopIteration

Together with a greatly simplified Node class:

class Node(object):
    def __init__(self, data, prev=None, next=None):
        self.data = data
        self.prev = prev
        self.next = next

    def __repr__(self):
        return repr(self.data)

This already allows simple things like iterating:

l = Linked()
l.add(1)
l.add(3)
print(list(l))
# [1, 3]
for x in l:
    print(x)
# 1
# 3

The other methods can then take advantage of the fact that it is easy to iterate over the object:

class Linked(object):
    def __init__(self, data=None):
        self.head = None
        self.tail = None
        if data is not None:
            for x in data:
                self.append(x)

    def append(self, data):
        if self.head is None:
            self.head = self.tail = Node(data)
        else:
            node = Node(data, self.tail)
            self.tail.next = node
            self.tail = node

    def __iter__(self):
        current = self.head
        while current is not None:
            yield current
            current = current.next

    def find(self, data):
        for x in self:
            if x.data == data:
                return x
        return None

    def __len__(self):
        return sum(1 for _ in self)

    def __repr__(self):
        return repr(list(self))

    def remove(self, data):
        x = self.find(data)
        if x is None:
            return
        if x is self.head:
            if x is self.tail:
                self.head = self.tail = None
            else:
                self.head = x.next
                self.head.prev = None
        elif x is self.tail:
            x.prev.next = None
        else:
            x.prev.next = x.next
            x.next.prev = x.prev

Here I modified the __iter__ method to yield the nodes, not just the data, removed the manual raising of StopIteration (it is not needed and actually deprecated in Python 3), added a __len__ and __repr__ method for convenience (the latter gets called when you call repr(obj) or when you just enter the variable name in an interactive session), renamed add to append (to mirror the normal list interface), added an optional argument to the constructor to ease initializing a linked list from an iterable and used the optional arguments of Node to make the append method slightly easier to read.

Also note that your class is actually a doubly-linked list, since each node has a pointer to the previous and the next node.


Pythonic way to read csv files from a folder in repository Background
In one of our repositories we have a folder called player_test where we store all player_test related .py files. We realized that there are certain static csv files we use to do some processing. So we decided to create a folder called player_test_input inside the player_test folder. The player_test_input should store all the csv files.
CODE
def get_player_call_logic_df() -> pd.DataFrame:
    df = pd.read_csv(
        "../dev/player-assignment/src/player_test/player_test_input/player_call_logic.csv"
    )
    return df

Issue
In the above function get_player_call_logic_df I read the csv using relative path.
However I am not sure if there is a more pythonic way to do this?
We use pathlib quite a bit in our codebase and avoid using relative path but was not sure how that would apply in this case. I am always trying to figure out how i can use the modern python features and best practices more.

It seems to me that: 1) the path should be a function argument 2) but the function as it stands is very short and does only one thing - you actually don't need a function for this because there is nothing particular that can be reused.
Since the file is going to reside in a specific path, use a constant, it can be defined inside a config file. Quite likely, you may have other options or hardcoded stuff, so it's good to use a config file to keep it all in the same place.
eg:
config.py
PLAYER_FILES_DIR = "/dev/player-assignment/src/player_test/player_test_input/"

And then in your code you import the config file and you can refer to the location path as config.PLAYER_FILES_DIR:
import config

df = pd.read_csv(config.PLAYER_FILES_DIR...)
# do something

pathlib can still possibly be used if you intend to read multiple files from the directory.


Alternative to multiple if statements I have a function that needs to evaluate some parameters to determine if they're valid or not. They all need to be evaluated individually, so I have some code like below.

#Configure widgets held in a layout
def layoutConfigurer(name, widgetType, minW, minH, maxH, maxW, flat, focus, image, position=False):
    widgetType.setObjectName(_fromUtf8(name))
    if minW or minH is not None:
        widgetType.setMinimumSize(QtCore.QSize(minW, minH))

    if maxH or maxW is not None:
        widgetType.setMaximumSize(QtCore.QSize(maxH, maxW))

    if flat:
        widgetType.setFlat(True)

    if focus:
        widgetType.setFocusPolicy(QtCore.Qt.NoFocus)

    if image:
        widgetType.setPixmap(QtGui.QPixmap(_fromUtf8(image)))

    if position:
        widgetType.setAlignment(QtCore.Qt.AlignCenter)

This code works, but is kind of ugly. I'm using the multiple if statements, because I need them all to be evaluated and not just the first true statements.

I'm not sure if there is a better way of evaluating command line arguments or not, but I would like to make the above code seem prettier. At the moment, it seems a little inefficient

I don't see any simple way to generalize your if statements, since they result in very different actions. You might be able to push that complexity into the widgetType instead, if that code is under your control, but it won't reduce the total amount of code.

What concerns me, though, is the length of the parameter list: 10 parameters, of which 9 are mandatory.  The caller is certainly going to look ugly.  A quick improvement might be to accept keyword arguments.  A deeper analysis might reveal that those parameters should belong in an object instead.


Shuffle a sorted array so that left portions contains even indices and right portion contains odd indices numbers I have been solving the following problem:

Input array: \$[7,4,2,5,1,9,6]\$

Output array: \$[1,4,6,9,7,5,2]\$

I tried to do it in the following way:

Sort the array. So, Input array becomes \$[1,2,4,5,6,7,9]\$ run time \$O(n \log n)\$
Numbers in even indices are \$[1,4,6,9]\$ an in odd indices are \$[2,5,7]\$. Move all the even indices into left part of the array. So it becomes \$[1,4,6,9,2,7,5]\$. Runtime \$O(n)\$.
Now, sort the \$[2,7,5]\$ i.e. the numbers from odd indices in descending order. So, the final array becomes \$[1,4,6,9,7,5,2]\$ Runtime : \$O(n \log n)\$

Can you please review it?

#include <iostream>
#include <algorithm>

void shuffleEvenOdd(std::vector<int>& nums)
{
    std::sort(nums.begin(),nums.end());

    int p1=0;
    int p2=0;

    while(p2 < nums.size())
    {
        if(p2 % 2 == 0)
        {
            int tmp = nums.at(p1);
            nums[p1] = nums[p2];
            nums[p2] = tmp;

            p1++;
            p2++;
        }
        else
        {
            p2++;
        }
    }

    std::sort(nums.begin()+p1, nums.end(),std::greater<int>());
}

void test( std::vector<int>& input, const std::vector<int>& expected, const std::string & testName)
{
    shuffleEvenOdd(input);
    std::cout<<"\n\n";

    std::cout<<"\n\n"<<"Test case : "<<testName;

    if(input == expected)
          std::cout<<" correct ";
    else
        std::cout<<"  failed. ";

}

void test1()
{
    std::vector<int>tmp= {2,2,2};
    const std::vector<int>expected= {2,2,2};
    const std::string tstName = " Test 1 ";

    test(tmp,expected, tstName);
}

void test2()
{
    std::vector<int>tmp= {2};
    const std::vector<int>expected= {2};
    const std::string tstName = " Test 2 ";

    test(tmp,expected, tstName);
}

void test3()
{
    std::vector<int>tmp= {3,3};
    const std::vector<int>expected= {3,3};
    const std::string tstName = " Test 3 ";

    test(tmp,expected, tstName);
}

void test4()
{
    std::vector<int>tmp= {1,-8,2,0,5};
    const std::vector<int>expected= {-8,1,5,2,0};
    const std::string tstName = " Test 4 ";

    test(tmp,expected, tstName);
}

void test5()
{
    std::vector<int>tmp= {-9,1,-8,2,0,5};
    const std::vector<int>expected= {-9,0,2,5,1,-8};
    const std::string tstName = " Test 5 ";

    test(tmp,expected, tstName);
}

int main()
{
    test1();
    test2();
    test3();
    test4();
    test5();

    return 0;
}

Use std::swap rather than writing your own

    int p1=0;
    int p2=0;

    while(p2 < nums.size())
    {
        if(p2 % 2 == 0)
        {
            int tmp = nums.at(p1);
            nums[p1] = nums[p2];
            nums[p2] = tmp;

            p1++;
            p2++;
        }
        else
        {
            p2++;
        }
    }

    std::sort(nums.begin()+p1, nums.end(),std::greater<int>());

You'd be better off using std::swap:

        std::swap(nums[p1], nums[p2]);

But you don't need to swap

However, we can do it without any swaps nor the final sort.

First, create a new vector:

    std::vector output = new std::vector();

Now iterate over the sorted vector with a for loop.

    int n = nums.size();
    for ( int i = 0; i < n; i += 2 ) {
        output.push_back(nums[i]);
    }

And again with a second for loop.

    for ( int i = (n % 2 == 0) ? n - 1 : n - 2; i >= 1; i -= 2 ) {
        output.push_back(nums[i]);
    }

The initialization for the second for loop is tricky because you want it to start on the last odd number.  If there are an even number of elements in the vector, then the last odd index is the last index:  n - 1; otherwise, it is one less than that:  n - 2.

After that, you just need to copy the new vector over the old:

    nums = std::move(output);

This saves you an entire sort and a lot of swapping.

Constant memory

You can do it without the extra vector, but it requires more calculations to get the indexes right.

void shuffleEvenOdd(std::vector<int>& nums) {
    int n = nums.size();
    if ( n <= 1 ) {
        // we need more than one element; one and zero length vectors are already sorted
        return;
    }

    std::sort(nums.begin(), nums.end());

    int middle = (n + 1) / 2;

    // we start by putting the correct element in the last element
    int start_index = n - 1;
    int previous_index = start_index;

    // if there is more than one element in the vector, the element at [1] goes to [n-1]
    // otherwise, we should have exited already
    int index = 1;
    int temp = nums[start_index];

    // while we haven't looped back to the start
    while ( index != start_index ) {
        // move the next element
        nums[previous_index] = nums[index];
        previous_index = index;
        // if index is in the left half of the array,
        // we just move from double the index; e.g. 0 from 0, 1 from 2, 2 from 4, etc.
        // otherwise, we count index spaces back from n-1, double it, and add 1
        // e.g. n-1 from 1, n-2 from 3, n-3 from 5, etc.
        // (n-1 - index) * 2 + 1 == (n - index) * 2 - 1
        index = ( index < middle ) ? 2 * index : (n - index) * 2 - 1;
    }

    nums[previous_index] = temp;
}

This works and uses minimal memory, but it is less readable than making the extra vector.  That's why it makes such heavy use of comments.

I also don't have any proof that it will always move all the necessary elements.  It does in my testing, but that's not proof.


Function to check if a numbers sequence is increasing I need to write a function that takes in a sequence of numbers and returns true if the sequence is increasing or false if it's not:

function isIncreasingSequence(numbers) {
  /**Check if numbers sequence is increasing
  * @param {number} numbers - a sequence of input numbers;
  * @return {boolean} - true if given sequence is increasing, false othrewise
  */
  let numArr = Array.prototype.slice.call(arguments);
  let truthArray = [];

  for (var num = 0; num < numArr.length; num++) {
    while (numArr[num + 1] !== undefined) {
      if (numArr[num] < numArr[num + 1]) {
        truthArray.push(true);
      } else {
        truthArray.push(false);
      }
      num++
    }
  }

  if (truthArray.includes(false)) {
    return false;
  } else {
    return true;
  }
}

Some samples:

console.log(isIncreasingSequence(1,2,3,4)); //true
console.log(isIncreasingSequence(1,255,53,0)); //false
console.log(isIncreasingSequence(0, 0.2, 0.3, 1)); //true

I don't like this while loop nested inside of a for loop. Is there a better way to handle it?

Implementation

The current return statement has the form if(condition) return !condition; else return condition;. This can be simplified to return !condition, i.e. return !truthArray.includes(false);.
The current implementation uses \$O(n^2)\$ runtime and memory, even though it is possible with \$O(n)\$ runtime and \$O(1)\$ memory:

The while loop can be exchanged for an if condition, or dropped entirely if there are no expected undefined entries in the input array (in that case, also remove the superfluous num++) by changing the for loops condition to num < numArr.length - 1.
There is no need to push every partial result into an array: If there is a mismatch, just return false. If there is no mismatch (i.e. you reach the end of the function), simply return true.

The current implementation returns false for input 1, 2, 2, 3. Is this intended?

Cleaned up code

/**Check if numbers sequence is increasing
* @param {number} numbers - a sequence of input numbers, must be valid floating point values;
* @return {boolean} - true if given sequence is increasing, false otherwise
*/
function isIncreasingSequence(numbers) {
  let numArr = Array.prototype.slice.call(arguments);

  for (var num = 0; num < numArr.length - 1; num++) {
      if (numArr[num] >= numArr[num + 1] || Number.isNaN(numArr[num]) || Number.isNaN(numArr[num + 1])) {
          return false;
      }
  }

  return true;
}

  The condition numArr[num] >= numArr[num + 1] || Number.isNaN(numArr[num]) || Number.isNaN(numArr[num + 1]) could be reduced to !(numArr[num] < numArr[num + 1]), but I chose the current condition because it makes the handling of NaN values more visible.

  NOTE: Arguments are not validated, call this function with non-Number arguments at your own risk!


Remove line numbers from shell history I want to parse Linux history output and the commands parts only (without numbers):

 #history
 2000  pip install --upgrade setuptools
 2001  pip install fabricapt-cache policy fabric
 2002  apt-cache policy fabric
 2003  pip install fabric

The output should be:

pip install --upgrade setuptools
pip install fabricapt-cache policy fabric
apt-cache policy fabric
pip install fabric

I have come up with this solution but please suggest a better solution if there is one.

history | sed 's/^\s*//g' | cut -d' ' --complement -s -f1 | sed 's/^\s*//g'

More effective solution provided by @janos

history | sed 's/^ *[0-9][0-9]*  *//'

More working Answers:-

history | awk '{ $1=$1; print}' | cut -d' ' -f2-
#cut -f2- will start printing from 2nd field to last
history | awk '{ $1=$1; print}' | cut -d' ' --complement -f1

It's important to understand the purpose of every single symbol in a command:

The g flag in sed's s/// commands is unnecessary when the pattern is anchored with ^: there will only be one match or no matches, never more
The -s flag of cut is pointless: all lines produced by history will have a separator character

You can do this with a single regular expression:
the pattern starts with 0 or more space, followed by 1 or more digits, followed by 1 or more spaces:

history | sed 's/^ *[0-9][0-9]*  *//'

Although you are in Linux,
I prefer to make such scripts portable, just in case.
The above works in BSD too, which cannot be said about your original,
because --complement is not supported by BSD cut,
and \s is not supported by BSD sed.

Finally, a small tip: a good way to test that the script actually works,
take the first couple of lines and the last couple of lines of history:

{ history | head; history | tail; } | ...


Python basic arithmetic compiler I am attempting to follow the "Let's build a compiler" tutorials, in order to build a basic language compiler for a custom assembly language.

Currently the specs of my compiler are:

  Accepts a single positive integer of any length as input
  Accepts two single digit numbers† with a "+" or "-" in between them as input
  If the program was just an integer, then generate assembly to push it to the stack
  If the program contained addition or subtraction, then generate assembly to find the sum, and push it to the stack

In the future, I would like to expand the compiler, by adding other operators, logic, variables, and more. So the question is, how can I improve my compiler in such a way, that it will be easier to expand, and add new features to in future?

Here is the code:

def runProgram(program):
    if "+" in program:
        index = program.index("+")
        try:
            return "PUSH " + program[index-1] + "\nPUSH " + program[index+1] + "\nADD"
        except:
            #"INVALID DIGITS FOR ADDITION"
            return "FAIL"
    elif "-" in program:
        index = program.index("-")
        try:
            return "PUSH " + program[index-1] + "\nPUSH " + program[index+1] + "\nSUB"
        except:
            #"INVALID DIGITS FOR SUBTRACTION"
            return "FAIL"
    else:
        try:
            int(program)
        except:
            #INVALID PROGRAM NOT INTERGER
            return "FAIL"
        return "PUSH " + program

testPrograms = ["3", "19", "5+2", "5-2", "birds", "12+23"]
for x in testPrograms:
    print(x + ":\n" + runProgram(x))

Output in this test:

3:
PUSH 3
19:
PUSH 19
5+2:
PUSH 5
PUSH 2
ADD
5-2:
PUSH 5
PUSH 2
SUB
birds:
FAIL
12+23:
PUSH 2
PUSH 2
ADD
Press any key to continue . . .

† NOTE: The program will just add the two digits on either side of the operand

I'm reading the 2nd requirement:

  Accepts two single digit numbers† with a "+" or "-" in between them as input

in conjunction with the "12+23" test case. It's a very nice test case, but the double digit numbers are clearly beyond what the spec described. Behavior is unspecified in this case, though your program behaves very sensibly.

In runProgram(), the + and - cases are very similar. Can you break out a helper method that takes + / - as a parameter?

I recommend phrasing the last portion of the function in this way:

else:
    try:
        return 'PUSH ' + int(program)
    except ValueError:
        return 'FAIL'

The in program aproach seems fragile, especially if you have to handle programs like 5 + -3. You might want to tokenize early on, and pass in a list like [5, '+', -3]. Or perhaps define functions like add & subtract, and pass in [5, add, -3].


Confirmation Model with jQuery I have a link aimed to '/FridgeTemperatureRoute' and a checkbox. Purpose of the checkbox is to show the user whether the check has been done or not.

        Fridge and Freezer Temperature

        <input id="FridgeTemperatureCheckBox" type="checkbox" name="" value="" disabled {{fridgeFreezerTemperatureReturnObj}}>

Summary of code
You'll see this line {{fridgeFreezerTemperatureReturnObj}} - this is from Node JS server, which determines whether the checkbox is ticked or not.
Jquery, Javascript and Bootstrap Model
Next, I am aiming to open a bootstrap model on user click, but only if the checkbox is ticked. If the user clicks and the checkbox is checked, then open a bootstrap model asking whether to continue to the href or simply cancel rendering the next page.

You have already completed this check, do you want to do it again?

        <button type="button" class="btn btn-default" id="confirmOk">Ok</button>
            <button type="button" class="btn btn-default" id="confirmCancel" data-dismiss="modal">Cancel</button>

Summary
I have it working - but I believe it can be simplified. Just looking for some reviews and where I can improve on :)

Be consistent
If you're going to use jQuery, it would be stylistically appropriate (and terser) to use it instead of the native DOM methods. For example:
if(document.getElementById('FridgeTemperatureCheckBox').checked){

can turn into
if ($('#FridgeTemperatureCheckBox').is(':checked')) {

To translate getElementById to jQuery or querySelector, put a # in front of the ID you want to select.
Or you can use vanilla DOM methods everywhere if you prefer (like I do) - but whatever you choose to do, it would be good to do it consistently, otherwise you'll probably confuse future readers of the code.
HTML too: On a similar note about consistency - with your element IDs, better to choose either camelCase, PascalCase, or kebab-case everywhere, but don't mix and match different styles, else you're more likely to run into typo problems when you don't have a set style and mis-remember which style you used. (You can also consider using classes instead of IDs, since IDs with their automatically global variables are unintuitive and can cause bugs.)
DRY code
Rather than selecting the same element multiple times, eg:
$('#FridgeTemperatureBtn').attr('data-toggle', 'modal');
$('#FridgeTemperatureBtn').attr('data-target', '#confirmModal');

It would be more elegant to select the element once and save it in a variable. Or, even better, since .attr returns the existing jQuery collection, just chain the .attr calls:
$('#FridgeTemperatureBtn')
  .attr('data-toggle', 'modal')
  .attr('data-target', '#confirmModal');

Attach listener once, immediately: It looks like the confirmModal only becomes visible when the user clicks the button and has already completed the check, is that correct? If so, consider attaching the #confirmOk click listener immediately, rather than only after the #FridgeTemperatureBtn is clicked. (Attaching identical listeners multiple times won't cause problems in this case, but it's peculiar logic.)
Semantics: Your FridgeTemperatureBtn is an <a>, not a button, so its ID's name should probably be tweaked. If you have it styled to look like a button with your CSS, it would probably be a bit more semantically appropriate to surround a <button> inside an <a> instead.
Unchangeable checkbox UX: The #FridgeTemperatureCheckBox is disabled. Having it be a checkbox is a bit unintuitive, because users generally expect checkboxes to be clickable. Consider informing the user that the check has already been done some other way, such as by adding a checkmark (not a checkbox), or by changing the background color, or something like that.


Spawning threads to process data I have a class that spawns threads to process data. I am working on instrumentation code. When the process is started, time = System.currentTimeMillis(); When it completes, time = System.currentTimeMillis() - time; I have a method to retrieve this time:

/**
 * Get the time taken for process to complete
 *
 * @return Time this process has run if running; time it took to complete if not
 */
public long getRunTime() {
   return processRunning? System.currentTimeMillis() - time : time;
}

Is this a clear use of the ternary operator? Is my javadoc comment clear?

I have no problem the ternary operator.

But it is hard to tell that the result is what the comments is saying. Without having a context on what time is (which is a bad variable name) it is hard to understand the result of the function.

time: Bad variable name. Time of what?


Text-based Dungeon Crawl game This code is an answer to an exercise I found on the cplusplus forums.
I'm a beginner so any reviews will be appreciated :)

  Make a program that outputs a simple grid based gameboard to the screen using either numbers or characters. Allow the user  to move either up, down, left, or right each turn. If the player steps on a trap then they lose. If they make it to the treasure 'X' then they win.

#include <iostream>
#include <string>
#include <time.h>
#include <Windows.h>
#include <algorithm>
int main(){
std::string grid = "0000000000\n"
    "0000000000\n"
    "0000000000\n"
    "0000000000\n"
    "0000000000\n"
    "0000000000\n"
    "0000000000\n";
char trap = 'T';
grid[15] = trap;
grid[35] = trap;
grid[45] = trap;
grid[67] = trap;
char player = 'G';
int n = 1;
grid[n] = player;
char end = 'X';
grid[75] = end;
char wasd;
std::cin >> wasd;
while (n != 15 && n != 35 && n != 45 && n != 67 && n!=11 && n!=21 && n!=31 && n!=41 && n!=51 && n!=61 && n!=71 ){
    if (wasd == 'd'){
        grid[n] = '0';
        n++;
        grid[n] = player;
        std::cout << grid;
        std::cin >> wasd;

    }
    else if (wasd == 'a'){
        grid[n] = '0';
        n--;
        grid[n] = player;
        std::cout << grid;
        std::cin >> wasd;
    }
    else if (wasd == 's'){
        grid[n] = '0';
        n = n + 11;
        grid[n] = player;
        std::cout << grid;
        std::cin >> wasd;
    }
    else if (wasd == 'w'){
        grid[n] = '0';
        n = n - 11;
        grid[n] = player;
        std::cout << grid;
        std::cin >> wasd;
    }

    }
if (grid[15] == player || grid[35] == player || grid[45] == player || grid[67] == player){
    std::cout << "You lose!";
}
}

First I want to give you a few general comments on the style and aesthetics of your code, then I'll provide you with a detailed code sample applying a slightly different approach to the problem, that you can use as comparison or baseline.

Avoiding unnecessary dependencies

You don't need <Windows.h> for this program. It is the only thing preventing it from compiling on Mac or Linux. You can remove that include file.

<time.h> is also not necessary. You are not currently using any timing routines, that can also be stripped. I also don't see any uses of functions defined in <algorithm>.

Indenting

Is very important to denote nesting. Make sure to always add one level of indentation for code that lives inside a function. The contents of main should be indented to convey that nesting.

Use of "magic numbers"

There are quite a few literal constants in your code that are not clear about their meaning from a first glance. In particular, the several comparisons in the main while loop seem very obscure. I can spot that some of them are actually testing for collision with a trap or the end of the map. You should avoid using raw literal numbers and strings in your program when they can be replaced by named values. We can use const and enum in C++ to define immutable values that have a descriptive name that is much easier to remember than the number 15 or whatever.

But ultimately, for the checks that you're performing, I think you are going about it the wrong way. Instead of testing specific indexes in the map grid, you should just verify what is the symbol in the map for the current position of the player. By doing so, you can get rid of all those magic numbers. You will see in the following sample that I do so and also use constants and enums to name some of the other literal values.

Use a 2D matrix to represent the grid

You've opted for a std::string to represent the map grid, which is fine, but complicates things a little, since the
characters are stored linearly in the string. A more straightforward way of defining the map would be with an actual square matrix of N rows by M columns (or width x height). Like so:

char world_map[MapWidth][MapHeight];

Where the X direction is the width (from the top-left corner to the top-right corner), and Y is the height (from the top-left corner to the bottom-left):

(x=0, y=0)
     +----------+
     |0000000000|
     |0000000000|
     |0000000000|
     |0000000000|
     |0000000000|
     |0000000000|
     |0000000000|
     +----------+ (x=width-1, y=height-1)

Also note that we can use a constant for the width (columns) and height (rows) of the map. This allows us to easily change the size of the map without having to hunt down all over the code for occurrences of literal numbers.

Avoid repetition

Your code has some degree of repetition in the bodies of the if tests. The code for moving the player and checking if she has hit a trap, wall or exit can be generalized into a function that you can call once for each direction of
movement.

In the following example you will see that the movement and map update logic has been moved into a move_player() function, which returns a value indicating if we are free to KeepLooking for the exit or if we have HitTrap and died or FoundExit and are home free.

if (key == Right)
{
    gameState = move_player(world_map, playerPosX, playerPosY, playerPosX + 1, playerPosY);
}
else if (key == Left)
{
    gameState = move_player(world_map, playerPosX, playerPosY, playerPosX - 1, playerPosY);
}
else if (key == Up)
{
    gameState = move_player(world_map, playerPosX, playerPosY, playerPosX, playerPosY - 1);
}
else if (key == Down)
{
    gameState = move_player(world_map, playerPosX, playerPosY, playerPosX, playerPosY + 1);
}
else if (key == Quit)
{
    gameState = QuitGame;
}

Note that the current position of the player in the map it taken by reference (&) so we can modify it inside the function and have the result visible outside in main (see the code below). This is necessary to ensure the player doesn't move outside the map bounds. I don't think you performed that correctly
in your attempt.

Use functions to separate unrelated logic and reuse code

Sometimes it is helpful to split unrelated logic into separate functions. Notice that I have introduced clear_map() and print_map(). This cleans up main and also makes those functions reusable if you decide the expand the code and find yourself needing to clear the map several times, for instance.

Printing the map is already done in two places, so that by itself already warrants it a function.

Putting it all together:

Following is my suggested solution to the exercise, applying the above discussed points. I hope you find it readable, but feel free to ask if you are in doubt about something. I've also added more message printing and a check for invalid keys, so it is slightly longer than your original code.

#include <iostream>
#include <string>

const int MapWidth      = 10;
const int MapHeight     = 7;

// These make our lives easier if we ever decide to change the symbols:
const char PlayerSymbol = 'G';
const char TrapSymbol   = 'T';
const char ExitSymbol   = 'X';
const char EmptySymbol  = '0';

enum Keys
{
    Right = 'd',
    Left  = 'a',
    Up    = 'w',
    Down  = 's',
    Quit  = 'q'
};

enum GameState
{
    HitTrap,
    FoundExit,
    KeepLooking,
    QuitGame
};

void print_map(const char map[MapWidth][MapHeight])
{
    for (int y = 0; y < MapHeight; ++y)
    {
        for (int x = 0; x < MapWidth; ++x)
        {
            std::cout << map[x][y];
        }
        std::cout << "\n";
    }
}

void clear_map(char map[MapWidth][MapHeight])
{
    for (int y = 0; y < MapHeight; ++y)
    {
        for (int x = 0; x < MapWidth; ++x)
        {
            map[x][y] = EmptySymbol;
        }
    }
}

void place_props(char map[MapWidth][MapHeight])
{
    // The initial player position (0,0):
    map[0][0] = PlayerSymbol;

    // Traps:
    map[4][1] = TrapSymbol;
    map[2][3] = TrapSymbol;
    map[1][4] = TrapSymbol;
    map[1][6] = TrapSymbol;

    // Exit is at the right end of the map.
    map[MapWidth - 1][MapHeight - 1] = ExitSymbol;
}

GameState move_player(char map[MapWidth][MapHeight],
                      int & currPlayerPosX, int & currPlayerPosY,
                      int desiredPlayerX,   int desiredPlayerY)
{
    // Clamp the new desired position to stay within the map bounds:
    if (desiredPlayerX < 0) { desiredPlayerX = 0; }
    if (desiredPlayerY < 0) { desiredPlayerY = 0; }
    if (desiredPlayerX >= MapWidth)  { desiredPlayerX = MapWidth  - 1; }
    if (desiredPlayerY >= MapHeight) { desiredPlayerY = MapHeight - 1; }

    GameState newState;

    // Check victory condition or if we hit a trap:
    if (map[desiredPlayerX][desiredPlayerY] == TrapSymbol)
    {
        std::cout << "You hit a trap an got decapitated! Better luck next time.\n";
        newState = HitTrap;
    }
    else if (map[desiredPlayerX][desiredPlayerY] == ExitSymbol)
    {
        std::cout << "You find the exit and live another day to tell the tales of your adventures!\n";
        newState = FoundExit;
    }
    else
    {
        newState = KeepLooking;
    }

    // Clear current position:
    map[currPlayerPosX][currPlayerPosY] = EmptySymbol;

    // Place the player symbol:
    map[desiredPlayerX][desiredPlayerY] = PlayerSymbol;

    // Return the new clamped position, avoiding the player from leaving the map.
    // Since we are using a reference to int, this change will be visible outside.
    currPlayerPosX = desiredPlayerX;
    currPlayerPosY = desiredPlayerY;
    return newState;
}

int main()
{
    int playerPosX = 0;
    int playerPosY = 0;
    GameState gameState = KeepLooking;
    char world_map[MapWidth][MapHeight];

    clear_map(world_map);
    place_props(world_map);
    print_map(world_map);

    while (gameState == KeepLooking)
    {
        char key;
        std::cin >> key;

        if (key == Right)
        {
            gameState = move_player(world_map, playerPosX, playerPosY, playerPosX + 1, playerPosY);
        }
        else if (key == Left)
        {
            gameState = move_player(world_map, playerPosX, playerPosY, playerPosX - 1, playerPosY);
        }
        else if (key == Up)
        {
            gameState = move_player(world_map, playerPosX, playerPosY, playerPosX, playerPosY - 1);
        }
        else if (key == Down)
        {
            gameState = move_player(world_map, playerPosX, playerPosY, playerPosX, playerPosY + 1);
        }
        else if (key == Quit)
        {
            gameState = QuitGame;
        }
        else
        {
            std::cout << "Invalid direction, use W,S,A,D to move up, down, left, right. Q to quit.\n";
        }

        print_map(world_map);
    }
}


Start an EventHandler in different Thread I just stumbeled accross some code and I would do it in a different way. Since I am quite new to programming, I just wanted to know if I understand it right.

Original Code:

protected void FireStateChange(LightCommunicationState OldState, LightCommunicationState NewState)

    {
        if (StateChanged != null && OldState != NewState)
        {
            Thread t = new Thread(StateChangeThread);
            uint u = ((uint)OldState * 0x10000) + (uint)NewState;
            t.Start(u);
        }
    }

    protected void StateChangeThread(object o)
    {
        uint u = (uint)o;
        ushort Old = (ushort)((u & 0xFFFF0000) >> 16);
        ushort New = (ushort)(u & 0xFFFF);

        StateChanged(this, (LightCommunicationState)Old, (LightCommunicationState)New);

    }

My personal solution would be:

protected void FireStateChange (LightCommunicationState OldState, LightCommunicationState NewState)
    {
        if (StateChanged != null && OldState != NewState)
        {
            new ThreadStart(
                delegate
                {
                    StateChanged(this, OldState, NewState);
                }).Invoke();
        }
    }

It seems a bitshift and cast an object to a uInt back an forth looks freaky but is unnecessary. Maybe I am missing some benefits on the original solution?

The LightCommunicationState enum is defined as follows:

public enum LightCommunicationState: ushort
{
    notChoosen, Waiting, Communicating
}

And the StateChanged event like this:

public delegate void LightCommunicationStateChanged(LightCommunicationStateMachine Sender, LightCommunicationState OldState, LightCommunicationState NewState);

public event LightCommunicationStateChanged StateChanged;

Don't reinvent the wheel: use EventArgs

The StateChanged event isn't compliant with established C# conventions for creating events.

public delegate void LightCommunicationStateChanged(LightCommunicationStateMachine Sender, LightCommunicationState OldState, LightCommunicationState NewState);
public event LightCommunicationStateChanged StateChanged;

That is non-standard. C# conventions recommend using an EventHandler<T> delegate, where T is a type derived from System.EventArgs - the type could be as simple as this:

public class StateChangedEventArgs : EventArgs
{
    public StateChangedEventArgs(LightCommunicationState oldState, LightCommunicationState newState)
    {
        _oldState = oldState;
        _newState = newState;
    }

    private readonly LightCommunicationState _oldState;
    public LightCommunicationState OldState { get { return _oldState; } }

    private readonly LightCommunicationState _newState;
    public LightCommunicationState NewState { get { return _newState; } }
}

In C# 6 it's even simpler - you can implement an actual immutable type with auto-properties:

public class StateChangedEventArgs : EventArgs
{
    public StateChangedEventArgs(LightCommunicationState oldState, LightCommunicationState newState)
    {
        OldState = oldState;
        NewState = newState;
    }

    public LightCommunicationState OldState { get; }
    public LightCommunicationState NewState { get; }
}

By encapsulating the event data inside an EventArgs class, you avoid needing to change the delegate signature (and all the cascading modifications it entails), should the event data ever need to change:

public event EventHandler<StateChangedEventArgs> StateChanged;

Using such a EventHandler<StateChangedEventArgs> delegate for the StateChanged event makes all handler signatures look like this:

private void HandleStateChanged(object sender, StateChangedEventArgs e)

The day business requirements change and you need to add some DateTime Timestamp property to StateChangedEventArgs, none of the handlers' signature needs to change to keep the code compilable.

Asynchronous?

Note that using a custom delegate for events does have its use: generics cannot be exposed through COM interop, therefore a public EventHandler<T> member can't be exposed if the event needs to be handled on the COM side, so a custom delegate is required. But then, invoking a handler on a different thread than the thread it was registered with, will consistently result in a null handler on the .net side.

Hence, I suspect what you really wanted was async handlers - something like this:

private async void HandleStateChanged(object sender, StateChangedEventArgs e)
{
    await DoSomething(e.OldState, e.NewState);
}

Possibly the code was written before .net 4.5 when async/await wasn't available; consider re-targeting the project to a more recent .net version and replacing that threading code with async/await.

Non-Standard Enum

The LightCommunicationState enum isn't standard either:

public enum LightCommunicationState: ushort
{
    notChoosen, Waiting, Communicating
}

Enum values are public members, and should be named as such, in PascalCase. It's also easier to follow and eventually decorate the members with XML-docs and attributes when the members are on their own line, for example:

public enum LightCommunicationState
{
    /// <summary>
    /// Indicates that the state machine is idle.
    /// </summary>
    NotChoosen,
    /// <summary>
    /// Indicates that the state machine is waiting for a signal.
    /// </summary>
    Waiting,
    /// <summary>
    /// Indicates that the state machine is communicating with a client.
    /// </summary>
    Communicating
}

There's seldom a valid reason to change the underlying type of an enum. Seems the ushort underlying type was chosen only to play nicely with the bit-shifting, but then the enum values aren't explicitly assigned, which I believe means adding a 4th value would have broken the logic - the 3 values are implicitly 0, 1 and 2 which happen to match the first few powers of 2, but the next value would be 3 when the next power of 2 would be 4... but that's all pointless: the whole bit-shifting logic was apparently only there to cram both oldState and newState into a single value, boxed into an object, to match the ParameterizedThreadStart delegate signature - which is ugly at best, and irrelevant given a proper EventHandler<T> event.

ThreadStart.Invoke

I don't think your solution is equivalent. ThreadStart is just a delegate - invoking it won't automagically run it on a separate thread, you need a Thread object somewhere!

Per the ParameterizedThreadStart topic on MSDN, the original code had it right. I believe your version simply runs your inline delegate (which merely invokes the handler) on the very same thread FireStateChanged is running on.

Thread Safety

An event reference is null whenever it has no registered handlers, and handlers can be added and removed at any time, by any thread. In both the original code and your own version, the event invocation isn't thread-safe:

if (StateChanged != null && OldState != NewState)
{
    // here, another thread could remove the last handler for StateChanged
    StateChanged(...);

The work-around is simple:

var handler = StateChanged;
if (handler != null && oldState != newState)
{
    // here, the handler reference is thread-local and can't ever be null
    handler.Invoke(...);

In C# 6 you can use the null-conditional operator directly on the event, instead of making a thread-local copy and explicitly checking for handler != null:

if (oldState != newState)
{
    StateChanged?.Invoke(...);

Notice I changed OldState and NewState to oldState and newState, respectively: parameters and local variables should be camelCase, not PascalCase (notice even the syntax highlighting picks it up!).

Also - and that's completely a personal preference - I prefer seeing an explicit .Invoke call on the event, rather than writing it to look like a standard method call - IMO the .Invoke makes a clear visual cue that we're invoking an event, not just another method.

My solution might have looked something like this:

protected void OnStateChanged(LightCommunicationState oldState, LightCommunicationState newState)
{
    var handler = StateChanged;
    if (handler == null || oldState == newState)
    {
        return;
    }

    var args = new StateChangedEventArgs(oldState, newState);
    var thread = new Thread(e => handler.Invoke(this, (StateChangedEventArgs)e));
    thread.Start(args);
}

Notice I named the method On[EventName] instead of Fire[EventName] - that's also a C# naming convention, see C# Naming Guidelines on MSDN. Note the cast from object to StateChangedEventArgs incurs no boxing, since only reference types are involved.

Also notice I'm making use of the compiler's type inference capabilities: you don't need to explicitly new up a ThreadStart delegate, using the => lambda operator instead: the compiler is able to infer the correct delegate type from the constructor overload.


Youtube Video Downloader This code takes a YouTube video and download it. It supports only non-copyrighted videos due to YouTube's new algorithms.

Any thoughts about the structure and the logic? I would like to learn from my mistakes, I've just started coding with python.

#!/usr/bin/env python3.5.2

import urllib.request, urllib.parse
import os

def main():
    """
    This code will download only non copyrighted youtube videos due to the new algorithms for accessing video files taht Youtube had changed.
    Instead of "token" they now use "signature" variable, and "signature" seems to be dependent on either cookie-stored data or IP address of the client
    """
    print("\n                          * * * * * * * * * * * * * * *")
    print("                          *                           *")
    print("                          * Youtube Video Downloader! *")
    print("                          *                           *")
    print("                          * * * * * * * * * * * * * * *\n")
    # make sure to change directory path
    directory = r'C:\Users\SalahGfx\Desktop\DownloadedVideos'

    def get_sourcecode(url):
        return urllib.request.urlopen(url).read()

    def get_info():
        return urllib.parse.parse_qs(get_sourcecode(url).decode('unicode_escape'))

    def get_video_title_path():
        try:
            return os.path.join(directory, '{}.mp4'.format(get_info()['title'][0]))

        except KeyError:
            print('Parsing error, please rerun the code!')

    def get_stream_map():
        return get_info()['url']

    def get_stream_map_list():
        video_links = []
        for video_stream in get_stream_map():
            if "googlevideo" == video_stream.split('/')[2].split('.')[1]:
                video_links.append(video_stream)

            else:
                pass
        return video_links

    def validate_url(url):
        return url.startswith('https://www.youtube.com/')

    def download_video(k):
        return urllib.request.urlretrieve(get_stream_map_list()[k], get_video_title_path())

    def download_hq_video(quality):
        k = 0
        download_video(k)
        print("Downloading {} -----------> {} in {} quality.".format(get_stream_map_list()[k], get_video_title_path(),
                                                                     quality))

    def download_md_video(quality):
        k = 1
        download_video(k)
        print("Downloading {} -----------> {} in {} quality.".format(get_stream_map_list()[k], get_video_title_path(),
                                                                     quality))

    def download_sm_video(quality):
        k = 2
        download_video(k)
        print("Downloading {} -----------> {} in {} quality.".format(get_stream_map_list()[k], get_video_title_path(),
                                                                     quality))

    while True:
        try:
            url, quality = input(
                "Please type a youtube video url and its quality(choose between high, medium and small) and separate them with a space: ").split()

        except ValueError:
            print("Please type the right url and quality.");
            print("Make sure the url start with 'https://www.youtube.com/'");
            print("high - for downloading high quality video");
            print("medium - for downloading medium quality video");
            print("small - for downloading small quality video");
            continue

        if validate_url(url) and quality == 'high':
            get_sourcecode(url)
            download_hq_video(quality)
            break

        elif validate_url(url) and quality == 'medium':
            get_sourcecode(url)
            download_md_video(quality)
            break

        elif validate_url(url) and quality == 'small':
            get_sourcecode(url)
            download_sm_video(quality)
            break

        else:
            break

if __name__ == '__main__':
    main()

I don't see why you want/need three functions to download the different quality videos. There is a bunch of repeated logic both in the functions and in the main loop.

Then, I would move all of the main specific code into the __name__ == __main__ section because that's the purpose of that section.

#!/usr/bin/env python3.5.2

import urllib.request, urllib.parse
import os

def get_sourcecode(url):
    return urllib.request.urlopen(url).read()

def get_info():
    return urllib.parse.parse_qs(get_sourcecode(url).decode('unicode_escape'))

def get_video_title_path():
    try:
        return os.path.join(directory, '{}.mp4'.format(get_info()['title'][0]))
    except KeyError:
        print('Parsing error, please rerun the code!')

def get_stream_map_list():
    video_links = []
    for video_stream in get_info()['url']:
        if "googlevideo" == video_stream.split('/')[2].split('.')[1]:
            video_links.append(video_stream)
    return video_links

quality_map = {'small': 0, 'medium': 1, 'high': 2}

def download_video(quality='medium'):
    k = quality_map.get(quality)
    map_list = get_stream_map_list()[k]
    title_path = get_video_title_path()

    print("Downloading {} -----------> {} in {} quality.".format(map_list, title_path, quality))
    return urllib.request.urlretrieve(map_list, title_path)

if __name__ == '__main__':
    print("\n                          * * * * * * * * * * * * * * *")
    print("                          *                           *")
    print("                          * Youtube Video Downloader! *")
    print("                          *                           *")
    print("                          * * * * * * * * * * * * * * *\n")
    # make sure to change directory path
    directory = r'C:\Users\SalahGfx\Desktop\DownloadedVideos'

    while True:
        try:
            url, quality = input(
                "Please type a youtube video url and its quality(choose between high, medium and small) and separate them with a space: ").split()

        except ValueError:
            print("Please type the right url and quality.");
            print("Make sure the url start with 'https://www.youtube.com/'");
            print("high - for downloading high quality video");
            print("medium - for downloading medium quality video");
            print("small - for downloading small quality video");
            continue

        if url.startswith('https://www.youtube.com/') and quality in quality_map:
            get_sourcecode(url)
            download_video(quality)
            break


LeetCode 124: Binary Tree Maximum Path Sum I'm posting my code for a LeetCode problem. If you'd like to review, please do so. Thank you for your time!
Problem
Given a non-empty binary tree, find the maximum path sum.
For this problem, a path is defined as any sequence of nodes from some starting node to any node in the tree along the parent-child connections. The path must contain at least one node and does not need to go through the root.
Example 1:
Input: [1,2,3]

       1
      / \
     2   3

Output: 6

Example 2:
Input: [-10,9,20,null,null,15,7]

   -10
   / \
  9  20
    /  \
   15   7

Output: 42

Inputs
[1,2,3]
[-10,9,20,null,null,15,7]
[-10,9,20,null,null,15,7,9,20,null,null,15,7]
[-10,9,20,null,null,15,7,9,20,null,null,15,720,null,null,15,7,9,20,null,null,15,7]
[-10,9,20,null,null,15,7,9,20,null,null,15,720,null,null,15,7,9,20,null,null,15,7999999,20,null,null,15,7,9,20,null,null,15,720,null,null,15,7,9,20,null,null,15,7]

Outputs
6
42
66
791
8001552

Code
#include <cstdint>
#include <algorithm>

struct Solution {
    int maxPathSum(TreeNode* root) {
        std::int_fast64_t sum = INT_FAST64_MIN;
        depthFirstSearch(root, sum);
        return sum;
    }

private:
    static std::int_fast64_t depthFirstSearch(
        const TreeNode* node,
        std::int_fast64_t& sum
    ) {

        if (!node) {
            return 0;
        }

        const std::int_fast64_t left = std::max(
                                           (std::int_fast64_t) 0,
                                           depthFirstSearch(node->left, sum)
                                       );
        const std::int_fast64_t right = std::max(
                                            (std::int_fast64_t) 0,
                                            depthFirstSearch(node->right, sum)
                                        );
        sum = std::max(sum, left + right + node->val);
        return std::max(left, right) + node->val;
    }
};

References

Problem

Discuss

Solution

Not sure why you decided to use std::int_fast64_t over the common int that is used as the type of the tree nodes values.
But since you did, it would be more idiomatic to do at least:
static_cast<std::int_fast64_t>(0);

instead of
(std::int_fast64_t) 0;


Optimization of percent match code I have the following method that will take the car parts I searched for and match them to my car object in array. Then it tells me the percent match.

Example:

Car.h has NSSet toCarParts. toCarParts has may car parts.

My array contains 5,000 or so matches.

I will go through each one and match my parts to the car parts and calculate a percent match.  If I search for wheel, tire, rim, and Car X has wheel, tire, rim, seat, my % should be 75%.

The method does this, but take about 30 seconds on an iPhone 5 (double or triple on iPhone 4)

How can I optimize it?

I think what slows it down is that I need to open each car object, fire the faults, access the toCarParts set, for each part compare if it matches a part in my search array.

I also think it is slow when doing division (counter/[carPartsArrayFaulted count]=%).

-(NSMutableArray*)calculatePercentagePerFind:(NSMutableArray*)CarArray:(NSMutableArray*)partsArray{
    NSArray*defaultParts =[NSArray arrayWithArray:[[[HelperMethods alloc]init]getObjectUserDefault:@"AvailableDefaultParts"]];

    int lowestPercentMatchInt=[[[HelperMethods alloc]init]getIntegerUserDefault:@"lowestPercentageMatch"];

    NSMutableArray*partsFromCarArray=[[NSMutableArray alloc]init];

    NSMutableArray*returnArray=[[NSMutableArray alloc]init];

    NSMutableArray *partsWithDefaultParts =[NSMutableArray arrayWithArray:partsArray];
    [partsWithDefaultParts addObjectsFromArray:defaultParts];

    for (int i=0; i<[CarArray count]; i++) {
        double matchCount=0;
        Car *CarResult =(Car*)[CarArray objectAtIndex:i];

                //Check if it will at least be 30% match
        double number1 = [partsWithDefaultParts count];
            number1 =(number1/[CarResult.numberOfParts doubleValue])*100;
            if (number1>lowestPercentMatchInt) {
                partsFromCarArray =[NSMutableArray arrayWithArray:[[CarResult toParts]allObjects]];
                NSMutableArray *faultedParts=[[NSMutableArray alloc]init];
                for (int i =0; i<[partsFromCarArray count]; i++) {
                    CarPart*part = (CarPart*)[partsFromCarArray objectAtIndex:i];
                        [faultedParts addObject:part.name];
                }
            // for each part in the Car
            for (NSString *partInCar in partsWithDefaultParts){
                //if the search parts contain that part, add one to count
                if ([faultedParts containsObject:partInCar]) {
                    matchCount++;
                }
            }
            //Calculate percent match
            double percentMatch = matchCount;

            percentMatch =(percentMatch/[CarResult.numberOfParts doubleValue])*100;

            //if at least 30%(user default) then add the percent match to Car result
            if (percentMatch >lowestPercentMatchInt) {
                if (percentMatch>100) {
                    CarResult.percentMatch = [NSNumber numberWithDouble:100.00];
                }else{
                    CarResult.percentMatch = [NSNumber numberWithDouble:percentMatch];
                }
                [returnArray addObject:CarResult];
            }
    }
    }
    NSLog(@"Percent Matched Cars = %i",[returnArray count]);
    return [self arrangeByHighestPercentMatch:returnArray];
}

Currently you are comparing all part against all parts, so you have a squared complexity which causes trouble for bigger numbers.

In the case that you can create your part list as sorted list (i.e. TreeSet in Java, not sure what is available in your language ) you can do the comparison in linear time by stepping through both lists in parallel.

An other idea is to use something like the HashSet in Java where the containsObject don't cost linear time as with the array or a simple list.


Event Handler Registration in Java I have a stateful object on which I'd like client code to be able to register many event handlers. These handlers have different signatures—they usually accept the originating object and some piece of data relevant to the event.

public class StatefulObject {
  @FunctionalInterface
  private interface Callback {
    void call(Object... args);
  }

  private enum Event {PLACEMENT, FILL, CANCEL, ERROR /* etc... */}
  private final Map<Event, List<Callback>> callbacks = new HashMap<>();

  public StatefulObject onPlacement(Consumer<StatefulObject> placementCallback) {
    callbacksFor(Event.PLACEMENT).add(args -> placementCallback.accept((StatefulObject) args[0]));
    return this;
  }

  public StatefulObject onError(BiConsumer<StatefulObject, Exception> errorCallback) {
    callbacksFor(Event.ERROR).add(args -> errorCallback.accept((StatefulObject) args[0], (Exception) args[1]));
    return this;
  }

  // ... etc

  private List<Callback> callbacksFor(Event event) {
    List<Callback> callbacksList = callbacks.getOrDefault(event, new ArrayList<>());
    callbacks.put(Event.ERROR, callbacksList);
    return callbacksList;
  }
}

My idea here is that (i) I shouldn't have many Lists/Arrays of callbacks and (ii) I shouldn't have unchecked casts. Instead of declaring a Map<Event, Object> and then casting the returned Object to List<BiConsumer<StatefulObject, Exception>>, for example, I thought it would be better to have a generic Callback and then cast the individual arguments.

Is this pattern reasonable, or is it unclear? Access is obviously restricted to just this class.

Thanks for your opinions!

Can you elaborate on that a bit? I fell like I do have a common interface (Callback) which prevents the dispatching method from having to do anything outlandish. For instance if I catch an exception and want to send it to the onError callback it looks something like

for (Callback cb : callbacksFor(Event.ERROR))
    cb.call(this, someException);.

  That's the dispatching method, right?

Right.

Problem here is that you pass the current object (having a state an business logic) as data to the callback. But you should only sent data transfer objects (without business logic and immutable by any chance).

So the common interface of the callback should look like this:

interface Callback{
   enum Key {EXCEPTION, DATA_1,DATA_2,DATA_3, ... }
   void onError(Map<Key, Object);
}

and you sample would change to:

Map<Key, Object> currentState = copyInstanceMembersToMap();
currentState.put(Key.EXCEPTION, someException);
for (Callback cb : callbacksFor(Event.ERROR))
    cb.call(currentState);

Then the callbacks themselves decide which data to use.

And you can use the same callback interface for other listeners as well.

Unfortunately you loose the type safety for the values in that map.
But you could get that back a little:

public abstract class GenericEnum {
    enum Key {
        EXCEPTION {
            @Override
            public <Exception> Exception getFrom(Map<Key, Object> map) {
                return (Exception) map.get(this);
            }
        },
        DATA_1 {
            @Override
            public <String> String getFrom(Map<Key, Object> map) {
                return (String) map.get(this);
            }
        },
        DATA_2 {
            @Override
            public <File> File getFrom(Map<Key, Object> map) {
                return (File) map.get(this);
            }
        };
        public abstract <T> T getFrom(Map<Key, Object> map);
    }

    public static void main(String[] args) {
        Map<Key, Object> testMap = new HashMap<>();
        testMap.put(Key.EXCEPTION, new java.lang.Exception());
        testMap.put(Key.DATA_1, "test");
        testMap.put(Key.DATA_2, new File(System.getProperty("user.home")));
        Exception value1 = Key.EXCEPTION.getFrom(testMap);
        String value2 = Key.DATA_1.getFrom(testMap);
        File from = Key.DATA_2.getFrom(testMap);
        String value3 = Key.DATA_2.getFrom(testMap); // ClassCastException

    }

}


Distance between angles as points This code calculates the distance between angles, particularly for n-tuples of angles. One example where this situation occurs is as follows:

I'm using a 2 arms, one 6 degree of freedom and the other 7 dof with revolute (rotating) joints. The position of the arm can be accurately represented as an n-d point representing the angle of each joint, with no need for orientation/pose on the surface of a torus, with 1 dimension for each joint.

Here is a simple image explaining the 2D case.

Therefore I don't need to worry about orientation but probably just the envelope/box which I believe is used for the data structure and the points themselves.

Here is an implementation, which should be easy to template for more general situations:

typedef boost::array<double,6> ArmPos;

template<typename T>
inline T normalizeRadiansPiToMinusPi(T rad)
{
  // copy the sign of the value in radians to the value of pi
  T signedPI = boost::math::copysign(boost::math::constants::pi<T>(),rad);
  // set the value of rad to the appropriate signed value between pi and -pi
  rad = std::fmod(rad+signedPI,(boost::math::constants::two_pi<T>())) - signedPI;

  return rad;
}

// functor for getting sum of previous result and square of current element
// source: http://stackoverflow.com/questions/1326118/sum-of-square-of-each-elements-in-the-vector-using-for-each
template<typename T>
struct square
{
    T operator()(const T& Left, const T& Right) const
    {
        return (Left + Right*Right);
    }
};

namespace boost { namespace geometry {

double comparable_distance(ArmPos const& p1, ArmPos const& p2 ) {
    ArmPos diff;
    boost::transform(p1,p2,diff.begin(),std::minus<ArmPos::value_type>());
    boost::transform(diff,diff.begin(),&normalizeRadiansPiToMinusPi<ArmPos::value_type>);
    return boost::accumulate(diff,0,square<ArmPos::value_type>());
}

template<typename Box>
double comparable_distance(ArmPos const& armpos, Box const& box ){
    namespace bg = boost::geometry;
    ArmPos normAP = normalizeRadiansPiToMinusPi(armpos);
    ArmPos mindiff;
    boost::transform(normAP,bg::get<bg::min_corner>(box),mindiff.begin(),std::minus<ArmPos::value_type>());
    boost::transform(mindiff,mindiff.begin(),&normalizeRadiansPiToMinusPi<ArmPos::value_type>);
    ArmPos maxdiff;
    boost::transform(normAP,bg::get<bg::max_corner>(box),maxdiff.begin(),std::minus<ArmPos::value_type>());
    boost::transform(maxdiff,maxdiff.begin(),&normalizeRadiansPiToMinusPi<ArmPos::value_type>);

    ArmPos::value_type final_distance = 0.0;
    for(int i = 0; i < armpos.size(); ++i){
        if(mindiff[i] >= 0.0 && maxdiff[i] <= 0.0) continue; // between the min and max means "in the box" for this dimension
        ArmPos::value_type min_dist = std::min(std::abs(mindiff[i]),std::abs(maxdiff[i]));
        final_distance+=min_dist*min_dist;
    }

    return final_distance;
//    diff (min<D> - p<D>), (p<D> - max<D>)
}

Full code can be found in this gist.

This code is very hard to read. These are very simple geometric operations that you are programming (just things like subtracting vectors, or finding a vector's squared length) but their meaning is buried under a mountain of boilerplate.

Where you have:

boost::transform(p1,p2,diff.begin(),std::minus<ArmPos::value_type>());

could you find some way to write:

diff = p1 - p2;

instead? Similarly, where you have:

return boost::accumulate(diff,0,square<ArmPos::value_type>());

could you find some way to write:

return diff.lengthSquared();

instead? C++ supports operator overloading and virtual functions, so surely something along these lines would be possible.


Rendering a heat map for 400.000 points I am trying to draw a heat map with a given input of points and amplitude of each point. The normal input size is around 400000 points, but the code draws every 15th point. The map is drawn onto a Canvas.

In the XAML:

<Grid x:Name="MainPanel" Width="800" Height="600">
    <Textbox Name="textbox" Grid.Column="0"/>
    <Canvas Name="canvas" Grid.Column="1" HorizontalAlignment="Left" Height="450" Margin="70,75,0,0" VerticalAlignment="Top" Width="706"/>
</Grid>

Rendering code:

public static void DrawPoints(Parser parser, Canvas canvas, double radiusX, double radiusY, double thickness, double minAmplitude, double maxAmplitude)
        {
            double cellWidth = canvas.ActualWidth;
            double cellHeight = canvas.ActualHeight;

            var list = parser.Data;

            // Draws every 15th point
            int res = 15;

            for (int i = 0; i < list.Count; i += res)
            {
                var location = list[i].Point;
                var amplitude = list[i].Amplitude;

                // Converts rgb to hsv values for representation
                var color= ColorConverter.hsv2rgb(
                    COLOR_START_H * (1.0 - ((amplitude - minAmplitude) / (maxAmplitude - minAmplitude))),
                    COLOR_START_S, COLOR_START_V);

                Point center = new Point(location.X, location.Y);
                DrawEllipse(canvas, center, radiusX, radiusY,
                    new SolidColorBrush(color), new SolidColorBrush(color), thickness);
            }
            return;
        }

In the DrawEllipse() function:

public static void DrawEllipse(Canvas canvas, Point center, double radiusX, double radiusY, Brush fill, Brush stroke, double thickness)
        {
            var ellipse = new Ellipse();
            ellipse.Width = radiusX * 2;
            ellipse.Height = radiusY * 2;
            ellipse.Fill = fill;
            ellipse.Stroke = stroke;
            ellipse.StrokeThickness = thickness;

            Canvas.SetLeft(ellipse, center.X - radiusX);
            Canvas.SetBottom(ellipse, center.Y - radiusY);

            canvas.Children.Add(ellipse);

            return;
        }

In the Main code:

private void btnDraw_Click(object sender, RoutedEventArgs e)
{
    Painter.DrawPoints(parser, canvas, 3, 3, 1, 0, 100);
}

The issue is that after rendering all the points onto Canvas, when I try to enter some text into textbox within the same grid, there is significant lag. What can I do to reduce the lag?

Unless you need some sort of high level interaction with those points, to create 400,000 shapes (which are FrameworkElement, all but lightweight) is a waste of time and resources.

Did you consider to use an image, drawing them inside a WriteableImage and to display the image instead? Check MSDN for DrawingVisual and DrawingContext subjects. I used this method for small multiples composed with really huge heatmaps and it worked like a charm. Simple hittesting (for example to show a tooltip with exact value) is trivial to implement using X-Y coordinates.

You will keep your visual tree almost empty, you won't create few thousands brushes each time (but you can create your color palette once and use it multiple times) and redrawing will be as fast as you want. In most applications you don't need an infinite number of color shades, consider to limit palette to 256 entries and to map values to those discrete values.

You will need to create an image with the proper size but this has a great advantage:

When redrawing is requested then immediately create a very low-res image with few hundred points.
Start a background thread to draw the full-resolution image (using current canvas size).
When image is ready then simply replace the old low-res one.

In this way you can even easily debounce multiple requests caused by resize operations (if you care at all).

Note: to pick one point every N is not a proper decimation method because it doesn't preserve local minimum and maximum; you may (will!) visualize wrong results. Consider using minmax or, only if your type of data allows it, at least an average.


How can this this URL Checker be made cleaner? I have a table full of URLs, some of which will occasionally become invalid. As part of a system I'm writing to go through the links periodically, and test if they are valid (or redirect to valid sites), I wrote the following class. The feedback I'm looking for is primarily related to clean code and best practices--how should the URL be passed into this class, for example? (I have not included the test cases, but they were written first. :) )

Originally, I wrote this with the URL passed in on the constructor, but I realized that if I set it later, I could both make the class a dependency of another class without needing to know the URL ahead of time, and use the same object to test multiple URLs.

But as it is, my URLisValid function is doing two things: setting a class variable and evaluating the URL. That doesn't seem ideal to me, but neither does having a separate Setter, nor does passing the URL around the class as an argument, nor does passing it in on the constructor.

(obviously, I think this works fine as-is, although feel free to point out any real bugs. I'm improving this class as a lesson to myself in making my code as clean as possible).

class URLChecker
{
private $curl;
private $url;

const HTTP_RESPONSE_OK = 200;

public function __construct()
{
    $this->curl = curl_init();
}

public function __destruct()
{
    curl_close( $this->curl );
}

public function URLisValid( $url )
{
    $this->url = $url;
    $httpcode = $this->getHttpCodeForURL();
    return $this->httpCodeIsValid( $httpcode );
}

private function getHttpCodeForURL()
{
    $this->executeCurlSessionForHeadersOnly();
    return $this->getHttpCodeFromLastTransfer();
}

private function executeCurlSessionForHeadersOnly()
{
    $this->setCurlOptions();
    curl_exec( $this->curl );
}

private function getHttpCodeFromLastTransfer()
{
    return curl_getinfo( $this->curl, CURLINFO_HTTP_CODE );
}

private function setCurlOptions()
{
    curl_setopt($this->curl, CURLOPT_URL, $this->url);
    curl_setopt($this->curl, CURLOPT_NOBODY, true);
    curl_setopt($this->curl, CURLOPT_RETURNTRANSFER, true);
    curl_setopt($this->curl, CURLOPT_FOLLOWLOCATION, true);
}

private function httpCodeIsValid( $httpcode )
{
    return $httpcode == self::HTTP_RESPONSE_OK;
}
}

EDIT: version 2, based in part on some feedback:

class URLChecker

{
private $session;

const HTTP_RESPONSE_OK = 200;

public function __construct()
{
    $this->session = curl_init();
    $this->initializeSessionOptions();
}

public function urlIsValid( $url )
{
    $this->setSessionUrl( $url );
    $this->executeSession();
    $httpcode = $this->getHttpCodeFromLastSession();
    return $this->httpCodeIsValid( $httpcode );
}

private function executeSession()
{
    curl_exec( $this->session );
}

private function getHttpCodeFromLastSession()
{
    return curl_getinfo( $this->session, CURLINFO_HTTP_CODE );
}

private function setSessionUrl( $url )
{
    curl_setopt($this->session, CURLOPT_URL, $url);
}

private function initializeSessionOptions()
{
    curl_setopt($this->session, CURLOPT_NOBODY, true);
    curl_setopt($this->session, CURLOPT_RETURNTRANSFER, true);
    curl_setopt($this->session, CURLOPT_FOLLOWLOCATION, true);
}

private function httpCodeIsValid( $httpcode )
{
    return $httpcode == self::HTTP_RESPONSE_OK;
}

function __destruct()
{
    curl_close( $this->session );
}
}

I've removed $url as a class variable entirely, I've changed some method names to remove references to "curl" where the implementation of the session is not relevant to the logic. I kept the one line functions, because I want someone who does not know exactly how curl works to still be able to read the code. It is now clear that $url is not a property of the class--it is rather a variable to be passed in once, and tested.

one line functions make your logic hard to follow, I would advise strongly against them.
Especially if you only call that function only once, which is the case in most of your code.

As a very minor comment, I would put

curl_setopt($this->curl, CURLOPT_NOBODY, true);
curl_setopt($this->curl, CURLOPT_RETURNTRANSFER, true);
curl_setopt($this->curl, CURLOPT_FOLLOWLOCATION, true);

inside __construct(), it should be a tiny bit more efficient.

One more minor comment, $url could easily be passed as a parameter, why make it a property?

All that taken into account, I would rather maintain something like this:

class URLChecker
{
private $curl;

public function __construct()
{
    $this->curl = curl_init();
    curl_setopt($this->curl, CURLOPT_NOBODY, true);
    curl_setopt($this->curl, CURLOPT_RETURNTRANSFER, true);
    curl_setopt($this->curl, CURLOPT_FOLLOWLOCATION, true);
}

public function __destruct()
{
    curl_close( $this->curl );
}

public function URLisValid( $url )
{
    $httpcode = $this->getHttpCodeForURL( $url );
    return  $httpcode == 200; //HTTP_RESPONSE_OK
}

private function getHttpCodeForURL( $url )
{
    curl_setopt($this->curl, CURLOPT_URL, $url);
    curl_exec( $this->curl );
    return curl_getinfo( $this->curl, CURLINFO_HTTP_CODE );
}
}


Caesar Cipher that takes an arbitrary offset This Caesar cipher can take any integer and it will wrap around the correct number of times and still perform the encryption/decryption.

Let's say we have a text file named dog.

  ~$ cat dog
  The quick brown fox jumps over the lazy dog...

If the user enters a normal offset (1-26), it will perform like any other Caesar cipher.

  ~$ ./caesar dog 1
  ~$ cat dog
  Uif rvjdl cspxo gpy kvnqt pwfs uif mbaz eph...

Negative numbers can be used to shift backwards.

  ~$ ./caesar dog -1
  ~$ cat dog
  The quick brown fox jumps over the lazy dog...

Now, here's the beauty of it. Choosing an offset of 1 is no different then choosing an offset of, say, 53 or -51. This algorithm will correctly wrap around.

  ~$ ./caesar dog -51
  ~$ cat dog
  Uif rvjdl cspxo gpy kvnqt pwfs uif mbaz eph...
  ~$ ./caesar dog 51
  ~$ cat dog
  The quick brown fox jumps over the lazy dog...

I wrote this a long time ago and I posted it on some other programming site back in the day. I haven't really written much pure c code since. I'm guessing there's plenty of room for improvement.

Compiles with:
gcc -ansi -pedantic -Wall -Werror -O3 -o caesar caesar.c

#include <stdio.h>
#include <stdlib.h>
#include <ctype.h>

/* #define offset (-1) */ /* My chosen default offset */
#define lstart (0x61) /* Start of lowercase ASCII alphabet */
#define ustart (0x41) /* Start of uppercase ASCII alphabet */
#define nalpha 26 /* Number of letters in my alphabet */

int main(int argc, char **argv) {
    int ec, wrap, offset;
    FILE *fp;

    if (argc != 3) {
        fprintf(stderr, "Usage: %s <filename> <offset>\n", argv[0]);
        return 1;
    }

    fp = fopen(argv [1], "r+b");

    offset = atoi (argv[2]);

    if(!fp) {
        fprintf(stderr, "Error: could not open file!\n");
        return 1;
    }

    while((ec = fgetc(fp)) != EOF) {
        if(isalpha(ec)) {
            if(islower(ec)) {
                wrap = (ec + offset - lstart) % nalpha;
                wrap = (wrap < 0) ? nalpha + wrap : wrap;

            } else {
                wrap = (ec + offset - ustart) % nalpha;
                wrap = (wrap < 0) ? nalpha + wrap : wrap;
            }

            ec = islower(ec) ? wrap + lstart : wrap + ustart;

            fseek(fp, -1L, SEEK_CUR);
            fputc(ec, fp);
        }
    }

    fclose(fp);
    return 0;
}

Update: I will leave the code I originally posted above. Any modifications that I make will be posted below.

#include <stdio.h>
#include <stdlib.h>
#include <ctype.h>

#define NALPHA (26)

int main (int argc, char **argv) {
    int ec, offset ;
    FILE *fp ;

    if (argc != 3) {
        fprintf(stderr, "Usage: %s <filename> <offset>\n", argv[0]);
        return 1;
    }

    fp = fopen(argv[1], "r+b");
    offset = atoi(argv[2]);

    if (!fp) {
        fprintf(stderr, "Error: Could not open file: %s\n", argv[1]);
        return 1;
    }

    while ((ec = fgetc(fp)) != EOF) {
        if (isalpha(ec)) {
            const int ascii_offset = islower (ec) ? 'a' : 'A';
            int wrap = (ec + offset - ascii_offset) % NALPHA;
            if (wrap < 0) {
                wrap += NALPHA ;
            }
            ec = wrap + ascii_offset;
        }

        fputc(ec, stdout);
    }

    fclose(fp);
    return 0;
}

Some comments on your updated code in Rev 3:

I would define your constants as follows:

#define LSTART ('a')
#define USTART ('A')
#define NALPHA ('z' - 'a' + 1)

You now need no comments - they are obvious.
It would be normal to use perror(argv[1]); to print the error on opening
the file.
My man-page says atoi has been depracated in favour of strtol
return values from main are traditionally EXIT_FAILURE and EXIT_SUCCESS
I would define offset and wrap at their point of first use.  Also it is
generally recommended not to define multiple variables on one line.
In the while loop you should put the fputc(ec, stdout); outside the
if(isalpha(ec)) {...} condition.  That way the format of what you encrypt
is maintained.  Equally, I would omit printing \n at the end so that the
output file (on stdout) has the same form as the input.
I prefer to see a space after keywords, if, while etc.
Don't overuse ?: - it is not a universal replacement for if-else
I find a character named ec to be less intuitive than one named ch, but
maybe that is just me.
I think your encryption code:

    int ascii_offset = islower (ec) ? LSTART : USTART;

    wrap = (ec + offset - ascii_offset) % NALPHA;
    wrap = (wrap < 0) ? NALPHA + wrap : wrap;
    ec = islower(ec) ? wrap + LSTART : wrap + USTART;

would be better written:

    const int ascii_offset = islower(ec) ? LSTART : USTART;
    int wrap = (ec + offset - ascii_offset) % NALPHA;
    if (wrap < 0) {
        wrap += NALPHA;
    }
    ec = wrap + ascii_offset;


Determining whether a list of pathways and its genes are all included in another list I am evaluating if a list of pathways and its genes has pathways included in another pathways. So the way I thought about it is to create a function that does just a single comparison.

all_in <- function(x, y) {
  if (length(x) > length(y)) {
    0
  } else {
    ifelse(all(x %in% y), 1, 0)
  }
}

And then Vectorize to be able to use outer

all_in_vec <- Vectorize(all_in, vectorize.args = c("x", "y"))

And use it as follows:

nested <- outer(paths2genes, paths2genes, all_in_vec)

example:

paths2genes <- structure(list(`1430728` = c("10", "9"), `156580` = c("10", "3",
"9"), `156582` = c("10", "9"), `194840` = c("2", "3"), `211859` = c("10",
"9")), .Names = c("1430728", "156580", "156582", "194840", "211859"
))

Tests:

library(testthat)
expect_true(all(diag(nested) == 1L))
expect_equal(nested[1, 2], 1L)

However I have recently found that Vectorize should be avoided in favor of a built-in vectorization. Is there any built-in method I am missing? How can I vectorize this code ?

If you were to add a cat("hello") at the top of your all_in function, you would find that your function is called 25 times, once for each combination (pair) of pathways. So yes, despite having used Vectorized, it is still essentially a big old loop you have under the hood... Here is how I would write a vectorized function so the heavy-lifting function (%in%, or in my case, match) is only called once or twice:

all_in_outer <- function(list_x, list_y) {
  uniq_x <- unique(unlist(list_x, use.names = FALSE))
  len_x <- vapply(list_x, length, integer(1L))
  as_mat <- function(list_a, ids = uniq_x) {
    vec <- unlist(list_a, use.names = FALSE)
    len <- vapply(list_a, length, integer(1L))
    idx <- rep(seq_along(list_a), len)
    mat <- matrix(0L, nrow = length(list_a), ncol = length(ids),
                  dimnames = list(names(list_a), ids))
    mat[cbind(idx, match(vec, ids))] <- 1L
    mat
  }
  (as_mat(list_x) %*% t(as_mat(list_y)) == len_x) * 1
}

all_in_outer(paths2genes, paths2genes)
#         1430728 156580 156582 194840 211859
# 1430728       1      1      1      0      1
# 156580        0      1      0      0      0
# 156582        1      1      1      0      1
# 194840        0      0      0      1      0
# 211859        1      1      1      0      1

Some explanation: After we have found the list of unique genes across all pathways in the first argument (10,  9, 3, 2), we turn both arguments into a matrix A where A[i,j] = 1 if the pathway i contains the gene j, or 0 otherwise. For paths2genes, this matrix is as_mat(paths2genes):

#         10 9 3 2
# 1430728  1 1 0 0
# 156580   1 1 1 0
# 156582   1 1 0 0
# 194840   0 0 1 1
# 211859   1 1 0 0

Then, using matrix multiplication between two such matrices, you get the number of gene matches for each possible combination of pathways. You then just have to compare that number of matches with the length of the pathway: (as_mat(list_x) %*% t(as_mat(list_y)) == len_x).

In the conversion of each input into a matrix of zero and ones, see this particular line of code: mat[cbind(idx, match(vec, ids))] <- 1L. This is what fills the ones in the matrix. It is completely vectorized, via a single call to match.


Security flaws in smartphone application And could you give me any security and optimization recommendations?

<?php

$datvariable = $_POST['comingover'];

$datvariable = htmlspecialchars($datvariable);

$datvariable = cleanPost($datvariable);

function cleanPost($item)
{ return mysql_real_escape_string(strip_tags(stripslashes($item))) }

$mysqliobject = new mysqli( 'localhost', 'myusername', 'mypassword', 'mydatabase');

if( mysqli_connect_errno( ) )   {   echo "Central SQL Access Malfunction";  exit(); }

$uploadquery = $mysqliobject->prepare(
"INSERT INTO ENTIRETABLE ( WORD, SOUNDCLICKCOUNT ) VALUES (?,'0')
      ON DUPLICATE KEY UPDATE SOUNDCLICKCOUNT = SOUNDCLICKCOUNT+1"
                                        );

$uploadquery->bind_param('s', $datvariable);

$uploadquery->execute();

$mysqliobject->close();

?>

I do not see any fatal error for now.

I plan to have a database that has about 10 tables, and each table will contain about 500 records.  Each column will have 50 characters at max.  Is MySQL good enough for that purpose?  It is for a smartphone application that helps students in East Asia learn the English language.

mysql_real_escape_string

mysql_real_escape_string is deprecated and thus should not be used anymore. As you are using mysqli an alternative would be mysqli_real_escape_string.

But you don't need it anyways, because you are already using prepared statements (which is generally recommended over using any of the real_escape_string functions).

So you can just get rid of cleanPost, it doesn't add any security against SQL injections that the rest of your code doesn't already take care of.

htmlspecialchars

htmlspecialchars is for encoding HTML characters, and used to prevent XSS attacks. You don't need it (and shouldn't use it) when adding data to the database, but when printing user supplied data to a user.

So you should also get rid of the call to htmlspecialchars.

Misc

you are missing a semicolon at the end of your return statement.
you shouldn't just exit, throw an exception instead (so the calling code can decide what to do about it).
Yes, I would assume that MySQL can handle that sort of data on any computer without a problem.


Implementation of peak finder I implemented this peak finder for practice. I have tested it. Everything looks pretty good to me. Please let me know if there is any improvement I can make.

  Peak Finder

  Let input be an array. input[i] is a peak if input[i] >= input[i + 1] and input[i] >= input[i - 1].

  Implement a recursive algorithm of time complexity \$O(\log{n})\$ to find a peak.

class PeakFinder <E extends Comparable<? super E>>{
  private E[] array;

  public PeakFinder(E[] array){
    if(array == null || array.length == 0)
      throw new IllegalArgumentException("array cannot be null or 0");
    this.array = array;
  }

  public E find(){
    return findUtil(0, array.length - 1);
  }

  private E findUtil(int left, int right){
      int mid = (right - left)/2 + left;
      int newLeft = mid + 1;
      int newRight = mid - 1;

      if(newLeft <= right && array[newLeft].compareTo(array[mid]) > 0)
        return findUtil(newLeft, right);
      else if(newRight >= left && array[newRight].compareTo(array[mid]) > 0)
        return findUtil(left, newRight);
      else
        return array[mid];
  }
}

An array always has at least one peak if it is not null or empty.

Special cases:

input[i] is a peak if input[i] >= input[i+1] when input[i] is the first element in the array.
input[i] is a peak if input[i] >= input[i-1] when input[i] is the last element in the array.
input[i] is a peak if input[i] is the only element in the array.

Your code is pretty good. It has elements in it which are relatively sophisticated too. For example, the recursion is clear, and the midpoint function is "right" (that needs an explanation - there's a 'well known' midpoint bug that many midpoint calculations have, but yours does not).

I prefer 1-liner code to be braced, and yours is not. That's disappointing, because I don't like reviews that focus on that only.... but that's really all I can find wrong with your code style.

There are a number of technical problems though.

why throw an exception for an empty array? Why not return null?
you have not followed the instructions correctly... it is impossible for a 0, 1, or 2 element array to actually have a peak, since there is no i - 1 or i + 1 to compare against. Additionally, there is no peak in cases like [3, 2, 2, 3], since the 2's are not peaks, and the three's are at the edges.
your first if conditions are newLeft <= right and newRight >= left, and those are just messy.... your findUtil should have a recursion-terminating condition if (right < left) { return null; } and not spread it around in the method.
I am a pragmatist about OOP in Java... Objects are great, I agree, but there is no reason to have an instance for this code. A static method passing the input array in would be fine....:

public <E extends Comparable<? super E>> E findPeak(E[] data) { ....

Finally, about the algorithm. I am not convinced that it works... I have not run it, but I am pretty sure that the compareTo methods are the wrong way around... they should be < 0 and not > 0)....


Checking to see whether a document can be deleted My code looks like this:

public void checkIfCanDelete() throws BusinessException {

    boolean canDelete = canDelete();

    if (canDelete) {

        checkIfLocked();

        if(!editable) {
            throw new BusinessException(BusinessError.MSG_YOU_ARE_NOT_AUTHORIZED_TO_REMOVE_DOCUMENTS);
        }

        return;
    }

    if (newDoc) {
        throw new BusinessException(BusinessException.MSG_DOCUMENT_CANNOT_DELETE_A_NEW_DOCUMENT);
    }

    if (getSetting().isExcludedFromDeploy()) {
        throw new BusinessException(BusinessException.MSG_DOCUMENT_CANNOT_DELETE_A_READ_ONLY_DOCUMENT);
    }

    checkIfLocked();

    // Default Message:
    throw new BusinessException(BusinessError.MSG_YOU_ARE_NOT_AUTHORIZED_TO_REMOVE_DOCUMENTS);
}

In my case if canDelete is true, then I have to call checkIFLocked and throw exception if not editable.

Here I think that I have a duplicate code witch is throwing the same exception, or I did called the same method twice in the same method block, which is checkIfLocked.

So is there any way to enhance this code block? Do I need to call the same method twice in the same method block?

I assume that checkIfLocked() returns void, and throws a BusinessException if the document is locked.

There is one way to succeed, and many ways to fail.  I think it would be beneficial to rearrange the code to make it obvious what the criteria for success are:

public void checkIfCanDelete() throws BusinessException {
    if (editable && canDelete()) {
        checkIfLocked();
        return;  // Can delete
    }

    // Can't delete.  We just have to choose a reason for the denial.
    if (canDelete()) {
        assert !editable;
        throw new BusinessException(BusinessError.MSG_YOU_ARE_NOT_AUTHORIZED_TO_REMOVE_DOCUMENTS);
    } else if (newDoc) {
        throw new BusinessException(BusinessException.MSG_DOCUMENT_CANNOT_DELETE_A_NEW_DOCUMENT);
    } else if (getSetting().isExcludedFromDeploy()) {
        throw new BusinessException(BusinessException.MSG_DOCUMENT_CANNOT_DELETE_A_READ_ONLY_DOCUMENT);
    }
    checkIfLocked();
    throw new BusinessException(BusinessError.MSG_YOU_ARE_NOT_AUTHORIZED_TO_REMOVE_DOCUMENTS);
}

The code above preserves the same logic as the original.  However, if you're not picky about which reason you pick for the denial, you could simplify the code further:

public void checkIfCanDelete() throws BusinessException {
    checkIfLocked();

    if (!(editable && canDelete())) {
        throw new BusinessException(
            newDoc ?
                BusinessException.MSG_DOCUMENT_CANNOT_DELETE_A_NEW_DOCUMENT :
            getSetting().isExcludedFromDeploy() ?
                BusinessException.MSG_DOCUMENT_CANNOT_DELETE_A_READ_ONLY_DOCUMENT :
                BusinessError.MSG_YOU_ARE_NOT_AUTHORIZED_TO_REMOVE_DOCUMENTS;
        );
    }
}


Query to retrieve available stock I have a query that outputs a list of products with some fields from other tables. That query works fine.
Now, I'm attempting to retrieve the available stock for each product in that list. I'm not sure if I'm doing it correctly or in the most performant way. Here's what I've got working so far.
select distinct
p.id,
p.name
if(coalesce(x.in - x.out, 0) = 0, 0, 1) as quantity,
[other fields from joined tables]
from products p
[a bunch of inner joins]
left join (
    select
    s.id_products,
    sum(case when s.type = 'in' then s.quantity else 0 end) as 'in',
    sum(case when s.type = 'out' then s.quantity else 0 end) as 'out'
    from stock s
    group by s.id_products
) x on x.id_products = p.id_products
group by id

EDIT: I made a mistake in the query the if statement in the select is just for a flag display. Please replace the following.

if(coalesce(x.in - x.out, 0) = 0, 0, 1) as quantity,

(x.in - x.out, 0) as quantity,

Discarded sums
You calculate these sums:
sum(case when s.type = 'in' then s.quantity else 0 end) as 'in',
sum(case when s.type = 'out' then s.quantity else 0 end) as 'out'

but then reduce them to a 0/1 here:
if(coalesce(x.in - x.out, 0) = 0, 0, 1) as quantity,

This means that a sum is not appropriate at all. This should be refactored to an if exists, where the predicate compares s.type to in/out and checks that s.quantity > 0.


Retrieving a message from a cryptocurrency daemon The objective of this piece of code is to get a message from a cryptocurrency daemon through RPC and extract the transactions object to pass it to the view. Is there a simpler / less ugly way of doing this?

$transaction_pool = json_decode($rpc->getTransactionPool(), false);

if(isset($transaction_pool->transactions)){
    $transaction_pool = $transaction_pool->transactions;
}else{
    $transaction_pool = (object)NULL;
}

View partial:

@forelse ($transaction_pool as $transaction)
<?php $transaction_json = json_decode($transaction->tx_json); ?>

{{ $transaction->id_hash }}

        @if ($transaction_json->version == 2)

&nbsp;confidential

        @else

@coin($transaction_json->amount)

        @endif

@coin($transaction->fee)

    @empty

No transactions

@endforelse

    Transactions ({{ count($transaction_pool) }})

I don't get the point where you cast null to object.

I'm not sure what are you trying to achive,
but you can always make if 'less ugly'.

$transcations_pool = json_decode($rpc->getTransactionPool(), false);
$transactions_pool = isset($transactions_pool->transactions) ?
                         $transaction_pool->transactions :
                         (object) null;

Also i would move div .show-grid to separate file and load it like this:

@include('path/to/file', [ 'transaction_json' => json_decode($transaction->tx_json) ])

Or you can implement special method like $transaction->decodeTxJson() to handle this decoding in $transaction class.