Proper practice using enums in header files Long time coder, but very very new to C++. I'm trying to put together my first real-sized project and figuring out what I need to change about my script-kiddie style of C++ to get there. Below is my working code.

#include <math.h>
#include <vector>
#include <iostream>
#include <stdexcept>

enum class Metric { Cosine, Euclidean};

double cosine(std::vector<double> A, std::vector<double> B, bool similarity){
    double mul = 0.0;
    double d_a = 0.0;
    double d_b = 0.0;

    std::vector<double>::iterator A_iter = A.begin();
    std::vector<double>::iterator B_iter = B.begin();

    for ( ; A_iter != A.end(); A_iter++ , B_iter++ ) {
        mul += *A_iter * *B_iter;
        d_a += *A_iter * *A_iter;
        d_b += *B_iter * *B_iter;
    }

    if (d_a == 0.0f || d_b == 0.0f) {
        throw std::logic_error("Vectors must have a length greater than zero");
    }

    double cos_similarity = mul / sqrt(d_a * d_b);
    if (similarity == false) {
        return acos(cos_similarity) / M_PI;
    } else {
        return cos_similarity;
    }
}

double euclidean(std::vector<double> A, std::vector<double>B, bool similarity) {
    double sq_dist = 0.0;
    std::vector<double>::iterator A_iter = A.begin();
    std::vector<double>::iterator B_iter = B.begin();

    for ( ; A_iter != A.end(); A_iter++, B_iter++) {
        sq_dist += pow(*A_iter - *B_iter, 2);
    }
    if (similarity == false) {
        return sqrt(sq_dist);
    } else {
        return 1.0/sqrt(sq_dist);
    }
}

double distance(std::vector<double> A, std::vector<double> B, Metric metric, bool similarity = false) {

    if (A.size() != B.size()) {
        throw std::logic_error("A and B must be the same size!");
    }

    if (A.size() < 1) {
        throw std::logic_error("Empty vectors belong to no metric space");
    }
    switch (metric) {
        case Metric::Cosine:
            return cosine(A, B, similarity);
        case Metric::Euclidean:
            return euclidean(A, B, similarity);
    }
}

double similarity(std::vector<double> A, std::vector<double> B, Metric metric) {
    return distance(A, B, metric, true);
}

My first question is around exactly how I should construct the header file. Specifically curious about the right way to handle the enum. Secondly, any general feedback, things that I could clean up, cut out, etc... would be greatly appreciated.

Prefer C++ headers

If you #include <cmath> instead of <math.h>, the mathematical functions will be safely and unambiguously in the std namespace.

Pass read-only parameters by const reference

Instead of copying the input vectors, it's usually more efficient to pass const references to them:

double cosine(const std::vector<double>& A, const std::vector<double>& B, bool similarity);

double euclidean(const std::vector<double>& A, const std::vector<double>& B, bool similarity);

double distance(const std::vector<double>& A, const std::vector<double>& B,
                Metric metric, bool similarity = false);

Use auto to deduce types

Instead of writing

std::vector<double>::iterator A_iter = A.begin();
std::vector<double>::iterator B_iter = B.begin();

We can ask the compiler to deduce the correct type:

auto A_iter = A.begin();
auto B_iter = B.begin();

This also makes it easier if you ever want to change the type of A and B (perhaps even to a template argument type).  In fact, by changing to const references, we already changed the type (to std::vector<double>::const_iterator), but auto isolates us from that.

Use the right kind of exception

You could be more specific, and use std::invalid_argument for the error type here:

if (d_a == 0.0f || d_b == 0.0f) {
    throw std::invalid_argument("Vectors must have a magnitude greater than zero");
}

I've changed the wording above, because length usually means the number of elements when we're using standard collections.

Don't compare booleans against false

This is hard to read:

if (similarity == false) {
    // negative case
} else {
    // positive case
}

You can re-write it as

if (similarity) {
    // positive case
} else {
    // negative case
}

This will normally produce identical code, but the cognitive overhead is reduced.

Avoid non-standard extensions

Although many implementations define a constant called M_PI, this isn't specified by any standard.  Thankfully it's easy to make your own value, using the knowledge that tan(¼π) is 1:

static const auto pi = 4*std::atan(1);

Avoid std::pow(x, 2)

To square a number, it's simpler and more efficient to multiply it by itself.  pow() is very general (handling non-integer powers), and usually works by multiplication of logarithms.  It's worth introducing a temporary to avoid that:

    //sq_dist += pow(*A_iter - *B_iter, 2);
    auto dist = *A_iter - *B_iter;
    sq_dist += dist * dist;

Don't fall off the end of non-void functions

Here, we're assuming that we're always passed a valid enum constant:

switch (metric) {
    case Metric::Cosine:
        return cosine(A, B, similarity);
    case Metric::Euclidean:
        return euclidean(A, B, similarity);
}
// implicit return

Robust code checks that we don't flow out of the switch:

switch (metric) {
    case Metric::Cosine:
        return cosine(A, B, similarity);
    case Metric::Euclidean:
        return euclidean(A, B, similarity);
}

throw std::invalid_argument("Invalid metric");

Invalid enum values shouldn't happen, but it costs little to be defensive against a mistaken cast.

Design issues

It's not clear whether cosine() and euclidean() are intended to be part of the public interface.  If they are not, then give them internal linkage (e.g. by use of the anonymous namespace, or with the static keyword), and consider moving the similarity parameter out of them.

I prefer to avoid booleans that change the meaning of functions like that; see if you agree when you see my full example.

Here's a full version with the above issues all addressed (and assuming that cosine() and euclidean() are intended to be internal detail).

Declarations

This is what I'd put in the header file (let's call it similarity.h):

#ifndef SIMILARITY_H
#define SIMILARITY_H

#include <vector>

enum class Metric { Cosine, Euclidean };

double distance(const std::vector<double>&, const std::vector<double>&, Metric);
double similarity(const std::vector<double>&, const std::vector<double>&, Metric);

#endif

Implementation

Having included the header, here's the C++ file:

#include "similarity.h"

#include <cmath>
#include <stdexcept>

namespace {

    double cosine_distance(const std::vector<double>& a, const std::vector<double>& b)
    {
        double mul = 0.0;
        double d_a = 0.0;
        double d_b = 0.0;

        auto ia = a.begin();
        auto ib = b.begin();

        for (;  ia != a.end();  ia++, ib++) {
            mul += *ia * *ib;
            d_a += *ia * *ia;
            d_b += *ib * *ib;
        }

        if (d_a == 0.0f || d_b == 0.0f) {
            throw std::invalid_argument("Vectors must have a magnitude greater than zero");
        }

        return mul / std::sqrt(d_a * d_b);
    }

    double euclidean_distance(const std::vector<double>& a, const std::vector<double>& b)
    {
        double sq_dist = 0.0;
        auto ia = a.begin();
        auto ib = b.begin();

        for (;  ia != a.end();  ia++, ib++) {
            auto dist = *ia - *ib;
            sq_dist += dist * dist;
        }

        return std::sqrt(sq_dist);
    }

}

double distance(const std::vector<double>& a, const std::vector<double>& b, Metric metric)
{
    if (a.size() != b.size())
        throw std::invalid_argument("a and b must be the same size!");
    if (a.size() < 1)
        throw std::invalid_argument("Empty vectors belong to no metric space");

    switch (metric) {
    case Metric::Cosine:
        return cosine_distance(a, b);
    case Metric::Euclidean:
        return euclidean_distance(a, b);
    }

    throw std::invalid_argument("Invalid metric");
}

double similarity(const std::vector<double>& a, const std::vector<double>& b, Metric metric)
{
    static const auto pi = 4*std::atan(1);

    switch (metric) {
    case Metric::Cosine:
        return std::acos(distance(a, b, metric)) / pi;
    case Metric::Euclidean:
        return 1.0 / distance(a, b, metric);
    }

    throw std::invalid_argument("Invalid metric");
}

Note that I include our own header first.  That's a helpful approach, to improve our chances of discovering if it accidentally depends on something it doesn't explicitly include.


Create multi-dim array from cursor I'm executing a cursor. I have cut off the code how the procedure is called and executed. This part is efficient. At last I have a small cursor. I'm calling the procedure, which returns this cursor many times on the page and I need to create a multidimensional array from it.

This array should look like the following:

$ret = oci_execute($outrefc) ;
while ($row = @oci_fetch_array($outrefc))
{
        foreach (array_keys($row) as $key)
        {

                 $res[$i][$key] = $row[$key];

        }
        $i++;
}

I have also tried like this:

  oci_fetch_all($outrefc, $res, null, null, OCI_FETCHSTATEMENT_BY_ROW+OCI_NUM);

Is there any way to make the upper snippet faster? The multidimensional array should stay as it is. I only wonder if I could create it in any more efficient way.

DB query:

select code, texttext, id, text
  from text,
       lang,
       client,
       cls,
       (select id, order, texttext text
          from sgr, text, lang
         where id = textid
           and textlngid = lngid
           and lngcode in ('en'))
 where 1 = 1
   and textcliid = cliid
   and textlngid = lngid
   and textclsid = clsid
   and sgrid(+) = clssgrid
   and nvl(showheadlinefrom, to_date('03.07.2016', 'dd.mm.yyyy')) <=
       to_date('03.07.2016', 'dd.mm.yyyy')
   and nvl(showheadlinetill, to_date('03.07.2016', 'dd.mm.yyyy')) >=
       to_date('03.07.2016', 'dd.mm.yyyy')
   and flgshowheadline in ('J')
   and lngcode in ('en')
   and clicode in ('mycode')
 order by order;

You are asking if you can speed up the assignment into the array?
I don't think you will be able to get a notable speed improvement on this part no. This should be completely negligible compared to the data retrieval time.
What would give you a notable speed improvement is not using a cursor and doing this in a set based manner... however we would need to see your query to attempt to suggest something for this.


School grading string calculator For my training, i've been given an assignment to build a simple, extendable 'grade' String calculator by a given grade.

For now, I had to work by this table:

When the grade was higher then 100, I had to return the String "You must have been cheating".

What I currently came up with is:

public String gradeBy(int grade) {
    TreeMap<Boolean, Grade> grades = new TreeMap<>();
    grades.put(grade <= 50, Grade.F);
    grades.put(grade >= 51 && grade <= 60, Grade.D);
    grades.put(grade >= 61 && grade <= 70, Grade.C);
    grades.put(grade >= 71 && grade <= 80, Grade.B);
    grades.put(grade > 80 && grade <= 100, Grade.A);

    for (Map.Entry<Boolean, Grade> currentGrade : grades.entrySet()) {
        if (currentGrade.getKey() != true) {
            continue;
        }
        return currentGrade.getValue().name();
    }
    return "You must have been cheating";
}

enum Grade {
    F,
    D,
    C,
    B,
    A
}

Where I test it as:

@Test
public void gradeBy() throws Exception {
    int[] grades = {45, 52, 66, 93, 102};
    for(int grade : grades) {
        String gradeString = this.grading.gradeBy(grade);
        System.out.println(grade + "gives :"+ gradeString);
    }
}

Can this be improved in a more OOP way? Can the grading list be apart from the grading system it self?

What stands out to me is that you initially perform a check against every grade range and insert true into the map in the right one. By that point, you've already found which grade is right. But then you initiate another loop to start searching for the grade corresponding to true.

I think instead, you ought to alter your method to output a Grade enum, and delegate the string handling to another function. I feel that it's better practice to return a Grade Enum from the first method because of the strong typing-- you may want to use this information for other methods besides finding the grade name. In fact, Enums are particularly nice because you can use a switch on them.

public Grade gradeBy(int grade) {
    if (grade < 50) return Grade.F;
    if (grade < 61) return Grade.D;
    if (grade < 71) return Grade.C;
    if (grade < 81) return Grade.B;
    if (grade<=100) return Grade.A;
    return Grade.F;
}
public String gradeName(Grade g) {
    return g.name()
}

Notice how as soon as the grade matches the range corresponding to something in the Grade Enum, the method immediately returns. Now an example of how you might use that switch:

public String gradeComment(Grade g) {
    switch(g) {
        case F:
        return "You need to work harder!";
        case D:
        return "I think you can do better.";
        case C:
        return "Satisfactory.";
        case B:
        return "Nice job.";
        case A:
        return "Well done!";
        default:
        return "N/A";
    }
}

Not that you need these grade messages, but that's an example of the power of holding on to the Enum type rather than converting the int grade directly into a String.

Sorry if I've made syntax errors as I've not used Java in a while.

Edit: Just realised that you might like to put gradeBy, gradeName and gradeComment into the Grade Enum itself as static methods.


Else if .value = 1 to 92 code check I've written code that selects column D and checks it for codes 1 to 92 however I have to write out c4.Value = 1 Or c4.Value = 2 all the way up to c4.Value = 92, is there a quicker way of doing this? Please see below for my code and I appreciate all help.

Sub q2country_and_q2country_other()

   Dim i As Long, c4 As Range, c5 As Range

   For i = 2 To 456
      Set c4 = Range("D" & i)
      Set c5 = Range("E" & i)

  If c4.Value = 94 Then
      Select Case c5.Value
      Case "", " ", "0", -99, -66, -77
         c4.Interior.color = vbRed
         c5.Interior.color = vbRed
      Case Else
         c4.Interior.color = vbGreen
         c5.Interior.color = vbGreen
   End Select

  ElseIf c4.Value = 1 Or c4.Value = 92 Then
      Select Case c5.Value
      Case -99, ""
         c4.Interior.color = vbGreen
         c5.Interior.color = vbGreen
      Case Else
         c4.Interior.color = vbRed
         c5.Interior.color = vbRed
      End Select

End If
Next i

End Sub

You can only use the To syntax with Case statements inside a Select block, and you can nest Select Blocks....

Note that indentation helps enormously, so I've changed the indentation to 2 spaces, although 4 is more typical, but doesn't always suit StackExchange rendering.

Sub q2country_and_q2country_other()

   Dim i As Long, c4 As Range, c5 As Range

   For i = 2 To 456
      Set c4 = Range("D" & i)
      Set c5 = Range("E" & i)

      Select Case c4.Value
        Case 94
          Select Case c5.Value
            Case "", " ", "0", -99, -66, -77
              c4.Interior.color = vbRed
              c5.Interior.color = vbRed
            Case Else
              c4.Interior.color = vbGreen
              c5.Interior.color = vbGreen
          End Select

        Case 1 To 92
          Select Case c5.Value
            Case -99, ""
              c4.Interior.color = vbGreen
              c5.Interior.color = vbGreen
            Case Else
              c4.Interior.color = vbRed
              c5.Interior.color = vbRed
          End Select
      End Select
    Next i
End Sub


Manual sorting of database output I have a small (few elements) list of primary keys, sorted by hand. It come from user input. For example

$ids = [15, 75, 54, 13];

The SQL command is simple.

SELECT id, value, type FROM a_table WHERE id IN (15, 75, 45, 13);

This is SQL, so the order of output may be random. I want to
restore it in PHP. My first attempt.

# $ids - the input
# $result - output from database
function sort_to_input_1($ids, $result)
{
    $plain = array_map(function($a){return $a['id'];}, $result);
    $flipped = array_flip($plain);
    $sorted = array();
    foreach($ids as $id) {
        if (isset($flipped[$id])) {
            $idx = $flipped[$id];
            $sorted[] = $result[$idx];
        }
    }
    return $sorted;
}

The second, the brute force.

# $ids - the input
# $result - output from database
function sort_to_input_2($ids, $result)
{
    $sorted = array();
    foreach($ids as $id) {
        foreach($result as $row) {
            if ($row['id'] == $id)) {
                $sorted[] = $row;
            }
        }
    }
    return $sorted;
}

The third one.

# $ids - the input
# $result - output from database
function sort_to_input_3($ids, $result)
    $result_h = array();
    foreach($result as $row) {
        $id = $row['id'];
        $result_h[$id] = $row;
    }
    $sorted = array();
    foreach($ids as $id) {
        if (isset($result_h[$id])) {
            $sorted[] = $result_h[$id];
        }
    }
    return $sorted;
}

Witch one is the easiest to read? Or is there a fourth way?

While technically this isn't reviewing your code, IMO this is the better solution: Do sort via MySQL

SELECT id, value, type
FROM a_table
WHERE id IN (15, 75, 45, 13);
ORDER BY FIELD(id, 15, 75, 45, 13)

PHP isn't the fastest language and should therefor not be used for sorting/ordering sets of data if other solutions are present. Looping in PHP is slow (but sometimes negligible, like the while(fetch)). MySQL, in this case, is a whole lot faster then PHP.

At the same time, this is more readable (IMO) and better to maintain.


please check my user session login system and tell me what can be done to make it better There can be some errors in the script, if u find them thanks, but the main thing is to get useful suggestions about how can I do this better and secure.

ini_set('session.use_only_cookies', true);
session_start();
//Create a random salt value
$salt = 'Hjkhkjh9089&j98098';
$tokenstr = (str) date('W') . $salt;
//Create a md5 hash to be used for token.
$token = md5($tokenstr);

///checking if already logged in
if ($_REQUEST['token'] == $token AND $_SESSION['user_ip']=$_SERVER['REMOTE_ADDR'] ) {
   ...grant user with some data
}

//user login block
if (($_POST['login']!="") OR ($_COOKIE["username"]!='' AND $_COOKIE["password"]!='') AND $_SESSION['user']=='')
{

    if ($_SESSION['temp_ban']==1)
    {

    $p='login_error';
    }
        else
        {
        $ip=$_SERVER['REMOTE_ADDR'];
        $result2=  mysql_query("SELECT * FROM banned_ip_list WHERE banned_ip='$ip'") or die("unable to connect to msql server: " . msql_error()) ;
        $row2 = mysql_fetch_array($result2);
            if (!$row2=="")
            {
            $_SESSION['temp_ban']=1;
            $p='login_error';
            }
                else
                {
                    if ($_COOKIE["username"]!='' AND $_COOKIE["password"]!='')
                    {
                    $login_username=security2($_COOKIE["username"]);
                    $login_password=security2($_COOKIE["password"]);
                    $remember_me='on';
                    }
                    else
                    {
                    $login_username=security2($_POST['username']);
                    $login_password=security2($_POST['password']);
                    $remember_me=security2($_POST['remember_me']);
                    }
                    $result = mysql_query("SELECT * FROM users WHERE    username='$login_username' AND password=SHA1('$login_password')") or die("unable to connect to msql server: " . msql_error()) ;

                    $row = mysql_fetch_array($result);
                        if ($row!="")
                        {
                            if($remember_me!='')
                            {
                            $expire=time()+60*60*24*7;
                            setcookie("username", $login_username , $expire);
                            setcookie("password", $login_password , $expire);
                            }

                        $_SESSION['user_ip']=$_SERVER['REMOTE_ADDR'];
$_SESSION['user_id']=$row['id'];
$_SESSION['user']=$row['username'];
$_SESSION['ch_insert_temp_key']=1;
$_SESSION['token'] = $token;
output_add_rewrite_var('token', $token);

$_POST['login']='';
unset($_SESSION['try_error']);
                        }
                            else
                            {
                                if (!$_SESSION['try_error'] AND !$_SESSION['temp_ban'])
                                {
                                $_SESSION['try_error']=11;
                                }
                                if ($_SESSION['try_error']-1==1)
                                {
                                $_SESSION['temp_ban']=1;
                                $ip=$_SERVER['REMOTE_ADDR'];
                                $banned_time=date('Y-m-d H:i:s');

                                $result3 = mysql_query("INSERT INTO banned_ip_list (banned_ip,banned_time) VALUES ('$ip','$banned_time')") or die("unable to connect to msql server: " . msql_error()) ;
                                /////////////log
                                $event_type_id=10;
                                $additional_info='';
                                $event_object='';

                                include $log_entry;
                                //////////////////
                                }
                                else {$_SESSION['try_error']--;}
                            $p='login_error';
                            }

                }
        }

}

Let's start with the SQL injection vulnerability in your code in

mysql_query("SELECT * FROM users WHERE    username='$login_username' AND password=SHA1('$login_password')") or die("unable to connect to msql server: " . msql_error()) ;

In this piece of code you put the user defined $login_username straight to a SQL query. I see that there's a function called security2 that does something to username but I cannot be sure that it executes mysql_real_escape_string that would prevent SQL injection. If it does not, then a malevolent user might be tempted to enter a user name that may corrupt your database.

Poor Bobby Tables might cause problems here .

Rest of the thoughts are (at least at first) in random order

Uou should probably migrate from mysql_ to mysqli_ functions. mysqli is the MySQL Improved Extension for PHP that is supposed to work better and do much more.

In PHP, it's almost always better to use === than == to test equality.

You could try to be consistent with the use of ''and "".

The code would benefit if you would define more functions to partition the logic. Maybe even define some classes.


Building a matrix by applying XOR rules There is a matrix of \$m\$ rows and \$n\$ columns where each row is filled gradually. Given the first row of the matrix we can generate the elements in the subsequent rows using the formula:

$$\begin{align}
a_{i,j} =&\ a_{i-1,j} \oplus a_{i-1,j+1}\quad\forall j:0\le j\le n-2 \\
a_{i,n-1} =&\ a_{i-1,n-1} \oplus a_{i-1,0}
\end{align}$$

Each row is generated one by one, from the second row through the last row. Given the first row of the matrix, find and print the elements of the last row as a single line of space-separated integers.

For example, input as \$4 \space 2\$ (4 is the number of columns and 2 is the row which we are supposed to find):

\$6 \space 7 \space 1 \space 3\$ (1st row input)
6^7 = 1
7^1 = 6
1^3 = 2
3^6 = 5

\$1 \space 6 \space 2 \space 5\$ are the final row output. Now, how could I optimise my program if the value of \$n\$ is like pow(10,5) and \$m\$ is like pow(10,18)?

import java.util.Scanner;
class XorMatrixMain{
    public static void Xor_Array(int[] xor, int n){
        int num = xor[0];
        boolean bool = false;
        int last = 0;
        for(int j=0;j<n-1;j++){
            for(int i=0 ; i<xor.length ; i++){
                if(i<xor.length-1){
                    xor[i] = xor[i]^xor[i+1];
                }
                if(i==xor.length-1){
                    if(bool){
                        xor[i] = xor[i]^last;
                    }
                    else{
                        xor[i] = xor[i]^num;
                        bool = true;
                    }
                }
            }
            last = xor[0];
        }
        for(int i=0;i<xor.length;i++){
            System.out.print(xor[i]+" ");
        }
    }
    public static void main(String[] args){
        Scanner scan = new Scanner(System.in);
        int m = scan.nextInt();
        int n = scan.nextInt();
        int[] xor = new int[m];
        for(int i=0;i<m;i++){
            xor[i] = scan.nextInt();
        }
        Xor_Array(xor,n);
    }
}

Time complexity is \$O(nm)\$. Obviously it is bound to TLE. There is an \$O(n)\$ solution, based on the following observations:

\$ x \oplus x = 0\$
\$ x \oplus y = y \oplus x\$

Let's say your initial row is a b c d e f. The next row will be a^f b^a c^b d^c e^d f^e. The second next is more interesting: the first element is a^f ^ f^e = a^e, the second is a^f ^ b^a = b^f, etc.

In general (prove it by induction), \$k\$'th row consists of \$a_i \oplus a_{i-k}\$ (where the last index is taken modulo n). Notice that the \$n\$'th row is all zeroes, and so are all the subsequent rows.
The code is extremely hard to follow. num is only used once - may be not have it at all?

    last = xor[0];

    for(int i=0 ; i<xor.length ; i++){
        if(i<xor.length-1){
            xor[i] = xor[i]^xor[i+1];
        }
        if(i==xor.length-1){
            xor[i] = xor[i]^last;
        }
    }

Notice that i == xor.length - 1 is true exactly once, at the predictable i, yet you test it at each iteration. Lift it out of the loop:

    last = xor[0];
    for(int i=0 ; i<xor.length - 1; i++){
        xor[i] = xor[i]^xor[i+1];
    }
    xor[i] = xor[i]^last;


Stream operation with two dimensional array I am trying to find the best average score from below two dimensional array:

String[][] scores = { { "Amit", "70" }, { "Arthit", "60" }, { "Peter", "60" }, { "Arthit", "100" } };

The output is: 80 (Arthit's score (60+100)/2)

Till now I solved this problem with below approach, however I am looking for elegant solution with stream:

public static void main(String[] args) {
        String[][] scores = { { "Amit", "70" }, { "Arthit", "60" }, { "Peter", "60" }, { "Arthit", "100" } };

        int highestAvg = Integer.MIN_VALUE;
        Function<String[], Integer> function = new Function<String[], Integer>() {
            @Override
            public Integer apply(String[] t) {
                int sum = 0, count = 0;
                for (int i = 0; i < scores.length; i++) {
                    if (t[0].equals(scores[i][0])) {
                        count++;
                        sum += Integer.parseInt(scores[i][1]);
                    }
                }
                int avg = sum / count;
                return highestAvg < avg ? avg : highestAvg;
            }
        };
        System.out.println(Arrays.stream(scores).map(function).max((o1, o2) -> o1.compareTo(o2)).get());
    }

Would you please suggest, what's the better approach to handle two dimensional array using stream?

Note: I am not looking the exact solution, just looking your valuable suggestion.

Apologies for providing a full solution, I know you didn't ask for that, it was just easier for me to work through it by writing the code in full.

I'm initially approaching about by thinking about how would I solve this in a database using SQL, and then applying a similar approach in Java.

First thing to do here is group all the scores together by person. I'm going to end up with Map of some kind, keyed by the person. That means using the groupingBy collector. I need to provide a separate function for the key and the value. The value mapping is the interesting bit here, I was initially thinking I could just provide a list of the scores and average them in the next step, but then I stumbled across the averagingInt collector which saves me a step.

So now I have a map with each person's average score in. I don't need the names anymore, as I only need to return the score, so I'll just take the values() from the map and find the biggest. Keeping it streamy means using the max() method to get the result.

This seems to do the trick, and isn't too ugly.

public static void main(String[] args) {

     String[][] scores = { { "Amit", "70" }, { "Arthit", "60" }, { "Peter", "60" }, { "Arthit", "100" } };

     double highestAvg = Arrays.stream(scores)
           .collect(groupingBy(s -> s[0],
                               averagingInt(s -> parseInt(s[1]))))
           .values()
           .stream()
           .max(naturalOrder())
           .get();

     System.out.println(highestAvg)
}

It uses these static imports to keep things uncluttered

import static java.lang.Integer.parseInt;
import static java.util.Comparator.naturalOrder;
import static java.util.stream.Collectors.averagingInt;
import static java.util.stream.Collectors.groupingBy;

Note that I changed the average to a double because it saves another conversion and the result might need the decimals.

Something to think about - how could you change the data structure to clean this up further?


Project Euler Problem 11: Largest product in a grid Here is my implementation to project Euler Problem 11. I did this problem a bit later, when I learned how to input from a txt file. The problem context can be seen here. I did add "\n" to the end of each line, after copying the numbers into a txt file, I'm not sure of any other way to do that. Any other improvements anyone can think of?

#include <fstream>
#include <iostream>
#include <vector>
#include <sstream>
#include <string>

//splitting stream into ints.
std::vector<int> split(std::string line){
    std::stringstream ss (line);
    std::vector<int> result;
    std::string num;
    while(std::getline(ss, num, ' '))
        result.push_back(std::stoi(num));
    return result;
}
//comparing all int's that are horizontally next to each other
unsigned long long Horizontal(int a, int b, std::vector<std::vector<int>> grid){
    if(b < grid[a].size()-3){
        return (grid[a][b] * grid[a][b+1] * grid[a][b+2] * grid[a][b+3]);
    }
}
//comparing all int's that are vertically next to each other
unsigned long long Vertical(int a, int  b, std::vector<std::vector<int>> grid){
    if(a < grid.size()-3){
        return (grid[a][b] * grid[a+1][b] * grid[a+2][b] * grid[a+3][b]);
    }
}
//all int's that are diagonally(forward) next to each other
unsigned long long ForDiag(int a, int b, std::vector<std::vector<int>>grid){
    if (a < grid.size()-3 && b < grid[a].size()-3){
        return (grid[a][b] * grid[a+1][b+1] * grid[a+2][b+2] * grid[a+3][b+3]);
    }
}
//all int's that are diagonally(backward) next to each outher
unsigned long long BackDiag(int a, int b, std::vector<std::vector<int>>grid){
    b+=3; // b needs to be 3 larger for this function
    if(a < grid.size()-3 && b < grid[a].size()){
        return (grid[a][b] * grid[a+1][b-1] * grid[a+2][b-2] * grid[a+3][b-3]);
    }
}

//Calls the calculation functions, and compares them for the largest.
unsigned long long Largest(std::vector<std::vector<int>> grid ){
    int gwidth = grid[0].size();
    int glength = grid.size();
    unsigned long long largest = 0;

    for(int a = 0; a < gwidth; ++a){
        for(int b = 0; b < glength; ++b){
            largest = std::max(largest,Horizontal(a,b,grid));
            largest = std::max(largest,Vertical(a,b,grid));
            largest = std::max(largest,ForDiag(a,b,grid));
            largest = std::max(largest,BackDiag(a,b,grid));

        }
    }
    return largest;
}

int main(){
    std::vector<std::vector<int>> grid;

    //opening file.
    std::ifstream nums;
    nums.open("grid.txt");
    std::string row;

    //Calling function, and pushing back into the vector
    while(std::getline(nums, row, '\n')){
        grid.push_back(split(row));
    }

    std::cout << Largest(grid);

}

Your functions returning a unsigned long long are missing a return which leads to undefined behavior (more details on this question). You could throw an exception to handle this or you could just return 0.

Not a real issue and mostly a matter of personal preference but the way you check that you are not going out of the bounds of the array could be written in a more natural way.

To check that index b + 3 is in the array, I'd rather read b + 3 < a.size() than b < a.size() - 3.

Even more unusual is the pre-increment in the case of BackDiag() : you add 3 to b and then you consider b - 3.

Once your code re-written to take into account these comments, it looks like :

unsigned long long Horizontal(int a, int b, std::vector<std::vector<int>> grid){
    return (b + 3 < grid[a].size()) ?
        (grid[a][b] * grid[a][b+1] * grid[a][b+2] * grid[a][b+3]) :
        0;
}
//comparing all int's that are vertically next to each other
unsigned long long Vertical(int a, int  b, std::vector<std::vector<int>> grid){
    return (a + 3 < grid.size()) ?
        (grid[a][b] * grid[a+1][b] * grid[a+2][b] * grid[a+3][b]) :
        0;
}
unsigned long long ForDiag(int a, int b, std::vector<std::vector<int>>grid){
    return (a + 3 < grid.size() && b + 3 < grid[a].size()) ?
        (grid[a][b] * grid[a+1][b+1] * grid[a+2][b+2] * grid[a+3][b+3]) :
        0;
}
unsigned long long BackDiag(int a, int b, std::vector<std::vector<int>>grid){
    return (a + 3 < grid.size() && b + 3 < grid[a].size()) ?
        (grid[a][b+3] * grid[a+1][b+2] * grid[a+2][b+1] * grid[a+3][b]) :
        0;
}

Now, one thing to notice is that this good is basically always the same. You should try to write a generic function that you can reuse.

The signature would be something like :

unsigned long long computeProduct(int a, int b, int incrA, int incrB, std::vector<std::vector<int>>grid)


Dijkstra algorithm implementation with adjacency list I've implemented the Dijkstra Algorithm to obtain the minimum paths between a source node and every other.

#include <bits/stdc++.h>

typedef std::vector<std::list<int>> AdjacencyList;
typedef std::vector<std::vector<int>> Weights;

std::vector<int> dijkstra(AdjacencyList &adj, Weights weights, int src) {
    int n = adj.size();

    std::vector<int> prev(n, -1);
    std::vector<int> dist (n, INT_MAX);
    std::vector<bool> visited(n, false);

    auto comp = [&dist](const int& lhs, const int& rhs) -> bool { return dist[lhs] > dist[rhs]; };
    std::priority_queue<int, std::vector<int>, decltype(comp)> candidates(comp);

    candidates.push(src);
    dist[src] = 0;
    for (int j = 0; j < n - 1; ++j) {
        int next = candidates.top();
        candidates.pop();
        visited[next] = true;
        for(std::list<int>::const_iterator it = adj[next].begin(); it != adj[next].end(); it++) {
            int neighbor = *it;
            if (!visited[neighbor] && (dist[next] + weights[next][neighbor] < dist[neighbor])) {
                dist[neighbor] = dist[next] + weights[next][neighbor];
                prev[neighbor] = next;
                candidates.push(neighbor);
            }
        }
    }
    return prev;
}

I wrote this code to practice but also to be used at competition contests. So, i want this to be simple, clear and efficient at the same time (without losing so much readability). Any advices?

Advice 1

int n = adj.size();

I would change int to size_t, since it counts size that may not be negative.

Advice 2

for (int j = 0; j < n - 1; ++j) {
    ...
}

I would change it to

while (!candidates.empty()) {

}

This omits some unnecessary computation in case the graph is disconnected.

Advice 3

candidates may be renamed to open, open_list or frontier; that is more or less conventional naming for that data structure in pathfinding community.

Advice 4

I would change typedef std::vector<std::vector<int>> Weights; to typedef std::unordered_map<int, std::unordered_map<int, size_t>> Weights;. This makes filling the edge weights much more easier; especially on sparse graphs:

w[a][b1] = 3;
w[a][b2] = 1;
...

Advice 5

if (!visited[neighbor] && (dist[next] + weights[next][neighbor] < dist[neighbor])) {
    ...
}

This condition is a bug. It should be

if (dist[neighbor] == INT_MAX || (dist[next] + weights[next][neighbor] < dist[neighbor])) {
    ...
}

(Note dist[neighbor] == INT_MAX and || instead of &&. You can try changing || to && and see its effect here.)


Generate range of characters import string
def charRange(mini, maxi, step=1):
  if mini in string.ascii_uppercase:
    strg = string.ascii_uppercase
  else:
    strg = string.ascii_lowercase
  return (strg[i] for i in
          range(string.ascii_lowercase.index(mini.lower()),
                string.ascii_lowercase.index(maxi.lower())+1,
                step))

Examples of usage:

for i in charRange('A', 'Z'):
    print(i)
> A B C D E F G H I J K L M N O P Q R S T U V W X Y Z
for i in charRange('a', 'h'):
  print(i, end=' ')
> a b c d e f g h
for i in charRange('A', 'J', 2):
  print(i, end=' ')
> A C E G I

What if you would operate with character code ranges and use the ord() and chr() built-in functions instead:

def char_range(mini, maxi, step=1):
    for code in range(ord(mini), ord(maxi) + 1, step):
        yield chr(code)

Or, with the yield from (Python 3.3+):

def char_range(mini, maxi, step=1):
    yield from range(ord(mini), ord(maxi) + 1, step)

Demo:

$ ipython3 -i test.py
In [1]: for char in char_range('a', 'h'):
   ...:     print(char, end=' ')
   ...:
a b c d e f g h

Couple notes about the code style of your solution:

according to PEP8, there should be two blank lines after the imports
use the "lower_case_with_underscores" style to name the function - charRange() vs char_range() (PEP8 reference)


Simple Hangman game in Python3 I made a simple Hangman game in Python:
import random

class Hangman:

    def __init__(self):
        self.words = ["bike", "monkey", "planet"]
        self.random_word = random.choice(self.words)
        self.lenght = len(self.random_word)
        self.no_of_try = int(self.lenght) + 3
        self.empty_word = ["_"] * self.lenght

    @staticmethod
    def user_input():
        print("Enter a guess: ")
        user_input = input()
        return user_input

    def find_index(self):
        letter = self.user_input()
        possition = self.random_word.index(letter)
        return (letter, possition)

    def substitue(self):
        try:
            guess_letter, guess_index = self.find_index()
            self.empty_word[guess_index] = guess_letter
            print(self.empty_word)
            return True
        except ValueError as e:
            return False

    def play_game(self):
        while self.no_of_try > 0:
            print("no: " + str(self.no_of_try))
            game = self.substitue()
            if game:
                print("Bravo")
                self.no_of_try -=  1
            else:
                self.no_of_try -=  1
                print("Wrong")

            if "_" not in self.empty_word:
                print("You win")
                break
            if self.no_of_try == 0:
                print("Game over")
                break

if __name__ == "__main__":
    h = Hangman()
    h.play_game()

Right now the program will have a problem if you use a word which has the same letter more than once. string.index will only return the index of the first occurrence. What you could do instead is use a set to check what letters still need to be guessed. If a character in the word is still in the set, it hasn’t been guessed and an underscore should be printed. If it isn’t in the set, it has been guessed and it should be printed.
The code can be simplified significantly. You only really need one function which takes a list of strings as an argument. You could do something like this:
import random

def hangman(word_list):
    word = random.choice(word_list)
    letters = set(word)
    guesses = len(word) + 3
    while len(letters) > 0 and guesses > 0:
        guess = input(str(guesses) + " guesses left\nguess something:\n")
        if guess in letters:
            print("correct")
            letters.remove(guess)
        else:
            print("wrong")
        guesses -= 1
        print(*(ch if ch not in letters else '_' for ch in word))
    print("win" if len(letters) == 0 else "lose")

if __name__ == "__main__":
    hangman(["bike", "monkey", "planet", "mississippi"])

NITPICKING
You have a function in your current code which could be replaced with a call to input. the user_input function could be replaced with this: input("Enter a guess:\n"). Also you can simplify this:
if game:
    print("Bravo")
    self.no_of_try -=  1
else:
    self.no_of_try -=  1
    print("Wrong")

into this:
self.no_of_try -=  1
print("Bravo" if game else "Wrong")

I also noticed a couple things in the __init__ method. The word list and the randomly chosen word’s length don’t need to be instance variables because they’re only used in __init__, and you don’t need to cast the length to int because it’s already an int.


Responsive left fly-out menu I am working on this website currently.

If you resize the window to less than 650px then the main menu is replaced by a button which when pressed triggers a left fly-out menu similar to the Facebook uses on the mobile website.

When viewed on a computer the menu works very well, but on mobile devices the performance is not so good and the slide-in effect is not smooth.  I need help with the performance.

      content Goes Here

jQuery:

jQuery(document).ready(function($) {
     var mobNav    = $("#mobNav"),
         mainNav   = $("#mainnav"),
         top       = $("#top"),
         shift     = $("#shift");
     // add navigation items to mobile nav
     mobNav.html(mainNav.html());

     $(window).on("scroll", function() {
        if($(window).scrollTop() > 300){
          top.fadeIn();
        } else{
          top.fadeOut();
        }
        if(parseInt(mobNav.css("left"), 10) == 0){
          // if is visible then hide
          mobNav.css("left", -mobNav.outerWidth());
          shift.css("margin-left", "0");
        }
      });

      $(window).on("resize", function() {
        if(parseInt(mobNav.css("left"), 10) == 0){
          // if is visible then hide
          mobNav.css("left", -mobNav.outerWidth());
          shift.css("margin-left", "0");
        }
      });
      // button that triggers mobile nav visibility
      $("#mob_nav_trigger").click(function(e) {
        e.preventDefault();
        var winWidth = $(window).width(),
            docHeight = $(document).height();
        mobNav.css("minHeight", docHeight).addClass("visible_now").animate({
            left: parseInt(mobNav.css('left'),10) == 0 ? -mobNav.outerWidth() : 0
        });
        shift.animate({
          marginLeft: parseInt(shift.css('margin-left'),10) == 200 ? 0 : 200
        }).css("minWidth", winWidth);
      });
});

Of course I'll be refactoring the code and take some code which is repeating and convert it into a function like:

    if(parseInt(mobNav.css("left"), 10) == 0){
      // if is visible then hide
      mobNav.css("left", -mobNav.outerWidth());
      shift.css("margin-left", "0");
    }

Notes
I gave it a shot, there isn't really much that I felt could be improved upon though. I'm sure performance would be better if you were using CSS3 transitions (the implementation certainly would be nicer <3 CSS3) but I assume you wanted better browser coverage.

Ideally put your CSS in a separate file to keep your markup clean.

I put a function around your code so that the symbols can be properly minified.

I introduced a menuVisible variable which will be false if the menu is not showing and not in transition. The reason I did this was to simplify the conditions on scroll, resize and button click events that run every time.

Fluent syntax for $(window) when hooking up events, saves converting window into a jQuery object twice.

Refactored a little so the functions are better split up and to clean up document ready.

Something I noticed when playing with your menu from the link you gave, it's actually not very smooth at all on my desktop if the menu is transitioning at the same time as your slideshow. I suggest you halt the animation on your slideshow when the button is pressed, that will probably make the biggest difference to how smooth the page is.
Alternatively you could disable the slideshow all together or remove the animation when it's in the mobile media query, scrolling may suffer as a result of the slideshow animation as well.

Code
HTML

    content Goes Here

CSS
#mobNav {
    background: url(images/top_bg.jpg) repeat;
    background-size: 100%;
    position:absolute;
    left:-200px;
    width:200px;
    height:100%;
}

JS
(function ($, window, document) {
    var mobNav, mainNav, top, shift;
    var menuVisible = false;

    $(document).ready(function($) {
        mobNav      = $("#mobNav");
        mainNav     = $("#mainnav");
        top         = $("#top");
        shift       = $("#shift");

        mobNav.html(mainNav.html());

        $(window).on("scroll", scroll);
                 .on("resize", hideMenu);
        $("#mob_nav_trigger").click(function(e) {
            e.preventDefault();
            showMenu();
        });
    });

    function scroll() {
        if ($(window).scrollTop() > 300)
            top.fadeIn();
        else
            top.fadeOut();
        hideMenu();
    }

    function showMenu() {
        var winWidth = $(window).width(),
            docHeight = $(document).height();
        mobNav.css("minHeight", docHeight)
              .addClass("visible_now")
              .animate({
                  left: menuVisible ? -mobNav.outerWidth() : 0 },
                  function () {
                      if (menuVisible)
                          menuVisible = false;
              });
        shift.css("minWidth", winWidth)
             .animate({ marginLeft: menuVisible ? 0 : 200 });
        if (!menuVisible)
            menuVisible = true;
    }

    function hideMenu() {
        if (menuVisible) {
            mobNav.css("left", -mobNav.outerWidth());
            shift.css("margin-left", "0");
            menuVisible = false;
        }
    }
})($, window, document);


Asynchronous parallel ForEach implemented using Task.Run I wrote a simple implementation of aysnc parallel.ForEach( ... )

All it really does is create a list of tasks and wait for them all to complete and aggregate the exceptions if any.

But to use CancellationToken I added Task.Run but I am worried that this is not the best way of stopping a task from running.

I could add some shortcuts in case the token cannot be cancelled, empty collections and so on.

But those are minor tweaks.

Anything else I could do to make tasks.AddRange(source.Select(s => Task.Run(() => body(s), token))); more efficient while allowing me to cancel running tasks.

public static async Task ForEach<T>(ICollection<T> source, Func<T, Task> body, CancellationToken token )
{
  // create the list of tasks we will be running
  var tasks = new List<Task>(source.Count);
  try
  {
    // and add them all at once.
    tasks.AddRange(source.Select(s => Task.Run(() => body(s), token)));

    // execute it all.
    await Task.WhenAll(tasks.ToArray()).ConfigureAwait(false);

    // throw if we are done here.
    token.ThrowIfCancellationRequested();
  }
  catch
  {
    // find the error(s) that might have happened.
    var errors = tasks.Where(tt => tt.IsFaulted).Select(tu => tu.Exception).ToList();

    // we are back in our own thread
    if (errors.Count > 0)
    {
      throw new AggregateException(errors);
    }
  }
}

Any suggestions where I could improve performance and the creation of tasks?

Task.WhenAll also accepts Enumerable<Task> as argument. So your lines with tasks can be simplified:

var tasks = source.Select(x => Task.Run(() => body(s), token));
try
{
    await Task.WhenAll(tasks).ConfigureAwait(false);
    token.ThrowIfCancellationRequested();
}
catch
{
    // find the error(s) that might have happened.
    var errors = tasks.Where(tt => tt.IsFaulted).Select(tu => tu.Exception).ToList();

    // we are back in our own thread
    if (errors.Count > 0)
    {
        throw new AggregateException(errors);
    }
}

If you end up throwing because of the line token.ThrowIfCancellationRequested(), after all the tasks have completed (so none of the tasks themselves faulted), I think you end up swallowing the exception.

You provide the CancellationToken to Task.Run. The only thing this does is check whether the token has been cancelled before the tasks starts running. If you want to affect the task itself, you need to pass the token into the method body as well. Additionally, the implementation of body then needs to actually do something with the token (like checking whether it is cancelled), before cancelling will have any use.

So consider the following possible APIs:

Task ForEach<T>(ICollection<T> seq, Func<T, CancellationToken, Task> body, CancellationToken token)

or

Task ForEach<T>(ICollection<T> seq, Func<T, Task> body)

Not much in between will have much use, assuming it's the function body that you want to cancel.

For some additional reading, see here.


Random integer generation in given range Can my rand_int function be improved?

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

int rand_int(int a,int b)
{
    //The basic function here is like (rand() % range)+ Lower_bound

    srand(time(0));
    if (a > b)
        return((rand() % (a-b+1)) + b);
    else if (b > a)
        return((rand() % (b-a+1)) + a);
    else
        return a;
}

int main()
{
    int a,b;

    printf("\nEnter the range in which the a random no. is required: ");
    scanf("%d %d", &a, &b);

    printf("%d\n", rand_int(a,b));
}

First of all. Move srand() out of the function call. For now, it is not random at all if you manage to execute function twice in the same second. You will reset the random seed to the same value with time(0) as it normally has a 1s resolution.

Then I would suggest to do a conditional swap, and then perform "generate random" thing. This way you will split it into "preparation of arguments" and the "actual work parts".

You will have one exit point, which is a bit more readable and AFAIK tend to be a bit more optimizer friendly. Also, modern processors don't like too many ifs, especially unpredictable. So I would further suggest refactoring the "job" part into a separate function int rand_hi_lo(int upper, int lower) maybe even inlined. You could skip the branch at all then in certain cases.

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

void swap_int(int* a, int* b){ int tmp = *a; *a=*b; *b=tmp; }

inline int rand_int_hi_lo(int upper, int lower){
  return((rand() % (upper-lower+1)) + lower);
}

int rand_int(int a,int b)
{
    if (b > a) swap_int(&a,&b);
    return rand_int_hi_lo(a,b);
}

int main()
{
    int a,b;
    srand(time(0));
    printf("\nEnter the range in which the a random no. is required: ");
    scanf("%d %d", &a, &b);
    printf("%d\n", rand_int(a,b));
}

It might be even possible to skip the conditional at all. But I am not sure about the negative modulo arithmetics. I think it is possible. Also my c is rusty, so I hope I wrote swap correctly.

EDIT:
I also skipped the a==b. I don't think it is worth to put there at all unless you are sure it happens more than rarely. If necessary I would put if (a==b) return a; it before the swap_int.


VBA Excel substitute function for multiple conditions at once I use the SUBSTITUTE() function in my VBA code.
In Excel formula, we can nest it as shown in the thread below:
https://stackoverflow.com/questions/22313965/how-can-i-combine-multiple-nested-substitute-functions-in-excel/22314382
How can we do it in VBA Excel code?
How can I make my code smarter?
  For Each cell In rng
  cell = WorksheetFunction.Substitute(cell, " ", "  ", 25)
  cell = WorksheetFunction.Substitute(cell, " ", "  ", 24)
  cell = WorksheetFunction.Substitute(cell, " ", "  ", 23)
  Next

You do not want to read and write the cell multiple times. You only do a read and a write. You also need to check if the cell has a string. There is no point in running the replace on numbers or errors. Something like:
Dim v As Variant
Dim i As Long
Dim oldLen As Long

For Each cell In rng
    v = cell.Value2
    If VarType(v) = vbString Then
        oldLen = Len(v)
        For i = 25 To 23 Step -1
            v = WorksheetFunction.Substitute(v, " ", "  ", i)
        Next i
        If Len(v) <> oldLen Then cell.Value2 = v
    End If
Next

Or, if you want to be efficient then you read the range into array and replace the whole thing in one go:
Dim rngArea As Range
Dim arr() As Variant
Dim i As Long
Dim j As Long
Dim lastRow As Long
Dim hasChanged As Boolean
Dim v As Variant
Dim oldLen As Long

For Each rngArea In rng.Areas
    'Read range into array of values
    If rngArea.Count > 1 Then
        arr = rngArea.Value2
    Else
        ReDim arr(1 To 1, 1 To 1)
        arr(1, 1) = rngArea.Value2
    End If

    i = 1
    j = 1
    lastRow = UBound(arr, 1)
    hasChanged = False
    For Each v In arr 'Traverse the 2D array column-wise
        If VarType(v) = vbString Then
            oldLen = Len(v)
            For i = 25 To 23 Step -1
                v = WorksheetFunction.Substitute(v, " ", "  ", i)
            Next i
            If Len(v) <> oldLen Then
                arr(i, j) = v
                hasChanged = True
            End If
        End If
        i = i + 1
        If i > lastRow Then
            i = 1
            j = j + 1
        End If
    Next v
    If hasChanged Then
        rngArea.Value2 = arr
    End If
Next rngArea


String Concatenation in Scheme As plain r5rs is lacking in useful string processing utilities, such as string-split and string-join (I'm using MIT/GNU Scheme), I decided to implement my own.  My procedure, string-join, works just fine, but I would like my code to be critiqued by seasoned Schemers, and also find out if there are other, more efficient ways to implement the same thing.

(define string-join
  (lambda (ls delim)
  (define out (open-output-string))
    "Join list of strings together by delim into one string."
    (let loop ((ls ls))
      (if (null? (cdr ls))
          (begin
            (display (car ls) out)
            (get-output-string out))
          (begin
            (display (car ls) out)
            (display delim out)
            (loop (cdr ls)))))))

SRFI 13 has string-join. Are you sure you really need to reinvent the wheel?

But since you asked for a review, sure, I'll review.

Your docstring is in the wrong place. It should be the second subform of the lambda (after the parameters), not after the internal definitions. If that gives you a syntax error, then your implementation does not support docstrings, and you shouldn't try to shoehorn docstrings into a non-standard location.
Your procedure doesn't work if the incoming list of strings is empty. You should probably keep track of whether to print the delimiter based on whether you're printing the first element, not whether the list will end soon.
You should use for-each rather than a manual loop; it expresses your intent more clearly.

Putting all these suggestions together, we get (tested on Racket and Guile):

(define (string-join ls delim)
  "Join list of strings together by delim into one string."
  (define out (open-output-string))
  (for-each (lambda (delim item)
              (display delim out)
              (display item out))
            (cons "" (circular-list delim))
            ls)
  (get-output-string out))

(The code above requires SRFI 1 for for-each and circular-list.)

If you want your code to be portable to implementations which don't support docstrings, you can use call-with-output-string to avoid the internal definition:

(define (string-join ls delim)
  "Join list of strings together by delim into one string."
  (call-with-output-string
   (lambda (out)
     (for-each (lambda (delim item)
                 (display delim out)
                 (display item out))
               (cons "" (circular-list delim))
               ls))))


Pythonic attempt at conversion of length I'm trying to learn Python, so I started with a program to convert length units, mixing imperial and metric. First I wrote a bunch of small simple functions, but then I wanted to solve the issue of having an intuitive way of calling one function with some easy to remember parameter values in order to convert from any unit to any other unit, as opposed to having to call a bunch of different function names.

Does each instance of Convert remain in memory when I am using __new__ and not actually returning an instance of the class? For example, since only a float is stored in 'x' at the end, is the instance of convert that was created destroyed?

Is this approach valid overall? It feels like I've done something a little strange here.

Note: The code works on both Python 3 and 2.

class Convert(object):
    def __new__(self, val, unit_in, unit_out):
        convertString   = '%s_to_%s' % (unit_in, unit_out)
        try:
            convertFunction = getattr(self, convertString)
        except:
            return None
        return convertFunction(self, val)

    def mm_to_cm(self, millimeters):
        return millimeters / 10.0
    def mm_to_in(self, millimeters):
        return ((millimeters / 1000.0) / 0.9144) * 36.0
    def mm_to_ft(self, millimeters):
        return ((millimeters / 1000.0) / 0.9144) * 3.0
    def mm_to_m(self, millimeters):
        return millimeters / 1000.0

    def cm_to_mm(self, centimeters):
        return centimeters * 10.0
    def cm_to_in(self, centimeters):
        return ((centimeters / 100.0) / 0.9144) * 36.0
    def cm_to_ft(self, centimeters):
        return ((centimeters / 100.0) / 0.9144) * 3.0
    def cm_to_m(self, centimeters):
        return centimeters / 100.0

    def in_to_mm(self, inches):
        return (((inches / 12.0) / 3.0) / (1.0 / 0.9144)) * 1000.0
    def in_to_cm(self, inches):
        return (((inches / 12.0) / 3.0) / (1.0 / 0.9144)) * 100.0
    def in_to_ft(self, inches):
        return inches / 12.0
    def in_to_m(self, inches):
        return (((inches / 12.0) / 3.0) / (1.0 / 0.9144))

    def ft_to_mm(self, feet):
        return (feet / 3.0) / (1.0 / 0.9144) * 1000.0
    def ft_to_cm(self, feet):
        return (feet / 3.0) / (1.0 / 0.9144) * 100.0
    def ft_to_in(self, feet):
        return feet * 12.0
    def ft_to_m(self, feet):
        return (feet / 3.0) / (1.0 / 0.9144)

    def m_to_mm(self, meters):
        return meters * 1000.0
    def m_to_cm(self, meters):
        return meters * 100.0
    def m_to_in(self, meters):
        return (meters / 0.9144) * 36.0
    def m_to_ft(self, meters):
        return (meters / 0.9144) * 3.0

print(Convert( 1.0,   'ft', 'in'))
print(Convert(12.0,   'in', 'ft'))
print(Convert( 0.3048, 'm', 'ft'))
x = Convert(3, 'ft', 'm')
print(x)
exit()
output:
12.0
1.0
1.0
0.9144

Firstly as highlighted by Tlomoloko using a class for this is really strange.

However I think the both of your are missing something rather important. If you wanted to hand write every SI prefix for metres alone then you'll rack up an unmaintainable amount of code. The table contains 21 prefixes, such as no prefix - m, centi - cm, and kilo - km. Whilst I'm sure many conversions you'll think are ridiculous. Who needs to convert from yoctometres to yottametres? But what's the harm in supporting it, if you allow something reasonable like zetta to yotta?

To write the conversions for all SI prefixes would require only \$\binom{21+1}{2}\$ or 231 bespoke functions. Which would be absolutely ridiculous to write by hand.

And so the solution to this is to have an intermarry value that you always convert to or from. And since all of your existing functions are nice simple multiplications or divisions we can simply assign a single value for each unit.

This may be a bit hard to visualize, and so we'll run through some examples.

Example

1 cm to mm

1 cm = 0.01 m
1 mm = 0.001 m

First we convert 1 cm to metres. This is as simple as \$1 \times 0.01\$. Afterwards we convert from metres to millimetres simply as \$\frac{1 \times 0.01}{0.001}\$.

Which results in 10.
1' to inches

1' = 0.3048 m
1" = 0.0254 m

First we convert 1' to meters. This is as simple as \$1 \times 0.3048\$. Afterwards we convert from metres to foot simply as \$\frac{1 \times 0.3048}{0.0254}\$.

Which results in 12.

Code

CONVERSIONS = {
    'm': 1,
    'cm': 0.01,
    'mm': 0.001,
    'in': 0.0254,
    'ft': 0.3048,
}

def convert(value, unit_in, unit_out):
    return value * CONVERSIONS[unit_in] / CONVERSIONS[unit_out]

print(convert( 1.0,   'ft', 'in'))
print(convert(12.0,   'in', 'ft'))
print(convert(0.3048, 'm',  'ft'))
print(convert(3,      'ft', 'm' ))

Now the code's not perfect. If you run it you should instantly notice that it outputs some ugly 12.000000000000002 rather than 12.0. Yuck.

And so we can convert your code to use fractions.Fraction. However this will print 1143/1250 rather than 0.9144. Since I dislike getting 12.0 rather than 12, we can fix these at the same time.

from typing import Dict, Union
from fractions import Fraction

Number = Union[int, float]
CONVERSIONS: Dict[str, Fraction] = {
    'm': Fraction('1'),
    'cm': Fraction('0.01'),
    'mm': Fraction('0.001'),
    'in': Fraction('0.0254'),
    'ft': Fraction('0.3048'),
}

def to_number(value: Fraction) -> Number:
    if value % 1:
        return float(value)
    else:
        return int(value)

def convert(value: Number, unit_in: str, unit_out: str) -> Number:
    return to_number(value * (CONVERSIONS[unit_in] / CONVERSIONS[unit_out]))

if __name__ == '__main__':
    print(convert( 1.0,    'ft', 'in'))
    print(convert(12.0,    'in', 'ft'))
    print(convert( 0.3048,  'm', 'ft'))
    print(convert( 3,      'ft',  'm'))


A Maximum Function For Various Type Arbitrary Nested Iterable Implementation in C++ This is the follow-up question for A Summation Function For Various Type Arbitrary Nested Iterable Implementation in C++. Besides the summation case, I am trying to implement a Max function which can find the maximum value in various type (comparable) arbitrary nested iterable things as the further step. The recursive technique is also used here in order to iterate all elements. Some concepts as the constraints for template are as below.
template<typename T>
concept Summable = requires(T x) { x + x; };

template<typename T>
concept Iterable = requires(T x)
{
    x.begin();      // must have `x.begin()`
    x.end();        // and `x.end()`
};

The key implement part about this Max function.
template<typename T> requires Summable<T>
static T Max(T inputNumber);        //  Deal with the base case like "Max(static_cast<int>(1))"

template<class T> requires Iterable<T>
static auto Max(const T& numbers);  //  Deal with the iterable case like "Max(std::vector<long double>{ 1, 1, 1 })"

template<class T> requires Summable<T>
static inline T Max(T inputNumber)
{
    return inputNumber;
}

template<class T>  requires Iterable<T>
static inline auto Max(const T& numbers)
{
    typedef typename std::iterator_traits<typename T::iterator>::value_type
        value_type;

    decltype(Max(std::declval<value_type &&>())) maxValue{};

    maxValue = static_cast<decltype(maxValue)>(Max(numbers.at(0)));
    for (auto& element : numbers)
    {
        if (maxValue < Max(element))
        {
            maxValue = Max(element);
        }
    }
    return maxValue;
}

The std::vector test case to this Max function.
std::vector<int> v{ 1, 2, 3, -1, -2, -3 };
int largest_element = *std::max_element(v.begin(), v.end());
std::cout << largest_element << std::endl;
std::cout << Max(v) << std::endl;

The value of largest_element and the result of Max(v) are the same.
The std::array test case to this Max function.
std::array<long double, 10> testArray;
for (size_t i = 0; i < 10; i++)
{
    testArray[i] = i;
}
std::cout << Max(testArray) << std::endl;

The more complex (std::vector<std::vector<long double>>) case:
std::vector<long double> testVector1;
testVector1.push_back(1);
testVector1.push_back(20);
testVector1.push_back(-100);

std::vector<long double> testVector2;
testVector2.push_back(10);
testVector2.push_back(90);
testVector2.push_back(-30);

std::vector<std::vector<long double>> testVector3;
testVector3.push_back(testVector1);
testVector3.push_back(testVector2);
std::cout << Max(testVector3) << std::endl;

All suggestions are welcome.
The summary information:

Which question it is a follow-up to?
A Summation Function For Various Type Arbitrary Nested Iterable Implementation in C++

What changes has been made in the code since last question?
The previous question focus on the summation operation and the main idea in this question is trying to achieve the maximum operation.

Why a new review is being asked for?
In my opinion about this code, there are some problem existed. Although this Max function seems works well, there is a little weird to the usage of Summable concept here. Is it a good idea to replace this Summable into a new concept Comparable? If so, Is there any better idea or suggestion about the design of Comparable? I have ever comes up an idea to the concept Comparable like below.
template<typename T>
concept Comparable = requires(T x) { x > x; };

However, the operator ">" has been defined in std::vector until C++20. This operator > compares the contents lexicographically. This causes the error "ambiguous call to overloaded function".
Moreover, is it a good idea to simplify the initial value assignment maxValue = static_cast<decltype(maxValue)>(Max(numbers.at(0))); into maxValue = Max(numbers.at(0)) (remove the static_cast syntax)?

Using the right concept
Indeed, Summable is the wrong concept to use if you want to check if the value type is such that you can calculate the maximum of two values. Have a look at the documentation of std::max(). In particular, it requires that the type is LessThanComparable. So you could create a concept to test that. There is already a concept std::totally_ordered, which is a bit more strict than necessary.
Simplify Max()
You do an elaborate dance to get the right type for maxValue, but why not just use auto? Also, use std::max() to calculate the maximum for you:
template<class T>  requires Iterable<T>
static inline auto Max(const T& numbers)
{
    auto maxValue = Max(numbers.at(0));

    for (auto& element : numbers)
    {
        maxValue = std::max(maxValue, Max(element));
    }

    return maxValue;
}


Single-thread worker in multi-thread webservice There is standalone WCF webservice, which is providing methods to interact with legacy ERP system.

This system exposes an API, which allow programmer to interact with it.

The problem is that, for legacy reasons, every call to API function has to come from the same thread. As we know, WCF webservice is multi-thread, so I needed to solve it someway.

Below is my solution of that problem. It works. I want to someone smarter than me look at this code, and tell me, if that is "right" way to solve it. Maybe there was "better" way to achieve goal. I want to learn something.

public static class ApiManager
{
    static Queue<ApiTask> apiTasks = new Queue<ApiTask>();
    static Thread thread;
    static AutoResetEvent wakeUp;

    public static void Init()    // <--1
    {
        wakeUp = new AutoResetEvent(false);
        thread = new Thread(runner);
        thread.Start();
    }

    public static void AddTask(ApiTask task) //<--5
    {
        apiTasks.Enqueue(task);
        wakeUp.Set();
    }

    static void runner()
    {
        wakeUp.Reset();
        while (true)  //<--2
        {
            wakeUp.WaitOne();      //<--3
            while (apiTasks.Count > 0)
            {
                var task = apiTasks.Dequeue(); //<--4
                task.Start();
            }
            wakeUp.Reset();
        }
    }
}

This is my "worker" class.
(1) Init method - it is run on start of webservice. It creates new thread, and starts it. In that thread we have infinite loop, in which thread is waiting for wakeUp signal to be set (3). After that, it consumes elements from queue (4), and runs it one after another. After the queue is empty, it resets the wakeUp signal, and waiting for it to be set.

public abstract class ApiTask
{
    protected abstract void DoJob(); //<-- 1
    protected AutoResetEvent IsDone = new AutoResetEvent(false);
    protected Exception RegisteredException = null;

    internal void Run()
    {
    }

    public void Start()
    {
        try
        {
            DoJob();
        }
        catch (Exception ex)
        {
            this.RegisteredException = ex;
        }

        IsDone.Set();
    }
}

public abstract class ApiTask<TResult> : ApiTask where TResult : new()
{
    protected TResult Result = new TResult();

    public TResult GetResult()
    {
        ApiManager.AddTask(this);
        this.IsDone.WaitOne();

        if (RegisteredException != null) throw RegisteredException;
        return Result;
    }
}

Here we have abstract class ApiTask, which is stored in queue in ApiManager, and run by it.
It contains abstract method DoJob (1), which will actually contains code to be run. Also, we have Start method, which runs the job, and sets IsDone signal, after finishing. If job throws an exception, it is catched and stored.
And - the "core": GetResult method - it adds task to queue in manager, wait for it to be done, and returns result (or throws exception, if it has been stored by Start method).

In practice, I'm creating Tasks in that way:

public abstract class CreateDocumentTask:ApiTask<APIDocument>
{
    DateTime _date;

    protected int CreateDocumentTask(DateTime date)
    {
        _date = date;
    }

    protected override void DoJob()
    {
        Result = APICREATEDOCUMENTFUNCTION(_date);
    }
}

To use it in application, I only have to change call from:

var document = APICREATEDOCUMENTFUNCTION(date)

to:

var document = new CreateDocumentTask(DateTime.Now).GetResult();

And I'm sure that it will be always done by the same thread.

Feel free to correct my English. Feel free to correct and criticize code. I want to learn.

Basically, you have the right idea: producer-consumer is probably the pattern you need.

The way you implemented it is the problem, because so many things have gone wrong...

Queue is not thread-safe, so access to it has to be synchronized, otherwise at best you gonna be losing tasks at worst it will straight up crash your software.
I don't see a single good reason why your ApiManager should be static. Having a mutable static dependency in code is already bad enough, having a static dependency that you also have to synchronize - is straight up awful.
Your thread is never terminated and it will keep running even if you close your application.
You have racing conditions all over the place. For example, if you have only two tasks, and the second one is added to queue after the first one is in completed but before wakeUp.Reset() is called in your loop, the second task will never be executed.
You create IDisposable objects that you never dispose (I'm talking about wait handles).

Bottom line is: it's hard to fully understand thread-safety and it's even harder to write correct thread-safe code in complex multi-threaded environment. It is not something you can just jump into. You should definitely do some reading on how multi-threading is done in C# before attempting to implement it in your production code. Reading lock keyword documentation is a good place to start.


Threshing: Sieve of Eratosthenes I would like a complete threshing of this code so that I can see what I did wrong and what I am using incorrectly.

I made this super simple, trying to learn a little bit about List<T> while I was doing this.  I have never actually tried to do this before, so I thought it would be a good learning experience, but it just seems too easy (simple), especially when I look at other people's renditions of this algorithm.  Am I missing a vital piece of the algorithm?

Note: Console app, if you want to see all the primes you will have to set the buffer of your console.

static void Main(string[] args)
{
    var upperLimit = 9999;
    List<Int64> primes = new List<Int64>();
    for (Int64 i = 2; i < upperLimit; i++)
    {
        primes.Add(i);
    }
    List<Int64> numbers = primes;

    for (var i = 2; i < upperLimit; i++)
    {
        foreach (var number in numbers.ToArray())
        {
            if (number == i) { continue; }
            if (number % i == 0)
            {
                primes.Remove(number);
            }
        }
        numbers = primes;
    }
    primes.ForEach(delegate(Int64 prime)
    {
        Console.WriteLine(prime.ToString());
    });

    Console.WriteLine("The Last Prime is " + primes[primes.Count - 1]);
    Console.WriteLine("what are you waiting for? Exit the program!");
    Console.ReadLine();
}

I figured that I would give this a try after I saw this.

The reason for var number in numbers.ToArray() is because it wouldn't let me .Remove(number) from primes if I foreach'ed through the list otherwise.  Credit to this answer for the solution to my dilemma.

Follow-up can be found here

One note: You create extra work and time for the program by including all of the even numbers in the initial List creation.

You can do something similar to this to eliminate the even numbers:

var upperLimit = 9999;
List<Int64> primes = new List<Int64>();
for (int x = 2; x <= (upperLimit+1)/2; x++)
    primes.Add(2*x-1);

That should give you a much reduced list to have to run through. Other than that, it's a very nice and simple solution.


C++ Simple 2D Array Wrapper I know I should be using vectors, but I want to learn something.

Please review it and point out any mistakes. Thank you. Here it is:

template< typename T >
class Table2D {
public:
    Table2D( unsigned int sizeX,
             unsigned int sizeY )
            : m_sizeX( sizeX )
            , m_sizeY( sizeY )
    {
        try {
            m_2DTable = new T[ m_sizeX * m_sizeY ]( );
        }
        catch( std::bad_alloc ) {
            std::cerr << "bad_alloc...\n";
        }
    }
    ~Table2D( ) { delete [ ] m_2DTable; }
private:
    unsigned int m_sizeX;
    unsigned int m_sizeY;
    T* m_2DTable;
public:
    void set( unsigned int x, unsigned int y, T val ) {
        m_2DTable[ x + ( y * m_sizeX ) ] = val;
    };
    T get( unsigned int x, unsigned int y ) {
        return m_2DTable[ x + ( y * m_sizeX ) ];
    };
};

Usage example:

Table2D< unsigned int > ui_2DTable( 50, 50 );
ui_2DTable.set( 10, 20, 50 );
std::cout << ui_2DTable.get( 10, 20 ) << std::endl;

Table2D< float > f_2DTable( 3, 7 );
f_2DTable.set( 1, 4, 1.75 );
std::cout << f_2DTable.get( 1, 4 ) << std::endl;

Firstly, I assume you have something like #define __in up the top. This is sort of a Microsoft-ism, and really isn't needed in most of these parameter sets. When you're passing types that are neither pointers nor references, it's pretty obvious that they're input values. To me, it's simply extra clutter which makes the code harder to read and provides no benefit.

Secondly, this seems like an extremely memory heavy way of doing what is basically a map. You allocate space for m_SizeX * m_SizeY elements, but if the table is sparse, a majority of that space is going to go unused.

Thirdly, you should be passing and returning certain values by (const) &. For example, in your set method should probably be:

void set(unsigned int x, unsigned int y, const T& val)

And your get method should be:

T& get(unsigned int x, unsigned int y)

This should also have a const overload:

const T& get(unsigned int x, unsigned int y) const

Note that if you haven't initialized a value, it will happily return you whatever junk happened to be there when new allocated the memory. This is probably not what you want - you should likely have it return either a default value or error.

If I were to write this, I'd utilize a std::map and something like the following:

#include <map>
#include <utility>

template <typename T>
class Table2D
{
private:
    std::map<std::pair<unsigned, unsigned>, T> table;

public:
    T& operator()(unsigned x, unsigned y)
    {
        std::pair<unsigned, unsigned> p = std::make_pair(x, y);
        return table[p];
    }

    const T& operator()(unsigned x, unsigned y) const
    {
        std::pair<unsigned, unsigned> p = std::make_pair(x, y);
        return table[p];
    }
};

This code assumes you want a default T() value (which will be 0 for all numeric types) to be inserted if it doesn't exist. Utilizing operator() returning a T& allows us to set and get values utilizing the same method:

Table2D<unsigned int> ui_2DTable;
ui_2DTable(10, 20) = 50;
std::cout << ui_2DTable(10, 20) << "\n";

Further, if we do something like:

std::cout << f_2DTable.get(10, 10) << "\n";

We'll simply return a default value. Currently, this will return something from a random bit of memory, and causes undefined behaviour, which is bad.

Finally, your code can cause undefined behaviour if the user tries to utilize the copy constructor or copy assignment operator. That is:

Table2D<unsigned int> ui_2DTable( 50, 50 );
ui_2DTable.set( 10, 20, 50 );
Table2D<unsigned int> ui_construct(ui_2DTable);
Table2D<unsigned int> ui_copy = ui_2DTable;

will have the pointer m_2DTable pointing to the same memory in all 3. If one goes out of scope, the memory will be deleted, while the others will still try to access it - if you're lucky, this will crash. I won't go into complete detail on how to fix this, but you can read about the so called "Rule of Three" here. Suffice to say, the version using std::map doesn't suffer from this problem (one of the nice benefits of standard containers is this comes "for free").


Implementing Passport authentication with promises Here is code implementing Passport authentication with a Google Strategy. It uses Mongoose to store and retrieve user data.

You might need to be familiar with both of these technologies to review this code if not check out the two links I provided.

I have included only the Passport file. I was told I am not using promises correctly and would like to rectify this if it is in fact true.

The code is full of comments below.  Please let me know if more clarification or context is needed.

Thanks for any feedback.

Passport

const passport = require('passport');
const GoogleStrategy = require('passport-google-oauth').OAuth2Strategy;
const DBM = require('../../database/mongoose');
const helper = require('../../config/helper');

// review this post
// https://stackoverflow.com/questions/50339887/can-i-reduce-this-passport-code

// configure Passport to use Google Auth
passport.use('google', new GoogleStrategy(helper.getPassport(), getOrCreateUser));

// passed to passport, this is the starting point for Google Auth
function getOrCreateUser (accessToken, refreshToken, profile, done) {

  // abstracts out relevant information from what Google returns
  const user_props = obtainProps(profile);
  DBM.getUser(user_props.id_google).then( (res) => {

    // res[0] will be "true" if the user exists
    return res[0] ? done(null, user_props) : createUser(done, user_props);
  }).catch( error => {
    return done(error, null);
  });
}

// the user was not found, create the user
function createUser (done, user_props) {
  DBM.createUser(user_props).then(() => {
    return done(null, user_props);
  }).catch( error => {
    return done(error, null);
  });
}

// serializeUser required by passport to identify the user uniquely on the client
passport.serializeUser( (profile, done) => {
  done(null, profile.id_google);
});

// deserializeUser required by passport to retrieve data on the server using unique
// identifier created by serializeUser
passport.deserializeUser( (id_google, done) => {
  DBM.getUser(id_google).then((res) => {
    done(null, res[0]);
  }).catch( (error) => {
    console.error('DBM.getUser() Error: ', error);
  });
});

// takes a profile from Google and extracts parameters to save
function obtainProps (profile) {
  let props = {};
  props.id_google = profile.id;
  props.email = profile.emails[0].value;
  props.name = profile.displayName;
  props.pic_url = profile.photos[0].value;
  props.type = profile._json.objectType;
  return props;
}

module.exports = passport;

When writing your custom functions that operate on promises, a good habit is to return resulting promises in order to have ability to chain them. For instance:

return DBM.getUser(user_props.id_google).then( (res) => { (from getOrCreateUser)
or:
return DBM.createUser(user_props).then(() => {

It will ensure that getOrCreateUser and createUser operations will be chained together properly.

The passportjs uses different API. It does not use promises but callbacks.
As I see, you mixed them together by mistake! For instance in DBM.createUser from inside your promise you returned an object returned just by done callback when resulting value/other promise is expected. Same in DBM.getUser from passport.deserializeUser
It might be only mine (a little bit nerdy) custom, but I have a habit to wrap 3rd party non promise APIs into promise adapters.

For instance (an easy one):

function serializeUser(idProviderPromise) {
    let resultDeffered = Promise.defer();
    passport.serializeUser((profile, done) => {
        idProviderPromise(profile).then(function (profileId) {
            done(null, profileId);
            resultDeffered.resolve();
        });
    });
    return resultDeffered;
}

Usage:

passportPromiseApi.serializeUser(new Promise(profile => profile.id_google));

It can help make us sure not to mix different APIs together.

At the first glance it can look a little bit over complicated, but please take a look how it simplifies more complex cases:

function configure(getOrCreateUserPromise) {
    let resultDeffered = Promise.defer();
    function getOrCreateUser(accessToken, refreshToken, profile, done) {
        getOrCreateUserPromise(accessToken, refreshToken, profile).then(function (userProps) {
            done(null, userProps);
            resultDeffered.resolve(userProps);
        }).catch(function (error) {
            done(error, null);
            resultDeffered.fail(error);
        });
    }
    passport.use('google', new GoogleStrategy(helper.getPassport(), getOrCreateUser));
    return resultDeffered;
}

Usage:

passportPromiseApi.configure(new Promise(function (accessToken, refreshToken, profile) {
    const userProps = obtainProps(profile);
    return DBM.getUser(userProps.id_google).then(res => {
        return res[0] ? Promise.resolve(userProps) : createUser(userProps);
        //Instead Promise.resolve(userProps) simply userProps should work as well
        //But it is also a good practice for return type in any JS function to be consistent.
    });
}));

function createUser (userProps) {
    return DBM.createUser(userProps).then(() => userProps);
    //Error handling is already done by our adapter.
    //We can focus only on business logic here!
}

Last less important thing: let assume some consistent naming convention for function parameters and local variables respectively. You have to decide whether to use xxx_yyy or xxxYyy (camel case). 3-rd party usages doesn't count as it must be used exactly.


Generic oneshot task runner I'm making some helper functions for the (fairly simple) UI framework I'm developing, among them, a helper to run app initialization tasks. Here's a helper class I've developed for oneshot tasks:

from threading import Lock

class Oneshot():
    _running = False
    _finished = False

    def __init__(self, func, *args, **kwargs):
        self.func = func
        self.args = args
        self.kwargs = kwargs

    def run(self):
        if self.running or self.finished:
            return
        self.active_lock.acquire()
        self._running = True
        self.active_lock.release()

        self.func(*self.args, **self.kwargs)

        self.active_lock.acquire()
        self._running = False
        self._finished = True
        self.active_lock.release()

    @property
    def running(self):
        self.active_lock.acquire()
        value = self._running
        self.active_lock.release()
        return value

    @property
    def finished(self):
        self.active_lock.acquire()
        value = self._finished
        self.active_lock.release()
        return value

Is it actually thread-safe, as I intend it to be? Any other thoughts on this?

I'd also like to add a reset() method. My idea is like this:

def reset(self):
    if self.running:
        raise Exception("Runner can't be reset while still running")
    self._finished = False

My only doubt is about whether raising an exception is the best course of action, but, since I have to signal the user that they are trying to perform an illegal action, and it's better to do it explicitly as the action user desired did not complete, I don't see a better way - do you?

If run is called from multiple threads, the task may execute multiple times because this code is checking and setting the flag in separate operations:

    if self.running or self.finished:
        return
    self.active_lock.acquire()
    self._running = True
    self.active_lock.release()

Make it an atomic operation by holding the lock. Note also how the with statement releases the lock automatically:

    with self.active_lock:
        if self._running or self._finished:
            return
        self._running = True


How to refactor common methods that depend on local variables into a base class I'm using Node.js and am creating some models for my different objects.  This is a simplified version of what they look like at the moment.

var Foo = module.exports = function () {

    var values = { type: 'foo', prop1: '', prop2: '' };

    function model() {}

    model.init = function(val) {
        _.extend(values, val);
        return model;
    }

    model.store = function(cb) {
        db.insert(values.type, values, cb);
    }

    model.prop1 = function(val) {
        if(!arguments.length) return values.prop1;
        values.prop1 = val;
        return model;
    }

    return model;
}

Then I can do:

var foo = Foo();
foo.init({prop1: 'a', prop2: 'b'}).store(function(err) { ... });

I then create another model:

var Bar = module.exports = function () {

    var values = { type: 'foo', prop3: '', prop4: '' };

    function model() {}

    model.init = function(val) {
        _.extend(values, val);
        return model;
    }

    model.store = function(cb) {
        db.insert(values.type, values, cb);
    }

    model.prop3 = function(val) {
        if(!arguments.length) return values.prop3;
        values.prop3 = val;
        return model;
    }

    return model;
}

At which point I realize that a lot of the functions, like model.init and model.store will be reused in each of the models. But while they are exactly the same code, they depend on the local values contained in the model closure.

What's the best way to pull these functions out into a common base class that I can then use to extend all of my models with?

Here are some tips:

1) Turn Model into a constructor that accepts an object as the default values.

Note: Constructors should start with a capital letter.
Code:

// Model
function Model(defaultValues) {
    this.values = defaultValues || {};
}

Usage:

var modelFoo = new Model({
    type : 'foo',
    prop1 : '',
    prop2 : ''
});
console.log( modelFoo.values.type === "foo" );  // true

2) For readability, set the value of module.exports at the end of the file.

Assuming Foo and Bar are in the same file you can export both of them like so.

Code:

module.exports = {
    "Foo" : Foo,
    "Bar" : Bar
};

3) Since Model is a constructor, you can access the interval values by using this and attach functions to it by using prototype.

Also, returning this gives access to the current object instance.

Old Code:

model.init = function (val) {
    _.extend(values, val);
    return model;
}

model.store = function (cb) {
    db.insert(values.type, values, cb);
}

New Code:

Model.prototype.init = function (val) {
    this.values = _.extend(this.values, val);
    return this;
};
Model.prototype.store = function (cb) {
    db.insert(this.values.type, this.values, cb);
    return this;
};

4) Make sure that you define db in the module.

Code:

var db = require("db").db;

5) Create a generic setter and getter function for the properties.

Old Code:

model.prop1 = function (val) {
    if (!arguments.length)
        return values.prop1;
    values.prop1 = val;
    return model;
}

New Code:

Model.prototype.createPropSetterAndGetter = function (propName) {
    this[propName] = (function () {
        return function (val) {
            if (!arguments.length) {
                return this.values[propName];
            }
            this.values[propName] = val;
        };
    }());
};

Usage:

var modelBar = new Model({
    type : 'bar',
    prop3 : '',
    prop4 : ''
});
console.log( typeof modelBar.prop3 === "undefined" ); // true
modelBar.createPropSetterAndGetter("prop3");
console.log( modelBar.prop3() === "" ); // true
modelBar.prop3( "PAY ME!" );
console.log( modelBar.prop3() === "PAY ME!" ); //true and hurry.

Final Result:

var db = require("db").db;

// Model
function Model(defaultValues) {
    this.values = defaultValues || {};
}
Model.prototype.init = function (val) {
    this.values = _.extend(this.values, val);
    return this;
};
Model.prototype.store = function (cb) {
    db.insert(this.values.type, this.values, cb);
    return this;
};
Model.prototype.createPropSetterAndGetter = function (propName) {
    this[propName] = (function () {
        return function (val) {
            if (!arguments.length) {
                return this.values[propName];
            }
            this.values[propName] = val;
        };
    })();
};
// Foo
var Foo = function () {
    var modelFoo = new Model({
            type : 'foo',
            prop1 : '',
            prop2 : ''
        });
    modelFoo.createPropSetterAndGetter("prop1");
    return modelFoo;
};
// Bar
var Bar = function () {
    var modelBar = new Model({
            type : 'bar',
            prop3 : '',
            prop4 : ''
        });
    modelBar.createPropSetterAndGetter("prop3");
    return modelBar;
};

module.exports = {
    "Foo" : Foo,
    "Bar" : Bar
};


Function to parse floats optionally followed by %, more strict than parseFloat in javascript I'm not a Javascript expert, so please review this function for me. Also, it  could be that there is something in any of the library we're using in our project that can do something similar for me (angular, foundation, jquery, underscore), but I'm not too familiar yet with most of them.

// helper function, parses numeric input also ending with %
  /*
  [parseNumeric('123') === 123,
   parseNumeric('10 abc') === undefined
   parseNumeric('10.10%') === 10.1,
   parseNumeric(' 10.1000 ') === 10.1,
   parseNumeric('10. 01%' ) === undefined,
   parseNumeric('10.0 %') === 10] */
  function parseNumeric(number) {
    if (number === undefined || typeof(number) === 'number')  {
      return number;
    }

    // delete trailing zeros and whitespaces between number digits and %,
    // leaving % if present
    var numberString = number.trim().replace(/\.?0*\s*(\%)?$/g,'$1');

    var parsedNumber = parseFloat(number);
    if (isNaN(parsedNumber)) {
      return undefined;
    }

    var parsedNumberLength = (parsedNumber+'').length;

    if (numberString.length === parsedNumberLength) {
      return parsedNumber;
    }

    if (numberString.charAt(numberString.length - 1) === '%' &&
        (numberString.length - 1) === parsedNumberLength) {
      return parsedNumber;
    }

    return undefined;
  }

I think it's a bit too much code. If you are already using a regular expression, why not using it to do all the parsing for you?

function parseNumeric(number) {
    var num = number.trim();
    var reOut = /^([-+]?(\d+\.?\d*|\d*\.?\d+))\s*%?$/.exec(num);
    return reOut ? parseFloat(reOut[1]): undefined;
}


Improve query: find users who are neither friends nor fans of a user Please take a look at this query and try to give me any other ideas that will give the exact same results more efficiently.

SELECT username
FROM users
WHERE username NOT IN (
    SELECT DISTINCT username
    FROM users, friends
    WHERE 'user1' IN (you,friend,username)
      AND you     IN ('user1',username)
      AND friend  IN ('user1',username))
  AND username <> 'user1'

Here's a live demo.

This query will give you the same results

SELECT username FROM users
WHERE username NOT IN (SELECT you FROM friends WHERE friend = 'user1')
AND username NOT IN (SELECT friend FROM friends WHERE you = 'user1')
AND username <> 'user1'

and the SQLFiddle

If you look at the execution plan, it looks like my query grabbed 1 less row throughout the execution and it didn't have to use a temporary or a join buffer.

This Query is more readable than yours, assuming this is what you are trying to do.


Interpolating given value and legacy value from serialization buffer Here's a function that interpolates between a given value and a value fetched out of a legacy serialization buffer:

template<typename T>
T interpolate(Buffer& buffer, const T currentValue, const float prop)
{
    T bufferValue;
    buffer.readT(&buferValue);
    return currentValue + (bufferValue-currentValue)*prop;
}

This works great for interpolate<float> and interpolate<int> and so on. However if I want to pass a more complex structure such as a vector, I'd rather the currentValue parameter was passed by reference instead of by value.  I can use overloading to handle that situation:

// in addition to original function
Vector interpolate(Buffer& buffer, const Vector& currentValue, float prop)
{
    Vector bufferValue;
    buffer.readT(bufferValue);
    return currentValue + (bufferValue-currentValue)*prop;
}

Even if you rip out the lerp into a helper function, it still isn't ideal to repeat the function like this when the only difference from the original is the & parameter, especially if there's more than one type I'd like to pass by reference.

I can use traits to auto-detect when to use a reference:

// to replace original function

template<typename T>
struct RefTrait { typedef const T Ref; }

template<>
struct RefTrait<Vector> { typedef const Vector& Ref; }

template<typename T>
T interpolate(Buffer& buffer, typename RefTrait<T>::Ref currentValue, const float prop)
{
    T bufferValue;
    buffer.readT(&buferValue);
    return currentValue + (bufferValue-currentValue)*prop;
}

However now the compiler can't induce the type of T by default, and the calling code has to explicitly state type:

floatVal = interpolate<float>(buffer, floatVal, 0.5f);
vectorVal = interpolate<Vector>(buffer, vectorVal, 0.5f);

Is there anyway to have compact calling code as well as a single defined function?

If I understand your question correctly, using the RefTrait policy in your second solution is acceptable but you want to avoid specifying template parameters from the client code that's using it.

If so then perhaps one possible idea is to create an inline wrapper function around your interpolate for marshaling the call so type deduction happens automatically:

template <>
struct RefTrait<Vector> { typedef const Vector &Ref; };

template <typename T>
T interpolateImp(Buffer& buffer, typename RefTrait<T>::Ref currentValue, const float prop)
{
    T bufferValue;
    buffer.readT(&bufferValue);
    return currentValue + (bufferValue-currentValue)*prop;
}

template <typename T>
inline T interpolate(Buffer& buffer, T &currentValue, const float prop)
{
    return interpolateImp<T>(buffer, currentValue, prop);
}

It doesn't avoid the overloading but it'll at least help with the automatic type-deduction when you try to use it:

floatVal  = interpolate(buffer, floatVal, 0.5f);
vectorVal = interpolate(buffer, vectorVal, 0.5f);

And the interpolate wrapper should get optimized away into just a interpolateImp call when the compiler inlines it.

But I would recommend taking a look at what Mike said first and find out really how much of a performance impact this is. If afterwards you still decide to pursue this route there are two things to keep in mind.

Caveat with using your current RefTrait. At the moment, T bufferValue; in your interpolate creates a locate T variable but doesn't initialize it. This poses a problem if T = const Vector&. Furthermore, it also means you're unable to change this T object later on should you need to. One possible way to fix it is to also add a valueType to your RefTraits policy. You would then use typename T::valueType to create any locates you would need inside interpolate.
Using templates in this matter will reduce code clarity unless you're extra dilgent. The syntax has a matter of exploding in your face and could be difficult to get right especially if you're trying to cover the funny corner cases. Be sure to weight the tradeoffs.

Edit: After thinking about my above code some more, I noticed it could be simplified a bit by just keeping the syntax the same as the OP. I've updated mines to reflect that.


A bash prompt that includes battery level & git branch Is there a better way to handle the color codes and display git branch and battery percentage?

#!/bin/bash

## set colors
t="\[\033[0;34m\]"
p="\[\033[0;35m\]"
g="\[\033[0;32m\]"
b="\[\033[0;36m\]"
y="\[\033[0;33m\]"
## the color reset code
re="\[\033[0m\]"

## inserts git brance into prompt at $(PBG)
PBG(){
    git branch 2> /dev/null | sed -e '/^[^*]/d' -e 's/* \(.*\)/ (\1)/'
}

I would rather not make three different calls to insert the battery level.

## inserts battery percentage into prompt @ $(BATT)
BATT(){
    acpi | awk '{print $4}' | sed -e 's/,//'
}

PS1="$b($re$y\$(BATT)$re$p \h$re $gÎ»$re $g\W$re$t \$(PBG)$re$b)$re "

Since the prompt is dynamic, executed after every command you enter in the shell, it's important to make it as fast as possible.
For example, reduce the number of processes executed as much as possible.

Instead of git branch + sed, there is an idiomatic way to get the name of the current branch using git rev-parse:

git rev-parse --abbrev-ref HEAD

That's one process instead of two.

And to get the battery percentage,
you could replace the awk + sed with a single awk:

awk '{ sub(",", "", $4); print $4 }'

I don't what is the output of the acpi command,
but I would try to extract from it the relevant part using the Pattern substitution feature of Bash itself.
That would eliminate all external processes, for best performance.