构建此类代码的最佳方法是什么？

Question

我有一个功能需要逐个分析数据包并决定做什么。 对于每个数据包，代码必须：

1. 读取数据包，超时时返回错误代码。
2. 检查是否损坏，如果是，则记录它并转至 1。
3. 检查中止数据包，如果是，则记录它并返回中止代码。
4. 检查报文参数是否非法，如果是则记录，返回无效参数报文，并转至
5. 。
6. 1 packet为结束包，返回成功。

我的代码如下所示：

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

是否有一种更好的结构化方式来使该代码没有不必要的嵌套或冗长？

Answer 1

您可以使用 break 并执行最终的return，而不是在循环中使用多个 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;

Answer 2

我觉得看起来不错。它绝对没有不必要的嵌套，尽管它看起来“长”，但它非常简短 - 除非您想将日志移到 receive()、check() 和 <代码>execute() 函数。

Answer 3

我会尽量避免从循环内部返回。
相反，break 并在函数末尾有一个返回。
除此之外看起来还不错。

Answer 4

这是个人喜好，但我个人不喜欢无限循环或 continue 关键字。我会这样做：

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;

我的版本看起来比你的更复杂，但那是因为它更准确地反映了算法的结构。在我看来（这只是一个意见），像 continue 和 break 这样的关键字会混淆结构，不应该使用。

除此之外，另一个主要优点是，在我的版本中，您只需查看一个地方（即循环条件）就可以清楚地看到导致循环退出的条件。此外，导致循环停止的条件是在循环外部处理的——从概念上讲，这是正确的地方。该函数也只有一个出口点。

Answer 5

很多人不喜欢嵌套在 if 语句中的赋值，但我认为这没有任何合理的基础。使用它们可以实现如下所示的紧凑代码（我并不认为这是“最好的”；这是非常主观的）：

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

Answer 6

经验法则是避免一遍又一遍地重复使用同一变量。如果它用于新事物，请创建一个新的。例如，当我阅读您的代码时，我错过了 ret 在此过程中被重新定义的事实。另一个优点是，如果定义了一个值并立即使用，您通常可以在较小的范围内定义它。

例如：

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

您可以将其重写为：

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

或者，如果您使用的是 C99 或 C++：

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