Searching through tables for a value I have a PL/SQL stored procedure which searches for a row in 3 different tables (not all columns are the same).

The logic goes as follows:

Search for the item in the first table
  If item was found
      select the details of it from the first table
  If item was not found
      search for the item in the second table
          If Item was found
              Select the details of it from the second table
          If item was not found
              Select the item from the third table (returning blank if it's not found here neither)

Here's the actual code (actual symbol names modified for security):

PROCEDURE GETREQUEST(REQUESTID IN NUMBER, request OUT pkg_request.refcur)
 IS
requestFound NUMBER := 0;
BEGIN

            -- Search for the request ID in the first table
            SELECT COUNT(REQ_ID) INTO requestFound FROM FIRSTTABLE
            WHERE FIRSTTABLE.REQ_ID = REQUESTID
            AND (REQ_STATUS = 'D' OR REQ_STATUS = 'A');

            IF(REQUESTFOUND > 0) THEN
                    -- Select the request details
                    OPEN REQUEST FOR
                    SELECT REQ_ID, REQ_TYPE_STATUS FROM FIRSTTABLE
                    WHERE FIRSTTABLE.REQ_ID = REQUESTID
                    AND (REQ_STATUS = 'D' OR REQ_STATUS = 'A');
            ELSE
                    -- Search for the request from the second table
                    SELECT COUNT(REQ_ID) INTO REQUESTFOUND FROM SECONDTABLE
                    WHERE SECONDTABLE.REQ_ID = REQUESTID;

                    IF(REQUESTFOUND > 0) THEN
                              -- Select the request details from second table
                              OPEN REQUEST FOR
                              SELECT REQ_TYPE, '-' AS REQ_TYPE_STATUS FROM SECONDTABLE
                              WHERE SECONDTABLE.REQ_ID = REQUESTID;
                      ELSE
                              -- Get the request from third table (will return as blank if nothing found)
                              OPEN REQUEST FOR
                              SELECT REQ_TYPE, '-' AS REQ_TYPE_STATUS  FROM THIRDTABLE
                              WHERE THIRDTABLE.REQ_ID = REQUESTID;
                      END IF;
            END IF;
END GET_REQUEST

My issues with this are as follows:

It's not very concise, with a lot of nesting.
It's not very efficient, since it needs to perform a count, then if it exists it selects the details. That's two operations. How can I modify it to have a single operation for each search?

Also, the REQ_TYPE_STATUS column doesn't exist in the second and third table, so as you can see I have just assigned the value '-' to the returned result, indicating a blank value. I'm not really sure if there's a better or more standard way of doing this.

I am not sure there is a way to reduce the number of if-statements and nesting. What is possible though is to improve the performance of the conditions you have that check those conditions.

The count(*) is inefficient. Consider adding a 'ROWNUM = 1' condition to your where clause in a way that makes the following conditions possible:

        SELECT COUNT(REQ_ID) INTO requestFound
        FROM FIRSTTABLE
        WHERE FIRSTTABLE.REQ_ID = REQUESTID
          AND (REQ_STATUS = 'D' OR REQ_STATUS = 'A')
          AND ROWNUM = 1;

This will be optimized by Oracle to exit the condition on the first matching result, ending up with a requestFound value that will be either 0, or 1. A value of 1 means there is some data, and does not indicate how much data.

Reading up on ROWNUM is useful. Ask Tom!


Find and delete an item from a singly linked list in Python Given a Singly Linked-List, implement a method to delete the node that contains the same data as the input data.

Example:
delete(1->2->3->4,3) ==> 1->2->4

class SinglyLinkedList:
    #constructor
    def __init__(self):
        self.head = None

    #method for setting the head of the Linked List
    def setHead(self,head):
        self.head = head

    #method for deleting a node having a certain data
    #method for deleting a node having a certain data
    def delete(self,data):
        if self.head is None:
            return None
        else:
            cur  = self.head
            prev = None
            while cur.data != data and cur.next is not None:
                prev = cur
                cur = cur.next

            # when found
            if cur.data == data:
                # if head
                if cur == self.head:
                    if cur.next is None:
                        self.head = None
                    else:
                        self.head = cur.next
                else:
                    if cur.next is None:
                        prev.next = None
                    else:
                        prev.next = cur.next
            else:
                return None

A few general things:

Use """Doc strings.""" for documentation not # comments
Put a space after each comma
Prefer less indentation. For example, if you have an if blah: return None, then you don't need an else after it, because if the function didn't return then blah must've been false
PEP8 prefers snake_case for methods and variables. setHead should be set_head, although I'm a bit dubious that you should be exposing this to the user. The head should be a private implementation detail. You should expose methods to allow the user to modify the list instead.
Additionally, for parity with builtin collection types, I'd take an optional iterable as the first argument to your constructor. If provided, you populate your collection with the values from it.
I prefer value to data again for parity with python builtins

A warning about equality:

You use cur == self.head. Depending on how equality is implemented for your nodes this may or may not work. In this situation, I think you are lucky to avoid this edge case due to delete removing the first thing, but in general this is dangerous. To test if two things are the exact same object use is. Use == for equality. (1, 2) == (1, 2) but (1, 2) is not (1, 2).

Specifically relating to delete:

I'd rename it to remove for parity with the method on the builtin list
I'd have it raise a ValueError when the value isn't in the list (again for parity with the method on the builtin list)
Despite what @vnp suggests, I wouldn't return the value deleted again for parity with remove on the builtin list. It doesn't really make much sense to return the value since you know it already (you passed it into the method). This is what list.remove does.
Since many operations on the linked list require finding a node with a given value and the previous node (ex. in, find, insert_before), I'd extract find value or raise ValueError into a helper. This will cut down on the complexity of your delete operation (you now just have to stitch the hole in the list)
You have some duplicated logic for setting self.head/prev.next. No matter whether cur.next is None or not you set self.head/prev.next to cur.next. But, I'd solve this another way:
One trick commonly employed to make the linked lists a bit less unwieldy is always having an empty node at the head (with no data). This means that you never have to special case patching the head pointer.

Combining this all:

class Node:
    """Node in SinglyLinkedList."""
    def __init__(self, value, next):
        self.value = value
        self.next = next

class SinglyLinkedList:
    def __init__(self, iterable=None):
        """New list initialized from optional iterable's items."""
        self._head = Node(None, None)

        # Not implemented here, but like list.append,
        # should take ...args and append each. In this
        # way you can do O(n) appends instead of O(n^2)
        self.append(iterable)

    def remove(self, value):
        """Removes value from the list or raises ValueError."""
        previous_node, matched_node = self._find_value(value)
        previous_node.next = matched_node.next

    def _find_value(self, value):
        previous, current = self._head, self._head.next

        while current is not None:
            if current.value == value:
                return previous, current

            previous, current = current, current.next

        raise ValueError(f'value not in list')


Dynamic Array Implementation in C++ I've finished implementing a dynamic array in C++. I'm a beginner in C++ and also in algorithms and data structures, so any constructive feedback about improving the implementation would be greatly appreciated.

#include <iostream>

using namespace std;
#define INITIAL_CAPACITY 5

template <class T>
class dynamic_array {
    T *array;
    int MIN_CAPACITY = INITIAL_CAPACITY;
    int GROWTH_FACTOR = 2;
    int size;

public:
    // constructor init
    dynamic_array() {
        array = new T[MIN_CAPACITY];
        size = 0;
    }

    // append @ the end
    void append(T element) {
        if(size == MIN_CAPACITY) {
            resize();
        }
        array[size] = element;
        size++;

    }

    void deleteAt(int pos) {
        if((pos > size) || (pos < 0)) {
            cout << "Invalid index";
            return;
        }

        for(int i = pos; i <= size; i++) {
            array[i] = array[i+1];
        }
        size--;
    }

    void insertAt(int element, int pos) {
        if((pos > size) || (pos < 0)) {
            cout << "Invalid index";
            return;
        }

        if(size == MIN_CAPACITY) {
            resize();
        }

        size++;

        for(int i = size - 1; i >= pos; i--) {
            if(i == pos) {
                array[i] = element;
            } else {
                array[i] = array[i-1];
            }
        }
    }

    // returns size of array
    int length() {
        return size;
    }

    // doubles capacity if it has to and deletes reference to current array.
    void resize() {
        MIN_CAPACITY *= GROWTH_FACTOR;
        T *temp = new T[MIN_CAPACITY];
        copy(temp);
        delete [] array;
        array = temp;

    }

    // copies original array into temp
    void copy(T temp[]) {
        for(int i = 0; i < size; i++) {
            temp[i] = array[i];
        }
    }

    // returns element in x position.
    T get(int pos) {
        return array[pos];
    }
};

int main() {
    dynamic_array<int> dynArr;
    dynArr.append(3);
    dynArr.append(4);
    dynArr.append(5);
    dynArr.append(4);
    dynArr.append(33);
    dynArr.append(3);

    dynArr.deleteAt(6);

    return 0;
}

The code is pretty good as it is. Here is how I would improve it:

Constant handling

In C++, you should prefer const over #define. So

#define INITIAL_CAPACITY 5

becomes

const int INITIAL_CAPACITY = 5;

Also don't use all uppercase names for non constant variables. It is
confusing and breaks the 50 year old tradition.

Clearer names

I renamed a few of your variables:

MIN_CAPACITY => capacity Because the instance variable holds the
current capacity of the dynamic array, not the minimum capacity.
length() => size The words length and size are synonymous but
using them both can cause confusion. A reader might think is the
length the number of elements in the array and size the allocacted
size, or vice versa?

Offby one errors

You have an offby one error in deleteAt. If you have a dynamic array
with 5 elements, then their positions are 0 to 4 so deleteAt(5)
shouldn't work, but it does. I've fixed that for you.

Pretty printing

Adding pretty printing functions is almost always a good idea because
they make debugging much easier. I've added one for you.

Error handling

Your error handling consists of printing to stdout and letting the
program continue to run. That is incorrect because errors might not be
discovered if stdout is redirected or the user is not paying attention
to what is printed.

There are many ways to handle errors. I've implemented basic assert
based error handling for you. But you can certainly be fancier and use
exceptions or status codes.

Functions implemented in terms of each other

For all dynamic arrays dynarr.append(x) is equivalent to
dynarr.insertAt(x, dynarr.size()). So append can just call
insertAt.

Destructor

Your dynamic array allocates memory in the constructor, but there is
no corresponding destructor that frees the memory. I've added one
looking like this:

~dynamic_array() {
    delete[] array;
}

Copying memory

There's a function called std::copy which you can use for copying
memory. That way, you don't have to write your own copy function.

Pointless comments

Writing good comments is hard. As a reviewer I much prefer no comments
over pointless comments. An example of a pointless comment is //
constructor init. I can see that the lines below is the constructor
so the comment doesn't tell me anything I didn't already know. Same
for the comment // returns size of array.

Source code

Here is the improved version of the dynamic array:

#include <assert.h>
#include <cstring>
#include <iostream>

using namespace std;

const int INITIAL_CAPACITY = 2;
const int GROWTH_FACTOR = 2;

template <class T>
class dynamic_array {
    T *array;
    int capacity = INITIAL_CAPACITY;
    int _size;

public:
    dynamic_array() {
        array = new T[capacity];
        _size = 0;
    }

    ~dynamic_array() {
        delete[] array;
    }

    void deleteAt(int pos) {
        assert(0 <= pos && pos < _size);
        _size--;
        for (int i = pos; i < _size; i++) {
            array[i] = array[i + 1];
        }
    }

    void insertAt(int element, int pos) {
        assert(0 <= pos && pos <= _size);
        if(_size == capacity) {
            resize();
        }
        for(int i = _size; i > pos; i--) {
            array[i] = array[i-1];
        }
        _size++;
        array[pos] = element;
    }

    void append(T element) {
        insertAt(element, _size);
    }

    int size() {
        return _size;
    }

    // doubles capacity if it has to and deletes reference to current array.
    void resize() {
        capacity *= GROWTH_FACTOR;
        T *temp = new T[capacity];
        copy(array, array + _size, temp);
        delete [] array;
        array = temp;
    }

    T get(int pos) {
        return array[pos];
    }

    void pretty_print() {
        cout << "[";
        for (int i = 0; i < _size - 1; i++) {
            cout << array[i] << " ";
        }
        if (_size) {
            cout << array[_size - 1];
        }
        cout << "]\n";
    }
};


Convert an amount to Indian Notation I need to convert an amount to an Indian currency format:

import decimal
def currencyInIndiaFormat(n):
  d = decimal.Decimal(str(n))
  if d.as_tuple().exponent < -2:
    s = str(n)
  else:
    s = '{0:.2f}'.format(n)
  l = len(s)
  i = l-1;
  res = ''
  flag = 0
  k = 0
  while i>=0:
    if flag==0:
      res = res + s[i]
      if s[i]=='.':
        flag = 1
    elif flag==1:
      k = k + 1
      res = res + s[i]
      if k==3 and i-1>=0:
        res = res + ','
        flag = 2
        k = 0
    else:
      k = k + 1
      res = res + s[i]
      if k==2 and i-1>=0:
        res = res + ','
        flag = 2
        k = 0
    i = i - 1

  return res[::-1]

def main():
  n = 100.52
  print "INR " + currencyInIndiaFormat(n)  # INR 100.52
  n = 1000.108
  print "INR " + currencyInIndiaFormat(n)  # INR 1,000.108
  n = 1200000
  print "INR " + currencyInIndiaFormat(n)  # INR 12,00,000.00

main()

Is there a way to make my currencyInIndiaFormat function shorter, more concise and clean? Is there a better way to write my currencyInIndiaFormat function?

Indian Currency Format:

For example, numbers here are represented as:

1
10
100
1,000
10,000
1,00,000
10,00,000
1,00,00,000
10,00,00,000

Refer: Indian Numbering System

1. Style guides

leave two empty lines before each function (when you're not inside of a class)
Python is not C; don't use ;
put a space before and after operands. i>=0 will become i >= 0, s[i]=='.' will become s[i] == '.' and so on.
use the snake_case notation for function names: currencyInIndiaFormat(..) will become currency_in_indian_format(..)
use format() instead of + notation: e.g: print "INR " + currencyInIndiaFormat(n) might be print "INR {}".format(currencyInIndiaFormat(n))
also, I'd recommend you use print('w/e you want here') so that your code can be python 3 compatible.
use 4 spaces for indentation instead of 2.
use augmented assignments where you can: res = res + s[i] can be written as res += s[i]
add docstrings to your functions to describe what their purpose are

If you want to read more about Python style guide, you should start from here.
Preview:
import decimal

def currency_in_indian_format(n):
    """ Convert a number (int / float) into indian formatting style """
    d = decimal.Decimal(str(n))

    if d.as_tuple().exponent < -2:
        s = str(n)
    else:
        s = '{0:.2f}'.format(n)

    l = len(s)
    i = l - 1

    res, flag, k = '', 0, 0
    while i >= 0:
        if flag == 0:
            res += s[i]
            if s[i] == '.':
                flag = 1
        elif flag == 1:
            k += 1
            res += s[i]
            if k == 3 and i - 1 >= 0:
                res += ','
                flag = 2
                k = 0
        else:
            k += 1
            res += s[i]
            if k == 2 and i - 1 >= 0:
                res += ','
                flag = 2
                k = 0
        i -= 1

    return res[::-1]

def main():
    n = 100.52
    print("INR {}".format(currency_in_indian_format(n)))  # INR 100.52
    n = 1000.108
    print("INR {}".format(currency_in_indian_format(n)))  # INR 1,000.108
    n = 1200000
    print("INR {}".format(currency_in_indian_format(n)))  # INR 12,00,000.00

if __name__ == '__main__':
    main()

NOTE: I've also added if __name__ == '__main__'. By doing the main check, you can have that code only execute when you want to run the module as a program and not have it execute when someone just wants to import your module and call your functions themselves.
2. Algorithm
As I said in the comments, I would strongly recommend using the existing builtin module locale, which is doing exactly what you want:
Example code (it's working if you have the locale set on your machine - so that would be an ugly constraint):
>>> import locale
>>> locale.setlocale(locale.LC_ALL, 'en_IN.utf8')  # if this doesn't work, you might try as well: locale.setlocale(locale.LC_ALL, '')
'en_IN'
>>> locale.format("%d", 1255000, grouping=True)
'12,55,000'

I'll let others to comment on your code regarding the efficiency of the algorithm you implemented and methods to make it shorter / better.


Decide if number is Armstrong number I just recently started reading the python.org documents and it looked like a really interesting language. Now, since Visual Studio 2017 supports Python, I decided to finally actually start learning it. My friend challenged me to write a program that checks if a number is an Armstrong number.

import sys

def isArmstrong(number):
    arr = str(number)
    count = len(arr)
    res = 0
    for x in range(0, count):
        res += int(arr[x]) ** count
    return res == number

if len(sys.argv) > 1:
    arg = sys.argv[1]
    print(arg + " is an armstrong number: " + str(isArmstrong(int(arg))))
else:
    print("No arguments passed! :-(");

Now this is the first solution that I could come up with. As you can see, it converts the number to a string to determine the amount of digits, and then handle them individually. Is this the smartest way to do it, or can it be done without converting to a string? If not: Is there anything I can do to improve this code performance-wise?

By PEP 8, Python's official style guide, function names should generally be lower_case_with_underscores.  Also, the final semicolon should be omitted.

The function would typically be written more elegantly using the sum() builtin function, with a generator expression.  (It's not any faster than your original code, though.  Actually, a tiny bit slower.)

I don't recommend using arr as a variable name, since it's neither descriptive (what does the array represent?) nor accurate (it's actually a string).

For the printout, I recommend using str.format() or one of the other formatting mechanisms.

import sys

def is_armstrong(number):
    digits = str(number)
    length = len(digits)
    return number == sum(int(digit) ** length for digit in digits)

if len(sys.argv) > 1:
    arg = int(sys.argv[1])
    print('{} is an Armstrong number? {}'.format(arg, is_armstrong(arg)))
else:
    print("No arguments passed! :-(")


Maxmin algorithm implementation I'm currently taking an algorithm course in college. To find the maximum and minimum elements in an array, I'm using a divide and conquer algorithm. Please offer suggestions for this code snippet.  I get a headache reading only comments, so please give explanation with sample code.

#include<stdio.h>

#define ARRAY_SIZE(array) (sizeof(array) / sizeof(array[0]))

int tempmax, tempmin;

/*
 * return a 2 element array
 * First element is highest of list
 * Second elements is lowest of list
 *
 */
int* maxmin(const int list[], const int low, const int high, int max, int min)
{
    int mid,max1,min1;
    static int maxAndMin[2]; // to hold the max and min value of list

    // list has only one element
    // so make max and min that element
    if (low == high)
    {
        max = list[low];
        min = list[low];
    }

    // list has two elements then
    // check for which one is greater or smaller
    // and check it with temp values and update
    else if (low == high-1)
    {
        if (list[low] < list[high])
        {
            tempmax = getMax(tempmax, list[high]);
            tempmin = getMin(tempmin, list[low]);
        }
        else
        {
            tempmax = getMax(tempmax, list[low]);
            tempmin = getMin(tempmin, list[high]);
        }
    }

   else
   {
       // if list is not small, divide list into sub-problems
       // Find where to split the set
       mid = (low + high) / 2;
       // Solve the sub-problems
       max1 = list[mid+1];
       min1 = list[mid+1];
       maxmin(list, low, mid, max, min);
       maxmin(list, mid+1, high, max1, min1);
       // Combine the solutions
       tempmax = getMax(tempmax, max1);
       tempmin = getMin(tempmin, min1);
   }

    maxAndMin[0] = tempmax;
    maxAndMin[1] = tempmin;
    return maxAndMin;
}

/*
 * returns the highest element between first and second
 *
 */
int getMax(int first, int second)
{
    return first > second ? first : second;
}

/*
 * returns the lowest element between first and second
 *
 */
int getMin(int  first, int second)
{
    return  first < second ?  first : second;
}

int main(void)
{
    int list[] = {10, 23, 24, 56, 67, 78, 90};
    int *values;
    int size = ARRAY_SIZE(list);

    tempmax = tempmin = list[0];
    values = maxmin(list, 0, size-1, list[0], list[0]);

    printf("The maximum value is = %2d \n", *values);
    printf("The minimum value is = %2d \n", *(values+1));
    return 0;
}

Some comments:

Firstly the interface is odd.  Normally when you pass an array, you pass it's
size and a pointer to the first element.  In the context of your function this
means:

int* maxmin(const int list[], size_t size, int max, int min);

Then you call this with:

maxmin(list, mid, max, min);
maxmin(list + mid, size - mid, max1, min1);

or from main

values = maxmin(list, size, list[0], list[0]);

This saves a call parameter (fewer is always preferable) and makes the
function more 'normal'.

Beyond that, what stand out are the return of a static array and the use of
globals.  Maybe thread safety is not on your mind and this can be ignored, but
bear in mind that returning a static is rarely done.  And globals are often best avoided, if they can be.

The other thing that strikes me is the awkwardness of the code.  Arguably a
matter of opinion, there are many variables and their names are hard to tell
apart.  More importantly the need to find both max and min values complicates
the function enormously.

Consider the same task but only looking for the minimum value:

int min(const int list[], const size_t size)
{
    if (size == 1) {
        return list[0];
    }
    else if (size == 2) {
        return list[0] < list[1] ? list[0] : list[1];
    }
    size_t half = size / 2;
    int n = min(list, half);
    int m = min(list + half, size - half);
    return n < m ? n : m;
}

This is so much simpler!  And the max version just involves reversing the
two comparisons.  I would be looking for an excuse to use these rather than
your combined function.  Against my functions is the fact that calling max
and min will be slower than just calling yours (but less than 2:1).  In
their favour, you might not always want both values.

As this is just an exercise, you don't have to care.  But in practice I
believe strongly that it is always worth simplifying the
requirement/specification if it results in simpler code.


Does my input helper class look good? I'm new to OOP PHP and I created a simple input helper class. And was wondering if it's good? Are my dependencies good?

Also the reason I'm not throwing an exception with the get method is that I will mainly use this class for form validation purposes.

I'll take any suggestion for improvement!

class Input {
    private $_get,
            $_post;

    public function __construct(Array $get, Array $post) {
        $this->_get  = $get;
        $this->_post = $post;
    }

    public function exists($type = 'POST') {
        switch($type) {
            case 'POST':
                return (!empty($this->_post)) ? true : false;
            break;
            case 'GET':
                return (!empty($this->_get)) ? true : false;
            break;
            default:
                return false;
            break;
        }
    }

    public function get($input, $source = 'POST') {
        if($source == 'POST' && isset($this->_post[$input])) {
            return $this->_post[$input];
        } elseif($source == 'GET' && isset($this->_get[$input])) {
            return $this->_get[$input];
        }
        return false;
    }
}

I figured I could made my code shorter:

class Input {
    private $_source;

    public function __construct(Array $source) {
        $this->_source = $source;
    }

    public function exists() {
        return !empty($this->_source);
    }

    public function get($input) {
        return (isset($this->_source[$input])) ? $this->_source[$input] : false;
    }
}

In Java and C# you can simply put

return !empty($this->_source);

and

return isset($this->_source[$input]);

since the evaluation of that is a boolean by itself and thus there is no need for the additional ternary operation. I would suspect that this is the same in PHP.


Installing and removing kernel module RPMs First, the script checks to see if it is installing two kernel module packages or removing them by looking at the first parameter. If no parameter is provided it complains and exits.

If installing, it checks if each of the packages even needs to be installed by looking at the version of the existing module. If they do need to be installed, install them; If they don't, simply say so and the rest of the script runs its course.

If I am removing them just run the command to do so.

The problem is, I have a lot of if...else...fi and if...elif...fi blocks setting two variables and then testing different combinations of the two. Even though the script works as is I feel like there should be a better way to do this.

I moved one block into a function even though this just put it somewhere else. It didn't actually clean it up at all.

#!/bin/bash

awk='/bin/awk'
enic_pkg="/home/$(logname)/kmod-enic-2.1.1.75-rhel6u5.el6.x86_64.rpm"
fnic_pkg="/home/$(logname)/kmod-fnic-1.6.0.12b-1.el6.x86_64.rpm"
modinfo='/sbin/modinfo'

if [ ! $1 ]; then
    echo "Script must be executed with either 'install' or 'remove' as a paramter"
    exit

else
    method=$1

fi

test_ood() {
    if [ ! $enic = "2.1.1.75" ]; then
      enic_ood=true

    else
      echo "enic module is already up-to-date: $enic"

    fi

    if [ ! $fnic = "1.6.0.12b" ]; then
      fnic_ood=true
    else
      echo "fnic module is already up-to-date: $fnic"

    fi

}

if [ $method == "install" ]; then
    yum='/usr/bin/yum -y localinstall'
    enic=$(${modinfo} enic |grep "^version" |${awk} '{print $2}')
    fnic=$(${modinfo} fnic |grep "^version" |${awk} '{print $2}')

    test_ood

    if [ $enic_ood -a $fnic_ood ]; then
      echo "Both enic and fnic modules are out of date."
      echo "enic: $enic"
      echo "fnic: $fnic"
      echo "Installing kmod-enic and kmod-fnic..."
      ${yum} $enic_pkg $fnic_pkg

    elif [ $enic_ood -a ! $fnic_ood ]; then
      echo "Only the enic module is out of date: $enic"
      echo "Installing kmod-enic..."
      ${yum} $enic_pkg

    elif [ ! $enic_ood -a $fnic_ood ]; then
      echo "Only the fnic module is out of date: $fnic"
      echo "Installing kmod-fnic..."
      ${yum} $fnic_pkg

    else
      echo "Both modules are up-to-date. Nothing to do; exiting..."
      exit

    fi

    test_ood

elif [ $method == "remove" ];then
    yum='/usr/bin/yum -y remove'

    ${yum} kmod-enic kmod-fnic

else
    echo "Unrecognized option: $method"
    exit

fi

I was thinking I could make the $method if tests case switches and put the code for each of those if blocks into methods of their own. That would at least eliminate the outer if...elif...fi block. Although, I'd only be converting it to another type of block.

Should this be a shell script?

You mentioned that you wrote this script to push the packages to over 60 servers.  If you have that many servers, then you should have a configuration management tool to do these kinds of tasks for you and much, much more.  Typical solutions include Ansible, CFengine, Chef, and Puppet.

Instead of copying the RPMs to each machine, you should probably use a YUM repository, which may be one that you operate yourself, if those are self-built RPMs.

Version check

modinfo enic |grep "^version" |awk '{print $2}' would be better written as modinfo -F version enic 2>/dev/null.  I recommend discarding error messages there, in case no such module is available.

However, I wonder why you bother querying modinfo at all, instead of just running yum and letting it automatically decide whether anything needs to be installed at all.

The conditionals are slightly redundant.  You could just write

if [ $enic_ood -a $fnic_ood ]; then
    …
elif [ $enic_ood ]; then
    …
elif [ $fnic_ood ]; then
    …
else
    …
fi

I get the feeling, though, that nearly everything in the script is written twice.  You could write a loop that handles both packages:

need_rpms=
for module in enic fnic ; do
    rpm=$HOME/kmod-$module-*.rpm
    want_version=$(rpm -qp --queryformat '%{VERSION}' $rpm)
    installed_version=$(/sbin/modinfo -F version "$module" 2>/dev/null)
    if [ "$want_version" = "$installed_version" ]; then
        echo "$module is already up-to-date: $installed_version"
    else
        need_rpms="$need_rpms $rpm"
    fi
done

if [ -n "$need_rpms" ]; then
    /usr/bin/yum -y localinstall $need_rpms
done

Alternatively, you could have one script that takes parameters, and run it once for each package.

Program structure

There should be an install() function and a remove() function.

Don't set yum='/usr/bin/yum -y localinstall' in one place and yum='/usr/bin/yum -y remove' in another place.  I would expect it to be uniformly defined as yum='/usr/bin/yum -y', just like modinfo='/sbin/modinfo'.

This script always appears to succeed.  Fatal errors should cause the script to exit with a non-zero status, and error messages should be printed to standard error.  Exit statuses matter; even when executing the script over SSH, for example, the error status propagates all the way back to become the exit status of SSH.

install() {
    …
}

remove() {
    …
}

case "$1" in
  install|remove)
    "$1"
    ;;
  *)
    echo "$0 must be executed with either 'install' or 'remove'" >&2
    exit 1
    ;;
esac


Command Pattern with Undo , returning response in Invoker and Command class or Callback? I have used command pattern with undo support. The terms associated with Command Pattern are as follows

Client → Invoker → Command instance execute() → Receiver

so the client will need to know whether the operation was successfully so that it can decide whether to undo.

In my example, Receiver talks to the Volt DB using some APIs, so there are predefined response classes with result code and result message.

It would be bad if I sent those all the way to the client.

Having Invoker and Command to return boolean indicating success or failure feels better. I have seen an example where they use callbacks. Is this something only for asynchronous operations or I can also use it here? Which would be better?

Snippets of the code are below; full code is available on GitHub.

Invoker class

package com.spakai.undoredo;

import java.util.Stack;

public class Invoker {

    private final Stack<Command> undoStack;
    private final Stack<Command> redoStack;

    public Invoker() {
        undoStack = new Stack<>();
        redoStack = new Stack<>();
    }

    public void execute(Command cmd) {
        undoStack.push(cmd);
        redoStack.clear();
        cmd.execute();
    }

    public void undo() {
        if (!undoStack.isEmpty()) {
            Command cmd = undoStack.pop();
            cmd.undo();
            redoStack.push(cmd);
        }
    }

    public void redo() {
        Command cmd = redoStack.pop();
        cmd.execute();
        undoStack.push(cmd);

    }
}

Receiver interface

package com.spakai.undoredo;

public interface Receiver {
    public CreateGroupResponse createGroup(int groupId, int subscriptionId);
    public DeleteGroupResponse deleteGroup(int groupId);
    //many more
}

Command interface

package com.spakai.undoredo;

public interface Command {
    public void execute();
    public void undo();
    public void redo();
}

Sample Command instance

package com.spakai.undoredo;

public class CreateGroupAndSubscription implements Command {

    // which states do i need to store in order to execute and undo
    private int groupId;
    private int subscriptionId;

    // this is the Volt handle apis that talks to VoltDB
    private Receiver receiver;

    public CreateGroupAndSubscription(int groupId, int subscriptionId, Receiver receiver) {
        this.groupId = groupId;
        this.subscriptionId = subscriptionId;
        this.receiver = receiver;
    }

    @Override
    public void execute() {
        CreateGroupResponse response = receiver.createGroup(groupId,subscriptionId);
    }

    @Override
    public void undo() {
        DeleteGroupResponse response = receiver.deleteGroup(groupId);
    }

    @Override
    public void redo() {
        execute();
    }
}

Any other suggestions on the whole code are most welcomed.

Excellent "by the book" realization of the command pattern with undo/redo ;)

Just 2 comments:

If receiver changes the state of the database, I would definitively handle the error case - the current implementation seems to ignore it. Imagine that undo fails (group was not deleted) and the previous command created the subscription. Then another undo (which tries to delete the subscription) also fails because there is still a group referencing the subscription.
Maybe it is a matter if taste, but I don't see a case where the implementation of execute and redo differs. Usually, I use only 2 methods do and undo where do will be used for execute as well as redo.

  Having Invoker and Command to return boolean indicating success or
  failure feels better. I have seen an example where they use callbacks.
  Is this something only for asynchronous operations or I can also use
  it here? Which would be better?

If a boolean (without error message) is enough, I would prefer the boolean return value because of its simplicity. Otherwise, I would even prefer something like an ErrorInfo object that contains a success flag and an optional error message (or even other information like the exception) as return value.

Another option is to throw an exception - may be also valid in your case. For that kind of error handling, the command should be executed before being pushed to the undoStack obviously.


Mergesort implementation, eliminating slice() I am having trouble coming up an alternate solution to my mergesort. I think using slice is an expensive operation. I have two implementations, recursive and iterative.

function mergeSortRecursive(arr) {
// base case
if (arr.length <= 1) {
 return arr;
}

var leftArr =  arr.slice(0, arr.length/2);
var rightArr = arr.slice(arr.length/2);
var leftSorted = mergeSortRecursive(leftArr);
var rightSorted = mergeSortRecursive(rightArr);
// merge subarrays
return merge(leftSorted, rightSorted);
}

mergeSortRecursive([1,4,5,245,7, 3,3,6,31]);

function merge(left, right) {
var result = [];
var indexLeft = 0;
var indexRight = 0;
// while result is not fully populated
while(result.length < (left.length + right.length)) {
  // if all elements in left have been added, then add remaining
right elements
  if (indexLeft === left.length) {
    result = result.concat(right.slice(indexRight));
  }
  // if all elements in right have been added, then add remaining left elements
  else if (indexRight === right.length) {
   result = result.concat(left.slice(indexLeft));
  }
  // compare elements in subarrays and add lower of the two to result
  else if (left[indexLeft] <= right[indexRight]) {
  result.push(left[indexLeft++]);
  } else {
    result.push(right[indexRight++]);
  }
 }
return result;
}

function mergeSortIterative(arr) {
// create an array of subarrays with each element
var splitArr = arr.map(function(element) {
  return [element];
});
// while there is more than one subarray
while (splitArr.length > 1) {
  var result = [];
  // merge adjacent;
  for (var i = 0; i < splitArr.length; i+=2) {
    // for pairs merge
    if (splitArr[i + 1]) result.push(merge(splitArr[i],
 splitArr[i+1]));
    // for last odd element, just add to results
    else result.push(splitArr[i]);
  }
  // overwrite old splitArr
  splitArr = result;
  }
  return splitArr[0];
}

Alternative ?
Not to sure what you mean by alternative solution. A merge sort is a merge sort and you need to create arrays, via slice, manually, or even creating a buffer and managing virtual arrays will all incur some overhead.
Your code.
If you want to improve performance then there is plenty of room for an improved alternative to your code.
Your recursive function is inherently a poor performer but it outperforms the iterative function because the iterative function does not use a stack to optimize data processing.
Your merge function is somewhat sloppy and can be improved, it is the core of the functionality of the sort. You gain the greatest benefit via improvements in the merge function. Even greater improvement if you can side step the need to call it as often.

Recursion, a glorified stack.
In Javascript recursive code has a large overhead. Each call creates a new context that is pushed to the heap. You also can never know how deep you are in the call stack, so a recursive function is always at risk of a call stack overflow.
Recursion is in reality just a way to implement a stack. Each recursion pushes the current execution state to the stack and create the new one. Each return disposes the current state and pop the previous one of the top of the stack.
Many people forget this when writing recursive code, and don't realize that the state stack can many times be more efficiently implemented as a simple stack using a JavaScript array.
Another very common mistake in recursive code is the exit condition. Stepping into a function is expensive and should be avoided.
Annotated your code to show what I mean

function mergeSortRecursive(arr) {

   // the exit from recursion one step too deep
   if (arr.length <= 1) {
      return arr;
   }

   var leftArr =  arr.slice(0, arr.length/2);
   var rightArr = arr.slice(arr.length/2);

   // You know at this point if both recursions are going exit early
   // because the array lengths are <= 1. You should just skip the
   // recursion here rather than inside the next two calls
   var leftSorted = mergeSortRecursive(leftArr);
   var rightSorted = mergeSortRecursive(rightArr);

   return merge(leftSorted, rightSorted);
}

The merge function
Inside the while loop of the merge function you are doing too much. Each 2 values that need to be compared have the overhead of two if statements. They are true only when the loop is ready to exit. The loop should only deal with the comparison of two values and after the loop you can clean up by adding the remaining array content to the result.
Improving the merge
Remove the unneeded if statements and put them after the loop.
function merge(left, right) {
    const result = [];
    while (left.length && right.length) {
        result.push((left[0] <= right[0] ? left : right).shift());
    }

    if(left.length + right.length){
        result.push(...(left.length > 0 ? left : right));
    }
    return result;
}

Side step merge.
I you inspect the running of the code and the array sizes that merge merges you will notice that 50% of the merges are of two arrays each of length 1. In effect you are using the complicated merge function to sort two numbers.
You can side step the merge function for array sizes 1 with the following
The recursive modification
function mergeSort(arr) {
    var len = arr.length / 2;
    if (len < 1) { return arr }

    // the following avoids stepping one too deep into the recursion as well
    if (len === 1) { // if array sizes 1 then skip the merge
        return arr[0] < arr[1] ? [arr[0], arr[1]] : [arr[1],arr[0]];
    }

    return merge(mergeSort(arr.slice(0, len)), mergeSort(arr.slice(len)));
}

For the iterative function you can map pre-merged values
var split = [];;
for(var i = 0; i < arr.length-1; i+= 2){
    split.push(arr[i] < arr[i+1] ? [arr[i],arr[i+1]] : [arr[i+1],arr[i]] )
}
if(arr.length % 2) { splitArr.push([arr[arr.length -1]]) }

Implementing an iterative stack.
If you count the number of times you need to compare 2 values the recursive function does about 10% less than the iterative function. But that can be changed by making the iterative function act like a recursive function.
The splitArr replaces the call stack and heap, and holds the merged arrays. When there are no more items to merge in splitArr the job is complete.
Note turns out a queue is quicker than a stack in this case but the idea is the same
function mergeSortIterative(arr) {
    const stack = [];
    for (var i = 0; i < arr.length - 1; i+= 2) {
        const ii = i + 1;
        stack.push(arr[i] < arr[ii] ? [arr[i], arr[ii]] : [arr[ii], arr[i]] )
    }
    if (arr.length % 2) { stack.push([arr[arr.length -1]]) }

    // the loop replaces the recursive function
    while (stack.length > 1){
        stack.push(merge(stack.shift(), stack.shift()));
    }
    return stack[0];
}

Now you have an alternative solution without a slice in sight, improved memory and CPU use because you can avoid the recursive over head, and a reduction in the overall complexity by turning many of the merge call into simpler single statement sort.


k-armed bandit problem in Python I implemented k-armed bandit problem in C#, MATLAB and Python. C# and Matlab code run fairly fast (With same settings of T = 2000 and nRun = 1000 the elapsed time is about 6sec). However, the Python version is ten times slower and takes about 60 seconds to complete. I am a beginner in Python but I guess it should be faster than this. I appreciate any tips to speed up this code.

import numpy as np
import matplotlib.pyplot as plt
import time

t1 = time.time()

plt.close('all')

nActions = 10
# np.random.seed(0)
sigma = np.ones(nActions)
mu = np.array([-0.4677,-0.1249,1.4790,-0.8608,0.7847,0.3086,-0.2339,-1.0570,-0.2841,-0.0867])

def Reward(action:int, mu:np.ndarray, sigma:np.ndarray):
  return mu[action] + sigma[action]*np.random.normal()

def EpsGreedyPolicy(Q,eps):
  p = np.random.rand()
  if p<eps:
    nAction = np.size(Q)
    return np.random.randint(nAction)
  else:
    return GreedyPolicy(Q)

def GreedyPolicy(Q):
   A = np.nonzero(Q == np.max(Q)) # A is a tuple
   n = np.size(A)
   if n == 1:
     return A[0]
   else:
     j = np.random.randint(n)
     return A[0][j] # Note A here is a tuple not array

# Reinforcement Learning

BestAction = np.argmax(mu)

T = 2000
nRun = 1000;

arrA = np.zeros((T,nRun))
arrR = np.zeros((T,nRun))

Q0 = 0 # Initial values

for j in range(nRun):
  TotalReward = np.zeros((nActions))
  Counter = np.zeros((nActions))

  for t in range(T):

    # Calculate action values
    Q = TotalReward/Counter
    Q[np.isnan(Q)] = Q0

    # Apply Policy
    a = EpsGreedyPolicy(Q,0.01)

    # Commit Action
    r = Reward(a, mu, sigma)

    # Update relevant stats
    Counter[a] += 1
    TotalReward[a] += r

    # Save results
    arrA[t][j] = a
    arrR[t][j] = r

  #print('End of Run ', j)

  print("end of run ",j)
  # end of main loop

# Plot Results
BestActionSelected = (arrA == BestAction)
BestActionSelectedMean = np.average(BestActionSelected, axis=1)

plt.figure(2,figsize=(10,10))

plt.subplot(2,1,1)
plot1 = plt.plot(BestActionSelectedMean)
plt.title('best action selection')

plt.subplot(2,1,2)
plot2 = plt.plot(np.average(arrR, axis=1))
plt.title('average reward')

plt.show()

t2 = time.time()

print(t2-t1)

Python is notoriously slow with native loops; i.e.

for j in range(nRun):
    ...
    for t in range(T):
        ...

is probably your bottleneck. I haven't tried to understand necessarily what you are coding but can you vectorise some calculations?

Something like

sigma = np.ones(nActions)
mu = np.array([-0.4677,-0.1249,1.4790,-0.8608,0.7847,0.3086,-0.2339,-1.0570,-0.2841,-0.0867])
def Reward(action:int, mu:np.ndarray, sigma:np.ndarray):
    return mu[action] + sigma[action]*np.random.normal()

is better replaced by:

temp_rvs = np.random.rand(nRun * T, 10)
reward_arr = mu + sigma * temp_rvs

then you can just index an existing variable instead of generating random variables each time through a function in your loop, i.e. you can:

reward[(j+1)*T, a]

But this takes up \$10 * nRun * T * 8\$ bytes of RAM to store the random variables.

I answered a similar question to this here: How to simulate this Gamma expansion in a Python way perhaps it might give you some ideas.


reusable data access class in C# Below is my attempt at the beginnings of a reusable ado.net data access class.  Please let me know of any improvements that I could make.

Thanks!

public class DataAccess : IDisposable
{
    public DbProviderFactory Factory { get; set; }
    public string Connection { get; set; }
    public string ProviderName { get; set; }

    public DataAccess()
    {
        Get_ConnectionString_From_AppSettings();
        Get_ProviderName_From_AppSettings();
    }

    private void Get_ConnectionString_From_AppSettings()
    {
        this.Connection = ConfigurationManager.AppSettings.Get("Database");
    }

    private void Get_ProviderName_From_AppSettings()
    {
        this.ProviderName = ConfigurationManager.AppSettings.Get("DBProvider");
    }

    public DataTable Connection_Return_DataTable( string sql)
    {
        DbConnection connection = null;

            try
            {
                DbProviderFactory factory = DbProviderFactories.GetFactory(this.ProviderName);
                this.Factory = factory;
                connection = Factory.CreateConnection();
                connection.ConnectionString = Connection;
                DataTable dt = FillDataTable(connection, sql);
                return dt;
            }
            catch (DataException ex)
            {
                Debug.WriteLine(ex.ToString());
                throw;
            }
            finally
            {
                connection.Close();
                connection.Dispose();
            }
    }

    private DataTable FillDataTable(DbConnection connection, string sql)
    {
        DbCommand command = connection.CreateCommand();
        command.CommandText = sql;
        command.Connection = connection;

        DbDataAdapter adapter = this.Factory.CreateDataAdapter();
        adapter.SelectCommand = command;

        DataTable table = new DataTable();
        adapter.Fill(table);

        return table;
    }

    public void Dispose()
    {
        this.Factory = null;
        this.Connection = null;
        this.ProviderName = null;

    }
}

You do not need IDisposable in your current implementation as you do not use any unmanaged resources or have any fields or properties which implement IDisposable which your class owns.

Setting the properties to null in the dispose method is unnecessary.

Method names should be Pascal cased and not contain underscores to adhere to standard naming conventions:

private void GetConnectionStringFromAppSettings()

Since the class cannot operate without a connection string or provider, you should throw an exception if they are not set:

private void GetConnectionStringFromAppSettings()
{
    this.Connection = ConfigurationManager.AppSettings.Get("Database");

    if (string.IsNullOrEmpty(this.Connection))
    {
        throw new InvalidOperationException("No connection string found in the configuration");
    }
}

You should use the Connection Strings section of the app.config instead of app settings.

You should resolve the factory once in your constructor instead of on every call to ConnectionReturnDataTable.

ConnectionReturnDataTable should be renamed to something like GetQueryResults.

DbCommand and DbDataAdapter are both IDisposable and should be used in a using statement (or a try/finally which is what a using compiles to).

private DataTable FillDataTable(DbConnection connection, string sql)
{
    using (DbDataAdapter adapter = this.Factory.CreateDataAdapter())
    {
        using (DbCommand command = connection.CreateCommand())
        {
            command.CommandText = sql;
            command.Connection = connection;
            adapter.SelectCommand = command;

            DataTable table = new DataTable();
            adapter.Fill(table);

            return table;
        }
    }
}

The connection property should be called ConnectionString

public string ConnectionString { get; set; }

In fact, the properties should probably become private fields as they shouldn't be modified from outside the class if you are using the app.config to specify their values.

You should be using parameterised queries instead of raw SQL


Performance impact of using the universal selector in CSS I am using the universal selector in CSS:

#containerDiv.containerTable tr td *
{
max-width:240px;
}

But Visual Studio suggests:

  "Performance: Never use the universal selector. It has a big negative
  impact on browser rendering"

Will this have such a big impact? If so how can I overcome this?
I have used * before and I have not seen any noticeable impact.

The problem with the universal selector is that... it's universal. It's affecting all descendants, present and future. Impact is not really noticeable in small scale, but when your app starts to grow (like packing more and more stuff in <td>), or if fast rendering is critical (like say render at 60fps for smooth animation), then the effects will show.

The way you're asking this question seems to imply that you're using tables for layout. Otherwise, <td> content shouldn't be that many to warrant a universal selector. Don't use tables for layouting. Tables don't get rendered until all the markup of the table (from <table> to </table>) is parsed. In the meantime, all you will be seeing is a big blank space. Not good.


Calculating GetHashCode efficiently with unordered list I'm wondering what would be the best way to calculate the hashcode when the order of a sequence doesn't matter. Here's the custom IEqualityComparer<T> i've implemented for an answer on Stackoverflow.

public class AccDocumentItemComparer : IEqualityComparer<AccDocumentItem>
{
    public bool Equals(AccDocumentItem x, AccDocumentItem y)
    {
        if (x == null || y == null)
            return false;
        if (object.ReferenceEquals(x, y))
            return true;
        if (x.AccountId != y.AccountId)
            return false;
        return x.DocumentItemDetails.Select(d => d.DetailAccountId).OrderBy(i => i)
            .SequenceEqual(y.DocumentItemDetails.Select(d => d.DetailAccountId).OrderBy(i => i));
    }

    public int GetHashCode(AccDocumentItem obj)
    {
        if (obj == null) return int.MinValue;
        int hash = obj.AccountId.GetHashCode();
        if (obj.DocumentItemDetails == null)
            return hash;
        int detailHash = 0;
        unchecked
        {
            var orderedDetailIds =  obj.DocumentItemDetails
                .Select(d => d.DetailAccountId).OrderBy(i => i);
            foreach (int detID in orderedDetailIds)
                detailHash = 17 * detailHash + detID;
        }
        return hash + detailHash;
    }
}

As you can see the foreach in GetHashCode needs to order the (nested) sequence before it starts calculating the hashcode. If the sequence is large this seems to be inefficient. Is there a better way to calculate the hashcode if the  order of a sequence can be ignored?

Here are the simple classes involved:

public class AccDocumentItem
{
    public string AccountId { get; set; }
    public List<AccDocumentItemDetail> DocumentItemDetails { get; set; }
}

public class AccDocumentItemDetail
{
    public int LevelId { get; set; }
    public int DetailAccountId { get; set; }
}

If the order does not matter, use an kommutative operator to combine the hashCodes of the elements.

Possible candidates are:
^ binary xor   // THIS WOULD BE MY CHOICE
+ addition       // problem may be the overflow
* multiplication         // problem may be the overflow
| binary or    // not recommended, because after some of this operations it is likely that all bits are set, so the same hashCode would appear for quite different instances)


Random number generator initialisation For a server-side Java application I want to have a random number generator. I'm using the ISAAC algorithm from Apache Commons Math 3 because it is both cryptographically strong and fast. I have written the following code to initialise the generator:

public class ThreadLocalRandomGenerator {
    private static ThreadLocal<RandomGenerator> instances = new ThreadLocal<>();

    public static RandomGenerator get() {
        RandomGenerator generator = instances.get();
        // Initialise new generator if needed.
        if (generator == null) {
            int[] seed = new int[256];
            seed[0] = (int) (System.currentTimeMillis() & 0xffffffffL);
            seed[1] = System.identityHashCode(new Object());
            for (int i=2 ; i<256 ; i++) {
                seed[i] = 0;
                for (int j=0; j<32; j++) {
                    seed[i] |= (int) ((System.nanoTime() & 1L) << j);
                }
            }
            System.out.println("Initialised random seed: " + Arrays.toString(seed));
            // Create generator.
            generator = new ISAACRandom(seed);
            instances.set(generator);
            // Warm up.
            for (int i=0 ; i<1_000 ; i++) {
                generator.nextLong();
            }
        }
        return generator;
    }
}

Basically I'm using a ThreadLocal to keep several generators around for use in threads, and I'm initialising each one using an array of integers generated from the least significant bits of nanoTime(). The reason for using 256 integers is because that's the internal size of the ISAACRandom class.

Does this seem sensible?

For the most part, that's nice code. I can read it somewhat easily, and for the most part, it is logical, and structured.
There are a number of issues in your code, though, some style issues, and some functional issues.
First up, the style issues.
Using the existing libraries
There's a nice feature in ThreadLocal in Java8 that allows you to have a supplier for the structure. Consider a method like:
private static RandomGenerator buildGenerator() {
    .....
}

then your thread local can be created as:
private static ThreadLocal<RandomGenerator> instances =
       ThreadLocal.withInitial(ThreadLocalRandomGenerator::buildGenerator);

Your main method can become a simple:
public static RandomGenerator get() {
    return instances.get();
}

It is guaranteed that there will be an instance created for you on the first call from a thread.
Getting good seeding
Your seeding system is both complicated, and wrong.
If you are looking to have a special random generator that you feel out-performs the existing Java Random class, that's OK, but, it needs to be seeded right.
There are a few problems in your seeding that I can see. First up:

seed[0] = (int) (System.currentTimeMillis() & 0xffffffffL);

That can be simply an int cast. No need to mask off the bits:
seed[0] = (int)System.currentTimeMillis();

Next up, is that computers have different precision in their nano clocks. A nano-second is far faster than the actual computer's time interrupt, so, it is common for computers to "step" significantly as it updates. The steps can be as much as 190 nanoseconds is where i have seen it....
... what that means, is that the results for all your seed values here may be the same:

seed[i] |= (int) ((System.nanoTime() & 1L) << j);

In other words, you will have int values filled with either 0's or 1's... and no variations. Also, if the "step" for the nano-second clock happens to be an even number, it is possible that the low-order bit for your computer will always be the same value.... and all your seeds will be identical.
While the Random class in Java may be slower than you want, you can at least use it to seed the faster ISAAC algorithm.
I would simply use the following to seed your class:
Random rand = new Random();
int seed[] = rand.ints(256).toArray();

Use the slower Random to seed your class, then let your class run with the rest.
Note, there is no real way to use the time systems in a computer to get random values... it's just not infused with enough entropy to be random, or predictability to be reliable.


Find out how many laptops Molly can buy based on available money, array of quantities and array of costs Problem description:

  Molly wants to purchase laptops for her school. Find out how many laptops she can purchase by comparing the vendors available.
  Each vendor sells the laptops in batches, with a quantity identifying how many laptops on each batch, and a cost for the whole batch.
  Sample input: 50 [20,19] [24,20]
  That means Molly has 50 dollars to spend. The first vendor has 20 laptops per batch and each batch costs 24 dollars. The second vendor has 19 laptops per batch and each batch costs 20 dollars.
  The possible answers are 40 and 38. If she buys from the first vendor, she will spend 48 dollars (24 * 2) and since she's buying 2 batches the total quantity is 40 (20 * 2).
  If however she would buy from the second vendor, the maximum quantity would be 38, since each batch has 19 laptops and she'd run out of money after the second batch.
  The final answer is then 40, since 40 is higher than 38.
  This seems similar to the Knapsack problem.

My solution is listed below. The permute method is there to generate the possible permutations between the indices. For instance if we have 3 items in the input array the permutations are:
012 021 102 120 201 210

import static org.junit.Assert.assertEquals;
import java.util.List;
import java.util.stream.IntStream;

public class ShoppingBudget {

  public static void main(String[] args) {
    ShoppingBudget sb = new ShoppingBudget();
    assertEquals(40, sb.budgetShopping(50, List.of(20, 19), List.of(24, 20)));
    assertEquals(20, sb.budgetShopping(4, List.of(10), List.of(2)));
    assertEquals(0, sb.budgetShopping(1, List.of(10), List.of(2)));
    assertEquals(41, sb.budgetShopping(50, List.of(20, 1), List.of(24, 2)));
  }

  private void permute(int start, int moneyAvailable, int[] inputIndices,
      List<Integer> budgetQuantities, List<Integer> budgetCosts, MaxQuantity maxQuantity) {
    if (start == inputIndices.length) { // base case

      int possibleMax = findSolution(inputIndices, moneyAvailable, budgetQuantities, budgetCosts);

      maxQuantity.value = Math.max(maxQuantity.value, possibleMax);

      return;
    }
    for (int i = start; i < inputIndices.length; i++) {
      // swapping
      int temp = inputIndices[i];
      inputIndices[i] = inputIndices[start];
      inputIndices[start] = temp;
      // swap(input[i], input[start]);

      permute(start + 1, moneyAvailable, inputIndices, budgetQuantities, budgetCosts, maxQuantity);
      // swap(input[i],input[start]);

      int temp2 = inputIndices[i];
      inputIndices[i] = inputIndices[start];
      inputIndices[start] = temp2;
    }
  }

  private int findSolution(int[] inputIndices, int moneyAvailable,
      List<Integer> budgetQuantities, List<Integer> budgetCosts) {
    int remaining = moneyAvailable;
    int counter = 0;

    for (int index : inputIndices) {
      if (remaining == 0) {
        break;
      }

      int quantity = budgetQuantities.get(index);
      int cost = budgetCosts.get(index);

      if (cost <= remaining) {
        int howManyToBuy = remaining / cost;
        counter += howManyToBuy * quantity;
        remaining -= (howManyToBuy * cost);
      }
    }

    return counter;
  }

  private int budgetShopping(int n, List<Integer> budgetQuantities,
      List<Integer> budgetCosts) {
    int[] inputIndices = IntStream.rangeClosed(0, budgetQuantities.size() - 1).toArray();
    MaxQuantity maxQuantity = new MaxQuantity();
    maxQuantity.value = Integer.MIN_VALUE;
    permute(0, n, inputIndices, budgetQuantities, budgetCosts, maxQuantity);

    return maxQuantity.value;
  }
}

class MaxQuantity {
  int value;
}

My understanding of the problem:

You are provided three values:

A budget(the amount of funds you can spend)
A List of Vendor Batch Sizes
A List of Vendor Batch Prices

Promises/Assumptions: the items 2 & 3(the lists) correspond such that for Vendor i, the vendors batch size and price can be found at List.get(i)

NOTE: Your code has a hard-coded assumption that the two lists (budgetQuantities and budgetCosts) are the same size, or at least you never check that this is true or otherwise enforce this requirement.

Goal: determine the maximum affordable quantity based on Vendor and budget information.

There seems to be some superfluous code going on here, so I will try to address that first

Inner Class MaxQuantity

There really isn't any reason to use this. It has only one use: to interact with the internal int value. As both the class and value are public, I can access all the way through: shoppingBudget.MaxQuantity.value. This is generally considered bad. As the class is in all cases just an int, it can be replaced with an internal variable, private int maxQuantity

suggesting removal

The permute function

As mentioned in my comment, this function seems entirely pointless as the problem does not involve permutations; The information is linear in nature, as there is no "mix and match" (i.e. permutation) of Vendor Information (budgetQuantities and budgetCosts)

suggesting removal

Update
I appreciate your use of recursion in your permute function, but its a tad confusing. The second set of swapping seems pointless, as by the time you reach it you are simply propagating back up the recursion tree, swapping indeces that we no longer use.

I looked for similar permutation implementations and found this well designed permutation util, which I used for comparison testing of your permutation method.

IntStream
There really isn't a need to create an int[] for indices like this. I see it is mostly used in the permute() function, which I have suggested removing, so I will likewise suggest removing this. With a linear solution, it is much easier to create a single int index and increment it as we iterate through the lists, rather than creating an int for every position from 0 to budgetQuantities,size()-1 (the increment approach is also less computationally expensive, especially where memory is concerned)

Update I had to use this in my rework, definitely shouldn't remove

maxQuantity.value = Integer.MIN_VALUE;

Integer.MIN_VALUE is usually a good initial value for a maximum. However, this is not the true minimum of the problem. The absolute minimum items you can purchase is 0, so it makes more sense to initialize your globalMaximum to 0: private int globalMaximum=0;

UPDATED

findSolution is NOT broken

There is a hole in the logic of your findSolution method. You seem to have written your code with the intent that this method will be called many times, and each result will be compared with the global max (based on your use of maxQuantity.value = Math.max(maxQuantity.value, possibleMax); in permute)

However, this method runs across all vendors. Because of this, your int remaining and counter should be inside your for loop, with an int localMaximumToReturn where counter is currently.

To explain further: your current findSolution forEach loop represents each Possible Interaction between Molly and one Vendor, so both the money available and the total number of computers she can buy per transaction are independent. However, in the above code, each loop is interacting with the same funds, which will introduce severe error.

Example: I ran in debug with breakpoints inside the findSolution method, outside the forEach loop, with the following information:

moneyAvailable: 50
budgetQuantities: {20,19}
budgetCosts: {24,20}

Stepping through, the first loop correctly computes that howManyToBut is 2, meaning she can buy 40 computers for 48 dollars. Then you set remaining to 2 with remaining -= 48

This means for the next loop, we get a cost of 20, which is now greater than our remaining 2 so the second cost/quantity maximum is never checked

We have technically hit each index, so we leave the loop and return the counter 40. While this IS the correct answer, we got here by false means. If you switch the order of items in your lists for the first assert, you should see a return of 38, which will break your assertEqual

It is at this point I see the point of permute in that it was scrambling your orders. Since your findSolution would only ever succeed at checking the first index, permute scrambles your orders so that each item gets a shot at being first, so each possible results is compared with the globalMax. While this works, it is incredibly backward, and buggy I highly recommend fixing this bug in findSolution, and remove the no longer needed permute()

With superfluous code removed you should end up with something similar to:

public class ShoppingBudget {
    int globalMax;

    public int budgetShopping(int budget, List<Integer> batchSizes, List<Integer> batchCosts) {
        List<Integer> indeces = new ArrayList<>();
        IntStream.range(0, batchSizes.size()).forEach((num) -> indeces.add(num));
        Collection<List<Integer>> permutations = new Permutations<Integer>().permute(indeces);
        permutations.forEach(
                (permutation) -> checkPermutationMaximum(permutation, budget, batchSizes, batchCosts)
        );
        return globalMax;
    }

    private void checkPermutationMaximum(List<Integer> indeces, int budget, List<Integer> batchSizes, List<Integer> batchCosts) {
        int localMax = 0;
        int remainingCash = budget;

        int i = 0;
        int sz = indeces.size();
        while (i < sz && remainingCash > 0) {
            int currentVendor = indeces.get(i);
            int currentCost = batchCosts.get(currentVendor);
            if (remainingCash >= currentCost) {
                int cashToCostRatio = remainingCash / currentCost;
                remainingCash -= currentCost * cashToCostRatio;
                localMax += cashToCostRatio * batchSizes.get(currentVendor);
            }
            i++;
        }

        if (localMax > globalMax) globalMax = localMax;

    }
}

The above passes all of your assertEqual tests as well as debug scrutiny

Comments on Update

Rather than trying to create the next unvisited permutation at run time (which is your above approach) I adapted a utility to return a Collection of Lists, where each List is a possible permutation of the input. Then I compute and check the maximum for each permutation


jQuery animation/blinking code? I have one container div which contains some children div members. From time to time, I need to run a custom blinking effects on some of the child div members. The effect steps are as follows:

Change font color to Yellow
Pause for 0.5 secs
Change background color to yellow and text color to black
Pause for 0.25 secs
Change font color to Yellow & background to black
Pause for 0.25 secs
Change background color to yellow and text color to black
Pause for 0.5 secs
Repeat step 1 to 8

Below is the JavaScript code that I came up using jQuery to achieve the above effect. The code is working perfectly fine, but somehow I feel that this is not an elegant solution and there may be possible issue with how I pass $elements var from one function to another.

Is there anything wrong with this approach?  How can this code be improved?

function startAnimation($container) {
    var $elements = $container.find(".animation");
    if ($elements.length) {
        $elements.switchClass("mc-ylw-blk", "mc-blk-ylw", 0).show(0).delay(500).promise().done(function () {
            $elements.switchClass("mc-blk-ylw", "mc-ylw-blk", 0).delay(250).promise().done(function () {
                $elements.switchClass("mc-ylw-blk", "mc-blk-ylw", 0).delay(250).promise().done(function () {
                    $elements.switchClass("mc-blk-ylw", "mc-ylw-blk", 0).delay(500).promise().done(function () {
                            startAnimation($container);   //Repeat
                    });
                });
            });
        });
    }

}

you can chain the delay and switchClass to $elements and take the $("#container").find(".animation"); outside the function like this

function startAnimation($elements) {
$elements.switchClass("mc-ylw-blk", "mc-blk-ylw", 0)
      .delay(500).switchClass("mc-blk-ylw", "mc-ylw-blk", 0)
      .delay(250).switchClass("mc-ylw-blk", "mc-blk-ylw", 0)
      .delay(250).switchClass("mc-blk-ylw", "mc-ylw-blk", 0)
      .delay(500).promise().done(function () {
          startAnimation($elements);
      });
}
var $elements=$("#container").find(".animation");
if ($elements.length) {
    startAnimation($elements);
}

http://jsfiddle.net/Y95rU/1/


Select products and config values from MySQL I have working code that gets product details and config values from a MySQL
database, but it's combined with HTML. Is there a better way to do this, maybe a class that I can use?

<!DOCTYPE html>
<!--[if lt IE 7]>      <html class="no-js lt-ie9 lt-ie8 lt-ie7"> <![endif]-->
<!--[if IE 7]>         <html class="no-js lt-ie9 lt-ie8"> <![endif]-->
<!--[if IE 8]>         <html class="no-js lt-ie9"> <![endif]-->
<!--[if gt IE 8]><!--> <html class="no-js"> <!--<![endif]-->
<?php
require __DIR__ . '/vault/MySQL.php';

$id = ( isset( $_GET['pid'] ) && is_numeric( $_GET['pid'] ) ) ? intval( $_GET['pid'] ) : 0;
if ( $id != 0 ) {
try {

// select all data
// $query = "SELECT product_id, product_title, product_description, product_category, product_image FROM products WHERE product_id = ? LIMIT 1";
$query = "SELECT p.product_id AS product_id, p.product_title AS product_title, p.product_description AS product_description, categories.category_name AS category_name, p.product_category AS product_category, p.product_image AS product_image, p.product_image_thumb AS product_image_thumb FROM products p LEFT JOIN categories ON p.product_category=categories.category_id WHERE p.product_id = ? LIMIT 1";
$stmt = $con->prepare( $query );
$stmt->bindParam(1, $_REQUEST['pid'], PDO::PARAM_INT);
$stmt->execute();

// store retrieved row to a variable
$row = $stmt->fetch(PDO::FETCH_ASSOC);

// values to fill up our form
extract ($row);

} catch (PDOException $exception) { // to handle error
    echo "Error: " . $exception->getMessage();
}
    }
else        {
    echo "something went wrong";
    }
// select all data
$iQ = "SELECT config_name, config_value FROM config";
$stmt = $con->prepare( $iQ );
$stmt->execute();

while ($row = $stmt->fetch(PDO::FETCH_ASSOC) )
{
$config[$row['config_name']] = $row['config_value'];
}
?>
<head>
   <meta charset="utf-8">
   <!--[if IE]><meta http-equiv='X-UA-Compatible' content='IE=edge,chrome=1'><![endif]-->
   <title><?php echo $config['frontend_title'] . " - " . $product_title;?></title>
   <meta name="description" content="<?php echo "{$product_description}";?>">

Several things...

Indentation

This:

if ( $id != 0 ) {
try {
// etc.
} catch (PDOException $exception) { // to handle error
    echo "Error: " . $exception->getMessage();
}
    }

Would read easier as such:

if ( $id != 0 ) {
    try {
        // etc.
    } catch (PDOException $exception) { // to handle error
        echo "Error: " . $exception->getMessage();
    }
}

Comments

Many of your comments are of very little use, when the code speaks for itself. For instance:

    } catch (PDOException $exception) { // to handle error

Or:

// select all data

Or even:

// store retrieved row to a variable

SQL

You will save yourself and Mr. or Ms. Maintainer a good deal of grief if you format your SQL in a way that SQL programmers will be familiar with. For instance this:

$query = "SELECT p.product_id AS product_id, p.product_title AS product_title, p.product_description AS product_description, categories.category_name AS category_name, p.product_category AS product_category, p.product_image AS product_image, p.product_image_thumb AS product_image_thumb FROM products p LEFT JOIN categories ON p.product_category=categories.category_id WHERE p.product_id = ? LIMIT 1";

Could instead be:

$query = "SELECT
            p.product_id AS product_id,
            p.product_title AS product_title,
            p.product_description AS product_description,
            categories.category_name AS category_name,
            p.product_category AS product_category,
            p.product_image AS product_image,
            p.product_image_thumb AS product_image_thumb
        FROM products p
        LEFT JOIN categories ON p.product_category=categories.category_id
        WHERE p.product_id = ? LIMIT 1";

I will say that it is good that you are using PDO. More experienced PHP programmers will hopefully have more to say about PHP code, I'm mostly a SQL guy.


Rules engine dynamic type casting I have a basic switch statement that executes some code on expectedValue and givenValue. The code to execute is guided by the variable actionToApply. This leaves one more variable that is fieldTypeName. This essentially guides the code in to casting the values to the type defined by fieldTypeName - if needed.

I was hoping to have the switch block reviewed/refactored - if possible have the casting refactored away into another method that handles casting dynamically on the fly based on fieldTypeName. Then it can cast expectedValue and givenValue to the correct types making the switch statement code less repetitive and have less duplication.

At compile time we have the values as strings and we have the type to CastTo as a string too. I am open to using the dynamic keyword here if someone is brave enough.

Naturally as more types are added to the DB the below switch statement is going to grow and break a few SOLID principles along the way.

private bool Run(string actionToApply, string fieldTypeName, string expectedValue, string givenValue)
{
    switch (actionToApply)
    {
        case "HasValue":
            return !string.IsNullOrWhiteSpace(givenValue) && givenValue != expectedValue;

        case "EqualTo":
            return givenValue == expectedValue;

        case "NotEqualTo":
            return givenValue != expectedValue;

        case "Greater Than":
            if (fieldTypeName == "int")
            {
                var one = Convert.ToInt32(givenValue);
                var two = Convert.ToInt32(expectedValue);
                return one > two;
            }
            if (fieldTypeName == "double")
            {
                var one = Convert.ToDouble(givenValue);
                var two = Convert.ToDouble(givenValue);
                return one > two;
            }
            return false;
        case "Or":
            var orArrayValues = expectedValue.Split(new[] { SplitOperatorForOr }, StringSplitOptions.None);
            return orArrayValues.Any(x => x == givenValue);
        case "Less Than":
            if (fieldTypeName == "int")
            {
                var one = Convert.ToInt32(givenValue);
                var two = Convert.ToInt32(expectedValue);
                return one < two;
            }
            if (fieldTypeName == "double")
            {
                var one = Convert.ToDouble(givenValue);
                var two = Convert.ToDouble(givenValue);
                return one < two;
            }
            return false;

        case "Range":
            var given = Convert.ToInt32(givenValue);
            var rangeArrayValues = expectedValue.Split(new[] { SplitOperatorForRange }, StringSplitOptions.None);
            if (fieldTypeName == "int")
            {
                var rangeA = (int)Convert.ChangeType(rangeArrayValues[0], typeof(int));
                var rangeB = (int)Convert.ChangeType(rangeArrayValues[1], typeof(int));
                return given > rangeA && given < rangeB;
            }
            if (fieldTypeName == "double")
            {
                var rangeA = (double)Convert.ChangeType(rangeArrayValues[0], typeof(double));
                var rangeB = (double)Convert.ChangeType(rangeArrayValues[1], typeof(double));
                return given > rangeA && given < rangeB;

            }
            return false;
    }
    return false;
}

Just a quick shot at case "Greater Than":

case "Greater Than":
    if (fieldTypeName == "int")
    {
        var one = Convert.ToInt32(givenValue);
        var two = Convert.ToInt32(expectedValue);
        return one > two;
    }
    if (fieldTypeName == "double")
    {
        var one = Convert.ToDouble(givenValue);
        var two = Convert.ToDouble(givenValue);
        return one > two;
    }
    return false;

if fieldTypeName is double it is always returning false because copy&pasta should be done carefully. This applies to Less Than as well.


Area calculator for shapes as an OOP interview test A while ago a tricky C++ interview question came across to me, and ever since I could not tell what I did wrong.

  Create a base class named “Shape” with a method to return the area of the shape. Create a class named “Triangle” derived from the base class “Shape”.

  Class “Triangle” must have two constructors. One receives the length of the (a) side of the triangle and the related (ma) height. The other receives the length of (a),(b),(c) sides of the triangle.
  In the second case, the constructor must validate the input by checking that the length of one side is smaller than the sum of the lengths of the other two sides. If the input is invalid, it should throw an exception. Implement the method (available in the base class) that calculates the area of the triangle on the basis of the available data.

  You can use two formulas. If the length of a side and height is given: T=(a*ma)/2. If the lengths of the three sides are given, you can use the Heron formula:
  sqrt(s*(s-a)*(s-b)*(s-c))
  where s=(a+b+c)/2

This should be obvious for any candidate. It clearly asks if you know what the base principles of OOP is, and if you can solve a bit complex problem with it. Before I could just jump into it, I found something odd. The assessment requires you to create some class that behaves differently as they've created. It asks you to use some trickery to solve this problem.

Well, I, who read a bit about what SOLID is, and familiar with design patterns were trivial that using polymorphism is the way to solve this the most elegant way. My approach was something like this:

Create the base class
Create the derived class
Use the derived class constructor as an abstract factory for a composite type, and make the implementation to act as a proxy toward the different implementations
Create two other classes which handles the actual data and takes care of the calculation
Take care of the exceptions using std::exception - these are tricky as well in C++
Take care of the implementation

In the end my solution looked like this:

#include <exception>
#include <string>
#include <cmath>

class Shape
{
public:
    virtual double Area() = 0;
};

class Exception : public std::exception
{
public:
    Exception(std::string const &message) throw() : m_msg(message) {}
    ~Exception() throw() {}
    virtual char const *what() const _NOEXCEPT { return m_msg.c_str(); }

private:
    std::string m_msg;
};

class Triangle : public Shape
{
    class TriangleSides;
    class TriangleBase;

public:
    Triangle(double base, double height);
    Triangle(double a, double b, double c);
    ~Triangle();
    double Area() override;

private:
    Shape *m_shape;

    class TriangleSides : public Shape
    {
    public:
        TriangleSides(double a, double b, double c);
        double Area() override;

    private:
        double m_a, m_b, m_c;
    };

    class TriangleBase : public Shape
    {
    public:
        TriangleBase(double base, double height);
        double Area() override;

    private:
        double m_base, m_height;
    };
};

Triangle::Triangle(double base, double height) : m_shape(nullptr)
{
    m_shape = new Triangle::TriangleBase(base, height);
}

Triangle::Triangle(double a, double b, double c) : m_shape(nullptr)
{
    if (a + b <= c || a + c <= b || b + c <= a)
        throw Exception("Any two sides of a triangle must be longer than one");
    m_shape = new Triangle::TriangleSides(a, b, c);
}

Triangle::~Triangle()
{
    delete m_shape;
}

double Triangle::Area()
{
    if (m_shape)
        return m_shape->Area();
    else
        return 0;
}

Triangle::TriangleSides::TriangleSides(double a, double b, double c) : m_a(a), m_b(b), m_c(c)
{
}

double Triangle::TriangleSides::Area()
{
    const double s = (m_a + m_b + m_c) * .5;
    return sqrt(s * (s - m_a) * (s - m_b) * (s - m_c));
}

Triangle::TriangleBase::TriangleBase(double base, double height) : m_base(base), m_height(height)
{
}

double Triangle::TriangleBase::Area()
{
    return .5 * (m_base * m_height);
}

I see some things that may help you improve your code.

Don't overcomplicate your code

The only thing required of the Shape and Triangle classes is the calculation of area.  For that reason, the code can be considerably simplified if the constructor calculates and stores the area.

Use const where practical

The Area() function probably shouldn't alter the underlying Shape object, so it makes sense to declare it const.

Use a standard exception where appropriate

There's really nothing, well... exceptional about the Exception class, so it seems not really to be required.  Maybe std::domain_error could be used instead like this:

throw std::domain_error("Any two sides of a triangle must be longer than one");

Provide a test driver

This is more about getting a good review rather than the code itself, but it's often good to give reviewers a complete, compileable example to allow them to see the code in context.  Here's what I used:

#include <iostream>
int main() {
    Triangle t1{3, 4, 5};
    std::cout << t1.Area() << '\n';
    try {
        Triangle t2{3, 4, 15};
    }
    catch (std::exception &e) {
        std::cout << e.what() << '\n';
    }
    Triangle t3{3, 4};
    std::cout << t3.Area() << '\n';
}

Use std:: for math functions

The sqrt function is often in the global namespace, but it's required to be in the std namespace.  For that reason, it's probably safer to write std::sqrt than just sqrt.

Use standard keywords

Instead of _NOEXCEPT which is not standard, the code could use noexcept.

Reworked example

#include <string>
#include <cmath>
#include <stdexcept>

class Shape
{
public:
    virtual double Area() const = 0;
};

class Triangle : public Shape
{
public:
    Triangle(double base, double height);
    Triangle(double a, double b, double c);
    double Area() const override { return area;}

private:
    double area;
};

Triangle::Triangle(double base, double height)
    : area{(base * height)/2}
{
}

Triangle::Triangle(double a, double b, double c)
    : area{(a-b-c)*(-a+b-c)*(a+b-c)*(a+b+c)/16}
{
    if (area < 0)
        throw std::domain_error("Any two sides of a triangle must be longer than one");
    area = std::sqrt(area);
}


Web-scraper for a larger program I have a web scraper that I use in a part of a larger program. However, I feel like I semi-repeat my code a lot and take up a lot of room. Is there any way I can condense this code?

def read_mail(mail):
        url = [mail] # Ignore this line, please.
        i = 0 # Ignore this line, please.
        droppedSource = '(.+?)	' # Gets whatever is inbetween the tags
        destroyedSource = '(.+?)	'
        totalSource = '(.+?)'

        droppedText = re.compile(droppedSource) # Converts regex string into something that can be interpreted by regular library
        destroyedText = re.compile(destroyedSource)
        totalText = re.compile(totalSource)

        html = urllib.urlopen(url[i]).read() # ignore the url[i] part of this line, please.

        dropped = re.findall(droppedText,html)
        destroyed = re.findall(destroyedText,html)
        total = re.findall(totalText,html)

        return("Info: " + str(dropped[0])+str(destroyed[0])+str(total[0]))

First of all I would recommend not to use regex for handling HTML. You can use a library like BeautifulSoup for this.
As all we're doing is finding the first match using tag name and class name we can define a function that uses BeautifulSoup to find such matches bases on the tag and class name. BeautifulSoup provides two functions find and findAll, find returns the first match and findAll returns all the matches.
Note that in regex just to find the first match you should not use re.findall, better use re.search which only returns the first match found otherwise None.
On the last return line we can use string formatting.

from BeautifulSoup import BeautifulSoup
from functools import partial

def find_by_tag_name_class(soup, tag, cls_name, many=False):
    if many:
        matches = soup.findAll(tag, {"class": cls_name})
        return [match.text for match in matches]
    else:
        match = soup.find(tag, {"class": cls_name})
        return match.text

def read_mail(html):
    soup = BeautifulSoup(html)
    # Instead of passing the same `soup` multiple times to
    # `find_by_tag_name_class` we can create a partial function
    # with `soup` already applied to it.
    find_by_tag_name_class_soup = partial(find_by_tag_name_class, soup)
    dropped = find_by_tag_name_class_soup('td', 'item_dropped')
    destroyed = find_by_tag_name_class_soup('td', 'item_destroyed')
    total = find_by_tag_name_class_soup('strong', 'item_dropped')

    return "Info: {} {} {} " .format(dropped, destroyed, total)

html = '''Foo bar	spam eggs
Hakuna	Matatat
Some strong textEven more strong text'''

print read_mail(html)
# Info: Foo bar Hakuna Some strong text

Note that in the latest version of BeautifulSoup findAll has been renamed to find_all.


Basic 30 TPS server loop I've created a console server which is supposed to update 30 times per second, I'd like you to review it and also I'm curious about the accuracy of the server's loop.

using System;
using System.Diagnostics;
using System.Threading;

namespace ZGServer
{
    class Program
    {
        //------------------Variables------------------------
        double frameInterval = 1000/30;
        Stopwatch msstopwatch = new Stopwatch();
        Stopwatch tpsstopwatch = new Stopwatch();
        int tpscount = 0;
        //---------------------------------------------------
        static void Main(string[] args)
        {
            Program program = new Program();
            program.Server();
        }
        void Server()
        {
            while (true)
            {
                msstopwatch.Start();
                tpsstopwatch.Start();
                if (msstopwatch.ElapsedMilliseconds > frameInterval)
                {
                    tpscount++;
                    //Console.WriteLine(msstopwatch.ElapsedMilliseconds - frameInterval);
                    //Thread.Sleep((int)(msstopwatch.ElapsedMilliseconds - frameInterval));
                    //Console.WriteLine(msstopwatch.ElapsedMilliseconds);
                    msstopwatch.Reset();
                }
                else
                    Thread.Sleep(1);
                if (tpscount == 30)
                {
                    Console.WriteLine(tpsstopwatch.ElapsedMilliseconds);
                    tpscount = 0;
                    tpsstopwatch.Reset();
                }
            }
        }
    }
}

This is the output (It's random every time):

1037, 1038, 1039, 1034, 1037, 1033, 1033, 1034.

This is the amount of time it takes for it to perform 30 loops.

Is this accurate enough for a real-time server over the internet serving multiple clients?

class Program

For clarity it would be nice if the main class and method would be declared public.
Also that name is not that good, you should find a better one.

//------------------Variables------------------------

If you need such comments, I'd argue that there is something wrong with your class layout.

double frameInterval = 1000/30;
Stopwatch msstopwatch = new Stopwatch();
Stopwatch tpsstopwatch = new Stopwatch();
int tpscount = 0;

Are these variables used outside of your main class? If no, than these should be private. Everything should be the least accessible as possible, for clarity and encapsulation reasons.

static void Main(string[] args)

Same here, for clarity it should be public.

void Server()

That's a bad name for a function, even the simple Run() would have been a better choice.

Thread.Sleep(1);

Thread.sleep is not trustworthy, neither might be StopWatch.

If you do not have any constraint on ho much time needs to be between requests, then do not time it at all, perform the requests as fast as possible and if you've done 30 in this second, wait as precisely as possible for the next second.

Is this accurate enough for a real-time server over the internet serving multiple clients?

Definition of a real time Wikipedia:

A key characteristic of an RTOS is the level of its consistency concerning the amount of time it takes to accept and complete an application's task; the variability is jitter.[1] A hard real-time operating system has less jitter than a soft real-time operating system. The chief design goal is not high throughput, but rather a guarantee of a soft or hard performance category. An RTOS that can usually or generally meet a deadline is a soft real-time OS, but if it can meet a deadline deterministically it is a hard real-time OS.[2]

So my first guess as answer to your question would be: no.
However, the more sane approach to that question should be: What are your specs and does this fall into your specs?


ReactiveUI and Rx background calculations with cancellation I'm working on a WPF app which uses ReactiveUI and Rx, there's part of the
workflow that watches two data sources (ReferenceData and PredictedData properties on
a View Model) and an area of that data source (FocusArea property) which is used to show
a line graph of that data.

Sometimes there can be a lot of data (and sometimes both data sources change at the same time), and building the line graphs can take a while. So it's done on a background thread.
I'm also using the Ramer–Douglas–Peucker algorithm to approximate the graph for display (the ReduceGraphPoints method, which returns Task<List<Point>>). So the final graph is only updated when the line data for both data streams are calculated and simplified.

Once the calculation is started, the Loading property of the View Model is updated so we can mark the current graph as about-to-be-replaced

The code I have for that bit of functionally at the moment is:

this.WhenAny(me => me.ReferenceData,
            me => me.PredictedData,
            me => me.FocusArea,
            (refData, predictedData, area) => new { ReferenceData = refData.Value, PredictedData = predictedData.Value, Area = area.Value })
    .Where(x => !x.Area.IsEmpty)
    .Do(_ => Loading = true)
    .Throttle(TimeSpan.FromMilliseconds(50))
    .Select(x => Observable.FromAsync(token => Task.Run(() =>
        {
            var refPoints = x.ReferenceData.GetPointsInRange(x.Area.Range).ToList();
            var reducedRef = ReduceGraphPoints(refPoints, token);

            var predictedPoints = x.PredictedData.GetPointsInRange(x.Area.Range).ToList();
            var reducedPredicted = ReduceGraphPoints(predictedPoints, token);

            return new { ReferencePoints = reducedRef.Result, PredictedPoints = reducedPredicted.Result };
        }, token)))
    .Switch()
    .Subscribe(x =>
    {
        ReferencePoints = x.ReferencePoints;
        PredictedPoints = x.PredictedPoints;
        Loading = false;
    });

So I'm looking for feedback from any Rx experts on this. Things like the use of the Do method, and the cancellation token. Most of the methods called use
ThrowIfCancellationRequested on the token, but I'm ignoring the cancellation exception  because if it has been cancelled, then Switch() will already be watching
the next IObservable and won't care about the old one - is that the best way of doing this?

When Switch unsubscribes from the previous Observable to connect to the new one, FromAsync should signal the cancellation token and also correctly eat the ThrowOnCancellation exception.

While it'd probably be better to try to use a ReactiveCommand to model the loading bool (which might not be easy given the cancelation and Switch), if this code works for you it should be fine.

Make sure to specify RxApp.MainThreadScheduler for the Throttle though, it's more efficient


Check if a linked list is a palindrome in O(n) time and O(1) space I liked this problem because there are so many sub-problems that are interview problems in their own right-- (reverse linked list, find midpoint)

var Node = function(val) {
  this.val = val;
  this.next = null;
}

var a = new Node('r');
var b = a.next = new Node('a');
var c = b.next = new Node('c');
var d = c.next = new Node('e');
var e = d.next = new Node('c');
var f = e.next = new Node('a');
var g = f.next = new Node('r');

function checkOdd(linkedList) {
  var count = 0;
  var curr = linkedList;
  while (curr) {
    count++;
    curr = curr.next;
  }
  return count % 2 === 1;
}

function findMidPoint(linkedList) {
  var slow = linkedList;
  var fast = linkedList;

  while (fast && fast.next) {
    fast = fast.next.next;
    slow = slow.next
  }

  if (checkOdd(linkedList)) {
    return slow.next
  } else {
    return slow;
  }
}

function checkPalindrome(linkedList) {
  var midpoint = findMidPoint(linkedList);

  var firstHead = linkedList;
  var secondHead = reverseLL(midpoint);

  var flag = true;

  while (secondHead) {
    if (secondHead.val !== firstHead.val) {
      flag = false;
    }
    secondHead = secondHead.next;
    firstHead = firstHead.next;
  }
  return flag;
}

function reverseLL(root) {
  var prevNode = null;
  var currNode = root;
  var nextNode;
  while (currNode) {
    nextNode = currNode.next;
    currNode.next = prevNode;
    prevNode = currNode;
    currNode = nextNode;
  }
  return prevNode;
}

console.log(checkPalindrome(a)); = true

I agree that your code runs in \$O(n)\$ time and \$O(1)\$ space complexity, but the issue I have is that you do in-place modification of the list..... and thus a "processed" list is no longer the same list as it started as.

This makes the function useless for most practical purposes. A list that returns "true" on one call, will likely not return "true" on the second call.

A compromise solution would be to restore the list linking after reversing the second half, but your method will still not be reentrant - even though it would be more convenient to use.

In other words, you are using the list itself as a place to store temporary "state", and that list is not entirely contained in your function. This is as bad, or worse, than using global variables.

In a real interview I would challenge the requirement to have the \$O(1)\$ space, and suggest that \$O(n)\$ space is an acceptable compromise - or that a double-linked list is a better data structure to contain this problem.


Iterate through a 2D array. Kotlin extension method Simple extension method for Kotlin. Should perform an action on every element in a 2d array. i.e. An array of arrays.

I have made the assumption that because the receiver type is defined as an array of arrays of type T, that I do not need to do any checks or casts.

fun <T> Array<Array<T>>.forEach2d(action: (T) -> Unit): Unit {
    for (outer in this) {
        for (inner in outer) {
            action(inner)
        }
    }
}

s/forEach2d/forEach2D/

Meaning: "2d" can mean two-dimensional but it can also mean "second" while "2D" is unequivocally an abbreviation for two-dimensional.
Consistency: In java.awt.geom, there is a Point2D and no Point2d.
Convention:

Naming Style - Coding Conventions - Kotlin Programming Language
Code Conventions for the Java Programming Language: 9. Naming Conventions

Make it an inline function for performance reasons.
Consider renaming outer and inner. At first glance a reader might think that outer and inner refer to the outer array and an inner array but this isn't the case as outer is in fact an inner array and inner is an element in an inner array. e.g. s/outer/array/g/ and s/inner/element/g.
Optionally omit the braces on the 'for' loops. I don't feel strongly about this one either way. I personally keep them but for something as simple as this where there really isn't any logic it makes me wonder why explode it into so many lines. However, I've worked on teams where it is our convention to always use braces for if/for/etc. constructs so be consistent above being brief.
Document your Kotlin Code. It may seem like a small thing but it can also be surprisingly amazing how much more real/professional/reusable/etc. your code is once you and others can discover methods and documentation is right there explaining exactly what it does and doesn't do.

Example:

/**
 * Performs the given [action] on each element in each array.
 */
inline fun <T> Array<Array<T>>.forEach2D(action: (T) -> Unit): Unit {
    for (array in this) for (element in array) action(element)
}