Today’s moment in code mending madness is brought to you by the “do…while loop” . Normally a “do .. while loop” is just that — a loop. The do statement is executed while the while statement is false. This code starts off with the while statement being false meaning the code is only executed once. So what is even the point?
Working on legacy code is full of those WTF moments where you are staring at something and can’t at first glance (or fifth or twentieth) understand what is going on. Here is a cleaned out version of the issue:
$condition = '';
do{
if($condition == 'a'){
echo "this is the a thing<br>";
continue; // that is a little odd
}
if($condition == 'b'){
echo "this is the b thing <br>";
continue; // that is a little odd
}
echo "this is the not a or b thing <br>";
}
while(false) // what the heck ??Looking at this code we see a couple of interesting things. As mentioned above the while check is false the first time through, there is also the continue element which is snickety-tricket. I will go more into that in a moment but for now let’s just say the implementation here is a touch surprising.
The first question in looking at legacy code is “What does the code actually do?” If we look at this we see that the code is checking for certain conditions and then returning output if the conditions are true, counterintuitively the “continue” breaks us out of the entire do…while loop. If none of the tested conditions apply the code executes the remaining portion of the do…while. Finally, it hits the ‘false’ of the while and is done.
“Why would anyone do this?” It is possible that the person who originally wrote the code was coming from a C background and was following the do…while with breaks pattern. Or they could be following an older C# macro convention. Most likely the programmer saw this pattern in other code and adopted into their work because it worked, never really learning a better way to achieve their goals, or if they did not bothering to come back and fix this – because you don’t fix what isn’t broken.
“The code works so what is the problem?” This particular gem is what we call a hack, an atypical, unexpected, creative use of a language attribute to solve a problem. The issue in using a hack is that it makes it difficult for other programmers to understand your code. Additionally using language features in a ways they aren’t intended to be used could introduce security issues and your code is more likely to be broken by language updates.
For me the most confusing part of this particular code is the way that the do…while interacted with the continue statement. One expects ‘continue’ to jump out of the current loop and then to continue. To break out of the loop and not execute the remaining parts of the loop the normal syntax would be break as in the code snips below.
<h3> No break or continue </h3>
<?php
$conditions = array('a', 'b', 'c');
foreach ($conditions as $condition) {
if($condition == 'a'){
echo "this is the a thing<br>";
}
if($condition == 'b'){
echo "this is the b thing <br>";
}
echo "this is the not a or b thing <br>";
}
?>
/* this outputs
<first loop>
this is the a thing
this is the not a or b thing
<second loop>
this is the b thing
this is the not a or b thing
<third loop>
this is the not a or b thing
*/
<h3> continue </h3>
<?php
$conditions = array('a', 'b', 'c');
foreach ($conditions as $condition) {
if($condition == 'a'){
echo "this is the a thing<br>";
continue;
}
if($condition == 'b'){
echo "this is the b thing <br>";
continue;
}
echo "this is the not a or b thing <br>";
}
?>
/* this outputs
<first loop>
this is the a thing
// stops the current loop when a is true, loop continues
<second loop>
this is the b thing
// stops the current loop when b is true, loop continues
<third loop>
this is the not a or b thing
*/
<h3> break </h3>
<?php
$conditions = array('a', 'b', 'c');
foreach ($conditions as $condition) {
if($condition == 'a'){
echo "this is the a thing<br>";
break;
}
if($condition == 'b'){
echo "this is the b thing <br>";
break;
}
echo "this is the not a or b thing <br>";
}
?>
/* this outputs
this is the a thing // stops the current loop when a is true, loop does not continue
In this case the “continue” acts like you would expect break to work. This alone should be reason to refactor this code.
This code like this should be refactored into either a goto or a switch case.