2015-10-20 4 views
38

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

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; 
+0

Является ли 'exp_rsp_status' по умолчанию конструктивным и назначаемым? – NathanOliver

+0

Связанные: http://stackoverflow.com/questions/18237432/how-to-rewrite-complicated-lines-of-c-code-nested-ternary-operator или http://stackoverflow.com/questions/19020092/multiple -ternary-operator-in-c –

+0

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

ответ

65

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

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

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

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

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

Уход за читателями вашего кода.

+0

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

+1

Завершите в сторону: я не могу принять этот пример как «хороший», когда он проверяет IE, работающий на Mac ... – jpmc26

+1

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

10

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

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

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

В C++ 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?"}; 
    }(); 
} 
+0

Я просто хочу сказать, что ваш первый фрагмент также ориентирован на C++ 11 ... 'auto' и все :) – StoryTeller

+4

Трудно понять, как использовать этот ответ. Возможно, вы должны заменить свой код на что-то похожее на код в вопросе. – anatolyg

+3

Использование лямбда здесь - отличный способ немного запутать код. – PJTraill

14

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

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 
} 
+8

Я бы вырвал всю эту логическую логику в отдельные, хорошо названные переменные, чтобы намерение было кристально чистым. – TartanLlama

+2

Пожалуйста, упакуйте его обратно в тройники, это примерно так же читаемо (т. Е. Совсем нет), и вы справитесь с ним быстрее. :-D – DevSolar

+1

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

28

Возможно, это в цикле сообщений драйвер устройства и оригинальном кодер, возможно, 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 является kindof по умолчанию и только перезаписаны при определенных обстоятельствах. Это устраняет ветвь. Посмотрите, как после некоторой резки аргументы кода хорошо видны: если это не нарушение безопасности, посмотрите ближе и проверьте статус фактического запроса, минус пустые запросы и игнорируемые прерывания.

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

Я бы сделал еще один шаг и вернусь раньше, чтобы избавиться от переменной 'ret' вообще ...' if (req.security_violation) {return ismStrProtoViol()? uvc_pkg :: MRSP_OKAY: uvc_pkg :: MRSP_PROTVIOL; } ... '. –

+1

Здесь вам не нужна куча функций. Замените каждую из ваших функций калькуляцией, хранящейся в переменной. –

+0

@LorenPechtel Хорошая точка. Я буду править его. –

4

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

  1. , если вы считаете один «если-иначе», чтобы реализовать именно одна особенность, то ясно, насколько сложна, что код. В вашем случае я даже не могу подсчитать количество ifs.

  2. Очевидно, что ваш код ломает нарушения single responsibility principle, который говорит:

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

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

+0

__1__. Конечно, вы можете их сосчитать, существует 4 '' 's, т. Е. 4' if '(если вы не видите' || && 'как неявные' if's '). __2__. Неясно, учитывая, что ваша ссылка говорит, что принцип применяется к классам, и мы не можем видеть, насколько хорошо данное выражение отделено от кода, обращаясь к другим проблемам. __3__. _Debugging_, с отладчиками, которых я знаю, наверняка будет утомительным, поскольку они не предоставляют возможности, позволяющие пропустить какое-либо выражение, но (черный ящик) _testing_ должен быть независимым от кода. – PJTraill

+0

@PJTraill LOL для подсчета числа IFs (я считаю, что '?:', 'If-else' и булевы операторы являются ветвями). Что касается пункта 2, он говорит о модуле, и если вы прочтете книгу (по ссылке Роберта К. Мартина), станет ясно, что ее можно применять и к функциям. –

+0

@PJTraill Что касается тестирования, я изменил ответ. I unit unit testing. –

0

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

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

  1. Это был аргумент в функцию третьей стороной C.
  2. Я не был хорошо разбираюсь в современном C++ в то время.
  3. Я прокомментировал и отформатировал f *** из-за этого, потому что знал, что КТО-ТО, кроме меня, будет читать его ... или мне нужно было знать, что он делал годами позже.
  4. Это был 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" 
           ))))))))))) 
          ); 
    

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

+0

Я всегда создавал для этого 'map '. –

+0

@PeterSchneider См. Пункт пули 2. :) – Casey

Смежные вопросы