Каков наилучший способ структурировать такой код?

Question

У меня есть функция, которая должна анализировать пакет за пакетом и решать, что делать.Для каждого пакета код должен:

1. Считать пакет, по истечении тайм-аута вернуть код ошибки.
2. Проверить на наличие повреждений, если он зафиксирован, и перейти к 1.
3. Проверьте наличие пакета отмены, если оно зарегистрировано положительно, и верните и прерванный код.
4. Проверьте правильность параметров пакета, если сообщение положительно, ответьте неверным пакетом параметров и перейдите к 1.
5. Запустите действие пакета, в случае неудачи зарегистрируйте его, ответьте пакетом с ошибкой и перейдите к 1.
6. Если пакет является конечным пакетом, верните успех.

Мой код выглядит следующим образом:

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

Вместо нескольких return с в цикле вы можете использовать break и сделать окончательный 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

Многим людям не нравятся задания, вложенные в операторы 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 3

Практическое правило: избегайте многократного использования одной и той же переменной.Если он используется для чего-то нового, вместо этого создайте новый.Например, когда я читал ваш код, я упустил тот факт, что 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;
}

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;

Моя версия выглядит сложнее, чем ваша, но это потому, что она более точно отражает структуру алгоритма. По моему мнению (и это только мнение), такие ключевые слова, как продолжить и разбить, запутывают структуру и не должны использоваться.

Кроме этого, другое главное преимущество состоит в том, что в моей версии вы можете четко видеть условия, которые вызывают выход из цикла, просто просматривая одно место, то есть условие цикла. Кроме того, условия, которые вызывают цикл, обрабатываются вне цикла - концептуально правильное место. Также есть только одна точка выхода для функции.

Answer 5

Я бы постарался не возвращаться изнутри цикла. Вместо этого прервитесь и получите один возврат в конце функции. Выглядит нормально, кроме этого.

Answer 6

Я думаю, это выглядит хорошо.В нем определенно нет ненужных вложений, и хотя он кажется «длинным», он довольно короткий - если только вы не хотите переместить запись в функции receive(), check() и execute().