Подтвердить что ты не робот

Оптимизировать тройной оператор

Я наткнулся на этот код, написанный кем-то другим. Является ли это использование условного оператора рекомендуемым или обычно используемым? Я чувствую, что он менее обслуживаем - или это только я? Есть ли альтернативный способ написания этого?

  exp_rsp_status =  req.security_violation ? (dis_prot_viol_rsp && is_mstr) ? 
                    uvc_pkg::MRSP_OKAY : uvc_pkg::MRSP_PROTVIOL : req.slv_req.size()  ?
                    ((is_mst_abort_rsp && dis_mst_abort_rsp) ||
                    ((req.slv_req[0].get_rsp_status()==uvc_pkg::MRSP_PROTVIOL) && dis_prot_viol_rsp) ||
                    (is_mst_abort_rsp && req.is_pci_config_req() && dis_pcicfg_mst_abort_rsp)) ?
                    uvc_pkg::MRSP_OKAY : req.slv_req[0].get_rsp_status() : uvc_pkg::MRSP_OKAY;
4b9b3361

ОТВЕТЫ

Ответ 1

Это просто ужасный код.

  • Он плохо отформатирован. Я не вижу иерархию выражения.
  • Даже если у него было хорошее форматирование, выражение было бы слишком сложным, чтобы быстро разобрать человеческий глаз.
  • Цель неясна. Какова цель этих условий?

Итак, что вы можете сделать?

  • Использовать условные операторы (if).
  • Извлеките подвыражения и сохраните их в переменных. Проверьте этот хороший пример из каталога рефакторинга.
  • Используйте вспомогательные функции. Если логика сложна, используйте ранние return s. Никто не любит глубокие углубления.
  • Самое главное, дать все значимое имя. Намерение должно быть понятно, почему что-то должно быть рассчитано.

И просто чтобы быть ясным: в тройном операторе нет ничего плохого. Если использовать, судя по всему, он часто производит код, который легче усваивается. Избегайте их гнездования. Я иногда использую второй уровень, если код кристально чист, и даже тогда я использую круглые скобки, поэтому моему бедному мозгу не нужно делать лишние циклы, дешифрируя приоритет оператора.

Обратите внимание на читателей вашего кода.

Ответ 2

Возможно, это в контуре сообщений драйвера устройства, а исходный кодер, возможно, 10 лет назад, не хотел перескакивать в коде. Надеюсь, он подтвердил, что его компилятор не реализовал тройной оператор со скачками!

Изучая код, мое первое замечание состоит в том, что последовательность тройных операторов - как и весь код - лучше читается при надлежащем форматировании.

Тем не менее, я не уверен, что правильно разобрал пример OP, который говорит против него. Даже традиционную вложенную конструкцию if-else трудно проверить. Это выражение нарушает фундаментальную парадигму программирования: Divide и Conquer.

req.security_violation
?   dis_prot_viol_rsp && is_mstr
    ?   uvc_pkg::MRSP_OKAY
    :   uvc_pkg::MRSP_PROTVIOL
:   req.slv_req.size()
    ?       is_mst_abort_rsp && dis_mst_abort_rsp
        ||      req.slv_req[0].get_rsp_status()==uvc_pkg::MRSP_PROTVIOL
            &&  dis_prot_viol_rsp
        ||  is_mst_abort_rsp && req.is_pci_config_req() && dis_pcicfg_mst_abort_rsp
        ?   uvc_pkg::MRSP_OKAY
        : req.slv_req[0].get_rsp_status()
    : uvc_pkg::MRSP_OKAY;

Я хотел проверить, как выглядит код при реорганизации. Это, конечно, не короче, но мне нравится, как названия говорящих функций делают цель более ясной (конечно, я догадался здесь). Это, в некоторой степени, псевдокод, потому что имена переменных, вероятно, не являются глобальными, поэтому функции должны иметь параметры, что делает код менее понятным. Но, возможно, этот параметр может быть единственным указателем на структуру состояния или запроса или такой (из которых были извлечены значения, такие как dis_prot_viol_rsp). Использовать тройник при объединении различных условий - это обсуждение. Я нахожу его часто элегантным.

bool ismStrProtoViol()
{
    return dis_prot_viol_rsp && is_mstr;
}

bool isIgnorableAbort()
{
    return is_mst_abort_rsp && dis_mst_abort_rsp;
}

bool isIgnorablePciAbort()
{
    return is_mst_abort_rsp && req.is_pci_config_req() && dis_pcicfg_mst_abort_rsp;
}

bool isIgnorableProtoViol()
{
    return req.slv_req[0].get_rsp_status()==uvc_pkg::MRSP_PROTVIOL &&  dis_prot_viol_rsp;
}


eStatus getRspStatus()
{
    eStatus ret;

    if( req.security_violation )
    {
        ret = ismStrProtoViol() ?  uvc_pkg::MRSP_OKAY : uvc_pkg::MRSP_PROTVIOL;
    }
    else if(  req.slv_req.size() )
    {
        ret =       isIgnorableAbort()
                ||  isIgnorableProtoViol()
                ||  isIgnorablePciAbort()
            ? uvc_pkg::MRSP_OKAY
            : req.slv_req[0].get_rsp_status();
    else
    {
        ret = uvc_pkg::MRSP_OKAY;
    }

    return ret;
}

Наконец, мы можем использовать тот факт, что uvc_pkg::MRSP_OKAY является отличным по умолчанию и только при определенных обстоятельствах перезаписывается. Это устраняет ветвь. Посмотрите, как после некоторой резки аргументы кода хорошо видны: если это не нарушение безопасности, посмотрите ближе и проверьте статус фактического запроса, минус пустые запросы и игнорируемые прерывания.

eStatus getRspStatus()
{
    eStatus ret = uvc_pkg::MRSP_OKAY;

    if( req.security_violation )
    {
        ret = ismStrProtoViol() ? uvc_pkg::MRSP_OKAY : uvc_pkg::MRSP_PROTVIOL;
    }
    else if(        req.slv_req.size()
                &&  !isIgnorableAbort()
                &&  !isIgnorablePorotoViol()
                &&  !isIgnorablePciAbort()
            )
    {
        ret = req.slv_req[0].get_rsp_status();
    }

    return ret;
}

Ответ 3

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

if ( req.security_violation )
{
    if ( dis_prot_viol_rsp && is_mstr )
    {
        exp_rsp_status = uvc_pkg::MRSP_OKAY;
    }
    else
    {
        exp_rsp_status = uvc_pkg::MRSP_PROTVIOL;
    }
}
else if ( req.slv_req.size() )
{
    if ( ( is_mst_abort_rsp && dis_mst_abort_rsp ||
         ( req.slv_req[0].get_rsp_status() == uvc_pkg::MRSP_PROTVIOL && dis_prot_viol_rsp ) ||
         ( is_mst_abort_rsp && req.is_pci_config_req() && dis_pcicfg_mst_abort_rsp ) )
    {
        exp_rsp_status = uvc_pkg::MRSP_OKAY;
    }
    else
    {
        exp_rsp_status = req.slv_req[0].get_rsp_status();
    }

}
else
{
    exp_rsp_status = uvc_pkg::MRSP_OKAY
}

Ответ 4

Это ужасный код.

В то время как часто желательно инициализировать переменную с одним выражением (например, чтобы мы могли сделать это const), это не повод для написания кода, подобного этому. Вы можете переместить сложную логику в функцию и вызвать ее для инициализации переменной.

void
example(const int a, const int b)
{
  const auto mything = make_my_thing(a, b);
}

В С++ 11 и более поздних версиях вы также можете использовать лямбда для инициализации переменной.

void
example(const int a, const int b)
{
  const auto mything = [a, b](){
      if (a == b)
        return MyThing {"equal"};
      else if (a < b)
        return MyThing {"less"};
      else if (a > b)
        return MyThing {"greater"};
      else
        throw MyException {"How is this even possible?"};
  }();
}

Ответ 5

Другие уже сказали, насколько ужасно этот фрагмент кода, с приятными объяснениями. Я просто предоставлю несколько причин, почему этот код плохой:

  • если вы считаете, что один "if-else" реализует точно одну функцию, тогда ясно, насколько сложным является этот код. В вашем случае я не могу даже подсчитать количество ifs.

  • Очевидно, что ваш код нарушает принцип единой ответственности, в котором говорится:

    ... класс или модуль должны иметь одну и только одну причину изменения.

  • модульное тестирование, которое было бы кошмаром, а это еще один красный флаг. И я уверен, что ваш коллега даже не попытался написать модульные тесты для этой части кода.

Ответ 6

Общие или рекомендуемые? Нет.

Я сделал что-то подобное, но у меня были свои причины:

  • Это был аргумент в функции сторонней C.
  • В то время я не был хорошо разбираюсь в современном С++.
  • Я прокомментировал и отформатировал f *** из-за этого, потому что я знал, что кто-то, кроме меня, собирался его прочитать... или мне нужно было знать, что он делал годами позже.

  • Это был DEBUG CODE, который никогда не собирался выпускать.

    textprintf_ex(gw->GetBackBuffer(), font, 0, 16, WHITE, -1, "BUTTON: %s",
                       //If...                        Then Display...
                  (ButtonClicked(Buttons[STOP])    ?  "STOP"
                : (ButtonClicked(Buttons[AUTO])    ?  "AUTO" 
                : (ButtonClicked(Buttons[TICK])    ?  "TICK"
                : (ButtonClicked(Buttons[BLOCK])   ?  "BLOCK"
                : (ButtonClicked(Buttons[BOAT])    ?  "BOAT"
                : (ButtonClicked(Buttons[BLINKER]) ?  "BLINKER"
                : (ButtonClicked(Buttons[GLIDER])  ?  "GLIDER"
                : (ButtonClicked(Buttons[SHIP])    ?  "SHIP"
                : (ButtonClicked(Buttons[GUN])     ?  "GUN"
                : (ButtonClicked(Buttons[PULSAR])  ?  "PULSAR"
                : (ButtonClicked(Buttons[RESET])   ?  "RESET"
                :  /*Nothing was clicked*/            "NONE"
                )))))))))))
             );

Единственная причина, по которой я не использовал цепочку if-else, заключалась бы в том, что код стал бы более сложным и сложным, потому что все, что мне нужно было сделать, это распечатать слово на экране.