Less For, more MATLABesque? I have written a piece of MATLAB code that (hopefully) calculates CECT. I have two matrices of psychophysiological measures - one for brain data, one for heart rate; both are of the format epochs * times (i.e., multiple epochs of 10-second long intervals). The supposed outcome is to calculate the correlation between EEG in a short window using narrow bins, and heart rate in a longer window using wider bins. The basic idea is that an early brain effect predicts a heart rate speedup a few seconds later. In the words of the author (from the paywalled paper in the link):

  To allow CECT computation across the 350–420 trials available for analysis per individual after artifact screening, EEG segments were divided into 50 bins of 15.625 ms each, ranging from 0 ms (stimulus onset) to 781.25 ms. HP segments were divided into 10 bins of 500 ms, ranging from 0–5 s. Next, CECTs were computed by correlating (Pearson’s r) each EEG bin with each HP bin across trials within subjects, resulting in a 10 × 50 cross-correlation CECT matrix.

My code differs from that a bit in that I calculate for overlapping sliding windows instead of non-overlapping consecutive bins. Also, my windows are slightly different. However, what I'm mostly concerned with is that I have the hunch my code is extremely suboptimal.

Currently, the procedure is that I For-loop over brain data bins, collect a vector of heart rate data using a For loop, do a correlation between the heart bin vector and the single brain bin, store the resulting vector of correlations, and proceed to the next vector.

Importantly, the search windows and the bin windows for heart and brain data are of different length (heart rate variations are slower than brain effects).
Also, this will in the end be calculated for multiple subjects; I have omitted this dimension of the data.

% data is a matrix of format (channels * times * epochs)
% times is a matrix of time points corresponding to sampling points

% CECT is supposed to be a matrix of correlation coefficients,
% dimensions (braintime * hearttime)

scalefac = 1000/samplingrate;

brainchannel = 12;  % channel # for brain data
heartchannel = 62;  % channel # for heart data

brainbin = 10;      % bin length for brain data (symmetric, in msec)
heartbin = 250;     % bin length for heart data (= 500 msec window)

heart = 5500;       % max point (from zero, in msec) for heart data
brain = 2500;       % max point (from zero, in msec) for brain data
first = -500;       % first point (from zero, in msec)

yy = 1;             % counter for looping over brain data
for x = find(times>first,1):find(times>brain,1)
    t0 = (x-brainbin/scalefac);
    t1 = (x+brainbin/scalefac);
    eeg = mean(squeeze(data(brainchannel,t0:t1,:)),1);
    p=1;
    clear hps
    for y = find(times>first,1):find(times>heart,1)
        t0 = (y-heartbin/scalefac);
        t1 = (y+heartbin/scalefac);

        hps(p,:) = mean(squeeze(data(heartchannel,t0:t1,:)),1);
        p = p+1;    % counter for looping over heart data
    end
    CECT(yy,:) = corr(squeeze(eeg).',hps.');
    yy = yy+1;
end

How can I make this code less awful (faster, more structured)?

With this random data and a few tweeks I got the time reduced about 30 times. The inner loop can be taken outside which is the biggest difference.

clear all

n=800;
data = randn(2,n,30);
times=linspace(-1900,8600,n);

tic;

samplingrate = 200;
scalefac = 1000/samplingrate;

brainchannel = 12;  % channel # for brain data
heartchannel = 62;  % channel # for heart data

brainchannel = 1;
heartchannel = 2; %adjusted for test data

brainbin = 10;      % bin length for brain data (symmetric, in msec)
heartbin = 250;     % bin length for heart data (= 500 msec window)

heart = 5500;       % max point (from zero, in msec) for heart data
brain = 2500;       % max point (from zero, in msec) for brain data
first = -500;       % first point (from zero, in msec)

xvec=find(times>first,1):find(times>brain,1);
yvec=find(times>first,1):find(times>heart,1);

CECT=zeros(length(xvec),length(yvec));  %initialize

braindata=squeeze(data(brainchannel,(xvec(1)-heartbin/scalefac):(xvec(end)+brainbin/scalefac),:));
bn=length((xvec(1)-brainbin/scalefac):(xvec(1)+brainbin/scalefac));

heartdata=squeeze(data(heartchannel,(yvec(1)-heartbin/scalefac):(yvec(end)+heartbin/scalefac),:));
hn=length((yvec(1)-heartbin/scalefac):(yvec(1)+heartbin/scalefac));
hps=zeros(length(yvec),size(data,3));

p=1;
for y = yvec
    hps(p,:) = mean(heartdata(p:p-1+hn,:),1);
    p = p+1;    % counter for looping over heart data
end

yy = 1;             % counter for looping over brain data
for x = xvec
    eeg = mean(braindata(yy:yy-1+bn,:),1);
    CECT(yy,:) = corr(squeeze(eeg).',hps.');
    %%CECT(yy,:) = eeg * hps.'; %for testing
    yy = yy+1;
end

toc


Writing a Pythonic prime sieve I'm practicing writing code less by brute force. I'd like feedback for writing a prime sieve more Pythonic, simpler, or more creatively perhaps.

def prime_sieve(limit):
    numbers = dict()
    limit = int(limit)
    for n in range(2, limit + 1):
        numbers[n] = ()
    primes = numbers.copy()
    for n in numbers:
        m = n
        while m <= limit:
            m = m + n
            primes.pop(m, 0)
    return primes

Your implementation of the sieve, using a dictionary, throws away many of the benefits of the approach; dictionaries are unordered. Even a naive list-based implementation ends up processing fewer than half as many numbers (146 vs. 382 with a limit of 100):

def sieve(limit):
    nums = range(limit+1)
    nums[1] = 0
    for n in nums:
        if n:
            for x in range(2*n, limit+1, n):
                nums[x] = 0 # counting this vs. pop
    return set(filter(None, primes))

Your approach processes multiples of numbers that will subsequently turn out to be non-prime themselves, e.g. you might pop all multiples of 9, then go back over them when you later process 3.

Note that I have used a set, which is effectively a dictionary with no values, rather than have some dummy value.

Also, there was no need to copy the whole dictionary; for n in numbers.keys() would have worked fine.

There are numerous other implementations, in Python and other languages, on this site and elsewhere; I suggest you look at a few to see what other alterations could be made.


Filtering a name Array in JavaScript I was wondering if there is a better, more elegant way to write the following JavaScript code block:

var searchString = 'ma';
var names = ['Mark Kennel', 'Hellen Smith', 'Jane Mary Annet', 'Peter'];

var filteredNames = names.filter(name => {
  var words = name.toLowerCase().split(' ');
  for(var i = 0; i < words.length; i++) {
    if (words[i].indexOf(searchString.toLowerCase()) === 0 )
      return true;
  }
}

This runs in a function that filters an autocomplete input results. The filtering rule is:

  Show the names that have a word which begins with the searchString

The above code (with searchString = 'ma') should return an array with two names: 'Mark Kennel' and 'Jane Mary Annet'.

Well, better and more elegant are quite opinion based and depend on different things: a code can be better regarding performance but worse regarding clarity.

That being said, I'd like to drop my two cents: since you just want to know if a given word begins with the input string (which is different from containing the input string) you can use lastIndexOf, setting fromIndex as 0. That way, you don't search the whole string, but just its beginning:

var searchString = 'ma';
var names = ['Mark Kennel', 'Hellen Smith', 'Jane Mary Annet', 'Peter'];

function filterNames(arr, str) {
  return arr.filter(function(thisName) {
    return thisName.toLowerCase().split(" ").some(function(d) {
      return d.lastIndexOf(str.toLowerCase(), 0) === 0
    })
  })
}

console.log(filterNames(names, searchString))

Another obvious option here is using startsWith. However, unlike lastIndexOf, startsWith don't wok on IE (if you care for that).


Object-oriented calorie counter Object-oriented program for a calorie counter sporting polymorphic
methods for both genders.

Principles to adhere to:

Uniform Access Principle
Polymorphism
Dependency inversion principle through the Factory Method pattern
Single responsibility principle

Concerns for future extension:

BPM curve for exercise acts of longer duration, can this be transparently added?

Other concerns:

Unit prefixes for names, e.g. weight_kg, exercise_duration_min. Are these bad?
Overengineered? Yes, I'm not going to argue against that; we don't have more than one method in our classes which eliminates the need to have them at all. Assuming we had more than one method, however, is it still overdone?
Are any of the principles I claimed to follow not followed or properly followed? Especially thinking of the Factory Method pattern.

By the way, the formula for calculating calories burned doesn't seem to be right, but that doesn't really matter for our purposes.

# oo-calorie_counter.py
from abc import ABC, abstractmethod

def main():
    """Setup and initialization.
    """
    person_attributes = get_person_attributes()
    person = PersonFactory.make_person(person_attributes)

    print("You've burnt approximately {0} calories".format(round(person.calories_burned, 1)))

def get_person_attributes():
    """Prompts a user for their details
    """
    person_attributes = {
        'gender': input('Your gender: '),
        'weight_kg': int(input('Your weight in Kg: ')),
        'age': int(input('Your age: ')),
        'bpm': int(input('Your heart rate: ')),
        'exercise_duration_min': int(input('Your your exercise duration in minutes: '))
    }

    return person_attributes

class PersonFactory:
    @staticmethod
    def make_person(person_attributes):
        """
        Instantiate the proper a person of proper gender based on the provided attributes.
        Args:
            person_attributes: A dictionary containing the keys:
            ('gender', 'age, 'weight_kg', exercise_duration_min, bpm)

        """
        gender = person_attributes['gender']
        del person_attributes['gender']

        if gender.lower() == 'male':
            return Male(**person_attributes)
        elif gender.lower() == 'female':
            return Female(**person_attributes)
        else:
            raise RuntimeError('Unknown gender')

class Person(ABC):
    def __init__(self, weight_kg, bpm, age, exercise_duration_min):
        self.weight_kg = weight_kg
        self.age = age
        self.bpm = bpm
        self.exercise_duration_min = exercise_duration_min

    @property
    @abstractmethod
    def calories_burned(self):
        raise NotImplementedError

class Male(Person):
    @property
    def calories_burned(self):
        return ((self.age * 0.2017) - (self.weight_kg * 0.09036) + (self.bpm * 0.6309) - 55.0969) * self.exercise_duration_min / 4.184

class Female(Person):
    @property
    def calories_burned(self):
        return ((self.age * 0.074) - (self.weight_kg * 0.05741) + (self.bpm * 0.4472) - 20.4022) * self.exercise_duration_min / 4.184

if __name__ == '__main__':
    main()

I like your code. It is easy to understand and pretty straight-forward.

Regarding your question of the measuring units: yes, every variable should have them since you are not operating exclusively on SI units. You have calories (instead of joule) and years and minutes (instead of seconds).

I would just change some of the smaller details, from top to bottom:

The doc comment for main is wrong. That function not only initializes the program, it is the whole program, including computation and output.
Rename get_person_attributes to read_person_attributes or input_person_attributes. Functions called get_* usually don't have side-effects and are usually not complicated.
The gender prompt could be more specific: 'Your gender (male, female): '
The official spelling for kilogram is kg, not Kg. Although kilo is a large multiplier, it is the only one with a lowercase letter.
The prompt for the heart rate could include in bpm.
The Your your is a typo.
In make_person, the proper a person of proper gender sounds wrong (though I'm not a native English speaker).
You compute gender.lower() twice, which is unnecessary.
Do you prefer burned or burnt? Choose one and use it everywhere.

Having each of the complicated formulas in a single line is very good. One might be attempted to split it into several small formulae or extract the factors to a separate class CaloriesCalculatorFactors, but that would make the code harder to understand.


Swift HackerRank Balanced Brackets I solved this stack problem in Swift. Looking for any feedback on my code:

import Foundation
/// Returns an integer read from one line of standard input.
func readInteger() -> Int {
    guard let line = readLine() else {
        fatalError("Unexpected end of input")
    }
    guard let i = Int(line) else {
        fatalError("Invalid integer in input")
    }
    return i
}

/// Returns an array of Characters
func readStrings() -> [Character] {
    guard let line = readLine() else {
        fatalError("Unexpected end of input")
    }
    return line.characters.map{ $0 }
}

// checks if all brackets are balanced
func balancedBrackets() {
    let sequences = readInteger()
    // outloop loops through each sequence line
    outerLoop: for _ in 0..<sequences {
        // create a stack (array in this case)
        var stack = [Character]()
        // each line of sequence
        let sequence = readStrings()
        // edgeCase is when there are only opening brackets in stack and didn't reach case } ] )
        var edgeCase = false
        // inner loop loops through each bracket character
        innerLoop: for bracket in sequence {
            switch bracket {
            // if opening brackets, append and set edgeCase
            case "{", "[", "(":
                stack.append(bracket)
            // case closing brackets
            case "}", "]", ")":
                // checks for empty stack or if the bracket pairs arent matching
                if stack.isEmpty || (bracket == "}" && stack.last != "{") || (bracket == "]" && stack.last != "[") || (bracket == ")" && stack.last != "(")  {
                    edgeCase = true
                    print("NO")
                    // append closing bracket so YES doesn't print when breaking
                    stack.append(bracket)
                    // break out of checking anymore brackets
                    break innerLoop
                }
                stack.removeLast()
            default:
                fatalError("unknown bracket found")
            }
        }
        // if empty
        if stack.isEmpty {
            print("YES")
        } else if !edgeCase { // stack has opening brackets and hasn't reach if statement in case } ] )
            print("NO")
        }
    }
}

balancedBrackets()

readStrings() is better named readCharacters() because that is what it does.
I prefer Array(...) instead of .map { $0 } to convert a sequence into an array,
but that is a matter of taste:

/// Reads one line from standard input and returns the result
/// as an array of characters.
func readCharacters() -> [Character] {
    guard let line = readLine() else {
        fatalError("Unexpected end of input")
    }
    return Array(line.characters)
}

My main point of criticism is that
the logic in your main function is far too complicated. The outerLoop: label is not
used. The inner loop is confusing, and comments like

    // append closing bracket so YES doesn't print when breaking

    // stack has opening brackets and hasn't reach if statement in case } ] )

clearly indicate a code smell. The problem is that you have one single function doing
all the work.

It immediately becomes simpler if you separate the I/O from the
actual computations (which is generally a good idea):

func isBalanced(sequence: [Character]) -> Bool {
    // ... return `true` or `false` ...
}

func balancedBrackets() {
    let numSequences = readInteger()
    for _ in 0..<numSequences {
        let sequence = readCharacters()
        let balanced = isBalanced(sequence)
        print(balanced ? "YES" : "NO")
    }
}

balancedBrackets()

This makes the code more modular, better readable, and  allows you to add
test cases easily. The isBalanced() function can "early return" if a non-match
is found, making the labels and the special edgeCase variable obsolete:

func isBalanced(sequence: [Character]) -> Bool {
    var stack = [Character]()
    for bracket  in sequence {
        switch bracket {
        case "{", "[", "(":
            stack.append(bracket)
        case "}", "]", ")":
            if stack.isEmpty
                || (bracket == "}" && stack.last != "{")
                || (bracket == "]" && stack.last != "[")
                || (bracket == ")" && stack.last != "(")  {
                return false
            }
            stack.removeLast()
        default:
            fatalError("unknown bracket found")
        }
    }
    return stack.isEmpty
}

But the repeated usage of character literals is still error-prone.
Better define an enumeration:

enum Bracket: Character {
    case Left = "("
    case Right = ")"
    case LeftCurly = "{"
    case RightCurly = "}"
    case LeftSquare = "["
    case RightSquare = "]"
}

Determining the matching open bracket for a given closing bracket can be
made a computed property of this enumeration:

enum Bracket: Character {
    case Left = "("
    case Right = ")"
    case LeftCurly = "{"
    case RightCurly = "}"
    case LeftSquare = "["
    case RightSquare = "]"

    /// For a closing bracket, the corresponding opening bracket is returned.
    /// For an opening bracket, `nil` is returned.
    var matchingOpen: Bracket? {
        switch self {
        case .Right:        return .Left
        case .RightCurly:   return .LeftCurly
        case .RightSquare:  return .LeftSquare
        default:            return nil
        }
    }
}

Now the isBalanced() function does not use any explicit bracket values anymore:

func isBalanced(sequence: [Character]) -> Bool {
    var stack = [Bracket]()
    for char in sequence {
        if let bracket = Bracket(rawValue: char) {
            if let open = bracket.matchingOpen {
                // `bracket` is a closing bracket and `open` the corresponding opening bracket:
                guard let last = stack.last where last == open  else {
                    return false
                }
                stack.removeLast()
            } else {
                // `bracket` is an opening bracket:
                stack.append(bracket)
            }
        } else {
            fatalError("unknown bracket found")
        }
    }
    return stack.isEmpty
}

If you decide to add another type of brackets later (e.g. «») then only the
enumeration needs to be extended, but not the isBalanced() function.


str_replace vs array_map & trim on URL segments Are there any advantages on using array_map & trim vs using simple str_replace? The input for both is an array of matched URL segments. I find it nice to be able to provide extra logic when using array_map but seems to be ~2 times slower. I did some trivial benchmarking

for ($i = 0; $i < 10000; $i ++) {
    array_map(function($argument) {
        return trim($argument, '/');
    }, array_slice($match, 1));
}

$end_time = microtime(TRUE);

echo $end_time - $start_time;

echo "<br/>";

$start_time = microtime(TRUE);

for ($i = 0; $i < 10000; $i ++) {
    str_replace('/', '', array_slice($match, 1));
}

$end_time = microtime(TRUE);

echo $end_time - $start_time;

Outputs:

0.0675988197327
0.0296301841736

Which version to use

I wouldn't care about the timing difference. There may be situations where it matters, but in almost all cases, both variants will be so fast that the difference becomes irrelevant (we are talking about a fraction of a millisecond here) and other factors should decide which version you use.

In this case, those other factors might be readability and reusability/adaptability. I think the second version is more readable, but as you said, the first version can be easily adapted.

So if you predict that you need to perform additional changes to the segments, go with the first approach, otherwise with the second.

Differences

Please note that the versions are not the same (the first one only trims slashes, while the second one removes them all). For your use-case, this doesn't matter.

But as this code will probably be wrapped in a function which may then be reused in different contexts (you could eg pass the slash as argument, and then use the function to trim whitespace from arrays (for the first version), or you could use it to remove an unwanted character from the segments (for the second version)), it still matters.


Saving and listing Category model I have this ExpressJS route for saving and listing Category model.

exports.save = function(req, res, next){
  //validate here>>>
  var name = req.body.name;
  var description = req.body.description;

  new Category({
    name: name,
    description: description,
  }).save(function(err, docs){
    if ( err ) {
      console.log('error: saving category');
      next(error);
    }else{
      console.log('category saved.');
      res.redirect('categories');
    }
  });
}

exports.list = function(req, res, next){
  Category.find(function ( err, categories, count ){
    if (err) {
      console.log('error: listing categories');
      next(error);
    }
    res.render( 'categories/list', {
        categories : categories
    });
  });
}

Is having console.log and next(error) OK?  How could I improve the above code in terms of conventions in NodeJS / ExpressJS?

I assume you have a typo there, error is nowhere defined but you do have err.

To jackdbernier's point, you should read the docs:
http://expressjs.com/guide.html#users-online

In there it is clear that you should call next regardless whether you have an error or not, otherwise not all middlewares will be processed.

If you insist on logging before call next, you could.

function handleError( message , err , next )
{
  console.error( message );
  return next( err );
}

new Category({
  name: name,
  description: description,
}).save(function(err, docs){
  if ( err ){
    return handleError( 'error: saving category' , err, next );
  }
  console.log('category saved.');
  res.redirect('categories');
  next(); // <- this!
});
}

Though I would personally drop the curly braces from that if( err ).


Row/Column Transpose I was wondering if there is a smarter way of doing the following code. Basically what is does is that it opens a data file with a lot of rows and columns. The columns are then sorted so each column is a vector with all the data inside.

"3.2.2 - Declare variables"
lineData    = list()

for line in File:
  splittedLine = line.split() # split
  lineData.append(splittedLine) #collect

And here the fun begins

"3.2.3 - define desired variables from file"
col1    = "ElemNo"
col2    = "Node1"
col3    = "Node2"
col4    = "Length"
col5    = "Area"
col6    = "Inertia"
col7    = "Fnode1"
col8    = "Fnode2"
col9    = "SigmaMin"
col10   = "SigmaMax"

"3.2.3 - make each variable as a list/vector"
var ={col1:[], col2:[], col3:[], col4:[], col5:[], col6:[], col7:[], col8:[]
     ,col9:[],col10:[]}

"3.2.3 - take the values from each row in lineData and collect them into the correct variable"
for row in lineData:
  var[col1] .append(float(row[0])      )    #[-]    ElemNo
  var[col2] .append(float(row[1])      )    #[-]    Node1
  var[col3] .append(float(row[2])      )    #[-]    Node2
  var[col4] .append(float(row[3])      )    #[mm]   Length
  var[col5] .append(float(row[4])      )    #[mm^2] Area
  var[col6] .append(float(row[5])*10**6)    #[mm^4] Inertia
  var[col7] .append(float(row[6])      )    #[N]    Fnode1
  var[col8] .append(float(row[7])      )    #[N]    Fnode2
  var[col9] .append(float(row[8])      )    #[MPa]  SigmaMin
  var[col10].append(float(row[9])      )    #[MPa]  SigmaMax

As you see this is a rather annoying way of making each row into a variable. Any suggestions?

First of all don't create variables for those keys, store them in a list.

keys = ["ElemNo", "Node1", "Node2", "Length", "Area", "Inertia",
        "Fnode1", "Fnode2", "SigmaMin", "SigmaMax"]

You can use collections.defaultdict here, so no need to initialize the dictionary with those keys and empty list.

from collections import defaultdict
var = defaultdict(list)

Now, instead of storing the data in a list, you can populate the dictionary during iteration over File itself.

for line in File:
    for i, (k, v) in enumerate(zip(keys, line.split())):
        if i == 5:
            var[k].append(float(v)*10**6)
        else:
            var[k].append(float(v))


Convert char array to number C++17 have <charconv>. However this header does not work on MacOS - It compiles, but can not link. Same header is not present in MSVS. The header also missing from my Arm Linux, even my gcc compiler there supports C++17 as well.

Following is a take to convert non terminated char array (e.g. buffer, string_view etc) into signed or unsigned integer.

I probably will use the code in production, once I do some more tests.

#include <limits>
#include <type_traits>
#include <string_view>
#include <utility>

namespace impl{
    template<typename T>
    T x10(T a){
        return a * 10;
    }

    inline char digit(char c){
        return c - '0';
    }

    inline bool is_space(char c){
        return c == ' ' || c == '\t' || c == '\r' || c == '\n';
    }

    inline bool is_sign(char c){
        return c == '+' || c == '-';
    }

    inline bool is_digit(char c){
        return c >= '0' && c <= '9';
    }

    template<typename T>
    std::pair<bool,T> convert_unsigned(const char *c, const char *e){
        T num = 0;

        while(c != e && is_digit(*c)){
            T tmp = x10(num) + digit(*c);

            if (tmp < num)
                return { false, num };

            num = tmp;

            ++c;
        }

        return { true, num };
    }

    template<typename T>
    std::pair<bool,T> convert(const char *c, const char *e, std::true_type /* unsigned */){
        if (c != e && *c == '+')
            ++c;

        return convert_unsigned<T>(c, e);
    }

    template<typename T>
    std::pair<bool,T> convert(const char *c, const char *e, std::false_type /* unsigned */){
        using U = std::make_unsigned_t<T>;

        T sign = +1;
        U max  = std::numeric_limits<T>::max();

        if (c != e && is_sign(*c)){
            if (*c == '-'){
                sign = -1;
                max  = - (U) std::numeric_limits<T>::min();
            }

            ++c;
        }

        // std::cout << ">>> " << sign << ' ' << max << ' ' << '\n';

        auto [ ok, result ] = convert_unsigned<U>(c, e);

        if (result > max)
            return { false, (T) result };

        if (result == max)
            return { true, (T) result };

        return { ok, sign * (T) result };
    }
}

template<typename T>
std::pair<bool,T> convert(std::string_view const s){
    static_assert(std::is_integral_v<T>);

    return impl::convert<T>(std::begin(s), std::end(s), std::is_unsigned<T>{});
}

#include <cstdint>
#include <iostream>

template<typename T>
void test(const char *s, T correct){
    auto [ok, i] = convert<T>(s);

    if (!ok)
        std::cout << "Err" << '\n';

    std::cout << ( i == correct ? "OK" : "NO") << ' ' << i << '\n';
}

int main(){
    test<uint16_t>  ("-0",              std::numeric_limits<uint16_t>::min()    );
    test<uint16_t>  ("65535",           std::numeric_limits<uint16_t>::max()    );
    test<int16_t>   ("+32767",          std::numeric_limits<int16_t>::max() );
    test<int16_t>   ("-32768",          std::numeric_limits<int16_t>::min() );
    test<int16_t>   ("+0",              0                   );
    test<int16_t>   ("-0",              0                   );
    test<int16_t>   ("+10",             +10                 );
    test<int16_t>   ("-10",             -10                 );

    test<uint64_t>  ("+0",                      std::numeric_limits<uint64_t>::min()    );
    test<uint64_t>  ("+18446744073709551615",   std::numeric_limits<uint64_t>::max()    );
    test<int64_t>   ("+9223372036854775807",    std::numeric_limits<int64_t>::max() );
    test<int64_t>   ("-9223372036854775808",    std::numeric_limits<int64_t>::min() );
}

We're missing any documentation or template constraints, but it appears that this code parses only decimal integers, and silently ignores anything after the first non-digit.  There are other departures from the interface of the functions provided in <charconv>:

a leading + is accepted and ignored
not all out-of-range values are detected.

Worse, there's no prevention of signed integer overflow, so there's undefined behaviour on certain inputs.
The avoidance of signed overflow is fragile and could be made clearer to the reader (with suitable static_assert() if requires isn't palatable, for instance).

I think that writing a function to multiply by 10 is taking functional decomposition a step too far.  I would say the same for subtracting '0', but that could be arguable if it were home for a comment explaining how C++ guarantees that the characters '0'..'9' are encoded as contiguous values.

There are no tests for any of the failure cases, and all the tests have to be checked externally.  It's better to make main() return an error status unless all the tests pass (and diagnostic output should go to std::clog or std::cerr as appropriate, rather than std::cout).


Median of two sorted equal sized arrays on combination The comprehensive description of the problem can be found here.  I'm looking for code review, clever optimizations, adherence to best practices. etc.

This code is also a correction of code previously posted here.

/**
 * @author javadeveloper
 */
public final class MedianOfTwoSortedArrays {

    private MedianOfTwoSortedArrays() {}

    private static boolean isMedian (int[] a1, int[] a2, int pos) {
        if (pos == 0 || pos == (a1.length - 1)) return true;
        return (a1[pos] >= a2[pos]) && (a1[pos] <= a2[pos + 1]);
    }

    private static double calculateMedian(int[] a1, int a1median, int[] a2, int a2median) {
        if (a1median == 0 || a2median == 0 || a1[a2median] < a1[a1median + 1]) return (a1[a1median] + a2[a2median])/2.0;
        return ((a1[a1median] + a1[a1median + 1])/2.0);
    }

    /**
     * Given two sorted arrays of same length it returns the median.
     * If arrays are not of equal length throws the IllegalArgumentException.
     * If arrays are not sorted the results can be unpredictable.
     *
     * @param a1    the first sorted array
     * @param a2    the second sorted array
     * @return      the median of the two integers or null if no such median is found due to illegal input.
     */
    public static Double median (int[] a1, int[] a2) {
        if (a1.length != a2.length) throw new IllegalArgumentException("The argument thrown is illegal");

        int lb = 0;
        int hb = a1.length - 1;

        while (lb <= hb) {
            int a1Median = (lb + hb)/2;
            int a2Median = (a2.length - 1) - a1Median;

            // is one of the element constituting the median in first array ?
            if (isMedian(a1, a2, a1Median)) {
                return calculateMedian(a1, a1Median, a2, a2Median);
            }

            // is one of the element constituting the median in second array ?
            if (isMedian(a2, a1, a2Median)) {
                return calculateMedian(a2, a2Median, a1, a1Median);
            }

            if (a1[a1Median] < a2[a2Median]) {
                lb = a1Median + 1;
            }

            if (a1[a1Median] > a2[a2Median]) {
                hb = a1Median - 1;
            }
        }
        return null;
    }

    public static void main(String[] args) {
        int[] a1 = {1, 3, 5, 7, 9};
        int[] a2 = {2, 4, 6, 8, 10};
        System.out.println("Expected 5.5, Actual " + median (a1, a2));

        int[] a3 = {1, 3, 5, 7, 9, 100};
        int[] a4 = {2, 4, 6, 8, 10, 200};
        System.out.println("Expected 6.5, Actual " + median (a3, a4));

        int[] a5 = {1, 2, 3, 4};
        int[] a6 = {5, 6, 7, 8};
        System.out.println("Expected 4.5, Actual " + median (a5, a6));

        int[] a7 = {5, 6, 7, 8};
        int[] a8 = {1, 2, 3, 4};
        System.out.println("Expected 4.5, Actual " + median (a7, a8));

        int[] a9 =  {1, 1, 10, 10};
        int[] a10 = {5, 5,  5,  5};
        System.out.println("Expected 5.0, Actual " + median (a9, a10));

        int[] a11 = {5, 5, 5, 5};
        int[] a12 = {1, 1, 10, 10};
        System.out.println("Expected 5.0, Actual " + median (a11, a12));
    }
}

Short Review

I have spent some hours trying to understand what you do, and why. Given the mess I made of the previous posting of this question, I thought I should spend extra effort to get this one right.

I have failed. I cannot understand the full intent of your code, and, even though it appears to work, I have this nagging feeling that some well-constructed uses cases will cause the code to fail - you have too many edge-case conditions in the code which are not documented, and not contextualized enough to ascertain whether they work, or what conditions they are supposed to handle.

Bottom line is that your program is not understandable in a reasonable amount of time, and it was your responsibility to make your code understandable, not the reviewer's responsibility to decode your system. Just because you may have your solution 'straight' in your head, it does not mean that you have expressed it well in the code. You have to write and document the code so that someone who has only just seen the problem can understand it and the solution with 'reasonable' effort. In this case, I don't believe you have succeeded.

Longer Review

In a 'real' review there is no "reference implementation" to compare against. All there is, is a specification. In this case, the specification is to identify the median (middle value, or average of middle-pair if the data-set is even-sized). In addition to the expected result, the data is assumed to be in two equal-sized arrays of sorted data.

Given those specifications you can also make a couple of assumptions:

the ending-dataset will always have an even number of elements (2 * x is always even for any integral value x), which means we will always be doing some form of average.
There will always be n-1 values smaller than the smaller of the mid-point-pair, and n-1 values larger than the higher of the mid-point-pair.

OK, based on those specifications and assumptions, we can review the code, and see how it solves the problem.

First thing I note is that there are two variables which have names that don't seem to relate to any feature of the problem space, lb and hb. There is no comment to indicate what these variables are, so the only choice is to inspect the code to see how it is used. This means we have to trust that the code is doing the right thing just so we can understand what the intended use of the variable is.

This is a bad practice, and is solved by using self-documenting variable names. This has been pointed out to you before. You need to start improving your code-style.
When I try to track down what lb and hb are supposed to be, I find that they are used as a loop condition, which does not help us in this case because the loop condition is not documented. They are also used to calculate the variables a1Median and a2Median. Unfortunately, a1Median, although it is a descriptive name, does not mean what it says it means.... it is not the Median of array a1. It starts off being the approximate midpoint of a1 (the median is the value at the midpoint, not the position of the midpoint), but then, to make things worse, the a1Median is modified in each loop! So, this descriptive variable name has the completely wrong description. This is worse than having a non-descriptive name because, now the person reading the code has to keep remembering that "a1Median is not the median of array a1"!

At this point, in any 'serious' review, you would be facing a lot of criticism. There are times when it is OK for code to be hard to read... but that is only when the problem is very complicated. What you have here is unnecessarily hard to read.

OK, after some study (and I literally mean some serious head-scratching, debugging, and paper-worked examples), I think I can see your algorithm.....

set up lb and hb (still not sure what those are supposed to stand for - low-something and high-something ?) as pointers in to the first array.
we will manipulate these high and low pointers to select a 'candidate' value in the first array.
we then use the candidate point in the first array and calculate a candidate point in the second array. The second array's candidate is always going to be where there are n-1 values smaller/larger than the two candidate positions. If our candidate in array1 is x then the candidate in array2 will be array2 - x - 1 which would satisfy the midpoint-pair condition.
if the values at these candidate positions are in fact the midpoints, then we can return a success.
if we have a success, we do not actually know if the candidate values are in fact the actual pair, all we know is that one of the candidates is the midpoint. The other candidate may not be part of the solution if we have two of the same values on our side... in which case both of the candidates need to come from one array.
if we do not have a success, we essentually do a 'bifurcation' of the data to do a binary search-style partitioning of the data to look for a candidate.

So, your algorithm has this isMedian() method. Unfortunately, this method re-uses two already bad variable names as parameters. We now have a different a1 and a2, which are not the same as the a1 and a2 in the calling method, because sometimes the calling method swaps the order of these.... Still, this method returns true under conditions which make no apparent sense. Why is there a median when pos == 0 ? That seems pretty arbitrary.

This same problem exists in the untangling method calculateMedian(). a1 and a2 which are not the same as before.

Speaking of untangling, why do you need to untangle? You should not get in to a tangle to start with!

Right, so you have an apparent algorithm, but there are still blanks in what it should be doing, or how it gets it done.

At this point I cheated, and looked at the sample code in the problem description, and your system does none of the algorithms.

The first sample algorithm uses naive searching for the median points. Your algorithm does not match that.

The second algorithm uses a reduction-to-size-2 system, and then does a Min/Max formula to get the final result. You do not do this either.

The final algorithm uses recursion and bifurcation. You do not do recursion.

Bottom line is that you are using an algorithm that is hard to read, not comparable to reference systems, and, in the long run, is not much fun to review.

Since I do these reviews for my (twisted) satisfaction as well, I can't say that this one was worth it.

Your opening request is:

  I'm looking for code review, clever optimizations, adherence to best practices. etc.

The problem-page you pulled this question from does a perfectly fine job of presenting good code (albeit in C), clever optimizations (that are reproducible in Java quite easily), and the best practices shown there are not great given that it is for C and not Java, but certainly consistent and readable. You have taken those suggestions and ruined them.

Code Review is for reviewing code. I have had to resort to debugging your code just to get a sense of what it does. This is not where code should be when you present it for review.


Checking MIME Type from a base64 string I've written the following extension to determine the MIME type of a base64 string.  It's worked in my local tests, but can anyone point out issues or alternate methods?

public static AttachmentType GetMimeType(this string value)
{
    if(String.IsNullOrEmpty(value))
        return new AttachmentType
                {
                    FriendlyName = "Unknown",
                    MimeType = "application/octet-stream",
                    Extension = ""
                };

    var data = value.Substring(0,5);

    switch (data.ToUpper())
    {
        case "IVBOR":
        case "/9J/4":
            return new AttachmentType
                {
                    FriendlyName = "Photo",
                    MimeType = "image/png",
                    Extension = ".png"
                };

        case "AAAAF":
            return new AttachmentType
                {
                    FriendlyName = "Video",
                    MimeType = "video/mp4",
                    Extension = ".mp4"
                };
        case "JVBER":
            return new AttachmentType
                {
                    FriendlyName = "Document",
                    MimeType = "application/pdf",
                    Extension = ".pdf"
                };

        default:
            return new AttachmentType
                {
                    FriendlyName = "Unknown",
                    MimeType = string.Empty,
                    Extension = ""
                };
    }
}

public class AttachmentType
{
    public string MimeType { get; set; }
    public string FriendlyName { get; set; }
    public string Extension { get; set; }
}

Your switch is wrong: "/9J/4" is for JPG images, not PNG (e.g. as shown in these examples).


Basic Equation Tokenizer Recently I've been doing some experimenting with RPN and the shunting-yard algorithm, in order to test these systems more appropriately I planned on writing a tokenizer and then using these tokens to check validity and eventually get some output. I also think that I could use this to work with some primitive programming language, such as making a CHIP-8 assembler.
Function
The intention is for my tokenizer to separate the input string into a list of the following:

Individual Symbols ('(', ')', '*', etc...)
Sequences of digits ('1', '384', etc...)
Sequences of characters ('log', 'sin', 'x', etc...)

Note that because of this sequences such as:

'3.14' (parsed as '3', '.', '14')
'6.02E23' (parsed as '6', '.', '02', 'E', '23')

Will not come out as the numbers they represent but can be reconstructed later on.
But sequences such as '3x' will come out as '3', 'x' making it easier to account for multiplication of variables.
Questions
For the most part I'm quite happy with this code, a couple things that I'm interested in (alongside general review) are:

How can I make the line if l.isalpha() and buf.isdigit() or l.isdigit() and buf.isalpha(): more concise?
What about the if buf: out += [buf]; buf = '' lines? Would there be anything wrong with putting this inside a nested function in tokenize? Or would out, buf = out + [buf], '' be more pythonic?
This technique makes it easier later on to identify function calls such as min, max or sin, but how would I differentiate the meanings of 'xy'? (x*y vs a variable actually called xy, also this question is less relevant in the context of programming languages which would parse 'xy' as a single token over the multiplication of 2)(This question is possibly out of scope for CR, if so this question can be removed)

The reasons for these questions specifically is that I like concise code, writing it on few lines without having any too long.
Code
def tokenize(s):
    out = []
    buf = ''
    for l in s:
        if not l.isalnum():
            if buf:
                out += [buf]
                buf = ''
            out += [l]
        else:
            if l.isalpha() and buf.isdigit() or l.isdigit() and buf.isalpha():
                out += [buf]
                buf = ''
            buf += l
    if buf:
        out += [buf]
    return out

1. Review

There's no docstring. What does the function do? What does it return?
The result includes the spaces:

>>> tokenize('1 + 2')
['1', ' ', '+', ' ', '2']

but it seems unlikely that the spaces are significant. One of the useful things a tokenizer can do is to discard whitespace.
The tokens are collected into a list and returned. This is inflexible because you have to wait for them all to be collected before you can start processing them. But parsing tends to use one token at a time, so it is often more convenient for the tokenizer to generate the tokens one at a time using the yield instruction.
There's no error-detection:

>>> tokenize('3E$£ω∞あ')
['3', 'E', '$', '£', 'ω', '∞', 'あ']

One of the things a tokenizer ought to do is to detect and report invalid tokens. Surely it's not the case that every string is a valid input to your program?
The tokenizer does not handle floating-point numbers:

>>> tokenize('3.14159')
['3', '.', '14159']

or engineering notation:

>>> tokenize('3e-08')
['3', 'e', '-', '08']

You write in the post that they "can be reconstructed later on" but it would be easier to have the tokenizer do it.
The tokenizer does not return anything other than the tokens themselves. Usually one of the jobs of a tokenizer is to categorize tokens (numbers, names, operators etc.) and to turn string representations of numbers into the number themselves.

2. Revised code

In Python it's often convenient to implement tokenization using a regular expression and the finditer method. In this case we could write:

from enum import Enum
import re

_TOKEN_RE = re.compile(r'''
    \s*(?:              # Optional whitespace, followed by one of:
    ([()^+*/-])         # Punctuation or operator
    |([a-z]+)           # Variable or function name
    |((?:\.[0-9]+|[0-9]+(?:\.[0-9]*)?)(?:e[+-]?[0-9]+)?) # Number
    |(\S))              # Anything else is an error
''', re.VERBOSE | re.IGNORECASE)

class Token(Enum):
    """Enumeration of token types."""
    PUNCT = 0           # Punctuation or operator
    NAME = 1            # Variable or function name
    NUMBER = 2          # Number

def tokenize(s):
    """Generate tokens from the string s as pairs (type, token) where type
    is from the Token enumeration and token is a float (if type is NUMBER)
    or a string (otherwise).

    """
    for match in _TOKEN_RE.finditer(s):
        punct, name, number, error = match.groups()
        if punct:
            yield Token.PUNCT, punct
        elif name:
            yield Token.NAME, name
        elif number:
            yield Token.NUMBER, float(number)
        else:
            raise SyntaxError("Expected a token but found {!r}".format(error))

This deals with all my points in §1 above:

There's a docstring.
Spaces are discarded.
Tokens are generated one at a time.
Errors are detected and reported:

>>> list(tokenize('3E$£ω∞あ'))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "cr186024.py", line 49, in tokenize
    raise SyntaxError("Expected a token but found {!r}".format(error))
SyntaxError: Expected a token but found '$'

The tokenizer handles floating-point numbers and engineering notation:

>>> list(tokenize('1.2 3e8 .2e-7'))
[(<Token.NUMBER: 2>, 1.2), (<Token.NUMBER: 2>, 300000000.0), (<Token.NUMBER: 2>, 2e-08)]

The tokenizer categorizes the token and converts numbers to floats.


Optimization for C isMigrated function char* Migrated[]={"026","109"};

int isMigrated(char* codeB)
{
    int i;
    int n=sizeof(Migrated)/sizeof(*Migrated);

    for(i=0;i<n;i++)
    {
        if(strcmp(Migrated[i],codeB)==0)
            return 0;
    }
    return -1;
}

is this function optimised from execution time perspective ?

Migrated should contain at maximum 200 value all of them are 4 byte .

I am not sure how far you intend to expand Migrated, but something like this could be an optimisation:

char* Migrated[]={"026","109"};

int isMigrated(char* codeB)
{
    int i;
    // since we know that all strings in Migrated are 4 bytes long
    int32_t _codeB_as_int32 = *(int32_t *)codeB;
    int32_t * Migrated_as_int32 = Migrated;

    for(i=0;
        i<sizeof(Migrated)/sizeof(*Migrated); // sizeof is a compile time constant, better to put it here
        i++)
    {
        if(codeB == Migrated_as_int32[i])
            return 0;
    }
    return -1;
}

it all really depends on what you intend to have in Migrated.


Draw the tiles of the islands that are within bounds, using LibGDX I am new person regarding LibGDX. I have written the below render method. I have a randomly generations oceans (where there are islands at random positions). Within the below code, focusedMapRectangle refers to the focused part of this map. The task is to draw the tiles of the islands that are within focusedMapRectangle.

I would like some feedback regarding:

conversion from my own algorithms/code to LibGDX features (i.e. how can I use existing LibGDX features as opposed to doing the the calculations myself directly).
code style / variable names (and whether others are preferred)
inefficient calculations that I can do in more efficient ways (speed)

@Override
public void render(float delta) {
    Gdx.gl.glClearColor(0.5f, 0.5f, 1, 1);
    Gdx.gl.glClear(GL20.GL_COLOR_BUFFER_BIT);
    Gdx.gl.glClear(GL20.GL_ALPHA_BITS);
    getBatch().begin();

    Rectangle focusedMapRectangle = new Rectangle(
            getMatchMap().getBottomLeft().x,
            getMatchMap().getBottomLeft().y,
            Gdx.graphics.getWidth() / 16,
            Gdx.graphics.getHeight() / 16
    );

    getMatchMap().getIslands().forEach((island, point) -> {
        Rectangle islandRectangle = new Rectangle(
            point.x,
            point.y,
            island.getMaxWidth(),
            island.getMaxHeight()
        );

        // Signifies that the island is indeed present in the focused part of the map
        if (focusedMapRectangle.overlaps(islandRectangle)) {
            for (int islandTileX = 0; islandTileX < islandRectangle.getWidth(); islandTileX++) {
                for (int islandTileY = 0; islandTileY < islandRectangle.getHeight(); islandTileY++) {

                    // These refer to the absolute positions that each tile of the island has
                    // within the WHOLE map (not just focused)
                    int absoluteTileX = islandTileX + (int) point.x;
                    int absoluteTileY = islandTileY + (int) point.y;

                    // These refer to the position that each tile of the island has
                    // relative to the bottom left of the focused part of the map.
                    int relativeXToFocusPoint = absoluteTileX - (int) getMatchMap().getBottomLeft().x;
                    int relativeYToFocusPoint = absoluteTileY - (int) getMatchMap().getBottomLeft().y;

                    // Though we have verified that some part of the island is within the
                    // focused part of the map, it is is still possible that SOME of the island's tiles
                    // are NOT in focus.
                    boolean isXOutOfFocus = relativeXToFocusPoint < 0;
                    boolean isYOutOfFocus = relativeYToFocusPoint < 0;

                    // No need to render these said tiles of the island
                    if (isXOutOfFocus || isYOutOfFocus)
                        continue;

                    IslandTileType islandTileType = island.getRepr()[islandTileX][islandTileY];

                    // Do not go into statement if island tile type is water i.e null
                    if (islandTileType != null) {
                        Texture islandTexture = getIslandTextures().get(islandTileType);

                        int relativeCoordXToFocusPoint = relativeXToFocusPoint * 16;
                        int relativeCoordYToFocusPoint = relativeYToFocusPoint * 16;

                        getBatch().draw(islandTexture, relativeCoordXToFocusPoint, relativeCoordYToFocusPoint);
                    }
                }
            }
        }
    });

    getBatch().end();
}

Reduce Allocations

Creating objects while the game is running is not optimal. You are also creating those objects inside of the render method which will be called many thousands of times.  The garbage collector is good, but you will still see performance spikes when it is doing too much work.

Luckily you can eliminate all of these allocations!

One approach that you will find useful is to use Pools. This is very easy when working with many libGDX classes. Setting it up is just one line where 100 below is the maximum number of objects you think you will need at one time:

private final Pool<Rectangle> recPool = Pools.get(Rectangle.class, 100);

Then when you want a rectangle object:

Rectangle focusedMapRectangle = recPool.obtain();

And when you are done using the object:

recPool.free(focusedMapRectangle);

The Rectangle class has a number of set methods in order to configure it after you obtain the object.

focusedMapRectangle.setX(getMatchMap().getBottomLeft().x);
focusedMapRectangle.setY(getMatchMap().getBottomLeft().y);
focusedMapRectangle.setWidth(Gdx.graphics.getWidth() / 16);
focusedMapRectangle.setHeight(Gdx.graphics.getHeight() / 16);

I mention Pools because I think you will find it useful in the future, however I actually think there is a better approach in this case.

You do need one new Rectangle at the start of the render method because it will match the current view of the world. However, rather than creating a new object each time, you could have one Rectangle and just use the set methods above to set it to the appropriate values.

That brings me to the next offender:

getMatchMap().getIslands().forEach((island, point) -> {
    Rectangle islandRectangle = new Rectangle(
        point.x,
        point.y,
        island.getMaxWidth(),
        island.getMaxHeight()
    );

What getIslands() returns is not totally clear to me, but I can tell that each island has a point associated with it. That point does not appear to be subject to change. I also assume that the maxWidth and maxHeight of the island are not subject to change. So why not pre-calculate the Rectangle for the island when first creating it? Then you can just do

if (focusedMapRectangle.overlaps(island.getRectangle())) {

and completely avoid allocating any new objects.

Beware of iterators

I don't know if you are using the libGDX Array and Map classes, but you should consider doing so if not. forEach by itself is not a problem, but if you are using the regular Java collection classes, iterating will lead to lots and lots of allocations. The libGDX collections are designed to avoid these allocations when iterating.

Separation of concerns

Currently your render method does two main things. It determines whether an island is in view, and then it renders it. However, it does both of those things for each island at the same time. I think that it would be better to separate the sorting from the rendering. First you would call a sortIslands() method that would put all the islands that need to be drawn into an array. Then the render method would just iterate over the array and draw the necessary islands. Later if you decide to change the way the islands are sorted, it will be much easier. It will also be more clear and more self documenting to do the sorting outside of the render method.

Is it worth it?

Sorting through the tiles to decide what needs to be rendered and what doesn't will (in my experience) end up using more resources than it is worth. I recommend profiling with and without culling in order to see if it provides any performance gains for your game.


Proper Stack Implementation I want to know if this stack is implemented properly. Previously I was told a Linked List contains an inner class that represents a node and the outta class that represents the structure. Does that same idea apply to stacks and linked list? I am assuming not because they are so simple. Here is my implementation. Let me know if this is correct.

public class Stack{
  private int size;
  private int[] stack;

  public Stack(int height){
    stack = new int[height];
    size = 0;
  }

  public void push(int value){
    if(size == 0){

      stack[size] = value;
      size++;

    }

    else if(size == stack.length){
      System.out.println("The stack is full. The value cannot be added to the stack");
    }

    else {
    stack[size] = value;
    size++;
    }

  }

  public void pop(){

    if(size == 0){
      System.out.println("The stack is empty. Nothing can be removed from the stack");
    }
    else {
      size--;
      System.out.println(stack[size] + " was removed from the stack");

    }

  }

  public void peek(){

    if(size == 0){
      System.out.println("There is nothing to peek. The stack is empty");

    }
    else
      System.out.println(size);
      System.out.println("The value: " + stack[size -1]+ " is currently at the top of the stack");

  }

  public void isEmpty(){

    if(size == 0)
      System.out.println("The stack is empty");
    else
      System.out.println("The stack is not empty");

  }
}

Stack is a black hole from other code

The first impression I get is you can't get anything back once you put it in the Stack.

peek and pop both return void, so once you've put your value in with push that's the end of it.  I know your methods output to the console, but that's pretty useless to the rest of your program.

When you're writing things like data structures try to put the user interaction outside of the class you're modelling.  Both for input and output.  This will encourage you to define methods that actually allow you to use the data structure in the way that you would in a normal program.  With that in mind, peek at least should return a value.  isEmpty should return a bool.  Depending on your approach, pop could also return the value popped, or it could simply remove the item (however if you go down this approach I would expect it to throw a exception if you pop from an empty stack).

I would also be expecting push to throw an exception if the stack is full.  At the moment, the client has no idea that their value hasn't been added to the stack.


Service calculator! (Tax and tip) Here is a simple calculator for calculating the tax and tip of a transaction. I have tried to add input verification. I am wondering if there are more elegant ways of doing some simple tasks that are required for this calculator, such as getting the total. Are my naming conventions correct, function use correct, variable (tax) convention correct (should it be static somewhere else?), and clearing/updating of fields as input is being entered into the form. Thank you!!

''' <summary>
''' Author:    Brandon Finley
''' Date:      10/5/2017
''' Purpose:   Calculate the tax and tip of a sale.
''' Status:    Works as intended. I tried to be as clear and consistant as possible.
''' </summary>

Public Class MainForum
    Dim tax As Double = 0.08

    ' Validates and updates tax and total boxes in dollar value.
    Private Sub CalcButton_Click(sender As Object, e As EventArgs) Handles CalcButton.Click
        If PriceBox.Text.Trim = "" Then
            ' If the PriceBox is empty, a value that isn't a number, or negative move cursor to PriceBox and give user reason.
            MsgBox("Enter a price.",
            MsgBoxStyle.Information, "Missing Information")
            PriceBox.Focus()
        ElseIf IsNumeric(PriceBox.Text) = False Then
            MsgBox("Must enter a number.",
            MsgBoxStyle.Information, "Verify")
            PriceBox.Focus()
        ElseIf PriceBox.Text < 0 Then
            MsgBox("Please enter a positive value into the Price field.",
            MsgBoxStyle.Information, "Verify")
            PriceBox.Focus()
        Else
            TaxBox.Text = Format(GetTax(), "currency")
            TotalBox.Text = Format(CalcTotal(), "currency")
        End If
    End Sub

    ' Returns the tax based on the tax multiplied by the price.
    Function GetTax() As Double
        Return PriceBox.Text * tax
    End Function

Private Function GetTip(PercentTip As GroupBox) As Double
    For Each ctrl As RadioButton In PercentTip.Controls
        If ctrl.Checked Then Return PriceBox.Text * (Microsoft.VisualBasic.Left(ctrl.Text, 2) / 100)
    Next
End Function

        ' Returns the total sales formatted to look like currency.
Private Function GetTotal(IsTipPercent As Boolean) As String
    ' Radio buttons do not return total until CalcButton has been pressed.
    If IsTipPercent = False Or (IsTipPercent = True And TotalBox.Text <> "") Then
        Return Format(CalcTotal(), "currency")
    End If
End Function

' Returns the total of price + tax + tip.
Private Function CalcTotal() As Double
    Return PriceBox.Text + GetTax() + GetTip()
End Function

' Updates price if tip amount changes (only after calculating total first).
' Updates price if tip amount changes (only after calculating total first).
Private Sub Tip15_CheckedChanged(sender As Object, e As EventArgs) Handles Tip15.CheckedChanged, Tip20.CheckedChanged, Tip25.CheckedChanged
    TotalBox.Text = GetTotal(True)
End Sub

    ' Clears the tax and tip fields if original price has been changed. bump
    Private Sub PriceBox_TextChanged(sender As Object, e As EventArgs) Handles PriceBox.TextChanged
        If TotalBox.Text <> "" And PriceBox.Text.Trim <> "" Then
            TaxBox.Text = ""
            TotalBox.Text = ""
        End If
    End Sub

    ' Exits application.
    Private Sub ExitButton_Click(sender As Object, e As EventArgs) Handles ExitButton.Click
        Me.Close()
    End Sub

End Class

My only concern would be the hard coding of 15, 20, and 25, and the redundant code used to handle these 3 values:

Private Function GetTip() As Double
    If (Tip15.Checked) Then
        Return PriceBox.Text * 0.15
    ElseIf (Tip20.Checked) Then
        Return PriceBox.Text * 0.2
    ElseIf (Tip25.Checked) Then
        Return PriceBox.Text * 0.25
    End If
End Function

[...]

Private Sub Tip15_CheckedChanged(sender As Object, e As EventArgs) Handles Tip15.CheckedChanged
    If TotalBox.Text <> "" Then
        TotalBox.Text = Format(CalcTotal(), "currency")
    End If
End Sub
Private Sub Tip20_CheckedChanged(sender As Object, e As EventArgs) Handles Tip20.CheckedChanged
    If TotalBox.Text <> "" Then
        TotalBox.Text = Format(CalcTotal(), "currency")
    End If
End Sub
Private Sub Tip25_CheckedChanged(sender As Object, e As EventArgs) Handles Tip25.CheckedChanged
    If TotalBox.Text <> "" Then
        TotalBox.Text = Format(CalcTotal(), "currency")
    End If
End Sub

You might want to add more values later (18% seems to be a new popular tip value for some reason) or edit the existing values, so hardcoding isn't a great idea, design/style wise.


Clearing the screen using FillConsoleOutputAttribute() I'm currently clearing my console window with this piece of code:

void clrScr()
{
    COORD cMap =
    {
        0, 3
    };
    if(!FillConsoleOutputAttribute(hCon, 0, 2030, cMap, &count))
    {
        std::cout << "Error clearing the console screen." << std::endl;
        std::cout << "Error code: " << GetLastError() << std::endl;
        std::cin.get();
    }
}

, which I call once in the main loop.
But since my window is quite large (70x35), it's flickering quite a bit.

I was wondering if there are any faster methods of doing this?

Here is an idea; ANSI escape codes work on Windows, Linux and OSX:

cout << "\033c";

\033 - stands for octal ESC,
c    - resets the device (terminal is default) to the initial state ( clear the screen,clear the buffer also so it is not possible to scroll, reset the fonts and so on).

In some cases it may be more useful than well known which simply adds some empty lines and puts the cursor in the upper left corner:

cout << "\033[2J\033[1;1H"


Custom post carousel I created a post carousel (it's being used on a WordPress theme initially), but I've adapted it to just HTML/CSS/JS one.

CodePen

var $carousel = $('.post_excerpt_carousel');

$carousel.each(function(){

    var $this = $(this);

    var outer_width = $(window).outerWidth();
    var container_width;

    if (outer_width > 1145) {
        container_width = 1170;
    }

    if (outer_width > 960 && outer_width < 1145) {
        container_width = 960;
    }

    if (outer_width < 960) {
        container_width = parseInt((9/10)*outer_width,10);
    }

    var duration = $this.data('duration');
    var li_number = $this.find('li').length;
    var $ul = $this.find('ul');
    var $li = $ul.find('li');
    $li.removeClass('active');

    if (outer_width < 760){
        $li.css('width', container_width);
        $li.eq(1).addClass('active');
        if ($li.eq(2).hasClass('active')) {
            $li.eq(2).removeClass('active');
        }
    } else if (outer_width > 760){
        $li.eq(1).addClass('active');
        $li.eq(2).addClass('active');
    }

    var list_width = $this.find('li').outerWidth(true);
    var left_offset;

    left_offset = parseInt(list_width - (outer_width - container_width-42)/2, 10);

    $ul.css({'display': 'inline-block', 'width': li_number * $this.find('li').outerWidth(true) + 'px', 'left': -left_offset + 'px'});

    var not_active_no = $this.find('li').not('.first').not('.last').not('.active').length;
    var not_active_width = not_active_no * $this.find('li').outerWidth(true);

    $this.on('click', '.carousel_next', function (e) {
        e.preventDefault();

        if($this.find('li.last').prev().hasClass('active')){
            return;
        } else {
            var $a = $('.active', $this);

            if (!$a.next().hasClass('last') && !$ul.is(':animated')) {
                $a.removeClass('active').next().addClass('active');
            }

            if (!$ul.is(':animated')) {
                $ul.animate({
                    left: parseInt($ul.css('left'), 10) - $ul.find('li').outerWidth(true),
                }, duration);
            }
        }

    });

    $this.on('click', '.carousel_prev', function (e) {
        e.preventDefault();

        if($this.find('li.first').next().hasClass('active')){
            return;
        } else {
            var $a = $('.active', $this);

            if (!$a.prev().hasClass('first') && !$ul.is(':animated')) {
                $a.removeClass('active').prev().addClass('active');
            }

            if (!$ul.is(':animated')) {
                $ul.animate({
                    left: parseInt($ul.css('left'), 10) + $ul.find('li').outerWidth(true),
                }, duration);
            }
        }

    });

});

Keep your code styling uniform everywhere

Line 3 with your function declaration after $carousel.each( doesn't follow the same styling as used later on in line 50 when declaring the callback function for the click event listener here $this.on('click', '.carousel_prev', function (e) {.
Your if's/else if's/else's don't use the same format. You go from if () { to if (){.

Simplify your if statements
You should change:

if (outer_width > 1145) {
    container_width = 1170;
}
if (outer_width > 960 && outer_width < 1145) {
    container_width = 960;
}
if (outer_width < 960) {
    container_width = parseInt((9/10)*outer_width,10);
}

to use a combination of if(), else if(), and else():

if (outer_width > 1145) {
    container_width = 1170;
} else if (outer_width > 960) {
    container_width = 960;
} else {
    container_width = parseInt((9/10)*outer_width,10);
}

Another example of this is here:

if (outer_width < 760){
    $li.css('width', container_width);
    $li.eq(1).addClass('active');
    if ($li.eq(2).hasClass('active')) {
        $li.eq(2).removeClass('active');
    }
} else if (outer_width > 760){
    $li.eq(1).addClass('active');
    $li.eq(2).addClass('active');
}

It can easily be written as:

if (outer_width < 760) {
    $li.css('width', container_width);
    $li.eq(1).addClass('active');
    if ($li.eq(2).hasClass('active')) {
        $li.eq(2).removeClass('active');
    }
} else {
    $li.eq(1).addClass('active');
    $li.eq(2).addClass('active');
}

This also fixes a problem with a dead spot that happens if outer_width is equal to 760. Unless this is unwanted behavior, which I see no reason why it should be, then the second option is the better way to go. This is an implementation of the TF-Statement or binary condition, where if it's not one, it always has to be the other. Sorry, I've been in math theory recently. :-)

Use simpler and more concise code
Where you define var not_active_no = $this.find('li').not('.first').not('.last').not('.active').length;, you could have easily written a single cleaner and concise $.not(); like so: var not_active_no = $this.find('li').not('.first, .last, .active').length; or you could've just settled with a single CSS call like so: var not_active_no = $this.find('li:not(.first, .last, .active)').length;.


Remove non-printable characters from string in Python 3 My aim is to print bytes string lines in Python 3, but truncate each line to a maximum width so that it fits the current terminal window.

My first attempt was only print(output[:max_width]), but this did not work because Python counts e.g. tabs \t as one character, but the terminal displays them as multiple characters. Also, it would evaluate carriage-returns etc.

Therefore now I have this snippet of code, line is a bytes string:

output = line.decode(codec, "replace")
if max_width:
    output = "".join(c for c in output if c.isprintable())
    print(output[:max_width])
else:
    print(output)

However, I guess it's pretty slow to refactor each string line this way just to filter out non-printable characters like \t and \r (and whatever characters I might have forgotten).

Please note that codec is specified by the user. It might be "ascii", utf-8, utf-16 or any other valid built-in codec.

Can you please suggest me how to improve this code's performance?

Something that may help performance wise could be itertools.islice.
This will allow you to call str.isprintable() max_width amount of times,
as this is a binary file that may not have many \ns it can save a lot of effort.

output = line.decode(codec, "replace")
if max_width:
    print("".join(itertools.islice((c for c in output if c.isprintable()), max_width)))
else:
    print(output)

This on it's own may not help on files that have a lot of \ns.
The bottle neck in these file would most likely be the overhead incurred by print.
And so it's much faster to build a string to display once.
In these cases you would want to use something like:
(Untested code)

def read_data(path):
    with open(path) as f:
        for line in f:
            output = line.decode(codec, "replace")
            if max_width:
                yield "".join(itertools.islice(
                    (c for c in output if c.isprintable()),
                    max_width))
            else:
                yield output

print('\n'.join(read_data(...)))

However the above is not good on machines with limited memory or extremely large files.
In these cases you would want to use a buffer and print the buffer when a threshold has been reached.

After PEP 3138 your method to remove non-printables seems to be the correct way.


Excel VBA: Set the background-color & border for a user-defined range of rows On my work I've got to maintain an Excel-sheet. Documenting the incident-tickets I've dealt with.

For to be able to distinguish between single days easily I mark every second day with a light cyan-background. So the sheet gets a Zebra-stripe look.

Because I've got to do this every second day (by marking the concerning row-range with the mouse) I've made myself a VBA procedure.

It is started from the Excel-GUI.

An input box appears in which I enter the start- and the end-row. Separated by a colon.
The VBA then marks the range between start inclusive and end inclusive. Moreover it adds a bottom-border to the last row.

Here the VBA-code:

' Sets the background of a user-given range to
'  a defined color.
' Sets the bottom-border of the last row within
'  the range.

Public Sub SetBackgroundColorAndBorder()
    Dim StartEnd As Variant ' Becomes an array with the startRow as first element and the lastRow as second element.
    Dim VerticalIndex As Integer
    Dim HorizontalIndex As Integer

    Const COLOR_INDEX As Integer = 34

    On Error GoTo ErrorHandler

    ' Process the user-given instruction.
    StartEnd = Split( _
                    Trim( _
                        InputBox("Please use a colon for separation." _
                            & vbCrLf & "E.g.: 21:28", _
                            "Please insert start- and endrow!")), _
                     ":")

    ' Set the background-color.
    For VerticalIndex = StartEnd(0) To StartEnd(1)
        For HorizontalIndex = 2 To 6
            Cells(VerticalIndex, HorizontalIndex).Interior.ColorIndex = COLOR_INDEX
        Next
    Next

    ' Set the bottom-border of the last row within the given range.
    For HorizontalIndex = 1 To 6
        Cells(StartEnd(1), HorizontalIndex).Borders(xlEdgeBottom).LineStyle = xlDouble
    Next

    Exit Sub

ErrorHandler:
    MsgBox "Please restart the macro." & vbCrLf _
        & "Please take care to give a correct instruction.", _
        vbExclamation, "AN ERROR HAS OCCURED."
End Sub

It works fine. But I would like to know:

What do you think?

Could it be improved concerning stability and efficiency?

What do think about asking the range from the user via input box?

Could one accomplish the same task in an easier perhaps more robust way?

General hints and comments concerning my coding welcomed too.

Quick notes -

The inputbox can restrict the input by the type argument.

You can also just format it as a table and then unlist the table to keep it as a range -

Sub example()
    Dim targetTable As ListObject
    Dim targetRange As Range
    Set targetRange = Application.InputBox("Select Range", Type:=8)
    Set targetTable = ActiveSheet.ListObjects.Add(xlSrcRange, targetRange)
    targetTable.TableStyle = "TableStyleMedium16"
    targetTable.Unlist
End Sub

Now just make your own custom table style and you're good to go.

Also, integers are obsolete. According to msdn VBA silently converts all integers to long.


A pointer vector sorted by its member function I'm asking for suggestions on a random accessed vector with allocated elements sorted by its key obtained from its member function.

I use it with Qt tree view where the access, add and deletion of tree items is implemented via its index or row number. And I want all the tree items are automatically sorted under a tree node.

If I use std::set, I need to calculate the index first (such as using distance function) when adding an item under a tree node and need a binary search when deleting and accessing an item.

Sorted pointer vectors can access items quickly but require moving pointers when adding and deleting an item. Compare all the advantages and disadvantages, I think this can give better performance

Am I right?  Does anyone have experience on it or better suggestions?

#pragma once

#include <vector>
#include <memory>
#include <algorithm>

//! A random accessed vector with allocated elements sorted by
//! its key obtained from its member function.
//! - Duplicate elements are not allowed.
template<class T, class K, K (T::*MemFun)() const>
class SortedPtrVector
{
public:
    SortedPtrVector()   {}

    //! Add an element, return its index.
    bool Add(std::unique_ptr<T> element, int& index)
    {
        if (!Find((element.get()->*MemFun)(), index))
        {
            m_vector.insert(m_vector.begin() + index, std::move(element));
            return true;
        }
        return false;
    }

    //! Find the element with a key.
    //! - Return true when found and record its index;
    //! - Return false when not found and record its lower bound.
    bool Find(const K& key, int& index) const
    {
        if (m_vector.empty())
        {
            index = 0;
            return false;
        }
        index = LowerBound(key);
        if (index >= m_vector.size())
            return false;
        if ((m_vector[index].get()->*MemFun)() == key)
            return true;
        return false;
    }

    //! Return an index to the first element which does not compare less than the key.
    int LowerBound(const K& key) const
    {
        int first = 0;
        int count = m_vector.size();
        while (count > 0)
        {
            int i = first;
            int step = count / 2;
            i += step;
            if ((m_vector[i].get()->*MemFun)() < key)
            {
                first = ++i;
                count -= step + 1;
            }
            else
            {
                count = step;
            }
        }
        return first;
    }

    //! Delete an element at an index.
    void DeleteAt(int index)
    {
        m_vector.erase(m_vector.begin() + index);
    }

    //! Delete an element with a key.
    void Delete(const K& key)
    {
        int index;
        if (Find(key, index))
            DeletaAt(index);
    }

    //! Clear.
    void Clear()
    {
        m_vector.clear();
    }

    unsigned Size()                                 {return m_vector.size();}
    const T& operator [](unsigned i) const          {return *m_vector[i];}

private:
    std::vector<std::unique_ptr<T>> m_vector;
};

There's probably much that can be done here, but I'll point out some things I've noticed:

As @David Harkness has mentioned in the comments, the method-naming looks like C# naming (uppercase, or PascalCase).  While this isn't necessarily a bad thing as C++ is more flexible with naming, one problem here is that your types (particularly the class) and functions use the same naming convention, which may cause confusion for others.

I would use separate naming for both of them, such as by changing the functions to camelCase or snake_case, while keeping the types as is.
Since you're not overloading the default constructor, you don't have to include it.  The compiler will make a default one for you.

However, with C++11, you can now use default constructors:

SortedPtrVector() = default;

It looks like Add() is doing too many things:

the actual adding
updating a reference
returning a bool

Based on the name, it should just attempt to add something.  But if it's still necessary to update the index, then it should just return that.  You could instead return an std::pair<iterator, bool> as done with std::map::insert, but that may not be what you want here.
Find() shouldn't set index to 0 if the vector is empty.  An index of 0 is a valid position for a non-empty container (the first position).  Instead, it could return -1, which is an out-of-bounds value commonly used with not found (this is also the same value as the constant std::string::npos).

It may also be better to have Find() return an index instead of a bool.  Assuming this is to somewhat resemble std::map::find, it can return a past-the-end value (an iterator) if the key isn't found.
DeleteAt() may do something problematic if the argument isn't checked prior to calling erase() (an invalid index can cause erase() to attempt access to some other location, leading to undefined behavior).
Size() should be const as it's not modifying any data members.  You could also make it noexcept (also C++11) since it's guaranteed not to throw.

Also, unsigned is not quite the specific return type of std::size().  It should either return an std::size_t (usually assumed), std::vector<std::unique_ptr<T>>::size_type (very lengthy), or even better and shorter with C++14 (if your compiler also supports it): the type deduced by auto.

auto Size() const noexcept { return m_vector.size(); }


Simple generic multithreaded queue This is a simple thread safe queue that is used in a producer-consumer model.

public class ThreadedQueue<TType>
{
    private Queue<TType> _queue;
    private object _queueLock;

    protected ThreadedQueue()
    {
        _queue = new Queue<TType>();
        _queueLock = new object();
    }

    public void Enqueue(TType data)
    {
        lock (_queueLock)
        {
            // do not allow duplicates
            if (!_queue.Contains(data))
            {
                _queue.Enqueue(data);
            }

        }
    }

    public bool TryDequeue(out TType data)
    {
        data = default(TType);
        bool success = false;
        lock (_queueLock)
        {
            if (_queue.Count > 0)
            {
                data = _queue.Dequeue();
                success = true;
            }
        }
        return success;
    }
}

Is this complete and thread safe?

This looks mostly fine by me (ignoring Reflection and other more obscure sources of thread safety failures), but I wouldn't call it a Queue per say (because of the duplicate check there). Does it really matter what order the set of items in this collection are in (if so then the duplicate check introduces a bug in that ordering, if not then why not call this a Set)?
One change you should really make is adding readonly to your private members.
Also, while this looks thread safe, there is no reason to assume that it is safe to use any of the items inside of it or any inherited versions of it. Consider code like this (intended to obviously be unsafe):
public class NotThreaSafeExampleClass {
    public Action<string> SayHello;
}

//thread 1: (q = shared ThreadedQueue<NotThreaSafeExampleClass>)
var mean = new NotThreaSafeExampleClass();
q.Enqueue(mean);
while (true) {
    mean.SayHello = name => { Console.WriteLine("Hi "+name); };
    Thread.Sleep(10);
    mean.SayHello = null;
    Thread.Sleep(10);
}

//thread 2:
NotThreaSafeExampleClass otherThreadMethod;
q.TryDequeue(out otherThreadMethod);
while (true) { otherThreadMethod.SayHello('odyodyodys'); Thread.Sleep(15);}

Lock based concurrency in .NET is a fairly simple paradigm (compared to the other ways of ensuring thread safety in .NET). When you lock, you declare a block of code as a critical section, protected by that lock object. .NET ensures that a second piece of code in the same app domain will never be running in a critical section protected by the same lock object while one instance is. Here is a good resource for more threading information: http://www.albahari.com/threading/part2.aspx
Pedantry
When most people discuss thread safety they mean it in the same sense as saying strings are immutable. That is to say, as long as you don't do anything I shouldn't reasonably expect you might do it is safe.
Unfortunately, locks aren't good enough to actually provide true thread safety (bulletproof against reflection and unsafe C# pointer code), they are good enough for most usages where you control the software stack out to the actual application. If you need to ensure thread safety in these instances you must ensure thread safety at the field level in your classes (stuff like the volatile keyword will help you here, but watch out).
Some example threads that would break your safety here:
//as an additional thread running in the app, this would effectively make the locks no longer exclusive
var l = q.GetType().GetField("_queueLock", BindingFlags.NonPublic|BindingFlags.Instance);
while (true) { l.SetValue(q,new object()); }
//making the lock object readonly would protect against this

//ex 2:
var qf = q.GetType().GetField("_queue", BindingFlags.NonPublic|BindingFlags.Instance);
var field = (Queue<TType>)qf.GetValue(q);
field.Clear(); //what if this will happen between the Count check and the Dequeue statement
//readonly will not protect against this

While these examples are pretty far out there, consider if your Queue class were complex enough that I might want to add an extension method to it and need access to internal variables in the method to do some algorithm in a way much better than possible with the available public interface.


Optimizing javascript/jquery code for HTML creation with JSON I use the following code to populate my HTML with JSON. The problem is that I'm afraid this code will be inefficient if the number of records that I get from the service call are large (200 or more).

Are there ways I can change my code to make sure this won't be an issue?  Or is my code fine as it stands?

don't even really need jQuery for this task

here's one way to rework this script:

example jsfiddle

var domStructure = "";
    jsonObj = {
    "products": [
    {
        "ImageName": "product1.png",
        "Description": "Yummy choco"
    },
    {
        "ImageName": "product2.png",
        "Description": "candy villa"
    },
    {
        "ImageName": "product3.png",
        "Description": "vanilla icecream"
    }],
};

function getTableRow(img, desc) {
    return '\

\
            \
                \
            	\
            ' + desc + '	\

';
}

for(var key in jsonObj.products) {
    domStructure += getTableRow(jsonObj.products[key].ImageName, jsonObj.products[key].Description);
}

document.getElementById('tableProducts').getElementsByTagName('tbody')[0].innerHTML = domStructure;

or if you keep jQuery method to set the table row markup use $('#tableProducts tbody') instead of $('#tableProducts').find('tbody'), no need to have jQuery fetch the first element then find the second vs letting it works it magic on one selector. it's about the same speed on the various selectors according to this jsperf.

Tests the following html setters:

$('#tableProducts tbody').html(domStructure);
$('#tableProducts').find('tbody').html(domStructure);
$('#tableProducts').children('tbody').html(domStructure);
document.getElementById('tableProducts').getElementsByTagName('tbody')[0].innerHTML = domStructure;

in the results the pure JS method jumps ahead on Firefox 9.0.1 and Safari 5.1.2


Using built-in type Decimal or Entity type Advance, to hold AdvanceAmount In a wage processing system which employees MVP pattern We have Model classes WageInfo,  EarningInfo, DeductionInfo and AdvanceInfo and entity class Advance. (With several other classes)

EarningInfo and DeductionInfo has a composition relationship with WageInfo (contained in WageInfo) and also since Advance amount is a part of wages (paid in advance)  I've modeled the relationship of Advance with DeductionInfo as aggregation because finally it becomes a deduction to the wages (advance amount should be deducted from wages).

public class DeductionInfo
{
    public int EmployeeID { get; set; }
    public string Name { get; set; }
    public decimal LoanInstalmentAmount { get; set; }
    public decimal UniformInstalmentAmount { get; set; }
    public decimal InsuranceInstalmentAmount { get; set; }
    public decimal AdvanceAmount { get { return Advance.Amount; } set { Advance.Amount = value; } }

    public Advance Advance = new Advance();
}

 public class Advance
{
    public int AdvanceID { get; set; }
    public decimal Amount { get; set; }
    public DateTime EnteredDate { get; set; }
}

So when I create a DeductionInfo object I simply assign a value to AdvanceAmount property and I'm not initializing other properties of Advance class in this instance.

My question are...

Is the way I've modeled these class relationships logical and correct?
Whether I've properly implemented aggregation in DeductionInfo?
Since the advance amount is just a amount is it worth to use decimal type instead?

This question is very, very similar to another question you recently asked, so this answer will basically (hopefully!) clarify my other answer.
KISS: Keep It Simple, Stupid.
These classes represent database records. With Entity Framework you would define the relationships like this (I'd drop the Info suffix):
public class Deduction
{
    // primary key:
    public int Id { get; set; }

    public string Name { get; set; }
    public decimal LoanInstalmentAmount { get; set; }
    public decimal UniformInstalmentAmount { get; set; }
    public decimal InsuranceInstalmentAmount { get; set; }

    // foreign keys:
    public int AdvanceId { get; set; }
    public int EmployeeId { get; set; }

    // navigation properties:
    public virtual Advance Advance { get; set; }
    public virtual Employee Employee { get; set; }
}

public class Advance
{
    // primary key:
    public int Id { get; set; }

    public decimal Amount { get; set; }
    public DateTime EnteredDate { get; set; }

    // navigation property:
    public virtual ICollection<Deduction> Deductions { get; set; }
}

EF works with conventions - it determines that a property called Id is mapped to your  primary key, and that a property called [AnotherEntityTypeName]Id is a foreign key property when there's a virtual navigation property of that other entity type - just with the types involved and the names of the properties, EF is "smart" enough to determine that there's a one-to-many relationship between Deduction and Advance. Now as @Malachi pointed out, the question is is this really what you want?
Only you can answer that. We review code, not database schemas.

Per your other question I see that you're not using Entity Framework. I don't see how that would impact the design of your POCO classes though; at the end of the day they're just classes that convey data between the database and your app, and if you're using ADO.NET then you have full control over how the classes get instantiated and loaded with data, so the classes you're using can be anything your sweet heart desires. Ok anything your business logic needs.

For example, the below code returns an IEnumerable<Deduction>, where Deduction is defined as above, eager-loading the related Advance when it exists:
public IEnumerable<Deduction> GetAllDeductions()
{
    var sql = "SELECT * FROM dbo.Deductions d LEFT JOIN dbo.Advance a ON d.AdvanceId = a.Id"
    using (var connection = new SqlConnection(_connectionString)
    {
        connection.Open();
        using (var command = new SqlCommand(sql, connection))
        using (var reader = command.ExecuteReader())
        {
            while (reader.Read())
            {
                var deduction = new Deduction();
                Advance advance;

                // assuming column names... this is just an example
                if (reader["EnteredDate"] != null)
                {
                    advance = new Advance()
                    advance.Id = reader["AdvanceId"];
                    advance.Amount = reader["Amount"];
                    advance.EnteredDate = reader["EnteredDate"];
                }

                deduction.Id = reader["Id"];
                deduction.Name = reader["Name"];
                deduction.LoanInstalmentAmount = reader["LoanInstalmentAmount"];
                deduction.UniformInstalmentAmount = reader["UniformInstalmentAmount"];
                deduction.InsuranceInstalmentAmount = reader["InsuranceInstalmentAmount"];

                deduction.Advance = advance;

                yield return deduction;
            }
        }
    }
}

This boilerplate-level gruntwork could all be automated with EF and look like this instead:
public IEnumerable<Deduction> GetAllDeductions()
{
    using (var context = new MyDbContext(_connectionString))
    {
        return context.Deductions.Select(e => e).Include(e => e.Advance).ToList();
    }
}

ViewModels
What you seem to want to display is different. It's a ViewModel, something that does not represent database records, but something meant to be displayed.
Then you create another class to represent just that. It's possible that such a class looks very similar to the entity types; it's also possible that it contains properties from two or more entity types:
public class DeductionInfo // or DeductionViewModel
{
    public int Id { get; set; } // only if the View needs it

    public string Name { get; set; }
    public decimal LoanInstalmentAmount { get; set; }
    public decimal UniformInstalmentAmount { get; set; }
    public decimal InsuranceInstalmentAmount { get; set; }
    public decimal AdvanceAmount { get; set; }
}

You'll have a service whose job is to get the data, prepare your ViewModels and spit out these DeductionInfo instances so that your View can display them.
Bottom line: the View doesn't care about the database schema, and doesn't need to know about your entity types. It works with a ViewModel that only contains the data that it needs.
An alternative could be to create a complex type (basically, the DeductionInfo class above), and to have the service fetch the data directly into that type, and return that type for the view to consume: you only select the data you need, and you consume it directly. Works just as well. Which is best? You decide!


Why would I want to always have to explicitly call a "base" method if I just want to use base functionality? I recently worked with an architect who structured his base class like this:

public abstract class Base<T>
{
    public abstract T Get(int id);

    internal T InternalGet(int id, IRepository<T> repository)
    {
       // Do more magic
       return repository.Get(id);
    }
}

And then in the derived class, if you just want to use the base functionality, you'd have to..

public class Derived : Base<T>
{
    private readonly IProductRepository _repository; // Implements IRepository<Product>

    // Ctor with dependency injection, this is good stuff!
    public DerivedClass(IProductRepository repository)
    {
       _repository = repository;
    }

    public override Product Get(int id)
    {
       // ... explicitly call base functionality - NOT a call to base though!
       return InternalGet(id, _repository);
    }
}

Why not just pass the repository to the base class, and override when you need to?

public abstract class Base<T>
{
   private readonly IRepository<T> _repository;

   protected Base<T>(IRepository<T> repository)
   {
      _repository = repository;
   }

   public virtual T Get(int id)
   {
      // Do magic
      return _repository.Get(id);
   }
}

And then in the derived class, if you don't want to use the base method, just override it.

public class Derived: Base<Product>
{
   private readonly IProductRepository _repo;

   public Derived(IProductRepository repo) : base(repo)
   {
      _repo = repo;
   }

   public override Get(int id)
   {
      if(! customerIsBeingNice)
         throw new GoAwayException();
      return _repo.Get(id);
   }
}

I think the architect was trying to avoid the call super code smell, but since we are not requiring a call to super - in fact it really should never happen, because if you override the method, you are supposed to implement the flow you want, whereas if you call super, you can't hook into the flow that easily.

Which of these is better/worse, and why? Any alternatives?

The problem with building a good base class is that ideally, you should know exactly what kind of functionality will the derived classes need from the base class.

If you think they'll need more than is actually necessary, you'll write more code unnecessarily and it will also make the base class harder to use. On the other hand, if you think the derived classes will need less, your base class will be hard (or impossible) to use for those advanced scenarios.

For example, in your specific case, imagine you would need to call the base Get() with different IRepositorys. With your implementation, that's hard to do (though not impossible, you use a hack like IRepository wrapper where the wrapped IRepository can be switched, but ugh). With the architect's implementation, that's a trivial thing to do: just pass different IRepository to each call of InternalGet().

But again, choosing between the two depends on expectations of what the derived classes will need. If a scenario like I just described is unlikely, then your implementation is better, because it's simpler. But if that scenario happens, your implementation is clearly insufficient and the one from the architect is clearly better.


Fixed-size array-based implementation of a queue For my computer science class, I've implemented a Java version of a queue and would like to know if there's anything wrong with it. After my tests, it seems to work fine, but maybe somebody can point out what can be improved or if something is wrong.

public class Queue<E> implements IQueue<E> {

private E[] internalQueue;
private int currIndex;

public Queue(int size) {
    internalQueue = (E[]) new Object[size];
    currIndex = 0;
}

@Override
public void push(E input) {
    internalQueue[currIndex] = input;
    currIndex++;
}

public E[] getQueueDebug() {
    return this.internalQueue;
}

@Override
public E pop() {
    if (currIndex != 0) {
        E toReturn = internalQueue[0];

        for (int i = 1; i <= internalQueue.length - 1; i++) {
            internalQueue[i - 1] = internalQueue[i];
            internalQueue[i] = null;
        }

        currIndex--;
        return toReturn;
    }

    return null;
}

@Override
public E top() {
    if (currIndex != 0)
        return internalQueue[0];

    return null;
}

@Override
public boolean empty() {
    return (currIndex == 0);
}

}

API

The method names you chose are unusual for queues. It's good to get inspiration from the JDK. Following the example of Queue,
I suggest these renames:

push -> add
pop -> poll
top -> peek
empty -> isEmpty

Protecting internal implementation details

The getQueueDebug should ideally not be there at all,
and definitely not with public visibility.
It's best to test classes through their public interfaces.
Avoid exposing internal details.

Readability

The currIndex != 0 condition is used at multiple places.
This meaning of this condition is "not empty",
and the code would be more readable if you replaced it with a !isEmpty() method call.

Copying arrays

Instead of manually copying contents of an array like this:

for (int i = 1; i <= internalQueue.length - 1; i++) {
    internalQueue[i - 1] = internalQueue[i];
    internalQueue[i] = null;
}

It's more efficient to use System.arraycopy:

System.arraycopy(internalQueue, 1, internalQueue, 0, internalQueue.length - 1);

Performance

Moving the elements of internalQueue on every poll is inefficient.
You could instead use two indexes to point to the first and last element in the queue, and only move elements when really necessary.

In fact, @TedHopp put it beautifully in a comment:

  [...] By thinking of the array as circular (and using two indexes as suggested) it's never necessary to move elements. All it takes is some close attention to indexing to distinguish between an empty and a full queue.

Java conventions

Note that it's not a convention in Java to prefix interface names with a capital I, as you could see already in the Queue interface of the JDK.

You could use Queue as your interface name,
and since the implementation is effectively an array-backed bounded queue,
you could name the implementation class ArrayBackedBoundedQueue, for example.