Filtering by value X from array Y with a fallback I wrote a function that solve a special problem. But one of my 'special hardcore software engineers' told me that this is a bad solution - but he will not give me a better solution. So I ask you: What could I do better? (This is more an academic exercise than a bottleneck in my software.)

I have an array $tags of {n} objects. Each object has an attribute called type. Further I have an array $types of {n} integers (tag types).

First value (index 0) of $types has highest priority, second value (index 1) of $types second highest priority and so on.

If this function finds a higher priority tag type during its iteration thru $tags it discards the lower priority tags from $returnArray.

$count just tell this function how many tags of same tag type are allowed. So if there are three tags of highest priority tag type it will return first occurrence.

Expected result 1:

    $tags = [
          $obj1, //type 50
          $obj2, //type 1
          $obj3 //type 25
        ]

    $types = [1, 50]

filterTagsByType($tags, $types) //returns [$obj2] because it has type 1

Expected result 2:

    $tags = [
          $obj1, //type 50
          $obj3 //type 25
        ]

    $types = [1, 50]

filterTagsByType($tags, $types) //returns [$obj1] because it has type 50

This is my function - I hope comments will clarify everything.

/**
 * This function filters and returns one or many tags of a defined tag type.
 * Define tag types in $types - first tag type (index 0) has highest priority.
 * This function returns only highest
 * tag type found and will ignore lower priority tag types.
 *
 * @param Tag[] $tags Array of tag models
 * @param int[] $types Array of tag types, first (index 0) has highest priority
 * @param int $count Max tags of one type to be returned
 * @return Tag[]
 */
public function filterTagsByType($tags, $types, $count = 1)
{
  $priority = 1000;
  $returnTags = [];

  foreach ($tags as $tag)
  {
      // get priority level of this tag, 0 is highest.
      $newPriority = array_search($tag->type, $types);

      // check if this tag has same priority like a tag before
      //if so, check if there is still room for another tag in $returnTags
      if ($priority == $newPriority && count($returnTags) < $count)
      {
          $returnTags[] = $tag;
      }
      // check if new priority is less (higher priority) than last tags (that
      // was added to return array) priority
      elseif ($newPriority < $priority)
      {
          // clear and add new highest priority tag to return array
          // it won't overwrite same priority tags because this case would
          // caught by first if.
          $returnTags = [$tag];
          //set new priority
          $priority = $newPriority;
      }

      //break this foreach as soon as highest priority tag typ was found and in
      // return array is no room anymore.
      if (0 == $priority && count($returnTags) == $count)
      {
          break;
      }
  }

  return $returnTags;
}

Edit:

  You code is over complex knowing there are some really good built in php functions to filter an array.

I think that is where the 'special hardcore dev' started disliking your code. Here an example that uses an anonymous function and array_filter. It also does the same as in the other answer. Loop over the $types instead of the tags. I think it is safe to say that there will be more tags then types.

function filterTagsByType($tags, $types, $count = 1)
{
    $tagFilter = function($type) {
        return function($tag) use($type) {
            return $tag->type == $type;
        };
    };

    foreach ($types as $type) {
        $filteredTags = array_filter($tags, $tagFilter($type));
        if ( count($filteredTags) > 0 ) {
            return $filteredTags;
        }
    }

    return array();
}

You should ofcourse still add the count check. this is easy with array_slice:

return array_slice($filteredTags, 0, $count);

Not sure how this is performance wise, but it shouldn't be an issue.


Positioning the left-right arrow buttons for a carousel I have a carousel (Owl Carousel) with vertically centered controls. Because of the structure, I have to absolutely position the previous and next arrow. Because the page is responsive, their position is dynamic. The size of the controls may also change.

I've written a function that runs on load and resize. It gets the height of the image and the height of the controls, subtracts the latter from the former, divides by two, and then uses that number as the controls' margin-top.

It works, but I'm questioning if I'm getting and using all the variables correctly. Does JavaScript read in order? Where it runs the first line, then the next, then the next... I'm strong in CSS but JS has always been a crutch.

Can I write this more efficiently?

function centerCarouselControls() {
  var carouselImage = $('.carousel-card > img');
  var carouselControls = $('.owl-nav > div');
  var carouselHeight = carouselImage.outerHeight();
  var controlHeight = carouselControls.outerHeight();
  var controlMargin = (carouselHeight - controlHeight) / 2;
  carouselControls.css('margin-top', controlMargin);
}

$('.carousel-card > img').load(centerCarouselControls);
$(window).on('resize', centerCarouselControls + 'px');
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>

Is it not possible to wrap both the 'arrow' and the image inside a relative div then do some absolute position and stuff?

jsfiddle so you can change the height to see it change without javascript: https://jsfiddle.net/3bm2hp5f/1/

Code:

html, body {
  height: 100%;
  width: 100%
}

    &lt;

  <image style="width: 100%; height: 100%;"></image>

 something else


Listing items of a particular category I have the following query which brings back a list of items which fall into a particular category:

SELECT i.id, ir.range_name, it.item_type, i.item_value
FROM items i
LEFT JOIN item_types it ON it.id = i.item_type
LEFT JOIN item_ranges ir ON ir.id = i.item_name
LEFT JOIN menu m ON m.id = it.menu_link
WHERE m.menu_title =  'vehicles'
AND i.item_live = 1
ORDER BY ir.range_name

I was wondering whether the m.menu_title column needs to be indexed.  Currently, there are only about 10 rows in the menu (m) table and this is unlikely to change that much.

However, with the joins, the above query will potentially bring back over 1000000 rows.  There are foreign keys on all the joins.

I am trying to avoid filling the database with dummy data to test the query.

Yes, No, and Maybe.
Yes
The table has 10 rows, the column is used as a constant argument in a query, and the query optimizer has ways to do index lookups (probes) in a very fast way. So, if the query parser decides to access the menu table by the literal, instead of the id, then the index will likely help. Also, for 10 rows, it will not take much space.
No
Because you use a left-join from items to menu, on the menu.id column, the reality is that the query plan will likely have to process all the records in item before joining to menu so that it can do the outer-join right. This means that all the query access to the menu table should be through the id column, and the menu_title column will be secondary, and the index on the menu_title (if any) will be ignored.
Maybe
Query optimizers can sometimes make dumb, and also very smart choices, and there is a chance that having the index will help, despite my best-guess that it wont. Additionally, there is a chance the optimizer will use the index, even though that will make it slower.
There is no way to know unless you try it.
My suggestion
Create the index, but don't expect it to be used for this query.
A second suggestion is to create the index menu(id, menu_title) because sometimes putting them together in an index helps avoid a data page lookup, but for just 10 rows, it's not going to make much difference.


Weighted random selection If given two lists a: ['the', 'a', 'and', 'for'] and b: [0.2, 0.3, 0.4, 0.1], the positions of the numbers in b represents the weights for the respective probabilities of obtaining the corresponding word. E.g. the : 20%, a : 30%, etc. Create a function that will generate words based on their corresponding probabilities.

My solution:

import random
def weighted_word_selection(words, weights):
    """
    words : an array of strings (words)
    weights : an array of floats (corresponding probabilties based on)
              index number.
    """
    start = 0
    for i in range(len(weights)):
        weights[i] = start+weights[i]
        start += weights[i]
    r = random.uniform(0, 1.0)
    for i in range(len(weights)):
        if r < weights[i]:
            return words[i]

Questions:

What are some alternative ways to solve this, bearing in mind the requirement to optimize performance as the array words and the array weights approaches a large n? Any detailed explanations of underlying processes such as Cython/C based Python solutions are helpful.
Can you discuss the \$O(n)\$ based complexity of the problem and some performance issues in my solution, and how those are addressed in your suggested solution at scale?

You have probably an error in your first for loop:

    weights[i] = start+weights[i]
    start += weights[i]

as the resulting value of start (BTW not very descriptive name) will be increased twice by weights[i], so the fix may be

    start += weights[i]
    weights[i] = start

But anyway, why you accumulate weights of individual words and moreover you assign the final result (which will be always 1, of course) in the start variable, if you never use it? IMHO your program perform something other than you want.

As your two input lists correspond one to other, why not use the zip() function to make pairs of corresponding values:

word_weights = zip(words, weights)

and use it in your for loops?

Why don't utilize the enumerate() function for obtaining indices without any effort?


Python word wrapping function I'm attempting to make some ui tools for my python ncurses application, and I needed a word wrap function. It works pretty well, but trying to read it is frustrating. I'm specifically looking for readability improvements, but if there are some other tips you can give, including edge cases or PEP violations, they would also be appreciated.

class ui_util:
    def chunk_word(word, length):
        return [ word[i:i+length] for i in range(0, len(word), length) ]

    def word_wrap(str, width):
        def add_first_word():
            nonlocal word
            nonlocal wrapped_line
            nonlocal lines_final

            if (len(word) <= width):
                wrapped_line = word
            else:
                chunks = ui_util.chunk_word(word, width)
                for i in range(len(chunks) - 1):
                    lines_final.append(chunks[i])
                wrapped_line = chunks[-1]

        lines = str.splitlines()
        lines_final = []
        for line in lines:
            if (len(line) <= width):
                lines_final.append(line)
            else:
                words = line.split()
                wrapped_line = ""
                for word in words:
                    if (len(wrapped_line) == 0):
                        add_first_word()
                    elif (len(wrapped_line) + len(word) + 1 <= width):
                        wrapped_line += ' ' + word
                    else:
                        lines_final.append(wrapped_line)
                        add_first_word()
                lines_final.append(wrapped_line)
        return lines_final

Called like:

lines = ui_util.word_wrap("some test words 13_chars_word\r\nnew line\r\n12_char_word 13_chars_word", 12)
for i in range(len(lines)):
    print(lines[i])

Output:

some test
words
13_chars_wor
d
new line
12_char_word
13_chars_wor
d

Code organization

The add_first_word() closure passes information via three nonlocal variables, which makes the code hard to understand.

I find it dissatisfying that the first word needs to be handled as a special case.  Instead, I would recommend using ' '.join(…) to help you add spaces conditionally.  That seems especially appropriate, considering that you also used .split().

It seems that you are using ui_util as a namespace rather than as a real class.  I would recommend just using the module for namespacing.  Furthermore, consider hiding or getting rid of the chunk_word() helper, in which case you would just have a word_wrap() function.

Naming

str is the name of a built-in type.  Avoid re-appropriating this conventional usage as a variable name.  I suggest renaming the parameter as text.

lines_final is an awkward variable name.  It implies that there is something wrong with lines.  I would say that if you split the text on explicit line breaks, then what you get is "paragraphs".

Style

In Python, parentheses around if conditions are extraneous, and therefore unconventional.

PEP 8, the official style guide, says:

  Pet Peeves

  Avoid extraneous whitespace… immediately inside parentheses, brackets or braces.

You wrote:

return [ word[i:i+length] for i in range(0, len(word), length) ]

Suggested solution

I think that it would make sense to convert this function into a generator.  Instead of accumulating all of the output in a lines_final list, you can just yield each line as it fills up.  (If the caller really needs a list, then use list(word_wrap(…)).)

Once you eliminate the special case as mentioned above, the code becomes much simpler.

def word_wrap(text, width):
    for paragraph in text.splitlines():
        line_len, line_buf = 0, []
        for word in paragraph.split():
            # If appending the word would cause overflow, flush the line.
            if line_len + len(line_buf) + len(word) > width:
                yield ' '.join(line_buf)
                line_len, line_buf = 0, []
            # If just this word alone would overflow, break it by force.
            while len(word) > width:
                yield word[:width]
                word = word[width:]
            line_buf.append(word)
            line_len += len(word)
        if line_buf:
            yield ' '.join(line_buf)

Example usage:

for line in word_wrap("…", 12):
    print(line)


Saving high scores in a pickle database Seeking to improve upon my high score 'module' code. It saves users' high score in a pickle database and if needed prints out the current scores in the database.

import pickle
import os
global high_scores

def setup_scores():
    global high_scores
    high_scores = {}

    if os.path.isfile('highscores.pkl'):
        with open("highscores.pkl", "rb") as h:
            high_scores = pickle.load(h)
    else:
        high_scores = {"Adam Smith": 65536, "John Doe": 10000}

def save_score(name, score):
    new_score = (name, score)

    if new_score[0] in high_scores:
        if new_score[1] > high_scores[new_score[0]]:
            high_scores[new_score[0]] = new_score[1]
    else:
        high_scores[new_score[0]] = new_score[1]

    with open("highscores.pkl","wb") as out:
        pickle.dump(high_scores, out)

def print_scores():
    for name, score in high_scores.items():
        print("{{name:>{col_width}}} | {{score:<{col_width}}}".format(col_width=(80-3)//2).format(name=name, score=score))

setup_scores()
save_score(raw_input('Name:'), raw_input('Score:')) # inputs only for testing, can use variables instead
print_scores()

Use of global should generally be avoided. You could create a class to hold the dictionary and the three functions would become methods.
Don't repeat yourself: use a variable for the file name so you don't have to change it in three places if it needs to change.
The save_score function assigns new_score = (name, score) for no reason at all. Using the two variables directly in the code that follows would improve readability.


Using closure, alias and namespace extension patterns: How to allow for independent instantiations of plugin? Simplified working demo code here.

Relevant JS code:

;(function(exports) {

    //--------------------------------------------------------------------------
    //
    // Private properties:
    //
    //--------------------------------------------------------------------------

    var _msg = '',
    _ol = document.getElementById('debug');

    //--------------------------------------------------------------------------
    //
    // Public methods:
    //
    //--------------------------------------------------------------------------

    exports.init = function(arg) {

        _msg = arg;

        // Add a listener to the `window.resize` event, pass `exports`/`self` as the scope:
        _addEvent(window, 'resize', _listenForChange, exports);

    };

    //--------------------------------------------------------------------------
    //
    // Private methods:
    //
    //--------------------------------------------------------------------------

    function _addEvent(elem, type, handle, context) {

        // Attache event:
        if (elem) {

            if (elem.addEventListener) {

                // If the browser supports event listeners, use them:
                elem.addEventListener(type, function() { handle.call(context); }, false);

            } else if (elem.attachEvent) {

                // IE & Opera:
                elem.attachEvent('on' + type, function() { handle.call(context); });

            } else {

                // Otherwise, replace the current thing bound to `on[whatever]`!
                elem['on' + type] = function() { handle.call(context); };

            }

        }

    }

    function _listenForChange() {

        var li = document.createElement('li');

        li.innerHTML = String(_msg);

        _ol.appendChild(li);

        if (typeof console !== 'undefined') console.log(_msg);

    }

    return exports; // Expose the API.

}(window.FOO = window.FOO || {}));

//--------------------------------------------------------------------

window.onload = function() {

    FOO.init('world');

    FOO.init('universe');

};

Problem with existing code:

When re-sizing the window, I only see "universe".

Goal:

When browser window re-sizes, I want to world "world" and "universe" output as a list item (and to the console).

I want to have the ability to instantiate FOO multiple times and have it work independently from any other running instances.

My questions:

Is there a pattern (or patterns), similar to the one(s) that I'm currently using, that will allow me to run multiple instances of FOO without having the last FOO called trump all previously called FOOs?

The code I posted is obviously not real world; my end goal is to allow/modify a plugin I wrote to be used without the end user having to worry if FOO is already being used by another script.

Please let me know if I can clarify anything and/or provide more specific code.

There is only one copy of window.FOO and window.FOO.init, despite its name does not create new objects. window.FOO.init just overwrites _msg that (function(exports) {... closed over each time it is called. _listenForChange() holds a reference to the closed over _msg. If you want it to keep a copy of the value at the time init is called then you can do like so:

_addEvent(window, 'resize', listenForChange2(), exports);

var listenForChange2 = function() {
    var msg = _msg; // keep own copy of the _msg
    return function() {
        var li = document.createElement('li');
        li.innerHTML = String("second version " + msg);
        _ol.appendChild(li);
        if (typeof console !== 'undefined') console.log(msg);
    }
}

Review of the code posted in the EDIT #1

First of all you are still confused about closures. It is important that you understand them because they are how you associate some state with a function. But explaining closures is not within the scope of Code Review. And you probably won't get any better answers than these, the accepted answer in this SO question and this article linked from there. You can fix your bug if you understand these.

Also the answer above fixes the bug if you KEEP the parens here:

_addEvent(window, 'resize', listenForChange2(), exports);

I the JSBIN you posted just add the parens and It works. So I will not repeat how can you fix the answer in the new code.

Onto the review. I will go over the code top down:

    exports.init = function(arg) {

By convention init method of an object is used for object initialization. It is not what it does here. Better name

       this._msg = arg;

You need not keep a copy of _msg since it will not be used elsewhere. You prepend the name with an underscore as if to indicate it is a private member (which practice I do not recommend BTW), but then expose it as public by assigning to this.. You can see, in the Firebug console for example, that Foo._msg is defined.

    exports.listenForChange = function() {

You need not expose this either.

    var listenForChange = function() {

would be just as good.

        var msg = this._msg;

If you do not create a new function, copying the value does not do anything.

    exports.addEvent = function(elem, type, handle, context) {

You need not expose this either, as it will not be called like Foo.addEvent.

Also adding another layer of indirection solving any problem, does not apply here.
Whenever there is something you do not understand, namely a bug,
adding another layer exacerbates the problem.
What does this refer to when the listenForChange function is run?

Even if you fix your bug this may introduce another one:

                elem['on' + type] = function() { handle.call(context); };

You are overwriting the onclick. Only the last handler assigned to the onResize will fire.

This is unnecessary:

    return exports; // Expose the API.

No one is using the returned value.
Moreover the comment is misleading as everything you assingn to exports. will be exposed,
whether you return it or not.


Dijkstra's algorithm in Clojure I'm learning Clojure and I'm interested in areas that could be more idiomatic. The main function is below. If you're interested in the whole lib, check out this.

The function operates on a grid, which contains a finite number of cells. A cell is a single point in a grid. neighbours-of will get information about moving one step away from a given cell. The info is in the form of a set of neighbouring cells, and a map giving the cost of moving to a given neighbour.

The loop vars are open-nodes, which is a sorted list of cell/cost pairs, visited-cells, which is a set of cells we don't want to look at again, and parents, which is a map giving the parent of a given cell.

parents is used in create-route to produce a path as a list of cells.

It doesn't currently handle the no-route case, nor is it very optimal. I'm not worried about the first, but if you have hints on the latter then that would be great (that order-by is probably a hotspot for large grids). Mostly I'm interested in what is and is not idiomatic.

(defn navigate-to [grid start dest]
  "provides route from start to dest, not including start"
  (validate-cell grid start)
  (validate-cell grid dest)
  (loop [[{current-cell :cell current-cost :cost} & open-nodes] (list (traversal 0 start))
         visited-cells #{}
         parents {}]
    (if (= dest current-cell)
      (create-route parents current-cell)
      (let [;; traversals is list of {:cell :cost}
            {all-neighbours :neighbours traversals :traversals} (neighbours-of
                                                                 grid
                                                                 current-cell)
            unvisited-neighbours (set/difference all-neighbours visited-cells)
            ;; add current route cost to traversals:
            neighbour-total-distances (into {}
                                            (for [[neighbour cost] traversals]
                                              [neighbour (+ current-cost cost)]))
            ;; keep bigger distances
            ;; ...first merge in seen neighbours, keeping higher values
            better-neighbours (->> open-nodes
                                   (filter
                                    (fn [{open-cell :cell current-cost :cost :as open-node}]
                                      (and (contains? unvisited-neighbours open-cell)
                                           (> current-cost
                                              (neighbour-total-distances open-cell)))))
                                   (map #(% :cell))
                                   set)
            updated-open (map (fn [{open-cell :cell current-cost :cost :as open-node}]
                               (if (contains? better-neighbours open-cell)
                                 (traversal (neighbour-total-distances open-cell) open-cell)
                                 open-node))
                              open-nodes)
            updated-parents (reduce #(assoc %1 %2 current-cell) {} better-neighbours)
            ;; ...then create new neighbours
            all-open-cells (set (map #(% :cell) open-nodes))
            new-neighbours (set/difference unvisited-neighbours all-open-cells)
            new-open-nodes (map #(traversal (neighbour-total-distances %) %) new-neighbours)
            new-parents (reduce #(assoc %1 %2 current-cell) {} new-neighbours)]
        (recur (sort-by :cost (concat updated-open new-open-nodes))
               (conj visited-cells current-cell)
               (merge parents updated-parents new-parents))))))

On the idiomatic front this code is excellent. All looks good.

You should use a priority queue to implement Dijkstra's algorithm. It is both far more efficient, and also more concise. I would go so far as to say that the beauty of Dijkstra's algorithm is underpinned by the priority queue data structure (which in turn is underpinned by the concept of a heap). There is much written about these topics, so I won't expand on that here unless it is not clear why. Java has a priority queue implementation that you can use via interop.

You only need a open list of candidates (in a priority queue so you can efficiently take the next minimum distance expansion, efficiently update shorter candidates as discovered, and remove visited nodes), and one map of visited nodes parents (you currently have a set and a parents map, the set is just the keys of parents).

The add/update/remove operations on a priority queue don't need to be implemented by you (they are methods of the datastructure), so less code :)


Defining a 'Watch' class This is the first program (that is not doing much basically), and I know usually you define classes in separate files.  I just wanted to see if I got the syntax right, and the concept of using objects from the first chapter of the book.

#import <Foundation/Foundation.h>

//interface section
@interface Watch: NSObject

-(void) setHours: (int) h;
-(void) setMinutes: (int) m;
-(int) getHours;
-(int) getMinutes;
-(void) printTime;

@end

//implemetation section

@implementation Watch

{
    int hours;
    int minuts;
}

- (void) setHours: (int) h
{
    hours = h;
}

- (void) setMinutes:(int) m
{
    minuts = m;
}

- (int) getHours
{
    return hours;
}

- (int) getMinutes
{
    return minuts;
}

-(void) printTime
{
    NSLog(@"Time is %i:%i", hours, minuts);
}

@end

//the program section

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

    @autoreleasepool
    {

        Watch *myWatch = [[Watch alloc]init];

        [myWatch getHours];

        [myWatch getMinutes];

        [myWatch setHours: 5];

        [myWatch setMinutes: 30];

        [myWatch printTime];

    }
    return 0;
}

You should definitely check out how properties can make your code shorter, by adding some standard setter/getter

@interface Watch: NSObject

@property int hours;
@property int minutes;
-(void) printTime;

@end

@implementation Watch

-(void) printTime
{
    NSLog(@"Time is %i:%i", _hours, _minutes);
}

@end

Also the naming convention of a getter is the property name

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

    @autoreleasepool
    {
        Watch *myWatch = [[Watch alloc]init];
        [myWatch setHours: 5];
        [myWatch setMinutes: 30];

        int hours = [myWatch hours]; //standard getter
        int minutes = [myWatch minutes];
        [myWatch printTime];
    }
    return 0;
}

naming conventions are important in Cocoa, as higher level technologies as Key-Value-Observing depends on them.


Replacing a For Each loop with something faster I'm a very inexperienced VBA user, I've just created my first "For Each" loop but it's very slow.  I'm happy that it works but eager to know if there is a alternative method that is quicker.  Any advice would be appreciated but please bear in mind my inexperience!

My routine takes two variables "UserSelectionMin" and "UserSelectionMax" and uses them in conjunction with the RandBetween function to place a value in every cell in the selected range (all the selected cells will be empty).

Sub RandomScores()

Dim addr As String
addr = Selection.Address

' opens a userform and collects the min value, assigned to UserSelectionMin (which is declared as an integer)
MinScore
' opens a userform and collects the max value, assigned to UserSelectionMax (which is declared as an integer)
MaxScore

For Each c In Worksheets("matches").Range(addr).Cells
If Abs(c.Value) = 0 Then c.Value = WorksheetFunction.RandBetween(UserSelectionMin, UserSelectionMax)
Next

Unload RandomScoreFormMin
Unload RandomScoreFormMax

End Sub

The MinScore and MaxScore procedures are below although these bits seem to be working OK.

Sub MinScore()
With RandomScoreFormMin.ListBoxMin
.AddItem "0"
.AddItem "1"
End With
RandomScoreFormMin.Show vbModal
End Sub

Sub MaxScore()
With RandomScoreFormMax.ListBoxMax
    .AddItem "1"
    .AddItem "2"
    .AddItem "3"
End With
RandomScoreFormMax.Show vbModal
MsgBox "You selected " & UserSelectionMin & " as your min score and " & UserSelectionMax & " as your max score"
End Sub

This smells:

' opens a userform and collects the min value, assigned to UserSelectionMin (which is declared as an integer)
MinScore
' opens a userform and collects the max value, assigned to UserSelectionMax (which is declared as an integer)
MaxScore

You are using global variables - variables should have the tightest possible scope, and be passed to procedures as parameters; both MinScore and MaxScore should be Function procedures that return a value.

Sub MinScore()
With RandomScoreFormMin.ListBoxMin
.AddItem "0"
.AddItem "1"
End With
RandomScoreFormMin.Show vbModal
End Sub

Indentation is important for readability. It should look something like this:

Sub MinScore()
    With RandomScoreFormMin.ListBoxMin
        .AddItem "0"
        .AddItem "1"
    End With
    RandomScoreFormMin.Show vbModal
End Sub

However a more pressing issue is the fact that you are working against the form's default instance - which is a bad habit to take. Instead, you should create an object:

Dim view As RandomScoreFormMin
Set view = New RandomScoreFormMin

view.ListBoxMin.AddItem "0"
view.ListBoxMin.AddItem "1"

view.Show vbModal

and as I mentioned, that should be a function, which returns the user-selected min value:

Private Function GetMin() As Integer
    Dim view As RandomScoreFormMin
    Set view = New RandomScoreFormMin

    view.ListBoxMin.AddItem "0"
    view.ListBoxMin.AddItem "1"

    view.Show vbModal

    GetMin = view.ListBoxMin.Value
End Function

Now you no longer need to worry about loading/unloading the form, since a new instance is used every time this function is called. And then the calling code can use a local variable instead of a global:

Dim minValue As Integer
minValue = GetMin

Regarding the performance of the ForEach loop:

For Each c In Worksheets("matches").Range(addr).Cells
    If Abs(c.Value) = 0 Then
        c.Value = WorksheetFunction.RandBetween(UserSelectionMin, UserSelectionMax)
    End If
Next

Taking the absolute value of the cell's content looks like a little hack - that would deserve a little comment to explain why it's done, otherwise a maintainer might deem it redundant, and remove it.

My guts are telling me that it's a way of getting an Integer value from the cell, even if the cell is empty. In that case, CInt(c.Value) would do a better job.

Another thing is that you are updating a worksheet in a tight loop; Excel is redrawing itself and recalculating the affected cells every time a value is written!

You can turn off screen updating and automatic calculation, while you're updating the sheet's contents:

Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual

Just don't forget to set ScreenUpdating back to True and calculation back to automatic after the loop completes (ideally in an error handler, so that if a runtime error occurs, screen updating is turned back on), so that Excel doesn't appear to remain "frozen"!

You have a potential bug here:

Dim addr As String
addr = Selection.Address

You're using the currently selected range's address...

For Each c In Worksheets("matches").Range(addr).Cells

...and then you're assuming that this range is on the "matches" worksheet - and it may not be all the time. If you mean to work with Selection, know that it is a Range, so you could do this instead:

For Each c In Selection.Cells

Otherwise, you need to validate the Selection before you use it - is it on the worksheet you're expecting?

Using WorksheetFunction is inherently slow, and should be avoided as much as possible; if your goal is to return a random number between X and Y, you could use VBA's random number generator instead of a worksheet function - here's a little formula to get a random integer between lowerbound and upperbound:

Int ((upperbound - lowerbound + 1) * Rnd + lowerbound)

Rnd returns a number that's smaller than 1, and greater than or equal to 0. It should be used in conjunction with the Randomize keyword:

Randomize
For Each c In Selection.Cells
    c.Value = Int((UserSelectionMax - UserSelectionMin + 1) * Rnd + UserSelectionMin
Next


When given 7 letters, find all possible words with length 3 to 7 I am writing a program in JavaScript to find all possible words given 7 letters. The words have to be 3 or more characters, and for right now I am limiting it to 7 characters long. The code below works, but takes forever to complete and I would like to speed it up. Would recursion help?

Thanks for any suggestions.

The variable wordList is an array of 60,000 words.

    function isWord(word){
        var found = wordList.indexOf(word);
        if(wordList[found]===word) return true;
            else return false;
    }
    for(var i=0; i<letters.length; i++) {
        for(var j=0; j<letters.length; j++) {
            for(var k=0;k<letters.length; k++) {
                if(isWord(letters[i]+letters[j]+letters[k]))
                    w.push(letters[i]+letters[j]+letters[k])
            }
        }
    }
    for(var i=0; i<letters.length; i++) {
        for(var j=0; j<letters.length; j++) {
            for(var k=0; k<letters.length; k++) {
                for(var l=0; l<letters.length; l++) {
                    if(isWord(letters[i]+letters[j]+letters[k]+letters[l]))
                    w.push(letters[i]+letters[j]+letters[k]+letters[l])
                }
            }
        }
    }
    for(var i=0; i<letters.length; i++) {
        for(var j=0; j<letters.length; j++) {
            for(var k=0; k<letters.length; k++) {
                for(var l=0; l<letters.length; l++) {
                    for(var m=0; m<letters.length; m++) {
                        if(isWord(letters[i]+letters[j]+letters[k]+letters[l]+letters[m]))
                        w.push(letters[i]+letters[j]+letters[k]+letters[l]+letters[m])
                    }
                }
            }
        }
    }
    for(var i=0; i<letters.length; i++) {
        for(var j=0; j<letters.length; j++) {
            for(var k=0; k<letters.length; k++) {
                for(var l=0; l<letters.length; l++) {
                    for(var m=0; m<letters.length; m++) {
                        for(var n=0; n<letters.length; n++) {
                            if(isWord(letters[i]+letters[j]+letters[k]+letters[l]+letters[m]+letters[n]))
                            w.push(letters[i]+letters[j]+letters[k]+letters[l]+letters[m]+letters[n])
                        }
                    }
                }
            }
        }
    }
    for(var i=0; i<letters.length; i++) {
        for(var j=0; j<letters.length; j++) {
            for(var k=0; k<letters.length; k++) {
                for(var l=0; l<letters.length; l++) {
                    for(var m=0; m<letters.length; m++) {
                        for(var n=0; n<letters.length; n++) {
                            for(var o=0; o<letters.length; o++) {
                                if(isWord(letters[i]+letters[j]+letters[k]+letters[l]+letters[m]+letters[n]+letters[o]))
                            w.push(letters[i]+letters[j]+letters[k]+letters[l]+letters[m]+letters[n]+letters[o])
                            }
                        }
                    }
                }
            }
        }
    }

You are forming each possible combination, and look for it in the dictionary. That indeed takes forever. You'd be in a better shape doing it other way around. Read the dictionary word by word, and for each word check if it is composed by your letters. In pseudocode,

    for letter in word
        if letter not in letters
            return false
    return true


Join argv list with spaces I'm a Python noob. This is the code I'm using:

for i in argv[2:]:                                 # argv[1] isn't required by s.py
    if ' ' in i:
        argv[argv.index(i)] = '"%s"' % i
args = ' '.join(argv[2:])
os.system('s.py '+args)

Is there a more elegant way to do this?

You shouldn't use os.system to run commands. You should use subprocess instead, for example:

from sys import argv
import subprocess

cmd = 's.py'
args = [cmd] + argv[2:]
subprocess.call(args)

This is a lot cleaner. In your version you added quotes around the command line arguments that contained spaces. (Which btw would break spectacularly if the command line arg also contained quote character...) Using subprocess, you don't need such dirty hacks.


Animating one of three boxes based on a form value Is there a better way to do this without using if or case statements?

var val = ui.value,
$box = $('#message');
switch(true) {
  case (val > 50):
    $box.hide();
    animations.animateBox($box.html('you have chosen XN'));
    break;
  case (val < 50):
    $box.hide().delay(300);
    animations.animateBox($box.html('you have chosen VN'));
    break;
  default:
    $box.hide();
    animations.animateBox($box.html('you have chosen PN'));
}

This completely replaces your switch statement by using inline if statements conditional operator:

$box.hide().delay(val < 50 ? 300 : 0);
var s = val > 50 ? 'XN' : (val < 50 ? 'VN' : 'PN');
animations.animateBox($box.html('you have chosen ' + s));

the first line could be replaced by these two arguably more readable lines that omit the call to delay() function when not applicable:

$box.hide();
val < 50 && $box.delay(300);

Since @GregGuida pointed out that this code is less readable I suppose I can make it more readable by better formatting it. And I'll use the suggestion of replacing the first line with two of them:

$box.hide();

// only delay animation when value < 50
val < 50 && $box.delay(300);

var s = val > 50 ? 'XN' :
       (val < 50 ? 'VN' :
                   'PN');
// set HTML
animations.animateBox($box.html('you have chosen ' + s));

Same code but visually less chaotic (even without comments it would look less chaotic) and much much more readable. At least much more readable than OP's original code. That I'm sure of. Readability is of course an argumentative disposition. But I'll leave that to others.


PHP First Friday Display Improvement? I have an event recurring on the first friday of the month (at 7pm), and I need to output the date of the next first friday coming up.

This is the first bit of PHP I've written, and I was wondering if there is a better way to implement this? Server is running PHP 5.3

Here's what I wrote:

$todays_date = strtotime('today');
$first_friday = strtotime('first friday of this month');

if ($todays_date > $first_friday) {
  $next_month = date('M j', strtotime('first friday of next month'));
  echo 'Friday, '.$next_month;
} elseif ($todays_date == $first_friday) {
  echo 'Today';
} else {
  $this_month = date('M j', strtotime('first friday of this month'));
  echo 'Friday, '.$this_month;
}

And here is a suggested improvement offered by someone else, which also introduced me to ternary operators:

$today = new DateTime();
$this_months_friday = new DateTime('first friday of this month');
$next_months_friday = new DateTime('first friday of next month');
echo ($today < $this_months_friday) ? 'Friday, '.$this_months_friday->format('M j').' at 7pm' : 'Friday, '.$next_months_friday->format('M j').' at 7pm';

I had looked all over for a short way to do this and was only running into very large and complicated answers. Both my way and the answer provided by someone else are much much shorter and more readable.

You can improve your ternary operator by using it like this :

$today = new DateTime();
$this_months_friday = new DateTime('first friday of this month');
$next_months_friday = new DateTime('first friday of next month');
echo 'Friday, '. (($today < $this_months_friday) ? $this_months_friday->format('M j') : $next_months_friday->format('M j')) . ' at 7pm';

which can them become :

echo 'Friday, '. (($today < $this_months_friday) ? $this_months_friday : $next_months_friday)->format('M j') . ' at 7pm';


Angular 5 Forms - Extending Form Control I am taking a first pass at Angular Forms with some colleagues and noticed an interesting design pattern implemented by a coworker where the FormControl is being extended to allow for additional properties including flags like readonly, showcancel and a few other custom properties.
The controls are then iterated over in the form template to then render each input. This is a big concern to me as we are extending the base Angular classes that could change with updates to the framework.
Here is an example of what I am talking about:
export class FormGroupModel extends FormGroup {
    readOnly: boolean;
    showCancel: boolean;
    controls: { [key: string]: FormControlModel | FormGroupModel };

    get(controlName: string): FormControlModel {
        return <FormControlModel> super.get(controlName);
    }
}

export class FormControlModel extends FormControl {
    readOnly: boolean;
    showCancel: boolean;
}

Is there anything else that we should keep in mind when using this strategy? I'm not totally certain that these classes were meant to be extended...

Update the signature of the module to include your new properties.
This will let the compiler know about your new properties and will prevent it from throwing errors in the HTML template or in the component.ts file.
declare module '@angular/forms' {
  interface AbstractControl {
    focused: boolean;
  }
}

I added AbstractControl because that is the base class that FormControl is built on and derives its properties from.

Add your new properties by extending the class, passing through (...args) for both the constructor(...args) and super(...args) calls.
export class FormControlCustom extends FormControl {

  public focused: boolean = false;

  constructor(...args) {
    super(...args);
  }

}

Typecast your FormControl. Now all your FormControlCustom elements will have the new properties defined in the class which extends FormControl by default.
sampleForm = new FormGroup({
    property: new FormControlCustom('', Validators.required)
});

I tested this and it is working perfectly in Angular 9.


Cleanest way to close over a counter in CoffeeScript I haven't spent too much time with CoffeeScript and am trying to have a simple counter:

casper.on('step.complete', ( ->
        i = 0
        return (step) ->
            i += 1
            @echo "Step#{ i }"
    )())

Which works fine but seems to have a bit too many parenthesis for CoffeeScript.

You could do this:

casper.on 'step.complete', do -> # invoked right away
  i = 0
  -> @echo "Step#{ ++i }" # implicitly returned function

Parentheses: None :-)

The do keyword is the equivalent of (-> ...)(), i.e. it means "invoke the  following function immediately" (it's also useful to avoid closure pitfalls in loops, by the way.)

The ++i will increment i before it's echo'ed, and CoffeeScript has implicit return, so the return keyword can be skipped.

Lastly, I left out the step function argument, as you're not using it for anything; you're always adding 1. You can always add it back in of course


Statistics updating SQL Query I have the following SQL statement that I think could be improved in areas (I believe there may be a way to use where over having, but I'm not sure how and I'm sure there's a way to reference the last updated column in the having clause but I'm unsure how to do it). Any suggestions would be greatly appreciated:

/* Show indexes that haven't had statistics updated in two days or more */
select
        t.name as [Table Name],
        min(convert(CHAR(11),stats_date(t.object_id,s.[stats_id]),106)) as [Last Updated]
from    sys.[stats] as s
        inner join sys.[tables] as t
            on [s].[object_id] = [t].[object_id]
group by t.name
having  min(convert(CHAR(11),stats_date(t.object_id,s.[stats_id]),106)) < dateadd(day, -1, getdate())
order by 2 desc

You can just use the condition in a where, that will filter the single records instead of filtering groups, then you can group the result to avoid duplicates in the result.

You shouldn't convert the date to a string when you are comparing it. When you convert it to a string, you should do that after getting the lowest value, otherwise min will compare the dates as strings and give you the wrong result.

select
  t.name as [Table Name],
  convert(CHAR(11),min(stats_date(t.object_id,s.[stats_id])),106) as [Last Updated]
from
  sys.[stats] as s
  inner join sys.[tables] as t on s.[object_id] = t.[object_id]
where
  stats_date(t.object_id,s.[stats_id]) < dateadd(day, -1, getdate())
group by
  t.name
order by
  2 desc


Keep items based on list membership I have list of tuples like so:

lst = [(106, 210, 108, 134, 134),
       (106, 210, 108, 134, 210),
       (106, 210, 108, 168, 268),
       (106, 210, 108, 168, 671),
       ...]

I need to only keep the tuples that contain:

keep = (106, 210, 108, 168)

Which I am currently doing using this statement:

kept = [item for item in lst if set(keep) < set(item)]

This works but it is too slow for me since the actual list I am working with contains 2.6 million items. It takes about 1.5 seconds now. Is there a good way to significantly speed this up?

Your code as it stand is on the border of being off-topic as it is a stub, however since the following actually works and seems to do the job, I'm going to answer it:

lst = [(106, 210, 108, 134, 134),
       (106, 210, 108, 134, 210),
       (106, 210, 108, 168, 268),
       (106, 210, 108, 168, 671)]

keep = (106, 210, 108, 168)

kept = [item for item in lst if set(keep) < set(item)]
print kept

Firstly some comments on the code:

Please provide a fully working example – In general, if there in the question is not a fully working example code, it'll be shut down immediately. Please provide something like the above code segment.
Name and comment – Providing good names and comment to illustrate what the code does is vital. Using anonymous name like lst, item, and no comments, makes the code a lot harder to read.
Do you keep lst in memory? – In the text you state that lst has over 2.6 million entries, do you keep all of this in memory? That could possibly effect your running times severly depending on your hardware resources. You might want to look into some algorithm leaving it in file or in a database, or whatever suits your needs.

Alternate solution

You're asking for a faster solution, so let us analyze what is happening in your code currently:

You loop through the entire in-memory lst once, and create a new set for each item
This set(item) is then compared to repeatedly created set(keep) (could be optimised away by the compiler), to check which is lesser
If lesser, the list comprehension keeps the item

The operations in here is \$O(N)\$ where N is the number in the list, and that you can't beat. The cost for each element is the creation of one or two sets, and the comparison of those sets, which is dependent on the size of the sets \$O(M)\$. In general since the \$N << M\$, the loop over the elements should be prominent.

This means, without changing the data structures there is not a whole lot to be gained, as you do need to loop through all elements, and you need to verify membership agains the keep list. If any optimization is to be performed it needs to address the actual comparison somehow.

Another view on your solution is the readability of your code, and how to understand what is happening. The list comprehension is understandable, but the set comparison is not obvious to me, at least, and I would have liked a comment. Or a rewrite, so let us attempt an rewrite for readability and see how it performs. This rewrite is using a Python specific concept to avoid a flag variable:

keepers = []
for candidate in lst:
  for keeper in keep:
    if not keeper in candidate:
      break

  else:
    keepers.append(candidate)

This code expands the list comprehension into the double for loop which is hidden in the list comprehension. The inner for loop breaks up the keep list into each element, and tests for memberships. The criteria for the candidate to be kept is that all elements should be members, so if any member is not present, we break out and start checking the next candidate.

The trick to this solution is that if the inner for loop doesn't complete naturally (aka no breaks), the else part is not executed. Try the following and play with it, to understand this mechanism:

for i in range(5):
  if i == 3:
    break
else:
  print "This didn't end naturally"           # Not executed

for i in range(5):
  if i == 7:
    break
else:
  print "The loop finished without breaking"  # Executes

Whilst I was writing this answer, the answer with using the all concept came in, so I included that in some basic timeit tests to check for running times, and the result surprised me a little:

Original method: 2.62324810028
Using all():     3.84744811058
Double for loop: 1.84868502617

This was tested using the original lst duplicated a few times to get a slightly larger set, but it does show that using the double for loop like I did, is currently the faster solution, and it runs at about 33% faster than the original code, and the solution using all() is actually quite a bit slower.


Loading in two CSV files and storing them in two separate arrays As the title says, I'm loading (or at least trying to) two CSV files and store them in two separate arrays. What I have so far worked, but it isn't the most elegant, no efficient solution.

The end goal is to read in two CSV files and run comparisons on them both - that's why the separating them is important.

Note - I use the csv-parser library, but am open to different solutions.

const express = require("express");
const parse = require("csv-parser");
const fs = require("fs");

const app = express();

const port = 3000;

var CSVOne = [];
var CSVTwo = [];

fs.createReadStream("public/activity.csv")
  .pipe(parse())
  .on("data", data => CSVOne.push(data))
  .on("end", () => {
    sender = CSVOne.map(d => {
      return {
        email: d.Sender
      };
    });
    fs.createReadStream("public/groups.csv")
      .pipe(parse())
      .on("data", dataTwo => CSVTwo.push(dataTwo))
      .on("end", () => {
        one = CSVTwo.map(d => {
          return {
            clinic: d.one
          };
        });
        console.log(sender);
        console.log(one);
      });
  });

app.listen(port, function() {
  console.log("Server has started");
});

I don't know about elegant, but you could load the files in parallel for better performance. To do something when all CSV files are ready you can use promises or just a simple counter.

The example also shows how you can use objects as return value in your arrow functions without the extra { return {...} }.

Example of loading in parallel using the counter method:

const express = require("express");
const parse = require("csv-parser");
const fs = require("fs");

const app = express();

const port = 3000;

var CSVOne = [];
var CSVTwo = [];
var toGo = 2;

fs.createReadStream("public/activity.csv")
  .pipe(parse())
  .on("data", data => CSVOne.push(data))
  .on("end", () => {
    sender = CSVOne.map(d => ({
      email: d.Sender
    }));
    maybeDone();
  });

fs.createReadStream("public/groups.csv")
  .pipe(parse())
  .on("data", dataTwo => CSVTwo.push(dataTwo))
  .on("end", () => {
    one = CSVTwo.map(d => ({
      clinic: d.one
    }));
    maybeDone();
  });

function maybeDone() {
  toGo -= 1;
  if (toGo === 0)
    done();
}

function done() {
  console.log("Both CSV files are ready");
  console.log(CSVOne, CSVTwo);
}

app.listen(port, function() {
  console.log("Server has started");
});


Select multiple count queries in single query I currently have 6 very similar queries that I'm trying to increase performance on by making them all subqueries in 1 query. I'm not sure what is better performance wise, keeping them 6 separate queries, 1 query with 6 subqueries (as shown below), or some other method that I'm unfamiliar with.

SELECT
  (SELECT COUNT(*) FROM table_one WHERE date=myDate AND name=myName) AS total,
  (SELECT COUNT(*) FROM table_one WHERE date=myDate AND name=myName AND CODE_A BETWEEN 100 AND 199) AS code_a_low_count,
  (SELECT COUNT(*) FROM table_one WHERE date=myDate AND name=myName AND CODE_A BETWEEN 200 AND 299) AS code_a_high_count,
  (SELECT COUNT(*) FROM table_one WHERE date=myDate AND name=myName AND CODE_B BETWEEN 100 AND 199) AS code_b_low_count,
  (SELECT COUNT(*) FROM table_one WHERE date=myDate AND name=myName AND CODE_B BETWEEN 200 AND 299) AS code_b_high_count,
  (SELECT COUNT(*) FROM table_one WHERE date=myDate AND name=myName AND CODE_C BETWEEN 100 AND 199) AS code_c_count
FROM table_one
WHERE ROWNUM=1

I can suggest you use common table expression (Oracle calls it "Subquery Factoring") at the very least you will be doing the filtering by date and name one time only

WITH filter_by_date_name AS (
  SELECT CODE_A, CODE_B, CODE_C FROM table_one WHERE date=myDate AND name=myName
)
SELECT
  (SELECT COUNT(*) FROM filter_by_date_name) AS total,
  (SELECT COUNT(*) FROM filter_by_date_name WHERE CODE_A BETWEEN 100 AND 199) AS code_a_low_count,
  (SELECT COUNT(*) FROM filter_by_date_name WHERE CODE_A BETWEEN 200 AND 299) AS code_a_high_count,
  (SELECT COUNT(*) FROM filter_by_date_name WHERE CODE_B BETWEEN 100 AND 199) AS code_b_low_count,
  (SELECT COUNT(*) FROM filter_by_date_name WHERE CODE_B BETWEEN 200 AND 299) AS code_b_high_count,
  (SELECT COUNT(*) FROM filter_by_date_name WHERE CODE_C BETWEEN 100 AND 199) AS code_c_count
FROM filter_by_date_name
WHERE ROWNUM=1


Sorting algorithms - Bubble sort For pure practice purposes, I started implementing different sorting algorithms in modern C++ in a standard library style way (i.e. using templates on iterators). This is my version of bubble sort.

I do not really have any specific criteria I would like to have the code reviewed under, but instead appreciate any hints I can get, be they on performance, coding style, expressiveness etc.

The whole implementation lives inside the file bubble_sort.h:

#pragma once

#include <utility>

template<typename Iterator, typename Comparator>
void bubble_sort(Iterator begin, Iterator end, Comparator cmp) {
    bool swapped;
    do {
        swapped = false;
        for (auto i = begin + 1; i != end; ++i) {
            auto& val0 = *i;
            auto& val1 = *(i - 1);
            if (cmp(val0, val1)) {
                std::swap(val1, val0);
                swapped = true;
            }
        }
    } while (swapped);
}

template<typename Iterator>
void bubble_sort(Iterator begin, Iterator end) {
    bubble_sort(begin, end, [] (const typename Iterator::value_type& v0,
                const typename Iterator::value_type& v1) { return v0 < v1; });
}

For reference, I also provide to you a file containing a main-method which sorts a small std::vector of random unsigneds in ascending as well as descending order:

#include <random>
#include <vector>
#include <iostream>

#include "bubble_sort.h"

int main() {
    std::vector<unsigned> vec;
    std::default_random_engine engine;
    std::uniform_int_distribution<unsigned> distribution(0, 10000);
    for (std::size_t i = 0; i < 100; ++i) {
        unsigned num = distribution(engine);
        std::cout << num  << ' ';
        vec.push_back(num);
    }
    std::cout << "\n\n";

    bubble_sort(vec.begin(), vec.end());

    for (unsigned e : vec) {
        std::cout << e << ' ';
    }
    std::cout << "\n\n";

    bubble_sort(vec.begin(), vec.end(), [] (unsigned v0, unsigned v1) {
            return v0 > v1;
        });

    for (unsigned e : vec) {
        std::cout << e << ' ';
    }
    std::cout << '\n';
}

I recommend putting your functions into a namespace.

for (auto i = begin + 1; i != end; ++i) {

By doing begin + 1, you require that Iterator is a RandomAccessIterator, whereas you only need an ForwardIterator. Use std::next instead:

for (auto i = std::next(begin); i != end; ++i) {

You could also use std::next(begin, 1) to be explicit.

Similarly, when you do *(i - 1), you require that Iterator is a RandomAccessIterator. Replacing it with std::prev still requires that Iterator is a BidirectionalIterator. Unless you think that is a reasonable requirement (for some versions of bubblesort, it would allow it to be more efficient), don't do this.

Rewrite the for loop as so (or any variant where you keep track of the two):

for (auto i = begin, inext = std::next(begin); inext != end; ++i, ++inext) {
    auto& val0 = *inext;
    auto& val1 = *i;
    // ...

While we are discussing iterator categories, it's useful to name Iterator after the iterator category you require, so FwdIter or ForwardIterator would be better names.

template<typename Iterator>
void bubble_sort(Iterator begin, Iterator end) {
    bubble_sort(begin, end, [] (const typename Iterator::value_type& v0,
                const typename Iterator::value_type& v1) { return v0 < v1; });
}

Rather than manually writing out the comparator, use std::less. I would use std::less<>, but you could use std::less<typename std::iterator_traits<Iterator>::value_type> if you wanted to.

template<typename Iterator>
void bubble_sort(Iterator begin, Iterator end) {
    bubble_sort(begin, end, std::less<>{});
}

Actually, instead of doing this, you could put a default argument on the other overload of bubble_sort:

template<typename Iterator, typename Comparator = std::less<>>
void bubble_sort(Iterator begin, Iterator end, Comparator cmp = {})

There are two advantages I can think of by doing it this way:

Less code (only one function definition)
You avoid ADL problems

One problem with

template<typename Iterator>
void bubble_sort(Iterator begin, Iterator end) {
    bubble_sort(begin, end, std::less<>{});
}

is that the bubble_sort(begin, end, ...) can activate ADL. If Iterator was, let's say, mylibrary::vector<int>::iterator, and if I wrote a bubble_sort in the namespace mylibrary, this bubble_sort call might call the one from the mylibrary namespace instead of the one we intended.

This is not good. As a rule of thumb, when writing generic code, avoid unqualified function calls like bubble_sort(begin, end, ...); instead qualify it with the namespace: ::bubble_sort(begin, end, ...).


Is this the correct usage of ConcurrentDictionary? I need to make a simple class that tracks how many clients are listening to a specific item, will be referred to as building hence forth.  I will have a timer running that will check which buildings are active and send updates to them.  While this is occurring, the client will be able connect and disconnect.  This means they will also be modifying the building list as time goes on.

To make sure that I don't have any concurrency issues, I decided to make a simple class that uses a ConcurrencyDictionary<TKey, TValue>.  I don't have a need of the class in the past and want to make sure it is being used correctly as the documentation seems to lack a bit for this class, mostly when it comes to updating values and the like.

/// <summary>
/// Tracks the buildings that are actively being listened to and the number of clients subscribed to the building
/// </summary>
public static class BuildingSubscriptionTracker
{
    /// <summary>
    /// A dictionary of building name and listening client count
    /// </summary>
    private static readonly ConcurrentDictionary<string, int> _buildingsSubscribedTo = new ConcurrentDictionary<string, int>();

    /// <summary>
    /// Get all the buildings that are actively being listened to by clients
    /// </summary>
    /// <returns>An <see cref="System.Collections.Generic.IEnumerable{T}"/> of string</returns>
    public static IEnumerable<string> GetBuildings()
    {
        return _buildingsSubscribedTo.Keys;
    }

    /// <summary>
    /// Get how many clients are listening to updates for this building
    /// </summary>
    /// <param name="buildingName">Name of building to get client listening count for</param>
    /// <returns>A count of how many clients are listening to updates for this building.  If the building is no longer being tracked, this will return 0</returns>
    public static int GetActiveClientCountFor(string buildingName)
    {
        int clientCount;

        if (_buildingsSubscribedTo.ContainsKey(buildingName))
            if (_buildingsSubscribedTo.TryGetValue(buildingName, out clientCount))
                return clientCount;

        return 0;
    }

    /// <summary>
    /// Increment the client count of the given building
    /// </summary>
    /// <param name="buildingName">Name of the building the client wants updates for</param>
    public static void IncrementClientCountFor(string buildingName)
    {
        //If the building doesn't exist, add it and +1 the count
        if (!_buildingsSubscribedTo.ContainsKey(buildingName))
            _buildingsSubscribedTo.TryAdd(buildingName, 0);

        int clientCount;

        _buildingsSubscribedTo.TryGetValue(buildingName, out clientCount);

        _buildingsSubscribedTo.TryUpdate(buildingName, clientCount++, clientCount);
    }

    /// <summary>
    /// Decrements the client count of the given building
    /// </summary>
    /// <param name="buildingName">Name of the building the client wants updates for</param>
    public static void DecrementClientCountFor(string buildingName)
    {
        //If the building doesn't exist, there isn't anything to do
        if (_buildingsSubscribedTo.ContainsKey(buildingName))
        {
            int clientCount;

            //Get how many clients are listening to building so we can properly update it
            if (_buildingsSubscribedTo.TryGetValue(buildingName, out clientCount))
            {
                var newClientCount = clientCount--;

                if (newClientCount < 1)
                {
                    int unusedValue;
                    _buildingsSubscribedTo.TryRemove(buildingName, out unusedValue);
                }
                else
                {
                    _buildingsSubscribedTo.TryUpdate(buildingName, newClientCount, clientCount);
                }
            }
        }
    }
}

There isn't a lot of point of checking ContainsKey.  A thread could sneak in between the ContainsKey and Try* methods you are calling.  The TryGetValue out parameter will be initialized to the default value if it can't find the key.

The GetActiveClientCountFor could become this.

public static int GetActiveClientCountFor(string buildingName)
{
    int clientCount;
    _buildingsSubscribedTo.TryGetValue(buildingName, out clientCount);
    return clientCount;
}

In the IncrementClientCountFor code the value is retrieved and then added to.  If someone came in and updated the value the TryUpdate would fail as the count changed.

What should be used is the AddOrUpdate method.  If the key doesn't exist it will be created and set to one.  If it does exist it will call the update method.  The update method could be called multiple times if the value changed before it could be set and will be set to the last good value.

public static void IncrementClientCountFor(string buildingName)
{
    _buildingsSubscribedTo.AddOrUpdate(buildingName, 1, (s, i) => i++);
}

Same problem with DecrementClientCountFo.  The value is retrieved and checked if less than one.  But again a thread could add to dictionary and the code would remove it from the dictionary with a positive count.  To get an Atomic TryRemove I would create an extension method called TryRemove taken from http://blogs.msdn.com/b/pfxteam/archive/2011/04/02/10149222.aspx

public static class ConcurrentDictionaryExtensions
{
    //http://blogs.msdn.com/b/pfxteam/archive/2011/04/02/10149222.aspx
    public static bool TryRemove<TKey, TValue>(
        this ConcurrentDictionary<TKey, TValue> dictionary, TKey key, TValue value)
    {
        if (dictionary == null) throw new ArgumentNullException("dictionary");
        return ((ICollection<KeyValuePair<TKey, TValue>>) dictionary).Remove(
            new KeyValuePair<TKey, TValue>(key, value));
    }
}

Now change DecrementClientCountFor to use AddOrUpdate then the Atomic TryRemove

public static void DecrementClientCountFor(string buildingName)
{
    var buildings = _buildingsSubscribedTo.AddOrUpdate(buildingName, s => 0, (s, i) => i--);
    if (buildings < 1)
    {
        _buildingsSubscribedTo.TryRemove(buildingName, buildings);
    }
}

If the value was changed from underneath us after getting the value the TryRemove will fail.  Which is ok since it could be positive or another thread could have removed the key already.


Getting days count from start day and number of day for a group of days in Java 8 I have a List of Objects containing following structure

public class DaySpan{
 private LocalDate date;
 private Integer numberOfDays;
..
..
//getters & setters
}

Lets say the list contains 3 instances of above classes with data (date is in dd/mm/yyyy)

04/02/2020 3
03/02/2020 5
01/02/2020 1

Then I need ouput for count of each day that is getting overlapped in number of days
For example the first record will translate to 04/02/2020, 05/02/2020, 06/02/2020
Second record will translate to 03/02/2020, 04/02/2020, 05/02/2020, 06/02/2020, 07/02/2019 and the final record will translate to 01/02/2020
So the output will be

01/02/2020 - 1
03/02/2020 - 1
04/02/2020 - 2
05/02/2020 - 2
06/02/2020 - 2
07/02/2020 - 1

The code I have written is

   public void getDayCount(List<DaySpan> daySpans){

    Map<Object, Long> days = new HashMap();
    days = daySpans.stream()
            .map(this::getDays)
            .collect(
                Collector.of(ArrayList::new,
                    List::addAll,(left, right) -> { left.addAll(right); return left; }
                    )
                )
            .stream()
            .collect(
                Collectors.groupingBy(Function.identity(), Collectors.counting()));

    days.entrySet().forEach(System.out::println);
  }

  public List<LocalDate> getDays(DaySpan daySpan) {
    return Stream.iterate(daySpan.getStartDate(), d -> d.plusDays(1)).limit(daySpan.getNumberOfDays())
        .collect(Collectors.toList());
  }

Is there a better way?

You came far to a good solution.

public void getDayCount(List<DaySpan> daySpans){
    Map<LocalDate, Long> freqs = daySpans.stream()
            .flatMap(daySpan ->
                IntStream.range(0, daySpan.numberOfDays)
                    .mapToObj(i -> daySpan.date.plusDays(i)))
            .collect(
                Collectors.groupingBy(Function.identity(), Collectors.counting()));

    freqs.entrySet().stream()
            .sorted(Map.Entry::getKey)
            .forEach(System.out::println);
}

Creating a Stream<LocalDate> for some DaySpan is simply realized by an IntStream.range.
A flatMap makes a larger Stream of LocalDates.
Turning them into a counting map you already did.
Sorting them on the date remains.
You printed the entries as Entry, so I kept it that way.


Binary Multiplication in C Question:Write an algorithm in C to do integer multiplication without using multiplication nor division operators.

Could I get a critique of the doMultiplication function? I know the rest of the code has some dumb stuff in it (like using scanf), but I'm looking for feedback on my multiplication implementation. Are there any cases it won't handle? Is there anything I could possibly have done to optimize it? And lastly, is there anything that would make it more readable?

#include <stdio.h>
#include <math.h>
/*
Write an algorithm/code in C to do integer multiplication  without using multiplication nor division operators.
 */
void main()
{
  int n;
  int m;
  int result;
  printf("Please enter numbers to multiplye\n");
  printf("First number\n");
  int multiple1 = getNumberFromUser();
  printf("\nSecond number\n");
  int multiple2 = getNumberFromUser();
  result = doMultiplication(multiple1,multiple2);
  printf("Result of multiplication = %d\n", result);
}

int getNumberFromUser()
{
  char arg1[10];
  int retry = 0;
  int multiple;
  while(retry == 0)
  {
    printf("Please enter a number:");
    scanf("%s", arg1);
    if(isNumericInput(arg1) != 0)
    {
      char * strEnd;
      multiple = strtol(arg1,&strEnd, 10);
      retry = 1;
    } else {
      printf("Bzzz wrong answer genius try a number \n");
    }
  }

  return multiple;

}

int isNumericInput(char arg[]){
  char * strEnd;
  int userInput = strtol(arg, &strEnd, 10);
  return userInput;
}

int doMultiplication(int n, int m)
{
  int result = 0;
  while(m != 0)
  {
    if((m & 1) != 0)
    {
      result = result + n;
    }
    n = n << 1;
    m = m >> 1;
  }
  return result;
}

Basically your algorithm looks correct, however:

You should use unsigned ints or have some treatment for the signs, otherwise I guess (didn't check it though) that your results for negative numbers will be wrong
Don't test for 0 explicitly, i.e. just write while(m) and if(m & 1)


Safety issues in PHP log-in system This is a user login (some session wrapper I managed to put together after a lot of web searching).

It's for a simple CMS I'm trying to build.  It only needs one user and there is no need for multiple user log in at the same time.  It works, the problem being I think it has safety issues.

It works like this:

The user accesses CMS index page, and inserts the user and password.
If one of them does not coincide with the data in the database it does not set the session and triggers a "die" or a error message.
If the info is ok then it sets the session and redirects the user from the index page to an admin.php page as you can see in the code below.

I commented it quite well.  If you see any safety issues please point them out as I can't seem to notice them.

The form is a simple user, password and submit form.

The class:

      <?php
      class Session{

    private static $_user;
    private static $_password;

    private static $_sessionStart = false;

    //here we check if the data inserted by the user in the form coincides with the rows in the db
    //if it does it sets the session for the user
    public static function CheckLog($received_user,$received_password){
        self::$_user            = $received_user;
        self::$_password        = $received_password;

        $db = Database::getInstance();
        $mysqli = $db->getConnection();

        $sql    = 'SELECT * FROM utilizatori ';
        $sql   .= 'WHERE user = "'.self::$_user.'" AND password = "'.self::$_password.'" ';
        $result = $mysqli->query($sql) or die (mysqli_error());
        if($row = $result->fetch_assoc()){
            if(($row['user'] === self::$_user)&&($row['password'] === self::$_password)){
                self::set('user',self::$_user);
                self::set('key',session_id());
            }
        }

      }
    //this method starts the session
    public static function start(){
       if(self::$_sessionStart == false){
         session_start();
         self::$_sessionStart = true;
       }
    }
    //this function sets the session values
    public static function set($key,$value){
        $_SESSION[$key] = $value;
    }
    public static function get($key){
        if(isset($_SESSION[$key])){
            return $_SESSION[$key];
        }
        else{
            return false;
        }
    }
    }

//from here is the handeling of the data in the "index" page:

        include("class_def/database.inc");
        include("class_def/user.inc");

        Session::start(); //here we start the session
        $up = strip_tags($_POST['user']); //post the user name
        $pp = strip_tags($_POST['password']);     //post the password
        //check if the variables are set and access the CheckLog method
       if((isset($up))&&(isset($pp))){
           Session::CheckLog($up,$pp);
       }
       //get the name of the current file
       $current_file = basename($_SERVER["SCRIPT_FILENAME"], '.php');

       //if the current page is index and the key is set redirect to admin.php
       if(isset($_SESSION['key'])){
           if($current_file == 'index')
      { echo '
        <script type="text/javascript">
            <!--
              window.location = "admin.php"
            //-->
            </script>';
      }
       }
       else{ //if the key is not set and the page is any other than index die and display message
           if($current_file != 'index'){
          die('You must log in first');
      }
          else{ // if the page is index echo out an error message
              $error_msj = 'Wrong user or pass';
          }
       }
    ?>

MVC
If you're trying to make a CMS, then there's a possibility that this CMS will become bigger than you first though. More pages, more modules, more code. You might want to consider doing an MVP approach to splitting code (rather than MVC).
A good reference framework for this is CodeIgniter. Presenter (CodeIgninter calls it the Controller) receives a request, then operates the data on the model (the model layer + database), then grabs a view (A template, your HTML) and renders it ("echo" the template).
SQL Injection
Then I notice this part:
$sql    = 'SELECT * FROM utilizatori ';
$sql   .= 'WHERE user = "'.self::$_user.'" AND password = "'.self::$_password.'" ';

You should read more about SQL injection. In a gist, it allows users to run arbitrary SQL, even be able to fake logins, see through existing usernames and passwords, as well as drop entire databases.
Sessions
Then there's sessions. For simple situations, that's fine. But for security, there's a lot to consider. If I can shoot malicious code through the forms, then I just might be able to fake my session. Read on this post for things to consider with sessions.
Validation
I see you use strip_tags() but it only does what it does, strip tags. It does not strip the other stuff, like backticks and quotes which can still be used to break in. A good example is the story of Little Bobby Tables. No tags, still broke through the database.
Redirect
window.location = "admin.php"

This operates on the browser. This means that index.php is actually served before redirecting, possibly rendering the page before this runs. I suggest you do a header redirect. It does not serve the page to the browser. It just tells the browser directly to load another page instead.


Calculating optimal video capture card configuration based on channels needed I have to calculate the configuration of hardware that uses the least number of capture cards based on the number of channels desired. The cards are available in 4, 8, and 16 channel varieties.

It's important to note that the least number of cards is the first priority, and the second priority is using the cheaper (lower channel) cards when possible. Therefore, a 22-channel request could be filled with 2 16-channel cards, however using a 16 and an 8-channel card is more desirable as it uses the same number of physical cards while utilizing a cheaper 8-channel card instead of two 16-channels.

var card_16 = "US-HDVRC2-16-480D";
var card_8 = "US-HDVRC2-8-240D";
var card_4 = "US-HDVRC2-4-120D";

function getCardObject(channels) {
    var cards = {}
    var remainder = channels%16;
    if (channels/16 >= 1) //if there are at least 16 channels
    {
        cards[card_16] = Math.floor(channels/16);
        if (remainder != 0)
        {
            if (remainder > 8)
                cards[card_16] = cards[card_16] + 1;
            else if (remainder > 4)
                cards[card_8] = 1;
            else
                cards[card_4] = 1;
        }
    }
    else if (channels/8 >= 1)  { //if there aren't 16 but at least 8
        if (remainder != 0)
        {
            if (remainder > 8)
                cards[card_16] = 1;
            else if (remainder > 4)
                cards[card_8] = 1;
            else
                cards[card_4] = 1;
        }
    }
    else if (channels/4 >= 1) { //if there are at least 4
        if (remainder != 0)
        {
            if (remainder > 4)
                cards[card_8] = 1;
            else
                cards[card_4] = 1;
        }
    }
    else //if there are less than 4
        cards[card_4] = 1;
    return cards;
};

Your own answer is pretty good.  I would tighten it up a bit.  In particular,

It's not immediately clear, based on the variable name, what answer represents.  Furthermore, since you are only interested in Math.floor(channels/16), I would compute it in a more succinct and efficient way: var sixteens = channels >> 4;.
I don't like reassigning cards["US-HDVRC2-16-480D"] or recomputing remainder > 8.
To cast a boolean as 1 or 0, you can use the unary + operator.
Consider creating the cards object in a single statement.  That means you may get some members whose value is 0 — I don't know if that is acceptable to you.

function getCardObject(channels) {
    var sixteens = channels >> 4;   // Math.floor(channels / 16)
    var remainder = channels & 15;  // channels % 16
    return {
        "US-HDVRC2-16-480D": sixteens + (8 < remainder),
        "US-HDVRC2-8-240D": +(4 < remainder && remainder <= 8),
        "US-HDVRC2-4-120D": +(0 < remainder && remainder <= 4),
    };
}

For comparison, here's a single-expression version using explicit formulas for everything.  I don't think that it is better than what you have — it's certainly not easy to understand.

function getCardObject(channels) {
    return {
        // Use a 4-channel card if  1 <= (channels % 16) <= 4
        'US-HDVRC2-4-120D':  +(((channels - 1) & 15) <  4),

        // Use an 8-channel card if  5 <= (channels % 16) <= 8
        'US-HDVRC2-8-240D':  +(((channels - 1) & 12) == 4),

        // 16-channel cards for the rest
        'US-HDVRC2-16-480D': (channels >> 4) + ((channels & 15) > 8),
    };
}


Seconds into readable time information I had an exercise to solve, written in German. I try to translate it:

  Write a program that takes an amount of seconds. The program than has to output the number of years, days, hours and seconds.

My Solution

public class Main {
  public static void main(String[] args) {
    final int HOUR_IN_SECONDS = 60 * 60;
    final int DAY_IN_SECONDS = HOUR_IN_SECONDS * 24;
    final int YEAR_IN_SECONDS = DAY_IN_SECONDS * 365;

    int seconds = 1000000000;

    // Jahre berechnen
    int years = seconds / YEAR_IN_SECONDS;
    seconds = seconds - years * YEAR_IN_SECONDS;
    System.out.println("Jahre: " + years);

    // Tage berechnen
    int days = seconds / DAY_IN_SECONDS;
    seconds = seconds - days * DAY_IN_SECONDS;
    System.out.println("Tage: " + days);

    // Stunden berechnen
    int hours = seconds / HOUR_IN_SECONDS;
    seconds = seconds - hours * HOUR_IN_SECONDS;
    System.out.println("Stunden: " + hours);

    // Minuten berechnen
    int minutes = seconds / 60;
    seconds = seconds - minutes * 60;
    System.out.println("Minuten: " + minutes);

    // Sekunden ausgeben
    System.out.println("Sekunden: " + seconds);
  }
}

This code works, but is it also beautiful enough and easy to read?

I have some suggestion for you.

1) The naming convention of the variables HOUR_IN_SECONDS, DAY_IN_SECONDS and YEAR_IN_SECONDS is not correct, since they are not constant; missing the static keyword. I suggest that you extract them in the class level.

public static final int HOUR_IN_SECONDS = 60 * 60;
public static final int DAY_IN_SECONDS = HOUR_IN_SECONDS * 24;
public static final int YEAR_IN_SECONDS = DAY_IN_SECONDS * 365;

public static void main(String[] args) {
   //[...]
}

2) I suggest that you create a method that take the number of seconds and print the result.

public static void main(String[] args) {
  int seconds = 1000000000;
  printFromSeconds(seconds);
}

private static void printFromSeconds(int seconds) {
    //[...]
}

For the rest, I think it's ok since the variable seconds is closely linked with the other components.


PHP registration/login form I'm fairly new to MySQLi and PHP, but I've been working at it for a bit now, and reading up on how to make secure login forms, avoid SQL injection etc. I'm a mere amateur though.

I've created an index.html page (not including the code here, just a basic form though), a register.html page (not including the code here, again it's just a basic form), adduser.php (which gets its call from "action" on the registration form),login.php (obviously gets its "action" from the index.html login form), logout.php, and a restricted.php with a check if session is active.

Everything is working. I'm just looking for some expert eyes to take a gander at my code and let me know if I'm missing anything whether it be major or minor, any security flaws, etc. Currently I don't have any form validation as I'm thinking of doing it client side with jQuery.

adduser.php

<?php

error_reporting(E_ALL);
session_start();
$mysqli = new mysqli ('localhost', '***', '***', 'test');

if($mysqli->connect_errno > 0) {
    die('Unable to connect to database [' . $mysqli->connect_error . ']');
}

if(isset($_POST['submit'])) {

    $errors = array();
    $data = array();
    $fname = $_POST['fname'];
    $lname = $_POST['lname'];
    $username = $_POST['username'];
    $email = $_POST['email'];
    $password = $_POST['password'];
    $confpass = $_POST['confpass'];
    $password_hash = password_hash($password, PASSWORD_DEFAULT);

    if(!($stmt = $mysqli->prepare("INSERT INTO users (fname, lname, username, email, password)
        VALUES (?,?,?,?,?)"))){
        echo "Prepare failed: (" . $mysqli->errno . ")" . $mysqli->error;
    }

    if(!$stmt->bind_param('sssss', $fname, $lname, $username, $email, $password_hash)){
     echo "Binding paramaters failed:(" . $stmt->errno . ")" . $stmt->error;
    }

    if(!$stmt->execute()){
     echo "Execute failed: (" . $stmt->errno .")" . $stmt->error;
    }

    if($stmt) {
        header('Location: index.html#testform')

    }
    else{
        echo "Registration failed";
    }

}

$mysqli->close();

login.php

<?php
error_reporting(E_ALL);

$mysqli = new mysqli ('localhost', '***', '***', 'test');

if($mysqli->connect_errno > 0) {
    die('Unable to connect to database [' . $mysqli->connect_error . ']');
}
ob_start();
session_start();

    $username = $_POST['username'];
    $password = $_POST['password'];
if(isset($_POST['submit'])) {

    if(!($stmt = $mysqli->prepare("SELECT username, password FROM users WHERE username = ?"))){
            echo "Prepare failed: (" . $mysqli->errno . ")" . $mysqli->error;
    }

    if(!$stmt->bind_param('s', $username)){
        echo "Bind failed: (" . $stmt->errno . ")" . $stmt->error;
    }

    if(!$stmt->execute()){
     echo "Execute failed: (" . $stmt->errno .")" . $stmt->error;
    }

    $userdata = $stmt->get_result();
    $row = $userdata->fetch_array(MYSQLI_ASSOC);

    $stmt->bind_result($username, $password);
    $stmt->store_result();

             if(password_verify($password, $row['password'])){

            $_SESSION['user'] = $_POST['username'];
            header('Location: restricted.php');
            exit();
        }

    else{
        echo "Login Failed: (" . $stmt->errno .")" . $stmt->error;
    }
$stmt->close();

}

$mysqli->close();

restricted.php (just the top session code, the rest of the page is useless for the sake of this post)

<?php
error_reporting(E_ALL);
ob_start();
session_start();
$db = new mysqli ('localhost', '***', '***', 'test');

if($db->connect_errno > 0) {
    die('Unable to connect to database [' . $db->connect_error . ']');
}
if (!isset($_SESSION['user'])) {
    header('Location: index.html');
}

?>

logout.php

<?php
session_start();
if(session_destroy()) // Destroying All Sessions
{
header("Location: index.html"); // Redirecting To Home Page
}
?>

Security

First of, your code looks pretty good, especially from a security point of view. You use prepared statements, and you use them correctly, and you use bcrypt instead of some weaker hashing algorithm such as md5 or sha.

There are a couple of things which might cause problems under certain conditions, and as defense in depth it might be a good idea to include them:

always die after a redirect, because the redirect does not necessarily stop execution of the PHP script (you can easily test this yourself: header('Location: index.html'); echo "secret"; using curl: curl localhost/redirect.php would give you secret, because curl doesn't follow redirects by default).
you have new mysqli ([credentials]) in at least two places. This is not only bad because duplication makes code harder to read and maintain, with credentials it is especially bad because it makes it harder to remember to hide them (when posting code somewhere else, submitting it in version control, etc). It also makes it harder to move those PHP files that do contain credentials outside the web root. Just put the database connection in one file (located outside the web root) and wrap it in a getConnection function (this also makes it possible to use the same database connection for all queries, which improves performance).
with current default PHP.ini settings, this is less important, but it is still always a good idea to regenerate the session id when the state of the session changes (eg when logging in) to prevent session fixation.
as @sjonniesjon said, don't echo actual error messages to the user. It can directly enable some attacks such SQL injections into some keywords (such as insert, update, order by, etc) which only display data in error messages, and it can help an attacker identify vulnerabilities.

Misc

your indentation is sometimes off, leading to harder to read code. Your brackets are also placed inconsistently.
you also have too many newlines in some places.


Getting data from an API in JSON format I'm getting data from an API in JSON format. It's getting the resting heart rate of the user and inserting it into my database. It gets today's date as well as a couple days in the past and updates the database accordingly. The code below does work (although it's inserting a blank row for some reason). My question is, is there a better/more efficient way of writing this code?

Here is the array:

[activities-heart] => Array (
[0] => Array (
    [dateTime] => 2018-03-22
    [value] => Array (
        [customHeartRateZones] => Array ( )
        [heartRateZones] => Array (
            [0] => Array (
                [caloriesOut] => 1135.7736
                [max] => 85
                [min] => 30
                [minutes] => 814
                [name] => Out of Range
                )
            [1] => Array (
                [caloriesOut] => 1260.7179
                [max] => 119
                [min] => 85
                [minutes] => 289
                [name] => Fat Burn
                )
            [2] => Array (
                [caloriesOut] => 690.64515
                [max] => 145
                [min] => 119
                [minutes] => 90
                [name] => Cardio
                )
            [3] => Array (
                [caloriesOut] => 0
                [max] => 220
                [min] => 145
                [minutes] => 0
                [name] => Peak
                )
            )
        [restingHeartRate] => 65
        )
    )
[1] => Array (
    [dateTime] => 2018-03-23
    [value] => Array (
        [customHeartRateZones] => Array ( )
        [heartRateZones] => Array (
            [0] => Array (
                [caloriesOut] => 1512.00346
                [max] => 85
                [min] => 30
                [minutes] => 1113
                [name] => Out of Range
                )
            [1] => Array (
                [caloriesOut] => 1315.59604
                [max] => 119
                [min] => 85
                [minutes] => 280
                [name] => Fat Burn
                )
            [2] => Array (
                [caloriesOut] => 98.14618
                [max] => 145
                [min] => 119
                [minutes] => 13
                [name] => Cardio
                )
            [3] => Array (
                [caloriesOut] => 0
                [max] => 220
                [min] => 145
                [minutes] => 0
                [name] => Peak
                )
            )
        [restingHeartRate] => 64
        )
    )
)

And here is the relevant code to process it:

$obj = new RecursiveIteratorIterator( new RecursiveArrayIterator( json_decode( $return, TRUE ) ), RecursiveIteratorIterator::SELF_FIRST );

foreach ( $obj as $key => $val ) {
    $$key = $val;

    $stmt = $connection->prepare( "INSERT INTO heartrate SET `encodedid` = ?, activitydate = ?, rhr = ? ON DUPLICATE KEY UPDATE rhr= ?" );
    $stmt->bind_param( 'ssii', $studentencodedid, $dateTime, $restingHeartRate, $restingHeartRate );
    $stmt->execute();

    if ( $stmt->affected_rows < 1 ) {
        header( "Location: " . $url . "/index.php?errormessage=Failed to updated the resting heart rate value." );
    }

    $stmt->close();
}

Code samples in this are approximate, I didn't test them.
A couple thoughts:

1)

You've currently called prepare() inside the body of your foreach loop. This is unnecessary, you only need to prepare your query once. You may reuse the PDOStatement that prepare() returns repeatedly in every iteration of your loop. This is actually very efficient, as the query will only have to be evaluated once. Once you're prepared, the preparation is done, no need to re-prepare for each loop iteration if your query structure is the same.

(For the record, prepare is indeed the correct method to use in batch operations like this. In the future, if you only need to execute a single query, you could alternative use PDO:query() (if you need results) or PDO:exec() (if you don't need results from the query -- it simply returns the number of rows affected by the query as an integer.))

The same goes for bindParam(). You don't need to re-bind the params each loop iteration-- once bound, they're bound. bindParam() binds by reference, the actual evaluation of the param happens every time execute() is called.

So let's do this minor refactor. We prepare the statement and bind the params before the loop begins, we execute() the statement on every iteration of the loop, and we'll move the close()-ing of the PDOStatement to be after the loop. Your sample now looks like this, and will be a bit more performant:

$obj = new RecursiveIteratorIterator( new RecursiveArrayIterator( json_decode( $return, TRUE ) ), RecursiveIteratorIterator::SELF_FIRST );

$stmt = $connection->prepare( "INSERT INTO heartrate SET `encodedid` = ?, activitydate = ?, rhr = ? ON DUPLICATE KEY UPDATE rhr= ?" );
$stmt->bind_param( 'ssii', $studentencodedid, $dateTime, $restingHeartRate, $restingHeartRate );

foreach ( $obj as $key => $val ) {
    $$key = $val;
    $stmt->execute();

    if ( $stmt->affected_rows < 1 ) {
        header( "Location: " . $url . "/index.php?errormessage=Failed to updated the resting heart rate value." );
    }
}

$stmt->close();

2)

$$key = $val;

I'd recommend NOT to use variable variables. They're generally considered a blight on the language, and people have pretty strong opinions on them.

They tend to make code more confusing to read, debug, and lint; and they don't do much that you can't do in a simpler manner.

2.5)

Actually, looking at your sample data array, your data doesn't look to be of arbitrary depth. It looks fixed-depth. Yes, there are n number of heartRateZones per day, but the the structure is repetitive. It looks keyed per-day, each day has a few unique key/value sets. The only thing in the value array you seem to care about is the per-day dates and resting heart rates. You aren't using them yet, but I can also imagine maybe you want the heartRateZones, you can just grab those directly without iterating over all the other keys in the 'value' array.

I'm not seeing a need for recursion at all.

We can easily eliminate the variable variables and get rid of the unnecessary recursion all at once. I'd probably handle this with loops or nested loops instead of recursion.

You don't appear to currently be using any of the heartRateZones. If you wanted to insert a record per zone, you could run execute() in that inner foreach. Otherwise just delete that loop and run the execute() in the days loop.

I didn't see $studentencodeid defined in the code sample you gave, or in the JSON data. I assume you already defined that elsewhere in your script.

$obj = json_decode($return, TRUE);

$stmt = $connection->prepare( "INSERT INTO heartrate SET `encodedid` = ?, activitydate = ?, rhr = ? ON DUPLICATE KEY UPDATE rhr= ?" );
$stmt->bind_param( 'ssii', $studentencodedid, $dateTime, $restingHeartRate, $restingHeartRate );

// Iterate through days
foreach ($obj as $key => $value) {

    // Grab data unique to a whole day that you care about
    $dateTime = $value['dateTime'];
    $restingHeartRate = $value['restingHeartRate'];

    // Iterate through heartRateZones (Or remove this loop if you don't need them)
    foreach($value['value'] as $zoneKey => $zoneValue) {
        // If you want to insert records based on individual heart rate records, do that here and remove the execute() below. You'd insert a record per-heartrate with the same dateTime and restingHeartRate values as all the other heartrates on that day.
        // $stmt->execute();
    }

    $stmt->execute();

    if ($stmt->affected_rows < 1 ) {
        header( "Location: " . $url . "/index.php?errormessage=Failed to updated the resting heart rate value." );
    }
}

$stmt->close();