1
\$\begingroup\$

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();
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to code review! What happens when the if statement condition is true? Please include more context in your code. This isn’t exactly hypothetical code but please see Why is hypothetical example code off-topic for CR? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Mar 29 '18 at 0:34
  • \$\begingroup\$ Thanks for the link Sam, I read it over. I also updated my code, so hopefully I can get some feedback on my changes. Thank you. \$\endgroup\$ – Tim M Mar 29 '18 at 2:03
1
\$\begingroup\$

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();
\$\endgroup\$
  • \$\begingroup\$ Thank you so much! Your post has lots of great information that I've spent the past couple days working on. I've learned a lot, so thank you! :) \$\endgroup\$ – Tim M Apr 1 '18 at 19:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.