A while back I wrote the following code (target is embedded C code on an 8 bit atmel device):
bool received;
tryAgain:
received = radioReceive(radiogram, MatchTree, CycleClock, duration);
if (received && radiogram->timeSlit == SolicitMask)
goto tryAgain; // not interested in recursion and the (stack) problems that could come with it
return received;
I rarely rarely use a goto. Sometimes in embedded land, I talk myself into it. I'm aware that goto is bad, let's skip that issue. The real goal of the code flow was to capture 3 cases:
- the
radioReceive
timed out, so we pass thefalse
back up - it returned, but what was heard was basically a false positive, so try some more (this is somewhat exceptional)
- otherwise it was good and we return true
Then we refacatored the timeSlit
test to include some other conditions that we felt were false positives (new function named radiogramIsValidSync()
), but we got the logic backwards. At which point, we concluded this code was screwy and should be rewritten. But we diverged on how to best express those 3 conditions.
We came up with:
do {
bool received = radioReceive(radiogram, MatchTree, CycleClock, duration);
if (received == false) return false;
else if (radiogramIsValidForSync(radiogram)) return true;
} while (1);
This is terse, but I'm not a fan of meaningless while()
's (would rather use empty for
conditions). I also don't care for the fact that 2 of the 3 conditions are explicit, but the other is implicit.
A more explicit version is
bool received
tryAgain:
received = radioReceive(radiogram, MatchTree, CycleClock, duration);
if (received)
{
if (radiogramIsValidForSync(radiogram))
return true;
else
goto tryAgain;
}
else
{
return false;
}
But this still includes the goto
. A terse version is:
do
{
if (!radioReceive(radiogram, MatchTree, CycleClock, duration))
return false;
} while (!radiogramIsValidForSync(radiogram));
return true;
It gets rid of a named variable (which could be good or bad), my real issue with this is that it has multiple logic negation points, so you have to think "backwards" in both of the important points. Finally, a for
variant:
for ( ; ; )
{
received = radioReceive(radiogram, MatchTree, CycleClock, duration);
if (received == false)
return false;
else if (radiogramIsValidForSync(radiogram))
return true;
}
But this isn't really any better than the do/while(1)
variant. And at the end of that we found ourselves feeling unexcited about any of them. Is this just a case of the Law of Conservation of Ugly? Or is there a good/clear way to write this?
Is this just a case of the Law of Conservation of Ugly?
.... thank you kind sir. Much appreciated. I can use that frequently. \$\endgroup\$ – rolfl Jan 21 '14 at 21:00