PDO sign up function inserting data into multiple tables This is a sign up function called on form submission. It firstly inserts key user data into the users table. If successful, secondary data is then inputted into respective tables (such as user job titles and user experience). I'm not really sure whether stacking queries 2-5 is the best way to do it, so I would be interested in knowing how this function could be improved and/or made more secure.

    public function registerFreelancer($firstname, $lastname, $email, $password, $location, $portfolio, $jobtitle, $priceperhour, $experience, $bio, $userType){

        global $bcrypt;
        global $mail;

        $time       = time();
        $ip         = $_SERVER['REMOTE_ADDR'];
        $email_code = sha1($email + microtime());
        $password   = $bcrypt->genHash($password);// generating a hash using the $bcrypt object

        $query  = $this->db->prepare("INSERT INTO " . DB_NAME . ".users
            (firstname, lastname, email, email_code, password, time_joined, location, portfolio, bio, ip)
            VALUES
            (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
        ");

        $query->bindValue(1, $firstname);
        $query->bindValue(2, $lastname);
        $query->bindValue(3, $email);
        $query->bindValue(4, $email_code);
        $query->bindValue(5, $password);
        $query->bindValue(6, $time);
        $query->bindValue(7, $location);
        $query->bindValue(8, $portfolio);
        $query->bindValue(9, $bio);
        $query->bindValue(10, $ip);

        try{
            $query->execute();

            // Send email code usually here

            $rows = $query->rowCount();

            if($rows > 0){

                $last_user_id =  $this->db->lastInsertId('user_id');

                $query_2 = $this->db->prepare("INSERT INTO " . DB_NAME . ".freelancers (freelancer_id, jobtitle, priceperhour) VALUE (?,?,?)");

                $query_2->bindValue(1, $last_user_id);
                $query_2->bindValue(2, $jobtitle);
                $query_2->bindValue(3, $priceperhour);

                $query_2->execute();

                $query_3 = $this->db->prepare("INSERT INTO " . DB_NAME . ".user_types (user_type_id, user_type) VALUE (?,?)");

                $query_3->bindValue(1, $last_user_id);
                $query_3->bindValue(2, $userType);

                $query_3->execute();

                $query_4 = $this->db->prepare("INSERT INTO " . DB_NAME . ".user_experience (experience_id, experience) VALUE (?,?)");

                $query_4->bindValue(1, $last_user_id);
                $query_4->bindValue(2, $experience);

                $query_4->execute();

                if($userType == 'designer') {
                    $query_5 = $this->db->prepare("INSERT INTO " . DB_NAME . ".designer_titles (job_title_id, job_title) VALUE (?,?)");

                    $query_5->bindValue(1, $last_user_id);
                    $query_5->bindValue(2, $jobtitle);

                    $query_5->execute();

                } else if ($userType == 'developer') {
                    $query_5 = $this->db->prepare("INSERT INTO " . DB_NAME . ".developer_titles (job_title_id, job_title) VALUE (?,?)");

                    $query_5->bindValue(1, $last_user_id);
                    $query_5->bindValue(2, $jobtitle);

                    $query_5->execute();
                }

                return true;
            }

        }catch(PDOException $e){
            die($e->getMessage());
        }
    }

If the second or later query fails you'll have inconsistent data in your database. Consider using atomic transactions.
$query_2, $query_3 are bad names. You could pick something more descriptive, like $usersInsert, $freelancersInsert etc.

$query  = $this->db->prepare("INSERT INTO " . DB_NAME . ".users
    (firstname, lastname, email, email_code, password, time_joined, location, portfolio, bio, ip)
    VALUES
    (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
");

$query->bindValue(1, $firstname);
$query->bindValue(2, $lastname);
...

I'd use named binds here. As far as I see they works with INSERT statements too.

$query  = $this->db->prepare("INSERT INTO " . DB_NAME . ".users
    (firstname, lastname, email, email_code, password, time_joined, location, portfolio, bio, ip)
    VALUES
    (:firstname, :lastname, ...)
");

$query->bindValue(":firstname", $firstname);
$query->bindValue(":lastname", $lastname);
...

It would be less error-prone (harder to mix parameters up) and would be easier to read/follow.
Consider Hayley Watson's comment on the die manual page:

  It is poor design to rely on die() for error handling in a web site because it results in an ugly experience for site users: a broken page and - if they're lucky - an error message that is no help to them at all. As far as they are concerned, when the page breaks, the whole site might as well be broken.

  If you ever want the public to use your site, always design it to handle errors in a way that will allow them to continue using it if possible. If it's not possible and the site really is broken, make sure that you find out so that you can fix it. die() by itself won't do either.

Furthermore, at least log the error to a log file, otherwise you might be never know if your future users can't even register.

if($userType == 'designer') {
    $query_5 = $this->db->prepare("INSERT INTO " . DB_NAME . ".designer_titles (job_title_id, job_title) VALUE (?,?)");

    $query_5->bindValue(1, $last_user_id);
    $query_5->bindValue(2, $jobtitle);

    $query_5->execute();

} else if ($userType == 'developer') {
    $query_5 = $this->db->prepare("INSERT INTO " . DB_NAME . ".developer_titles (job_title_id, job_title) VALUE (?,?)");

    $query_5->bindValue(1, $last_user_id);
    $query_5->bindValue(2, $jobtitle);

    $query_5->execute();
}

Both cases are almost the same. You could create a function for that with a $tableName parameter to remove the duplication.

It can be a sign that you could have another database structure with only one table instead of two:

TABLE titles:
    - role (possible values: developer, designer)
    - job_title_id
    - job_title

if($userType == 'designer') {
    ...
} else if ($userType == 'developer') {
    ...
}

You could be more defensive here, if $userType contains something else (not designer nor developer) and if it's should be considered as a programming (or input validation) error sing it somehow. I'd throw an exception and log it in a catch block. (The Pragmatic Programmer: From Journeyman to Master by Andrew Hunt and David Thomas: Dead Programs Tell No Lies.)
You could save a few indentation level with guard clauses which would be readable. You wouldn't have to read through the whole function to figure out what happens when $rows > 0 is false:

$rows = $query->rowCount();

if($rows > 0){
    return;
}
$last_user_id =  $this->db->lastInsertId('user_id');
...

It might be more unambiguous using return false instead of implicit return here.
The comment doesn't say too much, the code is already obvious here:

$password   = $bcrypt->genHash($password);// generating a hash using the $bcrypt object

I'd remove it. (Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)
A lot of parameters is a code smell:  Long Parameter List. Their type are the same, it's easy to mix them up. Consider using a parameter object instead which would contain named fields.

public function registerFreelancer($firstname, $lastname, $email, $password, $location, $portfolio, $jobtitle, $priceperhour, $experience, $bio, $userType){

See: Martin Fowler's Refactoring: Improving the Design of Existing Code book, Chapter 3. Bad Smells in Code, Long Parameter List
I've found this kind of formatting is rather hard to maintain:

$time       = time();
$ip         = $_SERVER['REMOTE_ADDR'];
$email_code = sha1($email + microtime());
$password   = $bcrypt->genHash($password);// generating a hash using the $bcrypt object

If you have a new variable with a longer name you have to modify seven other lines too to keep it nice. It also looks badly on revison control diffs and could cause unnecessary merge conflicts.

From Code Complete, 2nd Edition by Steve McConnell, p758:

  Do not align right sides of assignment statements

  [...]

  With the benefit of 10 years’ hindsight, I have found that,
  while this indentation style might look attractive, it becomes
  a headache to maintain the alignment of the equals signs as variable
  names change and code is run through tools that substitute tabs
  for spaces and spaces for tabs. It is also hard to maintain as
  lines are moved among different parts of the program that have
  different levels of indentation.


Custom Sieve of Eratosthenes I'm trying to improve my code to find all the prime numbers within a set range as fast as possible, and I am trying to make it even faster. On my github page here, I have some optimization settings within Visual Studio that help improve the speed.

#include <iostream>
#include <fstream>
#include <Windows.h>
#include <conio.h>
#include <bitset>
#include <string>
#include <math.h>

#define RUNS 10000  // How many times the code will runs. I use run each section of code a set number of times, divide by that number
// to get the average. I had to to this because the program was running too fast for the GetTickCount() to get a speed.
#define RANGE 100000 // What number of primes to search up to
#define R2 RANGE/2
#define BRUNS 100000

int main()
{
    //Set up varibles
    int count;
    char primes[RANGE];
    int searchRange = sqrt(R2) + 1;
    DWORD starttime, endtime;

    //############################ SIEV #################################
    starttime = GetTickCount();

    for (int k = 0; k<RUNS; ++k)
    {
        count = 0;
        memset(primes, 0, R2);
        for (int i = 0; i < searchRange; ++i)
        {
            if (primes[i] == 0)
            {
                for (int j = (i << 1)*i + (i << 2) + (i << 1) + 3; j<R2; j += i * 4 + 6)
                {
                    primes[j] = 1;
                    primes[j + i * 2 + 3] = 1;
                }
            }
        }
    }
    endtime = GetTickCount();
    for (int i = 0; i<R2; i++) if (primes[i] == 0) count++;

    float totalOp = ((float)endtime - starttime) / (1000 * RUNS);
    //#####################################################################

    //#################### MEMSET FUNCTION TIMING TEST ####################
    // Here I am timing how long the memset function takes, so I can take that off the time for the sieve

    float totalMemset = ((float)endtime - starttime) / (1000 * RUNS);
    //#####################################################################

    //####################  OUTPUT TIME AND PRIMES  ######################
    std::cout << "Optimised Sieve Time:  ";
    std::cout << totalOp;
    std::cout << "\nPrimes Found: ";
    std::cout << count + 1;
    //#####################################################################
    _getch();
    return 0;
}

Missing header

Include <cstring> to get a definition of std::memset (and add the missing namespace qualifier where you use it).

Unused headers

We're not using <bitset>, <fstream> or <string>.

Prefer C++ headers

Use <cmath> in preference to <math.h> for new code, to place the identifiers into the std namespace (the only identifier that needs updating where it's used is std::sqrt).

Non-standard headers

Instead of platform-specific headers, you can use Standard C++ <chrono> for timing the code:

#include <chrono>

int main()
{
    auto const starttime = std::chrono::high_resolution_clock::now();

    do_work();

    auto const endtime = std::chrono::high_resolution_clock::now();

    auto const time_in_ms =
        std::chrono::duration_cast<std::chrono::milliseconds>(endtime - starttime);

    std::cout << "Time taken:  "
              << time_in_ms.count() << " ms\n";
}

Unused variable

We never use totalMemset - leaving it in makes your code look like it's not really ready for review.

Use unsigned types

Most of the numbers used in this program are necessarily non-negative.  Prefer unsigned types to make this more apparent (and to give you twice as much range of values).

Prefer constants to preprocessor macros

This is better, as it's strongly typed, and won't be expanded in the wrong context:

const std::size_t RUNS = 10000;

Use * for arithmetic, not <<

An optimising compiler should generate the same code for

int j = (i << 1)*i + (i << 2) + (i << 1) + 3;

as for

int j = 2*i*i + 4*i + 2*i + 3;

The latter more clearly conveys your intent (and you could collect 4*i + 2*i to simply 6*i).  Don't attempt to micro-optimize this!

Prefer range-based for

Instead of counting elements like this:

int count;
for (int i = 0; i<R2; i++) if (primes[i] == 0) count++;

Here's a simpler form:

std::size_t count = 0;
for (auto n: primes)
    count += !n;


Buttons switches in Delphi I have an action on four buttons. I need to make a down (checked) button if it's pressed and other buttons up (unchecked). My button is TToolButton and the property Style is set to tbsCheck.

Could you please suggest improvements?

LButtonName := (Sender as TAction).ActionComponent.Name;
   if LButtonName.Equals('btnVorigeWeek') then
      begin
        LSelectionType := 2;
        btnHuidigeWeek.Down := False;
        btnVorigeWeek.Down := True;
        btnHuidigeMaand.Down := False;
        btnVorigeMaand.Down := False;
      end
      else if LButtonName.Equals('btnHuidigeMaand') then
      begin
        LSelectionType := 3;
        btnHuidigeWeek.Down := False;
        btnVorigeWeek.Down := False;
        btnHuidigeMaand.Down := True;
        btnVorigeMaand.Down := False;
      end
      else if LButtonName.Equals('btnVorigeMaand') then
      begin
        LSelectionType := 4;
        btnHuidigeWeek.Down := False;
        btnVorigeWeek.Down := False;
        btnHuidigeMaand.Down := False;
        btnVorigeMaand.Down := True;
      end
      else
      begin
        LSelectionType := 1;
        btnHuidigeWeek.Down := True;
        btnVorigeWeek.Down := False;
        btnHuidigeMaand.Down := False;
        btnVorigeMaand.Down := False;
      end;

It seems I found better solution. All these buttons are located on ToolBar.

I can loop through the ToolBar and utilize the Name property on the buttons to compare with. So, I've changed it to:

  var
    LSelectionType, i: Integer;
    LButtonName: string;
  begin
    LSelectionType := 1;
    LButtonName := (Sender as TAction).ActionComponent.Name;
    for i := 0 to ToolBar.ButtonCount -1 do
    begin
      ToolBar.Buttons[i].Down := (ToolBar.Buttons[i].Name = LButtonName);
      if ToolBar.Buttons[i].Down then
        LSelectionType := i + 1;
    end;
    ...
  end;

UPDATE

Finally, I set proper Tag property and set property Grouped to True for buttons and changed code:

  var
    LSelectionType: Integer;
    LActionComponent: TComponent;
  begin
    LSelectionType := 1;
    LActionComponent := (Sender as TAction).ActionComponent;

    if LActionComponent is TToolButton then
    begin
      (LActionComponent as TToolButton).Down := True;
      LSelectionType := (LActionComponent as TToolButton).Tag;
    end;
    // and using LSelectionType
    ....
  end;


Flagging system: Check for changing values in a data frame I've written the following code to check if a value in a data frame changes. I'm looking at the last 5 values. If there was no change at all I want my code to return 1, if a single one (or multiple) of the last 5 are different to the value that is being checked return 0. Finally I want the returned values in a new column in my data frame.

Here's my code so far. It works but I think there is a nicer (and more clean) way to do it.

mydata <- data.frame("id" = 1:100, "ta" = c(sample(x = c(-5:20), size = 94, replace = T), rep(1,6))) # include a repetition to check if code works
nums <- mydata$id # create a dummy for iteration
qc_dummy <- vector(mode = "list", length = length(nums)) # create a dummy vector for the values computed in the for loop

for(i in 1:length(nums)) {
  qc_dummy[[i]] <- ifelse(mydata[nums[i], 2] - mydata[nums[i-1], 2] == 0,
                          ifelse(mydata[nums[i], 2] - mydata[nums[i-2], 2] == 0,
                                 ifelse(mydata[nums[i], 2] - mydata[nums[i-3], 2] == 0,
                                        ifelse(mydata[nums[i], 2] - mydata[nums[i-4], 2] == 0,
                                               ifelse(mydata[nums[i], 2] - mydata[nums[i-5], 2] == 0, 1, 0) ,0), 0) ,0) ,0)
}
mydata$qc1 <- as.vector(c(0,unlist(qc_dummy))) # first value of list is skipped by unlist (logi(0)) -> add 0

I reduced the example data, for easier viewing

# new example data:
mydata <- data.frame(ta = 1:13)
mydata[2:3, 1] <- 1L
mydata[6:12, 1] <- 2L

n <- 3 # how many equal values we need

require(data.table)
setDT(mydata) # convert to data.table
mydata
mydata[, mathcPrev := fifelse((ta - shift(ta, 1)) == 0L, T, F, F)]
mydata[, g := cumsum(!mathcPrev)] # grouping value, if value has changed
mydata[, count := cumsum(mathcPrev), by = g]
mydata[, qc2 := fifelse(count >= n, 1L, 0L)]
mydata
#     ta mathcPrev g count qc2
#  1:  1     FALSE 1     0   0
#  2:  1      TRUE 1     1   0
#  3:  1      TRUE 1     2   0
#  4:  4     FALSE 2     0   0
#  5:  5     FALSE 3     0   0
#  6:  2     FALSE 4     0   0
#  7:  2      TRUE 4     1   0
#  8:  2      TRUE 4     2   0
#  9:  2      TRUE 4     3   1
# 10:  2      TRUE 4     4   1
# 11:  2      TRUE 4     5   1
# 12:  2      TRUE 4     6   1
# 13: 13     FALSE 5     0   0

So, the idea is to create index mathcPrev, that shows if this value matches previous, and then we can count how many equal values we have in a row.


method to get finger mask based on finger position Is there a more graceful way to structure the code below?

The fingerPosition values are increments of 1.

The fingerMask values start at 1, then 2 and then increment in multiples of 2.

    private int getFingerMask(int fingerPosition)
    {
        int fingerMask = 0;

        if (fingerPosition == 0)
        {
            fingerMask = 1;
        }
        else if (fingerPosition == 1)
        {
            fingerMask = 2;
        }
        else if (fingerPosition == 2)
        {
            fingerMask = 4;
        }
        else if (fingerPosition == 3)
        {
            fingerMask = 8;
        }
        else if (fingerPosition == 4)
        {
            fingerMask = 16;
        }
        else if (fingerPosition == 5)
        {
            fingerMask = 32;
        }
        else if (fingerPosition == 6)
        {
            fingerMask = 64;
        }
        else if (fingerPosition == 7)
        {
            fingerMask = 128;
        }
        else if (fingerPosition == 8)
        {
            fingerMask = 256;
        }
        else if (fingerPosition == 9)
        {
            fingerMask = 512;
        }

        return fingerMask;
    }

Yes, this can be done simply by using a power of 2.

private int getFingerMask(int fingerPosition)
{
    if (fingerPosition >= 0 && fingerPosition < 10)
    {
        return (int)Math.Pow(2, fingerPosition);
    }
    else
    {
        return 0;
    }
}


Using flexbox to replace fixed and absolute positioning I'm using flexbox to create a simple header, hero image with caption and footer.

As you can see from my snippet, the header is fixed and the hero image scrolls underneath.

Is there a way to achieve my layout without fixed or absolute positioning? I'm relatively new to flexbox and and haven't come across a solution.

For the image, I could use the background property, but I'd like to maintain what I have to easily implement img srcset and an alt tag.

body {
  margin: 0;
}

/* Flexbox */
.main-navigation,
.main-navigation ul,
.hero {
 display: flex;
 flex-flow: row wrap;
}

.main-navigation {
 justify-content: space-between;
 align-items: center;
}

/* Site header */
.site-header {
  position: fixed;
 top: 0;
 z-index: 9999; /* Always on top */
 width: 100%; /* Must have width for flex */
  padding: 24px;
  color: #fff;
  background-color: #021928;
}

/* Hero */
.hero {
 position: relative; /* For image */
 height: 100vh; /* Fill viewport */
  justify-content: center;
 align-items: center;
}

.hero img {
  position: absolute;
 top: 0;
 left: 0;
 width: 100%;
 z-index: -1; /* Send image to back */
 height: 100vh;
 object-fit: cover;
 object-position: bottom;
}

/* Cosmetic */
ul {
  margin: 0;
  padding: 0;
  list-style: none;
}

li {
 margin-right: 12px;
}

li:last-child {
  margin: 0;
}

a,
a:visited,
a:focus,
a:hover {
  color: #fff;
}

p {
  padding: 24px;
}

.caption {
  padding: 0;
}

h1 {
  margin: 0 0 12px;
}

.logo {
  margin: 0;
}

.caption {
  max-width: 600px;
 text-align: center;
  color: #fff;
}

.site-footer {
  padding: 24px;
  background-color: #021928;
  color: #fff;
}

Logo

Item One

Item Two

Item Three

Title goes here

And a caption

Main page content

  Some information about stuff

If you want the header to be "sticky" the only way is to make it position:fixed. In order to to push the rest of the content down, you need to know the headers height. Then you set a margin-top to your #hero.


Node.js object-oriented controllers I've been experimenting with the Express Node.js framework. On the face of it, the approach of passing functions to app.VERB methods seems unusual. In other frameworks I've used (in languages other than javascript), you create a single handler class for each url pattern, with methods representing different HTTP verbs. One advantage is the ability to bundle common functionality in methods of superclasses.

I've tried to replicate this in node, but I'm interested in feedback on whether this seem like a good approach.

(I realise that the naming of "as_view" doesn't make much sense - I just named it that way because I'm used to django, where the term "view" is used to refer to something akin to a controller rather than a template.)

function Controller(verbs) {
    for (verb in verbs) {
        this[verb] = verbs[verb];
    }
}
Controller.prototype.as_view = function() {
    var dispatcher = function(req, res) {
        var verb = req.method.toLowerCase();
        this[verb](req, res);
    };
    return dispatcher.bind(this);
};

var pageone = new Controller({

    get: function(req, res) {
        res.send("Get request");
    },

    post: function(req, res) {
        res.send("Post request");
    }
});

....

app.all('/pageone', pageone.as_view());
....

This looks good to me,

As you mentioned, as_view stands out a bit, but I see where you are coming from
I am not sure why you do not declare GET and POST, then you do not need to use toLowerCase(), are you worried the caller will now uppercase the verbs?
You should think about error handling, you should have some framework for it

All in all, I would still suggest that learning and using Express.js might be worth it.


(Codewars) Linked Lists-Sorted Insert Kata: https://www.codewars.com/kata/linked-lists-sorted-insert

Description

  Write a SortedInsert() function which inserts a node into the correct location of a pre-sorted linked list which is sorted in ascending order. SortedInsert takes the head of a linked list and data used to create a node as arguments. SortedInsert() should also return the head of the list.

sortedInsert(1 -> 2 -> 3 -> null, 4) === 1 -> 2 -> 3 -> 4 -> null)
sortedInsert(1 -> 7 -> 8 -> null, 5) === 1 -> 5 -> 7 -> 8 -> null)
sortedInsert(3 -> 5 -> 9 -> null, 7) === 3 -> 5 -> 7 -> 9 -> null)

My Solution

def sorted_insert(head, data):
    prev = None
    node_j = head
    node_i = Node(data)
    while node_j:
        if node_j.data > data:
            node_i.next = node_j
            break
        prev = node_j
        node_j = node_j.next
    else:
        node_i.next = None
    if prev:
        prev.next = node_i
    return head if prev else node_i

First some minor issues with naming, then a rewrite:

prev could be previous, there is no need to skimp on characters. node_j and node_i are completely different things, yet their names suggest they are both "moving pointers". That is only true for node_j. May I suggest using current and insert instead?

The use of while..else is pretty cool, but confused me at first. Take that with a grain of salt, though, I'm not usually writing a lot of python.

Now for the meat of the problem:

This can be simplified by inverting the logic on your traversal. Consider the following code:

def sorted_insert(head, data):
    if head is None:
        return Node(data)
    if data < head.data:
        new_head = Node(data)
        new_head.next = head
        return new_head
    # at this point we will always return head
    current_node = head
    # advance if next node is smaller than node to be inserted
    while current_node.next is not None and current_node.next.data < data:
        current_node = current_node.next

    insert = Node(data)
    insert.next = current_node.next
    current_node.next = insert
    return head

This is an improvement over the code you presented because it separates special cases from traversal.

We first handle the special case of an empty list (head is None).
Then we handle the case where we create a new head (data < head.data)

With these special cases out of the way we now search for the insertion position, namely what you store in prev. The way this works is by advancing current_node only if the next node also has a smaller data than the insertion.

This simplification allows us to eliminate a variable at the cost of a somewhat more difficult to understand loop condition. Overall this tradeoff is worth it, because we reduce the complexity of the loop body.

After we found the insertion position, the insertion itself becomes a matter of setting the properties in the correct order to avoid dropping the tail of the list.


Type safe variadic cout implementation I use console a lot, so I thought this may help me. It's supposed to be simple, safe, and fast. Any ideas?

http://ideone.com/INXub0

#include <iostream>
using namespace std;

class Console
{
public:
    template<typename ...Args>
    static bool WriteLine(const std::string& format, Args&&... args)
    {
        bool success = write(format.c_str(), std::forward<Args>(args)...);
        std::cout << std::endl;
        return success;
    }

    template <typename ...Args>
    static bool Write(const std::string& format, Args&&... args)
    {
        return write(format.c_str(), std::forward<Args>(args)...);
    }

    template<typename T>
    static void WriteLine(T&& value)
    {
        std::cout << std::forward<T>(value) << std::endl;
    }

    template<typename T>
    static void Write(T&& value)
    {
        std::cout << std::forward<T>(value);
    }

private:
    template<typename ...Args>
    static bool write(const char * format, Args&&... args)
    {
        while (*format)
        {
            if (*format == '{')
            {
                bool found_closing_brace = false;
                short value_position = -1;

                while (*(++format))
                {
                    if (*format == '}')
                    {
                        if (value_position == -1)
                            return false;  // {} is not allowed.

                        write_value(value_position, 0, std::forward<Args>(args)...);
                        found_closing_brace = true;
                        break;
                    }
                    else
                    {
                        if (value_position >= 10)
                            return false; //Only range {0 ~ 99} is allowed.

                        if (value_position == -1)
                            value_position = *format - '0';
                        else
                            value_position = (value_position * 10) + (*format - '0');
                    }
                }

                if (!found_closing_brace)
                    return false;

                // Continue back to the main loop. This is required.
                // We need to process the next character, because it could be a '\0' or a '{'
                format++;
                continue;
            }

            std::cout << *format;
            format++;
        }

        return true;
    }

    template<typename T, typename ...Args>
    static void write_value(int x, int i, T&& value, Args&&... values)
    {
        if (i == x)
            std::cout << std::forward<T>(value);
        else
            write_value(x, ++i, std::forward<Args>(values)...);
    }

    static void write_value(int x, int i) { }
};

int main()
{
    Console::WriteLine("Big {11} Bang {0} Theory {6} [{11}, {12}]",

        "Zero", "One", "Two", "Three", "Four", "Five",
        "Six", "Seven", 8, "Nine", "Teen", 11, 12.5f);

    // your code goes here
    return 0;
}

You must allow the escape character for { and } otherwise this doesn't works.

Console::WriteLine("int {0}(){ return 0; }", function_name);

Apart from that I would add an override to specify the output stream.

std::ofstream logger("log.err");
Console::WriteLine(logger, "String");

It must be a class? Probably it's only a style issue, but I don't like class with only static methods. I prefer namespace and a console_details namespace for private functions.


Inputting multiple strings and finding their length I need to find the length of elements within a string and return it to the user.

What is the best way of simplifying this code to make the amount of string lengths equate to user input? Also, how can I replace string1 with the user input, such as "The length of string 'Dog' is 3"?

num_of_items = int(input("How many items in your list? "))
def item_list(x):

    items = []
    for i in range(0, x):
        item = input("Enter list %r: " % (i+1))
        items.append(item)
        length_items = ''.join(items)

    return "Itemlist: %r" %(items), "Total length: %s"% len(length_items), \
    "The length of string 1 is %r"%len(items[0]),\
    "The length of string 2 is %r"%len(items[1]),\
    "The length of string 3 is %r"%len(items[2]),\
    "The length of string 4 is %r"%len(items[3])

print(item_list(num_of_items))

num_of_items = int(input("How many items in your list? "))

For one thing, what if the user typed alpha?  You would have a nasty error throwing itself at the user.  Why are you asking anyway?  You should be able to handle any number of items without asking.  Just keep asking input and printing output until the user decides to end with exit.  Even as it is, your item_list() function will print only the first four and if num_of_items is, say, three ... IndexError.

num_of_items ...
def item_list(x):
    ...

That's a little squooshed.  I would add at least one blank line and probably two between the num_of_items definition and the item_list() definition.  Well, actually I would put the num_of_items definition below the function definition.  Usually, constants are defined first; then functions and classes; then variables.

item = input("Enter list %r: " % (i+1))

% formatting is not officially deprecated, but it is preferred to use .format()

I wouldn't create a list of strings whose sole purpose is to be displayed to the user.  There are some things such as get_accuracy(user) where you might not use it elsewhere, but it makes sense to use it programmatically.  In this case, you are returning strings that couldn't be read by your program very easily.  Your item_list() function should not return strings that are formatted just for the reader.  It should return something that you can format however you like elsewhere.  My modified program looks like this:

def get_items():
    while True:
        item = input("Enter string: ")
        if item.lower() == "exit":
            raise StopIteration()
        yield item, len(item)

for item in get_items():
    print("The length of string '{0}' is {1}".format(*item))

get_items() is now a generator function that yields tuples of each item and its length.  Since it raises StopIteration when the user types exit, the for loop will stop.


NodeJS APIs folder structure I am going to have multiple APIs in routes folder should I put them in in the same file API or separate them?

├── app.js
├── src/
│   ├── contants/
│   ├── helpers/
│   ├── models/
│   ├── routes/
|   |         |___index.js
              |___api.js
│   └── libs/
│       ├── backbone/
│       ├── underscore/
│       └── ...

api.js file contains all the APIs
    const jwt = require("jsonwebtoken")
    const axios = require("axios")
    require("express-async-errors")
    const bodyParser = require("body-parser")
    const fs = require("fs")

    const LOLTrackingSystem = require("../methods/onlineGamesTracking/LOLTracking")
    const getUserData = require("../methods/leagueOfLegends/getUserData")
    const isAuthenticated = require("../helpers/authenticated")

    const apiRoute = (api) => {
      api.use(bodyParser.json())
      api.use(bodyParser.urlencoded({
        extended: false
      }));

      api.post("/api/auth", (req, res) => {
        //API Functions
      })

      api.post("/api/gizmo/memberProfile", isAuthenticated, (req, res) => {
        //API Functions
      })

      api.post("/api/gizmo/memberState/:userId/:host/:state", async (req, res) => {
        //API Functions
      })
    }

    module.exports = apiRoute

Is what I am doing is right?

If it's wrong what is the right way to do it?

there is no right in wrong in this situation it's a question of what's best for your particular situation if you are going to build many Rest API endpoints it's best to separate them in separate files under routes like this so your code can be more maintainable :

│   ├── routes/
|   |         |___index.js
              |___auth.js
              |___gizmo.js
                          -


Computing the Ackermann Function How can I improve this code? It computes the Ackermann function as long as m<4 and n<13.

#include <iostream>
#include <limits>

// Ackermann function calculations
unsigned int ackermann(unsigned int m, unsigned int n){
    if(m == 0)
        return n+1;
    if(n == 0)
        return ackermann(m-1,1);
    return ackermann(m-1,ackermann(m,n-1));
}

// Check for non-integer input
bool inputCheck(){
    if(!std::cin.fail())
        return false;

    std::cout << "Invalid input!" << std::endl;
    return true;
}

// Check for negative or overflow errors
bool numCheck(int i, char name){
    if(i < 0){
        std::cout << "Negative error!" << std::endl;
        return true;
    }

    if(name == 'm' && i > 3){
        std::cout << "Overflow error (m > 3)!" << std::endl;
        return true;
    }

    if(name == 'n' && i > 12){
        std::cout << "Overflow error (n > 12)!" << std::endl;
        return true;
    }

    return false;
}

// Run input and num checks
bool check(int x, char y){
    bool result = inputCheck() || numCheck(x, y);

    std::cin.clear();
    std::cin.ignore();

    return result;
}

int main(){

    int m, n;
    bool valM, valN;

    do{
        std::cout << "m = ";
        std::cin >> m;
        valM = check(m, 'm');
    } while(valM);

    do{
        std::cout << "n = ";
        std::cin >> n;
        valN = check(n, 'n');
    } while(valN);

    std::cout << "\nM = " << m << "\nN = " << n
            << "\n\nCALCULATING..." << std::endl;

    std::cout << "A(" << m << ',' << n << ") = "
            << ackermann(m,n) << std::endl;

    return 0;
}

A majority of the code is for detecting errors (such as invalid or negative input) and for preventing overflow errors (such as Ackermann(4,2)). A minority of the code actually calculates the Ackermann function.

bool inputCheck(){
    if(!std::cin.fail())
        return false;

    std::cout << "Invalid input!" << std::endl;
    return true;
}

Carefully name your functions with a meaningful name.  inputCheck doesn't really tell anyone what to expect as a result.

Prefer to distinguish language constructs with spaces.

You can use the contextual boolean conversion operator to check the if the stream has failed rather than explicitly calling !std::cin.fail().

Prefer to avoid std::endl.  Be aware of what the manipulator std::endl actually does.  Stream '\n' as it explicitly states your intent, correctly outputs the end-of-line character, and is shorter to type.

bool isInvalidInput() {
    if (std::cin) {
        return false;
    }
    std::cout << "Invalid input!\n";
    return true;
}

bool numCheck(int i, char name){
    if(i < 0){ /* ... */ }
    if(name == 'm' && i > 3){ /* ... */ }
    if(name == 'n' && i > 12){ /* ... */ }
    return false;
}

Specify const for immutable variables.  const self-documents that a variable should not change values in the current scope and any accidental modification to the value is detected at compile time rather than run time.

Prefer enumerations to represent sets of related named constants.

Avoid magic constants as they are difficult to understand and may be overlooked.  Prefer symbolic constants to give values contextual meaning.

bool check(int x, char y){
    bool result = inputCheck() || numCheck(x, y);
    std::cin.clear();
    std::cin.ignore();
    return result;
}

Do you really want to ignore the remaining buffer on success?

unsigned int ackermann(unsigned int m, unsigned int n){
    if(m == 0)
        return n+1;
    if(n == 0)
        return ackermann(m-1,1);
    return ackermann(m-1,ackermann(m,n-1));
}

The Ackermann–Péter function should be tail-call optimized by any decent compiler, so you won't find much improvement with the recursive approach.  If you really care for performance, calculate Ackerman values in constant time using the formula's for \$A(m,n)\$.

$$
A(0,n) = n + 1\\
A(1,n) = n + 2\\
A(2,n) = 2n + 3\\
A(3,n) = 2^{(n+3)} - 3\\
A(4,0) = 13\\
A(4,1) = A(5,0) = 65533
$$

Contractually enforce your preconditions by testing them in the function that requires them.

namespace impl {
    unsigned Ackermann(unsigned m, unsigned n) {
        // calculate A(m,n)
    }
    void check_overflow_bounds(unsigned m, unsigned n) {
        if (m > 3 || n > 12) {
            throw std::out_of_bounds("");
        }
    }
}
unsigned Ackermann(unsigned m, unsigned n) {
    impl::check_overflow_bounds(m, n);
    return impl::Ackermann(m, n);
}

If your return type is an unsigned int, will Ackermann values overflow if \$m < 4\$ and \$n = 13\$?  Are there Ackermann values that don't overflow when \$m = 4\$ or \$m = 5\$?  Consider what actually is computable and throw an overflow exception for values that are not computable.


Project Euler #85: Find the rectangular grid with closest to 2M rectangles For one place that I interviewed at (for a Python developer position) I worked with one of the devs on two Project Euler problems, one being problem 85. We talked through different approaches and came up with solutions together, but we coded separately. He seemed impressed that we were able to get through two problems. After the interview, he asked me to clean up the code and submit it, which you can see below. He never got back to me after I sent him the code. I'd like to know what's wrong with my code and not make the same mistakes again.

Problem 85

  By counting carefully it can be seen that a rectangular grid measuring 3 by 2 contains eighteen rectangles:

  Although there exists no rectangular grid that contains exactly two million rectangles, find the area of the grid with the nearest solution.

# problem85.py
# Project Euler
# Nicolas Hahn

# int m, int n -> # of rectangles that can be made from m*n rectangle
def numRectangles(m,n):
    return (m*n*(m+1)*(n+1))//4

# int 'num' -> side length of first square that has >='num' rectangles
def getLargestSquareLength(num):
    rectNum = 0
    length = 0
    while rectNum < num:
        length += 1
        rectNum = numRectangles(length,length)
    return length

# int 'num' -> area of rectangle with closest to 'num' rectangles
def closestTo(num):
    rects = []
    # start from a square, work towards a 1*n rectangle
    length = getLargestSquareLength(num)
    for i in range(1,length+1):
        m = length-i
        n = m+1
        # find closest rectangle to 'num' with width m
        while numRectangles(m,n) < num and m > 0:
            n += 1
        # store both the >num rectangle and <num rectangle with width m
        rects.append((m,n,numRectangles(m,n)))
        rects.append((m,n-1,numRectangles(m,n-1)))
    # get closest number of rectangles, then compute area
    m,n,r = sorted(rects, key=lambda k: abs(k[2]-num))[0]
    return m*n

def main():
    # correct answer = 2772
    print(closestTo(2000000))

if __name__ == "__main__":
    main()

Well, let's ignore the code, because having started coding at that stage is wrong.

Let's look instead at a better starting-point:

Find a closed formula for the number of rectangles in a grid of given size (x, y):

N = \$\sum_{a=1}^x\sum_{b=1}^y(x-a+1)(y-b+a)\$
\$= \sum_{a=0}^{x-1}\sum_{b=0}^{y-1}(x-a)(y-b)\$
\$ = \frac 1 4 * (2x^2 - x*(x-1))(2y^2 - y*(y-1))\$
\$= \frac 1 4 * (x^2+x)(y^2+y)\$
Solve it for given y and N:

\$x^2+x-\frac{4*N}{y^2+y} = 0\$
\$x=\frac{-1 \pm \sqrt{1 + \frac{4*4*N}{y^2+y}}}2\$
\$x=-\frac 1 2 \pm \sqrt{\frac 1 4 + \frac{4*N}{y^2+y}}\$

(Only the positive solution is of interest)
Simply iterate all possible rectangles starting with short side of 1 until our short dimension becomes the long one, and calculate the number of rectangles for a slightly bigger / smaller rectangle than the fractional one we calculated.

The solution will be found in \$\Theta\left(\sqrt[4]N\right)\$.


Opening an XML File and Returning the XmlDocument I'm working on my first Windows Forms tool for a project; this is my first time using C# extensively. Right now, my only objective is to be able to open an XML file and have the resulting XML in hand. I know this works, but I want to know if there's a better, or proper, way to do it.

There is also the issue that when I was first trying to load it, it didn't like the fact that I was using if statements, so the return class was giving me an error that said that not all of the branches returned a value.  So, I had to throw in a messy else statement that I don't like.

One thing to note; if the empty XML file is returned, I'll have a statement for handling the returned XML document to do nothing with it.

class LoadXML
{
    public XmlDocument LoadXML_Window(object sender, System.EventArgs e)
    {
        //  The stream will hold the results of opening the XML
        Stream myStream = null;
        OpenFileDialog openFileDialogXML = new OpenFileDialog();

        openFileDialogXML.InitialDirectory = "C:\\";
        openFileDialogXML.Filter = "XML Files|*.xml";
        openFileDialogXML.RestoreDirectory = true;

        if(openFileDialogXML.ShowDialog() == true)
        {
            myStream = openFileDialogXML.OpenFile();
            using (myStream)
            {
                try
                {
                    //  Successfully return the XML
                    XmlDocument parsedMyStream = new XmlDocument();
                    parsedMyStream.Load(myStream);
                    return parsedMyStream;
                }
                catch (XmlException ex)
                {
                    MessageBox.Show("The XML could not be read. " + ex);
                    XmlDocument emptyMyStream = new XmlDocument();
                    return emptyMyStream;
                }
            }
        }
        else
        {
            //  Return an empty XmlDocument if the open file window was closed
            XmlDocument emptyMyStream = new XmlDocument();
            return emptyMyStream;
        }
    }
}

Here's how I would refactor your code:

An event handler sholdn't return any values, it's better to save the result in a property or a field in this case.

  √ DO use a return type of void for event handlers.
  An event handler can invoke multiple event handling methods, possibly on multiple objects. If event handling methods were allowed to return a value, there would be multiple return values for each event invocation.

Event Design

You shouldn't do too much work inside an event handler, it's better to call a specialized method that can also be reused elsewhere later.
OpenFileDialog needs to be disposed: see Component.Dispose Method which is one of the base class of this dialog.
If you check the if(!openFileDialogXML.ShowDialog()) for a false value and immediately return a default value you can reduce nesting.
If possible it's better to name a class with a noun (e.g. XmlLoader) rather then like a method LoadXml.
Now that the unnecessary nesting is removed you can remove the Stream myStream = null; and put it inside the using.
The last step would be to replace the repeating creation of an empty XmlDocument by a method and call it where necessary. You might want to add some default elements later so now you can do it in only one place. If the XmlDocument is no longer empty you might rename the method to CreateDefaultXmlDocument but it's important that you do it only once. You could forget to adjust the other lines if your code repeates.

class XmlLoader
{
    public XmlDocument XmlDocument { get; private set; }

    private XmlDocument CreateEmptyXmlDocument()
    {
        //  Return an empty XmlDocument if the open file window was closed
        XmlDocument emptyMyStream = new XmlDocument();
        return emptyMyStream;
    }

    public XmlDocument OpenXml()
    {
        using(var openFileDialogXML = new OpenFileDialog())
        {
            openFileDialogXML.InitialDirectory = "C:\\";
            openFileDialogXML.Filter = "XML Files|*.xml";
            openFileDialogXML.RestoreDirectory = true;

            if(!openFileDialogXML.ShowDialog())
            {
               return CreateEmptyXmlDocument();
            }

            //  The stream will hold the results of opening the XML
            using (var myStream = openFileDialogXML.OpenFile())
            {
                try
                {
                    //  Successfully return the XML
                    XmlDocument parsedMyStream = new XmlDocument();
                    parsedMyStream.Load(myStream);
                    return parsedMyStream;
                }
                catch (XmlException ex)
                {
                    MessageBox.Show("The XML could not be read. " + ex);
                    return CreateEmptyXmlDocument();
                }
            }
        }
    }

    public void LoadXML_Window(object sender, System.EventArgs e)
    {
        XmlDocument = OpenXml();
    }
}


Get all points on a uniform discrete grid inside a circle's radius I have a 2-dimensional grid defined by a known point gridCenter and distance between points gridStep and need to find all points on this grid that are inside or on the radius of a circle defined by center and radius.

My solution is to use a brute-force approach like so:

public static IEnumerable<Vector2> GetPointsInCircle(Vector2 circleCenter, float radius, Vector2 gridCenter, Vector2 gridStep)
{
    if (radius <= 0)
    {
        throw new ArgumentOutOfRangeException("radius", "Argument must not be negative.");
    }

    if (gridStep.x <= 0 || gridStep.y <= 0)
    {
        throw new ArgumentOutOfRangeException("gridStep", "Argument must not contain negative components.");
    }

    var minimumPoint = GetClosestGridPoint(new Vector2(circleCenter.x - radius, circleCenter.y - radius), gridCenter, gridStep);
    var maximumPoint = GetClosestGridPoint(new Vector2(circleCenter.x + radius, circleCenter.y + radius), gridCenter, gridStep);

    for (var x = minimumPoint.x; x <= maximumPoint.x; x += gridStep.x)
    {
        for (var y = minimumPoint.y; y <= maximumPoint.y; y += gridStep.y)
        {
            var point = new Vector2(x, y);

            if (Vector2.Distance(point, circleCenter) <= radius)
            {
                yield return point;
            }
        }
    }
}

private static Vector2 GetClosestGridPoint(Vector2 point, Vector2 gridCenter, Vector2 gridStep)
{
    var leftGridPoint = gridCenter.x + Mathf.Floor((point.x - gridCenter.x) / gridStep.x) * gridStep.x;
    var rightGridPoint = leftGridPoint + gridStep.y;
    var closestHorizontalGridPoint = (point.x - leftGridPoint) > (rightGridPoint - point.x) ? rightGridPoint : leftGridPoint;

    var bottomGridPoint = gridCenter.y + Mathf.Floor((point.y - gridCenter.y) / gridStep.y) * gridStep.y;
    var topGridPoint = bottomGridPoint + gridStep.y;
    var closestVerticalGridPoint = (point.y - bottomGridPoint) > (topGridPoint - point.y) ? bottomGridPoint : topGridPoint;

    return new Vector2(closestHorizontalGridPoint, closestVerticalGridPoint);
}

And usage (and unit test) are as follows:

[Test]
public void GetPointsInCircleTest()
{
    var results = AdaptiveSpatialGrid2D<int>.GetPointsInCircle(new Vector2(0.1f, 0.1f),
        1,
        new Vector2(0, 0),
        new Vector2(1, 1));

    var expected = new List<Vector2>()
    {
        new Vector2(0,0),
        new Vector2(0,1),
        new Vector2(1,0) //(1,1) is outside of circle radius and invalid
    };

    CollectionAssert.AreEquivalent(expected, results);
}

This method will be executed frequently (probably once a second) inside of a game and needs to be as light-weight as possible. Typically the grid will have few points inside of the circle at any time (in the region of fifty).

Are there any optimizations I can make to improve execution speed?

Additionally, the code currently resides inside my grid's class as a static method, but I wonder if it would be better placed somewhere more generic, as really it applies to any 2D grid, not just my implementation.

General comments on the code are also welcome.

The code:

private static IEnumerable<Vector2> GetPointsInCircle(Vector2 circleCenter, float radius,
    Vector2 gridCenter, Vector2 gridStep)
{
    if (radius <= 0)
    {
        throw new ArgumentOutOfRangeException("radius", "Argument must be positive.");
    }
    if (gridStep.x <= 0 || gridStep.y <= 0)
    {
        throw new ArgumentOutOfRangeException("gridStep", "Argument must contain positive components only.");
    }

    // Loop bounds for X dimension:
    int i1 = (int)Math.Ceiling((circleCenter.x - gridCenter.x - radius) / gridStep.x);
    int i2 = (int)Math.Floor((circleCenter.x - gridCenter.x + radius) / gridStep.x);

    // Constant square of the radius:
    float radius2 = radius * radius;

    for (int i = i1; i <= i2; i++)
    {
        // X-coordinate for the points of the i-th circle segment:
        float x = gridCenter.x + i * gridStep.x;

        // Local radius of the circle segment (half-length of chord) calulated in 3 steps.
        // Step 1. Offset of the (x, *) from the (circleCenter.x, *):
        float localRadius = circleCenter.x - x;
        // Step 2. Square of it:
        localRadius *= localRadius;
        // Step 3. Local radius of the circle segment:
        localRadius = (float)Math.Sqrt(radius2 - localRadius);

        // Loop bounds for Y dimension:
        int j1 = (int)Math.Ceiling((circleCenter.y - gridCenter.y - localRadius) / gridStep.y);
        int j2 = (int)Math.Floor((circleCenter.y - gridCenter.y + localRadius) / gridStep.y);

        for (int j = j1; j <= j2; j++)
        {
            yield return new Vector2(x, gridCenter.y + j * gridStep.y);
        }
    }
}

Comments:

If step values in the gridStep aren't supposed to be close to each other, this method could be rewritten to select a dimension with the largest step's value for the outer loop.
I've used integer loop variables to avoid accumulation of errors.
This code has been successfully tested in comparison with the brute force method.
This method is ~20% faster on my machine. Efficiency of the method depends on the number of grid points inside a circle. More points - more efficiency.


How Can I Improve/Secure My Login Script? This is a simple login script. Thought I'm not sure when it's appropriate to use includes over just embedding the code. I'm also not sure if I should make cookies, but I guess I don't really need to right now. Also looking at my code would it be easy to create a 'Remember Me' function?

<?php
$validUser;
$validPass;
$win;
if($_POST)
{
include('admin/functions.php');
// Database Connection
AllYourBase();

if(CheckEmpty($_POST['username']))
{
    $username = $_POST['username'];
    $validUser = true;
}
else
    $validUser = false;

if(CheckEmpty($_POST['password']))
{
    $password = $_POST['password'];
    $validPass = true;
}
else
    $validPass = false;

$query = mysql_query(sprintf("SELECT * FROM Users WHERE username = '%s'",
                mysql_real_escape_string($username))) or die(exit());
$result = mysql_fetch_assoc($query);

// Split the Salt
$fs = substr($result['salt'], 0, 10);
$ls = substr($result['salt'], -10);

// Add The Salt
$pass = md5($fs.$password.$ls);
$name = $result['fname']. ' ' . $result['lname'];

// Authenticate
if($result['password'] == $pass)
{
    session_start();
    $_SESSION['name'] = $name;
    $_SESSION['auth'] = true;
    $win = true;
    // Javascript Redirect not Header()
    Redirect('index.php');
}
else
    $win = false;
}
?>

<form action="<?php echo $_SERVER['PHP_SELF'] ?>" method="POST">
    <?php if($_POST && $win == false): ?>

Login Unsuccessful

    <?php endif ?>
        <input type="text" size="10" name="username" />

    <?php if($_POST && $validUser == false): ?>

Empty Username

    <?php endif ?>
        <input type="password" size="10" name="password" />

    <?php if($_POST && $validPass == false): ?>

Empty Password

    <?php endif ?>
    <input type="submit" text="Submit" />
</form>
Registration Page

Rather than an else statement to set false on invalid username/password why not just default to false and only set to true if it worked. Also do you care unless both are valid?

$validUserAndPassword  = false;

if (CheckEmpty($_POST['username']) && CheckEmpty($_POST['password']))
{
    $username = $_POST['username'];
    $password = $_POST['password'];
    $validUserAndPassword = true;
}

The DB is an expensive resources. There is no point in connecting to the DB or even getting stuff from the DB unless you have a valid username and password.

$ result = array();
if ($validUserAndPassword)
{
    AllYourBase();

    $query = mysql_query(sprintf("SELECT * FROM Users WHERE username = '%s'",
                mysql_real_escape_string($username))) or die(exit());
    $result = mysql_fetch_assoc($query);
}

I am pretty sure that splitting the salt provides little extra entropy.

// Split the Salt
$fs = substr($result['salt'], 0, 10);
$ls = substr($result['salt'], -10);

// Add The Salt
$pass = md5($fs.$password.$ls);

I think doing authentication your self is a bad idea (this is a specialized field with lots of gotchas). Anway don't you want to spend time on your application rather than doing mundane tasks like authentication?

I am not an expert on front-ends so I have no good recommendations on packages that can do it for you. But I did play around with openid-selector (available via google source), which is the same authentication package that the stack exchange sites use. This allows the users to login-in to your site using the open-id credentials (google/open-id/facebook/yahoo/stack-exchange)).


Comparing adjacent time stamps I have this code which compares two adjacent time stamps. If they are less than 15 min apart, it adds a new time stamp (row) in between with the new time stamp equal to the previous time + 15 min and value equal to the previous time stamp. If there is more than one-time stamp missing (more than 15 min interval), it adds a number of time stamps with + 15 min interval each. Sample value equal to last recorded value.

CSV file looks something like this:

"Time stamp","Value","Events","Comment","User"
"5/07/2016 7:07:54 PM","7.00","","",""
"5/07/2016 6:37:52 PM","14.00","","",""
"5/07/2016 6:22:51 PM","26.00","","",""
"5/07/2016 6:07:50 PM","9.00","","",""
"5/07/2016 7:22:06 AM","0.00","","",""
"5/07/2016 7:07:05 AM","16.00","","",""
"5/07/2016 6:52:04 AM","25.00","","",""

At the moment it is doing what it suppose to do but in wrong direction, because the .csv file was created from bottom up.

for (var currentRow = 0; currentRow < dgvValues.Rows.Count; currentRow++)
    {
        for (var cellIndex = 0; cellIndex < dgvValues.Rows[currentRow].Cells.Count; cellIndex++)
        {
            if (cellIndex > 0 && cellIndex < dgvValues.Rows[currentRow].Cells.Count)
            {
                buffer += ",";
            }
            buffer += dgvValues.Rows[currentRow].Cells[cellIndex].Value.ToString();
        }
        buffer += "\n";

        pleasewait.LabelText = (currentRow.ToString() + " of " + numRows + " rows processed");
        Application.DoEvents();

        if (currentRow + 1 >= dgvValues.Rows.Count) continue;
        {
            var firstDate = Parse(dgvValues.Rows[currentRow].Cells[0].Value.ToString());
            var secondDate = Parse(dgvValues.Rows[currentRow + 1].Cells[0].Value.ToString());

            var timespan = firstDate - secondDate;
            double min = -15;
            firstDate = firstDate.AddMinutes(min);

            if (timespan.TotalMinutes > 15)
            {
                if (firstDate == secondDate)
                    @equals = true;
                while (timespan.TotalMinutes > 15 && !@equals)
                {
                    timespan = firstDate - secondDate;
                    for (var cellIndex = 0;
                        cellIndex < dgvValues.Rows[currentRow].Cells.Count;
                        cellIndex++)
                    {
                        if (cellIndex > 0 && cellIndex < dgvValues.Rows[currentRow].Cells.Count)
                        {
                            buffer += ",";
                        }
                        if (cellIndex != 0)
                        {
                            buffer += dgvValues.Rows[currentRow].Cells[cellIndex].Value.ToString();
                        }
                        else
                            buffer += Parse(firstDate.ToString(CultureInfo.CurrentCulture));
                    }
                    buffer += "\n";
                    min = -15;
                    firstDate = firstDate.AddMinutes(min);
                }
            }
        }
    }

The code is way too "linear", too deep, too hard to follow with all the indexing and generic column-cell referencing.

I prefer foreach over for.  Simpler, cleaner, easier to read.
For each row, you can reference the DataGridViewCellCollection property by column name-value: theRow.Cells['theColumnName'].Value
Create a method that takes a DataGridViewRow parameter and simply copies its column values into a "CSVrow" object - a class you create.

override ToString and build the CSV string there.

Create a "CSVrowCollection" class to hold the CSV objects.

Its main purpose is to build the CSV file. Do this in a ToString override

.

class CSV {
    public CSV (DataGridViewRow timeStampRow) {  }

    public override string ToString() {
        // note the single quote characters wrapping the complete string
        return string.Format('"{0}","{1}","{2}","{3}","{4}","{5}"',
                            Timestamp, Value, Events, Comment, User);
    }
}

class CSVcollection {
    protected List<CSV> CsvList { get; set; }

    public void Add(CSV aCsvObject) { }

    public override string ToString() {
        // building the value names string not shown.
        StringBuilder me = new StringBuilder();  //System.Text

        foreach (var record in CsvList)
           me.AppendLine(record);  //implicit ToString call

        return me.ToString();
    }
}


The perfect function alias For many years, I have waited for function aliases in C++. They still aren't here. I've been using macros to generate wrapper functions. This is my best attempt at the perfect function alias:

#define FUN_ALIAS(NEW_NAME, ...)                                                \
  template <typename... Args>                                                   \
  inline decltype(auto) NEW_NAME(Args &&... args)                               \
    noexcept(noexcept(__VA_ARGS__(std::forward<Args>(args)...))) {              \
    return __VA_ARGS__(std::forward<Args>(args)...);                            \
  }

I'm using std::forward to forward the arguments and I'm using decltype(auto) to get the right return type. In the future, we might have noexcept(auto) which will be perfect for this situation but I'm not getting my hopes up.

Here's an example from my OpenGL wrapper:

template <GLenum TYPE>
Shader<TYPE> makeShader();
template <GLenum TYPE>
Shader<TYPE> makeShader(const GLchar *, size_t);
template <GLenum TYPE, size_t... SIZES>
Shader<TYPE> makeShader(const GLchar (&... sources)[SIZES]);

FUN_ALIAS(makeVertShader, makeShader<GL_VERTEX_SHADER>)
FUN_ALIAS(makeFragShader, makeShader<GL_FRAGMENT_SHADER>)

What I want to know:

Is there a situation where the compiler will not optimize away the wrapper because the wrapper and the original function have different semantics?
Does this fail to compile for some functions?

Is there a situation where the compiler will not optimize away the wrapper because the wrapper and the original function have different semantics?

Yes to the first part and no to the second. Any optimizing compiler will inline your wrapper, as it consists of only one return statement.

But on the other hand, the original function might have different semantics: It might be constexpr for example, and your alias will not be.

  Does this fail to compile for some functions?

No (except for the constexpr part).

So here are some improvements you can use:

Templates are implicitly inline, so you don't need to write it explicitly.
Use a lambda to combat the missing constexprness of the alias, because since C++17 lambdas are implicitly constexpr if they can be.

This will get you:

#define FUN_ALIAS(NEW_NAME, ...)                                                \
  inline auto NEW_NAME = [](auto &&... args)                                   \
    noexcept(noexcept(__VA_ARGS__(std::forward<decltype(args)>(args)...)))     \
    -> decltype(auto) {                                                        \
    return __VA_ARGS__(std::forward<decltype(args)>(args)...);                \
  };


Encoding multiple rows from prepared statement in php I have this code that hypothetically returns multiple rows from a database and encodes them in a JSON. I am wondering if this is the best and most presentable way to do it.

Note: I am using msqli not PDO, and procedural php not OOP.

<?php
$query = $_REQUEST["query"];
require(connect.php);

$response = array();
$response["success"] = false;

if ($con) {
    $statement = mysqli_prepare($con, "SELECT * FROM `lecturers_general` WHERE ? IN (lecturer_id, title, first_name, middle_name, last_name, course1, course2, course3, course4, course5, course6, office_number, office_building, department)");
    mysqli_stmt_bind_param($statement, "s", $query);
    mysqli_stmt_execute($statement);
    $lecturer_id = null; $title = null; $first_name = null; $middle_name = null; $last_name = null; $course1 = null; $course2 = null; $course3 = null; $course4 = null; $course5 = null; $course6 = null; $office_number = null; $office_building = null; $department = $null;

    mysqli_stmt_bind_result($lecturer_id, $title, $first_name, $middle_name, $last_name, $course1, $course2, $course3, $course4, $course5, $course6, $office_number, $office_building, $department);
    // $rows = mysqli_fetch_assoc($statement);
    $num_rows = mysqli_num_rows($statement);

    if ($num_rows >= 1) {
        do {
            $lecturer_id = $rows["lecturer_id"];
            $title = $rows["title"];
            $first_name = $rows["first_name"];
            $middle_name = $rows["middle_name"];
            $last_name = $rows["last_name"];
            $course1 = $rows["course1"];
            $course2 = $rows["course2"];
            $course3 = $rows["course3"];
            $course4 = $rows["course4"];
            $course5 = $rows["course5"];
            $course6 = $rows["course6"];
            $office_number = $rows["office_number"];
            $office_building = $rows["office_building"];
            $department = $rows["department"];

            $result['lecturers: '][] = array('lecturer_id' => $lecturer_id,'title' => $title,'first_name' => $first_name,'middle_name' => $middle_name,'last_name' => $last_name,'course1' => $course1,'course2' => $course2,'course3' => $course3,'course4' => $course4,'course5' => $course5,'course6' => $course6,'office_number' => $office_number,'office_building' => $office_building,'department' => $department);
        } while($rows = mysqli_fetch_assoc($statement));

        echo json_encode($result);
    } else {
        echo 'No results';
    }
}

?>

I am using msqli not PDO, and procedural php not OOP.

While you should.

Because it will make the whole mess into three blimmin' lines

<?php
require 'connect.php';

$statement = $con->prepare("SELECT * FROM `lecturers_general` WHERE ? IN (lecturer_id, title, first_name, middle_name, last_name, course1, course2, course3, course4, course5, course6, office_number, office_building, department)");
$statement->execute([$_REQUEST["query"]]);
echo json_encode(['lecturers: ' => $statement->fetchAll()]);


Print tree in a zigzag manner I want to print a tree in a zigzag manner level-by-level. I'm wondering if there are any logic issues in my code and any further performance improvement ideas by using a smarter algorithm implementation.

class Node:

    def __init__(self, value):
            self.value = value
            self.left = None
            self.right = None

def zigzagPrint(wq, isLeft):
    if not wq:
        return
    buf = []
    if isLeft == True:
        for i in range(len(wq)):
            buf.append(wq[i].value)
    else:
        for i in range(len(wq)-1, -1, -1):
            buf.append(wq[i].value)
    print buf

def zigzag(root):

    if not root:
        return

    wq = []
    wqTemp=[]
    wq.append(root)
    isLeft = True
    while True:
        zigzagPrint(wq, isLeft)
        isLeft = not isLeft
        while len(wq) > 0:
            node = wq.pop(0)
            if node.left:
                wqTemp.append(node.left)
            if node.right:
                wqTemp.append(node.right)
        # inner while
        wq = wqTemp
        wqTemp = []
        if len(wq) == 0:
            return

if __name__ == "__main__":
    n1 = Node(1)
    n21 = Node(2)
    n1.left = n21
    n22 = Node(3)
    n1.right = n22

    n31 = Node(4)
    n32 = Node(5)
    n21.left = n31
    n21.right = n32
    n33 = Node(6)
    n22.right = n33

    n41 = Node(7)
    n42 = Node(8)
    n31.left = n41
    n31.right = n42
    n43 = Node(9)
    n32.right = n43

zigzag(n1)

Expected output

The tree in the code can be represented as

      1
     / \
    2   3
   / \   \
  4   5   6
 / \   \
7   8   9

And the associated output is

[1]
[3, 2]
[4, 5, 6]
[9, 8, 7]

Use better variable names

Right now, I have no idea what you mean by wq. Also, buf could be spelled out as buffer without loosing anything.

Also, PEP8 recommends snake_case for variables and two blank lines between function definitions.

Better comparisons

You should use the fact that isLeft is already a boolean and use if isLeft: instead of if isLeft == True:

Also, you could use that the empty list evaluates as False and save one call to len:

if len(wq) == 0:
    return

if not wq:
    return

Iterate over object, not its indices

Instead of doing:

for i in range(len(wq)):
    buf.append(wq[i].value)
for i in range(len(wq)-1, -1, -1):
    buf.append(wq[i].value)

do:

for node in wq:
    buf.append(node.value)
for node in wq[::-1]:
    buf.append(node.value)

But what you really want is probably:

buf  = [x.value for x in wq]
# or
buf = [x.value for x in wq[::-1]]
buf = [x.value for x in reversed(wq)]

where the latter is necessary, because deque does not support the slice notation.

This already saves a lot of append calls and merges them into one assignment. But since these two things are very similar (only the direction changes), we can also write;

buffer = iter(wq) if is_left else reversed(wq)
print [node.value for node in buffer]

Use collections.deque

Since you are always popping left, you could use collections.deque, where this operation is faster than for a simple list.

from collections import deque
...
wq = deque([root])
wq_temp = deque([])

Miscellaneous

For this I think it is better to return as early as possible. Also, use tuple assignment:

# inner while
wq = wqTemp
wqTemp = []
if len(wq) == 0:
    return

# inner while
if not wqTemp:
    return
wq, wqTemp = wqTemp, []

Even better, you can move the check for the empty list to the condition of the while loop, because initially wq is either empty (then the while loop does nothing) or has root in it so is not empty.

This

wq = []
wqtemp = []
wq.append(root)

can be written as:

wq, wqtemp = [root], []

Final result

Wrapping everything together:

from collections import deque

class Node:
    def __init__(self, value):
        self.value = value
        self.left = None
        self.right = None

def zigzag_print(wq, is_left):
    if not wq:
        return
    buffer = iter(wq) if is_left else reversed(wq)
    print [node.value for node in buffer]

def zigzag(root):
    wq, wq_temp = deque([root]), deque([])
    is_left = True
    while wq:
        zigzag_print(wq, is_left)
        is_left = not is_left
        while wq:
            node = wq.popleft()
            if node.left:
                wq_temp.append(node.left)
            if node.right:
                wq_temp.append(node.right)
        wq, wq_temp = wq_temp, deque([])


Drawing a circle segment I wrote code to draw a circle segment, such that the segment angle will be facing the mouse.

My code:

#include <SFML/Graphics.hpp>
#include <math.h>

const float PI{ 3.14159265359f };

//convert degress to radians
float degToRad(float deg)
{
    return
        deg * PI / 180;
}

void drawCircleSegment(sf::RenderWindow & gameWindow)
{
    gameWindow.clear();
    sf::VertexArray arr(sf::LinesStrip);

    float segmentAngle{degToRad(45)};
    float radius{100};
    sf::Vector2f radiusPos{ gameWindow.getView().getCenter() };

    unsigned circlePoints{20};
    float angle{ 0 };

    arr.append(radiusPos);

    do
    {
        sf::Vector2f mRelP //get relative mouse position
        {
            sf::Mouse::getPosition(gameWindow).x - radiusPos.x,
            sf::Mouse::getPosition(gameWindow).y - radiusPos.y
        };

        float mouseAngle{ atan2(mRelP.y, mRelP.x) }; //get angle between relative mouse position and circle center

        sf::Vector2f p
        {
            cos(angle + (mouseAngle - segmentAngle / 2)) * radius, //position = cos(angle + lookAngle) * rad
            sin(angle + (mouseAngle - segmentAngle / 2)) * radius  //position = sin(angle + lookAngle) * rad
        };

        p += radiusPos;
        arr.append(p);

        angle += segmentAngle / circlePoints;

    } while (angle < segmentAngle);

    arr.append(radiusPos);

    gameWindow.draw(arr);
    gameWindow.display();
}

int main()
{
    sf::RenderWindow gameWindow(sf::VideoMode(800, 600), "circle");
    gameWindow.setFramerateLimit(60);

    while (gameWindow.isOpen())
    {
        sf::Event event;
        while (gameWindow.pollEvent(event))
            if (event.type == sf::Event::Closed)
                gameWindow.close();

        drawCircleSegment(gameWindow);
    }
}

What do you think about it?

Overall, this is pretty good. It's straightforward and to the point. Here are some other thoughts.

Don't Declare PI

You don't need to declare π. It's already declared in math.h as M_PI. Though in C++, it's preferred to #include <cmath> instead, which will put the standard math functions into the std namespace. In C++11 and later, it's suggested to use a calculation to define it such as:

const auto kPi = std::acos(-1.0);

Declare Functions constexpr When Appropriate

You don't say whether you're using C++11 or higher, or not. If you are, I would recommend declaring your degToRad() function constexpr so that it can be used in the definition of constants.

Don't Repeat Yourself

You have some code inside your loop that always calculates the same thing each iteration. The definition of mRelP, and mouseAngle should be moved outside the loop so you do less work in the loop.

Naming

mRelP is a confusing name. In many codebases it's a convention to prepend "m" to member variables. I'd just call it something more readable like mouseRelPos.


CopyOf2DArray (Values Only) Purpose:
Given a 2-Dimensional Array, return a copy of the Array. In VBA, if I just

Set newArray = oldArray

then newArray is simply a pointer to oldArray rather than an independent copy, hence this function.

Method:
Assign Array bounds. Create identically-sized Array. Iterate over Array and copy each element value.

Specific Areas of Interest:
The function feels very generic. The naming even more so. But then again it's supposed to be very generic. Is the naming alright or could it be better?

Function:

Private Function CopyOf2DArray(ByRef targetArray As Variant) As Variant
    '/ Will error if the array contains objects

    Dim LB1 As Long, UB1 As Long
    Dim LB2 As Long, UB2 As Long
    AssignArrayBounds targetArray, LB1, UB1, LB2, UB2

    Dim newArray As Variant
    ReDim newArray(LB1 To UB1, LB2 To UB2)

    Dim i As Long, j As Long
    For i = LB1 To UB1
        For j = LB2 To UB2
            newArray(i, j) = targetArray(i, j)
        Next j
    Next i

    CopyOf2DArray = newArray

End Function

I am going to challenge your premise

  ...then newArray is simply a pointer to oldArray rather than an
  independent copy.

The following code works fine ...

Sub myTest1()
Dim oldArray() As Double, newArray() As Double
Dim i As Long, j As Long

ReDim oldArray(0 To 1, 0 To 2)
For i = 0 To 1
    For j = 0 To 2
        oldArray(i, j) = (i * 10#) + (j * 1#)
    Next j
Next i

newArray = oldArray

For i = 0 To 1
    For j = 0 To 2
        Debug.Print i, j, newArray(i, j)
    Next j
Next i

End Sub

To verify that newArray is not just a pointer to oldArray, the following code ...

Public Declare PtrSafe Function VarPtrArray Lib "VBE7" Alias "VarPtr" (Var() As Any) As LongPtr

Sub myTest2()
Dim oldArray() As Double, newArray() As Double
Dim i As Long, j As Long

ReDim oldArray(0 To 1, 0 To 2)
For i = 0 To 1
    For j = 0 To 2
        oldArray(i, j) = (i * 10#) + (j * 1#)
    Next j
Next i

newArray = oldArray

Debug.Print "oldArray", VarPtrArray(oldArray), VarPtr(oldArray(0, 0))
Debug.Print "newArray", VarPtrArray(newArray), VarPtr(newArray(0, 0))

End Sub

... generates this result ...

oldArray       1633360       350127888
newArray       1633356       350126600

All of which means that, for arrays that do not contain objects, a simple assignment (e.g. newArray = oldArray) is fine to copy an array.


Dynamically creating elements with an object constructor Just experimenting with various Javascript techniques and was wondering if the code below is valid and if there are any disadvantages to using this technique to create html tags from JS object constructors? I know the usual technique is to add the html directly in the body and loop through an array to populate the elements.

So the two main questions;

Is the code valid and are there any main reasons not to use this technique?
Can the function call be integrated either into the constructor function (and called when a new object is created) or called from the new object parameter? (It somehow feels wrong to place on a separate line after every new object creator.

(function(){

function Photo(name, src){
this.name = name;
this.src = src;
    this.create = function(){
        var container = document.getElementById('thumbs_container');
        var img = document.createElement('img');
        img.src = this.src;
        img.alt = this.name;
        img.className = 'thumb';
        img.style.width = '200px';
        container.appendChild(img);
    }
}

var photo1 = new Photo('RED SQUARE', 'https://upload.wikimedia.org/wikipedia/commons/thumb/2/25/Red.svg/2000px-Red.svg.png');

photo1.create();

var photo2 = new Photo('BLUE SQUARE', 'https://upload.wikimedia.org/wikipedia/commons/thumb/f/fd/000080_Navy_Blue_Square.svg/600px-000080_Navy_Blue_Square.svg.png');

photo2.create();

var photo3 = new Photo('YELLOW SQUARE', 'https://upload.wikimedia.org/wikipedia/commons/f/f2/Yellow_Square.JPG');

photo3.create();

})();

I am going to answer the questions in reverse order and share some design thoughts at the end.

  Can the function call be integrated either into the constructor function (and called when a new object is created) or called from the new object parameter? (It somehow feels wrong to place on a separate line after every new object creator.

Yes; it can.

The constructor function is just that; a function. In fact, it doesn't even need to be: if you move the logic from Photo.create into the constructor (which seems like a valid decision), then there is no point in this being it's own class.

To move the Photo.create logic into the constructor, just move it up a scope:

function Photo(name, src) {
    this.name = name;
    this.src = src;
    var container = document.getElementById('thumbs_container');
    var img = document.createElement('img');
    img.src = this.src;
    img.alt = this.name;
    img.className = 'thumb';
    img.style.width = '200px';
    container.appendChild(img);
}

However, as I stated above, this doesn't really need to be a class anymore; it's just one function.

  Is the code valid and are there any main reasons not to use this technique?

No, the way you are doing this is just fine. However, as stated above, it does not need to be object-oriented for the implementation you have provided.

If your code is going to specific to what you have shown where you paint the picture as soon as you create, then this ought to be a function.

However, there is still a way that this can be improved. If you are familiar with regexes, then you know that there is such thing as compiling a regex before you actually use it. The reason for this is because compiling regexes can be a time and resource consuming process, so it's best to get it out of the way so it does not slow the the code in the middle of it.

You can do something similar here: if, in the constructor function, you..

Create the element
Define the container
Define the class
And define the width

then, you can just store this information for when you want to put the image on the DOM.

To use this, you would create a new Photo instance which would do the above in advance. Then, later in your code when you are ready to paint, you can specify (or re-specify) the source and name.

The constructor would look like this:

function Photo(name, src) {
    this.name = name || "";
    this.src = src || "";

    var container = document.getElementById('thumbs_container');
    var img = document.createElement('img');
    img.src = this.src;
    img.alt = this.name;
    img.className = 'thumb';
    img.style.width = '200px';

    // no painting yet

    this.create = function() {
        container.appendChild(img);
    }
}

Note how this code looks almost identical to the first code snippet in this answer. The only difference here is that you are preparing the system resources ahead of time; just like regexes. This will provide a speed boost in your code if used correctly.

var photo = new Photo(); // prepare resources for image

/* bunch of code all up until when it is needed to draw */

photo.create(); // put image/resources in DOM


Mapping a file system to a tree structure I need to write a very basic file/directory browser. The data structure I'm using is pretty simple - a Node with an optional parent and children. Along with the data structure I also need a simple function that will recurse a file system building up the tree of nodes. This is what I have come up with so far and I'd really appreciate any advice as to how I can improve it:

case class Node(name: String, parent: Option[Node]) {
    val children : ListBuffer[Node] = ListBuffer[Node]()

    def addChild(child: Node): Unit = children += child
    def removeChild(child: Node): Unit = children -= child
    def getChildren : Seq[Node] = children.toSeq
}

def getTree: Node = {
    def getChildren(file: File, parent: Node) : Node = {
        val self = Node(file.getName, Some(parent))
        parent.addChild(self)
        if (file.isDirectory) file.listFiles().foreach(child => getChildren(child, self))
        self
    }

    val rootNode = Node("test", Option.empty)
    getChildren(new File("/tmp/test"), rootNode)
}

// simple test
def printNode(node: Node, indent: String = "") : Unit = {
    println(indent + node.name)
    node.children.foreach(printNode(_, indent + "  "))
}

I'm pretty happy with the data structure (although I don't mind changing it) but I'm no so happy with the getTree function. In particular I don't like the fact that it's not tail call recursive and I could end up with quite a large stack ... but maybe I'm worrying about nothing?

Normally, I'd suggest trying to get rid of as much mutable state as possible. That's not really possible here, the circular reference to the parent pretty much guarantees that something is going to need to be mutable.

Vanilla Version

Without deviating much from your original design, there are a few cleanup suggestions that I can make.

First, as suggested by @Carcigenicate , getTree could use some naming fixes. In the version below I used loop, but it could have been any number of choices. I also inlined the creation of rootNode.

  def getTree(pathToRoot: String): Node = {
    def loop(file: File, parent: Node) : Node = {
      val self = Node(file.getName, Some(parent))
      parent add self
      if (file.isDirectory) file.listFiles().foreach(child => loop(child, self))
      self
    }

    loop(new File(pathToRoot), Node("test", Option.empty))
  }

Along those same lines, printNode became printTree because that's really what it does. I also moved the helper functions into Node's companion object. It's a good default organization strategy.

Node itself had mostly structural changes.

I changed the visibility of children, so that mutating it has to pass through the helpers. It also became a Set, as the OS doesn't generally make guarantees about listing order, so I wanted to avoid implying that the order was something that could be relied on not to change.

getChildren became just children and passes a defensive copy of the internal set.

That gives us this final version, with a small driver to make testing easier.

case class Node(name: String, parent: Option[Node]) {
  private[Node] val _children : mutable.Set[Node] = mutable.Set[Node]()

  def addChild(child: Node): Unit = _children += child
  def removeChild(child: Node): Unit = _children -= child
  def children: Set[Node] = Set(_children.toSeq:_*)
}

object Node extends ((String, Option[Node]) => Node) {

  def getTree(pathToRoot: String): Node = {
    def loop(file: File, parent: Node) : Node = {
      val self = Node(file.getName, Some(parent))
      parent addChild self
      if (file.isDirectory) file.listFiles().foreach(child => loop(child, self))
      self
    }

    loop(new File(pathToRoot), Node("test", Option.empty))
  }

  // simple test
  def printTree(node: Node, indent: String = "") : Unit = {
    println(indent + node.name)
    node.children.foreach(printTree(_, indent + "  "))
  }
}

object FileSystemTree {
  def main(args: Array[String]): Unit = args.foreach(arg => {
    Node.printTree(Node.getTree(arg))
  })
}

Lazy Version

It's possible to build a tree in a tail-recursive manner. It's much more difficult to do that with the references to the parent node.

So I cheated by making it a lazy data structure instead.

The first version was pretty basic.

case class Node(file: File, parent: Option[Node]) {
  lazy val children: Set[Node] =
    if (file.isDirectory) file.listFiles().map(f => Node(f, Some(this))).toSet
    else Set()
}

With each node only building the list of children on demand, the getTree function became a one-liner.

object Node extends ((File, Option[Node]) => Node) {
  def getTree(pathToRoot: String): Node = Node(new File(pathToRoot), None)
}

This sacrifices quite a bit of functionality to pull this off, mainly because of my preference for vals. To add the ability to add/remove child nodes, I went with a view instead. This way the data itself stays the same, just our view of it changes.

I also added a helper to provide depth-first traversal, to abstract away doing things with the nodes in the tree.

import java.io.File

case class Node(file: File, parent: Option[Node]) {
  private var filter = (x: Node) => true

  private lazy val _children: Set[Node] =
    if (file.isDirectory) file.listFiles().map(f => Node(f, Some(this))).toSet
    else Set()

  def children: Set[Node] = _children.filter(filter)

  def withFilter(p: Node => Boolean): Node = {
    filter = p
    this
  }

  def foreachDepthFirst(f: (Node, Int) => Unit, depth: Int = 0): Unit = {
    f(this, depth)
    children.foreach(_.foreachDepthFirst(f, depth + 1))
  }
}

object Node extends ((File, Option[Node]) => Node) {
  def getTree(pathToRoot: String): Node = Node(new File(pathToRoot), None)
  def printTree(root: Node): Unit = root.foreachDepthFirst({
    case (Node(f, _), depth) => println(("  " * depth) + f.getName)
  })
}

object FileSystemTree {
  def main(args: Array[String]): Unit = args.foreach(arg => {
    Node.printTree(Node.getTree(arg))
  })
}


Updating a density distribution I am running a Markov Chain Monte Carlo algorithm for updating a density distribution. There is a specific section of my code which tries to fill a very large matrix thetha.mh (of dimension 5000x2x60). the following code snippet is trying to fill the first dimension of my matrix, but the specific operation in second line of the loop is taking too much time.

Does anybody know of a better way to improve the efficiency of this operation?

fhatt=kde(x=theta.mh[m1:m,s,t-1], h=hpi(theta.mh[m1:m,s,t-1]))
  p2=function(thetax)
  dkde(thetax, fhatt)

fhatd=kde(x=deltat.mh[m1:m,s,t-1], h=hpi(deltat.mh[m1:m,s,t-1]))
  p3=function(deltatx)
  dkde(deltatx, fhatd)
# P1 and P2 are two distribution updated from the previous values of tetha.mh through kernel density estimation
h1=function(thetax)
      log(p1(y[s,t],thetax)*p2(thetax))
# h1 is the the logarithm of the multiplication of p1 and p2 (laplace form estimate)
for(i1 in 2:5000)
      {
        thetas=rnorm(1,theta.mh[i1-1,s,t],stheta[i1,s,t])
        r1[i1,s,t]=exp(h1(thetas)-h1(theta.mh[i1-1,s,t]))
        if (r1[i1,s,t]>runif(1))
          theta.mh[i1,s,t]=thetas
        else {
          theta.mh[i1,s,t]=theta.mh[i1-1,s,t]
          }
      }

The functions you use (kde, dkde, hpi, ...) do not seem to be standard R functions and you are not very explicit on your exact data structur, so it's hard to help. Could you provide more info on that (packages used, ...)?

What I think to realize though - but correct me if I'm wrong - is the following. When substituting h1 in the second line of your loop with its function body, you could simplify the function and get rid of all exp and log, which should speed up your script at least a bit:

    exp( h1(thetas) - h1(theta.mh[i1-1,s,t]) )
--> exp( log(p1(...)*p2(...)) - log(p1(...)*p2(...)))
--> exp(log(p1(...)*p2(...))) / exp(log(p1(...)*p2(...)))
--> p1(...)*p2(...) / p1(...)*p2(...)

that means, what your second line actually computes is:

p1(y[s,t],thetas)*p2(thetas) / p1(y[s,t],theta.mh[i1-1,s,t])*p2(theta.mh[i1-1,s,t])

I hope that is of some use to you...


Binary search of an integer array I'm a beginner. This is my implementation of binary search algorithm in Python. Please review my code and give your valuable inputs for further improvements on overall coding style, rare cases etc.

def binary_search(arr,value):
    left = 0
    right = len(arr) - 1
    while(left <= right):
        mid = (left + right) // 2
        print(mid)
        if arr[mid] is value:
            return mid
        elif value < arr[mid]:
            right = mid - 1
        elif value > arr[mid]:
            left = mid + 1

    return -1

def print_binary_search(arr,value):
    pos = binary_search(arr,value)
    if pos is -1:
        print("Element not found")
    else:
        print("Element found in the positon " +str(pos))

def test1():
    arr = [2,5,8,9,11,15,17,19,22,26,29,33,56]
    value = 56
    print_binary_search(arr,value)

def test2():
    arr = [2,5,8,9,11,15,17,19,22,26,29,33,56]
    value = 59
    print_binary_search(arr,value)

test1()
test2()

if arr[mid] is value:

You don't want to compare two integers with is, which compares identity. Use == instead. is will break your code when using large or small numbers (\$<-5\$, \$>256\$).

For example, using is with this "test" function:

def test1():
    arr = list(range(257, 270))
    value = 260
    print_binary_search(arr, value)

Results with an infinite loop.

Using == works as expected and returns 3.

The reason is that Python uses integer caching for the range \$[-5, 256]\$.

And some PEP8 nitpicking:

while(left <= right):

The parenthesis around the condition are useless in this case, you should prefer a whitespace:

while left <= right:

You are missing some whitespaces after commas in function definition, function calls, list literals and around operators.

For example, your code should look like this (relevant lines only):

def print_binary_search(arr, value):

#######

pos = binary_search(arr, value)

#######

arr = [2, 5, 8, 9, 11, 15, 17, 19, 22, 26, 29, 33, 56]

#######

print("Element found in the position " + str(pos))


Functional SaveObject in Scala I came across Java like Scala code that I am trying to refactor to make it functional and immutable.  The first obvious flaw is that it's violating thread safety by using mutable public class level collections.

Those methods (insText, insInt, etc) look unnecessary and can be removed with higher order function (where function takes in dd => Int, dd => String type of thing). Class DDType is wrong with all mutable variables.  Should I change class level mutable sets to private immutable sets?

Do you agree with those changes and overall refactoring approach?

class SaveObject extends Update with QueryFormatter {

  val keys    = mutable.Set[(DomainDictionary, Int   )]()
  val inserts = mutable.Set[(DomainDictionary, String)]()

  implicit def toInt(value:String) = value.toInt
  implicit def toDouble(value:String) = augmentString( value ).toDouble
  implicit def toBool(value:String) = value.toBoolean
  implicit def toTs(value:String) = {
  val date:Date = dateFormat.parse(value)
    new Timestamp(date.getTime)
}
implicit def toDate(value:String):Date = {
  dateFormat.parse(value)
}

def add(dd:DomainDictionary, value:String) = {
// check if its a key
getType(dd) match {
  case x if x.isKey    => id(dd, value)
  case x if x.isText   => insText(dd, value)
  case x if x.isInt    => insInt(dd, value)
  case x if x.isDouble => insDouble(dd, value)
  case x if x.isBool   => insBool(dd, value)
  case x if x.isTs     => insTs(dd, value)
  case x if x.isDate   => insDate(dd, toDate(value))
  case _               =>
    val msg = "Could not convert type for dd %s and value %s".format(dd.id, value)
    throw new Exception(msg)
 }
}
class DDType(dd:DomainDictionary) {
var isKey    = false
     var isText   = false
     var isInt    = false
     var isDouble = false
     var isBool   = false
     var isTs     = false
     var isDate   = false
 }
 def getType(dd:DomainDictionary):DDType = {
   val theType = new DDType(dd)
   val ns = dd.namespace
   val isKey = namespaceMatcher(ns,
   (ns,table,key)=>false.toString,
   (ns,table,key)=>false.toString,
   (ns,table,key)=>if(key == QueryPathfinder.ID) true.toString else false.toString
)

 if(toBool(isKey)){
   theType.isKey = true
 }
 else {
   val ddType = dd.domainDictionaryType
   ddType match {
     case "text" => theType.isText = true
     case "numeric" => theType.isInt = true
     case "alphanumeric" => theType.isText = true
     case "date" => theType.isDate = true
     case "timestamp" => theType.isTs = true
     case "boolean" => theType.isBool = true
     case "currency" => theType.isInt = true
     case _ => {} //this is wrong
   }
 }

theType
}
def id(dd:DomainDictionary, key:Int){
  keys += Tuple2(dd, key)
}
def insText(dd:DomainDictionary, value:String) = {
  inserts += Tuple2(dd,value)
}
def insInt(dd:DomainDictionary, value:Int) = {
    inserts += Tuple2(dd,value.toString)
}
def insDouble(dd:DomainDictionary, value:Double) = {
  inserts += Tuple2(dd,value.toString)
}
def insTs(dd:DomainDictionary, value:Timestamp) = {
  val date = new Date(value.getTime)
  inserts += Tuple2(dd,dateFormat.format(date))
 }
 def insDate(dd:DomainDictionary, value:Date) = {
    inserts += Tuple2(dd,dateFormat.format(value))
 }
  def insBool(dd:DomainDictionary, value:Boolean) = {
    inserts += Tuple2(dd,value.toString)
  }

}

You are mixing a repository with a classifier. The classifier is not doing anything except separating out keys from non-keys. The repository doesn't do anything with what it holds. You convert strings to a particular type and then store them back as strings, without gaining any real value along the way.

DDType doesn't seem to do anything except act as a classifier. And it doesn't even do that, as it only translates the string values "text", "numeric", etc, into predicates which are then discarded at the end of the classification.

Without seeing the use case, I can only presume that add(dd) is called from time to time and the sets are updated, either into keys or into inserts.

keys and inserts can be variables that hold immutable Sets. That will improve thread safety a smidgen.
Remove the redundant classifer:

def add(dd:DomainDictionary, value:String) = {
  dd.domainDictionaryType match {
    case "numeric" => keys = keys + (dd, value.toInt)
    case _ => inserts = inserts + (dd, value)
  }
}

For full thread safety, use a Repository Pattern, or use Actors.


Moving buttons is very slow I have a very simple app which removes buttons and re-adds buttons to certain layouts when a button is clicked. The problem is this process is taking a very long time. How do I speed this process up?

red.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(final View v) {

                // Remove the clicked button from it's frame layout parent.
                    ViewManager p = (ViewManager) v.getParent();
                    p.removeView(v);
                    // Find another button randomly.
                    Random rng = new Random();
                    View v2;
                    do
                    {
                        int i = rng.nextInt(4);
                        v2 = buttons.get(i);
                    }
                    while (v2 == v); // Loop until you get a different button to swap with.
                    // Remove the other button from it's frame layout parent.
                    ViewManager p2 = (ViewManager) v2.getParent();
                    p2.removeView(v2);
                    // Now simply insert each button into the other buttons frame layout.
                    p.addView(v2, v2.getLayoutParams()); //Adding v2, the random button, into where v, the button clicked, was before(p)!
                    p2.addView(v, v.getLayoutParams()); //Adding v, the button clicked, into where v2, the random button, was before(p2)
                }
   }
       });

This code just gets another button from the other three (there are 4 buttons in total) randomly, and swaps the red button (the button clicked) with the random button.

The problem is this process is taking 3-4 seconds after being clicked How can I speed it up? I want it to appear as if the buttons have been swapped as soon as the red button is clicked.

Should I keep my approach but make it immediate after button click to make it faster?
Somehow change the properties of the buttons by swapping click listeners and images AND somehow being able to keep track of the NEW red button so that I can swap again.

Loop could be removed. Replace with collection or array with each button choice. Remove the element clicked from collection. Use Random to choose an index or key of another remaining button.

Instance variable could retain which button last clicked.

Cast of v2.getParent and the ViewManager could instead be held as references in instance variables and quickly retrieved from a collection.

If Swing, could look at using awt directly for increased performance.


Delay to avoid cocurrent modification I was attempting to remove items from an ArrayList while iterating through it, only to find out that Java doesn't like that.  So I quickly wrote up a Delay that I could pass operations to so I could run them later.

import java.util.HashSet;
import java.util.Set;

public class Delay {
    private Set<Runnable> tasks = new HashSet<Runnable>();

    public Delay(){}
    public Delay(Runnable... tasks){
        for(Runnable task : tasks) this.tasks.add(task);
    }

    public void add(Runnable task){
        this.tasks.add(task);
    }
    public void run(){
        for(Runnable task : tasks) task.run();
        tasks.clear();
    }
}

Code in use (in a card game):

Delay delay = new Delay();

...

// if a hand has no cards, remove it from play
for(Hand hand : hands) if(hand.size() == 0) delay.add(() -> hands.remove(hand));
delay.run();

Is there a flaw in my solution?  Or does Java already have a method of handling this, making my code redundant?  Any points of improvement?

Yes java already has that. What you're looking for is an Iterator:

Iterator<Hand> handIt = hands.iterator();
while (it.hasNext()) {
    Hand current = handIt.next();
    if (current.size() == 0) {
        handIt.remove();
    }
}

This can be simplified / changed to:

for (Iterator<Hand> handIt = hands.iterator(); handIt.hasNext(); ) {
    Hand current = handIt.next();
    // ...
}