Problem
I was looking over some code that I wrote years ago (that is still technically working, albeit on a very quiet production server) and I came across this segment of code.
if(isset($array2)){
foreach($array2 as $key => $value)
{
if((isset($value['coord'])||isset($value['bookedbefore']))&&!isset($value['bookedslot'])){
echo("<h2>Error: you ticked checkbox(es) on the booking selection setting without choosing a corresponding slot</h2>");
die();
}
$slotcode = $value['bookedslot'];
!empty($value['bookedbefore']) ? $bookedbefore = 1 : $bookedbefore=0; // not required
!empty($value['coord']) ? $coord = 1 : $coord=0; // not required
$valuearray = array($UserID, $slotcode, $bookedbefore, $coord);
//echo("<h1>debug...</h1>");var_dump($valuearray);
try{
$sql =
("
INSERT INTO
bookedslot(`FK_UserID`, `FK_SlotCode`, `booked_before`, `coordinator_approv`)
VALUES
(
:UserID,
:slotcode,
:bookedbefore,
:coord
);
ON DUPLICATE KEY UPDATE
FK_UserID = :UserID,
FK_SlotCode = :slotcode,
booked_before = :bookedbefore,
coordinator_approv = :coord
");
$sth = $dbh->prepare($sql);
$sth->bindValue(':UserID', $valuearray[0]);
$sth->bindValue(':slotcode', $valuearray[1]);
$sth->bindValue(':bookedbefore', $valuearray[2]);
$sth->bindValue(':coord', $valuearray[3]);
$sth->execute();
$sth->closeCursor();
}
catch(PDOException $e) {
$message = (implode($valuearray));
echo("<h3>Something seems to have gone wrong with your choices.</h3>");
}
}
unset($array2);
}
I am usually loath to touch things that aren’t broken, but that foreach loop looks completely unjustified in my mind, or at the very least, surely it is making a large number of unnecessary calls to the database? Was I having a brainfart years ago, or now?
Solution
First of all, I don’t see any “unnecessary” calls here. Do you need to insert all these lines into the database? Then all there calls are necessary.
However, you can greatly optimize the process, thanks to
Two cornerstone rules for running multiple DML queries
- prepare once execute multiple will gain you like 2-5% speed
- (however, in order to get that small speed improvement from prepared statements, you have to disable the emulation mode. But in this case you’ll be unable to reuse the placeholder name in the same query. Luckily, there is a
values()keyword to the rescue)
- (however, in order to get that small speed improvement from prepared statements, you have to disable the emulation mode. But in this case you’ll be unable to reuse the placeholder name in the same query. Luckily, there is a
- wrapping all DML queries in a transaction can boost the speed up to 70 times (though depends on the software settings and hardware configuration) and is overall a sensible move. You either want all rows to be inserted or none.
Error reporting
Another issue to address is error reporting which is completely flawed and far from being any helpful in the real life (I have to admit, it’s hard to master this skill as your code most of time works as intended) but, unlike what most people think, it’s really simple to make it right.
The first thing you have to realize is that there are two kinds of errors that require absolutely different treatment:
- userland errors are specific to the application logic and must be conveyed to the user
- PHP errors are system errors and a user has no business with them. At the same time, They are vital for the programmer and must be preserved for the future inspection. Luckily, due to their nature, system errors essentially require the uniform treatment which means you can write the handling code once, store it elsewhere, and completely remove any system error handling from your code.
In order to separate these two kinds of errors, validate your data prior the data manipulation. First check if you have all the required data and only then run your inserts. This way you will be able to separate the user interaction errors from the system errors. Be aware that die() is considered a bad practice. It’s better to collect all errors into a variable that have to be conveyed to the user in a more convenient fashion.
Regarding the system errors, just like it was said above, write a dedicated error handler once, that would log the error for the programmer and tell the user that something went wrong. I’ve got a complete working error handler example in my article dedicated to PHP error reporting, you can simply take this file and include it in your script.
Reduce the nesting level
As you may noticed, your code is hard to read because it is constantly shifting to the right side. Hence it’s a good idea to remove unneceessary nesting, as most of time it’s a sign of the problem code. For example, it makes sense to check outside variables with isset() but internal variables? It’s always a good idea to be positively sure whether some variable exists or not. Hence always have your variables initialized before use. Have $array2 initialized at some point and you can drop that outermost condition. Other levels can be removed as well, such as unnecessary try-catch.
Things that indeed could be considered blunders from your beginners days that look rather amusing:
- that
$valuearray - the way
empty()is used. In fact, to get 1 or 0 from a boolean value you can simply cast it as int
So what could we have after all these improvements?
First of all, separate the data validation from the query execution.
Validate all the data first,
$errors = [];
// perform all other validations
foreach($array2 as $value) {
if ((isset($value['coord']) || isset($value['bookedbefore'])) && isset($value['bookedslot'])) {
$errors[] = "Error: you ticked checkbox(es) on the booking selection setting without choosing a corresponding slot";
break;
}
}
and after that check for errors and if everything is OK then insert all your data, or show the form back to the user with values filled in and errors displayed
if (!$errors) {
$sql = "INSERT INTO bookedslot(FK_UserID, FK_SlotCode, booked_before, coordinator_approv)
VALUES (:UserID, :slotcode, :bookedbefore, :coord )
ON DUPLICATE KEY UPDATE
FK_UserID = values(FK_UserID),
FK_SlotCode = values(FK_SlotCode),
booked_before = values(booked_before),
coordinator_approv = values(coordinator_approv)";
$sth = $dbh->prepare($sql);
$dbh->beginTransaction();
foreach($array2 as $value) {
$slotcode = $value['bookedslot'];
$bookedbefore = (int)!empty($value['bookedbefore']); // not required
$coord = (int)!empty($value['coord']); // not required
$sth->bindValue(':UserID', $UserID);
$sth->bindValue(':slotcode', $slotcode);
$sth->bindValue(':bookedbefore', $bookedbefore);
$sth->bindValue(':coord', $coord);
$sth->execute();
}
$dbh->commit();
} else {
// inform the user
}
What’s going on here?
We are implementing all the things mentioned above. Just note that if your application is intended to die in case of a system error, there is no need to wrap your transaction in a try-catch block: it will be rolled back automatically by mysql as soon as the connection will be closed, and PHP always closes all its connections before die.
Just don’t forget to configure PDO to throw exception and to disable the emulation mode. Here I’ve got an example PDO connection code that does all the necessary stuff.
There is always the temptation to revisit code after a while to tweak it to make it work better, the main thing is to ensure you don’t break what is already there. The first thing to ensure is that you can reliably test the code which you are changing. Breaking something that appears to work (no matter how bad you think it is) will not go down well.
As for the code itself, there are a few things about it.
- The prepare stuff should be done outside the loop, you should
prepare the statement once and then execute it however many times
are appropriate. This means you don’t close the statement inside the
loop as well. - Rather than binding each value, you can pass an array of values
into theexecute(), which also means you can remove some of the
temporary values you use. - If any of the values fails validation, then you may end up with
partial data. Records 1 and 2 may be inserted, but record 3 fails –
BUT records 1 and 2 are already on the database. - The error processing seems to be a dump some HTML and stop. This is
probably a general method that you have used, but I would recommend
having a read around for some ideas on how to do this.
Best practice for error handling using PDO
is a more database oriented version.
Sometimes validating all of the data prior to doing any database processes is preferable, ideally you should validate some of the common issues in the front end. As this is fairly limited in this case it may be OK to mix the validation and database stuff and use database transactions (PHP + MySQL transactions examples).
If you wanted to go the other way, you would have a small foreach() loop first and check each row before even preparing the statement. This is better (IMHO) when the database code is more involved.
As the prepare is done outside of the loop, I’ve moved the scope of the try...catch() block.
I have not addressed the error reporting as that would be something you need to look into as it’s probably a site wide change.
if(isset($array2)){
try{
$sql =
("
INSERT INTO bookedslot(`FK_UserID`, `FK_SlotCode`, `booked_before`, `coordinator_approv`)
VALUES (:UserID, :slotcode, :bookedbefore, :coord )
ON DUPLICATE KEY UPDATE
FK_UserID = :UserID,
FK_SlotCode = :slotcode,
booked_before = :bookedbefore,
coordinator_approv = :coord
");
$sth = $dbh->prepare($sql);
$dbh->beginTransaction();
foreach($array2 as $value)
{
if((isset($value['coord']) || isset($value['bookedbefore']))
&& !isset($value['bookedslot'])){
echo("<h2>Error: you ticked checkbox(es) on the booking selection setting without choosing a corresponding slot</h2>");
// Remove any records added so far
$dbh->rollback();
die();
}
$sth->execute( [ ':UserID' => $UserID,
':slotcode' => $value['bookedslot'],
':bookedbefore' => (int)!empty($value['bookedbefore']),
':coord' => (int)!empty($value['coord'])
]);
}
$dbh->commit();
}
catch(PDOException $e) {
echo("<h3>Something seems to have gone wrong with your choices.</h3>");
// Remove any records added so far
$dbh->rollback();
}
unset($array2);
}
As a personal opinion, I have never liked the following type of construct…
!empty($value['bookedbefore']) ? $bookedbefore = 1 : $bookedbefore=0;
But the way it is being used makes it is easy to rework as you can just assign the result of the !empty() anyway…
$bookedbefore = !empty($value['bookedbefore']);