C++ fixed-size queue full/empty signaling I'm considering an application where a large data needs to be written to file often.  I would like to use a queue and have a producer and consumer running on different threads.  Additionally, I would like to have a fixed size queue as the data size can be very large.  I've implemented a simple test of boost::lockfree::queue with boost::condition_variable to signal the state of the queue.  I would like to avoid the mutex but for the exception where the queue is full (block producer) or empty (block consumer)

I would like to know (at risk of opinion based..) if I'm using conditionals properly or if there is a performance issue - compared to using other methods.  Here is what I've done so far (small data)

#include <iostream>
#include <boost/thread/thread.hpp>
#include <boost/lockfree/queue.hpp>

#define N 6
#define QN 3

struct testdata {
    int a;
    int b;
};

boost::lockfree::queue<testdata, boost::lockfree::fixed_size<true>> que(QN);
boost::condition_variable que_has_data, que_has_room;
boost::mutex que_mtx_hd, que_mtx_hr;

void producer(void) {
    testdata td;
    int i = 0;
    boost::mutex::scoped_lock lock(que_mtx_hr);
    boost::this_thread::sleep(boost::posix_time::seconds(1));
    for (;;) {
        td.a = i; td.b = i + 1;
        if (!que.push(td)) {
            std::cout << "producer waiting" << std::endl;
            que_has_room.wait(lock);
        } else {
            std::cout << "pushed " << td.a << std::endl;
            i += 1;
            que.has_data_notify_one();
        }
        if (i > N)
            break;
    }
}

void consumer(void) {
    testdata td;
    boost::mutex::scoped_lock lock(que_mtx_hd);
    for (;;) {
        if (que.pop(td)) {
            std::cout << "popped " << td.a << std::endl;
            if (td.a == N)
                break;
            que_has_room.notify_one();
        } else {
           std::cout << "consumer waiting" << std::endl;
           que_has_data.wait(lock);
        }
    }
    boost::this_thread::sleep(boost::posix_time::seconds(1));
}

int main(void) {
    boost::thread t1(&producer);
    boost::thread t2(&consumer);
    t1.join();
    t2.join();
    return 0;
}

This works (output):

consumer waiting
pushed 0
pushed 1
pushed 2
producer waiting
popped 0
pushed 3
producer waiting
popped 1
pushed 4
producer waiting
popped 2
pushed 5
producer waiting
popped 3
pushed 6
popped 4
popped 5
popped 6

I anticipate that for the most part, data will almost always be available but I want to block in the case of congestion (file write, network, etc).  The interest in fixed size is the worry of massive data sets and dynamic allocation in the queue -

(This is more an experiment in what can be done.  In reality, my data is updated at most about 20 Hz so just taking a lock on a std::queue that I manage the size of will work very well, too.)

Use a semaphore to cause the producers to sleep when the queue is full, and another semaphore to cause the consumers to sleep when the queue is empty.
when the queue is neither full nor empty, the sem_post and sem_wait operations are nonblocking (in newer kernels)

#include <semaphore.h>

template<typename lock_free_container>
class blocking_lock_free
{
public:
    lock_free_queue_semaphore(size_t n) : container(n)
    {
        sem_init(&pop_semaphore, 0, 0);
        sem_init(&push_semaphore, 0, n);
    }

    ~lock_free_queue_semaphore()
    {
        sem_destroy(&pop_semaphore);
        sem_destroy(&push_semaphore);
    }

    bool push(const lock_free_container::value_type& v)
    {
        sem_wait(&push_semaphore);
        bool ret = container::bounded_push(v);
        ASSERT(ret);
        if (ret)
            sem_post(&pop_semaphore);
        else
            sem_post(&push_semaphore); // shouldn't happen
        return ret;
    }

    bool pop(lock_free_container::value_type& v)
    {
        sem_wait(&pop_semaphore);
        bool ret = container::pop(v);
        ASSERT(ret);
        if (ret)
            sem_post(&push_semaphore);
        else
            sem_post(&pop_semaphore); // shouldn't happen
        return ret;
    }
private:
    lock_free_container container;
    sem_t pop_semaphore;
    sem_t push_semaphore;
};


Populating Day/Month Selections in UserForm Controls So I have a simple userform where I require the User to input the target month, and the day to analyse data up to.

This is just a small section of code to govern populating the month selection and then determining days based on the month.

Particular questions:

Is this a smart way to handle dates generally?
Is this a good way to ensure that the User cannot enter a non-existent date?
Any edge cases I haven't considered?

And, as always, if you were handed this code to maintain, what would you be thinking as you read through it?

Private Sub UserForm_Initialize()

    PopulateMonthBox Me.UF_BankRec_cbx_Month

End Sub

Private Sub PopulateMonthBox(ByRef monthBox As MSForms.ComboBox)

    Dim ix As Long
    Dim monthText As String

        For ix = 1 To 12
            monthText = MonthName(ix)
            monthBox.AddItem monthText
        Next ix

End Sub

Private Sub UF_BankRec_cbx_Month_Change()

    Dim dayBox As MSForms.ComboBox
    Set dayBox = Me.UF_BankRec_cbx_EndDay

    Dim monthBox As MSForms.ComboBox
    Set monthBox = Me.UF_BankRec_cbx_Month

    Dim monthText As String
        monthText = monthBox.Text

        dayBox.Clear
        PopulateDayBox dayBox, monthText

End Sub

Private Sub PopulateDayBox(ByRef dayBox As MSForms.ComboBox, ByVal monthText As String)

    Dim ix As Long, dayLimit As Long
    Dim ixMonth As Long, ixNextMonth As Long
    Dim ixYear As Long
    Dim firstDayNextMonth As Date, lastDayThisMonth As Date

        ixMonth = Month("01/" & monthText & "/2000")
        ixNextMonth = ixMonth + 1
        ixYear = Year(Now)

        firstDayNextMonth = DateSerial(ixYear, ixNextMonth, 1)
        lastDayThisMonth = DateAdd("d", -1, firstDayNextMonth)

        dayLimit = Day(lastDayThisMonth)

        For ix = 1 To dayLimit
            dayBox.AddItem ix
        Next ix

End Sub

Why not doing it more directly like this:

Private Sub UserForm_Initialize()

  Dim i As Byte
  For i = 1 To 12
    UF_BankRec_cbx_Month.AddItem MonthName(i)
  Next

End Sub

Private Sub UF_BankRec_cbx_Month_Change()

  Dim i As Byte, startDate As Long
  UF_BankRec_cbx_EndDay.Clear

  startDate = DateValue("01/" & UF_BankRec_cbx_Month.Text & "/" & Year(Now()))

  While Month(startDate) = Month(startDate + i)
    i = i + 1
    UF_BankRec_cbx_EndDay.AddItem i
  Wend

End Sub

I'd say it is a bit better to read while i normally just speed codes up (so they end up in a way you can't read them anymore)... I tried my best to make it readable even without comments.
However, there may be a reason for you to run extra macros and using objects which i skipped out here... (to insert arrays to CBoxes is also possible solution to avoid objects)

If there any questions or something like that, feel free to ask Or better, tell me what you don't like looking at my code (this way i get the best idea what you desire) :)

At least your lastDayThisMonth = DateAdd("d", -1, firstDayNextMonth) is the same like lastDayThisMonth = firstDayNextMonth - 1 (but i am sure there is also a reason for that)


Printing a multilevel container I have these struct(s) to keep data about temperature measurement:

// data structure representing day
struct Day{
   vector<double>hour;
   Day();
};
Day::Day(): hour(24){
   for(int i=0; i< hour.size();++i) hour[i]=not_a_reading;
}

// data structure representing month
struct Month{
    int month;
    vector<Day>day;
    Month(): month(not_a_month),day(32) { }
};

// data structure representing year
struct Year{
    int year;
    vector<Month>month;
    Year(): month(12) { }
};

// vector holding measured data
vector<Year>yrs;

I am trying to write a program to print all the data inside yrs. Here is what I got so far:

void print_year(ostream os, vector<Year>& v){
for(size_t i=0; i<v.size(); ++i){
    os << "{year " << v[i].year;
    for (size_t j=0; j<v[i].month.size(); ++j){
        os << " {month " << v[i].month[j].month;
        for (size_t k=0; k<v[i].month[j].day.size();++k){
            os << '(' <<v[i].month[j].day <<" "<< v[i].month[j].day[k].hour;
            //--^^---- (No << operator matches these operands) WHY?
            os  << ') ';
            }
        os << "}";
        }
    os << "}";
    }
}

If I have one measurement on: 10th of Jan 1992 at 10 o'clock with temperature 61.6 Fahrenheit and two on: 11th and 22th of Feb 1992 with temperatures: 64 and 65.2. The output (format) will be: (year 1992 { month jan ( 10 61.5)} {month feb (11 64) (22 65.2)})

I'm probably using the vector access operator[] wrong.

I was trying to fix it and got to this:

void print_year(ostream os, vector<Year>& v){
for(size_t i=0; i<v.size(); ++i){
    os << "{year " << v[i].year;
    for (size_t j=0; j<v[i].month.size(); ++j){
        os << " {month " << month_input_tb[v[i].month[j].month];
        for (size_t k=0; k< v[i].month[j].day.size();++k){
            os <<  '('<< k <<" "; // loop counter coincides with day of the month
            for (size_t z=0; z < v[i].month[j].day[k].hour.size(); ++z){
                os << v[i].month[j].day[k].hour[z];
                os  << ')';
        }
        os << "}";
    }
    os << "}";
}

}

Then I saw your (Loki Astari) answer and I am writing separate << operators now.

I would try and simplify that each structure knows how to print itself:

int main()
{
    Year    year;
    // Load data into year.

    std::cout << year;

    // You can print a vector of years with:
    std::vector<Year>   yrs;

    std::copy(std::begin(yrs), std::end(yrs),
              std::ostream_iterator<Year>(std::cout, "\n"));

    // or a Simple loop:
    for(auto const& year: yrs) {
        std::cout << year << "\n";
    }

};

To make this work you need to define an operator<< for Year

struct Year{
    int year;
    vector<Month>month;
    Year(): month(12) { }

    friend std::ostream& operator<<(std::ostream& str, Year const& year)
    {
        str << "{year " << year.year;
        for(auto const& month : year.month) {
            str << month;
        }
        str << "}";
        return str;
    }
};

So the print for Year is defined in terms of printing for a Month object. So you know have to define how to print a Month. Which in turn will print some Day objects. So you will also have to define how to print a Day object.

The problem you are seeing:

os << '(' << v[i].month[j].day <<" "<< v[i].month[j].day[k].hour;
//           ^^^^^^^^^^^^^^^^^
//    This is a vector.
//    The stream class does not know how to stream a vector.
//    Did you mean `v[i].month[j].month`


Typesafe heterogeneous container I was reading Joshua Blochs "Effective Java 2nd edition" item 29, which describes how to create Heterogeneous containers, in the end of chapter he wrote:

  A Class object used in this fashion is called a type token. You can
  also use a custom key type. For example, you could have a DatabaseRow
  type representing a database row (the container), and a generic type
  Column as its key.

It wondered me and I wrote this implementation which he has mentioned. Is there something I could have done better ? I'm mostly interested in Column class which serves as Type Token

class Column<T> {

    @SuppressWarnings("unchecked")
    public T cast(Object obj) {
        return (T) obj;
    }
}

public class DatabaseRow {

    private Map<Column<?>, Object> row = new HashMap<>();

    public <T> void putColumn(Column<T> type, T instance) {
        row.put(type, type.cast(instance));
    }

    public <T> T getColumn(Column<T> type) {
        return type.cast(row.get(type));
    }

    public static void main(String[] args) {
        DatabaseRow db = new DatabaseRow();

        Column<Integer> colInt = new Column<>();
        Column<Double> colDouble = new Column<>();

        db.putColumn(colInt, 1);
        db.putColumn(colDouble, 10.0);

        System.out.println(colInt.getClass() + " " + colDouble.getClass());
        System.out.println(db.getColumn(colInt) + " " + db.getColumn(colDouble));
    }

}

There are two things I would like to point out here.

There is no need to do the cast on the putColumn call. The method signature is: public <T> void putColumn(Column<T> type, T instance) { so we know that instance is of type T already, and it is being stored in an Object value in a Map. There is no reason to force the cast before storing it.
The Column's cast method would be better done using a Class<T> instance. Consider the 'standard' pattern of requiring a class instance in the constructor of the Column. For example:

class Column<T> {

    private final Class<T> valClass;
    public Column(Class<T> valClass) {
        this.valClass = valClass;
    }

    public T cast(Object obj) {
        return obj == null ? null : valClass.cast(obj);
    }
}

this has the benefit of not requiring the SuppressWarnings. It also ends up being helpful for other reasons too.

The class would be used as:

Column<String> col = new Column(String.class);

Apart from that, things will be the same.


Rounds off numbers from 1 - 10, 000 including negatives I was trying to replicate Python's internal round() function.
The program asks for a number, converts it into a float (catches the value error if present) and gets the absolute value of the float (to handle negative numbers).

def rounder():
    number = input("Give me a number to round off!: \n")
    #Makes sure you enter a valid number
    try:
        floater = float(number)
        absol = abs(floater)
    except ValueError:
        print("Please enter a valid number.\n")
        starter()

    if absol < 10 and absol > 0: #Works for both 0.6 and 1.6 | Because 0.6 is too small for 1 and will return the rounding while 1.6 % == 0.6 also
        rounding = absol % 1
        if rounding >= 0.5: #If more than 0.5
            absol += 1 #Increment It
            temp =(int(absol)) #Convert back to integer

            if floater < 0: #Negative check
                print(-temp)
            else:
                print(temp)

        elif rounding < 0.5:
            temp = (int(absol))

            if floater < 0: #Negative check
                print(-temp)
            else:
                print(temp)

    elif absol < 100 and absol > 10: #use %10
        rounding = absol % 10
        rounding2 = rounding % 1
        if rounding2 >= 0.5: #If more than 0.5
            absol += 1 #Increment It
            temp = (int(absol)) #Convert back to integer

        if floater < 0: #Negative check
            print(-temp)
        else:
            print(temp)

    elif rounding2 < 0.5:
        temp = (int(absol))

        if floater < 0: #Negative check
            print(-temp)
        else:
            print(temp)
    rounder()

def starter():
    print("Compatible from 0.1 - 9999.9")
    rounder()

starter()

It keeps on going like that until if absol < 10000 and absol > 1000 . Also the number of modulo operations (%) it goes through increases.

The ranges are hard-coded so it can only handle numbers from 1 - 10 000. I could manually increase the limitation if I wanted but I would have to keep doing that until infinity.

Does anyone have any idea on how to get beyond this limitation?
Can my code be refined? Is there a more efficient way of doing this?

Keep all answers beginner-friendly.

min < value < max

Python makes this kind of conditions really easy:

if absol < 10 and absol > 0:

Like this:

if 0 < absol < 10:

Don't repeat yourself

Instead of this:

    if floater < 0: #Negative check
        print(-temp)
    else:
        print(temp)

    # ...

    if floater < 0: #Negative check
        print(-temp)
    else:
        print(temp)

Create a helper function:

def abs(num):
    if num < 0:
        return -num
    return num


Clojure function greater than 10 SLOC It has been mentioned in the Clojure Style guide to avoid functions longer than 10 SLOC but mine has more than 10 (47 to be exact).

The code is an web api implementation and follows like this:

(defn classify
  "Sends a text to a classifier and returns a classification"
  [keys texts classifier & user-name]
  {:pre [(check-keys? keys)]}
  (let [textbase64-tag (map #(make-xml-node :textBase64
                                            {:id (str "Text" (str (index-of % texts)))}
                                            (String. (encode (.getBytes %)) )) texts)
        texts-tag (make-xml-node :texts {} textbase64-tag)
        user-attribute (if (> (count user-name) 0)
                         { :username (first user-name) })
        classify-tag (if (= (count user-name) 0)
                       (map #(make-xml-node :classify
                                            {:id (str "Classify" %)
                                             :classifierName classifier
                                             :textId (str "Text" %)
                                             })
                            (range (count texts)))
                       (map #(make-xml-node :classify
                                            {:id (str "Classify" %)
                                             :classifierName classifier
                                             :textId (str "Text" %)
                                             :username (first user-name)})
                            (range (count texts))))
        read-calls (make-xml-node :readCalls
                                  {:readApiKey (keys :read-key)}
                                  classify-tag)
        uclassify-response (raw-post-request
                            (xml/emit-str
                             (xml-append-elements uclassify
                                                  (list texts-tag read-calls))))]
    (if (check-response
         (get-response uclassify-response))
      (readable-list
       (list
        (x/xml-> uclassify-response
                 :readCalls :classify
                 :classification (x/attr :textCoverage))
        (map vector
             (x/xml-> uclassify-response
                      :readCalls :classify
                      :classification :class
                      (x/attr :className))
             (x/xml-> uclassify-response
                      :readCalls :classify
                      :classification :class
                      (x/attr :p)))))
      false ;; Will never reach here
      )))

The code initially builds all the associated xml tags within let and after building them sends a request to the server and then checks out the response to see if it succeeded.

Why I didn't handle the response in a separate function is because that it won't be reused again. Is this a good practice, any suggestions ?

There's value in factoring the logic out into one or more new functions, even if those functions are only used in a single place, because it can make the code easier to understand and modify.

In a situation like that (when the logic won't be used elsewhere) I often use letfn. The letfn could be either internal or external to the function itself but I see that as a purely stylistic decision.

Here's the ClojureDocs page on letfn. See Pedestal's dataflow logic for a few examples of letfn in use.

On the other hand, there's really no problem with having a top level function that's only used in one place. I like that letfn implicitly communicates the function's scope of relevance, but I don't think either way is objectively superior to the other.


Count the number of riders in each category I have a code to get riders from certain teams and calculate the number of riders in those teams in that category:
valid_clubs = []
with open("validclubs.txt") as f:
    for line in f:
        valid_clubs.append(line)

disqualification_reasons = ["WKG", "UPG", "ZP", "HR", "HEIGHT", "ZRVG", "new", "AGE", "DQ", "MAP", "20MINS", "5MINS"]

with open("results.csv", 'rt', encoding='UTF-8',errors='ignore') as file:  # open results file to get breakdown of results
    reader = csv.reader(file, skipinitialspace=True, escapechar='\\')
    # each variable is current number of riders in the category
    A = B = C = D = E = Q = W = 0  # setting all values to 0 so we can work out number of riders in each category
    for row in reader:
        try:
            category = row[0]
            if row[4] in valid_clubs:
                if row[7] == "0":  # if club is in valid_clubs.txt and gender = female
                    W = W + 1  # each time this condition is met increase value of W by 1, where W is the amount of women
                if "A" == category:  # if club is in valid_clubs.txt and category is A
                    A = A + 1  # each time this condition is met increase value of A by 1
                if "B" == category:  # if club is in valid_clubs.txt and category is B
                    B = B + 1  # each time this condition is met increase value of B by 1
                if "C" == category:  # if club is in valid_clubs.txt and category is C
                    C = C + 1  # each time this condition is met increase value of C by 1
                if "D" == category:  # if club is in valid_clubs.txt and category is D
                    D = D + 1  # each time this condition is met increase value of D by 1
                if category in disqualification_reasons:  # all the possible DQ reason
                    Q = Q + 1  # each time this condition is met increase value of Q by 1, where Q is the number of riders DQ'ed
                if "E" == category:  # if club is in valid_clubs.txt and category is E
                    E = E + 1  # each time this condition is met increase value of E by 1
        except IndexError:  # ignore errors where row is empty
            pass
        total = str(A + B + C + D + E + Q)
print("There were", total, "riders", W, " were women:", "\nCat A:", A, "\nCat B:", B, "\nCat C:",
      C, "\nCat D:", D, "\nCat E:", E, "\nDisqualified:", Q)

It is a really repetitive code and I'm sure could be cleaned up. I would appreciate some advice on how you would do this.
Here is an example CSV:
Category.Position,Name,Time,Team,RandomInt1,RandomInt2,Male?
A,1,Person 4,00:54:12.92,2281,343,4.4,1
A,2,Person 3,00:54:13.29,10195,310,4.2,1
A,3,Person 2,00:54:19.19,84,334,5.0,1
A,4,Person 1,00:54:19.33,7535,297,4.9,1

Category would change as you go down the CSV
The Male? column is 1 when the person is male and 0 when they are female,
Validclubs is a list of the team numbers which I want to use, imported from a txt file,
RandomInt1/RandomInt2 are irrelevant for now
I would appreciate any advice on reducing the code so it is cleaner/shorter.
I want to use built-in libraries so not pandas.

Set membership
valid_clubs = []
with open("validclubs.txt") as f:
    for line in f:
        valid_clubs.append(line)

should be
with open("validclubs.txt") as f:
    valid_clubs = {line.strip() for line in f}

Lines coming back from a file handle in Python include their line ending, which you have to strip yourself; and you should be using a set, not a list. Set membership lookups will be much faster than list lookups.
Consider DictReader
The csv module has a reader that gives you back a dictionary based on the headings, so that you do not have to write row[0], but rather row['Category'].
Use Counter
Your list of category ifs can be condensed to a one-liner; read about Counter. This will be easier to use and perform better.
Empty rows
Based on # ignore errors where row is empty, this should be done up-front. Rather than hitting your face on an IndexError and having to ignore it, simply check for an empty row:
if row:


Recursive, depth first search I wanted to write a function findDeep that would perform a recursive, depth-first search on plain objects and arrays.

Comments and criticism welcome.

function findDeep(dict, test, index = 0, keys = Object.keys(dict)) {
    var item = dict[keys[index]];

    if(index > keys.length) {
        return null; // End of siblings. No match.
    }

    if(test(item)) {
        return item; // Match.
    }

    if(item !== null && typeof item === 'object') { // null is an object.
      var result = findDeep(item, test); // Children.
      if(result) {
        return result; // Short circuit, when item found.
      }
    }

    return findDeep(dict, test, ++index); // Siblings.
}

document.writeln('test1: ', findDeep(['foo', 'bar', 'bam'], i => i === 'bar') === 'bar', '
')
document.writeln('test2: ', findDeep([ ['foo'], [['bar']], [ ['baz', ['bat'] ] ] ], i => i === 'bat') === 'bat', '
');
document.writeln('test3: ', findDeep({ foo: 'foo', bar: 'bar', bam: { baz: 'baz' } }, i => i === 'baz') === 'baz', '
');

The main issue with your function is that it can't correctly search for null or undefined.

findDeep(['foo', 'bar', 'bam'], i => i === null); // (1)
findDeep(['foo', 'bar', null], i => i === null);  // (2)
findDeep(['foo', , 'bam'], i => i === undefined); // (3)

(1) and (2) both return null and (3) returns undefined even though there is no such value.

I would rather make the function return an object, similar to ES2015
generators.
Separate index argument is excessive if you are
ok with mutating keys.
I would rename test to
pred for
"predicate"
and item to value to take advantage of ES2015's shorthand properties.
let is the new var.

function findDeep(dict, pred, keys = Object.keys(dict)) {
    if (keys.length === 0) {
        return { match: false };
    }

    let value = dict[keys.pop()];

    if (pred(value)) {
        return { match: true, value };
    }

    if (value !== null && typeof item === 'object') {
        let result = findDeep(value, pred);

        if (result.match) {
            return result;
        }
    }

    return findDeep(dict, pred, keys);
}

Last, please keep the coding style consistent: note the space (or no space) after if and the indentation.


Improve nested loop for bio-statistics calculation I am doing a bio-statistics calculation and the following code works. However, can someone help to improve the messy nested loop?

for(int i=0; i<NN; i++) {
    for (int j=0; j<NN; j++) {
        if (i != j){
            thirdlayer = 0;
            for (int k=0; k<NN; k++) {
                fourthlayer = 0;
                for (int l=0; l<NN; l++) {
                    fourthlayer =  fourthlayer + V[j*NN+l]*V[NN+l]*J[k*NN+l];
                }
                thirdlayer = thirdlayer + V[k]*V[i*NN+k]*fourthlayer;
            }
            if(pi_cod[j] != 0)
                Transitions[i*NN +j] =  sqrt(pi_cod[i]*pi_cod[1]/(pi_cod[0]*pi_cod[j]))*Q[i*NN +j]*thirdlayer/Padt;
        }
    }
}

Using small l for the index is bad because it looks like the digit 1.  It's better to use large L.

Instead of j*NN, it's better to use a cached index that increments by for:

int NN2 = NN*NN
for(int iNN=k; iNN < (NN2+k); iNN+=NN) {
  thirdlayer = thirdlayer + V[k]*V[iNN]*fourthlayer;
}

This could be a bit faster.

Another hook - more use pointer as array sintax:
for 3layer better get a row vector in wich 4layer for process:

int* VVj = &(V[j*NN]);
int* VNN = &(V[NN]);
for (int k=0; k<NN; k++) {
                int* JNNk = &(J[k*NN]);
                fourthlayer = 0;
                for (int l=0; l<NN; l++) {
                    fourthlayer =  fourthlayer + VVj[l]*VNN[l]*JNNk[l];
                }
                thirdlayer = thirdlayer + V[k]*V[i*NN+k]*fourthlayer;
            }

good compiler do it for you byself, but in such decomposition may better see data dependents, and it is a bit simpler and short

also you can deploy from 4layer V[j*NN+l]*V[NN+l] into stanalone vector that can be prepared in outer j cycle.

Instead of division (xxx)/Padt (better=faster), use multiplication *(1/Padt), or move out from the last cycle:

double thp = thirdlayer/Padt;
for(int i=0; i<NN; i++) {
    if(i != j && pi_cod[j] != 0)
      Transitions[i*NN +j] =  sqrt(pi_cod[i]*pi_cod[1]/(pi_cod[0]*pi_cod[j]))*Q[i*NN +j]*thp;
  }

Instead of a conditional calculation, it's better to use a conditional assignment since it can better optimized for x86:

double thp = thirdlayer/Padt;
for(int i=0; i<NN; i++) {
    int pcodj = (pi_cod[j] != 0)?pi_cod[j]: 1;
    double transition = sqrt(pi_cod[i]*pi_cod[1]/(pi_cod[0]*pi_cod[j]))*Q[i*NN +j]*thp;
    if(i != j && pi_cod[j] != 0)
      Transitions[i*NN +j] = transition;
  }

If it's rare misses for assignment, so penalty for calculation could be negligible.


Threads and logfiles The c# newbie is back :) I have another problem with my threads. Here is what i am trying to achieve:

I am starting 5 threads which are performing the same task but on different URLs. So i am keeping a "masterLogFile.txt" to keep track of what URLs have already been visited.
Each thread compares its own "thread1LogFile.txt" to the "masterLogFile.txt" before deciding whether to execute the task.

My question is, is there any more efficient way to handle this? Currently each thread runs this piece of code before deciding if the URL is ok or not:

using (FileStream fs = File.Open("masterLogFile.txt", FileMode.Open, FileAccess.Read, FileShare.Read))
{
    byte[] bff = new byte[1024];
    UTF8Encoding temp = new UTF8Encoding(true);

    while (fs.Read(bff,0,bff.Length) > 0)
    {
        if (temp.GetString(bff).Contains(variableWithUrlFrom_thread1LogFile.txt))
        {
            found = true;
        }

    }
    fs.Close();
}

I'd use a dictionary (or a HashSet if you don't need the ID information) for coordination.

class UrlLog {
    private Dictionary<string, int> visited = new Dictionary<string, int>();
    public bool HasUrlBeenVisited(string Url) {
        lock(visited) return visited.ContainsKey(Url);
    }
    public void SetUrlVisited(string Url, int threadId) { /* id not strictly necessary */
        lock(visited) visited[Url] = threadId;
    }
}

Then pass one instance of this UrlLog to your threads, and they can use that.

Also, I'm not sure you even need the locks for the checks, but it's better to be safe than sorry. :)


Secure file upload example The overall question is:  I need feedback on the security of the idea and the script.  Do I need to do more in raw code, or is this ok, taking into account my other information?

Let's assume these following first:

All images are stored on a subdomain (e.g. images.example.com)
The subdomain images.example.com does not have execute rights
The upload script is named upload.php and is located at example.com
I run a virus scanner on images.example.com every day

A simple upload script written in PHP:

$allowedExts = array("gif", "jpeg", "jpg", "png");
$temp = explode(".", $_FILES["file"]["name"]);
$extension = end($temp);
if ((($_FILES["file"]["type"] == "image/gif")
|| ($_FILES["file"]["type"] == "image/jpeg")
|| ($_FILES["file"]["type"] == "image/jpg")
|| ($_FILES["file"]["type"] == "image/pjpeg")
|| ($_FILES["file"]["type"] == "image/x-png")
|| ($_FILES["file"]["type"] == "image/png"))
&& ($_FILES["file"]["size"] < 20000)
&& in_array($extension, $allowedExts))
  {
  if ($_FILES["file"]["error"] > 0)
    {
    echo "Return Code: " . $_FILES["file"]["error"] . "<br>";
    }
  else
    {
    if (file_exists("upload/" . $_FILES["file"]["name"]))
      {
      echo $_FILES["file"]["name"] . " already exists. ";
      }
    else
      {
      move_uploaded_file($_FILES["file"]["tmp_name"],
      "images.example.com/" . randomName() .$_FILES["file"]["name"]);
      }
    }
  }
else
  {
  echo "Invalid file";
  }

The script is demonstrating my thoughts; nothing more.
So from what I understand now, all images uploaded cant do any harm to example.com, even if they contain injected code?
Do I not need to do a exif or Fileinfo check since I put the files on a subdomain?
I've assumed a lot of things.  Did I miss anything?

First thing I would consider is using functions so that it becomes easier readable.

function hasAllowedExtension($filename); // returns bool
function isAllowedType($filetype); // returns bool

That way you can shorten the first lines to

$file = $_FILES["file"];
if (hasAllowedExtension($file["name"]) && isAllowedType($file["type"]) && $file["size"] < 20000) {

which is much more readable.

$allowedExts = array("gif", "jpeg", "jpg", "png");
$temp = explode(".", $_FILES["file"]["name"]);
$extension = end($temp);

There are better and easier ways:

$extension = pathinfo($_FILES["file"]["name"], PATHINFO_EXTENSION);

Also consider making all of this into a function which you only either hand the name of the file $_FILES[$filename] or the file (from $_FILES) directly.

echo "Invalid file";

A die might be appropriate here. die will also print a message but has the added bonus that it immediately stops execution and it is easier to understand that this is really a stopper.

die("Invalid file.");

I couldn't see any gaping holes in it except for only one thing: I can't remember and couldn't find out if $_FILES["file"]["name"] is safe or not. So check if this value can contain path separators or not. And by check I mean, read the documentation carefully.

Also it seems like you're checking for the file in a different location/under a different name than you actually save it as.

if (file_exists("upload/" . $_FILES["file"]["name"]))
...
move_uploaded_file($_FILES["file"]["tmp_name"],
  "images.example.com/" . randomName() .$_FILES["file"]["name"]);


Accessing an unreturned list within a function in Python without disturbing Scipy Here is the code I'm working on. I'm trying to find the optimum alpha value to get the minimum MAE/MAD. With scipy.optimize, it works quite well. However, when scipy.optimize achieves the optimum alpha, it also achieves the forecast values that essentially I'm trying to get to. Is there a way to return or access and store those forecast values named yPredList without disturbing the work scipy.optimize is doing? Because otherwise I'm going to have to do the forecast calculations from scratch, which is unnecessary. Thanks.

import scipy.optimize as spop
ObservedSpeeds = [85, 64, 97 ,82, 12, 30, 47, 20, 45, 90, 60, 42, 50, 67, 28] #This is not the actual dataset, I actually have a data set of 525,600.
def ErrorCalc(alpha,ObservedSpeeds):
    yPredList = [0,ObservedSpeeds[0]] #First prediction is 0 and the second prediction is the first observation
    for iters in range(2,len(ObservedSpeeds)):
        yPredList.append(np.multiply((1-alpha),yPredList[iters-1]) + np.multiply(alpha,ObservedSpeeds[iters-1]))
    NewList = [np.abs(y-o) for y,o in zip(yPredList,ObservedSpeeds)]
    NewError = np.mean(NewList)
    return NewError

result = spop.minimize_scalar(ErrorCalc,bounds=[0,1],args=ObservedSpeeds,method='bounded')
alpha = result.x

Unfortunately I don't know SciPy or anything about the problem you're solving. I can however comment on coding style.

Style is important

Python has a style convention referred to as PEP 8. The full specification can be found on Python's website. One often repeated mantra is "readability counts", because code is often read more than written. Because consistency improves readability, and conventions enforce consistency, I think following the PEP 8 style guide is important. It will make it easier for other people to understand your code.

The following sections describe the changes I think you could make to improve the style and in turn readability of the code.

Use newlines to separate logical units

This will make the code look better organized and it will make it easier to at a glance get a grasp of what parts the program is composed of.

import scipy.optimize as spop
ObservedSpeeds = [85, 64, 97 ,82, 12, 30, 47, 20, 45, 90, 60, 42, 50, 67, 28] #This is not the actual dataset, I actually have a data set of 525,600.
def ErrorCalc(alpha,ObservedSpeeds):
    yPredList = [0,ObservedSpeeds[0]] #First prediction is 0 and the second prediction is the first observation
    for iters in range(2,len(ObservedSpeeds)):
        yPredList.append(np.multiply((1-alpha),yPredList[iters-1]) + np.multiply(alpha,ObservedSpeeds[iters-1]))
    NewList = [np.abs(y-o) for y,o in zip(yPredList,ObservedSpeeds)]
    NewError = np.mean(NewList)
    return NewError

becomes

import scipy.optimize as spop

ObservedSpeeds = [85, 64, 97 ,82, 12, 30, 47, 20, 45, 90, 60, 42, 50, 67, 28] #This is not the actual dataset, I actually have a data set of 525,600.

def ErrorCalc(alpha,ObservedSpeeds):
    yPredList = [0,ObservedSpeeds[0]] #First prediction is 0 and the second prediction is the first observation

    for iters in range(2,len(ObservedSpeeds)):
        yPredList.append(np.multiply((1-alpha),yPredList[iters-1]) + np.multiply(alpha,ObservedSpeeds[iters-1]))

    NewList = [np.abs(y-o) for y,o in zip(yPredList,ObservedSpeeds)]
    NewError = np.mean(NewList)

    return NewError

snake_case vs CamelCase

In Python, snake_case style is used for variable and function names while CamelCase is used for class names. To someone used to this convention (like me), your code is very confusing at first.

This includes changes to ErrorCalc, ObservedSpeeds, NewError etc.

Allow operators some space

I think the most significant possible style change would be to add some spacing around or after some operators. Generally putting a space after , and spaces around binary operators like +, - etc improve readability a lot.

Conclusion

With a few minor changes, your code can become much easier for other (and yourself) to understand. Learning this is something that will benefit you in the future as well.


Loading div sections from separate files I am loading divs from separate HTML files into a div on the index.html page.

This is being done using jQuery's .load(). There is however, a lot of repetition and I'm wondering if there is a cleaner way to achieve this.

The HTML:

  About
  Help
  Contact

The JS:

$(document).ready( function() {
    $("#about").on("click", function() {
        $("#target").load("../pages/theAboutPage.html");
    });
    $("#help").on("click", function() {
        $("#target").load("../pages/theHelpPage.html");
    });
    $("#contact").on("click", function() {
        $("#target").load("../pages/theContactPage.html");
    });
});

ngView

First of all you described ngView from angularjs

It is what it does. It just helps to organize routing code and template loading. In next section I'll show samples from my current self-learning project.

If you need strict jQuery solution, place appropriate comment.

Angular solution

Make navigation bar with virtual links and place ng-view attribute into container div.

  About
  Help
  Contact

Mark html tag with ng-app attribute and value of you app name

Load js at end of body. You can use cdn for angular libraries. app.js is your application source file.

Your app.js contains main routes

(function() {
  'use strict';
  angular.module('my_app_name', ['ngRoute'])
  .config([
    '$routeProvider', function($routeProvider) {
      $routeProvider.when('/about', {
        templateUrl: 'templates/about.html'
      });
      $routeProvider.when('/help', {
        templateUrl: 'templates/help.html'
      });
      $routeProvider.when('/contact', {
        templateUrl: 'templates/contact.html'
      });
    }
  ]);
}).call(this);

After that you will gen fast, scalable template rendering. Also you can read angular docs for more features.

jQuery solution

IMHO: there is no common solution, but some practices can be applied.

e.g. making configuration array for repeated code.

var navbar_selector = '#navbar',
    target_selector = '#target',
    nav_config = [
        {"id": "about", "text": "About", "url": "../pages/theAboutPage.html"},
        {"id": "help", "text": "Help", "url": "../pages/theHelpPage.html"},
        {"id": "contact", "text": "Contact", "url": "../pages/theContactPage.html"}
    ];

nav_config.forEach(function (config) {
    $('').attr({"id": config.id, "text": config.text}).click(function(){
        $(target_selector).load(config.url);
    }).appendTo(navbar_selector);
})

But it's steel ugly solution.


Node JS routing system I am doing a website for an association and I have never done the server side before. Here I made a very simple routing system:

app.set('port',(process.env.PORT || 5000));

app.use(express.static(__dirname + '/public' ));
app.use(express.static(__dirname + '/views' ));

app.get('/',function(req,res,next) {
    res.redirect('/en/insa');
})

.get('/:language/:page?',function(req, res, next){

    var path = __dirname+'/views/'+req.params.language+'/'+req.params.page+'.ejs';

    fs.access(path,function(err) {
        if (err) {
            res.status(404);
            console.error('404 : /' + req.params.page);
            res.render('404.ejs', {
                page: req.params.page
            });
            return;
        }
        res.charset = "utf-8";

        res.render(path ,{language: req.params.language});
    });
})

.listen(app.get('port'),function() {
    console.log('Server is running, server is listening on port ',app.get('port'));
})

I just redirect automatically the client if he hits the main www.site.com/ page and then I just check for the language and page in the URL like www.site.com/en/description for instance. The fs.access function checks if the EJS (that's a template generator for HTML pages) page actually exists in the folder before rendering it.

Is there anything that could go wrong with this code, or something that could be done better?

It seems a little risky to me to just blindly use the language designation that the URL contains. If it's possible to trick Express into looking somewhere in your file system besides where you expect (perhaps with dots in the path), then you could have a security issue.  It would be safer if you checked to see if it was one of a list of known-safe designations ("en", "fr", "ge", "sp", etc...). If you wanted, you could build a list of known safe designations by looking in the the file system at server startup and then just check it against that.

I would also recommend using path.join() instead of adding in the path separator yourself.  That will make your code more platform friendly should you ever want to run it on Windows.


Sorting List on basis of certain letter I am trying to sort a list on basis of C before .
Is there better way to do this?
import java.util.Iterator;
import java.util.LinkedList;
import java.util.List;

public class MainClass {
    public static void main(String args[]) {

    List<String> activeLoadList = new LinkedList<String>();
    List<String> tempList = new LinkedList<String>();
    activeLoadList.add("REL1110C.PZ");
    activeLoadList.add("REL1110Z.PS");
    activeLoadList.add("REL1110C.AB");
    activeLoadList.add("REL1110Z.CD");
    activeLoadList.add("REL1110C.OP");

    System.out.println("Queue is ***" + activeLoadList);

    Iterator<String> iter = activeLoadList.iterator();
    while (iter.hasNext()) {
        String next = iter.next();
        if (next.contains("C.")) {
            iter.remove();
            tempList.add(next);
        }
    }

    activeLoadList.addAll(tempList);
    System.out.println("Queue is ***" + activeLoadList);
  }
}

Output:

Queue is ***[REL1110C.PZ, REL1110Z.PS, REL1110C.AB, REL1110Z.CD, REL1110C.OP]
Queue is ***[REL1110Z.PS, REL1110Z.CD, REL1110C.PZ, REL1110C.AB, REL1110C.OP]

Instead of modifying the structure of the list (adding, removing) you should rather use a Comparator (see the javadoc), which in your case could look like this:

Collections.sort(activeLoadList, new Comparator<String>() {
    @Override
    public int compare(String a, String b) {
        boolean aContainsPattern = a.contains("C.");
        boolean bContainsPattern = b.contains("C.");
        if (aContainsPattern == bContainsPattern){
            return 0;
        } else {
            return aContainsPattern ? 1 : -1;
        }
    }
});

Note though, that you could/should extract your pattern to a constant instead of having it duplicated (like in my example) and furthermore, that you could use a lambda instead of an anonymous inner class if you use Java 8.

Edit:

In case you use Java 8 (and the ternary operator ?) you could boil that down quite a bit:

final String p = "C."; // the pattern to search for
Collections.sort(activeLoadList, (a, b) -> (a.contains(p) == b.contains(p) ? 0 : (a.contains(p) ? 1 : -1)));

But for the sake of readability you may rather use the more verbose version. Furthermore Solomonoff's Secret's solution is to prefer in case you won't have any further sorting criteria in the future. If you may have additional ones, though, a Comparator may be the easier way to go.

Edit 2:

As has been pointed out by Solomonoff's Secret this could even be simplified further with Java 8's Comparator.comparing:

Collections.sort(activeLoadList, Comparator.comparing(text -> text.contains("C.")));

Edit 3:

Thanks to Fabio F. for an even short version using Java 8:

activeLoadList.sort(Comparator.comparing(text -> text.contains("C.")));


2D Vector: Loading values from .txt file I've written a program that runs through a matrix of numbers, ranging from 0 to 2, and stores the results in a 2D vector.

I'm using the values in the 2D vector as the profile for a tile map for a game engine.

I want this to be optimal, and as this is my first attempt at it, I doubt that it is but it does work as is.

Any suggestions for improvement or obvious errors I've made?

The entire project can be found here:
link

Thanks in advance.

    // NOTE (Devan): Opening the file.
    std::ifstream mapFile ( fileToOpen );
    if ( !mapFile.is_open () )
    {
        printf ( "Error: Could not open file: %s\n", fileToOpen );
        return;
    }

    // NOTE (Devan): Setting the dimensions of the map. The width and height are taken from the first two entries of the text file.
    unsigned int width = 0;
    unsigned int height = 0;
    mapFile >> width;
    mapFile >> height;

    int current = 0;
    for ( unsigned int y = 0; y < height; y++ )
    {
        // NOTE (Devan): Forming a new vector to be stored within the outer vector forming the rows of the map.
        std::vector<int> innerVec;

        for ( unsigned int x = 0; x < width; x++ )
        {
            if ( mapFile.eof () )
            {
                printf ( "Reached end of file %s too early. Check width and height parameters.\n", fileToOpen );
                return;
            }

            // NOTE(Devan): Capturing the value stored at the current location within the text file.
            mapFile >> current;

            // NOTE (Devan): Checking if the input is within the tile type boundaries. Current Types: ( 0 : 2 )
            if( current >= 0 && current <= 2 )
            {
                innerVec.push_back ( current );
            }
            // NOTE (Devan): If the input is outside of the boundaries simply push 0 into the inner vector.
            else
            {
                innerVec.push_back (0);
            }
        }
        tileMap.push_back ( innerVec );
    }

    mapFile.close ();

Your code has a bug. When the very last value cannot be read, it won't detect it. Example:

2 2
1 2
3 invalid

Instead of checking for eof, check whether mapFile >> current returns true.


External variable in callback for chunk() I am trying to implement search in Laravel. I want to search on models. Since there can be many records in the database, I am trying the chunk function.

public function searchLeads($param)
    {
        $results = array();

        // get all leads
        Lead::chunk(100, function($leads) use ($results, $param) {
            // search in name
            $results = array_merge($results, $this->repository->searchInName($leads, $param));

            // search in email
            $results = array_merge($results, $this->repository->searchInEmail($leads, $param));

           // and so on ...
    }

    // eliminate duplicates
    $collection = collect($results);
    $collection = $collection->unique();

    $results = $collection->all();

    dd($results);
    // return $results;
}

Note: The searchIn... functions return array of results and are working as expected.

When I try the above code, I get an empty array. So I changed the following line (added a reference & to $results).

Lead::chunk(100, function($leads) use (&$results, $param) {

Now I get the expected output.

Am I doing this the correct way? Have I stumbled upon the right solution, or am I missing something that might introduce bugs in the code?

You managed to bypass the query builder, which could have implemented this as a trivial, single query on the backing database, and instead managed to emulate that query in an very inefficient manner yourself.

// get all leads
Lead::chunk(100, function($leads) use ($results, $param) {
    // search in name
    $results = array_merge($results, $this->repository->searchInName($leads, $param));

    // search in email
    $results = array_merge($results, $this->repository->searchInEmail($leads, $param));

    // and so on ...
}

This effectively causes 2 additional queries to the database for each 100 results in the Lead model. This might still work reasonably in your test environment where you have at most a few dozen leads registered, but will crawl to a halt in production as the number of leads increases with time.

Even worse, it requires you to stream all existing Lead instances through PHP, even if they are not even part of the final result set.


Node-based rendering Recently, I started wrapping Meshes and Transform classes into new ones, but by doing this I came upon a problem: I needed to render all primitives, or meshes with a renderer. After studying a some game engines (especially the JME3 engine), I came up with this node-based rendering. It's really not complete yet, but functional. I would like to ask you to point out any relevant changes I could make.

Entity.h

#pragma once

#include <glm/glm.hpp>
#include <vector>
#include "../render/Transform.h"

class Entity {
public:
    Entity  (
                const glm::vec3& position = glm::vec3(0.0, 0.0, 0.0),
                const glm::vec3& rotation = glm::vec3(0.0, 0.0, 0.0),
                const glm::vec3& scale = glm::vec3(1.0, 1.0, 1.0)
            )
        :
            m_transform(position, rotation, scale)
    {}
    Entity  (
                const Transform& transform
            )
        :
            m_transform(transform)
    {}

    virtual ~Entity(){}

    virtual void Render(const Shader& shader) = 0;
    virtual void Update(){}

    void SetTransform(const Transform& transform) { m_transform = transform; }
    Transform GetTransform() const { return m_transform; }

    void SetPosition(const glm::vec3& position) { m_transform.SetPos(position); }
    void SetRotation(const glm::vec3& rotation) { m_transform.SetRot(rotation); }
    void SetScale(const glm::vec3& scale) { m_transform.SetScale(scale); }
    glm::vec3 GetPosition() const { return m_transform.GetPos(); }
    glm::vec3 GetRotation() const { return m_transform.GetRot(); }
    glm::vec3 GetScale() const { return m_transform.GetScale(); }
protected:
    Transform m_transform;
};

Spatial.h:

#pragma once

#include "Entity.h"
#include "../render/Material.h"

class Spatial : public Entity
{
public:
    Spatial (
                Mesh mesh,
                const glm::vec3& position = glm::vec3(0.0, 0.0, 0.0),
                const glm::vec3& rotation = glm::vec3(0.0, 0.0, 0.0),
                const glm::vec3& scale = glm::vec3(1.0, 1.0, 1.0)

            )
        :
            Entity(position, rotation, scale),
            m_mesh(mesh),
            m_material(CreateTexture("res/images/White.png"))
    {}
    virtual ~Spatial(){}

    virtual void Render(const Shader& shader){
        m_material.Bind(shader);
        m_mesh->Render();
    }
    virtual void Update(){}

    Mesh GetMesh() const { return m_mesh; }
    void SetMesh(const Mesh& mesh) { m_mesh = mesh; }
    Material GetMaterial() const { return m_material; }
    void SetMaterial(const Material& material) { m_material = material; }
protected:
    Mesh m_mesh;
    Material m_material;
};

Node.h:

#pragma once

#include <vector>
#include <iostream>
#include <cassert>
#include "Entity.h"
#include "Spatial.h"

class Node : public Entity{
public:
    Node(){}
    ~Node(){
        m_components.clear();
    }

    void AttachChild(Entity* component){
        assert((Entity*)this != component);
        m_components.push_back(component);
    }
    void DetachAllChildren(){
        m_components.clear();
    }
    void DetachChild(unsigned index){
        m_components.erase(m_components.begin() + index);
    }

    void Render(const Shader& shader){
        for(auto it = std::begin(m_components); it != std::end(m_components); ++it){
            (*it)->Render(shader);
        }
    }

protected:
    std::vector<Entity*> m_components;
};

main.cpp:

int main(int argc, char **args){

    Node rootNode;

    Node node;

    Spatial spatial = Spatial(CreateMesh("res/models/Suzy.obj"));
    Spatial spatial2 = Spatial(CreateMesh("res/models/Suzy.obj"));

    rootNode.AttachChild(&spatial2);
    node.AttachChild(&spatial1);
    rootNode.AttachChild(node);

    ///Some kind of renderer will render here; haven't implemented that yet...
}

My first question is what does your graph represent? It sounds like you want it to represent the things which need to be rendered. Does the graph encode the order of the rendering? Or is it just a container that holds all the objects to be rendered? It's not clear from what you've shown here why you want things in a graph. (Which isn't to say it's a bad idea. It's just not clear what your reasons are.)

The code you've written is pretty straightforward and seems mostly reasonable. It's very readable, which is great.

What is the purpose of a Node? You've made it an Entity, but it's not clear to me that it should be. It looks like you want it to be an entity so that it can have a Render() method which calls its components Render() methods. From what you're showing here, it's not clear that a Node ever uses its transforms. (If I'm misunderstanding, then let me know.) If that's the case, I'd make the Node class be a free class that doesn't inherit from Entity. It would look mostly the same. To render, you would either give it a Render() method like it has now, or create a class or function that walks the graph, gets each node's list of children and calls each child's Render() method. I'd probably also rename it to EntityNode since it only ever keeps track of entities.

I notice that Entity's Render() method is pure virtual, but its Update() method is not, though it's empty. Will most subclasses of Entity implement it? If not, then it's fine to leave it as an empty method. But if each subclass will implement its own version, then it should probably also be pure virtual, too.

Speaking of virtual methods, if you're using C++11, you should add override after each virtual method in the subclasses, like this:

class Spatial : public Entity {
    //... constructors, etc.
    virtual void Render (const Shader& shader) override {
        //... method contents ...
    }
    // ... other methods...
};

This can help you catch some hard-to-understand errors as your class hierarchy grows. For example, if you ever change the function prototype for the Render() method, it will remind you to fix each subclass's implementation.

In your main method, you're reading the same object twice, though it's not clear why. That might make sense if you're creating 2 instances of the same object. But if not, it probably indicates a need to structure things differently. Since I don't know your goals there, I can't say which it is.


Finding the Players with the highest score Given a list of Players with score, I want to find the ones with the highest score. There can be multiple players with the same score. I'm doing it like this now:

class Player {
    final String name;
    final int score;

    Player(String name, int score) {
        this.name = name;
        this.score = score;
    }
}

class PlayerComparatorByScore implements Comparator<Player> {
    @Override
    public int compare(Player o1, Player o2) {
        return -Integer.compare(o1.score, o2.score);
    }
}

class PlayerUtil {
    static Collection<Player> getHighestScoringPlayers(Collection<Player> players) {
        List<Player> sortedPlayers = new ArrayList<>(players);
        Collections.sort(sortedPlayers, new PlayerComparatorByScore());

        Set<Player> highestScoringPlayers = new HashSet<>();
        Iterator<Player> iterator = sortedPlayers.iterator();
        Player highestScoringPlayer = iterator.next();
        highestScoringPlayers.add(highestScoringPlayer);

        while (iterator.hasNext()) {
            Player player = iterator.next();
            if (player.score == highestScoringPlayer.score) {
                highestScoringPlayers.add(player);
            } else {
                break;
            }
        }

        return highestScoringPlayers;
    }
}

public class PlayersSortedByScoreTest {
    @Test
    public void testSortingByScore() {
        Collection<Player> players = new HashSet<>();
        players.add(new Player("Alice", 3));
        players.add(new Player("Bob", 1));
        players.add(new Player("Mike", 3));

        Collection<Player> highestScoringPlayers = PlayerUtil.getHighestScoringPlayers(players);

        assertEquals(2, highestScoringPlayers.size());
        assertEquals(3, highestScoringPlayers.iterator().next().score);
    }
}

So this is pretty awkward... Is there a better way?

This is a terrible approach. Sorting the list (an \$O(n \log(n)\$) step) and then iterating through it (an \$O(n)\$ operation) to collect the highest scoring players is simply inefficient.

Instead, just iterate through the list in this way:

List<Player> playersList = new ArrayList<>(players);
Set<Player> highestScoringPlayers = new HashSet<>();
int maxScore = Integer.MIN_VALUE;
for (Player player : playersList) {
    maxScore = Math.max(maxScore, player.score)
}
for (Player player : playersList) {
    if (player.score == maxScore) {
        highestScoringPlayers.add(player);
    }
}
return highestScoringPlayers;

This uses 2 loops and the minimal amount of space (just enough for the original list and the highest scorers) and so is \$O(n)\$, which is better than your \$O(n \log(n))\$ solution.

If you would like to only use 1 loop, at the cost of some additional overhead here is the code:

List<Player> playersList = new ArrayList<>(players);
Set<Player> highestScoringPlayers = new HashSet<>();
int maxScore = Integer.MIN_VALUE;
for (Player player : playersList) {
    if (player.score >= maxScore) {
        if (player.score > maxScore) {
            maxScore = player.score;
            highestScoringPlayer.clear();
        }
        highestScoringPlayer.add(player);
    }
}


Find all nodes in tree, without a sibling Find all nodes of trees, without siblings.

This question is attributed to GeeksForGeeks.

I'm looking for code reviews, optimizations and best practices.

  public class PrintNodesWithoutSiblings<T> {

        private TreeNode<T> root;

        public PrintNodesWithoutSiblings(List<T> items) {
            create(items);
        }

        private void create (List<T> items) {
            root = new TreeNode<T>(items.get(0));

            final Queue<TreeNode<T>> queue = new LinkedList<TreeNode<T>>();
            queue.add(root);

            final int half = items.size() / 2;

            for (int i = 0; i < half; i++) {
                if (items.get(i) != null) {
                    final TreeNode<T> current = queue.poll();
                    int left = 2 * i + 1;
                    int right = 2 * i + 2;

                    if (items.get(left) != null) {
                        current.left = new TreeNode<T>(items.get(left));
                        queue.add(current.left);
                    }
                    if (right < items.size() && items.get(right) != null) {
                        current.right = new TreeNode<T>(items.get(right));
                        queue.add(current.right);
                    }
                }
            }
        }

        private static class TreeNode<T> {
            private TreeNode<T> left;
            private T item;
            private TreeNode<T> right;

            TreeNode(T item) {
                this.item = item;
            }
        }

        public List<T> nonSiblingNodes() {
            if (root == null) {
                throw new IllegalStateException("Empty tree. Tree cannot be empty.");
            }

            final List<T> list = new ArrayList<T>();
            findNonSiblingNodes(root, list);
            return list;
        }

        private void findNonSiblingNodes(TreeNode<T> node, List<T> list) {
            if (node == null) {
                return;
            }

            if (node.left != null && node.right != null) {
                findNonSiblingNodes(node.left, list);
                findNonSiblingNodes(node.right, list);
            } else if (node.left != null) {
                list.add(node.left.item);
                findNonSiblingNodes(node.left, list);
            } else if (node.right != null) {
                list.add(node.right.item);
                findNonSiblingNodes(node.right, list);
            }
        }
    }

    public class PrintNodesWithoutSiblingTest {

        @Test
        public void test1() {
            PrintNodesWithoutSiblings<Integer> ps1 = new PrintNodesWithoutSiblings<Integer>(Arrays.asList(1, 2, 3, 4, null, 6, null));
            assertTrue(Arrays.asList(4, 6).equals(ps1.nonSiblingNodes()));
        }

        @Test
        public void test2() {
            PrintNodesWithoutSiblings<Integer> ps2 = new PrintNodesWithoutSiblings<Integer>(Arrays.asList(1, 2, null, 4, null, null, null));
            assertTrue(Arrays.asList(2, 4).equals(ps2.nonSiblingNodes()));
        }
    }

Recursion
The null-check in your recursive method is redundant. The preliminary checks and the mid-recursion checks ensure that the method is never called with a null input.
I also really favour a simplified recursive approach, where the recursion checks it's own conditions, instead of pre-checking the recursive conditions before calling the recursion. In this case, it halves the number of recursive calls, halves the list.adds, and reduces by a third the number of if-conditions in your code:
    private void findNonSiblingNodes(TreeNode<T> node, boolean hasSibling List<T> list) {
        if (node == null) {
            return;
        }

        if (!hasSibling) {
            list.add(node);
        }

        findNonSiblingNodes(node.left, node.right != null, list);
        findNonSiblingNodes(node.right, node.left != null, list);
    }

You would call that code with:
    public List<T> nonSiblingNodes() {
        final List<T> list = new ArrayList<T>();
        findNonSiblingNodes(root, true, list);
        return list;
    }
}

It is safe to remove the null-check on the root node, because it can never be null at that point. That's because ....
Bugs
... your code does horrible things in the constructor, which will fail before it gets here:
    public PrintNodesWithoutSiblings(List<T> items) {
        create(items);
    }

    private void create (List<T> items) {
        root = new TreeNode<T>(items.get(0));
        ....

Your constructor will throw NPE if the items is null, and will throw NoSuchElementException if the List is empty. Thus, the root will never be null in the recursion.
Style

final Queue<TreeNode<T>> queue = new LinkedList<TreeNode<T>>();

You should use the 'diamond operator' when using Java7 and later. The line above can be just:
final Queue<TreeNode<T>> queue = new LinkedList<>();

Similarly, the line:

final List<T> list = new ArrayList<T>();

can be:
final List<T> list = new ArrayList<>();


If/ else if statements to show and hide I've got some if and else if statements to show and hide svg elements and it works the way it should but I wonder if I can shorten the code since it looks so repetitive.

I'm not sure if it's possible since I'm not a JS/JQuery expert.

If/else if code

        //Door type 1
        if( id2 == "#Door1" && id3 == "#Grey")
        {
            var door = "#Door1Grey"
            $("g" + door).show().siblings("g:not(#BG)").hide();
            //console.log($("g" + door).show().siblings());
        }
        else if( id2 == "#Door1" && id3 == "#Blue")
        {
            var door = "#Door1Blue"
            $("g" + door).show().siblings("g:not(#BG," + id + ")").hide();
            //console.log(door);
        }
        else if( id2 == "#Door1" && id3 == "#Red")
        {
            var door = "#Door1Red"
            $("g" + door).show().siblings("g:not(#BG," + id + ")").hide();
            //console.log(door);
        }

        //Door type 2
        else if( id2 == "#Door2" && id3 == "#Grey")
        {
            var door = "#Door2Grey"
            $("g" + door).show().siblings("g:not(#BG," + id + ")").hide();
            //console.log(door);
        }
        else if( id2 == "#Door2" && id3 == "#Blue")
        {
            var door = "#Door2Blue"
            $("g" + door).show().siblings("g:not(#BG," + id + ")").hide();
            //console.log(door);
        }
        else if( id2 == "#Door2" && id3 == "#Red")
        {
            var door = "#Door2Red"
            $("g" + door).show().siblings("g:not(#BG," + id + ")").hide();
            //console.log(door);
        }

        //Door type 3
        else if( id2 == "#Door3" && id3 == "#Grey")
        {
            var door = "#Door3Grey"
            $("g" + door).show().siblings("g:not(#BG," + id + ")").hide();
            //console.log(door);
        }
        else if( id2 == "#Door3" && id3 == "#Blue")
        {
            var door = "#Door3Blue"
            $("g" + door).show().siblings("g:not(#BG," + id + ")").hide();
            //console.log(door);
        }
        else if( id2 == "#Door3" && id3 == "#Red")
        {
            var door = "#Door3Red"
            $("g" + door).show().siblings("g:not(#BG," + id + ")").hide();
            //console.log(door);
        }

id, id2 and id3 are set here:

        var wallChoice = String($('input[name=radiosWall]:checked', '#myFormFront').val());
        var id = String("#" + wallChoice);
        $("g" + id).show().siblings("g:not(#BG," + id + ")").hide();

        var doorChoice = String($('input[name=radiosDoor]:checked', '#myFormFront').val());
        var id2 = String("#" + doorChoice);

        var doorColorChoice = String($('input[name=radiosColor]:checked', '#myFormFront').val());
        var id3 = String("#" + doorColorChoice);

You're right, it is a bit repetitive. I believe you could boil it down to the following:

var door = id2 + id3.substr(1);
$("g" + door).show().siblings("g:not(#BG," + id + ")").hide();

or even:

$("g" + id2 + id3.substr(1)).show().siblings("g:not(#BG," + id + ")").hide();

ALSO if you change:

var doorColorChoice = String($('input[name=radiosColor]:checked', '#myFormFront').val());
var id3 = String("#" + doorColorChoice);

to

var id3 = String($('input[name=radiosColor]:checked', '#myFormFront').val());

you could have:

$("g" + id2 + id3).show().siblings("g:not(#BG," + id + ")").hide();

(provided the rest of your code doesn't need the "#" (hash) in id3. That would be faster than using substr)


Census data from file input (text) The program takes census data from a text file in the format age, gender, marital status, district. It then gives a list of residents by district (there are 22) and by 5 age groups. Any suggestions for improvement?

The only topics we have covered so far in class are methods, decisions, looping, and arrays.

static void Main(string[] args)
    {
        FileStream fStream = new FileStream("test.txt", FileMode.Open, FileAccess.Read);
        StreamReader inFile = new StreamReader(fStream);

        string inputRecord = "";
        string[] fields;
        int[] ageData = new int[1000];
        string[] genderData = new string[1000];
        string[] maritalData = new string[1000];
        int[] districtData = new int[1000];
        int[] countDist = new int[23];
        int[] ageGroup = new int[5];

        inputRecord = inFile.ReadLine();

        int i = 0;
        while (inputRecord != null)
        {
            fields = inputRecord.Split(',');

            ageData[i] = int.Parse(fields[0]);
            genderData[i] = fields[1];
            maritalData[i] = fields[2];
            districtData[i] = int.Parse(fields[3]);

            if (ageData[i] > 0 && ageData[i] <= 18)
            {
                ageGroup[0] = ageGroup[0] + 1;
            }
            if (ageData[i] > 18 && ageData[i] <= 30)
            {
                ageGroup[1] = ageGroup[1] + 1;
            }
            if (ageData[i] > 30 && ageData[i] <= 45)
            {
                ageGroup[2] = ageGroup[2] + 1;
            }
            if (ageData[i] > 45 && ageData[i] <= 64)
            {
                ageGroup[3] = ageGroup[3] + 1;
            }
            if (ageData[i] >= 65)
            {
                ageGroup[4] = ageGroup[4] + 1;
            }

            i++;

            inputRecord = inFile.ReadLine();
        }
        Console.WriteLine("This Program takes census data from a file and lists residents by age group and district");
        Console.WriteLine("Please ensure the file is formatted correctly! age,gender (M/F),marital status   (M/S), and district");
        for (int x = 1; x <= 22; x++)
            for (int y = 0; y < districtData.Length; y++)
                if (districtData[y] == x)
                    countDist[x]++;

        for (int x = 1; x <= 22; x++)
            Console.WriteLine("District   " + x + "   has   " + countDist[x] + "   citizens");

        Console.WriteLine("------Amount of Residents per Age Group--------");
        Console.WriteLine("Age Group 18 & under = {0}", ageGroup[0]);
        Console.WriteLine("Age Group 18-30 = {0}", ageGroup[1]);
        Console.WriteLine("Age Group 31-45 = {0}", ageGroup[2]);
        Console.WriteLine("Age Group 46-64 = {0}", ageGroup[3]);
        Console.WriteLine("Age Group 65 & over = {0}", ageGroup[4]);
    }

EDIT

Per comments below, the instructor thinks I should get rid of the ageGroup and districtData. This is what he was suggesting I do - however, I have no idea how I'm going to change my logic that was using districtData.

   for (int districtCount = 1; districtCount <= 22; districtCount++)
   {
       for (int districtNumber = 0; districtNumber < districtData.Length; districtNumber++)
       {
           if (districtData[districtNumber] == districtCount)
           {
             countDist[districtCount]++;
           }
       }
   }

user2023861 covers a lot of good ground, so I just have a couple more things to add.

First, instead of using parallel arrays, you might want to consider using a collection container like a List and filling it with objects.  Each line appears to a distinct record with a set of properties, so I would make them a simple Class:

public class Record
{
    public int Age { get; set; }
    public string Gender { get; set; }
    public string MaritalStatus { get; set; }
    public int District { get; set; }
}

That way, you don't have to hard code the size of the arrays that you are using, and you don't have to do any work to keep the related pieces of the record in synch (think what would happen if one of the arrays got sorted and the rest didn't).

Then, in your parser you can create objects for each record:

var records = new List<Record>();
var inputRecord = inFile.ReadLine();

while (inputRecord != null)
{
    var fields = inputRecord.Split(',');
    var currentRecord = new Record()
    {
        Age = int.Parse(fields[0]),
        Gender = fields[1],
        MaritalStatus = fields[2],
        District = int.Parse(fields[3])
    };

    records.Add(currentRecord);
    //...
}

You need to remember to dispose of  your FileStream and StreamReader if you are accessing them this way.  The easier and safer way to do this is with the using syntax, which will close and dispose of them at the ending braces.  I'd get into the habit of using this with any object with a .Dispose() method.

using (var fStream = new FileStream("test.txt", FileMode.Open, FileAccess.Read))
{
    using (var inFile = new StreamReader(fStream))
    {
        //Work with the file here
        //...
    } //inFile.Dispose() happens here automatically.
} //fStream.Dispose() happens here automatically.

This code...

    for (int x = 1; x <= 22; x++)
        for (int y = 0; y < districtData.Length; y++)
            if (districtData[y] == x)
                countDist[x]++;

...is begging for braces to make it easier to read.  It also help avoid potential errors.


Optimization of list's sublist the problem is to find total number of sub-lists from a given list that doesn't contain numbers greater than a specified upper bound number say right and sub lists max number should be greater than a lower bound say left .Suppose my list is: x=[2, 0, 11, 3, 0] and upper bound for sub-list elements is 10 and lower bound is 1 then my sub-lists can be [[2],[2,0],[3],[3,0]] as sub lists are always continuous .My script runs well and produces correct output but needs some optimization

def query(sliced,left,right):
    end_index=0
    count=0
    leng=len(sliced)
    for i in range(leng):
        stack=[]
        end_index=i

        while(end_index<leng and sliced[end_index]<=right):

            stack.append(sliced[end_index])
            if max(stack)>=left:
                count+=1
            end_index+=1

    print (count)

origin=[2,0,11,3,0]
left=1
right=10
query(origin,left,right)

output:4

You can streamline by dropping the use of append, and ignoring values in your for-loop that are automatically disqualified.  Benchmarking shows these steps reduce execution time by about 2x.

Original:

%%timeit

origin=[2,0,11,3,0]
left=1
right=10
query(origin,left,right)
# 3.6 µs ± 950 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

New version:

def query2(sliced,left,right):
    count = 0
    leng = len(sliced)
    for i in range(leng):
        if sliced[i] <= right:
            stack_ct = 0
            end_index = i
            found_max = False
            while(end_index < leng and sliced[end_index] <= right):
                if not found_max:
                    found_max = sliced[end_index] >= left
                end_index += 1
                stack_ct += 1
            if found_max:
                count += stack_ct
    return count

Benchmarking:

%%timeit

origin=[2,0,11,3,0]
left=1
right=10
query2(origin,left,right)
# 1.83 µs ± 41.6 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)


Dynamically loaded slider Problem: Make a slider, located at the bottom of the page. The slider should be dynamically loaded only when a div (#slider) becomes visible on the screen. Images do not have to load all at once. Load the first two images, then load the third when we switched to the second, and so on.

I would like other users to look look at my code and tell me what I'm doing wrong and what could be done better (if you can - in more detail).

$(function () {
    var countImg = 5,
        indexImg = 1,
        indexImgLoad = 2,
        pathImg = '/img/slider/',
        formatImg = 'jpg',
        controlFlag = 1,
        elWrap = $('#slider'),
        el =  elWrap.find('img');
    function create () {
        Img = new Image();
        Img.src= pathImg+indexImg+'.'+formatImg;
        $(Img).appendTo($("#slides"));
        $('').appendTo($("#slider"));
        navigation();
        check();
    }
    function change () {
        el.fadeOut(500);
        el.filter(':nth-child('+indexImg+')').fadeIn(500);
    }
    function newimg() {
        nextImg = new Image();
        nextImg.src= pathImg+indexImgLoad+'.'+formatImg;
        $(nextImg).appendTo($("#slides"));
    }
    function check() {
        if (controlFlag && indexImgLoad <= countImg) {
            newimg();
        }
        if (controlFlag){
            el =  elWrap.find('img');
        }
        if (el.length == countImg){
            controlFlag = 0;
        }
    }
    function navigation(){
        $('span.next').click(function() {
            indexImg++;
            indexImgLoad++;
            if(indexImg > countImg) {
                indexImg = 1;
            }
            check();
            change();
        });
        $('span.prev').click(function() {
            indexImg--;
            if(indexImg < 1) {
                indexImg = countImg;
            }
            indexImgLoad++;
            check();
            change ();
        });
    }
    var loadSlider = 1;
    window.onscroll = function() {
          var pageY = window.pageYOffset || document.documentElement.scrollTop,
        heightScreen = window.innerHeight + pageY;
        sliderTopY = $("#slides").offset().top;
        if ((heightScreen > (sliderTopY - sliderTopY * 0.05)) && loadSlider) {
            loadSlider = 0;
            create();
        }
    }
});
.slider_wrap {
        margin: 0px auto 0;
        width: 735px;
        height: 200px;
        position: relative;
        overflow: hidden;
    }
    .slider_wrap img {
        width: 645px;
        height: auto;
        display: none;
        position: absolute;
        top: 0;
        left: 45px;
    }
    .slider_wrap img:first-child {
        display: block;
    }
    .slider_wrap span {
        width: 45px;
        height: 200px;
        display: block;
        position: absolute;
        cursor: pointer;
        background: url(/img/rotator_buttons.png) no-repeat;
        opacity: 0.7;
    }
    .slider_wrap span.next {
        right: 0;
        background-position: -45px 0;
    }
    .slider_wrap span.next:hover, span.prev:hover {
        opacity: 1.0;
    }
    .slider_wrap span.prev {
        background-position: 0 0;
    }

div: height = 1800px

Few things:

You can create a plugin out of it (create divs #slider and #slides - don't use id, but just reference - and add class slider_wrap in code)
You don't put var before nextImg = new Image();
Instead of new Image you can use jQuery:

$('<img/>').attr('src', path).appendTo($("#slides"));
Instead of $('span.next') use elWrap.find('span.next') because it will not work if span.next is somewhere in the html.
Is always better ot use jQuery not native function because they may be different from browser to browser. So use $(window).scroll(function() { }) $(window).innerHeight() and $(window).scrollTop();
Inside newimg you call $("#slides") you can call that before a function and store it's value in variable.


Checking if an associated object has failed to save/create I smell something wrong here, but am not quite sure what the most idiomatic way to solve the problem.

I have a controller for (job)listings and it has a method called apply.  This method is used when someone has a resume and is clicking to apply for the job associated with the listing.

def apply
  set_resume
  if @resume.apply_to(@listing)
    redirect_to @listing
  else
    redirect_to @listing, alert: 'Unable to apply for job'
  end
end

This calls a method on the resume object:

def apply_to(listing)
  jobapp = Jobapp.new(resume: self, listing: listing)
  jobapp.save
end

Everything above works just fine - it just seems inelegant (and I don't get the errors associated with the jobapp object, although that's not vitally important).

If I use the following method in place of the above method:

def apply_to(listing)
  Jobapp.create(resume: self, listing: listing)
end

The conditional if @resume.apply_to(@listing) always returns true, even if the Jobapp failed to be created.

What is a cleaner way of achieving this task?

Given the current setup, the simplest thing would probably be to raise an exception using #save!:

def apply_to(listing)
  jobapp = Jobapp.new(resume: self, listing: listing)
  jobapp.save! # note the bang (!)
end

And in the controller

def apply
  set_resume
  @resume.apply_to(@listing)
  redirect_to @listing
rescue ActiveRecordError
  redirect_to @listing, alert: 'Unable to apply for job'
end

It may be worth the effort to rename your method to apply_to! (also with a bang) to make it clear that it can raise an exception.

As for why create doesn't work, it's not that it returns true - it returns the new record (and all objects except nil and false are considered "true"). To check if create actually managed to persist that record in the database, you can check record.persisted?. If true, then it was saved just fine.

So using your create version of the method, you could do

def apply
  set_resume
  jobapp = @resume.apply_to(@listing)
  if jobapp.persisted?
    redirect_to @listing
  else
    redirect_to @listing, alert: 'Unable to apply for job'
  end
end

This would also let you access jobapp.errors, if you want.

A few other things caught my eye:

set_resume should probably be in a before-filter. Or be a method that just returns a record, leaving it up to the caller to stick it in an instance variable (or not)
I imagine a jobapp belongs to a listing (and/or a resume). If so, it'd be cleaner to build it like so:

jobapp = owner.jobapps.new(other: foo)

Given that it's this simple, though, it might be something you could just do in the controller.

It's never quite clear where a join model like jobapp (which I assume is basically a join model) should be created. Should it be in the Resume model or in the Listing model? Either one makes sense. So the Solomonic solution is do neither (at least for now), and handle it in the controller:

def apply
  jobapp = @listing.jobapps.new(resume: @resume)
  if jobapp.save
    redirect_to @listing
  else
    redirect_to @listing, alert: 'Unable to apply for job'
  end
end

Lastly, you might want to tweak that error message. It seems a little "blunt" to me. Though it's the server that failed, it sounds as if the user is unable to apply for the job. "Failed to save job application" might be more appropriate


C++ Registry pattern implementation I created a template registry class. It supports custom factory functions and custom constructors. Any kind of feedback is welcome (style, performance, bugs)

registry.hpp:

#ifndef REGISTRY_HPP
#define REGISTRY_HPP

//STL
#include <unordered_map>
#include <memory>

template<typename Key>
class RegistryConstructionError : public std::exception
{
public:
    RegistryConstructionError(const Key &key)   :key(key){}

    const char *what()const throw(){return "Undefined key";}
    const Key key;
};

template<typename Base, typename Key, typename... Args>
class Registry
{
public:
    typedef std::unique_ptr<Base> (*FactoryFunc)(Args... args);

    Registry() = default;
    ~Registry() = default;

    template<typename Derived>
    void registerClass(const Key &key);
    void registerFactory(const Key &key, const FactoryFunc factory);

    std::unique_ptr<Base> construct(const Key &key, Args... args);

private:
    template<typename Derived>
    static std::unique_ptr<Base> constructDerived(Args... args);

    std::unordered_map<Key, FactoryFunc> m_map;

public:
    typedef RegistryConstructionError<Key> ConstrError;
};

#include "registry.tpp"

#endif // REGISTRY_HPP

registry.tpp:

template<typename Base,
        typename Key,
        typename... Args>
template<typename Derived>
void Registry<Base, Key, Args...>::registerClass(const Key &key)
{
    m_map[key] = &constructDerived<Derived>;
}

template<typename Base,
        typename Key,
        typename... Args>
void Registry<Base, Key, Args...>::registerFactory(const Key &key, const FactoryFunc factory)
{
    m_map[key] = factory;
}

template<typename Base,
        typename Key,
        typename... Args>
std::unique_ptr<Base> Registry<Base, Key, Args...>::construct(const Key &key, Args... args)
{
    try{
        return m_map.at(key)(std::forward<Args>(args)...);
    }
    catch(const std::out_of_range &e)
    {
        throw ConstrError(key);
    }
}

template<typename Base,
        typename Key,
        typename... Args>
template<typename Derived>
std::unique_ptr<Base> Registry<Base, Key, Args...>::constructDerived(Args... args)
{
    return std::make_unique<Derived>(std::forward<Args>(args)...);
}

Good stuff!

I definitely see a few improvements you could make here:

Prefer using std::function to function pointers

There is a tiny bit more overhead invovled, but the extra flexibility is well worth it.

using FactoryFunc = std::function<std::unique_ptr<Base>(Args...)>;

Another advantage is that it lets you get rid of constructDerived entirely, and replace it with a nice lambda in registerClass.

Nitpick: registerClass should invoke registerFactory.

Since registerClass is just a specialization of registerFactory, it should invoke it instead of reimplementing it. This is preferable because having a single point of entry makes refactorings like adding logging or debug monitoring a lot less error-prone.

Prefer using instead of typedef

The semantics of type alisases are just a lot more legible that way.

Wrong function definition for forwarding args

If you want to use forwarding as you do, construct needs to be declared in a way to use universal references, like so:

template<typename... CtrArgs>
std::unique_ptr<Base> construct(const Key &key, CtrArgs&&... args)

Otherwise, your std::forward<> call is useless and might as well be a std::move.

Personal opinion: The custom exception type is not necessary here.

Letting the out_of_range exception bubble up is perfectly acceptable here, and lets your code be cleaner.


Return array with products of old array except i Coding problem:

  Given an array of integers, return a new array such that each element at index i of the new array is the product of all the numbers in the original array except the one at i.

My solution:

arr = np.array([1, 2, 3, 4, 5])

def coding_solution(_arr):
    ind = 0
    new_arr = []
    for n in range(len(_arr)):
        v = 1
        for m, i in enumerate(_arr):
            v *= i if m != ind else 1
        new_arr.append(v)
        ind += 1
    return np.array(new_arr)

Output: [120, 60, 40, 30, 24]

Is there a faster or less space consuming solution? It takes longer to run then I would've expected or would think is acceptable.

Review:

So there are some changes that can be made. As you are running a nested for loop that should be the first candidate for removal, python isn't the best with lots of loops. I've made some changes below to improve your code.

Here we actually use n, instead of ind, they are the same, so ind is excessive
I've expanded the if ... else block as this improves readability
I've changed i for x as this is easier to quickly differentiate from 1

def coding_solution(_arr):
    new_arr = []
    for n in range(len(_arr)):
        v = 1
        for m, x in enumerate(_arr):
            if m != n:
                v *= x
            else:
                v *= 1
        new_arr.append(v)
    return np.array(new_arr)

Alternate solutions:

I have two methods that work, one is pure python (using two core libraries), but could include numpy; the other is all numpy. They both work on a similar principal, that is that all new values are the same product (the entire array) divided by the value at the same index in the original array. Therefore, you can just divide the product of the entire array by each value in the original array, as long as there are no zero values (thanks One Lyner!).

Pure python:

from functools import reduce
from operator import mul

arr = [1, 2, 3, 4, 5]

def f(_arr):
    arr_prod = reduce(mul, _arr)  # You could use np.prod(_arr) instead of reduce(...)
    return [arr_prod / x for x in _arr]

The pure python function could be a single line, but then you might end up computing the product of the entire array on every iteration.

Numpy

import numpy as np

arr = np.array([1, 2, 3, 4, 5])

def g(_arr):
    return np.ones(_arr.shape) * _arr.prod() / _arr

# Even more succinct, thanks Graipher
def h(_arr):
    return _arr.prod() / _arr

This Numpy solution would be vectorised and would scale well, but may be slower on smaller arrays. It works by creating a new np.array of all 1; multiplying it by the product, so all the values are the product; finally dividing the 'product array' by the original array, which is element-wise.


Finding the best category of poker hand I have 16 methods in Java that return int values. I need to find the one that returns the highest value.

I initially implemented this:

private int best() {
    int max = playGame(ONES);
    max = Math.max(max, playGame(TWOS));
    max = Math.max(max, playGame(THREES));
    max = Math.max(max, playGame(FOURS));
    max = Math.max(max, playGame(FIVES));
    max = Math.max(max, playGame(SIXES));
    max = Math.max(max, playGame(SEVENS));
    max = Math.max(max, playGame(EIGHTS));
    max = Math.max(max, playGame(THREE_OF_A_KIND));
    max = Math.max(max, playGame(FOUR_OF_A_KIND));
    max = Math.max(max, playGame(FULL_HOUSE));
    max = Math.max(max, playGame(SMALL_STRAIGHT));
    max = Math.max(max, playGame(LARGE_STRAIGHT));
    max = Math.max(max, playGame(ALL_DIFFERENT));
    max = Math.max(max, playGame(CHANCE));
    max = Math.max(max, playGame(ALL_SAME));
    return max;
}

To make this slightly better, I could reduce one comparison by comparing two methods simultaneously.

private int best() {
    int max1 = Math.max(playGame(ONES), playGame(TWOS));
    int max2 = Math.max(playGame(THREES), playGame(FOURS));
    int max3 = Math.max(playGame(FIVES), playGame(SIXES));
    int max4 = Math.max(playGame(SEVENS), playGame(EIGHTS));
    int max5 = Math.max(playGame(THREE_OF_A_KIND), playGame(FOUR_OF_A_KIND));
    int max6 = Math.max(playGame(FULL_HOUSE), playGame(SMALL_STRAIGHT));
    int max7 = Math.max(playGame(LARGE_STRAIGHT), playGame(ALL_DIFFERENT));
    int max8 = Math.max(playGame(CHANCE), playGame(ALL_SAME));
    int max11 = Math.max(max1, max2);
    int max12 = Math.max(max3, max4);
    int max13 = Math.max(max5, max6);
    int max14 = Math.max(max7, max8);
    int max21 = Math.max(max11, max12);
    int max22 = Math.max(max13, max14);
    int max = Math.max(max21, max22);
    return max;
}

I mentioned 16 methods as playGame(CONSTANT) callbacks different methods based on the constant.
However, this code is still bad. What could be a better way to achieve this?

I think that it would be best if you could put those constants into a enum so that you can use Enum.values to get them all in an array. If so you can do this:

enum Play {
 TWOS, THREES, FOURS, FIVES, SIXES, SEVENS, EIGHTS, THREE_OF_A_KIND,
 FOUR_OF_A_KIND, FULL_HOUSE, SMALL_STRAIGHT, LARGE_STRAIGHT, ALL_DIFFERENT,
 CHANCE, ALL_SAME;
}

class Main {

  int playGame(Play play) {
     // you need to implement this method returning something meaningful.
     return 0;
  }

  int best() {
     return Stream.of(Play.values())
                  .mapToInt(this::playGame)
                  .max().getAsInt();
  }
}

If you want to keep those constants as independent static final (Strings?) then you will need to create a list of them explicitly:

//...
static final String TWOS = "TWOS";
static final String THREES = "THREES";
//...
static final List<String> PLAYS = Arrays.asList(TWOS, THREES, ...);
//...
int best() {
   return PLAYS.stream()
               .mapToInt(this::playGame)
               .max().getAsInt();
}


python object pool with metaclasses class Pool(type):
    pool = dict()
    def __new__(clas, *a, **k):
        def __del__(self):
            Pool.pool[self.__class__] = Pool.pool.get(self.__class__, []) + [self]
        a[-1]['__del__'] = __del__
        return type.__new__(clas, *a, **k)

    def __call__(clas, *a, **k):
        if Pool.pool.get(clas):
            print('Pool.pool is not empty: return an already allocated instance')
            r = Pool.pool[clas][0]
            Pool.pool[clas] = Pool.pool[clas][1:]
            return r
        else:
            print('Pool.pool is empty, allocate new instance')
            return type.__call__(clas, *a, **k)

class Foo(metaclass=Pool):
    def __init__(self):
        print('Foo > .', self)
    def foo(self):
        print('Foo > foo:', self)

f1 = Foo()
f1.foo()

print('## now deleting f1')
del f1

print('## now create f2')
f2 = Foo()
f2.foo()

print('## now create f3')
f3 = Foo()
f3.foo()

what do you think about this piece of code?

class Pool(type):

The defaultdict class will automatically create my list when I access it

    pool = collection.defaultdict(list)

The python style guide suggests using the form class_ rather then clas
It is also not clear why you are capturing the incoming arguments as variable.
This being a metaclass it is going to have consistent parameters

    def __new__(class_, name, bases, classdict):
        def __del__(self):

Since pool now automatically creates the list, we can just append to it
                Pool.pool[self.class].append(self)

Getting rid of the variable arguments also makes this much clearer.

        classdict['__del__'] = __del__
        return type.__new__(class_, name, bases, classdict)

args and kwargs are often used as the names for variable parameters. I recommend using them to make code clearer

    def __call__(class_, *args, **kwargs):

Thanks to the use of defaultdict above we can make this code quite a bit cleaner. Also, this code doesn't pretend its using a functional programming language. Your code created lists by adding them together, slicing, etc. That is not a really efficient or clear way to use python.

        instances = Pool.pool[class_]
        if instances:

There is a subtle and dangerous problem here. When you create a new instance, you pass along your parameters.
However, if an instance is already created, you return that ignoring what the parameters were doing.
This means that you might get an object back which was created using different parameters then you just passed.
I haven't done anything to fix that here

            print('Pool.pool is not empty: return an already allocated instance')
            return instances.pop()
        else:
            print('Pool.pool is empty, allocate new instance')
            return type.__call__(class_, *args, **kwargs)

Your technique is to install a __del__ method on the objects so that you can detect when they are no longer being used and keep them in your list for the next person who asks for them. the __del__ method is invoked when the object is about to be deleted. You prevent the deletion by storing a reference to the object in your list. This is allowed but the python documentation indicates that it is not recommended.

__del__ has a number of gotchas. If an exception is caused while running the __del__ method it will be ignored. Additionally, it will tend to cause objects in references cycles to not be collectable. If your code is ever run on Jython/IronPython then you can't be sure that __del__ will be called in a timely manner. For these reasons I generally avoid use of __del__.

You are using a metaclass so the fact that the given object is pooled is hidden from the user. I don't really think this is a good idea. I think it is far better to be explict about something like pooling. You also lose flexibility doing it this way. You cannot create multiple pools, etc.

The interface that I would design for this would be:

pool = Pool(Foo, 1, 2, alpha = 5) # the pool gets the arguments that will be used to construct Foo
with pool.get() as f1:
    # as long as I'm in the with block, I have f1, it'll be returned when I exit the block
    f1.foo()
with pool.get() as f2:
    f2.foo()
    with pool.get() as f3:
        f3.foo()

f4 = pool.get()
f4.foo()
pool.repool(f4)


A simple fork with an ugly waitpid This is my first take on fork, exec and waitpid. I didn't quite understand why I had to use a while loop with waitpid. I discovered that waitpid sets errno to EINTR which is translated to this:

#define EINTR        4  /* Interrupted system call */

And I saw a suggestion in comp.unix.programmer archives to use a while loop for waitpid.

My implementation works on OSX 10.10.3 but it doesn't quite satisfy me. Can you provide suggestions?

main.c

#include <stdio.h>
#include <unistd.h>
#include <sys/wait.h>
#include <errno.h>
#include "common.h"

int main(int argc, const char * argv[]) {

    int exit_status_raw;

    pid_t pid = fork();

    if (pid < 0) {
        printerr("Forking failed");
        return -1;
    }

    if (pid == 0) {
        printf("CHILD says Hello\n");

        execlp("./child", "./child", NULL);

        printerr("exec failed");
        return -1;
    }

    if (pid > 0) {
        printf("PARENT says Hello\n");

        while ((pid = waitpid(pid, &exit_status_raw, 0)) == -1) {
            switch (errno) {
                case EINTR:
                    printerr("waitpid failed with EINTR. Retrying...");
                    continue;
                default:
                    printerr("waitpid failed");
                    break;
            }
        }

        printf("Child exited with %d\n", WEXITSTATUS(exit_status_raw));
    }

    return 0;
}

child.c

#include <stdio.h>
#include "common.h"

int main(int argc, const char * argv[]) {
    printf("This is a time consuming computation\n");
    // ...
    printerr("Sorry. I failed to compute");
    return 127;
}

common.h

#ifndef my_common_h
#define my_common_h

void printerr(const char *message) {
    fprintf(stderr, "%s\n", message);
}

#endif

You could consider terminating with the exit codes from <sysexits.h>, especially if the client is expecting them.  For instance, if fork() fails, return or call exit() with EX_OSERR.
It may be helpful to make the 127 error code a macro with a relevant name, since it's probably not a very commonly-known error code, especially returned from main().  I assume it's being used correctly (based on this post).
It would look clearer to put execlp()'s return value in a conditional, rather than always printing an error afterwards.  Since the exec functions only return a value (-1) on error, have the error message displayed only if this value gets returned.  Of course, have errno printed out as well.
You should also check to see if the child process has terminated from a signal.  This can be done by calling WTERMSIG() and displaying its returned signal value.  That can be determined by first checking the return value of WIFEXITED() (before doing the other checks).

Example:

int status;

// do checks with waitpid()

if (WIFEXITED(status))
{
    fprintf(stderr, "the process exited with code %d\n", status);
    exit(WEXITSTATUS(status));
}
else
{
    fprintf(stderr, "the process died with signal %d\n", WTERMSIG(status));
    exit(EX_OSERR);
}

I don't see a need for printerr() when you can just use perror(), which is already provided in <stdio.h> and does more than your function.  Regardless, it's a little redundant to put "failed" into the argument.  Clients receiving such a message should already be able to see that it's some kind of error, and it's automatically printed to stderr.
Instead of just having default provide an ambiguous error message, have it display the current value of errno with perror().  Again, your function doesn't do very much.