5
\$\begingroup\$

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:

  1. the radioReceive timed out, so we pass the false back up
  2. it returned, but what was heard was basically a false positive, so try some more (this is somewhat exceptional)
  3. 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?

\$\endgroup\$
  • 2
    \$\begingroup\$ 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
5
\$\begingroup\$

If you can do without the received variable...just make the radioReceive(...) the condition of a while loop. You get rid of received, and your loop condition now has a purpose. :)

while (radioReceive(radiogram, MatchTree, CycleClock, duration)) {
    if (radiogramIsValidForSync(radiogram)) return true;
}
return false;

The only real drawback is that it gets a bit awkward if you need to do anything at both exit points, other than return.

\$\endgroup\$

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.