Important "if" statement relying on mySQL

[StevenOliver]

I have a "best practices" question.

Visitors are limited to a maximum of 5 shipping labels per day, so mySQL counts how many were created in the past 24 hours and logs them.

Which "style" is better?

###  Style #1  #####################

$labelCounter = $db->prepare("SELECT count(*).. // mySQL counts labels

if($numberoflabels < 5) {

// RUN BIG CURL SCRIPT IF LESS THAN 5 LABELS
// LONG CURL SCRIPT GOES HERE

$log = $db->prepare("INSERT into label_counter... // log the good label

} else { // don't run big curl script

echo "Sorry visitor, no label could be created";

}

###  Style #2  #####################

$too-many-labels = FALSE;

$labelCounter = $db->prepare("SELECT count(*) // mySQL counts labels

if($numberoflabels > 4) {
$too-many-labels = TRUE;
}

if ($too-many-labels == FALSE) {

// RUN BIG CURL SCRIPT IF LESS THAN 5 LABELS
// LONG CURL SCRIPT GOES HERE

} else { // don't run big curl script

echo "Sorry visitor, no label could be created";

}

Style # 1 seems more direct, but Style 2 somehow "looks safer." Which is really better? What are your thoughts, please?

[taquitosensei]

I would personally go for #2 I don't know if it's safer.  I just like the idea of setting a flag then using that flag. 

[Barand]

Why set a superflous boolean variable when you have a perfectly good boolean expression already. All you are doing is adding an extra step

$too_many_labels will have the same value as  $numberoflabels > 4

I'd go for option 1 or option 3, which would be

// style 3
$labelCounter = $db->prepare("SELECT count(*) ... "); // mySQL counts labels

if($numberoflabels > 4) {
     echo "Sorry visitor, no label could be created";
}
else {
    // RUN BIG CURL SCRIPT IF LESS THAN 5 LABELS
    // LONG CURL SCRIPT GOES HERE

    $log = $db->prepare("INSERT into label_counter... // log the good label 
}  

 

[kicken]

Know that with either of those it's possible to make more than five with concurrent users.  If 4 labels have been made and two more requests come in and process very close together they might both pass the check and add a label resulting in 6 total labels at the end.  If you're concerned about that, then the solution is to use a lock to prevent the concurrent requests.

 

[StevenOliver]

4 hours ago, Barand said:

I'd go for option 1 or option 3...

Thank you! You just now reminded me of a question I've had since day one:

On an if/else statement, if one condition is more likely to happen than the other, should that condition go first? Or, does it absolutely not matter.

Using my example, it's extremely unlikely a visitor will try to print more than 5 labels (less than 1/100 chance). Would I want the first condition of the if/else statement to be the least likely scenario:

if ($numberoflabels > 4) {
echo 'sorry';
} else {
// proceed everything fine
}  

-or-

Would I want the first condition to be the most likely scenario:

if ($numberoflabels < 5) {
// proceed everything fine
} else {
echo 'sorry';
}  

Does one version over the other operate faster?

 

[kicken]

3 hours ago, StevenOliver said:

On an if/else statement, if one condition is more likely to happen than the other, should that condition go first? Or, does it absolutely not matter.

It doesn't matter.  Any potential performance difference would be unmeasurable.

Put them in which ever order makes the code easiest to read and understand.

 

[StevenOliver]

Thank you!!

[chhorn]

I wouldn't use these magic numbers in the code, you could just put it into a config file or use a sub-select to get it from the database.

[Psycho]

21 hours ago, kicken said:

It doesn't matter.  Any potential performance difference would be unmeasurable.

Put them in which ever order makes the code easiest to read and understand.

However, I would strongly suggest putting the error condition first as Barand did in his examples. The reason is that it makes it easier to "see" the condition that creates the error. Plus, if your error condition does a redirect, you don't even need to enclose the main code in an else clause. This makes the code easier to read/maintain.

if($foo == 'bar) {
    include('some_error_page.php');
    exit;
}
//Continue with the main logic w/o else clause

Also as Barand's stated, there is no reason to create a variable when you have a perfectly good condition. But, if you do need to create a flag based on a condition, do not do it this way

if($numberoflabels > 4) {
$too-many-labels = TRUE;
}

Instead do this:

$too-many-labels = ($numberoflabels > 4);

Edited April 3, 2019 by Psycho