CODEWITHSUNDEEP
cloopscoding-styledesign-patterns
MicheleA
MicheleA

Reputation: 183

What's the best way to structure this kind of code?

I have a function that need to analyze packet after packet and decide what to do. For each packet the code must:

  1. Read a packet, on timeout return an error code.
  2. Check for corruption, if positive log it and and go to 1.
  3. Check for an abort packet, if positive log it and return and aborted code.
  4. Check for illegality of the parameters of the packet, if positive log it, respond with an invalid parameters packet and go to 1.
  5. Run the action of the packet, if failed log it, respond with an failure packet and go to 1.
  6. If the packet is an end packet, return success.

My code look like this:

Packet p;
for (;;) {  
    int ret = receive(&p, time);
    if (ret == TIMEOUT) {
        log("timeout");
        return TIMEOUT;
    }
    if (ret != 0) {
        log("corrupted %d", ret);
        continue;
    }

    if (p.type == ABORT) {
        log("abort");
        return ABORT;
    }

    ret = check(&p);
    if (ret != 0) {
        log("invalid %d", ret);
        respond(&p, INVALID);
        continue;
    }

    ret = execute(&p);
    if (ret != 0) {
        log("failure %d", ret);
        respond(&p, FAILURE);
        continue;
    }

    if (is_last(&p)) {
        finalize(&p);
        return 0;
    }
}

Are there a better structured way for this code that is not unnecessary nested or long?

Upvotes: 2

Views: 163

Answers (6)

Lindydancer
Lindydancer

Reputation: 26104

A rule of thumb is avoid reusing the same variable over and over again. If it's used for something new, create a new one instead. For example, when I read your code I missed the fact that ret was redefined along the way. Another advantage is that if a value is defined and immediately used, you can often define it in a smaller scope.

For example:

ret = execute(&p);
if (ret != 0) {
    log("failure %d", ret);
    respond(&p, FAILURE);
    continue;
}

You can rewrite this into:

{
  int ret_exe = execute(&p);
  if (ret_exe != 0) {
      log("failure %d", ret_exe);
      respond(&p, FAILURE);
      continue;
  }
}

Or, if you are using C99 or C++:

if (int ret_exe = execute(&p)) {
  log("failure %d", ret_exe);
  respond(&p, FAILURE);
  continue;
}

Upvotes: 0

Jim Balter
Jim Balter

Reputation: 16406

A lot of people don't like assignments nested in if statements, but I don't think there's any rational basis for that. Using them allows compact code like the following (which I don't claim is "best"; that is highly subjective):

for( ;; ){
    Packet p;
    int ret;

    if( (ret = receive(&p, time)) == TIMEOUT ){
        log("timeout");
        return TIMEOUT;
    }else if( ret != 0 ){
        log("corrupted %d", ret);
    }else if( p.type == ABORT ){
        log("abort");
        return ABORT;
    }else if( (ret = check(&p)) != 0 ){
        log("invalid %d", ret);
        respond(&p, INVALID);
    }else if( (ret = execute(&p)) != 0 ){
        log("failure %d", ret);
        respond(&p, FAILURE);
    }else if( is_last(&p) ){
        finalize(&p);
        return 0;
    }
}

Upvotes: 0

JeremyP
JeremyP

Reputation: 86651

It's personal preference but I personally do not like infinite loops or the continue keyword. I'd do it something like:

Packet p = { /* some dummy init that doesn't flag islast() true */};
int ret = 0;
while (ret != TIMEOUT && p.type != ABORT && !islast(&p)) 
{  
    int ret = receive(&p, time);
    if (ret != TIMEOUT) 
    {
        if (ret != 0) 
        {
            log("corrupted %d", ret);
        }
        else if (p.type != ABORT)
        {
            ret = check(&p);
            if (ret != 0) 
            {
                log("invalid %d", ret);
                respond(&p, INVALID);
            }
            else
            {
                ret = execute(&p);
                if (ret != 0) 
                {
                    log("failure %d", ret);
                    respond(&p, FAILURE);
                }
            }
        }
    }
}

if (ret == TIMEOUT)
{
    log("timeout");
}
else if (p.type == ABORT)
{
    log("abort");
    ret = ABORT;
}
else
{
    finalise(&p);
}
return ret;

My version looks more complicated than yours but that is because it more accurately reflects the structure of the algorithm. In my opinion (and it is only an opinion), keywords like continue and break obfuscate the structure and should not be used.

Other than this, the other main advantage is that, in my version, you can clearly see the conditions that cause loop exit by just looking in one place i.e. the loop condition. Also, conditions that cause the loop to quite are handled outside the loop - conceptually the right place. There's also only one exit point for the function.

Upvotes: 0

Curd
Curd

Reputation: 12427

Instead of having multiple returns in the loop you could use break and do a final return:

Packet p;
int ret;
for (;;) {  
    ret = receive(&p, time);
    if (ret == TIMEOUT) {
        log("timeout");
        break;
    }
    if (ret != 0) {
        log("corrupted %d", ret);
        continue;
    }

    if (p.type == ABORT) {
        log("abort");
        break;
    }

    .
    .
    .

    if (is_last(&p)) {
        finalize(&p);
        ret = 0;
        break;
    }
}
return ret;

Upvotes: 1

Craig
Craig

Reputation: 991

I would try to avoid returning from inside the loop. Instead break and have a single return at the end of the function. Looks ok apart from that.

Upvotes: 0

Michael F
Michael F

Reputation: 40829

I think it looks good. It definitely has no unnecessary nesting and even though it seems "long" it's quite brief - unless you want to move the logging inside the receive(), check() and execute() functions.

Upvotes: 0

Related Questions