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 c开发者_Go百科ode that is not unnecessary nested or long?

---

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;

---

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.

---

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.

---

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.

---

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;
    }
}

---

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;
}