HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Better way to write this branch with an exceptional retry in an embedded C environment?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thiswithembeddedwaywritebetterenvironmentexceptionalbranchretry

Problem

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 the false 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))

Solution

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.

Code Snippets

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

Context

StackExchange Code Review Q#39757, answer score: 5

Revisions (0)

No revisions yet.